TLS origination to TiDB is not compatible with loopback-bound forwarding proxy

[LINKIWI]

Bug Report

Please answer these questions before submitting your issue. Thanks!

What did you do?

I compiled tidb-dashboard from source, ran the binary, loaded the webpage, and clicked the login button.

The cluster has cluster TLS enabled, but plaintext TiDB. The runtime invocation is as follows:

$ ./tidb-dashboard --host 127.0.0.1 --port 12333 --pd https://internal-pd-hostname:2379 --cluster-ca /path/to/ca --cluster-cert /path/to/cert --cluster-key /path/to/key --debug --experimental

What did you expect to see?

I expected to see the tidb-dashboard web interface after a successful login.

What did you see instead?

The login fails with the following error:

What version of TiDB Dashboard are you using (./tidb-dashboard --version)?

Internal Version 2022.01.12.1

eef0c13b5546b4363642296f554b5e5e0a8ea105

---

This bug occurs because internally, Dashboard connects to TiDB via a loopback-bound TCP proxy: https://cs.github.com/pingcap/tidb-dashboard/blob/d02b6af853a3cac31c7a4c031347b8f6f9be9732/pkg/tidb/forwarder.go?q=net.listen#L63

The code path triggered by pressing "login" makes an HTTP request to obtain the cluster config and schema, among other information. This happens here: https://github.com/pingcap/tidb-dashboard/blob/d02b6af853a3cac31c7a4c031347b8f6f9be9732/pkg/apiserver/user/verify_sql_user.go?q=%22%2Fconfig%22#L41

Unfortunately, this causes client-side TLS server verification to fail because the request is directed to a loopback address 127.0.0.1 (the forwarding proxy). The server certificate presented here is only valid for hostname internal-pd-hostname.

The patch we have put into place to fix this issue is to modify the client TLS configuration to only verify that the certificate is issued by a trusted root CA, and skipping hostname verification.

diff --git a/dashboard/cmd/tidb-dashboard/main.go b/dashboard/cmd/tidb-dashboard/main.go
index a62698583d..c3cadc7ad7 100755
--- a/dashboard/cmd/tidb-dashboard/main.go
+++ b/dashboard/cmd/tidb-dashboard/main.go
@@ -26,6 +26,7 @@ package main
 import (
        "context"
        "crypto/tls"
+       "crypto/x509"
        "fmt"
        "net"
        "net/http"
@@ -155,6 +156,24 @@ func buildTLSConfig(caPath, keyPath, certPath *string) *tls.Config {
        if err != nil {
                log.Fatal("Failed to load certificates", zap.Error(err))
        }
+       // Manually override the client-side server verification to only verify that the offered
+       // certificate is issued by a trusted CA. Internally, dashboard uses a loopback-bound proxy
+       // to forward requests to TiDB, which otherwise causes server hostname verification to fail.
+       // Reference: https://github.com/golang/go/blob/70deaa33ebd91944484526ab368fa19c499ff29f/src/crypto/tls/example_test.go#L186
+       tlsConfig.InsecureSkipVerify = true
+       tlsConfig.VerifyConnection = func(state tls.ConnectionState) error {
+               opts := x509.VerifyOptions{
+                       Intermediates: x509.NewCertPool(),
+                       Roots: tlsConfig.RootCAs,
+               }
+
+               for _, cert := range state.PeerCertificates[1:] {
+                       opts.Intermediates.AddCert(cert)
+               }
+
+               _, err := state.PeerCertificates[0].Verify(opts)
+               return err
+       }
        return tlsConfig
 }

I'm happy to submit a PR with this change if it looks acceptable. However I'm not sure if there are other nuances that need to be taken into account.

[breezewish]

Thanks for the great issue digging! This issue also occurs for all TLS connections performed through the proxy. Related: https://github.com/pingcap/tidb-dashboard/issues/523

I'm thinking about a more complicated but maybe secure way. Suppose that we want to connect to the remote foo-tidb.internal via a local proxy at 127.0.0.1. Is it possible to verify that the TLS certificate is indeed foo-tidb.internal (instead of 127.0.0.1)? Maybe in this way we can keep the common name check. This might be pretty hacky I know, since currently it is by design that the proxy user will not have any knowledge about the proxy upstreams.

[LINKIWI]

Thanks for taking a look. Adding a manual override for certificate name validation in addition to CA validation sounds like a reasonable idea, perhaps an additional command line option like --tls-verify-names <comma delimited acceptable names>.

I'll note though that use of CN for cert identities is considered deprecated practice for identity verification, in favor of SANs. (Interestingly I have a related open PR in PD to implement exactly that: tikv/pd#4309 though I haven't had time to revisit the implementation.)

Cert identities are exposed in the VerifyConnection callback so implementation would be relatively straightforward.

[LINKIWI]

I tested the following patch in our environment and it seems to work well.

diff --git a/cmd/tidb-dashboard/main.go b/cmd/tidb-dashboard/main.go
index 58865bf..713a277 100755
--- a/cmd/tidb-dashboard/main.go
+++ b/cmd/tidb-dashboard/main.go
@@ -15,6 +15,7 @@ package main
 import (
 	"context"
 	"crypto/tls"
+	"crypto/x509"
 	"fmt"
 	"net"
 	"net/http"
@@ -72,10 +73,12 @@ func NewCLIConfig() *DashboardCLIConfig {
 	clusterCaPath := flag.String("cluster-ca", "", "path of file that contains list of trusted SSL CAs")
 	clusterCertPath := flag.String("cluster-cert", "", "path of file that contains X509 certificate in PEM format")
 	clusterKeyPath := flag.String("cluster-key", "", "path of file that contains X509 key in PEM format")
+	clusterAllowedNames := flag.String("cluster-allowed-names", "", "comma-delimited list of acceptable peer certificate SAN identities")
 
 	tidbCaPath := flag.String("tidb-ca", "", "path of file that contains list of trusted SSL CAs")
 	tidbCertPath := flag.String("tidb-cert", "", "path of file that contains X509 certificate in PEM format")
 	tidbKeyPath := flag.String("tidb-key", "", "path of file that contains X509 key in PEM format")
+	tidbAllowedNames := flag.String("tidb-allowed-names", "", "comma-delimited list of acceptable peer certificate SAN identities")
 
 	// debug for keyvisual，hide help information
 	flag.Int64Var(&cfg.KVFileStartTime, "keyviz-file-start", 0, "(debug) start time for file range in file mode")
@@ -94,13 +97,13 @@ func NewCLIConfig() *DashboardCLIConfig {
 
 	// setup TLS config for TiDB components
 	if len(*clusterCaPath) != 0 && len(*clusterCertPath) != 0 && len(*clusterKeyPath) != 0 {
-		cfg.CoreConfig.ClusterTLSConfig = buildTLSConfig(clusterCaPath, clusterKeyPath, clusterCertPath)
+		cfg.CoreConfig.ClusterTLSConfig = buildTLSConfig(clusterCaPath, clusterKeyPath, clusterCertPath, clusterAllowedNames)
 	}
 
 	// setup TLS config for MySQL client
 	// See https://github.com/pingcap/docs/blob/7a62321b3ce9318cbda8697503c920b2a01aeb3d/how-to/secure/enable-tls-clients.md#enable-authentication
 	if (len(*tidbCertPath) != 0 && len(*tidbKeyPath) != 0) || len(*tidbCaPath) != 0 {
-		cfg.CoreConfig.TiDBTLSConfig = buildTLSConfig(tidbCaPath, tidbKeyPath, tidbCertPath)
+		cfg.CoreConfig.TiDBTLSConfig = buildTLSConfig(tidbCaPath, tidbKeyPath, tidbCertPath, tidbAllowedNames)
 	}
 
 	if err := cfg.CoreConfig.NormalizePDEndPoint(); err != nil {
@@ -135,16 +138,65 @@ func getContext() context.Context {
 	return ctx
 }
 
-func buildTLSConfig(caPath, keyPath, certPath *string) *tls.Config {
+func buildTLSConfig(caPath, keyPath, certPath, allowedNames *string) *tls.Config {
 	tlsInfo := transport.TLSInfo{
 		TrustedCAFile: *caPath,
 		KeyFile:       *keyPath,
 		CertFile:      *certPath,
 	}
+
 	tlsConfig, err := tlsInfo.ClientConfig()
 	if err != nil {
 		log.Fatal("Failed to load certificates", zap.Error(err))
 	}
+
+	// Disable the default server verification routine in favor of a manually defined connection
+	// verification callback. The custom verification process verifies that the server
+	// certificate is issued by a trusted root CA, and that the peer certificate identities
+	// matches at least one entry specified in verifyNames (if specified). This is required
+	// because tidb-dashboard directs requests to a loopback-bound forwarding proxy, which would
+	// otherwise cause server hostname verification to fail.
+	tlsConfig.InsecureSkipVerify = true
+	tlsConfig.VerifyConnection = func(state tls.ConnectionState) error {
+		opts := x509.VerifyOptions{
+			Intermediates: x509.NewCertPool(),
+			Roots:         tlsConfig.RootCAs,
+		}
+
+		for _, cert := range state.PeerCertificates[1:] {
+			opts.Intermediates.AddCert(cert)
+		}
+
+		_, err := state.PeerCertificates[0].Verify(opts)
+
+		// Optionally verify the peer SANs when available. If no peer identities are
+		// provided, simply reuse the verification result of the CA verification.
+		if err != nil || *allowedNames == "" {
+			return err
+		}
+
+		for _, name := range strings.Split(*allowedNames, ",") {
+			for _, dns := range state.PeerCertificates[0].DNSNames {
+				if name == dns {
+					return nil
+				}
+			}
+
+			for _, uri := range state.PeerCertificates[0].URIs {
+				if name == uri.String() {
+					return nil
+				}
+			}
+		}
+
+		return fmt.Errorf(
+			"no SANs in server certificate (%v, %v) match allowed names %v",
+			state.PeerCertificates[0].DNSNames,
+			state.PeerCertificates[0].URIs,
+			strings.Split(*allowedNames, ","),
+		)
+	}
+
 	return tlsConfig
 }

[AndreMouche]

@breezewish do we have plan to fix it?

[mornyx]

/assign

[mornyx]

Hi @LINKIWI , sorry it took so long to reply. Do you still plan to submit a PR for this patch? If you don't have any time, can we commit your patch?

[LINKIWI]

Hi, I no longer work at the company where we were using TiDB and encountering this issue.

I don't have any plans to submit a PR. If the patch in the issue summary works, please feel free to commit it.