Skip to content
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

Open
kfox1111 opened this issue Dec 15, 2024 · 2 comments
Open

[spiffe-oidc-discovery-provider] Issuer issues #5719

kfox1111 opened this issue Dec 15, 2024 · 2 comments
Labels
triage/in-progress Issue triage is in progress

Comments

@kfox1111
Copy link
Contributor

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:

  1. The discovery provider is behind a reverse proxy / load balancer. It cant properly detect what the issuer string should be automatically.
  2. The discovery provider is being contacted directly, setup on an alternate dns name, and needs spire-server issued jwt's issuer property to match the returned issuer from the discovery provider.

For use case 1, there are multiple features that could be used to implement it:

  1. The issuer returned can't be guessed by the discovery provider based on the domain / path returned by the reverse proxy / load balancer. It needs to be configurable.
  2. The discovery provider returns an absolute url to the /keys endpoint. It needs to be configurable to route the traffic back to the right endpoint
  3. Some reverse proxy / load balancers do not support rewriting the url before passing it to the discovery provider. The ability to specify a prefix to serve off of would allow the discovery provider to be used with more software successfully.

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:

  1. The ability to set the returned issuer explicitly
  2. The keys endpoint needs to be reachable the same way that the discovery document is. No rewriting should be done.

An example of 2 is:

  • configure the discovery provider:
    • override the jwt issuer to be the same as that spire is set to. ex: "oidc-discovery-provider.example.org"
    • listen on an alternate port. ex: 8181
    • configure /etc/hosts entry for 127.0.0.1 -> k8ssodp.example.org
    • configure the allowed domain names to be "k8ssodp.example.org"
  • configure the kubernetes apiserver

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

  1. Change the new jwt_issuer flag override just the issuer, no other behavior.
  2. Add an advertised_url option that overrides the url returned in the discovery document for keys (advertised_url + "/keys")
  3. Add a prefix option that defaults to / that configures where the url routes will listen on.
  4. Revert the domain checking changes. It was intended to make things easier, but hasn't had that affect.
@jer8me
Copy link

jer8me commented Jan 2, 2025

@kfox1111, thanks for the proposal.

  1. Instead of configuring advertised_url what do you think about being able to configure the JWKS URI instead (advertised_urljwks_uri)?
    This would allow a user to configure any kind of proxied URL, like https://jwks.external.org/somepath/jwks.json.

  2. What is the reason to add the prefix option? I understand that the user could add a path prefix, but what use case does this solve?
    Just wondering in what case there is a need to add this prefix.

@kfox1111
Copy link
Contributor Author

kfox1111 commented Jan 2, 2025

Hi @jer8me,

  1. jwks_uri would work too... I'd be ok with that. But, if we ever added any other urls to the service, they would need to be independently configured, while advertised_url would be used as the base for them all. Thoughts?

  2. The initial use case (discussed somewhat here Make the JWT issuer configurable in the OIDC Discovery Provider #5480 and exposed more through the unit tests implemented) made it clear they wanted to be able to hang multiple independent spire instances off of the same domain name with different paths.

So, say, server-a and server-b, they may want to do something like:
https://example.org/oidc/server-a -> discovery-provider-a
https://example.org/oidc/server-b -> discovery-provider-b

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...)
last time I looked at ingress support in azure, it didnt support it at all (was like 2 years ago. may have changed)
aws still doesn't seem to support it: kubernetes-sigs/aws-load-balancer-controller#835. The workaround is to stack a whole nother webserver/router in the middle. :/

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/in-progress Issue triage is in progress
Projects
None yet
Development

No branches or pull requests

3 participants