Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Correctly check original groups against system:masters in scope filter #3263

Merged
merged 1 commit into from
Jan 23, 2025
+97 −41
Merged

🐛 Correctly check original groups against system:masters in scope filter #3263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 26 additions & 11 deletions pkg/server/filters/impersonation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,13 @@ var (
}
)

// impersonationContextType is a context key for impersonation marker. It's true
// if the request is impersonated.
// impersonationContextType is a context key for impersonation markers.
type impersonationContextType int

const (
// impersonationContextKey is true if a request is impersonated.
impersonationContextKey impersonationContextType = iota
originalUserContextKey
)

// WithImpersonationGatekeeper checks the request for impersonations and validates them,
Expand All @@ -72,10 +73,10 @@ func WithImpersonationGatekeeper(handler http.Handler) http.Handler {
impersonationUser := req.Header.Get(authenticationv1.ImpersonateUserHeader)
impersonationGroups := req.Header[authenticationv1.ImpersonateGroupHeader]
impersonationExtras := []string{}
for _, header := range req.Header {
for _, h := range header {
if strings.HasPrefix(h, authenticationv1.ImpersonateUserExtraHeaderPrefix) {
impersonationExtras = append(impersonationExtras, h)
for header, headerValues := range req.Header {
for _, h := range headerValues {
if strings.HasPrefix(header, authenticationv1.ImpersonateUserExtraHeaderPrefix) {
impersonationExtras = append(impersonationExtras, fmt.Sprintf("%s=%s", header, h))
}
}
}
Expand All @@ -94,7 +95,6 @@ func WithImpersonationGatekeeper(handler http.Handler) http.Handler {
// remember that we impersonated for withScoping
ctx := req.Context()
ctx = context.WithValue(ctx, impersonationContextKey, true)
req = req.WithContext(ctx)

requester, exists := request.UserFrom(ctx)
if !exists {
Expand All @@ -103,6 +103,9 @@ func WithImpersonationGatekeeper(handler http.Handler) http.Handler {
errorCodecs, schema.GroupVersion{}, w, req)
return
}
ctx = context.WithValue(ctx, originalUserContextKey, requester)
req = req.WithContext(ctx)

if validImpersonation(requester.GetGroups(), req.Header[authenticationv1.ImpersonateGroupHeader]) {
handler.ServeHTTP(w, req)
return
Expand All @@ -126,15 +129,25 @@ func WithImpersonationScoping(handler http.Handler) http.Handler {
return
}

// get user from context. Should always be there.
var (
originalUser user.Info
ok bool
)
// fetch the original user from context. We stored this in WithImpersonationGatekeeper.
if originalUser, ok = req.Context().Value(originalUserContextKey).(user.Info); !ok {
responsewriters.InternalError(w, req, fmt.Errorf("no impersonating user in context"))
return
}

// get impersonated user from context. Should always be there.
u, exists := request.UserFrom(req.Context())
if !exists {
responsewriters.InternalError(w, req, fmt.Errorf("no user in context"))
return
}

// system:masters can impersonate any group, without a scope
if sets.New(u.GetGroups()...).Has(authorizationbootstrap.SystemMastersGroup) {
// system:masters can impersonate any group, without a scope.
if sets.New(originalUser.GetGroups()...).Has(authorizationbootstrap.SystemMastersGroup) {
handler.ServeHTTP(w, req)
return
}
Expand All @@ -146,18 +159,20 @@ func WithImpersonationScoping(handler http.Handler) http.Handler {
return
}

// add a scope to the user information.
extra := u.GetExtra()
if extra == nil {
extra = map[string][]string{}
}
extra["authentication.kcp.io/scopes"] = append(extra["authentication.kcp.io/scopes"], fmt.Sprintf("cluster:%s", cluster.Name))

userScoped := &user.DefaultInfo{
Name: u.GetName(),
UID: u.GetUID(),
Groups: u.GetGroups(),
Extra: extra,
}

extra["authentication.kcp.io/scopes"] = append(extra["authentication.kcp.io/scopes"], fmt.Sprintf("cluster:%s", cluster.Name))
handler.ServeHTTP(w, req.WithContext(request.WithUser(req.Context(), userScoped)))
})
}
Expand Down
101 changes: 71 additions & 30 deletions pkg/server/filters/impersonation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,13 +175,15 @@ func TestWithImpersonationGatekeeper(t *testing.T) {

// Define test cases
tests := []struct {
name string
impersonateUserHeader string
impersonateGroupHeader []string
otherHeaders map[string]string
user user.Info
expectedStatus int
handlerCalled bool
name string
impersonateUserHeader string
impersonateGroupHeader []string
otherHeaders map[string]string
user user.Info
expectedStatus int
expectedImpersonationKey bool
expectedOriginalUser bool
handlerCalled bool
}{
{
name: "No impersonation headers",
Expand Down Expand Up @@ -209,8 +211,10 @@ func TestWithImpersonationGatekeeper(t *testing.T) {
Groups: []string{"group1", "group2"},
Extra: nil,
},
expectedStatus: http.StatusOK,
handlerCalled: true,
expectedStatus: http.StatusOK,
expectedImpersonationKey: true,
expectedOriginalUser: true,
handlerCalled: true,
},
{
name: "Impersonation headers present, invalid impersonation with higher privilege group",
Expand All @@ -235,8 +239,10 @@ func TestWithImpersonationGatekeeper(t *testing.T) {
Groups: []string{SystemKcpAdminGroup, "group2"},
Extra: nil,
},
expectedStatus: http.StatusOK,
handlerCalled: true,
expectedStatus: http.StatusOK,
expectedImpersonationKey: true,
expectedOriginalUser: true,
handlerCalled: true,
},
{
name: "Impersonation headers present, invalid impersonation with higher privilege group",
Expand Down Expand Up @@ -272,8 +278,10 @@ func TestWithImpersonationGatekeeper(t *testing.T) {
Groups: []string{"group1"},
Extra: nil,
},
expectedStatus: http.StatusOK,
handlerCalled: true,
expectedStatus: http.StatusOK,
expectedImpersonationKey: true,
expectedOriginalUser: true,
handlerCalled: true,
},
{
name: "Impersonation headers present only for groups, invalid impersonation with privileged group",
Expand All @@ -298,8 +306,10 @@ func TestWithImpersonationGatekeeper(t *testing.T) {
Groups: []string{"group1"},
Extra: nil,
},
expectedStatus: http.StatusOK,
handlerCalled: true,
expectedStatus: http.StatusOK,
expectedImpersonationKey: true,
expectedOriginalUser: true,
handlerCalled: true,
},
}

Expand All @@ -309,6 +319,18 @@ func TestWithImpersonationGatekeeper(t *testing.T) {

// Create a mock handler that sets the flag when called
mockHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()

impersonationFlag, ok := ctx.Value(impersonationContextKey).(bool)
assert.True(t, ok, "context should always contain impersonation context key")
assert.Equal(t, tt.expectedImpersonationKey, impersonationFlag, "impersonation context key should match expected value")

if tt.expectedOriginalUser {
user, ok := ctx.Value(originalUserContextKey).(user.Info)
assert.True(t, ok, "context should include original user (impersonation requestor)")
assert.Equal(t, tt.user, user, "user stored in context should equal requesting user")
}

handlerCalledFlag = true
w.WriteHeader(http.StatusOK)
})
Expand Down Expand Up @@ -358,7 +380,8 @@ func TestWithImpersonationGatekeeper(t *testing.T) {
func TestWithScoping(t *testing.T) {
tests := []struct {
name string
user user.Info
originalUser user.Info
impersonatedUser user.Info
cluster *request.Cluster
expectedStatus int
handlerCalled bool
Expand All @@ -367,22 +390,25 @@ func TestWithScoping(t *testing.T) {
}{
{
name: "no scoping marker. Programmers error.",
user: &mockUser{Name: "test-user", UID: "uid-123", Groups: []string{"group1"}, Extra: nil},
originalUser: &mockUser{Name: "original-user"},
impersonatedUser: &mockUser{Name: "test-user", UID: "uid-123", Groups: []string{"group1"}, Extra: nil},
cluster: &request.Cluster{Name: "cluster-1"},
expectedStatus: http.StatusInternalServerError,
handlerCalled: false,
noImpersonationKey: true,
},
{
name: "No user in context",
user: nil,
cluster: &request.Cluster{Name: "cluster-1"},
expectedStatus: http.StatusInternalServerError,
handlerCalled: false,
name: "No user in context",
originalUser: &mockUser{Name: "original-user"},
impersonatedUser: nil,
cluster: &request.Cluster{Name: "cluster-1"},
expectedStatus: http.StatusInternalServerError,
handlerCalled: false,
},
{
name: "No cluster in context",
user: &mockUser{
name: "No cluster in context",
originalUser: &mockUser{Name: "original-user"},
impersonatedUser: &mockUser{
Name: "test-user",
UID: "uid-123",
Groups: []string{"group1"},
Expand All @@ -392,8 +418,9 @@ func TestWithScoping(t *testing.T) {
handlerCalled: false,
},
{
name: "Valid user and cluster with no existing extra scopes",
user: &mockUser{
name: "Valid user and cluster with no existing extra scopes",
originalUser: &mockUser{Name: "original-user"},
impersonatedUser: &mockUser{
Name: "test-user",
UID: "uid-456",
Groups: []string{"group1"},
Expand All @@ -405,8 +432,9 @@ func TestWithScoping(t *testing.T) {
expectedExtraScope: "cluster:cluster-2",
},
{
name: "Valid user and cluster with existing extra scopes",
user: &mockUser{
name: "Valid user and cluster with existing extra scopes",
originalUser: &mockUser{Name: "original-user"},
impersonatedUser: &mockUser{
Name: "test-user",
UID: "uid-789",
Groups: []string{"group1"},
Expand All @@ -419,6 +447,18 @@ func TestWithScoping(t *testing.T) {
handlerCalled: true,
expectedExtraScope: "cluster:cluster-3",
},
{
name: "Original user was part of system:masters group",
originalUser: &mockUser{Name: "original-user", Groups: []string{"system:masters"}},
impersonatedUser: &mockUser{
Name: "test-user",
UID: "uid-789",
},
cluster: &request.Cluster{Name: "cluster-4"},
expectedStatus: http.StatusOK,
handlerCalled: true,
expectedExtraScope: "",
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -455,9 +495,10 @@ func TestWithScoping(t *testing.T) {
ctx := req.Context()
if !tt.noImpersonationKey {
ctx = context.WithValue(ctx, impersonationContextKey, true)
ctx = context.WithValue(ctx, originalUserContextKey, tt.originalUser)
}
if tt.user != nil {
ctx = request.WithUser(ctx, tt.user)
if tt.impersonatedUser != nil {
ctx = request.WithUser(ctx, tt.impersonatedUser)
}
if tt.cluster != nil {
ctx = request.WithCluster(ctx, *tt.cluster)
Expand Down
Loading