From c9164bce0933409afe4f340af5e9e22eb2879977 Mon Sep 17 00:00:00 2001 From: Tristan Swadell Date: Tue, 30 Jul 2024 13:13:38 -0700 Subject: [PATCH] Variable and nested rule limits (#989) * Variable and nested rule limits * Update the config version to be a string and address feedback --- policy/compiler.go | 69 +++++++++++++++++++++++++++--- policy/compiler_test.go | 22 +++++----- policy/composer.go | 30 ++++++++++--- policy/config.go | 17 +++++++- policy/conformance.go | 1 + policy/helper_test.go | 42 ++++++++++++++++-- policy/testdata/errors/config.yaml | 2 +- policy/testdata/limits/config.yaml | 22 ++++++++++ policy/testdata/limits/policy.yaml | 47 ++++++++++++++++++++ policy/testdata/limits/tests.yaml | 38 ++++++++++++++++ 10 files changed, 260 insertions(+), 30 deletions(-) create mode 100644 policy/testdata/limits/config.yaml create mode 100644 policy/testdata/limits/policy.yaml create mode 100644 policy/testdata/limits/tests.yaml diff --git a/policy/compiler.go b/policy/compiler.go index 6aeb7c1a..1ee12e9a 100644 --- a/policy/compiler.go +++ b/policy/compiler.go @@ -33,6 +33,11 @@ type CompiledRule struct { matches []*CompiledMatch } +// SourceID returns the source metadata identifier associated with the compiled rule. +func (r *CompiledRule) SourceID() int64 { + return r.ID().ID +} + // ID returns the expression id associated with the rule. func (r *CompiledRule) ID() *ValueString { return r.id @@ -50,11 +55,17 @@ func (r *CompiledRule) Matches() []*CompiledMatch { // CompiledVariable represents the variable name, expression, and associated type-check declaration. type CompiledVariable struct { + id int64 name string expr *cel.Ast varDecl *decls.VariableDecl } +// SourceID returns the source metadata identifier associated with the variable. +func (v *CompiledVariable) SourceID() int64 { + return v.id +} + // Name returns the variable name. func (v *CompiledVariable) Name() string { return v.name @@ -110,12 +121,28 @@ func (o *OutputValue) Expr() *cel.Ast { return o.expr } +// CompilerOption specifies a functional option to be applied to new RuleComposer instances. +type CompilerOption func(*compiler) error + +// MaxNestedExpressions limits the number of variable and nested rule expressions during compilation. +// +// Defaults to 100 if not set. +func MaxNestedExpressions(limit int) CompilerOption { + return func(c *compiler) error { + if limit <= 0 { + return fmt.Errorf("nested expression limit must be non-negative, non-zero value: %d", limit) + } + c.maxNestedExpressions = limit + return nil + } +} + // Compile combines the policy compilation and composition steps into a single call. // // This generates a single CEL AST from a collection of policy expressions associated with a // CEL environment. -func Compile(env *cel.Env, p *Policy) (*cel.Ast, *cel.Issues) { - rule, iss := CompileRule(env, p) +func Compile(env *cel.Env, p *Policy, opts ...CompilerOption) (*cel.Ast, *cel.Issues) { + rule, iss := CompileRule(env, p, opts...) if iss.Err() != nil { return nil, iss } @@ -126,14 +153,21 @@ func Compile(env *cel.Env, p *Policy) (*cel.Ast, *cel.Issues) { // CompileRule creates a compiled rules from the policy which contains a set of compiled variables and // match statements. The compiled rule defines an expression graph, which can be composed into a single // expression via the RuleComposer.Compose method. -func CompileRule(env *cel.Env, p *Policy) (*CompiledRule, *cel.Issues) { +func CompileRule(env *cel.Env, p *Policy, opts ...CompilerOption) (*CompiledRule, *cel.Issues) { c := &compiler{ - env: env, - info: p.SourceInfo(), - src: p.Source(), + env: env, + info: p.SourceInfo(), + src: p.Source(), + maxNestedExpressions: defaultMaxNestedExpressions, } errs := common.NewErrors(c.src) iss := cel.NewIssuesWithSourceInfo(errs, c.info) + for _, o := range opts { + if err := o(c); err != nil { + iss.ReportErrorAtID(p.Name().ID, "error configuring compiler option: %s", err) + return nil, iss + } + } rule, ruleIss := c.compileRule(p.Rule(), c.env, iss) iss = iss.Append(ruleIss) return rule, iss @@ -143,6 +177,9 @@ type compiler struct { env *cel.Env info *ast.SourceInfo src *Source + + maxNestedExpressions int + nestedCount int } func (c *compiler) compileRule(r *Rule, ruleEnv *cel.Env, iss *cel.Issues) (*CompiledRule, *cel.Issues) { @@ -170,12 +207,22 @@ func (c *compiler) compileRule(r *Rule, ruleEnv *cel.Env, iss *cel.Issues) (*Com if err != nil { iss.ReportErrorAtID(v.Expression().ID, "invalid variable declaration") } - compiledVars[i] = &CompiledVariable{ + compiledVar := &CompiledVariable{ + id: v.name.ID, name: v.name.Value, expr: varAST, varDecl: varDecl, } + compiledVars[i] = compiledVar + + // Increment the nesting count post-compile. + c.nestedCount++ + if c.nestedCount == c.maxNestedExpressions+1 { + iss.ReportErrorAtID(compiledVar.SourceID(), "variable exceeds nested expression limit") + } } + + // Compile the set of match conditions under the rule. compiledMatches := []*CompiledMatch{} for _, m := range r.Matches() { condSrc := c.relSource(m.Condition()) @@ -208,6 +255,12 @@ func (c *compiler) compileRule(r *Rule, ruleEnv *cel.Env, iss *cel.Issues) (*Com cond: condAST, nestedRule: nestedRule, }) + + // Increment the nesting count post-compile. + c.nestedCount++ + if c.nestedCount == c.maxNestedExpressions+1 { + iss.ReportErrorAtID(nestedRule.SourceID(), "rule exceeds nested expression limit") + } } } return &CompiledRule{ @@ -232,4 +285,6 @@ func (c *compiler) relSource(pstr ValueString) *RelativeSource { const ( // Consider making the variables namespace configurable. variablePrefix = "variables" + + defaultMaxNestedExpressions = 100 ) diff --git a/policy/compiler_test.go b/policy/compiler_test.go index 07e33b6c..ecad14f9 100644 --- a/policy/compiler_test.go +++ b/policy/compiler_test.go @@ -33,7 +33,7 @@ func TestCompile(t *testing.T) { func TestCompileError(t *testing.T) { for _, tst := range policyErrorTests { - _, _, iss := compile(t, tst.name, []ParserOption{}, []cel.EnvOption{}) + _, _, iss := compile(t, tst.name, []ParserOption{}, []cel.EnvOption{}, tst.compilerOpts) if iss.Err() == nil { t.Fatalf("compile(%s) did not error, wanted %s", tst.name, tst.err) } @@ -61,15 +61,16 @@ func newRunner(t testing.TB, name, expr string, parseOpts []ParserOption, opts . } type runner struct { - name string - envOpts []cel.EnvOption - parseOpts []ParserOption - env *cel.Env - expr string - prg cel.Program + name string + envOpts []cel.EnvOption + parseOpts []ParserOption + compilerOpts []CompilerOption + env *cel.Env + expr string + prg cel.Program } -func compile(t testing.TB, name string, parseOpts []ParserOption, envOpts []cel.EnvOption) (*cel.Env, *cel.Ast, *cel.Issues) { +func compile(t testing.TB, name string, parseOpts []ParserOption, envOpts []cel.EnvOption, compilerOpts []CompilerOption) (*cel.Env, *cel.Ast, *cel.Issues) { config := readPolicyConfig(t, fmt.Sprintf("testdata/%s/config.yaml", name)) srcFile := readPolicy(t, fmt.Sprintf("testdata/%s/policy.yaml", name)) parser, err := NewParser(parseOpts...) @@ -84,6 +85,7 @@ func compile(t testing.TB, name string, parseOpts []ParserOption, envOpts []cel. t.Errorf("policy name is %v, wanted %q", policy.name, name) } env, err := cel.NewEnv( + cel.DefaultUTCTimeZone(true), cel.OptionalTypes(), cel.EnableMacroCallTracking(), cel.ExtendedValidations()) @@ -104,12 +106,12 @@ func compile(t testing.TB, name string, parseOpts []ParserOption, envOpts []cel. if err != nil { t.Fatalf("env.Extend() with config options %v, failed: %v", config, err) } - ast, iss := Compile(env, policy) + ast, iss := Compile(env, policy, compilerOpts...) return env, ast, iss } func (r *runner) setup(t testing.TB) { - env, ast, iss := compile(t, r.name, r.parseOpts, r.envOpts) + env, ast, iss := compile(t, r.name, r.parseOpts, r.envOpts, r.compilerOpts) if iss.Err() != nil { t.Fatalf("Compile() failed: %v", iss.Err()) } diff --git a/policy/composer.go b/policy/composer.go index f949815e..5b2f2c2d 100644 --- a/policy/composer.go +++ b/policy/composer.go @@ -24,7 +24,10 @@ import ( // NewRuleComposer creates a rule composer which stitches together rules within a policy into // a single CEL expression. func NewRuleComposer(env *cel.Env, p *Policy) *RuleComposer { - return &RuleComposer{env: env, p: p} + return &RuleComposer{ + env: env, + p: p, + } } // RuleComposer optimizes a set of expressions into a single expression. @@ -41,7 +44,8 @@ func (c *RuleComposer) Compose(r *CompiledRule) (*cel.Ast, *cel.Issues) { } type ruleComposerImpl struct { - rule *CompiledRule + rule *CompiledRule + maxNestedExpressionLimit int } // Optimize implements an AST optimizer for CEL which composes an expression graph into a single @@ -49,14 +53,17 @@ type ruleComposerImpl struct { func (opt *ruleComposerImpl) Optimize(ctx *cel.OptimizerContext, a *ast.AST) *ast.AST { // The input to optimize is a dummy expression which is completely replaced according // to the configuration of the rule composition graph. - ruleExpr, _ := optimizeRule(ctx, opt.rule) + ruleExpr, _ := opt.optimizeRule(ctx, opt.rule) return ctx.NewAST(ruleExpr) } -func optimizeRule(ctx *cel.OptimizerContext, r *CompiledRule) (ast.Expr, bool) { +func (opt *ruleComposerImpl) optimizeRule(ctx *cel.OptimizerContext, r *CompiledRule) (ast.Expr, bool) { matchExpr := ctx.NewCall("optional.none") matches := r.Matches() + vars := r.Variables() optionalResult := true + + // Build the rule subgraph. for i := len(matches) - 1; i >= 0; i-- { m := matches[i] cond := ctx.CopyASTAndMetadata(m.Condition().NativeRep()) @@ -78,7 +85,7 @@ func optimizeRule(ctx *cel.OptimizerContext, r *CompiledRule) (ast.Expr, bool) { matchExpr) continue } - nestedRule, nestedOptional := optimizeRule(ctx, m.NestedRule()) + nestedRule, nestedOptional := opt.optimizeRule(ctx, m.NestedRule()) if optionalResult && !nestedOptional { nestedRule = ctx.NewCall("optional.of", nestedRule) } @@ -90,10 +97,19 @@ func optimizeRule(ctx *cel.OptimizerContext, r *CompiledRule) (ast.Expr, bool) { ctx.ReportErrorAtID(nestedRule.ID(), "subrule early terminates policy") continue } - matchExpr = ctx.NewMemberCall("or", nestedRule, matchExpr) + if triviallyTrue { + matchExpr = ctx.NewMemberCall("or", nestedRule, matchExpr) + } else { + matchExpr = ctx.NewCall( + operators.Conditional, + cond, + nestedRule, + matchExpr, + ) + } } - vars := r.Variables() + // Bind variables in reverse order to declaration on top of rule-subgraph. for i := len(vars) - 1; i >= 0; i-- { v := vars[i] varAST := ctx.CopyASTAndMetadata(v.Expr().NativeRep()) diff --git a/policy/config.go b/policy/config.go index 3bbc1b43..e5aec3cb 100644 --- a/policy/config.go +++ b/policy/config.go @@ -16,6 +16,8 @@ package policy import ( "fmt" + "math" + "strconv" "github.com/google/cel-go/cel" "github.com/google/cel-go/ext" @@ -74,7 +76,7 @@ type ExtensionResolver interface { // ExtensionConfig represents a YAML serializable definition of a versioned extension library reference. type ExtensionConfig struct { Name string `yaml:"name"` - Version int `yaml:"version"` + Version string `yaml:"version"` ExtensionResolver } @@ -87,7 +89,18 @@ func (ec *ExtensionConfig) AsEnvOption(baseEnv *cel.Env) (cel.EnvOption, error) if !found { return nil, fmt.Errorf("unrecognized extension: %s", ec.Name) } - return fac(uint32(ec.Version)), nil + // If the version is 'latest', set the version value to the max uint. + if ec.Version == "latest" { + return fac(math.MaxUint32), nil + } + if ec.Version == "" { + return fac(0), nil + } + ver, err := strconv.ParseUint(ec.Version, 10, 32) + if err != nil { + return nil, fmt.Errorf("error parsing uint version: %w", err) + } + return fac(uint32(ver)), nil } // VariableDecl represents a YAML serializable CEL variable declaration. diff --git a/policy/conformance.go b/policy/conformance.go index 0d1df2ec..8900a732 100644 --- a/policy/conformance.go +++ b/policy/conformance.go @@ -36,6 +36,7 @@ type TestCase struct { Output string `yaml:"output"` } +// TestInput represents an input literal value or expression. type TestInput struct { Value any `yaml:"value"` Expr string `yaml:"expr"` diff --git a/policy/helper_test.go b/policy/helper_test.go index 1640fe52..afd2ae14 100644 --- a/policy/helper_test.go +++ b/policy/helper_test.go @@ -112,12 +112,33 @@ var ( return types.String("ir") } }))), - }}, + }, + }, + { + name: "limits", + expr: ` + cel.bind(variables.greeting, "hello", + cel.bind(variables.farewell, "goodbye", + cel.bind(variables.person, "me", + cel.bind(variables.message_fmt, "%s, %s", + (now.getHours() >= 20) + ? cel.bind(variables.message, variables.message_fmt.format([variables.farewell, variables.person]), + (now.getHours() < 21) + ? optional.of(variables.message + "!") + : ((now.getHours() < 22) + ? optional.of(variables.message + "!!") + : ((now.getHours() < 24) + ? optional.of(variables.message + "!!!") + : optional.none()))) + : optional.of(variables.message_fmt.format([variables.greeting, variables.person])) + ))))`, + }, } policyErrorTests = []struct { - name string - err string + name string + err string + compilerOpts []CompilerOption }{ { name: "errors", @@ -140,6 +161,21 @@ ERROR: testdata/errors/policy.yaml:34:67: undeclared reference to 'format' (in c | "invalid values provided on one or more labels: %s".format([variables.invalid]) | ..................................................................^`, }, + { + name: "limits", + err: `ERROR: testdata/limits/policy.yaml:22:14: variable exceeds nested expression limit + | - name: "person" + | .............^`, + compilerOpts: []CompilerOption{MaxNestedExpressions(2)}, + }, + + { + name: "limits", + err: `ERROR: testdata/limits/policy.yaml:30:14: rule exceeds nested expression limit + | id: "farewells" + | .............^`, + compilerOpts: []CompilerOption{MaxNestedExpressions(5)}, + }, } ) diff --git a/policy/testdata/errors/config.yaml b/policy/testdata/errors/config.yaml index f6f24bf1..2196e571 100644 --- a/policy/testdata/errors/config.yaml +++ b/policy/testdata/errors/config.yaml @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -name: "labels" +name: "errors" extensions: - name: "lists" - name: "sets" diff --git a/policy/testdata/limits/config.yaml b/policy/testdata/limits/config.yaml new file mode 100644 index 00000000..78fdbcb6 --- /dev/null +++ b/policy/testdata/limits/config.yaml @@ -0,0 +1,22 @@ +# Copyright 2024 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +name: "limits" +extensions: + - name: "strings" + version: latest +variables: + - name: "now" + type: + type_name: "google.protobuf.Timestamp" diff --git a/policy/testdata/limits/policy.yaml b/policy/testdata/limits/policy.yaml new file mode 100644 index 00000000..3931e1ba --- /dev/null +++ b/policy/testdata/limits/policy.yaml @@ -0,0 +1,47 @@ +# Copyright 2024 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +name: "limits" +rule: + variables: + - name: "greeting" + expression: "'hello'" + - name: "farewell" + expression: "'goodbye'" + - name: "person" + expression: "'me'" + - name: "message_fmt" + expression: "'%s, %s'" + match: + - condition: | + now.getHours() >= 20 + rule: + id: "farewells" + variables: + - name: "message" + expression: > + variables.message_fmt.format([variables.farewell, + variables.person]) + match: + - condition: > + now.getHours() < 21 + output: variables.message + "!" + - condition: > + now.getHours() < 22 + output: variables.message + "!!" + - condition: > + now.getHours() < 24 + output: variables.message + "!!!" + - output: > + variables.message_fmt.format([variables.greeting, variables.person]) diff --git a/policy/testdata/limits/tests.yaml b/policy/testdata/limits/tests.yaml new file mode 100644 index 00000000..8f50a519 --- /dev/null +++ b/policy/testdata/limits/tests.yaml @@ -0,0 +1,38 @@ +# Copyright 2024 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +description: Limits related tests +section: + - name: "now_after_hours" + tests: + - name: "7pm" + input: + now: + expr: "timestamp('2024-07-30T00:30:00Z')" + output: "'hello, me'" + - name: "8pm" + input: + now: + expr: "timestamp('2024-07-30T20:30:00Z')" + output: "'goodbye, me!'" + - name: "9pm" + input: + now: + expr: "timestamp('2024-07-30T21:30:00Z')" + output: "'goodbye, me!!'" + - name: "11pm" + input: + now: + expr: "timestamp('2024-07-30T23:30:00Z')" + output: "'goodbye, me!!!'"