feat(jwt) support for config.key_claim_name in header

[brycehemme]

Addresses the issue discussed in #2858.

Context

The JWT plug-in previously only supported setting the config.key_claim_name parameter to a claim in the body and not the header. Many identity providers who use JWKS (such as Amazon Cognito) support signing their tokens with more than one public key. This results in the need to support the "kid" (key ID) claim which identifies the public key required to validate the signature of the token. Due to the JWKS defining multiple public keys, the standard "iss" (issuer) claim is not unique. The "kid" claim is typically set in the header of the token and therefore requires also checking the token header for claims. The change that was made is fully backwards compatible due to keeping the functionality that checks the token body claims first and if the config.key_claim_name is not found there it then checks the header. An additional test case was also written to confirm that setting config.key_claim_name to a claim that is in the header succeeds.

Solution

The current release of the JWT plugin only checks the token body (claims) for the config.key_claim_name parameter. While this works for most scenarios, in the aforementioned scenario of the "kid" claim in the header, this does not suffice. Adding an additional check against the header of the token if the config.key_claim_name parameter isn't found in the body resolves this issue.

After making this change, an IdP that uses more than one public key can be configured for a single consumer as follows -

# Configure the plugin and ensure config.key_claim_name is set to the appropriate header claim -
curl -X POST http://localhost:8001/apis/myapi/plugins --data "name=jwt" --data "config.claims_to_verify=exp" --data "config.key_claim_name=kid"

# Configure the FIRST public key for the consumer. Each of the parameters in curly braces should be replaced to fit your scenario -
curl -i -X POST http://localhost:8001/consumers/{MY_CONSUMER}/jwt -F "algorithm=RS256" -F "rsa_public_key=@{PATH_TO_MY_FIRST_PUBLIC_KEY.pem}" -F "key={MY_FIRST_PUBLIC_KEY_KID}"

# Configure the SECOND public key for the consumer. Each of the parameters in curly braces should be replaced to fit your scenario -
curl -i -X POST http://localhost:8001/consumers/{MY_CONSUMER}/jwt -F "algorithm=RS256" -F "rsa_public_key=@{PATH_TO_MY_SECOND_PUBLIC_KEY.pem}" -F "key={MY_SECOND_PUBLIC_KEY_KID}"

See also

JWKS standard: https://tools.ietf.org/html/rfc7517#section-5
Related issue: #2858

[bungle]

If the claim is found in both payload and header, the header one is never used. And I think you did this for better backward compatibility. I was just thinking, could we try both in that scenario and if one works (to signature verification) then continue. For that you may want to extract signature verification (and cache/db key loading) to its own function(s). And then just try both. If one works, then it is ok. @brycehemme Any thoughts on this? Yes, it makes it a bit more complex, but also a bit more robust. Of course in most cases the payload or the header one is nil so you will still in most cases try signature verification just once?

[brycehemme]

That makes sense although it does seem to add unnecessary complexity, in my opinion. It seems like a major edge case where the same claim would exist in the body and the header, each having a different value, and one of them being intended to be used for looking up the proper key for checking the signature. Regardless, if you think this is necessary I can certainly make the change.

Additionally, I had considered instead handling the naming of the value for the "config.key_claim_name" as explicitly referring to the header. So, something like this -

config.key_claim_name=header.kid

So, if the value is prefixed with "header." then it will explicitly look at the header claims and if it isn't prefixed with "header." then it will use the standard claims.

Thoughts on this approach vs. what you suggested vs. the way I've already implemented it?

Also, thanks for kicking the build!

[brycehemme]

@bungle - Can I get your thoughts on the above comments? I'm happy to address however you feel is appropriate. Thanks!

[bungle]

I think we have a few options:

1. leave it as is
2. use prefix header. or something to refer claims in JWT headers
3. try both if first one didn't work
4. add new config value, e.g. config.key_claim_in=payload|header (default being payload)
5. similar to 4. but ordered array (of these two possible values) config.key_claim_in=header,payload
6. A boolean value like config.key_claim_in_header=true|false (false` being default)

Maybe there are other options as well. @brycehemme what do you think? I don't have a favourite.

[brycehemme]

I would opt for leaving it as is (option 1). I implemented option 2 above but after I completed it I didn't really like having the logic in the code. Although it would work just fine, it just makes things more confusing than necessary, in my opinion. As you can tell, I prefer reducing complexity unless it is absolutely necessary. Any issues with accepting this PR in this case?

[thibaultcha]

I don't think this test should be in the "refusals" section. The below test for "returns 200 on OPTIONS" either, but that is another issue.

[thibaultcha]

Rebased and merged with the necessary modifications, thank you for the patch!