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 does not respect transport-specific DNS resolution #2562

Closed
aschmahmann opened this issue Sep 8, 2023 · 2 comments · Fixed by #2564
Closed

DNSAddr does not respect transport-specific DNS resolution #2562

aschmahmann opened this issue Sep 8, 2023 · 2 comments · Fixed by #2564

Comments

@aschmahmann
Copy link
Collaborator

Problem

Dialing a DNSAddr multiaddr that somewhere down the recursion chain forwards to an address type that handles DNS specially (e.g. WSS) does not work because the host over-resolves the address.

Example:

  • Dial /dnsaddr/bootstrap.libp2p.io/p2p/QmbLHAnMoJPWSCR5Zhtx6BHJX9KiKNN6tpvbUcqanj75Nb (but only have WSS as a supported transport)
  • This will resolve to /dns4/am6.bootstrap.libp2p.io/tcp/443/wss/p2p/QmbLHAnMoJPWSCR5Zhtx6BHJX9KiKNN6tpvbUcqanj75Nb
  • Which will resolve to /ip4/147.75.87.27/tcp/443/wss/p2p/QmbLHAnMoJPWSCR5Zhtx6BHJX9KiKNN6tpvbUcqanj75Nb
  • Which will fail due to an invalid TLS certificate

However: dialing /dns4/am6.bootstrap.libp2p.io/tcp/443/wss/p2p/QmbLHAnMoJPWSCR5Zhtx6BHJX9KiKNN6tpvbUcqanj75Nb directly works fine.

Potential Fix

This is effectively a continuation of #1597, but accounting for DNSAddr recursion. I suspect the fix that will help here is to keep doing the type of check we do in

for _, a := range peerAddrs {
tpt := s.TransportForDialing(a)
resolver, ok := tpt.(transport.Resolver)
if ok {
resolvedAddrs, err := resolver.Resolve(ctx, a)
if err != nil {
log.Warnf("Failed to resolve multiaddr %s by transport %v: %v", a, tpt, err)
continue
}
peerAddrsAfterTransportResolved = append(peerAddrsAfterTransportResolved, resolvedAddrs...)
} else {
peerAddrsAfterTransportResolved = append(peerAddrsAfterTransportResolved, a)
}
}
not just at the top layer of address resolution, but also in the recursive DNSAddr resolution function
func (s *Swarm) resolveAddrs(ctx context.Context, pi peer.AddrInfo) ([]ma.Multiaddr, error) {

cc @dennis-tra (thanks for finding out something was going wrong here 😄)

@marten-seemann
Copy link
Contributor

Very interesting. Looks like the transport-specific name resolution is bypassed altogehter, since looking at /dnsaddr/..., we don't know it will resolve to a wss address. We'll need to do that check recursively.

Fix incoming.

@marten-seemann
Copy link
Contributor

Getting the fix merged took us a while, but it's merged now, and will be included in the v0.32 release later this week.

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