From 2755f3843c6f5d91d3f54e57091c531bd18170d5 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 1 Feb 2023 14:29:44 -0500 Subject: [PATCH] health, net/tlsdial: add healthcheck for self-signed cert When we make a connection to a server, we previously would verify with the system roots, and then fall back to verifying with our baked-in Let's Encrypt root if the system root cert verification failed. We now explicitly check for, and log a health error on, self-signed certificates. Additionally, we now always verify against our baked-in Let's Encrypt root certificate and log an error if that isn't successful. We don't consider this a health failure, since if we ever change our server certificate issuer in the future older non-updated versions of Tailscale will no longer be healthy despite being able to connect. Updates #3198 Change-Id: I00be5ceb8afee544ee795e3c7a2815476abc4abf Signed-off-by: Andrew Dunham --- cmd/derper/depaware.txt | 3 +++ cmd/tailscale/depaware.txt | 4 +++- health/health.go | 16 +++++++++++++ net/tlsdial/tlsdial.go | 49 +++++++++++++++++++++++++++++++------- 4 files changed, 63 insertions(+), 9 deletions(-) diff --git a/cmd/derper/depaware.txt b/cmd/derper/depaware.txt index c4a3803c6..90fad07c5 100644 --- a/cmd/derper/depaware.txt +++ b/cmd/derper/depaware.txt @@ -34,6 +34,7 @@ tailscale.com/cmd/derper dependencies: (generated by github.com/tailscale/depawa tailscale.com/derp/derphttp from tailscale.com/cmd/derper tailscale.com/disco from tailscale.com/derp tailscale.com/envknob from tailscale.com/derp+ + tailscale.com/health from tailscale.com/net/tlsdial tailscale.com/hostinfo from tailscale.com/net/interfaces+ tailscale.com/ipn from tailscale.com/client/tailscale tailscale.com/ipn/ipnstate from tailscale.com/client/tailscale+ @@ -81,6 +82,8 @@ tailscale.com/cmd/derper dependencies: (generated by github.com/tailscale/depawa tailscale.com/util/httpm from tailscale.com/client/tailscale tailscale.com/util/lineread from tailscale.com/hostinfo+ tailscale.com/util/mak from tailscale.com/syncs + tailscale.com/util/multierr from tailscale.com/health + tailscale.com/util/set from tailscale.com/health tailscale.com/util/singleflight from tailscale.com/net/dnscache tailscale.com/util/vizerror from tailscale.com/tsweb W 💣 tailscale.com/util/winutil from tailscale.com/hostinfo+ diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index 65b161f5c..16267ee59 100644 --- a/cmd/tailscale/depaware.txt +++ b/cmd/tailscale/depaware.txt @@ -57,6 +57,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep tailscale.com/derp/derphttp from tailscale.com/net/netcheck tailscale.com/disco from tailscale.com/derp tailscale.com/envknob from tailscale.com/cmd/tailscale/cli+ + tailscale.com/health from tailscale.com/net/tlsdial tailscale.com/health/healthmsg from tailscale.com/cmd/tailscale/cli tailscale.com/hostinfo from tailscale.com/net/interfaces+ tailscale.com/ipn from tailscale.com/cmd/tailscale/cli+ @@ -111,9 +112,10 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep tailscale.com/util/httpm from tailscale.com/client/tailscale tailscale.com/util/lineread from tailscale.com/net/interfaces+ tailscale.com/util/mak from tailscale.com/net/netcheck+ - tailscale.com/util/multierr from tailscale.com/control/controlhttp + tailscale.com/util/multierr from tailscale.com/control/controlhttp+ tailscale.com/util/must from tailscale.com/cmd/tailscale/cli tailscale.com/util/quarantine from tailscale.com/cmd/tailscale/cli + tailscale.com/util/set from tailscale.com/health tailscale.com/util/singleflight from tailscale.com/net/dnscache 💣 tailscale.com/util/winutil from tailscale.com/hostinfo+ tailscale.com/version from tailscale.com/cmd/tailscale/cli+ diff --git a/health/health.go b/health/health.go index 3517c4be3..bef416272 100644 --- a/health/health.go +++ b/health/health.go @@ -48,6 +48,7 @@ var ( controlHealth []string lastLoginErr error localLogConfigErr error + tlsConnectionErrors = map[string]error{} // map[ServerName]error ) // Subsystem is the name of a subsystem whose health can be monitored. @@ -209,6 +210,18 @@ func SetLocalLogConfigHealth(err error) { localLogConfigErr = err } +// SetTLSConnectionError sets the error state for connections to a specific +// host. Setting the error to nil will clear any previously-set error. +func SetTLSConnectionError(host string, err error) { + mu.Lock() + defer mu.Unlock() + if err == nil { + delete(tlsConnectionErrors, host) + } else { + tlsConnectionErrors[host] = err + } +} + func RegisterDebugHandler(typ string, h http.Handler) { mu.Lock() defer mu.Unlock() @@ -476,6 +489,9 @@ func overallErrorLocked() error { if err := envknob.ApplyDiskConfigError(); err != nil { errs = append(errs, err) } + for serverName, err := range tlsConnectionErrors { + errs = append(errs, fmt.Errorf("TLS connection error for %q: %w", serverName, err)) + } if e := fakeErrForTesting(); len(errs) == 0 && e != "" { return errors.New(e) } diff --git a/net/tlsdial/tlsdial.go b/net/tlsdial/tlsdial.go index 4a3f420b4..d571d38a6 100644 --- a/net/tlsdial/tlsdial.go +++ b/net/tlsdial/tlsdial.go @@ -11,9 +11,11 @@ package tlsdial import ( + "bytes" "crypto/tls" "crypto/x509" "errors" + "fmt" "log" "os" "sync" @@ -21,6 +23,7 @@ import ( "time" "tailscale.com/envknob" + "tailscale.com/health" ) var counterFallbackOK int32 // atomic @@ -33,6 +36,11 @@ var sslKeyLogFile = os.Getenv("SSLKEYLOGFILE") var debug = envknob.RegisterBool("TS_DEBUG_TLS_DIAL") +// tlsdialWarningPrinted tracks whether we've printed a warning about a given +// hostname already, to avoid log spam for users with custom DERP servers, +// Headscale, etc. +var tlsdialWarningPrinted sync.Map // map[string]bool + // Config returns a tls.Config for connecting to a server. // If base is non-nil, it's cloned as the base config before // being configured and returned. @@ -66,6 +74,16 @@ func Config(host string, base *tls.Config) *tls.Config { // (with the baked-in fallback root) in the VerifyConnection hook. conf.InsecureSkipVerify = true conf.VerifyConnection = func(cs tls.ConnectionState) error { + // Perform some health checks on this certificate before we do + // any verification. + if certIsSelfSigned(cs.PeerCertificates[0]) { + // Self-signed certs are never valid. + health.SetTLSConnectionError(cs.ServerName, fmt.Errorf("certificate is self-signed")) + } else { + // Ensure we clear any error state for this ServerName. + health.SetTLSConnectionError(cs.ServerName, nil) + } + // First try doing x509 verification with the system's // root CA pool. opts := x509.VerifyOptions{ @@ -79,18 +97,27 @@ func Config(host string, base *tls.Config) *tls.Config { if debug() { log.Printf("tlsdial(sys %q): %v", host, errSys) } - if errSys == nil { - return nil - } - // If that failed, because the system's CA roots are old - // or broken, fall back to trying LetsEncrypt at least. + // Always verify with our baked-in Let's Encrypt certificate, + // so we can log an informational message. This is useful for + // detecting SSL MiTM. opts.Roots = bakedInRoots() - _, err := cs.PeerCertificates[0].Verify(opts) + _, bakedErr := cs.PeerCertificates[0].Verify(opts) if debug() { - log.Printf("tlsdial(bake %q): %v", host, err) + log.Printf("tlsdial(bake %q): %v", host, bakedErr) + } else if bakedErr != nil { + if _, loaded := tlsdialWarningPrinted.LoadOrStore(host, true); !loaded { + if errSys == nil { + log.Printf("tlsdial: warning: server cert for %q is not a Let's Encrypt cert", host) + } else { + log.Printf("tlsdial: error: server cert for %q failed to verify and is not a Let's Encrypt cert", host) + } + } } - if err == nil { + + if errSys == nil { + return nil + } else if bakedErr == nil { atomic.AddInt32(&counterFallbackOK, 1) return nil } @@ -99,6 +126,12 @@ func Config(host string, base *tls.Config) *tls.Config { return conf } +func certIsSelfSigned(cert *x509.Certificate) bool { + // A certificate is determined to be self-signed if the certificate's + // subject is the same as its issuer. + return bytes.Equal(cert.RawSubject, cert.RawIssuer) +} + // SetConfigExpectedCert modifies c to expect and verify that the server returns // a certificate for the provided certDNSName. //