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

Issuer Name Mismatch #250

Closed
threez opened this issue Jun 9, 2020 · 4 comments · Fixed by #315
Closed

Issuer Name Mismatch #250

threez opened this issue Jun 9, 2020 · 4 comments · Fixed by #315

Comments

@threez
Copy link

threez commented Jun 9, 2020

Hello,

I understand this request was previously declined already: #240

But sometimes there are technical or domain reasons to allow a different issuer than the one returned by the server. The standard is not covering this scenario. If you look at the forks of this library, just pick the recent 20 forks, most of them are only doing the change to allow a different issuer. By not allowing this option the eco-system will diverge, we are basically requested to fork the library and go in a different direction.

I hope you can understand the need we have to do this change. Please consider adding a separate option to NewProvider to provide the expected issuer. That should be at least as secure or safe as the current implementation.

Sincerely threez

@ericchiang
Copy link
Collaborator

Hey @threez

Thanks for opening this. The main reasons I've been hesitant to do this is that choosing the right issuer is security critical (or another domain might be able to pass your validation), and because this often signifies misconfigurations. I'm afraid providing options to skip validation like this will lead to users, who could care less about OpenID Connect, just getting the damn thing working and end up in an insecure state.

There's been discussions of a safe way to skip this validation (e.g. #233), but this package can already work with providers that don't support discovery (#161). For Google for example, instead of this:

p, err := oidc.NewProvider(ctx, "https://accounts.google.com")
if err != nil {
    // ...
}
v := p.Verifier(config)

You can provide the jwks_uri directly from https://accounts.google.com/.well-known/openid-configuration

keySet := oidc.NewRemoteKeySet(ctx, "https://www.googleapis.com/oauth2/v3/certs")
v := oidc.NewVerifier("https://accounts.google.com", keySet, config) 

And validate ID tokens without doing discovery.

So if there's already a way to skip the mismatch, I'm inclined to document that better instead of adding multiple ways to achieve the same goal.

@threez
Copy link
Author

threez commented Jul 1, 2020

Hey, thx for the reply. We don't want to skip the check nor the discovery ;-). We wanted a way to add additional valid issuers. We started our work on that here. It is written in an API compatible way with your lib. pace@8b5101f

@cortex
Copy link

cortex commented Nov 16, 2020

I also ran in to this issue, Signicat, a major provider of eID services in EU has non-compliant OIDC Discovery configuration at https://login.signicat.io/.well-known/openid-configuration . I have contacted Signicat's support to verify if this is done on purpose for some reason like @threez wrote above.

In my case, I also can't work around it easily using the above code, because I need the UserInfo() functionality.

@ayushgoel
Copy link

Hi,

Is there a way to avoid this check for non-compliant OIDC providers like Azure? We are building a tool to support login via Azure.

The discovery document for organizations tenant on Azure - https://login.microsoftonline.com/organizations/v2.0/.well-known/openid-configuration contains issuer as https://login.microsoftonline.com/{tenantid}/v2.0.

One possible solution might be to make fields of Provider public so that we can create a lenient version of NewProvider. Any other suggestions are also welcome. :)

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 a pull request may close this issue.

4 participants