Use MapID as reported in SignedMapRoot #721
Conversation
gdbelvin
force-pushed
the
client/mapid
branch
2 times, most recently
from
f65d2d3 to
47915b0
Compare
MapID is part of SignedMapRoot and part of leaf and empty hash computation. We can remove a bit of state from the client without a reduction in security by relying on the value as reported by the server.
gdbelvin
force-pushed
the
client/mapid
branch
2 times, most recently
from
e880d78 to
c42470e
Compare
Codecov Report
@@ Coverage Diff @@
## master #721 +/- ##
==========================================
- Coverage 53.25% 53.22% -0.04%
==========================================
Files 38 38
Lines 3207 3207
==========================================
- Hits 1708 1707 -1
- Misses 1208 1209 +1
Partials 291 291
Continue to review full report at Codecov.
|
liamsi
left a comment
liamsi
left a comment
There was a problem hiding this comment.
LGTM
Question: the reason a server can not just present different (consistent) maps to different clients is that the server will staple back monitors signatures (and other gossip protocols), right?
|
That's right. An append-only log makes sure that if the server ever forks, it must continue to fork in perpetuity. This makes it much easier for a gossip protocol to detect the inconsistency. |
| // New creates a new client. | ||
| func New(mapID int64, | ||
| func New( | ||
| client spb.KeyTransparencyServiceClient, |
There was a problem hiding this comment.
Please move this to the line above it.
| "github.com/google/keytransparency/core/crypto/commitments" | ||
| "github.com/google/keytransparency/core/crypto/vrf" | ||
| "github.com/google/keytransparency/core/tree/sparse" | ||
| tv "github.com/google/keytransparency/core/tree/sparse/verifier" |
There was a problem hiding this comment.
Please keep this here, AFAIK we're putting all Key Transparency imports in the same group regardless of they're renamed.
There was a problem hiding this comment.
The grouping that we're using is:
- Standard packages.
- KT packages.
- Others.
- Proto.
I'm completely fine with a new scheme but we should apply it to all files.
| "github.com/golang/protobuf/proto" | ||
| "github.com/google/trillian" | ||
| "github.com/google/trillian/client" | ||
| tcrypto "github.com/google/trillian/crypto" |
| } | ||
|
|
||
| if err := v.tree.VerifyProof(leafProof.Inclusion, index[:], leafProof.Leaf.LeafValue, sparse.FromBytes(in.GetSmr().RootHash)); err != nil { | ||
| mapID := in.GetSmr().GetMapId() |
There was a problem hiding this comment.
No need to define mapID here. Its value can be inlined in the next line.
| // VerifyProof verifies a tree proof of a given leaf at a given index based on | ||
| // the provided root and neighbor list | ||
| func (v *Verifier) VerifyProof(neighbors [][]byte, index, leaf []byte, root sparse.Hash) error { | ||
| func (v *Verifier) VerifyProof(treeID int64, neighbors [][]byte, index, leaf []byte, root sparse.Hash) error { |
There was a problem hiding this comment.
Please use mapID instead of treeID to keep the terms consistent.
| // calculateRoot calculates the root of the tree branch defined by leaf and | ||
| // neighbors. | ||
| func (v *Verifier) calculateRoot(neighbors [][]byte, bindex string, leaf []byte) (sparse.Hash, error) { | ||
| func (v *Verifier) calculateRoot(treeID int64, neighbors [][]byte, bindex string, leaf []byte) (sparse.Hash, error) { |
|
I just realized this got merged. I can apply those changes in a "small fixes" PR. |
4 participants
MapID is part of SignedMapRoot and part of leaf and empty hash
computation. We can remove a bit of state from the client without a
reduction in security by relying on the value as reported by the server.