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

Update warning when users sign images by tag. #2313

Merged
merged 3 commits into from Oct 14, 2022

Conversation

znewman01
Copy link
Contributor

See #2047.

Summary

Signing by tag can lead to race conditions and possibly insecure behavior, signing images other than those intended. This PR adds a warning when people do that (later, we'll make it an error.)

Release Note

Added warning when users refer to images to sign by tag (example.com/image:latest). Instead, refer to images by digest (example.com/image@sha256:abcdef). This will be required in a future release.

Documentation

No change to web docs; changes to CLI docs included.

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2022

Codecov Report

Merging #2313 (3b6ec1b) into main (dc40467) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2313      +/-   ##
==========================================
- Coverage   30.06%   30.05%   -0.02%     
==========================================
  Files         131      131              
  Lines        8062     8066       +4     
==========================================
  Hits         2424     2424              
- Misses       5314     5318       +4     
  Partials      324      324              
Impacted Files Coverage Δ
cmd/cosign/cli/sign.go 0.00% <0.00%> (ø)
cmd/cosign/cli/sign/sign.go 14.44% <0.00%> (ø)

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

priyawadhwa
priyawadhwa previously approved these changes Oct 5, 2022
@znewman01
Copy link
Contributor Author

See #2314 for lint. Should be fixed once that's merged and I rebase

hectorj2f
hectorj2f previously approved these changes Oct 13, 2022
Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

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

LGTM, I'm wondering if we should add a NOTE in the README, so any newbies are aware of this security recommendation.

@znewman01
Copy link
Contributor Author

Newest change covers a lot of docs, and also adds some tests.

hectorj2f
hectorj2f previously approved these changes Oct 13, 2022
@cpanato
Copy link
Member

cpanato commented Oct 13, 2022

will close/reopen to see if that retriggers the ci, otherwise we will need some rebase

@cpanato cpanato closed this Oct 13, 2022
@cpanato cpanato reopened this Oct 13, 2022
cpanato
cpanato previously approved these changes Oct 13, 2022
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.

thank you

znewman01 and others added 3 commits October 13, 2022 12:56
See sigstore#2047.

Signed-off-by: Zachary Newman <zjn@chainguard.dev>
Signed-off-by: Zachary Newman <zjn@chainguard.dev>
Also explicitly check for Digest being set, rather than Tag not being set. This
doesn't actually make a difference because name.ParseReference just throws away
the tag in such cases (maybe a bug), but it does make the intent clearer.

Signed-off-by: Zachary Newman <zjn@chainguard.dev>
@znewman01
Copy link
Contributor Author

had to trigger tests 1 more time because the DCO check hung....fingers crossed!

@znewman01
Copy link
Contributor Author

no substantive changes since hector last approved

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.

LGTM

thanks!

Copy link
Contributor

@hectorj2f hectorj2f 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 💪

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

5 participants