Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

aschmahmann
Copy link
Collaborator

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 aschmahmann changed the base branch from fix/re-enable-ws-transport-tests to master October 8, 2024 19:17
Copy link
Collaborator Author

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines -238 to -241
remote, err := toWebtransportMultiaddr(sess.RemoteAddr())
if err != nil {
return nil, fmt.Errorf("error determining remote addr: %w", err)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 38 to 39
// NewConn creates a Conn given a regular gorilla/websocket Conn.
func NewOutboundConn(raw *ws.Conn, secure bool, sni string) (*Conn, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Should the function continue to be exported? (seems like not)
  2. Any preference for different names vs an inbound/outbound bool?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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})}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also add a shared rootCA here, thoughts?

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...))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +172 to 178
// 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())
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

func TestConnMatchingAddress(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment on lines +136 to +137
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())))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@aschmahmann aschmahmann marked this pull request as ready for review October 8, 2024 21:05
@aschmahmann 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 MarcoPolo mentioned this pull request Oct 9, 2024
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants