Merge pull request #3287 from cnvergence/auth-order-followup

kcp-ci-bot · web-flow · commit 52b110390acb · 2025-02-10T20:02:13.000+01:00
✨ authorizer: swap Webhook and RBAC in the default order
diff --git a/docs/content/concepts/authorization/authorizers.md b/docs/content/concepts/authorization/authorizers.md
@@ -13,18 +13,17 @@ In kcp, a request has four different ways of being admitted:
 * It can be permitted by an external HTTPS webhook backend.
 
 They are related in the following way:
-
 ``` mermaid
 graph TD
     start(Request):::state --> main_alt[/one of\]:::or
     main_alt --> aaga[Always Allow Groups Auth]
     main_alt --> aapa[Always Allow Paths Auth]
-    main_alt --> wa[Webhook Auth]
     main_alt --> rga[Required Groups Auth]
+    main_alt --> wa[Webhook Auth]
+
 
     aaga --> decision(Decision):::state
     aapa --> decision
-    wa --> decision
 
     subgraph "RBAC"
     rga --> wca[Workspace Content Auth]
@@ -40,6 +39,7 @@ graph TD
     lpa --> decision
     gpa --> decision
     bpa --> decision
+    wa --> decision
 
     classDef state color:#F77
     classDef or fill:none,stroke:none
@@ -276,9 +276,25 @@ The webhook will receive JSON-marshalled `SubjectAccessReview` objects, that (co
 }
 ```
 
-
     The extra field will contain the logical cluster _name_ (e.g. o43u2gh528rtfg721rg92), not the human-readable path. Webhooks need to resolve the name to a path themselves if necessary.
 
+### Authorizer Order
+
+By default, the authorizers are evaluated in the following order:
+
+1. **Always Allow Groups Authorizer** (`AlwaysAllowGroups`)
+2. **Always Allow Paths Authorizer** (`AlwaysAllowPaths`)
+3. **RBAC Chain** (`RBAC`)
+4. **Webhook Authorizer** (`Webhook`)
+
+The order in which authorizers are evaluated can be configured. This allows administrators to customize the authorization flow.
+It can be done by setting `--authorization-order` flag in the kcp server. This flag accepts a comma-separated list of authorizer names, which will be evaluated in the specified order.
+
+Here is an example on how to enable the Webhook to be the first one in the authorizers chain:
+```sh
+--authorization-order=Webhook,AlwaysAllowGroups,AlwaysAllowPaths,RBAC
+```
+
 ### Scopes
 
 Scopes are a way to limit the access of a user to a specific logical cluster.
diff --git a/pkg/server/options/authorization.go b/pkg/server/options/authorization.go
@@ -52,7 +52,7 @@ type Authorization struct {
 	Webhook *kubeoptions.BuiltInAuthorizationOptions
 
 	// AuthorizationOrder is the list of authorizers that allows to rearrange the default order.
-	// The default is four authorizers in a union: AlwaysAllowGroups, AlwaysAllowPaths, Webhook and RBAC.
+	// The default is four authorizers in a union: AlwaysAllowGroups, AlwaysAllowPaths, RBAC and Webhook.
 	AuthorizationOrder []string
 }
 
@@ -67,7 +67,7 @@ const (
 	authorizerRBAC string = "RBAC"
 )
 
-var defaultAuthorizers = []string{authorizerAlwaysAllowGroups, authorizerAlwaysAllowPaths, authorizerWebhook, authorizerRBAC}
+var defaultAuthorizers = []string{authorizerAlwaysAllowGroups, authorizerAlwaysAllowPaths, authorizerRBAC, authorizerWebhook}
 
 func isValidAuthorizer(authorizer string) bool {
 	return sets.NewString(defaultAuthorizers...).Has(authorizer)
@@ -147,7 +147,7 @@ func (s *Authorization) AddFlags(fs *pflag.FlagSet) {
 
 	fs.StringSliceVar(&s.AuthorizationOrder, "authorization-order", s.AuthorizationOrder,
 		"A list of authorizers that should be enabled, allowing administrator rearrange the default order."+
-			"The default order is: AlwaysAllowGroups, AlwaysAllowPaths, Webhook, RBAC")
+			"The default order is: AlwaysAllowGroups,AlwaysAllowPaths,RBAC,Webhook")
 
 	// Only surface selected, webhook-related CLI flags
 
diff --git a/test/e2e/authorizer/authorizationorder_test.go b/test/e2e/authorizer/authorizationorder_test.go
@@ -34,29 +34,103 @@ import (
 
 func TestAuthorizationOrder(t *testing.T) {
 	framework.Suite(t, "control-plane")
-	webhookPort := "8081"
-	ctx, cancelFunc := context.WithCancel(context.Background())
-	t.Cleanup(cancelFunc)
-	// start a webhook that allows kcp to boot up
-	webhookStop := RunWebhook(ctx, t, webhookPort, "kubernetes:authz:allow")
-	t.Cleanup(webhookStop)
-
-	server := framework.PrivateKcpServer(t, framework.WithCustomArguments(
-		"--authorization-order",
-		"Webhook,AlwaysAllowPaths,AlwaysAllowGroups,RBAC",
-		"--authorization-webhook-config-file",
-		"authzorder.kubeconfig",
-	))
-
-	// create clients
+	t.Parallel()
+	t.Run("Authorization order 1", func(t *testing.T) {
+		webhookPort := "8080"
+		ctx, cancelFunc := context.WithCancel(context.Background())
+		t.Cleanup(cancelFunc)
+		webhookStop := RunWebhook(ctx, t, webhookPort, "kubernetes:authz:allow")
+		t.Cleanup(webhookStop)
+
+		server, kcpClusterClient, kubeClusterClient := setupTest(t, "AlwaysAllowGroups,AlwaysAllowPaths,Webhook,RBAC", "testdata/webhook1.kubeconfig")
+
+		t.Log("Admin should be allowed to list Workspaces.")
+		_, err := kcpClusterClient.Cluster(logicalcluster.NewPath("root")).TenancyV1alpha1().Workspaces().List(ctx, metav1.ListOptions{})
+		require.NoError(t, err)
+
+		// stop the webhook and switch to a deny policy
+		webhookStop()
+		RunWebhook(ctx, t, webhookPort, "kubernetes:authz:deny")
+
+		t.Log("Admin should not be allowed to list ConfigMaps.")
+		_, err = kubeClusterClient.Cluster(logicalcluster.NewPath("root")).CoreV1().ConfigMaps("default").List(ctx, metav1.ListOptions{})
+		require.Error(t, err)
+		// access to health endpoints should still be granted based on --always-allow-paths,
+		// even if the webhook rejects the request
+		t.Log("Verify that it is allowed to access one of AllowAllPaths endpoints.")
+		verifyEndpointAccess(ctx, t, server, "/healthz", true)
+	})
+
+	t.Run("Authorization order 2", func(t *testing.T) {
+		webhookPort := "8081"
+		ctx, cancelFunc := context.WithCancel(context.Background())
+		t.Cleanup(cancelFunc)
+		webhookStop := RunWebhook(ctx, t, webhookPort, "kubernetes:authz:allow")
+		t.Cleanup(webhookStop)
+
+		server, kcpClusterClient, kubeClusterClient := setupTest(t, "Webhook,AlwaysAllowGroups,AlwaysAllowPaths,RBAC", "testdata/webhook2.kubeconfig")
+
+		t.Log("Verify that it is allowed to access one of AllowAllPaths endpoints.")
+		verifyEndpointAccess(ctx, t, server, "/livez", true)
+
+		t.Log("Admin should be allowed now to list Workspaces.")
+		_, err := kcpClusterClient.Cluster(logicalcluster.NewPath("root")).TenancyV1alpha1().Workspaces().List(ctx, metav1.ListOptions{})
+		require.NoError(t, err)
+
+		// stop the webhook and switch to a deny policy
+		webhookStop()
+		RunWebhook(ctx, t, webhookPort, "kubernetes:authz:deny")
+
+		t.Log("Admin should not be allowed now to list Logical clusters.")
+		_, err = kcpClusterClient.Cluster(logicalcluster.NewPath("root")).CoreV1alpha1().LogicalClusters().List(ctx, metav1.ListOptions{})
+		require.Error(t, err)
+
+		t.Log("Admin should not be allowed to list Services.")
+		_, err = kubeClusterClient.Cluster(logicalcluster.NewPath("root")).CoreV1().Services("default").List(ctx, metav1.ListOptions{})
+		require.Error(t, err)
+
+		t.Log("Verify that it is not allowed to access one of AllowAllPaths endpoints.")
+		verifyEndpointAccess(ctx, t, server, "/readyz", false)
+	})
+
+	t.Run("Default authorization order", func(t *testing.T) {
+		webhookPort := "8082"
+		ctx, cancelFunc := context.WithCancel(context.Background())
+		t.Cleanup(cancelFunc)
+		webhookStop := RunWebhook(ctx, t, webhookPort, "kubernetes:authz:deny")
+		t.Cleanup(webhookStop)
+		// This will setup the test with the default authorization order: AlwaysAllowGroups,AlwaysAllowPaths,RBAC,Webhook
+		server, kcpClusterClient, _ := setupTest(t, "", "testdata/webhook3.kubeconfig")
+
+		t.Log("Verify that it is allowed to access one of AllowAllPaths endpoints.")
+		verifyEndpointAccess(ctx, t, server, "/healthz", true)
+
+		t.Log("Admin should be allowed to list Workspaces.")
+		_, err := kcpClusterClient.Cluster(logicalcluster.NewPath("root")).TenancyV1alpha1().Workspaces().List(ctx, metav1.ListOptions{})
+		require.NoError(t, err)
+	})
+}
+
+func setupTest(t *testing.T, authOrder, webhookConfigFile string) (framework.RunningServer, kcpclientset.ClusterInterface, kcpkubernetesclientset.ClusterInterface) {
+	args := []string{
+		"--authorization-webhook-config-file", webhookConfigFile,
+	}
+	if authOrder != "" {
+		args = append(args, "--authorization-order", authOrder)
+	}
+
+	server := framework.PrivateKcpServer(t, framework.WithCustomArguments(args...))
+
 	kcpConfig := server.BaseConfig(t)
 	kubeClusterClient, err := kcpkubernetesclientset.NewForConfig(kcpConfig)
-	require.NoError(t, err, "failed to construct client for server")
+	require.NoError(t, err)
 	kcpClusterClient, err := kcpclientset.NewForConfig(kcpConfig)
-	require.NoError(t, err, "failed to construct client for server")
+	require.NoError(t, err)
+
+	return server, kcpClusterClient, kubeClusterClient
+}
 
-	// access to health endpoints should not be granted, as webhook is first
-	// in the order of authorizers and rejects the request
+func verifyEndpointAccess(ctx context.Context, t *testing.T, server framework.RunningServer, endpoint string, shouldSucceed bool) {
 	rootShardCfg := server.RootShardSystemMasterBaseConfig(t)
 	if rootShardCfg.NegotiatedSerializer == nil {
 		rootShardCfg.NegotiatedSerializer = kubernetesscheme.Codecs.WithoutConversion()
@@ -65,35 +139,16 @@ func TestAuthorizationOrder(t *testing.T) {
 	// in a reloadable authorizer that also always injects a privilegedGroup authorizer
 	// that lets system:masters users in.
 	rootShardCfg.BearerToken = ""
-	restClient, err := rest.UnversionedRESTClientFor(rootShardCfg)
-	require.NoError(t, err)
-
-	t.Log("Verify that you are allowed to access one of AllowAllPaths endpoints.")
-	req := rest.NewRequest(restClient).RequestURI("/livez")
-	t.Logf("%s should not be accessible.", req.URL().String())
-	_, err = req.Do(ctx).Raw()
-	require.NoError(t, err)
 
-	t.Log("Admin should be allowed now to list Workspaces.")
-	_, err = kcpClusterClient.Cluster(logicalcluster.NewPath("root")).TenancyV1alpha1().Workspaces().List(ctx, metav1.ListOptions{})
+	restClient, err := rest.UnversionedRESTClientFor(rootShardCfg)
 	require.NoError(t, err)
 
-	webhookStop()
-	// run the webhook with deny policy
-	webhookStop = RunWebhook(ctx, t, webhookPort, "kubernetes:authz:deny")
-	t.Cleanup(webhookStop)
-
-	t.Log("Admin should not be allowed now to list Logical clusters.")
-	_, err = kcpClusterClient.Cluster(logicalcluster.NewPath("root")).CoreV1alpha1().LogicalClusters().List(ctx, metav1.ListOptions{})
-	require.Error(t, err)
-
-	t.Log("Admin should not be allowed to list Services.")
-	_, err = kubeClusterClient.Cluster(logicalcluster.NewPath("root")).CoreV1().Services("default").List(ctx, metav1.ListOptions{})
-	require.Error(t, err)
-
-	t.Log("Verify that it is not allowed to access AllowAllPaths endpoints.")
-	req = rest.NewRequest(restClient).RequestURI("/healthz")
-	t.Logf("%s should not be accessible.", req.URL().String())
+	req := rest.NewRequest(restClient).RequestURI(endpoint)
+	t.Logf("Verifying access to: %s", req.URL().String())
 	_, err = req.Do(ctx).Raw()
-	require.Error(t, err)
+	if shouldSucceed {
+		require.NoError(t, err)
+	} else {
+		require.Error(t, err)
+	}
 }
diff --git a/test/e2e/authorizer/authorizer_test.go b/test/e2e/authorizer/authorizer_test.go
@@ -49,7 +49,7 @@ import (
 	"github.com/kcp-dev/kcp/test/e2e/framework"
 )
 
-//go:embed *.yaml
+//go:embed testdata/*.yaml
 var embeddedResources embed.FS
 
 func TestAuthorizer(t *testing.T) {
@@ -78,8 +78,8 @@ func TestAuthorizer(t *testing.T) {
 	_, org2Workspace1 := framework.NewWorkspaceFixture(t, server, org2, framework.WithName("workspace1"), framework.WithRootShard()) // on root for deep SAR test
 	framework.NewWorkspaceFixture(t, server, org2, framework.WithName("workspace2"))
 
-	createResources(ctx, t, dynamicClusterClient, kubeDiscoveryClient, org1.Join("workspace1"), "workspace1-resources.yaml")
-	createResources(ctx, t, dynamicClusterClient, kubeDiscoveryClient, org2.Join("workspace1"), "workspace1-resources.yaml")
+	createResources(ctx, t, dynamicClusterClient, kubeDiscoveryClient, org1.Join("workspace1"), "testdata/workspace1-resources.yaml")
+	createResources(ctx, t, dynamicClusterClient, kubeDiscoveryClient, org2.Join("workspace1"), "testdata/workspace1-resources.yaml")
 
 	framework.AdmitWorkspaceAccess(ctx, t, kubeClusterClient, org1, []string{"user-1", "user-2", "user-3", "user-4", "user-5"}, nil, false)
 
diff --git a/test/e2e/authorizer/testdata/webhook1.kubeconfig b/test/e2e/authorizer/testdata/webhook1.kubeconfig
@@ -3,7 +3,7 @@ kind: Config
 clusters:
   - name: httest
     cluster:
-      certificate-authority: .TestWebhook/ca.crt
+      certificate-authority: .TestAuthorizationOrder/Authorization_order_1/ca.crt
       server: https://localhost:8080/
 current-context: webhook
 contexts:
diff --git a/test/e2e/authorizer/testdata/webhook2.kubeconfig b/test/e2e/authorizer/testdata/webhook2.kubeconfig
@@ -3,7 +3,7 @@ kind: Config
 clusters:
   - name: httest
     cluster:
-      certificate-authority: .TestAuthorizationOrder/ca.crt
+      certificate-authority: .TestAuthorizationOrder/Authorization_order_2/ca.crt
       server: https://localhost:8081/
 current-context: webhook
 contexts:
diff --git a/test/e2e/authorizer/testdata/webhook3.kubeconfig b/test/e2e/authorizer/testdata/webhook3.kubeconfig
@@ -0,0 +1,12 @@
+apiVersion: v1
+kind: Config
+clusters:
+  - name: httest
+    cluster:
+      certificate-authority: .TestAuthorizationOrder/Default_authorization_order/ca.crt
+      server: https://localhost:8082/
+current-context: webhook
+contexts:
+  - name: webhook
+    context:
+      cluster: httest
diff --git a/test/e2e/authorizer/testdata/workspace1-resources.yaml b/test/e2e/authorizer/testdata/workspace1-resources.yaml
diff --git a/test/e2e/authorizer/utils.go b/test/e2e/authorizer/utils.go
@@ -35,7 +35,7 @@ func RunWebhook(ctx context.Context, t *testing.T, port string, response string)
 	address := fmt.Sprintf("localhost:%s", port)
 
 	ctx, cancel := context.WithCancel(ctx)
-	pkiDir := fmt.Sprintf(".%s", t.Name())
+	pkiDir := fmt.Sprintf("testdata/.%s", t.Name())
 	args := []string{
 		"--tls",
 		"--response", response,
diff --git a/test/e2e/authorizer/webhook_test.go b/test/e2e/authorizer/webhook_test.go
