[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.

[amartinezfayo]

@kfox1111 how close do you think we are here considering the discussion in #5719?
In terms of naming, maybe server_path_prefix is better than just prefix?
Also, I would probably be inclined to jwks_uri vs advertised_url.
Are we missing something else to address the known issues?

[kfox1111]

@kfox1111 how close do you think we are here considering the discussion in #5719? In terms of naming, maybe server_path_prefix is better than just prefix? Also, I would probably be inclined to jwks_uri vs advertised_url. Are we missing something else to address the known issues?

Just determining what the maintainers preferences are on naming (namings always hard). If you prefer server_path_prefix and jwks_uri, I'm good with that. I'll fix up the patch asap with that, and then I think we're good to go.

[sorindumitru]

Could we follow the more verbose error checking done for JWTIssuer? It gives a bit more detail about the way the uri is invalid..

[sorindumitru]

Would it be possible to simplify this a bit. I'm thinking something like:

var issuerURL url.URL
if h.jwtIssuer != "" {
  ...
  issuerURL = ...
} else {
   ...
  issuerURL = ...
}

var jwksURL url.URL
if h.jwksURL != "" {
  ...
  jwksURL = ...
} else {
   ...
  jwksURL = ...
}

Minimizes dependencies between the parts of the two URLs and makes it a bit easier to read, at least in my opinion.

[sorindumitru]

We could store these 2 at start up as *url.URL to avoid parsing at runtime.

[sorindumitru]

Maybe StatusInternalServerError would be better suited for such errors?