Fix old object validation check (#3248)

* fix validation check on UPDATE

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* prevent policy bypass using preconditions

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* separate replace

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* add error handling

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

pkg/engine/context/context.go

@@ -170,17 +170,33 @@ func (ctx *Context) AddResourceInOldObject(dataRaw []byte) error {
 }
 
 func (ctx *Context) AddResourceAsObject(data interface{}) error {
-	modifiedResource := struct {
-		Request interface{} `json:"request"`
-	}{
-		Request: struct {
-			Object interface{} `json:"object"`
-		}{
-			Object: data,
-		},
+	return ctx.addToRequest(data, "object")
+}
+
+func (ctx *Context) ReplaceResourceAsObject(data interface{}) error {
+	if err := ctx.addToRequest(nil, "object"); err != nil {
+		return err
 	}
 
-	objRaw, err := json.Marshal(modifiedResource)
+	return ctx.addToRequest(data, "object")
+}
+
+func (ctx *Context) ReplaceResourceAsOldObject(data interface{}) error {
+	if err := ctx.addToRequest(nil, "oldObject"); err != nil {
+		return err
+	}
+
+	return ctx.addToRequest(data, "oldObject")
+}
+
+func (ctx *Context) addToRequest(data interface{}, tag string) error {
+	requestData := make(map[string]interface{})
+	requestData[tag] = data
+	request := map[string]interface{}{
+		"request": requestData,
+	}
+
+	objRaw, err := json.Marshal(request)
 	if err != nil {
 		ctx.log.Error(err, "failed to marshal the resource")
 		return err

pkg/engine/validation.go

@@ -111,6 +111,22 @@ func validateResource(log logr.Logger, ctx *PolicyContext) *response.EngineRespo
 	return resp
 }
 
+func validateOldObject(log logr.Logger, ctx *PolicyContext, rule *kyverno.Rule) (*response.RuleResponse, error) {
+	ctxCopy := ctx.Copy()
+	ctxCopy.NewResource = *ctxCopy.OldResource.DeepCopy()
+	ctxCopy.OldResource = unstructured.Unstructured{}
+
+	if err := ctxCopy.JSONContext.ReplaceResourceAsObject(ctxCopy.NewResource.Object); err != nil {
+		return nil, errors.Wrapf(err, "failed to replace object in the JSON context")
+	}
+
+	if err := ctxCopy.JSONContext.ReplaceResourceAsOldObject(ctxCopy.OldResource.Object); err != nil {
+		return nil, errors.Wrapf(err, "failed to replace old object in the JSON context")
+	}
+
+	return processValidationRule(log, ctxCopy, rule), nil
+}
+
 func processValidationRule(log logr.Logger, ctx *PolicyContext, rule *kyverno.Rule) *response.RuleResponse {
 	v := newValidator(log, ctx, rule)
 	if rule.Validation.ForEachValidation != nil {
@@ -192,16 +208,28 @@ func (v *validator) validate() *response.RuleResponse {
 		return ruleResponse(v.rule, utils.Validation, "preconditions not met", response.RuleStatusSkip)
 	}
 
+	if v.deny != nil {
+		return v.validateDeny()
+	}
+
 	if v.pattern != nil || v.anyPattern != nil {
 		if err = v.substitutePatterns(); err != nil {
 			return ruleError(v.rule, utils.Validation, "variable substitution failed", err)
 		}
 
 		ruleResponse := v.validateResourceWithRule()
-		return ruleResponse
+		if isUpdateRequest(v.ctx) {
+			priorResp, err := validateOldObject(v.log, v.ctx, v.rule)
+			if err != nil {
+				return ruleError(v.rule, utils.Validation, "failed to validate old object", err)
+			}
+
+			if isSameRuleResponse(ruleResponse, priorResp) {
+				v.log.V(3).Info("skipping modified resource as validation results have not changed")
+				return nil
+			}
+		}
 
-	} else if v.deny != nil {
-		ruleResponse := v.validateDeny()
 		return ruleResponse
 	}
 
@@ -372,27 +400,23 @@ func (v *validator) validateResourceWithRule() *response.RuleResponse {
 		return v.validatePatterns(v.ctx.Element)
 	}
 
-	// if the OldResource is empty, the request is a CREATE
-	if isEmptyUnstructured(&v.ctx.OldResource) {
-		resp := v.validatePatterns(v.ctx.NewResource)
-		return resp
-	}
-
-	// if the OldResource is not empty, and the NewResource is empty, the request is a DELETE
-	if isEmptyUnstructured(&v.ctx.NewResource) {
+	if isDeleteRequest(v.ctx) {
 		v.log.V(3).Info("skipping validation on deleted resource")
 		return nil
 	}
 
-	// if the OldResource is not empty, and the NewResource is not empty, the request is a MODIFY
-	oldResp := v.validatePatterns(v.ctx.OldResource)
-	newResp := v.validatePatterns(v.ctx.NewResource)
-	if isSameRuleResponse(oldResp, newResp) {
-		v.log.V(3).Info("skipping modified resource as validation results have not changed")
-		return nil
-	}
+	resp := v.validatePatterns(v.ctx.NewResource)
+	return resp
+}
 
-	return newResp
+func isDeleteRequest(ctx *PolicyContext) bool {
+	// if the OldResource is not empty, and the NewResource is empty, the request is a DELETE
+	return isEmptyUnstructured(&ctx.NewResource)
+}
+
+func isUpdateRequest(ctx *PolicyContext) bool {
+	// is the OldObject and NewObject are available, the request is an UPDATE
+	return !isEmptyUnstructured(&ctx.OldResource) && !isEmptyUnstructured(&ctx.NewResource)
 }
 
 func isEmptyUnstructured(u *unstructured.Unstructured) bool {

pkg/engine/validation_test.go

@@ -3084,3 +3084,20 @@ func Test_delete_ignore_pattern(t *testing.T) {
 	engineResponseDelete := Validate(policyContextDelete)
 	assert.Equal(t, len(engineResponseDelete.PolicyResponse.Rules), 0)
 }
+
+func Test_block_bypass(t *testing.T) {
+	testcases := []testCase{
+		{
+			description:   "Blocks bypass of policy by manipulating pre-conditions",
+			policy:        []byte(`{"apiVersion":"kyverno.io/v1","kind":"Policy","metadata":{"name":"configmap-policy"},"spec":{"rules":[{"match":{"resources":{"kinds":["ConfigMap"]}},"name":"key-abc","preconditions":{"any":[{"key":"admin","operator":"Equals","value":"{{request.object.data.lock}}"}]},"validate":{"anyPattern":[{"data":{"key":"abc"}}],"message":"Configmap key must be \"abc\""}}]}}`),
+			request:       []byte(`{"uid":"7b0600b7-0258-4ecb-9666-c2839bd19612","kind":{"group":"","version":"v1","kind":"ConfigMap"},"resource":{"group":"","version":"v1","resource":"configmaps"},"subResource":"status","requestKind":{"group":"","version":"v1","kind":"configmaps"},"requestResource":{"group":"","version":"v1","resource":"configmaps"},"name":"test-configmap","namespace":"default","operation":"UPDATE","userInfo":{"username":"system:node:kind-control-plane","groups":["system:authenticated"]},"object":{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"test-configmap"},"data":{"key":"xyz","lock":"admin"}},"oldObject":{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"test-configmap"},"data":{"key":"xyz"}},"dryRun":false,"options":{"kind":"UpdateOptions","apiVersion":"meta.k8s.io/v1"}}`),
+			userInfo:      []byte(`{"roles":["kube-system:kubeadm:kubelet-config-1.17","kube-system:kubeadm:nodes-kubeadm-config"],"clusterRoles":["system:basic-user","system:certificates.k8s.io:certificatesigningrequests:selfnodeclient","system:public-info-viewer","system:discovery"],"userInfo":{"username":"kubernetes-admin","groups":["system:authenticated"]}}`),
+			requestDenied: true,
+		},
+	}
+
+	var err error
+	for _, testcase := range testcases {
+		executeTest(t, err, testcase)
+	}
+}