-
Notifications
You must be signed in to change notification settings - Fork 486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[spiffe-oidc-discovery-provider] Issuer issues #5719
Comments
@kfox1111, thanks for the proposal.
|
Hi @jer8me,
So, say, server-a and server-b, they may want to do something like: as a user then, you hit: https://example.org/oidc/server-a/.well-known/openid-configuration The routing by default in most proxies would then to just grab the path from the request and send that along, like /oidc/server-a/.well-known/openid-configuration to the backend provider. but that doesn't normally exist in the discovery provider. only /.well-known/openid-configuration today. So, some http routers support stripping off a prefix off of the url. so, strip /oidc/server-a off the request before it goes through to the other side. That works when the http router supports it. Unfortunately, some common ones don't support rewriting, just matching. :( k8s ingress doesn't have a native way of doing it. (some implementations like ingress-nginx support it via annotations...) So, having the user be able to give the prefix to listen on to the discovery provider, lets it be used by all the http routers, not just the advanced ones that support url rewriting. The code is pretty simple around it. So, the juice is worth the squeeze IMHO. |
|
For 1, I think it would be ideal for the s3 bundle publisher to push both the well known config and jwks url to the s3 bucket directly, and skip the oidc discovery provider all together? Why have another service when s3 would be the service? Is there any other reason to have the jwks_uri configurable besides that? (Just curious) 2, sounds good to me. Naming things is hard. |
Maybe the S3 bucket wasn't the best example. My point was that we should be able to publish While |
Right. But I guess the thing I'm questioning is, I think the purpose of jwks_uri or advertised_url or whatever we call it, is to help route requests back to one (or more) document(s) hosted by the oidc-discovery provider? IE, we're not going to have user retrieve the discovery document from the oidc-discovery-provider, but get the jwks document from a service other then the oidc-discovery-provider? If that holds true, then having the configuration be based on how to get back to the root of the oidc-discovery-provider makes sense to me. The documents are complex to generate and so, I'm thinking either all the stuff in the oidc-discovery-provider is hosted out of the oidc-discovery-provider, or, it all is hosted by some other service, but I'm not sure the use case for splitting it. Maybe there is a use case I'm missing? |
It's not so much about how the oidc-discovery provider is hosted, it's more about how it is exposed/published. Per OIDC standards, the well-know documents MUST be served at:
Internally, each tenant has its own oidc-discovery provider service:
This is where I would want to use |
Supporting one server instance for both https://example.org/issuer1 and https://issuer1.internal would mean that you would need a prefix, per hostname coming in. Thats harder to do... :/ For my use of that, I was expecting to run multiple, differently configured oidc-discovery-providers... Is that too much? |
Yeah so I am not suggesting that the OIDC provider should return a different /keys endpoints based on the external or internal host. The well-known document should always return the same In the example above, I would want to return the external URL. So I would set: But for internal use, I still want to be able to hit the /keys endpoint directly via https://issuer1.internal/keys |
ok, in that case, we could do it with two prefixes. one implicit one always being / ? or is there a reason not to allow https://issuer1.internal/issuer1/keys when its https://issuer.external/issuer1/keys in addition to https://issuer1.internal/keys? Though I'm still fuzzy on, how https://issuer1.internal/keys is useful when https://issuer1.internal/.well-known/openid-configuration doesn't point at it? |
In my case, I don't need to use a prefix with the internal host. I can point the external host to any internal URL. To be clear, I'm fine with the original idea of having As far as using the internal /keys endpoint directly (without getting it from the well-known endpoint), certain JWT verification implementations do not care about the well-known document. Envoy for instance just needs to know the JWKS URI: https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/jwt_authn_filter.html#remote-jwks-config-example |
Problem
There are multiple use cases for being able to override the issuer string that the discovery provider should return.
In #5657, the issuer was made configurable, but it also uses the same config option to perform multiple other behavioral changes that make it unusable for one of those use cases.
So, I think we need to step back and reconsider the design of these features.
There seems to be two primary use cases:
For use case 1, there are multiple features that could be used to implement it:
There is, arguably, an anti feature implemented in pr 5657 that rewrites the incoming url to look like it shows up as the issuer. This prevents the domain checking from working in all cases. I think it should probably be reverted outright.
For use case 2, we need:
An example of 2 is:
This allows the main issuer to be "oidc-discovery-provider.example.org" on port 443, but have local instances on k8s control planes for high availability / bootstrapping purposes.
Proposed Changes
jwt_issuer
flag override just the issuer, no other behavior.advertised_url
option that overrides the url returned in the discovery document for keys (advertised_url + "/keys")prefix
option that defaults to/
that configures where the url routes will listen on.The text was updated successfully, but these errors were encountered: