Remove proto from commitments package by gdbelvin · Pull Request #716 · google/keytransparency

gdbelvin · 2017-07-31T14:49:28Z

In order to make mobile bindings possible, we need to remove protobuf
references from verification interfaces.

AMarcedone · 2017-07-31T20:11:56Z

core/crypto/commitments/commitments.go


 // Commit makes a cryptographic commitment under a specific userID to data.
-func Commit(userID, appID string, data []byte) ([]byte, *tpb.Committed, error) {
+func Commit(userID, appID string, data []byte) (commitment, nonce []byte, err error) {


Despite being better than before (as it does not depend on protobuf), this function is still not gobind friendly, as it returns 3 outputs. To be "visible" from Java, functions need to return only one output (plus an optional second one which needs to be of type error).

To fix this, I've created a GenCommitmentKey function.

gdbelvin · 2017-07-31T20:15:39Z

Would it be easier to return a struct, or should those be avoided as well?

…

On Mon, Jul 31, 2017 at 9:11 PM Antonio Marcedone ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/crypto/commitments/commitments.go <#716 (comment)> : > } // Commit makes a cryptographic commitment under a specific userID to data. -func Commit(userID, appID string, data []byte) ([]byte, *tpb.Committed, error) { +func Commit(userID, appID string, data []byte) (commitment, nonce []byte, err error) { Despite being better than before (as it does not depend on protobuf), this function is still not gobind friendly, as it returns 3 outputs. To be "visible" from Java, functions need to return only one output (plus an optional second one which needs to be of type error). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#716 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMHTtdtBcs6JDNA_6HeKrvuGaZyg30vks5sTjUMgaJpZM4Oof3M> .

AMarcedone · 2017-07-31T20:26:02Z

A struct would work (as long as its members are of gobind friendly types such as []byte).
I am not sure what is best from a design perspective, but my original idea was to leave the interface as it currently is and add some simple gobind friendly wrappers that internally call the original proto based interfaces. I think it is worth discussing this face to face tomorrow.

gdbelvin · 2017-07-31T20:45:00Z

Sounds good

…

On Mon, Jul 31, 2017 at 9:26 PM Antonio Marcedone ***@***.***> wrote: A struct would work (as long as its members are of gobind friendly types such as []byte). I am not sure what is best from a design perspective, but my original idea was to leave the interface as it currently is and add some simple gobind friendly wrappers that internally call the original proto based interfaces. I think it is worth discussing this face to face tomorrow. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#716 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMHTkDCfBjf7Xi8f9cB7o18_Qe6IjTFks5sTjhbgaJpZM4Oof3M> .

AMarcedone · 2017-07-31T21:01:47Z

Here is a concrete example of the interface I was thinking about (the B prefix is meant to indicate the goBind friendly version of a function)
AMarcedone@6579ef0

codecov-io · 2017-08-08T17:24:23Z

Codecov Report

Merging #716 into master will increase coverage by 0.68%.
The diff coverage is 32%.

@@            Coverage Diff             @@
##           master     #716      +/-   ##
==========================================
+ Coverage   47.97%   48.66%   +0.68%     
==========================================
  Files          30       30              
  Lines        2491     2503      +12     
==========================================
+ Hits         1195     1218      +23     
+ Misses       1106     1088      -18     
- Partials      190      197       +7

Impacted Files	Coverage Δ
core/keyserver/validate.go	`36.53% <0%> (ø)`	⬆️
core/client/kt/requests.go	`0% <0%> (ø)`	⬆️
core/keyserver/keyserver.go	`0% <0%> (ø)`	⬆️
core/client/kt/verify.go	`51.35% <15.38%> (+32.68%)`	⬆️
core/crypto/commitments/commitments.go	`73.07% <63.63%> (-15.82%)`	⬇️
impl/sql/commitments/sql.go	`48.67% <63.63%> (+1.88%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c0bbc7...611caad. Read the comment docs.

cesarghali

Thanks for the PR. I have few comments below.

cesarghali · 2017-08-08T17:51:45Z

core/client/kt/verify.go

+		nonce := in.GetCommitted().GetKey()
+		if err := commitments.Verify(userID, appID, commitment, data, nonce); err != nil {
+			Vlog.Printf("✗ Commitment verification failed.")
+			return fmt.Errorf("VerifyCommitment(): %v", err)


This might make people go look for VerifyCommitment function which does not exist anymore. Please consider changing the error message.

cesarghali · 2017-08-08T17:53:24Z

core/client/kt/verify_test.go

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


Can we add tests to the other functions in verify.go?

Good idea. I've added a test for VerifyGetEntyrResponse

cesarghali · 2017-08-08T17:55:42Z

core/crypto/commitments/commitments.go

-
-// Commit makes a cryptographic commitment under a specific userID to data.
-func Commit(userID, appID string, data []byte) ([]byte, *tpb.Committed, error) {
+// GenCommitmentKey generates a commitment key for use in Commit.  This key


We might want to elaborate a bit more on this. GenCommitmentKey should be kept secret, that's correct, but it's sent over to the server as part of the request. Maybe we should explain why it's OK for the server to know this key.

cesarghali · 2017-08-08T18:00:12Z

core/keyserver/validate_test.go

 	vrfPriv, _ := p256.GenerateKey()
 	index, _ := vrfPriv.Evaluate(vrf.UniqueID(userID, appID))
-	commitment, committed, _ := commitments.Commit(userID, appID, profileData)
+	nonce, err := commitments.GenCommitmentKey()


Consider using a fixed nonce to make the test repeatable.

cesarghali · 2017-08-08T18:26:21Z

impl/sql/commitments/sql_test.go

 	// Create test data.
 	pdata := []byte("key")
-	commitmentC, committedC, err := commitments.Commit("foo", "app", pdata)
+	nonce, err := commitments.GenCommitmentKey()


gdbelvin · 2017-08-08T22:54:45Z

Added tests and cleaned up. PTAL.

In order to make mobile bindings possible, we need to remove protobuf references from verification interfaces.

cesarghali · 2017-08-09T18:14:46Z

core/client/kt/verify_test.go

+			appID:   "app",
+			trusted: &trillian.SignedLogRoot{},
+			in: &keytransparency_v1_types.GetEntryResponse{
+				VrfProof:  []byte{0x8, 0xca, 0x1, 0xaa, 0x1, 0x74, 0x70, 0xf4, 0xbf, 0x96, 0x69, 0xff, 0x73, 0x5d, 0xc7, 0x9d, 0x2b, 0x51, 0xb0, 0xa5, 0x42, 0xb8, 0x8c, 0x42, 0x34, 0xfc, 0xa1, 0xa2, 0xc0, 0x80, 0xdf, 0x76, 0x33, 0xfc, 0x7d, 0xee, 0x4a, 0xf3, 0x18, 0xea, 0x30, 0xc4, 0xa4, 0x6f, 0x31, 0xc9, 0x20, 0x73, 0x42, 0x84, 0xd9, 0x71, 0x39, 0x52, 0xb2, 0x8f, 0x58, 0x52, 0x4, 0x53, 0x87, 0x8, 0x3e, 0x81, 0x4, 0xb, 0x13, 0x89, 0xd7, 0xc6, 0x63, 0x22, 0x39, 0x18, 0x73, 0x72, 0xfa, 0x32, 0xf6, 0xeb, 0x3, 0x8, 0x5d, 0x7, 0x4e, 0x2, 0x3a, 0xc6, 0x7f, 0x89, 0xe8, 0x44, 0x27, 0xcb, 0x73, 0xdc, 0xf2, 0x2f, 0xcc, 0xcd, 0x90, 0x6e, 0x97, 0xcb, 0x22, 0xff, 0x6e, 0xdb, 0x74, 0x22, 0xbf, 0x28, 0x27, 0x9b, 0x9e, 0x26, 0x1a, 0xe4, 0xc6, 0x16, 0x59, 0x4f, 0x7d, 0xcc, 0xb9, 0x8e, 0x7d, 0x41, 0xf7},


Up to you: What do you think about expressing this as a hexa string and then convert to byte before using it? It'll be shorter. Same applies below.

That's true, but makes the test vectors harder to regenerate with log.Printf("%#v")

gdbelvin requested a review from AMarcedone July 31, 2017 14:49

AMarcedone reviewed Jul 31, 2017

View reviewed changes

gdbelvin mentioned this pull request Aug 7, 2017

Verifying a nil commitment silently passes #717

Closed

gdbelvin force-pushed the verifier_interface branch 8 times, most recently from 8512f30 to b7b1f62 Compare August 8, 2017 13:09

gdbelvin requested a review from cesarghali August 8, 2017 16:30

gdbelvin assigned cesarghali Aug 8, 2017

google deleted a comment from codecov-io Aug 8, 2017

cesarghali suggested changes Aug 8, 2017

View reviewed changes

gdbelvin force-pushed the verifier_interface branch from f186123 to da33524 Compare August 8, 2017 22:45

gdbelvin added 3 commits August 9, 2017 10:26

Remove proto from commitments package

1d02a9f

In order to make mobile bindings possible, we need to remove protobuf references from verification interfaces.

Use GenerateCommitmentKey to simplify API

1d9dd4d

add tests for VerifyGetEntryResponse

521db29

gdbelvin force-pushed the verifier_interface branch from da33524 to 521db29 Compare August 9, 2017 09:27

This was referenced Aug 9, 2017

Add gobindClient package and a gobind friendly interface to core/client/kt/verify.go #736

Merged

Create Test Vectors #613

Closed

cesarghali approved these changes Aug 9, 2017

View reviewed changes

Merge branch 'master' into verifier_interface

611caad

gdbelvin merged commit 7e01002 into google:master Aug 9, 2017

gdbelvin deleted the verifier_interface branch August 9, 2017 20:53

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

Comments

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin commented Jul 31, 2017 via email

Uh oh!

AMarcedone commented Jul 31, 2017

Uh oh!

gdbelvin commented Jul 31, 2017 via email

Uh oh!

AMarcedone commented Jul 31, 2017

Uh oh!

codecov-io commented Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cesarghali 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!

gdbelvin Aug 8, 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!

gdbelvin commented Aug 8, 2017

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gdbelvin commented Jul 31, 2017 •

edited

Loading

codecov-io commented Aug 8, 2017 •

edited

Loading

gdbelvin Aug 8, 2017 •

edited

Loading