On Sat, Jan 22, 2022 at 02:21:23PM +0100, Theo Buehler wrote:
> On Sat, Jan 22, 2022 at 12:42:30PM +0100, Theo Buehler wrote:
> > On Sat, Jan 22, 2022 at 11:47:17AM +0100, Claudio Jeker wrote:
> > > The valid_cert() and valid_roa() functions both redo the valid_aki_ski()
> > > call that the callee already did. Adjust the functions and skip this
> > > redundant call. Also move the place where we set the talid for roa to a
> > > better place.
> > >
> > > With RFC3779 support in LibreSSL these functions no longer trigger since
> > > the range checks for IP and ASnums happen in X509_verify_cert().
> > > The fucntions are still needed for libcrypto libs compiled without RFC3779
> > > support. Also they print a less generic error message and change the stats
> > > from failed parse to invalid (in case of roas).
> > >
> > > Lets start with this diff and then see how error reporting could be
> > > improved.
> >
> > I'm fine with this in principle, but I have one question.
> >
> > > --
> > > :wq Claudio
> > >
> > > Index: extern.h
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> > > retrieving revision 1.112
> > > diff -u -p -r1.112 extern.h
> > > --- extern.h 22 Jan 2022 09:18:48 -0000 1.112
> > > +++ extern.h 22 Jan 2022 10:33:45 -0000
> > > @@ -446,9 +446,8 @@ struct auth *valid_ski_aki(const char *,
> > > const char *, const char *);
> > > int valid_ta(const char *, struct auth_tree *,
> > > const struct cert *);
> > > -int valid_cert(const char *, struct auth_tree *,
> > > - const struct cert *);
> > > -int valid_roa(const char *, struct auth_tree *, struct roa
> > > *);
> > > +int valid_cert(const char *, struct auth *, const struct
> > > cert *);
> > > +int valid_roa(const char *, struct auth *, struct roa *);
> > > int valid_filehash(int, const char *, size_t);
> > > int valid_uri(const char *, size_t, const char *);
> > > int valid_origin(const char *, const char *);
> > > Index: parser.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> > > retrieving revision 1.50
> > > diff -u -p -r1.50 parser.c
> > > --- parser.c 22 Jan 2022 09:18:48 -0000 1.50
> > > +++ parser.c 22 Jan 2022 10:36:43 -0000
> > > @@ -268,6 +268,8 @@ proc_parser_roa(char *file, const unsign
> > > }
> > > X509_free(x509);
> > >
> > > + roa->talid = a->cert->talid;
> >
> > Previously there was a NULL check for a before doing this. As far as I
> > can tell, we can now get here with a == NULL. This also makes me wonder
> > if we should keep the NULL check for a in valid_roa() and valid_cert()
> > and do this assignment only after a successful call to valid_roa().
We can't really end up here with a == NULL because valid_x509() above will
fail for a == NULL (X509_verify_cert() with a empty trusted stack will
fail. Now maybe that is a bit too indirect but we depend on the same fact
in proc_parser_cert_validate().
> > > +
> > > /*
> > > * Check CRL to figure out the soonest transitive expiry moment
> > > */
> > > @@ -288,7 +290,7 @@ proc_parser_roa(char *file, const unsign
> > > * the code around roa_read() to check the "valid" field itself.
> > > */
> > >
> > > - if (valid_roa(file, &auths, roa))
> > > + if (valid_roa(file, a, roa))
>
> There is a second issue: a is not the return value of valid_ski_aki()
> here, but rather the auth struct of the trust anchor due to the for loop
> before this call.
This is indeed a good point. I moved the code up, so we do the valid_roa()
check before figuring out the expire time.
Adjusted diff below
--
:wq Claudio
Index: extern.h
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.112
diff -u -p -r1.112 extern.h
--- extern.h 22 Jan 2022 09:18:48 -0000 1.112
+++ extern.h 22 Jan 2022 10:33:45 -0000
@@ -446,9 +446,8 @@ struct auth *valid_ski_aki(const char *,
const char *, const char *);
int valid_ta(const char *, struct auth_tree *,
const struct cert *);
-int valid_cert(const char *, struct auth_tree *,
- const struct cert *);
-int valid_roa(const char *, struct auth_tree *, struct roa *);
+int valid_cert(const char *, struct auth *, const struct cert *);
+int valid_roa(const char *, struct auth *, struct roa *);
int valid_filehash(int, const char *, size_t);
int valid_uri(const char *, size_t, const char *);
int valid_origin(const char *, const char *);
Index: parser.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
retrieving revision 1.50
diff -u -p -r1.50 parser.c
--- parser.c 22 Jan 2022 09:18:48 -0000 1.50
+++ parser.c 22 Jan 2022 18:00:01 -0000
@@ -268,6 +268,16 @@ proc_parser_roa(char *file, const unsign
}
X509_free(x509);
+ roa->talid = a->cert->talid;
+
+ /*
+ * If the ROA isn't valid, we accept it anyway and depend upon
+ * the code around roa_read() to check the "valid" field itself.
+ */
+
+ if (valid_roa(file, a, roa))
+ roa->valid = 1;
+
/*
* Check CRL to figure out the soonest transitive expiry moment
*/
@@ -283,14 +293,6 @@ proc_parser_roa(char *file, const unsign
roa->expires = a->cert->expires;
}
- /*
- * If the ROA isn't valid, we accept it anyway and depend upon
- * the code around roa_read() to check the "valid" field itself.
- */
-
- if (valid_roa(file, &auths, roa))
- roa->valid = 1;
-
return roa;
}
@@ -401,8 +403,8 @@ proc_parser_cert_validate(char *file, st
cert->talid = a->cert->talid;
- /* Validate the cert to get the parent */
- if (!valid_cert(file, &auths, cert)) {
+ /* Validate the cert */
+ if (!valid_cert(file, a, cert)) {
cert_free(cert);
return NULL;
}
Index: validate.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/validate.c,v
retrieving revision 1.26
diff -u -p -r1.26 validate.c
--- validate.c 22 Jan 2022 09:18:48 -0000 1.26
+++ validate.c 22 Jan 2022 10:34:31 -0000
@@ -143,17 +143,12 @@ valid_ta(const char *fn, struct auth_tre
* Returns 1 if valid, 0 otherwise.
*/
int
-valid_cert(const char *fn, struct auth_tree *auths, const struct cert *cert)
+valid_cert(const char *fn, struct auth *a, const struct cert *cert)
{
- struct auth *a;
size_t i;
uint32_t min, max;
char buf1[64], buf2[64];
- a = valid_ski_aki(fn, auths, cert->ski, cert->aki);
- if (a == NULL)
- return 0;
-
for (i = 0; i < cert->asz; i++) {
if (cert->as[i].type == CERT_AS_INHERIT) {
if (cert->purpose == CERT_PURPOSE_BGPSEC_ROUTER)
@@ -207,17 +202,11 @@ valid_cert(const char *fn, struct auth_t
* Returns 1 if valid, 0 otherwise.
*/
int
-valid_roa(const char *fn, struct auth_tree *auths, struct roa *roa)
+valid_roa(const char *fn, struct auth *a, struct roa *roa)
{
- struct auth *a;
size_t i;
char buf[64];
- a = valid_ski_aki(fn, auths, roa->ski, roa->aki);
- if (a == NULL)
- return 0;
-
- roa->talid = a->cert->talid;
for (i = 0; i < roa->ipsz; i++) {
if (valid_ip(a, roa->ips[i].afi, roa->ips[i].min,
