Closed jblachly closed 9 months ago
This is kind of expected, the Algorithm enum only includes things that this library can handle. This library also intentionally does not support JWE to not get too crazy in terms of scope. I think this is a wontfix
That's kind of a bummer; this means I can't use the library to directly deserialize JWKS from my IdP.
I don't expect the library to support JWE, but I wonder if you'll consider a POC that at least includes the enum variants?
No, because then it looks like the library is supporting them
No, because then it looks like the library is supporting them
I added new variant InvalidAlgorithmType
for this case:
https://github.com/jblachly/jsonwebtoken/blob/79774926a9c58c442cd2fe433cc032d0eecb3f22/src/crypto/mod.rs#L107
Anyway, I will respect your decision. For myself or anyone else who lands here in the future (hello from 2022) via Google, what would you recommend since we can not safely depend on being able to deserialize an IdP's JWKS response directly into jsonwebtoken::jwk::JwkSet
?
@jblachly I landed here but unfortunately after I fixed our case in other way. We have introduced the other variant which can do the catch all and skip it altogether from the deserialization. https://github.com/nick9822/jsonwebtoken/commit/3774b4266e17484bbebc6d0a662848c40cb86ece
Since this issue remains open (Thank you @Keats for leaving it open for discoverability), I would add that my fork (https://github.com/jblachly/jsonwebtoken/commits/jwe_alg) which explicitly handles JWE alg types, or @nick9822 fork, is necessary to support KeyCloak as an IdP/JWT source
Can we have another enum containing https://github.com/jblachly/jsonwebtoken/commit/79774926a9c58c442cd2fe433cc032d0eecb3f22#diff-9f82b0117510d782b99dcd289413911ac2a437265e598c9562877ad9613736d8R50-R101 and have 2 types for JWK: one supported and one unsupported? So at least the deserialization works but you don't get the impression that those are supported for encoding/decoding.
Or a different enum that contains all of Algorithm
+ the JWE algs and a function that tells whether it's supported by jsonwebtoken
What is benefit of those keys in JWK Set when we don't have encoding/decoding capabilities yet? If we still want it in Jwk Set, I think we can cover sign
and verify
at _
arm by sending error ErrorKind::UnimplementedAlgorithmType
.
What is benefit of those keys in JWK Set when we don't have encoding/decoding capabilities yet?
Just so it doesn't error out like when deserializing like in your issue https://github.com/Keats/jsonwebtoken/issues/285
To be frank, I am not sure how many such algorithms are out there, hence I followed the Other
variant with #[serde(other)]
.
Me neither. If we use a different enum than Algorithm
and add a JWK.is_supported
or something it would be ok I think.
What issues you foresee with Sign and Verify erroring out with something ErrorKind::UnimplementedAlgorithmType
? Isn't that enough or am I missing something here?
It's just not self-documenting. The Algorithm
enum is there to list all the supported algorithms. Adding a catch-all that is in fact invalid later goes against that principle.
Me neither. If we use a different enum than
Algorithm
and add aJWK.is_supported
or something it would be ok I think.
Yes, I think this will work like replacing Algorithm
with KeyAlgorithm
. And I think it's a non-breaking change.
What is benefit of those keys in JWK Set when we don't have encoding/decoding capabilities yet?
Just so it doesn't error out like when deserializing like in your issue #285
Precisely!
I still return Err(new_error(ErrorKind::InvalidAlgorithmType))
, but now it can be handled more elegantly in client code, rather than failing with a deserialization error.
Kind regards
Anyone interested in writing the PR?
Sure. I will do it.
Anyone interested in writing the PR?
Sure, I will make one for my fork. May need to update to your HEAD first; will check.
Sure. I will do it.
I'll let you handle
Hey! Any progress?
Having troubles with Keycloak too because of this issue. Is there any news?
Upd: for anyone interested, temporal workaround – disable RSA-OAEP
Provider in Realms Settings in Keycloak, so that this algorithm will not be present in JWKS response
Sorry was caught up with other things. Will raise the PR this week.
Is there any progress on this?
That should be fixed
Hi,
First, thanks for this library which has been really helpful and is built in much the same way I would have built my own JWT/JWK library.
Background
When building an API that works with Keycloak and reads its JWKS (JSON web key set), I found that I could not properly deserialize the JWKS and my program panicked on startup. Ultimately, this is because one of the JWK has an
alg
field that is not a member of theAlgorithm
enum[1]. However, this JWK is allowed by the specs (details below) and thus we cannot deserialize a spec-compliant JWK.Detailed problem
JWK specifications [2] allow an
alg
field to take the same values as allowed in JWS (JSON web signature; these values match theAlgorithm
enum variants and include things likeHS256
,RS256
,ES256
, etc.; see ref [3]) , but also to take JWE (JSON web encryption) alg types defined in [4]. These include principallyRSA-OAEP
andECDH-ES
. Note that these values would ALSO be valid values for thealg
parameter of a JWT if the JWT were encrypted (and not just signed).I can write a patch and PR for this, but I wanted to get an ACK or NACK on the overall idea, and also on specific fix that would be backward-compatible.
Proposal for backward-compatible fix
To minimize complexity, admix the
alg
types from JWS [3] and JWE [4] as variants of theAlgorithm
enum [1]. This would be an easy fix.A more complex fix might involve separating the algorithms into signing algorithms and encryption algorithms, but I don't see a ton of value in this unless additional API work is done to the crate to introspect whether a key was simply signed, or whether was encrypted. This would likely be backward-incompatible/API breaking.
@Keats let me know what you think and I'll put it together ASAP
Example JWKS with two JWK keys:
References
Algorithm
enum https://github.com/Keats/jsonwebtoken/blob/e869bff62fddb039ea20c60813549d30aa00e7f2/src/algorithms.rs#L16