On Mon, Feb 15, 2021 at 04:58:50PM +0000, Job Snijders wrote:
> Hi,
>
> Thank you for the review
>
> On Mon, Feb 15, 2021 at 01:42:57PM +0100, Claudio Jeker wrote:
> > Please do not define variables in the middle of functions.
>
> now fixed
>
> > > + AUTHORITY_INFO_ACCESS_free(info);
> > > +
> > > + return aia;
> > > +}
> >
> > I really don't like the way x509_get_aia() is different to
> > x509_get_ski_ext and x509_get_aki_ext. The way the x509 extensions are
> > handled in rpki-client is strange. In x509_get_exts() the code walks over
> > all extensions and looks for the NID we're interested in.
>
> Yes, they are different in style. I think x509_get_{ski,aki}_ext() might
> be 'the odd ones out': http://man.openbsd.org/X509_EXTENSION_get_data states:
>
> "Most applications will want to parse or encode and add an
> extension: they should use the extension encode and decode functions
> instead such as X509_add1_ext_i2d(3) and X509_get_ext_d2i(3)."
>
> Which is why I wrote x509_get_aia() to use X509_get_ext_d2i(3).
I did not imply that your code is wrong. I tend to agree that
x509_get_{ski,aki}_ext() need the rewrite.
> > x509_get_aia() makes this approach even more strange by actually not using
> > the ext but instead do a lookup from the x509 object directly. So why call
> > it in the for loop at all. Just do it directly.
> >
> > Also the way the code is written in x509_get_exts() implies that x509
> > allows the same extension to be added more then once. Is that really true?
> > Wouldn't that be an error regarding RFC6487.
>
> https://tools.ietf.org/html/rfc5280#section-4.2.2.1 reads:
> "An authorityInfoAccess extension may include multiple instances of
> the id-ad-caIssuers accessMethod."
But does it mention that only one authorityInfoAccess extension may be
included?
> Then https://tools.ietf.org/html/rfc6487#section-4.8.7 narrows it down:
> "In this profile, a single reference to the publication point of the
> immediate superior certificate MUST be present"
>
> which is why i put "if (sk_ACCESS_DESCRIPTION_num(info) != 1) {" in
> x509_get_aia().
This part I have seen but my question was more aiming at the for() loop in
x509_get_exts() where we loop over the array but actually x509_get_aia()
will always fetch the same data for each call
> > Anyway I would prefer if x509_get_ski_ext(), x509_get_aki_ext() and
> > x509_get_aia() all work the same way.
>
> Perhaps x509_get_ski_ext() and x509_get_aki_ext() can be modified to use
> X509_get_ext_d2i(3) too.
They should and x509_get_exts() and the code in cert.c should be adjusted
as well. Let's see if I find the x509 hazmat suit.
> > You forgot to add *aia to at the start where *ski = *aki = NULL;
>
> fixed.
>
> > With the style and initialisation issue fixed I think this can go in
> > but as mentioned more x509 cleanup needs to follow.
>
> OK?
There are memory leaks (none of the aia strings are freed) and also the io
read and write bits are missing. If you sure the AIA is only used in the
parser process then you can skip the io bits.
> Index: usr.sbin/rpki-client/cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 cert.c
> --- usr.sbin/rpki-client/cert.c 8 Feb 2021 09:22:53 -0000 1.25
> +++ usr.sbin/rpki-client/cert.c 15 Feb 2021 16:54:05 -0000
> @@ -1079,6 +1079,11 @@ cert_parse_inner(X509 **xp, const char *
> case NID_crl_distribution_points:
> /* ignored here, handled later */
> break;
> + case NID_info_access:
> + free(p.res->aia);
> + p.res->aia = x509_get_aia(x, p.fn);
> + c = (p.res->aia != NULL);
> + break;
> case NID_authority_key_identifier:
> free(p.res->aki);
> p.res->aki = x509_get_aki_ext(ext, p.fn);
> Index: usr.sbin/rpki-client/gbr.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/gbr.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 gbr.c
> --- usr.sbin/rpki-client/gbr.c 4 Feb 2021 08:58:19 -0000 1.4
> +++ usr.sbin/rpki-client/gbr.c 15 Feb 2021 16:54:05 -0000
> @@ -62,7 +62,7 @@ gbr_parse(X509 **x509, const char *fn)
> err(1, NULL);
> if ((p.res->vcard = strndup(cms, cmsz)) == NULL)
> err(1, NULL);
> - if (!x509_get_ski_aki(*x509, fn, &p.res->ski, &p.res->aki)) {
> + if (!x509_get_exts(*x509, fn, &p.res->ski, &p.res->aki, &p.res->aia)) {
> gbr_free(p.res);
> X509_free(*x509);
> *x509 = NULL;
> Index: usr.sbin/rpki-client/mft.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 mft.c
> --- usr.sbin/rpki-client/mft.c 4 Feb 2021 08:58:19 -0000 1.25
> +++ usr.sbin/rpki-client/mft.c 15 Feb 2021 16:54:05 -0000
> @@ -395,7 +395,7 @@ mft_parse(X509 **x509, const char *fn)
> err(1, NULL);
> if ((p.res->file = strdup(fn)) == NULL)
> err(1, NULL);
> - if (!x509_get_ski_aki(*x509, fn, &p.res->ski, &p.res->aki))
> + if (!x509_get_exts(*x509, fn, &p.res->ski, &p.res->aki, &p.res->aia))
> goto out;
>
> /*
> Index: usr.sbin/rpki-client/roa.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 roa.c
> --- usr.sbin/rpki-client/roa.c 4 Feb 2021 08:58:19 -0000 1.13
> +++ usr.sbin/rpki-client/roa.c 15 Feb 2021 16:54:05 -0000
> @@ -349,7 +349,7 @@ roa_parse(X509 **x509, const char *fn)
>
> if ((p.res = calloc(1, sizeof(struct roa))) == NULL)
> err(1, NULL);
> - if (!x509_get_ski_aki(*x509, fn, &p.res->ski, &p.res->aki))
> + if (!x509_get_exts(*x509, fn, &p.res->ski, &p.res->aki, &p.res->aia))
> goto out;
> if (!roa_parse_econtent(cms, cmsz, &p))
> goto out;
> Index: usr.sbin/rpki-client/x509.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 x509.c
> --- usr.sbin/rpki-client/x509.c 12 Sep 2020 15:46:48 -0000 1.14
> +++ usr.sbin/rpki-client/x509.c 15 Feb 2021 16:54:05 -0000
> @@ -169,18 +169,64 @@ out:
> }
>
> /*
> - * Wraps around x509_get_ski_ext and x509_get_aki_ext.
> + * Parse the Authority Information Access (AIA) extension
> + * See RFC 6487, section 4.8.7 for details.
> + * Returns NULL on failure, on success returns the AIA URI
> + * (which has to be freed after use).
> + */
> +char *
> +x509_get_aia(X509 *x, const char *fn)
> +{
> + ACCESS_DESCRIPTION *ad;
> + AUTHORITY_INFO_ACCESS *info;
> + char *aia = NULL;
> +
> + info = X509_get_ext_d2i(x, NID_info_access, NULL, NULL);
> + if (!info) {
> + warnx("%s: RFC 6487 section 4.8.7: AIA: extension missing", fn);
> + return NULL;
> + }
> + if (sk_ACCESS_DESCRIPTION_num(info) != 1) {
> + warnx("%s: RFC 6487 section 4.8.7: AIA: "
> + "want 1 element, have %d", fn,
> + sk_ACCESS_DESCRIPTION_num(info));
> + return NULL;
> + }
> +
> + ad = sk_ACCESS_DESCRIPTION_value(info, 0);
> + if (OBJ_obj2nid(ad->method) != NID_ad_ca_issuers) {
> + warnx("%s: RFC 6487 section 4.8.7: AIA: "
> + "expected caIssuers, have %d", fn, OBJ_obj2nid(ad->method));
> + return NULL;
> + }
> + if (ad->location->type != GEN_URI) {
> + warnx("%s: RFC 6487 section 4.8.7: AIA: "
> + "want GEN_URI type, have %d", fn, ad->location->type);
> + return NULL;
> + }
> +
> + aia = strndup(
> + ASN1_STRING_get0_data(ad->location->d.uniformResourceIdentifier),
> + ASN1_STRING_length(ad->location->d.uniformResourceIdentifier));
> +
> + AUTHORITY_INFO_ACCESS_free(info);
> +
> + return aia;
> +}
> +
> +/*
> + * Wraps around x509_get_ski_ext, x509_get_aki_ext, and x509_get_aia.
> * Returns zero on failure (out pointers are NULL) or non-zero on
> * success (out pointers must be freed).
> */
> int
> -x509_get_ski_aki(X509 *x, const char *fn, char **ski, char **aki)
> +x509_get_exts(X509 *x, const char *fn, char **ski, char **aki, char **aia)
> {
> X509_EXTENSION *ext = NULL;
> const ASN1_OBJECT *obj;
> int extsz, i;
>
> - *ski = *aki = NULL;
> + *ski = *aki = *aia = NULL;
>
> if ((extsz = X509_get_ext_count(x)) < 0)
> cryptoerrx("X509_get_ext_count");
> @@ -199,21 +245,38 @@ x509_get_ski_aki(X509 *x, const char *fn
> free(*aki);
> *aki = x509_get_aki_ext(ext, fn);
> break;
> + case NID_info_access:
> + free(*aia);
> + *aia = x509_get_aia(x, fn);
> + break;
> }
> }
>
> + if (*aia == NULL) {
> + cryptowarnx("%s: RFC 6487 section 4.8.7: AIA: "
> + "missing AIA X509 extension", fn);
> + free(*ski);
> + free(*aki);
> + *ski = NULL;
> + *aki = NULL;
> + return 0;
> + }
> if (*aki == NULL) {
> cryptowarnx("%s: RFC 6487 section 4.8.3: AKI: "
> "missing AKI X509 extension", fn);
> free(*ski);
> + free(*aia);
> *ski = NULL;
> + *aia = NULL;
> return 0;
> }
> if (*ski == NULL) {
> - cryptowarnx("%s: RFC 6487 section 4.8.2: AKI: "
> + cryptowarnx("%s: RFC 6487 section 4.8.2: SKI: "
> "missing SKI X509 extension", fn);
> free(*aki);
> + free(*aia);
> *aki = NULL;
> + *aia = NULL;
> return 0;
> }
>
> Index: usr.sbin/rpki-client/extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.42
> diff -u -p -r1.42 extern.h
> --- usr.sbin/rpki-client/extern.h 8 Feb 2021 09:22:53 -0000 1.42
> +++ usr.sbin/rpki-client/extern.h 15 Feb 2021 16:54:05 -0000
> @@ -116,6 +116,7 @@ struct cert {
> char *mft; /* manifest (rsync:// uri) */
> char *notify; /* RRDP notify (https:// uri) */
> char *crl; /* CRL location (rsync:// or NULL) */
> + char *aia; /* AIA (or NULL, for trust anchor) */
> char *aki; /* AKI (or NULL, for trust anchor) */
> char *ski; /* SKI */
> int valid; /* validated resources */
> @@ -157,6 +158,7 @@ struct mft {
> int stale; /* if a stale manifest */
> char *ski; /* SKI */
> char *aki; /* AKI */
> + char *aia; /* AIA */
> };
>
> /*
> @@ -183,6 +185,7 @@ struct roa {
> int valid; /* validated resources */
> char *ski; /* SKI */
> char *aki; /* AKI */
> + char *aia; /* AIA */
> char *tal; /* basename of TAL for this cert */
> };
>
> @@ -193,6 +196,7 @@ struct gbr {
> char *vcard;
> char *ski; /* SKI */
> char *aki; /* AKI */
> + char *aia; /* AIA */
> };
>
> /*
> @@ -248,7 +252,7 @@ struct auth *auth_find(struct auth_tree
>
> /*
> * Resource types specified by the RPKI profiles.
> - * There are others (e.g., gbr) that we don't consider.
> + * There might be others we don't consider.
> */
> enum rtype {
> RTYPE_EOF = 0,
> @@ -418,9 +422,10 @@ void io_str_read(int, char **);
>
> /* X509 helpers. */
>
> +char *x509_get_aia(X509 *, const char *);
> char *x509_get_aki_ext(X509_EXTENSION *, const char *);
> char *x509_get_ski_ext(X509_EXTENSION *, const char *);
> -int x509_get_ski_aki(X509 *, const char *, char **, char **);
> +int x509_get_exts(X509 *, const char *, char **, char **, char **);
> char *x509_get_crl(X509 *, const char *);
> char *x509_crl_get_aki(X509_CRL *);
>
> Index: regress/usr.sbin/rpki-client/test-cert.c
> ===================================================================
> RCS file: /cvs/src/regress/usr.sbin/rpki-client/test-cert.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 test-cert.c
> --- regress/usr.sbin/rpki-client/test-cert.c 8 Feb 2021 09:28:58 -0000
> 1.8
> +++ regress/usr.sbin/rpki-client/test-cert.c 15 Feb 2021 16:54:05 -0000
> @@ -52,6 +52,8 @@ cert_print(const struct cert *p)
> printf("Subject key identifier: %s\n", p->ski);
> if (p->aki != NULL)
> printf("Authority key identifier: %s\n", p->aki);
> + if (p->aia != NULL)
> + printf("Authority info access: %s\n", p->aia);
>
> for (i = 0; i < p->asz; i++)
> switch (p->as[i].type) {
> Index: regress/usr.sbin/rpki-client/test-gbr.c
> ===================================================================
> RCS file: /cvs/src/regress/usr.sbin/rpki-client/test-gbr.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 test-gbr.c
> --- regress/usr.sbin/rpki-client/test-gbr.c 9 Dec 2020 11:30:44 -0000
> 1.1
> +++ regress/usr.sbin/rpki-client/test-gbr.c 15 Feb 2021 16:54:05 -0000
> @@ -42,6 +42,7 @@ gbr_print(const struct gbr *p)
>
> printf("Subject key identifier: %s\n", p->ski);
> printf("Authority key identifier: %s\n", p->aki);
> + printf("Authority info access: %s\n", p->aia);
> printf("vcard:\n%s", p->vcard);
> }
>
> Index: regress/usr.sbin/rpki-client/test-mft.c
> ===================================================================
> RCS file: /cvs/src/regress/usr.sbin/rpki-client/test-mft.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 test-mft.c
> --- regress/usr.sbin/rpki-client/test-mft.c 9 Nov 2020 16:13:02 -0000
> 1.10
> +++ regress/usr.sbin/rpki-client/test-mft.c 15 Feb 2021 16:54:05 -0000
> @@ -54,6 +54,7 @@ mft_print(const struct mft *p)
>
> printf("Subject key identifier: %s\n", p->ski);
> printf("Authority key identifier: %s\n", p->aki);
> + printf("Authority info access: %s\n", p->aia);
> for (i = 0; i < p->filesz; i++) {
> b64_ntop(p->files[i].hash, sizeof(p->files[i].hash),
> hash, sizeof(hash));
> Index: regress/usr.sbin/rpki-client/test-roa.c
> ===================================================================
> RCS file: /cvs/src/regress/usr.sbin/rpki-client/test-roa.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 test-roa.c
> --- regress/usr.sbin/rpki-client/test-roa.c 29 Jan 2021 10:15:42 -0000
> 1.8
> +++ regress/usr.sbin/rpki-client/test-roa.c 15 Feb 2021 16:54:05 -0000
> @@ -42,6 +42,7 @@ roa_print(const struct roa *p)
>
> printf("Subject key identifier: %s\n", p->ski);
> printf("Authority key identifier: %s\n", p->aki);
> + printf("Authority info access: %s\n", p->aia);
> printf("asID: %" PRIu32 "\n", p->asid);
> for (i = 0; i < p->ipsz; i++) {
> ip_addr_print(&p->ips[i].addr,
> Index: regress/usr.sbin/rpki-client/Makefile.inc
> ===================================================================
> RCS file: /cvs/src/regress/usr.sbin/rpki-client/Makefile.inc,v
> retrieving revision 1.6
> diff -u -p -r1.6 Makefile.inc
> --- regress/usr.sbin/rpki-client/Makefile.inc 3 Feb 2021 10:45:12 -0000
> 1.6
> +++ regress/usr.sbin/rpki-client/Makefile.inc 15 Feb 2021 16:54:05 -0000
> @@ -4,9 +4,9 @@
>
> PROGS += test-ip
> PROGS += test-cert
> +PROGS += test-gbr
> PROGS += test-mft
> PROGS += test-roa
> -PROGS += test-gbr
> PROGS += test-tal
>
> .for p in ${PROGS}
>
--
:wq Claudio
