feat(EC-1775): verify test attestation content#1728
Conversation
Add new policy package test_attestation that verifies the content of in-toto test-result attestations. Includes 4 rules (no_failed_tests, no_test_warnings, test_result_known, test_data_found) and 2 helpers (_test_name, _test_list) with 6 test cases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…d type safety Add test cases for mixed PASSED/FAILED, no-op behavior, test name extraction, WARNED+FAILED coexistence, multiple failures, and non-string result type safety. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove multi-line >- syntax from failure_msg fields to avoid YAML parsing issues. Use single-line format consistent with existing policy rules. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verify test_result_known rule fires specifically and message contains "unsupported result value" instead of just checking count > 0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verify each deny result message contains "has a failed result" to ensure the message template renders correctly for both attestations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a Rego package ChangesTest Attestation Policy Module
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Review Summary by QodoAdd test attestation content verification policy package
WalkthroughsDescription• Add new test_attestation policy package verifying in-toto test-result attestation content • Implement 4 deny/warn rules: no_failed_tests, no_test_warnings, test_result_known, test_data_found • Include 2 helper functions for test name extraction and safe array formatting • Provide 13 comprehensive test cases covering happy path, edge cases, and type safety Diagramflowchart LR
A["Verified Test Attestations"] -->|"_test_attestations"| B["Policy Rules"]
B -->|"no_failed_tests"| C["Deny on FAILED"]
B -->|"no_test_warnings"| D["Warn on WARNED"]
B -->|"test_result_known"| E["Deny on Invalid Result"]
B -->|"test_data_found"| F["Deny on Missing Result"]
C --> G["Result with Message"]
D --> G
E --> G
F --> G
File Changes1. policy/release/test_attestation/test_attestation.rego
|
Code Review by Qodo
1.
|
|
As a note, we should probably ensure that conforma/cli#3311 is merged before we merge this as it's predicated on some of the changes introduced in the CLI there. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
policy/release/test_attestation/test_attestation_test.rego (1)
463-480: ⚡ Quick winStrengthen
test_multiple_failuresto detect extra deny rows and unexpected warnings.Using set equality alone can still pass when duplicate/extra deny objects are emitted. Add explicit cardinality and
warn-empty checks.Proposed patch
test_multiple_failures if { results := test_attestation.deny with input.image.ref as _image_ref with ec.oci.image_referrers as _mock_referrers_two with ec.sigstore.verify_attestation as _mock_verify_two with ec.oci.blob as _mock_blob_multi_failed with ec.oci.image_manifests as _mock_manifests with data.trusted_task_rules as _trusted_task_rules.trusted_task_rules + count(results) == 2 + deny_codes := {r.code | some r in results} assertions.assert_equal(deny_codes, {"test_attestation.no_failed_tests"}) deny_terms := {r.term | some r in results} assertions.assert_equal(deny_terms, {"clair-scan", "sanity-check"}) every r in results { contains(r.msg, "has a failed result") } + + assertions.assert_empty(test_attestation.warn) with input.image.ref as _image_ref + with ec.oci.image_referrers as _mock_referrers_two + with ec.sigstore.verify_attestation as _mock_verify_two + with ec.oci.blob as _mock_blob_multi_failed + with ec.oci.image_manifests as _mock_manifests + with data.trusted_task_rules as _trusted_task_rules.trusted_task_rules }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@policy/release/test_attestation/test_attestation_test.rego` around lines 463 - 480, The test_multiple_failures should also assert there are exactly the expected number of deny rows and that no warnings are emitted: after computing results, add an assertion for the cardinality (e.g., assertions.assert_equal(count(results), 2) or the exact expected count) and assert that no result contains a warn/warning field (e.g., assertions.assert_equal({w | some r in results; w := r.warn}, {}) or add every r in results { not r.warn }) so duplicate/extra deny objects or unexpected warnings will fail the test; update the test_multiple_failures block with these checks alongside deny_codes and deny_terms.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@policy/release/test_attestation/test_attestation_test.rego`:
- Around line 463-480: The test_multiple_failures should also assert there are
exactly the expected number of deny rows and that no warnings are emitted: after
computing results, add an assertion for the cardinality (e.g.,
assertions.assert_equal(count(results), 2) or the exact expected count) and
assert that no result contains a warn/warning field (e.g.,
assertions.assert_equal({w | some r in results; w := r.warn}, {}) or add every r
in results { not r.warn }) so duplicate/extra deny objects or unexpected
warnings will fail the test; update the test_multiple_failures block with these
checks alongside deny_codes and deny_terms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: fba5387c-023f-4737-85e6-9d93d4b8f1e6
📒 Files selected for processing (2)
policy/release/test_attestation/test_attestation.regopolicy/release/test_attestation/test_attestation_test.rego
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
Reorder rules so all deny rules are adjacent, followed by the warn rule. Fixes Regal messy-rule violation where the warn rule broke the deny rule grouping. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![qodo-for-conforma[bot]](https://avatars.githubusercontent.com/in/3609154?s=60&v=4){kind=small}
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add count(results) == 2 cardinality check and assert warn set is empty to catch duplicate/extra deny objects and unexpected warnings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use object.get(statement, "predicate", {}) before accessing
configuration field to prevent type errors when a verified statement
has predicateType set but no predicate object. Adds test case 14 for
this edge case per Qodo review.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add is_string guard in _test_list to prevent type errors when failedTests/warnedTests arrays contain non-string elements. Adds test case 15 per Qodo review. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Split test_multiple_failures into two tests to avoid Regal non-loop-expression violation from standalone with-clause expression. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
st3penta
left a comment
st3penta
left a comment
There was a problem hiding this comment.
Very good overall, but i have a few questions and test gaps
| import data.lib.intoto | ||
| import data.lib.metadata | ||
|
|
||
| _test_attestations := intoto.verified_statements_by_predicate(intoto.predicate_test_result) |
There was a problem hiding this comment.
The package is a deliberate no-op when no test-result attestations are attached, but Jira AC-1 says "Policy denies if no test-result attestations are attached to the image." PR description says this was deferred per Joe -- is that recorded anywhere besides this PR? The ticket still lists it as an acceptance criterion. Worth updating EC-1775 to reflect the scope reduction so the decision is traceable.
|
|
||
| _test_list(predicate, key) := result if { | ||
| value := object.get(predicate, key, []) | ||
| is_array(value) |
There was a problem hiding this comment.
The is_array guard is untested. No test case provides a non-array value (string, object, number) for failedTests/warnedTests. That's a distinct equivalence class from "key missing" and "key present with valid array."
| value := object.get(predicate, key, []) | ||
| is_array(value) | ||
| items := [x | some x in value; is_string(x)] | ||
| count(items) > 0 |
There was a problem hiding this comment.
This boundary is also untested. An empty failedTests: [] takes a different path than a missing key, but both should produce "(none listed)."
| # collections: | ||
| # - redhat | ||
| # depends_on: | ||
| # - attestation_type.known_attestation_type |
There was a problem hiding this comment.
Worth noting a latent coupling here. attestation_type.known_attestation_type checks lib.pipelinerun_attestations -- attestations embedded in SLSA provenance via input.attestations. This package consumes a completely different trust chain: OCI referrer attestations verified independently through intoto.verified_statements_by_predicate.
Today both attestation pipelines are always present together, so depends_on works fine. But the semantic mismatch means if the project ever moves toward OCI referrer-only attestations (no pipelinerun attestation), known_attestation_type would deny and depends_on would suppress this package's findings -- even though they're independently valid from a separate trust chain.
This is the first consumer of verified_statements_by_predicate outside the intoto library, so there's no established depends_on pattern for referrer-based policies yet. Not actionable for this PR, but worth keeping in mind when a second referrer-based package appears -- that's when you'd want a new base rule analogous to known_attestation_type for the referrer trust chain.
| result := metadata.result_helper_with_term( | ||
| rego.metadata.chain(), | ||
| [_test_name(statement), failed], | ||
| _test_name(statement), |
There was a problem hiding this comment.
The term resolves to configuration[0].name. If two attestations share the same config name (e.g. two "clair-scan" runs at different pipeline stages), their violations get identical terms. A policy exclusion targeting one silently suppresses the other.
The existing test package doesn't have this problem because its term comes from pipeline task names, which are unique within a pipeline. Test attestation config names have no such guarantee.
Is configuration[0].name expected to be unique across test-result attestations for a given image? If not, would incorporating additional identity (e.g. the statement subject digest) into the term produce more precise exclusion targeting?
| # | ||
| deny contains result if { | ||
| some statement in _test_attestations | ||
| not statement.predicate.result |
There was a problem hiding this comment.
not statement.predicate.result in Rego is true for both absent fields and falsy values (false, null, 0, ""). The failure message says "missing the required result field" but result: false would trigger this too. Intentional, or should this check for key absence specifically?
3 participants
Summary
policy.release.test_attestationthat verifies in-toto test-result attestation contentno_failed_tests(deny),no_test_warnings(warn),test_result_known(deny),test_data_found(deny)_test_name(extracts configuration name),_test_list(safely formats test name arrays)intoto.verified_statements_by_predicatefrom EC-1774's trust layer — only chain-of-trust-verified attestations are evaluatedDesign Decisions
resultenum; optionalfailedTests/warnedTestsarrays enrich error messagestest_result_knownguards withstatement.predicate.resultbefore checking set membership, preventing overlap withtest_data_foundconfiguration[0].namerather than OCI annotationsTest plan
Jira: EC-1775
🤖 Generated with Claude Code