feat(EC-1950,EC-1982): test attestation parity and trust verification fixes#1770
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR adds shared intoto subject-digest helpers, changes trust verification to validate attestations directly, and expands release test-attestation policy logic to use rule-data-driven result categories, subject matching, and rule-data validation. Tests and Antora docs were updated to match the new policy behavior and naming. ChangesIntoto trust flow
Estimated code review effort: 4 (Complex) | ~55 minutes Release test attestation policy
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant trust.rego
participant oci
participant ec.sigstore
trust.rego->>oci: parsed_blob_from_image(referrer.ref)
trust.rego->>ec.sigstore: verify_attestation(referrer.ref, sigstore.opts)
ec.sigstore-->>trust.rego: success, attestations
trust.rego->>trust.rego: compare subject_digest(subject) to expected_digest
trust.rego->>trust.rego: _all_tasks_trusted(att)
sequenceDiagram
participant test_attestation.rego
participant rule_data.rego
participant intoto.rego
participant oci
test_attestation.rego->>rule_data.rego: supported_test_attestation_results, categorized result keys
test_attestation.rego->>oci: parse image reference
test_attestation.rego->>intoto.rego: subject_digests(subject), subject_digest(subject)
test_attestation.rego->>test_attestation.rego: emit deny/warn results
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🤖 Finished Review · ❌ Failure · Started 7:28 PM UTC · Completed 7:35 PM UTC |
Review — ✅ ApprovePR: feat(EC-1950,EC-1982): test attestation parity and trust verification fixes SummaryThis PR delivers two Jira items cleanly in a single branch: EC-1950 — EC-1982 — Trust verification fix. Corrects two issues in
Both changes remove the now-unnecessary two-hop verification path and the associated OPA v1.12.1 type-checker workaround, resulting in a net simplification. Shared refactor. Architecture & correctness
Trust model changeThe trust model simplification (single-hop verification) is architecturally significant but well-justified. The previous two-hop model ( Test adequacyTests comprehensively cover:
DocumentationAuto-generated Antora docs are updated with correct line numbers and new rule entries. Navigation and release policy docs include all new rules. FindingsNo blocking findings. The change is well-structured, properly tested, and the security improvements (subject mismatch validation, dual result detection) strengthen the policy framework. Low-severity observations (non-blocking)
Labels: PR adds new policy rules and modifies trust verification logic Previous runReview — approvePR: feat(EC-1950,EC-1982): test attestation parity and trust verification fixes SummaryThis PR contains two related changes:
Shared Correctness
Test CoverageComprehensive coverage including:
Findings
Previous run (2)ReviewVerdict: Approve ✅ SummaryThis PR fixes trust verification to discover Tekton Chains provenance via cosign Changes reviewed
Key observations
Minor notes (non-blocking)
All new rules are added to the
Previous run (3)Review —
|
|
🤖 Review · |
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 · ✅ Success · Started 8:33 PM UTC · Completed 8:38 PM UTC |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
policy/release/test_attestation/test_attestation.rego (1)
46-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCount-based trigger isn't documented in rule metadata.
_has_result's second clause fires purely from a count field (failures/warnings> 0), independent ofpredicate.resultand independent of the configuredresultslist (confirmed bytest_count_triggers_deny, which denies aPASSED-result statement due tofailures: 5). None of the three rule descriptions here (no_failed_informative_test_attestations, no_test_warnings, no_failed_tests) mention this fallback — they only describe the configurable result-string match. Worth a one-line addition to each description so users/auditors understand why a rule can fire even whenresultlooks passing.Also applies to: 63-93, 94-121, 122-151
🤖 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.rego` around lines 46 - 61, The rule metadata descriptions for the test attestation policies do not mention that `_has_result` can also match via count fields like failures or warnings, even when predicate.result is passing. Update the descriptions for no_failed_informative_test_attestations, no_test_warnings, and no_failed_tests in test_attestation.rego to explicitly document this fallback behavior so the count-based trigger is discoverable.policy/lib/intoto/intoto_test.rego (1)
109-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a multi-algorithm regression test for
subject_digest(singular).Current tests cover
subject_digestonly with single-algorithm maps andsubject_digestswith a multi-algorithm map, but neversubject_digestwith a multi-algorithm map — the exact input that triggers the eval-conflict issue flagged inpolicy/lib/intoto/intoto.rego(Lines 57-60).🤖 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/lib/intoto/intoto_test.rego` around lines 109 - 121, Add a regression test for intoto.subject_digest using a digest map with multiple algorithms, since only single-algorithm cases are currently covered and that is the path that exposes the eval-conflict behavior. Extend the existing intoto_test.rego tests near test_subject_digest and test_subject_digests_multi_algorithm so subject_digest is asserted against a multi-algorithm {"digest": {...}} input, and verify the expected result or conflict behavior matches the intended contract.
🤖 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.
Inline comments:
In `@antora/docs/modules/ROOT/pages/packages/release_test_attestation.adoc`:
- Around line 98-100: Update the `test_data_found` solution text in the release
test attestation docs so it only states that the predicate must include a
`result` field. Remove the mention of recognized values like
PASSED/WARNED/FAILED, since that guidance belongs to `test_result_known`; keep
the wording aligned with the existing `test_data_found` rule and its surrounding
documentation.
In `@policy/lib/intoto/intoto.rego`:
- Around line 51-60: `subject_digest` in intoto.rego currently uses `some digest
in subject_digests(subject)`, which can become ambiguous for subjects with
multiple digest algorithms and cause evaluation conflicts. Update
`subject_digest` to either require exactly one entry from
`subject_digests(subject)` or select a digest deterministically, and make sure
the caller in `slsa_build_scripted_build.rego` only relies on the safe
single-digest behavior.
---
Nitpick comments:
In `@policy/lib/intoto/intoto_test.rego`:
- Around line 109-121: Add a regression test for intoto.subject_digest using a
digest map with multiple algorithms, since only single-algorithm cases are
currently covered and that is the path that exposes the eval-conflict behavior.
Extend the existing intoto_test.rego tests near test_subject_digest and
test_subject_digests_multi_algorithm so subject_digest is asserted against a
multi-algorithm {"digest": {...}} input, and verify the expected result or
conflict behavior matches the intended contract.
In `@policy/release/test_attestation/test_attestation.rego`:
- Around line 46-61: The rule metadata descriptions for the test attestation
policies do not mention that `_has_result` can also match via count fields like
failures or warnings, even when predicate.result is passing. Update the
descriptions for no_failed_informative_test_attestations, no_test_warnings, and
no_failed_tests in test_attestation.rego to explicitly document this fallback
behavior so the count-based trigger is discoverable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 66c4af4b-1580-4a1b-989d-05ed07d46865
📒 Files selected for processing (13)
.github/workflows/release.yamlantora/docs/modules/ROOT/pages/packages/release_slsa_build_scripted_build.adocantora/docs/modules/ROOT/pages/packages/release_test_attestation.adocantora/docs/modules/ROOT/pages/release_policy.adocantora/docs/modules/ROOT/partials/release_policy_nav.adocpolicy/lib/intoto/intoto.regopolicy/lib/intoto/intoto_test.regopolicy/lib/intoto/trust.regopolicy/lib/intoto/trust_test.regopolicy/lib/rule_data/rule_data.regopolicy/release/slsa_build_scripted_build/slsa_build_scripted_build.regopolicy/release/test_attestation/test_attestation.regopolicy/release/test_attestation/test_attestation_test.rego
✅ Files skipped from review due to trivial changes (1)
- antora/docs/modules/ROOT/pages/packages/release_slsa_build_scripted_build.adoc
🚧 Files skipped from review as they are similar to previous changes (1)
- policy/lib/intoto/trust_test.rego
…kage Add configurable result values, ERROR/SKIPPED handling, informative test support, subject digest validation, and rule data schema validation to the test_attestation package, matching the capabilities of the test package for OCI-referrer-based test result attestations. - AC-1: Replace hardcoded result checks with rule_data.get lookups using predicate spec vocabulary (PASSED/FAILED/WARNED/ERROR/SKIPPED) - AC-2: Add no_erred_test_attestations deny rule - AC-3: Add no_skipped_test_attestations deny rule - AC-4: Add informative_test_attestations support (warn not deny) - AC-5: Add subject_mismatch deny rule with ANY-match semantics - AC-6: Add rule_data_provided schema validation for all 6 keys - AC-7: 9 new tests covering all acceptance criteria Moves subject_digest from private in trust.rego to public in intoto.rego to avoid reimplementing digest format conversion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The attest-test-result step action produces integer count fields (failures, warnings, successes) not string arrays (failedTests, warnedTests, passedTests). Update the policy rules to match: - Replace _test_list (string array extraction) with _count_detail (integer count formatting) and _has_result (dual check: result string match OR count > 0, following test.rego's _did_result) - Update all mock test data to use the step action's predicate structure - Quote failure_msg values containing colons for YAML compatibility Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
policy/lib/intoto/trust.rego (1)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify to satisfy Regal's
superfluous object.getlint.The default
falseis equivalent to a plain reference here (a missingsuccesskey makes the rule fail either way, so it stays fail-closed). Regal flags this at error level, which may break the lint stage.♻️ Proposed change
- object.get(verification, "success", false) == true + verification.success == true🤖 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/lib/intoto/trust.rego` at line 67, The trust.rego rule is using a redundant object.get check for verification success, which triggers Regal’s superfluous object.get lint. Update the expression in the policy rule to reference the success field directly from verification instead of calling object.get, keeping the same fail-closed behavior. Use the existing verification predicate in the trust policy to locate and simplify the condition.Source: Linters/SAST tools
🤖 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/lib/intoto/trust.rego`:
- Line 67: The trust.rego rule is using a redundant object.get check for
verification success, which triggers Regal’s superfluous object.get lint. Update
the expression in the policy rule to reference the success field directly from
verification instead of calling object.get, keeping the same fail-closed
behavior. Use the existing verification predicate in the trust policy to locate
and simplify the condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: c419969c-9ce5-472a-ba3e-4ddb6228726c
📒 Files selected for processing (12)
antora/docs/modules/ROOT/pages/packages/release_slsa_build_scripted_build.adocantora/docs/modules/ROOT/pages/packages/release_test_attestation.adocantora/docs/modules/ROOT/pages/release_policy.adocantora/docs/modules/ROOT/partials/release_policy_nav.adocpolicy/lib/intoto/intoto.regopolicy/lib/intoto/intoto_test.regopolicy/lib/intoto/trust.regopolicy/lib/intoto/trust_test.regopolicy/lib/rule_data/rule_data.regopolicy/release/slsa_build_scripted_build/slsa_build_scripted_build.regopolicy/release/test_attestation/test_attestation.regopolicy/release/test_attestation/test_attestation_test.rego
✅ Files skipped from review due to trivial changes (3)
- antora/docs/modules/ROOT/pages/packages/release_slsa_build_scripted_build.adoc
- policy/lib/intoto/intoto_test.rego
- antora/docs/modules/ROOT/pages/packages/release_test_attestation.adoc
🚧 Files skipped from review as they are similar to previous changes (8)
- antora/docs/modules/ROOT/pages/release_policy.adoc
- policy/release/slsa_build_scripted_build/slsa_build_scripted_build.rego
- policy/lib/intoto/intoto.rego
- policy/lib/rule_data/rule_data.rego
- antora/docs/modules/ROOT/partials/release_policy_nav.adoc
- policy/release/test_attestation/test_attestation.rego
- policy/lib/intoto/trust_test.rego
- policy/release/test_attestation/test_attestation_test.rego
|
🤖 Finished Review · ✅ Success · Started 6:02 PM UTC · Completed 6:07 PM UTC |
|
🤖 Review · |
|
🤖 Review · |
OPA strict mode requires unused arguments to use _ (wildcard). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add doc comment on intoto.subject_digest - Add test for missing subject field triggering subject_mismatch Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Change subject_digest to set-based subject_digests to handle multi-algorithm subjects without runtime conflicts - Add empty-digest guard to subject_mismatch rule for tag-only refs - Use subject_digests in _subject_matches for correct set membership - Add test for multi-algorithm subject digests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ification Chains stores SLSA provenance as cosign-tagged artifacts (.att suffix), not as OCI referrers. _has_trusted_provenance was using ec.oci.image_referrers to find provenance, which never found it. Now calls ec.sigstore.verify_attestation directly on the referrer ref, which uses cosign's tag-based discovery. Also fixes parsed_blob -> parsed_blob_from_image so the in-toto statement content is extracted from the OCI artifact layer rather than returning the manifest JSON. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-based trust Tests now mock ec.oci.image_manifest (used by parsed_blob_from_image to traverse manifest → layer → blob) and match verify_attestation calls on statement refs instead of provenance refs (since _has_trusted_provenance now calls verify_attestation directly rather than looking up provenance via image_referrers). Removed dead provenance referrer mock code that was no longer exercised. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address PR review feedback: remove unnecessary array-to-set conversions in `in` expressions since Rego handles both identically, and add comments explaining the zero-count and empty-string edge case tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The zero-count tests exercised the same result-string clause of _has_result already covered by the basic warned/failed tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🤖 Finished Review · ✅ Success · Started 6:21 PM UTC · Completed 6:28 PM UTC |
|
🤖 Finished Retro · ✅ Success · Started 7:53 PM UTC · Completed 7:58 PM UTC |
Retro Analysis: PR #1770 — test attestation parity and trust verification fixesOriginating PR: #1770 Context limitationsGitHub API access was unavailable during this analysis (403), so the review conversation, CI results, iteration count, and agent/human review comments could not be examined. Analysis is based solely on the merged code. The repo clone contains only the merge commit, making diff-based analysis of the PR branch impossible. ObservationsThis PR ( The merged codebase shows two architectural patterns that would benefit from better documentation for future agent work:
Proposals1 proposal filed — see below. Proposals filed
|
Summary
test_attestationpackage to feature parity with thetestpackage — adds rules for warned results, informative tests, erred/skipped results, subject mismatch, unsupported result values, and rule data validation._has_trusted_provenanceto callec.sigstore.verify_attestationdirectly on the referrer ref instead of looking for provenance viaec.oci.image_referrers. Tekton Chains stores SLSA provenance as cosign.atttags, not OCI referrers.verified_statementsto useparsed_blob_from_imageso the in-toto statement is extracted from the OCI artifact layer instead of returning the manifest JSON.Jira: EC-1950, EC-1982
Test plan
🤖 Generated with Claude Code