Issuer URL is not normalized for comparison

[voutasaurus]

The NewProvider function takes an issuer argument and creates the config URL from it with

wellKnown := strings.TrimSuffix(issuer, "/") + "/.well-known/openid-configuration"

Then later the issuer is checked against that returned by the config endpoint.

if p.Issuer != issuer {

If an issuer of https://issuer.com/ is passed in but the config endpoint returns https://issuer.com as the issuer, then this comparison will fail even though these are clearly the same issuer.

I suggest normalizing the issuer url for the comparison (e.g. like this):

if strings.TrimSuffix(p.Issuer, "/") != strings.TrimSuffix(issuer, "/") {

Does this sound appropriate? If so I can make the change and PR it.

[ericchiang]

Does this sound appropriate?

Issuers are exact strings. https://issuer.com/ is unique from https://issuer.com and those two aren't equal. We do the TrimSuffix when getting the provide metadata because the spec mandates it:

If the Issuer value contains a path component, any terminating / MUST be removed before appending /.well-known/openid-configuration.

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

However, the issuer value returned by the metadata must exactly match the original issuer string.

This check is strict, but often helps us identify mis-configurations (#154, #121). Probably better for us to make the docstrings stress this more. But I don't think we should do any normalization during comparison.

[voutasaurus]

Both of those mis-configurations would be still be caught if the terminating / was removed for the comparison.

The normalization is especially annoying because some issuers have terminating /s and some don't. So I can't just strip them or append them myself.

As a result without the normalization in the check itself I will be calling the /.well-known/openid-configuration endpoint twice. Once to get the exact issuer string and then again in NewProvider. If I don't do this myself then my users will get errors if they normalize their URLs in a way that doesn't exactly match what the json config for their provider is returning.

[ericchiang]

It seems odd to do this normalization here, but not elsewhere. I could provide an issuer URL https://issuer.com/ then later validated an ID token when it has an issuer URL https://issuer.com. That breaks a lot of assumptions about the issuer URL being unique across a program.

It also seems odd to be using this package just to validate issuer URLs.

I could see someone wanting to create an *oidc.Provider without having this package do the discovery. e.g. for providers that don't implement discovery. In that case, your code would grab the metadata, then use that information to create a provider by providing something like the "jwks_uri" endpoint. Does something along that lines work?

[voutasaurus]

I'm trying to be defensive from a user input validation perspective. I think the best option is to do the full validation by calling NewProvider when the user tries to input the issue value and rejecting the value if NewProvider returns an error.

It's annoying mostly because I am passing the value off to another service and the values in this library don't serialize well due to all the fields being unexported. So I have to call NewProvider in that service too which sort of seems wrong to me.

[ericchiang]

It's annoying mostly because I am passing the value off to another service and the values in this library don't serialize well due to all the fields being unexported. So I have to call NewProvider in that service too which sort of seems wrong to me.

Okay this seems to be boiling down to "I want to use this package without doing the discovery call."

I could see a couple possible APIs.

First we could expose the internal fields and let a user create a verifier directly from the KeysURL.

// Provider ...
//
// Providers must be built using NewProvider.
type Provider struct {
    // Expose existing, unexported fields.
    Issuer      string
    AuthURL     string
    TokenURL    string
    UserInfoURL string
    KeysURL     string

    // contains filtered or unexported fields
}

// NewVerifier returns an IDTokenVerifier that uses the public keys provided at keysURL to
// verify ID token signatures.
//
// It's recommended to use Provider.Verifier to create verifiers instead, since those will share a cache
// of keys common to the provider, instead of per verifier. NewVerifier should be only be used when
// discovery has been performed out-of-band.
func NewVerifier(ctx context.Context, config *Config, issuerURL, keysURL string) *IDTokenVerifier

Or we could let a user create a provider directly through a keyed struct constructor.

type Provider struct {
    // Expose existing, unexported fields.
    Issuer      string
    AuthURL     string
    TokenURL    string
    UserInfoURL string
    KeysURL     string

    // Ctx for this provider. Can only be set before calling Verifier.
    Ctx context.Context

    // raw claims. Only populated by NewProvider.
    raw []byte

    // keySet, if nil, will be built using the KeysURL and Ctx fields on the first call to Verifier.
    once   sync.Once
    keySet *remoteKeySet
}

I think I prefer the first API change.

[voutasaurus]

I've decided I want to validate the user input early anyway. So I'll call NewProvider twice. Once to validate on input and then again in the other service where I want to actually use the provider.

No API change is necessary. Feel free to consider this a user experience report and close. Thanks for your time and your help, it is appreciated and has helped me come to a decent decision.