api: fix inclusion proof verification flake

[asraa]

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

Fixes:

• inclusion proof failure: wrong proof size 11, want 12 cosign#2123
• [e2e]: generic release main default slsa3  slsa-framework/slsa-github-generator#658
• [e2e]: generic schedule main multi-subjects slsa3  slsa-framework/slsa-github-generator#601

Summary

This fixes a flakey problem with Rekor entry verification.

Every once in a while, we receive errors in validating the inclusion proof from Rekor:

it may either be an error verifying the inclusion proof:

 validating log entry 37: verifying inclusion proof: calculated root:
[73 180 215 55 7 58 247 212 29 155 130 228 142 72 218 20 131 130 3 179 61 150 19 180 243 114 50 157 177 182 130 69]
 does not match expected root:
[237 220 193 22 243 233 200 112 169 46 110 170 96 240 121 161 104 148 224 127 132 219 64 234 88 87 57 75 232 144 114 197]

or may be caught earlier than that with incorrect inclusion proof size:

2022/08/04 15:00:38 validating log entry 25: verifying inclusion proof: wrong proof size 3, want 4

This happens because we

1. get the current tree size X
2. ANOTHER client uploads an entry increasing the tree size to Y and then
3. trillian returns an inclusion proof for the index for tree size X.

See code here

The trillian response for the inclusion proof includes a SignedLogRoot for the tree size at size Y.
https://github.com/google/trillian/blob/44841b0bad99d6b7ed5ab20ff24cfa5ca6add9d3/trillian_log_api.proto#L231-L235

The Rekor server validates the inclusion proof at size X, returning the proof response successfully.

rekor/pkg/api/trillian_client.go

Line 229 in 547eb3c

if err := proof.VerifyInclusion(rfc6962.DefaultHasher, uint64(index), root.TreeSize, resp.GetLeaf().MerkleLeafHash, resp.Proof.Hashes, root.RootHash); err != nil {

Rekor returns the proof response with the SignedLogRoot at tree size Y, client attempts to validate with this. This errors out.

rekor/pkg/api/entries.go

Line 445 in 45fd37d

return logEntryFromLeaf(ctx, api.signer, tc, leaf, result.SignedLogRoot, result.Proof, tid, api.logRanges)

This PR returns the SignedLogRoot for the tree size in the requested proof.

On a side note, I dislike very much that Trillian's proof response does not contain the requested tree size X.

Release Note

• bug fix: fix inclusion proof response in Rekor server that causes flakey verification problems

Documentation

[dlorenc]

Amazing find!

[haydentherapper]

This is really fantastic, great work!

Can you add a comment briefly summarizing this design decision in the client so we have context for why it's designed this way?

[bobcallaway]

great catch and RCA - just some recommendations to reuse your helper function here.

any thoughts on how we could add regression testing for this?

[asraa]

any thoughts on how we could add regression testing for this?

Adding a regression test that fires two concurrent goroutine uploading and verifying. I'm able to repro the failure at HEAD, and fix here. I had to use 50 uploads/verifies, although I suspect the number can be less.

[codecov-commenter]

Codecov Report

Merging #956 (ba5726e) into main (102dc64) will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #956      +/-   ##
==========================================
+ Coverage   48.20%   48.35%   +0.14%     
==========================================
  Files          61       61              
  Lines        5383     5383              
==========================================
+ Hits         2595     2603       +8     
+ Misses       2506     2500       -6     
+ Partials      282      280       -2     

Impacted Files	Coverage Δ
pkg/types/rekord/v0.0.1/entry.go	49.49% <0.00%> (+0.66%)	⬆️
pkg/types/alpine/v0.0.1/entry.go	56.72% <0.00%> (+2.52%)	⬆️

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

[bobcallaway]

great work @asraa!