From bf10f157ab69104819a7a3257f826b1e4426f222 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Wed, 26 Oct 2022 15:54:04 +0300 Subject: [PATCH] Pull request: http3-tls-fix Merge in DNS/adguard-home from http3-tls-fix to master Squashed commit of the following: commit 4b4ac9f91d6a36654674c5e7037d2bf35a6b211a Author: Ainar Garipov Date: Wed Oct 26 15:35:07 2022 +0300 dnsforward: add crutch for quic-go bug --- CHANGELOG.md | 1 + internal/dnsforward/clientid.go | 28 ++++++++---- internal/dnsforward/clientid_test.go | 66 ++++++++++++++++++++++------ 3 files changed, 73 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54b88a13..10be9b1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ and this project adheres to ### Fixed +- ClientIDs not working when using DNS-over-HTTPS with HTTP/3. - Editing an enabled rule list's URL now also includes validation of the filter contents preventing from saving a bad one ([#4916]). - The default value of `dns.cache_size` accidentally set to 0 has now been diff --git a/internal/dnsforward/clientid.go b/internal/dnsforward/clientid.go index bef83182..6a111c47 100644 --- a/internal/dnsforward/clientid.go +++ b/internal/dnsforward/clientid.go @@ -161,18 +161,30 @@ func (s *Server) clientIDFromDNSContext(pctx *proxy.DNSContext) (clientID string func clientServerName(pctx *proxy.DNSContext, proto proxy.Proto) (srvName string, err error) { switch proto { case proxy.ProtoHTTPS: - if connState := pctx.HTTPRequest.TLS; connState != nil { - srvName = pctx.HTTPRequest.TLS.ServerName + // github.com/lucas-clemente/quic-go seems to not populate the TLS + // field. So, if the request comes over HTTP/3, use the Host header + // value as the server name. + // + // See https://github.com/lucas-clemente/quic-go/issues/2879. + // + // TODO(a.garipov): Remove this crutch once they fix it. + r := pctx.HTTPRequest + if r.ProtoAtLeast(3, 0) { + var host string + host, err = netutil.SplitHost(r.Host) + if err != nil { + return "", fmt.Errorf("parsing host: %w", err) + } + + srvName = host + } else if connState := r.TLS; connState != nil { + srvName = r.TLS.ServerName } case proxy.ProtoQUIC: qConn := pctx.QUICConnection conn, ok := qConn.(quicConnection) if !ok { - return "", fmt.Errorf( - "proxy ctx quic conn of proto %s is %T, want quic.Connection", - proto, - qConn, - ) + return "", fmt.Errorf("pctx conn of proto %s is %T, want quic.Connection", proto, qConn) } srvName = conn.ConnectionState().TLS.ServerName @@ -180,7 +192,7 @@ func clientServerName(pctx *proxy.DNSContext, proto proxy.Proto) (srvName string conn := pctx.Conn tc, ok := conn.(tlsConn) if !ok { - return "", fmt.Errorf("proxy ctx conn of proto %s is %T, want *tls.Conn", proto, conn) + return "", fmt.Errorf("pctx conn of proto %s is %T, want *tls.Conn", proto, conn) } srvName = tc.ConnectionState().ServerName diff --git a/internal/dnsforward/clientid_test.go b/internal/dnsforward/clientid_test.go index b29f7ef0..b52f2ad0 100644 --- a/internal/dnsforward/clientid_test.go +++ b/internal/dnsforward/clientid_test.go @@ -47,8 +47,6 @@ func (c testQUICConnection) ConnectionState() (cs quic.ConnectionState) { } func TestServer_clientIDFromDNSContext(t *testing.T) { - // TODO(a.garipov): Consider moving away from the text-based error - // checks and onto a more structured approach. testCases := []struct { name string proto proxy.Proto @@ -57,6 +55,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { wantClientID string wantErrMsg string strictSNI bool + useHTTP3 bool }{{ name: "udp", proto: proxy.ProtoUDP, @@ -65,6 +64,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { wantClientID: "", wantErrMsg: "", strictSNI: false, + useHTTP3: false, }, { name: "tls_no_clientid", proto: proxy.ProtoTLS, @@ -73,6 +73,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { wantClientID: "", wantErrMsg: "", strictSNI: true, + useHTTP3: false, }, { name: "tls_no_client_server_name", proto: proxy.ProtoTLS, @@ -82,6 +83,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { wantErrMsg: `clientid check: client server name "" ` + `doesn't match host server name "example.com"`, strictSNI: true, + useHTTP3: false, }, { name: "tls_no_client_server_name_no_strict", proto: proxy.ProtoTLS, @@ -90,6 +92,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { wantClientID: "", wantErrMsg: "", strictSNI: false, + useHTTP3: false, }, { name: "tls_clientid", proto: proxy.ProtoTLS, @@ -98,6 +101,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { wantClientID: "cli", wantErrMsg: "", strictSNI: true, + useHTTP3: false, }, { name: "tls_clientid_hostname_error", proto: proxy.ProtoTLS, @@ -107,6 +111,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { wantErrMsg: `clientid check: client server name "cli.example.net" ` + `doesn't match host server name "example.com"`, strictSNI: true, + useHTTP3: false, }, { name: "tls_invalid_clientid", proto: proxy.ProtoTLS, @@ -116,6 +121,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { wantErrMsg: `clientid check: invalid clientid "!!!": ` + `bad domain name label rune '!'`, strictSNI: true, + useHTTP3: false, }, { name: "tls_clientid_too_long", proto: proxy.ProtoTLS, @@ -127,6 +133,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { `pqrstuvwxyz0123456789abcdefghijklmnopqrstuvwxyz0123456789": ` + `domain name label is too long: got 72, max 63`, strictSNI: true, + useHTTP3: false, }, { name: "quic_clientid", proto: proxy.ProtoQUIC, @@ -135,6 +142,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { wantClientID: "cli", wantErrMsg: "", strictSNI: true, + useHTTP3: false, }, { name: "tls_clientid_issue3437", proto: proxy.ProtoTLS, @@ -144,6 +152,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { wantErrMsg: `clientid check: client server name "cli.myexample.com" ` + `doesn't match host server name "example.com"`, strictSNI: true, + useHTTP3: false, }, { name: "tls_case", proto: proxy.ProtoTLS, @@ -152,6 +161,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { wantClientID: "insensitive", wantErrMsg: ``, strictSNI: true, + useHTTP3: false, }, { name: "quic_case", proto: proxy.ProtoQUIC, @@ -160,6 +170,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { wantClientID: "insensitive", wantErrMsg: ``, strictSNI: true, + useHTTP3: false, }, { name: "https_no_clientid", proto: proxy.ProtoHTTPS, @@ -168,6 +179,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { wantClientID: "", wantErrMsg: "", strictSNI: true, + useHTTP3: false, }, { name: "https_clientid", proto: proxy.ProtoHTTPS, @@ -176,6 +188,16 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { wantClientID: "cli", wantErrMsg: "", strictSNI: true, + useHTTP3: false, + }, { + name: "https_clientid_quic", + proto: proxy.ProtoHTTPS, + hostSrvName: "example.com", + cliSrvName: "cli.example.com", + wantClientID: "cli", + wantErrMsg: "", + strictSNI: true, + useHTTP3: true, }} for _, tc := range testCases { @@ -197,18 +219,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { switch tc.proto { case proxy.ProtoHTTPS: - u := &url.URL{ - Path: "/dns-query", - } - - connState := &tls.ConnectionState{ - ServerName: tc.cliSrvName, - } - - httpReq = &http.Request{ - URL: u, - TLS: connState, - } + httpReq = newHTTPReq(tc.cliSrvName, tc.useHTTP3) case proxy.ProtoQUIC: qconn = testQUICConnection{ serverName: tc.cliSrvName, @@ -234,6 +245,33 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { } } +// newHTTPReq is a helper to create HTTP requests for tests. +func newHTTPReq(cliSrvName string, useHTTP3 bool) (r *http.Request) { + u := &url.URL{ + Path: "/dns-query", + } + + if useHTTP3 { + return &http.Request{ + ProtoMajor: 3, + ProtoMinor: 0, + URL: u, + Host: cliSrvName, + TLS: &tls.ConnectionState{}, + } + } + + return &http.Request{ + ProtoMajor: 1, + ProtoMinor: 1, + URL: u, + Host: cliSrvName, + TLS: &tls.ConnectionState{ + ServerName: cliSrvName, + }, + } +} + func TestClientIDFromDNSContextHTTPS(t *testing.T) { testCases := []struct { name string