Merge pull request #3624 from xrstf/token-reviews

Fix TokenReviews when using WorkspaceAuthentication

Author: kcp-ci-bot

pkg/authentication/authenticators.go

@@ -21,11 +21,45 @@ import (
 	"net/http"
 
 	"k8s.io/apiserver/pkg/authentication/authenticator"
+	"k8s.io/apiserver/pkg/authentication/group"
 	"k8s.io/apiserver/pkg/authentication/user"
+	"k8s.io/apiserver/pkg/endpoints/request"
 
+	"github.com/kcp-dev/kcp/pkg/index"
 	"github.com/kcp-dev/kcp/pkg/proxy/lookup"
 )
 
+func NewStandaloneWorkspaceAuthenticator(clusterIndex *index.State, authIndex AuthenticatorIndex) authenticator.Request {
+	return authenticator.RequestFunc(func(req *http.Request) (*authenticator.Response, bool, error) {
+		clusterName, err := request.ClusterNameFrom(req.Context())
+		if err != nil {
+			return nil, false, fmt.Errorf("request has no cluster context: %w", err)
+		}
+
+		result, found := clusterIndex.Lookup(clusterName.Path())
+		if !found {
+			return nil, false, nil
+		}
+
+		// workspacemounts have no type
+		if result.Type.Empty() {
+			return nil, false, nil
+		}
+
+		reqAuthenticator, ok := authIndex.Lookup(result.Type)
+		if !ok {
+			return nil, false, nil
+		}
+
+		reqAuthenticator = group.NewAuthenticatedGroupAdder(reqAuthenticator)
+
+		// make the authenticator always add the target cluster to the user scopes
+		reqAuthenticator = withClusterScope(reqAuthenticator)
+
+		return reqAuthenticator.AuthenticateRequest(req)
+	})
+}
+
 func NewWorkspaceAuthenticator() authenticator.Request {
 	return authenticator.RequestFunc(func(req *http.Request) (*authenticator.Response, bool, error) {
 		reqAuthenticator, ok := WorkspaceAuthenticatorFrom(req.Context())

pkg/server/config.go

@@ -405,6 +405,16 @@ func NewConfig(ctx context.Context, opts kcpserveroptions.CompletedOptions) (*Co
 		virtualWorkspaceServerProxyTransport = transport
 	}
 
+	// Prepare a local cluster index that can be used both by the LocalProxy, as well as the authenticator
+	// (the authenticator cannot always use the data the localproxy puts into the context because the
+	// authentication rest storage provider calls the authenticator outside of the handler chain).
+	clusterIndex := newLocalClusterIndex(
+		opts.Extra.ShardName,
+		opts.Extra.ShardBaseURL,
+		c.KcpSharedInformerFactory.Tenancy().V1alpha1().Workspaces(),
+		c.KcpSharedInformerFactory.Core().V1alpha1().LogicalClusters(),
+	)
+
 	// Prepare an authentication index to be used later by a middleware. We start it early
 	// because it can potentially fail and the BuildHandlerChainFunc() has no way to return
 	// an error.
@@ -420,6 +430,7 @@ func NewConfig(ctx context.Context, opts kcpserveroptions.CompletedOptions) (*Co
 
 		authIndex = authIndexState
 	}
+
 	// preHandlerChainMux is called before the actual handler chain. Note that BuildHandlerChainFunc below
 	// is called multiple times, but only one of the handler chain will actually be used. Hence, we wrap it
 	// to give handlers below one mux.Handle func to call.
@@ -477,7 +488,7 @@ func NewConfig(ctx context.Context, opts kcpserveroptions.CompletedOptions) (*Co
 		if kcpfeatures.DefaultFeatureGate.Enabled(kcpfeatures.WorkspaceAuthentication) {
 			genericConfig.Authentication.Authenticator = authenticatorunion.New(
 				genericConfig.Authentication.Authenticator,
-				authentication.NewWorkspaceAuthenticator(),
+				authentication.NewStandaloneWorkspaceAuthenticator(clusterIndex, authIndex),
 			)
 		}
 
@@ -515,13 +526,7 @@ func NewConfig(ctx context.Context, opts kcpserveroptions.CompletedOptions) (*Co
 		apiHandler = mux
 
 		apiHandler = filters.WithAuditInit(apiHandler) // Must run before any audit annotation is made
-		apiHandler, err = WithLocalProxy(apiHandler,
-			opts.Extra.ShardName,
-			opts.Extra.ShardBaseURL,
-			opts.Extra.AdditionalMappingsFile,
-			c.KcpSharedInformerFactory.Tenancy().V1alpha1().Workspaces(),
-			c.KcpSharedInformerFactory.Core().V1alpha1().LogicalClusters(),
-		)
+		apiHandler, err = WithLocalProxy(apiHandler, opts.Extra.ShardName, opts.Extra.AdditionalMappingsFile, clusterIndex)
 		if err != nil {
 			panic(err) // shouldn't happen due to flag validation
 		}

pkg/server/localproxy.go

@@ -49,15 +49,11 @@ import (
 	tenancyv1alpha1informers "github.com/kcp-dev/kcp/sdk/client/informers/externalversions/tenancy/v1alpha1"
 )
 
-// WithLocalProxy returns a handler with a local-only mini-front-proxy. It is
-// able to translate logical clusters with the data on the local shard. This is
-// mainly interesting for standalone mode, without a real front-proxy in-front.
-func WithLocalProxy(
-	handler http.Handler,
-	shardName, shardBaseURL, additionalMappingsFile string,
+func newLocalClusterIndex(
+	shardName, shardBaseURL string,
 	workspaceInformer tenancyv1alpha1informers.WorkspaceClusterInformer,
 	logicalClusterInformer corev1alpha1informers.LogicalClusterClusterInformer,
-) (http.Handler, error) {
+) *index.State {
 	indexState := index.New([]index.PathRewriter{
 		indexrewriters.UserRewriter,
 	})
@@ -99,6 +95,17 @@ func WithLocalProxy(
 		},
 	})
 
+	return indexState
+}
+
+// WithLocalProxy returns a handler with a local-only mini-front-proxy. It is
+// able to translate logical clusters with the data on the local shard. This is
+// mainly interesting for standalone mode, without a real front-proxy in-front.
+func WithLocalProxy(
+	handler http.Handler,
+	shardName, additionalMappingsFile string,
+	clusterIndex *index.State,
+) (http.Handler, error) {
 	defaultHandlerFunc := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		ctx := req.Context()
 		logger := klog.FromContext(ctx)
@@ -144,7 +151,7 @@ func WithLocalProxy(
 		}
 
 		// lookup in our local, potentially partial index
-		r, found := indexState.Lookup(path)
+		r, found := clusterIndex.Lookup(path)
 		if found && r.ErrorCode != 0 {
 			// return code if set.
 			// TODO(mjudeikis): Would be nice to have a way to return a custom error message.

test/e2e/authentication/workspace_test.go

@@ -567,3 +567,76 @@ func TestAcceptableWorkspaceAuthenticationConfigurations(t *testing.T) {
 		})
 	}
 }
+
+func TestWorkspaceOIDCTokenReview(t *testing.T) {
+	framework.Suite(t, "control-plane")
+
+	ctx := context.Background()
+
+	// start kcp and setup clients
+	server := kcptesting.SharedKcpServer(t)
+
+	if len(server.ShardNames()) > 1 {
+		t.Skip("This feature currently does not support multi shards because AuthConfigs are not replicated yet.")
+	}
+
+	baseWsPath, _ := kcptesting.NewWorkspaceFixture(t, server, logicalcluster.NewPath("root"), kcptesting.WithNamePrefix("workspace-auth-token-review"))
+
+	kcpConfig := server.BaseConfig(t)
+	kubeClusterClient, err := kcpkubernetesclientset.NewForConfig(kcpConfig)
+	require.NoError(t, err)
+	kcpClusterClient, err := kcpclientset.NewForConfig(kcpConfig)
+	require.NoError(t, err)
+
+	mock, ca := authfixtures.StartMockOIDC(t, server)
+	authConfig := authfixtures.CreateWorkspaceOIDCAuthentication(t, ctx, kcpClusterClient, baseWsPath, mock, ca, nil)
+	wsType := authfixtures.CreateWorkspaceType(t, ctx, kcpClusterClient, baseWsPath, "with-oidc", authConfig)
+
+	// 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)))
+
+	var (
+		userName       = "peter"
+		userEmail      = "[email protected]"
+		userGroups     = []string{"developers", "admins"}
+		expectedGroups = []string{"system:authenticated"}
+	)
+
+	for _, group := range userGroups {
+		expectedGroups = append(expectedGroups, "oidc:"+group)
+	}
+
+	authfixtures.GrantWorkspaceAccess(t, ctx, kubeClusterClient, teamPath, "grant-oidc-user", "cluster-admin", []rbacv1.Subject{{
+		Kind: "User",
+		Name: "oidc:" + userEmail,
+	}})
+
+	token := authfixtures.CreateOIDCToken(t, mock, userName, userEmail, userGroups)
+
+	t.Logf("Creating TokenReview in %s", teamPath)
+
+	const kcpDefaultAudience = "https://kcp.default.svc"
+
+	review := &authenticationv1.TokenReview{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "my-review",
+		},
+		Spec: authenticationv1.TokenReviewSpec{
+			Token:     token,
+			Audiences: []string{kcpDefaultAudience},
+		},
+	}
+
+	var response *authenticationv1.TokenReview
+	require.Eventually(t, func() bool {
+		var err error
+
+		response, err = kubeClusterClient.Cluster(teamPath).AuthenticationV1().TokenReviews().Create(ctx, review, metav1.CreateOptions{})
+		require.NoError(t, err)
+
+		return response.Status.Authenticated
+	}, wait.ForeverTestTimeout, 500*time.Millisecond)
+
+	require.Contains(t, response.Status.Audiences, kcpDefaultAudience)
+}