On Thu, May 16, 2024 at 11:44:25AM +0200, Claudio Jeker wrote:
> On Wed, May 15, 2024 at 08:49:02PM +0200, Claudio Jeker wrote:
> > 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.
>   
> Here is an updated version. I renamed valid_ski_aki() to valid_auth() and
> removed the ski argument. Still not super duper happy with the name but
> it is better I think.
>
> Also I print the AKI and SKI hashes in valid_auth() now. At least they
> should give an extra hint if something hits the fan.

Makes sense to me. The AKI is slightly redundant since we already have
the file name, but it doesn't hurt.

I can live with valid_auth(), but what it really does is returning the
issuing CA cert after looking it up by its internal id and seeing if
AKI/SKI match expectations. Maybe find_issuer()?

> +		warnx("%s: AKI %s doesn't match with cert SKI %s", fn,
> +		    aki, a->cert->ski);

If you go with find_issuer() this could become

		warnx("%s: AKI %s doesn't match issuer SKI %s", fn,
		    aki, a->cert->ski);

Other than that, the diff works well in the testing and now that NRO
fixed their new*.cer's AIA filemode proves to work nicely as well.

ok with either valid_auth() or find_issuer().