From c29e9780456a3afb28fbc3ee694a6f042df09593 Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Fri, 20 Oct 2023 17:26:31 +0000 Subject: [PATCH] [TEP-0145] Add sanity check for CEL expression This commit adds sanity check for CEL expression by pre-compiling the CEL expression at admission webhook without evluation. This can help fail fast if the CEL expression is invalid. And also disallow the usage of variable references without wrapper with single quotes and prevent CEL injection from variables. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- docs/pipelines.md | 13 ++++++- pkg/apis/pipeline/v1/when_validation.go | 15 ++++++- pkg/apis/pipeline/v1/when_validation_test.go | 39 ++++++++++++++++++- pkg/apis/pipeline/v1beta1/when_validation.go | 12 ++++++ .../pipeline/v1beta1/when_validation_test.go | 39 ++++++++++++++++++- 5 files changed, 113 insertions(+), 5 deletions(-) diff --git a/docs/pipelines.md b/docs/pipelines.md index 300835e9ddf..44071ab5a3a 100644 --- a/docs/pipelines.md +++ b/docs/pipelines.md @@ -777,12 +777,23 @@ There are a lot of scenarios where `when` expressions can be really useful. Some #### Use CEL expression in WhenExpression -> :seedling: **`CEL in WhenExpression` is an [alpha](install.md#alpha-features) feature.** +> :seedling: **`CEL in WhenExpression` is an [alpha](additional-configs.md#alpha-features) feature.** > The `enable-cel-in-whenexpression` feature flag must be set to `"true"` to enable the use of `CEL` in `WhenExpression`. > > :warning: This feature is in a preview mode. > It is still in a very early stage of development and is not yet fully functional +`CEL` expression is validated at admission webhook and a validation error will be returned if the expression is invalid. + +**Note:** To use Tekton's [variable substitution](variables.md), you need to wrap the reference with single quotes. This also means that if you pass another CEL expression via `params` or `results`, it won't be executed. Therefore CEL injection is disallowed. + +For example: +``` +This is valid: '$(params.foo)' == 'foo' +This is invalid: $(params.foo) == 'foo' +CEL's variable substitution is not supported yet and thus invalid: params.foo == 'foo' +``` + #### Guarding a `Task` and its dependent `Tasks` To guard a `Task` and its dependent Tasks: diff --git a/pkg/apis/pipeline/v1/when_validation.go b/pkg/apis/pipeline/v1/when_validation.go index fb8a92487d9..2a058299a7d 100644 --- a/pkg/apis/pipeline/v1/when_validation.go +++ b/pkg/apis/pipeline/v1/when_validation.go @@ -21,8 +21,7 @@ import ( "fmt" "strings" - // TODO(#7244): Pull the cel-go library for now, the following PR will use the library. - _ "github.com/google/cel-go/cel" + "github.com/google/cel-go/cel" "github.com/tektoncd/pipeline/pkg/apis/config" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/selection" @@ -54,6 +53,18 @@ func (we *WhenExpression) validateWhenExpressionFields(ctx context.Context) *api if we.Input != "" || we.Operator != "" || len(we.Values) != 0 { return apis.ErrGeneric(fmt.Sprintf("cel and input+operator+values cannot be set in one WhenExpression: %v", we)) } + + // We need to compile the CEL expression and check if it is a valid expression + // note that at the validation webhook, Tekton's variables are not substituted, + // so they need to be wrapped with single quotes. + // e.g. This is a valid CEL expression: '$(params.foo)' == 'foo'; + // But this is not a valid expression since CEL cannot recognize: $(params.foo) == 'foo'; + // This is not valid since we don't pass params to CEL's environment: params.foo == 'foo'; + env, _ := cel.NewEnv() + _, iss := env.Compile(we.CEL) + if iss.Err() != nil { + return apis.ErrGeneric("invalid cel expression: %s with err: %s", we.CEL, iss.Err().Error()) + } return nil } diff --git a/pkg/apis/pipeline/v1/when_validation_test.go b/pkg/apis/pipeline/v1/when_validation_test.go index f3dbeb962d6..eb4c615bcf3 100644 --- a/pkg/apis/pipeline/v1/when_validation_test.go +++ b/pkg/apis/pipeline/v1/when_validation_test.go @@ -116,10 +116,35 @@ func TestCELinWhenExpressions_Valid(t *testing.T) { name string wes WhenExpressions }{{ - name: "valid cel", + name: "valid operator - Equal", wes: []WhenExpression{{ CEL: " 'foo' == 'foo' ", }}, + }, { + name: "valid operator - NotEqual", + wes: []WhenExpression{{ + CEL: " 'foo' != 'foo' ", + }}, + }, { + name: "valid operator - In", + wes: []WhenExpression{{ + CEL: "'foo' in ['foo', 'bar']", + }}, + }, { + name: "valid operator - NotIn", + wes: []WhenExpression{{ + CEL: "!('foo' in ['foo', 'bar'])", + }}, + }, { + name: "valid regex expression", + wes: []WhenExpression{{ + CEL: "'release/v1'.matches('release/.*')", + }}, + }, { + name: "valid variable reference syntax", + wes: []WhenExpression{{ + CEL: "'$(params.foo)' in ['foo', 'bar']", + }}, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -141,6 +166,18 @@ func TestCELWhenExpressions_Invalid(t *testing.T) { CEL: " 'foo' == 'foo' ", }}, enableCELInWhenExpression: false, + }, { + name: "variable reference should be wrapped with single quotes", + wes: []WhenExpression{{ + CEL: " $(params.foo) == 'foo' ", + }}, + enableCELInWhenExpression: true, + }, { + name: "variables not declared in environment", + wes: []WhenExpression{{ + CEL: " params.foo == 'foo' ", + }}, + enableCELInWhenExpression: true, }, { name: "CEL should not coexist with input+operator+values", wes: []WhenExpression{{ diff --git a/pkg/apis/pipeline/v1beta1/when_validation.go b/pkg/apis/pipeline/v1beta1/when_validation.go index 8279950afab..33855040b2b 100644 --- a/pkg/apis/pipeline/v1beta1/when_validation.go +++ b/pkg/apis/pipeline/v1beta1/when_validation.go @@ -21,6 +21,7 @@ import ( "fmt" "strings" + "github.com/google/cel-go/cel" "github.com/tektoncd/pipeline/pkg/apis/config" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/selection" @@ -52,6 +53,17 @@ func (we *WhenExpression) validateWhenExpressionFields(ctx context.Context) *api if we.Input != "" || we.Operator != "" || len(we.Values) != 0 { return apis.ErrGeneric(fmt.Sprintf("cel and input+operator+values cannot be set in one WhenExpression: %v", we)) } + // We need to compile the CEL expression and check if it is a valid expression + // note that at the validation webhook, Tekton's variables are not substituted, + // so they need to be wrapped with single quotes. + // e.g. This is a valid CEL expression: '$(params.foo)' == 'foo'; + // But this is not a valid expression since CEL cannot recognize: $(params.foo) == 'foo'; + // This is not valid since we don't pass params to CEL's environment: params.foo == 'foo'; + env, _ := cel.NewEnv() + _, iss := env.Compile(we.CEL) + if iss.Err() != nil { + return apis.ErrGeneric("invalid cel expression: %s with err: %s", we.CEL, iss.Err().Error()) + } return nil } diff --git a/pkg/apis/pipeline/v1beta1/when_validation_test.go b/pkg/apis/pipeline/v1beta1/when_validation_test.go index 16ec85b52fd..0393cb3c288 100644 --- a/pkg/apis/pipeline/v1beta1/when_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/when_validation_test.go @@ -116,10 +116,35 @@ func TestCELinWhenExpressions_Valid(t *testing.T) { name string wes WhenExpressions }{{ - name: "valid cel", + name: "valid operator - Equal", wes: []WhenExpression{{ CEL: " 'foo' == 'foo' ", }}, + }, { + name: "valid operator - NotEqual", + wes: []WhenExpression{{ + CEL: " 'foo' != 'foo' ", + }}, + }, { + name: "valid operator - In", + wes: []WhenExpression{{ + CEL: "'foo' in ['foo', 'bar']", + }}, + }, { + name: "valid operator - NotIn", + wes: []WhenExpression{{ + CEL: "!('foo' in ['foo', 'bar'])", + }}, + }, { + name: "valid regex expression", + wes: []WhenExpression{{ + CEL: "'release/v1'.matches('release/.*')", + }}, + }, { + name: "valid variable reference syntax", + wes: []WhenExpression{{ + CEL: "'$(params.foo)' in ['foo', 'bar']", + }}, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -141,6 +166,18 @@ func TestCELWhenExpressions_Invalid(t *testing.T) { CEL: " 'foo' == 'foo' ", }}, enableCELInWhenExpression: false, + }, { + name: "variable reference should be wrapped with single quotes", + wes: []WhenExpression{{ + CEL: " $(params.foo) == 'foo' ", + }}, + enableCELInWhenExpression: true, + }, { + name: "variables not declared in environment", + wes: []WhenExpression{{ + CEL: " params.foo == 'foo' ", + }}, + enableCELInWhenExpression: true, }, { name: "CEL should not coexist with input+operator+values", wes: []WhenExpression{{