Merge pull request kubernetes#73807 from dekkagaijin/discovery-hardening

harden the default RBAC discovery clusterrolebindings

Author: k8s-ci-robot

pkg/registry/rbac/rest/storage_rbac.go

@@ -114,11 +114,12 @@ func (p RESTStorageProvider) storage(version schema.GroupVersion, apiResourceCon
 
 func (p RESTStorageProvider) PostStartHook() (string, genericapiserver.PostStartHookFunc, error) {
 	policy := &PolicyData{
-		ClusterRoles:            append(bootstrappolicy.ClusterRoles(), bootstrappolicy.ControllerRoles()...),
-		ClusterRoleBindings:     append(bootstrappolicy.ClusterRoleBindings(), bootstrappolicy.ControllerRoleBindings()...),
-		Roles:                   bootstrappolicy.NamespaceRoles(),
-		RoleBindings:            bootstrappolicy.NamespaceRoleBindings(),
-		ClusterRolesToAggregate: bootstrappolicy.ClusterRolesToAggregate(),
+		ClusterRoles:               append(bootstrappolicy.ClusterRoles(), bootstrappolicy.ControllerRoles()...),
+		ClusterRoleBindings:        append(bootstrappolicy.ClusterRoleBindings(), bootstrappolicy.ControllerRoleBindings()...),
+		Roles:                      bootstrappolicy.NamespaceRoles(),
+		RoleBindings:               bootstrappolicy.NamespaceRoleBindings(),
+		ClusterRolesToAggregate:    bootstrappolicy.ClusterRolesToAggregate(),
+		ClusterRoleBindingsToSplit: bootstrappolicy.ClusterRoleBindingsToSplit(),
 	}
 	return PostStartHookName, policy.EnsureRBACPolicy(), nil
 }
@@ -130,6 +131,8 @@ type PolicyData struct {
 	RoleBindings        map[string][]rbacapiv1.RoleBinding
 	// ClusterRolesToAggregate maps from previous clusterrole name to the new clusterrole name
 	ClusterRolesToAggregate map[string]string
+	// ClusterRoleBindingsToSplit maps from previous ClusterRoleBinding Name to a template for the new ClusterRoleBinding
+	ClusterRoleBindingsToSplit map[string]rbacapiv1.ClusterRoleBinding
 }
 
 func (p *PolicyData) EnsureRBACPolicy() genericapiserver.PostStartHookFunc {
@@ -166,6 +169,11 @@ func (p *PolicyData) EnsureRBACPolicy() genericapiserver.PostStartHookFunc {
 				return false, nil
 			}
 
+			if err := primeSplitClusterRoleBindings(p.ClusterRoleBindingsToSplit, clientset); err != nil {
+				utilruntime.HandleError(fmt.Errorf("unable to prime split ClusterRoleBindings: %v", err))
+				return false, nil
+			}
+
 			// ensure bootstrap roles are created or reconciled
 			for _, clusterRole := range p.ClusterRoles {
 				opts := reconciliation.ReconcileRoleOptions{
@@ -334,3 +342,40 @@ func primeAggregatedClusterRoles(clusterRolesToAggregate map[string]string, clus
 
 	return nil
 }
+
+// primeSplitClusterRoleBindings ensures the existence of target ClusterRoleBindings
+// by copying Subjects, Annotations, and Labels from the specified source
+// ClusterRoleBinding, if present.
+func primeSplitClusterRoleBindings(clusterRoleBindingToSplit map[string]rbacapiv1.ClusterRoleBinding, clusterRoleBindingClient rbacv1client.ClusterRoleBindingsGetter) error {
+	for existingBindingName, clusterRoleBindingToCreate := range clusterRoleBindingToSplit {
+		// If source ClusterRoleBinding does not exist, do nothing.
+		existingRoleBinding, err := clusterRoleBindingClient.ClusterRoleBindings().Get(existingBindingName, metav1.GetOptions{})
+		if apierrors.IsNotFound(err) {
+			continue
+		}
+		if err != nil {
+			return err
+		}
+
+		// If the target ClusterRoleBinding already exists, do nothing.
+		_, err = clusterRoleBindingClient.ClusterRoleBindings().Get(clusterRoleBindingToCreate.Name, metav1.GetOptions{})
+		if err == nil {
+			continue
+		}
+		if !apierrors.IsNotFound(err) {
+			return err
+		}
+
+		// If the source exists, but the target does not,
+		// copy the subjects, labels, and annotations from the former to create the latter.
+		klog.V(1).Infof("copying subjects, labels, and annotations from ClusterRoleBinding %q to template %q", existingBindingName, clusterRoleBindingToCreate.Name)
+		newCRB := clusterRoleBindingToCreate.DeepCopy()
+		newCRB.Subjects = existingRoleBinding.Subjects
+		newCRB.Labels = existingRoleBinding.Labels
+		newCRB.Annotations = existingRoleBinding.Annotations
+		if _, err := clusterRoleBindingClient.ClusterRoleBindings().Create(newCRB); err != nil && !apierrors.IsAlreadyExists(err) {
+			return err
+		}
+	}
+	return nil
+}

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go

@@ -213,7 +213,15 @@ func ClusterRoles() []rbacv1.ClusterRole {
 				rbacv1helpers.NewRule("create").Groups(authorizationGroup).Resources("selfsubjectaccessreviews", "selfsubjectrulesreviews").RuleOrDie(),
 			},
 		},
-
+		{
+			// a role which provides just enough power read insensitive cluster information
+			ObjectMeta: metav1.ObjectMeta{Name: "system:public-info-viewer"},
+			Rules: []rbacv1.PolicyRule{
+				rbacv1helpers.NewRule("get").URLs(
+					"/healthz", "/version", "/version/",
+				).RuleOrDie(),
+			},
+		},
 		{
 			// a role for a namespace level admin.  It is `edit` plus the power to grant permissions to other users.
 			ObjectMeta: metav1.ObjectMeta{Name: "admin"},
@@ -524,8 +532,9 @@ const systemNodeRoleName = "system:node"
 func ClusterRoleBindings() []rbacv1.ClusterRoleBinding {
 	rolebindings := []rbacv1.ClusterRoleBinding{
 		rbacv1helpers.NewClusterBinding("cluster-admin").Groups(user.SystemPrivilegedGroup).BindingOrDie(),
-		rbacv1helpers.NewClusterBinding("system:discovery").Groups(user.AllAuthenticated, user.AllUnauthenticated).BindingOrDie(),
-		rbacv1helpers.NewClusterBinding("system:basic-user").Groups(user.AllAuthenticated, user.AllUnauthenticated).BindingOrDie(),
+		rbacv1helpers.NewClusterBinding("system:discovery").Groups(user.AllAuthenticated).BindingOrDie(),
+		rbacv1helpers.NewClusterBinding("system:basic-user").Groups(user.AllAuthenticated).BindingOrDie(),
+		rbacv1helpers.NewClusterBinding("system:public-info-viewer").Groups(user.AllAuthenticated, user.AllUnauthenticated).BindingOrDie(),
 		rbacv1helpers.NewClusterBinding("system:node-proxier").Users(user.KubeProxy).BindingOrDie(),
 		rbacv1helpers.NewClusterBinding("system:kube-controller-manager").Users(user.KubeControllerManager).BindingOrDie(),
 		rbacv1helpers.NewClusterBinding("system:kube-dns").SAs("kube-system", "kube-dns").BindingOrDie(),
@@ -553,3 +562,16 @@ func ClusterRolesToAggregate() map[string]string {
 		"view":  "system:aggregate-to-view",
 	}
 }
+
+// ClusterRoleBindingsToSplit returns a map of Names of source ClusterRoleBindings
+// to copy Subjects, Annotations, and Labels to destination ClusterRoleBinding templates.
+func ClusterRoleBindingsToSplit() map[string]rbacv1.ClusterRoleBinding {
+	bindingsToSplit := map[string]rbacv1.ClusterRoleBinding{}
+	for _, defaultClusterRoleBinding := range ClusterRoleBindings() {
+		switch defaultClusterRoleBinding.Name {
+		case "system:public-info-viewer":
+			bindingsToSplit["system:discovery"] = defaultClusterRoleBinding
+		}
+	}
+	return bindingsToSplit
+}

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-role-bindings.yaml

@@ -51,9 +51,6 @@ items:
   - apiGroup: rbac.authorization.k8s.io
     kind: Group
     name: system:authenticated
-  - apiGroup: rbac.authorization.k8s.io
-    kind: Group
-    name: system:unauthenticated
 - apiVersion: rbac.authorization.k8s.io/v1
   kind: ClusterRoleBinding
   metadata:
@@ -71,9 +68,6 @@ items:
   - apiGroup: rbac.authorization.k8s.io
     kind: Group
     name: system:authenticated
-  - apiGroup: rbac.authorization.k8s.io
-    kind: Group
-    name: system:unauthenticated
 - apiVersion: rbac.authorization.k8s.io/v1
   kind: ClusterRoleBinding
   metadata:
@@ -155,6 +149,26 @@ items:
   - apiGroup: rbac.authorization.k8s.io
     kind: User
     name: system:kube-proxy
+- apiVersion: rbac.authorization.k8s.io/v1
+  kind: ClusterRoleBinding
+  metadata:
+    annotations:
+      rbac.authorization.kubernetes.io/autoupdate: "true"
+    creationTimestamp: null
+    labels:
+      kubernetes.io/bootstrapping: rbac-defaults
+    name: system:public-info-viewer
+  roleRef:
+    apiGroup: rbac.authorization.k8s.io
+    kind: ClusterRole
+    name: system:public-info-viewer
+  subjects:
+  - apiGroup: rbac.authorization.k8s.io
+    kind: Group
+    name: system:authenticated
+  - apiGroup: rbac.authorization.k8s.io
+    kind: Group
+    name: system:unauthenticated
 - apiVersion: rbac.authorization.k8s.io/v1
   kind: ClusterRoleBinding
   metadata:

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-roles.yaml

@@ -1127,6 +1127,22 @@ items:
     - create
     - patch
     - update
+- apiVersion: rbac.authorization.k8s.io/v1
+  kind: ClusterRole
+  metadata:
+    annotations:
+      rbac.authorization.kubernetes.io/autoupdate: "true"
+    creationTimestamp: null
+    labels:
+      kubernetes.io/bootstrapping: rbac-defaults
+    name: system:public-info-viewer
+  rules:
+  - nonResourceURLs:
+    - /healthz
+    - /version
+    - /version/
+    verbs:
+    - get
 - apiVersion: rbac.authorization.k8s.io/v1
   kind: ClusterRole
   metadata:

test/integration/auth/rbac_test.go

@@ -23,6 +23,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/http/httputil"
+	"reflect"
 	"strings"
 	"testing"
 	"time"
@@ -680,3 +681,91 @@ func TestBootstrapping(t *testing.T) {
 	}
 	t.Errorf("error bootstrapping roles: %s", string(healthBytes))
 }
+
+// TestDiscoveryUpgradeBootstrapping is primarily meant to test the behavior of
+// primePublicInfoClusterRoleBinding in storage_rbac.go during cluster upgrades.
+func TestDiscoveryUpgradeBootstrapping(t *testing.T) {
+	var tearDownFn func()
+	defer func() {
+		if tearDownFn != nil {
+			tearDownFn()
+		}
+	}()
+
+	superUser := "admin/system:masters"
+
+	masterConfig := framework.NewIntegrationTestMasterConfig()
+	masterConfig.GenericConfig.Authorization.Authorizer = newRBACAuthorizer(masterConfig)
+	masterConfig.GenericConfig.Authentication.Authenticator = bearertoken.New(tokenfile.New(map[string]*user.DefaultInfo{
+		superUser: {Name: "admin", Groups: []string{"system:masters"}},
+	}))
+	_, s, tearDownFn := framework.RunAMaster(masterConfig)
+
+	client := clientset.NewForConfigOrDie(&restclient.Config{BearerToken: superUser, Host: s.URL, ContentConfig: restclient.ContentConfig{GroupVersion: testapi.Groups[api.GroupName].GroupVersion()}})
+
+	// Modify the default RBAC discovery ClusterRoleBidnings to look more like the defaults that
+	// existed prior to v1.14, but with user modifications.
+	t.Logf("Modifying default `system:discovery` ClusterRoleBinding")
+	discRoleBinding, err := client.Rbac().ClusterRoleBindings().Get("system:discovery", metav1.GetOptions{})
+	discRoleBinding.Annotations["rbac.authorization.kubernetes.io/autoupdate"] = "false"
+	discRoleBinding.Annotations["rbac-discovery-upgrade-test"] = "pass"
+	discRoleBinding.Subjects = []rbacapi.Subject{
+		{
+			Name:     "system:authenticated",
+			Kind:     "Group",
+			APIGroup: "rbac.authorization.k8s.io",
+		},
+	}
+	if discRoleBinding, err = client.Rbac().ClusterRoleBindings().Update(discRoleBinding); err != nil {
+		t.Fatalf("Failed to update `system:discovery` ClusterRoleBinding: %v", err)
+	}
+	t.Logf("Modifying default `system:basic-user` ClusterRoleBinding")
+	basicUserRoleBinding, err := client.Rbac().ClusterRoleBindings().Get("system:basic-user", metav1.GetOptions{})
+	basicUserRoleBinding.Annotations["rbac.authorization.kubernetes.io/autoupdate"] = "false"
+	basicUserRoleBinding.Annotations["rbac-discovery-upgrade-test"] = "pass"
+	if basicUserRoleBinding, err = client.Rbac().ClusterRoleBindings().Update(basicUserRoleBinding); err != nil {
+		t.Fatalf("Failed to update `system:basic-user` ClusterRoleBinding: %v", err)
+	}
+	t.Logf("Deleting default `system:public-info-viewer` ClusterRoleBinding")
+	if err = client.Rbac().ClusterRoleBindings().Delete("system:public-info-viewer", &metav1.DeleteOptions{}); err != nil {
+		t.Fatalf("Failed to delete `system:public-info-viewer` ClusterRoleBinding: %v", err)
+	}
+
+	// Stop the first API server.
+	tearDownFn()
+	tearDownFn = nil
+
+	// Check that upgraded API servers inherit `system:public-info-viewer` settings from
+	// `system:discovery`, and respect auto-reconciliation annotations.
+	_, s, tearDownFn = framework.RunAMaster(masterConfig)
+
+	client = clientset.NewForConfigOrDie(&restclient.Config{BearerToken: superUser, Host: s.URL, ContentConfig: restclient.ContentConfig{GroupVersion: testapi.Groups[api.GroupName].GroupVersion()}})
+
+	newDiscRoleBinding, err := client.Rbac().ClusterRoleBindings().Get("system:discovery", metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Failed to get `system:discovery` ClusterRoleBinding: %v", err)
+	}
+	if !reflect.DeepEqual(newDiscRoleBinding, discRoleBinding) {
+		t.Errorf("`system:discovery` should have been unmodified. Wanted: %v, got %v", discRoleBinding, newDiscRoleBinding)
+	}
+	newBasicUserRoleBinding, err := client.Rbac().ClusterRoleBindings().Get("system:basic-user", metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Failed to get `system:basic-user` ClusterRoleBinding: %v", err)
+	}
+	if !reflect.DeepEqual(newBasicUserRoleBinding, basicUserRoleBinding) {
+		t.Errorf("`system:basic-user` should have been unmodified. Wanted: %v, got %v", basicUserRoleBinding, newBasicUserRoleBinding)
+	}
+	publicInfoViewerRoleBinding, err := client.Rbac().ClusterRoleBindings().Get("system:public-info-viewer", metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Failed to get `system:public-info-viewer` ClusterRoleBinding: %v", err)
+	}
+	if publicInfoViewerRoleBinding.Annotations["rbac.authorization.kubernetes.io/autoupdate"] != "false" {
+		t.Errorf("publicInfoViewerRoleBinding.Annotations[\"rbac.authorization.kubernetes.io/autoupdate\"] should be %v, got %v", publicInfoViewerRoleBinding.Annotations["rbac.authorization.kubernetes.io/autoupdate"], "false")
+	}
+	if publicInfoViewerRoleBinding.Annotations["rbac-discovery-upgrade-test"] != "pass" {
+		t.Errorf("publicInfoViewerRoleBinding.Annotations[\"rbac-discovery-upgrade-test\"] should be %v, got %v", publicInfoViewerRoleBinding.Annotations["rbac-discovery-upgrade-test"], "pass")
+	}
+	if !reflect.DeepEqual(publicInfoViewerRoleBinding.Subjects, newDiscRoleBinding.Subjects) {
+		t.Errorf("`system:public-info-viewer` should have inherited Subjects from `system:discovery` Wanted: %v, got %v", newDiscRoleBinding.Subjects, publicInfoViewerRoleBinding.Subjects)
+	}
+}