Download raw body.
rpki-client: improve SIA handling
On Mon, Jun 03, 2024 at 12:25:25PM +0200, Theo Buehler wrote:
> On Mon, Jun 03, 2024 at 11:09:57AM +0200, Claudio Jeker wrote:
> > On Sun, Jun 02, 2024 at 04:56:51PM +0200, Theo Buehler wrote:
> > > The way I read 4.8.8.1 of RFC 6487, the current handling of the SIA
> > > extension is not entirely correct for CA certs. We reject non-rsync URI
> > > for the caRepository and rpkiManifest access methods while the spec only
> > > requires that an rsync URI be present for both of them, not that all
> > > URIs use the rsync scheme. So accept all valid URIs and retain the first
> > > rsync one we encounter. If there's no rsync URI we will reject the cert.
> > > In contrast, RFC 8182 3.2 only allows https URI for rpkiNotify.
> > >
> > > Also make it clearer that sbgp_sia() is for CA certs and x509_get_sia()
> > > for EE certs.
> > >
> > > Index: cert.c
> > > ===================================================================
> > > RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> > > diff -u -p -r1.132 cert.c
> > > --- cert.c 31 May 2024 02:45:15 -0000 1.132
> > > +++ cert.c 2 Jun 2024 14:34:56 -0000
> > > @@ -495,7 +495,8 @@ sbgp_ipaddrblk(const char *fn, struct ce
> > > }
> > >
> > > /*
> > > - * Parse "Subject Information Access" extension, RFC 6487 4.8.8.
> > > + * Parse "Subject Information Access" extension for a CA cert,
> > > + * RFC 6487, section 4.8.8.1 and RFC 8182, section 3.2.
> > > * Returns zero on failure, non-zero on success.
> > > */
> > > static int
> > > @@ -525,13 +526,23 @@ sbgp_sia(const char *fn, struct cert *ce
> > > oid = ad->method;
> > >
> > > if (OBJ_cmp(oid, carepo_oid) == 0) {
> > > - if (!x509_location(fn, "SIA: caRepository",
> > > - RSYNC_PROTO, ad->location, &cert->repo))
> > > + if (!x509_location(fn, "SIA: caRepository", NULL,
> > > + ad->location, &cert->repo))
> > > goto out;
> > > + if (strncasecmp(cert->repo, RSYNC_PROTO,
> > > + RSYNC_PROTO_LEN) == 0)
> > > + continue;
> > > + free(cert->repo);
> > > + cert->repo = NULL;
> >
> > This seems very fragile on how multiple locations are handled.
>
> Not sure how it is fragile except that x509_location() isn't the
> greatest of APIs I ever came up with, but maybe the below is more
> robust.
Yes, it is.
> > Especially the warnx() in x509_location() depends on the order of uri.
>
> This warning is mostly useless noise here in my opinion. But yes, we
> could do better... The below will warn about every ignored URI, but only
> in verbose mode as I don't think this is of any interest to regular
> users.
Probably yes. We should just handle multiple locations properly (as your
diff is a step towards that).
> I think x509_location() should lose its proto argument since it's almost
> always NULL now. I'll clean that up as a follow-up.
Go for it.
Diff is OK claudio@
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> diff -u -p -r1.132 cert.c
> --- cert.c 31 May 2024 02:45:15 -0000 1.132
> +++ cert.c 3 Jun 2024 10:15:46 -0000
> @@ -495,7 +495,8 @@ sbgp_ipaddrblk(const char *fn, struct ce
> }
>
> /*
> - * Parse "Subject Information Access" extension, RFC 6487 4.8.8.
> + * Parse "Subject Information Access" extension for a CA cert,
> + * RFC 6487, section 4.8.8.1 and RFC 8182, section 3.2.
> * Returns zero on failure, non-zero on success.
> */
> static int
> @@ -505,8 +506,11 @@ sbgp_sia(const char *fn, struct cert *ce
> ACCESS_DESCRIPTION *ad;
> ASN1_OBJECT *oid;
> const char *mftfilename;
> + char *carepo = NULL, *rpkimft = NULL;
> int i, rc = 0;
>
> + assert(cert->repo == NULL && cert->mft == NULL && cert->notify == NULL);
> +
> if (X509_EXTENSION_get_critical(ext)) {
> warnx("%s: RFC 6487 section 4.8.8: SIA: "
> "extension not non-critical", fn);
> @@ -525,13 +529,35 @@ sbgp_sia(const char *fn, struct cert *ce
> oid = ad->method;
>
> if (OBJ_cmp(oid, carepo_oid) == 0) {
> - if (!x509_location(fn, "SIA: caRepository",
> - RSYNC_PROTO, ad->location, &cert->repo))
> + if (!x509_location(fn, "SIA: caRepository", NULL,
> + ad->location, &carepo))
> goto out;
> + if (cert->repo == NULL && strncasecmp(carepo,
> + RSYNC_PROTO, RSYNC_PROTO_LEN) == 0) {
> + cert->repo = carepo;
> + carepo = NULL;
> + continue;
> + }
> + if (verbose)
> + warnx("%s: RFC 6487 section 4.8.8: SIA: "
> + "ignoring location %s", fn, carepo);
> + free(carepo);
> + carepo = NULL;
> } else if (OBJ_cmp(oid, manifest_oid) == 0) {
> - if (!x509_location(fn, "SIA: rpkiManifest",
> - RSYNC_PROTO, ad->location, &cert->mft))
> + if (!x509_location(fn, "SIA: rpkiManifest", NULL,
> + ad->location, &rpkimft))
> goto out;
> + if (cert->mft == NULL && strncasecmp(rpkimft,
> + RSYNC_PROTO, RSYNC_PROTO_LEN) == 0) {
> + cert->mft = rpkimft;
> + rpkimft = NULL;
> + continue;
> + }
> + if (verbose)
> + warnx("%s: RFC 6487 section 4.8.8: SIA: "
> + "ignoring location %s", fn, rpkimft);
> + free(rpkimft);
> + rpkimft = NULL;
> } else if (OBJ_cmp(oid, notify_oid) == 0) {
> if (!x509_location(fn, "SIA: rpkiNotify",
> HTTPS_PROTO, ad->location, &cert->notify))
> @@ -844,6 +870,10 @@ cert_parse_pre(const char *fn, const uns
> case NID_sinfo_access:
> if (sia++ > 0)
> goto dup;
> + /*
> + * This will fail for BGPsec certs, but they must omit
> + * this extension anyway (RFC 8209, section 3.1.3.3).
> + */
> if (!sbgp_sia(fn, cert, ext))
> goto out;
> break;
> Index: x509.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
> diff -u -p -r1.90 x509.c
> --- x509.c 31 May 2024 11:27:34 -0000 1.90
> +++ x509.c 3 Jun 2024 09:59:17 -0000
> @@ -467,8 +467,8 @@ out:
> }
>
> /*
> - * Parse the Subject Information Access (SIA) extension
> - * See RFC 6487, section 4.8.8 for details.
> + * Parse the Subject Information Access (SIA) extension for an EE cert.
> + * See RFC 6487, section 4.8.8.2 for details.
> * Returns NULL on failure, on success returns the SIA signedObject URI
> * (which has to be freed after use).
> */
>
--
:wq Claudio
rpki-client: improve SIA handling