Consider a convenience method to obtain a `Jwk` from a `JwkSet` by Key ID

Question

Consider a convenience method to obtain a `Jwk` from a `JwkSet` by Key ID

rvesse opened this issue 5 months ago · 4 comments

Problem

I'm currently switching over some JWKS support in one of our modules from using a 3rd party library to adopt the new JWKS support in jjwt as of 0.12.0 since it is far more comprehensive (thanks for the great work on that 👍 ).

One difficulty I found compared to the 3rd party library we were previously using is that there isn't a simple key lookup method exposed on a JwkSet. So my current code looks like the following:

@SuppressWarnings("unchecked") 
Jwk<Key> jwk = (Jwk<Key>) jwks.getKeys()
                              .stream()
                              .filter(k -> StringUtils.equals(k.getId(), keyId))
                              .findFirst()
                              .orElse(null);

Which seems rather complicated for what should be a simple lookup and may force an unchecked cast.

Proposed Solution

Add a simple lookup method on a JwkSet:

Jwk<Key> jwk = jwks.getKeyById(keyId);

Alternatives Considered

A JwkSet is a Map so has a get() method BUT we can't use that because the only key present in the JwkSet map is the keys key which contains the Set<Jwk<?>>. This can already be retrieved via getKeys() method as in my first code snippet, but since it's a set of objects and not a Map<String, Jwk<?>> we can't directly lookup by Key ID. Also I suspect that the use of Set was quite intentional here and changing the signature of the underlying types would be a breaking API change.

Answer 1 · 2024-02-01T20:38:40.000Z

Hi @rvesse ! Thanks for the issue! Hopefully this will help clarify what's going on:

A JwkSet is a Map<String,?> not because we should use it as "here's a map key, return a Jwk please". It's a Map specifically to reflect the fact that the JWK RFC specification requires this to be a JSON Object with potentially multiple name/value pairs, one of which is the keys member.

This is because JSON does not have any built-in concept for providing metadata about JSON Array members. So the only way to do that is to wrap the array value in a JSON Object so that JWK Set creators/publishers can provide that additional metadata if desired. For example:

{
    "self": {
        "href": "https://foo.io/keys/someId",
        "contentType": "application/jwk-set+json"
    },
    "id": "someId",
    "application: {
        "href": "https://foo.io/applicatons/applicationId"
    }
    "size": 3,
    "keys": [ jwk1, jwk2, jwk3 ],
    ... etc ...
}

Once you receive/parse a JWK Set document, there are any number of ways to look up keys: the most common way is via a key ID, but there's others. Perhaps you want all RSA keys, or all HS256 keys, etc. All of this is application specific, so these additional helper methods are omitted from JJWT's JwkSet by design.

It is expected for application developers to convert the JwkSet into any form that they need for their application's purposes.

For this particular issue/ticket, it doesn't make much sense to add something to the public JwkSet API, mostly because it's already a reflection of RFC requirements and, probably the simplest reason, is that this is one line of code in Java, and built-in to the JDK APIs:

Map<String, Jwk<?>> jwks = jwkSet.getKeys().stream().collect(Collectors.toMap(Jwk::getId, Function.identity()));
// save jwks as a class attribute for convenient re-use if desired.

So we don't really see a need to add this to the JwkSet API (and test methods, code coverage assertions, and long-term maintenance) when it's already natively supported as a one-liner in Java.

Which seems rather complicated for what should be a simple lookup and may force an unchecked cast.

The above code (Map<String, Jwk<?>>) doesn't force an unchecked cast, and it's not necessary to do so. The ? is already constrained to be a Key in the generics declaration. It's clearer with the question mark IMO to indicate that, while it is a Jwk<Key>, you don't know what type of key the Jwk holds until you iterate/inspect/etc.

Then you can use other JDK 8 one-liners to find/filter only the types you care about, etc.

Answer 2 · 2024-02-01T20:49:06.000Z

Also note that a JWK id (kid) is entirely optional per the RFC:

https://datatracker.ietf.org/doc/html/rfc7517#section-4.5

So if a JWK Set has a JWK missing that value, it's an application-specific decision on how to handle that since there's no RFC restriction/requirement that it has to be there.

In other words, because of the RFC, JJWT has to treat it as a valid JWK Set, but perhaps it's not OK if your application requires key ids, so it's better for your application to make that decision instead of JJWT trying to handle that map conversion directly.

Answer 3 · 2024-02-02T09:20:59.000Z

@lhazlewood Thanks for the detailed response, I'm happy to close this issue out as a result.

Couple of comments inline:

Hi @rvesse ! Thanks for the issue! Hopefully this will help clarify what's going on:

A JwkSet is a Map<String,?> not because we should use it as "here's a map key, return a Jwk please". It's a Map specifically to reflect the fact that the JWK RFC specification requires this to be a JSON Object with potentially multiple name/value pairs, one of which is the keys member.

This is because JSON does not have any built-in concept for providing metadata about JSON Array members. So the only way to do that is to wrap the array value in a JSON Object so that JWK Set creators/publishers can provide that additional metadata if desired. For example:

I suspect this also relates to the fact that returning a raw JSON array as a response is a subtle JSON vulnerability that's been known for a long time.

{
    "self": {
        "href": "https://foo.io/keys/someId",
        "contentType": "application/jwk-set+json"
    },
    "id": "someId",
    "application: {
        "href": "https://foo.io/applicatons/applicationId"
    }
    "size": 3,
    "keys": [ jwk1, jwk2, jwk3 ],
    ... etc ...
}
Once you receive/parse a JWK Set document, there are any number of ways to look up keys: the most common way is via a key ID, but there's others. Perhaps you want all RSA keys, or all HS256 keys, etc. All of this is application specific, so these additional helper methods are omitted from JJWT's JwkSet by design.

It is expected for application developers to convert the JwkSet into any form that they need for their application's purposes.

For this particular issue/ticket, it doesn't make much sense to add something to the public JwkSet API, mostly because it's already a reflection of RFC requirements and, probably the simplest reason, is that this is one line of code in Java, and built-in to the JDK APIs:
Map<String, Jwk<?>> jwks = jwkSet.getKeys().stream().collect(Collectors.toMap(Jwk::getId, Function.identity()));
// save jwks as a class attribute for convenient re-use if desired.
So we don't really see a need to add this to the JwkSet API (and test methods, code coverage assertions, and long-term maintenance) when it's already natively supported as a one-liner in Java.

Totally get where you're coming from here, adding to an API always has costs and trade offs associated with it

Which seems rather complicated for what should be a simple lookup and may force an unchecked cast.

The above code (Map<String, Jwk<?>>) doesn't force an unchecked cast, and it's not necessary to do so. The ? is already constrained to be a Key in the generics declaration. It's clearer with the question mark IMO to indicate that, while it is a Jwk<Key>, you don't know what type of key the Jwk holds until you iterate/inspect/etc.

Yes this was a side effect of the wider code context that I omitted from this issue, I was able to simplify to just use Jwk<?> throughout the wider code.

Then you can use other JDK 8 one-liners to find/filter only the types you care about, etc.

Answer 4 · 2024-02-02T20:20:02.000Z

@rvesse just wanted to say thanks for the thoughtful discussion! Cheers 🍻