From a4e0532e61ce968badfaeb3e2f09110fe67385a4 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Wed, 2 Nov 2022 14:37:41 +0300 Subject: [PATCH] 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 Date: Wed Nov 2 14:29:08 2022 +0300 Merge branch 'master' into 4927-ddr-ip-san commit f4770abf98ea2c0db2f0c2ddb9509a29a06c9509 Author: Eugene Burkov Date: Wed Nov 2 13:50:40 2022 +0300 dnsforward: imp logs commit 8d71371365070e221e104ae20acc8312e840eff9 Author: Eugene Burkov Date: Tue Nov 1 20:57:43 2022 +0300 all: imp code, docs commit 9793820f2c581e0ffcb28a59677be5c8df0c43f3 Author: Eugene Burkov Date: Tue Nov 1 19:37:39 2022 +0300 all: remember the cert props --- internal/dnsforward/config.go | 40 ++++++++++++++++----------- internal/dnsforward/dns.go | 48 +++++++++++++++++---------------- internal/dnsforward/dns_test.go | 23 ++++++++-------- internal/home/tls.go | 20 ++------------ 4 files changed, 62 insertions(+), 69 deletions(-) diff --git a/internal/dnsforward/config.go b/internal/dnsforward/config.go index c49f0fdd785..6357d681a3f 100644 --- a/internal/dnsforward/config.go +++ b/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 - x, err = x509.ParseCertificate(s.conf.cert.Certificate[0]) - if err != nil { - 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) + if len(cert.DNSNames) != 0 { + s.conf.dnsNames = cert.DNSNames + log.Debug("dnsforward: using certificate's SAN as DNS names: %v", cert.DNSNames) sort.Strings(s.conf.dnsNames) } else { - s.conf.dnsNames = append(s.conf.dnsNames, x.Subject.CommonName) - log.Debug("dns: using DNS name from certificate's CN: %s", x.Subject.CommonName) + s.conf.dnsNames = append(s.conf.dnsNames, cert.Subject.CommonName) + log.Debug("dnsforward: using certificate's CN as DNS name: %s", cert.Subject.CommonName) } } diff --git a/internal/dnsforward/dns.go b/internal/dnsforward/dns.go index ecf41709933..e4ca6520bb3 100644 --- a/internal/dnsforward/dns.go +++ b/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 { - values := []dns.SVCBKeyValue{ - &dns.SVCBAlpn{Alpn: []string{"dot"}}, - &dns.SVCBPort{Port: uint16(addr.Port)}, - } - - ans := &dns.SVCB{ - Hdr: s.hdr(req, dns.TypeSVCB), - Priority: 1, - Target: domainName, - Value: values, + if s.conf.hasIPAddrs { + // Only add DNS-over-TLS resolvers in case the certificate contains IP + // addresses. + // + // 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{ + Hdr: s.hdr(req, dns.TypeSVCB), + Priority: 1, + Target: domainName, + Value: values, + } + + resp.Answer = append(resp.Answer, ans) } - - resp.Answer = append(resp.Answer, ans) } for _, addr := range s.dnsProxy.QUICListenAddr { diff --git a/internal/dnsforward/dns_test.go b/internal/dnsforward/dns_test.go index 1c558744ead..d07c30dc525 100644 --- a/internal/dnsforward/dns_test.go +++ b/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 diff --git a/internal/home/tls.go b/internal/home/tls.go index a0426a47a1a..7fdd64d822f 100644 --- a/internal/home/tls.go +++ b/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`) }