Merge pull request #118804 from benluddy/authz-deferred-errors

CEL lib: Expose errors on authz decisions instead of raising them from check()

Kubernetes-commit: 1d846a12da5b05e9b9e50b30fdaae2ea269822a0

go.mod

@@ -41,10 +41,10 @@ require (
 	google.golang.org/protobuf v1.30.0
 	gopkg.in/natefinch/lumberjack.v2 v2.2.1
 	gopkg.in/square/go-jose.v2 v2.6.0
-	k8s.io/api v0.0.0-20230711173311-4993e301ed85
+	k8s.io/api v0.0.0-20230712211402-283b145385c0
-	k8s.io/apimachinery v0.0.0-20230712063911-ddd02633a105
+	k8s.io/apimachinery v0.0.0-20230712210707-c9b3b3a37189
 	k8s.io/client-go v0.0.0-20230711210844-560efb3b8995
-	k8s.io/component-base v0.0.0-20230706070231-63369697f0ec
+	k8s.io/component-base v0.0.0-20230713173548-ea35e2fd8622
 	k8s.io/klog/v2 v2.100.1
 	k8s.io/kms v0.0.0-20230706235007-2273d4f89020
 	k8s.io/kube-openapi v0.0.0-20230601164746-7562a1006961
@@ -125,9 +125,9 @@ require (
 )
 
 replace (
-	k8s.io/api => k8s.io/api v0.0.0-20230711173311-4993e301ed85
+	k8s.io/api => k8s.io/api v0.0.0-20230712211402-283b145385c0
-	k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20230712063911-ddd02633a105
+	k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20230712210707-c9b3b3a37189
 	k8s.io/client-go => k8s.io/client-go v0.0.0-20230711210844-560efb3b8995
-	k8s.io/component-base => k8s.io/component-base v0.0.0-20230706070231-63369697f0ec
+	k8s.io/component-base => k8s.io/component-base v0.0.0-20230713173548-ea35e2fd8622
 	k8s.io/kms => k8s.io/kms v0.0.0-20230706235007-2273d4f89020
 )

go.sum

@@ -668,14 +668,14 @@ honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWh
 honnef.co/go/tools v0.0.1-2019.2.3/go.mod h1:a3bituU0lyd329TUQxRnasdCoJDkEUEAqEt0JzvZhAg=
 honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
 honnef.co/go/tools v0.0.1-2020.1.4/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
-k8s.io/api v0.0.0-20230711173311-4993e301ed85 h1:objTFO7gYE2CeGB/qKnBxdUoj2QX5a6ZlLhewph/B0M=
+k8s.io/api v0.0.0-20230712211402-283b145385c0 h1:ZYimmBg4vO3b4K7TLYlznRgOsYsmugKun6VKvhlvew4=
-k8s.io/api v0.0.0-20230711173311-4993e301ed85/go.mod h1:ghFHmaoujTvUqKl24+WADIX4gEvgTA8BEpgFe3ZPylQ=
+k8s.io/api v0.0.0-20230712211402-283b145385c0/go.mod h1:ie73AdCqQgSH/4ZAcr1d8KRqEUkyYlVGJSXgV5IH9VM=
-k8s.io/apimachinery v0.0.0-20230712063911-ddd02633a105 h1:P7M6UV6TD8SKsVuAmcE/5FV2yuGBd6BqHOiDUGXvovw=
+k8s.io/apimachinery v0.0.0-20230712210707-c9b3b3a37189 h1:y3e8tHyqhvrwV/79166VbMgSTy6i9/z8Tk4VrKZc8dg=
-k8s.io/apimachinery v0.0.0-20230712063911-ddd02633a105/go.mod h1:tAiIbF8KB8+Ri2DfUWwZGwNOThIwM0fhXLnOymriu+4=
+k8s.io/apimachinery v0.0.0-20230712210707-c9b3b3a37189/go.mod h1:tAiIbF8KB8+Ri2DfUWwZGwNOThIwM0fhXLnOymriu+4=
 k8s.io/client-go v0.0.0-20230711210844-560efb3b8995 h1:QLFo+ZtIUp9wHy2+x/0pAdpCG7W5rCNPEZWTcG+s0HY=
 k8s.io/client-go v0.0.0-20230711210844-560efb3b8995/go.mod h1:emw6SDhjDVJfii5Lj0dErkjqTvqKNiFMNfs209yp6Ps=
-k8s.io/component-base v0.0.0-20230706070231-63369697f0ec h1:Jp8V9IROgsd7hlRayWEwme9biJBB2cibC0CpP2HGveU=
+k8s.io/component-base v0.0.0-20230713173548-ea35e2fd8622 h1:BXv43X6tUb54xT61nujFqdk+k9IczwmvzozlpuJ8fDg=
-k8s.io/component-base v0.0.0-20230706070231-63369697f0ec/go.mod h1:aiE+qizM73R49NuVbaxClSQu2Yj+zIDCEN6OY8Zcp3w=
+k8s.io/component-base v0.0.0-20230713173548-ea35e2fd8622/go.mod h1:8ZC41kwK0dvUNquR5Dhcexss/5xGONDrQ37ZjY0pmmE=
 k8s.io/klog/v2 v2.100.1 h1:7WCHKK6K8fNhTqfBhISHQ97KrnJNFZMcQvKp7gP/tmg=
 k8s.io/klog/v2 v2.100.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0=
 k8s.io/kms v0.0.0-20230706235007-2273d4f89020 h1:6zftrcpbnLRCHE75wwmtawDwEcA2HkPZZUVfB+iO8FA=

pkg/admission/plugin/cel/filter_test.go

@@ -438,12 +438,18 @@ func TestFilter(t *testing.T) {
 				&condition{
 					Expression: "authorizer.group('').resource('endpoints').check('create').allowed()",
 				},
+				&condition{
+					Expression: "authorizer.group('').resource('endpoints').check('create').errored()",
+				},
 			},
 			attributes: newValidAttribute(&podObject, false),
 			results: []EvaluationResult{
 				{
 					EvalResult: celtypes.True,
 				},
+				{
+					EvalResult: celtypes.False,
+				},
 			},
 			authorizer: newAuthzAllowMatch(authorizer.AttributesRecord{
 				ResourceRequest: true,
@@ -516,6 +522,33 @@ func TestFilter(t *testing.T) {
 			},
 			authorizer: denyAll,
 		},
+		{
+			name: "test authorizer error",
+			validations: []ExpressionAccessor{
+				&condition{
+					Expression: "authorizer.group('').resource('endpoints').check('create').errored()",
+				},
+				&condition{
+					Expression: "authorizer.group('').resource('endpoints').check('create').error() == 'fake authz error'",
+				},
+				&condition{
+					Expression: "authorizer.group('').resource('endpoints').check('create').allowed()",
+				},
+			},
+			attributes: newValidAttribute(&podObject, false),
+			results: []EvaluationResult{
+				{
+					EvalResult: celtypes.True,
+				},
+				{
+					EvalResult: celtypes.True,
+				},
+				{
+					EvalResult: celtypes.False,
+				},
+			},
+			authorizer: errorAll,
+		},
 		{
 			name: "test authorizer allow path check",
 			validations: []ExpressionAccessor{
@@ -974,6 +1007,7 @@ func TestCompilationErrors(t *testing.T) {
 }
 
 var denyAll = fakeAuthorizer{defaultResult: authorizerResult{decision: authorizer.DecisionDeny, reason: "fake reason", err: nil}}
+var errorAll = fakeAuthorizer{defaultResult: authorizerResult{decision: authorizer.DecisionNoOpinion, reason: "", err: fmt.Errorf("fake authz error")}}
 
 func newAuthzAllowMatch(match authorizer.AttributesRecord) fakeAuthorizer {
 	return fakeAuthorizer{

pkg/cel/library/authz.go

@@ -174,6 +174,26 @@ import (
 // Examples:
 //
 //	authorizer.path('/healthz').check('GET').reason()
+//
+// errored
+//
+// Returns true if the authorization check resulted in an error.
+//
+//	<Decision>.errored() <bool>
+//
+// Examples:
+//
+//	authorizer.group('').resource('pods').namespace('default').check('create').errored() // Returns true if the authorization check resulted in an error
+//
+// error
+//
+// If the authorization check resulted in an error, returns the error. Otherwise, returns the empty string.
+//
+//	<Decision>.error() <string>
+//
+// Examples:
+//
+//	authorizer.group('').resource('pods').namespace('default').check('create').error()
 func Authz() cel.EnvOption {
 	return cel.Lib(authzLib)
 }
@@ -209,6 +229,12 @@ var authzLibraryDecls = map[string][]cel.FunctionOpt{
 			cel.BinaryBinding(pathCheckCheck)),
 		cel.MemberOverload("resourcecheck_check", []*cel.Type{ResourceCheckType, cel.StringType}, DecisionType,
 			cel.BinaryBinding(resourceCheckCheck))},
+	"errored": {
+		cel.MemberOverload("decision_errored", []*cel.Type{DecisionType}, cel.BoolType,
+			cel.UnaryBinding(decisionErrored))},
+	"error": {
+		cel.MemberOverload("decision_error", []*cel.Type{DecisionType}, cel.StringType,
+			cel.UnaryBinding(decisionError))},
 	"allowed": {
 		cel.MemberOverload("decision_allowed", []*cel.Type{DecisionType}, cel.BoolType,
 			cel.UnaryBinding(decisionAllowed))},
@@ -384,6 +410,27 @@ func resourceCheckCheck(arg1, arg2 ref.Val) ref.Val {
 	return resourceCheck.Authorize(context.TODO(), apiVerb)
 }
 
+func decisionErrored(arg ref.Val) ref.Val {
+	decision, ok := arg.(decisionVal)
+	if !ok {
+		return types.MaybeNoSuchOverloadErr(arg)
+	}
+
+	return types.Bool(decision.err != nil)
+}
+
+func decisionError(arg ref.Val) ref.Val {
+	decision, ok := arg.(decisionVal)
+	if !ok {
+		return types.MaybeNoSuchOverloadErr(arg)
+	}
+
+	if decision.err == nil {
+		return types.String("")
+	}
+	return types.String(decision.err.Error())
+}
+
 func decisionAllowed(arg ref.Val) ref.Val {
 	decision, ok := arg.(decisionVal)
 	if !ok {
@@ -478,10 +525,7 @@ func (a pathCheckVal) Authorize(ctx context.Context, verb string) ref.Val {
 	}
 
 	decision, reason, err := a.authorizer.authAuthorizer.Authorize(ctx, attr)
-	if err != nil {
+	return newDecision(decision, err, reason)
-		return types.NewErr("error in authorization check: %v", err)
-	}
-	return newDecision(decision, reason)
 }
 
 type groupCheckVal struct {
@@ -516,18 +560,16 @@ func (a resourceCheckVal) Authorize(ctx context.Context, verb string) ref.Val {
 		User:            a.groupCheck.authorizer.userInfo,
 	}
 	decision, reason, err := a.groupCheck.authorizer.authAuthorizer.Authorize(ctx, attr)
-	if err != nil {
+	return newDecision(decision, err, reason)
-		return types.NewErr("error in authorization check: %v", err)
-	}
-	return newDecision(decision, reason)
 }
 
-func newDecision(authDecision authorizer.Decision, reason string) decisionVal {
+func newDecision(authDecision authorizer.Decision, err error, reason string) decisionVal {
-	return decisionVal{receiverOnlyObjectVal: receiverOnlyVal(DecisionType), authDecision: authDecision, reason: reason}
+	return decisionVal{receiverOnlyObjectVal: receiverOnlyVal(DecisionType), authDecision: authDecision, err: err, reason: reason}
 }
 
 type decisionVal struct {
 	receiverOnlyObjectVal
+	err          error
 	authDecision authorizer.Decision
 	reason       string
 }

pkg/cel/library/cost.go

@@ -41,7 +41,7 @@ func (l *CostEstimator) CallCost(function, overloadId string, args []ref.Val, re
 		// This cost is set to allow for only two authorization checks per expression
 		cost := uint64(350000)
 		return &cost
-	case "serviceAccount", "path", "group", "resource", "subresource", "namespace", "name", "allowed", "denied", "reason":
+	case "serviceAccount", "path", "group", "resource", "subresource", "namespace", "name", "allowed", "reason", "error", "errored":
 		// All authorization builder and accessor functions have a nominal cost
 		cost := uint64(1)
 		return &cost
@@ -91,7 +91,7 @@ func (l *CostEstimator) EstimateCallCost(function, overloadId string, target *ch
 		// An authorization check has a fixed cost
 		// This cost is set to allow for only two authorization checks per expression
 		return &checker.CallEstimate{CostEstimate: checker.CostEstimate{Min: 350000, Max: 350000}}
-	case "serviceAccount", "path", "group", "resource", "subresource", "namespace", "name", "allowed", "denied", "reason":
+	case "serviceAccount", "path", "group", "resource", "subresource", "namespace", "name", "allowed", "reason", "error", "errored":
 		// All authorization builder and accessor functions have a nominal cost
 		return &checker.CallEstimate{CostEstimate: checker.CostEstimate{Min: 1, Max: 1}}
 	case "isSorted", "sum", "max", "min", "indexOf", "lastIndexOf":

pkg/cel/library/cost_test.go

@@ -351,6 +351,18 @@ func TestAuthzLibrary(t *testing.T) {
 			expectEstimatedCost: checker.CostEstimate{Min: 350007, Max: 350007},
 			expectRuntimeCost:   350007,
 		},
+		{
+			name:                "resource check errored",
+			expr:                "authorizer.group('apps').resource('deployments').subresource('status').namespace('test').name('backend').check('create').errored()",
+			expectEstimatedCost: checker.CostEstimate{Min: 350007, Max: 350007},
+			expectRuntimeCost:   350007,
+		},
+		{
+			name:                "resource check error",
+			expr:                "authorizer.group('apps').resource('deployments').subresource('status').namespace('test').name('backend').check('create').error()",
+			expectEstimatedCost: checker.CostEstimate{Min: 350007, Max: 350007},
+			expectRuntimeCost:   350007,
+		},
 	}
 
 	for _, tc := range cases {

pkg/cel/library/library_compatibility_test.go

@@ -37,12 +37,14 @@ func TestLibraryCompatibility(t *testing.T) {
 	// WARN: All library changes must follow
 	// https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/2876-crd-validation-expression-language#function-library-updates
 	// and must track the functions here along with which Kubernetes version introduced them.
-	knownFunctions := sets.New[string](
+	knownFunctions := sets.New(
 		// Kubernetes 1.24:
 		"isSorted", "sum", "max", "min", "indexOf", "lastIndexOf", "find", "findAll", "url", "getScheme", "getHost", "getHostname",
 		"getPort", "getEscapedPath", "getQuery", "isURL",
 		// Kubernetes <1.27>:
 		"path", "group", "serviceAccount", "resource", "subresource", "namespace", "name", "check", "allowed", "reason",
+		// Kubernetes <1.28>:
+		"errored", "error",
 		// Kubernetes <1.??>:
 	)