search

google / jwt_verify_lib

Provide c++ library to verify JWT tokens
Apache License 2.0
42 stars 43 forks source link

jwt_verify_lib complains about invalid kty/alg combination #49

Closed volkdir closed 4 years ago

volkdir commented 4 years ago

Our identity provider (forgerock am ) delivers IMHO a valid jwks:

   {
            "kty": "RSA",
            "kid": "....",
            "use": "sig",
            "x5t": "....",
            "x5c": [
                "......."
            ],
            "n": ....",
            "e": "AQAB",
            "alg": "PS512"
        },

(https://www.rfc-editor.org/rfc/rfc7518.html#section-3.3) Expected result: The key is read successfully by the istio envoy.

Actual result: envoy complains about: [alg] is not started with [RS] for a RSA key

This is referred in this istio issue: https://github.com/istio/istio/issues/22597

qiwzhang commented 4 years ago

I think "alg" field should be "RS512" not "PS512". A typo?

volkdir commented 4 years ago

No typo. This is provided by the forgerock AM Identity provider. According to the RFC it is a valid value for alg: https://www.rfc-editor.org/rfc/rfc7518.html#section-3.1

 +--------------+-------------------------------+--------------------+
   | "alg" Param  | Digital Signature or MAC      | Implementation     |
   | Value        | Algorithm                     | Requirements       |
   +--------------+-------------------------------+--------------------+
   | HS256        | HMAC using SHA-256            | Required           |
   | HS384        | HMAC using SHA-384            | Optional           |
   | HS512        | HMAC using SHA-512            | Optional           |
   | RS256        | RSASSA-PKCS1-v1_5 using       | Recommended        |
   |              | SHA-256                       |                    |
   | RS384        | RSASSA-PKCS1-v1_5 using       | Optional           |
   |              | SHA-384                       |                    |
   | RS512        | RSASSA-PKCS1-v1_5 using       | Optional           |
   |              | SHA-512                       |                    |
   | ES256        | ECDSA using P-256 and SHA-256 | Recommended+       |
   | ES384        | ECDSA using P-384 and SHA-384 | Optional           |
   | ES512        | ECDSA using P-521 and SHA-512 | Optional           |
   | PS256        | RSASSA-PSS using SHA-256 and  | Optional           |
   |              | MGF1 with SHA-256             |                    |
   | PS384        | RSASSA-PSS using SHA-384 and  | Optional           |
   |              | MGF1 with SHA-384             |                    |
   | PS512        | RSASSA-PSS using SHA-512 and  | Optional           |
   |              | MGF1 with SHA-512             |                    |
   | none         | No digital signature or MAC   | Optional           |
   |              | performed                     |                    |
   +--------------+-------------------------------+--------------------+
qiwzhang commented 4 years ago

then, PS256 is not supported yet. Someone needs add such supports. Just like this PR

jheiss commented 4 years ago

I do think there's an interesting question as to whether this library should ignore keys in the JWKS that it doesn't understand. I've gone back and forth on that while contemplating the ed25519 support. My initial experience was the same as described here, envoy just failed when it saw my JWKS with ed25519 keys. I made a local copy of the JWKS without the problematic keys and fed it to envoy with local_jwks instead of remote_jwks. That allowed me to test envoy with RSA keys, but since we prefer ed25519 in our environment I've ended up implementing ed25519 support here.

So far I think this is the only JWKS code I've encountered that rejects a JWKS with keys it doesn't support. I can see an argument for that behavior, as you might be surprised that the code seems to read the JWKS but then keys are missing. But on the other hand users are going to encounter JWKSs with new types of keys and the inability to use any of the keys in the JWKS is problematic.

qiwzhang commented 4 years ago

Thanks for raising this issue. I think this library should ignore "jwk" that it dosn't support. Filed this issue to track it.

ackerleytng commented 4 years ago

I'd like to give this a shot. I'll work on adding PS256 support if nobody else is working on this yet?

qiwzhang commented 4 years ago

@ackerleytng Please go ahead. Thanks.

ackerleytng commented 4 years ago

Please see PR #53 ! Thanks. Still WIP, need to add more test cases.