[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 [email protected]
tektoncd · Oct 21, 2023 · 8b81d38 · 8b81d38
1 parent 0111021
commit 8b81d38
Show file tree

Hide file tree

Showing 5 changed files with 121 additions and 7 deletions.
diff --git 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
@@ -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,21 @@ 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, err := cel.NewEnv()
+		if err != nil {
+			return apis.ErrGeneric("err: %s", err.Error())
+		}
+		_, 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
@@ -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{{
@@ -159,7 +196,7 @@ func TestCELWhenExpressions_Invalid(t *testing.T) {
 				},
 			})
 			if err := tt.wes.validate(ctx); err == nil {
-				t.Errorf("WhenExpressions.validate() did not return error for invalid when expressions: %s", tt.wes)
+				t.Errorf("WhenExpressions.validate() did not return error for invalid when expressions: %s, %s", tt.wes, err)
 			}
 		})
 	}

diff --git 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,20 @@ 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, err := cel.NewEnv()
+		if err != nil {
+			return apis.ErrGeneric("err: %s", err.Error())
+		}
+		_, 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
@@ -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{{
@@ -159,7 +196,7 @@ func TestCELWhenExpressions_Invalid(t *testing.T) {
 				},
 			})
 			if err := tt.wes.validate(ctx); err == nil {
-				t.Errorf("WhenExpressions.validate() did not return error for invalid when expressions: %s", tt.wes)
+				t.Errorf("WhenExpressions.validate() did not return error for invalid when expressions: %s, %s", tt.wes, err)
 			}
 		})
 	}