On Mon, Aug 22, 2022 at 12:14:53PM +0200, Theo Buehler wrote:
> rpki-client portable makes sure that libcrypto has RFC 3779 support.
> Therefore the X509_verify_cert() call in valid_x509() will already
> perform the checks that the RFC 3779 extensions are covered along the
> chain. While valid_cert()'s errors would be nicer than the validator's,
> they can't be reached anymore.
I wonderd if we could beef up the error handler of X509_verify_cert() to
return better errors. Like calling the valid_ip/valid_as functions to
figure out what is not covered.
More urgently a similar change is needed for .mft files where the common
error of no valid mft found is very unhelpful to diagnose issues.
> The check that a BGPsec cert's AS numbers must not be inherited can be
> done in cert_parse_pre() like most of the other checks for BGPsec certs.
Sounds good to me.
> With the removal of valid_cert(), valid_as() and valid_ip() are unused
> and can also go.
In general I'm ok with the diff but see above about the wish for better
error reporting.
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.85
> diff -u -p -r1.85 cert.c
> --- cert.c 19 Aug 2022 12:45:53 -0000 1.85
> +++ cert.c 22 Aug 2022 09:52:07 -0000
> @@ -736,6 +736,13 @@ cert_parse_pre(const char *fn, const uns
> p.fn);
> goto out;
> }
> + for (i = 0; i < p.res->asz; i++) {
> + if (p.res->as[i].type == CERT_AS_INHERIT) {
> + warnx("%s: inherited AS numbers in BGPsec cert",
> + p.fn);
> + goto out;
> + }
> + }
> if (sia_present) {
> warnx("%s: unexpected SIA extension in BGPsec cert",
> p.fn);
> Index: filemode.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 filemode.c
> --- filemode.c 19 Aug 2022 12:45:53 -0000 1.8
> +++ filemode.c 19 Aug 2022 19:53:17 -0000
> @@ -168,8 +168,7 @@ parse_load_certchain(char *uri)
> uri = filestack[i];
>
> crl = crl_get(&crlt, a);
> - if (!valid_x509(uri, ctx, cert->x509, a, crl, 0) ||
> - !valid_cert(uri, a, cert))
> + if (!valid_x509(uri, ctx, cert->x509, a, crl, 0))
> goto fail;
> cert->talid = a->cert->talid;
> a = auth_insert(&auths, cert, a);
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.74
> diff -u -p -r1.74 parser.c
> --- parser.c 19 Aug 2022 12:45:53 -0000 1.74
> +++ parser.c 19 Aug 2022 19:52:53 -0000
> @@ -404,8 +404,7 @@ proc_parser_cert(char *file, const unsig
> a = valid_ski_aki(file, &auths, cert->ski, cert->aki);
> crl = crl_get(&crlt, a);
>
> - if (!valid_x509(file, ctx, cert->x509, a, crl, 0) ||
> - !valid_cert(file, a, cert)) {
> + if (!valid_x509(file, ctx, cert->x509, a, crl, 0)) {
> cert_free(cert);
> return NULL;
> }
> Index: validate.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/validate.c,v
> retrieving revision 1.41
> diff -u -p -r1.41 validate.c
> --- validate.c 19 Aug 2022 12:45:53 -0000 1.41
> +++ validate.c 19 Aug 2022 19:52:22 -0000
> @@ -33,56 +33,6 @@
> extern ASN1_OBJECT *certpol_oid;
>
> /*
> - * Walk up the chain of certificates trying to match our AS number to
> - * one of the allocations in that chain.
> - * Returns 1 if covered or 0 if not.
> - */
> -static int
> -valid_as(struct auth *a, uint32_t min, uint32_t max)
> -{
> - int c;
> -
> - if (a == NULL)
> - return 0;
> -
> - /* Does this certificate cover our AS number? */
> - c = as_check_covered(min, max, a->cert->as, a->cert->asz);
> - if (c > 0)
> - return 1;
> - else if (c < 0)
> - return 0;
> -
> - /* If it inherits, walk up the chain. */
> - return valid_as(a->parent, min, max);
> -}
> -
> -/*
> - * Walk up the chain of certificates (really just the last one, but in
> - * the case of inheritance, the ones before) making sure that our IP
> - * prefix is covered in the first non-inheriting specification.
> - * Returns 1 if covered or 0 if not.
> - */
> -static int
> -valid_ip(struct auth *a, enum afi afi,
> - const unsigned char *min, const unsigned char *max)
> -{
> - int c;
> -
> - if (a == NULL)
> - return 0;
> -
> - /* Does this certificate cover our IP prefix? */
> - c = ip_addr_check_covered(afi, min, max, a->cert->ips, a->cert->ipsz);
> - if (c > 0)
> - return 1;
> - else if (c < 0)
> - return 0;
> -
> - /* If it inherits, walk up the chain. */
> - return valid_ip(a->parent, afi, min, max);
> -}
> -
> -/*
> * Make sure that the SKI doesn't already exist and return the parent by
> * its AKI.
> * Returns the parent auth or NULL on failure.
> @@ -131,65 +81,6 @@ valid_ta(const char *fn, struct auth_tre
> /* SKI must not be a dupe. */
> if (auth_find(auths, cert->ski) != NULL) {
> warnx("%s: RFC 6487: duplicate SKI", fn);
> - return 0;
> - }
> -
> - return 1;
> -}
> -
> -/*
> - * Validate a non-TA certificate: make sure its IP and AS resources are
> - * fully covered by those in the authority key (which must exist).
> - * Returns 1 if valid, 0 otherwise.
> - */
> -int
> -valid_cert(const char *fn, struct auth *a, const struct cert *cert)
> -{
> - size_t i;
> - uint32_t min, max;
> - char buf1[64], buf2[64];
> -
> - for (i = 0; i < cert->asz; i++) {
> - if (cert->as[i].type == CERT_AS_INHERIT) {
> - if (cert->purpose == CERT_PURPOSE_BGPSEC_ROUTER)
> - return 0; /* BGPsec doesn't permit inheriting */
> - continue;
> - }
> - min = cert->as[i].type == CERT_AS_ID ?
> - cert->as[i].id : cert->as[i].range.min;
> - max = cert->as[i].type == CERT_AS_ID ?
> - cert->as[i].id : cert->as[i].range.max;
> - if (valid_as(a, min, max))
> - continue;
> - warnx("%s: RFC 6487: uncovered AS: "
> - "%u--%u", fn, min, max);
> - return 0;
> - }
> -
> - for (i = 0; i < cert->ipsz; i++) {
> - if (valid_ip(a, cert->ips[i].afi, cert->ips[i].min,
> - cert->ips[i].max))
> - continue;
> - switch (cert->ips[i].type) {
> - case CERT_IP_RANGE:
> - ip_addr_print(&cert->ips[i].range.min,
> - cert->ips[i].afi, buf1, sizeof(buf1));
> - ip_addr_print(&cert->ips[i].range.max,
> - cert->ips[i].afi, buf2, sizeof(buf2));
> - warnx("%s: RFC 6487: uncovered IP: "
> - "%s--%s", fn, buf1, buf2);
> - break;
> - case CERT_IP_ADDR:
> - ip_addr_print(&cert->ips[i].ip,
> - cert->ips[i].afi, buf1, sizeof(buf1));
> - warnx("%s: RFC 6487: uncovered IP: "
> - "%s", fn, buf1);
> - break;
> - case CERT_IP_INHERIT:
> - warnx("%s: RFC 6487: uncovered IP: "
> - "(inherit)", fn);
> - break;
> - }
> return 0;
> }
>
>
--
:wq Claudio
