redesign how shard-per-workspace-authentication works to allow TokenReviews to be handled

The original design for per-workspace-auth was in the front-proxy only, only later did I add it to
each shard, primarily to make writing e2e tests easier.

In the front-proxy, there is a chain of middleware handlers that

* resolve the cluster path
* find the authenticator for a workspace
* inject the authenticator into the request context
* the handle the "optional auth", where the authenticator is read from the context again

This means the actual authenticator used (i.e. in genericConfig.Authentication.Authenticator) is one
that simply reads the authenticator from step 3 (out of the context) and calls it.

In the shard variant, this concept was kept the same, i.e. there are handlers that are involved to
make the per-workspace-authenticator work. This however means that any other component in kcp that
uses the .Authenticator has one that has no concept of per-workspace auth anymore, since the
middlewares are not being called.

Thankfully the concept of middlewares is not required to make per-workspace-auth working on the
shard level, so this commit condenses the logic on the shards to instead provide a "standalone"
authenticator that does all steps listed above in one big function.

Required for this refactoring is untangling the previous LocalProxy initialisation routine. Now the
cluster index is started separatedly (since the new standalone auther needs it) and then handed into
the middleware.

On-behalf-of: @SAP [email protected]
Signed-off-by: Marvin Beckers <[email protected]>

Author: xrstf

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.