Retrieve long term static key for Conn #4

nikkolasg · 2018-01-22T20:38:04Z

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 · 2018-01-23T14:38:30Z

noise/conn.go

@@ -319,6 +320,9 @@ ContinueHandshake:
 			if !c.config.PublicKeyVerifier(hs.rs.PublicKey[:], receivedPayload) {
 				return errors.New("Noise: the received public key could not be authenticated")
 			}
+			c.isRemoteAuthenticated = true


good point, I was toying with this idea of having a isRemoteAuthenticated value you can check.

I'm not sure it is useful as the handshake you're using should make it pretty clear that you're authenticating the remote, but I thought it could be "useful" as a defense-in-depth mechanism. What do you think?

It should also probably be exported if it's meant to be used by developers.

I put it there because I used the IsAuthenticated method in StaticKey. If it's not really needed (see comment below) then we can totally drop that.
In my opinion, the semantic of IsAuthenticated is a bit vague or redundant: do you mean authenticated with a longterm static key ? Because users of that library must intentionally declare the pattern they want to use and if the handshake succeeds and a static key must have been authenticated in some way, then it is by default Authenticated no ?

authenticated with a longterm static key?

not necessarily, I am trying to get back into the state of mind I had when I wrote this. But I think I wanted to create an out-of-band authentication. For example something like Noise_NN where you would extract a fingerprint and authenticate the connection post-handshake. Noise_NNoob maybe? Although I'm not sure it deserves a name. By having this isRemoteAuthenticated you can check if peers have actually verified the fingerprint. You can think of some application that would want to show some features only if this var been set.

mimoo · 2018-01-23T14:40:37Z

noise/conn.go

@@ -17,6 +17,7 @@ type Conn struct {
 	// handshake
 	config            *Config // configuration passed to constructor
 	hs                handshakeState
+	remotePub         []byte


why not making it a [32]byte to avoid having to make during the handshake?

Becase I wanted to see if you can see it ? ahah. well spotted.

mimoo · 2018-01-23T14:41:47Z

noise/conn.go

+// StaticKey returns the static key of the remote peer. It is useful in case the
+// static key is only transmitted during the handshake.
+func (c *Conn) StaticKey() ([]byte, error) {
+	if !c.IsRemoteAuthenticated() {


Are there usecases where you would still want to obtain the public key, even if the peer has not been authenticated yet? Maybe if you're planning to authenticate post-handshake (using a short-authentication string or something like that)

True. I was not sure about that, so I just thought going to the safest path. But Is it really possible to get the static key without verifying ? This library panics if there's no PublicKeyVerifier in the Config IIRC .

true.

Another scenario I can think of: setting up the PublicKeyVerifier() based on the static public key you received. But this is currently not possible with this library.

mimoo · 2018-01-23T14:42:13Z

noise/conn.go

+	if !c.IsRemoteAuthenticated() {
+		return nil, errors.New("noise: remote peer not authenticated")
+	}
+	if !c.handshakeComplete {


is it important that the handshake has to be finished?

Mhhhh I wanna make sure that the key I get when calling StaticKey is the right one (i.e. verified by PublicKeyVerifier). Maybe it is possible to deliver it before the handshake completely finishes, but in my use case, I'm fine with waiting the handshake to be finished so I'm sure.
In any case, looking at the code where the PublicKeyVerifier gets called, it's almost at the end of the Handshake method so not even sure what we can do "earlier".

mimoo · 2018-01-23T14:43:24Z

noise/conn.go

@@ -319,6 +320,9 @@ ContinueHandshake:
 			if !c.config.PublicKeyVerifier(hs.rs.PublicKey[:], receivedPayload) {
 				return errors.New("Noise: the received public key could not be authenticated")
 			}
+			c.isRemoteAuthenticated = true
+			c.remotePub = make([]byte, 32)
+			copy(c.remotePub, hs.rs.PublicKey[:])


shouldn't you do this when receiving the s token?

See comment above. I just want to get the static key when it has been verified.
BTW, It is possible also to retrieve the value directly from the PublicKeyVerifier but having a StaticKey method is more practical code-wise.

mimoo · 2018-01-23T14:46:35Z

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.

mimoo · 2018-01-24T12:20:21Z

noise/conn.go

+	if c.remotePub == nil {
+		return nil, errors.New("noise: no remote static key given")
+	}
+	return c.remotePub, nil


why not returning hs.rs.PublicKey instead? This way you wouldn't have to create a c.remotePub and copy the public key into it.

Because the hs.rs is cleared when the handshake completes (c.hs.clear()) and the clearing clears up also the public key. I think it could be relaxed to only clear up the private key but better be safe than sorry.

nikkolasg · 2018-01-24T20:02:27Z

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 · 2018-01-24T20:28:50Z

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

mimoo · 2018-01-25T17:57:16Z

noise/conn.go

@@ -17,6 +17,8 @@ type Conn struct {
 	// handshake
 	config            *Config // configuration passed to constructor
 	hs                handshakeState
+	remotePub         [32]byte // remote static public key


Because the hs.rs is cleared when the handshake completes (c.hs.clear()) and the clearing clears up also the public key. I think it could be relaxed to only clear up the private key but better be safe than sorry.

So. Can we think of good reasons for clear() to clear the public keys? If not, I'd rather have us modify the behavior of clear() rather than adding new fields here :o

The reasons to clear a key from memory is to avoid an attacker being able to read it (either because he's on the same machine or by using heartbleed style attacks, etc). Usually we clear a private key because that's what is most urgent to keep secret.
In a far far fetched scenario, we might also want to clear up the static public key to be able to deny later that we were participating in the communication ? (if we load the key via Yubikey on a computer which got compromised just after we used it so it still has the memory kept ? )
Lol this seems way too far fetched from me. So if you also can't find a realistic scenario, then let me know and I'll modify clear() so it only clears the private key.

yeah I'm not particularly interested in the deniability property which Noise doesn't provide in any case. I think a better idea would be to stop clear() from removing the public keys. If you can make this change I'll accept your PR! Thanks again for the initial PR and the changes :) you rock

mimoo · 2018-01-25T18:03:01Z

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 · 2018-01-25T19:52:29Z

Alright, added Travis! Thanks for the suggestion.

nikkolasg · 2018-01-26T12:56:04Z

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 · 2018-01-26T15:29:26Z

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

mimoo · 2018-02-03T12:52:09Z

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 · 2018-02-04T13:38:08Z

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 · 2018-10-08T14:47:17Z

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

removed thehs from the Conn struct since it looks like it's unused.
set the IsRemoteAuthenticated if there is a verification happening (which is still un-usued in NoiseGo).
added a remotePublicKey() function

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 · 2018-10-09T09:47:33Z

@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.

added StaticKey()

73b524a

mimoo reviewed Jan 23, 2018

View reviewed changes

mimoo reviewed Jan 24, 2018

View reviewed changes

fixed some commits

4c05fea

nikkolasg added 2 commits January 24, 2018 21:17

fix build errors

2d4c4b1

added statickey test

ecc3f3c

mimoo reviewed Jan 25, 2018

View reviewed changes

one proposal

e421856

mimoo merged commit 2ec65f8 into mimoo:master Feb 4, 2018

mimoo mentioned this pull request Oct 8, 2018

Document APIs mimoo/disco#29

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retrieve long term static key for Conn #4

Retrieve long term static key for Conn #4

nikkolasg commented Jan 22, 2018 •

edited

mimoo Jan 23, 2018

nikkolasg Jan 24, 2018

mimoo Jan 24, 2018 •

edited

mimoo Jan 23, 2018

nikkolasg Jan 24, 2018

mimoo Jan 23, 2018

nikkolasg Jan 24, 2018

mimoo Jan 24, 2018

mimoo Jan 23, 2018

nikkolasg Jan 24, 2018

mimoo Jan 23, 2018

nikkolasg Jan 24, 2018

mimoo commented Jan 23, 2018 •

edited

mimoo Jan 24, 2018

nikkolasg Jan 24, 2018

nikkolasg commented Jan 24, 2018 •

edited

nikkolasg commented Jan 24, 2018

mimoo Jan 25, 2018

nikkolasg Jan 25, 2018 •

edited

mimoo Jan 25, 2018

mimoo commented Jan 25, 2018

mimoo commented Jan 25, 2018

nikkolasg commented Jan 26, 2018 •

edited

nikkolasg commented Jan 26, 2018

mimoo commented Feb 3, 2018

mimoo commented Feb 4, 2018 •

edited

mimoo commented Oct 8, 2018 •

edited

nikkolasg commented Oct 9, 2018

Retrieve long term static key for Conn #4

Retrieve long term static key for Conn #4

Conversation

nikkolasg commented Jan 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimoo Jan 24, 2018 • edited

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

mimoo commented Jan 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikkolasg commented Jan 24, 2018 • edited

nikkolasg commented Jan 24, 2018

Choose a reason for hiding this comment

nikkolasg Jan 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimoo commented Jan 25, 2018

mimoo commented Jan 25, 2018

nikkolasg commented Jan 26, 2018 • edited

nikkolasg commented Jan 26, 2018

mimoo commented Feb 3, 2018

mimoo commented Feb 4, 2018 • edited

mimoo commented Oct 8, 2018 • edited

nikkolasg commented Oct 9, 2018

nikkolasg commented Jan 22, 2018 •

edited

mimoo Jan 24, 2018 •

edited

mimoo commented Jan 23, 2018 •

edited

nikkolasg commented Jan 24, 2018 •

edited

nikkolasg Jan 25, 2018 •

edited

nikkolasg commented Jan 26, 2018 •

edited

mimoo commented Feb 4, 2018 •

edited

mimoo commented Oct 8, 2018 •

edited