Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow the OIDC issuer verification to be skipped if desired. #467

Merged

Conversation

chkohner
Copy link
Contributor

Currently, if the issuer URL differs from the one specified in the config (and indeed, used for discovery), the process will exit with an error. This is expected behavior, and the default. However some providers, namely Azure AADv2 do not correctly report their issuer.

For example: the issuer URL: https://login.microsoftonline.com/organizations/v2.0 will incorrectly return https://login.microsoftonline.com/{tenantid}/v2.0, which while close, isn't a match so the verification fails. This flag allows you to explicitly disable that check.

This idea originally came from #308, however that solution was incomplete because it didn't handle the NewProvider() call which does issuer verification.

How Has This Been Tested?

Tested in Windows, Go 1.14, vs Microsoft Identity platform (Azure AADv2 OIDC).

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

options.go Outdated Show resolved Hide resolved
@JoelSpeed
Copy link
Member

How does this change relate to #458 (comment), there's a mention here that the OIDC issuer can be used with Azure without modification, is this a different Azure API?

@edemen
Copy link

edemen commented Apr 9, 2020

How does this change relate to #458 (comment), there's a mention here that the OIDC issuer can be used with Azure without modification, is this a different Azure API?

Just to clarify, I originally used the same URL for OIDC that I saw being used for provider=azure, which is https://login.microsoftonline.com/<tenantID>, which naturally gave me an error: oidc: issuer did not match the issuer returned by provider, expected "https://login.microsoftonline.com/<tenantID>" got "https://sts.windows.net/<tenantID>/"
Then, when I changed the issuer URL to https://sts.windows.net/<tenantID>/ it worked.

So, technically the original URL for provider=azure would probably work for provider=oidc because Microsoft can internally map it to the desired OIDC issuer URL, but the verification check in Ouath2 Proxy aborts due to a mismatch. Same happens if a trailing slash is omitted.

@chkohner
Copy link
Contributor Author

chkohner commented Apr 9, 2020

The "multi-tenant" endpoint, i.e. the ones that ends with either /organizations/v2.0 or /common/v2.0 returns a "placeholder" for the tenant as described (i.e. the literal text {tenantId}, not a substituted form). I haven't actually checked if that violates OIDC spec, but according to this https://docs.microsoft.com/en-us/azure/active-directory/develop/howto-convert-app-to-be-multi-tenant they seem to think that it's fine: "/common is a multiplexor, not an issuer"

@JoelSpeed
Copy link
Member

@chkohner Thanks for linking there, I can see we will need this if we want to support multi tenant, I need to re-review before we proceed any further though, I'll try review this by Monday, if I haven't, poke me

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will need an entry in the changelog before it can be merged though, thanks @chkohner

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chkohner, LGTM

Copy link
Member

@steakunderscore steakunderscore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@steakunderscore steakunderscore merged commit c6294c4 into oauth2-proxy:master Apr 19, 2020
@chkohner chkohner deleted the feature/skip_oidc_issuer branch May 4, 2020 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants