Merge pull request #30881 from deads2k/impersonate-user-extra

Automatic merge from submit-queue

Impersonate user extra

Second commit builds on https://github.com/kubernetes/kubernetes/pull/30803.

This adds a restriction to `user.Info.Extra`, keys must be lower case.  This is because HTTP headers are case insensitive, so we can't be sure that we'll get the right case through proxies or even Go (the go library capitalizes after dashes).  I don't think anyone is using them, if they are, they'll need to update to properly plumb through an impersonation flow.

@kubernetes/sig-auth 
@ericchiang since you have background here.

pkg/apis/authentication/types.go

@@ -28,6 +28,12 @@ const (
 	// ImpersonateGroupHeader is used to impersonate a particular group during an API server request.
 	// It can be repeated multipled times for multiple groups.
 	ImpersonateGroupHeader = "Impersonate-Group"
+
+	// ImpersonateUserExtraHeaderPrefix is a prefix for any header used to impersonate an entry in the
+	// extra map[string][]string for user.Info.  The key will be every after the prefix.
+	// It can be repeated multipled times for multiple map keys and the same key can be repeated multiple
+	// times to have multiple elements in the slice under a single key
+	ImpersonateUserExtraHeaderPrefix = "Impersonate-Extra-"
 )
 
 // +genclient=true

pkg/apiserver/handler_impersonation.go

@@ -17,7 +17,9 @@ limitations under the License.
 package apiserver
 
 import (
+	"fmt"
 	"net/http"
+	"strings"
 
 	"github.com/golang/glog"
 
@@ -32,20 +34,17 @@ import (
 // WithImpersonation is a filter that will inspect and check requests that attempt to change the user.Info for their requests
 func WithImpersonation(handler http.Handler, requestContextMapper api.RequestContextMapper, a authorizer.Authorizer) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-		requestedUser := req.Header.Get(authenticationapi.ImpersonateUserHeader)
-		if len(requestedUser) == 0 {
-			if len(req.Header[authenticationapi.ImpersonateGroupHeader]) > 0 {
-				glog.V(4).Infof("attempt to impersonate groups without impersonating a user: %v", req.Header[authenticationapi.ImpersonateGroupHeader])
+		impersonationRequests, err := buildImpersonationRequests(req.Header)
+		if err != nil {
+			glog.V(4).Infof("%v", err)
 			forbidden(w, req)
 			return
 		}
-
+		if len(impersonationRequests) == 0 {
 			handler.ServeHTTP(w, req)
 			return
 		}
 
-		impersonationRequests := buildImpersonationRequests(requestedUser, req.Header[authenticationapi.ImpersonateGroupHeader])
-
 		ctx, exists := requestContextMapper.Get(req)
 		if !exists {
 			forbidden(w, req)
@@ -65,6 +64,7 @@ func WithImpersonation(handler http.Handler, requestContextMapper api.RequestCon
 		// and group information
 		username := ""
 		groups := []string{}
+		userExtra := map[string][]string{}
 		for _, impersonationRequest := range impersonationRequests {
 			actingAsAttributes := &authorizer.AttributesRecord{
 				User:            requestor,
@@ -92,8 +92,15 @@ func WithImpersonation(handler http.Handler, requestContextMapper api.RequestCon
 				actingAsAttributes.Resource = "groups"
 				groups = append(groups, impersonationRequest.Name)
 
+			case authenticationapi.Kind("UserExtra"):
+				extraKey := impersonationRequest.FieldPath
+				extraValue := impersonationRequest.Name
+				actingAsAttributes.Resource = "userextras"
+				actingAsAttributes.Subresource = extraKey
+				userExtra[extraKey] = append(userExtra[extraKey], extraValue)
+
 			default:
-				glog.V(4).Infof("unknown impersonation request type: %v", impersonationRequest)
+				glog.V(4).Infof("unknown impersonation request type: %v\n", impersonationRequest)
 				forbidden(w, req)
 				return
 			}
@@ -109,7 +116,7 @@ func WithImpersonation(handler http.Handler, requestContextMapper api.RequestCon
 		newUser := &user.DefaultInfo{
 			Name:   username,
 			Groups: groups,
-			Extra:  map[string][]string{},
+			Extra:  userExtra,
 		}
 		requestContextMapper.Update(req, api.WithUser(ctx, newUser))
 
@@ -121,19 +128,55 @@ func WithImpersonation(handler http.Handler, requestContextMapper api.RequestCon
 }
 
 // buildImpersonationRequests returns a list of objectreferences that represent the different things we're requesting to impersonate.
-// Each request must be authorized against the current user before switching contexts
-func buildImpersonationRequests(requestedUser string, requestedGroups []string) []api.ObjectReference {
+// Also includes a map[string][]string representing user.Info.Extra
+// Each request must be authorized against the current user before switching contexts.
+func buildImpersonationRequests(headers http.Header) ([]api.ObjectReference, error) {
 	impersonationRequests := []api.ObjectReference{}
 
+	requestedUser := headers.Get(authenticationapi.ImpersonateUserHeader)
+	hasUser := len(requestedUser) > 0
+	if hasUser {
 		if namespace, name, err := serviceaccount.SplitUsername(requestedUser); err == nil {
 			impersonationRequests = append(impersonationRequests, api.ObjectReference{Kind: "ServiceAccount", Namespace: namespace, Name: name})
 		} else {
 			impersonationRequests = append(impersonationRequests, api.ObjectReference{Kind: "User", Name: requestedUser})
 		}
+	}
 
-	for _, group := range requestedGroups {
+	hasGroups := false
+	for _, group := range headers[authenticationapi.ImpersonateGroupHeader] {
+		hasGroups = true
 		impersonationRequests = append(impersonationRequests, api.ObjectReference{Kind: "Group", Name: group})
 	}
 
-	return impersonationRequests
+	hasUserExtra := false
+	for headerName, values := range headers {
+		if !strings.HasPrefix(headerName, authenticationapi.ImpersonateUserExtraHeaderPrefix) {
+			continue
+		}
+
+		hasUserExtra = true
+		extraKey := strings.ToLower(headerName[len(authenticationapi.ImpersonateUserExtraHeaderPrefix):])
+
+		// make a separate request for each extra value they're trying to set
+		for _, value := range values {
+			impersonationRequests = append(impersonationRequests,
+				api.ObjectReference{
+					Kind: "UserExtra",
+					// we only parse out a group above, but the parsing will fail if there isn't SOME version
+					// using the internal version will help us fail if anyone starts using it
+					APIVersion: authenticationapi.SchemeGroupVersion.String(),
+					Name:       value,
+					// ObjectReference doesn't have a subresource field.  FieldPath is close and available, so we'll use that
+					// TODO fight the good fight for ObjectReference to refer to resources and subresources
+					FieldPath: extraKey,
+				})
+		}
+	}
+
+	if (hasGroups || hasUserExtra) && !hasUser {
+		return nil, fmt.Errorf("requested %v without impersonating a user", impersonationRequests)
+	}
+
+	return impersonationRequests, nil
 }

pkg/apiserver/handler_impersonation_test.go

@@ -62,6 +62,19 @@ func (impersonateAuthorizer) Authorize(a authorizer.Attributes) (authorized bool
 		return true, "", nil
 	}
 
+	if len(user.GetGroups()) > 1 && user.GetGroups()[1] == "extra-setter-scopes" && a.GetVerb() == "impersonate" && a.GetResource() == "userextras" && a.GetSubresource() == "scopes" {
+		return true, "", nil
+	}
+
+	if len(user.GetGroups()) > 1 && user.GetGroups()[1] == "extra-setter-particular-scopes" &&
+		a.GetVerb() == "impersonate" && a.GetResource() == "userextras" && a.GetSubresource() == "scopes" && a.GetName() == "scope-a" {
+		return true, "", nil
+	}
+
+	if len(user.GetGroups()) > 1 && user.GetGroups()[1] == "extra-setter-project" && a.GetVerb() == "impersonate" && a.GetResource() == "userextras" && a.GetSubresource() == "project" {
+		return true, "", nil
+	}
+
 	return false, "deny by default", nil
 }
 
@@ -71,6 +84,7 @@ func TestImpersonationFilter(t *testing.T) {
 		user                    user.Info
 		impersonationUser       string
 		impersonationGroups     []string
+		impersonationUserExtras map[string][]string
 		expectedUser            user.Info
 		expectedCode            int
 	}{
@@ -106,6 +120,17 @@ func TestImpersonationFilter(t *testing.T) {
 			},
 			expectedCode: http.StatusForbidden,
 		},
+		{
+			name: "impersonating-extra-without-user",
+			user: &user.DefaultInfo{
+				Name: "tester",
+			},
+			impersonationUserExtras: map[string][]string{"scopes": {"scope-a"}},
+			expectedUser: &user.DefaultInfo{
+				Name: "tester",
+			},
+			expectedCode: http.StatusForbidden,
+		},
 		{
 			name: "disallowed-group",
 			user: &user.DefaultInfo{
@@ -135,6 +160,66 @@ func TestImpersonationFilter(t *testing.T) {
 			},
 			expectedCode: http.StatusOK,
 		},
+		{
+			name: "disallowed-userextra-1",
+			user: &user.DefaultInfo{
+				Name:   "dev",
+				Groups: []string{"wheel"},
+			},
+			impersonationUser:       "system:admin",
+			impersonationGroups:     []string{"some-group"},
+			impersonationUserExtras: map[string][]string{"scopes": {"scope-a"}},
+			expectedUser: &user.DefaultInfo{
+				Name:   "dev",
+				Groups: []string{"wheel"},
+			},
+			expectedCode: http.StatusForbidden,
+		},
+		{
+			name: "disallowed-userextra-2",
+			user: &user.DefaultInfo{
+				Name:   "dev",
+				Groups: []string{"wheel", "extra-setter-project"},
+			},
+			impersonationUser:       "system:admin",
+			impersonationGroups:     []string{"some-group"},
+			impersonationUserExtras: map[string][]string{"scopes": {"scope-a"}},
+			expectedUser: &user.DefaultInfo{
+				Name:   "dev",
+				Groups: []string{"wheel", "extra-setter-project"},
+			},
+			expectedCode: http.StatusForbidden,
+		},
+		{
+			name: "disallowed-userextra-3",
+			user: &user.DefaultInfo{
+				Name:   "dev",
+				Groups: []string{"wheel", "extra-setter-particular-scopes"},
+			},
+			impersonationUser:       "system:admin",
+			impersonationGroups:     []string{"some-group"},
+			impersonationUserExtras: map[string][]string{"scopes": {"scope-a", "scope-b"}},
+			expectedUser: &user.DefaultInfo{
+				Name:   "dev",
+				Groups: []string{"wheel", "extra-setter-particular-scopes"},
+			},
+			expectedCode: http.StatusForbidden,
+		},
+		{
+			name: "allowed-userextras",
+			user: &user.DefaultInfo{
+				Name:   "dev",
+				Groups: []string{"wheel", "extra-setter-scopes"},
+			},
+			impersonationUser:       "system:admin",
+			impersonationUserExtras: map[string][]string{"scopes": {"scope-a", "scope-b"}},
+			expectedUser: &user.DefaultInfo{
+				Name:   "system:admin",
+				Groups: []string{},
+				Extra:  map[string][]string{"scopes": {"scope-a", "scope-b"}},
+			},
+			expectedCode: http.StatusOK,
+		},
 		{
 			name: "allowed-users-impersonation",
 			user: &user.DefaultInfo{
@@ -238,6 +323,12 @@ func TestImpersonationFilter(t *testing.T) {
 		for _, group := range tc.impersonationGroups {
 			req.Header.Add(authenticationapi.ImpersonateGroupHeader, group)
 		}
+		for extraKey, values := range tc.impersonationUserExtras {
+			for _, value := range values {
+				req.Header.Add(authenticationapi.ImpersonateUserExtraHeaderPrefix+extraKey, value)
+			}
+		}
+
 		resp, err := http.DefaultClient.Do(req)
 		if err != nil {
 			t.Errorf("%s: unexpected error: %v", tc.name, err)

pkg/auth/user/user.go

@@ -36,6 +36,8 @@ type Info interface {
 	// This is a map[string][]string because it needs to be serializeable into
 	// a SubjectAccessReviewSpec.authorization.k8s.io for proper authorization
 	// delegation flows
+	// In order to faithfully round-trip through an impersonation flow, these keys
+	// MUST be lowercase.
 	GetExtra() map[string][]string
 }