From d221ddb89a5dde5a6f55674dc38aa71cc842d481 Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Mon, 6 Mar 2023 17:29:28 -0500 Subject: [PATCH] Implement validationActions and auditAnnotations --- pkg/apis/admissionregistration/types.go | 132 +++- .../validation/validation.go | 92 ++- .../validation/validation_test.go | 153 ++++- .../storage/storage_test.go | 5 +- .../strategy_test.go | 2 + .../admissionregistration/v1alpha1/types.go | 128 +++- .../apiserver/pkg/admission/cel/metrics.go | 12 + .../pkg/admission/plugin/cel/compile.go | 19 +- .../pkg/admission/plugin/cel/compile_test.go | 89 ++- .../pkg/admission/plugin/cel/filter.go | 4 - .../pkg/admission/plugin/cel/filter_test.go | 14 +- .../pkg/admission/plugin/cel/interface.go | 7 + .../admission_test.go | 612 +++++++++++++++--- .../validatingadmissionpolicy/controller.go | 135 +++- .../controller_reconcile.go | 15 +- .../validatingadmissionpolicy/interface.go | 30 +- .../policy_decision.go | 23 + .../validatingadmissionpolicy/validator.go | 93 ++- .../validator_test.go | 164 ++++- 19 files changed, 1546 insertions(+), 183 deletions(-) diff --git a/pkg/apis/admissionregistration/types.go b/pkg/apis/admissionregistration/types.go index 6edaca1fdea..abf6b826c75 100644 --- a/pkg/apis/admissionregistration/types.go +++ b/pkg/apis/admissionregistration/types.go @@ -154,18 +154,35 @@ type ValidatingAdmissionPolicySpec struct { // Required. MatchConstraints *MatchResources - // Validations contain CEL expressions which is used to apply the validation. - // A minimum of one validation is required for a policy definition. - // Required. + // validations contain CEL expressions which are used to validate admission requests. + // validations and auditAnnotations may not both be empty; a minimum of one validations or auditAnnotations is + // required. + // +optional Validations []Validation - // FailurePolicy defines how to handle failures for the admission policy. - // Failures can occur from invalid or mis-configured policy definitions or bindings. + // failurePolicy defines how to handle failures for the admission policy. Failures can + // occur from CEL expression parse errors, type check errors, runtime errors and invalid + // or mis-configured policy definitions or bindings. + // // A policy is invalid if spec.paramKind refers to a non-existent Kind. // A binding is invalid if spec.paramRef.name refers to a non-existent resource. + // + // failurePolicy does not define how validations that evaluate to false are handled. + // + // When failurePolicy is set to Fail, ValidatingAdmissionPolicyBinding validationActions + // define how failures are enforced. + // // Allowed values are Ignore or Fail. Defaults to Fail. // +optional FailurePolicy *FailurePolicyType + + // auditAnnotations contains CEL expressions which are used to produce audit + // annotations for the audit event of the API request. + // validations and auditAnnotations may not both be empty; a least one of validations or auditAnnotations is + // required. + // A maximum of 20 auditAnnotation are allowed per ValidatingAdmissionPolicy. + // +optional + AuditAnnotations []AuditAnnotation } // ParamKind is a tuple of Group Kind and Version. @@ -184,12 +201,12 @@ type ParamKind struct { type Validation struct { // Expression represents the expression which will be evaluated by CEL. // ref: https://github.com/google/cel-spec - // CEL expressions have access to the contents of the Admission request/response, organized into CEL variables as well as some other useful variables: + // CEL expressions have access to the contents of the API request/response, organized into CEL variables as well as some other useful variables: // - // - 'object' - The object from the incoming request. The value is null for DELETE requests. - // - 'oldObject' - The existing object. The value is null for CREATE requests. - // - 'request' - Attributes of the admission request([ref](/pkg/apis/admission/types.go#AdmissionRequest)). - // - 'params' - Parameter resource referred to by the policy binding being evaluated. Only populated if the policy has a ParamKind. + //'object' - The object from the incoming request. The value is null for DELETE requests. + //'oldObject' - The existing object. The value is null for CREATE requests. + //'request' - Attributes of the API request([ref](/pkg/apis/admission/types.go#AdmissionRequest)). + //'params' - Parameter resource referred to by the policy binding being evaluated. Only populated if the policy has a ParamKind. // - 'authorizer' - A CEL Authorizer. May be used to perform authorization checks for the principal (user or service account) of the request. // See https://pkg.go.dev/k8s.io/apiserver/pkg/cel/library#Authz // - 'authorizer.requestResource' - A CEL ResourceCheck constructed from the 'authorizer' and configured with the @@ -241,6 +258,43 @@ type Validation struct { Reason *metav1.StatusReason } +// AuditAnnotation describes how to produce an audit annotation for an API request. +type AuditAnnotation struct { + // key specifies the audit annotation key. The audit annotation keys of + // a ValidatingAdmissionPolicy must be unique. The key must be a qualified + // name ([A-Za-z0-9][-A-Za-z0-9_.]*) no more than 63 bytes in length. + // + // The key is combined with the resource name of the + // ValidatingAdmissionPolicy to construct an audit annotation key: + // "{ValidatingAdmissionPolicy name}/{key}". + // + // If an admission webhook uses the same resource name as this ValidatingAdmissionPolicy + // and the same audit annotation key, the annotation key will be identical. + // In this case, the first annotation written with the key will be included + // in the audit event and all subsequent annotations with the same key + // will be discarded. + // + // Required. + Key string + + // valueExpression represents the expression which is evaluated by CEL to + // produce an audit annotation value. The expression must evaluate to either + // a string or null value. If the expression evaluates to a string, the + // audit annotation is included with the string value. If the expression + // evaluates to null or empty string the audit annotation will be omitted. + // The valueExpression may be no longer than 5kb in length. + // If the result of the valueExpression is more than 10kb in length, it + // will be truncated to 10kb. + // + // If multiple ValidatingAdmissionPolicyBinding resources match an + // API request, then the valueExpression will be evaluated for + // each binding. All unique values produced by the valueExpressions + // will be joined together in a comma-separated list. + // + // Required. + ValueExpression string +} + // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // ValidatingAdmissionPolicyBinding binds the ValidatingAdmissionPolicy with paramerized resources. @@ -287,6 +341,47 @@ type ValidatingAdmissionPolicyBindingSpec struct { // Note that this is differs from ValidatingAdmissionPolicy matchConstraints, where resourceRules are required. // +optional MatchResources *MatchResources + + // validationActions declares how Validations of the referenced ValidatingAdmissionPolicy are enforced. + // If a validation evaluates to false it is always enforced according to these actions. + // + // Failures defined by the ValidatingAdmissionPolicy's FailurePolicy are enforced according + // to these actions only if the FailurePolicy is set to Fail, otherwise the failures are + // ignored. This includes compilation errors, runtime errors and misconfigurations of the policy. + // + // validationActions is declared as a set of action values. Order does + // not matter. validationActions may not contain duplicates of the same action. + // + // The supported actions values are: + // + // "Deny" specifies that a validation failure results in a denied request. + // + // "Warn" specifies that a validation failure is reported to the request client + // in HTTP Warning headers, with a warning code of 299. Warnings can be sent + // both for allowed or denied admission responses. + // + // "Audit" specifies that a validation failure is included in the published + // audit event for the request. The audit event will contain a + // `validation.policy.admission.k8s.io/validation_failure` audit annotation + // with a value containing the details of the validation failures, formatted as + // a JSON list of objects, each with the following fields: + // - message: The validation failure message string + // - policy: The resource name of the ValidatingAdmissionPolicy + // - binding: The resource name of the ValidatingAdmissionPolicyBinding + // - expressionIndex: The index of the failed validations in the ValidatingAdmissionPolicy + // - validationActions: The enforcement actions enacted for the validation failure + // Example audit annotation: + // `"validation.policy.admission.k8s.io/validation_failure": "[{\"message\": \"Invalid value\", {\"policy\": \"policy.example.com\", {\"binding\": \"policybinding.example.com\", {\"expressionIndex\": \"1\", {\"validationActions\": [\"Audit\"]}]"` + // + // Clients should expect to handle additional values by ignoring + // any values not recognized. + // + // "Deny" and "Warn" may not be used together since this combination + // needlessly duplicates the validation failure both in the + // API response body and the HTTP warning headers. + // + // Required. + ValidationActions []ValidationAction } // ParamRef references a parameter resource @@ -387,6 +482,23 @@ type MatchResources struct { MatchPolicy *MatchPolicyType } +// ValidationAction specifies a policy enforcement action. +type ValidationAction string + +const ( + // Deny specifies that a validation failure results in a denied request. + Deny ValidationAction = "Deny" + // Warn specifies that a validation failure is reported to the request client + // in HTTP Warning headers, with a warning code of 299. Warnings can be sent + // both for allowed or denied admission responses. + Warn ValidationAction = "Warn" + // Audit specifies that a validation failure is included in the published + // audit event for the request. The audit event will contain a + // `validation.policy.admission.k8s.io/validation_failure` audit annotation + // with a value containing the details of the validation failure. + Audit ValidationAction = "Audit" +) + // NamedRuleWithOperations is a tuple of Operations and Resources with ResourceNames. type NamedRuleWithOperations struct { // ResourceNames is an optional white list of names that the rule applies to. An empty set means that everything is allowed. diff --git a/pkg/apis/admissionregistration/validation/validation.go b/pkg/apis/admissionregistration/validation/validation.go index e29926b447e..c53b4b12667 100644 --- a/pkg/apis/admissionregistration/validation/validation.go +++ b/pkg/apis/admissionregistration/validation/validation.go @@ -37,6 +37,7 @@ import ( "k8s.io/kubernetes/pkg/apis/admissionregistration" admissionregistrationv1 "k8s.io/kubernetes/pkg/apis/admissionregistration/v1" admissionregistrationv1beta1 "k8s.io/kubernetes/pkg/apis/admissionregistration/v1beta1" + apivalidation "k8s.io/kubernetes/pkg/apis/core/validation" ) func hasWildcard(slice []string) bool { @@ -583,6 +584,14 @@ func ValidateMutatingWebhookConfigurationUpdate(newC, oldC *admissionregistratio }) } +const ( + maxAuditAnnotations = 20 + // use a 5kb limit the CEL expression, note that this is less than the length limit + // for the audit annotation value limit (10kb) since an expressions that concatenates + // strings will often produce a longer value than the expression + maxAuditAnnotationValueExpressionLength = 5 * 1024 +) + // ValidateValidatingAdmissionPolicy validates a ValidatingAdmissionPolicy before creation. func ValidateValidatingAdmissionPolicy(p *admissionregistration.ValidatingAdmissionPolicy) field.ErrorList { return validateValidatingAdmissionPolicy(p) @@ -590,11 +599,11 @@ func ValidateValidatingAdmissionPolicy(p *admissionregistration.ValidatingAdmiss func validateValidatingAdmissionPolicy(p *admissionregistration.ValidatingAdmissionPolicy) field.ErrorList { allErrors := genericvalidation.ValidateObjectMeta(&p.ObjectMeta, false, genericvalidation.NameIsDNSSubdomain, field.NewPath("metadata")) - allErrors = append(allErrors, validateValidatingAdmissionPolicySpec(&p.Spec, field.NewPath("spec"))...) + allErrors = append(allErrors, validateValidatingAdmissionPolicySpec(p.ObjectMeta, &p.Spec, field.NewPath("spec"))...) return allErrors } -func validateValidatingAdmissionPolicySpec(spec *admissionregistration.ValidatingAdmissionPolicySpec, fldPath *field.Path) field.ErrorList { +func validateValidatingAdmissionPolicySpec(meta metav1.ObjectMeta, spec *admissionregistration.ValidatingAdmissionPolicySpec, fldPath *field.Path) field.ErrorList { var allErrors field.ErrorList if spec.FailurePolicy == nil { allErrors = append(allErrors, field.Required(fldPath.Child("failurePolicy"), "")) @@ -613,14 +622,27 @@ func validateValidatingAdmissionPolicySpec(spec *admissionregistration.Validatin allErrors = append(allErrors, field.Required(fldPath.Child("matchConstraints", "resourceRules"), "")) } } - if len(spec.Validations) == 0 { - allErrors = append(allErrors, field.Required(fldPath.Child("validations"), "")) + if len(spec.Validations) == 0 && len(spec.AuditAnnotations) == 0 { + allErrors = append(allErrors, field.Required(fldPath.Child("validations"), "validations or auditAnnotations must contain at least one item")) + allErrors = append(allErrors, field.Required(fldPath.Child("auditAnnotations"), "validations or auditAnnotations must contain at least one item")) } else { for i, validation := range spec.Validations { allErrors = append(allErrors, validateValidation(&validation, spec.ParamKind, fldPath.Child("validations").Index(i))...) } + if spec.AuditAnnotations != nil { + keys := sets.NewString() + if len(spec.AuditAnnotations) > maxAuditAnnotations { + allErrors = append(allErrors, field.Invalid(fldPath.Child("auditAnnotations"), spec.AuditAnnotations, fmt.Sprintf("must not have more than %d auditAnnotations", maxAuditAnnotations))) + } + for i, auditAnnotation := range spec.AuditAnnotations { + allErrors = append(allErrors, validateAuditAnnotation(meta, &auditAnnotation, spec.ParamKind, fldPath.Child("auditAnnotations").Index(i))...) + if keys.Has(auditAnnotation.Key) { + allErrors = append(allErrors, field.Duplicate(fldPath.Child("auditAnnotations").Index(i).Child("key"), auditAnnotation.Key)) + } + keys.Insert(auditAnnotation.Key) + } + } } - return allErrors } @@ -712,6 +734,33 @@ func validateMatchResources(mc *admissionregistration.MatchResources, fldPath *f return allErrors } +var validValidationActions = sets.NewString( + string(admissionregistration.Deny), + string(admissionregistration.Warn), + string(admissionregistration.Audit), +) + +func validateValidationActions(va []admissionregistration.ValidationAction, fldPath *field.Path) field.ErrorList { + var allErrors field.ErrorList + actions := sets.NewString() + for i, action := range va { + if !validValidationActions.Has(string(action)) { + allErrors = append(allErrors, field.NotSupported(fldPath.Index(i), action, validValidationActions.List())) + } + if actions.Has(string(action)) { + allErrors = append(allErrors, field.Duplicate(fldPath.Index(i), action)) + } + actions.Insert(string(action)) + } + if actions.Has(string(admissionregistration.Deny)) && actions.Has(string(admissionregistration.Warn)) { + allErrors = append(allErrors, field.Invalid(fldPath, va, "must not contain both Deny and Warn (repeating the same validation failure information in the API response and headers serves no purpose)")) + } + if len(actions) == 0 { + allErrors = append(allErrors, field.Required(fldPath, "at least one validation action is required")) + } + return allErrors +} + func validateNamedRuleWithOperations(n *admissionregistration.NamedRuleWithOperations, fldPath *field.Path) field.ErrorList { var allErrors field.ErrorList resourceNames := sets.NewString() @@ -767,6 +816,38 @@ func validateValidation(v *admissionregistration.Validation, paramKind *admissio return allErrors } +func validateAuditAnnotation(meta metav1.ObjectMeta, v *admissionregistration.AuditAnnotation, paramKind *admissionregistration.ParamKind, fldPath *field.Path) field.ErrorList { + var allErrors field.ErrorList + if len(meta.GetName()) != 0 { + name := meta.GetName() + allErrors = append(allErrors, apivalidation.ValidateQualifiedName(name+"/"+v.Key, fldPath.Child("key"))...) + } else { + allErrors = append(allErrors, field.Invalid(fldPath.Child("key"), v.Key, "requires metadata.name be non-empty")) + } + + trimmedValueExpression := strings.TrimSpace(v.ValueExpression) + if len(trimmedValueExpression) == 0 { + allErrors = append(allErrors, field.Required(fldPath.Child("valueExpression"), "valueExpression is not specified")) + } else if len(trimmedValueExpression) > maxAuditAnnotationValueExpressionLength { + allErrors = append(allErrors, field.Required(fldPath.Child("valueExpression"), fmt.Sprintf("must not exceed %d bytes in length", maxAuditAnnotationValueExpressionLength))) + } else { + result := plugincel.CompileCELExpression(&validatingadmissionpolicy.AuditAnnotationCondition{ + ValueExpression: trimmedValueExpression, + }, plugincel.OptionalVariableDeclarations{HasParams: paramKind != nil, HasAuthorizer: true}, celconfig.PerCallLimit) + if result.Error != nil { + switch result.Error.Type { + case cel.ErrorTypeRequired: + allErrors = append(allErrors, field.Required(fldPath.Child("valueExpression"), result.Error.Detail)) + case cel.ErrorTypeInvalid: + allErrors = append(allErrors, field.Invalid(fldPath.Child("valueExpression"), v.ValueExpression, result.Error.Detail)) + default: + allErrors = append(allErrors, field.InternalError(fldPath.Child("valueExpression"), result.Error)) + } + } + } + return allErrors +} + var newlineMatcher = regexp.MustCompile(`[\n\r]+`) // valid newline chars in CEL grammar func hasNewlines(s string) bool { return newlineMatcher.MatchString(s) @@ -796,6 +877,7 @@ func validateValidatingAdmissionPolicyBindingSpec(spec *admissionregistration.Va } allErrors = append(allErrors, validateParamRef(spec.ParamRef, fldPath.Child("paramRef"))...) allErrors = append(allErrors, validateMatchResources(spec.MatchResources, fldPath.Child("matchResouces"))...) + allErrors = append(allErrors, validateValidationActions(spec.ValidationActions, fldPath.Child("validationActions"))...) return allErrors } diff --git a/pkg/apis/admissionregistration/validation/validation_test.go b/pkg/apis/admissionregistration/validation/validation_test.go index ef5c3e4bf03..b442cd82239 100644 --- a/pkg/apis/admissionregistration/validation/validation_test.go +++ b/pkg/apis/admissionregistration/validation/validation_test.go @@ -21,6 +21,7 @@ import ( "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/kubernetes/pkg/apis/admissionregistration" ) @@ -1999,7 +2000,7 @@ func TestValidateValidatingAdmissionPolicy(t *testing.T) { Spec: admissionregistration.ValidatingAdmissionPolicySpec{}, }, - expectedError: `spec.validations: Required value`, + expectedError: `spec.validations: Required value: validations or auditAnnotations must contain at least one item`, }, { name: "Invalid Validations Reason", @@ -2565,6 +2566,112 @@ func TestValidateValidatingAdmissionPolicy(t *testing.T) { }, expectedError: `spec.validations[0].expression: Invalid value: "object.x in [1, 2, ": compilation failed: ERROR: :1:19: Syntax error: missing ']' at '`, }, + { + name: "invalid auditAnnotations key due to key name", + config: &admissionregistration.ValidatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config", + }, + Spec: admissionregistration.ValidatingAdmissionPolicySpec{ + AuditAnnotations: []admissionregistration.AuditAnnotation{ + { + Key: "@", + ValueExpression: "value", + }, + }, + }, + }, + expectedError: `spec.auditAnnotations[0].key: Invalid value: "config/@": name part must consist of alphanumeric characters`, + }, + { + name: "auditAnnotations keys must be unique", + config: &admissionregistration.ValidatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config", + }, + Spec: admissionregistration.ValidatingAdmissionPolicySpec{ + AuditAnnotations: []admissionregistration.AuditAnnotation{ + { + Key: "a", + ValueExpression: "'1'", + }, + { + Key: "a", + ValueExpression: "'2'", + }, + }, + }, + }, + expectedError: `spec.auditAnnotations[1].key: Duplicate value: "a"`, + }, + { + name: "invalid auditAnnotations key due to metadata.name", + config: &admissionregistration.ValidatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nope!", + }, + Spec: admissionregistration.ValidatingAdmissionPolicySpec{ + AuditAnnotations: []admissionregistration.AuditAnnotation{ + { + Key: "key", + ValueExpression: "'value'", + }, + }, + }, + }, + expectedError: `spec.auditAnnotations[0].key: Invalid value: "nope!/key": prefix part a lowercase RFC 1123 subdomain`, + }, + { + name: "invalid auditAnnotations key due to length", + config: &admissionregistration.ValidatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "this-is-a-long-name-for-a-admission-policy-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", + }, + Spec: admissionregistration.ValidatingAdmissionPolicySpec{ + AuditAnnotations: []admissionregistration.AuditAnnotation{ + { + Key: "this-is-a-long-name-for-an-audit-annotation-key-xxxxxxxxxxxxxxxxxxxxxxxxxx", + ValueExpression: "'value'", + }, + }, + }, + }, + expectedError: `spec.auditAnnotations[0].key: Invalid value`, + }, + { + name: "invalid auditAnnotations valueExpression type", + config: &admissionregistration.ValidatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config", + }, + Spec: admissionregistration.ValidatingAdmissionPolicySpec{ + AuditAnnotations: []admissionregistration.AuditAnnotation{ + { + Key: "something", + ValueExpression: "true", + }, + }, + }, + }, + expectedError: `spec.auditAnnotations[0].valueExpression: Invalid value: "true": must evaluate to one of [string null_type]`, + }, + { + name: "invalid auditAnnotations valueExpression", + config: &admissionregistration.ValidatingAdmissionPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config", + }, + Spec: admissionregistration.ValidatingAdmissionPolicySpec{ + AuditAnnotations: []admissionregistration.AuditAnnotation{ + { + Key: "something", + ValueExpression: "object.x in [1, 2, ", + }, + }, + }, + }, + expectedError: `spec.auditAnnotations[0].valueExpression: Invalid value: "object.x in [1, 2, ": compilation failed: ERROR: :1:19: Syntax error: missing ']' at '`, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -2580,7 +2687,6 @@ func TestValidateValidatingAdmissionPolicy(t *testing.T) { } } }) - } } @@ -2709,6 +2815,7 @@ func TestValidateValidatingAdmissionPolicyUpdate(t *testing.T) { Spec: admissionregistration.ValidatingAdmissionPolicySpec{}, }, }, + // TODO: CustomAuditAnnotations: string valueExpression with {oldObject} is allowed } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -2902,6 +3009,7 @@ func TestValidateValidatingAdmissionPolicyBinding(t *testing.T) { ParamRef: &admissionregistration.ParamRef{ Name: "xyzlimit-scale-setting.example.com", }, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.Deny}, MatchResources: &admissionregistration.MatchResources{ ResourceRules: []admissionregistration.NamedRuleWithOperations{ { @@ -2931,6 +3039,7 @@ func TestValidateValidatingAdmissionPolicyBinding(t *testing.T) { ParamRef: &admissionregistration.ParamRef{ Name: "xyzlimit-scale-setting.example.com", }, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.Deny}, MatchResources: &admissionregistration.MatchResources{ NamespaceSelector: &metav1.LabelSelector{ MatchLabels: map[string]string{"a": "b"}, @@ -2969,6 +3078,7 @@ func TestValidateValidatingAdmissionPolicyBinding(t *testing.T) { ParamRef: &admissionregistration.ParamRef{ Name: "xyzlimit-scale-setting.example.com", }, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.Deny}, MatchResources: &admissionregistration.MatchResources{ ResourceRules: []admissionregistration.NamedRuleWithOperations{ { @@ -2998,6 +3108,7 @@ func TestValidateValidatingAdmissionPolicyBinding(t *testing.T) { ParamRef: &admissionregistration.ParamRef{ Name: "xyzlimit-scale-setting.example.com", }, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.Deny}, MatchResources: &admissionregistration.MatchResources{ ResourceRules: []admissionregistration.NamedRuleWithOperations{ { @@ -3027,6 +3138,7 @@ func TestValidateValidatingAdmissionPolicyBinding(t *testing.T) { ParamRef: &admissionregistration.ParamRef{ Name: "xyzlimit-scale-setting.example.com", }, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.Deny}, MatchResources: &admissionregistration.MatchResources{ NamespaceSelector: &metav1.LabelSelector{ MatchLabels: map[string]string{"a": "b"}, @@ -3065,6 +3177,7 @@ func TestValidateValidatingAdmissionPolicyBinding(t *testing.T) { ParamRef: &admissionregistration.ParamRef{ Name: "xyzlimit-scale-setting.example.com", }, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.Deny}, MatchResources: &admissionregistration.MatchResources{ ResourceRules: []admissionregistration.NamedRuleWithOperations{ { @@ -3094,6 +3207,7 @@ func TestValidateValidatingAdmissionPolicyBinding(t *testing.T) { ParamRef: &admissionregistration.ParamRef{ Name: "xyzlimit-scale-setting.example.com", }, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.Deny}, MatchResources: &admissionregistration.MatchResources{ ResourceRules: []admissionregistration.NamedRuleWithOperations{ { @@ -3112,6 +3226,38 @@ func TestValidateValidatingAdmissionPolicyBinding(t *testing.T) { }, expectedError: `spec.matchResouces.resourceRules[0].resources: Invalid value: []string{"*/*", "a"}: if '*/*' is present, must not specify other resources`, }, + { + name: "validationActions must be unique", + config: &admissionregistration.ValidatingAdmissionPolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config", + }, + Spec: admissionregistration.ValidatingAdmissionPolicyBindingSpec{ + PolicyName: "xyzlimit-scale.example.com", + ParamRef: &admissionregistration.ParamRef{ + Name: "xyzlimit-scale-setting.example.com", + }, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.Deny, admissionregistration.Deny}, + }, + }, + expectedError: `spec.validationActions[1]: Duplicate value: "Deny"`, + }, + { + name: "validationActions must contain supported values", + config: &admissionregistration.ValidatingAdmissionPolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config", + }, + Spec: admissionregistration.ValidatingAdmissionPolicyBindingSpec{ + PolicyName: "xyzlimit-scale.example.com", + ParamRef: &admissionregistration.ParamRef{ + Name: "xyzlimit-scale-setting.example.com", + }, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.ValidationAction("illegal")}, + }, + }, + expectedError: `Unsupported value: "illegal": supported values: "Audit", "Deny", "Warn"`, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -3149,6 +3295,7 @@ func TestValidateValidatingAdmissionPolicyBindingUpdate(t *testing.T) { ParamRef: &admissionregistration.ParamRef{ Name: "xyzlimit-scale-setting.example.com", }, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.Deny}, MatchResources: &admissionregistration.MatchResources{ NamespaceSelector: &metav1.LabelSelector{ MatchLabels: map[string]string{"a": "b"}, @@ -3184,6 +3331,7 @@ func TestValidateValidatingAdmissionPolicyBindingUpdate(t *testing.T) { ParamRef: &admissionregistration.ParamRef{ Name: "xyzlimit-scale-setting.example.com", }, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.Deny}, MatchResources: &admissionregistration.MatchResources{ NamespaceSelector: &metav1.LabelSelector{ MatchLabels: map[string]string{"a": "b"}, @@ -3222,6 +3370,7 @@ func TestValidateValidatingAdmissionPolicyBindingUpdate(t *testing.T) { ParamRef: &admissionregistration.ParamRef{ Name: "xyzlimit-scale-setting.example.com", }, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.Deny}, MatchResources: &admissionregistration.MatchResources{ NamespaceSelector: &metav1.LabelSelector{ MatchLabels: map[string]string{"a": "b"}, diff --git a/pkg/registry/admissionregistration/validatingadmissionpolicybinding/storage/storage_test.go b/pkg/registry/admissionregistration/validatingadmissionpolicybinding/storage/storage_test.go index 6961873463f..db4af525be0 100644 --- a/pkg/registry/admissionregistration/validatingadmissionpolicybinding/storage/storage_test.go +++ b/pkg/registry/admissionregistration/validatingadmissionpolicybinding/storage/storage_test.go @@ -27,6 +27,7 @@ import ( "k8s.io/apiserver/pkg/registry/generic" genericregistrytest "k8s.io/apiserver/pkg/registry/generic/testing" etcd3testing "k8s.io/apiserver/pkg/storage/etcd3/testing" + "k8s.io/kubernetes/pkg/apis/admissionregistration" "k8s.io/kubernetes/pkg/registry/admissionregistration/resolver" "k8s.io/kubernetes/pkg/registry/registrytest" @@ -127,6 +128,7 @@ func validPolicyBinding() *admissionregistration.ValidatingAdmissionPolicyBindin ParamRef: &admissionregistration.ParamRef{ Name: "param-test", }, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.Deny}, MatchResources: &admissionregistration.MatchResources{ MatchPolicy: func() *admissionregistration.MatchPolicyType { r := admissionregistration.MatchPolicyType("Exact") @@ -166,7 +168,8 @@ func newPolicyBinding(name string) *admissionregistration.ValidatingAdmissionPol ParamRef: &admissionregistration.ParamRef{ Name: "param-test", }, - MatchResources: &admissionregistration.MatchResources{}, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.Deny}, + MatchResources: &admissionregistration.MatchResources{}, }, } } diff --git a/pkg/registry/admissionregistration/validatingadmissionpolicybinding/strategy_test.go b/pkg/registry/admissionregistration/validatingadmissionpolicybinding/strategy_test.go index a3a2bf91841..57d4cedd2c0 100644 --- a/pkg/registry/admissionregistration/validatingadmissionpolicybinding/strategy_test.go +++ b/pkg/registry/admissionregistration/validatingadmissionpolicybinding/strategy_test.go @@ -21,6 +21,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/kubernetes/pkg/apis/admissionregistration" ) @@ -59,6 +60,7 @@ func validPolicyBinding() *admissionregistration.ValidatingAdmissionPolicyBindin ParamRef: &admissionregistration.ParamRef{ Name: "replica-limit-test.example.com", }, + ValidationActions: []admissionregistration.ValidationAction{admissionregistration.Deny}, }, } } diff --git a/staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go b/staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go index 4e18cf04976..5186a923913 100644 --- a/staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go +++ b/staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go @@ -107,18 +107,35 @@ type ValidatingAdmissionPolicySpec struct { MatchConstraints *MatchResources `json:"matchConstraints,omitempty" protobuf:"bytes,2,rep,name=matchConstraints"` // Validations contain CEL expressions which is used to apply the validation. - // A minimum of one validation is required for a policy definition. + // Validations and AuditAnnotations may not both be empty; a minimum of one Validations or AuditAnnotations is + // required. // +listType=atomic - // Required. - Validations []Validation `json:"validations" protobuf:"bytes,3,rep,name=validations"` + // +optional + Validations []Validation `json:"validations,omitempty" protobuf:"bytes,3,rep,name=validations"` - // FailurePolicy defines how to handle failures for the admission policy. - // Failures can occur from invalid or mis-configured policy definitions or bindings. + // failurePolicy defines how to handle failures for the admission policy. Failures can + // occur from CEL expression parse errors, type check errors, runtime errors and invalid + // or mis-configured policy definitions or bindings. + // // A policy is invalid if spec.paramKind refers to a non-existent Kind. // A binding is invalid if spec.paramRef.name refers to a non-existent resource. + // + // failurePolicy does not define how validations that evaluate to false are handled. + // + // When failurePolicy is set to Fail, ValidatingAdmissionPolicyBinding validationActions + // define how failures are enforced. + // // Allowed values are Ignore or Fail. Defaults to Fail. // +optional FailurePolicy *FailurePolicyType `json:"failurePolicy,omitempty" protobuf:"bytes,4,opt,name=failurePolicy,casttype=FailurePolicyType"` + + // auditAnnotations contains CEL expressions which are used to produce audit + // annotations for the audit event of the API request. + // validations and auditAnnotations may not both be empty; a least one of validations or auditAnnotations is + // required. + // +listType=atomic + // +optional + AuditAnnotations []AuditAnnotation `json:"auditAnnotations,omitempty" protobuf:"bytes,5,rep,name=auditAnnotations"` } // ParamKind is a tuple of Group Kind and Version. @@ -138,11 +155,11 @@ type ParamKind struct { type Validation struct { // Expression represents the expression which will be evaluated by CEL. // ref: https://github.com/google/cel-spec - // CEL expressions have access to the contents of the Admission request/response, organized into CEL variables as well as some other useful variables: + // CEL expressions have access to the contents of the API request/response, organized into CEL variables as well as some other useful variables: // // - 'object' - The object from the incoming request. The value is null for DELETE requests. // - 'oldObject' - The existing object. The value is null for CREATE requests. - // - 'request' - Attributes of the admission request([ref](/pkg/apis/admission/types.go#AdmissionRequest)). + // - 'request' - Attributes of the API request([ref](/pkg/apis/admission/types.go#AdmissionRequest)). // - 'params' - Parameter resource referred to by the policy binding being evaluated. Only populated if the policy has a ParamKind. // - 'authorizer' - A CEL Authorizer. May be used to perform authorization checks for the principal (user or service account) of the request. // See https://pkg.go.dev/k8s.io/apiserver/pkg/cel/library#Authz @@ -194,6 +211,43 @@ type Validation struct { Reason *metav1.StatusReason `json:"reason,omitempty" protobuf:"bytes,3,opt,name=reason"` } +// AuditAnnotation describes how to produce an audit annotation for an API request. +type AuditAnnotation struct { + // key specifies the audit annotation key. The audit annotation keys of + // a ValidatingAdmissionPolicy must be unique. The key must be a qualified + // name ([A-Za-z0-9][-A-Za-z0-9_.]*) no more than 63 bytes in length. + // + // The key is combined with the resource name of the + // ValidatingAdmissionPolicy to construct an audit annotation key: + // "{ValidatingAdmissionPolicy name}/{key}". + // + // If an admission webhook uses the same resource name as this ValidatingAdmissionPolicy + // and the same audit annotation key, the annotation key will be identical. + // In this case, the first annotation written with the key will be included + // in the audit event and all subsequent annotations with the same key + // will be discarded. + // + // Required. + Key string `json:"key" protobuf:"bytes,1,opt,name=key"` + + // valueExpression represents the expression which is evaluated by CEL to + // produce an audit annotation value. The expression must evaluate to either + // a string or null value. If the expression evaluates to a string, the + // audit annotation is included with the string value. If the expression + // evaluates to null or empty string the audit annotation will be omitted. + // The valueExpression may be no longer than 5kb in length. + // If the result of the valueExpression is more than 10kb in length, it + // will be truncated to 10kb. + // + // If multiple ValidatingAdmissionPolicyBinding resources match an + // API request, then the valueExpression will be evaluated for + // each binding. All unique values produced by the valueExpressions + // will be joined together in a comma-separated list. + // + // Required. + ValueExpression string `json:"valueExpression" protobuf:"bytes,2,opt,name=valueExpression"` +} + // +genclient // +genclient:nonNamespaced // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object @@ -244,6 +298,48 @@ type ValidatingAdmissionPolicyBindingSpec struct { // Note that this is differs from ValidatingAdmissionPolicy matchConstraints, where resourceRules are required. // +optional MatchResources *MatchResources `json:"matchResources,omitempty" protobuf:"bytes,3,rep,name=matchResources"` + + // validationActions declares how Validations of the referenced ValidatingAdmissionPolicy are enforced. + // If a validation evaluates to false it is always enforced according to these actions. + // + // Failures defined by the ValidatingAdmissionPolicy's FailurePolicy are enforced according + // to these actions only if the FailurePolicy is set to Fail, otherwise the failures are + // ignored. This includes compilation errors, runtime errors and misconfigurations of the policy. + // + // validationActions is declared as a set of action values. Order does + // not matter. validationActions may not contain duplicates of the same action. + // + // The supported actions values are: + // + // "Deny" specifies that a validation failure results in a denied request. + // + // "Warn" specifies that a validation failure is reported to the request client + // in HTTP Warning headers, with a warning code of 299. Warnings can be sent + // both for allowed or denied admission responses. + // + // "Audit" specifies that a validation failure is included in the published + // audit event for the request. The audit event will contain a + // `validation.policy.admission.k8s.io/validation_failure` audit annotation + // with a value containing the details of the validation failures, formatted as + // a JSON list of objects, each with the following fields: + // - message: The validation failure message string + // - policy: The resource name of the ValidatingAdmissionPolicy + // - binding: The resource name of the ValidatingAdmissionPolicyBinding + // - expressionIndex: The index of the failed validations in the ValidatingAdmissionPolicy + // - validationActions: The enforcement actions enacted for the validation failure + // Example audit annotation: + // `"validation.policy.admission.k8s.io/validation_failure": "[{\"message\": \"Invalid value\", {\"policy\": \"policy.example.com\", {\"binding\": \"policybinding.example.com\", {\"expressionIndex\": \"1\", {\"validationActions\": [\"Audit\"]}]"` + // + // Clients should expect to handle additional values by ignoring + // any values not recognized. + // + // "Deny" and "Warn" may not be used together since this combination + // needlessly duplicates the validation failure both in the + // API response body and the HTTP warning headers. + // + // Required. + // +listType=set + ValidationActions []ValidationAction `json:"validationActions,omitempty" protobuf:"bytes,4,rep,name=validationActions"` } // ParamRef references a parameter resource @@ -348,6 +444,24 @@ type MatchResources struct { MatchPolicy *MatchPolicyType `json:"matchPolicy,omitempty" protobuf:"bytes,7,opt,name=matchPolicy,casttype=MatchPolicyType"` } +// ValidationAction specifies a policy enforcement action. +// +enum +type ValidationAction string + +const ( + // Deny specifies that a validation failure results in a denied request. + Deny ValidationAction = "Deny" + // Warn specifies that a validation failure is reported to the request client + // in HTTP Warning headers, with a warning code of 299. Warnings can be sent + // both for allowed or denied admission responses. + Warn ValidationAction = "Warn" + // Audit specifies that a validation failure is included in the published + // audit event for the request. The audit event will contain a + // `validation.policy.admission.k8s.io/validation_failure` audit annotation + // with a value containing the details of the validation failure. + Audit ValidationAction = "Audit" +) + // NamedRuleWithOperations is a tuple of Operations and Resources with ResourceNames. // +structType=atomic type NamedRuleWithOperations struct { diff --git a/staging/src/k8s.io/apiserver/pkg/admission/cel/metrics.go b/staging/src/k8s.io/apiserver/pkg/admission/cel/metrics.go index 77d2210c20a..9f8a941105a 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/cel/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/cel/metrics.go @@ -109,3 +109,15 @@ func (m *ValidatingAdmissionPolicyMetrics) ObserveRejection(ctx context.Context, m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "deny", state).Inc() m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "deny", state).Observe(elapsed.Seconds()) } + +// ObserveAudit observes a policy validation audit annotation was published for a validation failure. +func (m *ValidatingAdmissionPolicyMetrics) ObserveAudit(ctx context.Context, elapsed time.Duration, policy, binding, state string) { + m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "audit", state).Inc() + m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "audit", state).Observe(elapsed.Seconds()) +} + +// ObserveWarn observes a policy validation warning was published for a validation failure. +func (m *ValidatingAdmissionPolicyMetrics) ObserveWarn(ctx context.Context, elapsed time.Duration, policy, binding, state string) { + m.policyCheck.WithContext(ctx).WithLabelValues(policy, binding, "warn", state).Inc() + m.policyLatency.WithContext(ctx).WithLabelValues(policy, binding, "warn", state).Observe(elapsed.Seconds()) +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile.go index b91907d37dd..e3327faaec3 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile.go @@ -223,11 +223,26 @@ func CompileCELExpression(expressionAccessor ExpressionAccessor, optionalVars Op ExpressionAccessor: expressionAccessor, } } - if ast.OutputType() != cel.BoolType { + found := false + returnTypes := expressionAccessor.ReturnTypes() + for _, returnType := range returnTypes { + if ast.OutputType() == returnType { + found = true + break + } + } + if !found { + var reason string + if len(returnTypes) == 1 { + reason = fmt.Sprintf("must evaluate to %v", returnTypes[0].String()) + } else { + reason = fmt.Sprintf("must evaluate to one of %v", returnTypes) + } + return CompilationResult{ Error: &apiservercel.Error{ Type: apiservercel.ErrorTypeInvalid, - Detail: "cel expression must evaluate to a bool", + Detail: reason, }, ExpressionAccessor: expressionAccessor, } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile_test.go index 216c067e789..04fcd6b6f58 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile_test.go @@ -20,6 +20,8 @@ import ( "strings" "testing" + celgo "github.com/google/cel-go/cel" + celconfig "k8s.io/apiserver/pkg/apis/cel" ) @@ -120,34 +122,79 @@ func TestCompileValidatingPolicyExpression(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { for _, expr := range tc.expressions { - result := CompileCELExpression(&fakeExpressionAccessor{ - expr, - }, OptionalVariableDeclarations{HasParams: tc.hasParams, HasAuthorizer: true}, celconfig.PerCallLimit) - if result.Error != nil { - t.Errorf("Unexpected error: %v", result.Error) - } + t.Run(expr, func(t *testing.T) { + t.Run("expression", func(t *testing.T) { + result := CompileCELExpression(&fakeValidationCondition{ + Expression: expr, + }, OptionalVariableDeclarations{HasParams: tc.hasParams, HasAuthorizer: tc.hasAuthorizer}, celconfig.PerCallLimit) + if result.Error != nil { + t.Errorf("Unexpected error: %v", result.Error) + } + }) + t.Run("auditAnnotation.valueExpression", func(t *testing.T) { + // Test audit annotation compilation by casting the result to a string + result := CompileCELExpression(&fakeAuditAnnotationCondition{ + ValueExpression: "string(" + expr + ")", + }, OptionalVariableDeclarations{HasParams: tc.hasParams, HasAuthorizer: tc.hasAuthorizer}, celconfig.PerCallLimit) + if result.Error != nil { + t.Errorf("Unexpected error: %v", result.Error) + } + }) + }) } for expr, expectErr := range tc.errorExpressions { - result := CompileCELExpression(&fakeExpressionAccessor{ - expr, - }, OptionalVariableDeclarations{HasParams: tc.hasParams, HasAuthorizer: tc.hasAuthorizer}, celconfig.PerCallLimit) - if result.Error == nil { - t.Errorf("Expected expression '%s' to contain '%v' but got no error", expr, expectErr) - continue - } - if !strings.Contains(result.Error.Error(), expectErr) { - t.Errorf("Expected compilation '%s' error to contain '%v' but got: %v", expr, expectErr, result.Error) - } - continue + t.Run(expr, func(t *testing.T) { + t.Run("expression", func(t *testing.T) { + result := CompileCELExpression(&fakeValidationCondition{ + Expression: expr, + }, OptionalVariableDeclarations{HasParams: tc.hasParams, HasAuthorizer: tc.hasAuthorizer}, celconfig.PerCallLimit) + if result.Error == nil { + t.Errorf("Expected expression '%s' to contain '%v' but got no error", expr, expectErr) + return + } + if !strings.Contains(result.Error.Error(), expectErr) { + t.Errorf("Expected validation '%s' error to contain '%v' but got: %v", expr, expectErr, result.Error) + } + }) + t.Run("auditAnnotation.valueExpression", func(t *testing.T) { + // Test audit annotation compilation by casting the result to a string + result := CompileCELExpression(&fakeAuditAnnotationCondition{ + ValueExpression: "string(" + expr + ")", + }, OptionalVariableDeclarations{HasParams: tc.hasParams, HasAuthorizer: tc.hasAuthorizer}, celconfig.PerCallLimit) + if result.Error == nil { + t.Errorf("Expected expression '%s' to contain '%v' but got no error", expr, expectErr) + return + } + if !strings.Contains(result.Error.Error(), expectErr) { + t.Errorf("Expected validation '%s' error to contain '%v' but got: %v", expr, expectErr, result.Error) + } + }) + }) } }) } } -type fakeExpressionAccessor struct { - expression string +type fakeValidationCondition struct { + Expression string } -func (f *fakeExpressionAccessor) GetExpression() string { - return f.expression +func (v *fakeValidationCondition) GetExpression() string { + return v.Expression +} + +func (v *fakeValidationCondition) ReturnTypes() []*celgo.Type { + return []*celgo.Type{celgo.BoolType} +} + +type fakeAuditAnnotationCondition struct { + ValueExpression string +} + +func (v *fakeAuditAnnotationCondition) GetExpression() string { + return v.ValueExpression +} + +func (v *fakeAuditAnnotationCondition) ReturnTypes() []*celgo.Type { + return []*celgo.Type{celgo.StringType, celgo.NullType} } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter.go index b9d898c2bdc..6531fecc835 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter.go @@ -75,11 +75,7 @@ func (a *evaluationActivation) Parent() interpreter.Activation { } // Compile compiles the cel expressions defined in the ExpressionAccessors into a Filter -// perCallLimit was added for testing purpose only. Callers should always use const PerCallLimit from k8s.io/apiserver/pkg/apis/cel/config.go as input. func (c *filterCompiler) Compile(expressionAccessors []ExpressionAccessor, options OptionalVariableDeclarations, perCallLimit uint64) Filter { - if len(expressionAccessors) == 0 { - return nil - } compilationResults := make([]CompilationResult, len(expressionAccessors)) for i, expressionAccessor := range expressionAccessors { compilationResults[i] = CompileCELExpression(expressionAccessor, options, perCallLimit) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter_test.go index bc9b63590bd..99e0f56f8e8 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter_test.go @@ -24,14 +24,10 @@ import ( "strings" "testing" + celgo "github.com/google/cel-go/cel" celtypes "github.com/google/cel-go/common/types" "github.com/stretchr/testify/require" - celconfig "k8s.io/apiserver/pkg/apis/cel" - "k8s.io/apiserver/pkg/authentication/user" - "k8s.io/apiserver/pkg/authorization/authorizer" - apiservercel "k8s.io/apiserver/pkg/cel" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -39,6 +35,10 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/admission/plugin/webhook/generic" + celconfig "k8s.io/apiserver/pkg/apis/cel" + "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/authorization/authorizer" + apiservercel "k8s.io/apiserver/pkg/cel" ) type condition struct { @@ -49,6 +49,10 @@ func (c *condition) GetExpression() string { return c.Expression } +func (v *condition) ReturnTypes() []*celgo.Type { + return []*celgo.Type{celgo.BoolType} +} + func TestCompile(t *testing.T) { cases := []struct { name string diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/interface.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/interface.go index 6bd6645ca40..fe5702c0afa 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/interface.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/interface.go @@ -19,6 +19,7 @@ package cel import ( "time" + "github.com/google/cel-go/cel" "github.com/google/cel-go/common/types/ref" v1 "k8s.io/api/admission/v1" @@ -31,6 +32,7 @@ var _ ExpressionAccessor = &MatchCondition{} type ExpressionAccessor interface { GetExpression() string + ReturnTypes() []*cel.Type } // EvaluationResult contains the minimal required fields and metadata of a cel evaluation @@ -50,6 +52,10 @@ func (v *MatchCondition) GetExpression() string { return v.Expression } +func (v *MatchCondition) ReturnTypes() []*cel.Type { + return []*cel.Type{cel.BoolType} +} + // OptionalVariableDeclarations declares which optional CEL variables // are declared for an expression. type OptionalVariableDeclarations struct { @@ -83,6 +89,7 @@ type OptionalVariableBindings struct { // Filter contains a function to evaluate compiled CEL-typed values // It expects the inbound object to already have been converted to the version expected // by the underlying CEL code (which is indicated by the match criteria of a policy definition). +// versionedParams may be nil. type Filter interface { // ForInput converts compiled CEL-typed values into evaluated CEL-typed values // runtimeCELCostBudget was added for testing purpose only. Callers should always use const RuntimeCELCostBudget from k8s.io/apiserver/pkg/apis/cel/config.go as input. diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go index 6a3c2395e3c..79d78bb266b 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go @@ -19,11 +19,13 @@ package validatingadmissionpolicy import ( "context" "fmt" + "strings" "sync" "sync/atomic" "testing" "time" + celgo "github.com/google/cel-go/cel" "github.com/stretchr/testify/require" "k8s.io/klog/v2" @@ -38,14 +40,18 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + utiljson "k8s.io/apimachinery/pkg/util/json" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/admission/initializer" "k8s.io/apiserver/pkg/admission/plugin/cel" "k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/internal/generic" whgeneric "k8s.io/apiserver/pkg/admission/plugin/webhook/generic" + auditinternal "k8s.io/apiserver/pkg/apis/audit" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/features" + "k8s.io/apiserver/pkg/warning" dynamicfake "k8s.io/client-go/dynamic/fake" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" @@ -144,6 +150,7 @@ var ( Name: fakeParams.GetName(), Namespace: fakeParams.GetNamespace(), }, + ValidationActions: []v1alpha1.ValidationAction{v1alpha1.Deny}, }, } denyBindingWithNoParamRef *v1alpha1.ValidatingAdmissionPolicyBinding = &v1alpha1.ValidatingAdmissionPolicyBinding{ @@ -152,7 +159,39 @@ var ( ResourceVersion: "1", }, Spec: v1alpha1.ValidatingAdmissionPolicyBindingSpec{ - PolicyName: denyPolicy.Name, + PolicyName: denyPolicy.Name, + ValidationActions: []v1alpha1.ValidationAction{v1alpha1.Deny}, + }, + } + + denyBindingWithAudit = &v1alpha1.ValidatingAdmissionPolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "denybinding.example.com", + ResourceVersion: "1", + }, + Spec: v1alpha1.ValidatingAdmissionPolicyBindingSpec{ + PolicyName: denyPolicy.Name, + ValidationActions: []v1alpha1.ValidationAction{v1alpha1.Audit}, + }, + } + denyBindingWithWarn = &v1alpha1.ValidatingAdmissionPolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "denybinding.example.com", + ResourceVersion: "1", + }, + Spec: v1alpha1.ValidatingAdmissionPolicyBindingSpec{ + PolicyName: denyPolicy.Name, + ValidationActions: []v1alpha1.ValidationAction{v1alpha1.Warn}, + }, + } + denyBindingWithAll = &v1alpha1.ValidatingAdmissionPolicyBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "denybinding.example.com", + ResourceVersion: "1", + }, + Spec: v1alpha1.ValidatingAdmissionPolicyBindingSpec{ + PolicyName: denyPolicy.Name, + ValidationActions: []v1alpha1.ValidationAction{v1alpha1.Deny, v1alpha1.Warn, v1alpha1.Audit}, }, } ) @@ -174,12 +213,13 @@ func (f *fakeCompiler) Compile( options cel.OptionalVariableDeclarations, perCallLimit uint64, ) cel.Filter { - key := expressions[0].GetExpression() - if fun, ok := f.CompileFuncs[key]; ok { - return fun(expressions, options) + if len(expressions) > 0 { + key := expressions[0].GetExpression() + if fun, ok := f.CompileFuncs[key]; ok { + return fun(expressions, options) + } } - - return nil + return &fakeFilter{} } func (f *fakeCompiler) RegisterDefinition(definition *v1alpha1.ValidatingAdmissionPolicy, compileFunc func([]cel.ExpressionAccessor, cel.OptionalVariableDeclarations) cel.Filter) { @@ -203,6 +243,10 @@ func (f *fakeEvalRequest) GetExpression() string { return "" } +func (f *fakeEvalRequest) ReturnTypes() []*celgo.Type { + return []*celgo.Type{celgo.BoolType} +} + var _ cel.Filter = &fakeFilter{} type fakeFilter struct { @@ -220,22 +264,28 @@ func (f *fakeFilter) CompilationErrors() []error { var _ Validator = &fakeValidator{} type fakeValidator struct { - *fakeFilter - ValidateFunc func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision + validationFilter, auditAnnotationFilter *fakeFilter + ValidateFunc func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult } -func (f *fakeValidator) RegisterDefinition(definition *v1alpha1.ValidatingAdmissionPolicy, validateFunc func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision) { +func (f *fakeValidator) RegisterDefinition(definition *v1alpha1.ValidatingAdmissionPolicy, validateFunc func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult) { //Key must be something that we can decipher from the inputs to Validate so using message which will be on the validationCondition object of evalResult - validateKey := definition.Spec.Validations[0].Expression + var key string + if len(definition.Spec.Validations) > 0 { + key = definition.Spec.Validations[0].Expression + } else { + key = definition.Spec.AuditAnnotations[0].Key + } + if validatorMap == nil { validatorMap = make(map[string]*fakeValidator) } f.ValidateFunc = validateFunc - validatorMap[validateKey] = f + validatorMap[key] = f } -func (f *fakeValidator) Validate(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision { +func (f *fakeValidator) Validate(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { return f.ValidateFunc(versionedAttr, versionedParams, runtimeCELCostBudget) } @@ -369,10 +419,11 @@ func setupTestCommon(t *testing.T, compiler cel.FilterCompiler, matcher Matcher, // Override compiler used by controller for tests controller = handler.evaluator.(*celAdmissionController) controller.policyController.filterCompiler = compiler - controller.policyController.newValidator = func(filter cel.Filter, fail *admissionRegistrationv1.FailurePolicyType, authorizer authorizer.Authorizer) Validator { - f := filter.(*fakeFilter) + controller.policyController.newValidator = func(validationFilter, auditAnnotationFilter cel.Filter, fail *admissionRegistrationv1.FailurePolicyType, authorizer authorizer.Authorizer) Validator { + f := validationFilter.(*fakeFilter) v := validatorMap[f.keyId] - v.fakeFilter = f + v.validationFilter = f + v.auditAnnotationFilter = auditAnnotationFilter.(*fakeFilter) return v } controller.policyController.matcher = matcher @@ -596,7 +647,7 @@ func waitForReconcileDeletion(ctx context.Context, controller *celAdmissionContr func attributeRecord( old, new runtime.Object, operation admission.Operation, -) admission.Attributes { +) *FakeAttributes { if old == nil && new == nil { panic("both `old` and `new` may not be nil") } @@ -622,19 +673,21 @@ func attributeRecord( panic(err) } - return admission.NewAttributesRecord( - new, - old, - gvk, - accessor.GetNamespace(), - accessor.GetName(), - mapping.Resource, - "", - operation, - nil, - false, - nil, - ) + return &FakeAttributes{ + Attributes: admission.NewAttributesRecord( + new, + old, + gvk, + accessor.GetNamespace(), + accessor.GetName(), + mapping.Resource, + "", + operation, + nil, + false, + nil, + ), + } } func ptrTo[T any](obj T) *T { @@ -716,11 +769,13 @@ func TestBasicPolicyDefinitionFailure(t *testing.T) { } }) - validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision { - return []PolicyDecision{ - { - Action: ActionDeny, - Message: "Denied", + validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: ActionDeny, + Message: "Denied", + }, }, } }) @@ -737,14 +792,22 @@ func TestBasicPolicyDefinitionFailure(t *testing.T) { testContext, controller, fakeParams, denyBinding, denyPolicy)) + warningRecorder := newWarningRecorder() + warnCtx := warning.WithWarningRecorder(testContext, warningRecorder) + attr := attributeRecord(nil, fakeParams, admission.Create) err := handler.Validate( - testContext, + warnCtx, // Object is irrelevant/unchecked for this test. Just test that // the evaluator is executed, and returns a denial - attributeRecord(nil, fakeParams, admission.Create), + attr, &admission.RuntimeObjectInterfaces{}, ) + require.Equal(t, 0, warningRecorder.len()) + + annotations := attr.GetAnnotations(auditinternal.LevelMetadata) + require.Equal(t, 0, len(annotations)) + require.ErrorContains(t, err, `Denied`) } @@ -776,11 +839,13 @@ func TestDefinitionDoesntMatch(t *testing.T) { } }) - validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision { - return []PolicyDecision{ - { - Action: ActionDeny, - Message: "Denied", + validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: ActionDeny, + Message: "Denied", + }, }, } }) @@ -887,11 +952,13 @@ func TestReconfigureBinding(t *testing.T) { } }) - validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision { - return []PolicyDecision{ - { - Action: ActionDeny, - Message: "Denied", + validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: ActionDeny, + Message: "Denied", + }, }, } }) @@ -907,6 +974,7 @@ func TestReconfigureBinding(t *testing.T) { Name: fakeParams2.GetName(), Namespace: fakeParams2.GetNamespace(), }, + ValidationActions: []v1alpha1.ValidationAction{v1alpha1.Deny}, }, } @@ -994,11 +1062,13 @@ func TestRemoveDefinition(t *testing.T) { } }) - validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision { - return []PolicyDecision{ - { - Action: ActionDeny, - Message: "Denied", + validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: ActionDeny, + Message: "Denied", + }, }, } }) @@ -1061,11 +1131,13 @@ func TestRemoveBinding(t *testing.T) { } }) - validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision { - return []PolicyDecision{ - { - Action: ActionDeny, - Message: "Denied", + validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: ActionDeny, + Message: "Denied", + }, }, } }) @@ -1169,11 +1241,13 @@ func TestInvalidParamSourceInstanceName(t *testing.T) { } }) - validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision { - return []PolicyDecision{ - { - Action: ActionDeny, - Message: "Denied", + validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: ActionDeny, + Message: "Denied", + }, }, } }) @@ -1235,11 +1309,13 @@ func TestEmptyParamSource(t *testing.T) { } }) - validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision { - return []PolicyDecision{ - { - Action: ActionDeny, - Message: "Denied", + validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: ActionDeny, + Message: "Denied", + }, }, } }) @@ -1335,11 +1411,13 @@ func TestMultiplePoliciesSharedParamType(t *testing.T) { } }) - validator1.RegisterDefinition(&policy1, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision { + validator1.RegisterDefinition(&policy1, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { evaluations1.Add(1) - return []PolicyDecision{ - { - Action: ActionAdmit, + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: ActionAdmit, + }, }, } }) @@ -1352,12 +1430,14 @@ func TestMultiplePoliciesSharedParamType(t *testing.T) { } }) - validator2.RegisterDefinition(&policy2, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision { + validator2.RegisterDefinition(&policy2, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { evaluations2.Add(1) - return []PolicyDecision{ - { - Action: ActionDeny, - Message: "Policy2Denied", + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: ActionDeny, + Message: "Policy2Denied", + }, }, } }) @@ -1460,20 +1540,24 @@ func TestNativeTypeParam(t *testing.T) { } }) - validator.RegisterDefinition(&nativeTypeParamPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision { + validator.RegisterDefinition(&nativeTypeParamPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { evaluations.Add(1) if _, ok := versionedParams.(*v1.ConfigMap); ok { - return []PolicyDecision{ - { - Action: ActionDeny, - Message: "correct type", + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: ActionDeny, + Message: "correct type", + }, }, } } - return []PolicyDecision{ - { - Action: ActionDeny, - Message: "Incorrect param type", + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: ActionDeny, + Message: "Incorrect param type", + }, }, } }) @@ -1516,6 +1600,366 @@ func TestNativeTypeParam(t *testing.T) { require.EqualValues(t, 1, evaluations.Load()) } +func TestAuditValidationAction(t *testing.T) { + testContext, testContextCancel := context.WithCancel(context.Background()) + defer testContextCancel() + + compiler := &fakeCompiler{} + validator := &fakeValidator{} + matcher := &fakeMatcher{ + DefaultMatch: true, + } + handler, _, tracker, controller := setupFakeTest(t, compiler, matcher) + + // Push some fake + noParamSourcePolicy := *denyPolicy + noParamSourcePolicy.Spec.ParamKind = nil + + compiler.RegisterDefinition(denyPolicy, func([]cel.ExpressionAccessor, cel.OptionalVariableDeclarations) cel.Filter { + + return &fakeFilter{ + keyId: denyPolicy.Spec.Validations[0].Expression, + } + }) + + validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: ActionDeny, + Message: "I'm sorry Dave", + }, + }, + } + }) + + require.NoError(t, tracker.Create(definitionsGVR, &noParamSourcePolicy, noParamSourcePolicy.Namespace)) + require.NoError(t, tracker.Create(bindingsGVR, denyBindingWithAudit, denyBindingWithAudit.Namespace)) + + // Wait for controller to reconcile given objects + require.NoError(t, + waitForReconcile( + testContext, controller, + denyBindingWithAudit, &noParamSourcePolicy)) + attr := attributeRecord(nil, fakeParams, admission.Create) + warningRecorder := newWarningRecorder() + warnCtx := warning.WithWarningRecorder(testContext, warningRecorder) + err := handler.Validate( + warnCtx, + attr, + &admission.RuntimeObjectInterfaces{}, + ) + + require.Equal(t, 0, warningRecorder.len()) + + annotations := attr.GetAnnotations(auditinternal.LevelMetadata) + require.Equal(t, 1, len(annotations)) + valueJson, ok := annotations["validation.policy.admission.k8s.io/validation_failure"] + require.True(t, ok) + var value []validationFailureValue + jsonErr := utiljson.Unmarshal([]byte(valueJson), &value) + require.NoError(t, jsonErr) + expected := []validationFailureValue{{ + ExpressionIndex: 0, + Message: "I'm sorry Dave", + ValidationActions: []v1alpha1.ValidationAction{v1alpha1.Audit}, + Binding: "denybinding.example.com", + Policy: noParamSourcePolicy.Name, + }} + require.Equal(t, expected, value) + + require.NoError(t, err) +} + +func TestWarnValidationAction(t *testing.T) { + testContext, testContextCancel := context.WithCancel(context.Background()) + defer testContextCancel() + + compiler := &fakeCompiler{} + validator := &fakeValidator{} + matcher := &fakeMatcher{ + DefaultMatch: true, + } + handler, _, tracker, controller := setupFakeTest(t, compiler, matcher) + + // Push some fake + noParamSourcePolicy := *denyPolicy + noParamSourcePolicy.Spec.ParamKind = nil + + compiler.RegisterDefinition(denyPolicy, func([]cel.ExpressionAccessor, cel.OptionalVariableDeclarations) cel.Filter { + + return &fakeFilter{ + keyId: denyPolicy.Spec.Validations[0].Expression, + } + }) + + validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: ActionDeny, + Message: "I'm sorry Dave", + }, + }, + } + }) + + require.NoError(t, tracker.Create(definitionsGVR, &noParamSourcePolicy, noParamSourcePolicy.Namespace)) + require.NoError(t, tracker.Create(bindingsGVR, denyBindingWithWarn, denyBindingWithWarn.Namespace)) + + // Wait for controller to reconcile given objects + require.NoError(t, + waitForReconcile( + testContext, controller, + denyBindingWithWarn, &noParamSourcePolicy)) + attr := attributeRecord(nil, fakeParams, admission.Create) + warningRecorder := newWarningRecorder() + warnCtx := warning.WithWarningRecorder(testContext, warningRecorder) + err := handler.Validate( + warnCtx, + attr, + &admission.RuntimeObjectInterfaces{}, + ) + + require.Equal(t, 1, warningRecorder.len()) + require.True(t, warningRecorder.hasWarning("Validation failed for ValidatingAdmissionPolicy 'denypolicy.example.com' with binding 'denybinding.example.com': I'm sorry Dave")) + + annotations := attr.GetAnnotations(auditinternal.LevelMetadata) + require.Equal(t, 0, len(annotations)) + + require.NoError(t, err) +} + +func TestAllValidationActions(t *testing.T) { + testContext, testContextCancel := context.WithCancel(context.Background()) + defer testContextCancel() + + compiler := &fakeCompiler{} + validator := &fakeValidator{} + matcher := &fakeMatcher{ + DefaultMatch: true, + } + handler, _, tracker, controller := setupFakeTest(t, compiler, matcher) + + // Push some fake + noParamSourcePolicy := *denyPolicy + noParamSourcePolicy.Spec.ParamKind = nil + + compiler.RegisterDefinition(denyPolicy, func([]cel.ExpressionAccessor, cel.OptionalVariableDeclarations) cel.Filter { + + return &fakeFilter{ + keyId: denyPolicy.Spec.Validations[0].Expression, + } + }) + + validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: ActionDeny, + Message: "I'm sorry Dave", + }, + }, + } + }) + + require.NoError(t, tracker.Create(definitionsGVR, &noParamSourcePolicy, noParamSourcePolicy.Namespace)) + require.NoError(t, tracker.Create(bindingsGVR, denyBindingWithAll, denyBindingWithAll.Namespace)) + + // Wait for controller to reconcile given objects + require.NoError(t, + waitForReconcile( + testContext, controller, + denyBindingWithAll, &noParamSourcePolicy)) + attr := attributeRecord(nil, fakeParams, admission.Create) + warningRecorder := newWarningRecorder() + warnCtx := warning.WithWarningRecorder(testContext, warningRecorder) + err := handler.Validate( + warnCtx, + attr, + &admission.RuntimeObjectInterfaces{}, + ) + + require.Equal(t, 1, warningRecorder.len()) + require.True(t, warningRecorder.hasWarning("Validation failed for ValidatingAdmissionPolicy 'denypolicy.example.com' with binding 'denybinding.example.com': I'm sorry Dave")) + + annotations := attr.GetAnnotations(auditinternal.LevelMetadata) + require.Equal(t, 1, len(annotations)) + valueJson, ok := annotations["validation.policy.admission.k8s.io/validation_failure"] + require.True(t, ok) + var value []validationFailureValue + jsonErr := utiljson.Unmarshal([]byte(valueJson), &value) + require.NoError(t, jsonErr) + expected := []validationFailureValue{{ + ExpressionIndex: 0, + Message: "I'm sorry Dave", + ValidationActions: []v1alpha1.ValidationAction{v1alpha1.Deny, v1alpha1.Warn, v1alpha1.Audit}, + Binding: "denybinding.example.com", + Policy: noParamSourcePolicy.Name, + }} + require.Equal(t, expected, value) + + require.ErrorContains(t, err, "I'm sorry Dave") +} + +func TestAuditAnnotations(t *testing.T) { + testContext, testContextCancel := context.WithCancel(context.Background()) + defer testContextCancel() + + compiler := &fakeCompiler{} + validator := &fakeValidator{} + matcher := &fakeMatcher{ + DefaultMatch: true, + } + handler, paramsTracker, tracker, controller := setupFakeTest(t, compiler, matcher) + + // Push some fake + policy := *denyPolicy + + compiler.RegisterDefinition(denyPolicy, func([]cel.ExpressionAccessor, cel.OptionalVariableDeclarations) cel.Filter { + + return &fakeFilter{ + keyId: denyPolicy.Spec.Validations[0].Expression, + } + }) + + validator.RegisterDefinition(denyPolicy, func(versionedAttr *whgeneric.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { + o, err := meta.Accessor(versionedParams) + if err != nil { + t.Fatal(err) + } + exampleValue := "normal-value" + if o.GetName() == "replicas-test2.example.com" { + exampleValue = "special-value" + } + return ValidateResult{ + AuditAnnotations: []PolicyAuditAnnotation{ + { + Key: "example-key", + Value: exampleValue, + Action: AuditAnnotationActionPublish, + }, + { + Key: "excluded-key", + Value: "excluded-value", + Action: AuditAnnotationActionExclude, + }, + { + Key: "error-key", + Action: AuditAnnotationActionError, + Error: "example error", + }, + }, + } + }) + + fakeParams2 := fakeParams.DeepCopy() + fakeParams2.SetName("replicas-test2.example.com") + denyBinding2 := denyBinding.DeepCopy() + denyBinding2.SetName("denybinding2.example.com") + denyBinding2.Spec.ParamRef.Name = fakeParams2.GetName() + + fakeParams3 := fakeParams.DeepCopy() + fakeParams3.SetName("replicas-test3.example.com") + denyBinding3 := denyBinding.DeepCopy() + denyBinding3.SetName("denybinding3.example.com") + denyBinding3.Spec.ParamRef.Name = fakeParams3.GetName() + + require.NoError(t, paramsTracker.Add(fakeParams)) + require.NoError(t, paramsTracker.Add(fakeParams2)) + require.NoError(t, paramsTracker.Add(fakeParams3)) + require.NoError(t, tracker.Create(definitionsGVR, &policy, policy.Namespace)) + require.NoError(t, tracker.Create(bindingsGVR, denyBinding, denyBinding.Namespace)) + require.NoError(t, tracker.Create(bindingsGVR, denyBinding2, denyBinding2.Namespace)) + require.NoError(t, tracker.Create(bindingsGVR, denyBinding3, denyBinding3.Namespace)) + + // Wait for controller to reconcile given objects + require.NoError(t, + waitForReconcile( + testContext, controller, + denyBinding, denyBinding2, denyBinding3, denyPolicy, fakeParams, fakeParams2, fakeParams3)) + attr := attributeRecord(nil, fakeParams, admission.Create) + err := handler.Validate( + testContext, + attr, + &admission.RuntimeObjectInterfaces{}, + ) + + annotations := attr.GetAnnotations(auditinternal.LevelMetadata) + require.Equal(t, 1, len(annotations)) + value := annotations[policy.Name+"/example-key"] + parts := strings.Split(value, ", ") + require.Equal(t, 2, len(parts)) + require.Contains(t, parts, "normal-value", "special-value") + + require.ErrorContains(t, err, "example error") +} + +// FakeAttributes decorates admission.Attributes. It's used to trace the added annotations. +type FakeAttributes struct { + admission.Attributes + annotations map[string]string + mutex sync.Mutex +} + +// AddAnnotation adds an annotation key value pair to FakeAttributes +func (f *FakeAttributes) AddAnnotation(k, v string) error { + return f.AddAnnotationWithLevel(k, v, auditinternal.LevelMetadata) +} + +// AddAnnotationWithLevel adds an annotation key value pair to FakeAttributes +func (f *FakeAttributes) AddAnnotationWithLevel(k, v string, _ auditinternal.Level) error { + f.mutex.Lock() + defer f.mutex.Unlock() + if err := f.Attributes.AddAnnotation(k, v); err != nil { + return err + } + if f.annotations == nil { + f.annotations = make(map[string]string) + } + f.annotations[k] = v + return nil +} + +// GetAnnotations reads annotations from FakeAttributes +func (f *FakeAttributes) GetAnnotations(_ auditinternal.Level) map[string]string { + f.mutex.Lock() + defer f.mutex.Unlock() + annotations := make(map[string]string, len(f.annotations)) + for k, v := range f.annotations { + annotations[k] = v + } + return annotations +} + +type warningRecorder struct { + sync.Mutex + warnings sets.Set[string] +} + +func newWarningRecorder() *warningRecorder { + return &warningRecorder{warnings: sets.New[string]()} +} + +func (r *warningRecorder) AddWarning(_, text string) { + r.Lock() + defer r.Unlock() + r.warnings.Insert(text) + return +} + +func (r *warningRecorder) hasWarning(text string) bool { + r.Lock() + defer r.Unlock() + return r.warnings.Has(text) +} + +func (r *warningRecorder) len() int { + r.Lock() + defer r.Unlock() + return len(r.warnings) +} + type fakeAuthorizer struct{} func (f fakeAuthorizer) Authorize(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) { diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go index 52ef2fb0085..7cdb86e4d0a 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller.go @@ -20,16 +20,19 @@ import ( "context" "errors" "fmt" - celconfig "k8s.io/apiserver/pkg/apis/cel" + "strings" "sync" "sync/atomic" "time" + "k8s.io/klog/v2" + "k8s.io/api/admissionregistration/v1alpha1" k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + utiljson "k8s.io/apimachinery/pkg/util/json" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" @@ -39,7 +42,9 @@ import ( "k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/internal/generic" "k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/matching" whgeneric "k8s.io/apiserver/pkg/admission/plugin/webhook/generic" + celconfig "k8s.io/apiserver/pkg/apis/cel" "k8s.io/apiserver/pkg/authorization/authorizer" + "k8s.io/apiserver/pkg/warning" "k8s.io/client-go/dynamic" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" @@ -169,6 +174,8 @@ func (c *celAdmissionController) Run(stopCh <-chan struct{}) { wg.Wait() } +const maxAuditAnnotationValueLength = 10 * 1024 + func (c *celAdmissionController) Validate( ctx context.Context, a admission.Attributes, @@ -239,6 +246,7 @@ func (c *celAdmissionController) Validate( continue } + auditAnnotationCollector := newAuditAnnotationCollector() for _, bindingInfo := range definitionInfo.bindings { // If the key is inside dependentBindings, there is guaranteed to // be a bindingInfo for it @@ -321,27 +329,72 @@ func (c *celAdmissionController) Validate( versionedAttr = va } - decisions := bindingInfo.validator.Validate(versionedAttr, param, celconfig.RuntimeCELCostBudget) + validationResult := bindingInfo.validator.Validate(versionedAttr, param, celconfig.RuntimeCELCostBudget) + if err != nil { + // runtime error. Apply failure policy + wrappedError := fmt.Errorf("failed to evaluate CEL expression: %w", err) + addConfigError(wrappedError, definition, binding) + continue + } - for _, decision := range decisions { + for i, decision := range validationResult.Decisions { switch decision.Action { case ActionAdmit: if decision.Evaluation == EvalError { celmetrics.Metrics.ObserveAdmissionWithError(ctx, decision.Elapsed, definition.Name, binding.Name, "active") } case ActionDeny: - deniedDecisions = append(deniedDecisions, policyDecisionWithMetadata{ - Definition: definition, - Binding: binding, - PolicyDecision: decision, - }) - celmetrics.Metrics.ObserveRejection(ctx, decision.Elapsed, definition.Name, binding.Name, "active") + for _, action := range binding.Spec.ValidationActions { + switch action { + case v1alpha1.Deny: + deniedDecisions = append(deniedDecisions, policyDecisionWithMetadata{ + Definition: definition, + Binding: binding, + PolicyDecision: decision, + }) + celmetrics.Metrics.ObserveRejection(ctx, decision.Elapsed, definition.Name, binding.Name, "active") + case v1alpha1.Audit: + c.publishValidationFailureAnnotation(binding, i, decision, versionedAttr) + celmetrics.Metrics.ObserveAudit(ctx, decision.Elapsed, definition.Name, binding.Name, "active") + case v1alpha1.Warn: + warning.AddWarning(ctx, "", fmt.Sprintf("Validation failed for ValidatingAdmissionPolicy '%s' with binding '%s': %s", definition.Name, binding.Name, decision.Message)) + celmetrics.Metrics.ObserveWarn(ctx, decision.Elapsed, definition.Name, binding.Name, "active") + } + } default: return fmt.Errorf("unrecognized evaluation decision '%s' for ValidatingAdmissionPolicyBinding '%s' with ValidatingAdmissionPolicy '%s'", decision.Action, binding.Name, definition.Name) } } + + for _, auditAnnotation := range validationResult.AuditAnnotations { + switch auditAnnotation.Action { + case AuditAnnotationActionPublish: + value := auditAnnotation.Value + if len(auditAnnotation.Value) > maxAuditAnnotationValueLength { + value = value[:maxAuditAnnotationValueLength] + } + auditAnnotationCollector.add(auditAnnotation.Key, value) + case AuditAnnotationActionError: + // When failurePolicy=fail, audit annotation errors result in deny + deniedDecisions = append(deniedDecisions, policyDecisionWithMetadata{ + Definition: definition, + Binding: binding, + PolicyDecision: PolicyDecision{ + Action: ActionDeny, + Evaluation: EvalError, + Message: auditAnnotation.Error, + Elapsed: auditAnnotation.Elapsed, + }, + }) + celmetrics.Metrics.ObserveRejection(ctx, auditAnnotation.Elapsed, definition.Name, binding.Name, "active") + case AuditAnnotationActionExclude: // skip it + default: + return fmt.Errorf("unsupported AuditAnnotation Action: %s", auditAnnotation.Action) + } + } } + auditAnnotationCollector.publish(definition.Name, a) } if len(deniedDecisions) > 0 { @@ -366,6 +419,25 @@ func (c *celAdmissionController) Validate( return nil } +func (c *celAdmissionController) publishValidationFailureAnnotation(binding *v1alpha1.ValidatingAdmissionPolicyBinding, expressionIndex int, decision PolicyDecision, attributes admission.Attributes) { + key := "validation.policy.admission.k8s.io/validation_failure" + // Marshal to a list of failures since, in the future, we may need to support multiple failures + valueJson, err := utiljson.Marshal([]validationFailureValue{{ + ExpressionIndex: expressionIndex, + Message: decision.Message, + ValidationActions: binding.Spec.ValidationActions, + Binding: binding.Name, + Policy: binding.Spec.PolicyName, + }}) + if err != nil { + klog.Warningf("Failed to set admission audit annotation %s for ValidatingAdmissionPolicy %s and ValidatingAdmissionPolicyBinding %s: %v", key, binding.Spec.PolicyName, binding.Name, err) + } + value := string(valueJson) + if err := attributes.AddAnnotation(key, value); err != nil { + klog.Warningf("Failed to set admission audit annotation %s to %s for ValidatingAdmissionPolicy %s and ValidatingAdmissionPolicyBinding %s: %v", key, value, binding.Spec.PolicyName, binding.Name, err) + } +} + func (c *celAdmissionController) HasSynced() bool { return c.policyController.HasSynced() && c.definitions.Load() != nil } @@ -377,3 +449,48 @@ func (c *celAdmissionController) ValidateInitialization() error { func (c *celAdmissionController) refreshPolicies() { c.definitions.Store(c.policyController.latestPolicyData()) } + +// validationFailureValue defines the JSON format of a "validation.policy.admission.k8s.io/validation_failure" audit +// annotation value. +type validationFailureValue struct { + Message string `json:"message"` + Policy string `json:"policy"` + Binding string `json:"binding"` + ExpressionIndex int `json:"expressionIndex"` + ValidationActions []v1alpha1.ValidationAction `json:"validationActions"` +} + +type auditAnnotationCollector struct { + annotations map[string][]string +} + +func newAuditAnnotationCollector() auditAnnotationCollector { + return auditAnnotationCollector{annotations: map[string][]string{}} +} + +func (a auditAnnotationCollector) add(key, value string) { + // If multiple bindings produces the exact same key and value for an audit annotation, + // ignore the duplicates. + for _, v := range a.annotations[key] { + if v == value { + return + } + } + a.annotations[key] = append(a.annotations[key], value) +} + +func (a auditAnnotationCollector) publish(policyName string, attributes admission.Attributes) { + for key, bindingAnnotations := range a.annotations { + var value string + if len(bindingAnnotations) == 1 { + value = bindingAnnotations[0] + } else { + // Multiple distinct values can exist when binding params are used in the valueExpression of an auditAnnotation. + // When this happens, the values are concatenated into a comma-separated list. + value = strings.Join(bindingAnnotations, ", ") + } + if err := attributes.AddAnnotation(policyName+"/"+key, value); err != nil { + klog.Warningf("Failed to set admission audit annotation %s to %s for ValidatingAdmissionPolicy %s: %v", key, value, policyName, err) + } + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go index 9c72319a6ff..7f9c0a50ff0 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go @@ -92,7 +92,7 @@ type policyController struct { authz authorizer.Authorizer } -type newValidator func(cel.Filter, *v1.FailurePolicyType, authorizer.Authorizer) Validator +type newValidator func(validationFilter cel.Filter, auditAnnotationFilter cel.Filter, failurePolicy *v1.FailurePolicyType, authorizer authorizer.Authorizer) Validator func newPolicyController( restMapper meta.RESTMapper, @@ -461,6 +461,7 @@ func (c *policyController) latestPolicyData() []policyData { optionalVars := cel.OptionalVariableDeclarations{HasParams: hasParam, HasAuthorizer: true} bindingInfo.validator = c.newValidator( c.filterCompiler.Compile(convertv1alpha1Validations(definitionInfo.lastReconciledValue.Spec.Validations), optionalVars, celconfig.PerCallLimit), + c.filterCompiler.Compile(convertv1alpha1AuditAnnotations(definitionInfo.lastReconciledValue.Spec.AuditAnnotations), optionalVars, celconfig.PerCallLimit), convertv1alpha1FailurePolicyTypeTov1FailurePolicyType(definitionInfo.lastReconciledValue.Spec.FailurePolicy), c.authz, ) @@ -513,6 +514,18 @@ func convertv1alpha1Validations(inputValidations []v1alpha1.Validation) []cel.Ex return celExpressionAccessor } +func convertv1alpha1AuditAnnotations(inputValidations []v1alpha1.AuditAnnotation) []cel.ExpressionAccessor { + celExpressionAccessor := make([]cel.ExpressionAccessor, len(inputValidations)) + for i, validation := range inputValidations { + validation := AuditAnnotationCondition{ + Key: validation.Key, + ValueExpression: validation.ValueExpression, + } + celExpressionAccessor[i] = &validation + } + return celExpressionAccessor +} + func getNamespaceName(namespace, name string) namespacedName { return namespacedName{ namespace: namespace, diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/interface.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/interface.go index 23ec19cd397..072b7e94aa0 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/interface.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/interface.go @@ -17,6 +17,8 @@ limitations under the License. package validatingadmissionpolicy import ( + celgo "github.com/google/cel-go/cel" + "k8s.io/api/admissionregistration/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -39,6 +41,24 @@ func (v *ValidationCondition) GetExpression() string { return v.Expression } +func (v *ValidationCondition) ReturnTypes() []*celgo.Type { + return []*celgo.Type{celgo.BoolType} +} + +// AuditAnnotationCondition contains the inputs needed to compile, evaluate and publish a cel audit annotation +type AuditAnnotationCondition struct { + Key string + ValueExpression string +} + +func (v *AuditAnnotationCondition) GetExpression() string { + return v.ValueExpression +} + +func (v *AuditAnnotationCondition) ReturnTypes() []*celgo.Type { + return []*celgo.Type{celgo.StringType, celgo.NullType} +} + // Matcher is used for matching ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding to attributes type Matcher interface { admission.InitializationValidator @@ -52,9 +72,17 @@ type Matcher interface { BindingMatches(a admission.Attributes, o admission.ObjectInterfaces, definition *v1alpha1.ValidatingAdmissionPolicyBinding) (bool, error) } +// ValidateResult defines the result of a Validator.Validate operation. +type ValidateResult struct { + // Decisions specifies the outcome of the validation as well as the details about the decision. + Decisions []PolicyDecision + // AuditAnnotations specifies the audit annotations that should be recorded for the validation. + AuditAnnotations []PolicyAuditAnnotation +} + // Validator is contains logic for converting ValidationEvaluation to PolicyDecisions type Validator interface { // Validate is used to take cel evaluations and convert into decisions // runtimeCELCostBudget was added for testing purpose only. Callers should always use const RuntimeCELCostBudget from k8s.io/apiserver/pkg/apis/cel/config.go as input. - Validate(versionedAttr *generic.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision + Validate(versionedAttr *generic.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/policy_decision.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/policy_decision.go index 47e50dd62c8..939cbea70c0 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/policy_decision.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/policy_decision.go @@ -47,6 +47,29 @@ type PolicyDecision struct { Elapsed time.Duration } +type PolicyAuditAnnotationAction string + +const ( + // AuditAnnotationActionPublish indicates that the audit annotation should be + // published with the audit event. + AuditAnnotationActionPublish PolicyAuditAnnotationAction = "publish" + // AuditAnnotationActionError indicates that the valueExpression resulted + // in an error. + AuditAnnotationActionError PolicyAuditAnnotationAction = "error" + // AuditAnnotationActionExclude indicates that the audit annotation should be excluded + // because the valueExpression evaluated to null, or because FailurePolicy is Ignore + // and the expression failed with a parse error, type check error, or runtime error. + AuditAnnotationActionExclude PolicyAuditAnnotationAction = "exclude" +) + +type PolicyAuditAnnotation struct { + Key string + Value string + Elapsed time.Duration + Action PolicyAuditAnnotationAction + Error string +} + func reasonToCode(r metav1.StatusReason) int32 { switch r { case metav1.StatusReasonForbidden: diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/validator.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/validator.go index 39efdd61daf..a1ee66b57a4 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/validator.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/validator.go @@ -21,6 +21,7 @@ import ( "strings" celtypes "github.com/google/cel-go/common/types" + "k8s.io/klog/v2" v1 "k8s.io/api/admissionregistration/v1" @@ -33,16 +34,18 @@ import ( // validator implements the Validator interface type validator struct { - filter cel.Filter - failPolicy *v1.FailurePolicyType - authorizer authorizer.Authorizer + validationFilter cel.Filter + auditAnnotationFilter cel.Filter + failPolicy *v1.FailurePolicyType + authorizer authorizer.Authorizer } -func NewValidator(filter cel.Filter, failPolicy *v1.FailurePolicyType, authorizer authorizer.Authorizer) Validator { +func NewValidator(validationFilter, auditAnnotationFilter cel.Filter, failPolicy *v1.FailurePolicyType, authorizer authorizer.Authorizer) Validator { return &validator{ - filter: filter, - failPolicy: failPolicy, - authorizer: authorizer, + validationFilter: validationFilter, + auditAnnotationFilter: auditAnnotationFilter, + failPolicy: failPolicy, + authorizer: authorizer, } } @@ -53,9 +56,16 @@ func policyDecisionActionForError(f v1.FailurePolicyType) PolicyDecisionAction { return ActionDeny } +func auditAnnotationEvaluationForError(f v1.FailurePolicyType) PolicyAuditAnnotationAction { + if f == v1.Ignore { + return AuditAnnotationActionExclude + } + return AuditAnnotationActionError +} + // Validate takes a list of Evaluation and a failure policy and converts them into actionable PolicyDecisions // runtimeCELCostBudget was added for testing purpose only. Callers should always use const RuntimeCELCostBudget from k8s.io/apiserver/pkg/apis/cel/config.go as input. -func (v *validator) Validate(versionedAttr *generic.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) []PolicyDecision { +func (v *validator) Validate(versionedAttr *generic.VersionedAttributes, versionedParams runtime.Object, runtimeCELCostBudget int64) ValidateResult { var f v1.FailurePolicyType if v.failPolicy == nil { f = v1.Fail @@ -64,13 +74,15 @@ func (v *validator) Validate(versionedAttr *generic.VersionedAttributes, version } optionalVars := cel.OptionalVariableBindings{VersionedParams: versionedParams, Authorizer: v.authorizer} - evalResults, err := v.filter.ForInput(versionedAttr, cel.CreateAdmissionRequest(versionedAttr.Attributes), optionalVars, runtimeCELCostBudget) + evalResults, err := v.validationFilter.ForInput(versionedAttr, cel.CreateAdmissionRequest(versionedAttr.Attributes), optionalVars, runtimeCELCostBudget) if err != nil { - return []PolicyDecision{ - { - Action: policyDecisionActionForError(f), - Evaluation: EvalError, - Message: err.Error(), + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: policyDecisionActionForError(f), + Evaluation: EvalError, + Message: err.Error(), + }, }, } } @@ -104,11 +116,60 @@ func (v *validator) Validate(versionedAttr *generic.VersionedAttributes, version } else { decision.Message = fmt.Sprintf("failed expression: %v", strings.TrimSpace(validation.Expression)) } - } else { decision.Action = ActionAdmit decision.Evaluation = EvalAdmit } } - return decisions + + options := cel.OptionalVariableBindings{VersionedParams: versionedParams} + auditAnnotationEvalResults, err := v.auditAnnotationFilter.ForInput(versionedAttr, cel.CreateAdmissionRequest(versionedAttr.Attributes), options, runtimeCELCostBudget) + if err != nil { + return ValidateResult{ + Decisions: []PolicyDecision{ + { + Action: policyDecisionActionForError(f), + Evaluation: EvalError, + Message: err.Error(), + }, + }, + } + } + + auditAnnotationResults := make([]PolicyAuditAnnotation, len(auditAnnotationEvalResults)) + for i, evalResult := range auditAnnotationEvalResults { + var auditAnnotationResult = &auditAnnotationResults[i] + // TODO: move this to generics + validation, ok := evalResult.ExpressionAccessor.(*AuditAnnotationCondition) + if !ok { + klog.Error("Invalid type conversion to AuditAnnotationCondition") + auditAnnotationResult.Action = auditAnnotationEvaluationForError(f) + auditAnnotationResult.Error = fmt.Sprintf("Invalid type sent to validator, expected AuditAnnotationCondition but got %T", evalResult.ExpressionAccessor) + continue + } + auditAnnotationResult.Key = validation.Key + + if evalResult.Error != nil { + auditAnnotationResult.Action = auditAnnotationEvaluationForError(f) + auditAnnotationResult.Error = evalResult.Error.Error() + } else { + switch evalResult.EvalResult.Type() { + case celtypes.StringType: + value := strings.TrimSpace(evalResult.EvalResult.Value().(string)) + if len(value) == 0 { + auditAnnotationResult.Action = AuditAnnotationActionExclude + } else { + auditAnnotationResult.Action = AuditAnnotationActionPublish + auditAnnotationResult.Value = value + } + case celtypes.NullType: + auditAnnotationResult.Action = AuditAnnotationActionExclude + default: + auditAnnotationResult.Action = AuditAnnotationActionError + auditAnnotationResult.Error = fmt.Sprintf("valueExpression '%v' resulted in unsupported return type: %v. "+ + "Return type must be either string or null.", validation.ValueExpression, evalResult.EvalResult.Type()) + } + } + } + return ValidateResult{Decisions: decisions, AuditAnnotations: auditAnnotationResults} } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/validator_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/validator_test.go index e5e1160f4c7..706e1987b54 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/validator_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/validator_test.go @@ -18,6 +18,7 @@ package validatingadmissionpolicy import ( "errors" + "fmt" "strings" "testing" @@ -64,11 +65,13 @@ func TestValidate(t *testing.T) { fakeVersionedAttr, _ := generic.NewVersionedAttributes(fakeAttr, schema.GroupVersionKind{}, nil) cases := []struct { - name string - failPolicy *v1.FailurePolicyType - evaluations []cel.EvaluationResult - policyDecision []PolicyDecision - throwError bool + name string + failPolicy *v1.FailurePolicyType + evaluations []cel.EvaluationResult + auditEvaluations []cel.EvaluationResult + policyDecision []PolicyDecision + auditAnnotations []PolicyAuditAnnotation + throwError bool }{ { name: "test pass", @@ -455,30 +458,161 @@ func TestValidate(t *testing.T) { failPolicy: &fail, throwError: true, }, + { + name: "test empty validations with non-empty audit annotations", + auditEvaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.String("string value"), + ExpressionAccessor: &AuditAnnotationCondition{ + ValueExpression: "'string value'", + }, + }, + }, + failPolicy: &fail, + auditAnnotations: []PolicyAuditAnnotation{ + { + Action: AuditAnnotationActionPublish, + Value: "string value", + }, + }, + }, + { + name: "test non-empty validations with non-empty audit annotations", + evaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.True, + ExpressionAccessor: &ValidationCondition{ + Reason: &forbiddenReason, + Expression: "this.expression == unit.test", + Message: "test1", + }, + }, + }, + auditEvaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.String("string value"), + ExpressionAccessor: &AuditAnnotationCondition{ + ValueExpression: "'string value'", + }, + }, + }, + policyDecision: []PolicyDecision{ + { + Action: ActionAdmit, + }, + }, + auditAnnotations: []PolicyAuditAnnotation{ + { + Action: AuditAnnotationActionPublish, + Value: "string value", + }, + }, + failPolicy: &fail, + }, + { + name: "test audit annotations with null return", + auditEvaluations: []cel.EvaluationResult{ + { + EvalResult: celtypes.NullValue, + ExpressionAccessor: &AuditAnnotationCondition{ + ValueExpression: "null", + }, + }, + { + EvalResult: celtypes.String("string value"), + ExpressionAccessor: &AuditAnnotationCondition{ + ValueExpression: "'string value'", + }, + }, + }, + auditAnnotations: []PolicyAuditAnnotation{ + { + Action: AuditAnnotationActionExclude, + }, + { + Action: AuditAnnotationActionPublish, + Value: "string value", + }, + }, + failPolicy: &fail, + }, + { + name: "test audit annotations with failPolicy=fail", + auditEvaluations: []cel.EvaluationResult{ + { + Error: fmt.Errorf("valueExpression ''this is not valid CEL' resulted in error: "), + ExpressionAccessor: &AuditAnnotationCondition{ + ValueExpression: "'this is not valid CEL", + }, + }, + }, + auditAnnotations: []PolicyAuditAnnotation{ + { + Action: AuditAnnotationActionError, + Error: "valueExpression ''this is not valid CEL' resulted in error: ", + }, + }, + failPolicy: &fail, + }, + { + name: "test audit annotations with failPolicy=ignore", + auditEvaluations: []cel.EvaluationResult{ + { + Error: fmt.Errorf("valueExpression ''this is not valid CEL' resulted in error: "), + ExpressionAccessor: &AuditAnnotationCondition{ + ValueExpression: "'this is not valid CEL", + }, + }, + }, + auditAnnotations: []PolicyAuditAnnotation{ + { + Action: AuditAnnotationActionExclude, // TODO: is this right? + Error: "valueExpression ''this is not valid CEL' resulted in error: ", + }, + }, + failPolicy: &ignore, + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { v := validator{ failPolicy: tc.failPolicy, - filter: &fakeCelFilter{ + validationFilter: &fakeCelFilter{ evaluations: tc.evaluations, throwError: tc.throwError, }, + auditAnnotationFilter: &fakeCelFilter{ + evaluations: tc.auditEvaluations, + throwError: tc.throwError, + }, } + validateResult := v.Validate(fakeVersionedAttr, nil, celconfig.RuntimeCELCostBudget) - policyResults := v.Validate(fakeVersionedAttr, nil, celconfig.RuntimeCELCostBudget) - - require.Equal(t, len(policyResults), len(tc.policyDecision)) + require.Equal(t, len(validateResult.Decisions), len(tc.policyDecision)) for i, policyDecision := range tc.policyDecision { - if policyDecision.Action != policyResults[i].Action { - t.Errorf("Expected policy decision kind '%v' but got '%v'", policyDecision.Action, policyResults[i].Action) + if policyDecision.Action != validateResult.Decisions[i].Action { + t.Errorf("Expected policy decision kind '%v' but got '%v'", policyDecision.Action, validateResult.Decisions[i].Action) } - if !strings.Contains(policyResults[i].Message, policyDecision.Message) { - t.Errorf("Expected policy decision message contains '%v' but got '%v'", policyDecision.Message, policyResults[i].Message) + if !strings.Contains(validateResult.Decisions[i].Message, policyDecision.Message) { + t.Errorf("Expected policy decision message contains '%v' but got '%v'", policyDecision.Message, validateResult.Decisions[i].Message) } - if policyDecision.Reason != policyResults[i].Reason { - t.Errorf("Expected policy decision reason '%v' but got '%v'", policyDecision.Reason, policyResults[i].Reason) + if policyDecision.Reason != validateResult.Decisions[i].Reason { + t.Errorf("Expected policy decision reason '%v' but got '%v'", policyDecision.Reason, validateResult.Decisions[i].Reason) + } + } + require.Equal(t, len(tc.auditEvaluations), len(validateResult.AuditAnnotations)) + + for i, auditAnnotation := range tc.auditAnnotations { + actual := validateResult.AuditAnnotations[i] + if auditAnnotation.Action != actual.Action { + t.Errorf("Expected policy audit annotation action '%v' but got '%v'", auditAnnotation.Action, actual.Action) + } + if auditAnnotation.Error != actual.Error { + t.Errorf("Expected audit annotation error '%v' but got '%v'", auditAnnotation.Error, actual.Error) + } + if auditAnnotation.Value != actual.Value { + t.Errorf("Expected policy audit annotation value '%v' but got '%v'", auditAnnotation.Value, actual.Value) } } })