fix(search): impersonate caller in karmada proxy plugin instead of using own credentials - #7825
Open
pujitha24 wants to merge 1 commit into
Open
fix(search): impersonate caller in karmada proxy plugin instead of using own credentials#7825pujitha24 wants to merge 1 commit into
pujitha24 wants to merge 1 commit into
Conversation
…ing own credentials The `karmada` search-proxy plugin is the fallback plugin that proxies requests to karmada-apiserver when the `cache` and `cluster` plugins don't handle them. It proxied every request using its own credentials instead of impersonating the original caller. In every current deployment path (karmadactl init, karmada-operator, and the Helm chart), karmada-search's kubeconfig is signed for `system:masters`, i.e. cluster-admin-equivalent. Since the plugin never forwarded caller identity, a request that reached this fallback plugin was executed against karmada-apiserver as `system:masters` regardless of the caller's own RBAC permissions on the karmada control plane. This mirrors the impersonation pattern already used elsewhere in this codebase for the same purpose (pkg/util/proxy/proxy.go's newProxyHandler, used for member cluster proxying, and pkg/registry/cluster/storage/aggregate.go): extract the original requester from the request context and set the Impersonate-User / Impersonate-Group headers (skipping the system:authenticated/system:unauthenticated pseudo-groups via the already-exported proxy.SkipGroup helper) before proxying, so karmada-apiserver authorizes the request as the real caller instead of as the plugin's own credentials. No RBAC manifest changes are needed: karmada-search's kubeconfig is a system:masters credential in all three deployment paths, which bypasses RBAC (including impersonation checks) entirely, so the existing credential already has implicit permission to impersonate any user. Validation: - go build ./pkg/search/... - go test -count=1 ./pkg/search/... (all pass, including the new/updated Test_karmadaProxy and Test_karmadaProxy_NoUser cases) - Confirmed the new tests are a genuine regression test: reverting only the karmada.go change and re-running the same tests fails them (missing Impersonate-User/Impersonate-Group headers, and a 200 instead of a 500 when no user is present in the request context); restoring the fix makes them pass again. - gofmt -l, hack/verify-license.sh, hack/verify-import-aliases.sh, and golangci-lint run ./pkg/search/proxy/framework/plugins/karmada/... all pass with no issues. - Not run: a live multi-cluster e2e reproduction (no live karmada-search deployment was available in this environment). The RBAC-safety reasoning above was verified by tracing the code paths that provision karmada-search's credentials instead. Report: karmada-io#5485 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
karmada-bot
added
the
size/M
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an RBAC privilege-escalation path in the karmada search-proxy fallback plugin by ensuring proxied requests to karmada-apiserver are executed as the original caller (via impersonation headers) instead of using the plugin’s own highly privileged credentials.
Changes:
- Extracts the authenticated requester from the incoming request context and sets
Impersonate-User/Impersonate-Groupheaders (skipping pseudo-groups) before proxying. - Returns an internal error when no user is present in the request context (instead of proxying with privileged credentials).
- Updates/extends unit tests to validate impersonation headers are forwarded and to cover the “no user” error case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/search/proxy/framework/plugins/karmada/karmada.go | Adds caller impersonation (and errors when caller identity is missing) for fallback proxying to karmada-apiserver. |
| pkg/search/proxy/framework/plugins/karmada/karmada_test.go | Adds assertions for impersonation headers and a regression test for missing user context. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+97
to
+102
| req.Header.Set(authenticationv1.ImpersonateUserHeader, requester.GetName()) | ||
| for _, group := range requester.GetGroups() { | ||
| if !proxy.SkipGroup(group) { | ||
| req.Header.Add(authenticationv1.ImpersonateGroupHeader, group) | ||
| } | ||
| } |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7825 +/- ##
==========================================
+ Coverage 42.08% 42.10% +0.02%
==========================================
Files 879 879
Lines 54852 54859 +7
==========================================
+ Hits 23083 23098 +15
+ Misses 30025 30019 -6
+ Partials 1744 1742 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
/assign @XiShanYongYe-Chang |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind bug
What this PR does / why we need it:
The
karmadasearch-proxy plugin (the fallback plugin that proxies requests tokarmada-apiserver when the
cacheandclusterplugins don't handle them) proxiedevery request using its own credentials (
dep.RestConfig), without impersonating theoriginal caller. In every current deployment path (karmadactl init, karmada-operator,
and the Helm chart), the kubeconfig karmada-search runs with is signed for
system:masters, i.e. it is cluster-admin-equivalent. Because the plugin neverforwarded caller identity, a request that reached this fallback plugin was executed
against karmada-apiserver as
system:mastersregardless of the caller's own RBACpermissions — e.g. a read-only user's request falling through to this plugin would be
authorized as cluster-admin on the karmada control plane.
This mirrors the exact impersonation pattern already used elsewhere in this codebase
for the same purpose (
pkg/util/proxy/proxy.go'snewProxyHandler, used for membercluster proxying, and
pkg/registry/cluster/storage/aggregate.go): extract theoriginal requester from the request context and set the
Impersonate-User/Impersonate-Groupheaders (skipping thesystem:authenticated/system:unauthenticatedpseudo-groups via the already-exported
proxy.SkipGrouphelper) before proxying, sokarmada-apiserver authorizes the request as the real caller instead of as the plugin's
own credentials.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
No RBAC manifest changes are needed: karmada-search's kubeconfig is provisioned as a
system:masterscredential in all three deployment paths (karmadactl init, thekarmada-operator, and the Helm chart), which bypasses RBAC (including impersonation
checks) entirely, so the existing credential already has implicit permission to
impersonate any user.
Validation:
go build ./pkg/search/...go test -count=1 ./pkg/search/...(all pass, including the new/updatedTest_karmadaProxyandTest_karmadaProxy_NoUsercases inpkg/search/proxy/framework/plugins/karmada/karmada_test.go)karmada.gochange and re-running the same tests fails them (missingImpersonate-User/Impersonate-Groupheaders, and a 200 instead of a 500 when nouser is present in the request context); restoring the fix makes them pass again.
gofmt -l,hack/verify-license.sh,hack/verify-import-aliases.sh, andgolangci-lint run ./pkg/search/proxy/framework/plugins/karmada/...all pass withno issues.
was available in this environment). The fix and its RBAC-safety reasoning were
verified by tracing the code paths that provision karmada-search's credentials
instead.
Does this PR introduce a user-facing change?:
Fixes #5485