Support scenario: AzureAD has "none" signature in ID token returned

[tommed]

If I create an OAuth2/OIDC client using this library and the oauth2 golang package, it will not work with AzureAD - as it fails to verify the ID token received in the code exchange procedure during callback...

https://sts.windows.net/MY_TENANT_ID_HERE/.well-known/openid-configuration contains "id_token_signing_alg_values_supported":["RS256"] meaning it'll intend to sign the ID tokens with RS256. However, when I exchange my code for a token, the JWS has a single signature with none as the alg:

// exchange token with pkce verifier
token, err := oauth2Config.Exchange(
	r.Context(),
	code,
	oauth2.SetAuthURLParam("code_verifier", codeVerifierStr)) // pkce

idTokenStr := token.Extra("id_token"))
idToken, err := idTokenVerifier.Verify(r.Context(), idTokenStr)
// BANG: err will always be "failed to verify id token signature"

After inspecting the idTokenStr using jwt.io, I can see that it doesn't have a signature 😢 .
When stepping through the code, I can see that if I create a verifier using this code:

idTokenVerifier := *auth.Verifier(&oidc.Config{ClientID: clientID})

...and not specifying SupportedSigningAlgs, it'll default to the ones in the directory. Therefore, I tried to manually add "none" as a supported algorithm. It gets further, but still fails inside idTokenVerifier.Verify 😢 .

Therefore, this library has no method of verifying an unsigned id token - which is the behaviour AzureAD exhibits.

I realise this behaviour may be incorrect and one could argue that this is a bug with AzureAD however, I believe this library should still allow me to override the signature check under the same vein you allow me to SkipClientIDCheck, SkipExpiryCheck and SkipIssuerCheck.

[tommed]

UPDATE: This SO entry shows that AzureAD considers the exchange procedure to be "secure enough" to not need to sign the ID Token (??). So this at least proves the behaviour exits and I'm not doing something wrong.

[ericchiang]

ID Tokens MUST NOT use none as the alg value unless the Response Type used returns no ID Token from the Authorization Endpoint (such as when using the Authorization Code Flow) and the Client explicitly requested the use of none at Registration time.

https://openid.net/specs/openid-connect-core-1_0.html#IDToken

So it looks like this technically could be supported by the spec? Did you enable this during client registration somewhere?

Can you ask Azure to send you a signed token instead somehow?

[tommed]

🤷‍♂️

I'm using AuthCodeURL which sets the response_type to code.
The ID token is there, but signed with "none".

I'm also using PKCE, but there is nothing special about this AzureAD registration:

[ericchiang]

Thanks for the update :) Always love some Azure "quirks" (#344)

For what it's worth, I get the argument that "none" isn't necessarily insecure here if you're not passing the token along. But:

1. I'd want to be extremely careful with this package's API to make sure you can't do something insecure.
2. You can already get an unsigned version of the ID Token through the UserInfo endpoint[1][2]

Parsing an unsigned ID Token is relatively trivial btw, I don't think you'd need this package for it:

// exchange token with pkce verifier
token, err := oauth2Config.Exchange(
	r.Context(),
	code,
	oauth2.SetAuthURLParam("code_verifier", codeVerifierStr)) // pkce

idTokenStr := token.Extra("id_token"))
parts := strings.Split(idTokenStr, ".")
if len(parts) < 2 {
    // handle error
}
payload, err := base64.RawURLEncoding.DecodeString(parts[1])
if err != nil {
    // handle error
}
var idToken struct {
    Subject  string `json:"sub"`
    Audience string `json:"aud"`
    // whatever else you want
}
if err := json.Unmarshal(payload, &idToken); err != nil {
    // handle error
}

So I'm inclined to not support the "none" algorithm on a JWT, at least not in the IDTokenVerifier. Instead, maybe you can use the UserInfo endpoint?

[1] https://pkg.go.dev/github.com/coreos/go-oidc/v3/oidc#Provider.UserInfo
[2] https://openid.net/specs/openid-connect-core-1_0.html#UserInfo

[tommed]

Indeed 🤣 AzureAD (like the rest of Azure) is certainly an untamed and quirky beast!

Yeah I see what you mean; though I guess the way I saw it, this is potentially a legitimate and secure scenario which, if un-catered for, would mean we'd have to re-write/duplicate your parsing code (thanks for the code above!!) and validation logic too - which is always dangerous (e.g. if you release a fix and we miss merging it).

I saw you have options in oidc.Config for disabling certain parts of the validation process - each one could cause security problems (e.g., SkipIssuerCheck 😬) - and I just thought this was in such a similar vein that it could be a candidate for another one of these?

Unfortunate this conversation exists at all, however those overlords out west do enjoy to make our lives interesting! ha ha

[ericchiang]

I sent #351 and cut a release https://github.com/coreos/go-oidc/releases/tag/v3.4.0

That hopefully solves this particular case