Merge pull request #1888 from libp2p/early-muxer-negotiation-preferences

tls / noise: prefer the client's muxer preferences
libp2p · Nov 16, 2022 · 090a084 · 090a084
2 parents 04a43b1 + 0957a9d
commit 090a084
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 34 deletions.
diff --git a/p2p/security/noise/transport.go b/p2p/security/noise/transport.go
@@ -88,10 +88,10 @@ func (t *Transport) ID() protocol.ID {
 }
 
 func matchMuxers(initiatorMuxers, responderMuxers []string) string {
-	for _, muxer := range responderMuxers {
-		for _, initMuxer := range initiatorMuxers {
-			if initMuxer == muxer {
-				return muxer
+	for _, initMuxer := range initiatorMuxers {
+		for _, respMuxer := range responderMuxers {
+			if initMuxer == respMuxer {
+				return initMuxer
 			}
 		}
 	}

diff --git a/p2p/security/noise/transport_test.go b/p2p/security/noise/transport_test.go
@@ -632,21 +632,53 @@ func TestEarlyfffDataAcceptedWithNoHandler(t *testing.T) {
 }
 
 type noiseEarlyDataTestCase struct {
-	initProtos     []string
-	respProtos     []string
+	clientProtos   []string
+	serverProtos   []string
 	expectedResult string
 }
 
 func TestHandshakeWithTransportEarlyData(t *testing.T) {
 	tests := []noiseEarlyDataTestCase{
-		{initProtos: nil, respProtos: nil, expectedResult: ""},
-		{[]string{"muxer1"}, []string{"muxer1"}, "muxer1"},
-		{[]string{"muxer1"}, []string{}, ""},
-		{[]string{}, []string{"muxer2"}, ""},
-		{[]string{"muxer2"}, []string{"muxer1"}, ""},
-		{[]string{"muxer1/1.0.0", "muxer2/1.0.1"}, []string{"muxer2/1.0.1", "muxer1/1.0.0"}, "muxer2/1.0.1"},
-		{[]string{"muxer1/1.0.0", "muxer2/1.0.1", "muxer3/1.0.0"}, []string{"muxer2/1.0.1", "muxer1/1.0.1", "muxer3/1.0.0"}, "muxer2/1.0.1"},
-		{[]string{"muxer1/1.0.0", "muxer2/1.0.0"}, []string{"muxer3/1.0.0"}, ""},
+		{
+			clientProtos:   nil,
+			serverProtos:   nil,
+			expectedResult: "",
+		},
+		{
+			clientProtos:   []string{"muxer1"},
+			serverProtos:   []string{"muxer1"},
+			expectedResult: "muxer1",
+		},
+		{
+			clientProtos:   []string{"muxer1"},
+			serverProtos:   []string{},
+			expectedResult: "",
+		},
+		{
+			clientProtos:   []string{},
+			serverProtos:   []string{"muxer2"},
+			expectedResult: "",
+		},
+		{
+			clientProtos:   []string{"muxer2"},
+			serverProtos:   []string{"muxer1"},
+			expectedResult: "",
+		},
+		{
+			clientProtos:   []string{"muxer1", "muxer2"},
+			serverProtos:   []string{"muxer2", "muxer1"},
+			expectedResult: "muxer1",
+		},
+		{
+			clientProtos:   []string{"muxer3", "muxer2", "muxer1"},
+			serverProtos:   []string{"muxer2", "muxer1"},
+			expectedResult: "muxer2",
+		},
+		{
+			clientProtos:   []string{"muxer1", "muxer2"},
+			serverProtos:   []string{"muxer3"},
+			expectedResult: "",
+		},
 	}
 
 	noiseHandshake := func(t *testing.T, initProtos, respProtos []string, expectedProto string) {
@@ -662,24 +694,17 @@ func TestHandshakeWithTransportEarlyData(t *testing.T) {
 
 		initData := []byte("Test data for noise transport")
 		_, err := initConn.Write(initData)
-		if err != nil {
-			t.Fatal(err)
-		}
+		require.NoError(t, err)
 
 		respData := make([]byte, len(initData))
 		_, err = respConn.Read(respData)
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		if !bytes.Equal(initData, respData) {
-			t.Errorf("Data transmitted mismatch over noise session. %v != %v", initData, respData)
-		}
+		require.NoError(t, err)
+		require.Equal(t, initData, respData)
 	}
 
 	for _, test := range tests {
 		t.Run("Transport EarlyData Test", func(t *testing.T) {
-			noiseHandshake(t, test.initProtos, test.respProtos, test.expectedResult)
+			noiseHandshake(t, test.clientProtos, test.serverProtos, test.expectedResult)
 		})
 	}
 }
diff --git a/p2p/security/tls/transport.go b/p2p/security/tls/transport.go
@@ -68,7 +68,26 @@ func (t *Transport) SecureInbound(ctx context.Context, insecure net.Conn, p peer
 	for _, muxer := range t.muxers {
 		muxers = append(muxers, string(muxer))
 	}
-	// Prepend the prefered muxers list to TLS config.
+	// TLS' ALPN selection lets the server select the protocol, preferring the server's preferences.
+	// We want to prefer the client's preference though.
+	getConfigForClient := config.GetConfigForClient
+	config.GetConfigForClient = func(info *tls.ClientHelloInfo) (*tls.Config, error) {
+	alpnLoop:
+		for _, proto := range info.SupportedProtos {
+			for _, m := range muxers {
+				if m == proto {
+					// Match found. Select this muxer, as it's the client's preference.
+					// There's no need to add the "libp2p" entry here.
+					config.NextProtos = []string{proto}
+					break alpnLoop
+				}
+			}
+		}
+		if getConfigForClient != nil {
+			return getConfigForClient(info)
+		}
+		return config, nil
+	}
 	config.NextProtos = append(muxers, config.NextProtos...)
 	cs, err := t.handshake(ctx, tls.Server(insecure, config), keyCh)
 	if err != nil {

diff --git a/p2p/security/tls/transport_test.go b/p2p/security/tls/transport_test.go
@@ -185,14 +185,41 @@ type testcase struct {
 
 func TestHandshakeWithNextProtoSucceeds(t *testing.T) {
 	tests := []testcase{
-		{clientProtos: nil, serverProtos: nil, expectedResult: ""},
-		{clientProtos: []protocol.ID{"muxer1", "muxer2"}, serverProtos: []protocol.ID{"muxer2", "muxer1"}, expectedResult: "muxer2"},
-		{clientProtos: []protocol.ID{"muxer1", "muxer2", "libp2p"}, serverProtos: []protocol.ID{"muxer2", "muxer1", "libp2p"}, expectedResult: "muxer2"},
-		{clientProtos: []protocol.ID{"muxer1", "libp2p"}, serverProtos: []protocol.ID{"libp2p"}, expectedResult: ""},
-		{clientProtos: []protocol.ID{"libp2p"}, serverProtos: []protocol.ID{"libp2p"}, expectedResult: ""},
-		{clientProtos: []protocol.ID{"muxer1"}, serverProtos: []protocol.ID{}, expectedResult: ""},
-		{clientProtos: []protocol.ID{}, serverProtos: []protocol.ID{"muxer1"}, expectedResult: ""},
-		{clientProtos: []protocol.ID{"muxer2"}, serverProtos: []protocol.ID{"muxer1"}, expectedResult: ""},
+		{
+			clientProtos:   []protocol.ID{"muxer1", "muxer2"},
+			serverProtos:   []protocol.ID{"muxer2", "muxer1"},
+			expectedResult: "muxer1",
+		},
+		{
+			clientProtos:   []protocol.ID{"muxer1", "muxer2", "libp2p"},
+			serverProtos:   []protocol.ID{"muxer2", "muxer1", "libp2p"},
+			expectedResult: "muxer1",
+		},
+		{
+			clientProtos:   []protocol.ID{"muxer1", "libp2p"},
+			serverProtos:   []protocol.ID{"libp2p"},
+			expectedResult: "",
+		},
+		{
+			clientProtos:   []protocol.ID{"libp2p"},
+			serverProtos:   []protocol.ID{"libp2p"},
+			expectedResult: "",
+		},
+		{
+			clientProtos:   []protocol.ID{"muxer1"},
+			serverProtos:   []protocol.ID{},
+			expectedResult: "",
+		},
+		{
+			clientProtos:   []protocol.ID{},
+			serverProtos:   []protocol.ID{"muxer1"},
+			expectedResult: "",
+		},
+		{
+			clientProtos:   []protocol.ID{"muxer2"},
+			serverProtos:   []protocol.ID{"muxer1"},
+			expectedResult: "",
+		},
 	}
 
 	clientID, clientKey := createPeer(t)