diff --git a/CHANGELOG.md b/CHANGELOG.md index 83d53617..54b7aa61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,8 @@ See also the [v0.107.59 GitHub milestone][ms-v0.107.59]. ### Fixed +- Validation process for the DNS-over-TLS, DNS-over-QUIC, and HTTPS ports on the *Encryption Settings* page. + - Rules with the `client` modifier not working ([#7708]). - The search form not working in the query log ([#7704]). diff --git a/internal/dnsforward/beforerequest.go b/internal/dnsforward/beforerequest.go index 469af019..952ea253 100644 --- a/internal/dnsforward/beforerequest.go +++ b/internal/dnsforward/beforerequest.go @@ -74,7 +74,7 @@ func (s *Server) clientIDFromDNSContext(pctx *proxy.DNSContext) (clientID string return "", nil } - hostSrvName := s.conf.ServerName + hostSrvName := s.conf.TLSConf.ServerName if hostSrvName == "" { return "", nil } @@ -87,7 +87,7 @@ func (s *Server) clientIDFromDNSContext(pctx *proxy.DNSContext) (clientID string clientID, err = clientIDFromClientServerName( hostSrvName, cliSrvName, - s.conf.StrictSNICheck, + s.conf.TLSConf.StrictSNICheck, ) if err != nil { return "", fmt.Errorf("clientid check: %w", err) diff --git a/internal/dnsforward/beforerequest_internal_test.go b/internal/dnsforward/beforerequest_internal_test.go index d4be8782..35a1157b 100644 --- a/internal/dnsforward/beforerequest_internal_test.go +++ b/internal/dnsforward/beforerequest_internal_test.go @@ -121,7 +121,7 @@ func TestServer_HandleBefore_tls(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - s, _ := createTestTLS(t, TLSConfig{ + s, _ := createTestTLS(t, &TLSConfig{ TLSListenAddrs: []*net.TCPAddr{{}}, ServerName: tlsServerName, }) @@ -259,6 +259,7 @@ func TestServer_HandleBefore_udp(t *testing.T) { }, ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ AllowedClients: tc.allowedClients, DisallowedClients: tc.disallowedClients, diff --git a/internal/dnsforward/clientid_internal_test.go b/internal/dnsforward/clientid_internal_test.go index 097930bc..ce325c4b 100644 --- a/internal/dnsforward/clientid_internal_test.go +++ b/internal/dnsforward/clientid_internal_test.go @@ -212,13 +212,13 @@ func TestServer_clientIDFromDNSContext(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - tlsConf := TLSConfig{ + tlsConf := &TLSConfig{ ServerName: tc.confSrvName, StrictSNICheck: tc.strictSNI, } srv := &Server{ - conf: ServerConfig{TLSConfig: tlsConf}, + conf: ServerConfig{TLSConf: tlsConf}, baseLogger: slogutil.NewDiscardLogger(), } diff --git a/internal/dnsforward/config.go b/internal/dnsforward/config.go index 37e8ce98..9c3a85ec 100644 --- a/internal/dnsforward/config.go +++ b/internal/dnsforward/config.go @@ -11,7 +11,6 @@ import ( "strings" "time" - "github.com/AdguardTeam/AdGuardHome/internal/aghalg" "github.com/AdguardTeam/AdGuardHome/internal/aghhttp" "github.com/AdguardTeam/AdGuardHome/internal/aghnet" "github.com/AdguardTeam/AdGuardHome/internal/aghtls" @@ -168,43 +167,34 @@ type EDNSClientSubnet struct { UseCustom bool `yaml:"use_custom"` } -// TLSConfig is the TLS configuration for HTTPS, DNS-over-HTTPS, and DNS-over-TLS +// TLSConfig contains the TLS configuration settings for DNS-over-HTTPS (DoH), +// DNS-over-TLS (DoT), DNS-over-QUIC (DoQ), and Discovery of Designated +// Resolvers (DDR). type TLSConfig struct { - cert tls.Certificate + // Cert is the TLS certificate used for TLS connections. It is nil if + // encryption is disabled. + Cert *tls.Certificate - TLSListenAddrs []*net.TCPAddr `yaml:"-" json:"-"` - QUICListenAddrs []*net.UDPAddr `yaml:"-" json:"-"` - HTTPSListenAddrs []*net.TCPAddr `yaml:"-" json:"-"` + // TLSListenAddrs are the addresses to listen on for DoT connections. Each + // item in the list must be non-nil if Cert is not nil. + TLSListenAddrs []*net.TCPAddr - // PEM-encoded certificates chain - CertificateChain string `yaml:"certificate_chain" json:"certificate_chain"` - // PEM-encoded private key - PrivateKey string `yaml:"private_key" json:"private_key"` + // QUICListenAddrs are the addresses to listen on for DoQ connections. Each + // item in the list must be non-nil if Cert is not nil. + QUICListenAddrs []*net.UDPAddr - CertificatePath string `yaml:"certificate_path" json:"certificate_path"` - PrivateKeyPath string `yaml:"private_key_path" json:"private_key_path"` - - CertificateChainData []byte `yaml:"-" json:"-"` - PrivateKeyData []byte `yaml:"-" json:"-"` + // HTTPSListenAddrs should be the addresses AdGuard Home is listening on for + // DoH connections. These addresses are announced with DDR. Each item in + // the list must be non-nil. + HTTPSListenAddrs []*net.TCPAddr // ServerName is the hostname of the server. Currently, it is only being // used for ClientID checking and Discovery of Designated Resolvers (DDR). - ServerName string `yaml:"-" json:"-"` - - // 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:"-"` + ServerName string // 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 + StrictSNICheck bool } // DNSCryptConfig is the DNSCrypt server configuration struct. @@ -239,8 +229,11 @@ type ServerConfig struct { // Remove that. AddrProcConf *client.DefaultAddrProcConfig + // TLSConf is the TLS configuration for DNS-over-TLS, DNS-over-QUIC, and + // HTTPS. It must not be nil. + TLSConf *TLSConfig + Config - TLSConfig DNSCryptConfig TLSAllowUnencryptedDoH bool @@ -608,45 +601,33 @@ func (conf *ServerConfig) ourAddrsSet() (m addrPortSet, err error) { } } -// prepareTLS - prepares TLS configuration for the DNS proxy +// prepareTLS sets up the TLS configuration for the DNS proxy. func (s *Server) prepareTLS(proxyConfig *proxy.Config) (err error) { - if len(s.conf.CertificateChainData) == 0 || len(s.conf.PrivateKeyData) == 0 { + if s.conf.TLSConf.Cert == nil { + return + } + + if s.conf.TLSConf.TLSListenAddrs == nil && s.conf.TLSConf.QUICListenAddrs == nil { return nil } - if s.conf.TLSListenAddrs == nil && s.conf.QUICListenAddrs == nil { - return nil - } + proxyConfig.TLSListenAddr = s.conf.TLSConf.TLSListenAddrs + proxyConfig.QUICListenAddr = s.conf.TLSConf.QUICListenAddrs - proxyConfig.TLSListenAddr = aghalg.CoalesceSlice( - s.conf.TLSListenAddrs, - proxyConfig.TLSListenAddr, - ) - - proxyConfig.QUICListenAddr = aghalg.CoalesceSlice( - s.conf.QUICListenAddrs, - proxyConfig.QUICListenAddr, - ) - - 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]) + cert, err := x509.ParseCertificate(s.conf.TLSConf.Cert.Certificate[0]) if err != nil { return fmt.Errorf("x509.ParseCertificate(): %w", err) } - s.conf.hasIPAddrs = aghtls.CertificateHasIP(cert) + s.hasIPAddrs = aghtls.CertificateHasIP(cert) - if s.conf.StrictSNICheck { + if s.conf.TLSConf.StrictSNICheck { if len(cert.DNSNames) != 0 { - s.conf.dnsNames = cert.DNSNames + s.dnsNames = cert.DNSNames log.Debug("dns: using certificate's SAN as DNS names: %v", cert.DNSNames) - slices.Sort(s.conf.dnsNames) + slices.Sort(s.dnsNames) } else { - s.conf.dnsNames = append(s.conf.dnsNames, cert.Subject.CommonName) + s.dnsNames = []string{cert.Subject.CommonName} log.Debug("dns: using certificate's CN as DNS name: %s", cert.Subject.CommonName) } } @@ -695,11 +676,11 @@ func anyNameMatches(dnsNames []string, sni string) (ok bool) { // Called by 'tls' package when Client Hello is received // If the server name (from SNI) supplied by client is incorrect - we terminate the ongoing TLS handshake. func (s *Server) onGetCertificate(ch *tls.ClientHelloInfo) (*tls.Certificate, error) { - if s.conf.StrictSNICheck && !anyNameMatches(s.conf.dnsNames, ch.ServerName) { + if s.conf.TLSConf.StrictSNICheck && !anyNameMatches(s.dnsNames, ch.ServerName) { log.Info("dns: tls: unknown SNI in Client Hello: %s", ch.ServerName) return nil, fmt.Errorf("invalid SNI") } - return &s.conf.cert, nil + return s.conf.TLSConf.Cert, nil } // preparePlain prepares the plain-DNS configuration for the DNS proxy. diff --git a/internal/dnsforward/dns64_internal_test.go b/internal/dnsforward/dns64_internal_test.go index 205cbe7e..2d2c5471 100644 --- a/internal/dnsforward/dns64_internal_test.go +++ b/internal/dnsforward/dns64_internal_test.go @@ -296,6 +296,7 @@ func TestServer_HandleDNSRequest_dns64(t *testing.T) { UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, UseDNS64: true, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{Enabled: false}, @@ -335,6 +336,7 @@ func TestServer_dns64WithDisabledRDNS(t *testing.T) { UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, UseDNS64: true, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{Enabled: false}, diff --git a/internal/dnsforward/dnsforward.go b/internal/dnsforward/dnsforward.go index 00cfbe30..2f695d7a 100644 --- a/internal/dnsforward/dnsforward.go +++ b/internal/dnsforward/dnsforward.go @@ -103,16 +103,26 @@ type SystemResolvers interface { // // The zero Server is empty and ready for use. type Server struct { - // dnsProxy is the DNS proxy for forwarding client's DNS requests. - dnsProxy *proxy.Proxy + // addrProc, if not nil, is used to process clients' IP addresses with rDNS, + // WHOIS, etc. + addrProc client.AddressProcessor - // dnsFilter is the DNS filter for filtering client's DNS requests and - // responses. - dnsFilter *filtering.DNSFilter + // bootstrap is the resolver for upstreams' hostnames. + bootstrap upstream.Resolver + + // clientIDCache is a temporary storage for ClientIDs that were extracted + // during the BeforeRequestHandler stage. + clientIDCache cache.Cache // dhcpServer is the DHCP server for accessing lease data. dhcpServer DHCP + // etcHosts contains the current data from the system's hosts files. + etcHosts upstream.Resolver + + // privateNets is the configured set of IP networks considered private. + privateNets netutil.SubnetSet + // queryLog is the query log for client's DNS requests, responses and // filtering results. queryLog querylog.QueryLog @@ -120,37 +130,43 @@ type Server struct { // stats is the statistics collector for client's DNS usage data. stats stats.Interface + // sysResolvers used to fetch system resolvers to use by default for private + // PTR resolving. + sysResolvers SystemResolvers + // access drops disallowed clients. access *accessManager + // anonymizer masks the client's IP addresses if needed. + anonymizer *aghnet.IPMut + // baseLogger is used to create loggers for other entities. It should not // have a prefix and must not be nil. baseLogger *slog.Logger - // localDomainSuffix is the suffix used to detect internal hosts. It - // must be a valid domain name plus dots on each side. - localDomainSuffix string + // dnsFilter is the DNS filter for filtering client's DNS requests and + // responses. + dnsFilter *filtering.DNSFilter + + // dnsProxy is the DNS proxy for forwarding client's DNS requests. + dnsProxy *proxy.Proxy + + // internalProxy resolves internal requests from the application itself. It + // isn't started and so no listen ports are required. + internalProxy *proxy.Proxy // ipset processes DNS requests using ipset data. It must not be nil after // initialization. See [newIpsetHandler]. ipset *ipsetHandler - // privateNets is the configured set of IP networks considered private. - privateNets netutil.SubnetSet + // dns64Pref is the NAT64 prefix used for DNS64 response mapping. The major + // part of DNS64 happens inside the [proxy] package, but there still are + // some places where response mapping is needed (e.g. DHCP). + dns64Pref netip.Prefix - // addrProc, if not nil, is used to process clients' IP addresses with rDNS, - // WHOIS, etc. - addrProc client.AddressProcessor - - // sysResolvers used to fetch system resolvers to use by default for private - // PTR resolving. - sysResolvers SystemResolvers - - // etcHosts contains the current data from the system's hosts files. - etcHosts upstream.Resolver - - // bootstrap is the resolver for upstreams' hostnames. - bootstrap upstream.Resolver + // localDomainSuffix is the suffix used to detect internal hosts. It + // must be a valid domain name plus dots on each side. + localDomainSuffix string // bootResolvers are the resolvers that should be used for // bootstrapping along with [etcHosts]. @@ -159,34 +175,26 @@ type Server struct { // [upstream.Resolver] interface. bootResolvers []*upstream.UpstreamResolver - // dns64Pref is the NAT64 prefix used for DNS64 response mapping. The major - // part of DNS64 happens inside the [proxy] package, but there still are - // some places where response mapping is needed (e.g. DHCP). - dns64Pref netip.Prefix - - // anonymizer masks the client's IP addresses if needed. - anonymizer *aghnet.IPMut - - // clientIDCache is a temporary storage for ClientIDs that were extracted - // during the BeforeRequestHandler stage. - clientIDCache cache.Cache - - // internalProxy resolves internal requests from the application itself. It - // isn't started and so no listen ports are required. - internalProxy *proxy.Proxy - - // isRunning is true if the DNS server is running. - isRunning bool - - // protectionUpdateInProgress is used to make sure that only one goroutine - // updating the protection configuration after a pause is running at a time. - protectionUpdateInProgress atomic.Bool + // dnsNames are the DNS names from certificate (SAN) or CN value from + // Subject. + dnsNames []string // conf is the current configuration of the server. conf ServerConfig // serverLock protects Server. serverLock sync.RWMutex + + // protectionUpdateInProgress is used to make sure that only one goroutine + // updating the protection configuration after a pause is running at a time. + protectionUpdateInProgress atomic.Bool + + // isRunning is true if the DNS server is running. + isRunning bool + + // hasIPAddrs is set during the certificate parsing and is true if the + // configured certificate contains at least a single IP address. + hasIPAddrs bool } // defaultLocalDomainSuffix is the default suffix used to detect internal hosts diff --git a/internal/dnsforward/dnsforward_internal_test.go b/internal/dnsforward/dnsforward_internal_test.go index 5f035f84..3889fb4c 100644 --- a/internal/dnsforward/dnsforward_internal_test.go +++ b/internal/dnsforward/dnsforward_internal_test.go @@ -213,17 +213,23 @@ func createServerTLSConfig(t *testing.T) (*tls.Config, []byte, []byte) { }, certPem, keyPem } -func createTestTLS(t *testing.T, tlsConf TLSConfig) (s *Server, certPem []byte) { +func createTestTLS(t *testing.T, tlsConf *TLSConfig) (s *Server, certPem []byte) { t.Helper() var keyPem []byte _, certPem, keyPem = createServerTLSConfig(t) + cert, err := tls.X509KeyPair(certPem, keyPem) + require.NoError(t, err) + + tlsConf.Cert = &cert + s = createTestServer(t, &filtering.Config{ BlockingMode: filtering.BlockingModeDefault, }, ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: tlsConf, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{Enabled: false}, @@ -232,10 +238,7 @@ func createTestTLS(t *testing.T, tlsConf TLSConfig) (s *Server, certPem []byte) ServePlainDNS: true, }) - tlsConf.CertificateChainData, tlsConf.PrivateKeyData = certPem, keyPem - s.conf.TLSConfig = tlsConf - - err := s.Prepare(&s.conf) + err = s.Prepare(&s.conf) require.NoErrorf(t, err, "failed to prepare server: %s", err) return s, certPem @@ -354,6 +357,7 @@ func TestServer(t *testing.T) { }, ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{Enabled: false}, @@ -395,6 +399,7 @@ func TestServer_timeout(t *testing.T) { t.Run("custom", func(t *testing.T) { srvConf := &ServerConfig{ UpstreamTimeout: testTimeout, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{Enabled: false}, @@ -422,6 +427,7 @@ func TestServer_timeout(t *testing.T) { }) require.NoError(t, err) + s.conf.TLSConf = &TLSConfig{} s.conf.Config.UpstreamMode = UpstreamModeLoadBalance s.conf.Config.EDNSClientSubnet = &EDNSClientSubnet{ Enabled: false, @@ -436,6 +442,7 @@ func TestServer_timeout(t *testing.T) { func TestServer_Prepare_fallbacks(t *testing.T) { srvConf := &ServerConfig{ + TLSConf: &TLSConfig{}, Config: Config{ FallbackDNS: []string{ "#tls://1.1.1.1", @@ -466,6 +473,7 @@ func TestServerWithProtectionDisabled(t *testing.T) { }, ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{Enabled: false}, @@ -487,7 +495,7 @@ func TestServerWithProtectionDisabled(t *testing.T) { } func TestDoTServer(t *testing.T) { - s, certPem := createTestTLS(t, TLSConfig{ + s, certPem := createTestTLS(t, &TLSConfig{ TLSListenAddrs: []*net.TCPAddr{{}}, }) s.conf.UpstreamConfig.Upstreams = []upstream.Upstream{newGoogleUpstream()} @@ -511,7 +519,7 @@ func TestDoTServer(t *testing.T) { } func TestDoQServer(t *testing.T) { - s, _ := createTestTLS(t, TLSConfig{ + s, _ := createTestTLS(t, &TLSConfig{ QUICListenAddrs: []*net.UDPAddr{{IP: net.IP{127, 0, 0, 1}}}, }) s.conf.UpstreamConfig.Upstreams = []upstream.Upstream{newGoogleUpstream()} @@ -596,6 +604,7 @@ func TestSafeSearch(t *testing.T) { forwardConf := ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{ @@ -690,6 +699,7 @@ func TestInvalidRequest(t *testing.T) { }, ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{ @@ -721,6 +731,7 @@ func TestBlockedRequest(t *testing.T) { forwardConf := ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{ @@ -758,6 +769,7 @@ func TestServerCustomClientUpstream(t *testing.T) { forwardConf := ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ CacheSize: defaultCacheSize, UpstreamMode: UpstreamModeLoadBalance, @@ -838,6 +850,7 @@ func TestBlockCNAMEProtectionEnabled(t *testing.T) { }, ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{ @@ -873,6 +886,7 @@ func TestBlockCNAME(t *testing.T) { forwardConf := ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{ @@ -947,6 +961,7 @@ func TestClientRulesForCNAMEMatching(t *testing.T) { forwardConf := ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{ @@ -994,6 +1009,7 @@ func TestNullBlockedRequest(t *testing.T) { forwardConf := ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{ @@ -1064,6 +1080,7 @@ func TestBlockedCustomIP(t *testing.T) { conf := &ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamDNS: []string{"8.8.8.8:53", "8.8.4.4:53"}, UpstreamMode: UpstreamModeLoadBalance, @@ -1119,6 +1136,7 @@ func TestBlockedByHosts(t *testing.T) { forwardConf := ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{ @@ -1172,6 +1190,7 @@ func TestBlockedBySafeBrowsing(t *testing.T) { forwardConf := ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{ @@ -1235,6 +1254,7 @@ func TestRewrite(t *testing.T) { assert.NoError(t, s.Prepare(&ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamDNS: []string{"8.8.8.8:53"}, UpstreamMode: UpstreamModeLoadBalance, @@ -1369,6 +1389,7 @@ func TestPTRResponseFromDHCPLeases(t *testing.T) { s.conf.UDPListenAddrs = []*net.UDPAddr{{}} s.conf.TCPListenAddrs = []*net.TCPAddr{{}} s.conf.UpstreamDNS = []string{"127.0.0.1:53"} + s.conf.TLSConf = &TLSConfig{} s.conf.Config.EDNSClientSubnet = &EDNSClientSubnet{Enabled: false} s.conf.Config.ClientsContainer = EmptyClientsContainer{} s.conf.Config.UpstreamMode = UpstreamModeLoadBalance @@ -1457,6 +1478,7 @@ func TestPTRResponseFromHosts(t *testing.T) { s.conf.UDPListenAddrs = []*net.UDPAddr{{}} s.conf.TCPListenAddrs = []*net.TCPAddr{{}} s.conf.UpstreamDNS = []string{"127.0.0.1:53"} + s.conf.TLSConf = &TLSConfig{} s.conf.Config.EDNSClientSubnet = &EDNSClientSubnet{Enabled: false} s.conf.Config.ClientsContainer = EmptyClientsContainer{} s.conf.Config.UpstreamMode = UpstreamModeLoadBalance @@ -1723,6 +1745,7 @@ func TestServer_Exchange(t *testing.T) { srv := createTestServer(t, &filtering.Config{ BlockingMode: filtering.BlockingModeDefault, }, ServerConfig{ + TLSConf: &TLSConfig{}, Config: Config{ UpstreamDNS: []string{upsAddr}, UpstreamMode: UpstreamModeLoadBalance, @@ -1746,6 +1769,7 @@ func TestServer_Exchange(t *testing.T) { srv := createTestServer(t, &filtering.Config{ BlockingMode: filtering.BlockingModeDefault, }, ServerConfig{ + TLSConf: &TLSConfig{}, Config: Config{ UpstreamDNS: []string{upsAddr}, UpstreamMode: UpstreamModeLoadBalance, diff --git a/internal/dnsforward/dnsrewrite_internal_test.go b/internal/dnsforward/dnsrewrite_internal_test.go index 56043b2e..f30c661b 100644 --- a/internal/dnsforward/dnsrewrite_internal_test.go +++ b/internal/dnsforward/dnsrewrite_internal_test.go @@ -37,6 +37,7 @@ func TestServer_FilterDNSRewrite(t *testing.T) { srv := createTestServer(t, &filtering.Config{ BlockingMode: filtering.BlockingModeDefault, }, ServerConfig{ + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{Enabled: false}, diff --git a/internal/dnsforward/filter_internal_test.go b/internal/dnsforward/filter_internal_test.go index 7f1ab293..b14df3f2 100644 --- a/internal/dnsforward/filter_internal_test.go +++ b/internal/dnsforward/filter_internal_test.go @@ -31,6 +31,7 @@ func TestHandleDNSRequest_handleDNSRequest(t *testing.T) { forwardConf := ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{ diff --git a/internal/dnsforward/http_internal_test.go b/internal/dnsforward/http_internal_test.go index bf04ee1b..f57783c7 100644 --- a/internal/dnsforward/http_internal_test.go +++ b/internal/dnsforward/http_internal_test.go @@ -76,6 +76,7 @@ func TestDNSForwardHTTP_handleGetConfig(t *testing.T) { forwardConf := ServerConfig{ UDPListenAddrs: []*net.UDPAddr{}, TCPListenAddrs: []*net.TCPAddr{}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamDNS: []string{"8.8.8.8:53", "8.8.4.4:53"}, FallbackDNS: []string{"9.9.9.10"}, @@ -159,6 +160,7 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) { forwardConf := ServerConfig{ UDPListenAddrs: []*net.UDPAddr{}, TCPListenAddrs: []*net.TCPAddr{}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamDNS: []string{"8.8.8.8:53", "8.8.4.4:53"}, RatelimitSubnetLenIPv4: 24, @@ -369,6 +371,7 @@ func TestServer_HandleTestUpstreamDNS(t *testing.T) { UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, UpstreamTimeout: upsTimeout, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{Enabled: false}, diff --git a/internal/dnsforward/process.go b/internal/dnsforward/process.go index 259aeff2..405ccc70 100644 --- a/internal/dnsforward/process.go +++ b/internal/dnsforward/process.go @@ -246,9 +246,9 @@ func (s *Server) makeDDRResponse(req *dns.Msg) (resp *dns.Msg) { // TODO(e.burkov): Think about storing the FQDN version of the server's // name somewhere. - domainName := dns.Fqdn(s.conf.ServerName) + domainName := dns.Fqdn(s.conf.TLSConf.ServerName) - for _, addr := range s.conf.HTTPSListenAddrs { + for _, addr := range s.conf.TLSConf.HTTPSListenAddrs { values := []dns.SVCBKeyValue{ &dns.SVCBAlpn{Alpn: []string{"h2"}}, &dns.SVCBPort{Port: uint16(addr.Port)}, @@ -265,7 +265,7 @@ func (s *Server) makeDDRResponse(req *dns.Msg) (resp *dns.Msg) { resp.Answer = append(resp.Answer, ans) } - if s.conf.hasIPAddrs { + if s.hasIPAddrs { // Only add DNS-over-TLS resolvers in case the certificate contains IP // addresses. // diff --git a/internal/dnsforward/process_internal_test.go b/internal/dnsforward/process_internal_test.go index dcff3b9f..d2aeda09 100644 --- a/internal/dnsforward/process_internal_test.go +++ b/internal/dnsforward/process_internal_test.go @@ -3,6 +3,7 @@ package dnsforward import ( "cmp" "context" + "crypto/tls" "net" "net/netip" "testing" @@ -77,6 +78,7 @@ func TestServer_ProcessInitial(t *testing.T) { t.Parallel() c := ServerConfig{ + TLSConf: &TLSConfig{}, Config: Config{ AAAADisabled: tc.aaaaDisabled, UpstreamMode: UpstreamModeLoadBalance, @@ -177,6 +179,7 @@ func TestServer_ProcessFilteringAfterResponse(t *testing.T) { t.Parallel() c := ServerConfig{ + TLSConf: &TLSConfig{}, Config: Config{ AAAADisabled: tc.aaaaDisabled, UpstreamMode: UpstreamModeLoadBalance, @@ -316,6 +319,8 @@ func TestServer_ProcessDDRQuery(t *testing.T) { }} _, certPem, keyPem := createServerTLSConfig(t) + cert, err := tls.X509KeyPair(certPem, keyPem) + require.NoError(t, err) for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ -328,19 +333,18 @@ func TestServer_ProcessDDRQuery(t *testing.T) { EDNSClientSubnet: &EDNSClientSubnet{Enabled: false}, ClientsContainer: EmptyClientsContainer{}, }, - TLSConfig: TLSConfig{ - ServerName: ddrTestDomainName, - CertificateChainData: certPem, - PrivateKeyData: keyPem, - TLSListenAddrs: tc.addrsDoT, - HTTPSListenAddrs: tc.addrsDoH, - QUICListenAddrs: tc.addrsDoQ, + TLSConf: &TLSConfig{ + ServerName: ddrTestDomainName, + Cert: &cert, + TLSListenAddrs: tc.addrsDoT, + HTTPSListenAddrs: tc.addrsDoH, + QUICListenAddrs: tc.addrsDoQ, }, ServePlainDNS: true, }) // TODO(e.burkov): Generate a certificate actually containing the // IP addresses. - s.conf.hasIPAddrs = true + s.hasIPAddrs = true req := createTestMessageWithType(tc.host, tc.qtype) @@ -657,6 +661,7 @@ func TestServer_HandleDNSRequest_restrictLocal(t *testing.T) { }, ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, // TODO(s.chzhen): Add tests where EDNSClientSubnet.Enabled is true. // Improve Config declaration for tests. Config: Config{ @@ -789,6 +794,7 @@ func TestServer_ProcessUpstream_localPTR(t *testing.T) { ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{Enabled: false}, @@ -818,6 +824,7 @@ func TestServer_ProcessUpstream_localPTR(t *testing.T) { ServerConfig{ UDPListenAddrs: []*net.UDPAddr{{}}, TCPListenAddrs: []*net.TCPAddr{{}}, + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{Enabled: false}, diff --git a/internal/dnsforward/svcbmsg_internal_test.go b/internal/dnsforward/svcbmsg_internal_test.go index 611549db..7de96da8 100644 --- a/internal/dnsforward/svcbmsg_internal_test.go +++ b/internal/dnsforward/svcbmsg_internal_test.go @@ -16,6 +16,7 @@ func TestGenAnswerHTTPS_andSVCB(t *testing.T) { s := createTestServer(t, &filtering.Config{ BlockingMode: filtering.BlockingModeDefault, }, ServerConfig{ + TLSConf: &TLSConfig{}, Config: Config{ UpstreamMode: UpstreamModeLoadBalance, EDNSClientSubnet: &EDNSClientSubnet{Enabled: false}, diff --git a/internal/home/config.go b/internal/home/config.go index 23cdd7fe..aa09746a 100644 --- a/internal/home/config.go +++ b/internal/home/config.go @@ -6,6 +6,7 @@ import ( "net/netip" "os" "path/filepath" + "slices" "sync" "github.com/AdguardTeam/AdGuardHome/internal/aghalg" @@ -23,6 +24,7 @@ import ( "github.com/AdguardTeam/golibs/log" "github.com/AdguardTeam/golibs/netutil" "github.com/AdguardTeam/golibs/timeutil" + "github.com/google/go-cmp/cmp" "github.com/google/renameio/v2/maybe" yaml "gopkg.in/yaml.v3" ) @@ -263,28 +265,116 @@ type dnsConfig struct { HostsFileEnabled bool `yaml:"hostsfile_enabled"` } +// tlsConfigSettings is the TLS configuration for DNS-over-TLS, DNS-over-QUIC, +// and HTTPS. When adding new properties, update the [tlsConfigSettings.clone] +// and [tlsConfigSettings.setPrivateFieldsAndCompare] methods as necessary. type tlsConfigSettings struct { - Enabled bool `yaml:"enabled" json:"enabled"` // Enabled is the encryption (DoT/DoH/HTTPS) status - ServerName string `yaml:"server_name" json:"server_name,omitempty"` // ServerName is the hostname of your HTTPS/TLS server - ForceHTTPS bool `yaml:"force_https" json:"force_https"` // ForceHTTPS: if true, forces HTTP->HTTPS redirect - PortHTTPS uint16 `yaml:"port_https" json:"port_https,omitempty"` // HTTPS port. If 0, HTTPS will be disabled - PortDNSOverTLS uint16 `yaml:"port_dns_over_tls" json:"port_dns_over_tls,omitempty"` // DNS-over-TLS port. If 0, DoT will be disabled - PortDNSOverQUIC uint16 `yaml:"port_dns_over_quic" json:"port_dns_over_quic,omitempty"` // DNS-over-QUIC port. If 0, DoQ will be disabled + // Enabled indicates whether encryption (DoT/DoH/HTTPS) is enabled. + Enabled bool `yaml:"enabled" json:"enabled"` - // PortDNSCrypt is the port for DNSCrypt requests. If it's zero, - // DNSCrypt is disabled. + // ServerName is the hostname of the HTTPS/TLS server. + ServerName string `yaml:"server_name" json:"server_name,omitempty"` + + // ForceHTTPS, if true, forces an HTTP to HTTPS redirect. + ForceHTTPS bool `yaml:"force_https" json:"force_https"` + + // PortHTTPS is the HTTPS port. If 0, HTTPS will be disabled. + PortHTTPS uint16 `yaml:"port_https" json:"port_https,omitempty"` + + // PortDNSOverTLS is the DNS-over-TLS port. If 0, DoT will be disabled. + PortDNSOverTLS uint16 `yaml:"port_dns_over_tls" json:"port_dns_over_tls,omitempty"` + + // PortDNSOverQUIC is the DNS-over-QUIC port. If 0, DoQ will be disabled. + PortDNSOverQUIC uint16 `yaml:"port_dns_over_quic" json:"port_dns_over_quic,omitempty"` + + // PortDNSCrypt is the port for DNSCrypt requests. If it's zero, DNSCrypt + // is disabled. PortDNSCrypt uint16 `yaml:"port_dnscrypt" json:"port_dnscrypt"` - // DNSCryptConfigFile is the path to the DNSCrypt config file. Must be - // set if PortDNSCrypt is not zero. + + // DNSCryptConfigFile is the path to the DNSCrypt config file. Must be set + // if PortDNSCrypt is not zero. // // See https://github.com/AdguardTeam/dnsproxy and // https://github.com/ameshkov/dnscrypt. DNSCryptConfigFile string `yaml:"dnscrypt_config_file" json:"dnscrypt_config_file"` - // Allow DoH queries via unencrypted HTTP (e.g. for reverse proxying) + // AllowUnencryptedDoH allows DoH queries via unencrypted HTTP (e.g. for + // reverse proxying). + // + // TODO(s.chzhen): Add this option into the Web UI. AllowUnencryptedDoH bool `yaml:"allow_unencrypted_doh" json:"allow_unencrypted_doh"` - dnsforward.TLSConfig `yaml:",inline" json:",inline"` + // CertificateChain is the PEM-encoded certificate chain. Must be empty if + // [tlsConfigSettings.CertificatePath] is provided. + CertificateChain string `yaml:"certificate_chain" json:"certificate_chain"` + + // PrivateKey is the PEM-encoded private key. Must be empty if + // [tlsConfigSettings.PrivateKeyPath] is provided. + PrivateKey string `yaml:"private_key" json:"private_key"` + + // CertificatePath is the path to the certificate file. Must be empty if + // [tlsConfigSettings.CertificateChain] is provided. + CertificatePath string `yaml:"certificate_path" json:"certificate_path"` + + // PrivateKeyPath is the path to the private key file. Must be empty if + // [tlsConfigSettings.PrivateKey] is provided. + PrivateKeyPath string `yaml:"private_key_path" json:"private_key_path"` + + // 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:"-"` + + // CertificateChainData is the PEM-encoded byte data for the certificate + // chain. + CertificateChainData []byte `yaml:"-" json:"-"` + + // PrivateKeyData is the PEM-encoded byte data for the private key. + PrivateKeyData []byte `yaml:"-" json:"-"` + + // StrictSNICheck controls if the connections with SNI mismatching the + // certificate's ones should be rejected. + StrictSNICheck bool `yaml:"strict_sni_check" json:"-"` +} + +// clone returns a deep copy of c. +func (c *tlsConfigSettings) clone() (clone *tlsConfigSettings) { + clone = &tlsConfigSettings{} + *clone = *c + + clone.OverrideTLSCiphers = slices.Clone(c.OverrideTLSCiphers) + clone.CertificateChainData = slices.Clone(c.CertificateChainData) + clone.PrivateKeyData = slices.Clone(c.PrivateKeyData) + + return clone +} + +// setPrivateFieldsAndCompare sets any missing properties in conf to match those +// in c and returns true if TLS configurations are equal. conf must not be be +// nil. +// It sets the following properties because these are not accepted from the +// frontend: +// +// [tlsConfigSettings.AllowUnencryptedDoH] +// [tlsConfigSettings.DNSCryptConfigFile] +// [tlsConfigSettings.OverrideTLSCiphers] +// [tlsConfigSettings.PortDNSCrypt] +// +// The following properties are skipped as they are set by +// [tlsManager.loadTLSConfig]: +// +// [tlsConfigSettings.CertificateChainData] +// [tlsConfigSettings.PrivateKeyData] +func (c *tlsConfigSettings) setPrivateFieldsAndCompare(conf *tlsConfigSettings) (equal bool) { + conf.OverrideTLSCiphers = slices.Clone(c.OverrideTLSCiphers) + + // TODO(s.chzhen): Remove this once the frontend supports it. + conf.AllowUnencryptedDoH = c.AllowUnencryptedDoH + + conf.DNSCryptConfigFile = c.DNSCryptConfigFile + conf.PortDNSCrypt = c.PortDNSCrypt + + // TODO(a.garipov): Define a custom comparer. + return cmp.Equal(c, conf) } type queryLogConfig struct { @@ -649,9 +739,8 @@ func (c *configuration) write(tlsMgr *tlsManager) (err error) { } if tlsMgr != nil { - tlsConf := tlsConfigSettings{} - tlsMgr.WriteDiskConfig(&tlsConf) - config.TLS = tlsConf + tlsConf := tlsMgr.config() + config.TLS = *tlsConf } if globalContext.stats != nil { diff --git a/internal/home/controlupdate.go b/internal/home/controlupdate.go index 724f6862..2974a1d1 100644 --- a/internal/home/controlupdate.go +++ b/internal/home/controlupdate.go @@ -164,11 +164,8 @@ func (vr *versionResponse) setAllowedToAutoUpdate(tlsMgr *tlsManager) (err error return nil } - tlsConf := &tlsConfigSettings{} - tlsMgr.WriteDiskConfig(tlsConf) - canUpdate := true - if tlsConfUsesPrivilegedPorts(tlsConf) || + if tlsConfUsesPrivilegedPorts(tlsMgr.config()) || config.HTTPConfig.Address.Port() < 1024 || config.DNS.Port < 1024 { canUpdate, err = aghnet.CanBindPrivilegedPorts() diff --git a/internal/home/dns.go b/internal/home/dns.go index 766136fc..1c68d948 100644 --- a/internal/home/dns.go +++ b/internal/home/dns.go @@ -2,6 +2,7 @@ package home import ( "context" + "crypto/tls" "fmt" "log/slog" "net" @@ -111,9 +112,6 @@ func initDNS( return err } - tlsConf := &tlsConfigSettings{} - tlsMgr.WriteDiskConfig(tlsConf) - return initDNSServer( globalContext.filters, globalContext.stats, @@ -121,7 +119,7 @@ func initDNS( globalContext.dhcpServer, anonymizer, httpRegister, - tlsConf, + tlsMgr.config(), tlsMgr, baseLogger, ) @@ -255,11 +253,16 @@ func newServerConfig( fwdConf := dnsConf.Config fwdConf.ClientsContainer = clientsContainer + intTLSConf, err := newDNSTLSConfig(tlsConf, hosts) + if err != nil { + return nil, fmt.Errorf("constructing tls config: %w", err) + } + newConf = &dnsforward.ServerConfig{ UDPListenAddrs: ipsToUDPAddrs(hosts, dnsConf.Port), TCPListenAddrs: ipsToTCPAddrs(hosts, dnsConf.Port), Config: fwdConf, - TLSConfig: newDNSTLSConfig(tlsConf, hosts), + TLSConf: intTLSConf, TLSAllowUnencryptedDoH: tlsConf.AllowUnencryptedDoH, UpstreamTimeout: time.Duration(dnsConf.UpstreamTimeout), TLSv12Roots: tlsMgr.rootCerts, @@ -304,14 +307,25 @@ func newServerConfig( } // newDNSTLSConfig converts values from the configuration file into the internal -// TLS settings for the DNS server. tlsConf must not be nil. -func newDNSTLSConfig(conf *tlsConfigSettings, addrs []netip.Addr) (dnsConf dnsforward.TLSConfig) { +// TLS settings for the DNS server. conf must not be nil. +func newDNSTLSConfig( + conf *tlsConfigSettings, + addrs []netip.Addr, +) (dnsConf *dnsforward.TLSConfig, err error) { if !conf.Enabled { - return dnsforward.TLSConfig{} + return &dnsforward.TLSConfig{}, nil } - dnsConf = conf.TLSConfig - dnsConf.ServerName = conf.ServerName + cert, err := tls.X509KeyPair(conf.CertificateChainData, conf.PrivateKeyData) + if err != nil { + return nil, fmt.Errorf("parsing tls key pair: %w", err) + } + + dnsConf = &dnsforward.TLSConfig{ + Cert: &cert, + ServerName: conf.ServerName, + StrictSNICheck: conf.StrictSNICheck, + } if conf.PortHTTPS != 0 { dnsConf.HTTPSListenAddrs = ipsToTCPAddrs(addrs, conf.PortHTTPS) @@ -325,7 +339,7 @@ func newDNSTLSConfig(conf *tlsConfigSettings, addrs []netip.Addr) (dnsConf dnsfo dnsConf.QUICListenAddrs = ipsToUDPAddrs(addrs, conf.PortDNSOverQUIC) } - return dnsConf + return dnsConf, nil } // newDNSCryptConfig converts values from the configuration file into the @@ -378,8 +392,7 @@ type dnsEncryption struct { // getDNSEncryption returns the TLS encryption addresses that AdGuard Home // listens on. tlsMgr must not be nil. func getDNSEncryption(tlsMgr *tlsManager) (de dnsEncryption) { - tlsConf := tlsConfigSettings{} - tlsMgr.WriteDiskConfig(&tlsConf) + tlsConf := tlsMgr.config() if !tlsConf.Enabled || len(tlsConf.ServerName) == 0 { return dnsEncryption{} diff --git a/internal/home/home.go b/internal/home/home.go index 0eef1826..dc5055d5 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -991,9 +991,9 @@ func printWebAddrs(proto, addr string, port uint16) { // // TODO(s.chzhen): Implement separate functions for HTTP and HTTPS. func printHTTPAddresses(proto string, tlsMgr *tlsManager) { - tlsConf := tlsConfigSettings{} + var tlsConf *tlsConfigSettings if tlsMgr != nil { - tlsMgr.WriteDiskConfig(&tlsConf) + tlsConf = tlsMgr.config() } port := config.HTTPConfig.Address.Port() diff --git a/internal/home/tls.go b/internal/home/tls.go index e012a309..cb52403a 100644 --- a/internal/home/tls.go +++ b/internal/home/tls.go @@ -24,11 +24,9 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/aghhttp" "github.com/AdguardTeam/AdGuardHome/internal/aghnet" "github.com/AdguardTeam/AdGuardHome/internal/aghtls" - "github.com/AdguardTeam/AdGuardHome/internal/dnsforward" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/logutil/slogutil" "github.com/c2h5oh/datasize" - "github.com/google/go-cmp/cmp" ) // tlsManager contains the current configuration and state of AdGuard Home TLS @@ -37,6 +35,9 @@ type tlsManager struct { // logger is used for logging the operation of the TLS Manager. logger *slog.Logger + // mu protects status, certLastMod, conf, and servePlainDNS. + mu *sync.Mutex + // status is the current status of the configuration. It is never nil. status *tlsConfigStatus @@ -52,6 +53,9 @@ type tlsManager struct { // Resolve it. web *webAPI + // conf contains the TLS configuration settings. It must not be nil. + conf *tlsConfigSettings + // configModified is called when the TLS configuration is changed via an // HTTP request. configModified func() @@ -59,9 +63,6 @@ type tlsManager struct { // customCipherIDs are the ID of the cipher suites that AdGuard Home must use. customCipherIDs []uint16 - confLock sync.Mutex - conf tlsConfigSettings - // servePlainDNS defines if plain DNS is allowed for incoming requests. servePlainDNS bool } @@ -91,9 +92,10 @@ type tlsManagerConfig struct { func newTLSManager(ctx context.Context, conf *tlsManagerConfig) (m *tlsManager, err error) { m = &tlsManager{ logger: conf.logger, + mu: &sync.Mutex{}, configModified: conf.configModified, status: &tlsConfigStatus{}, - conf: conf.tlsSettings, + conf: &conf.tlsSettings, servePlainDNS: conf.servePlainDNS, } @@ -112,17 +114,22 @@ func newTLSManager(ctx context.Context, conf *tlsManagerConfig) (m *tlsManager, m.logger.InfoContext(ctx, "using default ciphers") } - if m.conf.Enabled { - err = m.load(ctx) - if err != nil { - m.conf.Enabled = false + m.mu.Lock() + defer m.mu.Unlock() - return m, err - } - - m.setCertFileTime(ctx) + if !m.conf.Enabled { + return m, nil } + err = m.load(ctx) + if err != nil { + m.conf.Enabled = false + + return m, err + } + + m.setCertFileTime(ctx) + return m, nil } @@ -136,8 +143,9 @@ func (m *tlsManager) setWebAPI(webAPI *webAPI) { } // load reloads the TLS configuration from files or data from the config file. +// m.mu is expected to be locked. func (m *tlsManager) load(ctx context.Context) (err error) { - err = m.loadTLSConf(ctx, &m.conf, m.status) + err = m.loadTLSConfig(ctx, m.conf, m.status) if err != nil { return fmt.Errorf("loading config: %w", err) } @@ -145,15 +153,16 @@ func (m *tlsManager) load(ctx context.Context) (err error) { return nil } -// WriteDiskConfig - write config -func (m *tlsManager) WriteDiskConfig(conf *tlsConfigSettings) { - m.confLock.Lock() - *conf = m.conf - m.confLock.Unlock() +// config returns a deep copy of the stored TLS configuration. +func (m *tlsManager) config() (conf *tlsConfigSettings) { + m.mu.Lock() + defer m.mu.Unlock() + + return m.conf.clone() } // setCertFileTime sets [tlsManager.certLastMod] from the certificate. If there -// are errors, setCertFileTime logs them. +// are errors, setCertFileTime logs them. m.mu is expected to be locked. func (m *tlsManager) setCertFileTime(ctx context.Context) { if len(m.conf.CertificatePath) == 0 { return @@ -175,21 +184,21 @@ func (m *tlsManager) setCertFileTime(ctx context.Context) { func (m *tlsManager) start(_ context.Context) { m.registerWebHandlers() - m.confLock.Lock() - tlsConf := m.conf - m.confLock.Unlock() + m.mu.Lock() + defer m.mu.Unlock() // The background context is used because the TLSConfigChanged wraps context // with timeout on its own and shuts down the server, which handles current // request. - m.web.tlsConfigChanged(context.Background(), tlsConf) + m.web.tlsConfigChanged(context.Background(), m.conf) } // reload updates the configuration and restarts the TLS manager. func (m *tlsManager) reload(ctx context.Context) { - m.confLock.Lock() + m.mu.Lock() + defer m.mu.Unlock() + tlsConf := m.conf - m.confLock.Unlock() if !tlsConf.Enabled || len(tlsConf.CertificatePath) == 0 { return @@ -211,9 +220,7 @@ func (m *tlsManager) reload(ctx context.Context) { m.logger.InfoContext(ctx, "certificate file is modified") - m.confLock.Lock() err = m.load(ctx) - m.confLock.Unlock() if err != nil { m.logger.ErrorContext(ctx, "reloading", slogutil.KeyError, err) @@ -227,10 +234,6 @@ func (m *tlsManager) reload(ctx context.Context) { m.logger.ErrorContext(ctx, "reconfiguring dns server", slogutil.KeyError, err) } - m.confLock.Lock() - tlsConf = m.conf - m.confLock.Unlock() - // The background context is used because the TLSConfigChanged wraps context // with timeout on its own and shuts down the server, which handles current // request. @@ -238,15 +241,12 @@ func (m *tlsManager) reload(ctx context.Context) { } // reconfigureDNSServer updates the DNS server configuration using the stored -// TLS settings. +// TLS settings. m.mu is expected to be locked. func (m *tlsManager) reconfigureDNSServer() (err error) { - tlsConf := &tlsConfigSettings{} - m.WriteDiskConfig(tlsConf) - newConf, err := newServerConfig( &config.DNS, config.Clients.Sources, - tlsConf, + m.conf, m, httpRegister, globalContext.clients.storage, @@ -263,9 +263,11 @@ func (m *tlsManager) reconfigureDNSServer() (err error) { return nil } -// loadTLSConf loads and validates the TLS configuration. The returned error is -// also set in status.WarningValidation. -func (m *tlsManager) loadTLSConf( +// loadTLSConfig loads and validates the TLS configuration. It also sets +// [tlsConfigSettings.CertificateChainData] and +// [tlsConfigSettings.PrivateKeyData] properties. The returned error is also +// set in status.WarningValidation. +func (m *tlsManager) loadTLSConfig( ctx context.Context, tlsConf *tlsConfigSettings, status *tlsConfigStatus, @@ -357,10 +359,10 @@ type tlsConfigStatus struct { KeyType string `json:"key_type,omitempty"` // NotBefore is the NotBefore field of the first certificate in the chain. - NotBefore time.Time `json:"not_before,omitempty"` + NotBefore time.Time `json:"not_before"` // NotAfter is the NotAfter field of the first certificate in the chain. - NotAfter time.Time `json:"not_after,omitempty"` + NotAfter time.Time `json:"not_after"` // WarningValidation is a validation warning message with the issue // description. @@ -410,15 +412,23 @@ type tlsConfigSettingsExt struct { // handleTLSStatus is the handler for the GET /control/tls/status HTTP API. func (m *tlsManager) handleTLSStatus(w http.ResponseWriter, r *http.Request) { - m.confLock.Lock() + var tlsConf *tlsConfigSettings + var servePlainDNS bool + func() { + m.mu.Lock() + defer m.mu.Unlock() + + tlsConf = m.conf.clone() + servePlainDNS = m.servePlainDNS + }() + data := tlsConfig{ tlsConfigSettingsExt: tlsConfigSettingsExt{ - tlsConfigSettings: m.conf, - ServePlainDNS: aghalg.BoolToNullBool(m.servePlainDNS), + tlsConfigSettings: *tlsConf, + ServePlainDNS: aghalg.BoolToNullBool(servePlainDNS), }, tlsConfigStatus: m.status, } - m.confLock.Unlock() marshalTLS(w, r, data) } @@ -434,6 +444,9 @@ func (m *tlsManager) handleTLSValidate(w http.ResponseWriter, r *http.Request) { return } + m.mu.Lock() + defer m.mu.Unlock() + if setts.PrivateKeySaved { setts.PrivateKey = m.conf.PrivateKey } @@ -449,7 +462,7 @@ func (m *tlsManager) handleTLSValidate(w http.ResponseWriter, r *http.Request) { // Skip the error check, since we are only interested in the value of // status.WarningValidation. status := &tlsConfigStatus{} - _ = m.loadTLSConf(ctx, &setts.tlsConfigSettings, status) + _ = m.loadTLSConfig(ctx, &setts.tlsConfigSettings, status) resp := tlsConfig{ tlsConfigSettingsExt: setts, tlsConfigStatus: status, @@ -458,42 +471,23 @@ func (m *tlsManager) handleTLSValidate(w http.ResponseWriter, r *http.Request) { marshalTLS(w, r, resp) } -// setConfig updates manager conf with the given one. +// setConfig updates manager TLS configuration with the given one. m.mu is +// expected to be locked. func (m *tlsManager) setConfig( ctx context.Context, newConf tlsConfigSettings, status *tlsConfigStatus, servePlain aghalg.NullBool, ) (restartHTTPS bool) { - m.confLock.Lock() - defer m.confLock.Unlock() - - // Reset the DNSCrypt data before comparing, since we currently do not - // accept these from the frontend. - // - // TODO(a.garipov): Define a custom comparer for dnsforward.TLSConfig. - newConf.DNSCryptConfigFile = m.conf.DNSCryptConfigFile - newConf.PortDNSCrypt = m.conf.PortDNSCrypt - if !cmp.Equal(m.conf, newConf, cmp.AllowUnexported(dnsforward.TLSConfig{})) { + if !m.conf.setPrivateFieldsAndCompare(&newConf) { m.logger.InfoContext(ctx, "config has changed, restarting https server") restartHTTPS = true } else { m.logger.InfoContext(ctx, "config has not changed") } - // Note: don't do just `t.conf = data` because we must preserve all other members of t.conf - m.conf.Enabled = newConf.Enabled - m.conf.ServerName = newConf.ServerName - m.conf.ForceHTTPS = newConf.ForceHTTPS - m.conf.PortHTTPS = newConf.PortHTTPS - m.conf.PortDNSOverTLS = newConf.PortDNSOverTLS - m.conf.PortDNSOverQUIC = newConf.PortDNSOverQUIC - m.conf.CertificateChain = newConf.CertificateChain - m.conf.CertificatePath = newConf.CertificatePath - m.conf.CertificateChainData = newConf.CertificateChainData - m.conf.PrivateKey = newConf.PrivateKey - m.conf.PrivateKeyPath = newConf.PrivateKeyPath - m.conf.PrivateKeyData = newConf.PrivateKeyData + m.conf = &newConf + m.status = status if servePlain != aghalg.NBNull { @@ -515,6 +509,16 @@ func (m *tlsManager) handleTLSConfigure(w http.ResponseWriter, r *http.Request) return } + var restartHTTPS bool + defer func() { + if restartHTTPS { + m.configModified() + } + }() + + m.mu.Lock() + defer m.mu.Unlock() + if req.PrivateKeySaved { req.PrivateKey = m.conf.PrivateKey } @@ -526,7 +530,7 @@ func (m *tlsManager) handleTLSConfigure(w http.ResponseWriter, r *http.Request) } status := &tlsConfigStatus{} - err = m.loadTLSConf(ctx, &req.tlsConfigSettings, status) + err = m.loadTLSConfig(ctx, &req.tlsConfigSettings, status) if err != nil { resp := tlsConfig{ tlsConfigSettingsExt: req, @@ -538,20 +542,18 @@ func (m *tlsManager) handleTLSConfigure(w http.ResponseWriter, r *http.Request) return } - restartHTTPS := m.setConfig(ctx, req.tlsConfigSettings, status, req.ServePlainDNS) + restartHTTPS = m.setConfig(ctx, req.tlsConfigSettings, status, req.ServePlainDNS) m.setCertFileTime(ctx) if req.ServePlainDNS != aghalg.NBNull { func() { - m.confLock.Lock() - defer m.confLock.Unlock() + config.Lock() + defer config.Unlock() config.DNS.ServePlainDNS = req.ServePlainDNS == aghalg.NBTrue }() } - m.configModified() - err = m.reconfigureDNSServer() if err != nil { m.logger.ErrorContext(ctx, "reconfiguring dns server", slogutil.KeyError, err) @@ -567,18 +569,18 @@ func (m *tlsManager) handleTLSConfigure(w http.ResponseWriter, r *http.Request) } marshalTLS(w, r, resp) - if f, ok := w.(http.Flusher); ok { - f.Flush() + rc := http.NewResponseController(w) + err = rc.Flush() + if err != nil { + m.logger.ErrorContext(ctx, "flushing response", slogutil.KeyError, err) } // The background context is used because the TLSConfigChanged wraps context // with timeout on its own and shuts down the server, which handles current - // request. It is also should be done in a separate goroutine due to the + // request. It is also should be done in a separate goroutine due to the // same reason. if restartHTTPS { - go func() { - m.web.tlsConfigChanged(context.Background(), req.tlsConfigSettings) - }() + go m.web.tlsConfigChanged(context.Background(), &req.tlsConfigSettings) } } diff --git a/internal/home/tls_internal_test.go b/internal/home/tls_internal_test.go index e7e539d6..fdf327bc 100644 --- a/internal/home/tls_internal_test.go +++ b/internal/home/tls_internal_test.go @@ -239,11 +239,9 @@ func TestTLSManager_Reload(t *testing.T) { logger: logger, configModified: func() {}, tlsSettings: tlsConfigSettings{ - Enabled: true, - TLSConfig: dnsforward.TLSConfig{ - CertificatePath: certPath, - PrivateKeyPath: keyPath, - }, + Enabled: true, + CertificatePath: certPath, + PrivateKeyPath: keyPath, }, servePlainDNS: false, }) @@ -254,8 +252,7 @@ func TestTLSManager_Reload(t *testing.T) { m.setWebAPI(web) - conf := &tlsConfigSettings{} - m.WriteDiskConfig(conf) + conf := m.config() assertCertSerialNumber(t, conf, snBefore) certDER, key = newCertAndKey(t, snAfter) @@ -263,7 +260,7 @@ func TestTLSManager_Reload(t *testing.T) { m.reload(ctx) - m.WriteDiskConfig(conf) + conf = m.config() assertCertSerialNumber(t, conf, snAfter) } @@ -278,11 +275,9 @@ func TestTLSManager_HandleTLSStatus(t *testing.T) { logger: logger, configModified: func() {}, tlsSettings: tlsConfigSettings{ - Enabled: true, - TLSConfig: dnsforward.TLSConfig{ - CertificateChain: string(testCertChainData), - PrivateKey: string(testPrivateKeyData), - }, + Enabled: true, + CertificateChain: string(testCertChainData), + PrivateKey: string(testPrivateKeyData), }, servePlainDNS: false, }) @@ -342,47 +337,49 @@ func TestValidateTLSSettings(t *testing.T) { busyUDPPort := udpAddr.Port testCases := []struct { - setts tlsConfigSettingsExt name string wantErr string + setts tlsConfigSettingsExt }{{ name: "basic", - setts: tlsConfigSettingsExt{}, wantErr: "", + setts: tlsConfigSettingsExt{}, }, { + name: "disabled_all", + wantErr: "plain DNS is required in case encryption protocols are disabled", setts: tlsConfigSettingsExt{ ServePlainDNS: aghalg.NBFalse, }, - name: "disabled_all", - wantErr: "plain DNS is required in case encryption protocols are disabled", }, { + name: "busy_https_port", + wantErr: fmt.Sprintf("port %d for HTTPS is not available", busyTCPPort), setts: tlsConfigSettingsExt{ tlsConfigSettings: tlsConfigSettings{ Enabled: true, PortHTTPS: uint16(busyTCPPort), }, }, - name: "busy_https_port", - wantErr: fmt.Sprintf("port %d for HTTPS is not available", busyTCPPort), }, { + name: "busy_dot_port", + wantErr: fmt.Sprintf("port %d for DNS-over-TLS is not available", busyTCPPort), setts: tlsConfigSettingsExt{ tlsConfigSettings: tlsConfigSettings{ Enabled: true, PortDNSOverTLS: uint16(busyTCPPort), }, }, - name: "busy_dot_port", - wantErr: fmt.Sprintf("port %d for DNS-over-TLS is not available", busyTCPPort), }, { + name: "busy_doq_port", + wantErr: fmt.Sprintf("port %d for DNS-over-QUIC is not available", busyUDPPort), setts: tlsConfigSettingsExt{ tlsConfigSettings: tlsConfigSettings{ Enabled: true, PortDNSOverQUIC: uint16(busyUDPPort), }, }, - name: "busy_doq_port", - wantErr: fmt.Sprintf("port %d for DNS-over-QUIC is not available", busyUDPPort), }, { + name: "duplicate_port", + wantErr: "validating tcp ports: duplicated values: [4433]", setts: tlsConfigSettingsExt{ tlsConfigSettings: tlsConfigSettings{ Enabled: true, @@ -390,8 +387,6 @@ func TestValidateTLSSettings(t *testing.T) { PortDNSOverTLS: 4433, }, }, - name: "duplicate_port", - wantErr: "validating tcp ports: duplicated values: [4433]", }} for _, tc := range testCases { @@ -417,11 +412,9 @@ func TestTLSManager_HandleTLSValidate(t *testing.T) { logger: logger, configModified: func() {}, tlsSettings: tlsConfigSettings{ - Enabled: true, - TLSConfig: dnsforward.TLSConfig{ - CertificateChain: string(testCertChainData), - PrivateKey: string(testPrivateKeyData), - }, + Enabled: true, + CertificateChain: string(testCertChainData), + PrivateKey: string(testPrivateKeyData), }, servePlainDNS: false, }) @@ -434,11 +427,9 @@ func TestTLSManager_HandleTLSValidate(t *testing.T) { setts := &tlsConfigSettingsExt{ tlsConfigSettings: tlsConfigSettings{ - Enabled: true, - TLSConfig: dnsforward.TLSConfig{ - CertificateChain: base64.StdEncoding.EncodeToString(testCertChainData), - PrivateKey: base64.StdEncoding.EncodeToString(testPrivateKeyData), - }, + Enabled: true, + CertificateChain: base64.StdEncoding.EncodeToString(testCertChainData), + PrivateKey: base64.StdEncoding.EncodeToString(testPrivateKeyData), }, } @@ -476,6 +467,7 @@ func TestTLSManager_HandleTLSConfigure(t *testing.T) { require.NoError(t, err) err = globalContext.dnsServer.Prepare(&dnsforward.ServerConfig{ + TLSConf: &dnsforward.TLSConfig{}, Config: dnsforward.Config{ UpstreamMode: dnsforward.UpstreamModeLoadBalance, EDNSClientSubnet: &dnsforward.EDNSClientSubnet{Enabled: false}, @@ -511,11 +503,9 @@ func TestTLSManager_HandleTLSConfigure(t *testing.T) { logger: logger, configModified: func() {}, tlsSettings: tlsConfigSettings{ - Enabled: true, - TLSConfig: dnsforward.TLSConfig{ - CertificatePath: certPath, - PrivateKeyPath: keyPath, - }, + Enabled: true, + CertificatePath: certPath, + PrivateKeyPath: keyPath, }, servePlainDNS: true, }) @@ -526,19 +516,16 @@ func TestTLSManager_HandleTLSConfigure(t *testing.T) { m.setWebAPI(web) - conf := &tlsConfigSettings{} - m.WriteDiskConfig(conf) + conf := m.config() assertCertSerialNumber(t, conf, wantSerialNumber) // Prepare a request with the new TLS configuration. setts := &tlsConfigSettingsExt{ tlsConfigSettings: tlsConfigSettings{ - Enabled: true, - PortHTTPS: 4433, - TLSConfig: dnsforward.TLSConfig{ - CertificateChain: base64.StdEncoding.EncodeToString(testCertChainData), - PrivateKey: base64.StdEncoding.EncodeToString(testPrivateKeyData), - }, + Enabled: true, + PortHTTPS: 4433, + CertificateChain: base64.StdEncoding.EncodeToString(testCertChainData), + PrivateKey: base64.StdEncoding.EncodeToString(testPrivateKeyData), }, } diff --git a/internal/home/web.go b/internal/home/web.go index 9be52850..9de27782 100644 --- a/internal/home/web.go +++ b/internal/home/web.go @@ -157,8 +157,8 @@ func newWebAPI(ctx context.Context, conf *webConfig) (w *webAPI) { } // tlsConfigChanged updates the TLS configuration and restarts the HTTPS server -// if necessary. -func (web *webAPI) tlsConfigChanged(ctx context.Context, tlsConf tlsConfigSettings) { +// if necessary. tlsConf must not be nil. +func (web *webAPI) tlsConfigChanged(ctx context.Context, tlsConf *tlsConfigSettings) { defer slogutil.RecoverAndExit(ctx, web.logger, osutil.ExitCodeFailure) web.logger.DebugContext(ctx, "applying new tls configuration")