diff --git a/pkg/authorization/api/group_test.go b/pkg/authorization/api/group_test.go index 015e60948093..1b502c89d4d8 100644 --- a/pkg/authorization/api/group_test.go +++ b/pkg/authorization/api/group_test.go @@ -7,9 +7,60 @@ import ( ) func TestEscalating(t *testing.T) { - escalatingResources := ExpandResources(sets.NewString(GroupsToResources[EscalatingResourcesGroupName]...)) - nonEscalatingResources := ExpandResources(sets.NewString(GroupsToResources[NonEscalatingResourcesGroupName]...)) + escalatingResources := NormalizeResources(sets.NewString(GroupsToResources[EscalatingResourcesGroupName]...)) + nonEscalatingResources := NormalizeResources(sets.NewString(GroupsToResources[NonEscalatingResourcesGroupName]...)) if len(nonEscalatingResources) <= len(escalatingResources) { t.Errorf("groups look bad: escalating=%v nonescalating=%v", escalatingResources.List(), nonEscalatingResources.List()) } } + +func TestNormalizeResources(t *testing.T) { + tests := []struct { + name string + resource string + expected string + }{ + {"capA", "capA", "capa"}, + {"capH", "capH", "caph"}, + {"capZ", "capZ", "capz"}, + {"group", BuildGroupName, "builds"}, + } + + for _, test := range tests { + normalizedNames := NormalizeResources(sets.NewString(test.resource)) + + if !normalizedNames.Has(test.expected) { + t.Errorf("%s: expected %s, got %v", test.name, test.expected, normalizedNames) + } + + } +} + +func TestNeedsNormalization(t *testing.T) { + tests := []struct { + name string + resource string + expected bool + }{ + {"cap", "G", true}, + {"lowera", "lowera", false}, + {"lowerh", "lowerh", false}, + {"lowerz", "lowerz", false}, + {"0", "0", false}, + {"5", "5", false}, + {"9", "9", false}, + {"/", "/", false}, + {"-", "-", false}, + {".", ".", false}, + {ResourceGroupPrefix, ResourceGroupPrefix, true}, + } + + for _, test := range tests { + needsNormalizing := needsNormalizing(test.resource) + + if needsNormalizing != test.expected { + t.Errorf("%s: expected %v, got %v", test.name, test.expected, needsNormalizing) + } + + } +} diff --git a/pkg/authorization/api/helpers.go b/pkg/authorization/api/helpers.go index 4b2eaa2c34ed..935105b6ab2d 100644 --- a/pkg/authorization/api/helpers.go +++ b/pkg/authorization/api/helpers.go @@ -4,6 +4,7 @@ import ( "fmt" "sort" "strings" + "unicode" kapi "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/validation" @@ -13,7 +14,25 @@ import ( // uservalidation "github.com/openshift/origin/pkg/user/api/validation" ) -func ExpandResources(rawResources sets.String) sets.String { +// NormalizeResources expands all resource groups and forces all resources to lower case. +// If the rawResources are already normalized, it returns the original set to avoid the +// allocation and GC cost, since this is hit multiple times for every REST call. +// That means you should NEVER MODIFY THE RESULT of this call. +func NormalizeResources(rawResources sets.String) sets.String { + // we only need to expand groups if the exist and we don't create them with groups + // by default. Only accept the cost of expansion if we're doing work. + needsNormalization := false + for currResource := range rawResources { + if needsNormalizing(currResource) { + needsNormalization = true + break + } + + } + if !needsNormalization { + return rawResources + } + ret := sets.String{} toVisit := rawResources.List() visited := sets.String{} @@ -25,7 +44,7 @@ func ExpandResources(rawResources sets.String) sets.String { } visited.Insert(currResource) - if strings.Index(currResource, ResourceGroupPrefix+":") != 0 { + if !strings.HasPrefix(currResource, ResourceGroupPrefix) { ret.Insert(strings.ToLower(currResource)) continue } @@ -38,6 +57,18 @@ func ExpandResources(rawResources sets.String) sets.String { return ret } +func needsNormalizing(in string) bool { + if strings.HasPrefix(in, ResourceGroupPrefix) { + return true + } + for _, r := range in { + if unicode.IsUpper(r) { + return true + } + } + return false +} + func (r PolicyRule) String() string { return fmt.Sprintf("PolicyRule{Verbs:%v, APIGroups:%v, Resources:%v, ResourceNames:%v, Restrictions:%v}", r.Verbs.List(), r.APIGroups, r.Resources.List(), r.ResourceNames.List(), r.AttributeRestrictions) } diff --git a/pkg/authorization/api/types.go b/pkg/authorization/api/types.go index 33a48f566ccc..e555c665a829 100644 --- a/pkg/authorization/api/types.go +++ b/pkg/authorization/api/types.go @@ -33,43 +33,43 @@ const ( APIGroupExtensions = "extensions" // ResourceGroupPrefix is the prefix for indicating that a resource entry is actually a group of resources. The groups are defined in code and indicate resources that are commonly permissioned together - ResourceGroupPrefix = "resourcegroup" - BuildGroupName = ResourceGroupPrefix + ":builds" - DeploymentGroupName = ResourceGroupPrefix + ":deployments" - ImageGroupName = ResourceGroupPrefix + ":images" - OAuthGroupName = ResourceGroupPrefix + ":oauth" - UserGroupName = ResourceGroupPrefix + ":users" - TemplateGroupName = ResourceGroupPrefix + ":templates" - SDNGroupName = ResourceGroupPrefix + ":sdn" + ResourceGroupPrefix = "resourcegroup:" + BuildGroupName = ResourceGroupPrefix + "builds" + DeploymentGroupName = ResourceGroupPrefix + "deployments" + ImageGroupName = ResourceGroupPrefix + "images" + OAuthGroupName = ResourceGroupPrefix + "oauth" + UserGroupName = ResourceGroupPrefix + "users" + TemplateGroupName = ResourceGroupPrefix + "templates" + SDNGroupName = ResourceGroupPrefix + "sdn" // PolicyOwnerGroupName includes the physical resources behind the PermissionGrantingGroupName. Unless these physical objects are created first, users with privileges to PermissionGrantingGroupName will // only be able to bind to global roles - PolicyOwnerGroupName = ResourceGroupPrefix + ":policy" + PolicyOwnerGroupName = ResourceGroupPrefix + "policy" // PermissionGrantingGroupName includes resources that are necessary to maintain authorization roles and bindings. By itself, this group is insufficient to create anything except for bindings // to master roles. If a local Policy already exists, then privileges to this group will allow for modification of local roles. - PermissionGrantingGroupName = ResourceGroupPrefix + ":granter" + PermissionGrantingGroupName = ResourceGroupPrefix + "granter" // OpenshiftExposedGroupName includes resources that are commonly viewed and modified by end users of the system. It does not include any sensitive resources that control authentication or authorization - OpenshiftExposedGroupName = ResourceGroupPrefix + ":exposedopenshift" - OpenshiftAllGroupName = ResourceGroupPrefix + ":allopenshift" - OpenshiftStatusGroupName = ResourceGroupPrefix + ":allopenshift-status" + OpenshiftExposedGroupName = ResourceGroupPrefix + "exposedopenshift" + OpenshiftAllGroupName = ResourceGroupPrefix + "allopenshift" + OpenshiftStatusGroupName = ResourceGroupPrefix + "allopenshift-status" - QuotaGroupName = ResourceGroupPrefix + ":quota" + QuotaGroupName = ResourceGroupPrefix + "quota" // KubeInternalsGroupName includes those resources that should reasonably be viewable to end users, but that most users should probably not modify. Kubernetes herself will maintain these resources - KubeInternalsGroupName = ResourceGroupPrefix + ":privatekube" + KubeInternalsGroupName = ResourceGroupPrefix + "privatekube" // KubeExposedGroupName includes resources that are commonly viewed and modified by end users of the system. - KubeExposedGroupName = ResourceGroupPrefix + ":exposedkube" - KubeAllGroupName = ResourceGroupPrefix + ":allkube" - KubeStatusGroupName = ResourceGroupPrefix + ":allkube-status" + KubeExposedGroupName = ResourceGroupPrefix + "exposedkube" + KubeAllGroupName = ResourceGroupPrefix + "allkube" + KubeStatusGroupName = ResourceGroupPrefix + "allkube-status" // NonEscalatingResourcesGroupName contains all resources that can be viewed without exposing the risk of using view rights to locate a secret to escalate privileges. For example, view // rights on secrets could be used locate a secret that happened to be serviceaccount token that has more privileges - NonEscalatingResourcesGroupName = ResourceGroupPrefix + ":non-escalating" - KubeNonEscalatingViewableGroupName = ResourceGroupPrefix + ":kube-non-escalating" - OpenshiftNonEscalatingViewableGroupName = ResourceGroupPrefix + ":openshift-non-escalating" + NonEscalatingResourcesGroupName = ResourceGroupPrefix + "non-escalating" + KubeNonEscalatingViewableGroupName = ResourceGroupPrefix + "kube-non-escalating" + OpenshiftNonEscalatingViewableGroupName = ResourceGroupPrefix + "openshift-non-escalating" // EscalatingResourcesGroupName contains all resources that can be used to escalate privileges when simply viewed - EscalatingResourcesGroupName = ResourceGroupPrefix + ":escalating" - KubeEscalatingViewableGroupName = ResourceGroupPrefix + ":kube-escalating" - OpenshiftEscalatingViewableGroupName = ResourceGroupPrefix + ":openshift-escalating" + EscalatingResourcesGroupName = ResourceGroupPrefix + "escalating" + KubeEscalatingViewableGroupName = ResourceGroupPrefix + "kube-escalating" + OpenshiftEscalatingViewableGroupName = ResourceGroupPrefix + "openshift-escalating" ) var ( @@ -107,11 +107,11 @@ var ( func init() { // set the non-escalating groups - GroupsToResources[OpenshiftNonEscalatingViewableGroupName] = ExpandResources(sets.NewString(GroupsToResources[OpenshiftAllGroupName]...)). - Difference(ExpandResources(sets.NewString(GroupsToResources[OpenshiftEscalatingViewableGroupName]...))).List() + GroupsToResources[OpenshiftNonEscalatingViewableGroupName] = NormalizeResources(sets.NewString(GroupsToResources[OpenshiftAllGroupName]...)). + Difference(NormalizeResources(sets.NewString(GroupsToResources[OpenshiftEscalatingViewableGroupName]...))).List() - GroupsToResources[KubeNonEscalatingViewableGroupName] = ExpandResources(sets.NewString(GroupsToResources[KubeAllGroupName]...)). - Difference(ExpandResources(sets.NewString(GroupsToResources[KubeEscalatingViewableGroupName]...))).List() + GroupsToResources[KubeNonEscalatingViewableGroupName] = NormalizeResources(sets.NewString(GroupsToResources[KubeAllGroupName]...)). + Difference(NormalizeResources(sets.NewString(GroupsToResources[KubeEscalatingViewableGroupName]...))).List() } // PolicyRule holds information that describes a policy rule, but does not contain information diff --git a/pkg/authorization/authorizer/attributes.go b/pkg/authorization/authorizer/attributes.go index 27627ff462c5..27e2c7df9c5d 100644 --- a/pkg/authorization/authorizer/attributes.go +++ b/pkg/authorization/authorizer/attributes.go @@ -45,7 +45,7 @@ func (a DefaultAuthorizationAttributes) RuleMatches(rule authorizationapi.Policy if a.verbMatches(rule.Verbs) { if a.apiGroupMatches(rule.APIGroups) { - allowedResourceTypes := authorizationapi.ExpandResources(rule.Resources) + allowedResourceTypes := authorizationapi.NormalizeResources(rule.Resources) if a.resourceMatches(allowedResourceTypes) { if a.nameMatches(rule.ResourceNames) { // this rule matches the request, so we should check the additional restrictions to be sure that it's allowed diff --git a/pkg/authorization/rulevalidation/policy_comparator.go b/pkg/authorization/rulevalidation/policy_comparator.go index d53164daed53..6f3ddb087567 100644 --- a/pkg/authorization/rulevalidation/policy_comparator.go +++ b/pkg/authorization/rulevalidation/policy_comparator.go @@ -63,7 +63,7 @@ func breakdownRule(rule authorizationapi.PolicyRule) []authorizationapi.PolicyRu func breadownRuleForGroup(group string, rule authorizationapi.PolicyRule) []authorizationapi.PolicyRule { subrules := []authorizationapi.PolicyRule{} - for resource := range authorizationapi.ExpandResources(rule.Resources) { + for resource := range authorizationapi.NormalizeResources(rule.Resources) { for verb := range rule.Verbs { if len(rule.ResourceNames) > 0 { for _, resourceName := range rule.ResourceNames.List() { @@ -83,7 +83,7 @@ func breadownRuleForGroup(group string, rule authorizationapi.PolicyRule) []auth // ruleCovers determines whether the ownerRule (which may have multiple verbs, resources, and resourceNames) covers // the subrule (which may only contain at most one verb, resource, and resourceName) func ruleCovers(ownerRule, subrule authorizationapi.PolicyRule) bool { - allResources := authorizationapi.ExpandResources(ownerRule.Resources) + allResources := authorizationapi.NormalizeResources(ownerRule.Resources) ownerGroups := sets.NewString(ownerRule.APIGroups...) groupMatches := ownerGroups.Has(authorizationapi.APIGroupAll) || ownerGroups.HasAll(subrule.APIGroups...) || (len(ownerRule.APIGroups) == 0 && len(subrule.APIGroups) == 0) diff --git a/pkg/cmd/server/bootstrappolicy/policy.go b/pkg/cmd/server/bootstrappolicy/policy.go index 2a968a7eef3c..5131c029b808 100644 --- a/pkg/cmd/server/bootstrappolicy/policy.go +++ b/pkg/cmd/server/bootstrappolicy/policy.go @@ -35,7 +35,7 @@ func GetBootstrapOpenshiftRoles(openshiftNamespace string) []authorizationapi.Ro // our default roles and hard for them to reason about what power they are granting their users for i := range roles { for j := range roles[i].Rules { - roles[i].Rules[j].Resources = authorizationapi.ExpandResources(roles[i].Rules[j].Resources) + roles[i].Rules[j].Resources = authorizationapi.NormalizeResources(roles[i].Rules[j].Resources) } } @@ -560,7 +560,7 @@ func GetBootstrapClusterRoles() []authorizationapi.ClusterRole { // our default roles and hard for them to reason about what power they are granting their users for i := range roles { for j := range roles[i].Rules { - roles[i].Rules[j].Resources = authorizationapi.ExpandResources(roles[i].Rules[j].Resources) + roles[i].Rules[j].Resources = authorizationapi.NormalizeResources(roles[i].Rules[j].Resources) } } diff --git a/pkg/cmd/server/origin/reststorage_validation_test.go b/pkg/cmd/server/origin/reststorage_validation_test.go index 95762e2480c0..6edd74b0363e 100644 --- a/pkg/cmd/server/origin/reststorage_validation_test.go +++ b/pkg/cmd/server/origin/reststorage_validation_test.go @@ -49,7 +49,7 @@ func TestValidationRegistration(t *testing.T) { // TestAllOpenShiftResourceCoverage checks to make sure that the openshift all group actually contains all openshift resources func TestAllOpenShiftResourceCoverage(t *testing.T) { - allOpenshift := authorizationapi.ExpandResources(sets.NewString(authorizationapi.GroupsToResources[authorizationapi.OpenshiftAllGroupName]...)) + allOpenshift := authorizationapi.NormalizeResources(sets.NewString(authorizationapi.GroupsToResources[authorizationapi.OpenshiftAllGroupName]...)) config := fakeMasterConfig()