From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rework rpki-client certificate discovery
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Wed, 15 May 2024 20:49:02 +0200

On Wed, May 15, 2024 at 08:04:22PM +0200, Theo Buehler wrote:
> On Wed, May 15, 2024 at 05:04:57PM +0200, Claudio Jeker wrote:
> > In rpki-client we use the SKI / AKI to find the signing certificate and
> > build the validation stack based on this data. The problem is that the SKI
> > is not strictly unique and for example the NRO TAL abuses this.
> > 
> > Instead of indexing certs by SKI we now use a unique per certificate id
> > which is then passed back and forth between main process and the parser.
> > This way we never lose track on the cert that signed the current object
> > and can look that one up via its id.  This allows for multiple validation
> > paths to co-exist.
> 
> I never liked the auth lookup by SKI and I am really happy to see it go.
> I've long suspected that we throw away important information and the new
> approach shows that this is indeed the case. I am much happier with this.
> 
> The comparison of the aki with the issuer's ski ensures that we don't
> throw away information - this is redundant with check_issuer()'s call to
> X509_check_akid() in the verifier anyway.

Yeah, valid_aki_ski() feels like duplicated work now. We'll figure out how
and what we want to do.
 
> One other thing worth mentioning is that by keeping track of the cert
> depth in auth_insert() we reinstate the maximum depth check that was
> lost when doing the partial chain optimization.

:) forgot about that bit.
 
> > Now on top of this the filepath_add() api had to be extended since we now
> > allow a file to be visited more then once as long as it is reached by a
> > different TAL. Inside the same TAL an object can not be revisited.
> > 
> > Last but not least the filemode had to be adjusted, now the code keeps
> > track of the various AIA uri and uses an own rb tree to look them up.
> > Once there is a hit the stack can be unrolled like before and all new
> > certs are added to both the auth and the uripath trees.
> 
> I think this is also an improvement.

Me too.
 
> > More cleanup needs to follow (I'm looking at you valid_aki_ski())
> > 
> > This was developped together with tb@ at a femto-hackathon.
> 
> A nit and a bug in the beloved valid_ski_aki() below, with those fixed

Fixed in my tree. Thanks for spotting the valid_ski_aki() problem.
 
> ok tb
> 
> But I'd be happy to see some more testing.
> 
> > -- 
> > :wq Claudio
> > 
> > +	for (i = 0; i < tal->urisz; i++) {
> > +		if (strncasecmp(tal->uri[i], RSYNC_PROTO, RSYNC_PROTO_LEN) != 0)
> > +			continue;
> > +		/* Add all rsync uri since any of them could be use as AIA. */
> 
> my typo: s/use/used
> 
Fixed.

> >  struct auth *
> > -valid_ski_aki(const char *fn, struct auth_tree *auths,
> > +valid_ski_aki(const char *fn, struct auth_tree *auths, int id,
> >      const char *ski, const char *aki, const char *mftaki)
> >  {
> >  	struct auth *a;
> >  
> > +	/* XXX - ski is now unused. Inline these functions in parser.c? */
> >  	if (mftaki != NULL) {
> >  		if (strcmp(aki, mftaki) != 0) {
> >  			warnx("%s: AKI doesn't match Manifest AKI", fn);
> > @@ -97,32 +98,16 @@ valid_ski_aki(const char *fn, struct aut
> >  		}
> >  	}
> >  
> > -	if (auth_find(auths, ski) != NULL) {
> > -		warnx("%s: RFC 6487: duplicate SKI", fn);
> > -		return NULL;
> > -	}
> > -
> > -	a = auth_find(auths, aki);
> > +	a = auth_find(auths, id);
> >  	if (a == NULL)
> > -		warnx("%s: RFC 6487: unknown AKI", fn);
> > -
> > -	return a;
> > -}
> > +		warnx("%s: RFC 6487: unknown cert", fn);
> 
> I think we want to do
> 
> 	if (a == NULL) {
> 		warnx("%s: RFC 6487: unknown cert", fn);
> 		return NULL;
> 	}
> 
> because...
> 
> > +	if (strcmp(aki, a->cert->ski) != 0) {
> 
> ... at this point we don't know that a != NULL. I think it's nicer than:
> 
> 	else if (strcmp(aki, a->cert->ski) != 0) {
> 

Agreed.

-- 
:wq Claudio