Fix an issue ALPN handshake test does not respect "HTTPS_PROXY" (#27583)

* Fix an issue ALPN handshake test does not respect "HTTPS_PROXY"

* address review comment

* remove simplehttsproxy

* Add context to IsALPNConnUpgradeRequired in ten thousand places

* add goc and dial with context

api/client/alpn_conn_upgrade.go

@@ -26,6 +26,7 @@ import (
 	"net/url"
 	"os"
 	"strings"
+	"time"
 
 	"github.com/gravitational/trace"
 	"github.com/sirupsen/logrus"
@@ -48,19 +49,28 @@ import (
 // In those cases, the Teleport client should make a HTTP "upgrade" call to the
 // 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 {
+func IsALPNConnUpgradeRequired(ctx context.Context, addr string, insecure bool, opts ...DialOption) bool {
 	if result, ok := OverwriteALPNConnUpgradeRequirementByEnv(addr); ok {
 		return result
 	}
 
-	netDialer := &net.Dialer{
-		Timeout: defaults.DefaultIOTimeout,
-	}
+	// Use NewDialer which takes care of ProxyURL, and use a shorter I/O
+	// timeout to avoid blocking caller.
+	baseDialer := NewDialer(
+		ctx,
+		defaults.DefaultIdleTimeout,
+		5*time.Second,
+		append(opts,
+			WithInsecureSkipVerify(insecure),
+			WithALPNConnUpgrade(false),
+		)...,
+	)
+
 	tlsConfig := &tls.Config{
 		NextProtos:         []string{string(constants.ALPNSNIProtocolReverseTunnel)},
 		InsecureSkipVerify: insecure,
 	}
-	testConn, err := tls.DialWithDialer(netDialer, "tcp", addr, tlsConfig)
+	testConn, err := tlsutils.TLSDial(ctx, baseDialer, "tcp", addr, tlsConfig)
 	if err != nil {
 		if isRemoteNoALPNError(err) {
 			logrus.Debugf("ALPN connection upgrade required for %q: %v. No ALPN protocol is negotiated by the server.", addr, true)

api/client/alpn_conn_upgrade_test.go

@@ -32,6 +32,7 @@ import (
 
 	"github.com/gravitational/teleport/api/constants"
 	"github.com/gravitational/teleport/api/fixtures"
+	"github.com/gravitational/teleport/api/testhelpers"
 	"github.com/gravitational/teleport/api/utils/pingconn"
 )
 
@@ -70,10 +71,21 @@ func TestIsALPNConnUpgradeRequired(t *testing.T) {
 		},
 	}
 
+	ctx := context.Background()
+	forwardProxy, forwardProxyURL := mustStartForwardProxy(t)
+
 	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(), test.insecure))
+			t.Run("direct", func(t *testing.T) {
+				require.Equal(t, test.expectedResult, IsALPNConnUpgradeRequired(ctx, server.Addr().String(), test.insecure))
+			})
+
+			t.Run("with ProxyURL", func(t *testing.T) {
+				countBeforeTest := forwardProxy.Count()
+				require.Equal(t, test.expectedResult, IsALPNConnUpgradeRequired(ctx, server.Addr().String(), test.insecure, withProxyURL(forwardProxyURL)))
+				require.Equal(t, countBeforeTest+1, forwardProxy.Count())
+			})
 		})
 	}
 }
@@ -160,24 +172,50 @@ func TestALPNConnUpgradeDialer(t *testing.T) {
 			pool.AddCert(server.Certificate())
 
 			tlsConfig := &tls.Config{RootCAs: pool}
-			preDialer := newDirectDialer(0, 5*time.Second)
-			dialer := newALPNConnUpgradeDialer(preDialer, tlsConfig, test.withPing)
-			conn, err := dialer.DialContext(ctx, "tcp", addr.Host)
-			if test.wantError {
-				require.Error(t, err)
-				return
-			}
-			require.NoError(t, err)
-			defer conn.Close()
+			directDialer := newDirectDialer(0, 5*time.Second)
 
-			data := make([]byte, 100)
-			n, err := conn.Read(data)
-			require.NoError(t, err)
-			require.Equal(t, string(data[:n]), "hello")
+			t.Run("direct", func(t *testing.T) {
+				dialer := newALPNConnUpgradeDialer(directDialer, tlsConfig, test.withPing)
+				conn, err := dialer.DialContext(ctx, "tcp", addr.Host)
+				if test.wantError {
+					require.Error(t, err)
+					return
+				}
+				require.NoError(t, err)
+				defer conn.Close()
+
+				mustReadConnData(t, conn, "hello")
+			})
+
+			t.Run("with ProxyURL", func(t *testing.T) {
+				forwardProxy, forwardProxyURL := mustStartForwardProxy(t)
+				countBeforeTest := forwardProxy.Count()
+
+				proxyURLDialer := newProxyURLDialer(forwardProxyURL, directDialer)
+				dialer := newALPNConnUpgradeDialer(proxyURLDialer, tlsConfig, test.withPing)
+				conn, err := dialer.DialContext(ctx, "tcp", addr.Host)
+				if test.wantError {
+					require.Error(t, err)
+					return
+				}
+				require.NoError(t, err)
+				defer conn.Close()
+
+				mustReadConnData(t, conn, "hello")
+				require.Equal(t, countBeforeTest+1, forwardProxy.Count())
+			})
 		})
 	}
 }
 
+func mustReadConnData(t *testing.T, conn net.Conn, wantText string) {
+	data := make([]byte, len(wantText)*2)
+	n, err := conn.Read(data)
+	require.NoError(t, err)
+	require.Equal(t, len(wantText), n)
+	require.Equal(t, string(data[:n]), wantText)
+}
+
 type mockALPNServer struct {
 	net.Listener
 	cert            tls.Certificate
@@ -273,3 +311,20 @@ func mockConnUpgradeHandler(t *testing.T, upgradeType string, write []byte) http
 		}
 	})
 }
+
+func mustStartForwardProxy(t *testing.T) (*testhelpers.ProxyHandler, *url.URL) {
+	t.Helper()
+
+	listener, err := net.Listen("tcp", "localhost:0")
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		listener.Close()
+	})
+
+	url, err := url.Parse("http://" + listener.Addr().String())
+	require.NoError(t, err)
+
+	handler := &testhelpers.ProxyHandler{}
+	go http.Serve(listener, handler)
+	return handler, url
+}

api/client/contextdialer.go

@@ -48,6 +48,18 @@ type dialConfig struct {
 	// proxyHeaderGetter is used if present to get signed PROXY headers to propagate client's IP.
 	// Used by proxy's web server to make calls on behalf of connected clients.
 	proxyHeaderGetter PROXYHeaderGetter
+	// proxyURLFunc is a function used to get ProxyURL. Defaults to
+	// utils.GetProxyURL if not specified. Currently only used in tests to
+	// overwrite the ProxyURL as httpproxy.FromEnvironment skips localhost
+	// proxies.
+	proxyURLFunc func(dialAddr string) *url.URL
+}
+
+func (c *dialConfig) getProxyURL(dialAddr string) *url.URL {
+	if c.proxyURLFunc != nil {
+		return c.proxyURLFunc(dialAddr)
+	}
+	return utils.GetProxyURL(dialAddr)
 }
 
 // WithInsecureSkipVerify specifies if dialing insecure when using an HTTPS proxy.
@@ -74,6 +86,14 @@ func WithALPNConnUpgradePing(alpnConnUpgradeWithPing bool) DialOption {
 	}
 }
 
+func withProxyURL(proxyURL *url.URL) DialProxyOption {
+	return func(cfg *dialProxyConfig) {
+		cfg.proxyURLFunc = func(_ string) *url.URL {
+			return proxyURL
+		}
+	}
+}
+
 // WithPROXYHeaderGetter provides PROXY headers signer so client's real IP could be propagated.
 // Used by proxy's web server to make calls on behalf of connected clients.
 func WithPROXYHeaderGetter(proxyHeaderGetter PROXYHeaderGetter) DialProxyOption {
@@ -179,7 +199,7 @@ func NewDialer(ctx context.Context, keepAlivePeriod, dialTimeout time.Duration,
 		}
 
 		// Wrap with proxy URL dialer if proxy URL is detected.
-		if proxyURL := utils.GetProxyURL(addr); proxyURL != nil {
+		if proxyURL := cfg.getProxyURL(addr); proxyURL != nil {
 			dialer = newProxyURLDialer(proxyURL, dialer, opts...)
 		}
 
@@ -327,7 +347,7 @@ func newTLSRoutingWithConnUpgradeDialer(ssh ssh.ClientConfig, params connectPara
 				InsecureSkipVerify: insecure,
 				ServerName:         host,
 			},
-			ALPNConnUpgradeRequired: IsALPNConnUpgradeRequired(params.addr, insecure),
+			ALPNConnUpgradeRequired: IsALPNConnUpgradeRequired(ctx, params.addr, insecure),
 			GetClusterCAs: func(_ context.Context) (*x509.CertPool, error) {
 				tlsConfig, err := params.cfg.Credentials[0].TLSConfig()
 				if err != nil {

api/testhelpers/proxy.go

@@ -0,0 +1,100 @@
+// Copyright 2023 Gravitational, Inc
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package testhelpers
+
+import (
+	"io"
+	"net"
+	"net/http"
+	"sync"
+	"time"
+
+	"github.com/gravitational/trace"
+)
+
+// ProxyHandler is a http.Handler that implements a simple HTTP proxy server.
+type ProxyHandler struct {
+	sync.Mutex
+	count int
+}
+
+// ServeHTTP only accepts the CONNECT verb and will tunnel your connection to
+// the specified host. Also tracks the number of connections that it proxies for
+// debugging purposes.
+func (p *ProxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	// Validate http connect parameters.
+	if r.Method != http.MethodConnect {
+		trace.WriteError(w, trace.BadParameter("%v not supported", r.Method))
+		return
+	}
+	if r.Host == "" {
+		trace.WriteError(w, trace.BadParameter("host not set"))
+		return
+	}
+
+	// Dial to the target host, this is done before hijacking the connection to
+	// ensure the target host is accessible.
+	dialer := net.Dialer{}
+	dconn, err := dialer.DialContext(r.Context(), "tcp", r.Host)
+	if err != nil {
+		trace.WriteError(w, err)
+		return
+	}
+	defer dconn.Close()
+
+	// Once the client receives 200 OK, the rest of the data will no longer be
+	// http, but whatever protocol is being tunneled.
+	w.WriteHeader(http.StatusOK)
+
+	// Hijack request so we can get underlying connection.
+	hj, ok := w.(http.Hijacker)
+	if !ok {
+		trace.WriteError(w, trace.AccessDenied("unable to hijack connection"))
+		return
+	}
+	sconn, _, err := hj.Hijack()
+	if err != nil {
+		trace.WriteError(w, err)
+		return
+	}
+	defer sconn.Close()
+
+	// Success, we're proxying data now.
+	p.Lock()
+	p.count++
+	p.Unlock()
+
+	// Copy from src to dst and dst to src.
+	errc := make(chan error, 2)
+	replicate := func(dst io.Writer, src io.Reader) {
+		_, err := io.Copy(dst, src)
+		errc <- err
+	}
+	go replicate(sconn, dconn)
+	go replicate(dconn, sconn)
+
+	// Wait until done, error, or 10 second.
+	select {
+	case <-time.After(10 * time.Second):
+	case <-errc:
+	}
+}
+
+// Count returns the number of requests that have been proxied.
+func (p *ProxyHandler) Count() int {
+	p.Lock()
+	defer p.Unlock()
+	return p.count
+}

integration/helpers/proxy.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	"crypto/tls"
 	"fmt"
-	"io"
 	"net"
 	"net/http"
 	"net/url"
@@ -31,81 +30,12 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/gravitational/teleport/api/fixtures"
+	apitesthelpers "github.com/gravitational/teleport/api/testhelpers"
 	"github.com/gravitational/teleport/lib/utils"
 )
 
-type ProxyHandler struct {
-	sync.Mutex
-	count int
-}
-
-// ServeHTTP only accepts the CONNECT verb and will tunnel your connection to
-// the specified host. Also tracks the number of connections that it proxies for
-// debugging purposes.
-func (p *ProxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	// Validate http connect parameters.
-	if r.Method != http.MethodConnect {
-		trace.WriteError(w, trace.BadParameter("%v not supported", r.Method))
-		return
-	}
-	if r.Host == "" {
-		trace.WriteError(w, trace.BadParameter("host not set"))
-		return
-	}
-
-	// Dial to the target host, this is done before hijacking the connection to
-	// ensure the target host is accessible.
-	dconn, err := net.Dial("tcp", r.Host)
-	if err != nil {
-		trace.WriteError(w, err)
-		return
-	}
-	defer dconn.Close()
-
-	// Once the client receives 200 OK, the rest of the data will no longer be
-	// http, but whatever protocol is being tunneled.
-	w.WriteHeader(http.StatusOK)
-
-	// Hijack request so we can get underlying connection.
-	hj, ok := w.(http.Hijacker)
-	if !ok {
-		trace.WriteError(w, trace.AccessDenied("unable to hijack connection"))
-		return
-	}
-	sconn, _, err := hj.Hijack()
-	if err != nil {
-		trace.WriteError(w, err)
-		return
-	}
-	defer sconn.Close()
-
-	// Success, we're proxying data now.
-	p.Lock()
-	p.count++
-	p.Unlock()
-
-	// Copy from src to dst and dst to src.
-	errc := make(chan error, 2)
-	replicate := func(dst io.Writer, src io.Reader) {
-		_, err := io.Copy(dst, src)
-		errc <- err
-	}
-	go replicate(sconn, dconn)
-	go replicate(dconn, sconn)
-
-	// Wait until done, error, or 10 second.
-	select {
-	case <-time.After(10 * time.Second):
-	case <-errc:
-	}
-}
-
-// Count returns the number of requests that have been proxied.
-func (p *ProxyHandler) Count() int {
-	p.Lock()
-	defer p.Unlock()
-	return p.count
-}
+// ProxyHandler is a http.Handler that implements a simple HTTP proxy server.
+type ProxyHandler = apitesthelpers.ProxyHandler
 
 type ProxyAuthorizer struct {
 	next     http.Handler

lib/auth/register.go

@@ -433,7 +433,7 @@ func proxyJoinServiceConn(params RegisterParams, insecure bool) (*grpc.ClientCon
 	// skip verify as the Proxy server will present its host cert which is not
 	// fully verifiable at this point since the client does not have the host
 	// CAs yet before completing registration.
-	alpnConnUpgrade := client.IsALPNConnUpgradeRequired(getHostAddresses(params)[0], insecure)
+	alpnConnUpgrade := client.IsALPNConnUpgradeRequired(context.TODO(), getHostAddresses(params)[0], insecure)
 	if alpnConnUpgrade && !insecure {
 		tlsConfig.InsecureSkipVerify = true
 		tlsConfig.VerifyConnection = verifyALPNUpgradedConn(params.Clock)

lib/client/api.go

@@ -3266,7 +3266,7 @@ func (tc *TeleportClient) Login(ctx context.Context) (*Key, error) {
 	}
 
 	// Perform the ALPN test once at login.
-	tc.TLSRoutingConnUpgradeRequired = client.IsALPNConnUpgradeRequired(tc.WebProxyAddr, tc.InsecureSkipVerify)
+	tc.TLSRoutingConnUpgradeRequired = client.IsALPNConnUpgradeRequired(ctx, tc.WebProxyAddr, tc.InsecureSkipVerify)
 
 	// Get the SSHLoginFunc that matches client and cluster settings.
 	sshLoginFunc, err := tc.getSSHLoginFunc(pr)
@@ -4715,13 +4715,13 @@ func (tc *TeleportClient) NewKubernetesServiceClient(ctx context.Context, cluste
 // IsALPNConnUpgradeRequiredForWebProxy returns true if connection upgrade is
 // required for provided addr. The provided address must be a web proxy
 // address.
-func (tc *TeleportClient) IsALPNConnUpgradeRequiredForWebProxy(proxyAddr string) bool {
+func (tc *TeleportClient) IsALPNConnUpgradeRequiredForWebProxy(ctx context.Context, proxyAddr string) bool {
 	// Use cached value.
 	if proxyAddr == tc.WebProxyAddr {
 		return tc.TLSRoutingConnUpgradeRequired
 	}
 	// Do a test for other proxy addresses.
-	return client.IsALPNConnUpgradeRequired(proxyAddr, tc.InsecureSkipVerify)
+	return client.IsALPNConnUpgradeRequired(ctx, proxyAddr, tc.InsecureSkipVerify)
 }
 
 // RootClusterCACertPool returns a *x509.CertPool with the root cluster CA.

lib/client/client.go

@@ -1143,7 +1143,7 @@ func (proxy *ProxyClient) ConnectToAuthServiceThroughALPNSNIProxy(ctx context.Co
 		},
 		ALPNSNIAuthDialClusterName: clusterName,
 		CircuitBreakerConfig:       breaker.NoopBreakerConfig(),
-		ALPNConnUpgradeRequired:    proxy.teleportClient.IsALPNConnUpgradeRequiredForWebProxy(proxyAddr),
+		ALPNConnUpgradeRequired:    proxy.teleportClient.IsALPNConnUpgradeRequiredForWebProxy(ctx, proxyAddr),
 		PROXYHeaderGetter:          CreatePROXYHeaderGetter(ctx, proxy.teleportClient.PROXYSigner),
 		InsecureAddressDiscovery:   proxy.teleportClient.InsecureSkipVerify,
 	})

lib/reversetunnel/agentpool.go

@@ -733,7 +733,7 @@ func (c *agentPoolRuntimeConfig) updateRemote(ctx context.Context, addr *utils.N
 
 	c.remoteTLSRoutingEnabled = tlsRoutingEnabled
 	if c.remoteTLSRoutingEnabled {
-		c.tlsRoutingConnUpgradeRequired = client.IsALPNConnUpgradeRequired(addr.Addr, lib.IsInsecureDevMode())
+		c.tlsRoutingConnUpgradeRequired = client.IsALPNConnUpgradeRequired(ctx, addr.Addr, lib.IsInsecureDevMode())
 		logrus.Debugf("ALPN upgrade required for remote %v: %v", addr.Addr, c.tlsRoutingConnUpgradeRequired)
 	}
 	return nil
@@ -766,7 +766,7 @@ func (c *agentPoolRuntimeConfig) update(ctx context.Context, netConfig types.Clu
 	if c.proxyListenerMode == types.ProxyListenerMode_Multiplex && oldProxyListenerMode != c.proxyListenerMode {
 		addr, _, err := resolver(ctx)
 		if err == nil {
-			c.tlsRoutingConnUpgradeRequired = client.IsALPNConnUpgradeRequired(addr.Addr, lib.IsInsecureDevMode())
+			c.tlsRoutingConnUpgradeRequired = client.IsALPNConnUpgradeRequired(ctx, addr.Addr, lib.IsInsecureDevMode())
 		} else {
 			logrus.WithError(err).Warnf("Failed to resolve addr.")
 		}

lib/reversetunnelclient/transport.go

@@ -91,7 +91,7 @@ func (t *TunnelAuthDialer) DialContext(ctx context.Context, _, _ string) (net.Co
 				InsecureSkipVerify: t.InsecureSkipTLSVerify,
 			},
 			DialTimeout:             t.ClientConfig.Timeout,
-			ALPNConnUpgradeRequired: client.IsALPNConnUpgradeRequired(addr.Addr, t.InsecureSkipTLSVerify),
+			ALPNConnUpgradeRequired: client.IsALPNConnUpgradeRequired(ctx, addr.Addr, t.InsecureSkipTLSVerify),
 			GetClusterCAs:           client.ClusterCAsFromCertPool(t.ClusterCAs),
 		}))
 	}

lib/srv/alpnproxy/local_proxy_config_opt.go

@@ -42,7 +42,7 @@ type GetClusterCACertPoolFunc func(ctx context.Context) (*x509.CertPool, error)
 // already been set.
 func WithALPNConnUpgradeTest(ctx context.Context, getClusterCertPool GetClusterCACertPoolFunc) LocalProxyConfigOpt {
 	return func(config *LocalProxyConfig) error {
-		config.ALPNConnUpgradeRequired = client.IsALPNConnUpgradeRequired(config.RemoteProxyAddr, config.InsecureSkipVerify)
+		config.ALPNConnUpgradeRequired = client.IsALPNConnUpgradeRequired(ctx, config.RemoteProxyAddr, config.InsecureSkipVerify)
 		return trace.Wrap(WithClusterCAsIfConnUpgrade(ctx, getClusterCertPool)(config))
 	}
 }

lib/teleterm/clusters/cluster_auth.go

@@ -46,7 +46,7 @@ func (c *Cluster) SyncAuthPreference(ctx context.Context) (*webclient.WebConfigA
 	// Do the ALPN handshake test to decide if connection upgrades are required
 	// for TLS Routing. Only do the test once Ping verifies the cluster is
 	// reachable.
-	c.clusterClient.TLSRoutingConnUpgradeRequired = apiclient.IsALPNConnUpgradeRequired(c.clusterClient.WebProxyAddr, c.clusterClient.InsecureSkipVerify)
+	c.clusterClient.TLSRoutingConnUpgradeRequired = apiclient.IsALPNConnUpgradeRequired(ctx, c.clusterClient.WebProxyAddr, c.clusterClient.InsecureSkipVerify)
 
 	if err := c.clusterClient.SaveProfile(false); err != nil {
 		return nil, trace.Wrap(err)

lib/teleterm/clusters/storage.go

@@ -163,7 +163,7 @@ func (s *Storage) addCluster(ctx context.Context, dir, webProxyAddress string) (
 	// Do the ALPN handshake test to decide if connection upgrades are required
 	// for TLS Routing. Only do the test once Ping verifies the cluster is
 	// reachable.
-	clusterClient.TLSRoutingConnUpgradeRequired = apiclient.IsALPNConnUpgradeRequired(webProxyAddress, s.InsecureSkipVerify)
+	clusterClient.TLSRoutingConnUpgradeRequired = apiclient.IsALPNConnUpgradeRequired(ctx, webProxyAddress, s.InsecureSkipVerify)
 
 	if err := clusterClient.SaveProfile(false); err != nil {
 		return nil, trace.Wrap(err)

tool/tsh/common/proxy.go

@@ -258,7 +258,7 @@ func dialSSHProxy(ctx context.Context, tc *libclient.TeleportClient, sp sshProxy
 				InsecureSkipVerify: tc.InsecureSkipVerify,
 				ServerName:         sp.proxyHost,
 			},
-			ALPNConnUpgradeRequired: tc.IsALPNConnUpgradeRequiredForWebProxy(remoteProxyAddr),
+			ALPNConnUpgradeRequired: tc.IsALPNConnUpgradeRequiredForWebProxy(ctx, remoteProxyAddr),
 		})
 
 	default: