coreos / go-oidc

A Go OpenID Connect client.
Apache License 2.0
1.97k stars 395 forks source link

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

Closed mkhadilk closed 7 years ago

mkhadilk commented 7 years ago

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

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

ericchiang commented 7 years ago

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

Copying from a previous thread https://github.com/coreos/go-oidc/issues/154#issuecomment-318493898

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

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

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

@chaliy https://github.com/coreos/go-oidc/pull/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 commented 5 years ago

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

@robinbryce see https://github.com/coreos/go-oidc/issues/204#issuecomment-510987282 for some discussion of the Azure case.