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.

Sign up for GitHub

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

Jump to bottom

Breaking change: Remove timestamping authority #813

Merged
merged 1 commit into from
May 19, 2022
Merged

Breaking change: Remove timestamping authority #813

merged 1 commit into from
May 19, 2022

Conversation

haydentherapper
Copy link
Contributor

@haydentherapper haydentherapper commented May 9, 2022 •
edited
Loading

See #812 for more discussion.

Signed-off-by: Hayden Blauzvern [email protected]

Summary

Ticket Link

Fixes #812

Release Note

Removed timestamping authority API. This is a breaking API change. If you are relying on the timestamping authority to issue signed timestamps, create signed timestamps using either OpenSSL or a service such as FreeTSA.

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2022 •
edited
Loading

Codecov Report

Merging #813 (d0a09db) into main (2978cdc) will decrease coverage by 1.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #813      +/-   ##
==========================================
- Coverage   47.71%   46.62%   -1.09%     
==========================================
  Files          62       60       -2     
  Lines        5504     5094     -410     
==========================================
- Hits         2626     2375     -251     
+ Misses       2570     2445     -125     
+ Partials      308      274      -34
Impacted Files Coverage Δ
pkg/pki/x509/x509.go 56.25% <ø> (+0.82%) ⬆️
pkg/signer/memory.go 66.66% <ø> (-0.36%) ⬇️
cmd/rekor-cli/app/pflags.go 84.50% <0.00%> (-7.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2978cdc...d0a09db. Read the comment docs.

@haydentherapper haydentherapper marked this pull request as ready for review May 10, 2022 16:26
@haydentherapper haydentherapper requested review from asraa and a team as code owners May 10, 2022 16:26
dlorenc
dlorenc previously approved these changes May 13, 2022
View reviewed changes
Copy link
Member

@dlorenc dlorenc left a comment

Choose a reason for hiding this comment

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

I'm happy to remove as is, thanks for clarifying the metrics issue.

@haydentherapper haydentherapper requested a review from dlorenc May 13, 2022 17:22
@haydentherapper
Copy link
Contributor Author

Small update to remove unused docker compose env var

dlorenc
dlorenc previously approved these changes May 14, 2022
View reviewed changes
@dlorenc
Copy link
Member

dlorenc commented May 17, 2022

Should we merge this one?

@haydentherapper
Copy link
Contributor Author

Waiting for @asraa to take a look! Let's wait til EOD tomorrow.

asraa
asraa suggested changes May 18, 2022
View reviewed changes
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM, minus the e2e test and the timestamp fuzzer needs to be removed.

@asraa
Copy link
Contributor

asraa commented May 18, 2022

Sorry about the separate ping but I guess the chceks/tools/presubmits for fuzzing need to be removed too
https://github.com/sigstore/rekor/pull/488/files

@haydentherapper
Copy link
Contributor Author

Let's see if that worked - I made the fuzz target do nothing (Avoiding removing all of the fuzzing additions so it's easy to add fuzz tests in the future)

@haydentherapper haydentherapper force-pushed the remove-tsa branch 2 times, most recently from f93cbc5 to ce1c63c Compare May 18, 2022 22:52
@haydentherapper
Copy link
Contributor Author

@asraa @dlorenc - Ready for a last review, I added back an e2e test that tests uploading an rfc3161 timestamp, and removed the timestamp fuzzer (though left all of the fuzzing set up)

@haydentherapper
Remove timestamping authority
7cb70b1 
See sigstore#812 for more discussion.

Signed-off-by: Hayden Blauzvern <[email protected]>
asraa
asraa reviewed May 19, 2022
View reviewed changes
CODEOWNERS
@@ -5,9 +5,6 @@

/pkg/types/ @bobcallaway

/pkg/api/timestamp.go @asraa @loosebazooka
/pkg/types/rfc3161/ @asraa @loosebazooka
Copy link
Contributor

Choose a reason for hiding this comment

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

keep the rfc type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, I'll create a PR to fix this, sorry!

@dlorenc dlorenc merged commit 5a10385 into sigstore:main May 19, 2022
@github-actions github-actions bot added this to the v1.0.0 milestone May 19, 2022
jyotsna-penumaka added a commit to jyotsna-penumaka/rekor-rs that referenced this pull request Jul 16, 2022
@jyotsna-penumaka
Remove timestamping authority API and examples.
1b78839 
timestamping authority has been removed from rekor.
So this test will fail.
sigstore/rekor#813

Signed-off-by: jyotsna-penumaka <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asraa asraa asraa approved these changes

@dlorenc dlorenc dlorenc left review comments

@bobcallaway bobcallaway Awaiting requested review from bobcallaway

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.0.0
Development

Successfully merging this pull request may close these issues.

Remove the timestamping authority before GA
4 participants
@haydentherapper @codecov-commenter @dlorenc @asraa