[golang] Merkle verifier #1161

AlCutter · 2016-04-25T16:25:38Z

Lays groundwork for #1159
cc/ @gdbelvin

pphaneuf · 2016-04-26T11:46:54Z

go/merkletree/tree_hasher.go

+	h.hasher.Write([]byte{LeafPrefix})
+	return h.hasher.Sum(leaf)
+
+}


Can't you do something like this in Go instead? Wouldn't that be more Go-like?

func (h hash.Hash) HashLeaf(leaf []byte) []byte { h.Reset() h.Write([]byte{LeafPrefix}) return h.Sum(leaf) }

And just delete DigestSize. And inline the two-line implementation of HashEmpty entirely at its one call site?

Or if it's weird to add methods (which it kind of is for these methods in particular, from my perception of how this interface is meant to be used), HashLeafand HashChildren could just be plain functions that take a hash.Hash?

Nope, that's pretty Objective-C, though :)
AFAIK in go you can't add methods to types in other packages.

pphaneuf · 2016-04-26T12:36:41Z

Also:

--- FAIL: TestVerifyInclusionProof (0.00s)
    merkle_verifier_test.go:337: failed to verify proof: insuficient number of proof components (1) for treeSize 8
--- FAIL: TestVerifyConsistencyProof (0.00s)
    merkle_verifier_test.go:354: Failed to verify proof: invalid empty root1
--- FAIL: TestCollisionDifferentLeaves (0.00s)
    tree_hasher_test.go:93: Got unexpected leaf1 digest size 37, expected 32
FAIL
FAIL    github.com/google/certificate-transparency/go/merkletree    0.025s

pphaneuf · 2016-04-26T13:27:35Z

See cc5c322 for a suggestion trimming a chunk out. Also fails the tests, in the same way, I didn't fix it up, just flensed the fat away. 😉

RJPercival · 2016-04-29T11:11:47Z

go/merkletree/tree_hasher.go

+func (h TreeHasher) HashLeaf(leaf []byte) []byte {
+	h.hasher.Reset()
+	h.hasher.Write([]byte{LeafPrefix})
+	return h.hasher.Sum(leaf)


I don't think this does what you think it does... https://golang.org/pkg/hash/ (or in fact, what anyone would expect it to do!)

That is a really good point o_O
Thanks!

gdbelvin · 2016-05-03T16:52:36Z

Very happy to see progress on this!

AlCutter · 2016-05-20T13:22:07Z

PTAL :)

gdbelvin · 2016-05-25T18:59:14Z

go/merkletree/merkle_verifier.go

+}
+
+// VerifyInclusionProof verifies the correctness of the passed in proof given the passed in information about the tree and leaf.
+func (m MerkleVerifier) VerifyInclusionProof(leafIndex, treeSize int64, proof [][]byte, root []byte, data []byte) error {


In Go, multiple arguments with the same type can be rewritten as follows:

func (m MerkleVerifier) VerifyInclusionProof(leafIndex, treeSize int64, proof [][]byte, root, data []byte)

Like leafIndex and treeSize already are, you mean? :)
I didn't collapse the later arg types as it looks a bit weird to me, though I don't feel strongly about it.

It isn't clear what data is. Perhaps rename it to leaf or something like that?

gdbelvin · 2016-05-25T23:59:56Z

Will this be integrated with the client?

AlCutter · 2016-06-09T10:40:14Z

@RJPercival prod prod

RJPercival · 2016-06-15T13:05:24Z

go/merkletree/merkle_verifier.go

+	"fmt"
+)
+
+// MerkleVerifier is a class which knows how to verify merkle inclusion and consistency proofs.


The RFC refers to inclusion proofs as "audit proofs". Do we consistently use the term "inclusion proof" instead?

Yeah - "audit proof" is kinda vague, so we settled on using "inclusion-" and "consistency-proof" for clarity.

AlCutter · 2016-06-15T16:41:07Z

PTAL

AlCutter · 2016-06-17T09:37:56Z

ping

pphaneuf · 2016-06-17T09:54:24Z

This one could stand a rebase, being on top of a commit from back in April! 😉

RJPercival · 2016-06-17T15:17:36Z

go/merkletree/merkle_verifier.go

+}
+
+// RootFromInclusionProof calculates the expected tree root given the proof and leaf.
+func (m MerkleVerifier) RootFromInclusionProof(leafIndex, treeSize int64, proof [][]byte, data []byte) ([]byte, error) {


Rename data to leaf here as well.

AlCutter · 2016-06-21T09:06:25Z

PTAL

RJPercival · 2016-06-21T09:17:32Z

go/merkletree/merkle_verifier_test.go

+	sha256EmptyTreeHash = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
+)
+
+func verifierCheck(v *MerkleVerifier, leafIndex, treeSize int64, proof [][]byte, root []byte, data []byte) error {


data should be renamed to leaf here too.

RJPercival · 2016-06-21T09:26:31Z

go/merkletree/merkle_verifier_test.go

+		return errors.New("incorrectly verified against swapped roots")
+	}
+
+	// Wrong inclusion proofs


s/inclusion/consistency/

RJPercival · 2016-06-21T09:36:51Z

go/merkletree/tree_hasher_test.go

+		t.Fatalf("Empty leaf digest has length %d, but expected %d", got, want)
+	}
+
+	if bytes.Compare(leaf1Digest, leaf2Digest) == 0 {


nit: bytes.Equal is preferable to bytes.Compare. This applies in the next function as well.

Yup, surprised I wrote it using Compare tbh. Done, anyway.

RJPercival · 2016-06-21T09:40:57Z

LGTM, bar a few nits.

* upstream/master: (23 commits) Check for HTTP status 200 in fetchAndParse. Run everything through gofmt. Tweak the imports to make goimports happier. Initialize client if passed nil (google#1265) Have the caller supply the *http.Client used in logclient.go. Review fixes Use ASN1 lib to transform TLS encoded list into a Octet String Move SCTListLen Check to beginning Rework patch Add support for serializing SCT Lists Get rid of the Java MerkleLeafType enum. Add a missing entry to the .gitignore file. Get rid of the TLSSerializer class. Make the low-level serialization functions stand-alone ones in a namespace rather than members of a class. 6962-bis: Implement OID parsing/serialization. Do not use std::move in "return" statements. Improve stats comments and add additional inc. Add additonal stats to outer FixAndLog wrapper. [golang] Merkle verifier (google#1161) Ignore clang warnings due to unused stuff caused by glog (google#1253) Remove a few unnecessary uses of std::move. ...

[golang] Merkle verifier

* upstream/master: (25 commits) Check for HTTP status 200 in fetchAndParse. Run everything through gofmt. Tweak the imports to make goimports happier. Initialize client if passed nil (google#1265) Have the caller supply the *http.Client used in logclient.go. Review fixes Use ASN1 lib to transform TLS encoded list into a Octet String Move SCTListLen Check to beginning Rework patch Add support for serializing SCT Lists Get rid of the Java MerkleLeafType enum. Add a missing entry to the .gitignore file. Get rid of the TLSSerializer class. Make the low-level serialization functions stand-alone ones in a namespace rather than members of a class. 6962-bis: Implement OID parsing/serialization. Do not use std::move in "return" statements. Improve stats comments and add additional inc. Add additonal stats to outer FixAndLog wrapper. [golang] Merkle verifier (google#1161) Ignore clang warnings due to unused stuff caused by glog (google#1253) Remove a few unnecessary uses of std::move. ...

AlCutter assigned RJPercival Apr 25, 2016

googlebot added the cla: yes label Apr 25, 2016

pphaneuf reviewed Apr 26, 2016
View reviewed changes

RJPercival reviewed Apr 29, 2016
View reviewed changes

gdbelvin reviewed May 25, 2016
View reviewed changes

gdbelvin mentioned this pull request Jun 2, 2016

Create Verification Client google/keytransparency#171

Closed

5 tasks

RJPercival reviewed Jun 15, 2016
View reviewed changes

AlCutter added 4 commits June 17, 2016 14:26

[golang] Merkle verifier

0d3bd5e

Swapped out the b0rked hasher business.

68b07a6

Fixes

2e75618

fixes

394f5b4

AlCutter force-pushed the go_merkle_verifier branch from afdfb71 to 394f5b4 Compare June 17, 2016 13:28

RJPercival reviewed Jun 17, 2016
View reviewed changes

mo_leaf_less_data

e8a0051

RJPercival reviewed Jun 21, 2016
View reviewed changes

more leaf

b074a93

RJPercival reviewed Jun 21, 2016
View reviewed changes

fix comment

aead7ff

RJPercival reviewed Jun 21, 2016
View reviewed changes

bytes.Equal()

2ca9bdc

RJPercival added the LGTM label Jun 21, 2016

AlCutter merged commit dad64d1 into google:master Jun 21, 2016

AlCutter deleted the go_merkle_verifier branch June 21, 2016 10:19

gdbelvin pushed a commit to gdbelvin/certificate-transparency that referenced this pull request Jul 18, 2016

[golang] Merkle verifier (google#1161)

f48b091

[golang] Merkle verifier

RJPercival mentioned this pull request Nov 29, 2016

Golang: LogClient does not support fetching or verifying consistency proofs #1159

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[golang] Merkle verifier #1161

[golang] Merkle verifier #1161

AlCutter commented Apr 25, 2016

pphaneuf Apr 26, 2016

AlCutter Apr 28, 2016

pphaneuf commented Apr 26, 2016

pphaneuf commented Apr 26, 2016 •

edited

Loading

RJPercival Apr 29, 2016 •

edited

Loading

AlCutter Apr 29, 2016

gdbelvin commented May 3, 2016

AlCutter commented May 20, 2016

gdbelvin May 25, 2016 •

edited

Loading

AlCutter Jun 9, 2016

RJPercival Jun 15, 2016

AlCutter Jun 15, 2016

gdbelvin commented May 25, 2016

AlCutter commented Jun 9, 2016

RJPercival Jun 15, 2016

AlCutter Jun 15, 2016

AlCutter commented Jun 15, 2016

AlCutter commented Jun 17, 2016

pphaneuf commented Jun 17, 2016

RJPercival Jun 17, 2016

AlCutter Jun 20, 2016

AlCutter commented Jun 21, 2016

RJPercival Jun 21, 2016

AlCutter Jun 21, 2016

RJPercival Jun 21, 2016

RJPercival Jun 21, 2016 •

edited

Loading

AlCutter Jun 21, 2016

RJPercival commented Jun 21, 2016

[golang] Merkle verifier #1161

[golang] Merkle verifier #1161

Conversation

AlCutter commented Apr 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pphaneuf commented Apr 26, 2016

pphaneuf commented Apr 26, 2016 • edited Loading

RJPercival Apr 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdbelvin commented May 3, 2016

AlCutter commented May 20, 2016

gdbelvin May 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdbelvin commented May 25, 2016

AlCutter commented Jun 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlCutter commented Jun 15, 2016

AlCutter commented Jun 17, 2016

pphaneuf commented Jun 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlCutter commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RJPercival Jun 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RJPercival commented Jun 21, 2016

pphaneuf commented Apr 26, 2016 •

edited

Loading

RJPercival Apr 29, 2016 •

edited

Loading

gdbelvin May 25, 2016 •

edited

Loading

RJPercival Jun 21, 2016 •

edited

Loading