Verify returned entry matches client inputs in rekor-cli verify#2799
Verify returned entry matches client inputs in rekor-cli verify#2799Hayden-IO merged 1 commit intosigstore:mainfrom
Conversation
Signed-off-by: Bob Callaway <bcallaway@google.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2799 +/- ##
===========================================
- Coverage 66.46% 26.26% -40.20%
===========================================
Files 92 191 +99
Lines 9258 20163 +10905
===========================================
- Hits 6153 5296 -857
- Misses 2359 14033 +11674
- Partials 746 834 +88
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This issue really makes me think we should be deprecating this client, or at the very least anything with verification. It’s not integrated into the conformance test suite (nor should it be because it shouldn’t be yet another Go client). |
Let's go ahead and fix it and discuss deprecation separately. I know this is packaged and shipped in some distributions. |
Hayden-IO
left a comment
Hayden-IO
left a comment
There was a problem hiding this comment.
I think this is a net-positive, but it's still incomplete because for two of the three cases, providing an index or leaf hash, you aren't verifying the leaf hash against the original entry. It's a fine improvement and I'm good with merging, but it's not sufficient for verification. We should clearly define the trust model for using this tool.
| entry = v | ||
| } | ||
|
|
||
| if err := verifyBodyMatchesUUID(entry, o.EntryUUID); err != nil { |
There was a problem hiding this comment.
What's the purpose of this check? o.EntryUUID and entry both come from the server.
There was a problem hiding this comment.
Should this instead compare uuid to the entry's hash when uuid is provided?
There was a problem hiding this comment.
the check is to ensure the response from the server is coherent (body matches leaf hash)
the check on line 171 does what you asked (compares client provided uuid to the entry in the response's hash
There was a problem hiding this comment.
The underlying issue is the server shouldn’t return a leaf hash because a client should calculate it itself, but we aren’t gonna the API, this is fine
| entry = v | ||
| } | ||
|
|
||
| if err := verifyBodyMatchesUUID(entry, o.EntryUUID); err != nil { |
There was a problem hiding this comment.
The underlying issue is the server shouldn’t return a leaf hash because a client should calculate it itself, but we aren’t gonna the API, this is fine
2 participants
When
rekor-cli verifysearches by artifact, the client now locally canonicalizes the proposed entry and confirms the server's response matches before reporting success.For all lookup paths (uuid, log-index, artifact), the client now verifies the entry body's leaf hash matches the UUID in the response.
Previously the only binding between the user's local inputs and the verified entry was the server's honesty in answering the search query. The inclusion proof, checkpoint, and SET were all verified, but only against the server-returned body — never against the locally-derived expected entry. This adds client-side verification that closes that gap.