From: Claudio Jeker Subject: Re: rpki-client: improve SIA handling To: Theo Buehler Cc: tech@openbsd.org Date: Mon, 3 Jun 2024 14:15:07 +0200 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