Merge pull request kyverno#1303 from kyverno/1298_fix_variable_valida…

…tion

1298 fix variable validation

go.mod

@@ -3,63 +3,39 @@ module github.com/kyverno/kyverno
 go 1.13
 
 require (
-	cloud.google.com/go v0.52.0 // indirect
-	github.com/Azure/azure-sdk-for-go v38.2.0+incompatible // indirect
-	github.com/Azure/go-autorest/autorest v0.9.4 // indirect
-	github.com/Azure/go-autorest/autorest/adal v0.8.1 // indirect
-	github.com/Azure/go-autorest/autorest/to v0.3.0 // indirect
-	github.com/aws/aws-sdk-go v1.28.9 // indirect
 	github.com/cenkalti/backoff v2.2.1+incompatible
-	github.com/containerd/fifo v0.0.0-20200410184934-f15a3290365b // indirect
 	github.com/cornelk/hashmap v1.0.1
-	github.com/docker/distribution v2.7.1+incompatible // indirect
-	github.com/docker/docker v0.7.3-0.20190327010347-be7ac8be2ae0
-	github.com/docker/go-connections v0.4.0 // indirect
-	github.com/docker/go-metrics v0.0.1 // indirect
 	github.com/evanphx/json-patch v4.5.0+incompatible
-	github.com/evanphx/json-patch/v5 v5.0.0 // indirect
 	github.com/fatih/color v1.9.0 // indirect
-	github.com/fatih/structtag v1.2.0 // indirect
+	github.com/fsnotify/fsnotify v1.4.9 // indirect
 	github.com/gardener/controller-manager-library v0.2.0
 	github.com/ghodss/yaml v1.0.1-0.20190212211648-25d852aebe32
-	github.com/go-delve/delve v1.5.0 // indirect
 	github.com/go-logr/logr v0.1.0
-	github.com/go-openapi/spec v0.19.5
-	github.com/go-openapi/strfmt v0.19.5
-	github.com/go-openapi/validate v0.19.8
-	github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b
 	github.com/golang/groupcache v0.0.0-20200121045136-8c9f03a8e57e // indirect
+	github.com/google/go-cmp v0.4.0 // indirect
 	github.com/googleapis/gnostic v0.3.1
-	github.com/graymeta/stow v0.2.4 // indirect
 	github.com/hashicorp/golang-lru v0.5.4 // indirect
 	github.com/imdario/mergo v0.3.8 // indirect
 	github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af
 	github.com/json-iterator/go v1.1.9 // indirect
 	github.com/julienschmidt/httprouter v1.3.0
-	github.com/lyft/flytestdlib v0.2.31
+	github.com/kr/pretty v0.2.0 // indirect
 	github.com/mattbaird/jsonpatch v0.0.0-20171005235357-81af80346b1a
 	github.com/mattn/go-isatty v0.0.12 // indirect
 	github.com/minio/minio v0.0.0-20200114012931-30922148fbb5
-	github.com/morikuni/aec v1.0.0 // indirect
 	github.com/onsi/ginkgo v1.11.0
 	github.com/onsi/gomega v1.8.1
-	github.com/opencontainers/go-digest v1.0.0 // indirect
-	github.com/opencontainers/image-spec v1.0.1 // indirect
-	github.com/ory/go-acc v0.2.6 // indirect
 	github.com/pkg/errors v0.9.1
-	github.com/pkg/profile v1.2.1
-	github.com/prometheus/common v0.7.0
-	github.com/rogpeppe/godef v1.1.2 // indirect
+	github.com/sirupsen/logrus v1.6.0 // indirect
 	github.com/spf13/cobra v1.0.0
-	github.com/spf13/viper v1.6.2 // indirect
 	github.com/stretchr/testify v1.5.1
 	github.com/tevino/abool v0.0.0-20170917061928-9b9efcf221b5
-	golang.org/x/lint v0.0.0-20200302205851-738671d3881b // indirect
+	golang.org/x/net v0.0.0-20201021035429-f5854403a974 // indirect
+	golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d // indirect
 	golang.org/x/time v0.0.0-20191024005414-555d28b269f0 // indirect
-	golang.org/x/tools v0.0.0-20201117152513-9036a0f9af11 // indirect
+	golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect
 	google.golang.org/appengine v1.6.5 // indirect
 	gopkg.in/yaml.v2 v2.3.0
-	gopkg.in/yaml.v3 v3.0.0-20200121175148-a6ecf24a6d71
 	gotest.tools v2.2.0+incompatible
 	k8s.io/api v0.18.4
 	k8s.io/apiextensions-apiserver v0.18.4

pkg/engine/context/context.go

@@ -33,21 +33,22 @@ type EvalInterface interface {
 
 //Context stores the data resources as JSON
 type Context struct {
-	mu            sync.RWMutex
-	jsonRaw       []byte
-	whiteListVars []string
-	log           logr.Logger
+	mu          sync.RWMutex
+	jsonRaw     []byte
+	builtInVars []string
+	log         logr.Logger
 }
 
 //NewContext returns a new context
-// pass the list of variables to be white-listed
-func NewContext(whiteListVars ...string) *Context {
+// builtInVars is the list of known variables (e.g. serviceAccountName)
+func NewContext(builtInVars ...string) *Context {
 	ctx := Context{
 		// data:    map[string]interface{}{},
-		jsonRaw:       []byte(`{}`), // empty json struct
-		whiteListVars: whiteListVars,
-		log:           log.Log.WithName("context"),
+		jsonRaw:     []byte(`{}`), // empty json struct
+		builtInVars: builtInVars,
+		log:         log.Log.WithName("context"),
 	}
+
 	return &ctx
 }
 

pkg/engine/context/evaluate.go

@@ -10,9 +10,14 @@ import (
 
 //Query the JSON context with JMESPATH search path
 func (ctx *Context) Query(query string) (interface{}, error) {
+	query = strings.TrimSpace(query)
+	if query == "" {
+		return nil, fmt.Errorf("invalid query (nil)")
+	}
+
 	var emptyResult interface{}
 	// check for white-listed variables
-	if !ctx.isWhiteListed(query) {
+	if !ctx.isBuiltInVariable(query) {
 		return emptyResult, fmt.Errorf("variable %s cannot be used", query)
 	}
 
@@ -40,11 +45,11 @@ func (ctx *Context) Query(query string) (interface{}, error) {
 	return result, nil
 }
 
-func (ctx *Context) isWhiteListed(variable string) bool {
-	if len(ctx.whiteListVars) == 0 {
+func (ctx *Context) isBuiltInVariable(variable string) bool {
+	if len(ctx.builtInVars) == 0 {
 		return true
 	}
-	for _, wVar := range ctx.whiteListVars {
+	for _, wVar := range ctx.builtInVars {
 		if strings.HasPrefix(variable, wVar) {
 			return true
 		}

pkg/engine/validation.go

@@ -96,7 +96,7 @@ func startResultResponse(resp *response.EngineResponse, policy kyverno.ClusterPo
 
 func endResultResponse(log logr.Logger, resp *response.EngineResponse, startTime time.Time) {
 	resp.PolicyResponse.ProcessingTime = time.Since(startTime)
-	log.V(4).Info("finshed processing", "processingTime", resp.PolicyResponse.ProcessingTime.String(), "validationRulesApplied", resp.PolicyResponse.RulesAppliedCount)
+	log.V(4).Info("finished processing", "processingTime", resp.PolicyResponse.ProcessingTime.String(), "validationRulesApplied", resp.PolicyResponse.RulesAppliedCount)
 }
 
 func incrementAppliedCount(resp *response.EngineResponse) {

pkg/engine/variables/vars.go

@@ -10,12 +10,16 @@ import (
 	"github.com/kyverno/kyverno/pkg/engine/context"
 )
 
-const (
-	variableRegex = `\{\{([^{}]*)\}\}`
-)
+var regexVariables = regexp.MustCompile(`\{\{[^{}]*\}\}`)
+
+//IsVariable returns true if the element contains a 'valid' variable {{}}
+func IsVariable(element string) bool {
+	groups := regexVariables.FindAllStringSubmatch(element, -1)
+	return len(groups) != 0
+}
 
 //SubstituteVars replaces the variables with the values defined in the context
-// - if any variable is invaid or has nil value, it is considered as a failed varable substitution
+// - if any variable is invalid or has nil value, it is considered as a failed variable substitution
 func SubstituteVars(log logr.Logger, ctx context.EvalInterface, pattern interface{}) (interface{}, error) {
 	pattern, err := subVars(log, ctx, pattern, "")
 	if err != nil {
@@ -31,15 +35,16 @@ func subVars(log logr.Logger, ctx context.EvalInterface, pattern interface{}, pa
 		for k, v := range typedPattern {
 			mapCopy[k] = v
 		}
-
 		return subMap(log, ctx, mapCopy, path)
+
 	case []interface{}:
 		sliceCopy := make([]interface{}, len(typedPattern))
 		copy(sliceCopy, typedPattern)
-
 		return subArray(log, ctx, sliceCopy, path)
+
 	case string:
-		return subValR(ctx, typedPattern, path)
+		return subValR(log, ctx, typedPattern, path)
+
 	default:
 		return pattern, nil
 	}
@@ -81,35 +86,36 @@ func (n NotFoundVariableErr) Error() string {
 }
 
 // subValR resolves the variables if defined
-func subValR(ctx context.EvalInterface, valuePattern string, path string) (interface{}, error) {
+func subValR(log logr.Logger, ctx context.EvalInterface, valuePattern string, path string) (interface{}, error) {
 	originalPattern := valuePattern
+	vars := regexVariables.FindAllString(valuePattern, -1)
+	for _, v := range vars {
+		variable := strings.ReplaceAll(v, "{{", "")
+		variable = strings.ReplaceAll(variable, "}}", "")
+		variable = strings.TrimSpace(variable)
+		substitutedVar, err := ctx.Query(variable)
+		if err != nil {
+			return nil, fmt.Errorf("failed to resolve %v at path %s", variable, path)
+		}
+
+		log.V(3).Info("variable substituted", "variable", v, "value", substitutedVar, "path", path)
+
+		if val, ok := substitutedVar.(string); ok {
+			valuePattern = strings.Replace(valuePattern, v, val, -1)
+			continue
+		}
 
-	regex := regexp.MustCompile(`\{\{([^{}]*)\}\}`)
-	for {
-		if vars := regex.FindAllString(valuePattern, -1); len(vars) > 0 {
-			for _, variable := range vars {
-				underlyingVariable := strings.ReplaceAll(strings.ReplaceAll(variable, "}}", ""), "{{", "")
-				substitutedVar, err := ctx.Query(underlyingVariable)
-				if err != nil {
-					return nil, fmt.Errorf("failed to resolve %v at path %s", underlyingVariable, path)
-				}
-				if val, ok := substitutedVar.(string); ok {
-					valuePattern = strings.Replace(valuePattern, variable, val, -1)
-				} else {
-					if substitutedVar != nil {
-						if originalPattern == variable {
-							return substitutedVar, nil
-						}
-						return nil, fmt.Errorf("failed to resolve %v at path %s", underlyingVariable, path)
-					}
-					return nil, NotFoundVariableErr{
-						variable: underlyingVariable,
-						path:     path,
-					}
-				}
+		if substitutedVar != nil {
+			if originalPattern == v {
+				return substitutedVar, nil
 			}
-		} else {
-			break
+
+			return nil, fmt.Errorf("failed to resolve %v at path %s", variable, path)
+		}
+
+		return nil, NotFoundVariableErr{
+			variable: variable,
+			path:     path,
 		}
 	}
 

pkg/engine/variables/vars_test.go

@@ -152,5 +152,5 @@ func Test_SubvarRecursive(t *testing.T) {
 
 	ctx := context.NewContext()
 	assert.Assert(t, ctx.AddResource(resourceRaw))
-	subValR(ctx, string(patternRaw), "/")
+	subValR(log.Log, ctx, string(patternRaw), "/")
 }

pkg/policy/background.go

@@ -29,11 +29,11 @@ func ContainsVariablesOtherThanObject(policy kyverno.ClusterPolicy) error {
 		ctx := context.NewContext(filterVars...)
 		for condIdx, condition := range rule.Conditions {
 			if condition.Key, err = variables.SubstituteVars(log.Log, ctx, condition.Key); !checkNotFoundErr(err) {
-				return fmt.Errorf("invalid variable used at spec/rules[%d]/condition[%d]/key", idx, condIdx)
+				return fmt.Errorf("invalid variable %s used at spec/rules[%d]/condition[%d]/key",condition.Key, idx, condIdx)
 			}
 
 			if condition.Value, err = variables.SubstituteVars(log.Log, ctx, condition.Value); !checkNotFoundErr(err) {
-				return fmt.Errorf("invalid variable used at spec/rules[%d]/condition[%d]/value", idx, condIdx)
+				return fmt.Errorf("invalid %s variable used at spec/rules[%d]/condition[%d]/value", condition.Value, idx, condIdx)
 			}
 		}
 
@@ -67,10 +67,12 @@ func ContainsVariablesOtherThanObject(policy kyverno.ClusterPolicy) error {
 		if rule.Validation.Deny != nil {
 			for i := range rule.Validation.Deny.Conditions {
 				if _, err = variables.SubstituteVars(log.Log, ctx, rule.Validation.Deny.Conditions[i].Key); !checkNotFoundErr(err) {
-					return fmt.Errorf("invalid variable used at spec/rules[%d]/validate/deny/conditions[%d]/key", idx, i)
+					return fmt.Errorf("invalid variable %s used at spec/rules[%d]/validate/deny/conditions[%d]/key: %v",
+						rule.Validation.Deny.Conditions[i].Key, idx, i, err)
 				}
 				if _, err = variables.SubstituteVars(log.Log, ctx, rule.Validation.Deny.Conditions[i].Value); !checkNotFoundErr(err) {
-					return fmt.Errorf("invalid variable used at spec/rules[%d]/validate/deny/conditions[%d]/value", idx, i)
+					return fmt.Errorf("invalid variable %s used at spec/rules[%d]/validate/deny/conditions[%d]/value: %v",
+						rule.Validation.Deny.Conditions[i].Value, idx, i, err)
 				}
 			}
 		}

pkg/policy/validate.go

@@ -34,15 +34,15 @@ func Validate(policyRaw []byte, client *dclient.Client, mock bool, openAPIContro
 	}
 
 	if len(common.PolicyHasVariables(p)) > 0 && common.PolicyHasNonAllowedVariables(p) {
-		return fmt.Errorf("policy contains unknown variables")
+		return fmt.Errorf("policy contains invalid variables")
 	}
 
 	if path, err := validateUniqueRuleName(p); err != nil {
 		return fmt.Errorf("path: spec.%s: %v", path, err)
 	}
-	if p.Spec.Background == nil || (p.Spec.Background != nil && *p.Spec.Background) {
+	if p.Spec.Background == nil || *p.Spec.Background == true {
 		if err := ContainsVariablesOtherThanObject(p); err != nil {
-			return fmt.Errorf("only variables referring request.object are allowed in background mode. Set spec.background=false to disable background mode for this policy rule. %s ", err)
+			return fmt.Errorf("only select variables are allowed in background mode. Set spec.background=false to disable background mode for this policy rule. %s ", err)
 		}
 	}
 

pkg/policy/validate/validate.go

@@ -37,8 +37,6 @@ func (v *Validate) Validate() (string, error) {
 	}
 
 	if rule.AnyPattern != nil {
-		// validation := &kyverno.Validation{}
-		// rule.DeepCopyInto(validation)
 		anyPattern, err := rule.DeserializeAnyPattern()
 		if err != nil {
 			return "anyPattern", fmt.Errorf("failed to deserialze anyPattern, expect array: %v", err)

pkg/webhookconfig/checker.go

@@ -77,7 +77,7 @@ func (wrc *WebhookRegistrationClient) removeVerifyWebhookMutatingWebhookConfig(w
 	}
 
 	if err != nil {
-		logger.Error(err, "failed to delete verify wwebhook configuration")
+		logger.Error(err, "failed to delete verify webhook configuration")
 		return
 	}
 

pkg/webhooks/common.go

@@ -34,7 +34,7 @@ func toBlockResource(engineReponses []response.EngineResponse, log logr.Logger)
 			return true
 		}
 	}
-	log.V(4).Info("sepc.ValidationFailureAction set to audit for all applicable policies, won't block resource operation")
+	log.V(4).Info("spec.ValidationFailureAction set to audit for all applicable policies, won't block resource operation")
 	return false
 }