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

Resolves #522 set Created date to time of execution #2108

Merged
merged 6 commits into from
Jul 29, 2022
Merged

Resolves #522 set Created date to time of execution #2108

merged 6 commits into from
Jul 29, 2022

Conversation

Lerentis
Copy link
Contributor

Summary

This commit sets the Created date of the signature to the date of execution of the signing action.

Release Note

-changed: Signatures do now use the created timestamp based on the local time of signing

Documentation

n/a

Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

This mostly lgtm, aside from the one tiny style nit.

A couple more questions though:

  • Is there any kind of test we can add to check that the created date isn't zero?
  • Do we also want this for attestations, SBOMs, etc?

@Lerentis
Copy link
Contributor Author

  • Is there any kind of test we can add to check that the created date isn't zero?

for a test like this one would need to mock a timestamp right? i am still learning when it comes to writing tests, so i fear this is out of my skill level as of now :/

  • Do we also want this for attestations, SBOMs, etc?

would make sense to keep this consistent imo, but maybe i am missing some greater context why it was implemented with this fixed date stamp before

@imjasonh
Copy link
Member

  • Is there any kind of test we can add to check that the created date isn't zero?

for a test like this one would need to mock a timestamp right? i am still learning when it comes to writing tests, so i fear this is out of my skill level as of now :/

I don't know about everyone, but I'd personally accept a test that just checks that the created date is non-zero. Bonus points for it being any time in the last minute. Having a test that injects a fake timestamp and checks that it's exactly that date seems like overkill, but that's subjective.

  • Do we also want this for attestations, SBOMs, etc?

would make sense to keep this consistent imo, but maybe i am missing some greater context why it was implemented with this fixed date stamp before

Yeah I think we should stay consistent.

The reason it was zero before was probably some combination of (a) that was the default when using empty.Image and mutate.Append, and (b) some vague idea that the signatures should be reproducible, so that if two clients signed the same artifact with the same signature at different times they should produce the same layer bytes. But for (b) at least, that doesn't really make sense, since those two signatures would never be the same anyway.

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2022

Codecov Report

Merging #2108 (5325d71) into main (5b57154) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2108      +/-   ##
==========================================
+ Coverage   26.27%   26.35%   +0.07%     
==========================================
  Files         129      129              
  Lines        7574     7582       +8     
==========================================
+ Hits         1990     1998       +8     
  Misses       5329     5329              
  Partials      255      255              
Impacted Files Coverage Δ
pkg/oci/mutate/signatures.go 38.88% <100.00%> (+4.88%) ⬆️
pkg/oci/static/file.go 73.52% <100.00%> (+3.52%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@Lerentis
Copy link
Contributor Author

@imjasonh i hope i understood the requested changes correctly. initially i thought you were talking about a unit test and not a check about the previous date that was set 😅

…te change to files as well

Signed-off-by: Tobias Trabelsi <[email protected]>
@imjasonh
Copy link
Member

@imjasonh i hope i understood the requested changes correctly. initially i thought you were talking about a unit test and not a check about the previous date that was set 😅

Oh I meant we should have a unit test that when a signature/whatever gets appended, it also updates the image's created-at date to be something recent, or at least non-zero.

I think we should always update the image's created-at to be Now(), otherwise if I sign something today and wait 90 days and sign it again, the created-at date will be 90 days ago and Gitlab might housekeep it.

@Lerentis
Copy link
Contributor Author

ah okay then my initial understanding was correct. sorry about that.
added a simple zero unit test

Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

This looks good!

I can't approve your CI run, but except for a few minor comments, this should be good to go assuming tests pass.

pkg/oci/mutate/signatures_test.go Outdated Show resolved Hide resolved
pkg/oci/static/file_test.go Show resolved Hide resolved
pkg/oci/static/file_test.go Outdated Show resolved Hide resolved
pkg/oci/mutate/signatures_test.go Outdated Show resolved Hide resolved
Signed-off-by: Tobias Trabelsi <[email protected]>
Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

Only tiny tiny nits left, otherwise LGTM

pkg/oci/mutate/signatures_test.go Outdated Show resolved Hide resolved
pkg/oci/static/file.go Outdated Show resolved Hide resolved
pkg/oci/static/file.go Outdated Show resolved Hide resolved
pkg/oci/mutate/signatures.go Outdated Show resolved Hide resolved
Co-authored-by: Jason Hall <[email protected]>
Signed-off-by: Tobias Trabelsi <[email protected]>
Signed-off-by: Tobias Trabelsi <[email protected]>
@dlorenc dlorenc merged commit ae99f7b into sigstore:main Jul 29, 2022
@github-actions github-actions bot added this to the v1.11.0 milestone Jul 29, 2022
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.

5 participants