Re: [PATCH v2] keys: X.509 public key issuer lookup without AKID

[Jarkko Sakkinen <jarkko@kernel.org>]

On Fri, Jan 15, 2021 at 11:40:18AM +0100, Andrew Zaborowski wrote:
> On Fri, 15 Jan 2021 at 09:20, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > On Thu, Jan 14, 2021 at 09:40:35PM +0100, Andrew Zaborowski wrote:
> > > @@ -183,8 +193,8 @@ bool asymmetric_key_id_partial(const struct asymmetric_key_id *kid1,
> > >  EXPORT_SYMBOL_GPL(asymmetric_key_id_partial);
> > >
> > >  /**
> > > - * asymmetric_match_key_ids - Search asymmetric key IDs
> > > - * @kids: The list of key IDs to check
> > > + * asymmetric_match_key_ids - Search asymmetric key IDs 1 & 2
> > > + * @kids: The pair of key IDs to check
> > >   * @match_id: The key ID we're looking for
> > >   * @match: The match function to use
> > >   */
> > > @@ -198,7 +208,7 @@ static bool asymmetric_match_key_ids(
> > >
> > >       if (!kids || !match_id)
> > >               return false;
> > > -     for (i = 0; i < ARRAY_SIZE(kids->id); i++)
> > > +     for (i = 0; i < 2; i++)
> > >               if (match(kids->id[i], match_id))
> > >                       return true;
> > >       return false;
> >
> > Why are key ID 2 and key ID 3 handled differently? They are both
> > optional.
> 
> This is to minimise the impact of having a new ID added.  I guess the
> danger is that it could add ambiguity in the lookup, i.e. a different
> key could be returned for an existing search query.

Right I do get that. It could potentially break some scripts.

> 
> There's a specific logic in how ID 1 and 2 interact documented as
> follows in restrict.c:
> 
>                         * The first auth_id is the preferred id, and
>                         * the second is the fallback. If only one
>                         * auth_id is present, it may match against
>                         * either signer_id. If two auth_ids are
>                         * present, the first auth_id must match one
>                         * signer_id and the second auth_id must match
>                         * the second signer_id.
> 
> I'm not sure what the use case motivates this.  For the
> x509_public_key subtype you'd expect that ID 1 in the signature
> matches subject ID 1 of the issuer and ID 2 matches ID 2.  Most of the
> time both will be present for a CA certificate.
> 
> I imagine restrict.c only tries to mirror the logic that was already
> implemented in find_asymmetric_key when the restrict functions were
> added.
> 
> For ID 2, only ever filled in by the x509_public_key subtype (right
> now), we only have any use for it being matched against the issuer's
> ID 2.
> 
> Note: asymmetric_match_key_ids can be used as part of a generic key
> API query, or as part of a find_asymmetric_key call (only used in
> crypto/asymmetric_keys/ for trust verification and similar) but
> find_asymmetric_key will then perform an extra check.  In any case
> without more background I think it's preferable to minimize the
> matching logic changes, and even assuming that the logic could be
> improved it may be best to keep it as is because existing tools may
> rely on it.

You could give a couple of usage examples, by using this cert

https://letsencrypt.org/docs/certificates-for-localhost/

That is good information to store in the commit log for future and
also works as a tested-by criteria.

/Jarkko