From d650bb1af311f29e1f799d6044118c334356ff4e Mon Sep 17 00:00:00 2001 From: Nils Mueller <20240901+Tolsto@users.noreply.github.com> Date: Sat, 29 Nov 2025 02:08:54 +0200 Subject: [PATCH] Include extra attributes in SubjectAccessReview 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: - 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 #13169 Signed-off-by: Nils Mueller <20240901+Tolsto@users.noreply.github.com> --- pkg/k8s/authz.go | 6 +++- viz/tap/api/handlers.go | 24 ++++++++++++---- viz/tap/api/handlers_test.go | 55 ++++++++++++++++++++++++++++++++++++ viz/tap/api/server.go | 41 ++++++++++++++++++--------- viz/tap/api/server_test.go | 29 +++++++++++-------- 5 files changed, 124 insertions(+), 31 deletions(-) diff --git a/pkg/k8s/authz.go b/pkg/k8s/authz.go index 602cffe45f5b6..065954fab0b5f 100644 --- a/pkg/k8s/authz.go +++ b/pkg/k8s/authz.go @@ -48,11 +48,15 @@ func ResourceAuthz( func ResourceAuthzForUser( ctx context.Context, client kubernetes.Interface, - namespace, verb, group, version, resource, subresource, name, user string, userGroups []string) error { + namespace, verb, group, version, resource, subresource, name, user string, + userGroups []string, + extra map[string]authV1.ExtraValue, +) error { sar := &authV1.SubjectAccessReview{ Spec: authV1.SubjectAccessReviewSpec{ User: user, Groups: userGroups, + Extra: extra, ResourceAttributes: &authV1.ResourceAttributes{ Namespace: namespace, Verb: verb, diff --git a/viz/tap/api/handlers.go b/viz/tap/api/handlers.go index 5869eb7bb1b30..9ac0677da0a65 100644 --- a/viz/tap/api/handlers.go +++ b/viz/tap/api/handlers.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net/http" + "net/url" "strings" "github.com/go-openapi/spec" @@ -17,17 +18,19 @@ import ( "github.com/prometheus/client_golang/prometheus/promhttp" "github.com/sirupsen/logrus" "google.golang.org/grpc/metadata" + authV1 "k8s.io/api/authorization/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/version" ) type handler struct { - k8sAPI *k8s.API - usernameHeader string - groupHeader string - grpcTapServer pb.TapServer - log *logrus.Entry + k8sAPI *k8s.API + usernameHeader string + groupHeader string + extraHeaderPrefix string + grpcTapServer pb.TapServer + log *logrus.Entry } // TODO: share with api_handlers.go @@ -123,6 +126,16 @@ func (h *handler) handleTap(w http.ResponseWriter, req *http.Request, p httprout namespace, resource, name, h.usernameHeader, h.groupHeader, ) + extra := make(map[string]authV1.ExtraValue) + for key, values := range req.Header { + if strings.HasPrefix(key, h.extraHeaderPrefix) { + key, err := url.QueryUnescape(strings.TrimPrefix(key, h.extraHeaderPrefix)) + if err == nil { + extra[key] = authV1.ExtraValue(values) + } + } + } + // TODO: it's possible this SubjectAccessReview is redundant, consider // removing, more info at https://github.com/linkerd/linkerd2/issues/3182 err := pkgK8s.ResourceAuthzForUser( @@ -137,6 +150,7 @@ func (h *handler) handleTap(w http.ResponseWriter, req *http.Request, p httprout name, req.Header.Get(h.usernameHeader), req.Header.Values(h.groupHeader), + extra, ) if err != nil { err = fmt.Errorf("tap authorization failed (%w), visit %s for more information", err, pkg.TapRbacURL) diff --git a/viz/tap/api/handlers_test.go b/viz/tap/api/handlers_test.go index 48bc15e64829f..27dea2d00e997 100644 --- a/viz/tap/api/handlers_test.go +++ b/viz/tap/api/handlers_test.go @@ -11,6 +11,9 @@ import ( "github.com/julienschmidt/httprouter" "github.com/linkerd/linkerd2/controller/k8s" "github.com/sirupsen/logrus" + authV1 "k8s.io/api/authorization/v1" + k8sFake "k8s.io/client-go/kubernetes/fake" + k8sTesting "k8s.io/client-go/testing" ) func TestHandleTap(t *testing.T) { @@ -71,3 +74,55 @@ func TestHandleTap(t *testing.T) { }) } } + +func TestHandleTap_ExtraHeaders(t *testing.T) { + k8sAPI, err := k8s.NewFakeAPI() + if err != nil { + t.Fatalf("NewFakeAPI returned an error: %s", err) + } + + h := &handler{ + k8sAPI: k8sAPI, + log: logrus.WithField("test", t.Name()), + extraHeaderPrefix: "X-Remote-Extra-", + } + + recorder := httptest.NewRecorder() + req := httptest.NewRequest("POST", "/apis/tap.linkerd.io/v1alpha1/watch/namespaces/foo/tap", nil) + req.Header.Set("X-Remote-Extra-Foo", "bar") + req.Header.Set("X-Remote-Extra-Baz", "qux") + + params := httprouter.Params{ + {Key: "namespace", Value: "foo"}, + } + + h.handleTap(recorder, req, params) + + client := k8sAPI.Client.(*k8sFake.Clientset) + actions := client.Actions() + + var sar *authV1.SubjectAccessReview + for _, action := range actions { + if action.GetVerb() == "create" && action.GetResource().Resource == "subjectaccessreviews" { + createAction := action.(k8sTesting.CreateAction) + obj := createAction.GetObject() + sar = obj.(*authV1.SubjectAccessReview) + break + } + } + + if sar == nil { + t.Fatal("Expected SubjectAccessReview to be created") + } + + if len(sar.Spec.Extra) != 2 { + t.Errorf("Expected 2 extra headers, got %d", len(sar.Spec.Extra)) + } + + if v, ok := sar.Spec.Extra["Foo"]; !ok || v[0] != "bar" { + t.Errorf("Expected Extra['Foo'] to be ['bar'], got %v", v) + } + if v, ok := sar.Spec.Extra["Baz"]; !ok || v[0] != "qux" { + t.Errorf("Expected Extra['Baz'] to be ['qux'], got %v", v) + } +} diff --git a/viz/tap/api/server.go b/viz/tap/api/server.go index 0a04b42d5f1e0..07005ee808e84 100644 --- a/viz/tap/api/server.go +++ b/viz/tap/api/server.go @@ -50,7 +50,7 @@ func NewServer( } }() - clientCAPem, allowedNames, usernameHeader, groupHeader, err := serverAuth(ctx, k8sAPI) + clientCAPem, allowedNames, usernameHeader, groupHeader, extraHeaderPrefix, err := serverAuth(ctx, k8sAPI) if err != nil { return nil, err } @@ -80,11 +80,12 @@ func NewServer( var emptyCert atomic.Value h := &handler{ - k8sAPI: k8sAPI, - usernameHeader: usernameHeader, - groupHeader: groupHeader, - grpcTapServer: grpcTapServer, - log: log, + k8sAPI: k8sAPI, + usernameHeader: usernameHeader, + groupHeader: groupHeader, + extraHeaderPrefix: extraHeaderPrefix, + grpcTapServer: grpcTapServer, + log: log, } lis, err := net.Listen("tcp", addr) @@ -164,28 +165,28 @@ func (a *Server) validate(req *http.Request) error { // authentication. // kubectl -n kube-system get cm/extension-apiserver-authentication // accessible via the extension-apiserver-authentication-reader role -func serverAuth(ctx context.Context, k8sAPI *k8s.API) (string, []string, string, string, error) { +func serverAuth(ctx context.Context, k8sAPI *k8s.API) (string, []string, string, string, string, error) { cm, err := k8sAPI.Client.CoreV1(). ConfigMaps(metav1.NamespaceSystem). Get(ctx, pkgk8s.ExtensionAPIServerAuthenticationConfigMapName, metav1.GetOptions{}) if err != nil { - return "", nil, "", "", fmt.Errorf("failed to load [%s] config: %w", pkgk8s.ExtensionAPIServerAuthenticationConfigMapName, err) + return "", nil, "", "", "", fmt.Errorf("failed to load [%s] config: %w", pkgk8s.ExtensionAPIServerAuthenticationConfigMapName, err) } clientCAPem, ok := cm.Data[pkgk8s.ExtensionAPIServerAuthenticationRequestHeaderClientCAFileKey] if !ok { - return "", nil, "", "", fmt.Errorf("no client CA cert available for apiextension-server") + return "", nil, "", "", "", fmt.Errorf("no client CA cert available for apiextension-server") } allowedNames, err := deserializeStrings(cm.Data["requestheader-allowed-names"]) if err != nil { - return "", nil, "", "", err + return "", nil, "", "", "", err } usernameHeaders, err := deserializeStrings(cm.Data["requestheader-username-headers"]) if err != nil { - return "", nil, "", "", err + return "", nil, "", "", "", err } usernameHeader := "" if len(usernameHeaders) > 0 { @@ -194,14 +195,28 @@ func serverAuth(ctx context.Context, k8sAPI *k8s.API) (string, []string, string, groupHeaders, err := deserializeStrings(cm.Data["requestheader-group-headers"]) if err != nil { - return "", nil, "", "", err + return "", nil, "", "", "", err } groupHeader := "" if len(groupHeaders) > 0 { groupHeader = groupHeaders[0] } - return clientCAPem, allowedNames, usernameHeader, groupHeader, nil + extraHeaderPrefixes, err := deserializeStrings(cm.Data["requestheader-extra-headers-prefix"]) + if err != nil { + return "", nil, "", "", "", err + } + // The extra headers prefix is used to identify headers that contain additional + // user attributes for authorization (e.g., "X-Remote-Extra-"). These headers are + // forwarded by the Kubernetes API server when acting as an aggregating proxy. + // The prefix is configurable via the --requestheader-extra-headers-prefix flag + // on the API server (defaults to "X-Remote-Extra-"). + extraHeaderPrefix := "" + if len(extraHeaderPrefixes) > 0 { + extraHeaderPrefix = extraHeaderPrefixes[0] + } + + return clientCAPem, allowedNames, usernameHeader, groupHeader, extraHeaderPrefix, nil } // copied from https://github.com/kubernetes/apiserver/blob/781c3cd1b3dc5b6f79c68ab0d16fe544600421ef/pkg/server/options/authentication.go#L360 diff --git a/viz/tap/api/server_test.go b/viz/tap/api/server_test.go index 4b2d4825d0228..b1def0255d7cd 100644 --- a/viz/tap/api/server_test.go +++ b/viz/tap/api/server_test.go @@ -18,12 +18,13 @@ import ( func TestAPIServerAuth(t *testing.T) { expectations := []struct { - k8sRes []string - clientCAPem string - allowedNames []string - usernameHeader string - groupHeader string - err error + k8sRes []string + clientCAPem string + allowedNames []string + usernameHeader string + groupHeader string + extraHeaderPrefix string + err error }{ { err: fmt.Errorf("failed to load [%s] config: configmaps %q not found", k8sutils.ExtensionAPIServerAuthenticationConfigMapName, k8sutils.ExtensionAPIServerAuthenticationConfigMapName), @@ -44,11 +45,12 @@ data: requestheader-username-headers: '["X-Remote-User"]' `, }, - clientCAPem: "requestheader-client-ca-file", - allowedNames: []string{"name1", "name2"}, - usernameHeader: "X-Remote-User", - groupHeader: "X-Remote-Group", - err: nil, + clientCAPem: "requestheader-client-ca-file", + allowedNames: []string{"name1", "name2"}, + usernameHeader: "X-Remote-User", + groupHeader: "X-Remote-Group", + extraHeaderPrefix: "X-Remote-Extra-", + err: nil, }, } @@ -62,7 +64,7 @@ data: t.Fatalf("NewFakeAPI returned an error: %s", err) } - clientCAPem, allowedNames, usernameHeader, groupHeader, err := serverAuth(ctx, k8sAPI) + clientCAPem, allowedNames, usernameHeader, groupHeader, extraHeaderPrefix, err := serverAuth(ctx, k8sAPI) if err != nil && exp.err != nil { if err.Error() != exp.err.Error() { @@ -86,6 +88,9 @@ data: if groupHeader != exp.groupHeader { t.Errorf("apiServerAuth returned unexpected groupHeader: %q, expected: %q", groupHeader, exp.groupHeader) } + if extraHeaderPrefix != exp.extraHeaderPrefix { + t.Errorf("apiServerAuth returned unexpected extraHeaderPrefix: %q, expected: %q", extraHeaderPrefix, exp.extraHeaderPrefix) + } }) } }