On Sun, Feb 14, 2021 at 05:41:55PM +0000, Job Snijders wrote:
> Make the AIA more easily available for debugging purposes & future
> changesets
>
> In the context of the RPKI, the AIA extension identifies the publication
> point of the certificate of the issuer of the certificate in which the
> extension appears. A single reference to the publication point of the
> immediate superior certificate MUST be present, except for a
> "self-signed" certificate.
>
> OK?
I'm not against this going in but I think there is some improvements that
should be made. Some of it inline.
The extra data stored increases the memory usage by 16MB or 3.5% which is ok.
Also the data is already kind of known via the AKI / SKI stack but for
individual files it is much easier to to walk backup up the tree via AIA.
> 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 14 Feb 2021 17:29:17 -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 14 Feb 2021 17:29:17 -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 14 Feb 2021 17:29:17 -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 14 Feb 2021 17:29:17 -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 14 Feb 2021 17:29:17 -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}
Regress tests are OK.
> 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 14 Feb 2021 17:29:18 -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);
See x509.c comments
> 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 14 Feb 2021 17:29:18 -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 14 Feb 2021 17:29:18 -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 14 Feb 2021 17:29:18 -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 14 Feb 2021 17:29:18 -0000
> @@ -169,12 +169,57 @@ 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)
> +{
> + 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;
> + }
> +
> + ACCESS_DESCRIPTION *ad = sk_ACCESS_DESCRIPTION_value(info, 0);
Please do not define variables in the middle of functions. Add
ACCESS_DESCRIPTION *ad;
at the top.
> + 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;
> +}
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.
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.
Anyway I would prefer if x509_get_ski_ext(), x509_get_aki_ext() and
x509_get_aia() all work the same way.
> +
> +/*
> + * 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;
You forgot to add *aia to at the start where *ski = *aki = NULL;
> @@ -199,21 +244,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);
Since you did not initially NULL aia this free call may be bad.
> + *aia = x509_get_aia(x, fn);
> + break;
> }
> }
As mentioned above this function feels not quite right to me.
>
> + 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 14 Feb 2021 17:29:18 -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 *);
>
>
With the style and initialisation issue fixed I think this can go in but
as mentioned more x509 cleanup needs to follow.
--
:wq Claudio
