implement schnorrkel and fix various idioms and linting errors #45

actuallyachraf · 2019-11-24T13:22:17Z

I implemented the schnorrkel algorithm which in my opinion is better suited for curve25519, I also used the ristretto255 library instead of the previous one. Fixed various golang idiomatic errors and linting issues.
All tests are passing.

mimoo

Hey!

Thanks for submitting this PR! A lot of good stuff.

I've requested some changes.

I'd like to know more about schnorrkel before accepting this PR as well, is there a spec somewhere instead of just a repository?

mimoo · 2019-11-25T17:30:05Z

libdisco/apis.go

@@ -1,5 +1,5 @@
-// These Utility functions implement the net.Conn interface. Most of this code
-// was either taken directly or inspired from Go's crypto/tls package.
+// Package libdisco these utility functions implement the net.Conn interface.


"Package libdisco these utility"?

mimoo · 2019-11-25T17:31:34Z

libdisco/apis.go

@@ -123,21 +123,21 @@ var errNoProof = errors.New("Disco: no public key proof set in Config")

 func checkRequirements(isClient bool, config *Config) (err error) {
 	ht := config.HandshakePattern
-	if ht == Noise_NX || ht == Noise_KX || ht == Noise_XX || ht == Noise_IX {
+	if ht == NoiseNX || ht == NoiseKX || ht == NoiseXX || ht == NoiseIX {


for now let's keep the underscore, this is what the documentation uses as well: https://www.discocrypto.com/#/protocol/Noise_N

Hmm how about I upgrade both and make it consistent with Go idioms ?

https://github.com/golang/go/wiki/CodeReviewComments#mixed-caps

mimoo · 2019-11-25T17:35:40Z

libdisco/asymmetric.go

 	"golang.org/x/crypto/curve25519"
 )

 //
 // The following code defines the X25519, chacha20poly1305, SHA-256 suite.
 //
+// The following code implements the Schnorrkel variant of Schnorr signatures
+// over ristretto255.
+// This implementation was picked from https://github.com/w3f/schnorrkel


maybe move this comment to the actual implementation of schnorrkel

mimoo · 2019-11-25T17:36:10Z

libdisco/asymmetric.go

@@ -24,7 +27,7 @@ const (

 // KeyPair contains a private and a public part, both of 32-byte.
 // It can be generated via the GenerateKeyPair() function.
-// The public part can also be extracted via the ExportPublicKey() function.
+// The public part can also be extracted via the String() function.


Let's keep ExportPublicKey for now as well, this is what is in the doc

mimoo · 2019-11-25T17:37:49Z

libdisco/asymmetric.go

-func (kp SigningKeyPair) ExportPublicKey() string {
-	return hex.EncodeToString(kp.SecretKey.Bytes()) + hex.EncodeToString(kp.PublicKey.Bytes())
+// String returns a hexstring encoding of the signing keypair.
+func (kp SigningKeypair) String() string {


let's keep ExportPublicKey()

mimoo · 2019-11-25T17:42:08Z

libdisco/asymmetric_test.go

+	}
+}
+
+func BenchmarkSign(b *testing.B) {


mimoo · 2019-11-25T17:43:15Z

libdisco/asymmetric_test.go

+		sigNBytes := sigN.Encode()
+
+		if !bytes.Equal(sigBytes[:], sigNBytes[:]) {
+			b.Fatal("failed to generate deterministic signatures")


why is this a benchmark? Why would it fail?

mimoo · 2019-11-25T17:43:29Z

libdisco/asymmetric_test.go

+		sig := kp.Sign(input)
+
+		if ok, err := kp.Verify(input, sig); !ok || err != nil {
+			b.Fatal("failed to verify signature with error : ", err)


I don't think you're supposed to fail in a benchmark, since it's not a test per se : o

mimoo · 2019-11-25T17:43:41Z

libdisco/conn.go

@@ -47,14 +47,18 @@ func (c *Conn) LocalAddr() net.Addr {
 	return c.conn.LocalAddr()
 }

+// Addr abstract an address


"abstracts"

mimoo · 2019-11-25T17:44:15Z

libdisco/disco.go

@@ -127,47 +127,47 @@ type handshakeState struct {
 // Serialize is a helper function to serialize a handshake state, later to be unserialized via
 // the `RecoverState()` function.
 // For security purposes, the long-term static keypair is not serialized. Same for the psk
-func (hs handshakeState) Serialize() []byte {
+func (h *handshakeState) Serialize() []byte {


can we keep hs?

Also for changes like this, it's better to split them in multiple PRs : o

I'll update to hs to make it consistent across the API.

actuallyachraf · 2019-11-25T22:32:47Z

Will review the changes and make the modifications !
Schnorrkel is the name of the library, it uses a similar algorithm to EdDSA where s = k + xe instead of k - xe . The original library uses merlin transcript for to encode the message and compute hashes. The signature is also R,s instead of s,e .
Some more references :
dedis/kyber#131
https://crypto.stackexchange.com/questions/34863/ec-schnorr-signature-multiple-standard
Edit : There's no specific reference or standard for schnorr over elliptic curves, the wikipedia algorithm, paper... don't document the behavior of deterministic signatures.

mimoo · 2019-11-26T06:37:30Z

Ah I see, I think this is fine for now, but obviously it'd be good to have a better story here :)

mimoo · 2019-11-27T02:24:01Z

libdisco/apis.go

@@ -1,5 +1,5 @@
-// These Utility functions implement the net.Conn interface. Most of this code
-// was either taken directly or inspired from Go's crypto/tls package.
+// Package libdisco this file implements a net.Conn interface over Disco.


the "Package libdisco" is still here :p

actuallyachraf · 2019-11-27T19:03:39Z

the recent commit fixes the "package libdisco" comment and adjustes the examples (Noise_XK -> NoiseXK instead).

mimoo

Sorry for the delay and thanks again for that PR!

There's one minor change and this can be merged :)

(I think you should be able to merge it with my approval, if you can't I'll merge it : o )

mimoo · 2019-12-04T06:56:17Z

libdisco/disco.go

@@ -127,7 +127,7 @@ type handshakeState struct {
 // Serialize is a helper function to serialize a handshake state, later to be unserialized via
 // the `RecoverState()` function.
 // For security purposes, the long-term static keypair is not serialized. Same for the psk
-func (hs handshakeState) Serialize() []byte {
+func (hs *handshakeState) Serialize() []byte {


this shouldn't be mutable, unless I'm missing something

Yep you're missing the consistency in receiver types
https://github.com/golang/go/wiki/CodeReviewComments#receiver-type

Oh interesting, I always thought you shouldn't use a pointer if you don't intent to mutate your value

implement schnorrkel and fix various idioms and linting errors

aaa8279

mimoo requested changes Nov 25, 2019

View reviewed changes

mimoo mentioned this pull request Nov 26, 2019

story around signatures in disco #47

Open

implement requested changes

0606118

mimoo reviewed Nov 27, 2019

View reviewed changes

fix comment and adjust Go examples

5643933

mimoo approved these changes Dec 4, 2019

View reviewed changes

mimoo approved these changes Dec 5, 2019

View reviewed changes

mimoo merged commit 6b185fa into mimoo:master Dec 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement schnorrkel and fix various idioms and linting errors #45

implement schnorrkel and fix various idioms and linting errors #45

actuallyachraf commented Nov 24, 2019

mimoo left a comment

mimoo Nov 25, 2019

mimoo Nov 25, 2019

actuallyachraf Nov 25, 2019

mimoo Nov 25, 2019

mimoo Nov 25, 2019

mimoo Nov 25, 2019

mimoo Nov 25, 2019

mimoo Nov 25, 2019

mimoo Nov 25, 2019

mimoo Nov 25, 2019

mimoo Nov 25, 2019

actuallyachraf Nov 25, 2019

actuallyachraf commented Nov 25, 2019 •

edited

Loading

mimoo commented Nov 26, 2019

mimoo Nov 27, 2019

actuallyachraf commented Nov 27, 2019

mimoo left a comment •

edited

Loading

mimoo Dec 4, 2019

actuallyachraf Dec 5, 2019

mimoo Dec 5, 2019

implement schnorrkel and fix various idioms and linting errors #45

implement schnorrkel and fix various idioms and linting errors #45

Conversation

actuallyachraf commented Nov 24, 2019

mimoo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

actuallyachraf commented Nov 25, 2019 • edited Loading

mimoo commented Nov 26, 2019

Choose a reason for hiding this comment

actuallyachraf commented Nov 27, 2019

mimoo left a comment • 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

actuallyachraf commented Nov 25, 2019 •

edited

Loading

mimoo left a comment •

edited

Loading