-
Notifications
You must be signed in to change notification settings - Fork 279
backend: oidc: impersonate instead of forwarding oidc token to k8s api #2814
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
base: main
Are you sure you want to change the base?
backend: oidc: impersonate instead of forwarding oidc token to k8s api #2814
Conversation
8661611
to
cdb40c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lg2m
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello. Thanks for this!
I left a few questions/notes.
Can you please add some steps in the PR description on how we can test this?
Would it be possible for you to add some test cases?
} | ||
|
||
idToken, err := c.getValidToken(r.Context(), token) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add some logging in the error conditions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging added, but I'm not really sure, whether it is really the best case to log always when the token validation fails. It may also be, that OIDC impersonation is active, but the user still used a serviceaccount token, in those cases a warning in the log would be irritating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. That sounds like it makes sense to remove.
Is something like this possible? Otherwise remove that logging?
if ! OIDCImpersonationEnaled and serviceAccount {
log()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wether the user provided token is a kubernetes serviceaccount token can only be evaluated from the Kubernetes API, therefore this check won't be possible. Only option where this would be possible would be, if we disable the option to login in the ui via a serviceaccount token altogether. In this case we could give a log warning here, but as far as I can see, that's currently not possible.
I'll cleanup the logging for the cases, where it may not be an error case.
func StartHeadlampServer(config *HeadlampConfig) { | ||
handler := createHeadlampHandler(config) | ||
|
||
handler = config.OIDCTokenRefreshMiddleware(handler) | ||
if config.oidcImpersonate { | ||
handler = config.OIDCImpersonateMiddleware(handler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain why this middleware is after the OIDCTokenRefreshMiddleware?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was the most fitting position I thought of, but not a particular reason. Do you have better recommendations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering how it would interact with the other middleware? Wondering if it would make sense to go before OIDCTokenRefreshMiddleware or not?
Because would the token refresh need to use the impersonated stuff? If that's the case I was wondering if the OIDCImpersonateMiddleware should go first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, the OIDCTokenRefreshMiddleware needs the original token, therefore it must be evaluated before OIDCImpersonateMiddleware. I'll change the order of those two and add a comment.
@@ -166,6 +168,10 @@ func flagset() *flag.FlagSet { | |||
f.String("oidc-idp-issuer-url", "", "Identity provider issuer URL for OIDC") | |||
f.String("oidc-scopes", "profile,email", | |||
"A comma separated list of scopes needed from the OIDC provider") | |||
f.Bool("oidc-impersonate", false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@knrt10 would this need to be added to the helm chart as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to the helm chart, however I'm not convinced that I did it in a good way, since the chart is a little bit confusing for me. So please recheck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our main helm chart expert is on holidays at the moment. So this will take a bit longer to review. Bear with me :)
Some lint issues in CI job: https://github.com/headlamp-k8s/headlamp/actions/runs/13056969725/job/36711540140?pr=2814#step:7:27 |
Signed-off-by: Stefan May <[email protected]>
cdb40c2
to
a1bbd62
Compare
Have updated some code regarding the comments, but not fully tested yet and not added a test description. Will work on that tomorrow, but perhaps you can already review the answers to the comments. |
Hi, not an expert here. I am wondering if this implementation will support impersonation based on groups listed in the |
Thanks. Reading through it now :) |
Signed-off-by: Stefan May <[email protected]>
@@ -147,11 +177,21 @@ spec: | |||
# Check if scopes are non empty either from env or oidc.config | |||
- "-oidc-scopes=$(OIDC_SCOPES)" | |||
{{- end }} | |||
{{- if or ($oidc.impersonate) ($impersonate) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey great PR!!
I'm wondering would it be too heavy if you can provide the groups as well here within the chart and deployment?
Convention here is that groups claim in JWT hold the user group coming in from IDP. It would be good if it was configurable as some IDPs send the groups membership under other claim name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added functionality to use groups too and changed the parameters therefore to -oidc-user-claim and -oidc-groups-claim. Also added a filter for additional impersonate-* headers to prevent any authentication attacks, since the service account token is now used.
It looks like the backend/lint CI job is failing. You can run |
The helm template job is having an issue: https://github.com/headlamp-k8s/headlamp/actions/runs/13197046279/job/36885634312?pr=2814
To test the charts locally
|
Signed-off-by: Stefan May <[email protected]>
This is not a good idea. Dashboards should never be configured with privileged token. it's too easy to expose a dashbaord by accident. Even when impersonating the user, headlamp should rely on the token from the proxy, not its own. This way someone accidentally exposes the dashbaord outside of the proxy, they haven't created a vector to access the API server with a privleged account. |
It is a good idea to try and help people prevent making mistakes with configuration accidentally opening things up. Also, when hearing about other peoples use cases it is often the situation that they are best placed to evaluate and decide why they want to do things in a particular way. @iSE-stefan-may maybe you could give us some ideas about this? Could you elaborate on your use case a bit? Also, what do you think about this possibility of opening things up accidentally and how we could address this? |
So far we have avoided requiring or encouraging running Headlamp with its own token/role. We did have some users asking for this and we resorted to advising to use a reverse proxy. We even have thought of maybe having a side project to facilitate this for Headlamp; would this bring any advantage here? @mlbiam I'd like to ask your opinion whether this is still a security concern if we add the feature as optional and somehow make it clear, via CLI options' names, extra options, and documentation what the implications of this are. What would that look like? |
Just because users want it doesn't mean its a good idea. Users in general tend to rely on what they can easily control, and a port-forward to a dashboard that is privileged is something they control. Unfortunately, it's also something anyone else can control too! This was how Tesla was breached (https://www.trendmicro.com/vinfo/us/security/news/cybercrime-and-digital-threats/tesla-and-jenkins-servers-fall-victim-to-cryptominers) and has led to the assumption by security teams that the Kubernetes dashboard is "insecure". It's also the reason why the Kubernetes Dashboard moved away from being able to use its own ServiceAccount token and requiring a reverse proxy to inject a token in the 7.x version. Headlamp already has a "local" mode. If users don't want to do it "correctly" in a centralized way, they should use the local version.
easier with good security is always better. That said, i don't know you need a new project. OpenUnison does this today. You can also use oauth2proxy+kube-oidc-proxy too if you don't want to use openunison: https://www.tremolo.io/post/kubernetes-authentication-comparing-solutions That said, you're essentially talking about rolling your own authentication code, which i'd recommend against. Best to invest time in good docs, better helm charts, etc, for existing projects to make this easier. The hard part isn't the proxies, it's setting up DNS, TLS, integration with a authentication team that is in a different silo in your org, etc. Creating another project won't fix those issues, just create one more thing to support.
I'd still say this is an anti-pattern. All the red flags and warnings won't stop users from doing the easy thing. ie: clicking through a security warning for an invalid certificate. It will only take one major breach for Headlamp to earn an "insecure" badge. The Kubernetes Dashbaord moved awat from this pattern for 7.x and has received considerable pushback. Their answer (which I think is correct), is setup a reverse proxy that hard codes an SA in it. It's terribly insecure, but so's running the dashboard with a privileged sa. It at least makes it harder to do the insecure thing and protects the project's reputation. |
Hi @iSE-stefan-may will you close this one given the new circumstances? Or leave this setting optional? |
} | ||
r.Header.Set("Impersonate-User", user) | ||
for _, group := range groups { | ||
r.Header.Set("Impersonate-Group", group) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should use Add instead of Set.
r.Header.Set("Impersonate-Group", group) | |
r.Header.Add("Impersonate-Group", group) |
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
|
The security arguments above are valid. I will offer that I think it's quite simple to restrict impersonation side-effects. The service account mounted can be limited to which groups it's allowed to impersonate with simple Roles/ClusterRoles. If something like this isn't supported, it leaves users in a catch 22. None of the documentation or issues want to describe how to utilize this on managed platforms like GKE of which they are strongly moving away basic OIDC in favor of google-native solutions (ref). Impersonation offers a nice middle ground in these scenarios. To offer some motivation, this is how the weaveworks UI works for fluxcd. https://docs.gitops.weaveworks.org/docs/0.24.0/configuration/recommended-rbac-configuration/#summary. By tightly controlling the groups that can be impersonated, I can very confidently restrict what my app's service account is capable of doing. |
What if we do something similar to https://github.com/aslafy-z/k8s-dashboard-impersonation-proxy and provide a way to easily use this with Headlamp's Helm chart? |
To @engnatha 's point about using RBAC to limit impersonation - There are two threats:
Using the dashbaord's service account as privileged was pretty big security issue with the kubernetes dashboard and they removed that capability correctly (hell, i wrote an entire chapter of the Kube: an enterprise guide around how to attack the dashboard running a privileged account until they removed the capability in 7.x) While I think the lessons learned from the kubernetes dashboard project advise against making the headlamp service account privileged, if you really want to go this route it shouldn't be the default. The headlamp account shouldn't be used as part of impersonation, but if you want it to be there should be a switch that has to be turned on. @joaquimrocha including a reverse proxy in the helm charts to do this would probably be not as straight forward as you'd hope. This biggest issues we run into helping customers and users run OpenUnison is certificate management and ingress integration. Assuming you're encrypting e2e (and given these tokens are privileged, i certainly would hope that would be the requirement) you're talking about either requiring cert-manager or building your own cert-manager like system. Then there's testing with each ingress controller/gateway. You're going to get much better results ivesting efforts into documenting how to deploy headlamp with impersonation then trying to engineer the issue. Also, who "owns" that proxy you're including? Who's responsible for continuous testing? Not just happy-path testing, but failure mode testing to protect against mallicious use? If that component fails, they're going to blame the headlamp project the same way that the kubernetes dashboard still has the assumption of being "insecure" post tesla breach. |
I think impersonation works better for the weaveworks UI because the operations exposed are so much more limited. You're really only interested in viewing resources and patching existing resources of a certain kind. While you could do some harm with that, I think the blast radius minimized. In the headlamp system, there would be a much richer ecosystem of permissions to give more access to the cluster for admins of different components. I agree that it will never not be a threat. I'm mostly just trying to advocate as a potential user that the current system is forcing users away from the product. With no guides on how to set up the system on managed clusters (which is becoming the norm rather than the exception) and no willingness to provide alternate mechanisms like proxies, impersonation, or otherwise, we aren't left with many options. I've tried hard to search through existing issues and documentation on how to do this with modern systems and it just isn't there. Even looking at the platforms page, the big cloud provider mentioned for support is AWS and the rationale is someone in 2021 saying it worked with no mention of how. I do really respect the push for security here and if this PR does get blocked, I hope it provides motivation to teach users how to utilize this system properly. The alternative is everyone reinventing the wheel and likely introducing their own vulnerabilities with proxies and impersonations of their own or more widely sharing long lived service account tokens. |
Looking into this more, I actually think the current authentication strategy imposed by headlamp is impossible to accommodate in modern GKE. Based on the guidance in https://cloud.google.com/kubernetes-engine/docs/how-to/oidc#external-idp-authentication-methods, we see the following
They are migrating everyone to workforce identity federation which would require headlamp to route through that system to get the proper short-lived google service account tokens. I definitely don't know as much about this system, so I'd really like to be wrong about this if anyone has more experience 😁 |
By default, the OIDC JWT is forwarded to the Kubernetes API server. In some cases it might not be possible to have the Kubernetes API server configured with OIDC, in these cases the OIDC JWT should be evaluated by Headlamp and the request should use the service account of the pod together with impersonating the user in the token.
Related issue:
Testing
To test this PR, do the following: