Repository: libp2p/go-libp2p
Pull request #1888 (2022-11-16 04:50:38)

tls / noise: prefer the client's muxer preferences
As the integration test (#1887) has revealed, TLS and Noise both selected the muxer according to the server's preferences, whereas the multistream negotiation selects according to the client's preferences. This PR changes TLS' and Noise's behavior to match the multistream behavior.

Since TLS' ALPN selection algorithm also selects according to the server's preferences, we have to play a little trick to overwrite that behavior.


Patch:
diff --git a/p2p/security/noise/transport.go b/p2p/security/noise/transport.go
index e5640197..6e2882b8 100644
--- 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/tls/transport.go b/p2p/security/tls/transport.go
index bb134e96..a78abec2 100644
--- 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 {


Test patch:
diff --git a/p2p/security/noise/transport_test.go b/p2p/security/noise/transport_test.go
index 15b46bc9..056afa39 100644
--- 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_test.go b/p2p/security/tls/transport_test.go
index 986dc63c..0d42eb1e 100644
--- 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)