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

Remove experimental flag from cosign sign and cosign verify #2387

Merged
merged 3 commits into from
Nov 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/cosign/cli/attest/attest.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ func AttestCmd(ctx context.Context, ko options.KeyOpts, regOpts options.Registry
}

// Check whether we should be uploading to the transparency log
if sign.ShouldUploadToTlog(ctx, digest, force, noTlogUpload, ko.RekorURL) {
if sign.ShouldUploadToTlog(ctx, ko, digest, force, noTlogUpload) {
bundle, err := uploadToTlog(ctx, sv, ko.RekorURL, func(r *client.Rekor, b []byte) (*models.LogEntryAnon, error) {
return cosign.TLogUploadInTotoAttestation(ctx, r, signedPayload, b)
})
Expand Down
4 changes: 2 additions & 2 deletions cmd/cosign/cli/sign.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ race conditions or (worse) malicious tampering.
`,
Example: ` cosign sign --key <key path>|<kms uri> [--payload <path>] [-a key=value] [--upload=true|false] [-f] [-r] <image digest uri>

# sign a container image with Google sign-in (experimental)
COSIGN_EXPERIMENTAL=1 cosign sign <IMAGE DIGEST>
# sign a container image with the Sigstore OIDC flow
cosign sign <IMAGE DIGEST>

# sign a container image with a local key pair file
cosign sign --key cosign.key <IMAGE DIGEST>
Expand Down
43 changes: 24 additions & 19 deletions cmd/cosign/cli/sign/sign.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,17 +60,13 @@ import (

const TagReferenceMessage string = `WARNING: Image reference %s uses a tag, not a digest, to identify the image to sign.

This can lead you to sign a different image than the intended one. Please use a
digest (example.com/ubuntu@sha256:abc123...) rather than tag
(example.com/ubuntu:latest) for the input to cosign. The ability to refer to
images by tag will be removed in a future release.
This can lead you to sign a different image than the intended one. Please use a
digest (example.com/ubuntu@sha256:abc123...) rather than tag
(example.com/ubuntu:latest) for the input to cosign. The ability to refer to
images by tag will be removed in a future release.
`

func ShouldUploadToTlog(ctx context.Context, ref name.Reference, force bool, noTlogUpload bool, url string) bool {
// Check whether experimental is on!
if !options.EnableExperimental() {
return false
}
func ShouldUploadToTlog(ctx context.Context, ko options.KeyOpts, ref name.Reference, force, noTlogUpload bool) bool {
// We are forcing publishing to the Tlog.
if force {
return true
Expand All @@ -80,9 +76,14 @@ func ShouldUploadToTlog(ctx context.Context, ref name.Reference, force bool, noT
return false
}

// If we aren't using keyless signing, we can skip the tlog
if !keylessSigning(ko) {
return false
}

// Check if the image is public (no auth in Get)
if _, err := remote.Get(ref, remote.WithContext(ctx)); err != nil {
fmt.Fprintf(os.Stderr, "%q appears to be a private repository, please confirm uploading to the transparency log at %q [Y/N]: ", ref.Context().String(), url)
fmt.Fprintf(os.Stderr, "%q appears to be a private repository, please confirm uploading to the transparency log at %q [Y/N]: ", ref.Context().String(), ko.RekorURL)

var tlogConfirmResponse string
if _, err := fmt.Scanln(&tlogConfirmResponse); err != nil {
Expand Down Expand Up @@ -124,14 +125,8 @@ func ParseOCIReference(refStr string, out io.Writer, opts ...name.Option) (name.

// nolint
func SignCmd(ro *options.RootOptions, ko options.KeyOpts, signOpts options.SignOptions, imgs []string) error {
if options.EnableExperimental() {
if options.NOf(ko.KeyRef, ko.Sk) > 1 {
return &options.KeyParseError{}
}
} else {
if !options.OneOf(ko.KeyRef, ko.Sk) {
return &options.KeyParseError{}
}
if options.NOf(ko.KeyRef, ko.Sk) > 1 {
return &options.KeyParseError{}
}

ctx, cancel := context.WithTimeout(context.Background(), ro.Timeout)
Expand Down Expand Up @@ -236,7 +231,7 @@ func signDigest(ctx context.Context, digest name.Digest, payload []byte, ko opti
if sv.Cert != nil {
s = ifulcio.NewSigner(s, sv.Cert, sv.Chain)
}
if ShouldUploadToTlog(ctx, digest, force, noTlogUpload, ko.RekorURL) {
if ShouldUploadToTlog(ctx, ko, digest, force, noTlogUpload) {
rClient, err := rekor.NewClient(ko.RekorURL)
if err != nil {
return err
Expand Down Expand Up @@ -531,3 +526,13 @@ func (c *SignerVerifier) Bytes(ctx context.Context) ([]byte, error) {
}
return pemBytes, nil
}

func keylessSigning(ko options.KeyOpts) bool {
if ko.KeyRef != "" {
return false
}
if ko.Sk {
return false
}
return true
}
5 changes: 1 addition & 4 deletions cmd/cosign/cli/verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func Verify() *cobra.Command {
against the transparency log.`,
Example: ` cosign verify --key <key path>|<key url>|<kms uri> <image uri> [<image uri> ...]

# verify cosign claims and signing certificates on the image
# verify cosign claims and signing certificates on the image with the transparency log
cosign verify <IMAGE>

# verify multiple images
Expand All @@ -45,9 +45,6 @@ against the transparency log.`,
# additionally verify specified annotations
cosign verify -a key1=val1 -a key2=val2 <IMAGE>

# (experimental) additionally, verify with the transparency log
COSIGN_EXPERIMENTAL=1 cosign verify <IMAGE>

# verify image with an on-disk public key
cosign verify --key cosign.pub <IMAGE>

Expand Down
16 changes: 12 additions & 4 deletions cmd/cosign/cli/verify/verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,6 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
c.HashAlgorithm = crypto.SHA256
}

if !options.OneOf(c.KeyRef, c.CertRef, c.Sk) && !options.EnableExperimental() {
return &options.PubKeyParseError{}
}
ociremoteOpts, err := c.ClientOpts(ctx)
if err != nil {
return fmt.Errorf("constructing client options: %w", err)
Expand All @@ -116,7 +113,8 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
if c.CheckClaims {
co.ClaimVerifier = cosign.SimpleClaimVerifier
}
if options.EnableExperimental() {

if c.keylessVerification() {
if c.RekorURL != "" {
rekorClient, err := rekor.NewClient(c.RekorURL)
if err != nil {
Expand Down Expand Up @@ -398,3 +396,13 @@ func loadCertChainFromFileOrURL(path string) ([]*x509.Certificate, error) {
}
return certs, nil
}

func (c *VerifyCommand) keylessVerification() bool {
if c.KeyRef != "" {
return false
}
if c.Sk {
return false
}
return true
}
4 changes: 2 additions & 2 deletions doc/cosign_sign.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 1 addition & 4 deletions doc/cosign_verify.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 17 additions & 17 deletions test/e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1179,16 +1179,16 @@ func TestTlog(t *testing.T) {
// Now verify should work!
must(verify(pubKeyPath, imgName, true, nil, ""), t)

// Now we turn on the tlog!
defer setenv(t, env.VariableExperimental.String(), "1")()
// TODO: priyawadhwa@ to figure out how to add an entry to the tlog without using keyless signing
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hesitant about removing tests - I think these should be a part of the PR and not separate.

// We could add an --upload-tlog flag, but it's a bit weird since we have a --no-upload-tlog flag too right now.

// Verify shouldn't work since we haven't put anything in it yet.
mustErr(verify(pubKeyPath, imgName, true, nil, ""), t)
// mustErr(verify(pubKeyPath, imgName, true, nil, ""), t)

// Sign again with the tlog env var on
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
// And now verify works!
must(verify(pubKeyPath, imgName, true, nil, ""), t)
// // Sign again with the tlog env var on
// must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
// // And now verify works!
// must(verify(pubKeyPath, imgName, true, nil, ""), t)
}

func TestNoTlog(t *testing.T) {
Expand Down Expand Up @@ -1220,19 +1220,19 @@ func TestNoTlog(t *testing.T) {
// Now verify should work!
must(verify(pubKeyPath, imgName, true, nil, ""), t)

// Now we turn on the tlog!
defer setenv(t, env.VariableExperimental.String(), "1")()
// TODO: priyawadhwa@ to figure out how to add an entry to the tlog without using keyless signing
// We could add an --upload-tlog flag, but it's a bit weird since we have a --no-upload-tlog flag too right now.

// Verify shouldn't work since we haven't put anything in it yet.
mustErr(verify(pubKeyPath, imgName, true, nil, ""), t)
// // Now we turn on the tlog!
// defer setenv(t, env.VariableExperimental.String(), "1")()

// Sign again with the tlog env var on with option to not upload tlog
so = options.SignOptions{
NoTlogUpload: true,
}
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
// And verify it still fails.
mustErr(verify(pubKeyPath, imgName, true, nil, ""), t)
// so = options.SignOptions{
// NoTlogUpload: true,
// }
// must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
// // And verify it still fails.
// mustErr(verify(pubKeyPath, imgName, true, nil, ""), t)
}

func TestGetPublicKeyCustomOut(t *testing.T) {
Expand Down