From 330ac303242970d79b557e611bec3227b0af5a7b Mon Sep 17 00:00:00 2001
From: Ainar Garipov <a.garipov@adguard.com>
Date: Wed, 5 Oct 2022 18:11:09 +0300
Subject: [PATCH] Pull request: 3418-clientid-doh

Closes #3418.

Squashed commit of the following:

commit 8a1180f8ef03d30ea3ae6a3e3121ddcac513f45b
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Oct 5 17:26:22 2022 +0300

    all: imp docs, tests

commit 9629c69b39540db119044f2f79c1c4ed39de911f
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Wed Oct 5 15:34:33 2022 +0300

    dnsforward: accept clientids from doh client srvname
---
 CHANGELOG.md                         | 10 ++++
 internal/dnsforward/clientid.go      | 69 +++++++++++++++++-----------
 internal/dnsforward/clientid_test.go | 69 ++++++++++++++++++++++++----
 3 files changed, 114 insertions(+), 34 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index be4d47b0..551a3db1 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -15,6 +15,16 @@ and this project adheres to
 ## [v0.108.0] - TBA (APPROX.)
 -->
 
+## Added
+
+- The ability to put [ClientIDs][clientid] into DNS-over-HTTPS hostnames as
+  opposed to URL paths ([#3418]).  Note that AdGuard Home checks the server name
+  only if the URL does not contain a ClientID.
+
+[#3418]: https://github.com/AdguardTeam/AdGuardHome/issues/3418
+
+[clientid]: https://github.com/AdguardTeam/AdGuardHome/wiki/Clients#clientid
+
 
 
 <!--
diff --git a/internal/dnsforward/clientid.go b/internal/dnsforward/clientid.go
index 16bac881..bef83182 100644
--- a/internal/dnsforward/clientid.go
+++ b/internal/dnsforward/clientid.go
@@ -123,7 +123,14 @@ type quicConnection interface {
 func (s *Server) clientIDFromDNSContext(pctx *proxy.DNSContext) (clientID string, err error) {
 	proto := pctx.Proto
 	if proto == proxy.ProtoHTTPS {
-		return clientIDFromDNSContextHTTPS(pctx)
+		clientID, err = clientIDFromDNSContextHTTPS(pctx)
+		if err != nil {
+			return "", fmt.Errorf("checking url: %w", err)
+		} else if clientID != "" {
+			return clientID, nil
+		}
+
+		// Go on and check the domain name as well.
 	} else if proto != proxy.ProtoTLS && proto != proxy.ProtoQUIC {
 		return "", nil
 	}
@@ -133,31 +140,9 @@ func (s *Server) clientIDFromDNSContext(pctx *proxy.DNSContext) (clientID string
 		return "", nil
 	}
 
-	cliSrvName := ""
-	switch proto {
-	case proxy.ProtoTLS:
-		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,
-			)
-		}
-
-		cliSrvName = tc.ConnectionState().ServerName
-	case proxy.ProtoQUIC:
-		conn, ok := pctx.QUICConnection.(quicConnection)
-		if !ok {
-			return "", fmt.Errorf(
-				"proxy ctx quic conn of proto %s is %T, want quic.Connection",
-				proto,
-				pctx.QUICConnection,
-			)
-		}
-
-		cliSrvName = conn.ConnectionState().TLS.ServerName
+	cliSrvName, err := clientServerName(pctx, proto)
+	if err != nil {
+		return "", err
 	}
 
 	clientID, err = clientIDFromClientServerName(
@@ -171,3 +156,35 @@ func (s *Server) clientIDFromDNSContext(pctx *proxy.DNSContext) (clientID string
 
 	return clientID, nil
 }
+
+// clientServerName returns the TLS server name based on the protocol.
+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
+		}
+	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,
+			)
+		}
+
+		srvName = conn.ConnectionState().TLS.ServerName
+	case proxy.ProtoTLS:
+		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)
+		}
+
+		srvName = tc.ConnectionState().ServerName
+	}
+
+	return srvName, nil
+}
diff --git a/internal/dnsforward/clientid_test.go b/internal/dnsforward/clientid_test.go
index 31c55fcd..b29f7ef0 100644
--- a/internal/dnsforward/clientid_test.go
+++ b/internal/dnsforward/clientid_test.go
@@ -160,6 +160,22 @@ func TestServer_clientIDFromDNSContext(t *testing.T) {
 		wantClientID: "insensitive",
 		wantErrMsg:   ``,
 		strictSNI:    true,
+	}, {
+		name:         "https_no_clientid",
+		proto:        proxy.ProtoHTTPS,
+		hostSrvName:  "example.com",
+		cliSrvName:   "example.com",
+		wantClientID: "",
+		wantErrMsg:   "",
+		strictSNI:    true,
+	}, {
+		name:         "https_clientid",
+		proto:        proxy.ProtoHTTPS,
+		hostSrvName:  "example.com",
+		cliSrvName:   "cli.example.com",
+		wantClientID: "cli",
+		wantErrMsg:   "",
+		strictSNI:    true,
 	}}
 
 	for _, tc := range testCases {
@@ -173,16 +189,32 @@ func TestServer_clientIDFromDNSContext(t *testing.T) {
 				conf: ServerConfig{TLSConfig: tlsConf},
 			}
 
-			var conn net.Conn
-			if tc.proto == proxy.ProtoTLS {
-				conn = testTLSConn{
+			var (
+				conn    net.Conn
+				qconn   quic.Connection
+				httpReq *http.Request
+			)
+
+			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,
+				}
+			case proxy.ProtoQUIC:
+				qconn = testQUICConnection{
 					serverName: tc.cliSrvName,
 				}
-			}
-
-			var qconn quic.Connection
-			if tc.proto == proxy.ProtoQUIC {
-				qconn = testQUICConnection{
+			case proxy.ProtoTLS:
+				conn = testTLSConn{
 					serverName: tc.cliSrvName,
 				}
 			}
@@ -190,6 +222,7 @@ func TestServer_clientIDFromDNSContext(t *testing.T) {
 			pctx := &proxy.DNSContext{
 				Proto:          tc.proto,
 				Conn:           conn,
+				HTTPRequest:    httpReq,
 				QUICConnection: qconn,
 			}
 
@@ -205,56 +238,76 @@ func TestClientIDFromDNSContextHTTPS(t *testing.T) {
 	testCases := []struct {
 		name         string
 		path         string
+		cliSrvName   string
 		wantClientID string
 		wantErrMsg   string
 	}{{
 		name:         "no_clientid",
 		path:         "/dns-query",
+		cliSrvName:   "example.com",
 		wantClientID: "",
 		wantErrMsg:   "",
 	}, {
 		name:         "no_clientid_slash",
 		path:         "/dns-query/",
+		cliSrvName:   "example.com",
 		wantClientID: "",
 		wantErrMsg:   "",
 	}, {
 		name:         "clientid",
 		path:         "/dns-query/cli",
+		cliSrvName:   "example.com",
 		wantClientID: "cli",
 		wantErrMsg:   "",
 	}, {
 		name:         "clientid_slash",
 		path:         "/dns-query/cli/",
+		cliSrvName:   "example.com",
 		wantClientID: "cli",
 		wantErrMsg:   "",
 	}, {
 		name:         "clientid_case",
 		path:         "/dns-query/InSeNsItIvE",
+		cliSrvName:   "example.com",
 		wantClientID: "insensitive",
 		wantErrMsg:   ``,
 	}, {
 		name:         "bad_url",
 		path:         "/foo",
+		cliSrvName:   "example.com",
 		wantClientID: "",
 		wantErrMsg:   `clientid check: invalid path "/foo"`,
 	}, {
 		name:         "extra",
 		path:         "/dns-query/cli/foo",
+		cliSrvName:   "example.com",
 		wantClientID: "",
 		wantErrMsg:   `clientid check: invalid path "/dns-query/cli/foo": extra parts`,
 	}, {
 		name:         "invalid_clientid",
 		path:         "/dns-query/!!!",
+		cliSrvName:   "example.com",
 		wantClientID: "",
 		wantErrMsg:   `clientid check: invalid clientid "!!!": bad domain name label rune '!'`,
+	}, {
+		name:         "both_ids",
+		path:         "/dns-query/right",
+		cliSrvName:   "wrong.example.com",
+		wantClientID: "right",
+		wantErrMsg:   "",
 	}}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
+			connState := &tls.ConnectionState{
+				ServerName: tc.cliSrvName,
+			}
+
 			r := &http.Request{
 				URL: &url.URL{
 					Path: tc.path,
 				},
+				TLS: connState,
 			}
 
 			pctx := &proxy.DNSContext{