From 9fe2ef54ba81144b4e0863212ddc0367e2498325 Mon Sep 17 00:00:00 2001 From: xilabao Date: Fri, 26 May 2017 09:29:45 +0800 Subject: [PATCH] add policy_compact to rbac validation --- pkg/apis/rbac/helpers.go | 40 ++++ pkg/printers/internalversion/BUILD | 1 + pkg/registry/rbac/validation/BUILD | 6 + .../rbac/validation/policy_compact.go | 89 ++++++++ .../rbac/validation/policy_compact_test.go | 214 ++++++++++++++++++ .../rbac/validation/policy_comparator.go | 6 +- .../k8s.io/client-go/pkg/apis/rbac/helpers.go | 40 ++++ 7 files changed, 393 insertions(+), 3 deletions(-) create mode 100644 pkg/registry/rbac/validation/policy_compact.go create mode 100644 pkg/registry/rbac/validation/policy_compact_test.go diff --git a/pkg/apis/rbac/helpers.go b/pkg/apis/rbac/helpers.go index 830e227fd7..39db48ab9e 100644 --- a/pkg/apis/rbac/helpers.go +++ b/pkg/apis/rbac/helpers.go @@ -124,6 +124,38 @@ func SubjectsStrings(subjects []Subject) ([]string, []string, []string, []string return users, groups, sas, others } +func (r PolicyRule) String() string { + return "PolicyRule" + r.CompactString() +} + +// CompactString exposes a compact string representation for use in escalation error messages +func (r PolicyRule) CompactString() string { + formatStringParts := []string{} + formatArgs := []interface{}{} + if len(r.Resources) > 0 { + formatStringParts = append(formatStringParts, "Resources:%q") + formatArgs = append(formatArgs, r.Resources) + } + if len(r.NonResourceURLs) > 0 { + formatStringParts = append(formatStringParts, "NonResourceURLs:%q") + formatArgs = append(formatArgs, r.NonResourceURLs) + } + if len(r.ResourceNames) > 0 { + formatStringParts = append(formatStringParts, "ResourceNames:%q") + formatArgs = append(formatArgs, r.ResourceNames) + } + if len(r.APIGroups) > 0 { + formatStringParts = append(formatStringParts, "APIGroups:%q") + formatArgs = append(formatArgs, r.APIGroups) + } + if len(r.Verbs) > 0 { + formatStringParts = append(formatStringParts, "Verbs:%q") + formatArgs = append(formatArgs, r.Verbs) + } + formatString := "{" + strings.Join(formatStringParts, ", ") + "}" + return fmt.Sprintf(formatString, formatArgs...) +} + // +k8s:deepcopy-gen=false // PolicyRuleBuilder let's us attach methods. A no-no for API types. // We use it to construct rules in code. It's more compact than trying to write them @@ -341,3 +373,11 @@ func (r *RoleBindingBuilder) Binding() (RoleBinding, error) { return r.RoleBinding, nil } + +type SortableRuleSlice []PolicyRule + +func (s SortableRuleSlice) Len() int { return len(s) } +func (s SortableRuleSlice) Swap(i, j int) { s[i], s[j] = s[j], s[i] } +func (s SortableRuleSlice) Less(i, j int) bool { + return strings.Compare(s[i].String(), s[j].String()) < 0 +} diff --git a/pkg/printers/internalversion/BUILD b/pkg/printers/internalversion/BUILD index 05b07b4660..56460ec8ad 100644 --- a/pkg/printers/internalversion/BUILD +++ b/pkg/printers/internalversion/BUILD @@ -85,6 +85,7 @@ go_library( "//pkg/controller/deployment/util:go_default_library", "//pkg/fieldpath:go_default_library", "//pkg/printers:go_default_library", + "//pkg/registry/rbac/validation:go_default_library", "//pkg/util/node:go_default_library", "//pkg/util/slice:go_default_library", "//vendor/github.com/fatih/camelcase:go_default_library", diff --git a/pkg/registry/rbac/validation/BUILD b/pkg/registry/rbac/validation/BUILD index 415ed62656..2e97ba5a5c 100644 --- a/pkg/registry/rbac/validation/BUILD +++ b/pkg/registry/rbac/validation/BUILD @@ -11,14 +11,17 @@ load( go_test( name = "go_default_test", srcs = [ + "policy_compact_test.go", "policy_comparator_test.go", "rule_test.go", ], library = ":go_default_library", tags = ["automanaged"], deps = [ + "//pkg/api:go_default_library", "//pkg/apis/rbac:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/diff:go_default_library", "//vendor/k8s.io/apiserver/pkg/authentication/user:go_default_library", ], @@ -27,14 +30,17 @@ go_test( go_library( name = "go_default_library", srcs = [ + "policy_compact.go", "policy_comparator.go", "rule.go", ], tags = ["automanaged"], deps = [ + "//pkg/api:go_default_library", "//pkg/apis/rbac:go_default_library", "//vendor/github.com/golang/glog:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/errors:go_default_library", "//vendor/k8s.io/apiserver/pkg/authentication/serviceaccount:go_default_library", "//vendor/k8s.io/apiserver/pkg/authentication/user:go_default_library", diff --git a/pkg/registry/rbac/validation/policy_compact.go b/pkg/registry/rbac/validation/policy_compact.go new file mode 100644 index 0000000000..03e596b203 --- /dev/null +++ b/pkg/registry/rbac/validation/policy_compact.go @@ -0,0 +1,89 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "fmt" + "reflect" + + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/apis/rbac" +) + +// CompactRules combines rules that contain a single APIGroup/Resource, differ only by verb, and contain no other attributes. +// this is a fast check, and works well with the decomposed "missing rules" list from a Covers check. +func CompactRules(rules []rbac.PolicyRule) ([]rbac.PolicyRule, error) { + compacted := make([]rbac.PolicyRule, 0, len(rules)) + + simpleRules := map[schema.GroupResource]*rbac.PolicyRule{} + for _, rule := range rules { + if resource, isSimple := isSimpleResourceRule(&rule); isSimple { + if existingRule, ok := simpleRules[resource]; ok { + // Add the new verbs to the existing simple resource rule + if existingRule.Verbs == nil { + existingRule.Verbs = []string{} + } + existingRule.Verbs = append(existingRule.Verbs, rule.Verbs...) + } else { + // Copy the rule to accumulate matching simple resource rules into + objCopy, err := api.Scheme.DeepCopy(rule) + if err != nil { + // Unit tests ensure this should not ever happen + return nil, err + } + ruleCopy, ok := objCopy.(rbac.PolicyRule) + if !ok { + // Unit tests ensure this should not ever happen + return nil, fmt.Errorf("expected rbac.PolicyRule, got %#v", objCopy) + } + simpleRules[resource] = &ruleCopy + } + } else { + compacted = append(compacted, rule) + } + } + + // Once we've consolidated the simple resource rules, add them to the compacted list + for _, simpleRule := range simpleRules { + compacted = append(compacted, *simpleRule) + } + + return compacted, nil +} + +// isSimpleResourceRule returns true if the given rule contains verbs, a single resource, a single API group, and no other values +func isSimpleResourceRule(rule *rbac.PolicyRule) (schema.GroupResource, bool) { + resource := schema.GroupResource{} + + // If we have "complex" rule attributes, return early without allocations or expensive comparisons + if len(rule.ResourceNames) > 0 || len(rule.NonResourceURLs) > 0 { + return resource, false + } + // If we have multiple api groups or resources, return early + if len(rule.APIGroups) != 1 || len(rule.Resources) != 1 { + return resource, false + } + + // Test if this rule only contains APIGroups/Resources/Verbs + simpleRule := &rbac.PolicyRule{APIGroups: rule.APIGroups, Resources: rule.Resources, Verbs: rule.Verbs} + if !reflect.DeepEqual(simpleRule, rule) { + return resource, false + } + resource = schema.GroupResource{Group: rule.APIGroups[0], Resource: rule.Resources[0]} + return resource, true +} diff --git a/pkg/registry/rbac/validation/policy_compact_test.go b/pkg/registry/rbac/validation/policy_compact_test.go new file mode 100644 index 0000000000..cebb749ced --- /dev/null +++ b/pkg/registry/rbac/validation/policy_compact_test.go @@ -0,0 +1,214 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package validation + +import ( + "reflect" + "sort" + "testing" + + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/apis/rbac" +) + +func TestCompactRules(t *testing.T) { + testcases := map[string]struct { + Rules []rbac.PolicyRule + Expected []rbac.PolicyRule + }{ + "empty": { + Rules: []rbac.PolicyRule{}, + Expected: []rbac.PolicyRule{}, + }, + "simple": { + Rules: []rbac.PolicyRule{ + {Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}}, + {Verbs: []string{"list"}, APIGroups: []string{""}, Resources: []string{"builds"}}, + {Verbs: []string{"update", "patch"}, APIGroups: []string{""}, Resources: []string{"builds"}}, + + {Verbs: []string{"create"}, APIGroups: []string{"extensions"}, Resources: []string{"daemonsets"}}, + {Verbs: []string{"delete"}, APIGroups: []string{"extensions"}, Resources: []string{"daemonsets"}}, + + {Verbs: []string{"educate"}, APIGroups: []string{""}, Resources: []string{"dolphins"}}, + + // nil verbs are preserved in non-merge cases. + // these are the pirates who don't do anything. + {Verbs: nil, APIGroups: []string{""}, Resources: []string{"pirates"}}, + + // Test merging into a nil Verbs string set + {Verbs: nil, APIGroups: []string{""}, Resources: []string{"pods"}}, + {Verbs: []string{"create"}, APIGroups: []string{""}, Resources: []string{"pods"}}, + }, + Expected: []rbac.PolicyRule{ + {Verbs: []string{"create", "delete"}, APIGroups: []string{"extensions"}, Resources: []string{"daemonsets"}}, + {Verbs: []string{"get", "list", "update", "patch"}, APIGroups: []string{""}, Resources: []string{"builds"}}, + {Verbs: []string{"educate"}, APIGroups: []string{""}, Resources: []string{"dolphins"}}, + {Verbs: nil, APIGroups: []string{""}, Resources: []string{"pirates"}}, + {Verbs: []string{"create"}, APIGroups: []string{""}, Resources: []string{"pods"}}, + }, + }, + "complex multi-group": { + Rules: []rbac.PolicyRule{ + {Verbs: []string{"get"}, APIGroups: []string{"", "builds.openshift.io"}, Resources: []string{"builds"}}, + {Verbs: []string{"list"}, APIGroups: []string{"", "builds.openshift.io"}, Resources: []string{"builds"}}, + }, + Expected: []rbac.PolicyRule{ + {Verbs: []string{"get"}, APIGroups: []string{"", "builds.openshift.io"}, Resources: []string{"builds"}}, + {Verbs: []string{"list"}, APIGroups: []string{"", "builds.openshift.io"}, Resources: []string{"builds"}}, + }, + }, + + "complex multi-resource": { + Rules: []rbac.PolicyRule{ + {Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds", "images"}}, + {Verbs: []string{"list"}, APIGroups: []string{""}, Resources: []string{"builds", "images"}}, + }, + Expected: []rbac.PolicyRule{ + {Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds", "images"}}, + {Verbs: []string{"list"}, APIGroups: []string{""}, Resources: []string{"builds", "images"}}, + }, + }, + + "complex named-resource": { + Rules: []rbac.PolicyRule{ + {Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}, ResourceNames: []string{"mybuild"}}, + {Verbs: []string{"list"}, APIGroups: []string{""}, Resources: []string{"builds"}, ResourceNames: []string{"mybuild2"}}, + }, + Expected: []rbac.PolicyRule{ + {Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}, ResourceNames: []string{"mybuild"}}, + {Verbs: []string{"list"}, APIGroups: []string{""}, Resources: []string{"builds"}, ResourceNames: []string{"mybuild2"}}, + }, + }, + + "complex non-resource": { + Rules: []rbac.PolicyRule{ + {Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}, NonResourceURLs: []string{"/"}}, + {Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}, NonResourceURLs: []string{"/foo"}}, + }, + Expected: []rbac.PolicyRule{ + {Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}, NonResourceURLs: []string{"/"}}, + {Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}, NonResourceURLs: []string{"/foo"}}, + }, + }, + } + + for k, tc := range testcases { + rules := tc.Rules + originalRules, err := api.Scheme.DeepCopy(tc.Rules) + if err != nil { + t.Errorf("%s: couldn't copy rules: %v", k, err) + continue + } + compacted, err := CompactRules(tc.Rules) + if err != nil { + t.Errorf("%s: unexpected error: %v", k, err) + continue + } + if !reflect.DeepEqual(rules, originalRules) { + t.Errorf("%s: CompactRules mutated rules. Expected\n%#v\ngot\n%#v", k, originalRules, rules) + continue + } + if covers, missing := Covers(compacted, rules); !covers { + t.Errorf("%s: compacted rules did not cover original rules. missing: %#v", k, missing) + continue + } + if covers, missing := Covers(rules, compacted); !covers { + t.Errorf("%s: original rules did not cover compacted rules. missing: %#v", k, missing) + continue + } + + sort.Stable(rbac.SortableRuleSlice(compacted)) + sort.Stable(rbac.SortableRuleSlice(tc.Expected)) + if !reflect.DeepEqual(compacted, tc.Expected) { + t.Errorf("%s: Expected\n%#v\ngot\n%#v", k, tc.Expected, compacted) + continue + } + } +} + +func TestIsSimpleResourceRule(t *testing.T) { + testcases := map[string]struct { + Rule rbac.PolicyRule + Simple bool + Resource schema.GroupResource + }{ + "simple, no verbs": { + Rule: rbac.PolicyRule{Verbs: []string{}, APIGroups: []string{""}, Resources: []string{"builds"}}, + Simple: true, + Resource: schema.GroupResource{Group: "", Resource: "builds"}, + }, + "simple, one verb": { + Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}}, + Simple: true, + Resource: schema.GroupResource{Group: "", Resource: "builds"}, + }, + "simple, multi verb": { + Rule: rbac.PolicyRule{Verbs: []string{"get", "list"}, APIGroups: []string{""}, Resources: []string{"builds"}}, + Simple: true, + Resource: schema.GroupResource{Group: "", Resource: "builds"}, + }, + + "complex, empty": { + Rule: rbac.PolicyRule{}, + Simple: false, + Resource: schema.GroupResource{}, + }, + "complex, no group": { + Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{}, Resources: []string{"builds"}}, + Simple: false, + Resource: schema.GroupResource{}, + }, + "complex, multi group": { + Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{"a", "b"}, Resources: []string{"builds"}}, + Simple: false, + Resource: schema.GroupResource{}, + }, + "complex, no resource": { + Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{}}, + Simple: false, + Resource: schema.GroupResource{}, + }, + "complex, multi resource": { + Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds", "images"}}, + Simple: false, + Resource: schema.GroupResource{}, + }, + "complex, resource names": { + Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}, ResourceNames: []string{"foo"}}, + Simple: false, + Resource: schema.GroupResource{}, + }, + "complex, non-resource urls": { + Rule: rbac.PolicyRule{Verbs: []string{"get"}, APIGroups: []string{""}, Resources: []string{"builds"}, NonResourceURLs: []string{"/"}}, + Simple: false, + Resource: schema.GroupResource{}, + }, + } + + for k, tc := range testcases { + resource, simple := isSimpleResourceRule(&tc.Rule) + if simple != tc.Simple { + t.Errorf("%s: expected simple=%v, got simple=%v", k, tc.Simple, simple) + continue + } + if resource != tc.Resource { + t.Errorf("%s: expected resource=%v, got resource=%v", k, tc.Resource, resource) + continue + } + } +} diff --git a/pkg/registry/rbac/validation/policy_comparator.go b/pkg/registry/rbac/validation/policy_comparator.go index 5e4117fe14..6c69c24fb2 100644 --- a/pkg/registry/rbac/validation/policy_comparator.go +++ b/pkg/registry/rbac/validation/policy_comparator.go @@ -32,7 +32,7 @@ func Covers(ownerRules, servantRules []rbac.PolicyRule) (bool, []rbac.PolicyRule subrules := []rbac.PolicyRule{} for _, servantRule := range servantRules { - subrules = append(subrules, breakdownRule(servantRule)...) + subrules = append(subrules, BreakdownRule(servantRule)...) } uncoveredRules := []rbac.PolicyRule{} @@ -53,9 +53,9 @@ func Covers(ownerRules, servantRules []rbac.PolicyRule) (bool, []rbac.PolicyRule return (len(uncoveredRules) == 0), uncoveredRules } -// breadownRule takes a rule and builds an equivalent list of rules that each have at most one verb, one +// BreadownRule takes a rule and builds an equivalent list of rules that each have at most one verb, one // resource, and one resource name -func breakdownRule(rule rbac.PolicyRule) []rbac.PolicyRule { +func BreakdownRule(rule rbac.PolicyRule) []rbac.PolicyRule { subrules := []rbac.PolicyRule{} for _, group := range rule.APIGroups { for _, resource := range rule.Resources { diff --git a/staging/src/k8s.io/client-go/pkg/apis/rbac/helpers.go b/staging/src/k8s.io/client-go/pkg/apis/rbac/helpers.go index 7007a509a9..0ab368a349 100644 --- a/staging/src/k8s.io/client-go/pkg/apis/rbac/helpers.go +++ b/staging/src/k8s.io/client-go/pkg/apis/rbac/helpers.go @@ -124,6 +124,38 @@ func SubjectsStrings(subjects []Subject) ([]string, []string, []string, []string return users, groups, sas, others } +func (r PolicyRule) String() string { + return "PolicyRule" + r.CompactString() +} + +// CompactString exposes a compact string representation for use in escalation error messages +func (r PolicyRule) CompactString() string { + formatStringParts := []string{} + formatArgs := []interface{}{} + if len(r.Resources) > 0 { + formatStringParts = append(formatStringParts, "Resources:%q") + formatArgs = append(formatArgs, r.Resources) + } + if len(r.NonResourceURLs) > 0 { + formatStringParts = append(formatStringParts, "NonResourceURLs:%q") + formatArgs = append(formatArgs, r.NonResourceURLs) + } + if len(r.ResourceNames) > 0 { + formatStringParts = append(formatStringParts, "ResourceNames:%q") + formatArgs = append(formatArgs, r.ResourceNames) + } + if len(r.APIGroups) > 0 { + formatStringParts = append(formatStringParts, "APIGroups:%q") + formatArgs = append(formatArgs, r.APIGroups) + } + if len(r.Verbs) > 0 { + formatStringParts = append(formatStringParts, "Verbs:%q") + formatArgs = append(formatArgs, r.Verbs) + } + formatString := "{" + strings.Join(formatStringParts, ", ") + "}" + return fmt.Sprintf(formatString, formatArgs...) +} + // PolicyRuleBuilder let's us attach methods. A no-no for API types. // We use it to construct rules in code. It's more compact than trying to write them // out in a literal and allows us to perform some basic checking during construction @@ -338,3 +370,11 @@ func (r *RoleBindingBuilder) Binding() (RoleBinding, error) { return r.RoleBinding, nil } + +type SortableRuleSlice []PolicyRule + +func (s SortableRuleSlice) Len() int { return len(s) } +func (s SortableRuleSlice) Swap(i, j int) { s[i], s[j] = s[j], s[i] } +func (s SortableRuleSlice) Less(i, j int) bool { + return strings.Compare(s[i].String(), s[j].String()) < 0 +}