search

mimoo / NoiseGo

An implementation of Noise in Go
41 stars 9 forks source link

Retrieve long term static key for Conn #4

Closed nikkolasg closed 6 years ago

nikkolasg commented 6 years ago

Hi David!

It's Nicolas, working at EPFL, we met @RWC18. Thank you for this library, especially the net.Conn interface, that's what made me change to go here ;)

However, with this PR, I'd like to add the possiblity of getting the static key which is sent during a handshake after the handshake's completed. It's especially useful on the server side (listening side) when all static keys are known, which peer contacted us.

I hope it's still correct. Especially the isAuthenticated = true, it may not be necessary but I was thinking that it should be there..

Looking forwards to your comments !

mimoo commented 6 years ago

Hey Nicolas! Glad to see you ended up here from rwc :)

I posted some comments on your PR, I'm looking forward hearing your thoughts on these.

nikkolasg commented 6 years ago

I've removed the whole isRemoteAuthenticated story since it's not clear what it is supposed to do yet but I've kept the handshakeCompleted check. Let me know what you think :) EDIT: jai ete trop vite en besogne, un ptit moment !

nikkolasg commented 6 years ago

Avec un ptit test en plus c'est mieux :) Je te conseille vivement de setup Travis (minimal config travis.yml file here ) !

mimoo commented 6 years ago

Switching to english because people might read us here ;)

And sorry, one last comment on your PR :)

I'm gonna look at Travis, good idea.

mimoo commented 6 years ago

Alright, added Travis! Thanks for the suggestion.

nikkolasg commented 6 years ago

Turns out it also raises some questions about the way you use struct vs pointers. If I don't copy the static key, in Handshake you create a copy of the c.hs variable, and work on the copy instead of the c.hs. So in StaticKey I can't retrieve the static key anymore. I can make a quick hack hs := &c.hs for this PR, and it would work. However I think you should start looking at using pointers instead of just by-value (very common in Go) because sometimes it's not clear on which struct we're working on, and you've been lucky so far with the hs thing it has been undetected! Tell me your thoughts on this please...

(Just try to remove the hs.clear() and my "copy" to see what I'm talking about if it's not clear)

nikkolasg commented 6 years ago

See the latest commit to see if that's ok with you.

mimoo commented 6 years ago

Hey! Sorry for the delay, I've been pretty busy lately, will get to review this shortly :) And thanks for the thoughts, that's exactly what I wanted!

mimoo commented 6 years ago

you've been lucky so far with the hs thing it has been undetected!

Good catch with the c.hs never being updated! I think I got away with it because handshake() was blocking, so I didn't really need to have a hs field in the conn.

However I think you should start looking at using pointers instead of just by-value (very common in Go) because sometimes it's not clear on which struct we're working on

I've been trying to avoid pointers where I think it does not optimize the code that much and provides more confusion vs simply using values. (I think the Go blog has a blogpost on this, which is why I do it.)

But if you think there are places in the code where it would be better to use pointers, submit a PR :)

Anyway this looks good to me, I'll probably think of just doing operations on c.hs instead of using a hs := &c.hs in the future.

Thanks for that awesome PR! (and for the discussions)

mimoo commented 5 years ago

I'm trying to port this to https://github.com/mimoo/disco if you find that interesting take a look, I've:

How do you use this function with the standard Dial() and Listen()/Accept() as they return a net.Conn and not a disco.Conn? I've added a ListenDisco() that returns a disco.Listener and a AcceptDisco() that returns a disco.Conn so that I can use these functions.

In addition I've added a conn.Write([]byte{}) in AcceptDisco() so that the handshake is performed as soon as possible (instead of waiting for the user to call Write or Read)

nikkolasg commented 5 years ago

@mimoo cool, looks like you're progressing well ! Unfortunately, my project is on a hold for some time at the moment.. I'll definitely have a look at disco when I'll take it up again.