Bump cosign v2.4.1 to v3.0.4 and use trusted_root.json for verification#3108
Bump cosign v2.4.1 to v3.0.4 and use trusted_root.json for verification#3108simonbaird merged 1 commit intoconforma:mainfrom
Conversation
Review Summary by QodoUpgrade cosign to v3.0.4 and implement TrustedRoot.json verification
WalkthroughsDescription• Upgrade cosign from v2.4.1 to v3.0.4 with TrustedRoot.json support • Implement TUF-based verification using cosign.TrustedRoot() with fallback • Adapt to breaking API changes in cosign v3 (LoadPrivateKey, WriteSignedImageIndexImages, fake.NewClientset) • Update Go from 1.24.6 to 1.25.3 and conftest v0.57.0 to v0.66.0 • Fix acceptance test regex for "Effective-time is honored" scenario • Update OPA/conftest documentation to reflect new help text Diagramflowchart LR
A["cosign v2.4.1"] -->|upgrade| B["cosign v3.0.4"]
B -->|enables| C["TrustedRoot.json support"]
C -->|provides| D["TUF-based verification"]
D -->|with fallback| E["Individual Fulcio/CTLog/Rekor targets"]
F["Breaking API changes"] -->|adapt| G["LoadPrivateKey, WriteSignedImageIndexImages, fake.NewClientset"]
H["Go 1.24.6"] -->|upgrade| I["Go 1.25.3"]
J["conftest v0.57.0"] -->|upgrade| K["conftest v0.66.0"]
File Changes1. internal/policy/policy.go
|
Code Review by Qodo
1. Empty dir arg to cosignRemote
|
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
FYI we don't consider the Codecov failure to be blocking, so this is potentially ready to merge. Also, I filed a Jira just so we can have this visible in our RH scrum board. |
| } else { | ||
| log.Debugf("Could not fetch trusted_root.json from TUF, falling back to individual targets: %v", trErr) | ||
| } | ||
| } |
There was a problem hiding this comment.
This might help debugging one day:
| } | |
| } else { | |
| log.Debugf("Found Sigstore env overrides. Ignoring any trusted root from TUF.") | |
| } |
Just an idea, not a blocker.
| } | ||
| if opts.CTLogPubKeys, err = cosign.GetCTLogPubs(ctx); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Would a debug log here be useful also? The removed line 440 isn't very good, but maybe we could log something inside this block.
| } | |
| } | |
| log.Debug("Loaded roots, intermediates and CT log public keys") |
Maybe you can think of a better log message.
| return nil, err | ||
| } | ||
| } | ||
| log.Debug("Retrieved Rekor public keys") |
There was a problem hiding this comment.
Same thought here - perhaps this line can be kept and moved to after line 475 inside the if block.
| return &opts, nil | ||
| } | ||
|
|
||
| func hasSigstoreEnvOverrides() bool { |
There was a problem hiding this comment.
It's fairly clear already, but maybe a little comment will help future readers, e.g.:
| func hasSigstoreEnvOverrides() bool { | |
| // If any of these env vars are found, we'll assume the user wants to use | |
| // them instead of the TUF root values | |
| func hasSigstoreEnvOverrides() bool { |
(Again, feel free to improve my wording, I might be explaining it poorly or using the wrong terminology.)
simonbaird
left a comment
simonbaird
left a comment
There was a problem hiding this comment.
I had a couple of minor debug log suggestions, but no serious blockers. Nice work!
Ps, if you want to make the PR tidier, you could rebase it on a fresh upstream/main which would remove the need for @st3penta 's merge commit.
| os.Getenv("SIGSTORE_REKOR_PUBLIC_KEY") != "" || | ||
| os.Getenv("SIGSTORE_TSA_CERTIFICATE_FILE") != "" | ||
| } | ||
|
|
There was a problem hiding this comment.
I did say we don't care much about the CodeCov complaint, but actually perhaps you could add some lazy unit testing for this function and get it above the threshold.
| if trustedRoot, trErr := cosign.TrustedRoot(); trErr == nil { | ||
| log.Debug("Using trusted root from TUF for verification") | ||
| opts.TrustedMaterial = trustedRoot | ||
| } else { |
There was a problem hiding this comment.
I'm thinking about test coverage again - do you think it would it be possible to add some additional test cases in internal/policy/policy_test.go to cover these new code paths?
There was a problem hiding this comment.
I don't think it's a blocker for merging this PR, just wondering if you think it's doable.
There was a problem hiding this comment.
Fixed all review comments, improved coverage too will just need actions to run to see if there is anything else to clean up
|
Looks like #2852 is the source of the new merge conflict. |
|
Thanks for the revisions, looks good. I might try to get the conflicts resolved and merge today to help avoid another round of mod file conflicts next week. |
Upgrade from cosign/v2 to cosign/v3 so that `ec sigstore initialize` produces the modern sigstore TUF cache format (trusted_root.json + signing_config) used by current cosign and gitsign. In the keyless verification path, ec now loads TrustedMaterial from trusted_root.json via cosign.TrustedRoot() before falling back to fetching individual Fulcio/CTLog/Rekor targets. This unifies the TUF library used for both initialization and verification. Dependency changes: - cosign v2.4.1 -> v3.0.4 - conftest v0.57.0 -> v0.66.0 (required for tablewriter v1 compat) - Go 1.24.6 -> 1.25.3 (required by conftest v0.66.0) Adapt to breaking API changes: - cosign.LoadPrivateKey now takes an additional *[]signature.LoadOption - cosignRemote.WriteSignedImageIndexImages now takes a directory param - fake.NewSimpleClientset replaced with fake.NewClientset Fix pre-existing acceptance test bug where the "Effective-time is honored" scenario used embedded quotes that the step regex could not match. Update OPA snapshot and generated docs to reflect new OPA/conftest help text. Signed-off-by: SequeI <asiek@redhat.com>
|
If it goes green, I'm planning to hit merge. |
|
Ooops.... I resolved conflicts before seeing your messages, my bad |
|
Should be fine though once all green |
|
/ok-to-test |
|
🟢 🚀 |
3 participants
Upgrade from cosign/v2 to cosign/v3 so that
ec sigstore initializeproduces the modern sigstore TUF cache format (trusted_root.json + signing_config) used by current cosign and gitsign.In the keyless verification path, ec now loads TrustedMaterial from trusted_root.json via cosign.TrustedRoot() before falling back to fetching individual Fulcio/CTLog/Rekor targets. This unifies the TUF library used for both initialization and verification.
Dependency changes:
Adapt to breaking API changes:
Fix pre-existing acceptance test bug where the "Effective-time is honored" scenario used embedded quotes that the step regex could not match.
Update OPA snapshot and generated docs to reflect new OPA/conftest help text.
Closes #3107