Pull request: 4927-ddr-ip-san

Merge in DNS/adguard-home from 4927-ddr-ip-san to master

Updates #4927.

Squashed commit of the following:

commit 92e7498a7a9101648c4cfdf719adf4eb135fc903
Merge: f4770abf fa0fd90d
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Nov 2 14:29:08 2022 +0300

    Merge branch 'master' into 4927-ddr-ip-san

commit f4770abf98ea2c0db2f0c2ddb9509a29a06c9509
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Wed Nov 2 13:50:40 2022 +0300

    dnsforward: imp logs

commit 8d71371365070e221e104ae20acc8312e840eff9
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Nov 1 20:57:43 2022 +0300

    all: imp code, docs

commit 9793820f2c581e0ffcb28a59677be5c8df0c43f3
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Nov 1 19:37:39 2022 +0300

    all: remember the cert props

CHANGELOG.md

@@ -39,6 +39,8 @@ and this project adheres to
 
 ### Changed
 
+- DNS-over-TLS resolvers aren't returned anymore when the configured TLS
+  certificate contains no IP addresses ([#4927]).
 - Responses with `SERVFAIL` code are now cached for at least 30 seconds.
 
 ### Deprecated
@@ -63,6 +65,7 @@ and this project adheres to
 [#4898]: https://github.com/AdguardTeam/AdGuardHome/issues/4898
 [#4916]: https://github.com/AdguardTeam/AdGuardHome/issues/4916
 [#4925]: https://github.com/AdguardTeam/AdGuardHome/issues/4925
+[#4927]: https://github.com/AdguardTeam/AdGuardHome/issues/4927
 [#4942]: https://github.com/AdguardTeam/AdGuardHome/issues/4942
 [#4986]: https://github.com/AdguardTeam/AdGuardHome/issues/4986
 [#4990]: https://github.com/AdguardTeam/AdGuardHome/issues/4990

internal/aghtls/aghtls.go

@@ -3,7 +3,9 @@ package aghtls
 
 import (
 	"crypto/tls"
+	"crypto/x509"
 	"fmt"
+	"net/netip"
 
 	"github.com/AdguardTeam/golibs/log"
 )
@@ -69,3 +71,19 @@ func SaferCipherSuites() (safe []uint16) {
 
 	return safe
 }
+
+// CertificateHasIP returns true if cert has at least a single IP address among
+// its subjectAltNames.
+func CertificateHasIP(cert *x509.Certificate) (ok bool) {
+	if len(cert.IPAddresses) > 0 {
+		return true
+	}
+
+	for _, name := range cert.DNSNames {
+		if _, err := netip.ParseAddr(name); err == nil {
+			return true
+		}
+	}
+
+	return false
+}

internal/dnsforward/config.go

@@ -12,6 +12,7 @@ import (
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghalg"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
+	"github.com/AdguardTeam/AdGuardHome/internal/aghtls"
 	"github.com/AdguardTeam/AdGuardHome/internal/filtering"
 	"github.com/AdguardTeam/dnsproxy/proxy"
 	"github.com/AdguardTeam/dnsproxy/upstream"
@@ -146,13 +147,12 @@ type FilteringConfig struct {
 
 // TLSConfig is the TLS configuration for HTTPS, DNS-over-HTTPS, and DNS-over-TLS
 type TLSConfig struct {
+	cert tls.Certificate
+
 	TLSListenAddrs   []*net.TCPAddr `yaml:"-" json:"-"`
 	QUICListenAddrs  []*net.UDPAddr `yaml:"-" json:"-"`
 	HTTPSListenAddrs []*net.TCPAddr `yaml:"-" json:"-"`
 
-	// Reject connection if the client uses server name (in SNI) that doesn't match the certificate
-	StrictSNICheck bool `yaml:"strict_sni_check" json:"-"`
-
 	// PEM-encoded certificates chain
 	CertificateChain string `yaml:"certificate_chain" json:"certificate_chain"`
 	// PEM-encoded private key
@@ -168,13 +168,20 @@ type TLSConfig struct {
 	// used for ClientID checking and Discovery of Designated Resolvers (DDR).
 	ServerName string `yaml:"-" json:"-"`
 
-	cert tls.Certificate
 	// DNS names from certificate (SAN) or CN value from Subject
 	dnsNames []string
 
 	// OverrideTLSCiphers, when set, contains the names of the cipher suites to
 	// use.  If the slice is empty, the default safe suites are used.
 	OverrideTLSCiphers []string `yaml:"override_tls_ciphers,omitempty" json:"-"`
+
+	// StrictSNICheck controls if the connections with SNI mismatching the
+	// certificate's ones should be rejected.
+	StrictSNICheck bool `yaml:"strict_sni_check" json:"-"`
+
+	// hasIPAddrs is set during the certificate parsing and is true if the
+	// configured certificate contains at least a single IP address.
+	hasIPAddrs bool
 }
 
 // DNSCryptConfig is the DNSCrypt server configuration struct.
@@ -459,7 +466,7 @@ func (s *Server) prepareIpsetListSettings() (err error) {
 }
 
 // prepareTLS - prepares TLS configuration for the DNS proxy
-func (s *Server) prepareTLS(proxyConfig *proxy.Config) error {
+func (s *Server) prepareTLS(proxyConfig *proxy.Config) (err error) {
 	if len(s.conf.CertificateChainData) == 0 || len(s.conf.PrivateKeyData) == 0 {
 		return nil
 	}
@@ -478,25 +485,26 @@ func (s *Server) prepareTLS(proxyConfig *proxy.Config) error {
 		proxyConfig.QUICListenAddr,
 	)
 
-	var err error
 	s.conf.cert, err = tls.X509KeyPair(s.conf.CertificateChainData, s.conf.PrivateKeyData)
 	if err != nil {
 		return fmt.Errorf("failed to parse TLS keypair: %w", err)
 	}
 
+	cert, err := x509.ParseCertificate(s.conf.cert.Certificate[0])
+	if err != nil {
+		return fmt.Errorf("x509.ParseCertificate(): %w", err)
+	}
+
+	s.conf.hasIPAddrs = aghtls.CertificateHasIP(cert)
+
 	if s.conf.StrictSNICheck {
-		var x *x509.Certificate
+		if len(cert.DNSNames) != 0 {
-		x, err = x509.ParseCertificate(s.conf.cert.Certificate[0])
+			s.conf.dnsNames = cert.DNSNames
-		if err != nil {
+			log.Debug("dnsforward: using certificate's SAN as DNS names: %v", cert.DNSNames)
-			return fmt.Errorf("x509.ParseCertificate(): %w", err)
-		}
-		if len(x.DNSNames) != 0 {
-			s.conf.dnsNames = x.DNSNames
-			log.Debug("dns: using DNS names from certificate's SAN: %v", x.DNSNames)
 			sort.Strings(s.conf.dnsNames)
 		} else {
-			s.conf.dnsNames = append(s.conf.dnsNames, x.Subject.CommonName)
+			s.conf.dnsNames = append(s.conf.dnsNames, cert.Subject.CommonName)
-			log.Debug("dns: using DNS name from certificate's CN: %s", x.Subject.CommonName)
+			log.Debug("dnsforward: using certificate's CN as DNS name: %s", cert.Subject.CommonName)
 		}
 	}
 

internal/dnsforward/dns.go

@@ -259,21 +259,13 @@ func (s *Server) onDHCPLeaseChanged(flags int) {
 //
 // See https://www.ietf.org/archive/id/draft-ietf-add-ddr-10.html.
 func (s *Server) processDDRQuery(dctx *dnsContext) (rc resultCode) {
-	pctx := dctx.proxyCtx
-	q := pctx.Req.Question[0]
-
 	if !s.conf.HandleDDR {
 		return resultCodeSuccess
 	}
 
+	pctx := dctx.proxyCtx
+	q := pctx.Req.Question[0]
 	if q.Name == ddrHostFQDN {
-		if s.dnsProxy.TLSListenAddr == nil && s.conf.HTTPSListenAddrs == nil &&
-			s.dnsProxy.QUICListenAddr == nil || q.Qtype != dns.TypeSVCB {
-			pctx.Res = s.makeResponse(pctx.Req)
-
-			return resultCodeFinish
-		}
-
 		pctx.Res = s.makeDDRResponse(pctx.Req)
 
 		return resultCodeFinish
@@ -291,6 +283,10 @@ func (s *Server) processDDRQuery(dctx *dnsContext) (rc resultCode) {
 // [draft standard]: https://www.ietf.org/archive/id/draft-ietf-add-ddr-10.html.
 func (s *Server) makeDDRResponse(req *dns.Msg) (resp *dns.Msg) {
 	resp = s.makeResponse(req)
+	if req.Question[0].Qtype != dns.TypeSVCB {
+		return resp
+	}
+
 	// TODO(e.burkov):  Think about storing the FQDN version of the server's
 	// name somewhere.
 	domainName := dns.Fqdn(s.conf.ServerName)
@@ -312,20 +308,26 @@ func (s *Server) makeDDRResponse(req *dns.Msg) (resp *dns.Msg) {
 		resp.Answer = append(resp.Answer, ans)
 	}
 
-	for _, addr := range s.dnsProxy.TLSListenAddr {
+	if s.conf.hasIPAddrs {
-		values := []dns.SVCBKeyValue{
+		// Only add DNS-over-TLS resolvers in case the certificate contains IP
-			&dns.SVCBAlpn{Alpn: []string{"dot"}},
+		// addresses.
-			&dns.SVCBPort{Port: uint16(addr.Port)},
+		//
-		}
+		// See https://github.com/AdguardTeam/AdGuardHome/issues/4927.
+		for _, addr := range s.dnsProxy.TLSListenAddr {
+			values := []dns.SVCBKeyValue{
+				&dns.SVCBAlpn{Alpn: []string{"dot"}},
+				&dns.SVCBPort{Port: uint16(addr.Port)},
+			}
 
-		ans := &dns.SVCB{
+			ans := &dns.SVCB{
-			Hdr:      s.hdr(req, dns.TypeSVCB),
+				Hdr:      s.hdr(req, dns.TypeSVCB),
-			Priority: 1,
+				Priority: 1,
-			Target:   domainName,
+				Target:   domainName,
-			Value:    values,
+				Value:    values,
-		}
+			}
 
-		resp.Answer = append(resp.Answer, ans)
+			resp.Answer = append(resp.Answer, ans)
+		}
 	}
 
 	for _, addr := range s.dnsProxy.QUICListenAddr {

internal/dnsforward/dns_test.go

@@ -157,19 +157,9 @@ func TestServer_ProcessDDRQuery(t *testing.T) {
 func prepareTestServer(t *testing.T, portDoH, portDoT, portDoQ int, ddrEnabled bool) (s *Server) {
 	t.Helper()
 
-	proxyConf := proxy.Config{}
-
-	if portDoT > 0 {
-		proxyConf.TLSListenAddr = []*net.TCPAddr{{Port: portDoT}}
-	}
-
-	if portDoQ > 0 {
-		proxyConf.QUICListenAddr = []*net.UDPAddr{{Port: portDoQ}}
-	}
-
 	s = &Server{
 		dnsProxy: &proxy.Proxy{
-			Config: proxyConf,
+			Config: proxy.Config{},
 		},
 		conf: ServerConfig{
 			FilteringConfig: FilteringConfig{
@@ -181,8 +171,17 @@ func prepareTestServer(t *testing.T, portDoH, portDoT, portDoQ int, ddrEnabled b
 		},
 	}
 
+	if portDoT > 0 {
+		s.dnsProxy.TLSListenAddr = []*net.TCPAddr{{Port: portDoT}}
+		s.conf.hasIPAddrs = true
+	}
+
+	if portDoQ > 0 {
+		s.dnsProxy.QUICListenAddr = []*net.UDPAddr{{Port: portDoQ}}
+	}
+
 	if portDoH > 0 {
-		s.conf.TLSConfig.HTTPSListenAddrs = []*net.TCPAddr{{Port: portDoH}}
+		s.conf.HTTPSListenAddrs = []*net.TCPAddr{{Port: portDoH}}
 	}
 
 	return s

internal/home/tls.go

@@ -13,7 +13,6 @@ import (
 	"encoding/pem"
 	"fmt"
 	"net/http"
-	"net/netip"
 	"os"
 	"strings"
 	"sync"
@@ -21,6 +20,7 @@ import (
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghalg"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
+	"github.com/AdguardTeam/AdGuardHome/internal/aghtls"
 	"github.com/AdguardTeam/AdGuardHome/internal/dnsforward"
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
@@ -513,22 +513,6 @@ func validateCertChain(certs []*x509.Certificate, srvName string) (err error) {
 	return nil
 }
 
-// certHasIP returns true if cert has at least a single IP address either in its
-// DNS names or in the IP addresses section.
-func certHasIP(cert *x509.Certificate) (ok bool) {
-	if len(cert.IPAddresses) > 0 {
-		return true
-	}
-
-	for _, name := range cert.DNSNames {
-		if _, err := netip.ParseAddr(name); err == nil {
-			return true
-		}
-	}
-
-	return false
-}
-
 // parseCertChain parses the certificate chain from raw data, and returns it.
 // If ok is true, the returned error, if any, is not critical.
 func parseCertChain(chain []byte) (parsedCerts []*x509.Certificate, ok bool, err error) {
@@ -550,7 +534,7 @@ func parseCertChain(chain []byte) (parsedCerts []*x509.Certificate, ok bool, err
 
 	log.Info("tls: number of certs: %d", len(parsedCerts))
 
-	if !certHasIP(parsedCerts[0]) {
+	if !aghtls.CertificateHasIP(parsedCerts[0]) {
 		err = errors.Error(`certificate has no IP addresses` +
 			`, this may cause issues with DNS-over-TLS clients`)
 	}