jsonwebtoken
jsonwebtoken copied to clipboard
Keats
JWK `alg` types are missing and deserialization from spec-compliant JWKS will fail
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 the Algorithm 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 the Algorithm enum variants and include things like HS256, RS256, ES256, etc.; see ref [3]) , but also to take JWE (JSON web encryption) alg types defined in [4]. These include principally RSA-OAEP and ECDH-ES. Note that these values would ALSO be valid values for the alg 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 the Algorithm 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:
{"keys":[{"kid":"o_1BuEV1jxuAF5JjxqXHJGVz-TLQlLYjiN9B1-iJBjE","kty":"RSA","alg":"RSA-OAEP","use":"enc","n":"sfVxWNONsub1cZBTWecYDBZFTfZenOZuX7mN5KTpl3tT8Hh7e8Cx-ljBCKI8JJL0R4jjiQR_e4_ymA1E_73JKmE0kXakHHETHEia7-1ybM4h3aApksAmDGfLu5nG4a3BucEm3edW7Rpq_iX5_m658JL8dkwbgfUoZxjDq3L_Fsuzm5BluGeTjP5_QtzZP3Pv88O6sjZvIeiMG94JPfhgbT6nSlwTQ9kqs1vMQrps19tLFWSSaB-DJcO6tycR2h6haAUI_VyfMS10yiDsi_iCNR_x3JGLM4a1B_W1ABtzxdnrpI_gLJouh0v6UtU8SS9H4zoUJe_iVVKg3Gof9mN03Q","e":"AQAB","x5c":["MIICqTCCAZECBgGAbEoS+TANBgkqhkiG9w0BAQsFADAYMRYwFAYDVQQDDA13b2RlaG91c2UtZGV2MB4XDTIyMDQyNzE4Mjg1NVoXDTMyMDQyNzE4MzAzNVowGDEWMBQGA1UEAwwNd29kZWhvdXNlLWRldjCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALH1cVjTjbLm9XGQU1nnGAwWRU32Xpzmbl+5jeSk6Zd7U/B4e3vAsfpYwQiiPCSS9EeI44kEf3uP8pgNRP+9ySphNJF2pBxxExxImu/tcmzOId2gKZLAJgxny7uZxuGtwbnBJt3nVu0aav4l+f5uufCS/HZMG4H1KGcYw6ty/xbLs5uQZbhnk4z+f0Lc2T9z7/PDurI2byHojBveCT34YG0+p0pcE0PZKrNbzEK6bNfbSxVkkmgfgyXDurcnEdoeoWgFCP1cnzEtdMog7Iv4gjUf8dyRizOGtQf1tQAbc8XZ66SP4CyaLodL+lLVPEkvR+M6FCXv4lVSoNxqH/ZjdN0CAwEAATANBgkqhkiG9w0BAQsFAAOCAQEACz06NzjuOlam05axuDk0ndtgMLJpDR4CUHF8+lZ8V2+Z/0LgthxC147zCxWZb7evf9neWagSFj8tPgarysvmF4J5kQqhDaVC5Ow1jI0GPVOyiAvBuL15YqTGAu3XCDe2ZjgbIe/v2c/j/SF5AY0Xn9ld/nWgGmc1b/BpwEnIPjN/b4Uh9pAGKef7n/EdHtBVd2dL8wmi7xjh8xI3csBum0QSAgCm8JN1V77InzJSHiMfv8ByYM+DAbOqegfIk/fe5mGQMBgXQBE6aFOwEr9TaUON+w4Gt3IDo+7vAA2V5iIJfb15T1Dmfxl1Usn/tepkJd8sbBY+0v/IMJsbQTNaag=="],"x5t":"FwmELCykJFSLc6hr259wofuTN4c","x5t#S256":"U2cXH6ZDZ-8HL_o45HO510IFH4VNVTX5S4jykCu0aVs"},{"kid":"EepFZ_jHJSX1U-4Yg7m0qtVGuwK_1VUMQ-IQok5HMII","kty":"RSA","alg":"RS256","use":"sig","n":"qubKJfYNvTFQwWr0J-djEMAlz_FyQNCpAhDZ2DgbF2VZK-kUt2JQv-KsCDONiPj_YP7wDseOtrTHCvhuS9KzPSinSed3RSTabzyAm9Upn7upMdAl-nZYvEPQ0CfcuydUtaamrS0HyWdzX6mHRYwW20m2xxZiP0bU3n54ydo_ygRYM1JNK9JFS_3-GkxFP3bm0UhSiXDlnRtE-CRsq3k37KMjYt8hUfHwO6Y4syeDTEJyND3tXJFbdsvO3rqTa-bFQyx6qweaXCPJ_5iU4KKskHWYYr-Mmr5kHCh58JED8Uwhkd9lexcYefZ46BvmfRHYb3_KP8bjDZEmZ5xfES240w","e":"AQAB","x5c":["MIICqTCCAZECBgGAbEoSwDANBgkqhkiG9w0BAQsFADAYMRYwFAYDVQQDDA13b2RlaG91c2UtZGV2MB4XDTIyMDQyNzE4Mjg1NVoXDTMyMDQyNzE4MzAzNVowGDEWMBQGA1UEAwwNd29kZWhvdXNlLWRldjCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAKrmyiX2Db0xUMFq9CfnYxDAJc/xckDQqQIQ2dg4GxdlWSvpFLdiUL/irAgzjYj4/2D+8A7Hjra0xwr4bkvSsz0op0nnd0Uk2m88gJvVKZ+7qTHQJfp2WLxD0NAn3LsnVLWmpq0tB8lnc1+ph0WMFttJtscWYj9G1N5+eMnaP8oEWDNSTSvSRUv9/hpMRT925tFIUolw5Z0bRPgkbKt5N+yjI2LfIVHx8DumOLMng0xCcjQ97VyRW3bLzt66k2vmxUMseqsHmlwjyf+YlOCirJB1mGK/jJq+ZBwoefCRA/FMIZHfZXsXGHn2eOgb5n0R2G9/yj/G4w2RJmecXxEtuNMCAwEAATANBgkqhkiG9w0BAQsFAAOCAQEAaGcG+VQggrkES54Emv30gJrxrj2yIN8yPxlPp+yTN5UWD23gGRsqbtQ5ZXx6LPg29/1uDifsdVd82SYfhcQL8XMwivXddrOmiioiiMeWS5+eT1lZpJOe2SmLFgy6OPaRV/g1o2wHB3s5IL6I9Ea8ws+iPdLYpfbjJEngpWbZvN8ZNJ8szeDws0JPhYoL5i6tqt988vCmmKbojlQ4YeC1aDpejzN91aFRuKeU5aW9lxfTWxLyhk7benSK7fUKuMvBhiBSIYE2aIFd52hPE9NjBRK5+VCeYh4ANbvvfjZXKgjMmKi0OvZRvdtDldEuvXWibcHgfXiPmV4pURXzdl0oWA=="],"x5t":"s4r-ka7oONGt9FgMQ42b29Eqqw4","x5t#S256":"rhirwtE69oKd-Q4SyPF_ko5dnFOXLLfE5-FrBLkfUdU"}]}
References
Algorithmenum https://github.com/Keats/jsonwebtoken/blob/e869bff62fddb039ea20c60813549d30aa00e7f2/src/algorithms.rs#L16- JWK: https://datatracker.ietf.org/doc/html/rfc7517#section-4.4
- JWS algorithms: https://www.rfc-editor.org/rfc/rfc7518.html#page-6
- JWE algorithms: https://www.rfc-editor.org/rfc/rfc7518.html#page-12
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
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
Algorithmand add aJWK.is_supportedor 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 make one for my fork. May need to update to your HEAD first; will check.~~
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
