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

Bundle TUF timestamp with signature on signing #1294

Merged
merged 5 commits into from
Jan 13, 2022

Conversation

haydentherapper
Copy link
Contributor

Summary

This updates the code to add the TUF timestamp to the OCI signature when signing with Fulcio or writing to Rekor.

Changes to each package:

  • pkg/oci - Add support for reading and saving the timestamp by annotation key
  • TUF client - Put the timestamp in memory on client initialization
  • fulcio and rekor signers - Include the timestamp on signing

I also added some unit tests for the signers, including a mock Rekor client (that's incomplete, should be updated as needed).

Ticket Link

Ref #1273

Release Note

Bundled TUF timestamp with OCI signature on signing, to use to verify signatures with old targets

cc @asraa

@dlorenc
Copy link
Member

dlorenc commented Jan 11, 2022

This looks great to me! I accidentally merge conflicted you though :(

This updates the code to support adding the TUF timestamp
to the OCI signature.

Changes to pkg/oci add support for reading and saving the
timestamp by annotation key. Changes to the TUF client
add putting the timestamp in memory on client
initialization, so callers can access the timestamp.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
This adds the TUF timestamp to the Fulcio and Rekor
signers. Both are necessary since each relies on
TUF metadata. If both signers are used, the latter
one will overwrite the TUF timestamp.

I also added a basic mock Rekor client for tests.
A number of methods are not implemented yet.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@haydentherapper
Copy link
Contributor Author

Rebased. Though in the same vein as Priya's PR, it'd be useful to refactor this to move the Timestamp struct and getTimestamp functions to the TUF package. I'll take a look at that tomorrow.

@haydentherapper
Copy link
Contributor Author

Refactored, ready for review!

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@@ -115,6 +116,7 @@ func New(ctx context.Context, remote client.RemoteStore, cacheRoot string) (*TUF
// Now check to see if it needs to be updated.
trustedTimestamp, ok := trustedMeta["timestamp.json"]
if ok && !isExpiredMetadata(trustedTimestamp) {
t.timestamp = trustedTimestamp
Copy link
Contributor

Choose a reason for hiding this comment

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

should we just store local inside the the tuf wrapper? we store the timestamp and close now from it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back and forth on this when first implementing it. I've made the change now, and I think this actually looks nicer. We also can remove storing the Close function for the local store, since we're persisting the local store itself. Lemme know what you think!

Copy link
Contributor Author

@haydentherapper haydentherapper Jan 11, 2022

Choose a reason for hiding this comment

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

Is it fine to make a copy of LocalStore, or should the TUF struct be holding a pointer to it? From reading a bit online, from a performance standpoint, it seems like it's faster to just make a copy. Only question is if we leak anything from not closing the original local - My thought is no, we don't leak anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

LocalStore is an interface, so I don't think it should be a pointer (the impl can be a pointer) @dlorenc someone help me with Go hahha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://npf.io/2014/05/intro-to-go-interfaces/ - Says interfaces are just pointers, so then it seems like there's no reason to hold a pointer to it.

I need a primer on the memory model for golang :)

pkg/cosign/tuf/client.go Show resolved Hide resolved
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@dlorenc dlorenc merged commit d58fc63 into sigstore:main Jan 13, 2022
@github-actions github-actions bot added this to the v1.5.0 milestone Jan 13, 2022
haydentherapper added a commit to haydentherapper/cosign that referenced this pull request Jan 13, 2022
This follows sigstore#1294 in adding the TUF timestamp to the
annotations layer for attestations, when either
uploading to Rekor or signing with a Fulcio cert.

Ref sigstore#1273

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
dlorenc pushed a commit that referenced this pull request Jan 14, 2022
This follows #1294 in adding the TUF timestamp to the
annotations layer for attestations, when either
uploading to Rekor or signing with a Fulcio cert.

Ref #1273

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
* Bundle TUF timestamp with signature on signing

This updates the code to support adding the TUF timestamp
to the OCI signature.

Changes to pkg/oci add support for reading and saving the
timestamp by annotation key. Changes to the TUF client
add putting the timestamp in memory on client
initialization, so callers can access the timestamp.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

* Add TUF timestamp to OCI signature on sign

This adds the TUF timestamp to the Fulcio and Rekor
signers. Both are necessary since each relies on
TUF metadata. If both signers are used, the latter
one will overwrite the TUF timestamp.

I also added a basic mock Rekor client for tests.
A number of methods are not implemented yet.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

* Add license

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

* Move timestamp to TUF package

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

* Update TUF client to persist local store

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
This follows sigstore#1294 in adding the TUF timestamp to the
annotations layer for attestations, when either
uploading to Rekor or signing with a Fulcio cert.

Ref sigstore#1273

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
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.

3 participants