prevent workspace authentication from returning system groups or names

On-behalf-of: @SAP [email protected]

Author: xrstf

pkg/authentication/authenticators.go

@@ -26,51 +26,39 @@ import (
 	"github.com/kcp-dev/kcp/pkg/proxy/lookup"
 )
 
-type workspaceAuthenticator struct{}
-
 func NewWorkspaceAuthenticator() authenticator.Request {
-	return &workspaceAuthenticator{}
-}
-
-func (a *workspaceAuthenticator) AuthenticateRequest(req *http.Request) (*authenticator.Response, bool, error) {
-	reqAuthenticator, ok := WorkspaceAuthenticatorFrom(req.Context())
-	if !ok {
-		return nil, false, nil
-	}
-
-	return reqAuthenticator.AuthenticateRequest(req)
-}
+	return authenticator.RequestFunc(func(req *http.Request) (*authenticator.Response, bool, error) {
+		reqAuthenticator, ok := WorkspaceAuthenticatorFrom(req.Context())
+		if !ok {
+			return nil, false, nil
+		}
 
-type clusterScoper struct {
-	delegate authenticator.Request
+		return reqAuthenticator.AuthenticateRequest(req)
+	})
 }
 
 func withClusterScope(delegate authenticator.Request) authenticator.Request {
-	return &clusterScoper{
-		delegate: delegate,
-	}
-}
-
-func (a *clusterScoper) AuthenticateRequest(req *http.Request) (*authenticator.Response, bool, error) {
-	response, authenticated, ok := a.delegate.AuthenticateRequest(req)
-	if authenticated {
-		cluster := lookup.ClusterNameFrom(req.Context())
-
-		extra := response.User.GetExtra()
-		if extra == nil {
-			extra = map[string][]string{}
-		}
-		if true {
-			extra["authentication.kcp.io/scopes"] = append(extra["authentication.kcp.io/scopes"], fmt.Sprintf("cluster:%s", cluster))
-		}
-
-		response.User = &user.DefaultInfo{
-			Name:   response.User.GetName(),
-			UID:    response.User.GetUID(),
-			Groups: response.User.GetGroups(),
-			Extra:  extra,
+	return authenticator.RequestFunc(func(req *http.Request) (*authenticator.Response, bool, error) {
+		response, authenticated, ok := delegate.AuthenticateRequest(req)
+		if authenticated {
+			cluster := lookup.ClusterNameFrom(req.Context())
+
+			extra := response.User.GetExtra()
+			if extra == nil {
+				extra = map[string][]string{}
+			}
+			if true {
+				extra["authentication.kcp.io/scopes"] = append(extra["authentication.kcp.io/scopes"], fmt.Sprintf("cluster:%s", cluster))
+			}
+
+			response.User = &user.DefaultInfo{
+				Name:   response.User.GetName(),
+				UID:    response.User.GetUID(),
+				Groups: response.User.GetGroups(),
+				Extra:  extra,
+			}
 		}
-	}
 
-	return response, authenticated, ok
+		return response, authenticated, ok
+	})
 }

pkg/proxy/authentication/groups.go renamed to pkg/authentication/groups.go

@@ -235,5 +235,14 @@ func (c *state) Lookup(wsType logicalcluster.Path) (authenticator.Request, bool)
 		return nil, false
 	}
 
-	return authenticatorunion.New(authenticators...), true
+	authenticator := authenticatorunion.New(authenticators...)
+
+	// ensure that per-workspace auth cannot be used to become a system: user/group
+	authenticator = ForbidSystemUsernames(authenticator)
+	filtered := &GroupFilter{
+		Authenticator:     authenticator,
+		DropGroupPrefixes: []string{"system:"},
+	}
+
+	return filtered, true
 }

pkg/proxy/authentication/groups_test.go renamed to pkg/authentication/groups_test.go

@@ -0,0 +1,44 @@
+/*
+Copyright 2022 The KCP Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package authentication
+
+import (
+	"errors"
+	"net/http"
+	"strings"
+
+	"k8s.io/apiserver/pkg/authentication/authenticator"
+)
+
+// ForbidSystemUsernames wraps an authenticator and prevents it from returning
+// an internal system username (anything beginning with "system:"). This is so
+// per-workspace authenticators cannot impersonate low-level system accounts or
+// serviceaccounts.
+// This filter should be used together with the GroupsFilter to also strip
+// system groups.
+func ForbidSystemUsernames(delegate authenticator.Request) authenticator.Request {
+	return authenticator.RequestFunc(func(req *http.Request) (*authenticator.Response, bool, error) {
+		result, authenticated, err := delegate.AuthenticateRequest(req)
+		if err == nil {
+			if strings.HasPrefix(result.User.GetName(), "system:") {
+				return nil, false, errors.New("system usernames are not admitted")
+			}
+		}
+
+		return result, authenticated, err
+	})
+}

pkg/authentication/index.go

@@ -36,8 +36,8 @@ import (
 	kcpkubernetesinformers "github.com/kcp-dev/client-go/informers"
 	kcpkubernetesclientset "github.com/kcp-dev/client-go/kubernetes"
 
+	kcpauthentication "github.com/kcp-dev/kcp/pkg/authentication"
 	"github.com/kcp-dev/kcp/pkg/authorization/bootstrap"
-	kcpauthentication "github.com/kcp-dev/kcp/pkg/proxy/authentication"
 )
 
 const resyncPeriod = 10 * time.Hour

pkg/authentication/usernames.go

@@ -86,7 +86,7 @@ func startMockOIDC(t *testing.T, server kcptestingserver.RunningServer) (*mockoi
 	return m, ca
 }
 
-func mockJWTAuthenticator(t *testing.T, m *mockoidc.MockOIDC, ca *crypto.CA) tenancyv1alpha1.JWTAuthenticator {
+func mockJWTAuthenticator(t *testing.T, m *mockoidc.MockOIDC, ca *crypto.CA, userPrefix, groupPrefix string) tenancyv1alpha1.JWTAuthenticator {
 	cfg := m.Config()
 
 	caCert, _, err := ca.Config.GetPEMBytes()
@@ -101,11 +101,11 @@ func mockJWTAuthenticator(t *testing.T, m *mockoidc.MockOIDC, ca *crypto.CA) ten
 		ClaimMappings: tenancyv1alpha1.ClaimMappings{
 			Username: tenancyv1alpha1.PrefixedClaimOrExpression{
 				Claim:  "email",
-				Prefix: ptr.To("oidc:"),
+				Prefix: ptr.To(userPrefix),
 			},
 			Groups: tenancyv1alpha1.PrefixedClaimOrExpression{
 				Claim:  "groups",
-				Prefix: ptr.To("oidc:"),
+				Prefix: ptr.To(groupPrefix),
 			},
 		},
 	}

pkg/proxy/options/authentication.go

@@ -34,6 +34,7 @@ import (
 	kcpkubernetesclientset "github.com/kcp-dev/client-go/kubernetes"
 	"github.com/kcp-dev/logicalcluster/v3"
 
+	"github.com/kcp-dev/kcp/pkg/authorization/bootstrap"
 	tenancyv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/tenancy/v1alpha1"
 	kcpclientset "github.com/kcp-dev/kcp/sdk/client/clientset/versioned/cluster"
 	kcptesting "github.com/kcp-dev/kcp/sdk/testing"
@@ -205,6 +206,18 @@ func TestWorkspaceOIDC(t *testing.T) {
 				teamCPath: true,
 			},
 		},
+		{
+			name:     "impersonating system groups is not allowed and those groups will be stripped",
+			username: "hacker",
+			email:    "[email protected]",
+			groups:   []string{bootstrap.SystemKcpAdminGroup, "system:masters"},
+			mock:     mockB,
+			workspaceAccess: map[logicalcluster.Path]bool{
+				teamAPath: false,
+				teamBPath: false,
+				teamCPath: false,
+			},
+		},
 	}
 
 	for _, testcase := range testcases {
@@ -306,6 +319,106 @@ func TestUserScope(t *testing.T) {
 	require.Equal(t, user.Extra["authentication.kcp.io/scopes"], authenticationv1.ExtraValue{"cluster:" + teamWs.Spec.Cluster})
 }
 
+func TestForbiddenSystemAccess(t *testing.T) {
+	framework.Suite(t, "control-plane")
+
+	ctx := context.Background()
+
+	// start kcp and setup clients
+	server := kcptesting.SharedKcpServer(t)
+
+	baseWsPath, _ := kcptesting.NewWorkspaceFixture(t, server, logicalcluster.NewPath("root"), kcptesting.WithNamePrefix("oidc-scope"))
+
+	kcpConfig := server.BaseConfig(t)
+	kubeClusterClient, err := kcpkubernetesclientset.NewForConfig(kcpConfig)
+	require.NoError(t, err)
+	kcpClusterClient, err := kcpclientset.NewForConfig(kcpConfig)
+	require.NoError(t, err)
+
+	mock, ca := startMockOIDC(t, server)
+
+	// create an evil AuthConfig that would not prefix OIDC-provided groups, theoretically allowing
+	// users to become part of system groups.
+	// setup a new workspace auth config that uses mockoidc's server
+	authConfig := &tenancyv1alpha1.WorkspaceAuthenticationConfiguration{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "evil-oidc",
+		},
+		Spec: tenancyv1alpha1.WorkspaceAuthenticationConfigurationSpec{
+			JWT: []tenancyv1alpha1.JWTAuthenticator{
+				mockJWTAuthenticator(t, mock, ca, "", ""),
+			},
+		},
+	}
+
+	t.Logf("Creating WorkspaceAuthenticationConfguration %s...", authConfig.Name)
+	_, err = kcpClusterClient.Cluster(baseWsPath).TenancyV1alpha1().WorkspaceAuthenticationConfigurations().Create(ctx, authConfig, metav1.CreateOptions{})
+	require.NoError(t, err)
+
+	wsType := createWorkspaceType(t, ctx, kcpClusterClient, baseWsPath, authConfig.Name)
+
+	// create a new workspace with our new type
+	t.Log("Creating Workspaces...")
+	teamPath, _ := kcptesting.NewWorkspaceFixture(t, server, baseWsPath, kcptesting.WithName("team-a"), kcptesting.WithType(baseWsPath, tenancyv1alpha1.WorkspaceTypeName(wsType)))
+
+	// give a dummy user access
+	grantWorkspaceAccess(t, ctx, kubeClusterClient, teamPath, []rbacv1.Subject{{
+		Kind: "User",
+		Name: "[email protected]",
+	}})
+
+	// wait until the authenticator is ready
+	token := createOIDCToken(t, mock, "dummy", "[email protected]", nil)
+
+	client, err := kcpkubernetesclientset.NewForConfig(framework.ConfigWithToken(token, kcpConfig))
+	require.NoError(t, err)
+
+	t.Log("Waiting for authenticator to be ready...")
+	require.Eventually(t, func() bool {
+		_, err := client.Cluster(teamPath).CoreV1().ConfigMaps("default").List(ctx, metav1.ListOptions{})
+
+		return err == nil
+	}, wait.ForeverTestTimeout, 500*time.Millisecond)
+
+	// Now that we know that the authenticator is ready, run the actual tests that ensure we do NOT
+	// gain access based on our system names / groups.
+
+	testcases := []struct {
+		name     string
+		username string
+		email    string
+		groups   []string
+	}{
+		{
+			name:     fmt.Sprintf("%s should not give workspace access", bootstrap.SystemKcpAdminGroup),
+			username: "al",
+			email:    "[email protected]",
+			groups:   []string{bootstrap.SystemKcpAdminGroup},
+		},
+		{
+			name:     "shard-admin should not be admitted",
+			username: "al",
+			email:    "shard-admin",
+			groups:   nil,
+		},
+	}
+
+	t.Log("Testing tokens...")
+	for _, testcase := range testcases {
+		t.Run(testcase.name, func(t *testing.T) {
+			t.Parallel()
+
+			token := createOIDCToken(t, mock, testcase.username, testcase.email, testcase.groups)
+
+			client, err := kcpkubernetesclientset.NewForConfig(framework.ConfigWithToken(token, kcpConfig))
+			require.NoError(t, err)
+
+			_, err = client.Cluster(teamPath).CoreV1().ConfigMaps("default").List(ctx, metav1.ListOptions{})
+			require.Error(t, err, "user should have no access")
+		})
+	}
+}
+
 func createWorkspaceAuthentication(t *testing.T, ctx context.Context, client kcpclientset.ClusterInterface, workspace logicalcluster.Path, mock *mockoidc.MockOIDC, ca *crypto.CA) string {
 	name := fmt.Sprintf("mockoidc-%d", rand.Int())
 
@@ -316,7 +429,7 @@ func createWorkspaceAuthentication(t *testing.T, ctx context.Context, client kcp
 		},
 		Spec: tenancyv1alpha1.WorkspaceAuthenticationConfigurationSpec{
 			JWT: []tenancyv1alpha1.JWTAuthenticator{
-				mockJWTAuthenticator(t, mock, ca),
+				mockJWTAuthenticator(t, mock, ca, "oidc:", "oidc:"),
 			},
 		},
 	}