CONIKS hasher #691

gdbelvin · 2017-06-19T09:58:16Z

Add treeID, index, and depth to HashLeaf and HashEmtpy.
The fulfills the security requirements and closes #670.

Adds the CONIKS hasher to the set of map hashers.
The next PR will make it available through the command line.

Martin2112 · 2017-06-21T07:48:25Z

merkle/coniks/coniks.go

@@ -0,0 +1,89 @@
+// Copyright 2016 Google Inc. All Rights Reserved.


Martin2112 · 2017-06-21T07:48:57Z

merkle/coniks/coniks.go

+// Default is the standard CONIKS hasher.
+var Default = New(crypto.SHA512_256)
+
+// hasher implements the sparse merkel tree hashing algorithm specified in the CONIKS paper.


Same typo as before. Merkle. Please fix where you cut and pasted it from too.

Martin2112 · 2017-06-21T07:50:00Z

merkle/coniks/coniks.go

+	h := m.New()
+	h.Write(emptyIdentifier)
+	binary.Write(h, binary.BigEndian, uint64(treeID))
+	h.Write(index) // TODO block out the bits that are not part of the index.


What are the implications of this TODO. Does it work properly as is?

The result is that the hashes would be incorrect. Thanks for poking at this.
I've implemented and tested the proper masking function now.

Martin2112 · 2017-06-21T07:51:18Z

merkle/coniks/coniks.go

+// HashEmpty returns the hash of an empty branch at a given depth.
+// A depth of 0 indicates the hash of an empty leaf.
+// Empty branches within the tree are plain interior nodes e1 = H(e0, e0) etc.
+func (m *hasher) HashEmpty(treeID int64, index []byte, height int) []byte {


The comment says 'depth' but the parameter is 'height'. This will cause confusion. If it really is height can you fix it in the interface definition and the other implementation. We've tried to use 'depth' everywhere because of previous confusion.

After merging this PR, I wouldn't be opposed to changing the tree computation algorithms to all use depth. At the moment though, the algorithms themselves use height and it's a little simpler to hide the height to depth conversion inside this function.

Martin2112 · 2017-06-21T07:51:49Z

merkle/coniks/coniks.go

+	return h.Sum(nil)
+}
+
+// HashChildren returns the inner Merkle tree node hash of the the two child nodes l and r.


replace 'inner' with 'internal'.

Martin2112 · 2017-06-21T07:52:03Z

merkle/coniks/coniks_test.go

@@ -0,0 +1,47 @@
+// Copyright 2016 Google Inc. All Rights Reserved.


Martin2112 · 2017-06-21T07:54:25Z

merkle/hstar2_test.go

@@ -25,6 +24,8 @@ import (
 	"github.com/google/trillian/testonly"
 )

+const treeID = int64(0)


I'd suggest something other than zero for the ID. I'd also test that different treeID values result in different hashes.

I'm not exactly sure what to do here. The maphasher - which matches the Python / C++ merkle tree code - does not use TreeID and result in different hashes.

We could

Change the maphasher to include TreeID and generate new test vectors.

Keep the existing ones and add a new test with CONIKS test vectors. But do we really want to make the merkle package dependent on all the sub hashing algorithms?

Martin2112 · 2017-06-21T07:55:09Z

merkle/map_verifier.go

@@ -40,6 +40,11 @@ func VerifyMapInclusionProof(index, leafHash, expectedRoot []byte, proof [][]byt
 	if got, want := len(leafHash)*8, hBits; got != want {
 		return fmt.Errorf("invalid leafHash length %d, want %d", got, want)
 	}
+	for i, element := range proof {
+		if got, wanta, wantb := len(element), 0, h.Size(); got != wanta && got != wantb {
+			return fmt.Errorf("invalid proof: len(proof[%v]) %d, want %d or %d", i, got, wanta, wantb)


Should use "got:, want:" formatting in Errorf.

Martin2112 · 2017-06-21T07:56:10Z

merkle/objhasher/objhasher_test.go

@@ -24,6 +24,8 @@ import (
 	"github.com/google/trillian/merkle/rfc6962"
 )

+const treeID = int64(0)


Again, zero is probably not the best choice.

The subhashers that the test uses don't use treeID

OK then as long as hashers that do use it have their own tests.

Martin2112 · 2017-06-21T07:59:45Z

storage/cache/subtree_cache_test.go

@@ -56,8 +56,10 @@ var splitTestVector = []struct {
 var defaultLogStrata = []int{8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8}
 var defaultMapStrata = []int{8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 176}

+const treeID = int64(0)


Another use of zero.

Martin2112 · 2017-06-21T12:41:48Z

OK. We can leave maphasher as is. The coniks one should test that the treeID is being correctly handled.

…

On 21 June 2017 at 13:39, Gary Belvin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In merkle/hstar2_test.go <#691 (comment)>: > @@ -25,6 +24,8 @@ import ( "github.com/google/trillian/testonly" ) +const treeID = int64(0) I'm not exactly sure what to do here. The maphasher - which matches the Python / C++ merkle tree code - does not use TreeID and result in different hashes. We could - Change the maphasher to include TreeID and generate new test vectors. - Keep the existing ones and add a new test with CONIKS test vectors. But do we really want to make the merkle package dependent on all the sub hashing algorithms? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#691 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMv2TzPQ_tGkiCpPSr6fvrUTuX5YiqY7ks5sGQ8dgaJpZM4N-AQX> .

gdbelvin · 2017-06-21T16:25:39Z

I've implemented maskIndex PTAL.

gdbelvin · 2017-06-26T09:43:52Z

Rebased now that #694 has been merged. PTAL

vqhuy · 2017-06-26T16:57:19Z

Do you want to hash the treeID, as proposed at coniks-sys/coniks-go#177 (comment)?

Martin2112 · 2017-06-27T13:39:04Z

merkle/coniks/coniks.go

+}
+
+// maskIndex returns index with only the left depth bits set.
+// index must be of size m.Size() and 0 <= depth <= m.BitLen().


This could probably be an array. I think including an example in the comment would help here. Maybe also mention that it's only used for the last byte to explain the 0xff in position 0.

I would agree, but different hashers can have different sizes for their indexes, which makes using an array here difficult.

Not sure what you mean. It's only used by maskIndex and it's indexed by byte position from 0-7.

ah, yes. -- referring to leftmask?

Yes, leftmask, which should probably also be leftMask.

Martin2112 · 2017-06-27T13:41:58Z

merkle/coniks/coniks_test.go

+		{index: h2b("FF00000000000000000000000000000000000000"), depth: 5, want: h2b("F800000000000000000000000000000000000000")},
+		{index: h2b("FF00000000000000000000000000000000000000"), depth: 6, want: h2b("FC00000000000000000000000000000000000000")},
+		{index: h2b("FF00000000000000000000000000000000000000"), depth: 7, want: h2b("FE00000000000000000000000000000000000000")},
+		{index: h2b("FF00000000000000000000000000000000000000"), depth: 8, want: h2b("FF00000000000000000000000000000000000000")},


Can you add a couple of test cases that are less uniform and show the mask being applied across byte boundaries etc.

Not sure if github is showing me all the latest changes but I'd like to see a depth above 8.

Martin2112 · 2017-06-27T13:44:01Z

merkle/hstar2.go

+// e.g. 1 -> 0000000000000000000000000000000000000001
+func PaddedBytes(i *big.Int, size int) []byte {
+	b := i.Bytes()
+	ret := make([]byte, size)


Does this work if there is no padding needed?

Added at test for this.

Martin2112 · 2017-06-27T13:49:48Z

merkle/hstar2_test.go

+		i    int64
+		want []byte
+	}{
+		{i: 1, want: h2b("0000000000000000000000000000000000000001")},


This would be a good place to add the test for zero padding + possibly others.

Martin2112 · 2017-06-27T13:50:39Z

merkle/objhasher/objhasher_test.go

@@ -24,6 +24,8 @@ import (
 	"github.com/google/trillian/merkle/rfc6962"
 )

+const treeID = int64(0)


OK then as long as hashers that do use it have their own tests.

Martin2112 · 2017-06-27T13:51:41Z

server/trillian_log_server/main.go

@@ -29,6 +29,7 @@ import (
 	"github.com/google/trillian/crypto/keys"
 	"github.com/google/trillian/extension"
 	_ "github.com/google/trillian/merkle/objhasher" // Load hashers


I don't think you need the comments on these imports + in other files. The '_' implies it's being imported for side effects.

go vet sometimes complains if there isn't a comment on underscore imports.

Add treeID, index, and depth to HashLeaf and HashEmtpy. Fuffills the security requirements in google#670.

gdbelvin · 2017-06-27T14:47:38Z

Added more tests. PTAL

googlebot added the cla: yes label Jun 19, 2017

gdbelvin force-pushed the coniks branch from a88b78c to 448544a Compare June 20, 2017 15:35

gdbelvin requested a review from Martin2112 June 20, 2017 15:36

Martin2112 reviewed Jun 21, 2017

View reviewed changes

gdbelvin mentioned this pull request Jun 21, 2017

Create generic interface for hash functions coniks-sys/coniks-go#178

Open

gdbelvin force-pushed the coniks branch 3 times, most recently from fc2a412 to 5b89ec0 Compare June 26, 2017 09:42

gdbelvin requested a review from daviddrysdale June 26, 2017 09:48

gdbelvin force-pushed the coniks branch 3 times, most recently from 00dbf80 to e67184b Compare June 26, 2017 16:08

Martin2112 reviewed Jun 27, 2017

View reviewed changes

gdbelvin added 3 commits June 27, 2017 15:47

CONIKS Hasher

9493a31

Add treeID, index, and depth to HashLeaf and HashEmtpy. Fuffills the security requirements in google#670.

Pad index to correct size

751cf04

more test vectors

63928d6

gdbelvin force-pushed the coniks branch from f357fc8 to 63928d6 Compare June 27, 2017 14:47

Martin2112 approved these changes Jun 27, 2017

View reviewed changes

gdbelvin merged commit 492f275 into google:master Jun 27, 2017

gdbelvin deleted the coniks branch June 27, 2017 15:24

gdbelvin mentioned this pull request Jul 7, 2017

Map test cleanup #727

Merged

gdbelvin mentioned this pull request Aug 7, 2017

Map Verification: Fix for proofs of leaves in empty branches. #780

Merged

		@@ -0,0 +1,89 @@
		// Copyright 2016 Google Inc. All Rights Reserved.

		@@ -0,0 +1,47 @@
		// Copyright 2016 Google Inc. All Rights Reserved.

CONIKS hasher #691

CONIKS hasher #691

Conversation

gdbelvin commented Jun 19, 2017 • 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

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

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

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

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

Choose a reason for hiding this comment

Martin2112 commented Jun 21, 2017 via email

gdbelvin commented Jun 21, 2017

gdbelvin commented Jun 26, 2017

vqhuy commented Jun 26, 2017 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdbelvin Jun 27, 2017 • 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 Jun 27, 2017

gdbelvin commented Jun 19, 2017 •

edited

Loading

vqhuy commented Jun 26, 2017 •

edited

Loading

gdbelvin Jun 27, 2017 •

edited

Loading