-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JwkSet unexpectedly "secret" / Jackson cannot serialize JwkSet #976
Comments
IIRC, I ran into a similar issue, but from a different angle. I was creating a 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: jjwt/extensions/jackson/src/main/java/io/jsonwebtoken/jackson/io/JacksonSerializer.java Lines 40 to 44 in fd619e0
It's not directly exposed, but you could call the @gsprdev, I'm not sure if this helps or not, but let us know how you were using the serialized |
@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:
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 As such, you'll need to use a JJWT 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 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 When possible, it's best to render potentially unsafe JSON bytes directly to an I hope that helps! Please let us know. |
P.S. as shown in the code above, just using You're right that our documentation defaults to a runtime-only dependency on 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!
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, 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 Also, @bdemers you guess correctly; that's exactly my intended use case: Producing Due to the conventional use of a single ObjectMapper across application scope, I am somewhat uncomfortable with instantiating and immediately discarding a 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 |
Yes, indeed, and this was intentional at the time it was created. The I think we can now change that to
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 Limiting further impact on the application is also why we created our own
@gsprdev Glad to hear this worked well for you! This is exactly why all JWT and JWK implementations implement the That said, this worked because you have only public key parameters exposed, so none of them are wrapped in the redacting Based on all of the above, to translate this into actionable work, would you mind letting us know your thoughts on these questions?
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
OR this behavior can be configured in some way to allow proper serialization.
Screenshots
See code sample above.
The text was updated successfully, but these errors were encountered: