From c25921734005569667e6263a845bc6888ae452cc Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Fri, 25 Oct 2024 13:52:34 -0400 Subject: [PATCH] Improve error messaging for validating admission policy authz Kubernetes-commit: 1ad6fd7a0fa454cc3302b579dc73eb5c9afec49a --- .../policy/validating/caching_authorizer.go | 152 ----- .../validating/caching_authorizer_test.go | 523 ------------------ .../plugin/policy/validating/validator.go | 8 +- .../policy/validating/validator_test.go | 6 +- 4 files changed, 7 insertions(+), 682 deletions(-) delete mode 100644 pkg/admission/plugin/policy/validating/caching_authorizer.go delete mode 100644 pkg/admission/plugin/policy/validating/caching_authorizer_test.go diff --git a/pkg/admission/plugin/policy/validating/caching_authorizer.go b/pkg/admission/plugin/policy/validating/caching_authorizer.go deleted file mode 100644 index ac13dbeee..000000000 --- a/pkg/admission/plugin/policy/validating/caching_authorizer.go +++ /dev/null @@ -1,152 +0,0 @@ -/* -Copyright 2023 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 validating - -import ( - "context" - "encoding/json" - "sort" - "strings" - - "k8s.io/apimachinery/pkg/fields" - "k8s.io/apimachinery/pkg/labels" - "k8s.io/apiserver/pkg/authentication/user" - "k8s.io/apiserver/pkg/authorization/authorizer" -) - -type authzResult struct { - authorized authorizer.Decision - reason string - err error -} - -type cachingAuthorizer struct { - authorizer authorizer.Authorizer - decisions map[string]authzResult -} - -func newCachingAuthorizer(in authorizer.Authorizer) authorizer.Authorizer { - return &cachingAuthorizer{ - authorizer: in, - decisions: make(map[string]authzResult), - } -} - -// The attribute accessors known to cache key construction. If this fails to compile, the cache -// implementation may need to be updated. -var _ authorizer.Attributes = (interface { - GetUser() user.Info - GetVerb() string - IsReadOnly() bool - GetNamespace() string - GetResource() string - GetSubresource() string - GetName() string - GetAPIGroup() string - GetAPIVersion() string - IsResourceRequest() bool - GetPath() string - GetFieldSelector() (fields.Requirements, error) - GetLabelSelector() (labels.Requirements, error) -})(nil) - -// The user info accessors known to cache key construction. If this fails to compile, the cache -// implementation may need to be updated. -var _ user.Info = (interface { - GetName() string - GetUID() string - GetGroups() []string - GetExtra() map[string][]string -})(nil) - -// Authorize returns an authorization decision by delegating to another Authorizer. If an equivalent -// check has already been performed, a cached result is returned. Not safe for concurrent use. -func (ca *cachingAuthorizer) Authorize(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) { - type SerializableAttributes struct { - authorizer.AttributesRecord - LabelSelector string - } - - serializableAttributes := SerializableAttributes{ - AttributesRecord: authorizer.AttributesRecord{ - Verb: a.GetVerb(), - Namespace: a.GetNamespace(), - APIGroup: a.GetAPIGroup(), - APIVersion: a.GetAPIVersion(), - Resource: a.GetResource(), - Subresource: a.GetSubresource(), - Name: a.GetName(), - ResourceRequest: a.IsResourceRequest(), - Path: a.GetPath(), - }, - } - // in the error case, we won't honor this field selector, so the cache doesn't need it. - if fieldSelector, err := a.GetFieldSelector(); len(fieldSelector) > 0 { - serializableAttributes.FieldSelectorRequirements, serializableAttributes.FieldSelectorParsingErr = fieldSelector, err - } - if labelSelector, _ := a.GetLabelSelector(); len(labelSelector) > 0 { - // the labels requirements have private elements so those don't help us serialize to a unique key - serializableAttributes.LabelSelector = labelSelector.String() - } - - if u := a.GetUser(); u != nil { - di := &user.DefaultInfo{ - Name: u.GetName(), - UID: u.GetUID(), - } - - // Differently-ordered groups or extras could cause otherwise-equivalent checks to - // have distinct cache keys. - if groups := u.GetGroups(); len(groups) > 0 { - di.Groups = make([]string, len(groups)) - copy(di.Groups, groups) - sort.Strings(di.Groups) - } - - if extra := u.GetExtra(); len(extra) > 0 { - di.Extra = make(map[string][]string, len(extra)) - for k, vs := range extra { - vdupe := make([]string, len(vs)) - copy(vdupe, vs) - sort.Strings(vdupe) - di.Extra[k] = vdupe - } - } - - serializableAttributes.User = di - } - - var b strings.Builder - if err := json.NewEncoder(&b).Encode(serializableAttributes); err != nil { - return authorizer.DecisionNoOpinion, "", err - } - key := b.String() - - if cached, ok := ca.decisions[key]; ok { - return cached.authorized, cached.reason, cached.err - } - - authorized, reason, err := ca.authorizer.Authorize(ctx, a) - - ca.decisions[key] = authzResult{ - authorized: authorized, - reason: reason, - err: err, - } - - return authorized, reason, err -} diff --git a/pkg/admission/plugin/policy/validating/caching_authorizer_test.go b/pkg/admission/plugin/policy/validating/caching_authorizer_test.go deleted file mode 100644 index 5831cc3fe..000000000 --- a/pkg/admission/plugin/policy/validating/caching_authorizer_test.go +++ /dev/null @@ -1,523 +0,0 @@ -/* -Copyright 2023 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 validating - -import ( - "context" - "errors" - "fmt" - "testing" - - "k8s.io/apimachinery/pkg/fields" - "k8s.io/apimachinery/pkg/labels" - "k8s.io/apiserver/pkg/authentication/user" - "k8s.io/apiserver/pkg/authorization/authorizer" - genericfeatures "k8s.io/apiserver/pkg/features" - utilfeature "k8s.io/apiserver/pkg/util/feature" - featuregatetesting "k8s.io/component-base/featuregate/testing" -) - -func mustParseLabelSelector(str string) labels.Requirements { - ret, err := labels.Parse(str) - if err != nil { - panic(err) - } - retRequirements, _ /*selectable*/ := ret.Requirements() - return retRequirements -} - -func TestCachingAuthorizer(t *testing.T) { - featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, genericfeatures.AuthorizeWithSelectors, true) - - type result struct { - decision authorizer.Decision - reason string - error error - } - - type invocation struct { - attributes authorizer.Attributes - expected result - } - - for _, tc := range []struct { - name string - calls []invocation - backend []result - }{ - { - name: "hit", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{Name: "test name"}, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - attributes: authorizer.AttributesRecord{Name: "test name"}, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - { - name: "hit with differently-ordered groups", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - User: &user.DefaultInfo{ - Groups: []string{"a", "b", "c"}, - }, - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - attributes: authorizer.AttributesRecord{ - User: &user.DefaultInfo{ - Groups: []string{"c", "b", "a"}, - }, - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - { - name: "hit with differently-ordered extra", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - User: &user.DefaultInfo{ - Extra: map[string][]string{ - "k": {"a", "b", "c"}, - }, - }, - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - attributes: authorizer.AttributesRecord{ - User: &user.DefaultInfo{ - Extra: map[string][]string{ - "k": {"c", "b", "a"}, - }, - }, - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - { - name: "miss due to different name", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{Name: "alpha"}, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason alpha", - error: fmt.Errorf("test error alpha"), - }, - }, - { - attributes: authorizer.AttributesRecord{Name: "beta"}, - expected: result{ - decision: authorizer.DecisionDeny, - reason: "test reason beta", - error: fmt.Errorf("test error beta"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason alpha", - error: fmt.Errorf("test error alpha"), - }, - { - decision: authorizer.DecisionDeny, - reason: "test reason beta", - error: fmt.Errorf("test error beta"), - }, - }, - }, - { - name: "miss due to different user", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - User: &user.DefaultInfo{Name: "alpha"}, - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason alpha", - error: fmt.Errorf("test error alpha"), - }, - }, - { - attributes: authorizer.AttributesRecord{ - User: &user.DefaultInfo{Name: "beta"}, - }, - expected: result{ - decision: authorizer.DecisionDeny, - reason: "test reason beta", - error: fmt.Errorf("test error beta"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason alpha", - error: fmt.Errorf("test error alpha"), - }, - { - decision: authorizer.DecisionDeny, - reason: "test reason beta", - error: fmt.Errorf("test error beta"), - }, - }, - }, - { - name: "honor good field selector", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - FieldSelectorRequirements: fields.ParseSelectorOrDie("foo=bar").Requirements(), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason 2", - error: fmt.Errorf("test error 2"), - }, - }, - { - // now this should be cached - attributes: authorizer.AttributesRecord{ - Name: "test name", - FieldSelectorRequirements: fields.ParseSelectorOrDie("foo=bar").Requirements(), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - { - decision: authorizer.DecisionAllow, - reason: "test reason 2", - error: fmt.Errorf("test error 2"), - }, - }, - }, - { - name: "ignore malformed field selector first", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - FieldSelectorParsingErr: errors.New("malformed"), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - // notice that this does not have the malformed field selector. - // it should use the cached result - attributes: authorizer.AttributesRecord{ - Name: "test name", - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - { - name: "ignore malformed field selector second", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - // this should use the broader cached value because the selector will be ignored - attributes: authorizer.AttributesRecord{ - Name: "test name", - FieldSelectorParsingErr: errors.New("malformed"), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - - { - name: "honor good label selector", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - LabelSelectorRequirements: mustParseLabelSelector("foo=bar"), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason 2", - error: fmt.Errorf("test error 2"), - }, - }, - { - // now this should be cached - attributes: authorizer.AttributesRecord{ - Name: "test name", - LabelSelectorRequirements: mustParseLabelSelector("foo=bar"), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - LabelSelectorRequirements: mustParseLabelSelector("diff=zero"), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason 3", - error: fmt.Errorf("test error 3"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - { - decision: authorizer.DecisionAllow, - reason: "test reason 2", - error: fmt.Errorf("test error 2"), - }, - { - decision: authorizer.DecisionAllow, - reason: "test reason 3", - error: fmt.Errorf("test error 3"), - }, - }, - }, - { - name: "ignore malformed label selector first", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - LabelSelectorParsingErr: errors.New("malformed mess"), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - // notice that this does not have the malformed field selector. - // it should use the cached result - attributes: authorizer.AttributesRecord{ - Name: "test name", - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - { - name: "ignore malformed label selector second", - calls: []invocation{ - { - attributes: authorizer.AttributesRecord{ - Name: "test name", - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - { - // this should use the broader cached value because the selector will be ignored - attributes: authorizer.AttributesRecord{ - Name: "test name", - LabelSelectorParsingErr: errors.New("malformed mess"), - }, - expected: result{ - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - backend: []result{ - { - decision: authorizer.DecisionAllow, - reason: "test reason", - error: fmt.Errorf("test error"), - }, - }, - }, - } { - t.Run(tc.name, func(t *testing.T) { - var misses int - frontend := newCachingAuthorizer(func() authorizer.Authorizer { - return authorizer.AuthorizerFunc(func(_ context.Context, attributes authorizer.Attributes) (authorizer.Decision, string, error) { - if misses >= len(tc.backend) { - t.Fatalf("got more than expected %d backend invocations", len(tc.backend)) - } - result := tc.backend[misses] - misses++ - return result.decision, result.reason, result.error - }) - }()) - - for i, invocation := range tc.calls { - decision, reason, err := frontend.Authorize(context.TODO(), invocation.attributes) - if decision != invocation.expected.decision { - t.Errorf("(call %d of %d) expected decision %v, got %v", i+1, len(tc.calls), invocation.expected.decision, decision) - } - if reason != invocation.expected.reason { - t.Errorf("(call %d of %d) expected reason %q, got %q", i+1, len(tc.calls), invocation.expected.reason, reason) - } - if err.Error() != invocation.expected.error.Error() { - t.Errorf("(call %d of %d) expected error %q, got %q", i+1, len(tc.calls), invocation.expected.error.Error(), err.Error()) - } - } - - if len(tc.backend) > misses { - t.Errorf("expected %d backend invocations, got %d", len(tc.backend), misses) - } - }) - } -} diff --git a/pkg/admission/plugin/policy/validating/validator.go b/pkg/admission/plugin/policy/validating/validator.go index 5be9dde0f..4057e515e 100644 --- a/pkg/admission/plugin/policy/validating/validator.go +++ b/pkg/admission/plugin/policy/validating/validator.go @@ -41,13 +41,13 @@ import ( // validator implements the Validator interface type validator struct { celMatcher matchconditions.Matcher - validationFilter cel.Filter - auditAnnotationFilter cel.Filter - messageFilter cel.Filter + validationFilter cel.ConditionEvaluator + auditAnnotationFilter cel.ConditionEvaluator + messageFilter cel.ConditionEvaluator failPolicy *v1.FailurePolicyType } -func NewValidator(validationFilter cel.Filter, celMatcher matchconditions.Matcher, auditAnnotationFilter, messageFilter cel.Filter, failPolicy *v1.FailurePolicyType) Validator { +func NewValidator(validationFilter cel.ConditionEvaluator, celMatcher matchconditions.Matcher, auditAnnotationFilter, messageFilter cel.ConditionEvaluator, failPolicy *v1.FailurePolicyType) Validator { return &validator{ celMatcher: celMatcher, validationFilter: validationFilter, diff --git a/pkg/admission/plugin/policy/validating/validator_test.go b/pkg/admission/plugin/policy/validating/validator_test.go index dd1f49614..db75afd71 100644 --- a/pkg/admission/plugin/policy/validating/validator_test.go +++ b/pkg/admission/plugin/policy/validating/validator_test.go @@ -42,7 +42,7 @@ import ( "k8s.io/apiserver/pkg/cel/environment" ) -var _ cel.Filter = &fakeCelFilter{} +var _ cel.ConditionEvaluator = &fakeCelFilter{} type fakeCelFilter struct { evaluations []cel.EvaluationResult @@ -1035,8 +1035,8 @@ func TestContextCanceled(t *testing.T) { fakeAttr := admission.NewAttributesRecord(nil, nil, schema.GroupVersionKind{}, "default", "foo", schema.GroupVersionResource{}, "", admission.Create, nil, false, nil) fakeVersionedAttr, _ := admission.NewVersionedAttributes(fakeAttr, schema.GroupVersionKind{}, nil) - fc := cel.NewFilterCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true)) - f := fc.Compile([]cel.ExpressionAccessor{&ValidationCondition{Expression: "[1,2,3,4,5,6,7,8,9,10].map(x, [1,2,3,4,5,6,7,8,9,10].map(y, x*y)) == []"}}, cel.OptionalVariableDeclarations{HasParams: false, HasAuthorizer: false}, environment.StoredExpressions) + fc := cel.NewConditionCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), true)) + f := fc.CompileCondition([]cel.ExpressionAccessor{&ValidationCondition{Expression: "[1,2,3,4,5,6,7,8,9,10].map(x, [1,2,3,4,5,6,7,8,9,10].map(y, x*y)) == []"}}, cel.OptionalVariableDeclarations{HasParams: false, HasAuthorizer: false}, environment.StoredExpressions) v := validator{ failPolicy: &fail, celMatcher: &fakeCELMatcher{matches: true},