Several keyless verification task improvements#3187
Open
simonbaird wants to merge 1 commit intoconforma:mainfrom
Open
Several keyless verification task improvements#3187simonbaird wants to merge 1 commit intoconforma:mainfrom
simonbaird wants to merge 1 commit intoconforma:mainfrom
Conversation
Here's what's included: - Support for the -regexp versions of the keyless verification params. Prefer the non-regexp param if (for some reason) both are present. - Make it so we never use --ignore-rekor when doing keyless verification even if IGNORE_REKOR is true. This is because you need a transparency log entry from Rekor to do keyless verification. - Some minor bash env var handling logic tweaks related to handling of unlikely edge cases. Note that we're still trying not to add a layer of bash logic for param sanitizing as per the comment there. This could be broken up into multiple commits, and originally it was, but I've been working on a previous version of PR too long and I don't think it's worth the effort right now. Ref: https://redhat.atlassian.net/browse/EC-1652
Contributor
Review Summary by QodoAdd regexp support for keyless verification parameters
WalkthroughsDescription• Add support for regexp versions of keyless verification parameters • Implement preference logic for non-regexp params over regexp variants • Force --ignore-rekor=false for keyless verification workflows • Add comprehensive test scenarios for regexp-based certificate matching Diagramflowchart LR
A["Keyless Verification Parameters"] --> B["CERTIFICATE_IDENTITY<br/>CERTIFICATE_OIDC_ISSUER"]
A --> C["CERTIFICATE_IDENTITY_REGEXP<br/>CERTIFICATE_OIDC_ISSUER_REGEXP"]
B --> D["Preferred if present"]
C --> E["Fallback option"]
D --> F["Build command args"]
E --> F
F --> G["Force ignore-rekor=false<br/>for keyless flow"]
H["Traditional Signing"] --> I["Use PUBLIC_KEY<br/>and IGNORE_REKOR param"]
File Changes1. docs/modules/ROOT/pages/verify-conforma-konflux-ta.adoc
|
📝 WalkthroughWalkthroughThis PR adds regexp-based matching support for keyless signature verification. Two new task parameters ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Code Review by Qodo
1. PUBLIC_KEY precedence misdocumented
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/task_validate_image.feature`:
- Around line 382-412: Add two assertions to the keyless regexp scenarios: (1)
add a case proving exact params override regexp params by including both an
exact match parameter (e.g., CERTIFICATE_IDENTITY or CERTIFICATE_OIDC_ISSUER)
alongside CERTIFICATE_IDENTITY_REGEXP/CERTIFICATE_OIDC_ISSUER_REGEXP and assert
the task uses the exact values (update the "Keyless signing verification cosign
v2 style with regexp params" scenario and its counterpart around the other
block), and (2) add a scenario or variant that sets IGNORE_REKOR=true and runs
in keyless mode (IMAGES with keyless_v2) and assert Rekor is still used (e.g.,
via REKOR_HOST present in task results/logs) to ensure keyless forces Rekor;
update the step expectations (task success, "report-json" logs, and task results
snapshots) to include these verifications.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bb57d69-5b2a-497a-98b7-c65b428d8658
⛔ Files ignored due to path filters (1)
features/__snapshots__/task_validate_image.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
docs/modules/ROOT/pages/verify-conforma-konflux-ta.adocdocs/modules/ROOT/pages/verify-enterprise-contract.adocfeatures/task_validate_image.featuretasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yamltasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Here's what's included:
This could be broken up into multiple commits, and originally it was, but I've been working on a previous version of PR too long and I don't think it's worth the effort right now.
Ref: https://redhat.atlassian.net/browse/EC-1652
Note: These changes were originally in #3171 also.