Create generic interface for hash functions #178

vqhuy · 2017-06-19T17:10:18Z

Create PADHasher & TreeHasher interface to allow developers to use their own hash function.

Change the default hash function to SHA512/256

Part of #50 and #177

In favor of google/trillian#694, I implemented 29aa6a2.
Please let me know what you think.

gdbelvin · 2017-06-21T15:59:36Z

Would there be any interest in trying to syncronize this interface with the one in google/trillian#670 ?

gdbelvin · 2017-06-21T16:05:41Z

crypto/hasher/hasher_test.go

+		leaf      []byte
+		want      Hash
+	}{
+		{treeNonce: [32]byte{0}, index: []byte("foo"), depth: 128, leaf: []byte("leaf"), want: h2h("65e7f29787a6168affd016656bb1f4f03af91cf7416270f5015005f8594d3eb6")},


Question about the index since I ran into this too.
Should indexes be required to be the full length of the hash function? This would change the test vectors.

See google/trillian#691 and google/keytransparency#614

For HashLeaf, I think yes. But the length of the index of empty nodes vary.

Just a note that in trillian we're using int64 for treeID.

liamsi · 2017-06-21T16:09:21Z

I like @gdbelvin's idea. It makes it easier to sync up and might increase compatibility. Be aware that the interface described in google/trillian#670 isn't fully implemented yet (but will be soon).

vqhuy · 2017-06-21T16:10:22Z

Would there be any interest in trying to syncronize this interface with the one in google/trillian#670 ?

Definitely.

liamsi · 2017-06-21T16:13:48Z

crypto/hasher/hasher.go

+	return h.Sum(nil)
+}
+
+// ID returns the name of the cryptographic hash function in string.


s/in string/as a string/ or simply "ID returns the name of the cryptographic hash function"

liamsi

LGTM

liamsi · 2017-06-22T19:23:43Z

merkletree/pad.go

@@ -5,6 +5,7 @@ import (
 	"errors"

 	"github.com/coniks-sys/coniks-go/crypto"
+	conikshasher "github.com/coniks-sys/coniks-go/crypto/hasher/coniks"


I would rather prefer a shorter import alias. What do you think about hasher? As this whole project is called 'coniks', I feel hasher would be enough.

Ideally, this package should be imported using a blank import name. I used conikshasher for now because we import both hasher and hasher/coniks sometimes.

Makes sense! I would still prefer a shorter import. It would saves some typing when using it and it seems more consistent with the generally shorter package names in golang. But it isn't that important. What about chasher for example?

SGTM. Done.

liamsi

Thanks a lot for this pull! Sorry for the late review.
This is a first quick review of the updated code with a few nits/comments.

liamsi · 2017-10-24T19:58:02Z

crypto/hasher/coniks/coniks.go

+	leafIdentifier  = 'L'
+)
+
+type coniksHasher struct {


If the package is called coniks, it's a bit inelegant to call the struct coniksHasher. hasher would be optimal but would collide with github.com/coniks-sys/coniks-go/crypto/hasher. I still wonder if we could come up with a better name here?

liamsi · 2017-10-24T20:03:32Z

crypto/hasher/hasher.go

+}
+
+// treeHasher provides hash functions for tree implementations.
+type treeHasher interface {


On first sight, it's a bit confusing that the treeHasher is unexported but embedded in the exported PADHasher interface. Maybe there is a good reason which I don't see yet? OK, the 'tree-hashing logic' is separate from the rest (ID, Size, ...) but if one wants to implement a different PADHasher, one needs to find the private interface treeHasher. I think it makes sense to collapse both and a public interface should be self-contained (or embed other public interfaces if there are really good reasons). LMK what you think.

I don't remember why I didn't publish treeHasher interface but I can't find any reasons for not doing that.

I think it makes sense to collapse both and a public interface should be self-contained (or embed other public interfaces if there are really good reasons).

Can you elaborate?

I think this should be one interface instead of two. At least both should be public if there is a good reason for having two ifaces (which I don't see).

liamsi · 2017-10-24T20:40:46Z

crypto/hasher/hasher.go

+}
+
+// Hasher returns a PADHasher.
+func Hasher(h string) (PADHasher, error) {


Maybe rename to NewHasher or GetHasher?

liamsi · 2017-10-24T20:41:54Z

crypto/hasher/hasher.go

+	hashers[h] = f()
+}
+
+// Hasher returns a PADHasher.


returns a registered PADHasher identified by the given string. If no such PADHasher exists, it returns an error.

liamsi · 2017-10-24T20:57:05Z

crypto/hasher/coniks/coniks.go

+const (
+	// CONIKSHasher is the identity of the hashing algorithm
+	// specified in the CONIKSHasher paper.
+	CONIKSHasher = "CONIKS Hasher"


I wonder, if there will be different hashers, shouldn't be a central place of identifiers? I always thought we are only swapping the underlying hash function, but all these hashers will be "CONIKS Hashers".

The idea is each hasher implementation should have its own ID declared in its own package. But you're right, we should have a central place of identifiers which includes our "standard" hash schemes, and "CONIKS Hasher" should be replaced by something like "SHA256-xxxx".

…eir own hash function. * Convert the default hash function to SHA-512/256 * Part of #50 Credit to KT's team

gdbelvin · 2017-10-26T17:36:59Z

Glad to see this PR moving forward! Thanks @liamsi and @c633.
I'll ask the Trillian team if we can use a [32]byte under the hood for consistency. Perhaps we can get some test vectors for cross verification?

vqhuy · 2017-10-26T17:41:17Z

Perhaps we can get some test vectors for cross verification?

We would love to do that.

vqhuy requested review from liamsi, arlolra and masomel June 19, 2017 17:10

vqhuy mentioned this pull request Jun 19, 2017

Create Test Vectors #177

Open

vqhuy force-pushed the gh06 branch 3 times, most recently from 332faf0 to 9d040e8 Compare June 20, 2017 06:44

gdbelvin reviewed Jun 21, 2017

View reviewed changes

liamsi reviewed Jun 21, 2017

View reviewed changes

liamsi previously approved these changes Jun 22, 2017

View reviewed changes

vqhuy dismissed liamsi’s stale review via 299999d June 24, 2017 09:04

liamsi reviewed Oct 24, 2017

View reviewed changes

vqhuy force-pushed the gh06 branch 2 times, most recently from 34ac0a9 to b7b8da6 Compare October 26, 2017 16:49

vqhuy added 2 commits October 26, 2017 18:50

Create PADHasher & TreeHasher interface to allow developers to use th…

c5bde39

…eir own hash function. * Convert the default hash function to SHA-512/256 * Part of #50 Credit to KT's team

Fix import name

47f82f9

vqhuy force-pushed the gh06 branch from b7b8da6 to 47f82f9 Compare October 26, 2017 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create generic interface for hash functions #178

Create generic interface for hash functions #178

vqhuy commented Jun 19, 2017 •

edited

Loading

gdbelvin commented Jun 21, 2017

gdbelvin Jun 21, 2017

gdbelvin Jun 21, 2017

vqhuy Jun 21, 2017

gdbelvin Jun 21, 2017

liamsi commented Jun 21, 2017

vqhuy commented Jun 21, 2017

liamsi Jun 21, 2017 •

edited

Loading

liamsi left a comment

liamsi Jun 22, 2017

vqhuy Jun 23, 2017

liamsi Jun 23, 2017 •

edited

Loading

vqhuy Jun 24, 2017

liamsi left a comment

liamsi Oct 24, 2017

liamsi Oct 24, 2017

vqhuy Oct 25, 2017

liamsi Oct 26, 2017

vqhuy Oct 26, 2017

liamsi Oct 24, 2017

liamsi Oct 24, 2017

liamsi Oct 24, 2017

vqhuy Oct 25, 2017

gdbelvin commented Oct 26, 2017

vqhuy commented Oct 26, 2017

Create generic interface for hash functions #178

Are you sure you want to change the base?

Create generic interface for hash functions #178

Conversation

vqhuy commented Jun 19, 2017 • edited Loading

gdbelvin commented Jun 21, 2017

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

liamsi commented Jun 21, 2017

vqhuy commented Jun 21, 2017

liamsi Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi left a 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 commented Oct 26, 2017

vqhuy commented Oct 26, 2017

vqhuy commented Jun 19, 2017 •

edited

Loading

liamsi Jun 21, 2017 •

edited

Loading

liamsi Jun 23, 2017 •

edited

Loading