Skip to content
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

implement a new flag for sign-blob subcommand to save certificate to disk or stdout #1016

Merged
merged 1 commit into from
Nov 19, 2021

Conversation

shibumi
Copy link
Contributor

@shibumi shibumi commented Nov 9, 2021

Summary

With cosign 1.3.0 it is not possible to store the public certificate generated by the OIDC-issuer in a convenient way.
To store the public key locally on disk, the user has to intercept the cosign stdout, parse it and store it.
This is inconvenient and error-prone. We want to make signing and public key distribution as easy as possible, hence this PR introduces a new pubkey-output flag for the sign-blob sub-command.

The pubkey-output flag respects the b64 flag and prints the certificate/key as file in a given file path.

Ticket Link

Fixes None

Release Note

The cosign cli is now able to store the public certificate/key during sign-blob operations via the `pubkey-output` flag.

@@ -76,9 +76,9 @@ func SignBlobCmd(ctx context.Context, ko KeyOpts, regOpts options.RegistryOption
return nil, errors.Wrap(err, "signing blob")
}

var rekorBytes []byte
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this will only pick up the public key when COSIGN_EXPERIMENTAL=1 is enabled. That mostly makes sense to me since otherwise you probably already have the key, but I would caveat in the doc string or somewhere else.

May also be worth adding for the sign subcommand too?

@developer-guy
Copy link
Member

we did the same in #1021

@@ -49,4 +50,7 @@ func (o *SignBlobOptions) AddFlags(cmd *cobra.Command) {

cmd.Flags().StringVar(&o.Output, "output", "",
"write the signature to FILE")

cmd.Flags().StringVar(&o.PubKeyOutput, "pubkey-output", "",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about changing this to --output-cert?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dentrax I was a little bit confused about cert and public key... because this does not need to be a certificate it can also be a pubkey, IIRC.

@asraa are you fine with changing this to --output-cert instead of -pubkey-output?

Thoughts? @dlorenc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should always be a cert for the keyless modes right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, i think that's more clear.

@shibumi
Copy link
Contributor Author

shibumi commented Nov 10, 2021

we did the same in #1021

Yes :D just realized this. I like your PR more, but I am not sure if changing output to output-signature would break cosign's API stability.

@developer-guy
Copy link
Member

we did the same in #1021

Yes :D just realized this. I like your PR more, but I am not sure if changing output to output-signature would break cosign's API stability.

Thanks, @shibumi, yes we know, but it makes more sense to us instead of using just output, WDYT to make it consistent?

@dlorenc
Copy link
Member

dlorenc commented Nov 10, 2021

Thanks, @shibumi, yes we know, but it makes more sense to us instead of using just output, WDYT to make it consistent?

I think I'm fine with this breaking change. It's pretty minor and important for usability. If we need to we could always alias it for a release in cobra.

@shibumi shibumi force-pushed the shibumi/implement-pubkey-output branch 2 times, most recently from 54280e4 to 77706ed Compare November 10, 2021 18:13
@@ -22,16 +22,18 @@ import (
)

// SignBlobOptions is the top level wrapper for the sign-blob command.
// The new output-certificate flag is only in use when COSIGN_EXPERIMENTAL is enabled
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asraa is this docstring on the right position? I thought about adding it to the SignBlob function, but the SignBlob function has a // nolint on top.. I am not sure if I can have docstrings and nolint at the same time.

@shibumi shibumi force-pushed the shibumi/implement-pubkey-output branch from 77706ed to 3eaf1e6 Compare November 10, 2021 18:15
@shibumi
Copy link
Contributor Author

shibumi commented Nov 10, 2021

@developer-guy @dlorenc done. Or do you prefer the short form for commands? Right now I chose output-certificate and output-signature. I see that @developer-guy used output-cert and output-sig. Opinions?

@Dentrax
Copy link
Member

Dentrax commented Nov 10, 2021

@developer-guy @dlorenc done. Or do you prefer the short form for commands? Right now I chose output-certificate and output-signature. I see that @developer-guy used output-cert and output-sig. Opinions?

To make it more consistent with the --signature flag, I think it would better DX to use --output-signature, and eventually we should go with the --output-certificate if we decide to use long-form of signature instead of sig.

@shibumi
Copy link
Contributor Author

shibumi commented Nov 10, 2021

Good argument @Dentrax

@developer-guy
Copy link
Member

@shibumi would you mind adding I and friend @Dentrax as a collab to the project? Maybe we can add some code to this. 🤝🙋🏻‍♂️

@developer-guy developer-guy changed the title implement pubkey-output flag for sign-blob subcommand implement new flag for sign-blob subcommand to save certificate to disk or stdout Nov 11, 2021
@developer-guy developer-guy changed the title implement new flag for sign-blob subcommand to save certificate to disk or stdout implement a new flag for sign-blob subcommand to save certificate to disk or stdout Nov 11, 2021
@shibumi
Copy link
Contributor Author

shibumi commented Nov 11, 2021

@developer-guy do you mean editor permissions to this PR? I have no idea if this is possible in Github. I think you can fork my fork and then create a PR against my fork and I could merge it in my fork :D

OR

You just drop me a patch file via mail.

What do you would like to change in this PR?

EDIT: or do you mean collab in the cosign repository? I am not involved with cosign, too :D :D

Copy link
Contributor

@caarlos0 caarlos0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@Dentrax
Copy link
Member

Dentrax commented Nov 15, 2021

With the new release of goreleaser v1.0.0, I think we can merge this PR and plan to release on the upcoming cosign version. 🙏 Should we create Release v1.4 issue additionally to keep track of the next milestone? @cpanato @dlorenc

@developer-guy
Copy link
Member

in addition to the comment above, with this, we will be able to do the following:

signs:
  - cmd: cosign
    certificate: "${artifact}.cert"
    signature: "${artifact}.sig"
    args: ["sign-blob", "--oidc-issuer=https://token.actions.githubusercontent.com", "--output-signature=${signature}", "--output-certificate=${certificate}",  "${artifact}"]

For more detail: ko-build/ko#498

@dlorenc
Copy link
Member

dlorenc commented Nov 15, 2021

This is amazing!

@caarlos0
Copy link
Contributor

caarlos0 commented Nov 15, 2021

Once this is out, I'll write a post on how to use it with goreleaser on github actions as well

@dlorenc
Copy link
Member

dlorenc commented Nov 15, 2021

Looks like this just needs docs to be regenerated!

@shibumi
Copy link
Contributor Author

shibumi commented Nov 15, 2021

@dlorenc how do I regenerate the docs?

@caarlos0
Copy link
Contributor

BTW, this is a breaking change isnt it? As --output will not exist anymore...

Maybe worth deprecating it first?

@dlorenc
Copy link
Member

dlorenc commented Nov 15, 2021

@dlorenc how do I regenerate the docs?

Looks like: go run -tags pivkey,pkcs11key,cgo ./cmd/help/

BTW, this is a breaking change isnt it? As --output will not exist anymore...

Yeah, we probably should deprecate/alias first. I misunderstood and thought this flag only applied for experimental/keyless stuff, but it's used for normal blob signatures too. Great catch @caarlos0!

@shibumi
Copy link
Contributor Author

shibumi commented Nov 15, 2021

Yeah, we probably should deprecate/alias first. I misunderstood and thought this flag only applied for experimental/keyless stuff, but it's used for normal blob signatures too. Great catch @caarlos0!

Sounds, good. Let me apply a new patch in the afternoon (that's in 2 hours in my time :D )

@shibumi shibumi force-pushed the shibumi/implement-pubkey-output branch 3 times, most recently from a1bd513 to a073b45 Compare November 15, 2021 23:03
@shibumi
Copy link
Contributor Author

shibumi commented Nov 15, 2021

  • regenerated docs
  • deprecated output flag

New behavior:

❯ COSIGN_EXPERIMENTAL=1 ./cosign sign-blob --output=main.go.sig --output-certificate=cert.pub --oidc-issuer https://oauth2.sigstore.dev/auth main.go
WARNING: the '--output' flag is deprecated and will be removed in the future. Use '--output-signature'
Using payload from: main.go
Generating ephemeral keys...
Retrieving signed certificate...
Your browser will now be opened to: ....
❯ COSIGN_EXPERIMENTAL=1 ./cosign sign-blob --output-signature=main.go.sig --output-certificate=cert.pub --oidc-issuer https://oauth2.sigstore.dev/auth main.go
Using payload from: main.go
Generating ephemeral keys...
Retrieving signed certificate...
Your browser will now be opened to: ...

@shibumi shibumi force-pushed the shibumi/implement-pubkey-output branch from a073b45 to 17ad01c Compare November 15, 2021 23:05
Copy link
Member

@developer-guy developer-guy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @shibumi amazing work 👌🙋🏻‍♂️

@shibumi
Copy link
Contributor Author

shibumi commented Nov 16, 2021

@dlorenc The generate doc command does not seem to be sufficient.. I have generated the docs with it, but the pipeline still fails.

@developer-guy
Copy link
Member

@shibumi I think you should run goimports -w ./....

This commit breaks with past behavior in favor of two new flags.
The `output` flags gets replaced with the new `output-signature` and `output-certificate` flags.

Signed-off-by: Christian Rebischke <[email protected]>
@shibumi shibumi force-pushed the shibumi/implement-pubkey-output branch from 17ad01c to e0acb0d Compare November 16, 2021 13:32
@developer-guy
Copy link
Member

@dlorenc IMHO, this PR is ready for merge.

@shibumi
Copy link
Contributor Author

shibumi commented Nov 16, 2021

@developer-guy thanks for your hint to goimports. I didn't know that exists :D

@developer-guy
Copy link
Member

@developer-guy thanks for your hint to goimports. I didn't know that exists :D

I added two new targets to the makefile: fmt and checkfmt. You can use them now 🙋🏻‍♂️

@dlorenc dlorenc merged commit 18318ba into sigstore:main Nov 19, 2021
@github-actions github-actions bot added this to the v1.4.0 milestone Nov 19, 2021
@shibumi shibumi deleted the shibumi/implement-pubkey-output branch November 19, 2021 12:15
@caarlos0
Copy link
Contributor

👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants