amichair commented on PR #2744:
URL: https://github.com/apache/james-project/pull/2744#issuecomment-2979286911

   The kid changes are not related to low-level processing in the JWKS library 
itself, but to our partial, inconsistent and less intuitive use of it in our 
own providers. The basic idea is just that if a token contains a standard kid 
header, it references one specific key that was used to sign the token, and 
only that key should be used in verifying the token. Potentially better for 
both performance and security, and standard practice in JWT.
   
   Before the change:
   
   - We have two PublicKeyProviders (both in our own code) - the JWKS one and 
the Default one (where we load keys from pem files). The PublicKeyProvider 
interface had only one method to return all keys.
   
   - Our JWKS provider had a thing where (only in one flow) we first check for 
a kid header without verification, then if it exists we pre-set the kid in the 
provider, and then the "get all" method internally filters the keys and returns 
only the one with the matching kid. If there is no kid header it returns all 
keys as usual. So the same method is used both for "get all" and "get by kid", 
based on how the constructor was called. btw in the new jjwt api it no longer 
works - there is no way to extract the kid header without signature 
verification before setting up the provider/verifier.
   
   The changes in this PR simplify and consolidate the behavior:
   
   - Our PublicKeyProvider interface now has two separate methods, one for 
returning all keys and one for returning a single key by kid, simple and 
intuitive. Both providers now implement them. The JWKS provider no longer needs 
to be told at construction time which of the two will be called.
   
   - The verifier now internally (regardless of which provider is used) tries 
to first check for the kid (via the key locator in the jjwt api that exists for 
this purpose) and lookup the key. If found - only that key is used. If the kid 
header exists but the key is not found - it fails (it shouldn't try other 
keys). If the kid header is missing - then it falls back to trying all keys one 
by one (this is for backwards compatibility with existing tokens - we can 
decide some day to always require kid and easily get rid of the fallback).
   
   So the end result is now:
   1. using the new jjwt api
   2. using a simple and intuitive provider interface
   3. the default provider now also supports kid (as will future providers - 
they only need to implement the get(kid) method of the provider interface)
   4. the logic of first trying the kid and then falling back to try all keys 
is now in the verifier, agnostic to the specific provider in use.
   
   I added some comments in the code explaining the relevant parts of this - if 
you think it's still not enough I'd be happy to try to explain it better here 
or in the code.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org
For additional commands, e-mail: notifications-h...@james.apache.org

Reply via email to