Connection multiaddress should match the address actually dialed (WSS, WebTransport, WebRTC) #2988

aschmahmann · 2024-09-30T20:06:26Z

It seems like a bug that wssConn.RemoteMultiaddr() doesn't contain the TLS or SNI components (and may also have an over-resolved domain name, but haven't tested that yet) and shows a much simpler multiaddr.

Context: I noticed the domain names of wss addresses were not showing up in the https://check.ipfs.network tool due to https://github.com/ipfs/ipfs-check/blob/b35461c0ac6dda2f32efceec52f2559f4c105bc4/daemon.go#L256

If this is buggy behavior it seems like this can be fixed in the websocket conn type, should a generic test for this be added to the transport suite?

aschmahmann

Left a number of comments outlining logic or asking questions. @MarcoPolo @sukunrt lmk if this looks right to you.

This is technically a breaking change since if people were depending on the wrong information it could be a problem, I'm not sure in practice if this will be an issue though.

aschmahmann · 2024-10-08T19:20:21Z

p2p/transport/websocket/websocket_test.go

+	ttransport.SubtestTransport(t, ta, tb, fmt.Sprintf("/ip4/127.0.0.1/tcp/0/tls/sni/%s/ws", dnsName), peerA)
+}
+
+func generateSelfSignedCert(dnsName string) ([]byte, []byte, error) {


This code is the same between the transport suites, is there a better place I should put it so it can deduplicate?

aschmahmann · 2024-10-08T19:21:45Z

p2p/transport/webtransport/transport.go

-	remote, err := toWebtransportMultiaddr(sess.RemoteAddr())
-	if err != nil {
-		return nil, fmt.Errorf("error determining remote addr: %w", err)
-	}


The assumption here is that the remote address should be what we passed in which seems right. If it was wrong that'd seem to indicate a big problem, right?

aschmahmann · 2024-10-08T19:22:10Z

p2p/transport/webtransport/listener.go

-	if err != nil {
-		return nil, fmt.Errorf("error determiniting local addr: %w", err)
-	}
+	local := l.Multiaddr()


Maybe we should cache this computed information like we do in some of the other transports like WebRTC

aschmahmann · 2024-10-08T19:25:51Z

p2p/transport/websocket/conn.go

+// NewConn creates a Conn given a regular gorilla/websocket Conn.
+func NewOutboundConn(raw *ws.Conn, secure bool, sni string) (*Conn, error) {


NewConn doesn't seem needed anymore (and not sure why it was exported in the first place). There's some shared code between Inbound/Outbound which can be deduplicated but:

Should the function continue to be exported? (seems like not)

Any preference for different names vs an inbound/outbound bool?

aschmahmann · 2024-10-08T19:28:23Z

p2p/test/transport/transport_test.go

+			if opts.NoListen {
+				libp2pOpts = append(libp2pOpts, libp2p.NoListenAddrs)
+			} else {
+				dnsName := fmt.Sprintf("example%s.com", opts.HostSeed)


Can drop opts.HostSeed as it's currently unused. However, this seems like it'd be necessary if we had multiple servers in the same test, it just looks like the set of tests used in this file only have a single server. Should we remove the code but drop a comment in or something?

aschmahmann · 2024-10-08T19:28:55Z

p2p/test/transport/transport_test.go

+		Name: "Secure WebSocket with CA Certificate",
+		HostGenerator: func(t *testing.T, opts TransportTestCaseOpts) host.Host {
+			libp2pOpts := transformOpts(opts)
+			wsOpts := []interface{}{websocket.WithTLSClientConfig(&tls.Config{InsecureSkipVerify: true})}


We could also add a shared rootCA here, thoughts?

aschmahmann · 2024-10-08T19:31:23Z

p2p/test/transport/transport_test.go

+				libp2pOpts = append(libp2pOpts, libp2p.ListenAddrStrings(fmt.Sprintf("/ip4/127.0.0.1/tcp/0/tls/sni/%s/ws", dnsName)))
+				wsOpts = append(wsOpts, websocket.WithTLSConfig(&tls.Config{Certificates: []tls.Certificate{cert}}))
+			}
+			libp2pOpts = append(libp2pOpts, libp2p.Transport(websocket.New, wsOpts...))


This makes WSS a standout compared to the others in that the others all support multiple transports, but this host only supports WSS. Should the other host generators also be narrowed down to support a single transport?

aschmahmann · 2024-10-08T19:32:36Z

p2p/test/transport/transport_test.go

@@ -113,6 +122,26 @@ var transportsToTest = []TransportTestCase{
 			return h
 		},
 	},
+	{
+		Name: "Secure WebSocket with CA Certificate",


As with the WebSockets example the second security layer negotiation + multiplexer are left undefined (i.e. use the defaults), is that a mistake?

aschmahmann · 2024-10-08T19:39:51Z

p2p/test/transport/gating_test.go

+					// remove the certhash component from WebRTC and WebTransport addresses
 					require.Equal(t, stripCertHash(h2.Addrs()[0]), addrs.LocalMultiaddr())
 				}).AnyTimes()
 			} else {
 				connGater.EXPECT().InterceptAccept(gomock.Any()).Do(func(addrs network.ConnMultiaddrs) {
-					// remove the certhash component from WebTransport addresses
+					// remove the certhash component from WebRTC and WebTransport addresses
 					require.Equal(t, stripCertHash(h2.Addrs()[0]), addrs.LocalMultiaddr())


Wasn't sure what the correct move was for the connection gaters (both in terms of what should be expected and how people are using this and how it might break).

At the moment I made a decision here to strip out the certhashes for InterceptAccept but not InterceptSecured based on the idea that Accept shouldn't really be thinking about security-related, but if it's available maybe it's still worth exposing and perhaps the complexity of being different is too confusing.

Either way, once we decide what's going on here I think adding better documentation to the interfaces seems needed

aschmahmann · 2024-10-08T19:49:01Z

p2p/test/transport/transport_test.go

@@ -798,3 +867,29 @@ func TestConnClosedWhenRemoteCloses(t *testing.T) {
 		})
 	}
 }
+
+func TestConnMatchingAddress(t *testing.T) {


Here's the new test with the behavior that IIUC is that's expected and was driving most of the changes (although the connection gater ones might be different)

…ncluding sni and certhash components

… having certhashes

aschmahmann · 2024-10-08T20:56:47Z

p2p/transport/webtransport/transport_test.go

+		require.Equal(t, ma.StringCast(fmt.Sprintf("/ip4/127.0.0.1/udp/%s/quic-v1/webtransport", port)), stripCertHashes(conn.RemoteMultiaddr()))
+		require.Equal(t, 2, len(extractCertHashes(conn.RemoteMultiaddr())))


Changes like the ones in this file make me wonder if the fixes here are going to result in anyone's code breaking.

aschmahmann · 2024-10-08T21:05:06Z

p2p/transport/websocket/conn.go

+}
+
+// newConn creates a Conn given a regular gorilla/websocket Conn.
+func newConn(raw *ws.Conn, secure bool, sni string, inbound bool) (*Conn, error) {


This function could use some review. Additionally, I don't feel like we've tested the /dns/example.com/tcp/80/wss case very well in the associated tests. It seems like maybe we could use with a normalization function within go-libp2p for working with WSS addresses since IIUC we have a number of forms:

Ending in /wss or /tls/ws for when no SNI is needed

The /dns (and dns4/6) which implies the domain to use for verification if there's no /sni component (can end in /tls/ws or /wss)

Ending with /tls/sni/<domain>/ws

I haven't looked at this PR yet, but the connection's RemoteMultiaddr should not have a dns component. It should have the IP address that is used for the current connection (along with tls/sni info).

Apologies if that's already the case, and I haven't looked at this yet.

sukunrt mentioned this pull request Oct 5, 2024

feat(tcpreuse): add options for sharing TCP listeners amongst TCP, WS and WSS transports #2984

Open

aschmahmann force-pushed the fix/wss-conn-ma branch 2 times, most recently from 080d716 to 31e5123 Compare October 8, 2024 19:11

aschmahmann added 2 commits October 8, 2024 15:14

test(websocket): enable wss transport test

2f126d0

test(websocket): enable wss in newer transport testing

f008fe3

aschmahmann force-pushed the fix/wss-conn-ma branch from 31e5123 to 6dbd3ce Compare October 8, 2024 19:17

aschmahmann changed the base branch from fix/re-enable-ws-transport-tests to master October 8, 2024 19:17

aschmahmann force-pushed the fix/wss-conn-ma branch from 6dbd3ce to b411bd5 Compare October 8, 2024 19:18

aschmahmann commented Oct 8, 2024

View reviewed changes

aschmahmann force-pushed the fix/wss-conn-ma branch from b411bd5 to bac732a Compare October 8, 2024 20:17

fix(transport): make connection multiaddrs match the full multiaddr i…

2bf2ad6

…ncluding sni and certhash components

aschmahmann force-pushed the fix/wss-conn-ma branch from bac732a to 2bf2ad6 Compare October 8, 2024 20:17

test(webtransport): change tests to account for connection multiaddrs…

f9d8302

… having certhashes

aschmahmann commented Oct 8, 2024

View reviewed changes

aschmahmann marked this pull request as ready for review October 8, 2024 21:05

aschmahmann changed the title ~~WSS connection multiaddress should match the address actually dialed~~ Connection multiaddress should match the address actually dialed (WSS, WebTransport, WebRTC) Oct 8, 2024

MarcoPolo mentioned this pull request Oct 9, 2024

v0.37 #2901

Open

29 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connection multiaddress should match the address actually dialed (WSS, WebTransport, WebRTC) #2988

Connection multiaddress should match the address actually dialed (WSS, WebTransport, WebRTC) #2988

aschmahmann commented Sep 30, 2024

aschmahmann left a comment

aschmahmann Oct 8, 2024

aschmahmann Oct 8, 2024

aschmahmann Oct 8, 2024

aschmahmann Oct 8, 2024

aschmahmann Oct 8, 2024

aschmahmann Oct 8, 2024

aschmahmann Oct 8, 2024

aschmahmann Oct 8, 2024

aschmahmann Oct 8, 2024

aschmahmann Oct 8, 2024

aschmahmann Oct 8, 2024

aschmahmann Oct 8, 2024

MarcoPolo Oct 8, 2024

		// NewConn creates a Conn given a regular gorilla/websocket Conn.
		func NewOutboundConn(raw ws.Conn, secure bool, sni string) (Conn, error) {

		require.Equal(t, ma.StringCast(fmt.Sprintf("/ip4/127.0.0.1/udp/%s/quic-v1/webtransport", port)), stripCertHashes(conn.RemoteMultiaddr()))
		require.Equal(t, 2, len(extractCertHashes(conn.RemoteMultiaddr())))

Connection multiaddress should match the address actually dialed (WSS, WebTransport, WebRTC) #2988

Are you sure you want to change the base?

Connection multiaddress should match the address actually dialed (WSS, WebTransport, WebRTC) #2988

Conversation

aschmahmann commented Sep 30, 2024

aschmahmann left a 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

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