Pull request 1742: 5518-doh-panic

Updates #5425.
Updates #5518.

Squashed commit of the following:

commit 74873bd47bab569d0a45361f2d87c0e33e0ed3fb
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Feb 20 17:54:44 2023 +0300

    dnsforward: use Host when available

commit 4eee3d655ec7eb84e0327dcf30603578772609e1
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Mon Feb 20 17:24:24 2023 +0300

    dnsforward: fix panic on unencrypted doh
This commit is contained in:
Ainar Garipov 2023-02-20 18:01:00 +03:00
parent 6f6ced33c1
commit a556ce8fb8
3 changed files with 82 additions and 29 deletions

View File

@ -25,8 +25,10 @@ NOTE: Add new changes BELOW THIS COMMENT.
### Fixed ### Fixed
- Panic when using unencrypted DNS-over-HTTPS ([#5518]).
- Failing service installation via script on FreeBSD ([#5431]). - Failing service installation via script on FreeBSD ([#5431]).
[#5518]: https://github.com/AdguardTeam/AdGuardHome/issues/5518
[#5431]: https://github.com/AdguardTeam/AdGuardHome/issues/5431 [#5431]: https://github.com/AdguardTeam/AdGuardHome/issues/5431
<!-- <!--

View File

@ -147,11 +147,24 @@ func (s *Server) clientIDFromDNSContext(pctx *proxy.DNSContext) (clientID string
return clientID, nil return clientID, nil
} }
// clientServerName returns the TLS server name based on the protocol. // clientServerName returns the TLS server name based on the protocol. For
// DNS-over-HTTPS requests, it will return the hostname part of the Host header
// if there is one.
func clientServerName(pctx *proxy.DNSContext, proto proxy.Proto) (srvName string, err error) { func clientServerName(pctx *proxy.DNSContext, proto proxy.Proto) (srvName string, err error) {
switch proto { switch proto {
case proxy.ProtoHTTPS: case proxy.ProtoHTTPS:
srvName = pctx.HTTPRequest.TLS.ServerName r := pctx.HTTPRequest
if connState := r.TLS; connState != nil {
srvName = connState.ServerName
} else if r.Host != "" {
var host string
host, err = netutil.SplitHost(r.Host)
if err != nil {
return "", fmt.Errorf("parsing host: %w", err)
}
srvName = host
}
case proxy.ProtoQUIC: case proxy.ProtoQUIC:
qConn := pctx.QUICConnection qConn := pctx.QUICConnection
conn, ok := qConn.(quicConnection) conn, ok := qConn.(quicConnection)

View File

@ -50,136 +50,169 @@ func TestServer_clientIDFromDNSContext(t *testing.T) {
testCases := []struct { testCases := []struct {
name string name string
proto proxy.Proto proto proxy.Proto
hostSrvName string confSrvName string
cliSrvName string cliSrvName string
wantClientID string wantClientID string
wantErrMsg string wantErrMsg string
inclHTTPTLS bool
strictSNI bool strictSNI bool
}{{ }{{
name: "udp", name: "udp",
proto: proxy.ProtoUDP, proto: proxy.ProtoUDP,
hostSrvName: "", confSrvName: "",
cliSrvName: "", cliSrvName: "",
wantClientID: "", wantClientID: "",
wantErrMsg: "", wantErrMsg: "",
inclHTTPTLS: false,
strictSNI: false, strictSNI: false,
}, { }, {
name: "tls_no_clientid", name: "tls_no_clientid",
proto: proxy.ProtoTLS, proto: proxy.ProtoTLS,
hostSrvName: "example.com", confSrvName: "example.com",
cliSrvName: "example.com", cliSrvName: "example.com",
wantClientID: "", wantClientID: "",
wantErrMsg: "", wantErrMsg: "",
inclHTTPTLS: false,
strictSNI: true, strictSNI: true,
}, { }, {
name: "tls_no_client_server_name", name: "tls_no_client_server_name",
proto: proxy.ProtoTLS, proto: proxy.ProtoTLS,
hostSrvName: "example.com", confSrvName: "example.com",
cliSrvName: "", cliSrvName: "",
wantClientID: "", wantClientID: "",
wantErrMsg: `clientid check: client server name "" ` + wantErrMsg: `clientid check: client server name "" ` +
`doesn't match host server name "example.com"`, `doesn't match host server name "example.com"`,
inclHTTPTLS: false,
strictSNI: true, strictSNI: true,
}, { }, {
name: "tls_no_client_server_name_no_strict", name: "tls_no_client_server_name_no_strict",
proto: proxy.ProtoTLS, proto: proxy.ProtoTLS,
hostSrvName: "example.com", confSrvName: "example.com",
cliSrvName: "", cliSrvName: "",
wantClientID: "", wantClientID: "",
wantErrMsg: "", wantErrMsg: "",
inclHTTPTLS: false,
strictSNI: false, strictSNI: false,
}, { }, {
name: "tls_clientid", name: "tls_clientid",
proto: proxy.ProtoTLS, proto: proxy.ProtoTLS,
hostSrvName: "example.com", confSrvName: "example.com",
cliSrvName: "cli.example.com", cliSrvName: "cli.example.com",
wantClientID: "cli", wantClientID: "cli",
wantErrMsg: "", wantErrMsg: "",
inclHTTPTLS: false,
strictSNI: true, strictSNI: true,
}, { }, {
name: "tls_clientid_hostname_error", name: "tls_clientid_hostname_error",
proto: proxy.ProtoTLS, proto: proxy.ProtoTLS,
hostSrvName: "example.com", confSrvName: "example.com",
cliSrvName: "cli.example.net", cliSrvName: "cli.example.net",
wantClientID: "", wantClientID: "",
wantErrMsg: `clientid check: client server name "cli.example.net" ` + wantErrMsg: `clientid check: client server name "cli.example.net" ` +
`doesn't match host server name "example.com"`, `doesn't match host server name "example.com"`,
inclHTTPTLS: false,
strictSNI: true, strictSNI: true,
}, { }, {
name: "tls_invalid_clientid", name: "tls_invalid_clientid",
proto: proxy.ProtoTLS, proto: proxy.ProtoTLS,
hostSrvName: "example.com", confSrvName: "example.com",
cliSrvName: "!!!.example.com", cliSrvName: "!!!.example.com",
wantClientID: "", wantClientID: "",
wantErrMsg: `clientid check: invalid clientid "!!!": ` + wantErrMsg: `clientid check: invalid clientid "!!!": ` +
`bad domain name label rune '!'`, `bad domain name label rune '!'`,
inclHTTPTLS: false,
strictSNI: true, strictSNI: true,
}, { }, {
name: "tls_clientid_too_long", name: "tls_clientid_too_long",
proto: proxy.ProtoTLS, proto: proxy.ProtoTLS,
hostSrvName: "example.com", confSrvName: "example.com",
cliSrvName: `abcdefghijklmnopqrstuvwxyz0123456789abcdefghijklmno` + cliSrvName: `abcdefghijklmnopqrstuvwxyz0123456789abcdefghijklmno` +
`pqrstuvwxyz0123456789.example.com`, `pqrstuvwxyz0123456789.example.com`,
wantClientID: "", wantClientID: "",
wantErrMsg: `clientid check: invalid clientid "abcdefghijklmno` + wantErrMsg: `clientid check: invalid clientid "abcdefghijklmno` +
`pqrstuvwxyz0123456789abcdefghijklmnopqrstuvwxyz0123456789": ` + `pqrstuvwxyz0123456789abcdefghijklmnopqrstuvwxyz0123456789": ` +
`domain name label is too long: got 72, max 63`, `domain name label is too long: got 72, max 63`,
inclHTTPTLS: false,
strictSNI: true, strictSNI: true,
}, { }, {
name: "quic_clientid", name: "quic_clientid",
proto: proxy.ProtoQUIC, proto: proxy.ProtoQUIC,
hostSrvName: "example.com", confSrvName: "example.com",
cliSrvName: "cli.example.com", cliSrvName: "cli.example.com",
wantClientID: "cli", wantClientID: "cli",
wantErrMsg: "", wantErrMsg: "",
inclHTTPTLS: false,
strictSNI: true, strictSNI: true,
}, { }, {
name: "tls_clientid_issue3437", name: "tls_clientid_issue3437",
proto: proxy.ProtoTLS, proto: proxy.ProtoTLS,
hostSrvName: "example.com", confSrvName: "example.com",
cliSrvName: "cli.myexample.com", cliSrvName: "cli.myexample.com",
wantClientID: "", wantClientID: "",
wantErrMsg: `clientid check: client server name "cli.myexample.com" ` + wantErrMsg: `clientid check: client server name "cli.myexample.com" ` +
`doesn't match host server name "example.com"`, `doesn't match host server name "example.com"`,
inclHTTPTLS: false,
strictSNI: true, strictSNI: true,
}, { }, {
name: "tls_case", name: "tls_case",
proto: proxy.ProtoTLS, proto: proxy.ProtoTLS,
hostSrvName: "example.com", confSrvName: "example.com",
cliSrvName: "InSeNsItIvE.example.com", cliSrvName: "InSeNsItIvE.example.com",
wantClientID: "insensitive", wantClientID: "insensitive",
wantErrMsg: ``, wantErrMsg: ``,
inclHTTPTLS: false,
strictSNI: true, strictSNI: true,
}, { }, {
name: "quic_case", name: "quic_case",
proto: proxy.ProtoQUIC, proto: proxy.ProtoQUIC,
hostSrvName: "example.com", confSrvName: "example.com",
cliSrvName: "InSeNsItIvE.example.com", cliSrvName: "InSeNsItIvE.example.com",
wantClientID: "insensitive", wantClientID: "insensitive",
wantErrMsg: ``, wantErrMsg: ``,
inclHTTPTLS: false,
strictSNI: true, strictSNI: true,
}, { }, {
name: "https_no_clientid", name: "https_no_clientid",
proto: proxy.ProtoHTTPS, proto: proxy.ProtoHTTPS,
hostSrvName: "example.com", confSrvName: "example.com",
cliSrvName: "example.com", cliSrvName: "example.com",
wantClientID: "", wantClientID: "",
wantErrMsg: "", wantErrMsg: "",
inclHTTPTLS: true,
strictSNI: true, strictSNI: true,
}, { }, {
name: "https_clientid", name: "https_clientid",
proto: proxy.ProtoHTTPS, proto: proxy.ProtoHTTPS,
hostSrvName: "example.com", confSrvName: "example.com",
cliSrvName: "cli.example.com", cliSrvName: "cli.example.com",
wantClientID: "cli", wantClientID: "cli",
wantErrMsg: "", wantErrMsg: "",
inclHTTPTLS: true,
strictSNI: true,
}, {
name: "https_issue5518",
proto: proxy.ProtoHTTPS,
confSrvName: "example.com",
cliSrvName: "cli.example.com",
wantClientID: "cli",
wantErrMsg: "",
inclHTTPTLS: false,
strictSNI: true,
}, {
name: "https_no_host",
proto: proxy.ProtoHTTPS,
confSrvName: "example.com",
cliSrvName: "example.com",
wantClientID: "",
wantErrMsg: "",
inclHTTPTLS: false,
strictSNI: true, strictSNI: true,
}} }}
for _, tc := range testCases { for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) { t.Run(tc.name, func(t *testing.T) {
tlsConf := TLSConfig{ tlsConf := TLSConfig{
ServerName: tc.hostSrvName, ServerName: tc.confSrvName,
StrictSNICheck: tc.strictSNI, StrictSNICheck: tc.strictSNI,
} }
@ -195,7 +228,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) {
switch tc.proto { switch tc.proto {
case proxy.ProtoHTTPS: case proxy.ProtoHTTPS:
httpReq = newHTTPReq(tc.cliSrvName) httpReq = newHTTPReq(tc.cliSrvName, tc.inclHTTPTLS)
case proxy.ProtoQUIC: case proxy.ProtoQUIC:
qconn = testQUICConnection{ qconn = testQUICConnection{
serverName: tc.cliSrvName, serverName: tc.cliSrvName,
@ -222,20 +255,25 @@ func TestServer_clientIDFromDNSContext(t *testing.T) {
} }
// newHTTPReq is a helper to create HTTP requests for tests. // newHTTPReq is a helper to create HTTP requests for tests.
func newHTTPReq(cliSrvName string) (r *http.Request) { func newHTTPReq(cliSrvName string, inclTLS bool) (r *http.Request) {
u := &url.URL{ u := &url.URL{
Path: "/dns-query", Path: "/dns-query",
} }
return &http.Request{ r = &http.Request{
ProtoMajor: 1, ProtoMajor: 1,
ProtoMinor: 1, ProtoMinor: 1,
URL: u, URL: u,
Host: cliSrvName, Host: cliSrvName,
TLS: &tls.ConnectionState{
ServerName: cliSrvName,
},
} }
if inclTLS {
r.TLS = &tls.ConnectionState{
ServerName: cliSrvName,
}
}
return r
} }
func TestClientIDFromDNSContextHTTPS(t *testing.T) { func TestClientIDFromDNSContextHTTPS(t *testing.T) {