From fdd9bb5bddc81e8abd17608e650f95dfbc724f5b Mon Sep 17 00:00:00 2001 From: deads2k Date: Thu, 26 Feb 2015 13:44:49 -0500 Subject: [PATCH] let project admins use resource access review --- pkg/authorization/api/types.go | 2 +- .../authorizer/bootstrap_policy.go | 2 +- pkg/authorization/authorizer/subjects_test.go | 4 +- pkg/cmd/experimental/policy/add_user.go | 46 +++---- test/integration/authorization_test.go | 116 ++++++++++++++++++ 5 files changed, 143 insertions(+), 27 deletions(-) diff --git a/pkg/authorization/api/types.go b/pkg/authorization/api/types.go index c666b696cf60..c6a96caa5c3b 100644 --- a/pkg/authorization/api/types.go +++ b/pkg/authorization/api/types.go @@ -55,7 +55,7 @@ var ( UserGroupName: {"users", "useridentitymappings"}, OAuthGroupName: {"oauthauthorizetokens", "oauthaccesstokens", "oauthclients", "oauthclientauthorizations"}, PolicyOwnerGroupName: {"policies", "policybindings"}, - PermissionGrantingGroupName: {"roles", "rolebindings"}, + PermissionGrantingGroupName: {"roles", "rolebindings", "resourceaccessreviews", "subjectaccessreviews"}, OpenshiftExposedGroupName: {BuildGroupName, ImageGroupName, DeploymentGroupName, "templateconfigs", "routes", "projects"}, OpenshiftAllGroupName: {OpenshiftExposedGroupName, UserGroupName, OAuthGroupName, PolicyOwnerGroupName, PermissionGrantingGroupName}, diff --git a/pkg/authorization/authorizer/bootstrap_policy.go b/pkg/authorization/authorizer/bootstrap_policy.go index 039b2e87a798..0ea714879a0f 100644 --- a/pkg/authorization/authorizer/bootstrap_policy.go +++ b/pkg/authorization/authorizer/bootstrap_policy.go @@ -181,7 +181,7 @@ func GetBootstrapPolicyBinding(masterNamespace string) *authorizationapi.PolicyB Name: "cluster-admin", Namespace: masterNamespace, }, - Users: util.NewStringSet("system:admin"), + Groups: util.NewStringSet("system:cluster-admins"), }, "basic-user-binding": { ObjectMeta: kapi.ObjectMeta{ diff --git a/pkg/authorization/authorizer/subjects_test.go b/pkg/authorization/authorizer/subjects_test.go index 6e2e6530d4ba..59cec454ba58 100644 --- a/pkg/authorization/authorizer/subjects_test.go +++ b/pkg/authorization/authorizer/subjects_test.go @@ -31,8 +31,8 @@ func TestSubjects(t *testing.T) { Verb: "get", Resource: "pods", }, - expectedUsers: []string{"Anna", "ClusterAdmin", "Ellen", "Valerie", "system:admin", "system:kube-client", "system:openshift-client", "system:openshift-deployer"}, - expectedGroups: []string{"RootUsers", "system:authenticated", "system:unauthenticated"}, + expectedUsers: []string{"Anna", "ClusterAdmin", "Ellen", "Valerie", "system:kube-client", "system:openshift-client", "system:openshift-deployer"}, + expectedGroups: []string{"RootUsers", "system:authenticated", "system:cluster-admins", "system:unauthenticated"}, } test.policies = newDefaultGlobalPolicies() test.policies = append(test.policies, newAdzePolicies()...) diff --git a/pkg/cmd/experimental/policy/add_user.go b/pkg/cmd/experimental/policy/add_user.go index 38cb2a6c4d33..5f32055a5647 100644 --- a/pkg/cmd/experimental/policy/add_user.go +++ b/pkg/cmd/experimental/policy/add_user.go @@ -11,17 +11,17 @@ import ( "github.com/openshift/origin/pkg/cmd/util/clientcmd" ) -type addUserOptions struct { - roleNamespace string - roleName string - bindingNamespace string - client client.Interface +type AddUserOptions struct { + RoleNamespace string + RoleName string + BindingNamespace string + Client client.Interface - users []string + Users []string } func NewCmdAddUser(f *clientcmd.Factory) *cobra.Command { - options := &addUserOptions{} + options := &AddUserOptions{} cmd := &cobra.Command{ Use: "add-user [user]...", @@ -33,41 +33,41 @@ func NewCmdAddUser(f *clientcmd.Factory) *cobra.Command { } var err error - if options.client, _, err = f.Clients(cmd); err != nil { + if options.Client, _, err = f.Clients(cmd); err != nil { glog.Fatalf("Error getting client: %v", err) } - if options.bindingNamespace, err = f.DefaultNamespace(cmd); err != nil { + if options.BindingNamespace, err = f.DefaultNamespace(cmd); err != nil { glog.Fatalf("Error getting client: %v", err) } - if err := options.run(); err != nil { + if err := options.Run(); err != nil { glog.Fatal(err) } }, } - cmd.Flags().StringVar(&options.roleNamespace, "role-namespace", "master", "namespace where the role is located.") + cmd.Flags().StringVar(&options.RoleNamespace, "role-namespace", "master", "namespace where the role is located.") return cmd } -func (o *addUserOptions) complete(cmd *cobra.Command) bool { +func (o *AddUserOptions) complete(cmd *cobra.Command) bool { args := cmd.Flags().Args() if len(args) < 2 { cmd.Help() return false } - o.roleName = args[0] - o.users = args[1:] + o.RoleName = args[0] + o.Users = args[1:] return true } -func (o *addUserOptions) run() error { - roleBindings, err := getExistingRoleBindingsForRole(o.roleNamespace, o.roleName, o.client.PolicyBindings(o.bindingNamespace)) +func (o *AddUserOptions) Run() error { + roleBindings, err := getExistingRoleBindingsForRole(o.RoleNamespace, o.RoleName, o.Client.PolicyBindings(o.BindingNamespace)) if err != nil { return err } - roleBindingNames, err := getExistingRoleBindingNames(o.client.PolicyBindings(o.bindingNamespace)) + roleBindingNames, err := getExistingRoleBindingNames(o.Client.PolicyBindings(o.BindingNamespace)) if err != nil { return err } @@ -82,16 +82,16 @@ func (o *addUserOptions) run() error { roleBinding = roleBindings[0] } - roleBinding.RoleRef.Namespace = o.roleNamespace - roleBinding.RoleRef.Name = o.roleName + roleBinding.RoleRef.Namespace = o.RoleNamespace + roleBinding.RoleRef.Name = o.RoleName - roleBinding.Users.Insert(o.users...) + roleBinding.Users.Insert(o.Users...) if isUpdate { - _, err = o.client.RoleBindings(o.bindingNamespace).Update(roleBinding) + _, err = o.Client.RoleBindings(o.BindingNamespace).Update(roleBinding) } else { - roleBinding.Name = getUniqueName(o.roleName, roleBindingNames) - _, err = o.client.RoleBindings(o.bindingNamespace).Create(roleBinding) + roleBinding.Name = getUniqueName(o.RoleName, roleBindingNames) + _, err = o.Client.RoleBindings(o.BindingNamespace).Create(roleBinding) } if err != nil { return err diff --git a/test/integration/authorization_test.go b/test/integration/authorization_test.go index 30bf76d24c43..e26e12d987ce 100644 --- a/test/integration/authorization_test.go +++ b/test/integration/authorization_test.go @@ -3,12 +3,15 @@ package integration import ( + "reflect" "strings" "testing" "time" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" + "github.com/GoogleCloudPlatform/kubernetes/pkg/util" + authorizationapi "github.com/openshift/origin/pkg/authorization/api" policy "github.com/openshift/origin/pkg/cmd/experimental/policy" ) @@ -86,3 +89,116 @@ func TestRestrictedAccessForProjectAdmins(t *testing.T) { t.Errorf("expected mallet-project, got %#v", markProjects.Items) } } + +// TODO this list should start collapsing as we continue to tighten access on generated system ids +var globalClusterAdminUsers = util.NewStringSet("system:kube-client", "system:openshift-client", "system:openshift-deployer") +var globalClusterAdminGroups = util.NewStringSet("system:cluster-admins") + +func TestResourceAccessReview(t *testing.T) { + startConfig, err := StartTestMaster() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + openshiftClient, openshiftClientConfig, err := startConfig.GetOpenshiftClient() + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + // TODO remove once bootstrap authorization rules are tightened + removeInsecureOptions := &policy.RemoveGroupOptions{ + RoleNamespace: "master", + RoleName: "cluster-admin", + BindingNamespace: "master", + Client: openshiftClient, + Groups: []string{"system:authenticated", "system:unauthenticated"}, + } + if err := removeInsecureOptions.Run(); err != nil { + t.Errorf("unexpected error: %v", err) + } + + haroldClient, err := CreateNewProject(openshiftClient, *openshiftClientConfig, "hammer-project", "harold") + if err != nil { + t.Errorf("unexpected error: %v", err) + } + markClient, err := CreateNewProject(openshiftClient, *openshiftClientConfig, "mallet-project", "mark") + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + addValerie := &policy.AddUserOptions{ + RoleNamespace: "master", + RoleName: "view", + BindingNamespace: "hammer-project", + Client: haroldClient, + Users: []string{"anypassword:valerie"}, + } + if err := addValerie.Run(); err != nil { + t.Errorf("unexpected error: %v", err) + } + + addEdgar := &policy.AddUserOptions{ + RoleNamespace: "master", + RoleName: "edit", + BindingNamespace: "mallet-project", + Client: markClient, + Users: []string{"anypassword:edgar"}, + } + if err := addEdgar.Run(); err != nil { + t.Errorf("unexpected error: %v", err) + } + + requestWhoCanViewDeployments := &authorizationapi.ResourceAccessReview{Verb: "get", Resource: "deployments"} + + whoCanViewDeploymentInHammer, err := haroldClient.ResourceAccessReviews("hammer-project").Create(requestWhoCanViewDeployments) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + expectedHammerUsers := util.NewStringSet("anypassword:harold", "anypassword:valerie") + expectedHammerUsers.Insert(globalClusterAdminUsers.List()...) + expectedHammerGroups := globalClusterAdminGroups + actualHammerUsers := util.NewStringSet(whoCanViewDeploymentInHammer.Users...) + actualHammerGroups := util.NewStringSet(whoCanViewDeploymentInHammer.Groups...) + if !reflect.DeepEqual(expectedHammerGroups.List(), actualHammerGroups.List()) { + t.Errorf("expected %v, got %v", expectedHammerGroups.List(), actualHammerGroups.List()) + } + if !reflect.DeepEqual(expectedHammerUsers.List(), actualHammerUsers.List()) { + t.Errorf("expected %v, got %v", expectedHammerUsers.List(), actualHammerUsers.List()) + } + + whoCanViewDeploymentInMallet, err := markClient.ResourceAccessReviews("mallet-project").Create(requestWhoCanViewDeployments) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + expectedMalletUsers := util.NewStringSet("anypassword:mark", "anypassword:edgar") + expectedMalletUsers.Insert(globalClusterAdminUsers.List()...) + expectedMalletGroups := globalClusterAdminGroups + actualMalletUsers := util.NewStringSet(whoCanViewDeploymentInMallet.Users...) + actualMalletGroups := util.NewStringSet(whoCanViewDeploymentInMallet.Groups...) + if !reflect.DeepEqual(expectedMalletGroups.List(), actualMalletGroups.List()) { + t.Errorf("expected %v, got %v", expectedMalletGroups.List(), actualMalletGroups.List()) + } + if !reflect.DeepEqual(expectedMalletUsers.List(), actualMalletUsers.List()) { + t.Errorf("expected %v, got %v", expectedMalletUsers.List(), actualMalletUsers.List()) + } + + // mark should not be able to make global access review requests + _, err = markClient.ResourceAccessReviews("").Create(requestWhoCanViewDeployments) + if (err == nil) || (!strings.Contains(err.Error(), "Forbidden")) { + t.Errorf("expected forbidden error, but didn't get one") + } + + // a cluster-admin should be able to make global access review requests + whoCanViewDeploymentInAnyNamespace, err := openshiftClient.ResourceAccessReviews("").Create(requestWhoCanViewDeployments) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + actualAnyUsers := util.NewStringSet(whoCanViewDeploymentInAnyNamespace.Users...) + actualAnyGroups := util.NewStringSet(whoCanViewDeploymentInAnyNamespace.Groups...) + if !reflect.DeepEqual(globalClusterAdminGroups.List(), actualAnyGroups.List()) { + t.Errorf("expected %v, got %v", globalClusterAdminGroups.List(), actualAnyGroups.List()) + } + if !reflect.DeepEqual(globalClusterAdminUsers.List(), actualAnyUsers.List()) { + t.Errorf("expected %v, got %v", globalClusterAdminUsers.List(), actualAnyUsers.List()) + } +}