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

The URL to discover and Issue need not be same #159

Closed
mkhadilk opened this issue Oct 4, 2017 · 7 comments
Closed

The URL to discover and Issue need not be same #159

mkhadilk opened this issue Oct 4, 2017 · 7 comments

Comments

@mkhadilk
Copy link

mkhadilk commented Oct 4, 2017

In Package oidc and compilation unit oidc.go, "NewProvider (" call implementation is looks wrong.

if p.Issuer != issuer {
return nil, fmt.Errorf("oidc: issuer did not match the issuer returned by provider, expected %q got %q", issuer, p.Issuer)
}

This code looks wrong to me. I am reading the OIDC discovery code and I dont see why the URL one provides should be the URL of the Issuer.

@jmcarp
Copy link

jmcarp commented Oct 6, 2017

I'm running into the same issue. Would you be open to dropping that check or reviewing a patch that drops it @ericchiang ?

jmcarp added a commit to jmcarp/go-oidc that referenced this issue Oct 6, 2017
@ericchiang
Copy link
Collaborator

Sorry for the delayed response, have been out of the office.

Copying from a previous thread #154 (comment)

... the spec section you're looking for is https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig

OpenID Providers supporting Discovery MUST make a JSON document available at the path formed by concatenating the string /.well-known/openid-configuration to the Issuer.

We've seen this check regularly catch problems when users are referring to an provider through a DNS name alternative to the provider value. e.g. #121

From my reading of the spec these values MUST match.

@jmcarp
Copy link

jmcarp commented Oct 7, 2017

Thanks @ericchiang! I think the relevant part of the spec is

The issuer value returned MUST be identical to the Issuer URL that was directly used to retrieve the configuration information. This MUST also be identical to the iss Claim value in ID Tokens issued from this Issuer.

Looks like there's a bug in the provider we're using--we'll resolve upstream.

@chaliy
Copy link

chaliy commented Jan 20, 2018

In my case, we are building infrastructure for the project and it makes it hard to implement public and private DNS resolutions to be the same. So basically we need a way of having correct issuer and at the same time openid-configuration should be resolvable in private. There are few ways to do this:

  1. Tweak private DNS to also resolve public one. This is good approach. In our case it will be hard to implement because of multitenancy.
  2. Make oidc client so that it could have other way to get oidc configuration. For example allow custom URL to fetch /.well-known/openid-configuration

Would you consider to reopen this issue and accept PR with optional way to override wellknow URL?

ASP.NET OIDC has option to override it - https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.openidconnectoptions.metadataaddress?view=aspnetcore-1.1#Microsoft_AspNetCore_Builder_OpenIdConnectOptions_MetadataAddress
NodeJS OIDC same - https://github.com/IdentityModel/oidc-client-js/blob/dev/src/OidcClientSettings.js#L19

@ericchiang
Copy link
Collaborator

@chaliy #161 lets you construct a verifier without using NewProvider. See:

https://godoc.org/github.com/coreos/go-oidc#NewRemoteKeySet
https://godoc.org/github.com/coreos/go-oidc#NewVerifier

This should allow you to do discovery yourself, then construct a verifier using the remote key set with whatever issuer you want.

I think I prefer this over an option to NewProvider. We intentionally want you to know what you're doing if you go around that check. It regularly catches bugs and misconfigurations.

@robinbryce
Copy link

Microsoft active directory multi-tenancy support requires an initial configuration url of https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration

And responds with "common" end points and an issuer of

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

It's up to the application whether or not it cares to assert that the issuer in id tokens matches the template. In our case, it is a different part of the system which knows the valid issuers. A flag option to relax the check doesn't seem like an unreasonable ask and, while it solves this provider specific use case, doesn't require anything provider specific to do so.

@ericchiang
Copy link
Collaborator

@robinbryce see #204 (comment) for some discussion of the Azure case.

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

No branches or pull requests

5 participants