On Wed, Mar 12, 2025 at 10:02:04AM +0100, Theo Buehler wrote:
> A non-functional CA is a CA which has not signed any currently valid
> Manifest. Such a CA does not meaningfully participate in the RPKI and
> only consumes resources. The diff below collects these CAs and adds
> support code for outputting some info on them into the JSON dump. This
> is motivated by Job's policy proposal on revoking persistently
> non-functional CAs on the the RIPE-NCC routing-wg list:
> 
> https://mailman.ripe.net/archives/list/routing-wg@ripe.net/thread/USQUMNOE3L3UUD3JZVI6LH7VMDRPL7K4/
> 
> The strategy is straightforward: build a tree of TA/CA certs sorted by
> certid and when we encounter a mft issued by the CA with certid remove
> it from the tree. This will also make it straightforward to add that to
> stats/ometrics.

Seems like a good aproach.
 
> One slightly tricky bit is to avoid flagging CAs that were skipped or
> not shortlisted. That's why the call to cert_insert_nca() is where it
> is and not in entity_process() like most other trees.

Understood, I see no better option here indeed.
 
> The other annoying bit is to get the path of the cert without .rsync/
> and .rrdp/*/ artifacts prepended to it. While this can be obtained by
> chopping up the file in entity_process(), I think it's cleaner to
> construct the DIR_VALID path and pass that over the pipe.

I dislike this part, we start to ship around paths back and forth more and
more and there is a fair amount of data duplication because of that. Also
the path is not the best identifier (apart from using it for logging).
This is not something to be fixed now but at some point we probably need
to rethink and rewrite some codepaths.
 
> The third annoying bit is the number of trees we need to pass to the
> output functions. We should probably hang all the trees off a single
> struct so we can avoid this churn when we add the next tree.

Agreed. The context needed starts to be come rather big and so having a
single state struct starts to make sense here.

I also would like to switch from RB_GENERATE() to RB_GENERATE_STATIC() and
hide the RB tree internals from all other modules but then the
RB_FOREACH() used in output*.c need to be wrapped somehow.

A few more comments below:
 
> @@ -178,6 +180,18 @@ output_json(FILE *out, struct vrp_tree *
>  		json_do_int("expires", b->expires);
>  		json_do_end();
>  	}
> +	json_do_end();
> +
> +	json_do_array("nonfunc_cas");
> +	RB_FOREACH(nca, nca_tree, ncas) {
> +		json_do_object("nca", 1);
> +		json_do_string("location", nca->location);
> +		json_do_string("ta", taldescs[nca->talid]);
> +		json_do_string("caRepository", nca->carepo);
> +		json_do_string("rpkiManifest", nca->mfturi);
> +		json_do_string("ski", nca->ski);
> +		json_do_end();
> +	};

Please remove the ; here.

>  	json_do_end();
>  
>  	if (!excludeaspa)

> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> diff -u -p -r1.150 parser.c
> --- parser.c	12 Mar 2025 07:42:39 -0000	1.150
> +++ parser.c	12 Mar 2025 08:56:51 -0000
> @@ -596,6 +596,13 @@ proc_parser_cert(char *file, const unsig
>  	if (cert->purpose == CERT_PURPOSE_CA)
>  		auth_insert(file, &auths, cert, a);
>  
> +	cert->path = parse_filepath(entp->repoid, entp->path, entp->file,
> +	    DIR_VALID);
> +	if (cert->path == NULL) {
> +		warnx("%s: failed to create file path", file);
> +		goto out;
> +	}
> +

I think this block needs to be moved up before the auth_insert() call.
Else you may end up with a broken auth tree if the file path creation
fails.

I think this should be right after
        cert->talid = a->cert->talid;
since this is where we initalize the cert.

>  	return cert;
>  
>   out:
> @@ -677,6 +684,9 @@ proc_parser_root_cert(struct entity *ent
>  	}
>  
>  	if ((cmp = proc_parser_ta_cmp(cert1, cert2)) > 0) {
> +		if ((cert1->path = strdup(file2)) == NULL)
> +			err(1, NULL);
> +
>  		cert_free(cert2);
>  		free(file2);
>  
> @@ -694,6 +704,8 @@ proc_parser_root_cert(struct entity *ent
>  		if (cert2 != NULL) {
>  			cert2->talid = entp->talid;
>  			auth_insert(file2, &auths, cert2, NULL);
> +			if ((cert2->path = strdup(file2)) == NULL)
> +				err(1, NULL);

Again I would move this before auth_insert() even though it does not
matter here.

>  		}
>  
>  		*out_cert = cert2;
> 

With those fixes OK claudio@

-- 
:wq Claudio