Skip keyless image signature checks in tekton tasks#3191
Skip keyless image signature checks in tekton tasks#3191simonbaird wants to merge 1 commit intoconforma:mainfrom
Conversation
As explained in the comments, the reason for this is we're expecting
that we need to use different identities to verify the image vs the
attestation, and currently it's not possible.
For the record, I think for the image sig check we'll need this:
--certificate-oidc-identity-regexp="^https://kubernetes.io/namespaces/[a-z0-9-]+-tenant/serviceaccounts/build-pipeline-[a-z0-9-]+$"
and for the attestation sig check we'll need this:
--certificate-oidc-identity="https://kubernetes.io/namespaces/openshift-pipelines/serviceaccounts/tekton-chains-controller"
but it's not easy to confirm that right now without a working
cluster, so we'll aim to confirm it in future.
Ref: https://redhat.atlassian.net/browse/EC-1647
Review Summary by QodoSkip keyless image signature checks in Tekton verification tasks
WalkthroughsDescription• Skip keyless image signature checks in Tekton tasks • Add --skip-image-sig-check=true flag to Conforma verification • Workaround for different signing identities between image and attestation • Improve code formatting with better comment placement Diagramflowchart LR
A["Tekton Tasks"] -->|Add flag| B["--skip-image-sig-check=true"]
B -->|Skip| C["Image Signature Check"]
B -->|Verify| D["Attestation Signature Only"]
D -->|Use| E["Certificate OIDC Parameters"]
File Changes1. tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml
|
Code Review by Qodo
1. Image signature verification disabled
|
📝 WalkthroughWalkthroughTwo Tekton task YAML files are modified to add Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
![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`:
- Around line 375-384: Remove the unconditional --skip-image-sig-check=true flag
and instead add an explicit task parameter (e.g., params.skipImageSigCheck,
default false) to the task definition, then conditionally append the
--skip-image-sig-check=true argument to the Conforma step args only when
params.skipImageSigCheck is true (so callers must opt in for reduced
validation); reference the existing --skip-image-sig-check flag and the task
params block when making the change.
In `@tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml`:
- Around line 320-329: The task unconditionally sets --skip-image-sig-check=true
for keyless signing, weakening security; update verify-enterprise-contract.yaml
to make image signature skipping explicit and opt-in: introduce a task parameter
(e.g., skipImageSigCheck or allowKeylessImageSkip) defaulting to false, wire
that parameter into the command generation instead of hardcoding
--skip-image-sig-check=true, and if keyless mode is detected without the opt-in
parameter set, fail-closed (return an error/exit) to force explicit
authorization; ensure you modify the code that emits the --skip-image-sig-check
flag so it only appears when the new parameter is true and keep the explanatory
comment near the flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cadb290c-c845-4e88-aa06-73d4c0feb900
⛔ Files ignored due to path filters (1)
features/__snapshots__/task_validate_image.snapis excluded by!**/*.snap
📒 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
| # Force --ignore-rekor to false since we need rekor | ||
| --ignore-rekor=false | ||
| # Conforma expects that the attestation and the image are signed in the | ||
| # same way. But actually for Tekton Chains doing keyless signing that's | ||
| # not the cases. As a workaround, skip the image sig check and only do the | ||
| # attestation sig check. This means we're expecting the --certificate-* | ||
| # params provided to be applicable for verifying the attestation signature. | ||
| --skip-image-sig-check=true | ||
| ) |
There was a problem hiding this comment.
Avoid unconditional --skip-image-sig-check in keyless mode.
Line 383 applies the same implicit weakening of verification guarantees: any keyless run skips image signature checks. This should be controlled by an explicit task param (default false) so callers intentionally opt into this reduced validation mode.
As per coding guidelines, **: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 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 375 - 384, Remove the unconditional --skip-image-sig-check=true flag and
instead add an explicit task parameter (e.g., params.skipImageSigCheck, default
false) to the task definition, then conditionally append the
--skip-image-sig-check=true argument to the Conforma step args only when
params.skipImageSigCheck is true (so callers must opt in for reduced
validation); reference the existing --skip-image-sig-check flag and the task
params block when making the change.
There was a problem hiding this comment.
It's a reasonable suggestion, but I don't want to add that right now. I know the only use case for this task is with Tekton Chains produced signatures, and I know (or at least I'm fairly certain) we'll have to skip the image sig check for those.
I don't want to force consumers of these tasks (i.e. the Konflux release pipeline) to have to introduce a new param to make things work. I like the idea of making "default to more secure", but for now we're preferring "default to most likely to work".
There was a problem hiding this comment.
@simonbaird, understood — the trade-off makes sense given the fixed use case. Deferring to the PR author here.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
| # Force --ignore-rekor to false since we need rekor | ||
| --ignore-rekor=false | ||
|
|
||
| # Conforma expects that the attestation and the image are signed in the | ||
| # same way. But actually for Tekton Chains doing keyless signing that's | ||
| # not the cases. As a workaround, skip the image sig check and only do the | ||
| # attestation sig check. This means we're expecting the --certificate-* | ||
| # params provided to be applicable for verifying the attestation signature. | ||
| --skip-image-sig-check=true | ||
| ) |
There was a problem hiding this comment.
Make image-signature skipping explicit instead of unconditional.
In Line 328, keyless mode now always disables image signature verification. That’s a security guarantee downgrade for all existing keyless users and should be an explicit opt-in task parameter (or a fail-closed path) rather than implicit behavior.
Suggested hardening (opt-in switch)
+ - name: SKIP_IMAGE_SIG_CHECK
+ type: string
+ description: >-
+ Allow keyless verification without image signature verification.
+ Use only when image and attestation are expected to use different identities.
+ default: "false"
...
- cmd_args+=(
- # Force --ignore-rekor to false since we need rekor
- --ignore-rekor=false
-
- # Conforma expects that the attestation and the image are signed in the
- # same way. But actually for Tekton Chains doing keyless signing that's
- # not the cases. As a workaround, skip the image sig check and only do the
- # attestation sig check. This means we're expecting the --certificate-*
- # params provided to be applicable for verifying the attestation signature.
- --skip-image-sig-check=true
- )
+ cmd_args+=(
+ # Force --ignore-rekor to false since we need rekor
+ --ignore-rekor=false
+ )
+ if [[ "${SKIP_IMAGE_SIG_CHECK}" == "true" ]]; then
+ cmd_args+=(
+ --skip-image-sig-check=true
+ )
+ fi
...
+ - name: SKIP_IMAGE_SIG_CHECK
+ value: "$(params.SKIP_IMAGE_SIG_CHECK)"As per coding guidelines, **: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 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 320 - 329, The task unconditionally sets --skip-image-sig-check=true for
keyless signing, weakening security; update verify-enterprise-contract.yaml to
make image signature skipping explicit and opt-in: introduce a task parameter
(e.g., skipImageSigCheck or allowKeylessImageSkip) defaulting to false, wire
that parameter into the command generation instead of hardcoding
--skip-image-sig-check=true, and if keyless mode is detected without the opt-in
parameter set, fail-closed (return an error/exit) to force explicit
authorization; ensure you modify the code that emits the --skip-image-sig-check
flag so it only appears when the new parameter is true and keep the explanatory
comment near the flag.
There was a problem hiding this comment.
Same response as for the other similar suggestion, though honestly it would be easier here, since we own the integration test pipeline where this is used. Still, I'd rather be consistent with the other task.
There was a problem hiding this comment.
@simonbaird, understood — consistency between the two tasks is a reasonable priority. I'll defer to your judgment here.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
| --ignore-rekor=false | ||
| # Conforma expects that the attestation and the image are signed in the | ||
| # same way. But actually for Tekton Chains doing keyless signing that's |
There was a problem hiding this comment.
This is not entirely true. The same identity is used for the image signature and attestation signature. The change that we're pursuing in Konflux is that we want to make Chains stop signing the image altogether and instead sign the image from the build pipeline itself (tektoncd/chains#1346). That is unrelated to keyless. We could achieve the same with long-lived keys - keyless just makes this much more feasible.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # not the cases. As a workaround, skip the image sig check and only do the | ||
| # attestation sig check. This means we're expecting the --certificate-* | ||
| # params provided to be applicable for verifying the attestation signature. | ||
| --skip-image-sig-check=true |
There was a problem hiding this comment.
1. Image signature verification disabled 🐞 Bug ⛨ Security
Both tasks now unconditionally add --skip-image-sig-check=true whenever any keyless CERTIFICATE_* param is set, disabling image signature validation and allowing images with invalid/missing signatures to pass as long as attestation verification succeeds.
Agent Prompt
### Issue description
The Tekton tasks currently add `--skip-image-sig-check=true` automatically for any keyless verification configuration, which disables image signature validation in a broad, implicit way.
### Issue Context
The CLI supports `--skip-image-sig-check` and it bypasses image signature validation (`ValidateImageSignature`). Repo tests show this can make validation succeed even when the image signature is invalid.
### How to fix
- Add a **new task param** (e.g., `SKIP_IMAGE_SIG_CHECK`, default `"false"`).
- Only append `--skip-image-sig-check` when that param is true (and ideally emit a clear log line warning about the security tradeoff).
- Update both task READMEs to document the param and the security implication.
### Fix Focus Areas
- tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml[292-336]
- tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml[344-391]
- tasks/verify-enterprise-contract/0.1/README.md[1-60]
- tasks/verify-conforma-konflux-ta/0.1/README.md[1-80]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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:
|
|
I think I was wrong about the purpose of buildIdentityRegexp so my long comments are incorrect. Moving to draft. |
2 participants
As explained in the comments, the reason for this is we're expecting that we need to use different identities to verify the image vs the attestation, and currently it's not possible.
For the record, I think for the image sig check we'll need this:
and for the attestation sig check we'll need this:
but it's not easy to confirm that right now without a working cluster, so we'll aim to confirm it in future.
Ref: https://redhat.atlassian.net/browse/EC-1647