diff --git a/internal/home/controlinstall.go b/internal/home/controlinstall.go index 787fb5d1..0cbddf00 100644 --- a/internal/home/controlinstall.go +++ b/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) diff --git a/internal/home/home.go b/internal/home/home.go index dbe34f4e..70697a12 100644 --- a/internal/home/home.go +++ b/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() { diff --git a/internal/home/mobileconfig_test.go b/internal/home/mobileconfig_test.go index 48783d0d..3587154f 100644 --- a/internal/home/mobileconfig_test.go +++ b/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) diff --git a/internal/home/tls.go b/internal/home/tls.go index e7e8e35f..55c58fb5 100644 --- a/internal/home/tls.go +++ b/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) } diff --git a/internal/home/tls_internal_test.go b/internal/home/tls_internal_test.go index 46f14a2a..b6e02f24 100644 --- a/internal/home/tls_internal_test.go +++ b/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) }) }