Pull request: 4898-redirect-https

Merge in DNS/adguard-home from 4898-redirect-https to master

Updates #4898.
Updates #4927.

Squashed commit of the following:

commit bc41b6cae7ede0f1235e3956ab49204af1c9f38d
Merge: 815e2991 ac7634da
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Nov 1 13:02:23 2022 +0300

    Merge branch 'master' into 4898-redirect-https

commit 815e299137224fc3c7fd46924d7b936515b95d67
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Tue Nov 1 12:58:28 2022 +0300

    home: imp ip addr detection

commit 9d4ecd9ab0e13ef6c19c3b923363bff43394ea4c
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Oct 31 17:23:41 2022 +0300

    home: imp cyclo

commit 86c47b68fe6e3916cec97eee5d34e3e6c18e4892
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Oct 31 15:06:05 2022 +0300

    all: imp text

commit bcc25697b551668d1dab53a874e716fcadd83f09
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Oct 31 11:47:57 2022 +0300

    home: fix test

commit bb51a74cb82eeaa977821fa7314810c7b8be55cb
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Sun Oct 30 23:23:40 2022 +0300

    home: imp code

commit 38522330691baf8475a59ed4f40b1d45363df1e3
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Fri Oct 28 17:00:50 2022 +0300

    home: imp code

commit 7284f7288feb7491560f0f5d2754044c7a9f603a
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Oct 27 19:42:57 2022 +0300

    all: log changes

commit 540efcb013e15294b98efe581323f75ceefc8f5a
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Thu Oct 27 19:24:21 2022 +0300

    home: imp tls

CHANGELOG.md

@@ -17,6 +17,8 @@ and this project adheres to
 
 ## Added
 
+- The warning message when adding a certificate having no IP addresses
+  ([#4898]).
 - Several new blockable services ([#3972]).  Those will now be more in sync with
   the services that are already blockable in AdGuard DNS.
 - A new HTTP API, `GET /control/blocked_services/all`, that lists all available
@@ -51,6 +53,7 @@ and this project adheres to
 [#2926]: https://github.com/AdguardTeam/AdGuardHome/issues/2926
 [#3418]: https://github.com/AdguardTeam/AdGuardHome/issues/3418
 [#3972]: https://github.com/AdguardTeam/AdGuardHome/issues/3972
+[#4898]: https://github.com/AdguardTeam/AdGuardHome/issues/4898
 [#4916]: https://github.com/AdguardTeam/AdGuardHome/issues/4916
 [#4925]: https://github.com/AdguardTeam/AdGuardHome/issues/4925
 [#4942]: https://github.com/AdguardTeam/AdGuardHome/issues/4942

internal/home/tls.go

@@ -13,6 +13,7 @@ import (
 	"encoding/pem"
 	"fmt"
 	"net/http"
+	"net/netip"
 	"os"
 	"strings"
 	"sync"
@@ -160,6 +161,10 @@ func loadTLSConf(tlsConf *tlsConfigSettings, status *tlsConfigStatus) (err error
 	defer func() {
 		if err != nil {
 			status.WarningValidation = err.Error()
+			if status.ValidCert && status.ValidKey && status.ValidPair {
+				// Do not return warnings since those aren't critical.
+				err = nil
+			}
 		}
 	}()
 
@@ -176,6 +181,8 @@ func loadTLSConf(tlsConf *tlsConfigSettings, status *tlsConfigStatus) (err error
 			return fmt.Errorf("reading cert file: %w", err)
 		}
 
+		// Set status.ValidCert to true to signal the frontend that the
+		// certificate opens successfully while the private key can't be opened.
 		status.ValidCert = true
 	}
 
@@ -482,73 +489,75 @@ func validatePorts(
 	return nil
 }
 
-// validateCertChain validates the certificate chain and sets data in status.
+// validateCertChain verifies certs using the first as the main one and others
-// The returned error is also set in status.WarningValidation.
+// as intermediate.  srvName stands for the expected DNS name.
-func validateCertChain(status *tlsConfigStatus, certChain []byte, serverName string) (err error) {
+func validateCertChain(certs []*x509.Certificate, srvName string) (err error) {
-	defer func() {
+	main, others := certs[0], certs[1:]
-		if err != nil {
-			status.WarningValidation = err.Error()
-		}
-	}()
-
-	log.Debug("tls: got certificate chain: %d bytes", len(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)
-		}
-	}
-
-	parsedCerts, err := parsePEMCerts(certs)
-	if err != nil {
-		return err
-	}
-
-	status.ValidCert = true
-
-	opts := x509.VerifyOptions{
-		DNSName: serverName,
-		Roots:   Context.tlsRoots,
-	}
-
-	log.Info("tls: number of certs: %d", len(parsedCerts))
 
 	pool := x509.NewCertPool()
-	for _, cert := range parsedCerts[1:] {
+	for _, cert := range others {
 		log.Info("tls: got an intermediate cert")
 		pool.AddCert(cert)
 	}
 
-	opts.Intermediates = pool
+	opts := x509.VerifyOptions{
-
+		DNSName:       srvName,
-	mainCert := parsedCerts[0]
+		Roots:         Context.tlsRoots,
-	_, err = mainCert.Verify(opts)
+		Intermediates: pool,
-	if err != nil {
-		// Let self-signed certs through and don't return this error.
-		status.WarningValidation = fmt.Sprintf("certificate does not verify: %s", err)
-	} else {
-		status.ValidChain = true
 	}
-
+	_, err = main.Verify(opts)
-	if mainCert != nil {
+	if err != nil {
-		status.Subject = mainCert.Subject.String()
+		return fmt.Errorf("certificate does not verify: %w", err)
-		status.Issuer = mainCert.Issuer.String()
-		status.NotAfter = mainCert.NotAfter
-		status.NotBefore = mainCert.NotBefore
-		status.DNSNames = mainCert.DNSNames
 	}
 
 	return nil
 }
 
+// certHasIP returns true if cert has at least a single IP address either in its
+// DNS names or in the IP addresses section.
+func certHasIP(cert *x509.Certificate) (ok bool) {
+	if len(cert.IPAddresses) > 0 {
+		return true
+	}
+
+	for _, name := range cert.DNSNames {
+		if _, err := netip.ParseAddr(name); err == nil {
+			return true
+		}
+	}
+
+	return false
+}
+
+// parseCertChain parses the certificate chain from raw data, and returns it.
+// If ok is true, the returned error, if any, is not critical.
+func parseCertChain(chain []byte) (parsedCerts []*x509.Certificate, ok bool, err error) {
+	log.Debug("tls: got certificate chain: %d bytes", len(chain))
+
+	var certs []*pem.Block
+	for decoded, pemblock := pem.Decode(chain); decoded != nil; {
+		if decoded.Type == "CERTIFICATE" {
+			certs = append(certs, decoded)
+		}
+
+		decoded, pemblock = pem.Decode(pemblock)
+	}
+
+	parsedCerts, err = parsePEMCerts(certs)
+	if err != nil {
+		return nil, false, err
+	}
+
+	log.Info("tls: number of certs: %d", len(parsedCerts))
+
+	if !certHasIP(parsedCerts[0]) {
+		err = errors.Error(`certificate has no IP addresses` +
+			`, this may cause issues with DNS-over-TLS clients`)
+	}
+
+	return parsedCerts, true, err
+}
+
 // parsePEMCerts parses multiple PEM-encoded certificates.
 func parsePEMCerts(certs []*pem.Block) (parsedCerts []*x509.Certificate, err error) {
 	for i, cert := range certs {
@@ -568,106 +577,99 @@ func parsePEMCerts(certs []*pem.Block) (parsedCerts []*x509.Certificate, err err
 	return parsedCerts, nil
 }
 
-// validatePKey validates the private key and sets data in status.  The returned
+// validatePKey validates the private key, returning its type.  It returns an
-// error is also set in status.WarningValidation.
+// empty string if error occurs.
-func validatePKey(status *tlsConfigStatus, pkey []byte) (err error) {
+func validatePKey(pkey []byte) (keyType string, 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 decoded, pemblock := pem.Decode([]byte(pkey)); decoded != nil; {
-	for {
-		var decoded *pem.Block
-		decoded, pemblock = pem.Decode(pemblock)
-		if decoded == nil {
-			break
-		}
-
 		if decoded.Type == "PRIVATE KEY" || strings.HasSuffix(decoded.Type, " PRIVATE KEY") {
 			key = decoded
 
 			break
 		}
+
+		decoded, pemblock = pem.Decode(pemblock)
 	}
 
 	if key == nil {
-		return errors.Error("no valid keys were found")
+		return "", errors.Error("no valid keys were found")
 	}
 
-	_, keyType, err := parsePrivateKey(key.Bytes)
+	_, keyType, err = parsePrivateKey(key.Bytes)
 	if err != nil {
-		return fmt.Errorf("parsing private key: %w", err)
+		return "", fmt.Errorf("parsing private key: %w", err)
 	}
 
 	if keyType == keyTypeED25519 {
-		return errors.Error(
+		return "", errors.Error(
 			"ED25519 keys are not supported by browsers; " +
 				"did you mean to use X25519 for key exchange?",
 		)
 	}
 
-	status.ValidKey = true
+	return keyType, nil
-	status.KeyType = keyType
-
-	return nil
 }
 
-// validateCertificates processes certificate data and its private key.  All
+// validateCertificates processes certificate data and its private key.  status
-// parameters are optional.  status must not be nil.  The returned error is also
+// must not be nil, since it's used to accumulate the validation results.  Other
-// set in status.WarningValidation.
+// parameters are optional.
 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:]
-		}
-	}()
-
 	// Check only the public certificate separately from the key.
 	if len(certChain) > 0 {
-		err = validateCertChain(status, certChain, serverName)
+		var certs []*x509.Certificate
-		if err != nil {
+		certs, status.ValidCert, err = parseCertChain(certChain)
+		if !status.ValidCert {
+			// Don't wrap the error, since it's informative enough as is.
 			return err
 		}
+
+		mainCert := certs[0]
+		status.Subject = mainCert.Subject.String()
+		status.Issuer = mainCert.Issuer.String()
+		status.NotAfter = mainCert.NotAfter
+		status.NotBefore = mainCert.NotBefore
+		status.DNSNames = mainCert.DNSNames
+
+		if chainErr := validateCertChain(certs, serverName); chainErr != nil {
+			// Let self-signed certs through and don't return this error to set
+			// its message into the status.WarningValidation afterwards.
+			err = chainErr
+		} else {
+			status.ValidChain = true
+		}
 	}
 
 	// Validate the private key by parsing it.
 	if len(pkey) > 0 {
-		err = validatePKey(status, pkey)
+		var keyErr error
-		if err != nil {
+		status.KeyType, keyErr = validatePKey(pkey)
-			return err
+		if keyErr != nil {
+			// Don't wrap the error, since it's informative enough as is.
+			return keyErr
 		}
+
+		status.ValidKey = true
 	}
 
 	// If both are set, validate together.
 	if len(certChain) > 0 && len(pkey) > 0 {
-		_, err = tls.X509KeyPair(certChain, pkey)
+		_, pairErr := tls.X509KeyPair(certChain, pkey)
-		if err != nil {
+		if pairErr != nil {
-			err = fmt.Errorf("certificate-key pair: %w", err)
+			return fmt.Errorf("certificate-key pair: %w", pairErr)
-			status.WarningValidation = err.Error()
-
-			return err
 		}
 
 		status.ValidPair = true
 	}
 
-	return nil
+	return err
 }
 
 // Key types.

internal/home/tls_internal_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/AdguardTeam/golibs/testutil"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -43,8 +44,7 @@ func TestValidateCertificates(t *testing.T) {
 	t.Run("bad_certificate", func(t *testing.T) {
 		status := &tlsConfigStatus{}
 		err := validateCertificates(status, []byte("bad cert"), nil, "")
-		assert.Error(t, err)
+		testutil.AssertErrorMsg(t, "empty certificate", err)
-		assert.NotEmpty(t, status.WarningValidation)
 		assert.False(t, status.ValidCert)
 		assert.False(t, status.ValidChain)
 	})
@@ -52,20 +52,18 @@ func TestValidateCertificates(t *testing.T) {
 	t.Run("bad_private_key", func(t *testing.T) {
 		status := &tlsConfigStatus{}
 		err := validateCertificates(status, nil, []byte("bad priv key"), "")
-		assert.Error(t, err)
+		testutil.AssertErrorMsg(t, "no valid keys were found", err)
-		assert.NotEmpty(t, status.WarningValidation)
 		assert.False(t, status.ValidKey)
 	})
 
 	t.Run("valid", func(t *testing.T) {
 		status := &tlsConfigStatus{}
 		err := validateCertificates(status, testCertChainData, testPrivateKeyData, "")
-		assert.NoError(t, err)
+		assert.Error(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)