change position of webhook authorizer in authz chain so that it cannot override alwaysAllowPaths/Groups

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

Author: xrstf

docs/content/concepts/authorization/authorizers.md

@@ -16,36 +16,41 @@ They are related in the following way:
 
 ``` mermaid
 graph TD
-    start(Request):::state --> main_alt[/one of\]:::or
-    main_alt --> aapa[Always Allow Paths Auth]
-    main_alt --> aaga[Always Allow Groups Auth]
-    main_alt --> wa[Webhook Auth]
-    main_alt --> rga[Required Groups Auth]
+  start(Request):::state --> main_alt[/one of\]:::or
+  main_alt --> aapa[Always Allow Paths Auth]
+  main_alt --> aaga[Always Allow Groups Auth]
 
-    aapa --> decision(Decision):::state
-    aaga --> decision
-    wa --> decision
+  aapa --> decision(Decision):::state
+  aaga --> decision
+
+  main_alt --> kcp_alt[/one of\]:::or
+
+  subgraph "main authorizers"
+    kcp_alt --> rga[Required Groups Auth]
+    kcp_alt --> wa[Webhook Auth]
 
     subgraph "RBAC"
-    rga --> wca[Workspace Content Auth]
-    wca --> scrda[System CRD Auth]
-    scrda --> mppa[Max. Permission Policy Auth]
-
-    mppa --- mppa_alt[/one of\]:::or
-    mppa_alt --> lpa[Local Policy Auth]
-    mppa_alt --> gpa[Global Policy Auth]
-    mppa_alt --> bpa[Bootstrap Policy Auth]
+      rga --> wca[Workspace Content Auth]
+      wca --> scrda[System CRD Auth]
+      scrda --> mppa[Max. Permission Policy Auth]
+
+      mppa --- mppa_alt[/one of\]:::or
+      mppa_alt --> lpa[Local Policy Auth]
+      mppa_alt --> gpa[Global Policy Auth]
+      mppa_alt --> bpa[Bootstrap Policy Auth]
     end
+  end
 
-    lpa --> decision
-    gpa --> decision
-    bpa --> decision
+  wa --> decision
+  lpa --> decision
+  gpa --> decision
+  bpa --> decision
 
-    classDef state color:#F77
-    classDef or fill:none,stroke:none
+  classDef state color:#F77
+  classDef or fill:none,stroke:none
 ```
 
-[View graph on Kroki](https://kroki.io/mermaid/svg/eNqFkkFrwzAMhe_7Faa7dLBux0IOg7ZhvWxQusEOWRmKoyambuTZDln__RQnG02bUp-E3mfx9OzcginEe3wj-DgP1o_X-F2h83dRFHHDo5hMnsQeVPkF2iePVKKg7eeGZbLh2p8WQAADyUzXcHBipjXVYgW-4LryxWYIz0_wpaXKXORrSD4wLYh2lwjLA5sVlMWsPyywjb_AZSiVU1SO4674X7jj8j4XuvVJr42tSvMQ42g9ny1GoWe727Vkw2R3zoBEsaDSY-mPrLMeOCdtBsnbwXnci8U6PkKC1D6C4Wxf4edBrNDulWssiBVpJQ_HKzYY85NQXHy0TguDNc99IQm6P-2My5lbakqvgimDcyLvPAdzzmKZtVa1GQg5H2qmZih6qcG5GLei_amSNNno9nk67atkxVZpHZWcwz17oh2G-he4ue-L)
+[View graph on Kroki](https://kroki.io/mermaid/svg/eNqNkk1PwzAMhu_7Fda4DInBcVIPSGMTu4A0DSQOZUJumrVVs7okqcr49TgpHf3YgUtrxY9fv7GTaCxTeF1PAIxFbWc7-VlJY6-DIOADK2E-v4cjZsUHKhveUSGBDu97TpPmojbjMcQSw6Wq8WRgqRTVsEWbclzZdD-GkwG80VSVLc24k_NoLEVmMipm69_g7M5TSZ-aDDvlorxk3l25ihI_gKkrAOTOpLNvqc2Us9BWehXNdt1wMi3jvtUhWWP4JqOUKP-7S7fX7mG5avTBqTY1gotI56ZEIWFFhZWF7eiDIzxphI4xfDkZK4-w2q17kE82Kyt5F8_4dQtbqY-ZcZOBLalMnLquwINcMffB5SW32PmGirWfSKDqK14gEyY3iqJ_oBGjD0TWWB7TmJZFPGm-_KsHKwdnaXiUjI-icvxWhEJj1vIAzXsXpEgHV4-LRTdHGg6ZUkHBs7lhh5RLH_8Amf8GPg==)
 
 ### Always Allow Paths Authorizer
 
@@ -247,9 +252,14 @@ contexts:
       cluster: webhook
 ```
 
-The webhook will receive every authorization request made in kcp, including internal ones. This means if the webhook
-is badly configured, it can even prevent kcp from starting up successfully, but on the other hand this allows
-a lot of influence over the authorization in kcp.
+The webhook will receive every authorization request made in kcp and is therefore able to bypass traditional
+RBAC. However it cannot overrule the Always Allow Paths/Groups authorizers as these are required for core
+functionality in kcp like health checks.
+
+!!! note
+    However webhooks still have tremendous influence and a webhook that always denies every request will
+    block workspace creation, for example, potentially preventing kcp from even starting up because
+    the `root` workspace cannot be created.
 
 The webhook will receive JSON-marshalled `SubjectAccessReview` objects, that (compared to vanilla Kubernetes) include the name of target logical cluster as an `extra` field, like so:
 

pkg/server/options/authorization.go

@@ -131,7 +131,25 @@ func (s *Authorization) AddFlags(fs *pflag.FlagSet) {
 func (s *Authorization) ApplyTo(ctx context.Context, config *genericapiserver.Config, kubeInformers, globalKubeInformers kcpkubernetesinformers.SharedInformerFactory, kcpInformers, globalKcpInformers kcpinformers.SharedInformerFactory) error {
 	var authorizers []authorizer.Authorizer
 
+	localLogicalClusterLister := kcpInformers.Core().V1alpha1().LogicalClusters().Lister()
+	globalLogicalClusterLister := globalKcpInformers.Core().V1alpha1().LogicalClusters().Lister()
+
+	// group authorizer
+	if len(s.AlwaysAllowGroups) > 0 {
+		authorizers = append(authorizers, authorizerfactory.NewPrivilegedGroups(s.AlwaysAllowGroups...))
+	}
+
+	// path authorizer
+	if len(s.AlwaysAllowPaths) > 0 {
+		a, err := path.NewAuthorizer(s.AlwaysAllowPaths)
+		if err != nil {
+			return err
+		}
+		authorizers = append(authorizers, a)
+	}
+
 	// re-use the authorizer from the generic control plane (this is only set for webhooks)
+	var webhookAuthorizer authorizer.Authorizer
 	if webhook := s.Webhook; webhook != nil && webhook.WebhookConfigFile != "" {
 		authorizationConfig, err := webhook.ToAuthorizationConfig(informerfactoryhack.Wrap(kubeInformers))
 		if err != nil {
@@ -146,29 +164,12 @@ func (s *Authorization) ApplyTo(ctx context.Context, config *genericapiserver.Co
 			authorizationConfig.CustomDial = egressDialer
 		}
 
-		authorizer, _, err := authorizationConfig.New(ctx, config.APIServerID)
+		webhookAuthorizer, _, err = authorizationConfig.New(ctx, config.APIServerID)
 		if err != nil {
 			return err
 		}
 
-		authorizers = append(authorizers, authorizer)
-	}
-
-	localLogicalClusterLister := kcpInformers.Core().V1alpha1().LogicalClusters().Lister()
-	globalLogicalClusterLister := globalKcpInformers.Core().V1alpha1().LogicalClusters().Lister()
-
-	// group authorizer
-	if len(s.AlwaysAllowGroups) > 0 {
-		authorizers = append(authorizers, authorizerfactory.NewPrivilegedGroups(s.AlwaysAllowGroups...))
-	}
-
-	// path authorizer
-	if len(s.AlwaysAllowPaths) > 0 {
-		a, err := path.NewAuthorizer(s.AlwaysAllowPaths)
-		if err != nil {
-			return err
-		}
-		authorizers = append(authorizers, a)
+		webhookAuthorizer = authz.NewDecorator("10-webhook", webhookAuthorizer).AddAuditLogging().AddAnonymization().AddReasonAnnotation()
 	}
 
 	// kcp authorizers, these are evaluated in reverse order
@@ -209,7 +210,13 @@ func (s *Authorization) ApplyTo(ctx context.Context, config *genericapiserver.Co
 	requiredGroupsAuth := authz.NewRequiredGroupsAuthorizer(localLogicalClusterLister, globalLogicalClusterLister, contentAuth)
 	requiredGroupsAuth = authz.NewDecorator("01-requiredgroups", requiredGroupsAuth).AddAuditLogging().AddAnonymization()
 
-	authorizers = append(authorizers, requiredGroupsAuth)
+	// The kcp authorizer chain and the webhook form a union of themselves, so that a faulty webhook
+	// cannot prevent the health checks or system tasks from succeeding.
+	if webhookAuthorizer != nil {
+		authorizers = append(authorizers, union.New(webhookAuthorizer, requiredGroupsAuth))
+	} else {
+		authorizers = append(authorizers, requiredGroupsAuth)
+	}
 
 	config.RuleResolver = union.NewRuleResolvers(bootstrapRules, localResolver)
 	config.Authorization.Authorizer = union.New(authorizers...)