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 is handled as /dns4 in libp2p-websocket and libp2p-websocket-websys #5601

Closed
oblique opened this issue Sep 18, 2024 · 1 comment · Fixed by #5613
Closed

/dnsaddr is handled as /dns4 in libp2p-websocket and libp2p-websocket-websys #5601

oblique opened this issue Sep 18, 2024 · 1 comment · Fixed by #5613

Comments

@oblique
Copy link
Contributor

oblique commented Sep 18, 2024

Summary

/dnsaddr is handled as /dns4 in libp2p-websocket and libp2p-websocket-websys. This is incorrect because /dnsaddr is only for quering _dnsaddr.domain from DNS.

/dnsaddr support in WebSocket is tracked in #5529.

libp2p-websocket:

(Some(Protocol::Dns(h)), Some(Protocol::Tcp(port)))
| (Some(Protocol::Dns4(h)), Some(Protocol::Tcp(port)))
| (Some(Protocol::Dns6(h)), Some(Protocol::Tcp(port)))
| (Some(Protocol::Dnsaddr(h)), Some(Protocol::Tcp(port))) => {
break (format!("{h}:{port}"), tls::dns_name_ref(&h)?)
}

libp2p-websocket-websys:

(Some(Protocol::Dns(h)), Some(Protocol::Tcp(port)))
| (Some(Protocol::Dns4(h)), Some(Protocol::Tcp(port)))
| (Some(Protocol::Dns6(h)), Some(Protocol::Tcp(port)))
| (Some(Protocol::Dnsaddr(h)), Some(Protocol::Tcp(port))) => {
format!("{}:{}", &h, port)
}

Expected behavior

TransportError::MultiaddrNotSupported should be returned from dial.

Actual behavior

/dnsaddr multiaddress is dialed as if it was /dns4.

Relevant log output

No response

Possible Solution

No response

Version

0.54.1

Would you like to work on fixing this bug ?

Maybe

@dariusc93
Copy link
Member

Hey! Interesting find. I would have always assumed that using dnsaddr in this manner for websocket is fine but it does make sense that it would be meant to query the dns record itself.

@mergify mergify bot closed this as completed in #5613 Nov 5, 2024
@mergify mergify bot closed this as completed in d021ce2 Nov 5, 2024
jxs pushed a commit to jxs/rust-libp2p that referenced this issue Jan 6, 2025
## 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: libp2p#5529
Fixes: libp2p#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

Successfully merging a pull request may close this issue.

2 participants