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

/dnsaddr can not be properly used with secure WebSocket transport #5529

Open
oblique opened this issue Aug 5, 2024 · 1 comment
Open

/dnsaddr can not be properly used with secure WebSocket transport #5529

oblique opened this issue Aug 5, 2024 · 1 comment

Comments

@oblique
Copy link
Contributor

oblique commented Aug 5, 2024

Description

Currently there is no way to properly pass /dnsaddr to WSS transport and work as expected:

  1. If you wrap libp2p::websocket::WsConfig with libp2p::dns::Transport then you break secure WebSocket because DNS transport will resolve /dns4 to /ip4.
  2. If you wrap libp2p::dns::Transport with libp2p::websocket::WsConfig then you break /dnsaddr because it will never fetch the multiaddresses from DNS.

Currently 2 is the recommended way (link).

Motivation

I want to be able to use /dnsaddr with secure WebSocket.

Requirements

I want to be able to wrap libp2p::websocket::WsConfig with libp2p::dns::Transport which will resolve only /dnsaddr before it passing the multiaddr to WebSocket transport.

For example:

let inner_tcp_transport =
    dns::tokio::Transport::system(tcp::tokio::Transport::new(tcp::Config::default()))
        .unwrap();

let wss_transport = websocket::WsConfig::new(inner_tcp_transport);

let mut dns_opts = dns::ResolverOpts::default();
dns_opts.resolve_dnsaddr_only = true;

let transport =
    dns::tokio::Transport::custom(wss_transport, dns::ResolverConfig::default(), dns_opts);

In case of /dnsaddr the above transport should have the following flow:

  1. User dials to /dnsaddr/da-bridge-1.celestia-arabica-11.com/p2p/12D3KooWGqwzdEqM54Dce6LXzfFr97Bnhvm6rN7KM7MFwdomfm4S
  2. DNS transport resolves multiaddr to /dns/da-bridge-1.celestia-arabica-11.com/tcp/2122/tls/ws/p2p/12D3KooWGqwzdEqM54Dce6LXzfFr97Bnhvm6rN7KM7MFwdomfm4S
  3. DNS transport passes the /dns multiaddr to WebSocket transport.
  4. WebSocket transport initializes TLS based on /dns multiaddr and passes multiaddr to TCP transport.
  5. TCP transport resolves /dns multiaddr to /ip4 multiaddr and establishes the connection.

In case of /dns the above transport should have the following flow:

  1. User dials to /dns/da-bridge-1.celestia-arabica-11.com/tcp/2122/tls/ws/p2p/12D3KooWGqwzdEqM54Dce6LXzfFr97Bnhvm6rN7KM7MFwdomfm4S
  2. DNS transport passes the /dns multiaddr to WebSocket transport.
  3. WebSocket transport initializes TLS based on /dns multiaddr and passes multiaddr to TCP transport.
  4. TCP transport resolves /dns multiaddr to /ip4 multiaddr and establishes the connection.

Open questions

It would be hard to implement this kind of transport combination in SwarmBuilder.

Are you planning to do it yourself in a pull request ?

Maybe

@MarcoPolo
Copy link
Contributor

This sounds like a bug in the WebSocket transport that isn't properly using the SNI in the provided multiaddr.

mergify bot pushed a commit that referenced this issue Nov 5, 2024
## Description

Returns `Error::InvalidMultiaddr` when `parse_ws_dial_addr` is called
with `/dnsaddr`.

As per its specification, `/dnsaddr` domains are not meant to be
directly dialed, instead it should be appended with `_dnsaddr.` and used
for DNS lookups afterwards

Related: #5529
Fixes: #5601 

## Notes & open questions

* Is it okay to return an error, or should I perform a DNS lookup and
resolve that DNS afterwards if address has `/dnsaddr`?
* If so, how should I handle that case where DNS lookup returns multiple
multiaddrs?

## Change checklist

- [x] I have performed a self-review of my own code
- [ ] I have made corresponding changes to the documentation
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] A changelog entry has been made in the appropriate crates

---------

Co-authored-by: Darius Clark <[email protected]>
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

No branches or pull requests

2 participants