From c5565a9e4e5e3cd54dd6d8c34e326c5f166ec401 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Tue, 1 Nov 2022 13:09:52 +0300 Subject: [PATCH] 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 Date: Tue Nov 1 13:02:23 2022 +0300 Merge branch 'master' into 4898-redirect-https commit 815e299137224fc3c7fd46924d7b936515b95d67 Author: Eugene Burkov Date: Tue Nov 1 12:58:28 2022 +0300 home: imp ip addr detection commit 9d4ecd9ab0e13ef6c19c3b923363bff43394ea4c Author: Eugene Burkov Date: Mon Oct 31 17:23:41 2022 +0300 home: imp cyclo commit 86c47b68fe6e3916cec97eee5d34e3e6c18e4892 Author: Eugene Burkov Date: Mon Oct 31 15:06:05 2022 +0300 all: imp text commit bcc25697b551668d1dab53a874e716fcadd83f09 Author: Eugene Burkov Date: Mon Oct 31 11:47:57 2022 +0300 home: fix test commit bb51a74cb82eeaa977821fa7314810c7b8be55cb Author: Eugene Burkov Date: Sun Oct 30 23:23:40 2022 +0300 home: imp code commit 38522330691baf8475a59ed4f40b1d45363df1e3 Author: Eugene Burkov Date: Fri Oct 28 17:00:50 2022 +0300 home: imp code commit 7284f7288feb7491560f0f5d2754044c7a9f603a Author: Eugene Burkov Date: Thu Oct 27 19:42:57 2022 +0300 all: log changes commit 540efcb013e15294b98efe581323f75ceefc8f5a Author: Eugene Burkov Date: Thu Oct 27 19:24:21 2022 +0300 home: imp tls --- CHANGELOG.md | 3 + internal/home/tls.go | 200 +++++++++++++++-------------- internal/home/tls_internal_test.go | 10 +- 3 files changed, 108 insertions(+), 105 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45aac6eda56..c8a8f4b40a0 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/home/tls.go b/internal/home/tls.go index 55c58fb5b60..a0426a47a1a 100644 --- a/internal/home/tls.go +++ b/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,71 +489,73 @@ func validatePorts( return nil } -// 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() - } - }() - - 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 - } +// validateCertChain verifies certs using the first as the main one and others +// as intermediate. srvName stands for the expected DNS name. +func validateCertChain(certs []*x509.Certificate, srvName string) (err error) { + main, others := certs[0], certs[1:] - if decoded.Type == "CERTIFICATE" { - certs = append(certs, decoded) - } + pool := x509.NewCertPool() + for _, cert := range others { + log.Info("tls: got an intermediate cert") + pool.AddCert(cert) } - parsedCerts, err := parsePEMCerts(certs) + opts := x509.VerifyOptions{ + DNSName: srvName, + Roots: Context.tlsRoots, + Intermediates: pool, + } + _, err = main.Verify(opts) if err != nil { - return err + return fmt.Errorf("certificate does not verify: %w", err) } - status.ValidCert = true + 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 + } - opts := x509.VerifyOptions{ - DNSName: serverName, - Roots: Context.tlsRoots, + for _, name := range cert.DNSNames { + if _, err := netip.ParseAddr(name); err == nil { + return true + } } - log.Info("tls: number of certs: %d", len(parsedCerts)) + return false +} - pool := x509.NewCertPool() - for _, cert := range parsedCerts[1:] { - log.Info("tls: got an intermediate cert") - pool.AddCert(cert) - } +// 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)) - opts.Intermediates = pool + 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) + } - mainCert := parsedCerts[0] - _, err = mainCert.Verify(opts) + parsedCerts, err = parsePEMCerts(certs) 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 + return nil, false, err } - if mainCert != nil { - status.Subject = mainCert.Subject.String() - status.Issuer = mainCert.Issuer.String() - status.NotAfter = mainCert.NotAfter - status.NotBefore = mainCert.NotBefore - status.DNSNames = mainCert.DNSNames + 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 nil + return parsedCerts, true, err } // parsePEMCerts parses multiple PEM-encoded certificates. @@ -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 -// error is also set in status.WarningValidation. -func validatePKey(status *tlsConfigStatus, pkey []byte) (err error) { - defer func() { - if err != nil { - status.WarningValidation = err.Error() - } - }() - +// validatePKey validates the private key, returning its type. It returns an +// empty string if error occurs. +func validatePKey(pkey []byte) (keyType string, 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 - decoded, pemblock = pem.Decode(pemblock) - if decoded == nil { - break - } - + for decoded, pemblock := pem.Decode([]byte(pkey)); decoded != nil; { 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 - status.KeyType = keyType - - return nil + return keyType, nil } -// validateCertificates processes certificate data and its private key. All -// parameters are optional. status must not be nil. The returned error is also -// set in status.WarningValidation. +// validateCertificates processes certificate data and its private key. status +// must not be nil, since it's used to accumulate the validation results. Other +// 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) - if err != nil { + var certs []*x509.Certificate + 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) - if err != nil { - return err + var keyErr error + status.KeyType, keyErr = validatePKey(pkey) + 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) - if err != nil { - err = fmt.Errorf("certificate-key pair: %w", err) - status.WarningValidation = err.Error() - - return err + _, pairErr := tls.X509KeyPair(certChain, pkey) + if pairErr != nil { + return fmt.Errorf("certificate-key pair: %w", pairErr) } status.ValidPair = true } - return nil + return err } // Key types. diff --git a/internal/home/tls_internal_test.go b/internal/home/tls_internal_test.go index b6e02f24338..201cf55121d 100644 --- a/internal/home/tls_internal_test.go +++ b/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) - assert.NotEmpty(t, status.WarningValidation) + testutil.AssertErrorMsg(t, "empty certificate", err) 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) - assert.NotEmpty(t, status.WarningValidation) + testutil.AssertErrorMsg(t, "no valid keys were found", err) 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)