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))
> 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,
>
