Merge pull request #121223 from ritazh/authz-cel

[StructuredAuthorizationConfig] - CEL integration

pkg/kubeapiserver/authorizer/config.go

@@ -19,6 +19,7 @@ package authorizer
 import (
 	"errors"
 	"fmt"
+
 	utilnet "k8s.io/apimachinery/pkg/util/net"
 	"k8s.io/apimachinery/pkg/util/wait"
 	authzconfig "k8s.io/apiserver/pkg/apis/apiserver"
@@ -122,6 +123,7 @@ func (config Config) New() (authorizer.Authorizer, authorizer.RuleResolver, erro
 				configuredAuthorizer.Webhook.AuthorizedTTL.Duration,
 				configuredAuthorizer.Webhook.UnauthorizedTTL.Duration,
 				*config.WebhookRetryBackoff,
+				configuredAuthorizer.Webhook.MatchConditions,
 			)
 			if err != nil {
 				return nil, nil, err

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go

@@ -17,8 +17,8 @@ limitations under the License.
 package validation
 
 import (
+	"errors"
 	"fmt"
-	utilvalidation "k8s.io/apimachinery/pkg/util/validation"
 	"net/url"
 	"os"
 	"path/filepath"
@@ -27,10 +27,15 @@ import (
 
 	v1 "k8s.io/api/authorization/v1"
 	"k8s.io/api/authorization/v1beta1"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
+	utilvalidation "k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	api "k8s.io/apiserver/pkg/apis/apiserver"
+	authorizationcel "k8s.io/apiserver/pkg/authorization/cel"
+	"k8s.io/apiserver/pkg/cel"
+	"k8s.io/apiserver/pkg/cel/environment"
+	"k8s.io/apiserver/pkg/features"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/util/cert"
 )
 
@@ -334,24 +339,81 @@ func ValidateWebhookConfiguration(fldPath *field.Path, c *api.WebhookConfigurati
 		allErrs = append(allErrs, field.NotSupported(fldPath.Child("connectionInfo", "type"), c.ConnectionInfo, []string{api.AuthorizationWebhookConnectionInfoTypeInCluster, api.AuthorizationWebhookConnectionInfoTypeKubeConfigFile}))
 	}
 
-	// TODO: Remove this check and ensure that correct validations below for MatchConditions are added
-	// for i, condition := range c.MatchConditions {
-	//	 fldPath := fldPath.Child("matchConditions").Index(i).Child("expression")
-	//	 if len(strings.TrimSpace(condition.Expression)) == 0 {
-	//	     allErrs = append(allErrs, field.Required(fldPath, ""))
-	//	 } else {
-	//		 allErrs = append(allErrs, ValidateWebhookMatchCondition(fldPath, sampleSAR, condition.Expression)...)
-	//	 }
-	// }
-	if len(c.MatchConditions) != 0 {
-		allErrs = append(allErrs, field.NotSupported(fldPath.Child("matchConditions"), c.MatchConditions, []string{}))
-	}
+	_, errs := compileMatchConditions(c.MatchConditions, fldPath, utilfeature.DefaultFeatureGate.Enabled(features.StructuredAuthorizationConfiguration))
+	allErrs = append(allErrs, errs...)
 
 	return allErrs
 }
 
-func ValidateWebhookMatchCondition(fldPath *field.Path, sampleSAR runtime.Object, expression string) field.ErrorList {
-	allErrs := field.ErrorList{}
-	// TODO: typecheck CEL expression
-	return allErrs
+// ValidateAndCompileMatchConditions validates a given webhook's matchConditions.
+// This is exported for use in authz package.
+func ValidateAndCompileMatchConditions(matchConditions []api.WebhookMatchCondition) (*authorizationcel.CELMatcher, field.ErrorList) {
+	return compileMatchConditions(matchConditions, nil, utilfeature.DefaultFeatureGate.Enabled(features.StructuredAuthorizationConfiguration))
+}
+
+func compileMatchConditions(matchConditions []api.WebhookMatchCondition, fldPath *field.Path, structuredAuthzFeatureEnabled bool) (*authorizationcel.CELMatcher, field.ErrorList) {
+	var allErrs field.ErrorList
+	// should fail when match conditions are used without feature enabled
+	if len(matchConditions) > 0 && !structuredAuthzFeatureEnabled {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("matchConditions"), "", "matchConditions are not supported when StructuredAuthorizationConfiguration feature gate is disabled"))
+	}
+	if len(matchConditions) > 64 {
+		allErrs = append(allErrs, field.TooMany(fldPath.Child("matchConditions"), len(matchConditions), 64))
+		return nil, allErrs
+	}
+
+	compiler := authorizationcel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()))
+	seenExpressions := sets.NewString()
+	var compilationResults []authorizationcel.CompilationResult
+
+	for i, condition := range matchConditions {
+		fldPath := fldPath.Child("matchConditions").Index(i).Child("expression")
+		if len(strings.TrimSpace(condition.Expression)) == 0 {
+			allErrs = append(allErrs, field.Required(fldPath, ""))
+			continue
+		}
+		if seenExpressions.Has(condition.Expression) {
+			allErrs = append(allErrs, field.Duplicate(fldPath, condition.Expression))
+			continue
+		}
+		seenExpressions.Insert(condition.Expression)
+		compilationResult, err := compileMatchConditionsExpression(fldPath, compiler, condition.Expression)
+		if err != nil {
+			allErrs = append(allErrs, err)
+			continue
+		}
+		compilationResults = append(compilationResults, compilationResult)
+	}
+	if len(compilationResults) == 0 {
+		return nil, allErrs
+	}
+	return &authorizationcel.CELMatcher{
+		CompilationResults: compilationResults,
+	}, allErrs
+}
+
+func compileMatchConditionsExpression(fldPath *field.Path, compiler authorizationcel.Compiler, expression string) (authorizationcel.CompilationResult, *field.Error) {
+	authzExpression := &authorizationcel.SubjectAccessReviewMatchCondition{
+		Expression: expression,
+	}
+	compilationResult, err := compiler.CompileCELExpression(authzExpression)
+	if err != nil {
+		return compilationResult, convertCELErrorToValidationError(fldPath, authzExpression, err)
+	}
+	return compilationResult, nil
+}
+
+func convertCELErrorToValidationError(fldPath *field.Path, expression authorizationcel.ExpressionAccessor, err error) *field.Error {
+	var celErr *cel.Error
+	if errors.As(err, &celErr) {
+		switch celErr.Type {
+		case cel.ErrorTypeRequired:
+			return field.Required(fldPath, celErr.Detail)
+		case cel.ErrorTypeInvalid:
+			return field.Invalid(fldPath, expression.GetExpression(), celErr.Detail)
+		default:
+			return field.InternalError(fldPath, celErr)
+		}
+	}
+	return field.InternalError(fldPath, fmt.Errorf("error is not cel error: %w", err))
 }

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go

@@ -32,7 +32,10 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	api "k8s.io/apiserver/pkg/apis/apiserver"
+	"k8s.io/apiserver/pkg/features"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	certutil "k8s.io/client-go/util/cert"
+	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"k8s.io/utils/pointer"
 )
 
@@ -428,6 +431,8 @@ type (
 )
 
 func TestValidateAuthorizationConfiguration(t *testing.T) {
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StructuredAuthorizationConfiguration, true)()
+
 	badKubeConfigFile := "../some/relative/path/kubeconfig"
 
 	tempKubeConfigFile, err := os.CreateTemp("/tmp", "kubeconfig")
@@ -557,6 +562,39 @@ func TestValidateAuthorizationConfiguration(t *testing.T) {
 			knownTypes:      sets.NewString(string("Webhook")),
 			repeatableTypes: sets.NewString(string("Webhook")),
 		},
+		{
+			name: "bare minimum configuration with Webhook and MatchConditions",
+			configuration: api.AuthorizationConfiguration{
+				Authorizers: []api.AuthorizerConfiguration{
+					{
+						Type: "Webhook",
+						Name: "default",
+						Webhook: &api.WebhookConfiguration{
+							Timeout:                                  metav1.Duration{Duration: 5 * time.Second},
+							AuthorizedTTL:                            metav1.Duration{Duration: 5 * time.Minute},
+							UnauthorizedTTL:                          metav1.Duration{Duration: 30 * time.Second},
+							FailurePolicy:                            "NoOpinion",
+							SubjectAccessReviewVersion:               "v1",
+							MatchConditionSubjectAccessReviewVersion: "v1",
+							ConnectionInfo: api.WebhookConnectionInfo{
+								Type: "InClusterConfig",
+							},
+							MatchConditions: []api.WebhookMatchCondition{
+								{
+									Expression: "has(request.resourceAttributes) && request.resourceAttributes.namespace == 'kube-system'",
+								},
+								{
+									Expression: "request.user == 'admin'",
+								},
+							},
+						},
+					},
+				},
+			},
+			expectedErrList: field.ErrorList{},
+			knownTypes:      sets.NewString(string("Webhook")),
+			repeatableTypes: sets.NewString(string("Webhook")),
+		},
 		{
 			name: "bare minimum configuration with multiple webhooks",
 			configuration: api.AuthorizationConfiguration{
@@ -1156,8 +1194,6 @@ func TestValidateAuthorizationConfiguration(t *testing.T) {
 			knownTypes:      sets.NewString(string("Webhook")),
 			repeatableTypes: sets.NewString(string("Webhook")),
 		},
-
-		// TODO: When the CEL expression validator is implemented, add a few test cases to typecheck the expression
 	}
 
 	for _, test := range tests {
@@ -1166,20 +1202,120 @@ func TestValidateAuthorizationConfiguration(t *testing.T) {
 			if len(errList) != len(test.expectedErrList) {
 				t.Errorf("expected %d errs, got %d, errors %v", len(test.expectedErrList), len(errList), errList)
 			}
-
-			for i, expected := range test.expectedErrList {
-				if expected.Type.String() != errList[i].Type.String() {
-					t.Errorf("expected err type %s, got %s",
-						expected.Type.String(),
-						errList[i].Type.String())
-				}
-				if expected.BadValue != errList[i].BadValue {
-					t.Errorf("expected bad value '%s', got '%s'",
-						expected.BadValue,
-						errList[i].BadValue)
+			if len(errList) == len(test.expectedErrList) {
+				for i, expected := range test.expectedErrList {
+					if expected.Type.String() != errList[i].Type.String() {
+						t.Errorf("expected err type %s, got %s",
+							expected.Type.String(),
+							errList[i].Type.String())
+					}
+					if expected.BadValue != errList[i].BadValue {
+						t.Errorf("expected bad value '%s', got '%s'",
+							expected.BadValue,
+							errList[i].BadValue)
+					}
 				}
 			}
 		})
 
 	}
 }
+
+func TestValidateAndCompileMatchConditions(t *testing.T) {
+	testCases := []struct {
+		name            string
+		matchConditions []api.WebhookMatchCondition
+		featureEnabled  bool
+		expectedErr     string
+	}{
+		{
+			name: "match conditions are used With feature enabled",
+			matchConditions: []api.WebhookMatchCondition{
+				{
+					Expression: "has(request.resourceAttributes) && request.resourceAttributes.namespace == 'kube-system'",
+				},
+				{
+					Expression: "request.user == 'admin'",
+				},
+			},
+			featureEnabled: true,
+			expectedErr:    "",
+		},
+		{
+			name: "should fail when match conditions are used without feature enabled",
+			matchConditions: []api.WebhookMatchCondition{
+				{
+					Expression: "has(request.resourceAttributes) && request.resourceAttributes.namespace == 'kube-system'",
+				},
+				{
+					Expression: "request.user == 'admin'",
+				},
+			},
+			featureEnabled: false,
+			expectedErr:    `matchConditions: Invalid value: "": matchConditions are not supported when StructuredAuthorizationConfiguration feature gate is disabled`,
+		},
+		{
+			name:            "no matchConditions should not require feature enablement",
+			matchConditions: []api.WebhookMatchCondition{},
+			featureEnabled:  false,
+			expectedErr:     "",
+		},
+		{
+			name: "match conditions with invalid expressions",
+			matchConditions: []api.WebhookMatchCondition{
+				{
+					Expression: "  ",
+				},
+			},
+			featureEnabled: true,
+			expectedErr:    "matchConditions[0].expression: Required value",
+		},
+		{
+			name: "match conditions with duplicate expressions",
+			matchConditions: []api.WebhookMatchCondition{
+				{
+					Expression: "request.user == 'admin'",
+				},
+				{
+					Expression: "request.user == 'admin'",
+				},
+			},
+			featureEnabled: true,
+			expectedErr:    `matchConditions[1].expression: Duplicate value: "request.user == 'admin'"`,
+		},
+		{
+			name: "match conditions with undeclared reference",
+			matchConditions: []api.WebhookMatchCondition{
+				{
+					Expression: "test",
+				},
+			},
+			featureEnabled: true,
+			expectedErr:    "matchConditions[0].expression: Invalid value: \"test\": compilation failed: ERROR: <input>:1:1: undeclared reference to 'test' (in container '')\n | test\n | ^",
+		},
+		{
+			name: "match conditions with bad return type",
+			matchConditions: []api.WebhookMatchCondition{
+				{
+					Expression: "request.user = 'test'",
+				},
+			},
+			featureEnabled: true,
+			expectedErr:    "matchConditions[0].expression: Invalid value: \"request.user = 'test'\": compilation failed: ERROR: <input>:1:14: Syntax error: token recognition error at: '= '\n | request.user = 'test'\n | .............^\nERROR: <input>:1:16: Syntax error: extraneous input ''test'' expecting <EOF>\n | request.user = 'test'\n | ...............^",
+		},
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StructuredAuthorizationConfiguration, tt.featureEnabled)()
+			celMatcher, errList := ValidateAndCompileMatchConditions(tt.matchConditions)
+			if len(tt.expectedErr) == 0 && len(tt.matchConditions) > 0 && len(errList) == 0 && celMatcher == nil {
+				t.Errorf("celMatcher should not be nil when there are matchCondition and no error returned")
+			}
+			got := errList.ToAggregate()
+			if d := cmp.Diff(tt.expectedErr, errString(got)); d != "" {
+				t.Fatalf("ValidateAndCompileMatchConditions validation mismatch (-want +got):\n%s", d)
+			}
+		})
+	}
+}