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

Bug: parse the wrong ServerName from addr #4045

Closed
chaosmatrix opened this issue Aug 23, 2023 · 3 comments · Fixed by #4046
Closed

Bug: parse the wrong ServerName from addr #4045

chaosmatrix opened this issue Aug 23, 2023 · 3 comments · Fixed by #4046

Comments

@chaosmatrix
Copy link

quic-go/transport.go

Lines 176 to 184 in f633dca

// If no ServerName is set, infer the ServerName from the hostname we're connecting to.
if tlsConf.ServerName == "" {
if hostname == "" {
if udpAddr, ok := addr.(*net.UDPAddr); ok {
hostname = udpAddr.IP.String()
}
}
tlsConf.ServerName = hostname
}

Test case:

// env:
//  go1.21.0
// github.com/quic-go/quic-go v0.38.0
func TestQUICTLS(t *testing.T) {
	quicConf := &quic.Config{
		HandshakeIdleTimeout:    5 * time.Second,
		DisablePathMTUDiscovery: true,
	}

	tlsConf := &tls.Config{
		NextProtos: []string{"h3"},
	}

	//tlsConf.ServerName = "quic.nginx.org."

	addr := "quic.nginx.org:443"

	_, err := quic.DialAddr(context.TODO(), addr, tlsConf, quicConf)
	if err != nil {
		t.Error(err)
	}

}

The result is:

--- FAIL: TestQUICTLS (0.64s)
    example_test.go:30: CRYPTO_ERROR 0x12a (local): tls: failed to verify certificate: x509: certificate is valid for quic.nginx.org, not quic.nginx.org:443
FAIL
exit status 1

one way to fix it:

if tlsConf.ServerName == "" {
	if i := strings.LastIndex(hostname, ":"); i > 0 {
		tlsConf.ServerName = hostname[:i]
	} else {
		tlsConf.ServerName = hostname
	}
}
@chaosmatrix chaosmatrix changed the title Bug: set the wrong ServerName from addr Bug: parse the wrong ServerName from addr Aug 23, 2023
@marten-seemann
Copy link
Member

I see the bug. However, the fix is not correct, it would try to break split IPv6 addresses.

@marten-seemann
Copy link
Member

@chaosmatrix Could you try out #4046 please? A review would also be appreciated :)

If this fixes your problem, we could cut a patch release today or tomorrow.

@chaosmatrix
Copy link
Author

It fixes my problem, thanks.

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