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

respect tlog-upload flag with TSA #2474

Merged
merged 1 commit into from
Nov 21, 2022

Conversation

hectorj2f
Copy link
Contributor

Signed-off-by: Hector Fernandez [email protected]

Summary

--tlog-upload flag was ignored when relying on time-stamping verification. We should allow disabling the tlog entry creation when relying on time-stamping.

Release Note

Documentation

@hectorj2f hectorj2f added the enhancement New feature or request label Nov 21, 2022
@hectorj2f hectorj2f self-assigned this Nov 21, 2022
@hectorj2f hectorj2f added the bug Something isn't working label Nov 21, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2022

Codecov Report

Merging #2474 (705d8a1) into main (8fc0c46) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2474      +/-   ##
==========================================
- Coverage   30.35%   30.34%   -0.02%     
==========================================
  Files         139      139              
  Lines        8466     8469       +3     
==========================================
  Hits         2570     2570              
- Misses       5543     5546       +3     
  Partials      353      353              
Impacted Files Coverage Δ
cmd/cosign/cli/policy_init.go 1.35% <0.00%> (ø)
cmd/cosign/cli/sign/sign.go 15.34% <0.00%> (-0.13%) ⬇️
cmd/cosign/cli/sign/sign_blob.go 0.00% <0.00%> (ø)

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: Hector Fernandez <[email protected]>
Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

thanks!

@dlorenc dlorenc merged commit 8f66577 into sigstore:main Nov 21, 2022
@github-actions github-actions bot added this to the v1.14.0 milestone Nov 21, 2022
@hectorj2f hectorj2f deleted the respect_tlog_upload_with_tsa branch November 21, 2022 18:45
@haydentherapper
Copy link
Contributor

Hey @hectorj2f @priyawadhwa, I think we should revert this (or I'll remove it in my new PR).

Getting a timestamp should not affect whether or not you upload a rekor entry. It's not an alternative to signature transparency, it's only an alternative to where the timestamp comes from.

@priyawadhwa
Copy link
Contributor

I disagree, for private artifacts users may not want to store anything in a transparency log and because the artifacts are private they aren't looking for signature transparency. Using a TSA instead of Rekor is a good option in that case. I think it's important to support both cases, and we've made it clear via flags that using a TSA is an insecure option.

@haydentherapper
Copy link
Contributor

They should be setting tlog-upload=false in that case then. Only on verification is there an insecure-* flag, so it's not clear on signing that not uploading to the tlog is fundamentally changing the security model of Sigstore.

@haydentherapper
Copy link
Contributor

Or maybe we have a global private-artifact flag that changes behavior?

I don't think the default should be not uploading to the transparency log however. The defaults must be secure by default, and removing the tlog upload isn't.

@priyawadhwa
Copy link
Contributor

They should be setting tlog-upload=false in that case then.

That sounds good to me, that way it's explicit on sign and then we have insecure on verify. I'm ok with any reasonable combo of flags, my main point was just that the feature should be supported.

@haydentherapper
Copy link
Contributor

Agreed! I see the issue in the previous code, that if keyless=true and tlog-upload=false, we don't respect tlog-upload. I'll submit a fix to always respect tlog-upload, regardless of if a TSA is used. Does that sound good?

@priyawadhwa
Copy link
Contributor

SGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants