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
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
|
📝 WalkthroughWalkthroughAdds regexp-based keyless verification parameters and wiring across tasks, docs, and tests; task logic updated to prefer exact-match params over regexp variants and to select keyless mode when any certificate param is provided. 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:
|
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
simonbaird
force-pushed
the
improve-keyless-support
branch
from
7302a30 to
b7d26cf
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml (1)
86-98: Typo in parameter descriptions: "precendence" → "precedence".Lines 90 and 97 both have "precendence" instead of "precedence".
📝 Proposed fix
- name: CERTIFICATE_IDENTITY_REGEXP type: string description: >- Similar to CERTIFICATE_IDENTITY but the value is a regexp that will be matched. - Note that CERTIFICATE_IDENTITY takes precendence over this if both are present. + Note that CERTIFICATE_IDENTITY takes precedence over this if both are present. default: "" - name: CERTIFICATE_OIDC_ISSUER_REGEXP type: string description: >- Similar to CERTIFICATE_OIDC_ISSUER but a regexp that will be matched. Note that - CERTIFICATE_OIDC_ISSUER takes precendence over this if both are present. + CERTIFICATE_OIDC_ISSUER takes precedence over this if both are present. default: ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml` around lines 86 - 98, The two parameter descriptions for CERTIFICATE_IDENTITY_REGEXP and CERTIFICATE_OIDC_ISSUER_REGEXP contain a typo ("precendence"); update both descriptions to use the correct spelling "precedence" so the text reads "Note that CERTIFICATE_IDENTITY takes precedence over this..." and "Note that CERTIFICATE_OIDC_ISSUER takes precedence over this..." respectively; locate the descriptions by the parameter names CERTIFICATE_IDENTITY_REGEXP and CERTIFICATE_OIDC_ISSUER_REGEXP and replace the misspelled word.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml`:
- Around line 86-98: The two parameter descriptions for
CERTIFICATE_IDENTITY_REGEXP and CERTIFICATE_OIDC_ISSUER_REGEXP contain a typo
("precendence"); update both descriptions to use the correct spelling
"precedence" so the text reads "Note that CERTIFICATE_IDENTITY takes precedence
over this..." and "Note that CERTIFICATE_OIDC_ISSUER takes precedence over
this..." respectively; locate the descriptions by the parameter names
CERTIFICATE_IDENTITY_REGEXP and CERTIFICATE_OIDC_ISSUER_REGEXP and replace the
misspelled word.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0390ef42-15a1-40b5-9b88-bd0a5b27bf62
⛔ 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
✅ Files skipped from review due to trivial changes (3)
- docs/modules/ROOT/pages/verify-conforma-konflux-ta.adoc
- docs/modules/ROOT/pages/verify-enterprise-contract.adoc
- tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- features/task_validate_image.feature
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.