Optional issuer validation?

[jetersen]

Hi, Thanks for this library.

I wonder what the best way to solve an issue I have with validating the issuer. Microsoft OIDC uses {tenantId} in the issuer URL returned in the claims.

In the claims I can request the tid to get the tenant id.

For now I have a branch of openid where I just ignore the issuer check. Would be interested in a upstream change that would allow for optional issuer check? Or perhaps you have another suggestion.

See: https://login.microsoftonline.com/organizations/v2.0/.well-known/openid-configuration

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

openid/src/client.rs

Lines 254 to 258 in 94cb5e9

	if claims.iss() != &self.config().issuer {
	let expected = self.config().issuer.as_str().to_string();
	let actual = claims.iss().as_str().to_string();
	return Err(Validation::Mismatch(Mismatch::Issuer { expected, actual }).into());
	}

[kilork]

Hi, thank you for your feedback! I am going to handle upcomming issues for this project, so I will check, that we can do just for this case. Microsoft is not that small company, so I think making life easier for consumers of their OIDC makes sense.

[kilork]

Specs are actually pretty clear about this.

https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata

OpenID Providers have metadata describing their configuration. These OpenID Provider Metadata values are used by OpenID Connect:

• issuer
  REQUIRED. URL using the https scheme with no query or fragment component that the OP asserts as its Issuer Identifier. If Issuer discovery is supported (see Section 2), this value MUST be identical to the issuer value returned by WebFinger. This also MUST be identical to the iss Claim value in ID Tokens issued from this Issuer.

I think we have here the following options:

1. add feature no-validate-issuer to turn this check off.
2. you may write your own authenticate function without validation.
3. last variant is just extended previous variant: we can split validation in more submethods and allow user to combine them as he wishes.

I do not think it is right thing, to allow to skip validation easily. It is described as "MUST" in specification. But I also think happy user is more important.

What do you think? Which variant from those 3 fits your needs best of all? Or maybe another idea?

[jetersen]

Ah someone wrote an excellent post about the issue: https://medium.com/@abhinavsonkar/making-azure-ad-oidc-compliant-5734b70c43ff

I think actually this solves my problem 😅

[jetersen]

Settings accessTokenAcceptedVersion to 2 does not solve the issue 😢 Although it might be delayed before the changes apply

[jetersen]

I'd be okay extending the validation.

For my workflow it may be to unpack the claims grab the tid and replace {tenantid} with tid from the claim

[jetersen]

Ah the IdToken is the one with {tenantid} value while the access token that will be corrected to align with the issuer. 😰

[jetersen]

I might end up writing my own validate_token 😓

[kilork]

@jetersen should we still improve this somehow in library?

[jetersen]

well, clearly azure ad's oidc is not compliant 😓
Not sure whether it is something we want to fix but again doing it in the library might help other users.

[kilork]

I think probably, if Microsoft so specific - we can add feature microsoft.

This feature will enable providers::microsoft::*

And there we can put all, that can make help other with Microsoft OpenID implementation.

[kilork]

@jetersen can you check #9, does it work for you?

[teohhanhui]

The upstream issue: https://github.com/MicrosoftDocs/azure-docs/issues/95994

I think this is a good suggested workaround: ramosbugs/openidconnect-rs#121 (comment)

(Not validating the "iss" claim doesn't seem like a safe thing to do...)

[kilork]

Seems like there is MS specific field tid which we can use to evaluate this `${tenantId} part (why not call this ${tid} then, MS?). Yeah, those guys really a joke.