Re: rpki-client: get Authority Information Access (AIA) from CA & EE certs
On Mon, Feb 15, 2021 at 04:58:50PM +, 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 - 1.25
> +++ usr.sbin/rpki-client/cert.c 15 Feb 2021 16:54:05 -
> @@ -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.c4 Feb 2021 08:58:19 - 1.4
> +++ usr.sbin/rpki-client/gbr.c15 Feb 2021 16:54:05 -
> @@ -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, >ski, >aki)) {
> + if (!x509_get_exts(*x509, fn, >ski, >aki, >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
> ---
Re: rpki-client: get Authority Information Access (AIA) from CA & EE certs
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 - 1.25
+++ usr.sbin/rpki-client/cert.c 15 Feb 2021 16:54:05 -
@@ -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 - 1.4
+++ usr.sbin/rpki-client/gbr.c 15 Feb 2021 16:54:05 -
@@ -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, >ski, >aki)) {
+ if (!x509_get_exts(*x509, fn, >ski, >aki, >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 - 1.25
+++ usr.sbin/rpki-client/mft.c 15 Feb 2021 16:54:05 -
@@ -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, >ski, >aki))
+ if (!x509_get_exts(*x509, fn, >ski, >aki, >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 - 1.13
+++ usr.sbin/rpki-client/roa.c 15 Feb 2021 16:54:05 -
@@ -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,
Re: rpki-client: get Authority Information Access (AIA) from CA & EE certs
On Sun, Feb 14, 2021 at 05:41:55PM +, 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 -
> 1.8
> +++ regress/usr.sbin/rpki-client/test-cert.c 14 Feb 2021 17:29:17 -
> @@ -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 -
> 1.1
> +++ regress/usr.sbin/rpki-client/test-gbr.c 14 Feb 2021 17:29:17 -
> @@ -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 -
> 1.10
> +++ regress/usr.sbin/rpki-client/test-mft.c 14 Feb 2021 17:29:17 -
> @@ -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 -
> 1.8
> +++ regress/usr.sbin/rpki-client/test-roa.c 14 Feb 2021 17:29:17 -
> @@ -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(>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 -
> 1.6
> +++ regress/usr.sbin/rpki-client/Makefile.inc 14 Feb 2021 17:29:17 -
> @@ -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 - 1.25
> +++ usr.sbin/rpki-client/cert.c 14 Feb 2021 17:29:18 -
> @@ -1079,6 +1079,11 @@ cert_parse_inner(X509 **xp, const char *
> case NID_crl_distribution_points:
> /* ignored here, handled later */
> break;
> + case
rpki-client: get Authority Information Access (AIA) from CA & EE certs
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?
Kind regards,
Job
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.c8 Feb 2021 09:28:58 -
1.8
+++ regress/usr.sbin/rpki-client/test-cert.c14 Feb 2021 17:29:17 -
@@ -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 -
1.1
+++ regress/usr.sbin/rpki-client/test-gbr.c 14 Feb 2021 17:29:17 -
@@ -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 -
1.10
+++ regress/usr.sbin/rpki-client/test-mft.c 14 Feb 2021 17:29:17 -
@@ -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 -
1.8
+++ regress/usr.sbin/rpki-client/test-roa.c 14 Feb 2021 17:29:17 -
@@ -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(>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 -
1.6
+++ regress/usr.sbin/rpki-client/Makefile.inc 14 Feb 2021 17:29:17 -
@@ -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}
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 - 1.25
+++ usr.sbin/rpki-client/cert.c 14 Feb 2021 17:29:18 -
@@ -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
