From c64dd2c19dac8656242a17bbcccfddf2368d14b6 Mon Sep 17 00:00:00 2001 From: Yusef Napora Date: Wed, 4 Dec 2019 13:16:27 -0500 Subject: [PATCH 1/7] fix initiator / responder roles in test setup --- p2p/security/noise/transport_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/p2p/security/noise/transport_test.go b/p2p/security/noise/transport_test.go index 35c69fd53a..63b0048ed5 100644 --- a/p2p/security/noise/transport_test.go +++ b/p2p/security/noise/transport_test.go @@ -80,15 +80,15 @@ func newConnPair(t *testing.T) (net.Conn, net.Conn) { func connect(t *testing.T, initTransport, respTransport *Transport) (*secureSession, *secureSession) { init, resp := newConnPair(t) - var respConn sec.SecureConn - var respErr error + var initConn sec.SecureConn + var initErr error done := make(chan struct{}) go func() { defer close(done) - respConn, respErr = respTransport.SecureOutbound(context.TODO(), resp, initTransport.LocalID) + initConn, initErr = initTransport.SecureOutbound(context.TODO(), init, respTransport.LocalID) }() - initConn, initErr := initTransport.SecureInbound(context.TODO(), init) + respConn, respErr := respTransport.SecureInbound(context.TODO(), resp) <-done if initErr != nil { From a7d5094883e1608642605f52b89fe3048ce8b868 Mon Sep 17 00:00:00 2001 From: Yusef Napora Date: Wed, 4 Dec 2019 13:17:09 -0500 Subject: [PATCH 2/7] check right handshake is used in noise pipes tests --- p2p/security/noise/transport_test.go | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/p2p/security/noise/transport_test.go b/p2p/security/noise/transport_test.go index 63b0048ed5..585f64a3f1 100644 --- a/p2p/security/noise/transport_test.go +++ b/p2p/security/noise/transport_test.go @@ -176,25 +176,17 @@ func TestHandshakeXX(t *testing.T) { // Test IK handshake func TestHandshakeIK(t *testing.T) { - initTransport := newTestTransport(t, crypto.Ed25519, 2048) - respTransport := newTestTransport(t, crypto.Ed25519, 2048) - - // do initial XX handshake - initConn, respConn := connect(t, initTransport, respTransport) - initConn.Close() - respConn.Close() - - // turn on pipes, this will turn on IK - initTransport.NoisePipesSupport = true - respTransport.NoisePipesSupport = true + initTransport := newTestTransportPipes(t, crypto.Ed25519, 2048) + respTransport := newTestTransportPipes(t, crypto.Ed25519, 2048) // add responder's static key to initiator's key cache + respTransport.NoiseKeypair = GenerateKeypair() keycache := make(map[peer.ID]([32]byte)) keycache[respTransport.LocalID] = respTransport.NoiseKeypair.public_key initTransport.NoiseStaticKeyCache = keycache // do IK handshake - initConn, respConn = connect(t, initTransport, respTransport) + initConn, respConn := connect(t, initTransport, respTransport) defer initConn.Close() defer respConn.Close() @@ -213,6 +205,11 @@ func TestHandshakeIK(t *testing.T) { if !bytes.Equal(before, after) { t.Errorf("Message mismatch. %v != %v", before, after) } + + // make sure IK was actually used + if !(initConn.ik_complete && respConn.ik_complete) { + t.Error("Expected IK handshake to be used") + } } // Test noise pipes @@ -241,4 +238,9 @@ func TestHandshakeXXfallback(t *testing.T) { if !bytes.Equal(before, after) { t.Errorf("Message mismatch. %v != %v", before, after) } + + // make sure XX was actually used + if !(initConn.xx_complete && respConn.xx_complete) { + t.Error("Expected XXfallback handshake to be used") + } } From ca9fa6c563aa1dd2932738e74c671311a21e4505 Mon Sep 17 00:00:00 2001 From: Yusef Napora Date: Wed, 4 Dec 2019 13:17:57 -0500 Subject: [PATCH 3/7] use correct message decoder in XXfallback --- p2p/security/noise/xx_handshake.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/p2p/security/noise/xx_handshake.go b/p2p/security/noise/xx_handshake.go index 5eff33fa44..7a84564e83 100644 --- a/p2p/security/noise/xx_handshake.go +++ b/p2p/security/noise/xx_handshake.go @@ -202,7 +202,7 @@ func (s *secureSession) runHandshake_xx(ctx context.Context, fallback bool, payl } else { var msgbuf *xx.MessageBuffer - msgbuf, err = xx.Decode1(initialMsg) + msgbuf, err = xx.Decode0(initialMsg) if err != nil { log.Errorf("runHandshake_xx recv msg err", err) return err From fdf4c4428c894cd39f236451b24c7640a76d7fc6 Mon Sep 17 00:00:00 2001 From: Yusef Napora Date: Wed, 4 Dec 2019 13:18:28 -0500 Subject: [PATCH 4/7] use Errorf instead of Error --- p2p/security/noise/ik_handshake.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/p2p/security/noise/ik_handshake.go b/p2p/security/noise/ik_handshake.go index d05105e5a0..4231969d91 100644 --- a/p2p/security/noise/ik_handshake.go +++ b/p2p/security/noise/ik_handshake.go @@ -60,13 +60,13 @@ func (s *secureSession) ik_recvHandshakeMessage(initial_stage bool) (buf []byte, log.Debugf("ik_recvHandshakeMessage initiator=%v msgbuf=%v", s.initiator, msgbuf) if err != nil { - log.Error("ik_recvHandshakeMessage initiator=%v decode err=%s", s.initiator, err) + log.Errorf("ik_recvHandshakeMessage initiator=%v decode err=%s", s.initiator, err) return buf, nil, false, fmt.Errorf("ik_recvHandshakeMessage decode msg fail: %s", err) } s.ik_ns, plaintext, valid = ik.RecvMessage(s.ik_ns, msgbuf) if !valid { - log.Error("ik_recvHandshakeMessage initiator=%v err=%s", s.initiator, "validation fail") + log.Errorf("ik_recvHandshakeMessage initiator=%v err=%s", s.initiator, "validation fail") return buf, nil, false, fmt.Errorf("ik_recvHandshakeMessage validation fail") } From 0a7571794415d3193b9018e10f8b3a76fabe1381 Mon Sep 17 00:00:00 2001 From: Yusef Napora Date: Wed, 4 Dec 2019 13:18:52 -0500 Subject: [PATCH 5/7] fix hash function in Noise protocol name --- p2p/security/noise/ik/IK.noise.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/p2p/security/noise/ik/IK.noise.go b/p2p/security/noise/ik/IK.noise.go index 61a11ed038..724a98e572 100644 --- a/p2p/security/noise/ik/IK.noise.go +++ b/p2p/security/noise/ik/IK.noise.go @@ -409,7 +409,7 @@ func initializeInitiator(prologue []byte, s Keypair, rs [32]byte, psk [32]byte) var ss symmetricstate var e Keypair var re [32]byte - name := []byte("Noise_IK_25519_ChaChaPoly_BLAKE2s") + name := []byte("Noise_IK_25519_ChaChaPoly_SHA256") ss = initializeSymmetric(name) mixHash(&ss, prologue) mixHash(&ss, rs[:]) @@ -420,7 +420,7 @@ func initializeResponder(prologue []byte, s Keypair, rs [32]byte, psk [32]byte) var ss symmetricstate var e Keypair var re [32]byte - name := []byte("Noise_IK_25519_ChaChaPoly_BLAKE2s") + name := []byte("Noise_IK_25519_ChaChaPoly_SHA256") ss = initializeSymmetric(name) mixHash(&ss, prologue) mixHash(&ss, s.public_key[:]) From 423803f233b7f1b39abf1f58ab1d133e80c3a9e8 Mon Sep 17 00:00:00 2001 From: Yusef Napora Date: Wed, 4 Dec 2019 13:25:11 -0500 Subject: [PATCH 6/7] fix conditional for whether to use IK --- p2p/security/noise/protocol.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/p2p/security/noise/protocol.go b/p2p/security/noise/protocol.go index 259016a083..766ed565de 100644 --- a/p2p/security/noise/protocol.go +++ b/p2p/security/noise/protocol.go @@ -173,11 +173,15 @@ func (s *secureSession) runHandshake(ctx context.Context) error { return fmt.Errorf("runHandshake proto marshal payload err=%s", err) } - // if we have the peer's noise static key (and we're the initiator,) and we support noise pipes, - // we can try IK first. if we support noise pipes and we're not the initiator, try IK first - // otherwise, default to XX - if (!s.initiator && s.noiseStaticKeyCache[s.remotePeer] != [32]byte{}) && s.noisePipesSupport { - // known static key for peer, try IK + // If we support Noise pipes, we try IK first, falling back to XX if IK fails. + // The exception is when we're the initiator and don't know the other party's + // static Noise key. Then IK will always fail, so we go straight to XX. + tryIK := s.noisePipesSupport + if s.initiator && s.noiseStaticKeyCache[s.remotePeer] == [32]byte{} { + tryIK = false + } + if tryIK { + // we're either a responder or an initiator with a known static key for the remote peer, try IK buf, err := s.runHandshake_ik(ctx, payloadEnc) if err != nil { log.Error("runHandshake ik err=%s", err) From c3157aaaa13adc7ca9659269624e1eb23bc2b08d Mon Sep 17 00:00:00 2001 From: Yusef Napora Date: Wed, 4 Dec 2019 13:25:34 -0500 Subject: [PATCH 7/7] only set ik_complete = true if IK succeeds --- p2p/security/noise/protocol.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/p2p/security/noise/protocol.go b/p2p/security/noise/protocol.go index 766ed565de..77889c649c 100644 --- a/p2p/security/noise/protocol.go +++ b/p2p/security/noise/protocol.go @@ -194,10 +194,9 @@ func (s *secureSession) runHandshake(ctx context.Context) error { } s.xx_complete = true + } else { + s.ik_complete = true } - - s.ik_complete = true - } else { // unknown static key for peer, try XX err := s.runHandshake_xx(ctx, false, payloadEnc, nil)