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

fix: make rekor verify work with sharded uuids #970

Merged
merged 1 commit into from Aug 17, 2022

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Aug 12, 2022

Signed-off-by: Asra Ali asraa@google.com

Summary

Fixes #877

See issue for the problem: rekor verify didn't work with sharding: If the requested UUID was a sharded Entry UUID (Tree ID + UUID), then we would fail because the return keys of models.LogEntryAnon are always plain UUIDs. When we would compare the returned response UUID with the requested, we would fail on mismatch.

Question

Entry UUID = Tree ID + UUID
UUID = UUID

Why aren't the returned key UUIDs not the Entry UUIDs? It feels weird to me that if clients are requesting Entry UUIDs, and we are making clients aware of sharded UUIDs, then we better also return those in responses.

Meanwhile, we expose virtual log indices. Aren't Entry UUIDs the equivalent of virtual log indices for uuids?

On the other hand, it makes sense to return the actual UUID because that is the hash of the leaf value, and clients are expected to verify that. In that case, why are Entry UUIDs exposed at all? All of our endpoints handle them, see this bug in question.

Release Note

Documentation

Signed-off-by: Asra Ali <asraa@google.com>
@asraa asraa requested a review from a team as a code owner August 12, 2022 18:15
@asraa
Copy link
Contributor Author

asraa commented Aug 12, 2022

Will tackle factoring out as a follow-up, want to resolve the question.

@priyawadhwa
Copy link
Contributor

EntryID is mainly useful if the same UUID exists across shards, and you need to be able to specify which one you want. I think it's more useful for requesting specific entries from shards, and not as necessary when entries are being returned. That's mostly because the UUID is actually needed for verification as you pointed out, so it makes more sense to return that.

Totally agree the behavior is unexpected though so I'd be open to changing it if people have strong preferences😅

@asraa
Copy link
Contributor Author

asraa commented Aug 12, 2022

EntryID is mainly useful if the same UUID exists across shards, and you need to be able to specify which one you want.

I think my problem though was that people don't hold on to EntryUUIDs because they're not in return responses at all yet. In the cosign verify flow we reconstruct the UUID by the proposed entry. If I want to DoS a large project's artifact that was uploaded on an inactive shard, I'll just re-upload the entry to the active shard, which means that the latest shard (

return logEntry, nil
) will return with an invalid integratedTime for verification.

This means that cosign's verification flow will need to change: either be shard-aware (which is difficult for blobs), or have rekor return all the matching UUIDs across all shards, and verify if any of the integrated times are valid against cert expiration.

@asraa
Copy link
Contributor Author

asraa commented Aug 12, 2022

I guess for now we'll table this and at least fix this behavior!
I'll file a follow-up issue about client/cosign verification with sharding.

@bobcallaway
Copy link
Member

I think my problem though was that people don't hold on to EntryUUIDs because they're not in return responses at all yet

They are in return responses, just not easily extracted and returned through the cosign-defined interfaces.

}

// Note: the returned entry UUID is the UUID (not include the Tree ID)
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the API return the full entryUUID (including the treeID)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's my main question in the PR description. However, the behavior is mixed... e.g. rekor-cli get returns just the raw UUID as well, and the request API here also may take the raw UUID.

Originally I had changed the response payload to fix this. Should I go back to that?

@dlorenc dlorenc merged commit a4c88b2 into sigstore:main Aug 17, 2022
@github-actions github-actions bot added this to the v1.0.0 milestone Aug 17, 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.

Sharding: Will using a virtual index impact inclusion verification?
4 participants