RequestHeader authentication: add UID to recognized request headers #115834
Conversation
k8s-ci-robot
added
release-note
stlaz
force-pushed
the
remote-uid
branch
3 times, most recently
from
1ce1154
to
aef1832
Compare
k8s-ci-robot
added
area/cloudprovider
sig/cloud-provider
|
/test verify |
|
@stlaz: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test pull-kubernetes-verify |
|
@stlaz I haven't looked at the diff yet but remember that you need to add an integration test that shows this works correctly with aggregated API servers. I would recommend using a service account to call an aggregated API and maybe intercept the authorization checks made by the aggregated API server to see what identity it saw. |
|
Yes, an integration test is currently missing. I suppose I could write some kind of an aggregated API server similar to how the admission webhooks are being tested: |
k8s-ci-robot
added
area/test
sig/testing
stlaz
force-pushed
the
remote-uid
branch
2 times, most recently
from
b7c77b3
to
10aeed2
Compare
k8s-ci-robot
added
size/XL
|
The front-proxy config test now lives separately from the other, rather big aggregated apiserver test. edit: I split the addition of the test and a minor cleanup of the file into two commits so while reviewing, check the first test commit first as that gets a much smaller diff. |
k8s-ci-robot
added
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/reopen |
|
@stlaz: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
removed
the
lifecycle/rotten
|
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Wed Apr 17 07:56:14 UTC 2024. |
k8s-ci-robot
added
the
needs-rebase
There was a problem hiding this comment.
Comments from today's review meeting:
request header authn
proxy --(RH)--> KAS
this proxy is telling KAS who the user is
EKS -> request signing
KAS --(RH)--> aggregated API server
the proxy is KAS, and it is telling the aggregated server what the user is
bug:
david forgot that UID is a thing
username
UID
groups
extra fields (metadata)
for example, authz knows about UID
proxy --(RH)--> server
mTLS
you can constrain to a single CN
It is unclear how to make a change like this safe on upgrade if we want to update the defaults.
There was a problem hiding this comment.
Do we have a test case for the upgrade scenario where the existing CM has no UID config, but the API server now does have said config, meaning it should update the config map.
There was a problem hiding this comment.
I added a unit test that verifies that the controller should be capable of this step.
Looking at the controller, we might still face trouble for the upgrade period where kube-apiserver would exist in different versions, it doesn't seem to be guarded by leader election. That might lead to update hotloops.
The same issue would occur today if somebody wanted to configure additional headers, although that's probably not very typical action, compared to cluster upgrades.
I suspect we may want to address this. Not sure if adding leader election to KAS controllers is an OK practice?
Maybe we just have to leave KAS with no default value here, but can update the defaults for anything "behind" KAS (i.e. aggregated API servers) because the only actor that can assert that mTLS connection is KAS, and we can rely on KAS deleting the UID headers from any incoming request before it forwards the request onwards. |
| fs.StringSliceVar(&s.UIDHeaders, "requestheader-uid-headers", s.UIDHeaders, ""+ | ||
| "List of request headers to inspect for UIDs. X-Remote-Uid is suggested.") |
There was a problem hiding this comment.
Per triage meeting review from today:
This flag needs to not be wired up in the first release that contains this feature as then it would be possible for old KAS to proxy a request to a new aggregated API server without dropping the UID header. Everything else is safe to do in this PR. To be able to test this code path in integration tests, we can use an approach similar to SetServiceResolverForTests to let us set the UIDHeaders field. A future PR in a later release would simply drop that global in place of the actual flag wiring.
I believe it is safe to update NewDelegatingAuthenticationOptions with the X-Remote-Uid as a default because no matter what that relies on the front proxy dropping headers that the client should not set. And that code path isn't really used because the vast majority of cases should be using the config map, which will only get updated once KAS is updated and the --requestheader-uid-headers CLI flag is set. @liggitt any thoughts on the upgrade story here?
Once we let people configure this flag on KAS, we should require that it contains at least X-Remote-Uid so that API aggregation will work correctly (we can be stricter here because the config is new).
There was a problem hiding this comment.
Just got here, need to think about the skew implications.
There was just a lengthy discussion about what skew between kube-apiserver and aggregated servers are allowed / recommended / supported (kubernetes/website#46109, #124533, #124655, api-machinery meeting https://www.youtube.com/watch?v=0TXm-DGcK1k&t=31m). We don't really dictate whether aggregated API servers' use of k8s.io/apiserver library can / should be newer than kube-apiserver today. cc @deads2k as well who might have thoughts.
I agree our defaults (in kube-apiserver and k8s.io/apiserver library) should not lead to aggregated API servers trusting headers before kube-apiserver sets / guards / protects them.
I think the following defaults work:
- kube-apiserver 1.31+ does not default the uid CLI flag (just like the other requestheader flags)
- kube-apiserver 1.31+ (as kube-aggregator) unconditionally clears the standard uid header when proxying (just like it clears the other standard headers)
- kube-apiserver 1.31+ (as kube-aggregator) unconditionally sets the standard uid header when proxying (just like it sets the other standard headers)
- k8s.io/apiserver library v0.31.0+ wires the uid CLI flag
- k8s.io/apiserver library v0.31.0+ NewDelegatingAuthenticationOptions() does not default the uid header; this keeps the CLI flags safe against much older kube-apiservers which aren't protecting the header; in practice, most aggregated API servers pull their auth header config from the configmap published by kube-apiserver rather than the CLI flags, so if/when their kube-apiserver publishes uid header config, they'll start using it
In the future (kube-apiserver 1.32+) we can consider making kube-apiserver also include the standard username/groups/extra/uid headers the aggregator uses (which are not configurable) in the configmap it publishes containing auth config, so that aggregation works even if kube-apiserver only set non-standard requestheader flag options
There was a problem hiding this comment.
I removed the defaulting from the DelegatingAuthenticationOptions() which I think was the only thing missing to get us to the desired state as described by Jordan.
We'll probably still have to think how to update the cluster trust CM - #115834 (comment).
stlaz
force-pushed
the
remote-uid
branch
2 times, most recently
from
603a7ab
to
6d7b8c8
Compare
…ctions first The file is too big, test functions should be put first for clarity.
stlaz
force-pushed
the
remote-uid
branch
2 times, most recently
from
cda7dc1
to
ade608a
Compare
k8s-ci-robot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stlaz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@stlaz: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
The latest commit removes the |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds the ability to set a UID header in order to be recognized in the RequestHeader authentication. It pushes the new header configuration into the
kube-system/extension-apiserver-authenticationCM, and makes the client-go AuthProxyRoundTripper add user UID to the requests it handles.Which issue(s) this PR fixes:
Fixes #93699
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: