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

Validate tree ID on calls to /api/v1/log/entries/retrieve #1017

Merged
merged 2 commits into from Aug 30, 2022

Conversation

priyawadhwa
Copy link
Contributor

fixes #1014, cc @asraa

Signed-off-by: Priya Wadhwa priya@chainguard.dev

@priyawadhwa priyawadhwa requested a review from a team as a code owner August 30, 2022 18:32
@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2022

Codecov Report

Merging #1017 (a64668a) into main (5159d01) will increase coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head a64668a differs from pull request most recent head d0e2a95. Consider uploading reports for the commit d0e2a95 to get more accurate results

@@            Coverage Diff             @@
##             main    #1017      +/-   ##
==========================================
+ Coverage   41.57%   41.60%   +0.02%     
==========================================
  Files          71       71              
  Lines        6929     6932       +3     
==========================================
+ Hits         2881     2884       +3     
- Misses       3746     3747       +1     
+ Partials      302      301       -1     
Impacted Files Coverage Δ
pkg/api/entries.go 0.00% <0.00%> (ø)
pkg/types/alpine/v0.0.1/entry.go 54.87% <0.00%> (+1.21%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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.

Interesting! Thanks for the fix and debugging. So what was happening before was that we were actually retrieving the result via searchHashes on the raw uuid it seems.

So even now, if you send an something where sharding.ValidateEntryID(entryID) is an error, you will continue to do that behavior and return something from a different tree.

@asraa
Copy link
Contributor

asraa commented Aug 30, 2022

I'm a little brain-fried right now, but I think the fix is going to need to propogate the entire EntryUUID when you go through this case:

rekor/pkg/api/entries.go

Lines 346 to 350 in d0e2a95

hash, err := hex.DecodeString(uuid)
if err != nil {
return handleRekorAPIError(params, http.StatusBadRequest, err, malformedUUID)
}
searchHashes = append(searchHashes, hash)

Then in here: extract the uuid and validate the returned tree ID when one was in the EntryUUID request:

if leafResult != nil && leafResult.Leaf != nil {

@priyawadhwa
Copy link
Contributor Author

So even now, if you send an something where sharding.ValidateEntryID(entryID) is an error, you will continue to do that behavior and return something from a different tree.

Yep, but I think sharding.ValidateEntryID(entryID) should only be an error if a UUID was originally requested instead of an EntryID, in which case expected behavior (at the moment) is we would search all shards and return all matching entries since there is no Tree ID.

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>
@asraa
Copy link
Contributor

asraa commented Aug 30, 2022

Yep, but I think sharding.ValidateEntryID(entryID) should only be an error if a UUID was originally requested instead of an EntryID,

I think it'll error out if the tree ID doesn't parse as an int64 or if it's 0

i, err := strconv.ParseInt(t, 16, 64)
if err != nil {
return fmt.Errorf("could not convert treeID %v to int64: %v", t, err)
}
// Check for invalid TreeID values
// TODO: test for more of these
if i == 0 {
return fmt.Errorf("0 is not a valid TreeID")
}

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>
@priyawadhwa
Copy link
Contributor Author

I think it's fine if the tree is 0, since that maps to having a UUID and putting a bunch of 0s in front of it to form an EntryID.

To address the failure on parsing int64 I added in a validation for UUID so it should do something like this:

  1. Check if EntryID is valid, if yes then try to get it
  2. If no, assume we have a UUID and validate it. Fail if we don't have a UUID, otherwise add it to search hashes.

@asraa
Copy link
Contributor

asraa commented Aug 30, 2022

To address the failure on parsing int64 I added in a validation for UUID so it should do something like this:

That's perfect! Thank you!! That made the fix so clean

@priyawadhwa priyawadhwa merged commit 2081fbb into sigstore:main Aug 30, 2022
@priyawadhwa priyawadhwa deleted the bug branch August 30, 2022 19:26
@github-actions github-actions bot added this to the v1.0.0 milestone Aug 30, 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.

Rekor does not validate Tree ID in requests for entries/retrieve
4 participants