From: Theo Buehler Subject: Re: rework rpki-client certificate discovery To: tech@openbsd.org Date: Thu, 16 May 2024 12:07:21 +0200 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().