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

fix(websocket): don't dial /dnsaddr addresses #5613

Merged
merged 9 commits into from
Nov 5, 2024

Conversation

zetsuboii
Copy link
Contributor

@zetsuboii zetsuboii commented Sep 27, 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

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

## 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

## Notes & open questions

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

## 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
- [ ] A changelog entry has been made in the appropriate crates
@zetsuboii zetsuboii marked this pull request as ready for review September 27, 2024 08:45
Copy link
Member

@dariusc93 dariusc93 left a comment

Choose a reason for hiding this comment

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

LGTM.

Cargo.toml Outdated Show resolved Hide resolved
Co-authored-by: Darius Clark <dariusc93@users.noreply.github.com>
@oblique
Copy link
Contributor

oblique commented Oct 2, 2024

websocket-websys needs this change too.

@zetsuboii
Copy link
Contributor Author

bsys needs this change too.

I see, will update there as well

@DougAnderson444
Copy link
Contributor

Hello

I just opened #5619 because I want to dial /dnsaddr from webrtc-websys. Then I noticed this issue.

Would it make sense to simply support DNS multiaddr resolution in the browser?

@DougAnderson444
Copy link
Contributor

Also #5531 could enable this?

@oblique
Copy link
Contributor

oblique commented Oct 4, 2024

@DougAnderson444 As described in #5601, this PR removes an incorrect handling of /dnsaddr. #5531 is the only way forward which needs to implement resolution of only /dnsaddr multiaddresses through a new Transport that targets only browsers.

@zetsuboii
Copy link
Contributor Author

Updated websocket-websys (bf23e92) so that it returns None when a /dnsaddr is extracted.
This should be enough to fix incorrect handling of /dnsaddr and to resolve them correctly we can continue from #5531

cc: @dariusc93

@zetsuboii
Copy link
Contributor Author

Previous CI runs failed because I didn't update top-level versions, my bad. Can you restart the workflow? @oblique

@oblique
Copy link
Contributor

oblique commented Nov 1, 2024

cc @jxs

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

LGTM Thanks Yianis!

@jxs jxs added the send-it label Nov 5, 2024
@mergify mergify bot merged commit d021ce2 into libp2p:master Nov 5, 2024
69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/dnsaddr is handled as /dns4 in libp2p-websocket and libp2p-websocket-websys
5 participants