From 4e781465af32306ddd440de748c241c1c2976175 Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Thu, 9 Apr 2026 15:00:03 +0200 Subject: [PATCH 1/2] fix(attestation): deduplicate policy evaluations after protojson round-trip Replace reflect.DeepEqual with maps.Equal for policy evaluation dedup so nil and empty maps are treated as equivalent. protojson.Marshal omits empty maps, causing deserialized With fields to be nil instead of empty, which made the dedup comparison fail and duplicate evaluations appear. Signed-off-by: Jose I. Paris --- pkg/attestation/crafter/crafter.go | 13 ++- pkg/attestation/crafter/crafter_unit_test.go | 106 ++++++++++++++++++- 2 files changed, 113 insertions(+), 6 deletions(-) diff --git a/pkg/attestation/crafter/crafter.go b/pkg/attestation/crafter/crafter.go index f7c215aa0..001eb2087 100644 --- a/pkg/attestation/crafter/crafter.go +++ b/pkg/attestation/crafter/crafter.go @@ -19,9 +19,9 @@ import ( "context" "errors" "fmt" + "maps" "net/url" "os" - "reflect" "slices" "strings" "time" @@ -752,6 +752,13 @@ func (c *Crafter) addMaterial(ctx context.Context, m *schemaapi.CraftingSchema_M return mt, nil } +// policyEvalMatches returns true if two policy evaluations refer to the same policy +// with the same arguments. It treats nil and empty maps as equivalent to handle +// protojson round-trip serialization where empty maps are omitted. +func policyEvalMatches(a, b *api.PolicyEvaluation) bool { + return proto.Equal(a.GetPolicyReference(), b.GetPolicyReference()) && maps.Equal(a.GetWith(), b.GetWith()) +} + // EvaluateAttestationPolicies evaluates the attestation-level policies and stores them in the attestation state. // The phase parameter controls which policies are evaluated based on their attestation_phases spec field. func (c *Crafter) EvaluateAttestationPolicies(ctx context.Context, attestationID string, statement *intoto.Statement, phase policies.EvalPhase) error { @@ -783,7 +790,7 @@ func (c *Crafter) EvaluateAttestationPolicies(ctx context.Context, attestationID for _, ev := range policyEvaluations { var duplicated bool for _, existing := range filteredPolicyEvaluations { - if proto.Equal(existing.PolicyReference, ev.PolicyReference) && reflect.DeepEqual(existing.With, ev.With) { + if policyEvalMatches(existing, ev) { duplicated = true break } @@ -808,7 +815,7 @@ func (c *Crafter) EvaluateAttestationPolicies(ctx context.Context, attestationID // Check if this attestation-level evaluation was re-evaluated in the current phase var reEvaluated bool for _, newEv := range policyEvaluations { - if proto.Equal(newEv.PolicyReference, ev.PolicyReference) && reflect.DeepEqual(newEv.With, ev.With) { + if policyEvalMatches(newEv, ev) { reEvaluated = true break } diff --git a/pkg/attestation/crafter/crafter_unit_test.go b/pkg/attestation/crafter/crafter_unit_test.go index 1980952f8..727da2fb7 100644 --- a/pkg/attestation/crafter/crafter_unit_test.go +++ b/pkg/attestation/crafter/crafter_unit_test.go @@ -1,5 +1,5 @@ // -// Copyright 2023 The Chainloop Authors. +// Copyright 2023-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -21,12 +21,12 @@ import ( "testing" "time" + api "github.com/chainloop-dev/chainloop/pkg/attestation/crafter/api/attestation/v1" "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/config" "github.com/go-git/go-git/v5/plumbing/object" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - - "github.com/go-git/go-git/v5/config" "github.com/stretchr/testify/suite" ) @@ -214,6 +214,106 @@ func (s *crafterUnitSuite) TestGitRepoHead() { } } +func (s *crafterUnitSuite) TestPolicyEvaluationDedup() { + // Simulate the protojson round-trip issue: + // - Init phase sets With = map[string]string{} (empty map) + // - protojson.Marshal omits empty maps + // - protojson.Unmarshal sets With = nil (absent field) + // - Push phase produces With = map[string]string{} again + // The dedup comparison must treat nil and empty map as equal. + + policyRef := &api.PolicyEvaluation_Reference{ + Name: "source-commit", + Digest: "sha256:abc123", + } + + testCases := []struct { + name string + existing []*api.PolicyEvaluation + newEvals []*api.PolicyEvaluation + wantCount int + description string + }{ + { + name: "nil vs empty map With are deduplicated", + existing: []*api.PolicyEvaluation{ + {Name: "source-commit", PolicyReference: policyRef, With: nil}, + }, + newEvals: []*api.PolicyEvaluation{ + {Name: "source-commit", PolicyReference: policyRef, With: map[string]string{}}, + }, + wantCount: 1, + description: "after protojson round-trip, nil With should match empty map With", + }, + { + name: "empty map vs empty map With are deduplicated", + existing: []*api.PolicyEvaluation{ + {Name: "source-commit", PolicyReference: policyRef, With: map[string]string{}}, + }, + newEvals: []*api.PolicyEvaluation{ + {Name: "source-commit", PolicyReference: policyRef, With: map[string]string{}}, + }, + wantCount: 1, + description: "identical empty maps should deduplicate", + }, + { + name: "nil vs nil With are deduplicated", + existing: []*api.PolicyEvaluation{ + {Name: "source-commit", PolicyReference: policyRef, With: nil}, + }, + newEvals: []*api.PolicyEvaluation{ + {Name: "source-commit", PolicyReference: policyRef, With: nil}, + }, + wantCount: 1, + description: "both nil should deduplicate", + }, + { + name: "different With args are not deduplicated", + existing: []*api.PolicyEvaluation{ + {Name: "source-commit", PolicyReference: policyRef, With: map[string]string{"key": "val1"}}, + }, + newEvals: []*api.PolicyEvaluation{ + {Name: "source-commit", PolicyReference: policyRef, With: map[string]string{"key": "val2"}}, + }, + wantCount: 2, + description: "different With values should not deduplicate", + }, + { + name: "different policy references are not deduplicated", + existing: []*api.PolicyEvaluation{ + {Name: "policy-a", PolicyReference: &api.PolicyEvaluation_Reference{Name: "policy-a"}, With: nil}, + }, + newEvals: []*api.PolicyEvaluation{ + {Name: "policy-b", PolicyReference: &api.PolicyEvaluation_Reference{Name: "policy-b"}, With: nil}, + }, + wantCount: 2, + description: "different policies should both be kept", + }, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + all := append(tc.existing, tc.newEvals...) + + var filtered []*api.PolicyEvaluation + for _, ev := range all { + var duplicated bool + for _, existing := range filtered { + if policyEvalMatches(existing, ev) { + duplicated = true + break + } + } + if !duplicated { + filtered = append(filtered, ev) + } + } + + s.Len(filtered, tc.wantCount, tc.description) + }) + } +} + func TestSuite(t *testing.T) { suite.Run(t, new(crafterUnitSuite)) } From 6356caa5162c0d4d283af08300366796ef1d10bd Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Thu, 9 Apr 2026 15:17:03 +0200 Subject: [PATCH 2/2] fix(attestation): use slices.Concat to satisfy appendAssign lint rule Signed-off-by: Jose I. Paris --- pkg/attestation/crafter/crafter_unit_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/attestation/crafter/crafter_unit_test.go b/pkg/attestation/crafter/crafter_unit_test.go index 727da2fb7..9df546f97 100644 --- a/pkg/attestation/crafter/crafter_unit_test.go +++ b/pkg/attestation/crafter/crafter_unit_test.go @@ -18,6 +18,7 @@ package crafter import ( "os" "path/filepath" + "slices" "testing" "time" @@ -293,7 +294,7 @@ func (s *crafterUnitSuite) TestPolicyEvaluationDedup() { for _, tc := range testCases { s.Run(tc.name, func() { - all := append(tc.existing, tc.newEvals...) + all := slices.Concat(tc.existing, tc.newEvals) var filtered []*api.PolicyEvaluation for _, ev := range all {