kilork / openid

OpenID Connect Rust Library
The Unlicense
63 stars 22 forks source link

Optional issuer validation? #7

Closed jetersen closed 4 years ago

jetersen commented 4 years ago

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",
  // ...
}

https://github.com/kilork/openid/blob/94cb5e9683f557c05b5de613c9eaf6eef985b59f/src/client.rs#L254-L258

kilork commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

jetersen commented 4 years ago

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 commented 4 years ago

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

jetersen commented 4 years ago

I might end up writing my own validate_token 😓

kilork commented 4 years ago

@jetersen should we still improve this somehow in library?

jetersen commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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