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).
> 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."
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().
> 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.
> 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?
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}
