From: Theo Buehler Subject: Re: rpki-client: check certificate CRLDP alignment To: Job Snijders Cc: tech@openbsd.org Date: Sat, 12 Sep 2026 06:40:36 +0200 On Sat, Sep 12, 2026 at 12:55:04AM +0000, Job Snijders wrote: > RFC 6480 section 4.2 figure 2 shows that all valid products in the same > CA repository (i.e., listed on the same manifest) point to the same CRL. > > Already today rpki-client checks whether the CRLDP in a given Manifest > EE certificate aligns with the CRL location derived from the fileList > (see mft.c lines 193-197). This diff adds a similar check to verify > alignment of the CRLDP in CA certificates, BGPsec certs, and the EE > certs in other signed objects. As already mentioned elsewhere I very much agree with the idea. I think this is better and cleaner than the parse_filepath() approach. I believe the execution needs a bit more thought. See the comments inline. The diff is a bit repetitive because the proc_parser_foo() have multiple layering problems which I'm in the process of cleaning up. This adds one more knot to the tangle which I will need to undo again in a few days. I can deal with that, but I'd rather not. > > Index: crl.c > =================================================================== > RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v > diff -u -p -r1.52 crl.c > --- crl.c 1 Dec 2025 14:40:56 -0000 1.52 > +++ crl.c 12 Sep 2026 00:31:30 -0000 > @@ -360,6 +360,7 @@ crl_free(struct crl *crl) > return; > free(crl->aki); > free(crl->mftpath); > + free(crl->path); > X509_CRL_free(crl->x509_crl); > free(crl); > } > Index: extern.h > =================================================================== > RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v > diff -u -p -r1.294 extern.h > --- extern.h 3 Sep 2026 17:19:30 -0000 1.294 > +++ extern.h 12 Sep 2026 00:31:31 -0000 > @@ -543,6 +543,7 @@ struct crl { > RB_ENTRY(crl) entry; > char *aki; > char *mftpath; > + char *path; /* canonical path: rsync://... */ mftpath and path are extracted from the mft's EE cert. They end up in struct crl in two very different ways. I find this confusing. More on this below. Also, I'm not sure mftpath and path are great names. Perhaps we can come up with something better? Maybe mftsia and mftcrldp (yeah, that's ugly). At least these would give a clearer hint where they come from. > X509_CRL *x509_crl; > time_t thisupdate; /* do not use before */ > time_t nextupdate; /* do not use after */ > Index: parser.c > =================================================================== > RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v > diff -u -p -r1.184 parser.c > --- parser.c 3 Sep 2026 17:16:51 -0000 1.184 > +++ parser.c 12 Sep 2026 00:31:31 -0000 > @@ -220,6 +220,11 @@ proc_parser_roa(char *file, const unsign > goto out; > crl = crl_get(&crls, a); > > + if (strcmp(cert->crl, crl->path) != 0) { > + warnx("%s: invalid CRLDP pointer", file); > + goto out; > + } > + > if (!valid_x509(file, ctx, cert->x509, a, crl, &errstr)) { > warnx("%s: %s", file, errstr); > goto out; > @@ -262,6 +267,11 @@ proc_parser_spl(char *file, const unsign > goto out; > crl = crl_get(&crls, a); > > + if (strcmp(cert->crl, crl->path) != 0) { > + warnx("%s: invalid CRLDP pointer", file); > + goto out; > + } > + > if (!valid_x509(file, ctx, cert->x509, a, crl, &errstr)) { > warnx("%s: %s", file, errstr); > goto out; > @@ -433,6 +443,11 @@ proc_parser_mft_pre(struct entity *entp, > *crl = parse_load_crl_from_mft(entp, mft, DIR_TEMP, crlfile); > if (*crl == NULL) > *crl = parse_load_crl_from_mft(entp, mft, DIR_VALID, crlfile); > + if (*crl == NULL) > + goto err; The reason we currently don't check *crl here is to let valid_x509() set the errstr ("boo.mft: unable to get certificate CRL"). proc_parser_mft() picks this errstr up and reports it if needed. With this change we will fall back to the unexplained "no valid mft available" more often which I think is harder to debug. > + > + if (((*crl)->path = strdup(cert->crl)) == NULL) > + err(1, NULL); I am pretty sure this strdup() belongs into parse_load_crl_from_mft() right next to the strdup for crl->mftpath like you did in your parse_filepath() version of the diff. One possibility would be this: *crl = parse_load_crl_from_mft(entp, mft, DIR_TEMP, cert->crl, crlfile); if (*crl == NULL) *crl = parse_load_crl_from_mft(entp, mft, DIR_VALID, cert->crl, crlfile); and we can keep the error reporting as it is. But I'm not convinced it is the best way, again because mftpath and path are handled very differently. Ultimately, both crl->mftpath and crl->path come from the mft's EE cert, so I wonder if we should not just add mft->crldp which would be set next to the mft->aki and mft->sia in mft_cert_info(). Then we can strdup that member directly in parse_load_crl_from_mft(). > > a = find_issuer(file, entp->certid, mft->aki, NULL); > if (a == NULL) > @@ -622,6 +637,11 @@ proc_parser_cert(char *file, const unsig > goto out; > crl = crl_get(&crls, a); > > + if (strcmp(cert->crl, crl->path) != 0) { > + warnx("%s: invalid CRLDP pointer", file); > + goto out; > + } > + > if (!valid_x509(file, ctx, cert->x509, a, crl, &errstr) || > !valid_cert(file, a, cert)) { > if (errstr != NULL) > @@ -784,6 +804,11 @@ proc_parser_aspa(char *file, const unsig > goto out; > crl = crl_get(&crls, a); > > + if (strcmp(cert->crl, crl->path) != 0) { > + warnx("%s: invalid CRLDP pointer", file); > + goto out; > + } > + > if (!valid_x509(file, ctx, cert->x509, a, crl, &errstr)) { > warnx("%s: %s", file, errstr); > goto out; > @@ -824,6 +849,11 @@ proc_parser_tak(char *file, const unsign > if (a == NULL) > goto out; > crl = crl_get(&crls, a); > + > + if (strcmp(cert->crl, crl->path) != 0) { > + warnx("%s: invalid CRLDP pointer", file); > + goto out; > + } > > if (!valid_x509(file, ctx, cert->x509, a, crl, &errstr)) { > warnx("%s: %s", file, errstr); >