-
Notifications
You must be signed in to change notification settings - Fork 669
Updating sign-blob to also support signing with a certificate #4547
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Zach Steindler <steiza@github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4547 +/- ##
==========================================
- Coverage 40.10% 36.36% -3.75%
==========================================
Files 155 220 +65
Lines 10044 12290 +2246
==========================================
+ Hits 4028 4469 +441
- Misses 5530 7129 +1599
- Partials 486 692 +206 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| if !shouldUpload { | ||
| // TODO - this does not take ko.SigningConfig into account |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this didn't take ko.SigningConfig into account before, but now at least it doesn't prompt you with the immutable records notice when your signing config doesn't include sending content to Rekor.
Signed-off-by: Zach Steindler <steiza@github.com>
cmd/cosign/cli/sign/sign_blob.go
Outdated
| return nil, fmt.Errorf("getting signer: %w", err) | ||
| } | ||
| defer closeSV() | ||
| hashFunction := protoHashAlgoToHash(keypair.GetHashAlgorithm()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be moved up to above line 80 and used in getPayload
Signed-off-by: Zach Steindler <steiza@github.com>
Summary
A follow-on to #4525 to support user-managed certificate signing in
sign-blob.This ended up being trickier than I expected due to the forking codepaths for using signing config vs not using signing config. We might want to consider more cleanup and consolidation in the future.
I ran through a lot of
sign-blobtests locally, without using signing config and using signing config to cover both code branches (verify commands are only given once):Note that here
sign-blobwith--signing-configrequires--trusted-root, whereas--use-signing-config=falsedoes not, because of the forking codepaths:Don't forget here to do
cosign signing-config createwith both--fulcioand--oidc-provider:Release Note
--certificateand--certificate-chaintocosign sign-blobto sign with user-managed certificatesDocumentation
N/A