feat: allow dialing wss peers using DNS multiaddrs

[aschmahmann]

fixes #1593

This fixes an issue where the WSS dialer can only handle IP multiaddrs not DNS

go-libp2p/p2p/transport/websocket/websocket.go

Line 26 in d7ba372

var dialMatcher = mafmt.And(mafmt.IP, mafmt.Base(ma.P_TCP), mafmt.Or(mafmt.Base(ma.P_WS), mafmt.Base(ma.P_WSS)))

A couple areas to consider here for improvement (recommendations or maintainers pushing commits to this PR are welcome).

• We should use the host's DNS resolver rather than the OS resolver
• The basic host should probably not resolve the DNS addresses covered by WSS, or the dialer should at least prioritize DNS WSS dials over IP ones

[marten-seemann]

Does it make sense to dial WSS in the first place? We might want to consider disabling WSS dialing altogether (except for tests), as any node offering WSS will also offer TCP and that's always preferable.

We should use the host's DNS resolver rather than the OS resolver

Can we pass the resolver to this transport? Probably would have to teach Steven's constructor magic that there's a new parameter.

[BigLep]

Is there a way to have a test to demonstrate correctness?

[marten-seemann]

Does it make sense to dial WSS in the first place? We might want to consider disabling WSS dialing altogether (except for tests), as any node offering WSS will also offer TCP and that's always preferable.

Thinking about it, this logic should probably live in the swarm.

Is there a way to have a test to demonstrate correctness?

@BigLep, what do you mean? We have unit tests for dialing.

[BigLep]

Is there a way to have a test to demonstrate correctness?

@BigLep, what do you mean? We have unit tests for dialing.

We have a source code change without a corresponding test. I'm assuming we didn't have a failing test in the past that this code is now fixing. If that is true, then is there a test we should add to show this works as intended?

[marten-seemann]

Yes, I think we should:

• stop resolving wss addresses in the host
• pass the resolver to the WebSocket transport
• not dial wss addresses if we have a TCP address from the same peer

[aschmahmann]

not dial wss addresses if we have a TCP address from the same peer

Should we prefer WS over WSS as well, or the other way around? It seems like generally either what we have here is sufficient or we shouldn't be using ranking independent of this PR.

go-libp2p/p2p/net/swarm/dial_worker.go

Line 283 in d7ba372

// NonWS > WS

@marten-seemann are you able to take a look at this, or is this waiting on me/someone else to push things along?

Areas where I feel most in need of help here:

• A commit that uses the custom DNS resolver with the WSS library that'd be great. I can probably figure out the dependency injection, but you likely have more experience than me messing around with the TLS configs and/or gorilla
• How we want this tested before we're happy merging

[marten-seemann]

It seems like generally either what we have here is sufficient or we shouldn't be using ranking independent of this PR.

If I understand correctly, this ranking function isn't particularly useful, since we dial 8 addresses at the same time. My point is that we shouldn't dial WebSocket at all (if we have a TCP address), as there's no benefit in first establishing a TCP connection, the layering WebSocket on top of that, and then speaking libp2p. The only reason we do this is to enable browsers to connect to us. The same will apply for WebTransport and QUIC addresses.

Should we prefer WS over WSS as well, or the other way around?

We should prefer WS, WSS doesn't buy us anything, unless we encode the libp2p peer ID into the certificate (like we do for the TLS handshake). Since we don't expect to use WSS in any other environment than browsers (that don't provide an API to access the certificate), there's not really a point in doing so.

How we want this tested before we're happy merging

We should have unit tests in the websocket package that show that we can dial DNS multiaddrs (we might need to mock the DNS resolver for that). Given how close we are to basic Testground interop tests, I think we can defer integration tests.

[MarcoPolo]

I know this isn't the ideal solution, but it does fix dialing WSS with dns multiaddrs. But what are the downsides of merging this and doing the proper solution later?

[aschmahmann]

But what are the downsides of merging this and doing the proper solution later?

The only downside I can think of at the moment (aside from wasted dials, but IMO that's not as bad as failing to dial at all) is that there's potentially a security leak if someone doesn't want go-libp2p to be using the OS resolver since this will use the OS resolver rather than the one given to the libp2p host.

[marten-seemann]

Let's merge this for now, with the understanding that we'll work on a proper fix (#1597) soon. Unfortunately, we probably won't be able to tackle that issue as long as we're still working on rcmgr stuff, but I've put it on the release checklist for v0.21.0 (#1514).