-
Notifications
You must be signed in to change notification settings - Fork 547
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
bundle support for sign-blob #3138
Conversation
Signed-off-by: Ceridwen Coghlan <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #3138 +/- ##
==========================================
- Coverage 30.56% 30.38% -0.18%
==========================================
Files 155 155
Lines 9801 9859 +58
==========================================
Hits 2996 2996
- Misses 6359 6417 +58
Partials 446 446
|
Signed-off-by: Ceridwen Coghlan <[email protected]>
Ref #3139 |
Signed-off-by: Ceridwen Coghlan <[email protected]>
@@ -71,6 +72,9 @@ func (o *SignBlobOptions) AddFlags(cmd *cobra.Command) { | |||
"write everything required to verify the blob to a FILE") | |||
_ = cmd.Flags().SetAnnotation("bundle", cobra.BashCompFilenameExt, []string{}) | |||
|
|||
cmd.Flags().BoolVar(&o.UsePBBundleFormat, "use-new-bundle-format", false, |
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.
Can we add a TODO to eventually make this flag a no-op and add a deprecated-format flag?
if err != nil { | ||
return nil, fmt.Errorf("error getting signer: %w", err) | ||
} | ||
certChain, err := protobundle.GenerateX509CertificateChain(certBytes) |
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.
A recent decision was to have this only be the non-trusted certificates, and any CA certs that appear in the root of trust should not be included - https://github.com/sigstore/protobuf-specs/blob/main/protos/sigstore_common.proto#L171-L188
We can either:
- Load in just the leaf certificate and add a TODO to handle private deployments
- Just cut out the root and include leaves and intermediates
- Cut out any CA certs that are returned from these two funcs - https://github.com/sigstore/cosign/blob/main/internal/pkg/cosign/fulcio/fulcioroots/fulcioroots.go#L37-L57
return nil, err | ||
} | ||
signedPayload.Cert = base64.StdEncoding.EncodeToString(certBytes) | ||
certBytes, err := sv.Bytes(ctx) |
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.
We'll also need to handle where sv.Bytes()
returns a key, since that goes into a different message. We could change sv.Bytes()
to return whether it's a key or cert, or try to parse the return value to determine if it's a key or cert.
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
This PR was closed because it has been stalled for 10 days with no activity. |
Summary
Adds support for the new sigstore bundle format defined in github.com/sigstore/protobuf-specs to
sign-blob
. When the--bundle
flag is provided with an output file, users can now specify--use-new-bundle-format=true
to use the new common format. This flag defaults to false.#3139