Include extra attributes in SubjectAccessReview (#14768)

* 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>

* Handle extra auth headers safely

---------

Signed-off-by: Nils Mueller <20240901+Tolsto@users.noreply.github.com>
Co-authored-by: Alex Leong <alex@buoyant.io>

Author: Tolsto

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,

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,8 @@ func (h *handler) handleTap(w http.ResponseWriter, req *http.Request, p httprout
 		namespace, resource, name, h.usernameHeader, h.groupHeader,
 	)
 
+	extra := extractExtraHeaders(req.Header, h.extraHeaderPrefix)
+
 	// 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 +142,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)
@@ -180,6 +186,29 @@ func (h *handler) handleTap(w http.ResponseWriter, req *http.Request, p httprout
 	}
 }
 
+func extractExtraHeaders(header http.Header, extraHeaderPrefix string) map[string]authV1.ExtraValue {
+	if extraHeaderPrefix == "" {
+		return nil
+	}
+
+	extraHeaderPrefixLower := strings.ToLower(extraHeaderPrefix)
+	extraHeaderPrefixLen := len(extraHeaderPrefix)
+	extra := make(map[string]authV1.ExtraValue)
+
+	for key, values := range header {
+		if !strings.HasPrefix(strings.ToLower(key), extraHeaderPrefixLower) {
+			continue
+		}
+
+		extraKey, err := url.QueryUnescape(key[extraHeaderPrefixLen:])
+		if err == nil {
+			extra[extraKey] = authV1.ExtraValue(values)
+		}
+	}
+
+	return extra
+}
+
 // GET (not found)
 func handleNotFound() http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {

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,115 @@ 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)
+
+	sar := getSubjectAccessReview(t, k8sAPI)
+
+	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)
+	}
+}
+
+func TestHandleTap_ExtraHeadersPrefixCaseInsensitive(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")
+
+	params := httprouter.Params{
+		{Key: "namespace", Value: "foo"},
+	}
+
+	h.handleTap(recorder, req, params)
+	sar := getSubjectAccessReview(t, k8sAPI)
+
+	if v, ok := sar.Spec.Extra["Foo"]; !ok || v[0] != "bar" {
+		t.Errorf("Expected Extra['Foo'] to be ['bar'], got %v", v)
+	}
+}
+
+func TestHandleTap_ExtraHeadersEmptyPrefix(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()),
+	}
+
+	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-User", "alice")
+	req.Header.Set("Content-Type", "application/json")
+
+	params := httprouter.Params{
+		{Key: "namespace", Value: "foo"},
+	}
+
+	h.handleTap(recorder, req, params)
+	sar := getSubjectAccessReview(t, k8sAPI)
+
+	if len(sar.Spec.Extra) != 0 {
+		t.Errorf("Expected 0 extra headers, got %d", len(sar.Spec.Extra))
+	}
+}
+
+func getSubjectAccessReview(t *testing.T, k8sAPI *k8s.API) *authV1.SubjectAccessReview {
+	t.Helper()
+
+	client := k8sAPI.Client.(*k8sFake.Clientset)
+	actions := client.Actions()
+
+	for _, action := range actions {
+		if action.GetVerb() == "create" && action.GetResource().Resource == "subjectaccessreviews" {
+			createAction := action.(k8sTesting.CreateAction)
+			obj := createAction.GetObject()
+			return obj.(*authV1.SubjectAccessReview)
+		}
+	}
+
+	t.Fatal("Expected SubjectAccessReview to be created")
+	return nil
+}

viz/tap/api/server.go

@@ -22,6 +22,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+const defaultExtraHeaderPrefix = "X-Remote-Extra-"
+
 // Server holds the underlying http server and its config
 type Server struct {
 	*http.Server
@@ -50,7 +52,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 +82,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 +167,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 +197,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 := defaultExtraHeaderPrefix
+	if len(extraHeaderPrefixes) > 0 && 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

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,29 @@ 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,
+		},
+		{
+			k8sRes: []string{`
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: extension-apiserver-authentication
+  namespace: kube-system
+data:
+  requestheader-client-ca-file: 'requestheader-client-ca-file'
+`},
+			clientCAPem:       "requestheader-client-ca-file",
+			allowedNames:      nil,
+			usernameHeader:    "",
+			groupHeader:       "",
+			extraHeaderPrefix: defaultExtraHeaderPrefix,
+			err:               nil,
 		},
 	}
 
@@ -62,7 +81,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 +105,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)
+			}
 		})
 	}
 }