Client library for verifications #39

vqhuy · 2016-07-28T10:25:52Z

I merged branch no-getset since we can take the advantages of struct fields (e.g., https://github.com/coniks-sys/coniks-go/blob/9c253690f74c41d9763111bd660cbb8108236b86/client/cgo/coniks.go#L105-L107).
I will fixed other getters when the #36 is clarified.

arlolra · 2016-07-28T19:39:59Z

This is probably just a personal preference thing, but merging in branches complicates rebasing.

Next time you want to depend on another branch, I'd suggest you rebase your change on top of that branch. If that branch is already pushed, you can even create a pull request against that branch so that only the above changes are shown in the PR here.

vqhuy · 2016-07-28T19:43:51Z

Next time you want to depend on another branch, I'd suggest you rebase your change on top of that branch. If that branch is already pushed, you can even create a pull request against that branch so that only the above changes are shown in the PR here.

Yeah, thanks for your advices!

Btw, I also rebased this branch on top of master branch.

arlolra · 2016-07-29T17:29:31Z

client/cgo/cgotest.go

+    unsigned char pk[] = {28,149,90,106,83,99,227,227,211,89,29,250,220,141,17,203,254,130,127,134,175,133,156,164,83,58,139,228,79,64,252,75};
+    unsigned char m[] = {97,108,105,99,101};
+    unsigned char index[] = {96,43,9,174,212,169,113,184,98,128,48,210,186,218,171,145,115,82,29,23,177,126,63,169,232,125,248,165,91,134,76,32};
+    unsigned char proof[] = {213,244,36,141,83,73,206,202,94,3,40,174,237,156,101,140,222,18,24,211,136,24,74,63,246,211,142,186,255,123,73,12,33,252,200,161,18,208,124,150,91,42,229,156,96,225,154,226,206,143,161,1,141,188,34,7,124,45,222,189,89,116,36,3,233,126,71,41,140,29,36,34,138,167,217,239,16,210,51,125,28,127,7,34,89,219,181,82,86,154,10,155,159,144,158,179};


It would be clearer (and easier to maintain) if you moved generating and serializing this data into the go testVRFVerify and passed it as arguments to C.testVRFVerify.

int testVRFVerify(unsigned char *pk, ...) {

At present, it's hard to tell what it is and why it's correct.

Same for all the tests below.

Yes. Will do it.

I fixed it in 763c2649aba5826e3320c48b4eff3a78cfb688a9. I also made some changes on AuthenticationPath struct: converting PrunedTree from slice of slices to slice of arrays; and adding method to get latest STR from PAD (ec3675936cd8e32df44c0f9b494c60aa2df82c23)

vqhuy · 2016-07-30T04:19:35Z

I think this ready for another review @arlolra @liamsi @masomel

liamsi · 2016-07-30T11:24:50Z

Looks good to me!

arlolra · 2016-07-30T19:24:21Z

client/cgo/cgotest.go

+    unsigned char *m, int mSize,
+    unsigned char *index, int indexSize,
+    unsigned char *proof, int proofSize) {
+    if (cgoVrfVerify(pk, pkSize, m, mSize,


Couldn't you just return cgoVrfVerify(..)? Why are these if blocks here?

I just want to make sure the flow: converting from golang types to C types and then from C types to golang types works as expected.

You don't trust the golang developers? :)

No, it's not what I mean, haha. I mean my use of something like unsafe.Pointer and others ...
~~Ok, I agree that they could be simplified as you said.~~

I just re-read my code and realized that I missed something. What I mean when using these C functions as bridges is that I want to make the same scenario as when the lib being used in the add-on with js-ctypes. That is the input should be type unsigned char* and passed to go functions that take arguments type unsafe.Pointer. Hopefully it makes sense to you.

Sorry, I probably wasn't being clear. I was asking for this,

int testVRFVerify(unsigned char *pk, int pkSize, unsigned char *m, int mSize, unsigned char *index, int indexSize, unsigned char *proof, int proofSize) { return cgoVrfVerify(pk, pkSize, m, mSize, index, indexSize, proof, proofSize); }

Oops, my bad :(. Thanks!

arlolra · 2016-08-01T23:37:47Z

client/cgo/coniks.go

+func main() {}
+
+//export cgoEd25519SignatureVerify
+func cgoEd25519SignatureVerify(pk unsafe.Pointer, pkSize C.int,


Maybe just call this cgoSignatureVerify to not expose the scheme?

vqhuy · 2016-08-02T16:30:23Z

I wasn't finished reviewing before we went down that rabbit hole last night.

Aw, sorry :(

Would it be possible to rename cgo/coniks.go to something like cgo/verification.go?

Yup, makes sense.

masomel · 2016-08-02T17:07:34Z

client/cgo/cgotest.go

+	}
+}
+
+func testVerifyProofOfAbsenceSamePrefix(t *testing.T) {


Just to make sure I've understood this test correctly. The difference between this proof of absence test and the one in VerifyAuthPath is that the string "a" in this test definitely shares the same prefix as "key1", which will give you a user leaf node in the proof? While "123" in the previous test does not share the same prefix as "key1", "key2" or "key3", which will give you an empty node? I'm wondering if we should also be explicitly checking the leaf type in these tests. Though I know this is checked implicitly when the auth path is hashed, so maybe that's enough?

Did you mean we need to assert the string "a" shares the same prefix as "key1"?

I copied this test from proof_test.go, and it already has type assertion there. Hopefully it makes it clearer.

No, since this is done in merkletree.VerifyAuthPath(), right? I mean, should we assert that proof.Leaf.IsEmpty() is false for "a" and true for "123" in testVerifyAuthPath() above.

No, since this is done in merkletree.VerifyAuthPath(), right?

Yes, it is. So if there is anything go wrong, the hash verification would return false. So I think we don't need to explicitly assert it in these tests.

So if there is anything go wrong, the hash verification would return false.

True. I guess, I thought this was an additional way to test that the hash verification is working. But using the hash verification is probably enough for this assertion.

- added benchmarks for creating large directory and doing lookups from large directory - TB test - MakeRand and Digest test

- s/createLargePadForBenchmark/createPad <- could be useful for other tests, too - added tests for some edge cases - removed obsolete check `uint64 < 0` which can't happen and error

- benchmark pad.Update(nil) in tree with 10^5 users (takes on average 0.23 s and consumes 72 MB)

- more stress tests/benchmarks for PAD Update and LookUp (used for pprof)

- test coverage in utils package

masomel · 2016-08-02T17:27:04Z

README.md

@@ -19,6 +19,7 @@ hijacking secure communications without getting caught.
 ## Golang Library
 The packages in this library implement the various components of the CONIKS system and may be imported individually.

+- ``client``: Client-side protocol implementation


Should this explanation say something more like "Client-side protocol cgo (or C?) hooks"?

masomel · 2016-08-02T17:47:11Z

LGTM.

arlolra · 2016-08-02T20:08:05Z

client/cgo/verification.go

+func cgoVerifySignature(pk unsafe.Pointer, pkSize C.int,
+	message unsafe.Pointer, size C.int,
+	sig unsafe.Pointer, sigSize C.int) C.int {
+	if int(pkSize) < sign.PublicKeySize {


Should we define SignatureSize = 64 in crypto/sign.go?

What's the intention of these quick check anyways? Who are they benefiting? We know the expected lengths of these buffers, except for message. Passing in all these length is kind of verbose, but I guess you're trying to be safe and avoid the chance for an out-of-bounds read.

I guess you're trying to be safe and avoid the chance for an out-of-bounds read.

Yes, since we are dealing with unsafe.Pointer. Should I remove these checks?

Should I remove these checks?

Nah. But add one for the SignatureSize as well.

arlolra · 2016-08-02T20:47:41Z

client/cgo/cgotest.go

+	}
+}
+
+func testVerifyHashChain(t *testing.T) {


arlolra · 2016-08-02T21:10:17Z

LGTM

Squashed commits: [03cf223] Fix fatal bug in hash chain verification function [328b245] Compile client crypto library with cgo

vqhuy force-pushed the client-cgo branch from e84f779 to 274d7b3 Compare July 28, 2016 14:35

vqhuy force-pushed the client-cgo branch from 598f50a to a3ed342 Compare July 28, 2016 19:43

arlolra mentioned this pull request Jul 28, 2016

Hash random bytes before putting them on the wire #40

Closed

vqhuy force-pushed the client-cgo branch 2 times, most recently from eb7275f to 613725c Compare July 28, 2016 20:50

arlolra reviewed Jul 29, 2016
View reviewed changes

masomel force-pushed the master branch 3 times, most recently from 25a6925 to 2f704cf Compare July 29, 2016 20:03

vqhuy force-pushed the client-cgo branch 3 times, most recently from 6c7ae21 to b246175 Compare July 30, 2016 02:32

arlolra reviewed Jul 30, 2016
View reviewed changes

vqhuy force-pushed the master branch from 1a881b9 to 38dc55d Compare August 1, 2016 04:22

vqhuy force-pushed the client-cgo branch 2 times, most recently from feb6cf6 to 7801e29 Compare August 1, 2016 06:49

arlolra reviewed Aug 1, 2016
View reviewed changes

arlolra mentioned this pull request Aug 2, 2016

What's the protocol for rotating the signing key? #47

Open

vqhuy force-pushed the client-cgo branch from 0326bf0 to fb335a0 Compare August 2, 2016 16:28

masomel reviewed Aug 2, 2016
View reviewed changes

masomel mentioned this pull request Aug 2, 2016

Which PKI should we use for the server's keys? #52

Open

liamsi added 6 commits August 2, 2016 19:21

WIP: (stress) tests:

0819d89

- added benchmarks for creating large directory and doing lookups from large directory - TB test - MakeRand and Digest test

[WIP]: (stress) tests:

79a534a

- s/createLargePadForBenchmark/createPad <- could be useful for other tests, too - added tests for some edge cases - removed obsolete check `uint64 < 0` which can't happen and error

[WIP]: (stress) tests:

803075a

- benchmark pad.Update(nil) in tree with 10^5 users (takes on average 0.23 s and consumes 72 MB)

[WIP]: (stress) tests:

8525fba

- more stress tests/benchmarks for PAD Update and LookUp (used for pprof)

[WIP]: (stress) tests:

57effe9

- test coverage in utils package

minor changes

c5683a7

masomel reviewed Aug 2, 2016
View reviewed changes

vqhuy force-pushed the client-cgo branch from fb335a0 to 328b245 Compare August 2, 2016 17:29

rebase and fixes

4da5cb4

Use TB.Serialize instead of manually serializing

22d7236

arlolra reviewed Aug 2, 2016
View reviewed changes

vqhuy force-pushed the client-cgo branch 2 times, most recently from 38bbad4 to 03cf223 Compare August 2, 2016 20:44

arlolra reviewed Aug 2, 2016
View reviewed changes

client/cgo/cgotest.go

}

}

func testVerifyHashChain(t *testing.T) {

Copy link

Member

arlolra Aug 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

Make parameter names clearer (+2 squashed commits)

ddf4ab8

Squashed commits: [03cf223] Fix fatal bug in hash chain verification function [328b245] Compile client crypto library with cgo

vqhuy force-pushed the client-cgo branch from 4fdc108 to ddf4ab8 Compare August 2, 2016 21:12

vqhuy merged commit ddf4ab8 into master Aug 2, 2016

arlolra deleted the client-cgo branch August 2, 2016 21:16

masomel mentioned this pull request Aug 9, 2016

Implement registration function #29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client library for verifications #39

Client library for verifications #39

vqhuy commented Jul 28, 2016 •

edited

Loading

arlolra commented Jul 28, 2016

vqhuy commented Jul 28, 2016 •

edited

Loading

arlolra Jul 29, 2016

vqhuy Jul 30, 2016

vqhuy Jul 30, 2016 •

edited

Loading

vqhuy commented Jul 30, 2016

liamsi commented Jul 30, 2016

arlolra Jul 30, 2016

vqhuy Jul 30, 2016

arlolra Jul 30, 2016

vqhuy Jul 30, 2016 •

edited

Loading

vqhuy Jul 31, 2016

arlolra Jul 31, 2016 •

edited

Loading

vqhuy Jul 31, 2016

arlolra Aug 1, 2016

vqhuy Aug 2, 2016

vqhuy commented Aug 2, 2016

masomel Aug 2, 2016 •

edited

Loading

vqhuy Aug 2, 2016 •

edited

Loading

masomel Aug 2, 2016

vqhuy Aug 2, 2016 •

edited

Loading

masomel Aug 2, 2016

masomel Aug 2, 2016 •

edited

Loading

vqhuy Aug 2, 2016

masomel commented Aug 2, 2016

arlolra Aug 2, 2016

vqhuy Aug 2, 2016

arlolra Aug 2, 2016

arlolra Aug 2, 2016

arlolra commented Aug 2, 2016

Client library for verifications #39

Client library for verifications #39

Conversation

vqhuy commented Jul 28, 2016 • edited Loading

arlolra commented Jul 28, 2016

vqhuy commented Jul 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vqhuy Jul 30, 2016 • edited Loading

Choose a reason for hiding this comment

vqhuy commented Jul 30, 2016

liamsi commented Jul 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vqhuy Jul 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlolra Jul 31, 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

vqhuy commented Aug 2, 2016

masomel Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

vqhuy Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vqhuy Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masomel Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masomel commented Aug 2, 2016

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

arlolra commented Aug 2, 2016

vqhuy commented Jul 28, 2016 •

edited

Loading

vqhuy commented Jul 28, 2016 •

edited

Loading

vqhuy Jul 30, 2016 •

edited

Loading

vqhuy Jul 30, 2016 •

edited

Loading

arlolra Jul 31, 2016 •

edited

Loading

masomel Aug 2, 2016 •

edited

Loading

vqhuy Aug 2, 2016 •

edited

Loading

vqhuy Aug 2, 2016 •

edited

Loading

masomel Aug 2, 2016 •

edited

Loading