From 9c7d31928d26684bdcafdb801cbcdff0fc77171b Mon Sep 17 00:00:00 2001 From: Jake Sanders Date: Wed, 6 Feb 2019 20:25:01 -0800 Subject: [PATCH] harden the default RBAC discovery clusterrolebindings --- pkg/registry/rbac/rest/storage_rbac.go | 55 ++++++++++-- .../authorizer/rbac/bootstrappolicy/policy.go | 28 +++++- .../testdata/cluster-role-bindings.yaml | 26 ++++-- .../testdata/cluster-roles.yaml | 16 ++++ test/integration/auth/rbac_test.go | 89 +++++++++++++++++++ 5 files changed, 200 insertions(+), 14 deletions(-) diff --git a/pkg/registry/rbac/rest/storage_rbac.go b/pkg/registry/rbac/rest/storage_rbac.go index 1c0b500f9d..c3bf31e813 100644 --- a/pkg/registry/rbac/rest/storage_rbac.go +++ b/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 +} diff --git a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go index 6f5ebcac80..d9c6c7af30 100644 --- a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go +++ b/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"}, @@ -520,8 +528,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(), @@ -549,3 +558,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 +} diff --git a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-role-bindings.yaml b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-role-bindings.yaml index ab116df5fe..a41fcf20c7 100644 --- a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-role-bindings.yaml +++ b/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: diff --git a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-roles.yaml b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-roles.yaml index ea35a1b4b4..804af6a970 100644 --- a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-roles.yaml +++ b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-roles.yaml @@ -1109,6 +1109,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: diff --git a/test/integration/auth/rbac_test.go b/test/integration/auth/rbac_test.go index 0d8403c32a..e95e107f49 100644 --- a/test/integration/auth/rbac_test.go +++ b/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) + } +}