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);
>