trusted_task_rules: Add per-allow-rule signature verification - #1680
trusted_task_rules: Add per-allow-rule signature verification#1680arewm wants to merge 1 commit into
Conversation
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:
|
|
@simonbaird @joejstuart , is this something that you are interested in? |
f066bc2 to
d757058
Compare
|
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:
📝 WalkthroughWalkthroughTrusted Tekton task evaluation now supports Sigstore signature verification for OCI bundles. The schema accepts credential selectors and verification options, trust decisions incorporate verification results, and denial responses identify failed patterns. Unit and release tests cover these behaviors. ChangesTrusted task signature verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TrustedTaskRules
participant OCIBundle
participant Sigstore
TrustedTaskRules->>OCIBundle: Resolve bundle manifests
TrustedTaskRules->>Sigstore: Verify bundle with rule options
Sigstore-->>TrustedTaskRules: Return verification result
TrustedTaskRules-->>TrustedTaskRules: Trust task or report signature_verification_failed
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
policy/lib/tekton/trusted.rego (1)
337-353: 💤 Low valueDenial-reason branch logic looks correct, but consider deduplicating the matching predicate.
The new
signature_verification_failedbranch is reached only when no deny rule matches, some allow rule matches pattern+version, and_task_matches_allow_rulestill fails — i.e., signature verification is the only remaining failure cause. Git-resolved refs continue to short-circuit through_signature_verified_for_rule(ref, _) if { not ref.bundle }, so they do not surface this denial reason.One minor refactor opportunity: the pattern+version matching block here duplicates the predicate inside
_task_matches_allow_ruleminus the signature check. Extracting a helper like_task_matches_allow_rule_without_signature(ref, rule, bundle_manifests)would keep the two call sites aligned if pattern/version semantics ever evolve. Optional under the chill profile.🤖 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/tekton/trusted.rego` around lines 337 - 353, The denial branch for "signature_verification_failed" duplicates the pattern+version matching logic that also exists inside _task_matches_allow_rule; extract that shared predicate into a helper (e.g., _task_matches_allow_rule_without_signature(ref, rule, bundle_manifests)) and replace the duplicated checks in the signature_verification_failed branch and inside _task_matches_allow_rule so both call the new helper and only differ by the signature verification step (keeping existing short-circuit behavior from _signature_verified_for_rule(ref, _) when not ref.bundle intact).
🤖 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 `@policy/lib/tekton/trusted.rego`:
- Around line 405-441: The current _sigstore_opts_for_rule(rule) returns opts
with certificate/issuer keys set to empty strings which makes
signature_verification: {} permissive; update _sigstore_opts_for_rule to remove
any keys whose value is "" (and/or false when appropriate) before returning so
ec.sigstore.verify_image(bundle, opts) only receives explicitly configured
constraints, and/or add schema changes to require at least one identity and at
least one issuer field in signature_verification; ensure references to
_sigstore_opts_for_rule, _signature_verified_for_rule,
_sigstore_verify_has_errors and ec.sigstore.verify_image are updated to use the
stripped/validated opts.
---
Nitpick comments:
In `@policy/lib/tekton/trusted.rego`:
- Around line 337-353: The denial branch for "signature_verification_failed"
duplicates the pattern+version matching logic that also exists inside
_task_matches_allow_rule; extract that shared predicate into a helper (e.g.,
_task_matches_allow_rule_without_signature(ref, rule, bundle_manifests)) and
replace the duplicated checks in the signature_verification_failed branch and
inside _task_matches_allow_rule so both call the new helper and only differ by
the signature verification step (keeping existing short-circuit behavior from
_signature_verified_for_rule(ref, _) when not ref.bundle intact).
🪄 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: f7470a8e-3be9-4ee1-9d4a-b86852829975
📒 Files selected for processing (4)
policy/lib/tekton/trusted.regopolicy/lib/tekton/trusted_test.regopolicy/release/trusted_task/trusted_task.regopolicy/release/trusted_task/trusted_task_test.rego
d757058 to
47e49fc
Compare
8b470e8 to
0221318
Compare
|
Gave it a fresh rebase. My plan is to re-review with the goal of getting it merged. Maybe we'll get some agentic reviews triggered also. |
|
🤖 Finished Review · ✅ Success · Started 4:01 PM UTC · Completed 4:12 PM UTC |
|
I'm not certain, but I think we want to merge conforma/cli#3136 before merging this. |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)Review — approve
SummaryThis PR adds optional per-allow-rule signature verification to the
Correctness
Security
Findings
Labels: PR adds signature verification configuration to trusted task allow rules Previous run (4)ReviewFindingsMedium
Low
Info
|
fullsend-ai-review
Bot
added
the
requires-manual-review
|
It shouldn't matter whether this or the CLI change is merged first. This rule shouldn't be hit yet so it shouldn't be required to have the CLI fix in. Before leveraging this rule, however, we should make sure that the CLI is updated. |
0221318 to
a160a5a
Compare
|
🤖 Review · ❌ Terminated · Started 8:54 PM UTC · Ended 9:01 PM UTC |
|
Rebased again. FWIW conforma/cli#3136 is merged now. |
![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
|
🤖 Finished Review · ✅ Success · Started 8:54 PM UTC · Completed 9:01 PM UTC |
a160a5a to
6f37270
Compare
|
🤖 Review · |
|
I rebased and did a little refactoring. No functional change, just IMO slightly easier to follow logic. One more round of agentic review I guess, then I'd be happy to merge. |
|
It would be good to test this with a real signature. I haven't done that as yet. Update: Created https://redhat.atlassian.net/browse/EC-2030 to track doing that. |
PR Summary by QodoAdd Per-Rule Sigstore Verification for Trusted Tasks
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
🤖 Review · ❌ Terminated · Started 7:08 PM UTC · Ended 7:27 PM UTC |
Code Review by Qodo
Context used✅ Compliance rules (platform):
15 rules 1. Direct rules bypass signature schema
|
![qodo-for-conforma[bot]](https://avatars.githubusercontent.com/in/3609154?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
policy/lib/tekton/trusted.rego (2)
473-497: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMatching-rules logic is duplicated between allow-rule matching and signature-check gating.
_task_bundle_sig_check_okayre-derivesmatching_rulesvia the same pattern+version filtering criteria that_task_matches_allow_rulepresumably applies (used inis_trusted_task_rulesat Line 138). Keeping two independent implementations of "which allow rules match this ref" risks silent drift if one is updated without the other (e.g., a future change to version-constraint semantics).#!/bin/bash # Confirm whether _task_matches_allow_rule duplicates the same filter logic. rg -nP -A15 '_task_matches_allow_rule\(ref, bundle_manifests\) if' policy/lib/tekton/trusted.rego♻️ Suggested extraction
+_matching_allow_rules(ref, bundle_manifests) := [rule | + some rule in _effective_allow_rules + _pattern_matches(ref.key, rule.pattern) + _version_satisfies_all_rule_constraints(ref, rule, bundle_manifests) +] + _task_bundle_sig_check_okay(ref, bundle_manifests) if { - matching_rules := [rule | - some rule in _effective_allow_rules - _pattern_matches(ref.key, rule.pattern) - _version_satisfies_all_rule_constraints(ref, rule, bundle_manifests) - ] - _signature_verified_for_rules(ref, matching_rules) + _signature_verified_for_rules(ref, _matching_allow_rules(ref, bundle_manifests)) }🤖 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/tekton/trusted.rego` around lines 473 - 497, Update _task_bundle_sig_check_okay to reuse _task_matches_allow_rule for determining matching allow rules instead of independently applying _pattern_matches and _version_satisfies_all_rule_constraints. Preserve the existing matching_rules input expected by _signature_verified_for_rules, ensuring pattern and version filtering has a single shared implementation.
553-573: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEnforce mutually exclusive Sigstore credential selectors.
The
signature_verificationschema allows combiningpublic_keywith one or more certificate identity fields becauseanyOfonly requires at least one option. If those modes are meant to be exclusive, switch tooneOfso conflicting combinations fail validation instead of being passed toec.sigstore.verify_image.🤖 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/tekton/trusted.rego` around lines 553 - 573, Update the signature_verification schema’s credential selector constraint from anyOf to oneOf, ensuring exactly one of certificate_identity, certificate_identity_regexp, or public_key is accepted while conflicting combinations fail validation before ec.sigstore.verify_image.policy/release/trusted_task/trusted_task_test.rego (1)
1361-1392: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssertion is looser than the rest of the file's test conventions.
Every other test here uses
assertions.assert_equal_results(trusted_task.deny, expected)with an exact expected set. This new test only checkscount(results) > 0and that some message contains a substring, so it wouldn't catch a wrongcode/term, an incorrectly formatted message, or unexpected additional denials. Consider asserting the exact expected result set for consistency and stronger regression protection, matching the style oftest_deny_takes_precedence_over_allowetc.🤖 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/trusted_task/trusted_task_test.rego` around lines 1361 - 1392, Strengthen test_signature_verification_failed_error_rules by replacing the count and substring checks with assertions.assert_equal_results against the exact expected trusted_task.deny result set. Include the expected denial’s code, term, and fully formatted message, following the assertion style used by test_deny_takes_precedence_over_allow and nearby tests, while preserving the existing mocked verification failure setup.
🤖 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/tekton/trusted.rego`:
- Around line 473-497: Update _task_bundle_sig_check_okay to reuse
_task_matches_allow_rule for determining matching allow rules instead of
independently applying _pattern_matches and
_version_satisfies_all_rule_constraints. Preserve the existing matching_rules
input expected by _signature_verified_for_rules, ensuring pattern and version
filtering has a single shared implementation.
- Around line 553-573: Update the signature_verification schema’s credential
selector constraint from anyOf to oneOf, ensuring exactly one of
certificate_identity, certificate_identity_regexp, or public_key is accepted
while conflicting combinations fail validation before ec.sigstore.verify_image.
In `@policy/release/trusted_task/trusted_task_test.rego`:
- Around line 1361-1392: Strengthen
test_signature_verification_failed_error_rules by replacing the count and
substring checks with assertions.assert_equal_results against the exact expected
trusted_task.deny result set. Include the expected denial’s code, term, and
fully formatted message, following the assertion style used by
test_deny_takes_precedence_over_allow and nearby tests, while preserving the
existing mocked verification failure setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a1a7a42-6531-4aa2-a780-52b5ec7c7493
📒 Files selected for processing (4)
policy/lib/tekton/trusted.regopolicy/lib/tekton/trusted_test.regopolicy/release/trusted_task/trusted_task.regopolicy/release/trusted_task/trusted_task_test.rego
🚧 Files skipped from review as they are similar to previous changes (2)
- policy/release/trusted_task/trusted_task.rego
- policy/lib/tekton/trusted_test.rego
|
🤖 Finished Review · ❌ Failure · Started 7:08 PM UTC · Completed 7:27 PM UTC |
6f37270 to
6577722
Compare
|
Addressed some more review-bot feedback. |
|
🤖 Finished Review · ❌ Failure · Started 7:44 PM UTC · Completed 8:02 PM UTC |
6577722 to
e0d416e
Compare
|
🤖 Review · |
e0d416e to
a5a0f98
Compare
|
Fixing a line-too-long lint. No other changes. |
|
🤖 Finished Review · ✅ Success · Started 3:15 PM UTC · Completed 3:34 PM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
a5a0f98 to
5edd37b
Compare
Add optional `signature_verification` configuration to allow rules in trusted_task_rules, enabling sigstore-based signature verification as an additional trust dimension for task bundles. When an allow rule includes `signature_verification`, matching bundles must also pass sigstore verification with the configured identity/key. Rules without the field continue to work as before (pattern-only trust). Git-resolved tasks are exempt since ec.sigstore.verify_image only works on OCI refs. A new denial reason type `signature_verification_failed` is surfaced when a task matches an allow rule's pattern/version constraints but fails signature verification. Ref: EC-1545 Assisted-by: Claude Code (Opus 4.6)
5edd37b to
c116a14
Compare
|
🤖 Finished Review · ❌ Failure · Started 7:43 PM UTC · Completed 7:44 PM UTC |
| v != "" | ||
| v != false | ||
| } | ||
| } |
There was a problem hiding this comment.
Actually it might be possible for this part to go away once Joe's other PR is merged.
|
Probably want to rebase this on #1780 also. |
2 participants
Summary
Today,
trusted_task_rulestrusts task bundles based solely on URL pattern matching — if the bundle ref matchesoci://quay.io/konflux-ci/tekton-catalog/*, it's trusted. This PR adds an optionalsignature_verificationfield on allow rules so that matching bundles must also have a verified sigstore signature from a specific identity.This is per-allow-rule because different catalogs may be signed by different parties (e.g., Konflux catalog vs. a third-party catalog). Rules without the field work exactly as before.
Changes:
signature_verificationconfig to allow rules intrusted_task_rulesec.sigstore.verify_imageonly works on OCI refs)signature_verification_faileddenial reason type with proper error formattingCompanion CLI PR: conforma/cli#3136 (caches
ec.sigstore.verify_imageresults across component evaluations for performance).Ref: EC-1545
Test plan