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

x/net/quic: Endpoint.Dial prefers IPv4 to IPv6 #70223

Open
jfgiorgi opened this issue Nov 6, 2024 · 5 comments
Open

x/net/quic: Endpoint.Dial prefers IPv4 to IPv6 #70223

jfgiorgi opened this issue Nov 6, 2024 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@jfgiorgi
Copy link

jfgiorgi commented Nov 6, 2024

Go version

go version go1.23.2 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/user/.cache/go-build'
GOENV='/home/user/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/user/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/user/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go1.23.2.linux-amd64'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go1.23.2.linux-amd64/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.2'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/user/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/user/dev/nspeed-client/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1098587137=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Create a quic.Endpoint then call Dial method with a name (not a literal ip) in the address parameter.

here is a small POC :

package main

import (
	"context"
	"crypto/tls"
	"fmt"
	"net"

	"golang.org/x/net/quic"
)

func main() {
	// a http/3 enabled url like google.com or cloudflare.com
	address := "nspeed.app"

	quicConf := &quic.Config{
		TLSConfig: &tls.Config{
			MinVersion: tls.VersionTLS13,
			NextProtos: []string{"h3"},
			ServerName: address,
		},
	}

	// local endpoint
	endp, err := quic.Listen("udp", ":0", nil)

	if err != nil {
		panic(err)
	}

	// connect to address on port 443
	qconn, err := endp.Dial(context.Background(), "", address+":443", quicConf)
	if err != nil {
		panic(err)
	}

	fmt.Println("connected") // currently there is no way to display the remote ip , use strace or a network capture

	// unless you have https://github.com/golang/net/pull/225
	// fmt.Println("connected to", qconn.RemoteAddr())

	qconn.Close()
}

What did you see happen?

The connection happens with IPv4

What did you expect to see?

The connection should use IPv6 is it's available at the OS level.

This is a direct consequence of net.ResolveUDPAddr not preferring IPv6 (net.ResolveTCPAddr and net.ResolveIPAddr also have this issue)

The culprit is in src/net/ipsock.go#L82 (forResolve function)

see also #28666

A workaround is to resolve the address parameter before calling Endpoint.Dial
net.Dial (tcp or udp) doesn't have this issue.

@gopherbot gopherbot added this to the Unreleased milestone Nov 6, 2024
@dr2chase
Copy link
Contributor

dr2chase commented Nov 6, 2024

@neild @ianlancetaylor

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 6, 2024
@neild
Copy link
Contributor

neild commented Nov 6, 2024

Is this an x/net/quic issue, or #28666 happening to apply to x/net/quic as well as anything else that uses net.Resolve*Addr?

I think net.Dial avoids the problem since it attempts to connect to all the resolved addresses for a host, using a Happy Eyeballs style racing IPv4/IPv6 connect.

Possibly x/net/quic should do the same. Or possibly that's something that should be handled at a higher level.

If we wanted to be very fancy in x/net/quic, then the cost of a connection attempt doesn't need to be more than a single datagram sent--we could just send a datagram to every possible address and pick the first one to respond. But that'd be a bit tricky to integrate with the retry logic; if nothing comes back, do we resend to every address on PTO? Have a separate PTO timer per address? Simpler, but more expensive, to treat each potential remote address as a separate connection.

@jfgiorgi
Copy link
Author

jfgiorgi commented Nov 7, 2024

imho Happy Eyeballs should be higher level. HTTP/3 level for instance, along side the new HTTPS/SVCB DNS records (RFC 9460) .

I'm not a fan of Happy Eyeballs, it's only a transition mechanism , it's not the future and we ran into numerous issues with it, notably when people switch to IPv6 and forget to update or remove the IPv4 DNS record (which still point, for instance, to the older version of a service).

In 2024 "assuming that IPv6 is misconfigured" should not be handled by std lib code but should lead to have network/system people fix their stuff. Much like no one assumes IPv4 is misconfigured and try with IPv6 as a fallback.

net.Resolve*Addr should be deprecated (outside internal usage to keep compatibility). Or at minimum update their documentation to warn users about IPv6.

x/net/quic should be basic and respect IPv6 priority defined by the OS (getaddrinfo & gai.conf on Linux, prefixpolicies on Windows, etc). This shouldn't be in x/net/quic but in the resolver used by x/net/quic which should be may be an optional 'resolver' parameter of Endpoint.Dial ?

@mateusz834
Copy link
Member

net.Resolve*Addr should be deprecated (outside internal usage to keep compatibility). Or at minimum update their documentation to warn users about IPv6.

That is actually a good call. We probably should deprecate even more IP resolution functions, we have like 6-10 of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants