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

CEL lib: Expose errors on authz decisions instead of raising them from check()
This commit is contained in:
Kubernetes Prow Robot 2023-07-13 12:39:37 -07:00 committed by GitHub
commit 1d846a12da
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 103 additions and 13 deletions

View File

@ -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{

View File

@ -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 types.NewErr("error in authorization check: %v", err)
}
return newDecision(decision, reason)
return newDecision(decision, err, 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 types.NewErr("error in authorization check: %v", err)
}
return newDecision(decision, reason)
return newDecision(decision, err, reason)
}
func newDecision(authDecision authorizer.Decision, reason string) decisionVal {
return decisionVal{receiverOnlyObjectVal: receiverOnlyVal(DecisionType), authDecision: authDecision, reason: reason}
func newDecision(authDecision authorizer.Decision, err error, reason string) decisionVal {
return decisionVal{receiverOnlyObjectVal: receiverOnlyVal(DecisionType), authDecision: authDecision, err: err, reason: reason}
}
type decisionVal struct {
receiverOnlyObjectVal
err error
authDecision authorizer.Decision
reason string
}

View File

@ -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":

View File

@ -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 {

View File

@ -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.??>:
)