Bug fixes - policy validation, auto-generated rules, apiCall support in mutate and generate (#1629)

* Fix invalid policy reports generated for blocked resource

Signed-off-by: Shuting Zhao <shutting06@gmail.com>

* fix 1464 - copy context and preconditions to auto-gen rules

Signed-off-by: Shuting Zhao <shutting06@gmail.com>

* fix 1628 - add policy validations

Signed-off-by: Shuting Zhao <shutting06@gmail.com>

* fix 1593 - support apiCall in mutate and generate

Signed-off-by: Shuting Zhao <shutting06@gmail.com>

* fix test

Signed-off-by: Shuting Zhao <shutting06@gmail.com>

Makefile

@@ -68,6 +68,7 @@ docker-build-kyverno:
 docker-build-local-kyverno:
 	CGO_ENABLED=0 GOOS=linux go build -o $(PWD)/$(KYVERNO_PATH)/kyverno -ldflags=$(LD_FLAGS) $(PWD)/$(KYVERNO_PATH)/main.go
 	@docker build -f $(PWD)/$(KYVERNO_PATH)/localDockerfile -t $(REPO)/$(KYVERNO_IMAGE):$(IMAGE_TAG) $(PWD)/$(KYVERNO_PATH)
+	@docker tag $(REPO)/$(KYVERNO_IMAGE):$(IMAGE_TAG) $(REPO)/$(KYVERNO_IMAGE):latest
 
 docker-build-kyverno-amd64:
 	@docker build -f $(PWD)/$(KYVERNO_PATH)/Dockerfile -t $(REPO)/$(KYVERNO_IMAGE):$(IMAGE_TAG) . --build-arg LD_FLAGS=$(LD_FLAGS) --build-arg TARGETPLATFORM="linux/amd64"

pkg/engine/context/context.go

@@ -2,14 +2,14 @@ package context
 
 import (
 	"encoding/json"
+	"fmt"
 	"strings"
 	"sync"
 
-	"k8s.io/api/admission/v1beta1"
-
 	jsonpatch "github.com/evanphx/json-patch"
 	"github.com/go-logr/logr"
 	kyverno "github.com/kyverno/kyverno/pkg/api/kyverno/v1"
+	"k8s.io/api/admission/v1beta1"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 )
 
@@ -57,6 +57,16 @@ func NewContext(builtInVars ...string) *Context {
 	return &ctx
 }
 
+// InvalidVariableErr represents error for non-white-listed variables
+type InvalidVariableErr struct {
+	variable  string
+	whiteList []string
+}
+
+func (i InvalidVariableErr) Error() string {
+	return fmt.Sprintf("variable %s cannot be used, allowed variables: %v", i.variable, i.whiteList)
+}
+
 // AddJSON merges json data
 func (ctx *Context) AddJSON(dataRaw []byte) error {
 	var err error
@@ -71,7 +81,7 @@ func (ctx *Context) AddJSON(dataRaw []byte) error {
 	return nil
 }
 
-// AddRequest addes an admission request to context
+// AddRequest adds an admission request to context
 func (ctx *Context) AddRequest(request *v1beta1.AdmissionRequest) error {
 	modifiedResource := struct {
 		Request interface{} `json:"request"`
@@ -90,10 +100,10 @@ func (ctx *Context) AddRequest(request *v1beta1.AdmissionRequest) error {
 //AddResource data at path: request.object
 func (ctx *Context) AddResource(dataRaw []byte) error {
 
-	// unmarshall the resource struct
+	// unmarshal the resource struct
 	var data interface{}
 	if err := json.Unmarshal(dataRaw, &data); err != nil {
-		ctx.log.Error(err, "failed to unmarshall the resource")
+		ctx.log.Error(err, "failed to unmarshal the resource")
 		return err
 	}
 
@@ -203,3 +213,20 @@ func (ctx *Context) Restore() {
 	ctx.jsonRaw = make([]byte, len(ctx.jsonRawCheckpoint))
 	copy(ctx.jsonRaw, ctx.jsonRawCheckpoint)
 }
+
+// AddBuiltInVars adds given pattern to the builtInVars
+func (ctx *Context) AddBuiltInVars(pattern string) {
+	ctx.mutex.Lock()
+	defer ctx.mutex.Unlock()
+
+	builtInVarsCopy := ctx.builtInVars
+	ctx.builtInVars = append(builtInVarsCopy, pattern)
+}
+
+func (ctx *Context) getBuiltInVars() []string {
+	ctx.mutex.RLock()
+	defer ctx.mutex.RUnlock()
+
+	vars := ctx.builtInVars
+	return vars
+}

pkg/engine/context/evaluate.go

@@ -18,7 +18,10 @@ func (ctx *Context) Query(query string) (interface{}, error) {
 	var emptyResult interface{}
 	// check for white-listed variables
 	if !ctx.isBuiltInVariable(query) {
-		return emptyResult, fmt.Errorf("variable %s cannot be used", query)
+		return emptyResult, InvalidVariableErr{
+			variable:  query,
+			whiteList: ctx.getBuiltInVars(),
+		}
 	}
 
 	// compile the query
@@ -34,7 +37,7 @@ func (ctx *Context) Query(query string) (interface{}, error) {
 	var data interface{}
 	if err := json.Unmarshal(ctx.jsonRaw, &data); err != nil {
 		ctx.log.Error(err, "failed to unmarshal context")
-		return emptyResult, fmt.Errorf("failed to unmarshall context: %v", err)
+		return emptyResult, fmt.Errorf("failed to unmarshal context: %v", err)
 	}
 
 	result, err := queryPath.Search(data)
@@ -46,10 +49,10 @@ func (ctx *Context) Query(query string) (interface{}, error) {
 }
 
 func (ctx *Context) isBuiltInVariable(variable string) bool {
-	if len(ctx.builtInVars) == 0 {
+	if len(ctx.getBuiltInVars()) == 0 {
 		return true
 	}
-	for _, wVar := range ctx.builtInVars {
+	for _, wVar := range ctx.getBuiltInVars() {
 		if strings.HasPrefix(variable, wVar) {
 			return true
 		}

pkg/engine/generation.go

@@ -86,9 +86,8 @@ func filterRule(rule kyverno.Rule, policyContext *PolicyContext) *response.RuleR
 	policyContext.JSONContext.Checkpoint()
 	defer policyContext.JSONContext.Restore()
 
-	// add configmap json data to context
 	if err := LoadContext(logger, rule.Context, resCache, policyContext); err != nil {
-		logger.V(4).Info("cannot add configmaps to context", "reason", err.Error())
+		logger.V(4).Info("cannot add external data to the context", "reason", err.Error())
 		return nil
 	}
 

pkg/engine/jsonContext.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+
 	"github.com/go-logr/logr"
 	"github.com/jmespath/go-jmespath"
 	kyverno "github.com/kyverno/kyverno/pkg/api/kyverno/v1"

pkg/engine/mutate/patchJson6902_test.go

@@ -58,7 +58,7 @@ func TestTypeConversion(t *testing.T) {
 		[]byte(`{"path":"/spec/template/spec/containers/0/name","op":"replace","value":"my-nginx"}`),
 	}
 
-	// serilize resource
+	// serialize resource
 	inputJSONgo, err := yaml.YAMLToJSON(inputBytes)
 	assert.Nil(t, err)
 

pkg/engine/mutation.go

@@ -71,7 +71,7 @@ func Mutate(policyContext *PolicyContext) (resp *response.EngineResponse) {
 
 		policyContext.JSONContext.Restore()
 		if err := LoadContext(logger, rule.Context, resCache, policyContext); err != nil {
-			logger.V(2).Info("failed to load context", "reason", err.Error())
+			logger.Error(err, "failed to load context")
 			continue
 		}
 

pkg/engine/validation.go

@@ -106,7 +106,7 @@ func validateResource(log logr.Logger, ctx *PolicyContext) *response.EngineRespo
 
 		ctx.JSONContext.Restore()
 		if err := LoadContext(log, rule.Context, ctx.ResourceCache, ctx); err != nil {
-			log.V(2).Info("failed to load context", "reason", err.Error())
+			log.Error(err, "failed to load context")
 			continue
 		}
 

pkg/engine/variables/vars.go

@@ -96,7 +96,12 @@ func subValR(log logr.Logger, ctx context.EvalInterface, valuePattern string, pa
 			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)
+				switch err.(type) {
+				case context.InvalidVariableErr:
+					return nil, err
+				default:
+					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)

pkg/engine/variables/vars_test.go

@@ -202,3 +202,27 @@ func Test_SubstituteRecursive(t *testing.T) {
 		t.Errorf("expected %s received %v", "temp", results)
 	}
 }
+
+func Test_policyContextValidation(t *testing.T) {
+	policyContext := []byte(`
+	{
+		"context": [
+			{
+				"name": "myconfigmap",
+				"apiCall": {
+					"urlPath": "/api/v1/namespaces/{{ request.namespace }}/configmaps/generate-pod"
+				}
+			}
+		]
+	}
+	`)
+
+	var contextMap interface{}
+	err := json.Unmarshal(policyContext, &contextMap)
+	assert.NilError(t, err)
+
+	ctx := context.NewContext("request.object")
+
+	_, err = SubstituteVars(log.Log, ctx, contextMap)
+	assert.Assert(t, err != nil, err)
+}

pkg/generate/generate.go

@@ -132,6 +132,7 @@ func (c *Controller) applyGenerate(resource unstructured.Unstructured, gr kyvern
 		ResourceCache:       c.resCache,
 		JSONContext:         ctx,
 		NamespaceLabels:     namespaceLabels,
+		Client:              c.client,
 	}
 
 	// check if the policy still applies to the resource

pkg/policy/background.go

@@ -20,20 +20,43 @@ func ContainsVariablesOtherThanObject(policy kyverno.ClusterPolicy) error {
 		if path := userInfoDefined(rule.ExcludeResources.UserInfo); path != "" {
 			return fmt.Errorf("invalid variable used at path: spec/rules[%d]/exclude/%s", idx, path)
 		}
-		// Skip Validation if rule contains Context
-		if len(rule.Context) > 0 {
-			return nil
-		}
 
 		filterVars := []string{"request.object"}
 		ctx := context.NewContext(filterVars...)
+
+		for contextIdx, contextEntry := range rule.Context {
+			if contextEntry.APICall != nil {
+				ctx.AddBuiltInVars(contextEntry.Name)
+
+				if _, err := variables.SubstituteVars(log.Log, ctx, contextEntry.APICall.URLPath); !checkNotFoundErr(err) {
+					return fmt.Errorf("invalid variable used at spec/rules[%d]/context[%d]/apiCall/urlPath: %s", idx, contextIdx, err.Error())
+				}
+
+				if _, err := variables.SubstituteVars(log.Log, ctx, contextEntry.APICall.JMESPath); !checkNotFoundErr(err) {
+					return fmt.Errorf("invalid variable used at spec/rules[%d]/context[%d]/apiCall/jmesPath: %s", idx, contextIdx, err.Error())
+				}
+			}
+
+			if contextEntry.ConfigMap != nil {
+				ctx.AddBuiltInVars(contextEntry.Name)
+
+				if _, err = variables.SubstituteVars(log.Log, ctx, contextEntry.ConfigMap.Name); !checkNotFoundErr(err) {
+					return fmt.Errorf("invalid variable used at spec/rules[%d]/context[%d]/configMap/name", idx, contextIdx)
+				}
+
+				if _, err = variables.SubstituteVars(log.Log, ctx, contextEntry.ConfigMap.Namespace); !checkNotFoundErr(err) {
+					return fmt.Errorf("invalid variable used at spec/rules[%d]/context[%d]/configMap/namespace", idx, contextIdx)
+				}
+			}
+		}
+
 		for condIdx, condition := range rule.Conditions {
 			if condition.Key, err = variables.SubstituteVars(log.Log, ctx, condition.Key); !checkNotFoundErr(err) {
-				return fmt.Errorf("invalid variable %s used at spec/rules[%d]/condition[%d]/key", condition.Key, idx, condIdx)
+				return fmt.Errorf("invalid variable %v 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 %s variable used at spec/rules[%d]/condition[%d]/value", condition.Value, idx, condIdx)
+				return fmt.Errorf("invalid %v variable used at spec/rules[%d]/condition[%d]/value: %v", condition.Value, idx, condIdx, err)
 			}
 		}
 
@@ -43,9 +66,15 @@ func ContainsVariablesOtherThanObject(policy kyverno.ClusterPolicy) error {
 			}
 		}
 
+		if rule.Mutation.PatchStrategicMerge != nil {
+			if rule.Mutation.Overlay, err = variables.SubstituteVars(log.Log, ctx, rule.Mutation.PatchStrategicMerge); !checkNotFoundErr(err) {
+				return fmt.Errorf("invalid variable used at spec/rules[%d]/mutate/patchStrategicMerge", idx)
+			}
+		}
+
 		if rule.Validation.Pattern != nil {
 			if rule.Validation.Pattern, err = variables.SubstituteVars(log.Log, ctx, rule.Validation.Pattern); !checkNotFoundErr(err) {
-				return fmt.Errorf("invalid variable used at spec/rules[%d]/validate/pattern", idx)
+				return fmt.Errorf("invalid variable used at spec/rules[%d]/validate/pattern: %v", idx, err)
 			}
 		}
 
@@ -76,6 +105,26 @@ func ContainsVariablesOtherThanObject(policy kyverno.ClusterPolicy) error {
 				}
 			}
 		}
+
+		if _, err = variables.SubstituteVars(log.Log, ctx, rule.Generation.Name); !checkNotFoundErr(err) {
+			return fmt.Errorf("invalid variable used at spec/rules[%d]/generate/name: %v", idx, err)
+		}
+
+		if _, err = variables.SubstituteVars(log.Log, ctx, rule.Generation.Namespace); !checkNotFoundErr(err) {
+			return fmt.Errorf("invalid variable used at spec/rules[%d]/generate/name: %v", idx, err)
+		}
+
+		if _, err = variables.SubstituteVars(log.Log, ctx, rule.Generation.Data); !checkNotFoundErr(err) {
+			return fmt.Errorf("invalid variable used at spec/rules[%d]/generate/data: %v", idx, err)
+		}
+
+		if _, err = variables.SubstituteVars(log.Log, ctx, rule.Generation.Clone.Name); !checkNotFoundErr(err) {
+			return fmt.Errorf("invalid variable used at spec/rules[%d]/generate/clone/name: %v", idx, err)
+		}
+
+		if _, err = variables.SubstituteVars(log.Log, ctx, rule.Generation.Clone.Namespace); !checkNotFoundErr(err) {
+			return fmt.Errorf("invalid variable used at spec/rules[%d]/generate/clone/namespace: %v", idx, err)
+		}
 	}
 
 	return nil
@@ -86,6 +135,9 @@ func checkNotFoundErr(err error) bool {
 		switch err.(type) {
 		case variables.NotFoundVariableErr:
 			return true
+		case context.InvalidVariableErr:
+			// non-white-listed variable is found
+			return false
 		default:
 			return false
 		}

pkg/policy/generate/validate.go

@@ -65,7 +65,7 @@ func (g *Generate) Validate() (string, error) {
 
 	// Kyverno generate-controller create/update/deletes the resources specified in generate rule of policy
 	// kyverno uses SA 'kyverno-service-account' and has default ClusterRoles and ClusterRoleBindings
-	// instuctions to modify the RBAC for kyverno are mentioned at https://github.com/kyverno/kyverno/blob/master/documentation/installation.md
+	// instructions to modify the RBAC for kyverno are mentioned at https://github.com/kyverno/kyverno/blob/master/documentation/installation.md
 	// - operations required: create/update/delete/get
 	// If kind and namespace contain variables, then we cannot resolve then so we skip the processing
 	if err := g.canIGenerate(kind, namespace); err != nil {
@@ -96,7 +96,7 @@ func (g *Generate) validateClone(c kyverno.CloneFrom, kind string) (string, erro
 	return "", nil
 }
 
-//canIGenerate returns a error if kyverno cannot perform oprations
+//canIGenerate returns a error if kyverno cannot perform operations
 func (g *Generate) canIGenerate(kind, namespace string) error {
 	// Skip if there is variable defined
 	authCheck := g.authCheck

pkg/policy/validate.go

@@ -40,7 +40,7 @@ func Validate(policy *kyverno.ClusterPolicy, client *dclient.Client, mock bool,
 	}
 	if p.Spec.Background == nil || *p.Spec.Background == true {
 		if err := ContainsVariablesOtherThanObject(p); err != nil {
-			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)
+			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_test.go

@@ -983,11 +983,7 @@ func Test_BackGroundUserInfo_validate_pattern(t *testing.T) {
 	assert.NilError(t, err)
 
 	err = ContainsVariablesOtherThanObject(*policy)
-
-	if err.Error() != "invalid variable used at spec/rules[0]/validate/pattern" {
-		t.Log(err)
-		t.Error("Incorrect Path")
-	}
+	assert.Assert(t, err != nil, err)
 }
 
 func Test_BackGroundUserInfo_validate_anyPattern(t *testing.T) {

pkg/policymutation/policymutation.go

@@ -359,6 +359,8 @@ type kyvernoRule struct {
 	Name             string                    `json:"name"`
 	MatchResources   *kyverno.MatchResources   `json:"match"`
 	ExcludeResources *kyverno.ExcludeResources `json:"exclude,omitempty"`
+	Context          *[]kyverno.ContextEntry   `json:"context,omitempty"`
+	Conditions       *[]kyverno.Condition      `json:"preconditions,omitempty"`
 	Mutation         *kyverno.Mutation         `json:"mutate,omitempty"`
 	Validation       *kyverno.Validation       `json:"validate,omitempty"`
 }
@@ -423,6 +425,14 @@ func generateRuleForControllers(rule kyverno.Rule, controllers string, log logr.
 		MatchResources: match.DeepCopy(),
 	}
 
+	if len(rule.Context) > 0 {
+		controllerRule.Context = &rule.DeepCopy().Context
+	}
+
+	if len(rule.Conditions) > 0 {
+		controllerRule.Conditions = &rule.DeepCopy().Conditions
+	}
+
 	if !reflect.DeepEqual(exclude, kyverno.ExcludeResources{}) {
 		controllerRule.ExcludeResources = exclude.DeepCopy()
 	}

pkg/webhooks/generation.go

@@ -51,6 +51,7 @@ func (ws *WebhookServer) HandleGenerate(request *v1beta1.AdmissionRequest, polic
 			ExcludeResourceFunc: ws.configHandler.ToFilter,
 			ResourceCache:       ws.resCache,
 			JSONContext:         ctx,
+			Client:              ws.client,
 		}
 
 		for _, policy := range policies {

pkg/webhooks/mutation.go

@@ -45,6 +45,7 @@ func (ws *WebhookServer) HandleMutation(
 		ExcludeResourceFunc: ws.configHandler.ToFilter,
 		ResourceCache:       ws.resCache,
 		JSONContext:         ctx,
+		Client:              ws.client,
 	}
 
 	if request.Operation == v1beta1.Update {

pkg/webhooks/server.go

@@ -417,14 +417,14 @@ func (ws *WebhookServer) resourceValidation(request *v1beta1.AdmissionRequest) *
 
 	logger.V(6).Info("received an admission request in validating webhook")
 
-	// push admission request to audit handler, this won't block the admission request
-	ws.auditHandler.Add(request.DeepCopy())
-
 	policies := ws.pCache.Get(policycache.ValidateEnforce, nil)
 	// Get namespace policies from the cache for the requested resource namespace
 	nsPolicies := ws.pCache.Get(policycache.ValidateEnforce, &request.Namespace)
 	policies = append(policies, nsPolicies...)
 	if len(policies) == 0 {
+		// push admission request to audit handler, this won't block the admission request
+		ws.auditHandler.Add(request.DeepCopy())
+
 		logger.V(4).Info("no enforce validation policies; returning AdmissionResponse.Allowed: true")
 		return &v1beta1.AdmissionResponse{Allowed: true}
 	}