Pull request: 4925-refactor-tls-vol-2

Updates #4925.

Squashed commit of the following:

commit 4b221936ea6c2a244c404e95fa2a033571e07168
Author: Ainar Garipov <A.Garipov@AdGuard.COM>
Date:   Fri Oct 14 19:03:42 2022 +0300

    all: refactor tls

internal/home/controlinstall.go

@@ -424,7 +424,7 @@ func (web *Web) handleInstallConfigure(w http.ResponseWriter, r *http.Request) {
 	// moment we'll allow setting up TLS in the initial configuration or the
 	// configuration itself will use HTTPS protocol, because the underlying
 	// functions potentially restart the HTTPS server.
-	err = StartMods()
+	err = startMods()
 	if err != nil {
 		Context.firstRun = true
 		copyInstallSettings(config, curConfig)

internal/home/home.go

@@ -59,7 +59,7 @@ type homeContext struct {
 	auth       *Auth                // HTTP authentication module
 	filters    *filtering.DNSFilter // DNS filtering module
 	web        *Web                 // Web (HTTP, HTTPS) module
-	tls        *TLSMod              // TLS module
+	tls        *tlsManager          // TLS module
 	// etcHosts is an IP-hostname pairs set taken from system configuration
 	// (e.g. /etc/hosts) files.
 	etcHosts *aghnet.HostsContainer
@@ -117,7 +117,7 @@ func Main(clientBuildFS fs.FS) {
 			switch sig {
 			case syscall.SIGHUP:
 				Context.clients.Reload()
-				Context.tls.Reload()
+				Context.tls.reload()
 
 			default:
 				cleanup(context.Background())
@@ -495,9 +495,9 @@ func run(opts options, clientBuildFS fs.FS) {
 	}
 	config.Users = nil
 
-	Context.tls = tlsCreate(config.TLS)
-	if Context.tls == nil {
-		log.Fatalf("Can't initialize TLS module")
+	Context.tls, err = newTLSManager(config.TLS)
+	if err != nil {
+		log.Fatalf("initializing tls: %s", err)
 	}
 
 	Context.web, err = initWeb(opts, clientBuildFS)
@@ -507,7 +507,7 @@ func run(opts options, clientBuildFS fs.FS) {
 		err = initDNSServer()
 		fatalOnError(err)
 
-		Context.tls.Start()
+		Context.tls.start()
 
 		go func() {
 			serr := startDNSServer()
@@ -531,20 +531,22 @@ func run(opts options, clientBuildFS fs.FS) {
 	select {}
 }
 
-// StartMods initializes and starts the DNS server after installation.
-func StartMods() error {
+// startMods initializes and starts the DNS server after installation.
+func startMods() error {
 	err := initDNSServer()
 	if err != nil {
 		return err
 	}
 
-	Context.tls.Start()
+	Context.tls.start()
 
 	err = startDNSServer()
 	if err != nil {
 		closeDNSServer()
+
 		return err
 	}
+
 	return nil
 }
 
@@ -728,7 +730,6 @@ func cleanup(ctx context.Context) {
 	}
 
 	if Context.tls != nil {
-		Context.tls.Close()
 		Context.tls = nil
 	}
 }
@@ -738,7 +739,8 @@ func cleanupAlways() {
 	if len(Context.pidFileName) != 0 {
 		_ = os.Remove(Context.pidFileName)
 	}
-	log.Info("Stopped")
+
+	log.Info("stopped")
 }
 
 func exitWithError() {

internal/home/mobileconfig_test.go

@@ -32,7 +32,7 @@ func setupDNSIPs(t testing.TB) {
 		},
 	}
 
-	Context.tls = &TLSMod{}
+	Context.tls = &tlsManager{}
 }
 
 func TestHandleMobileConfigDoH(t *testing.T) {
@@ -65,7 +65,7 @@ func TestHandleMobileConfigDoH(t *testing.T) {
 		oldTLSConf := Context.tls
 		t.Cleanup(func() { Context.tls = oldTLSConf })
 
-		Context.tls = &TLSMod{conf: tlsConfigSettings{}}
+		Context.tls = &tlsManager{conf: tlsConfigSettings{}}
 
 		r, err := http.NewRequest(http.MethodGet, "https://example.com:12345/apple/doh.mobileconfig", nil)
 		require.NoError(t, err)
@@ -137,7 +137,7 @@ func TestHandleMobileConfigDoT(t *testing.T) {
 		oldTLSConf := Context.tls
 		t.Cleanup(func() { Context.tls = oldTLSConf })
 
-		Context.tls = &TLSMod{conf: tlsConfigSettings{}}
+		Context.tls = &tlsManager{conf: tlsConfigSettings{}}
 
 		r, err := http.NewRequest(http.MethodGet, "https://example.com:12345/apple/dot.mobileconfig", nil)
 		require.NoError(t, err)

internal/home/tls.go

@@ -26,216 +26,256 @@ import (
 	"github.com/google/go-cmp/cmp"
 )
 
-var tlsWebHandlersRegistered = false
+// tlsManager contains the current configuration and state of AdGuard Home TLS
+// encryption.
+type tlsManager struct {
+	// status is the current status of the configuration.  It is never nil.
+	status *tlsConfigStatus
 
-// TLSMod - TLS module object
-type TLSMod struct {
-	certLastMod time.Time // last modification time of the certificate file
-	status      tlsConfigStatus
-	confLock    sync.Mutex
-	conf        tlsConfigSettings
+	// certLastMod is the last modification time of the certificate file.
+	certLastMod time.Time
+
+	confLock sync.Mutex
+	conf     tlsConfigSettings
 }
 
-// Create TLS module
-func tlsCreate(conf tlsConfigSettings) *TLSMod {
-	t := &TLSMod{}
-	t.conf = conf
-	if t.conf.Enabled {
-		if !t.load() {
-			// Something is not valid - return an empty TLS config
-			return &TLSMod{conf: tlsConfigSettings{
-				Enabled:             conf.Enabled,
-				ServerName:          conf.ServerName,
-				PortHTTPS:           conf.PortHTTPS,
-				PortDNSOverTLS:      conf.PortDNSOverTLS,
-				PortDNSOverQUIC:     conf.PortDNSOverQUIC,
-				AllowUnencryptedDoH: conf.AllowUnencryptedDoH,
-			}}
+// newTLSManager initializes the TLS configuration.
+func newTLSManager(conf tlsConfigSettings) (m *tlsManager, err error) {
+	m = &tlsManager{
+		status: &tlsConfigStatus{},
+		conf:   conf,
+	}
+
+	if m.conf.Enabled {
+		err = m.load()
+		if err != nil {
+			return nil, err
 		}
-		t.setCertFileTime()
+
+		m.setCertFileTime()
 	}
-	return t
+
+	return m, nil
 }
 
-func (t *TLSMod) load() bool {
-	if !tlsLoadConfig(&t.conf, &t.status) {
-		log.Error("failed to load TLS config: %s", t.status.WarningValidation)
-		return false
+// load reloads the TLS configuration from files or data from the config file.
+func (m *tlsManager) load() (err error) {
+	err = loadTLSConf(&m.conf, m.status)
+	if err != nil {
+		return fmt.Errorf("loading config: %w", err)
 	}
 
-	// validate current TLS config and update warnings (it could have been loaded from file)
-	data := validateCertificates(string(t.conf.CertificateChainData), string(t.conf.PrivateKeyData), t.conf.ServerName)
-	if !data.ValidPair {
-		log.Error("failed to validate certificate: %s", data.WarningValidation)
-		return false
-	}
-	t.status = data
-	return true
-}
-
-// Close - close module
-func (t *TLSMod) Close() {
+	return nil
 }
 
 // WriteDiskConfig - write config
-func (t *TLSMod) WriteDiskConfig(conf *tlsConfigSettings) {
-	t.confLock.Lock()
-	*conf = t.conf
-	t.confLock.Unlock()
+func (m *tlsManager) WriteDiskConfig(conf *tlsConfigSettings) {
+	m.confLock.Lock()
+	*conf = m.conf
+	m.confLock.Unlock()
 }
 
-func (t *TLSMod) setCertFileTime() {
-	if len(t.conf.CertificatePath) == 0 {
+// setCertFileTime sets t.certLastMod from the certificate.  If there are
+// errors, setCertFileTime logs them.
+func (m *tlsManager) setCertFileTime() {
+	if len(m.conf.CertificatePath) == 0 {
 		return
 	}
-	fi, err := os.Stat(t.conf.CertificatePath)
+
+	fi, err := os.Stat(m.conf.CertificatePath)
 	if err != nil {
-		log.Error("TLS: %s", err)
+		log.Error("tls: looking up certificate path: %s", err)
+
 		return
 	}
-	t.certLastMod = fi.ModTime().UTC()
+
+	m.certLastMod = fi.ModTime().UTC()
 }
 
-// Start updates the configuration of TLSMod and starts it.
-func (t *TLSMod) Start() {
-	if !tlsWebHandlersRegistered {
-		tlsWebHandlersRegistered = true
-		t.registerWebHandlers()
-	}
+// start updates the configuration of t and starts it.
+func (m *tlsManager) start() {
+	m.registerWebHandlers()
 
-	t.confLock.Lock()
-	tlsConf := t.conf
-	t.confLock.Unlock()
+	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.
+	// The background context is used because the TLSConfigChanged wraps context
+	// with timeout on its own and shuts down the server, which handles current
+	// request.
 	Context.web.TLSConfigChanged(context.Background(), tlsConf)
 }
 
-// Reload updates the configuration of TLSMod and restarts it.
-func (t *TLSMod) Reload() {
-	t.confLock.Lock()
-	tlsConf := t.conf
-	t.confLock.Unlock()
+// reload updates the configuration and restarts t.
+func (m *tlsManager) reload() {
+	m.confLock.Lock()
+	tlsConf := m.conf
+	m.confLock.Unlock()
 
 	if !tlsConf.Enabled || len(tlsConf.CertificatePath) == 0 {
 		return
 	}
+
 	fi, err := os.Stat(tlsConf.CertificatePath)
 	if err != nil {
-		log.Error("TLS: %s", err)
-		return
-	}
-	if fi.ModTime().UTC().Equal(t.certLastMod) {
-		log.Debug("TLS: certificate file isn't modified")
-		return
-	}
-	log.Debug("TLS: certificate file is modified")
+		log.Error("tls: %s", err)
 
-	t.confLock.Lock()
-	r := t.load()
-	t.confLock.Unlock()
-	if !r {
 		return
 	}
 
-	t.certLastMod = fi.ModTime().UTC()
+	if fi.ModTime().UTC().Equal(m.certLastMod) {
+		log.Debug("tls: certificate file isn't modified")
+
+		return
+	}
+
+	log.Debug("tls: certificate file is modified")
+
+	m.confLock.Lock()
+	err = m.load()
+	m.confLock.Unlock()
+	if err != nil {
+		log.Error("tls: reloading: %s", err)
+
+		return
+	}
+
+	m.certLastMod = fi.ModTime().UTC()
 
 	_ = reconfigureDNSServer()
 
-	t.confLock.Lock()
-	tlsConf = t.conf
-	t.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.
+	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.
 	Context.web.TLSConfigChanged(context.Background(), tlsConf)
 }
 
-// Set certificate and private key data
-func tlsLoadConfig(tls *tlsConfigSettings, status *tlsConfigStatus) bool {
-	tls.CertificateChainData = []byte(tls.CertificateChain)
-	tls.PrivateKeyData = []byte(tls.PrivateKey)
-
-	var err error
-	if tls.CertificatePath != "" {
-		if tls.CertificateChain != "" {
-			status.WarningValidation = "certificate data and file can't be set together"
-			return false
-		}
-		tls.CertificateChainData, err = os.ReadFile(tls.CertificatePath)
+// loadTLSConf loads and validates the TLS configuration.  The returned error is
+// also set in status.WarningValidation.
+func loadTLSConf(tlsConf *tlsConfigSettings, status *tlsConfigStatus) (err error) {
+	defer func() {
 		if err != nil {
 			status.WarningValidation = err.Error()
-			return false
 		}
+	}()
+
+	tlsConf.CertificateChainData = []byte(tlsConf.CertificateChain)
+	tlsConf.PrivateKeyData = []byte(tlsConf.PrivateKey)
+
+	if tlsConf.CertificatePath != "" {
+		if tlsConf.CertificateChain != "" {
+			return errors.Error("certificate data and file can't be set together")
+		}
+
+		tlsConf.CertificateChainData, err = os.ReadFile(tlsConf.CertificatePath)
+		if err != nil {
+			return fmt.Errorf("reading cert file: %w", err)
+		}
+
 		status.ValidCert = true
 	}
 
-	if tls.PrivateKeyPath != "" {
-		if tls.PrivateKey != "" {
-			status.WarningValidation = "private key data and file can't be set together"
-			return false
+	if tlsConf.PrivateKeyPath != "" {
+		if tlsConf.PrivateKey != "" {
+			return errors.Error("private key data and file can't be set together")
 		}
-		tls.PrivateKeyData, err = os.ReadFile(tls.PrivateKeyPath)
+
+		tlsConf.PrivateKeyData, err = os.ReadFile(tlsConf.PrivateKeyPath)
 		if err != nil {
-			status.WarningValidation = err.Error()
-			return false
+			return fmt.Errorf("reading key file: %w", err)
 		}
+
 		status.ValidKey = true
 	}
 
-	return true
+	err = validateCertificates(
+		status,
+		tlsConf.CertificateChainData,
+		tlsConf.PrivateKeyData,
+		tlsConf.ServerName,
+	)
+	if err != nil {
+		return fmt.Errorf("validating certificate pair: %w", err)
+	}
+
+	return nil
 }
 
+// tlsConfigStatus contains the status of a certificate chain and key pair.
 type tlsConfigStatus struct {
-	ValidCert  bool      `json:"valid_cert"`           // ValidCert is true if the specified certificates chain is a valid chain of X509 certificates
-	ValidChain bool      `json:"valid_chain"`          // ValidChain is true if the specified certificates chain is verified and issued by a known CA
-	Subject    string    `json:"subject,omitempty"`    // Subject is the subject of the first certificate in the chain
-	Issuer     string    `json:"issuer,omitempty"`     // Issuer is the issuer of the first certificate in the chain
-	NotBefore  time.Time `json:"not_before,omitempty"` // NotBefore is the NotBefore field of the first certificate in the chain
-	NotAfter   time.Time `json:"not_after,omitempty"`  // NotAfter is the NotAfter field of the first certificate in the chain
-	DNSNames   []string  `json:"dns_names"`            // DNSNames is the value of SubjectAltNames field of the first certificate in the chain
+	// Subject is the subject of the first certificate in the chain.
+	Subject string `json:"subject,omitempty"`
 
-	// key status
-	ValidKey bool   `json:"valid_key"`          // ValidKey is true if the key is a valid private key
-	KeyType  string `json:"key_type,omitempty"` // KeyType is one of RSA or ECDSA
+	// Issuer is the issuer of the first certificate in the chain.
+	Issuer string `json:"issuer,omitempty"`
 
-	// is usable? set by validator
-	ValidPair bool `json:"valid_pair"` // ValidPair is true if both certificate and private key are correct
+	// KeyType is the type of the private key.
+	KeyType string `json:"key_type,omitempty"`
 
-	// warnings
-	WarningValidation string `json:"warning_validation,omitempty"` // WarningValidation is a validation warning message with the issue description
+	// NotBefore is the NotBefore field of the first certificate in the chain.
+	NotBefore time.Time `json:"not_before,omitempty"`
+
+	// NotAfter is the NotAfter field of the first certificate in the chain.
+	NotAfter time.Time `json:"not_after,omitempty"`
+
+	// WarningValidation is a validation warning message with the issue
+	// description.
+	WarningValidation string `json:"warning_validation,omitempty"`
+
+	// DNSNames is the value of SubjectAltNames field of the first certificate
+	// in the chain.
+	DNSNames []string `json:"dns_names"`
+
+	// ValidCert is true if the specified certificate chain is a valid chain of
+	// X509 certificates.
+	ValidCert bool `json:"valid_cert"`
+
+	// ValidChain is true if the specified certificate chain is verified and
+	// issued by a known CA.
+	ValidChain bool `json:"valid_chain"`
+
+	// ValidKey is true if the key is a valid private key.
+	ValidKey bool `json:"valid_key"`
+
+	// ValidPair is true if both certificate and private key are correct for
+	// each other.
+	ValidPair bool `json:"valid_pair"`
 }
 
-// field ordering is important -- yaml fields will mirror ordering from here
+// tlsConfig is the TLS configuration and status response.
 type tlsConfig struct {
-	tlsConfigStatus      `json:",inline"`
+	*tlsConfigStatus     `json:",inline"`
 	tlsConfigSettingsExt `json:",inline"`
 }
 
-// tlsConfigSettingsExt is used to (un)marshal PrivateKeySaved to ensure that
-// clients don't send and receive previously saved private keys.
+// tlsConfigSettingsExt is used to (un)marshal the PrivateKeySaved field to
+// ensure that clients don't send and receive previously saved private keys.
 type tlsConfigSettingsExt struct {
 	tlsConfigSettings `json:",inline"`
-	// If private key saved as a string, we set this flag to true
-	// and omit key from answer.
+
+	// PrivateKeySaved is true if the private key is saved as a string and omit
+	// key from answer.
 	PrivateKeySaved bool `yaml:"-" json:"private_key_saved,inline"`
 }
 
-func (t *TLSMod) handleTLSStatus(w http.ResponseWriter, r *http.Request) {
-	t.confLock.Lock()
+func (m *tlsManager) handleTLSStatus(w http.ResponseWriter, r *http.Request) {
+	m.confLock.Lock()
 	data := tlsConfig{
 		tlsConfigSettingsExt: tlsConfigSettingsExt{
-			tlsConfigSettings: t.conf,
+			tlsConfigSettings: m.conf,
 		},
-		tlsConfigStatus: t.status,
+		tlsConfigStatus: m.status,
 	}
-	t.confLock.Unlock()
+	m.confLock.Unlock()
+
 	marshalTLS(w, r, data)
 }
 
-func (t *TLSMod) handleTLSValidate(w http.ResponseWriter, r *http.Request) {
+func (m *tlsManager) handleTLSValidate(w http.ResponseWriter, r *http.Request) {
 	setts, err := unmarshalTLS(r)
 	if err != nil {
 		aghhttp.Error(r, w, http.StatusBadRequest, "Failed to unmarshal TLS config: %s", err)
@@ -244,7 +284,7 @@ func (t *TLSMod) handleTLSValidate(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if setts.PrivateKeySaved {
-		setts.PrivateKey = t.conf.PrivateKey
+		setts.PrivateKey = m.conf.PrivateKey
 	}
 
 	if setts.Enabled {
@@ -276,75 +316,74 @@ func (t *TLSMod) handleTLSValidate(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	status := tlsConfigStatus{}
-	if tlsLoadConfig(&setts.tlsConfigSettings, &status) {
-		status = validateCertificates(string(setts.CertificateChainData), string(setts.PrivateKeyData), setts.ServerName)
-	}
-
-	data := tlsConfig{
+	// Skip the error check, since we are only interested in the value of
+	// status.WarningValidation.
+	status := &tlsConfigStatus{}
+	_ = loadTLSConf(&setts.tlsConfigSettings, status)
+	resp := tlsConfig{
 		tlsConfigSettingsExt: setts,
 		tlsConfigStatus:      status,
 	}
 
-	marshalTLS(w, r, data)
+	marshalTLS(w, r, resp)
 }
 
-func (t *TLSMod) setConfig(newConf tlsConfigSettings, status tlsConfigStatus) (restartHTTPS bool) {
-	t.confLock.Lock()
-	defer t.confLock.Unlock()
+func (m *tlsManager) setConfig(newConf tlsConfigSettings, status *tlsConfigStatus) (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 = t.conf.DNSCryptConfigFile
-	newConf.PortDNSCrypt = t.conf.PortDNSCrypt
-	if !cmp.Equal(t.conf, newConf, cmp.AllowUnexported(dnsforward.TLSConfig{})) {
+	newConf.DNSCryptConfigFile = m.conf.DNSCryptConfigFile
+	newConf.PortDNSCrypt = m.conf.PortDNSCrypt
+	if !cmp.Equal(m.conf, newConf, cmp.AllowUnexported(dnsforward.TLSConfig{})) {
 		log.Info("tls config has changed, restarting https server")
 		restartHTTPS = true
 	} else {
-		log.Info("tls config has not changed")
+		log.Info("tls: config has not changed")
 	}
 
 	// Note: don't do just `t.conf = data` because we must preserve all other members of t.conf
-	t.conf.Enabled = newConf.Enabled
-	t.conf.ServerName = newConf.ServerName
-	t.conf.ForceHTTPS = newConf.ForceHTTPS
-	t.conf.PortHTTPS = newConf.PortHTTPS
-	t.conf.PortDNSOverTLS = newConf.PortDNSOverTLS
-	t.conf.PortDNSOverQUIC = newConf.PortDNSOverQUIC
-	t.conf.CertificateChain = newConf.CertificateChain
-	t.conf.CertificatePath = newConf.CertificatePath
-	t.conf.CertificateChainData = newConf.CertificateChainData
-	t.conf.PrivateKey = newConf.PrivateKey
-	t.conf.PrivateKeyPath = newConf.PrivateKeyPath
-	t.conf.PrivateKeyData = newConf.PrivateKeyData
-	t.status = status
+	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.status = status
 
 	return restartHTTPS
 }
 
-func (t *TLSMod) handleTLSConfigure(w http.ResponseWriter, r *http.Request) {
-	data, err := unmarshalTLS(r)
+func (m *tlsManager) handleTLSConfigure(w http.ResponseWriter, r *http.Request) {
+	req, err := unmarshalTLS(r)
 	if err != nil {
 		aghhttp.Error(r, w, http.StatusBadRequest, "Failed to unmarshal TLS config: %s", err)
 
 		return
 	}
 
-	if data.PrivateKeySaved {
-		data.PrivateKey = t.conf.PrivateKey
+	if req.PrivateKeySaved {
+		req.PrivateKey = m.conf.PrivateKey
 	}
 
-	if data.Enabled {
+	if req.Enabled {
 		err = validatePorts(
 			tcpPort(config.BindPort),
 			tcpPort(config.BetaBindPort),
-			tcpPort(data.PortHTTPS),
-			tcpPort(data.PortDNSOverTLS),
-			tcpPort(data.PortDNSCrypt),
+			tcpPort(req.PortHTTPS),
+			tcpPort(req.PortDNSOverTLS),
+			tcpPort(req.PortDNSCrypt),
 			udpPort(config.DNS.Port),
-			udpPort(data.PortDNSOverQUIC),
+			udpPort(req.PortDNSOverQUIC),
 		)
 		if err != nil {
 			aghhttp.Error(r, w, http.StatusBadRequest, "%s", err)
@@ -354,33 +393,33 @@ func (t *TLSMod) handleTLSConfigure(w http.ResponseWriter, r *http.Request) {
 	}
 
 	// TODO(e.burkov):  Investigate and perhaps check other ports.
-	if !webCheckPortAvailable(data.PortHTTPS) {
+	if !webCheckPortAvailable(req.PortHTTPS) {
 		aghhttp.Error(
 			r,
 			w,
 			http.StatusBadRequest,
-			"port %d is not available, cannot enable HTTPS on it",
-			data.PortHTTPS,
+			"port %d is not available, cannot enable https on it",
+			req.PortHTTPS,
 		)
 
 		return
 	}
 
-	status := tlsConfigStatus{}
-	if !tlsLoadConfig(&data.tlsConfigSettings, &status) {
-		data2 := tlsConfig{
-			tlsConfigSettingsExt: data,
-			tlsConfigStatus:      t.status,
+	status := &tlsConfigStatus{}
+	err = loadTLSConf(&req.tlsConfigSettings, status)
+	if err != nil {
+		resp := tlsConfig{
+			tlsConfigSettingsExt: req,
+			tlsConfigStatus:      status,
 		}
-		marshalTLS(w, r, data2)
+
+		marshalTLS(w, r, resp)
 
 		return
 	}
 
-	status = validateCertificates(string(data.CertificateChainData), string(data.PrivateKeyData), data.ServerName)
-
-	restartHTTPS := t.setConfig(data.tlsConfigSettings, status)
-	t.setCertFileTime()
+	restartHTTPS := m.setConfig(req.tlsConfigSettings, status)
+	m.setCertFileTime()
 	onConfigModified()
 
 	err = reconfigureDNSServer()
@@ -390,12 +429,12 @@ func (t *TLSMod) handleTLSConfigure(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	data2 := tlsConfig{
-		tlsConfigSettingsExt: data,
-		tlsConfigStatus:      t.status,
+	resp := tlsConfig{
+		tlsConfigSettingsExt: req,
+		tlsConfigStatus:      m.status,
 	}
 
-	marshalTLS(w, r, data2)
+	marshalTLS(w, r, resp)
 	if f, ok := w.(http.Flusher); ok {
 		f.Flush()
 	}
@@ -406,7 +445,7 @@ func (t *TLSMod) handleTLSConfigure(w http.ResponseWriter, r *http.Request) {
 	// same reason.
 	if restartHTTPS {
 		go func() {
-			Context.web.TLSConfigChanged(context.Background(), data.tlsConfigSettings)
+			Context.web.TLSConfigChanged(context.Background(), req.tlsConfigSettings)
 		}()
 	}
 }
@@ -443,89 +482,105 @@ func validatePorts(
 	return nil
 }
 
-func verifyCertChain(data *tlsConfigStatus, certChain, serverName string) error {
-	log.Tracef("TLS: got certificate: %d bytes", len(certChain))
+// validateCertChain validates the certificate chain and sets data in status.
+// The returned error is also set in status.WarningValidation.
+func validateCertChain(status *tlsConfigStatus, certChain []byte, serverName string) (err error) {
+	defer func() {
+		if err != nil {
+			status.WarningValidation = err.Error()
+		}
+	}()
 
-	// now do a more extended validation
-	var certs []*pem.Block // PEM-encoded certificates
+	log.Debug("tls: got certificate chain: %d bytes", len(certChain))
 
-	pemblock := []byte(certChain)
+	var certs []*pem.Block
+	pemblock := certChain
 	for {
 		var decoded *pem.Block
 		decoded, pemblock = pem.Decode(pemblock)
 		if decoded == nil {
 			break
 		}
+
 		if decoded.Type == "CERTIFICATE" {
 			certs = append(certs, decoded)
 		}
 	}
 
-	var parsedCerts []*x509.Certificate
-
-	for _, cert := range certs {
-		parsed, err := x509.ParseCertificate(cert.Bytes)
-		if err != nil {
-			data.WarningValidation = fmt.Sprintf("Failed to parse certificate: %s", err)
-			return errors.Error(data.WarningValidation)
-		}
-		parsedCerts = append(parsedCerts, parsed)
+	parsedCerts, err := parsePEMCerts(certs)
+	if err != nil {
+		return err
 	}
 
-	if len(parsedCerts) == 0 {
-		data.WarningValidation = "You have specified an empty certificate"
-		return errors.Error(data.WarningValidation)
-	}
-
-	data.ValidCert = true
-
-	// spew.Dump(parsedCerts)
+	status.ValidCert = true
 
 	opts := x509.VerifyOptions{
 		DNSName: serverName,
 		Roots:   Context.tlsRoots,
 	}
 
-	log.Printf("number of certs - %d", len(parsedCerts))
-	if len(parsedCerts) > 1 {
-		// set up an intermediate
-		pool := x509.NewCertPool()
-		for _, cert := range parsedCerts[1:] {
-			log.Printf("got an intermediate cert")
-			pool.AddCert(cert)
-		}
-		opts.Intermediates = pool
+	log.Info("tls: number of certs: %d", len(parsedCerts))
+
+	pool := x509.NewCertPool()
+	for _, cert := range parsedCerts[1:] {
+		log.Info("tls: got an intermediate cert")
+		pool.AddCert(cert)
 	}
 
-	// TODO: save it as a warning rather than error it out -- shouldn't be a big problem
+	opts.Intermediates = pool
+
 	mainCert := parsedCerts[0]
-	_, err := mainCert.Verify(opts)
+	_, err = mainCert.Verify(opts)
 	if err != nil {
-		// let self-signed certs through
-		data.WarningValidation = fmt.Sprintf("Your certificate does not verify: %s", err)
+		// Let self-signed certs through and don't return this error.
+		status.WarningValidation = fmt.Sprintf("certificate does not verify: %s", err)
 	} else {
-		data.ValidChain = true
+		status.ValidChain = true
 	}
-	// spew.Dump(chains)
 
-	// update status
 	if mainCert != nil {
-		notAfter := mainCert.NotAfter
-		data.Subject = mainCert.Subject.String()
-		data.Issuer = mainCert.Issuer.String()
-		data.NotAfter = notAfter
-		data.NotBefore = mainCert.NotBefore
-		data.DNSNames = mainCert.DNSNames
+		status.Subject = mainCert.Subject.String()
+		status.Issuer = mainCert.Issuer.String()
+		status.NotAfter = mainCert.NotAfter
+		status.NotBefore = mainCert.NotBefore
+		status.DNSNames = mainCert.DNSNames
 	}
 
 	return nil
 }
 
-func validatePkey(data *tlsConfigStatus, pkey string) error {
-	// now do a more extended validation
-	var key *pem.Block // PEM-encoded certificates
+// parsePEMCerts parses multiple PEM-encoded certificates.
+func parsePEMCerts(certs []*pem.Block) (parsedCerts []*x509.Certificate, err error) {
+	for i, cert := range certs {
+		var parsed *x509.Certificate
+		parsed, err = x509.ParseCertificate(cert.Bytes)
+		if err != nil {
+			return nil, fmt.Errorf("parsing certificate at index %d: %w", i, err)
+		}
 
-	// go through all pem blocks, but take first valid pem block and drop the rest
+		parsedCerts = append(parsedCerts, parsed)
+	}
+
+	if len(parsedCerts) == 0 {
+		return nil, errors.Error("empty certificate")
+	}
+
+	return parsedCerts, nil
+}
+
+// validatePKey validates the private key and sets data in status.  The returned
+// error is also set in status.WarningValidation.
+func validatePKey(status *tlsConfigStatus, pkey []byte) (err error) {
+	defer func() {
+		if err != nil {
+			status.WarningValidation = err.Error()
+		}
+	}()
+
+	var key *pem.Block
+
+	// Go through all pem blocks, but take first valid pem block and drop the
+	// rest.
 	pemblock := []byte(pkey)
 	for {
 		var decoded *pem.Block
@@ -542,61 +597,77 @@ func validatePkey(data *tlsConfigStatus, pkey string) error {
 	}
 
 	if key == nil {
-		data.WarningValidation = "No valid keys were found"
-
-		return errors.Error(data.WarningValidation)
+		return errors.Error("no valid keys were found")
 	}
 
-	// parse the decoded key
 	_, keyType, err := parsePrivateKey(key.Bytes)
 	if err != nil {
-		data.WarningValidation = fmt.Sprintf("Failed to parse private key: %s", err)
-
-		return errors.Error(data.WarningValidation)
-	} else if keyType == keyTypeED25519 {
-		data.WarningValidation = "ED25519 keys are not supported by browsers; " +
-			"did you mean to use X25519 for key exchange?"
-
-		return errors.Error(data.WarningValidation)
+		return fmt.Errorf("parsing private key: %w", err)
 	}
 
-	data.ValidKey = true
-	data.KeyType = keyType
+	if keyType == keyTypeED25519 {
+		return errors.Error(
+			"ED25519 keys are not supported by browsers; " +
+				"did you mean to use X25519 for key exchange?",
+		)
+	}
+
+	status.ValidKey = true
+	status.KeyType = keyType
 
 	return nil
 }
 
 // validateCertificates processes certificate data and its private key.  All
-// parameters are optional.  On error, validateCertificates returns a partially
-// set object with field WarningValidation containing error description.
-func validateCertificates(certChain, pkey, serverName string) tlsConfigStatus {
-	var data tlsConfigStatus
-
-	// check only public certificate separately from the key
-	if certChain != "" {
-		if verifyCertChain(&data, certChain, serverName) != nil {
-			return data
+// parameters are optional.  status must not be nil.  The returned error is also
+// set in status.WarningValidation.
+func validateCertificates(
+	status *tlsConfigStatus,
+	certChain []byte,
+	pkey []byte,
+	serverName string,
+) (err error) {
+	defer func() {
+		// Capitalize the warning for the UI.  Assume that warnings are all
+		// ASCII-only.
+		//
+		// TODO(a.garipov): Figure out a better way to do this.  Perhaps a
+		// custom string or error type.
+		if w := status.WarningValidation; w != "" {
+			status.WarningValidation = strings.ToUpper(w[:1]) + w[1:]
 		}
-	}
+	}()
 
-	// validate private key (right now the only validation possible is just parsing it)
-	if pkey != "" {
-		if validatePkey(&data, pkey) != nil {
-			return data
-		}
-	}
-
-	// if both are set, validate both in unison
-	if pkey != "" && certChain != "" {
-		_, err := tls.X509KeyPair([]byte(certChain), []byte(pkey))
+	// Check only the public certificate separately from the key.
+	if len(certChain) > 0 {
+		err = validateCertChain(status, certChain, serverName)
 		if err != nil {
-			data.WarningValidation = fmt.Sprintf("Invalid certificate or key: %s", err)
-			return data
+			return err
 		}
-		data.ValidPair = true
 	}
 
-	return data
+	// Validate the private key by parsing it.
+	if len(pkey) > 0 {
+		err = validatePKey(status, pkey)
+		if err != nil {
+			return err
+		}
+	}
+
+	// If both are set, validate together.
+	if len(certChain) > 0 && len(pkey) > 0 {
+		_, err = tls.X509KeyPair(certChain, pkey)
+		if err != nil {
+			err = fmt.Errorf("certificate-key pair: %w", err)
+			status.WarningValidation = err.Error()
+
+			return err
+		}
+
+		status.ValidPair = true
+	}
+
+	return nil
 }
 
 // Key types.
@@ -691,9 +762,9 @@ func marshalTLS(w http.ResponseWriter, r *http.Request, data tlsConfig) {
 	_ = aghhttp.WriteJSONResponse(w, r, data)
 }
 
-// registerWebHandlers registers HTTP handlers for TLS configuration
-func (t *TLSMod) registerWebHandlers() {
-	httpRegister(http.MethodGet, "/control/tls/status", t.handleTLSStatus)
-	httpRegister(http.MethodPost, "/control/tls/configure", t.handleTLSConfigure)
-	httpRegister(http.MethodPost, "/control/tls/validate", t.handleTLSValidate)
+// registerWebHandlers registers HTTP handlers for TLS configuration.
+func (m *tlsManager) registerWebHandlers() {
+	httpRegister(http.MethodGet, "/control/tls/status", m.handleTLSStatus)
+	httpRegister(http.MethodPost, "/control/tls/configure", m.handleTLSConfigure)
+	httpRegister(http.MethodPost, "/control/tls/validate", m.handleTLSValidate)
 }

internal/home/tls_internal_test.go

@@ -7,8 +7,7 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-const (
-	CertificateChain = `-----BEGIN CERTIFICATE-----
+var testCertChainData = []byte(`-----BEGIN CERTIFICATE-----
 MIICKzCCAZSgAwIBAgIJAMT9kPVJdM7LMA0GCSqGSIb3DQEBCwUAMC0xFDASBgNV
 BAoMC0FkR3VhcmQgTHRkMRUwEwYDVQQDDAxBZEd1YXJkIEhvbWUwHhcNMTkwMjI3
 MDkyNDIzWhcNNDYwNzE0MDkyNDIzWjAtMRQwEgYDVQQKDAtBZEd1YXJkIEx0ZDEV
@@ -21,8 +20,9 @@ eKO029jYd2AAZEQwDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOBgQB8
 LwlXfbakf7qkVTlCNXgoY7RaJ8rJdPgOZPoCTVToEhT6u/cb1c2qp8QB0dNExDna
 b0Z+dnODTZqQOJo6z/wIXlcUrnR4cQVvytXt8lFn+26l6Y6EMI26twC/xWr+1swq
 Muj4FeWHVDerquH4yMr1jsYLD3ci+kc5sbIX6TfVxQ==
------END CERTIFICATE-----`
-	PrivateKey = `-----BEGIN PRIVATE KEY-----
+-----END CERTIFICATE-----`)
+
+var testPrivateKeyData = []byte(`-----BEGIN PRIVATE KEY-----
 MIICeAIBADANBgkqhkiG9w0BAQEFAASCAmIwggJeAgEAAoGBALC/BSc8mI68tw5p
 aYa7pjrySwWvXeetcFywOWHGVfLw9qiFWLdfESa3Y6tWMpZAXD9t1Xh9n211YUBV
 FGSB4ZshnM/tgEPU6t787lJD4NsIIRp++MkJxdAitN4oUTqL0bdpIwezQ/CrYuBX
@@ -37,36 +37,43 @@ An/jMjZSMCxNl6UyFcqt5Et1EGVhuFECQQCZLXxaT+qcyHjlHJTMzuMgkz1QFbEp
 O5EX70gpeGQMPDK0QSWpaazg956njJSDbNCFM4BccrdQbJu1cW4qOsfBAkAMgZuG
 O88slmgTRHX4JGFmy3rrLiHNI2BbJSuJ++Yllz8beVzh6NfvuY+HKRCmPqoBPATU
 kXS9jgARhhiWXJrk
------END PRIVATE KEY-----`
-)
+-----END PRIVATE KEY-----`)
 
 func TestValidateCertificates(t *testing.T) {
 	t.Run("bad_certificate", func(t *testing.T) {
-		data := validateCertificates("bad cert", "", "")
-		assert.NotEmpty(t, data.WarningValidation)
-		assert.False(t, data.ValidCert)
-		assert.False(t, data.ValidChain)
+		status := &tlsConfigStatus{}
+		err := validateCertificates(status, []byte("bad cert"), nil, "")
+		assert.Error(t, err)
+		assert.NotEmpty(t, status.WarningValidation)
+		assert.False(t, status.ValidCert)
+		assert.False(t, status.ValidChain)
 	})
 
 	t.Run("bad_private_key", func(t *testing.T) {
-		data := validateCertificates("", "bad priv key", "")
-		assert.NotEmpty(t, data.WarningValidation)
-		assert.False(t, data.ValidKey)
+		status := &tlsConfigStatus{}
+		err := validateCertificates(status, nil, []byte("bad priv key"), "")
+		assert.Error(t, err)
+		assert.NotEmpty(t, status.WarningValidation)
+		assert.False(t, status.ValidKey)
 	})
 
 	t.Run("valid", func(t *testing.T) {
-		data := validateCertificates(CertificateChain, PrivateKey, "")
-		notBefore, _ := time.Parse(time.RFC3339, "2019-02-27T09:24:23Z")
-		notAfter, _ := time.Parse(time.RFC3339, "2046-07-14T09:24:23Z")
-		assert.NotEmpty(t, data.WarningValidation)
-		assert.True(t, data.ValidCert)
-		assert.False(t, data.ValidChain)
-		assert.True(t, data.ValidKey)
-		assert.Equal(t, "RSA", data.KeyType)
-		assert.Equal(t, "CN=AdGuard Home,O=AdGuard Ltd", data.Subject)
-		assert.Equal(t, "CN=AdGuard Home,O=AdGuard Ltd", data.Issuer)
-		assert.Equal(t, notBefore, data.NotBefore)
-		assert.Equal(t, notAfter, data.NotAfter)
-		assert.True(t, data.ValidPair)
+		status := &tlsConfigStatus{}
+		err := validateCertificates(status, testCertChainData, testPrivateKeyData, "")
+		assert.NoError(t, err)
+
+		notBefore := time.Date(2019, 2, 27, 9, 24, 23, 0, time.UTC)
+		notAfter := time.Date(2046, 7, 14, 9, 24, 23, 0, time.UTC)
+
+		assert.NotEmpty(t, status.WarningValidation)
+		assert.True(t, status.ValidCert)
+		assert.False(t, status.ValidChain)
+		assert.True(t, status.ValidKey)
+		assert.Equal(t, "RSA", status.KeyType)
+		assert.Equal(t, "CN=AdGuard Home,O=AdGuard Ltd", status.Subject)
+		assert.Equal(t, "CN=AdGuard Home,O=AdGuard Ltd", status.Issuer)
+		assert.Equal(t, notBefore, status.NotBefore)
+		assert.Equal(t, notAfter, status.NotAfter)
+		assert.True(t, status.ValidPair)
 	})
 }