Add keyless support to verify-conforma-konflux-ta release pipeline task also#3158
Open
simonbaird wants to merge 8 commits intoconforma:mainfrom
Open
Add keyless support to verify-conforma-konflux-ta release pipeline task also#3158simonbaird wants to merge 8 commits intoconforma:mainfrom
simonbaird wants to merge 8 commits intoconforma:mainfrom
Conversation
Contributor
Review Summary by QodoAdd keyless signing support to verify-conforma-konflux-ta release pipeline task
WalkthroughsDescription• Add keyless signing support to release pipeline task with CERTIFICATE_IDENTITY and CERTIFICATE_OIDC_ISSUER parameters • Refactor bash script to use consistent --flag=value style and environment variables for all parameters • Change timeout from 100h to 0 to indicate no timeout, letting Tekton handle timeouts • Improve parameter descriptions for clarity on keyless vs traditional signing verification • Enhance consistency between verify-enterprise-contract and verify-conforma-konflux-ta tasks Diagramflowchart LR
A["Release Pipeline Task"] -->|Add keyless params| B["CERTIFICATE_IDENTITY<br/>CERTIFICATE_OIDC_ISSUER"]
A -->|Refactor script| C["Consistent arg handling<br/>--flag=value style"]
A -->|Update timeout| D["0 timeout<br/>Tekton manages"]
B -->|Conditional logic| E["Keyless or Traditional<br/>Signing verification"]
C -->|Use env vars| F["All params via<br/>environment variables"]
File Changes1. tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml
|
Contributor
Code Review by Qodo
1. Keyless ignored if policy has key
|
Member
Author
|
For reviewers: The important functional change is in the top commit. You can look at that separately to understand the new keyless support. The other commits about are more about tidying and consistency. For review it might be easier to look at them one by one. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+321
to
+330
| if [ -n "${CERTIFICATE_IDENTITY}" ] && [ -n "${CERTIFICATE_OIDC_ISSUER}" ]; then | ||
| cmd_args+=( | ||
| --certificate-identity="${CERTIFICATE_IDENTITY}" | ||
| --certificate-oidc-issuer="${CERTIFICATE_OIDC_ISSUER}" | ||
| ) | ||
| elif [ -n "${PUBLIC_KEY}" ]; then | ||
| cmd_args+=( | ||
| --public-key="${PUBLIC_KEY}" | ||
| ) | ||
| fi |
Contributor
There was a problem hiding this comment.
1. Keyless ignored if policy has key 🐞 Bug ⛨ Security
Even when the task passes --certificate-identity/--certificate-oidc-issuer, Conforma only uses keyless identity when the resolved policy’s PublicKey is empty; otherwise it prioritizes the public-key workflow and ignores identity. With the default policy commonly containing publicKey, users may believe they enabled keyless verification but actually still run key-based verification (or get unexpected failures).
Agent Prompt
## Issue description
The task now supports keyless flags, but Conforma only enables keyless verification when the resolved policy has an empty `publicKey`. If the referenced EnterpriseContractPolicy still contains `publicKey`, keyless identity/issuer flags will not take effect.
## Issue Context
This can lead to silent misconfiguration: users provide `CERTIFICATE_IDENTITY`/`CERTIFICATE_OIDC_ISSUER` expecting keyless identity enforcement, but Conforma prioritizes the public-key workflow when `publicKey` is present.
## Fix Focus Areas
- docs/modules/ROOT/pages/verify-conforma-konflux-ta.adoc[24-27]
- docs/modules/ROOT/pages/verify-enterprise-contract.adoc[35-38]
- tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml[52-85]
- tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml[59-92]
(Optionally consider a code change in Conforma to allow identity flags to override publicKey, but that’s beyond task-only changes.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
This was suggested in #3140 also. My thinking is I'd rather not add a layer of bash param validation. I guess we could go harder in that direction and remove the "else" part of the logic.
Not sure what Conforma would do if it received the keyless params and a public key, but we could try it.
Contributor
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Add explicit warnings in the validate step when keyless parameters are partially provided or when keyless parameters are provided but verification may still fall back to key-based verification due to policy configuration. Keep existing argument selection behavior (keyless takes precedence over PUBLIC_KEY when both keyless params are set).
Suggested change
| if [ -n "${CERTIFICATE_IDENTITY}" ] && [ -n "${CERTIFICATE_OIDC_ISSUER}" ]; then | |
| cmd_args+=( | |
| --certificate-identity="${CERTIFICATE_IDENTITY}" | |
| --certificate-oidc-issuer="${CERTIFICATE_OIDC_ISSUER}" | |
| ) | |
| elif [ -n "${PUBLIC_KEY}" ]; then | |
| cmd_args+=( | |
| --public-key="${PUBLIC_KEY}" | |
| ) | |
| fi | |
| if [ -n "${CERTIFICATE_IDENTITY}" ] && [ -n "${CERTIFICATE_OIDC_ISSUER}" ]; then | |
| cmd_args+=( | |
| --certificate-identity="${CERTIFICATE_IDENTITY}" | |
| --certificate-oidc-issuer="${CERTIFICATE_OIDC_ISSUER}" | |
| ) | |
| echo "INFO: Keyless parameters provided. Note: if the referenced policy configuration still sets a publicKey, Conforma may prioritize key-based verification." >&2 | |
| elif [ -n "${CERTIFICATE_IDENTITY}" ] || [ -n "${CERTIFICATE_OIDC_ISSUER}" ]; then | |
| echo "WARNING: Only one of CERTIFICATE_IDENTITY/CERTIFICATE_OIDC_ISSUER was provided. Keyless verification requires both; falling back to PUBLIC_KEY/policy configuration if available." >&2 | |
| if [ -n "${PUBLIC_KEY}" ]; then | |
| cmd_args+=( | |
| --public-key="${PUBLIC_KEY}" | |
| ) | |
| fi | |
| elif [ -n "${PUBLIC_KEY}" ]; then | |
| cmd_args+=( | |
| --public-key="${PUBLIC_KEY}" | |
| ) | |
| fi |
See review comment here
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:
|
st3penta
previously approved these changes
Mar 5, 2026
Contributor
st3penta
left a comment
st3penta
left a comment
There was a problem hiding this comment.
thanks for tidying up the task args, looks much cleaner this way
1 task
Member
Author
|
Needs a rebase on #3118. |
simonbaird
force-pushed
the
keyless-support-in-release-task
branch
from
3930ae1 to
cae552e
Compare
📝 WalkthroughWalkthroughAdds keyless (certificate/OIDC) verification alongside traditional public-key verification. Introduces CERTIFICATE_IDENTITY and CERTIFICATE_OIDC_ISSUER across docs and tasks; tasks now build dynamic cmd_args, conditionally use certificate vs public-key flags, and extend VSA, retry, retry/backoff, operational knobs, and output handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Task as Task Executor
participant EC as Conforma/EC CLI
participant OIDC as OIDC Provider
participant Rekor as Transparency Log
participant VSA as VSA/Attestation Service
Task->>EC: Build cmd_args from params (cert identity/issuer OR public key, retries, VSA flags, outputs)
alt Certificate-based verification
Task->>EC: Execute with --certificate-identity="..." --certificate-oidc-issuer="..."
EC->>OIDC: Validate certificate identity via OIDC issuer
OIDC-->>EC: Identity assertion / certificate info
else Public-key verification
Task->>EC: Execute with --public-key="..."
EC->>EC: Verify signature against provided public key
end
EC->>Rekor: Consult/submit to transparency log (unless IGNORE_REKOR=true)
Rekor-->>EC: Rekor response
alt ENABLE_VSA
EC->>VSA: Upload attestation / request VSA signing
VSA-->>EC: Attestation/VSA result & artifacts
EC-->>Task: Write attestation-output-dir, copy artifacts to configured VSA output path
end
EC-->>Task: Return verification status and outputs (text/json/appstudio reports)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
Member
Author
|
Rebase done. |
See the commentary being removed for explanation. It's been over a year since that was supported, so should be safe to use it now, (fingers crossed I guess?). See commits 73f183e and b7c7d0e from https://issues.redhat.com/browse/EC-1030 . Misc cleanup while working on... Ref: https://issues.redhat.com/browse/EC-1652
- Use --flag=value style consistently instead of a mix of styles - Remove needless quoting when preparing args - Do the quoting consistently when it is needed Ref: https://issues.redhat.com/browse/EC-1652
- Make some of the bash more consistent with the verify-enterprise-contract ITS task. - Use vars for all params instead of referencing params directly in the bash. - Use --flag=value flag style consistently. - Remove some unneeded quoting and use consistent quoting style where it is needed. I'm about to add some new params for keyless signing, and this commit is meant to make that change tider and easier to review. Ref: https://issues.redhat.com/browse/EC-1652
A fixup for conforma#3118. Looks like we don't need it until later in the task, so remove the initial place where it was being calculated but not used. Also add a fixme descibing the problem what was mentioned by Qodo in the conforma#3118 PR.
This was added to the verify-enterprise-contract task very recently in PR conforma#3140, specifically in commit da68f18. Here we're bring the same change to the verify-conforma-konflux-ta which is the one used by the Konflux release pipeline. This commit also contains some param description improvements, which are "back-ported" to the other task as well. Ref: https://issues.redhat.com/browse/EC-1652
simonbaird
force-pushed
the
keyless-support-in-release-task
branch
from
cae552e to
51b498e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml`:
- Line 315: The snapshot input path is hardcoded as the literal flag value
"--images=/tekton/home/snapshot.json"; change it to use the HOMEDIR variable
instead (e.g., replace the hardcoded path with
"--images=${HOMEDIR}/snapshot.json" or the pipeline's equivalent variable
substitution) so the step that reads the snapshot uses the overridden HOMEDIR;
update the step that constructs the --images flag to reference the HOMEDIR
environment/parameter (symbol: the "--images=..." flag and the HOMEDIR
variable).
In `@tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml`:
- Line 267: The validate step is hardcoding the snapshot path via
"--images=/tekton/home/snapshot.json" which breaks when the HOMEDIR parameter is
changed; update the argument to use the parameterized HOMEDIR (replace the
literal path with the HOMEDIR variable used in this task) so the flag becomes
something like "--images=$(params.HOMEDIR)/snapshot.json" (or the file's
existing variable syntax) ensuring the validate step references HOMEDIR rather
than the hardcoded "/tekton/home".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c527df33-9674-452f-918c-8d13eac988eb
📒 Files selected for processing (4)
docs/modules/ROOT/pages/verify-conforma-konflux-ta.adocdocs/modules/ROOT/pages/verify-enterprise-contract.adoctasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yamltasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml
Outdated
Show resolved
Hide resolved
tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml (1)
264-268:⚠️ Potential issue | 🟠 MajorAvoid hardcoded snapshot path in
validatearguments.Line 267 hardcodes
/tekton/home/snapshot.json, which bypasses the configurableHOMEDIRpath used byreduce.Proposed fix
cmd_args=( validate image - --images=/tekton/home/snapshot.json + --images="${HOMEDIR}/snapshot.json" --policy="${POLICY_CONFIGURATION}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml` around lines 264 - 268, The validate command args currently hardcode /tekton/home/snapshot.json in the cmd_args tuple (the validate image invocation using --images), which bypasses the configurable HOMEDIR used by reduce; update the --images argument to reference the HOMEDIR variable (e.g., --images="${HOMEDIR}/snapshot.json" or the same snapshot path variable used by reduce) so the validate invocation uses the configurable home directory and not a hardcoded path, ensuring consistency with reduce and other steps that rely on HOMEDIR and preserving the existing POLICY_CONFIGURATION usage.tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml (1)
312-316:⚠️ Potential issue | 🟠 MajorUse
HOMEDIRfor the snapshot input path invalidate.Line 315 hardcodes
/tekton/home/snapshot.json, but snapshot generation is tied toHOMEDIR. IfHOMEDIRis overridden, validation reads the wrong path.Proposed fix
cmd_args=( validate image - --images=/tekton/home/snapshot.json + --images="${HOMEDIR}/snapshot.json" --policy="${POLICY_CONFIGURATION}" )🤖 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 312 - 316, The validate command currently hardcodes the snapshot path as /tekton/home/snapshot.json; update the cmd_args construction where validate image is assembled to use the HOMEDIR variable instead (e.g. replace the hardcoded "/tekton/home/snapshot.json" with "${HOMEDIR}/snapshot.json") so the validation reads the snapshot from the same HOMEDIR used during snapshot generation; ensure the --images argument uses the HOMEDIR-based path and keep the surrounding args (validate, image, --policy="${POLICY_CONFIGURATION}") unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml`:
- Around line 312-316: The validate command currently hardcodes the snapshot
path as /tekton/home/snapshot.json; update the cmd_args construction where
validate image is assembled to use the HOMEDIR variable instead (e.g. replace
the hardcoded "/tekton/home/snapshot.json" with "${HOMEDIR}/snapshot.json") so
the validation reads the snapshot from the same HOMEDIR used during snapshot
generation; ensure the --images argument uses the HOMEDIR-based path and keep
the surrounding args (validate, image, --policy="${POLICY_CONFIGURATION}")
unchanged.
In `@tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml`:
- Around line 264-268: The validate command args currently hardcode
/tekton/home/snapshot.json in the cmd_args tuple (the validate image invocation
using --images), which bypasses the configurable HOMEDIR used by reduce; update
the --images argument to reference the HOMEDIR variable (e.g.,
--images="${HOMEDIR}/snapshot.json" or the same snapshot path variable used by
reduce) so the validate invocation uses the configurable home directory and not
a hardcoded path, ensuring consistency with reduce and other steps that rely on
HOMEDIR and preserving the existing POLICY_CONFIGURATION usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a013171-703c-4478-863a-11b0086b3fe9
📒 Files selected for processing (4)
docs/modules/ROOT/pages/verify-conforma-konflux-ta.adocdocs/modules/ROOT/pages/verify-enterprise-contract.adoctasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yamltasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
We use it elsewhere, but this one was still hard-coded. Good catch by @coderabbitai. Ref: https://issues.redhat.com/browse/EC-1652
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 `@tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml`:
- Around line 273-282: The script currently allows partial keyless configuration
by only checking paired CERTIFICATE_IDENTITY and CERTIFICATE_OIDC_ISSUER or
falling back to PUBLIC_KEY, which can silently accept a single set variable;
update the conditional logic around CERTIFICATE_IDENTITY,
CERTIFICATE_OIDC_ISSUER and PUBLIC_KEY to fail-fast: detect when exactly one of
the keyless parameters is set (e.g., CERTIFICATE_IDENTITY without
CERTIFICATE_OIDC_ISSUER or vice versa) and emit an error/exit with a clear
message instead of continuing, otherwise proceed to append the correct args to
cmd_args as before; locate and modify the block that references
CERTIFICATE_IDENTITY, CERTIFICATE_OIDC_ISSUER, PUBLIC_KEY and cmd_args to
implement this validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a914bed-a148-477f-b58c-7e57967e9c29
📒 Files selected for processing (2)
tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yamltasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
Outdated
Show resolved
Hide resolved
The review bots complain because if you set one of the two required keyless params, but not the other, you might be surprised that it falls back to using the public key. It's a fair point, so let's avoid that by committing to only try keyless verification if either of the params are found. The bots want us to check explicitly that both are provided, but I'm still keen to avoid adding weak bash param sanitization when we should have stronger, better, and well-tested sanitization already in golang. (And if we don't then we should add it.) Ref: https://issues.redhat.com/browse/EC-1652
I've been runnning `make feature_ta_task_validate_image` and `make feature_task_validate_image` frequently and I'm tired of restoring the deleted snaps manually. Maybe there's another way to do this, but I don't want to go looking under the hood with the snapshot update mechanisms. Minor convenience tweak while working on... Ref: https://issues.redhat.com/browse/EC-1652
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml (1)
273-282:⚠️ Potential issue | 🟠 MajorRequire keyless params as a pair before bypassing
PUBLIC_KEY.At Line 273, using
||means a single keyless value disables thePUBLIC_KEYpath and passes an empty peer flag. That contradicts the parameter contract and can silently run with unintended verification inputs.Suggested fix
- if [ -n "${CERTIFICATE_IDENTITY}" ] || [ -n "${CERTIFICATE_OIDC_ISSUER}" ]; then + if { [ -n "${CERTIFICATE_IDENTITY}" ] && [ -z "${CERTIFICATE_OIDC_ISSUER}" ]; } || \ + { [ -z "${CERTIFICATE_IDENTITY}" ] && [ -n "${CERTIFICATE_OIDC_ISSUER}" ]; }; then + echo "Both CERTIFICATE_IDENTITY and CERTIFICATE_OIDC_ISSUER must be set together." >&2 + exit 1 + fi + + if [ -n "${CERTIFICATE_IDENTITY}" ] && [ -n "${CERTIFICATE_OIDC_ISSUER}" ]; then cmd_args+=( --certificate-identity="${CERTIFICATE_IDENTITY}" --certificate-oidc-issuer="${CERTIFICATE_OIDC_ISSUER}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml` around lines 273 - 282, The current condition uses [ -n "${CERTIFICATE_IDENTITY}" ] || [ -n "${CERTIFICATE_OIDC_ISSUER}" ] which allows a single keyless var to override PUBLIC_KEY and pass empty certificate flags; change the logic so both CERTIFICATE_IDENTITY and CERTIFICATE_OIDC_ISSUER must be non-empty (use logical AND) before appending --certificate-identity/--certificate-oidc-issuer to cmd_args, otherwise fall back to the existing PUBLIC_KEY branch (and ensure you don't append empty flags to cmd_args); reference the variables CERTIFICATE_IDENTITY, CERTIFICATE_OIDC_ISSUER, PUBLIC_KEY and the cmd_args array when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 155-157: The Makefile target that runs when UPDATE_SNAPS is set
can call `xargs git checkout` with no input and fail; modify the conditional
block that uses `git ls-files 'features/__snapshots__/*.snap' --deleted | xargs
git checkout` so it first checks for empty output (or uses xargs's
no-run-if-empty behavior) and only invokes `git checkout` when there are paths
to restore; refer to the UPDATE_SNAPS conditional and the pipeline that pipes
ls-files into xargs to locate and update the logic.
In `@tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml`:
- Around line 321-330: The current conditional adds certificate keyless flags if
either CERTIFICATE_IDENTITY or CERTIFICATE_OIDC_ISSUER is set, which can emit
empty/partial flags and drop PUBLIC_KEY; change the logic in the block that
builds cmd_args so that you only append --certificate-identity and
--certificate-oidc-issuer to cmd_args when both CERTIFICATE_IDENTITY and
CERTIFICATE_OIDC_ISSUER are non-empty (i.e., test both with -n), otherwise fall
through to the elif that appends --public-key when PUBLIC_KEY is set; ensure you
do not append any certificate flags when one of the pair is missing to avoid
sending empty keyless flags.
---
Duplicate comments:
In `@tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml`:
- Around line 273-282: The current condition uses [ -n "${CERTIFICATE_IDENTITY}"
] || [ -n "${CERTIFICATE_OIDC_ISSUER}" ] which allows a single keyless var to
override PUBLIC_KEY and pass empty certificate flags; change the logic so both
CERTIFICATE_IDENTITY and CERTIFICATE_OIDC_ISSUER must be non-empty (use logical
AND) before appending --certificate-identity/--certificate-oidc-issuer to
cmd_args, otherwise fall back to the existing PUBLIC_KEY branch (and ensure you
don't append empty flags to cmd_args); reference the variables
CERTIFICATE_IDENTITY, CERTIFICATE_OIDC_ISSUER, PUBLIC_KEY and the cmd_args array
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 523414f1-a668-4951-952d-0195f55dcb60
📒 Files selected for processing (3)
Makefiletasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yamltasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
Comment on lines
+155
to
+157
| @if [ -n "$$UPDATE_SNAPS" ]; then \ | ||
| git ls-files 'features/__snapshots__/*.snap' --deleted | xargs git checkout; \ | ||
| fi |
There was a problem hiding this comment.
Avoid failing feature_% when there are no deleted snapshots to restore.
At Line 156, xargs git checkout can run git checkout with no paths, which fails and aborts the target under -e. Guard the empty-input case.
Suggested fix
- `@if` [ -n "$$UPDATE_SNAPS" ]; then \
- git ls-files 'features/__snapshots__/*.snap' --deleted | xargs git checkout; \
- fi
+ `@if` [ -n "$$UPDATE_SNAPS" ]; then \
+ git ls-files --deleted -- 'features/__snapshots__/*.snap' | xargs -r git checkout --; \
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @if [ -n "$$UPDATE_SNAPS" ]; then \ | |
| git ls-files 'features/__snapshots__/*.snap' --deleted | xargs git checkout; \ | |
| fi | |
| `@if` [ -n "$$UPDATE_SNAPS" ]; then \ | |
| git ls-files --deleted -- 'features/__snapshots__/*.snap' | xargs -r git checkout --; \ | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 155 - 157, The Makefile target that runs when
UPDATE_SNAPS is set can call `xargs git checkout` with no input and fail; modify
the conditional block that uses `git ls-files 'features/__snapshots__/*.snap'
--deleted | xargs git checkout` so it first checks for empty output (or uses
xargs's no-run-if-empty behavior) and only invokes `git checkout` when there are
paths to restore; refer to the UPDATE_SNAPS conditional and the pipeline that
pipes ls-files into xargs to locate and update the logic.
Comment on lines
+321
to
+330
| if [ -n "${CERTIFICATE_IDENTITY}" ] || [ -n "${CERTIFICATE_OIDC_ISSUER}" ]; then | ||
| cmd_args+=( | ||
| --certificate-identity="${CERTIFICATE_IDENTITY}" | ||
| --certificate-oidc-issuer="${CERTIFICATE_OIDC_ISSUER}" | ||
| ) | ||
| elif [ -n "${PUBLIC_KEY}" ]; then | ||
| cmd_args+=( | ||
| --public-key="${PUBLIC_KEY}" | ||
| ) | ||
| fi |
There was a problem hiding this comment.
Enforce paired keyless inputs before switching away from PUBLIC_KEY.
Line 321 currently switches to keyless mode when either variable is set. With partial input, this bypasses PUBLIC_KEY and sends an empty keyless flag, which is unsafe and inconsistent with the parameter descriptions.
Suggested fix
- if [ -n "${CERTIFICATE_IDENTITY}" ] || [ -n "${CERTIFICATE_OIDC_ISSUER}" ]; then
+ if { [ -n "${CERTIFICATE_IDENTITY}" ] && [ -z "${CERTIFICATE_OIDC_ISSUER}" ]; } || \
+ { [ -z "${CERTIFICATE_IDENTITY}" ] && [ -n "${CERTIFICATE_OIDC_ISSUER}" ]; }; then
+ echo "Both CERTIFICATE_IDENTITY and CERTIFICATE_OIDC_ISSUER must be set together." >&2
+ exit 1
+ fi
+
+ if [ -n "${CERTIFICATE_IDENTITY}" ] && [ -n "${CERTIFICATE_OIDC_ISSUER}" ]; then
cmd_args+=(
--certificate-identity="${CERTIFICATE_IDENTITY}"
--certificate-oidc-issuer="${CERTIFICATE_OIDC_ISSUER}"
)🤖 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 321 - 330, The current conditional adds certificate keyless flags if
either CERTIFICATE_IDENTITY or CERTIFICATE_OIDC_ISSUER is set, which can emit
empty/partial flags and drop PUBLIC_KEY; change the logic in the block that
builds cmd_args so that you only append --certificate-identity and
--certificate-oidc-issuer to cmd_args when both CERTIFICATE_IDENTITY and
CERTIFICATE_OIDC_ISSUER are non-empty (i.e., test both with -n), otherwise fall
through to the elif that appends --public-key when PUBLIC_KEY is set; ensure you
do not append any certificate flags when one of the pair is missing to avoid
sending empty keyless flags.
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
2 participants
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.
Bring the changes from the
verify-enterprise-contracttask, used in Konflux Integration Tests, added in PR #3140, over to theverify-conforma-konflux-tatask, used in Konflux release pipeline, so we can support keyless verification in the release pipeline also.Ref: https://issues.redhat.com/browse/EC-1652