Improve proxy logging and add TLS-specific testing

config/config.go

@@ -251,10 +251,10 @@ type (
 
 func FromServerTLSConfig(cfg ServerTLSConfig) encryption.TLSConfig {
 	return encryption.TLSConfig{
-		CertificatePath:  cfg.CertificatePath,
-		KeyPath:          cfg.KeyPath,
-		RemoteCAPath:     cfg.ClientCAPath,
-		ValidateClientCA: cfg.RequireClientAuth,
+		CertificatePath: cfg.CertificatePath,
+		KeyPath:         cfg.KeyPath,
+		RemoteCAPath:    cfg.ClientCAPath,
+		VerifyCA:        cfg.RequireClientAuth,
 	}
 }
 func FromClientTLSConfig(cfg ClientTLSConfig) encryption.TLSConfig {

config/converter.go

@@ -119,22 +119,22 @@ func translateClientTCPTLSInfo(cfg TCPClientSetting) TCPTLSInfo {
 	return TCPTLSInfo{
 		ConnectionString: cfg.ServerAddress,
 		TLSConfig: encryption.TLSConfig{
-			CertificatePath:  cfg.TLS.CertificatePath,
-			KeyPath:          cfg.TLS.KeyPath,
-			RemoteCAPath:     cfg.TLS.ServerCAPath,
-			CAServerName:     cfg.TLS.ServerName,
-			ValidateClientCA: false,
+			CertificatePath: cfg.TLS.CertificatePath,
+			KeyPath:         cfg.TLS.KeyPath,
+			RemoteCAPath:    cfg.TLS.ServerCAPath,
+			CAServerName:    cfg.TLS.ServerName,
+			VerifyCA:        cfg.TLS.ServerName != "" && cfg.TLS.ServerCAPath != "",
 		},
 	}
 }
 func translateServerTCPTLSInfo(cfg TCPServerSetting) TCPTLSInfo {
 	return TCPTLSInfo{
 		ConnectionString: cfg.ListenAddress,
 		TLSConfig: encryption.TLSConfig{
-			CertificatePath:  cfg.TLS.CertificatePath,
-			KeyPath:          cfg.TLS.KeyPath,
-			RemoteCAPath:     cfg.TLS.ClientCAPath,
-			ValidateClientCA: cfg.TLS.RequireClientAuth,
+			CertificatePath: cfg.TLS.CertificatePath,
+			KeyPath:         cfg.TLS.KeyPath,
+			RemoteCAPath:    cfg.TLS.ClientCAPath,
+			VerifyCA:        cfg.TLS.RequireClientAuth,
 		},
 	}
 }

config/new_config_test.go

@@ -30,7 +30,7 @@ func TestBasic(t *testing.T) {
 	require.Equal(t, "127.0.0.1:9004", proxyConfig.ClusterConnections[0].RemoteServer.Connection.MuxAddressInfo.ConnectionString)
 	require.Equal(t, "", proxyConfig.ClusterConnections[0].RemoteServer.Connection.TcpServer.ConnectionString)
 	require.Equal(t, "", proxyConfig.ClusterConnections[0].RemoteServer.Connection.TcpClient.ConnectionString)
-	require.Equal(t, false, proxyConfig.ClusterConnections[0].RemoteServer.Connection.MuxAddressInfo.TLSConfig.ValidateClientCA)
+	require.Equal(t, false, proxyConfig.ClusterConnections[0].RemoteServer.Connection.MuxAddressInfo.TLSConfig.VerifyCA)
 	nsTranslation, err := proxyConfig.ClusterConnections[0].NamespaceTranslation.AsLocalToRemoteBiMap()
 	require.NoError(t, err)
 	require.Equal(t, "remoteName", nsTranslation.Get("localName"))
@@ -55,9 +55,26 @@ func TestConversion(t *testing.T) {
 	require.Nil(t, converted.Inbound)
 	require.Nil(t, converted.Outbound)
 	require.Equal(t, ConnTypeTCP, converted.ClusterConnections[0].RemoteServer.Connection.ConnectionType)
-	require.True(t, converted.ClusterConnections[0].RemoteServer.Connection.TcpServer.TLSConfig.ValidateClientCA)
+	require.True(t, converted.ClusterConnections[0].RemoteServer.Connection.TcpServer.TLSConfig.VerifyCA)
 	require.Equal(t, ConnTypeTCP, converted.ClusterConnections[0].LocalServer.Connection.ConnectionType)
 	require.Equal(t, "AddOrUpdateRemoteCluster", converted.ClusterConnections[0].LocalServer.ACLPolicy.AllowedMethods.AdminService[0])
 	require.Equal(t, "namespace1", converted.ClusterConnections[0].LocalServer.ACLPolicy.AllowedNamespaces[0])
 	require.Equal(t, int64(100), *converted.ClusterConnections[0].LocalServer.APIOverrides.AdminService.DescribeCluster.Response.FailoverVersionIncrement)
 }
+
+func TestConversionWithTLS(t *testing.T) {
+	samplePath := filepath.Join("..", "develop", "old-config-with-TLS.yaml")
+
+	proxyConfig, err := LoadConfig[S2SProxyConfig](samplePath)
+	require.NoError(t, err)
+	converted := ToClusterConnConfig(proxyConfig)
+	require.Equal(t, 1, len(converted.ClusterConnections))
+	require.Nil(t, converted.Inbound)
+	require.Nil(t, converted.Outbound)
+	require.Equal(t, ConnTypeMuxClient, converted.ClusterConnections[0].RemoteServer.Connection.ConnectionType)
+	require.False(t, converted.ClusterConnections[0].RemoteServer.Connection.TcpServer.TLSConfig.VerifyCA)
+	require.Equal(t, ConnTypeTCP, converted.ClusterConnections[0].LocalServer.Connection.ConnectionType)
+	require.Equal(t, "AddOrUpdateRemoteCluster", converted.ClusterConnections[0].LocalServer.ACLPolicy.AllowedMethods.AdminService[0])
+	require.Equal(t, 0, len(converted.ClusterConnections[0].LocalServer.ACLPolicy.AllowedNamespaces))
+	require.Nil(t, converted.ClusterConnections[0].LocalServer.APIOverrides)
+}

develop/old-config-with-TLS.yaml

@@ -0,0 +1,75 @@
+inbound:
+  name: "inbound-server"
+  server:
+    # No TLS here because the mux connection is already mTLS
+    type: "mux"
+    mux: "muxed"
+  client:
+    tcp:
+      # Frontend of local cluster
+      serverAddress: "local-frontend:7233"
+      tls:
+        # Certificate that identifies this host to local Temporal frontend
+        certificatePath: "/tls/internode/tls.crt"
+        # Key this host will use to encrypt local communication to local Temporal frontend
+        keyPath: "/tls/internode/tls.key"
+        # CA for the local Temporal frontend
+        serverCAPath: "/tls/internode/ca.crt"
+        # Servername that should be stamped on the local Temporal frontend's cert
+        serverName: "frontend.temporal.svc.cluster.local"
+  aclPolicy:
+    allowedMethods:
+      adminService:
+        - AddOrUpdateRemoteCluster
+        - RemoveRemoteCluster
+        - DescribeCluster
+        - DescribeMutableState
+        - GetNamespaceReplicationMessages
+        - GetWorkflowExecutionRawHistoryV2
+        - ListClusters
+        - StreamWorkflowReplicationMessages
+        - ReapplyEvents
+        - GetNamespace # for EagerGetNamespace
+outbound:
+  name: "outbound-server"
+  server:
+    tcp:
+      listenAddress: "0.0.0.0:9233"
+      tls:
+        # Certificate that identifies this host to *local* Temporal cluster
+        certificatePath: "/tls/internode/tls.crt"
+        # Key this host will use to encrypt local communication to *local* Temporal cluster
+        keyPath: "/tls/internode/tls.key"
+        # CA for *local* Temporal cluster
+        clientCAPath: "/tls/internode/ca.crt"
+        # We trust our local cluster, we don't need to verify its cert
+        requireClientAuth: false
+  client:
+    # No TLS here because the mux connection is already mTLS
+    type: "mux"
+    mux: "muxed"
+mux:
+  - name: "muxed"
+    mode: "client"
+    num_connections: 10
+    client:
+      serverAddress: "remote-s2s-proxy-endpoint:8233"
+      tls:
+        # Certificate that identifies this host to the remote s2s proxy
+        certificatePath: "/s2c-client-tls/tls.crt"
+        # Key for encrypting the mux tunnel to the remote s2s proxy
+        keyPath: "/s2c-client-tls/tls.key"
+        # CA for the remote s2s proxy
+        serverCAPath: "/s2c-server-tls/tls.crt"
+healthCheck:
+  protocol: "http"
+  listenAddress: "0.0.0.0:8234"
+namespaceNameTranslation:
+  mappings:
+    - localName: "mg-hybrid"
+      remoteName: "mg-hybrid"
+metrics:
+  prometheus:
+    listenAddress: "0.0.0.0:9090"
+# This enables profiling with the default config and port
+profiling: {}

develop/sample-cluster-conn-config.yaml

@@ -10,15 +10,15 @@ clusterConnections:
             keyPath: ""
             remoteCAPath: ""
             caServerName: ""
-            validateClientCA: false
+            verifyCA: false
         tcpServer:
           address: "127.0.0.1:9002"
           tls:
             certificatePath: ""
             keyPath: ""
             remoteCAPath: ""
             caServerName: ""
-            validateClientCA: false
+            verifyCA: false
       clusterInfo:
         serverVersion: "over 9000"
         shardCount: 42
@@ -53,7 +53,7 @@ clusterConnections:
             keyPath: ""
             remoteCAPath: ""
             caServerName: ""
-            validateClientCA: false
+            verifyCA: false
       clusterInfo:
         shardCount: 42
         serverVersion: "v1.22"

encryption/tls.go

@@ -17,12 +17,18 @@ import (
 )
 
 type (
+	// TLSConfig sets TLS options for the proxy's clients and servers
 	TLSConfig struct {
-		CertificatePath  string `yaml:"certificatePath"`
-		KeyPath          string `yaml:"keyPath"`
-		RemoteCAPath     string `yaml:"remoteCAPath"`
-		CAServerName     string `yaml:"caServerName"`
-		ValidateClientCA bool   `yaml:"validateClientCA"`
+		// CertificatePath is the path to the TLS cert that identifies this host
+		CertificatePath string `yaml:"certificatePath"`
+		// KeyPath is the path to the TLS key used to encrypt traffic
+		KeyPath string `yaml:"keyPath"`
+		// RemoteCAPath is the path to the TLS CA cert that is used to verify the remote host's certificate
+		RemoteCAPath string `yaml:"remoteCAPath"`
+		// CAServerName must match against the remote host's CA cert
+		CAServerName string `yaml:"caServerName"`
+		// If set to false, VerifyCA will skip the CA authentication step
+		VerifyCA bool `yaml:"verifyCA"`
 	}
 
 	HttpGetter interface {
@@ -38,102 +44,94 @@ var netClient HttpGetter = &http.Client{
 	Timeout: time.Second * 10,
 }
 
-func GetServerTLSConfig(serverConfig TLSConfig, logger log.Logger) (*tls.Config, error) {
-	certPath := serverConfig.CertificatePath
-	keyPath := serverConfig.KeyPath
-	clientCAPath := serverConfig.RemoteCAPath
-
+func GetServerTLSConfig(serverConfig TLSConfig, logger log.Logger) (tlsConfig *tls.Config, err error) {
 	if !serverConfig.IsEnabled() {
-		return nil, nil
+		return
 	}
 
-	var serverCert *tls.Certificate
-	var clientCAPool *x509.CertPool
-
-	clientAuthType := tls.NoClientCert
-	if serverConfig.ValidateClientCA {
-		clientAuthType = tls.RequireAndVerifyClientCert
-		caCertPool, err := fetchCACert(clientCAPath)
+	tlsConfig = auth.NewEmptyTLSConfig()
+	if serverConfig.VerifyCA {
+		tlsConfig.ClientAuth = tls.RequireAndVerifyClientCert
+		tlsConfig.ClientCAs, err = fetchCACert(serverConfig.RemoteCAPath)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("failed to read CACert from %s: %w", serverConfig.RemoteCAPath, err)
 		}
-		clientCAPool = caCertPool
+	} else {
+		tlsConfig.ClientAuth = tls.NoClientCert
 	}
 
-	if certPath != "" {
-		cert, err := tls.LoadX509KeyPair(certPath, keyPath)
+	if serverConfig.CertificatePath != "" {
+		serverCert, err := tls.LoadX509KeyPair(serverConfig.CertificatePath, serverConfig.KeyPath)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("failed to load cert-key pair (%s, %s): %w",
+				serverConfig.CertificatePath, serverConfig.KeyPath, err)
 		}
-		serverCert = &cert
+		tlsConfig.Certificates = []tls.Certificate{serverCert}
 	}
 
-	c := auth.NewEmptyTLSConfig()
-	c.ClientAuth = clientAuthType
-	c.Certificates = []tls.Certificate{*serverCert}
-	c.ClientCAs = clientCAPool
-	c.GetConfigForClient = func(hello *tls.ClientHelloInfo) (*tls.Config, error) {
-		logger.Info("Received TLS handshake", tag.Address(hello.Conn.RemoteAddr().String()))
+	tlsConfig.GetConfigForClient = func(hello *tls.ClientHelloInfo) (*tls.Config, error) {
+		logger.Info("Received TLS handshake", tag.Address(hello.Conn.RemoteAddr().String()), tag.ServerName(hello.ServerName))
 		return nil, nil
 	}
 
-	c.VerifyPeerCertificate = func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
+	tlsConfig.GetClientCertificate = func(clientInfo *tls.CertificateRequestInfo) (*tls.Certificate, error) {
+		var allCertFailures error
+		for _, cert := range tlsConfig.Certificates {
+			certErr := clientInfo.SupportsCertificate(&cert)
+			if certErr == nil {
+				return &cert, nil
+			}
+			allCertFailures = errors.Join(allCertFailures, certErr)
+		}
+		logger.Warn("Could not match cert request. Check cert failures in error tag", tag.Error(allCertFailures))
+		return nil, allCertFailures
+	}
+
+	tlsConfig.VerifyPeerCertificate = func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
 		if len(rawCerts) == 0 {
-			logger.Info("No client certificate provided")
+			logger.Info("No client certificate provided, so no verification performed")
 		} else {
 			cert, _ := x509.ParseCertificate(rawCerts[0])
 			logger.Info(fmt.Sprintf("Client certificate subject: %s", cert.Subject))
 		}
 		return nil
 	}
 
-	return c, nil
+	return tlsConfig, nil
 }
 
-func GetClientTLSConfig(clientConfig TLSConfig) (*tls.Config, error) {
-	certPath := clientConfig.CertificatePath
-	keyPath := clientConfig.KeyPath
-	caPath := clientConfig.RemoteCAPath
-	serverName := clientConfig.CAServerName
-
+func GetClientTLSConfig(clientConfig TLSConfig) (tlsConfig *tls.Config, err error) {
 	if !clientConfig.IsEnabled() {
-		return nil, nil
+		return
 	}
 
-	var cert *tls.Certificate
-	var caPool *x509.CertPool
-
-	if caPath != "" {
-		caCertPool, err := fetchCACert(caPath)
-		if err != nil {
-			return nil, err
+	tlsConfig = auth.NewEmptyTLSConfig()
+	if !clientConfig.VerifyCA {
+		tlsConfig.InsecureSkipVerify = true
+	} else {
+		if clientConfig.CAServerName == "" || clientConfig.RemoteCAPath == "" {
+			return nil, errors.New("CAServerName and RemoteCAPath must be set when VerifyCA is true")
 		}
-		caPool = caCertPool
+		tlsConfig.ServerName = clientConfig.CAServerName
 	}
 
-	if certPath != "" {
-		myCert, err := tls.LoadX509KeyPair(certPath, keyPath)
+	if clientConfig.RemoteCAPath != "" {
+		caCertPool, err := fetchCACert(clientConfig.RemoteCAPath)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("failed to load CA cert: %w", err)
 		}
-		cert = &myCert
+		tlsConfig.RootCAs = caCertPool
 	}
 
-	// If we are given arguments to verify either server or client, configure TLS
-	if caPool != nil || cert != nil || serverName != "" {
-		enableHostVerification := serverName != "" && caPath != ""
-		tlsConfig := auth.NewTLSConfigForServer(serverName, enableHostVerification)
-		if caPool != nil {
-			tlsConfig.RootCAs = caPool
-		}
-		if cert != nil {
-			tlsConfig.Certificates = []tls.Certificate{*cert}
+	if clientConfig.CertificatePath != "" {
+		myCert, err := tls.LoadX509KeyPair(clientConfig.CertificatePath, clientConfig.KeyPath)
+		if err != nil {
+			return nil, fmt.Errorf("failed to load cert-key pair from path %s: %w", clientConfig.CertificatePath, err)
 		}
-
-		return tlsConfig, nil
+		tlsConfig.Certificates = []tls.Certificate{myCert}
 	}
 
-	return nil, nil
+	return
 }
 
 func fetchCACert(pathOrUrl string) (caPool *x509.CertPool, err error) {