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().
>
> > +
> > /*
> > * 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.
> > 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,
> >
>
