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

Conversation

priyawadhwa
Copy link
Contributor

@priyawadhwa priyawadhwa commented Oct 26, 2022

part 1/n for #2255

figure there's a decent amount of refactoring to do here, but this is a good start. will have to include the other commands in follow-ups as well.

Signed-off-by: Priya Wadhwa priya@chainguard.dev

Summary

Release Note

Remove the experimental flag from cosign sign and cosign verify, make keyless signing the default

Documentation

this changes cosign behavior to automatically add all signatures to the transparency log. in the next commit i'll move the verification for this into the 'ShouldUploadTlog' function.

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>
Signed-off-by: Priya Wadhwa <priya@chainguard.dev>
@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2022

Codecov Report

Merging #2387 (4a8c396) into main (c3c4ea9) will increase coverage by 0.00%.
The diff coverage is 10.34%.

@@           Coverage Diff           @@
##             main    #2387   +/-   ##
=======================================
  Coverage   30.12%   30.13%           
=======================================
  Files         136      136           
  Lines        8451     8456    +5     
=======================================
+ Hits         2546     2548    +2     
- Misses       5574     5578    +4     
+ Partials      331      330    -1     
Impacted Files Coverage Δ
cmd/cosign/cli/sign.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify.go 0.00% <0.00%> (ø)
cmd/cosign/cli/verify/verify.go 18.92% <0.00%> (-0.35%) ⬇️
cmd/cosign/cli/sign/sign.go 16.80% <17.64%> (+0.41%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

there is a release note that we need to write for this?

@haydentherapper
Copy link
Contributor

Can we be merge this into a release branch instead of main? Otherwise, we'll need to block future cosign releases until this and all other 2.0 work is done.

@haydentherapper
Copy link
Contributor

@priyawadhwa, do you think a one-pager on this would be helpful? I noticed some TODOs, I think it'd be helpful to write up a brief doc where the community and maintainers can chime in with thoughts about flags and whatnot.

@priyawadhwa
Copy link
Contributor Author

@cpanato suggested forking off the 1.0 branch so that we can merge 2.0 changes in main. I like that proposal since I think it'll be easier than constantly rebasing a 2.0 branch (and I don't think we'll have to merge to 1.0 very often). WDYT @haydentherapper?

@haydentherapper
Copy link
Contributor

Is there much difference between forking and creating a separate branch and rebasing only once we're ready to merge in the changes? I'm good with either!

I think we should also ask that no major refactors occur in Cosign during the 2.0 work.

@priyawadhwa
Copy link
Contributor Author

Created 1.0-fork, can merge these changes in now!

@priyawadhwa priyawadhwa merged commit 1c15b03 into sigstore:main Nov 1, 2022
@priyawadhwa priyawadhwa deleted the sign-verify-experimental branch November 1, 2022 18:31
@github-actions github-actions bot added this to the v1.14.0 milestone Nov 1, 2022
@@ -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.

@dmitris
Copy link
Contributor

dmitris commented Apr 26, 2023

have we made or do we need a corresponding PR in https://github.com/sigstore/docs to update the docs on https://docs.sigstore.dev/cosign/keyless/? See also the Sigstore Slack discussion https://sigstore.slack.com/archives/C01PZKDL4DP/p1682515342798789

/cc @priyawadhwa @haydentherapper

@haydentherapper
Copy link
Contributor

Docs are being actively updated. Signing and verifying has been updated

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.

None yet

6 participants