[oidc-discovery-provider] Fix keys url

[kfox1111]

When jwt_issuer is specified, it is overriding the jwks key url in addition to the issuer property. This may cause the subsequent key retrieval to hit the wrong server, or fail if that server doesn't actually exist.

[rturner3]

Good catch, @kfox1111. This looks like an appropriate fix to me.

[jer8me]

I think that @kfox1111 nailed it in this comment: #5657 (comment)
The only thing that should have changed in #5657 is the logic around the issuer, not the JWKS endpoint.

The fix proposed in this PR would solve some of the issues but jwksURI could still end up with the wrong scheme (if jwt_issuer is specified but the request comes on with an http scheme for instance).

Maybe we could revert the code in handler.go to version 1.11.0 and only change the way issuerURL is created. Something like this:

var issuerURL *url.URL
if h.jwtIssuer != "" {
    issuerURL, _ = url.Parse(h.jwtIssuer)
} else {
    issuerURL = &url.URL{
        Scheme: urlScheme,
        Host:   r.Host,
    }
}

This should take care of the custom issuer case and maintain the old behavior for the JWKS URI.

[kfox1111]

I think that @kfox1111 nailed it in this comment: #5657 (comment) The only thing that should have changed in #5657 is the logic around the issuer, not the JWKS endpoint.

The fix proposed in this PR would solve some of the issues but jwksURI could still end up with the wrong scheme (if jwt_issuer is specified but the request comes on with an http scheme for instance).

Maybe we could revert the code in handler.go to version 1.11.0 and only change the way issuerURL is created. Something like this:

var issuerURL *url.URL
if h.jwtIssuer != "" {
    issuerURL, _ = url.Parse(h.jwtIssuer)
} else {
    issuerURL = &url.URL{
        Scheme: urlScheme,
        Host:   r.Host,
    }
}

This should take care of the custom issuer case and maintain the old behavior for the JWKS URI.

Hmm.... I think this is another reason the advertised_url needs its own option (filed: #5719 with details)

In some cases you may actually want it http, and in others, you may always want it to be https (when lb fronted https -> http)... So that too needs to be configurable.

In the non specified case though, I think I fixed the issue. Thanks for bringing it up. Please have another look to see if I addressed it properly.

[sorindumitru]

LGTM, with a small suggestion

[sorindumitru]

This part is common to the two so we could move it outside of the two ifs (for issuer and jwks urls).

[jer8me]

Is this meant to sanitize the URL? Wondering if there is a reason not to set issuerURL directly to h.jwtIssuer.

[kfox1111]

No. Just evolutionary cruft I think... it was patched and moved around so much it just ended up this way. I'll collapse it.

[jer8me]

Should we include serverPathPrefix in the Issuer here?
Per the specs:

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.

So technically, if the well-known document includes a path component, the issuer should too.

[kfox1111]

Depends on if there are any proxies doing rewrites in front...

But, yeah. I think your probably right, as the default, if the serverPathPrefix is set, the default may benefit from having it set too. The user can always override issuer if they are using a proxy in front.

[rturner3]

@kfox1111 I took a closer look at this PR, and I think this will introduce a breaking change from 1.11.1. If a user of 1.11.1 sets the jwt_issuer configuration field and upgrades to 1.11.2 with the changes from this PR, the JWKS URL will change if they don't set the new jwks_uri config. In 1.11.1 the JWKS URL would be $ISSUER_URL/keys, but in 1.11.2, it would be $SERVER_URL/keys. I don't think we can make that breaking change until 1.13.0 based on our backwards compatibility guarantees.

Could we preserve the existing behavior of the keys URL being derived from the issuer URI if jwks_uri isn't set? If jwks_uri is set, we can still override the JWKS URI without breaking back-compat.

[kfox1111]

The jwt_issuer setting in 1.11.1 incorrectly assumed you would want to change the behavior of JWKS URL as well. If we left it with that behavior when unset, there is no way to set jwt_issuer without having it not affect the JWKS URL too. IE, we're continuing to promote the buggy, assumed behavior. For my needs, I need to set the issuer, and have the pre 1.11.1 behavior for the JWKS URL. How do we support that in the 1.11/1.12 timeframe?

There are tradeoffs to be made here for sure. Nothing is ideal. :/ We could have fixed it before we released it, but that was forced through, so now we have a mess to deal with. The question is, which folks do we most inconvenience?

The jwt_issuer feature has only been in place, for a few weeks, and was marked problematic at the time. The number of people any behavioral changes around this would be pretty low and manageable I think. So, I think that is probably the best approach. Fix the implementation as buggy, and put in a release note about it. The blast damage is pretty small.

If we want to insist we keep backwards compatibility for this, we should add another bool in this release, saying something like, preserve bug compatibility for when jwks url is unset, defaulting to true. I can then set it to false, and not set jwks url and it will work. then in the future, default it to false and deprecate it, and then in some version remove it.

Do we really want to go down that path though? Its a lot of effort for a pretty small thing.

[rturner3]

For my needs, I need to set the issuer, and have the pre 1.11.1 behavior for the JWKS URL. How do we support that in the 1.11/1.12 timeframe?

Would this config shape work for your use case by controlling each of the issuer URL and JWKS URL yourself?

jwt_issuer = "https://example.org/my/issuer/path"
jwks_uri = "https://example.org/keys"

Then if you had this config from 1.11.1:

jwt_issuer = "https://example.org/my/issuer/path"

the JWKS URI would still be https://example.org/my/issuer/path/keys (same as 1.11.1).

[jer8me]

@kfox1111 I think that the 1.11.1 behavior is a "sensible" default.
If a user only specifies jwt_issuer and not jwks_uri, it is reasonable to set jwks_uri to $ISSUER_URL/keys.
I understand that this will not work for all use cases but I would not call that buggy either.

[kfox1111]

Having one config option leak into another when unset is unexpected behavior IMO. Not intuitive to a user. But, I'm not going to argue about it. I've updated the default behavior to follow the suggested change, and we can deal with it again, in a future pr so we can unblock release.

[kfox1111]

Updated and passing the tests. Please rereview.

[amartinezfayo]

Thank you @kfox1111 for all the work on this.
I've noticed that the PR does not include an update of the oidc-discovery-provider README.md file. Could you please update it to include the new config settings that are being introduced?