fix: IVPOL required has no effect (kyverno#16853)

Author: lucchmielowski

pkg/cel/libs/imageverify/impl.go

@@ -45,6 +45,7 @@ type ivfuncs struct {
 	ivCache         imageverifycache.Client
 	authOpts        []remote.Option
 	nameOpts        []name.Option
+	verifications   *ImageVerificationResults
 }
 
 func ImageVerifyCELFuncs(
@@ -54,6 +55,7 @@ func ImageVerifyCELFuncs(
 	lister corev1listers.SecretLister,
 	ivCache imageverifycache.Client,
 	adapter types.Adapter,
+	verifications *ImageVerificationResults,
 ) (*ivfuncs, error) {
 	if ivpol == nil {
 		return nil, fmt.Errorf("nil image verification policy")
@@ -88,6 +90,7 @@ func ImageVerifyCELFuncs(
 		ivCache:         ivCache,
 		nameOpts:        nameOpts,
 		authOpts:        authOpts[:],
+		verifications:   verifications,
 	}, nil
 }
 
@@ -135,6 +138,7 @@ func (f *ivfuncs) verify_image_signature_string_stringarray(image ref.Val, attes
 				f.logger.Error(err, "error occurred during image verify cache get", "image", image)
 			} else if found {
 				f.logger.V(4).Info("image signature verification cache hit", "image", image, "policy", f.policy.GetName())
+				f.verifications.Record(image, true)
 				return f.NativeToValue(len(attestors))
 			}
 		}
@@ -178,6 +182,9 @@ func (f *ivfuncs) verify_image_signature_string_stringarray(image ref.Val, attes
 				f.logger.Error(err, "error occurred during image verify cache set", "image", image)
 			}
 		}
+		if len(attestors) > 0 {
+			f.verifications.Record(image, count > 0)
+		}
 		return f.NativeToValue(count)
 	}
 }
@@ -208,6 +215,7 @@ func (f *ivfuncs) verify_image_attestations_string_string_stringarray(args ...re
 				f.logger.Error(err, "error occurred during image verify cache get", "image", image)
 			} else if found {
 				f.logger.V(4).Info("image attestation verification cache hit", "image", image, "policy", f.policy.GetName())
+				f.verifications.Record(image, true)
 				return f.NativeToValue(len(attestors))
 			}
 		}
@@ -257,6 +265,9 @@ func (f *ivfuncs) verify_image_attestations_string_string_stringarray(args ...re
 				f.logger.Error(err, "error occurred during image verify cache set", "image", image)
 			}
 		}
+		if len(attestors) > 0 {
+			f.verifications.Record(image, count > 0)
+		}
 		return f.NativeToValue(count)
 	}
 }

pkg/cel/libs/imageverify/impl_test.go

@@ -68,7 +68,7 @@ func Test_impl_verify_image_signature_string_stringarray(t *testing.T) {
 
 	options := []cel.EnvOption{
 		cel.Variable("attestors", cel.MapType(cel.StringType, cel.DynType)),
-		Lib(nil, imgCtx, ivpol, nil, logr.Discard(), nil),
+		Lib(nil, imgCtx, ivpol, nil, logr.Discard(), nil, NewImageVerificationResults()),
 	}
 	env, err := cel.NewEnv(options...)
 	assert.NoError(t, err)
@@ -106,7 +106,7 @@ func Test_impl_verify_image_attestations_string_string_stringarray(t *testing.T)
 
 	options := []cel.EnvOption{
 		cel.Variable("attestors", cel.MapType(cel.StringType, cel.DynType)),
-		Lib(nil, imgCtx, ivpol, nil, logr.Discard(), nil),
+		Lib(nil, imgCtx, ivpol, nil, logr.Discard(), nil, NewImageVerificationResults()),
 	}
 	env, err := cel.NewEnv(options...)
 	assert.NoError(t, err)

pkg/cel/libs/imageverify/lib.go

@@ -16,27 +16,32 @@ import (
 const libraryName = "kyverno.imageverify"
 
 type lib struct {
-	logger  logr.Logger
-	version *version.Version
-	imgCtx  imagedataloader.ImageContext
-	ivpol   policiesv1beta1.ImageValidatingPolicyLike
-	lister  corev1listers.SecretLister
-	ivCache imageverifycache.Client
+	logger        logr.Logger
+	version       *version.Version
+	imgCtx        imagedataloader.ImageContext
+	ivpol         policiesv1beta1.ImageValidatingPolicyLike
+	lister        corev1listers.SecretLister
+	ivCache       imageverifycache.Client
+	verifications *ImageVerificationResults
 }
 
 func Latest() *version.Version {
 	return versions.KyvernoLatest
 }
 
-func Lib(v *version.Version, imgCtx imagedataloader.ImageContext, ivpol policiesv1beta1.ImageValidatingPolicyLike, lister corev1listers.SecretLister, logger logr.Logger, ivCache imageverifycache.Client) cel.EnvOption {
+// Lib builds the image verification CEL library. The verification results are shared
+// with the caller, which reads them back after evaluation to enforce
+// validationConfigurations.required; pass nil when that enforcement is not needed.
+func Lib(v *version.Version, imgCtx imagedataloader.ImageContext, ivpol policiesv1beta1.ImageValidatingPolicyLike, lister corev1listers.SecretLister, logger logr.Logger, ivCache imageverifycache.Client, verifications *ImageVerificationResults) cel.EnvOption {
 	// create the cel lib env option
 	return cel.Lib(&lib{
-		logger:  logger,
-		version: v,
-		imgCtx:  imgCtx,
-		ivpol:   ivpol,
-		lister:  lister,
-		ivCache: ivCache,
+		logger:        logger,
+		version:       v,
+		imgCtx:        imgCtx,
+		ivpol:         ivpol,
+		lister:        lister,
+		ivCache:       ivCache,
+		verifications: verifications,
 	})
 }
 
@@ -59,7 +64,7 @@ func (*lib) ProgramOptions() []cel.ProgramOption {
 }
 
 func (c *lib) extendEnv(env *cel.Env) (*cel.Env, error) {
-	impl, err := ImageVerifyCELFuncs(c.logger, c.imgCtx, c.ivpol, c.lister, c.ivCache, env.CELTypeAdapter())
+	impl, err := ImageVerifyCELFuncs(c.logger, c.imgCtx, c.ivpol, c.lister, c.ivCache, env.CELTypeAdapter(), c.verifications)
 	if err != nil {
 		return nil, err
 	}

pkg/cel/libs/imageverify/results.go

@@ -0,0 +1,58 @@
+package imageverify
+
+import "sync"
+
+// ImageVerificationResults records, per image, whether a real cryptographic
+// signature or attestation check succeeded. IVPOL analogue of CPOL's
+// ImageVerificationMetadata (pkg/engine/api/imageverifymetadata.go).
+//
+// required can't trust a CEL expression's return value (that's exactly what it
+// exists to distrust) so verification functions record the real outcome here
+// instead, for EnforceRequired to read back.
+//
+// One instance is shared by every policy compiled for the same admission request,
+// so a wildcard required policy can see verifications done by other policies in
+// that request. It's bound into the CEL environment at compile time, so a compiled
+// policy belongs to that request and must not be cached/reused across requests.
+//
+// Only verification functions write to it (not exposed to CEL), and Record is
+// monotonic so a later no-op check can't clear an earlier genuine verification.
+// Both success and failure are recorded, to tell "never checked" apart from
+// "checked and failed".
+type ImageVerificationResults struct {
+	mu       sync.RWMutex
+	verified map[string]bool
+}
+
+func NewImageVerificationResults() *ImageVerificationResults {
+	return &ImageVerificationResults{verified: make(map[string]bool)}
+}
+
+// Record notes the outcome of a verification attempt for image. Callers must only
+// report success when a cryptographic check genuinely passed, not when a CEL
+// expression evaluated to true.
+func (r *ImageVerificationResults) Record(image string, verified bool) {
+	if r == nil {
+		return
+	}
+	r.mu.Lock()
+	defer r.mu.Unlock()
+	if r.verified == nil {
+		r.verified = make(map[string]bool)
+	}
+	r.verified[image] = r.verified[image] || verified
+}
+
+// Status reports whether image passed verification, and whether it was checked at
+// all. The two are separate because an image no policy tried to verify is a
+// different failure from one that was checked and rejected, and the caller
+// reports them with different messages.
+func (r *ImageVerificationResults) Status(image string) (verified bool, attempted bool) {
+	if r == nil {
+		return false, false
+	}
+	r.mu.RLock()
+	defer r.mu.RUnlock()
+	v, ok := r.verified[image]
+	return v, ok
+}

pkg/cel/libs/imageverify/results_test.go

@@ -0,0 +1,95 @@
+package imageverify
+
+import (
+	"sync"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestImageVerificationResults_StatusDistinguishesNeverCheckedFromFailed(t *testing.T) {
+	t.Parallel()
+	l := NewImageVerificationResults()
+
+	// never recorded
+	verified, attempted := l.Status("ghcr.io/kyverno/unseen:latest")
+	assert.False(t, verified)
+	assert.False(t, attempted, "an image no expression checked must not look like a failed check")
+
+	l.Record("ghcr.io/kyverno/failed:latest", false)
+	verified, attempted = l.Status("ghcr.io/kyverno/failed:latest")
+	assert.False(t, verified)
+	assert.True(t, attempted, "a failed check must be distinguishable from no check at all")
+
+	l.Record("ghcr.io/kyverno/ok:latest", true)
+	verified, attempted = l.Status("ghcr.io/kyverno/ok:latest")
+	assert.True(t, verified)
+	assert.True(t, attempted)
+}
+
+// A policy's expressions all share one set of results, so an image verified by one
+// expression must stay verified when a later expression checks it against a
+// different attestor and fails. Without this the verdict would depend on the
+// order the expressions happen to be written in.
+func TestImageVerificationResults_RecordIsMonotonic(t *testing.T) {
+	t.Parallel()
+	const image = "ghcr.io/kyverno/test-verify-image:signed"
+
+	t.Run("success then failure", func(t *testing.T) {
+		t.Parallel()
+		l := NewImageVerificationResults()
+		l.Record(image, true)
+		l.Record(image, false)
+		verified, _ := l.Status(image)
+		assert.True(t, verified, "a later failed check must not clear an earlier genuine verification")
+	})
+
+	t.Run("failure then success", func(t *testing.T) {
+		t.Parallel()
+		l := NewImageVerificationResults()
+		l.Record(image, false)
+		l.Record(image, true)
+		verified, _ := l.Status(image)
+		assert.True(t, verified)
+	})
+}
+
+// The results are shared by every policy in an admission request, so they must be
+// safe to write from more than one goroutine even though policies are evaluated
+// sequentially today.
+func TestImageVerificationResults_ConcurrentRecordAndStatus(t *testing.T) {
+	t.Parallel()
+	const image = "ghcr.io/kyverno/test-verify-image:signed"
+	l := NewImageVerificationResults()
+
+	var wg sync.WaitGroup
+	for i := range 50 {
+		wg.Add(2)
+		go func() {
+			defer wg.Done()
+			l.Record(image, i%2 == 0)
+		}()
+		go func() {
+			defer wg.Done()
+			l.Status(image)
+		}()
+	}
+	wg.Wait()
+
+	verified, attempted := l.Status(image)
+	assert.True(t, verified)
+	assert.True(t, attempted)
+}
+
+// The results are optional at the API boundary, so every method has to tolerate a
+// nil receiver rather than panicking inside a CEL function.
+func TestImageVerificationResults_NilReceiverIsSafe(t *testing.T) {
+	t.Parallel()
+	var l *ImageVerificationResults
+	assert.NotPanics(t, func() {
+		l.Record("ghcr.io/kyverno/test-verify-image:signed", true)
+		verified, attempted := l.Status("ghcr.io/kyverno/test-verify-image:signed")
+		assert.False(t, verified)
+		assert.False(t, attempted)
+	})
+}

pkg/cel/policies/ivpol/engine/engine.go

@@ -9,6 +9,7 @@ import (
 	"github.com/kyverno/kyverno/pkg/admissionpolicy"
 	"github.com/kyverno/kyverno/pkg/cel/engine"
 	"github.com/kyverno/kyverno/pkg/cel/libs"
+	"github.com/kyverno/kyverno/pkg/cel/libs/imageverify"
 	"github.com/kyverno/kyverno/pkg/cel/matching"
 	"github.com/kyverno/kyverno/pkg/config"
 	engineapi "github.com/kyverno/kyverno/pkg/engine/api"
@@ -245,7 +246,9 @@ func (e *engineImpl) handleMutation(
 		if !matches {
 			continue
 		}
-		compiled, errList := c.Compile(ivpol.Policy, ivpol.Exceptions)
+		// digest mutation performs no verification, so it takes no part in the
+		// request-scoped verification results
+		compiled, errList := c.Compile(ivpol.Policy, ivpol.Exceptions, nil)
 		if errList != nil {
 			// compile errors are surfaced by the validating webhook, skip mutation
 			continue
@@ -369,14 +372,18 @@ func (e *engineImpl) evaluatePolicies(
 		return nil, err
 	}
 	c := eval.NewCompiler(ictx, e.lister, requestResource, e.ivCache)
+	// shared by every policy compiled below, so required sees cross-policy evidence
+	verifications := imageverify.NewImageVerificationResults()
+	// resolved after the loop: evidence may come from a policy evaluated later
+	var pendingRequired []pendingRequiredCheck
 	for _, ivpol := range policies {
 		response := eval.ImageVerifyPolicyResponse{
 			Policy:     ivpol.Policy,
 			Actions:    ivpol.Actions,
 			Exceptions: ivpol.Exceptions,
 		}
 		startTime := time.Now()
-		compiled, errList := c.Compile(ivpol.Policy, ivpol.Exceptions)
+		compiled, errList := c.Compile(ivpol.Policy, ivpol.Exceptions, verifications)
 		if errList != nil {
 			response.Result = *engineapi.RuleError("evaluation", engineapi.ImageVerify, "failed to compile policy", errList.ToAggregate(), nil)
 			response.Result = response.Result.WithStats(engineapi.NewExecutionStats(startTime, time.Now()))
@@ -414,12 +421,48 @@ func (e *engineImpl) evaluatePolicies(
 				response.Result = *engineapi.RuleError(ruleName, engineapi.ImageVerify, "error", result.Error, nil)
 			} else if result.Result {
 				response.Result = *engineapi.RulePass(ruleName, engineapi.ImageVerify, "success", result.AuditAnnotations)
+				pendingRequired = append(pendingRequired, pendingRequiredCheck{
+					name:             ivpol.Policy.GetName(),
+					compiled:         compiled,
+					images:           result.MatchedImages,
+					auditAnnotations: result.AuditAnnotations,
+					startTime:        startTime,
+				})
 			} else {
 				response.Result = *engineapi.RuleFail(ruleName, engineapi.ImageVerify, result.Message, result.AuditAnnotations)
 			}
 		}
 		response.Result = response.Result.WithStats(engineapi.NewExecutionStats(startTime, time.Now()))
 		responses[ivpol.Policy.GetName()] = response
 	}
+	enforceRequired(pendingRequired, responses)
 	return responses, nil
 }
+
+// pendingRequiredCheck defers a passing policy's required check until every
+// policy in the request has contributed its verifications.
+type pendingRequiredCheck struct {
+	name             string
+	compiled         eval.CompiledPolicy
+	images           []string
+	auditAnnotations map[string]string
+	startTime        time.Time
+}
+
+// enforceRequired turns a policy that passed its validations into a failure when
+// one of the images it matched was never verified, by any policy in the request.
+func enforceRequired(checks []pendingRequiredCheck, responses map[string]eval.ImageVerifyPolicyResponse) {
+	for _, check := range checks {
+		err := check.compiled.EnforceRequired(check.images)
+		if err == nil {
+			continue
+		}
+		response, ok := responses[check.name]
+		if !ok {
+			continue
+		}
+		result := *engineapi.RuleFail(check.name, engineapi.ImageVerify, err.Error(), check.auditAnnotations)
+		response.Result = result.WithStats(engineapi.NewExecutionStats(check.startTime, time.Now()))
+		responses[check.name] = response
+	}
+}