Open gsprdev opened 3 days ago
IIRC, I ran into a similar issue, but from a different angle.
I was creating a .well-known/jwks.json
, eventually i settled on:
return Jwks.set()
.add(Jwks.builder()
.id(keyId)
.rsaKeyPair(keyPair)
.build()
.toPublicJwk())
.build();
But this is specific to RSA keys, I remember seeing a similar exception to yours though, which IIRC, was related to me not having registering the JJWT Jackson Module: https://github.com/jwtk/jjwt/blob/fd619e0a4229e01cbd3ab1bd0a7a4f6cab21d784/extensions/jackson/src/main/java/io/jsonwebtoken/jackson/io/JacksonSerializer.java#L40-L44
It's not directly exposed, but you could call the JacksonSerializer
constructor to work around it.
@gsprdev, I'm not sure if this helps or not, but let us know how you were using the serialized JwkSet
that might help us better expose support for your use case.
@bdemers Thank you. Interesting. Forcing inclusion of that module does work in a forced (debug-only or unsafe reflection) scenario, but does not appear to be achievable under normal circumstances. A few issues:
runtimeOnly
It's particularly curious that JWTs and JWKs serialize just fine, with no intervention. All defaults, just with the Jackson classpath dependency. JwkSet is the only exception so far.
@gsprdev welcome!
And thank you for creating such a well-thought and detailed issue, it's a pleasure to help when receiving quality submissions :)
I think this issue elucidates a gap in our documentation, and perhaps a feature that could be added to make people's lives easier.
As you've already seen, JWKs (and by extension, JWK Sets that contain them), can contain secure data values, so we need to be careful about exposing them; JJWT is designed to hide these values by default by wrapping them in a generic Supplier
. This implies that those Supplier
wrappers aren't designed to be serialized via an 'unaware' ObjectMapper
or printed via toString
directly. They are instead designed to be serialized to JSON by passing them through a Serializer
implementation that 'knows' how to properly extract the raw value from the Supplier
wrapper.
As such, you'll need to use a JJWT Serializer
implementation to render the JSON instead of using the ObjectMapper
directly. For example:
JacksonSerializer<JwkSet> serializer = new JacksonSerializer<>(); // or new JacksonSerializer(yourObjectMapper);
JwkSet set = Jwks.set().add(jwk).build();
byte[] serialized = serializer.serialize(set);
The assumption then would be next to simply convert the serialized
byte array into a String, e.g. new String(serialized, StandardCharsets.UTF_8);
, BUT this can be very risky:
In your particular case, because your JWK Set will only contain public key data, which means there are no secret or private values, you'll be ok doing this, and your JSON string will be safe.
However, for anyone else reading this thread or this reply, converting that to a String may not be at all safe if it reflects secret or private key material: the String could easily be added to a log or printed to System.out, etc, plus the String's underlying resulting char[]
array will remain in memory.
When possible, it's best to render potentially unsafe JSON bytes directly to an OutputStream
(e.g. a ServletOutputStream
in a webapp) without converting to Strings.
I hope that helps! Please let us know.
P.S. as shown in the code above, just using new JacksonSerializer(yourObjectMapper)
will automatically register the Jackson-specific jjwt-jackson
module on that ObjectMapper
instance to ensure this behavior works as expected - no need to use the internal/impl-private fields directly.
You're right that our documentation defaults to a runtime-only dependency on jjwt-jackson
, unless you want to specify the ObjectMapper
itself. That's shown with a compile-time dependency with associated usage here:
https://github.com/jwtk/jjwt?tab=readme-ov-file#jackson-json-processor
I hope that helps!
@gsprdev Very good points, I was thinking about these when I wrote my previous message, but didn't call them out, sorry about that!
- Documentation instructs import of Jackson module as
runtimeOnly
- JacksonSerializer.MODULE is package-private
- JacksonSupplierSerializer is package-private
- JacksonSerializer constructor IS public and therefore can be theoretically be manually constructed and registered, but would need to be repeated for each relevant type individually (including hierarchically).
Related to this is how you are using the serialized data, for example, if you are creating a REST endpoint, there might be different usage expectations than reading/writing application configuration.
For example, a Spring REST Endpoint you might expect something like this to just work ™️ (or with minimal effort)
@GetMapping(path = "/.well-known/jwks.json")
JwkSet keySet() {
// ...
}
Regardless, I think we could make small tweaks in area to improve simplify the usage!
@lhazlewood Thank you for your detailed response. I certainly understand the risks of accidentally exposing secret information in logs or otherwise, and respect the effort you put into ensuring some "safe by default" behaviors in the library design. It is with that acknowledgement, however, that I filed this report after investigation. Perhaps I should have reported the Jackson serialization issue and the "private" issues separately, but after browsing your code base it seemed to me that the former was a symptom of the latter. Perhaps not.
Some items are always redacted. Some are never. As demonstrated, RsaPublicJwk
(for example) contains no redacted elements during serialization because it is understood to contain only non-sensitive parameters. PublicJwk<?>
comes from different provider configurations than other Jwk<?>
s for good reason. These work exactly as expected and desired when serialized independently (not part of a JwkSet).
I am somewhat surprised by the suggestion of overriding/avoiding both then classpath-implicit autoconfiguration and the serialization protection so carefully added in this library. Please forgive me if I misunderstand your design, but it seemed to me from a code reading was that the one-and-only JwkSet
implementation DefaultJwkSet
, hardcodes param(JwkConverter.ANY)
+ ParameterBuilder.setSecret(true)
, and no alternative (e.g. PublicJwkSet
) exists to use param(JwkConverter.PUBLIC_JWK)
+ ParameterBuilder.setSecret(false)
instead. Would you agree?
Also, @bdemers you guess correctly; that's exactly my intended use case: Producing /.well-known/jwks.json
via a simple Spring controller. Although, I encountered and reported this issue while testing in anticipation of that case, not during implementation.
Due to the conventional use of a single ObjectMapper across application scope, I am somewhat uncomfortable with instantiating and immediately discarding a io.jsonwebtoken.jackson.io.JacksonSerializer
simply for its immediate, permanent, application-wide side-effect of module registration.
Fortunately, because only JwtSet is affected (and not any other type from this library), an extremely simple workaround was possible and apparent. Speaking in Kotlin for a moment for brevity, it was a simple replacement of
fun getPubJwks() : io.jsonwebtoken.security.JwkSet {
return Jwks.set().add(listOf(pubJwk1, pubJwk2)).build()
}
with
data class MyJwkSet(val keys: List<RsaPublicJwk>)
fun getPubJwks() : MyJwkSet {
return MyJwkSet(listOf(pubJwk1, pubJwk2))
}
which is enough to be fully functional with no configuration of Jackson or its ObjectMapper whatsoever.
@gsprdev thanks for following up (and including the work around)!
We will look into ways to simplify this. My gut feel, is that public key usage (e.g. the jwks.json
endpoint should work out of the box and objects containing private keys could require a little more configuration.)
the one-and-only
JwkSet
implementationDefaultJwkSet
, hardcodesparam(JwkConverter.ANY)
+ParameterBuilder.setSecret(true)
, and no alternative (e.g.PublicJwkSet
) exists to useparam(JwkConverter.PUBLIC_JWK)
+ParameterBuilder.setSecret(false)
instead. Would you agree?
Yes, indeed, and this was intentional at the time it was created. The keys
parameter was configured as secret as a 'catch all' to indicate that anything it contains might be confidential, just to be safe and ensure something was not accidentally exposed. In retrospect, looking at this now, that was probably too aggressive a default 😅.
I think we can now change that to setSecret(false)
since each JWK within the set should always have its own parameters indicated as confidential or not.
Due to the conventional use of a single ObjectMapper across application scope, I am somewhat uncomfortable with instantiating and immediately discarding a
io.jsonwebtoken.jackson.io.JacksonSerializer
simply for its immediate, permanent, application-wide side-effect of module registration.
While I agree this is an inelegant solution (to call a constructor and then have that created object unused and immediately available for garbage collection), this does appear a valid workaround in the short term - the jjwt-jackson
module only registers a JacksonSupplierSerializer
to ensure io.jsonwebtoken.lang.Supplier
instances can be serialized, and nothing more. This is intended/expected for an application-wide ObjectMapper
so that any encounter of such instances anywhere in the application can be serialized to JSON properly (and not just, say, limited to a single REST endpoint).
Limiting further impact on the application is also why we created our own Supplier
interface and wouldn't use the JDK's native one - to ensure that logic only applied to JJWT-specific instances and wouldn't impact anything else in application code.
which is enough to be fully functional with no configuration of Jackson or its ObjectMapper whatsoever.
@gsprdev Glad to hear this worked well for you! This is exactly why all JWT and JWK implementations implement the Map
interface - for one, it's an accurate reflection of a JSON Object structure, but also for interoperability like this.
That said, this worked because you have only public key parameters exposed, so none of them are wrapped in the redacting Supplier
.
Based on all of the above, to translate this into actionable work, would you mind letting us know your thoughts on these questions?
I assume that setting the JwkSet
implementation's keys
parameter to setSecret(false)
would have done what you expected, and you probably wouldn't have ever needed to open a ticket. Is that a sufficient resolution of this issue, or do you think we should address the next question as well?
Given that the JacksonSerializer
constructor that accepts an ObjectMapper
currently purposefully intends to modify an application-wide instance (for universal ability to serialize JJWT Supplier
instances wherever they may be encountered in application code), do you feel it is necessary/preferred to make that Module
constant public
so that developers can manually configure some ObjectMapper
instances but not others? It's unclear to me how likely/desirable this is given that my assumption is that most devs want it to 'just work everywhere in my application'.
Thoughts? Thank you for your feedback! It is appreciated :)
Describe the bug
The default (and only) JWK set builder considers all "keys" members as secret, even if the set contains only public keys. In turn, this results in a failure to serialize the set using Jackson redacted data cannot be serialized.
To Reproduce
To repro
A complete example follows.
Expected behavior
Screenshots
See code sample above.