Include extra attributes in SubjectAccessReview#14768
Conversation
Problem Kubernetes authorization plugins can rely on extra attributes on a user, provided via X-Remote-Extra- headers, e.g. AWS EKS with AccessEntry authentication. Currently, the Linkerd Viz tap API doesn't include these attributes when making SubjectAccessReview requests, preventing tap from working in clusters that use authorization plugins relying on these extra attributes. Solution Updated the tap API to extract X-Remote-Extra- headers from incoming requests and include them in SubjectAccessReview calls. The header prefix is read from the extension-apiserver-authentication ConfigMap to support custom configurations. This implementation is based on the original work by David Symons in PR linkerd#13170. Changes: - Modified ResourceAuthzForUser in pkg/k8s/authz.go to accept extra attributes as map[string]authV1.ExtraValue - Updated viz/tap/api/handlers.go to extract and URL-decode extra headers - Modified viz/tap/api/server.go to read the configurable header prefix from the Kubernetes ConfigMap - Added tests to verify extra attributes are correctly passed through Validation Ran go test ./viz/tap/api/... ./pkg/k8s/... and all tests pass. Added TestHandleTap_ExtraHeaders to verify extra attributes are correctly extracted and passed to the Kubernetes client. Tested with an actual EKS cluster with AccessEntry authentication. Fixes linkerd#13169 Signed-off-by: Nils Mueller <20240901+Tolsto@users.noreply.github.com>
d650bb1 to
5757502
Compare
|
@olix0r Anything I can do to get this closer to being merged? |
adleong
left a comment
There was a problem hiding this comment.
Hey @Tolsto
Thanks for your extreme patience and I apologize for the long delay in getting this reviewed. This looks great and fixes a clear deficiency in our auth handling here. I left a couple small comments about header handling but otherwise this looks good to go.
Thanks again!
viz/tap/api/handlers.go
Outdated
|
|
||
| extra := make(map[string]authV1.ExtraValue) | ||
| for key, values := range req.Header { | ||
| if strings.HasPrefix(key, h.extraHeaderPrefix) { |
There was a problem hiding this comment.
if the extraHeaderPrefix is the empty string (if, for example, extra header prefix was not specified in the configmap) then does this treat every header as an extra auth header?
There was a problem hiding this comment.
do we also need to ignore case here since HTTP headers are case insensitive?
There was a problem hiding this comment.
On “empty prefix” concern:
Implemented. extractExtraHeaders now returns early when extraHeaderPrefix == "", so we no longer treat all headers as extra auth headers. I also added a server-side default (X-Remote-Extra-) when requestheader-extra-headers-prefix is missing/empty in the configmap, plus tests for both paths.
On “case-insensitive headers” concern:
Implemented. Extra-header prefix matching is now case-insensitive (strings.ToLower on both header key and prefix), while preserving the original suffix for URL decoding. Added a test covering mixed-case header handling.
Problem\n\nReview comments on linkerd#14768 called out two header-handling gaps: an empty extra-header prefix could match every request header, and prefix matching did not account for case-insensitive HTTP header names.\n\nSolution\n\nAdd centralized extra-header extraction that bails out when the prefix is empty and matches prefixes case-insensitively. Also default the server auth extra-header prefix to X-Remote-Extra- when the configmap omits it or provides an empty value. Extend tap API tests to cover case-insensitive matching, empty-prefix behavior, and default-prefix parsing.\n\nValidation\n\ngo test ./viz/tap/api/... ./pkg/k8s/...
551eb36 to
8a6849b
Compare
Problem
Kubernetes authorization plugins can rely on extra attributes on a user, provided via X-Remote-Extra- headers, e.g. AWS EKS with AccessEntry authentication. Currently, the Linkerd Viz tap API doesn't include these attributes when making SubjectAccessReview requests, preventing tap from working in clusters that use authorization plugins relying on these extra attributes.
Solution
Updated the tap API to extract X-Remote-Extra- headers from incoming requests and include them in SubjectAccessReview calls. The header prefix is read from the extension-apiserver-authentication ConfigMap to support custom configurations.
This implementation is based on the original work by David Symons in PR #13170.
Changes:
Validation
Ran go test ./viz/tap/api/... ./pkg/k8s/... and all tests pass. Added TestHandleTap_ExtraHeaders to verify extra attributes are correctly extracted and passed to the Kubernetes client.
Tested with an actual EKS cluster with AccessEntry authentication.
Fixes #13169