fix(EC-1912): validate in-toto statement type in bundle attestation path#3418
Conversation
ProvenanceFromBundlePayload accepted any _type value in in-toto statements, unlike the SLSA-specific parsers which reject unknown types. Add validation to accept only StatementInTotoV1 and StatementInTotoV01, consistent with the non-bundle code path. Ref: EC-1838
📝 WalkthroughWalkthrough
ChangesAttestation validation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🤖 Finished Review · ✅ Success · Started 11:12 AM UTC · Completed 11:16 AM UTC |
Review of PR #3418fix(EC-1912): validate in-toto statement type in bundle attestation path SummaryClean, well-scoped defense-in-depth fix. FindingsNo blocking findings. Low:
|
| Dimension | Verdict | Notes |
|---|---|---|
| Correctness | ✅ Pass | Validation logic mirrors slsa_provenance_v1.go:56-58 exactly. Both accepted types (StatementInTotoV1, StatementInTotoV01) are correct. Error format matches codebase convention. Two test cases cover unsupported and empty types. |
| Security | ✅ Pass | Defense-in-depth: rejects unknown statement types earlier in the pipeline, before signature entity creation. No new attack surface. |
| Intent & coherence | ✅ Pass | Change matches PR description and Jira ticket (EC-1912). Appropriately scoped to the bundle attestation path. |
| Style & conventions | ✅ Pass | Follows established patterns for type validation and error messages. Test structure consistent with existing table-driven tests. |
| Documentation | ✅ Pass | No documentation changes needed for internal validation logic. |
| Cross-repo contracts | ✅ Pass | No API surface changes. Only stricter validation of invalid inputs that were already caught by policy rules at evaluation time. |
Previous run
Review of PR #3418
fix(EC-1912): validate in-toto statement type in bundle attestation path
Summary
This PR closes a validation gap in ProvenanceFromBundlePayload where any _type value in in-toto statements was accepted without checking. The SLSA-specific parsers (SLSAProvenanceFromSignature at slsa_provenance_02.go:55 and SLSAProvenanceFromSignatureV1 at slsa_provenance_v1.go:56-58) already perform this validation, but the generic bundle path did not.
Analysis
Correctness — The validation check exactly mirrors the pattern in slsa_provenance_v1.go:56-58, accepting both StatementInTotoV1 (https://in-toto.io/Statement/v1) and StatementInTotoV01 (https://in-toto.io/Statement/v0.1). The check is placed correctly — after the statement JSON is unmarshaled and before entity signatures are created. Both callers handle the new error appropriately:
application_snapshot_image.go:334-337propagates the error (consistent with its existing hard-error handling of all parse failures)sigstore.go:442-450appends toerrorTermsand continues processing remaining attestations
Test coverage is adequate: two new table-driven test cases cover an unsupported type string and an empty/missing _type field.
Security — This is a security improvement. It adds defense-in-depth validation to close the inconsistency between bundle and non-bundle attestation paths. The attestation_type policy rule provides a second layer at evaluation time, but rejecting unknown types at parse time is the safer default.
Intent & coherence — The change is well-scoped. The PR description correctly explains why only ProvenanceFromBundlePayload is changed and why ProvenanceFromSignature is intentionally left unchanged (it feeds into downstream SLSA-specific parsers that handle their own type validation; adding validation there would break non-SLSA attestation types).
Style/conventions — Code follows existing patterns exactly: same condition structure, same error message format ("unsupported attestation type: %s"), same table-driven test style. The whitespace addition at line 128 is a trivial cleanup.
Documentation — No documentation updates needed for this internal defense-in-depth change.
Cross-repo contracts — The Attestation interface is unchanged. The stricter validation is in an internal/ package. Callers already handle errors from this function.
Verdict
Approve — Clean, well-tested, defense-in-depth fix that closes a validation gap between the bundle and non-bundle attestation code paths. No blocking findings.
Labels: Defense-in-depth bug fix for attestation type validation in bundle path
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
🤖 Finished Review · ❌ Failure · Started 9:29 AM UTC · Completed 9:33 AM UTC |
|
🤖 Finished Retro · ✅ Success · Started 12:14 PM UTC · Completed 12:23 PM UTC |
|
PR #3418 was a clean, well-scoped defense-in-depth bug fix that added in-toto statement type validation to Proposals filed
|
2 participants
ProvenanceFromBundlePayloadaccepted any_typevalue in in-toto statements, unlike the SLSA-specific parsers which reject unknown types. This adds the same validation to the bundle path — acceptv0.1andv1, reject anything else.Defense-in-depth only — the
attestation_typepolicy rule already catches this at evaluation time.Jira: EC-1912 | Ref: EC-1838