Allow Query Params in Issuer URL

[maboehm]

Hi,

there was already a lot of discussion around some Azure specific stuff, so here is another:

When using Azure Active Directory B2C the configuration can be found under: https://fabrikamb2c.b2clogin.com/775527ff-9a37-4307-8b3d-cc311f58d925/b2c_1_sign_in/v2.0/.well-known/openid-configuration

However, this is non-complaint, since the issuer does not match.

But, you can also access a similar configuration using query params: https://fabrikamb2c.b2clogin.com/775527ff-9a37-4307-8b3d-cc311f58d925/v2.0/.well-known/openid-configuration?p=b2c_1_sign_in

Which has a correct issuer.

However, this currently is not supported, because of the way the URL is formed here: https://github.com/coreos/go-oidc/blob/v2/oidc.go#L114

There already was a pull request addressing this issue, but it was closed, since a proper motivation/discussion was missing (#217).

I hope we can have this discussion here.

[ericchiang]

Thanks for opening a bug! I don't think the spec allows this. Issuer URLs aren't allowed to have query parameters:

REQUIRED. Issuer Identifier for the Issuer of the response. The iss value is a case sensitive URL using the https scheme that contains scheme, host, and optionally, port number and path components and no query or fragment components.

https://openid.net/specs/openid-connect-core-1_0.html#IDToken

And building the discovery URL is pretty clear cut. It MUST be the issuer concatenated with the well-known string:

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.

https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig

Can you open an issue against Azure Active Directory B2C? We've had a ton of issues with them (#212) and I'm conflicted. It's really important that issuers are the same opaque string, particularly if other parts of the app are verifying it. Maybe we could add a new API for when these are different?

type Discovery struct {
    IssuerURL    string
    DiscoveryURL string
}
func (d *Discovery) NewProvider(ctx context.Context) (*oidc.Provider, error)

That way the user still has to provide the correct value for the issuer instead of taking whatever discovery returns.

[ericchiang]

Though, just to note, you can already get around discovery by providing the jwks_uri. For your example, that would be:

issuerURL := "https://fabrikamb2c.b2clogin.com/775527ff-9a37-4307-8b3d-cc311f58d925/v2.0/"
jwksURI := "https://fabrikamb2c.b2clogin.com/775527ff-9a37-4307-8b3d-cc311f58d925/b2c_1_sign_in/discovery/v2.0/keys"
clientID := "foo"

keySet := oidc.NewRemoteKeySet(ctx, jwksURI)
v := oidc. NewVerifier(issuerURL, keySet, &oidc.Config{ClientID: clientID})

[ericchiang]

Closing this per the spec. If you need me to chime in on a Azure thread or bug, please link it here and I'm happy to help argue for a fix on their end.

[johnmaguire]

That way the user still has to provide the correct value for the issuer instead of taking whatever discovery returns.

@ericchiang Apologies for the tag in this issue, but I found this issue when trying to research a question: Is it acceptable, as an RP which is configured by admins to allow organizational user auth, to ask for a discovery URI and learn the issuer from it? Or is it necessary to ask for an issuer and then use the discovery URI to fill in the rest of the details?

If it's necessary to know the issuer prior to learning it from the discovery URI, could you explain why? I can't find much on this in the OIDC specs or discussion online. Thanks!

[ericchiang]

@johnmaguire are you try to integrate with Azure as well? If so, would you mind looking at #344?

[johnmaguire]

@ericchiang Apologies, as I am not, and I know this ticket isn't the best place for the question, but I wasn't sure of a better one.

I'm implementing an RP using go-oidc, which accepts an Issuer and builds the Discovery URI from it. One side effect is that trailing slashes can be an issue when validating the issuer during auth. Our team had the idea of accepting a discovery URI instead - which can be a little more forgiving of a trailing slash and has clear semantic rules on it - and then use the issuer inside to determine whether a trailing slash is necessary or not.

This of course would also require verifying that the issuer still matches the discovery URL minus /?.well-known/openid-configuration/?$ to avoid impersonation attacks.

Based on your comment above and wording in the OpenID specs, this sounds like it's not the expected way to determine the issuer, but I was unclear if there was security reasoning behind it. In our research, some RPs seem to operate in this fashion.

[ericchiang]

@johnmaguire you can take a look at #238, which sounds more relevant to what you're trying to do than this issue.

It's pretty simple to make a request yourself to the discovery document if you want to validate the issuer. The error message from this package also should be pretty explicitly (e.g. oidc: issuer did not match the issuer returned by provider, expected "https://foo.com/" got "https://foo.com").

I'm pretty against relaxing comparison for the trailing slash #238 (comment)