tekul / jose-jwt

Haskell implementation of JOSE/JWT standards
BSD 3-Clause "New" or "Revised" License
35 stars 22 forks source link

Add more JWE and JWS algorithms #31

Closed wwwater closed 3 years ago

wwwater commented 3 years ago

This change adds the missing JWS and JWE algorithms. It doesn't add any new JWK constructors that can use these algorithms. But even so it's very beneficial to have them in the enums, so that if a server has any of these algorithms in their JWK set, the parsing isn't breaking.

The actual support for signing or encrypting JWK with these algorithms can be added later.

wwwater commented 3 years ago

@tekul would you please have a look at this PR?

We're using this library extensively and it proved to be very helpful, unfortunately yesterday one of our external dependencies added a new JWK to their set exposed in OpenID Connect discovery, and it broke things for us, because the algorithm the new JWK uses is not on the list, and so, even though we don't need that JWK, we cannot even retrieve the other ones, that we do need. We had to go for an ugly hack to fix the things temporarily, therefore we'd really appreciate if you could merge this change as soon as possible.

tekul commented 3 years ago

Hi. Could you confirm that the error is just when loading a JWK set? If that's the case, then it might be better to fix the issue in that module to deal with other potential failures when reading the key data (for example an unrecognised kty field). I'd prefer not to just extend the Jwa type beyond the supported algorithms, since that may also cause failures down the line (the JWK spec also allows for custom algorithm names). One option might be to add an additional constructor to the Jwk type which just wraps the JSON value (e.g. UnknownJwk Data.Aeson.Object), so it would be possible to load any key set without failure.

wwwater commented 3 years ago

The library already has all possible kty values, so this won't be a problem. The new algorithms I've added are for RSA or EC kty types. What won't be possible now, is to actually use these algorithms to sign/verify signature or encode/decode JWTs using them. But that would require much deeper understanding of how exactly these algorithms works, which at the moment I don't possess.

Adding an UnknownJwk as you suggest wouldn't help in this case, because the problem happens inside the FromJSON for JwkSet, when one parses JwkData, because what's in the field alg cannot be parsed to Alg type.

Sorry that I wrote a misleading description when I said that I didn't add a new constructor for Jwk type. Actually no new constructor is needed, because these are the EcPrivate/PublicJwk and RsaPrivate/PublicJwk ones.

Here is an example of our problematic JWK:

{"kty":"EC"
,"use":"enc"
,"crv":"P-256"
,"kid":"jwe-encryption-key"
,"key_ops":["encrypt"]
,"x":"G8CZn6Zo7UqKLhpDFOh9njuDwBRn2xawrvf65c_t1UM"
,"y":"L3tlkG4tQhNitkJp-5XxvqnbgBGjJFpFr-WEyIUbEvA"
,"alg":"ECDH-ES"
}
tekul commented 3 years ago

The library already has all possible kty values, so this won't be a problem.

The set of kty values is not necessarily fixed. OKP was added by RFC8037 and the JWA spec allows for additonal values to be registered as it does for new algorithms.

What won't be possible now, is to actually use these algorithms to sign/verify signature or encode/decode JWTs using them

Leaving out the standard elliptic curve algorithms in favour of the RFC8037 ones is a deliberate choice since the latter are usually regarded as best practice (see https://safecurves.cr.yp.to/ or https://cryptobook.nakov.com/digital-signatures/eddsa-and-ed25519 for example).

Adding an UnknownJwk as you suggest wouldn't help in this case, because the problem happens inside the FromJSON for JwkSet

I still think this should be feasible, though "Unsupported" would probably be a better name than "Unknown". The JwkData type was originally just a hack to be able to load the data. It should be possible to customize the parseJSON implementation for Jwk appropriately. I will have a go and see.

tekul commented 3 years ago

@wwwater I added the UnsupportedJwk constructor and included your key as an additional test case, so I think this should solve your problem and also protect against similar future issues (while still expecting failure for things like invalid Base64 data or missing fields in known key types). Are you happy with that as a solution?

wwwater commented 3 years ago

@tekul thank you very much!! Your solution works perfectly! I'm closing this PR then.

tekul commented 3 years ago

Hey @wwwater. I've pushed a new release (0.9.2) with the changes. Let me know if you have any issues.