Create Hasher Registry by gdbelvin · Pull Request #694 · google/trillian

gdbelvin · 2017-06-20T15:36:12Z

Implements a hasher registry in the style of go's crypto package

Replaces tree.Hasher with hashers.NewLogHasher

Moves tree_hasher.go to merkle/hashers to break some import cycles.

codingllama

Travis looks unhappy, PTAL.

codingllama · 2017-06-20T16:07:47Z

merkle/hashers/tree_hasher.go

 }
+
+var (
+	maxHashers = trillian.HashStrategy(len(trillian.HashStrategy_value))


There's a dangerous assumption here that len(trillian.HashStrategy_value) is greater than the highest proto tag used. This works for now, but will break if we retire a value from the enum or if we simply skip tag numbers.

Good point. I'll replace this with a function that iterates through all the possible enums to find the max.

I'd suggest a map[HashStrategy]XHasher instead. No need to worry about proto tags that way.

Good idea. Done.

codingllama · 2017-06-20T16:10:05Z

merkle/hashers/tree_hasher.go

+// RegisterLogHasher registers a hasher for use.
+func RegisterLogHasher(h trillian.HashStrategy, f LogHasher) {
+	if h < 0 || h >= maxHashers {
+		panic(fmt.Sprintf("RegisterLogHasher(%v) of unknown hasher", h))


nit: If it's on the enum, then the value has to be known. On a related note, we probably shouldn't let people register hashers for UNKNOWN_HASH_STRATEGY.

I would probably prefer an error return to a panic as well.

Ditto for others.

Good point. should I remove the check?

I'm following the style of golang's crypto package here. Since this function should only be called in an init method, I assumed that a panic was more ok (ish). Ie. it should only be triggered right on launch.

I think we should check for:

hash == UNKNOWN_HASH

hasher == nil (possibly?)

hash previously registered (ie, warn about overrides)

Re panic or non-panic, would all callers do:

if err := RegisterLogHash(h, hasher); err != nil { panic(err) }

... or something to that effect? If yes, then I guess the panic inside Register() is appropriate.

We're checking for UNKNOWN_HASH.

hasher == nil is ok, and will be caught when requested.

I'll add a check for overrides.

codingllama · 2017-06-20T16:14:26Z

merkle/hashers/tree_hasher.go

+}
+
+// NewLogHasher returns a LogHasher
+func NewLogHasher(h trillian.HashStrategy) (LogHasher, error) {


nit: Technically, it's not a new hasher. GetLogHasher, maybe?

Ditto for NewMapHasher.

Get* isn't very Go style friendly... How's LogHasher()?

Turns out LogHasher conflicts with the interface name.
Even though this isn't technically create new objects (because there's no need to), it seems like it fits in the New* category because it's like a factory method. What do you think?

codingllama · 2017-06-20T16:19:56Z

merkle/hashers/tree_hasher.go

+)
+
+// RegisterLogHasher registers a hasher for use.
+func RegisterLogHasher(h trillian.HashStrategy, f LogHasher) {


This solution assumes a hasher is a function of HashStrategy, but in fact it's a mix of HashAlgorithm and HashStrategy. We had this right on the (now deleted) trees functions.

I don't mind about the interface changes, but I'd strongly prefer if you kept trees.Hash and trees.XHasher as they were. I don't think we need a registry for hashers either; we've only got a few (hand-coded) of them anyway.

I wanted to ask you about this. I saw hash algorithm being incorporated into the call to rfc6962.New.

HashAlgorithm is used to determine the signature algorithm -- that's why it's a subfield of DigitallySigned. Could we consider not overloading the HashAlgorith field? In my experience, its better to include algorithms into the suite identifier, eg. RFC69626_SHA256. Would you be open to that change? Happy to create a separate PR for it. This would greatly simplify several software level checks we do to determine compatibility between various fields since we'd only put permitted values in the enum.

Discussed offline, we'll go ahead with #698.

gdbelvin · 2017-06-21T17:21:50Z

Let's chat about this tomorrow. @Martin2112 Would love your thoughts as well.

Martin2112 · 2017-06-22T08:31:21Z

Still doesn't build.

codingllama · 2017-06-22T10:45:37Z

I'm not totally sold on the need for a hasher registry, specially considering that we're unlikely to have many variations. I'd probably stick to a simpler approach, eg, a switch-case hardcoded for each type.

I won't hold you back if you prefer this, but could you explain why a simpler approach won't work?

Implements a hasher registry in the style of go's crypto package Moves tree_hasher.go to merkle/hashers to break some import cycles.

codingllama

No major comments, only a few nits and typos.

codingllama · 2017-06-23T17:44:13Z

merkle/hashers/tree_hasher.go

+// RegisterLogHasher registers a hasher for use.
+func RegisterLogHasher(h trillian.HashStrategy, f LogHasher) {
+	if h == trillian.HashStrategy_UNKNOWN_HASH_STRATEGY {
+		panic(fmt.Sprintf("RegisterLogHasher(%v) of unknown hasher", h))


nit: Either use %s or h.String(), so the message gets the enum name?

codingllama · 2017-06-23T17:44:24Z

merkle/hashers/tree_hasher.go

+		panic(fmt.Sprintf("RegisterLogHasher(%v) of unknown hasher", h))
+	}
+	if logHashers[h] != nil {
+		panic(fmt.Sprintf("%v already registred as a LogHasher", h))


Ditto.

Same for others.

codingllama · 2017-06-23T17:45:02Z

merkle/hashers/tree_hasher.go

+		panic(fmt.Sprintf("RegisterLogHasher(%v) of unknown hasher", h))
+	}
+	if logHashers[h] != nil {
+		panic(fmt.Sprintf("%v already registred as a LogHasher", h))


Typo on "registered".

codingllama · 2017-06-23T17:45:07Z

merkle/hashers/tree_hasher.go

+		panic(fmt.Sprintf("RegisterMapHasher(%v) of unknown hasher", h))
+	}
+	if mapHashers[h] != nil {
+		panic(fmt.Sprintf("%v already registred as a MapHasher", h))


Typo on "registered".

codingllama · 2017-06-23T17:45:45Z

merkle/hashers/tree_hasher.go

+	if f != nil {
+		return f, nil
+	}
+	return nil, fmt.Errorf("LogHasher(%v) is unknown hasher", h)


nit: "is unknown hasher" -> "is an unknown hasher".

Same below.

codingllama · 2017-06-23T17:47:27Z

merkle/objhasher/objhasher.go

-// NewMapHasher returns a new ObjectHasher based on the passed in MapHasher
-func NewMapHasher(baseHasher merkle.MapHasher) merkle.MapHasher {
+// GetMapHasher returns a new ObjectHasher based on the passed in MapHasher
+func GetMapHasher(baseHasher hashers.MapHasher) hashers.MapHasher {


nit: I think this one was better named as NewMapHasher.

Same below (NewLogHasher).

codingllama · 2017-06-23T17:49:32Z

storage/testonly/admin_storage_tester.go

 	ktestonly "github.com/google/trillian/crypto/keys/testonly"
 	"github.com/google/trillian/crypto/keyspb"
 	spb "github.com/google/trillian/crypto/sigpb"
+	_ "github.com/google/trillian/merkle/maphasher" // TESET_MAP_HASHER


nit: Typo on TEST_MAP_HASHER

gdbelvin · 2017-06-26T08:39:47Z

Updated. PTAL.

googlebot added the cla: yes label Jun 20, 2017

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

codingllama suggested changes Jun 20, 2017

View reviewed changes

gdbelvin force-pushed the register2 branch from 7ca188f to 8f1f582 Compare June 22, 2017 09:12

gdbelvin force-pushed the register2 branch from 8f1f582 to d823f22 Compare June 22, 2017 11:00

vqhuy mentioned this pull request Jun 22, 2017

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

Open

gdbelvin force-pushed the register2 branch 4 times, most recently from affc16b to 06a2820 Compare June 23, 2017 13:07

gdbelvin added 6 commits June 23, 2017 17:47

Hasher registry

af86c7a

Implements a hasher registry in the style of go's crypto package Moves tree_hasher.go to merkle/hashers to break some import cycles.

reviwer comments

9b9499b

make hashers available

a734654

Renamed hasher.LogHasher

32b2e54

Back to NewLogHasher and NewMapHasher

3c5a581

Add checks to hasher registry

c4d222e

gdbelvin force-pushed the register2 branch from de42998 to c4d222e Compare June 23, 2017 16:47

codingllama reviewed Jun 23, 2017

View reviewed changes

Reviewer updates. GetLogHasher -> NewLogHasher

1e99e55

codingllama approved these changes Jun 26, 2017

View reviewed changes

gdbelvin merged commit 8676ec0 into google:master Jun 26, 2017

gdbelvin deleted the register2 branch June 26, 2017 08:58

gdbelvin mentioned this pull request Jun 26, 2017

CONIKS hasher #691

Merged

Comments

Conversation

gdbelvin commented Jun 20, 2017

Uh oh!

codingllama left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin commented Jun 21, 2017

Uh oh!

Martin2112 commented Jun 22, 2017

Uh oh!

codingllama commented Jun 22, 2017

Uh oh!

codingllama left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin commented Jun 26, 2017

Uh oh!

gdbelvin Jun 21, 2017 •

edited

Loading

gdbelvin Jun 21, 2017 •

edited

Loading