search

MicrosoftDocs / azure-docs

Open source documentation of Microsoft Azure
https://docs.microsoft.com/azure
Creative Commons Attribution 4.0 International
10.2k stars 21.35k forks source link

Issuer validation and the spec violation of the `common` OpenID discovery URL #113944

Open V0ldek opened 1 year ago

V0ldek commented 1 year ago

This feedback spans a bit more than just this one page, but I'll try to lay it down step by step.

1. Issuer validation is not mentioned in the docs

When validating ID tokens in an OpenID flow it is important to validate a number of claims. One of them is Issuer (the iss claim), the authority issuing the token. The section What to validate in an ID token says:

In addition to validating ID token's signature, you should validate several of its claims as described in Validating an ID token.

and links to a different doc page. That one talks about using existing libraries, and then lists claims that are important to validate:

The following JWT claims should be validated in the ID token after validating the signature on the token. Your token validation library may also validate the following claims:

  • Timestamps: the iat, nbf, and exp timestamps should all fall before or after the current time, as appropriate.
  • Audience: the aud claim should match the app ID for your application.
  • Nonce: the nonce claim in the payload must match the nonce parameter passed into the /authorize endpoint during the initial request.

it misses the crucial Issuer validation step. There's also a link to another doc page, Secure applications and APIs by validating claims, but that one doesn't mention the Issuer claim either.

2. Issuer validation is REQUIRED by the spec and is done with UTF-8 string equality

In practice, any respectable OpenID library will validate the Issuer against a list of accepted valid issuer values. It is required by the OpenID spec, see 3.1.3.7. ID Token Validation:

The Issuer Identifier for the OpenID Provider (which is typically obtained during Discovery) MUST exactly match the value of the iss (issuer) Claim.

Please note the MUST exactly match part. The spec elaborates on how string values in claims are supposed to be compared in 14. String Operations, but it's what one would expect – parse escape sequences and then compare UTF-8 codepoints one-by-one.

3. login.microsoftonline.com violates the OpenID Discovery spec

A plain English reading of the doc suggests that the Microsoft Entra platform implements the OpenID Discovery spec, with the Discovery endpoint of https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration. Especially this paragraph under Sample response:

The configuration metadata is returned in JSON format as shown in the following example (truncated for brevity). The metadata returned in the JSON response is described in detail in the OpenID Connect 1.0 discovery specification.

Let us query the endpoint. The response looks like a standard OpenID Discovery response, with one crucial difference – the issuer property:

  "issuer": "https://login.microsoftonline.com/{tenantid}/v2.0"

This is NOT the issuer that is returned by an authorization request to a tenant. In practice (and I know this only because I work at MSFT and can check the code...) the endpoint infers the tenant ID from the clientId parameter (for multitenant apps), and issues a token with, for the example tenant of bad1dea-dead-beef-0000-00000000000:

  "iss": "https://login.microsoftonline.com/bad1dea-dead-beef-0000-00000000000/v2.0"

this is NOT an iss value that "exactly matches" the valid issuer from the Discovery endpoint. This token is invalid according to the OpenID spec. I could not find a place in the docs that points this out. If it's there it's not sufficiently visible.

4. In practice there is no spec for the issuer field in MSFT's Discovery endpoint

This carries the additional issue that currently there seems to be no documented guarantee as to what the iss will be. It is not the value found at the common endpoint, but using out-of-band information we can infer that we should actually substitute the {tenantid} with, well, the tenant's ID, and then validate the iss. But this is not documented or guaranteed by MSFT to work. There is nothing that would tell a customer that the common Discovery endpoint won't start returning https://login.microsoftonline.com/{tenant}/v2.0 as the issuer tomorrow, or https://login.microsoftonline.com/<REPLACE ME WITH THE TENANT>/v2.0, or whatever else. When following the OpenID spec changing the issuer is not a problem, since the configuration should be refetched by the validating code and then compared using standard equality.

This is essentially a single point of failure for the protocol that could introduce a breaking change without telling the customers directly that it's there and, hopefully, that MSFT commits to not changing the issuer value in the Discovery endpoint or the format of the issuers.

Moreover, the current documentation may lead to the pitfall of a developer using standard OpenID libraries (which is correct and ecouraged), bumping into the issue of the Issuer never actually matching the one obtained from Discovery, and circumventing the issue by disabling issuer validation. After all, it's Entra that is issuing spec-noncomformant tokens, so it's on us.

What would remedy this

First, the fact that the issuer field works differently than is governed by the OpenID Discovery spec should be explicitly noted in the doc. Then, some kind of guarantee from MSFT should be given in the doc that the issuer will behave predictably – essentially, since we're breaking the spec here, we should have our own specification of this particular field. Something along the lines of:

The issuer is a string value that MAY contain the special {tenantid} token. The iss of tokens issued by Microsoft Entra will be equal to that value with the token replaced with the issuing tenant's GUID. To correctly validate the token, the consumer should consult the tid claim to retrieve the tenant's ID, replace the {tenantid} token in the issuer retrieved from the Discovery endpoint, and compare the result against the iss field.

This carries the implicit contract that we realize we introduced a special parsing step to the Issuer, we commit to using the specific {tenantid} value, and gives the consumers clear and direct steps on how to securely validate tokens issued from Microsoft Entra.

Document Details

⚠ Do not edit this section. It is required for learn.microsoft.com ➟ GitHub issue linking.

RamanathanChinnappan-MSFT commented 1 year ago

@V0ldek Thanks for your feedback! We will investigate and update as appropriate.

RamanathanChinnappan-MSFT commented 1 year ago

@V0ldek I've delegated this to @OwenRichards1, a content author, to review and share their valuable insights.

osdiab commented 9 months ago

This appears to be a continuation of #38427 , where many people are frustrated by Microsoft's failure to be OIDC compliant, which causes downstream libraries to fail to work with Microsoft Auth.

I use next-auth, and ended up needing to make a patch to make the oauth4webapi library more lax in its issuer check specifically for Microsoft login: https://github.com/nextauthjs/next-auth/issues/8374#issuecomment-1803387771

In that thread, the creator of OAuth chimes in and confirms that Microsoft indeed is failing to be OIDC compliant.

For the benefit of not having developers who want to target Microsoft auth to need to roll their own security (or more likely, just not support Microsoft login), I strongly encourage Microsoft to do something about this. Understood from the previous issue that there are backwards compatibility issues with becoming OIDC compliant, but the status quo is breaking for a large number of systems (it is my understanding that large, popular auth tools like Amazon Cognito and Firebase Auth do not support Microsoft for this reason without hacks).

V0ldek commented 9 months ago

38427 was closed 4 years ago with the following:

I followed up with engineering, unfortunately it's not possible to make /common OIDC compliant right now. Closing this doc issue, feel free to reopen if necessary. #please-close

Actually being OIDC compliant is a larger issue and in all honesty I don't have a horse in that race. I accept that MSFT wants to do things differently, but those differences should be clearly documented.

antony-jr commented 6 months ago

I just commented out the issuer validation as my project has to be in Azure due to compliance issues, can't use any other OAuth provider. I hope we have some kind of workaround since Microsoft will not (i mean ever) fix this because they just don't care and also being backward compatible, another solution would be to use msal-node library which requires us to implement all api backend which I think is a decent solution compared to patching things.

EDIT: It's just too much work.

V0ldek commented 6 months ago

I just commented out the issuer validation

That's terrible and exactly what I would like to avoid by changing the docs.

The correct solution is to implement validation logic that replaces {tenantid} with the actual tenant ID, otherwise the token validation is insecure. Validation errors open you up to nasty attacks, as MSFT should well know by now.

The fact that the docs don't even mention this as a validation issue is inviting trouble.

antony-jr commented 6 months ago

@V0ldek actually, I don't know what I did but nextauth v5-beta.4, Azure AD b2c is working without any modification. I don't know what I did. It's just a mystery, so I ended up not patching the library. The ideal solution would be to move to MSAL, but as I said, too much work.