From 38f58df943529bd86b427d567418be33700a533c Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 21 Aug 2026 14:28:48 +0200 Subject: [PATCH 1/2] fix(verifier): fail closed when a required attestation is absent isVerificationResultMatchingServerProvenance gated the attestation comparison behind a four-way AND: if p.Attestation != nil && r.Statement != nil && p.Attestation.Predicate != nil && r.Statement.Predicate != nil { When the provenance declared an attestation but the artifact carried none (r.Statement == nil, or a statement with no predicate), the guard was false, control fell through to `return true`, and the artifact verified. The constraint was skipped in exactly the case it exists to catch. The "attestations not set, skip the check" comment is only sound for the expected side; applying it to the actual side was the defect. The same expression hid a second bug in the opposite direction. When both sides were populated the comparison could never succeed: p.Attestation.Predicate is decoded registry data (map[string]any for a JSON object) while r.Statement.Predicate is a *structpb.Struct, so reflect.DeepEqual compared two different dynamic types and was unconditionally false. Any entry that set `predicate` was unverifiable. Extract the logic into compareAttestation and follow the per-field convention already used by compareBaseProperties, where an unset expectation means "do not constrain that dimension" rather than "skip the check entirely": - p.Attestation == nil -> unconstrained - p.Attestation != nil -> require r.Statement != nil - PredicateType != "" -> must match - Predicate != nil -> must DeepEqual Predicate.AsMap() - &VerifiedAttestation{} -> must carry some attestation, without constraining which Comparing against AsMap() normalises the two sides so a declared predicate is actually enforceable. Note AsMap renders every number as float64, matching encoding/json but not YAML's integer decoding; both directions are pinned by tests. VerifyServer's signature is unchanged and compareBaseProperties keeps its existing semantics. The function had no direct test. Add table-driven coverage for the guard clauses and for each attestation branch. Reverting verifier.go to the previous logic fails seven of the new cases, covering both the fail-open and the never-matched directions. Package coverage is 79.1%, with isVerificationResultMatchingServerProvenance and compareAttestation at 100%. Behaviour change: a server entry that declares `provenance.attestation` and passes today against an unattested artifact will now fail. No entry in toolhive-catalog or toolhive sets the key, so nothing regresses on upgrade. go.mod: github.com/in-toto/attestation and google.golang.org/protobuf move from indirect to direct at the same versions. VerificationResult exposes *in_toto.Statement, so testing this function requires importing it; go.sum is unchanged. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Samuele Verzi --- container/verifier/verifier.go | 39 ++++- container/verifier/verifier_test.go | 227 ++++++++++++++++++++++++++++ go.mod | 4 +- 3 files changed, 264 insertions(+), 6 deletions(-) diff --git a/container/verifier/verifier.go b/container/verifier/verifier.go index b057060..6767b3f 100644 --- a/container/verifier/verifier.go +++ b/container/verifier/verifier.go @@ -166,12 +166,43 @@ func isVerificationResultMatchingServerProvenance(r *verify.VerificationResult, return false } - // If the attestations are not set, we can skip this check - if p.Attestation != nil && r.Statement != nil && p.Attestation.Predicate != nil && r.Statement.Predicate != nil { - if p.Attestation.PredicateType != r.Statement.PredicateType { + return compareAttestation(r, p) +} + +// compareAttestation compares the attestation constraint declared in the +// server provenance against the in-toto statement carried by the verification +// result. Following the same per-field convention as compareBaseProperties, an +// unset expectation means "do not constrain that dimension" rather than "skip +// the check". +func compareAttestation(r *verify.VerificationResult, p *registry.Provenance) bool { + // A nil attestation places no constraint on the artifact. + if p.Attestation == nil { + return true + } + + // The provenance asks for an attestation, so the artifact has to carry + // one. Treating a missing statement as a match would skip the constraint + // exactly when it matters. + if r.Statement == nil { + return false + } + + if p.Attestation.PredicateType != "" && p.Attestation.PredicateType != r.Statement.PredicateType { + return false + } + + if p.Attestation.Predicate != nil { + if r.Statement.Predicate == nil { + return false + } + // The expected predicate is decoded registry data (map[string]any for + // a JSON object) while the statement carries a *structpb.Struct, so + // the two are only comparable once the struct is normalised to its map + // form. Note that AsMap yields float64 for every number, matching + // encoding/json but not YAML's integer decoding. + if !reflect.DeepEqual(p.Attestation.Predicate, r.Statement.Predicate.AsMap()) { return false } - return reflect.DeepEqual(p.Attestation.Predicate, r.Statement.Predicate) } return true diff --git a/container/verifier/verifier_test.go b/container/verifier/verifier_test.go index 464fa52..2bb4d66 100644 --- a/container/verifier/verifier_test.go +++ b/container/verifier/verifier_test.go @@ -7,8 +7,12 @@ import ( "testing" "github.com/google/go-containerregistry/pkg/authn" + in_toto "github.com/in-toto/attestation/go/v1" + "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" + "github.com/sigstore/sigstore-go/pkg/verify" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/structpb" registry "github.com/stacklok/toolhive-core/registry/types" ) @@ -64,3 +68,226 @@ func TestVerifyServer_NilProvenance_InvalidRef(t *testing.T) { err := s.VerifyServer("", nil) require.Error(t, err) } + +// ------ isVerificationResultMatchingServerProvenance ------ + +// testSAN is a Fulcio-style subject alternative name. compareBaseProperties +// runs signerIdentityFromCertificate unconditionally, which errors on an empty +// SAN, so every fixture certificate carries one. +const testSAN = "https://github.com/stacklok/toolhive/.github/workflows/build.yml@refs/heads/main" + +const ( + slsaPredicateType = "https://slsa.dev/provenance/v1" + buildTypeKey = "buildType" + buildTypeGHA = "gha" + attemptKey = "attempt" +) + +// newTestResult builds a minimal verification result whose certificate is +// populated enough for compareBaseProperties to run cleanly. +func newTestResult(statement *in_toto.Statement) *verify.VerificationResult { + return &verify.VerificationResult{ + Signature: &verify.SignatureVerificationResult{ + Certificate: &certificate.Summary{ + SubjectAlternativeName: testSAN, + }, + }, + Statement: statement, + } +} + +// newTestStatement builds an in-toto statement. A nil predicate leaves +// Statement.Predicate nil, modelling a statement that carries no predicate. +func newTestStatement(t *testing.T, predicateType string, predicate map[string]any) *in_toto.Statement { + t.Helper() + stmt := &in_toto.Statement{PredicateType: predicateType} + if predicate != nil { + s, err := structpb.NewStruct(predicate) + require.NoError(t, err) + stmt.Predicate = s + } + return stmt +} + +func TestIsVerificationResultMatchingServerProvenance_Attestation(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + statement *in_toto.Statement + expected *registry.VerifiedAttestation + want bool + }{ + { + name: "nil expected attestation is unconstrained", + statement: newTestStatement(t, slsaPredicateType, map[string]any{buildTypeKey: buildTypeGHA}), + expected: nil, + want: true, + }, + { + name: "nil expected attestation with no statement is unconstrained", + statement: nil, + expected: nil, + want: true, + }, + { + // Regression: this used to fall through to a match, skipping the + // constraint exactly when the artifact carried no attestation. + name: "expected attestation but statement absent", + statement: nil, + expected: ®istry.VerifiedAttestation{PredicateType: slsaPredicateType}, + want: false, + }, + { + name: "empty expected attestation but statement absent", + statement: nil, + expected: ®istry.VerifiedAttestation{}, + want: false, + }, + { + // An empty expectation means "must carry some attestation", + // without constraining which one. + name: "empty expected attestation with statement present", + statement: newTestStatement(t, slsaPredicateType, map[string]any{buildTypeKey: buildTypeGHA}), + expected: ®istry.VerifiedAttestation{}, + want: true, + }, + { + name: "predicate type matches, predicate unconstrained", + statement: newTestStatement(t, slsaPredicateType, map[string]any{buildTypeKey: buildTypeGHA}), + expected: ®istry.VerifiedAttestation{PredicateType: slsaPredicateType}, + want: true, + }, + { + name: "predicate type mismatch", + statement: newTestStatement(t, "https://slsa.dev/provenance/v0.2", map[string]any{buildTypeKey: buildTypeGHA}), + expected: ®istry.VerifiedAttestation{PredicateType: slsaPredicateType}, + want: false, + }, + { + name: "expected predicate but statement carries none", + statement: newTestStatement(t, slsaPredicateType, nil), + expected: ®istry.VerifiedAttestation{ + PredicateType: slsaPredicateType, + Predicate: map[string]any{buildTypeKey: buildTypeGHA}, + }, + want: false, + }, + { + name: "predicate mismatch", + statement: newTestStatement(t, slsaPredicateType, map[string]any{buildTypeKey: buildTypeGHA}), + expected: ®istry.VerifiedAttestation{ + PredicateType: slsaPredicateType, + Predicate: map[string]any{buildTypeKey: "local"}, + }, + want: false, + }, + { + name: "predicate type and predicate both match", + statement: newTestStatement(t, slsaPredicateType, map[string]any{ + buildTypeKey: buildTypeGHA, + "builder": map[string]any{"id": "https://github.com/actions/runner"}, + }), + expected: ®istry.VerifiedAttestation{ + PredicateType: slsaPredicateType, + Predicate: map[string]any{ + buildTypeKey: buildTypeGHA, + "builder": map[string]any{"id": "https://github.com/actions/runner"}, + }, + }, + want: true, + }, + { + name: "predicate matches with predicate type unconstrained", + statement: newTestStatement(t, slsaPredicateType, map[string]any{buildTypeKey: buildTypeGHA}), + expected: ®istry.VerifiedAttestation{Predicate: map[string]any{buildTypeKey: buildTypeGHA}}, + want: true, + }, + { + // Documents the AsMap normalisation caveat: every number in a + // statement predicate comes back as float64, matching how + // encoding/json decodes the expected side but not YAML. + name: "numeric predicate matches as float64", + statement: newTestStatement(t, slsaPredicateType, map[string]any{attemptKey: 1}), + expected: ®istry.VerifiedAttestation{Predicate: map[string]any{attemptKey: float64(1)}}, + want: true, + }, + { + name: "numeric predicate does not match as int", + statement: newTestStatement(t, slsaPredicateType, map[string]any{attemptKey: 1}), + expected: ®istry.VerifiedAttestation{Predicate: map[string]any{attemptKey: 1}}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + r := newTestResult(tt.statement) + p := ®istry.Provenance{Attestation: tt.expected} + assert.Equal(t, tt.want, isVerificationResultMatchingServerProvenance(r, p)) + }) + } +} + +func TestIsVerificationResultMatchingServerProvenance_Guards(t *testing.T) { + t.Parallel() + + statement := newTestStatement(t, slsaPredicateType, map[string]any{buildTypeKey: buildTypeGHA}) + + tests := []struct { + name string + result *verify.VerificationResult + prov *registry.Provenance + want bool + }{ + { + name: "nil result", + result: nil, + prov: ®istry.Provenance{}, + want: false, + }, + { + name: "nil provenance", + result: newTestResult(statement), + prov: nil, + want: false, + }, + { + name: "nil signature", + result: &verify.VerificationResult{Statement: statement}, + prov: ®istry.Provenance{}, + want: false, + }, + { + name: "nil certificate", + result: &verify.VerificationResult{ + Signature: &verify.SignatureVerificationResult{}, + Statement: statement, + }, + prov: ®istry.Provenance{}, + want: false, + }, + { + // Base properties are checked before the attestation, so a + // mismatch there short-circuits regardless of the attestation. + name: "base property mismatch short-circuits", + result: newTestResult(statement), + prov: ®istry.Provenance{RepositoryURI: "https://github.com/other/repo"}, + want: false, + }, + { + name: "no constraints at all", + result: newTestResult(statement), + prov: ®istry.Provenance{}, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, isVerificationResultMatchingServerProvenance(tt.result, tt.prov)) + }) + } +} diff --git a/go.mod b/go.mod index 3230ff2..db9a316 100644 --- a/go.mod +++ b/go.mod @@ -36,6 +36,7 @@ require ( github.com/go-openapi/swag/conv v0.29.0 github.com/golang-jwt/jwt/v5 v5.3.1 github.com/google/certificate-transparency-go v1.3.3 + github.com/in-toto/attestation v1.2.0 github.com/lestrrat-go/httprc/v3 v3.0.6 github.com/lestrrat-go/jwx/v3 v3.2.0 github.com/prometheus/client_golang v1.24.1 @@ -52,6 +53,7 @@ require ( go.opentelemetry.io/otel/sdk/log v0.21.0 go.opentelemetry.io/otel/trace v1.45.0 golang.org/x/oauth2 v0.36.0 + google.golang.org/protobuf v1.36.11 ) require ( @@ -112,7 +114,6 @@ require ( github.com/grpc-ecosystem/grpc-gateway/v2 v2.29.0 // indirect github.com/hashicorp/go-cleanhttp v0.5.2 // indirect github.com/hashicorp/go-retryablehttp v0.7.8 // indirect - github.com/in-toto/attestation v1.2.0 // indirect github.com/in-toto/in-toto-golang v0.11.0 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/jackc/pgpassfile v1.0.0 // indirect @@ -168,6 +169,5 @@ require ( google.golang.org/genproto/googleapis/api v0.0.0-20260803160001-6ac0973c030d // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260803160001-6ac0973c030d // indirect google.golang.org/grpc v1.83.0 // indirect - google.golang.org/protobuf v1.36.11 // indirect k8s.io/klog/v2 v2.140.0 // indirect ) From 9edd0bff1f590a836fb06f18e47e9fafb4732f85 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 21 Aug 2026 14:36:28 +0200 Subject: [PATCH 2/2] fix(verifier): normalize both predicate sides before comparing The previous commit normalized only the actual side, comparing the expected predicate against r.Statement.Predicate.AsMap(). That left equality dependent on how the registry entry was decoded: AsMap renders every number as float64, encoding/json decodes numbers to float64, but yaml.v3 decodes them to int. Two semantically identical predicates would therefore verify when loaded from JSON and fail when loaded from YAML. registry.Provenance carries both json and yaml tags and registry data is YAML-decodable, so this was a reachable correctness bug rather than a theoretical one. Documenting it as a known boundary was not sufficient. Round the expected predicate through structpb.NewStruct and compare the AsMap output of both sides, so the comparison is on normalized values rather than on whichever types the decoder happened to produce. Verified that yaml.v3 and encoding/json disagree on the raw values and agree once normalized, including for nested objects and lists. Two failure modes fail closed rather than passing: - an expected predicate that is not an object cannot match an in-toto predicate, which is always an object - an expected predicate structpb cannot represent is one we cannot confirm, so it is a mismatch Tests: the integer case flips from "does not match" to "matches". Add TestIsVerificationResultMatchingServerProvenance_PredicateDecoderIndependence, which decodes the same provenance from JSON and from YAML and asserts both verify against one statement. It guards its own premise by first asserting the two decoders really do produce different Go values, so it cannot silently become vacuous. Add cases for the non-object and unrepresentable predicates. Reverting verifier.go to the previous commit fails the decoder independence test and the integer case. Package coverage is 79.6%, with compareAttestation and normalizeAttestationPredicate at 100%. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Samuele Verzi --- container/verifier/verifier.go | 38 ++++++++++-- container/verifier/verifier_test.go | 94 +++++++++++++++++++++++++++-- 2 files changed, 120 insertions(+), 12 deletions(-) diff --git a/container/verifier/verifier.go b/container/verifier/verifier.go index 6767b3f..8f85049 100644 --- a/container/verifier/verifier.go +++ b/container/verifier/verifier.go @@ -15,6 +15,7 @@ import ( "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" "github.com/sigstore/sigstore-go/pkg/root" "github.com/sigstore/sigstore-go/pkg/verify" + "google.golang.org/protobuf/types/known/structpb" registry "github.com/stacklok/toolhive-core/registry/types" ) @@ -195,12 +196,20 @@ func compareAttestation(r *verify.VerificationResult, p *registry.Provenance) bo if r.Statement.Predicate == nil { return false } - // The expected predicate is decoded registry data (map[string]any for - // a JSON object) while the statement carries a *structpb.Struct, so - // the two are only comparable once the struct is normalised to its map - // form. Note that AsMap yields float64 for every number, matching - // encoding/json but not YAML's integer decoding. - if !reflect.DeepEqual(p.Attestation.Predicate, r.Statement.Predicate.AsMap()) { + // Round both sides through structpb before comparing. The expected + // predicate is decoded registry data whose Go types depend on the + // serialisation format - encoding/json yields float64 for every + // number where yaml.v3 yields int - while the statement carries a + // *structpb.Struct. Comparing the raw values would make verification + // depend on how the registry entry happened to be decoded. + expected, err := normalizeAttestationPredicate(p.Attestation.Predicate) + if err != nil { + // An expected predicate we cannot normalise is one we cannot + // confirm, so treat it as a mismatch rather than a pass. + slog.Error("cannot normalize expected attestation predicate", "error", err) + return false + } + if !reflect.DeepEqual(expected, r.Statement.Predicate.AsMap()) { return false } } @@ -208,6 +217,23 @@ func compareAttestation(r *verify.VerificationResult, p *registry.Provenance) bo return true } +// normalizeAttestationPredicate renders an expected predicate from the registry +// into the same shape structpb produces for a statement predicate, so that +// semantically equal predicates compare equal regardless of whether they were +// decoded from JSON or YAML. An in-toto predicate is always an object, so an +// expectation that is not a map cannot match one. +func normalizeAttestationPredicate(predicate any) (map[string]any, error) { + fields, ok := predicate.(map[string]any) + if !ok { + return nil, fmt.Errorf("expected predicate is %T, want an object", predicate) + } + s, err := structpb.NewStruct(fields) + if err != nil { + return nil, fmt.Errorf("expected predicate is not representable as a struct: %w", err) + } + return s.AsMap(), nil +} + // compareBaseProperties compares the base properties of the verification result and the server provenance func compareBaseProperties(r *verify.VerificationResult, p *registry.Provenance) bool { // Extract the signer identity from the certificate diff --git a/container/verifier/verifier_test.go b/container/verifier/verifier_test.go index 2bb4d66..1bf61b8 100644 --- a/container/verifier/verifier_test.go +++ b/container/verifier/verifier_test.go @@ -4,6 +4,7 @@ package verifier import ( + "encoding/json" "testing" "github.com/google/go-containerregistry/pkg/authn" @@ -13,6 +14,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/protobuf/types/known/structpb" + "gopkg.in/yaml.v3" registry "github.com/stacklok/toolhive-core/registry/types" ) @@ -81,6 +83,8 @@ const ( buildTypeKey = "buildType" buildTypeGHA = "gha" attemptKey = "attempt" + builderKey = "builder" + builderID = "https://github.com/actions/runner" ) // newTestResult builds a minimal verification result whose certificate is @@ -186,13 +190,13 @@ func TestIsVerificationResultMatchingServerProvenance_Attestation(t *testing.T) name: "predicate type and predicate both match", statement: newTestStatement(t, slsaPredicateType, map[string]any{ buildTypeKey: buildTypeGHA, - "builder": map[string]any{"id": "https://github.com/actions/runner"}, + builderKey: map[string]any{"id": builderID}, }), expected: ®istry.VerifiedAttestation{ PredicateType: slsaPredicateType, Predicate: map[string]any{ buildTypeKey: buildTypeGHA, - "builder": map[string]any{"id": "https://github.com/actions/runner"}, + builderKey: map[string]any{"id": builderID}, }, }, want: true, @@ -204,18 +208,40 @@ func TestIsVerificationResultMatchingServerProvenance_Attestation(t *testing.T) want: true, }, { - // Documents the AsMap normalisation caveat: every number in a - // statement predicate comes back as float64, matching how - // encoding/json decodes the expected side but not YAML. + // Numbers are normalised on both sides, so an expectation compares + // equal whether the decoder produced float64 (encoding/json) or + // int (yaml.v3). name: "numeric predicate matches as float64", statement: newTestStatement(t, slsaPredicateType, map[string]any{attemptKey: 1}), expected: ®istry.VerifiedAttestation{Predicate: map[string]any{attemptKey: float64(1)}}, want: true, }, { - name: "numeric predicate does not match as int", + name: "numeric predicate matches as int", statement: newTestStatement(t, slsaPredicateType, map[string]any{attemptKey: 1}), expected: ®istry.VerifiedAttestation{Predicate: map[string]any{attemptKey: 1}}, + want: true, + }, + { + name: "numeric predicate still mismatches on value", + statement: newTestStatement(t, slsaPredicateType, map[string]any{attemptKey: 1}), + expected: ®istry.VerifiedAttestation{Predicate: map[string]any{attemptKey: 2}}, + want: false, + }, + { + // An in-toto predicate is always an object, so a scalar + // expectation cannot match and must not pass. + name: "non-object expected predicate does not match", + statement: newTestStatement(t, slsaPredicateType, map[string]any{buildTypeKey: buildTypeGHA}), + expected: ®istry.VerifiedAttestation{Predicate: buildTypeGHA}, + want: false, + }, + { + // A predicate structpb cannot represent is one we cannot confirm, + // so it fails closed rather than passing. + name: "unrepresentable expected predicate does not match", + statement: newTestStatement(t, slsaPredicateType, map[string]any{buildTypeKey: buildTypeGHA}), + expected: ®istry.VerifiedAttestation{Predicate: map[string]any{buildTypeKey: make(chan int)}}, want: false, }, } @@ -230,6 +256,62 @@ func TestIsVerificationResultMatchingServerProvenance_Attestation(t *testing.T) } } +// Predicate equality must not depend on how the registry entry was decoded. +// The same provenance expressed as JSON and as YAML has to verify against the +// same statement, even though encoding/json decodes numbers to float64 while +// yaml.v3 decodes them to int. +func TestIsVerificationResultMatchingServerProvenance_PredicateDecoderIndependence(t *testing.T) { + t.Parallel() + + const jsonProvenance = `{ + "attestation": { + "predicate_type": "https://slsa.dev/provenance/v1", + "predicate": { + "buildType": "gha", + "attempt": 1, + "builder": {"id": "https://github.com/actions/runner", "version": 2}, + "steps": [1, 2] + } + } + }` + + const yamlProvenance = ` +attestation: + predicate_type: https://slsa.dev/provenance/v1 + predicate: + buildType: gha + attempt: 1 + builder: + id: https://github.com/actions/runner + version: 2 + steps: + - 1 + - 2 +` + + statement := newTestStatement(t, slsaPredicateType, map[string]any{ + buildTypeKey: buildTypeGHA, + attemptKey: 1, + builderKey: map[string]any{"id": builderID, "version": 2}, + "steps": []any{1, 2}, + }) + result := newTestResult(statement) + + var fromJSON registry.Provenance + require.NoError(t, json.Unmarshal([]byte(jsonProvenance), &fromJSON)) + + var fromYAML registry.Provenance + require.NoError(t, yaml.Unmarshal([]byte(yamlProvenance), &fromYAML)) + + // Guard the premise: the two decoders really do produce different Go + // types, so this test would be vacuous if they ever converged. + require.NotEqual(t, fromJSON.Attestation.Predicate, fromYAML.Attestation.Predicate, + "decoders expected to disagree on numeric types; test is vacuous otherwise") + + assert.True(t, isVerificationResultMatchingServerProvenance(result, &fromJSON), "JSON-decoded provenance should match") + assert.True(t, isVerificationResultMatchingServerProvenance(result, &fromYAML), "YAML-decoded provenance should match") +} + func TestIsVerificationResultMatchingServerProvenance_Guards(t *testing.T) { t.Parallel()