Support validating keyless signed images in v-e-c tekton task#3140
Open
simonbaird wants to merge 4 commits intoconforma:mainfrom
Open
Support validating keyless signed images in v-e-c tekton task#3140simonbaird wants to merge 4 commits intoconforma:mainfrom
simonbaird wants to merge 4 commits intoconforma:mainfrom
Conversation
Contributor
Review Summary by QodoAdd keyless signing verification support to verify-enterprise-contract task
WalkthroughsDescription• Add keyless signing verification support to verify-enterprise-contract task - New CERTIFICATE_IDENTITY and CERTIFICATE_OIDC_ISSUER parameters - Conditional logic to use keyless or public-key verification • Convert task validate step from command to script format - Enables flexible parameter handling for keyless signatures • Create test infrastructure for keyless signed images - Scripts to generate and verify test images with cosign v2 and v3 • Add acceptance tests for keyless signature verification - Tests for both cosign v2 and v3 signature formats Diagramflowchart LR
A["verify-enterprise-contract Task"] -->|"Add Parameters"| B["CERTIFICATE_IDENTITY<br/>CERTIFICATE_OIDC_ISSUER"]
A -->|"Convert to Script"| C["Flexible Parameter<br/>Handling"]
C -->|"Support"| D["Keyless Verification"]
C -->|"Support"| E["Public-Key Verification"]
F["Test Image Creation"] -->|"Generate"| G["Keyless Signed Images<br/>v2 and v3"]
G -->|"Verify in"| H["Acceptance Tests"]
File Changes1. tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
|
Contributor
Code Review by Qodo
1. Tekton script param injection
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+253
to
+300
| script: | | ||
| #!/bin/bash | ||
| set -euo pipefail | ||
|
|
||
| cmd_args=( | ||
| "validate" | ||
| "image" | ||
| "--images" "/tekton/home/snapshot.json" | ||
| "--policy" "$(params.POLICY_CONFIGURATION)" | ||
| ) | ||
|
|
||
| if [ -n "$(params.CERTIFICATE_IDENTITY)" ] && [ -n "$(params.CERTIFICATE_OIDC_ISSUER)" ]; then | ||
| cmd_args+=( | ||
| "--certificate-identity" "$(params.CERTIFICATE_IDENTITY)" | ||
| "--certificate-oidc-issuer" "$(params.CERTIFICATE_OIDC_ISSUER)" | ||
| ) | ||
| elif [ -n "$(params.PUBLIC_KEY)" ]; then | ||
| cmd_args+=( | ||
| "--public-key" "$(params.PUBLIC_KEY)" | ||
| ) | ||
| fi | ||
|
|
||
| cmd_args+=( | ||
| "--rekor-url" "$(params.REKOR_HOST)" | ||
| "--ignore-rekor=$(params.IGNORE_REKOR)" | ||
| "--workers" "$(params.WORKERS)" | ||
| # NOTE: The syntax below is required to negate boolean parameters | ||
| "--info=$(params.INFO)" | ||
| # Fresh versions of ec support "--timeout=0" to indicate no timeout, but this would break | ||
| # the task if it's used with an older version of ec. In an abundance of caution, let's set | ||
| # an arbitrary high value instead of using 0 here. In future we can change it to 0. | ||
| # (The reason to not use an explicit timeout for ec is so Tekton can handle the timeouts). | ||
| "--timeout=100h" | ||
| "--strict=false" | ||
| "--show-successes" | ||
| "--effective-time=$(params.EFFECTIVE_TIME)" | ||
| "--extra-rule-data=$(params.EXTRA_RULE_DATA)" | ||
| "--retry-max-wait" "$(params.RETRY_MAX_WAIT)" | ||
| "--retry-max-retry" "$(params.RETRY_MAX_RETRY)" | ||
| "--retry-duration" "$(params.RETRY_DURATION)" | ||
| "--retry-factor" "$(params.RETRY_FACTOR)" | ||
| "--retry-jitter" "$(params.RETRY_JITTER)" | ||
| "--output" "text=$(params.HOMEDIR)/text-report.txt?show-successes=false" | ||
| "--output" "appstudio=$(results.TEST_OUTPUT.path)" | ||
| "--output" "json=$(params.HOMEDIR)/report-json.json" | ||
| ) | ||
|
|
||
| exec ec "${cmd_args[@]}" |
Contributor
There was a problem hiding this comment.
1. Tekton script param injection 🐞 Bug ⛨ Security
The validate step now inlines $(params.*) into a bash script; parameter values containing quotes/newlines (e.g., JSON policy configs) can break the script or be exploited for shell injection, and unquoted --flag=$(params.X) entries can be word-split by bash.
Agent Prompt
### Issue description
The `validate` step uses a Tekton `script:` that directly interpolates `$(params.*)` into bash. If a param contains quotes/newlines (notably `POLICY_CONFIGURATION` when supplied as JSON), it can break the script or allow shell injection. Several `--flag=$(params.X)` arguments are also unquoted and can be word-split.
### Issue Context
This repo already documents this exact problem and solves it in `verify-conforma-konflux-ta` by passing `POLICY_CONFIGURATION` via environment variables.
### Fix Focus Areas
- tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml[253-310]
### Suggested approach
- Add env vars for values used in the script (at least `POLICY_CONFIGURATION`, `PUBLIC_KEY`, `CERTIFICATE_IDENTITY`, `CERTIFICATE_OIDC_ISSUER`, and any other user-controlled strings).
- In the script, reference variables (e.g. `"$POLICY_CONFIGURATION"`) and ensure any `--flag=value` entries are wrapped/constructed safely (e.g. `"--extra-rule-data=${EXTRA_RULE_DATA}"`).
- Avoid placing raw `$(params.X)` inside the script body wherever possible.
ⓘ 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:
|
simonbaird
force-pushed
the
task-keyless-support
branch
from
3f4a73d to
0a2662b
Compare
simonbaird
changed the title
Member
Author
|
I added a commit to address Qodo's "action required" about the env vars vs params. The other two suggestions I think I'm okay with leaving as-is. The partial oidc values I want to handle in ec rather than handle in bash. For the "tests use canned image", it's explain in comments, and I'll file a new Jira to track improving this. |
simonbaird
force-pushed
the
task-keyless-support
branch
from
0a2662b to
94691c7
Compare
simonbaird
commented
Mar 2, 2026
Comment on lines
+251
to
+282
| "signatures": [ | ||
| { | ||
| "keyid": "", | ||
| "sig": "" | ||
| }, | ||
| { | ||
| "keyid": "", | ||
| "sig": "" | ||
| } | ||
| ], | ||
| "attestations": [ | ||
| { | ||
| "type": "https://in-toto.io/Statement/v0.1", | ||
| "predicateType": "https://slsa.dev/provenance/v1", | ||
| "signatures": [ | ||
| { | ||
| "keyid": "", | ||
| "sig": "MEQCIDj5l7I0bPCua+H1ZfAAUnd4Hd4k7wUUEi/lpWYSLkOFAiBGgK9KWiNR1t+C4TbmkU/vnpHonmg5hNnwLRC70xc2Rg==" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "type": "https://in-toto.io/Statement/v0.1", | ||
| "predicateType": "https://sigstore.dev/cosign/sign/v1", | ||
| "signatures": [ | ||
| { | ||
| "keyid": "", | ||
| "sig": "MEUCIBZc+dmgTn8SCx30h9yvCOjsBwj1+aZX0gW53c7TeyuSAiEAp4zWGNHMrjql9NFl/fCmFXnJkgDkOqbN5n7H7mw6aqI=" | ||
| } | ||
| ] | ||
| } | ||
| ] |
Member
Author
There was a problem hiding this comment.
There's some unexpected output here, but I want to fix it in a future PR.
What's happening is that in the bundle, both the image sig and the attestation are considered attestations. So we see both of them. Similarly, both the image sig and the attestation have signatures, so we see both signatures in the output.
simonbaird
commented
Mar 2, 2026
| }, | ||
| { | ||
| "keyid": "", | ||
| "sig": "" |
Member
Author
There was a problem hiding this comment.
Missing sig value will be fixed in a future PR also.
I'm doing this first in its own separate commit because it will make the next commit easier to understand and review. There's no functional change happening, we're just defining the task step in a different way. Note: - There are some clear inconsistencies in the way we use `--arg=value` or `--arg value` but I'm keeping all that as-is, since I want this change to be as small as possible. - For the same reason I'm keeping the commentary, and the timeout param, which I think could be removed now. Not touching it. - I'm focussing on just the verify-enterprise-contract task only here. My plan is to get that working first, then tackle the other tasks. Ref: https://issues.redhat.com/browse/EC-1652 Co-authored-by: Claude Code <[email protected]>
We're keeping the new param handling bash as light as possible. My thinking is that ec should produce a good enough error if the params are wrong or incorrectly specified, so we don't want or need a layer messy bash to check that they are correct. Ref: https://issues.redhat.com/browse/EC-1652 Co-authored-by: Claude Code <[email protected]>
As per the comments, I'm creating an image that we can use with the acceptance tests. Unlike the "golden-container" it wasn't built in Konflux and hence it doesn't have a realistic Chains style attestation. But it does have a keylessly sig and an a valid attestation. And actually there are two images, one created with cosign v2, and one with cosign v3. This should be useful since we need to support the new sigstore bundle introduced in cosign v3. The verify.sh isn't important for anything, but I want to check it in anyhow, since it serves as a nice instruction on how to verify the keyless signed images and attestations, and it's useful to sanity check the images when/if they need recreating. Ref: https://issues.redhat.com/browse/EC-1652
Ref: https://issues.redhat.com/browse/EC-1652 Co-authored-by: Claude Code <[email protected]>
simonbaird
force-pushed
the
task-keyless-support
branch
from
94691c7 to
125d6d0
Compare
Member
Author
|
/review |
Contributor
|
Persistent review updated to latest commit 125d6d0 |
Member
Author
|
I think the acceptance test fail is spurious since it passes locally. |
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.
See commit messages for explanations.
Ref: https://issues.redhat.com/browse/EC-1652