ALPN handshake test improvements (#23348) (#23798)

* ALPN connect test improvements

* fix typos

* remove extra period

* simplify error check

* fix func name typo

* add a few comments to clarify things

* fix lint

api/defaults/defaults.go

@@ -145,4 +145,15 @@ const (
 const (
 	// TunnelPublicAddrEnvar optionally specifies the alternative reverse tunnel address.
 	TunnelPublicAddrEnvar = "TELEPORT_TUNNEL_PUBLIC_ADDR"
+
+	// TLSRoutingConnUpgradeEnvVar overwrites the test result for deciding if
+	// ALPN connection upgrade is required.
+	//
+	// Sample values:
+	// true
+	// <some.cluster.com>=yes,<another.cluster.com>=no
+	// 0,<some.cluster.com>=1
+	//
+	// TODO(greedy52) DELETE IN 15.0
+	TLSRoutingConnUpgradeEnvVar = "TELEPORT_TLS_ROUTING_CONN_UPGRADE"
 )

api/profile/profile.go

@@ -85,6 +85,13 @@ type Profile struct {
 	// all proxy services are exposed on a single TLS listener (Proxy Web Listener).
 	TLSRoutingEnabled bool `yaml:"tls_routing_enabled,omitempty"`
 
+	// TLSRoutingConnUpgradeRequired indicates that ALPN connection upgrades
+	// are required for making TLS routing requests.
+	//
+	// Note that this is applicable to the Proxy's Web port regardless of
+	// whether the Proxy is in single-port or multi-port configuration.
+	TLSRoutingConnUpgradeRequired bool `yaml:"tls_routing_conn_upgrade_required,omitempty"`
+
 	// AuthConnector (like "google", "passwordless").
 	// Equivalent to the --auth tsh flag.
 	AuthConnector string `yaml:"auth_connector,omitempty"`

lib/client/api.go

@@ -73,6 +73,7 @@ import (
 	"github.com/gravitational/teleport/lib/services"
 	"github.com/gravitational/teleport/lib/session"
 	"github.com/gravitational/teleport/lib/shell"
+	"github.com/gravitational/teleport/lib/srv/alpnproxy"
 	alpncommon "github.com/gravitational/teleport/lib/srv/alpnproxy/common"
 	"github.com/gravitational/teleport/lib/sshutils"
 	"github.com/gravitational/teleport/lib/sshutils/scp"
@@ -370,6 +371,13 @@ type Config struct {
 	// all proxy services are exposed on a single TLS listener (Proxy Web Listener).
 	TLSRoutingEnabled bool
 
+	// TLSRoutingConnUpgradeRequired indicates that ALPN connection upgrades
+	// are required for making TLS routing requests.
+	//
+	// Note that this is applicable to the Proxy's Web port regardless of
+	// whether the Proxy is in single-port or multi-port configuration.
+	TLSRoutingConnUpgradeRequired bool
+
 	// Reason is a reason attached to started sessions meant to describe their intent.
 	Reason string
 
@@ -585,6 +593,7 @@ func (c *Config) LoadProfile(ps ProfileStore, proxyAddr string) error {
 	c.MySQLProxyAddr = profile.MySQLProxyAddr
 	c.MongoProxyAddr = profile.MongoProxyAddr
 	c.TLSRoutingEnabled = profile.TLSRoutingEnabled
+	c.TLSRoutingConnUpgradeRequired = profile.TLSRoutingConnUpgradeRequired
 	c.KeysDir = profile.Dir
 	c.AuthConnector = profile.AuthConnector
 	c.LoadAllCAs = profile.LoadAllCAs
@@ -604,6 +613,10 @@ func (c *Config) LoadProfile(ps ProfileStore, proxyAddr string) error {
 		log.Warnf("Unable to parse dynamic port forwarding in user profile: %v.", err)
 	}
 
+	if required, ok := alpnproxy.OverwriteALPNConnUpgradeRequirementByEnv(c.WebProxyAddr); ok {
+		c.TLSRoutingConnUpgradeRequired = required
+	}
+	log.Infof("ALPN connection upgrade required for %q: %v.", c.WebProxyAddr, c.TLSRoutingConnUpgradeRequired)
 	return nil
 }
 
@@ -615,20 +628,21 @@ func (c *Config) SaveProfile(makeCurrent bool) error {
 	}
 
 	p := &profile.Profile{
-		Username:          c.Username,
-		WebProxyAddr:      c.WebProxyAddr,
-		SSHProxyAddr:      c.SSHProxyAddr,
-		KubeProxyAddr:     c.KubeProxyAddr,
-		PostgresProxyAddr: c.PostgresProxyAddr,
-		MySQLProxyAddr:    c.MySQLProxyAddr,
-		MongoProxyAddr:    c.MongoProxyAddr,
-		ForwardedPorts:    c.LocalForwardPorts.String(),
-		SiteName:          c.SiteName,
-		TLSRoutingEnabled: c.TLSRoutingEnabled,
-		AuthConnector:     c.AuthConnector,
-		MFAMode:           c.AuthenticatorAttachment.String(),
-		LoadAllCAs:        c.LoadAllCAs,
-		PrivateKeyPolicy:  c.PrivateKeyPolicy,
+		Username:                      c.Username,
+		WebProxyAddr:                  c.WebProxyAddr,
+		SSHProxyAddr:                  c.SSHProxyAddr,
+		KubeProxyAddr:                 c.KubeProxyAddr,
+		PostgresProxyAddr:             c.PostgresProxyAddr,
+		MySQLProxyAddr:                c.MySQLProxyAddr,
+		MongoProxyAddr:                c.MongoProxyAddr,
+		ForwardedPorts:                c.LocalForwardPorts.String(),
+		SiteName:                      c.SiteName,
+		TLSRoutingEnabled:             c.TLSRoutingEnabled,
+		TLSRoutingConnUpgradeRequired: c.TLSRoutingConnUpgradeRequired,
+		AuthConnector:                 c.AuthConnector,
+		MFAMode:                       c.AuthenticatorAttachment.String(),
+		LoadAllCAs:                    c.LoadAllCAs,
+		PrivateKeyPolicy:              c.PrivateKeyPolicy,
 	}
 
 	if err := c.ClientStore.SaveProfile(p, makeCurrent); err != nil {
@@ -829,6 +843,16 @@ func (c *Config) DatabaseProxyHostPort(db tlsca.RouteToDatabase) (string, int) {
 	return c.WebProxyHostPort()
 }
 
+// DoesDatabaseUseWebProxyHostPort returns true if database is using web port.
+//
+// This is useful for deciding whether local proxy is required when web port is
+// behind a load balancer.
+func (c *Config) DoesDatabaseUseWebProxyHostPort(db tlsca.RouteToDatabase) bool {
+	dbHost, dbPort := c.DatabaseProxyHostPort(db)
+	webHost, webPort := c.WebProxyHostPort()
+	return dbHost == webHost && dbPort == webPort
+}
+
 // GetKubeTLSServerName returns k8s server name used in KUBECONFIG to leverage TLS Routing.
 func GetKubeTLSServerName(k8host string) string {
 	isIPFormat := net.ParseIP(k8host) != nil
@@ -3030,6 +3054,9 @@ func (tc *TeleportClient) Login(ctx context.Context) (*Key, error) {
 		return nil, trace.Wrap(err)
 	}
 
+	// Perform the ALPN test once at login.
+	tc.TLSRoutingConnUpgradeRequired = alpnproxy.IsALPNConnUpgradeRequired(tc.WebProxyAddr, tc.InsecureSkipVerify)
+
 	// Get the SSHLoginFunc that matches client and cluster settings.
 	sshLoginFunc, err := tc.getSSHLoginFunc(pr)
 	if err != nil {

lib/srv/alpnproxy/conn_upgrade.go

@@ -20,9 +20,11 @@ import (
 	"bufio"
 	"context"
 	"crypto/tls"
+	"errors"
 	"net"
 	"net/http"
 	"net/url"
+	"os"
 	"strings"
 
 	"github.com/gravitational/trace"
@@ -31,6 +33,7 @@ import (
 	"github.com/gravitational/teleport"
 	apiclient "github.com/gravitational/teleport/api/client"
 	"github.com/gravitational/teleport/api/defaults"
+	"github.com/gravitational/teleport/api/utils"
 	"github.com/gravitational/teleport/lib/srv/alpnproxy/common"
 )
 
@@ -46,6 +49,10 @@ import (
 // Proxy Service to establish a tunnel for the originally planned traffic to
 // preserve the ALPN and SNI information.
 func IsALPNConnUpgradeRequired(addr string, insecure bool) bool {
+	if result, ok := OverwriteALPNConnUpgradeRequirementByEnv(addr); ok {
+		return result
+	}
+
 	netDialer := &net.Dialer{
 		Timeout: defaults.DefaultIOTimeout,
 	}
@@ -55,14 +62,14 @@ func IsALPNConnUpgradeRequired(addr string, insecure bool) bool {
 	}
 	testConn, err := tls.DialWithDialer(netDialer, "tcp", addr, tlsConfig)
 	if err != nil {
-		// If dialing TLS fails for any reason, we assume connection upgrade is
-		// not required so it will fallback to original connection method.
-		//
-		// This includes handshake failures where both peers support ALPN but
-		// no common protocol is getting negotiated. We may have to revisit
-		// this situation or make it configurable if we have to get through a
-		// middleman with this behavior. For now, we are only interested in the
-		// case where the middleman does not support ALPN.
+		if isRemoteNoALPNError(err) {
+			logrus.Debugf("ALPN connection upgrade required for %q: %v. No ALPN protocol is negotiated by the server.", addr, true)
+			return true
+		}
+
+		// If dialing TLS fails for any other reason, we assume connection
+		// upgrade is not required so it will fallback to original connection
+		// method.
 		logrus.Infof("ALPN connection upgrade test failed for %q: %v.", addr, err)
 		return false
 	}
@@ -75,6 +82,66 @@ func IsALPNConnUpgradeRequired(addr string, insecure bool) bool {
 	return result
 }
 
+func isRemoteNoALPNError(err error) bool {
+	var opErr *net.OpError
+	return errors.As(err, &opErr) && opErr.Op == "remote error" && strings.Contains(opErr.Err.Error(), "tls: no application protocol")
+}
+
+// OverwriteALPNConnUpgradeRequirementByEnv overwrites ALPN connection upgrade
+// requirement by environment variable.
+//
+// TODO(greedy52) DELETE in 15.0
+func OverwriteALPNConnUpgradeRequirementByEnv(addr string) (bool, bool) {
+	envValue := os.Getenv(defaults.TLSRoutingConnUpgradeEnvVar)
+	if envValue == "" {
+		return false, false
+	}
+	result := isALPNConnUpgradeRequiredByEnv(addr, envValue)
+	logrus.WithField(defaults.TLSRoutingConnUpgradeEnvVar, envValue).Debugf("ALPN connection upgrade required for %q: %v.", addr, result)
+	return result, true
+}
+
+// isALPNConnUpgradeRequiredByEnv checks if ALPN connection upgrade is required
+// based on provided env value.
+//
+// The env value should contain a list of conditions separated by either ';' or
+// ','. A condition is in format of either '<addr>=<bool>' or '<bool>'. The
+// former specifies the upgrade requirement for a specific address and the
+// later specifies the upgrade requirement for all other addresses. By default,
+// upgrade is not required if target is not specified in the env value.
+//
+// Sample values:
+// true
+// <some.cluster.com>=yes,<another.cluster.com>=no
+// 0,<some.cluster.com>=1
+func isALPNConnUpgradeRequiredByEnv(addr, envValue string) bool {
+	tokens := strings.FieldsFunc(envValue, func(r rune) bool {
+		return r == ';' || r == ','
+	})
+
+	var upgradeRequiredForAll bool
+	for _, token := range tokens {
+		switch {
+		case strings.ContainsRune(token, '='):
+			if _, boolText, ok := strings.Cut(token, addr+"="); ok {
+				upgradeRequiredForAddr, err := utils.ParseBool(boolText)
+				if err != nil {
+					logrus.Debugf("Failed to parse %v: %v", envValue, err)
+				}
+				return upgradeRequiredForAddr
+			}
+
+		default:
+			if boolValue, err := utils.ParseBool(token); err != nil {
+				logrus.Debugf("Failed to parse %v: %v", envValue, err)
+			} else {
+				upgradeRequiredForAll = boolValue
+			}
+		}
+	}
+	return upgradeRequiredForAll
+}
+
 // alpnConnUpgradeDialer makes an "HTTP" upgrade call to the Proxy Service then
 // tunnels the connection with this connection upgrade.
 type alpnConnUpgradeDialer struct {

lib/srv/alpnproxy/conn_upgrade_test.go

@@ -44,29 +44,82 @@ func TestIsALPNConnUpgradeRequired(t *testing.T) {
 	tests := []struct {
 		name           string
 		serverProtos   []string
+		insecure       bool
 		expectedResult bool
 	}{
 		{
-			name:           "upgrade required",
+			name:           "upgrade required (handshake success)",
 			serverProtos:   nil, // Use nil for NextProtos to simulate no ALPN support.
+			insecure:       true,
 			expectedResult: true,
 		},
 		{
 			name:           "upgrade not required (proto negotiated)",
 			serverProtos:   []string{string(common.ProtocolReverseTunnel)},
+			insecure:       true,
 			expectedResult: false,
 		},
 		{
-			name:           "upgrade not required (handshake error)",
+			name:           "upgrade required (handshake with no ALPN error)",
 			serverProtos:   []string{"unknown"},
+			insecure:       true,
+			expectedResult: true,
+		},
+		{
+			name:           "upgrade not required (other handshake error)",
+			serverProtos:   []string{string(common.ProtocolReverseTunnel)},
+			insecure:       false, // to cause handshake error
 			expectedResult: false,
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			server := mustStartMockALPNServer(t, test.serverProtos)
-			require.Equal(t, test.expectedResult, IsALPNConnUpgradeRequired(server.Addr().String(), true))
+			require.Equal(t, test.expectedResult, IsALPNConnUpgradeRequired(server.Addr().String(), test.insecure))
+		})
+	}
+}
+
+func TestIsALPNConnUpgradeRequiredByEnv(t *testing.T) {
+	t.Parallel()
+
+	addr := "example.teleport.com:443"
+	tests := []struct {
+		name     string
+		envValue string
+		require  require.BoolAssertionFunc
+	}{
+		{
+			name:     "upgraded required (for all addr)",
+			envValue: "yes",
+			require:  require.True,
+		},
+		{
+			name:     "upgraded required (for target addr)",
+			envValue: "0;example.teleport.com:443=1",
+			require:  require.True,
+		},
+		{
+			name:     "upgraded not required (for all addr)",
+			envValue: "false",
+			require:  require.False,
+		},
+		{
+			name:     "upgraded not required (no addr match)",
+			envValue: "another.teleport.com:443=true",
+			require:  require.False,
+		},
+		{
+			name:     "upgraded not required (for target addr)",
+			envValue: "another.teleport.com:443=true,example.teleport.com:443=false",
+			require:  require.False,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			test.require(t, isALPNConnUpgradeRequiredByEnv(addr, test.envValue))
 		})
 	}
 }

lib/srv/alpnproxy/local_proxy_config_opt.go

@@ -42,12 +42,27 @@ type GetClusterCACertPoolFunc func(ctx context.Context) (*x509.CertPool, error)
 func WithALPNConnUpgradeTest(ctx context.Context, getClusterCertPool GetClusterCACertPoolFunc) LocalProxyConfigOpt {
 	return func(config *LocalProxyConfig) error {
 		config.ALPNConnUpgradeRequired = IsALPNConnUpgradeRequired(config.RemoteProxyAddr, config.InsecureSkipVerify)
+		return trace.Wrap(WithClusterCAsIfConnUpgrade(ctx, getClusterCertPool)(config))
+	}
+}
+
+// WithClusterCAsIfConnUpgrade is a LocalProxyConfigOpt that fetches the
+// cluster CAs when ALPN connection upgrades are required.
+func WithClusterCAsIfConnUpgrade(ctx context.Context, getClusterCertPool GetClusterCACertPoolFunc) LocalProxyConfigOpt {
+	return func(config *LocalProxyConfig) error {
 		if !config.ALPNConnUpgradeRequired {
 			return nil
 		}
 
 		// If ALPN connection upgrade is required, explicitly use the cluster
 		// CAs since the tunneled TLS routing connection serves the Host cert.
+		return trace.Wrap(WithClusterCAs(ctx, getClusterCertPool)(config))
+	}
+}
+
+// WithClusterCAs is a LocalProxyConfigOpt that fetches the cluster CAs.
+func WithClusterCAs(ctx context.Context, getClusterCertPool GetClusterCACertPoolFunc) LocalProxyConfigOpt {
+	return func(config *LocalProxyConfig) error {
 		clusterCAs, err := getClusterCertPool(ctx)
 		if err != nil {
 			return trace.Wrap(err)

tool/tsh/app.go

@@ -37,7 +37,6 @@ import (
 	"github.com/gravitational/teleport/lib/asciitable"
 	"github.com/gravitational/teleport/lib/client"
 	"github.com/gravitational/teleport/lib/defaults"
-	"github.com/gravitational/teleport/lib/srv/alpnproxy"
 	"github.com/gravitational/teleport/lib/tlsca"
 	"github.com/gravitational/teleport/lib/utils"
 )
@@ -194,7 +193,7 @@ func onAppLogin(cf *CLIConf) error {
 }
 
 func localProxyRequiredForApp(tc *client.TeleportClient) bool {
-	return alpnproxy.IsALPNConnUpgradeRequired(tc.WebProxyAddr, tc.InsecureSkipVerify)
+	return tc.TLSRoutingConnUpgradeRequired
 }
 
 // appLoginTpl is the message that gets printed to a user upon successful login

tool/tsh/app_aws.go

@@ -287,7 +287,7 @@ func (a *awsApp) startLocalALPNProxy(port string) error {
 	a.localALPNProxy, err = alpnproxy.NewLocalProxy(
 		makeBasicLocalProxyConfig(a.cf, tc, listener),
 		alpnproxy.WithClientCerts(appCerts),
-		alpnproxy.WithALPNConnUpgradeTest(a.cf.Context, tc.RootClusterCACertPool),
+		alpnproxy.WithClusterCAsIfConnUpgrade(a.cf.Context, tc.RootClusterCACertPool),
 		alpnproxy.WithHTTPMiddleware(&alpnproxy.AWSAccessMiddleware{
 			AWSCredentials: cred,
 		}),