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

feat: allow dialing wss peers using DNS multiaddrs #1592

Merged
merged 1 commit into from
Jun 20, 2022

Conversation

aschmahmann
Copy link
Collaborator

@aschmahmann aschmahmann commented Jun 9, 2022

fixes #1593

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

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
Copy link
Contributor

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
Copy link
Contributor

BigLep commented Jun 9, 2022

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

@marten-seemann
Copy link
Contributor

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
Copy link
Contributor

BigLep commented Jun 9, 2022

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
Copy link
Contributor

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

aschmahmann commented Jun 10, 2022

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.

@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
Copy link
Contributor

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
Copy link
Collaborator

MarcoPolo commented Jun 17, 2022

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

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.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

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).

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.

Allow dialing wss peers using DNS multiaddrs
4 participants