Root hash signature verification v2 #634
Conversation
Signed-off-by: Jack Leightcap <[email protected]> TASK(jl): `checkpoint` breakout Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: Jack Leightcap <[email protected]>
Signed-off-by: Jack Leightcap <[email protected]>
The canonical implementation [0] passes the message, message digest, and public key into their verification method. Our verification method only takes data and the public key. If we pass the message digest in as the data, it fails, but if we pass the message in as the data, it succeeds. [0] https://github.com/sigstore/rekor/blob/4b1fa6661cc6dfbc844b4c6ed9b1f44e7c5ae1c0/pkg/util/signed_note.go#L75 Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
sigstore/_internal/merkle.py
Outdated
| @@ -133,3 +135,34 @@ def verify_merkle_inclusion(entry: LogEntry) -> None: | |||
| f"Inclusion proof contains invalid root hash: expected {inclusion_proof}, calculated " | |||
| f"{calc_hash}" | |||
| ) | |||
|
|
|||
|
|
|||
| def verify_checkpoint(client: RekorClient, entry: LogEntry) -> None: | |||
There was a problem hiding this comment.
The location of this function was determined only by the fact that it's called immediately proceeding the Merkle tree verification done above; not because of any real connection to Merkle trees. IMO having this in merkle.py isn't super clear as a result...
There was a problem hiding this comment.
Gotcha, I'll move it to _internal.rekor.checkpoint
There was a problem hiding this comment.
Thanks for picking this up @tnytown 😄 I had some detritus around my local branch that seems cleaned up by your un-borking commit.
This PR was largely stuck by what's pointed out by @woodruffw here: #527 (comment)
I couldn't get the signature to pass and really didn't have the debugging know-how to approach why that might be.
There was a problem hiding this comment.
Thanks for picking this up @tnytown 😄
Yep, of course!
I couldn't get the signature to pass and really didn't have the debugging know-how to approach why that might be.
I think I was able to fix this with 43b968b. You were really close, the Go impl's signature verification API needs a digest of the data but ours needs the data itself. Just needed a fresh set of eyes :)
Signed-off-by: Andrew Pan <[email protected]>
Signed-off-by: Andrew Pan <[email protected]>
There was a problem hiding this comment.
This LGTM, but I'd appreciate a review from @asraa or @haydentherapper if either of you has the time!
In particular: we now verify the signature that comes with the checkpoint and cross-check the signed data (i.e. the root hash) against the inclusion proof's. We do this in two circumstances:
- Whenever we're doing online verification (the default)
- During offline verification, if the input bundle contains an inclusion proof (including a checkpoint)
|
Ack, will take a look tomorrow! |
|
Sorry, had a busy week! Will look first thing Monday. |
|
No problem at all! And please let us know if we can do anything to help with review as well 🙂 |
@haydentherapper Doesn't look like your questions made it into your review, FYI. |
This better reflects the field's intent, rather than an internal fact about its structure. Signed-off-by: William Woodruff <[email protected]>
Always perform the inclusion proof opportunistically, even if the proof it provides is no stronger than the inclusion promise. Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Can't use this here yet. Signed-off-by: William Woodruff <[email protected]>
woodruffw
added
enhancement
Now with Signed Note verification!
Thanks to @jleightcap for doing all of the work! I'm just here to push it over the finish line :)
Supersedes #527.
Fixes #248.