lestrrat-go / jwx

Implementation of various JWx (Javascript Object Signing and Encryption/JOSE) technologies
MIT License
1.95k stars 165 forks source link

Decoding a Microsoft OAuth token: use of "jku" field specified, but the field is empty #954

Closed zrhoffman closed 1 year ago

zrhoffman commented 1 year ago

Contribution Guidelines

Before filing an issue, please read the contents of CONTRIBUTING.md, and follow its instructions.

Describe the bug What I did: Tried decode a token from https://login.microsoftonline.com/01234567-89ab-cdef-0123-456789abcdef/oauth2/v2.0/token

From https://github.com/apache/trafficcontrol/blob/79219bf6ac378ce3e6a9fd71e72d02043c59b377/traffic_ops/traffic_ops_golang/login/login.go#L486-L490 :

decodedToken, err := jwt.Parse(
[]byte(encodedToken),
jwt.WithVerifyAuto(true),
jwt.WithJWKSetFetcher(jwksFetcher),
)

Please attach the output of go version

go version devel go1.21-e126572f8a Tue Jul 4 16:54:39 2023 +0000 linux/amd64

To Reproduce / Expected behavior Please attach a standalone Go test code that shows the problem, and what you expected to happen.

To reproduce, send a valid POST request to a Microsoft Oauth endpoint like https://login.microsoftonline.com/01234567-89ab-cdef-0123-456789abcdef/oauth2/v2.0/token with valid post data in this format:

code=oauth-code
client_id=00000000-0000-0000-0000-000000000000
grant_type=authorization_code
redirect_uri=https://my.website/sso

Expected results: JWX decodes the token

Actual results: Received this error:

use of "jku" field specified, but the field is empty

Using github.com/lestrrat-go/jwx@v1.2.26. I see that useJKU is hard-coded to true in verifyCtx

https://github.com/lestrrat-go/jwx/blob/d9ddbc8e5009cfdd8c28413390b67afa7f576dd6/jws/jws.go#L262-L263

However, changing it to

ctx.useJKU = false

simply results in a different error, failed to create verifier.

Additional context In case it is useful, here is the token, decoded using https://jwt.io/ (and anonymized):

Header: Algorithm & token type ```json { "typ": "JWT", "nonce": "256-bit base64 string", "alg": "RS256", "x5t": "-152-bit base64 string", "kid": "-same 152-bit base64 string" } ```
Payload: data ```json { "aud": "00000003-0000-0000-c000-000000000000", "iss": "https://sts.windows.net/11111111-1111-1111-1111-111111111111/", "iat": 1688598206, "nbf": 1688598206, "exp": 1688602677, "acct": 0, "acr": "1", "aio": "1096-bit base 64 string", "amr": [ "pwd", "mfa" ], "app_displayname": "My app display name", "appid": "33333333-3333-3333-3333-333333333333", "appidacr": "1", "family_name": "Lastname", "given_name": "Firstname", "idtyp": "user", "ipaddr": "ipv4 address", "name": "Lastname, Firstname", "oid": "22222222-2222-2222-2222-222222222222", "onprem_sid": "S-1-5-21-4444444444-444444444-4444444444-4444444", "platf": "8", "puid": "100555555E555D55", "rh": "0.80 bit base64 string-216 bit base64 string.", "scp": "email openid profile User.Read", "signin_state": [ "kmsi" ], "sub": "13 characters-29 characters", "tenant_region_scope": "NA", "tid": "11111111-1111-1111-1111-111111111111", "unique_name": "email@address.com", "upn": "email@address.com", "uti": "128-bit base64 string", "ver": "1.0", "wids": [ "00000000-0000-0000-0000-000000000000" ], "xms_st": { "sub": "256-bit base 64 string" }, "xms_tcdt": 1375126334 } ```
Public key ```json { "e": "(removed)", "kty": "RSA", "n": "(removed)" } ```
lestrrat commented 1 year ago

From the error message, and the header information that you have provided, I'm guessing most likely means exactly what the error says: For whatever reason, you (trafficcontrol) requested jwx to decode it using JKU, but the JWS did not contain a JKU field, suggesting that the code that lead to the execution of WithVerifyAuto is wrong?

lestrrat commented 1 year ago

Was the old code expecting it to say: "Oh, this JWS doesn't have JKU, so I'm just going to let it go"? Otherwise, I think jwx is doing the right thing by rejecting JWS without a JKU.

zrhoffman commented 1 year ago

Was the old code expecting it to say: "Oh, this JWS doesn't have JKU, so I'm just going to let it go"? Otherwise, I think jwx is doing the right thing by rejecting JWS without a JKU.

It looks like before apache/trafficcontrol@aa990dfc80 (using dgrijalva/jwt-go), the old code would have panicked if there was not a JKU:


        decodedToken, err := jwt.Parse(encodedToken, func(unverifiedToken *jwt.Token) (interface{}, error) {
            publicKeyUrl := unverifiedToken.Header["jku"].(string)
            publicKeyId := unverifiedToken.Header["kid"].(string)
            /*...*/

So maybe Traffic Control never worked with Microsoft Oauth? It looks like we'll need to support decoding it without a JKU.

lestrrat commented 1 year ago

So maybe Traffic Control never worked with Microsoft Oauth?

I really don't know much about MS OAuth, but I have had people report to me strange, i.e. MS-specific, behavior from time to time in the workflow, so it wouldn't surprise me... :)

It looks like we'll need to support decoding it without a JKU.

In that case I think the current workaround would be

if err := jwx.Parse( withJKU) ;  err != nil {
   if err := jwx.Parse( withoutJKU ); err != nil {
      return fmt.Errorf(`we got a problem: %w`, err)
   }
}

I suppose jwx could incorporate a new feature, some sort of "fallback to using keys if JKU is not present" as well, but I have slight concerns in terms of security (note: haven't thought it through, so I may be wrong)

zrhoffman commented 1 year ago

I suppose jwx could incorporate a new feature, some sort of "fallback to using keys if JKU is not present" as well

If I hard-code the JKU as https://login.microsoftonline.com/<app ID>/discovery/v2.0/keys (which is in the same format as https://login.microsoftonline.com/common/discovery/v2.0/keys) at https://github.com/lestrrat-go/jwx/blob/51142ca5b628cd3c3be57e40ed477e62332a4015/jws/jws.go#L557 , verification passes, so providing a way to provide a default or override JKU would apparently make Microsoft OAuth work.

but I have slight concerns in terms of security (note: haven't thought it through, so I may be wrong)

Do you have those same concerns about providing a fallback JKU, rather than specifying fallback keys directly?

lestrrat commented 1 year ago

I think my beef is in that the jku mechanism implicitly fetches remote resources. Adding a way for users to specify that URL is always scary to me, because I firmly believe that users (not you per se, but other random users) will go out of their way to harm themselves.

I have a feeling in your usecase it's far safer to use jwk.CachedSet. e.g.

  1. determine if this is a MS token
  2. if yes, use a jwk.CacheeSet w/o using JKU
  3. if no, use JKU

(I didn't check with you if it's possible to determine if the toke is from MS, but I figured it wouldn't make sense to specify a fallback URL unless you could determine if where the token came from, so....)

that's my initial assessment. let me know if I'm missing something.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

github-actions[bot] commented 1 year ago

This issue was closed because it has been stalled for 7 days with no activity. This does not mean your issue is rejected, but rather it is done to hide it from the view of the maintains for the time being. Feel free to reopen if you have new comments