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..9df546f97 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. @@ -18,15 +18,16 @@ package crafter import ( "os" "path/filepath" + "slices" "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 +215,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 := slices.Concat(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)) }