Respect HTTP_PROXY/HTTPS_PROXY

[atburke]

Fixes #8108.

[russjones]

@atburke This needs test coverage.

[jimbishopp]

I tested this locally as well. tsh login seems to work with and without proxy environment variables set.

[russjones]

@atburke Take a look at the comments (and diff) from @johns-carta in #8108.

[johns-carta]

Sorry, that fix is incomplete, this is going to dial directly , again, instead of through the proxy:
https://github.com/gravitational/teleport/blob/atburke/tsh-https-proxy/lib/client/client.go#L718

you need to implement a CONNECT proxy dialer in api/client/contextdialer.go and pass that as a dialer (instead of Addr) to ConnectToAuthServiceThroughALPNSNIProxy

There are almost certainly other similar cases littered about

I suggest testing this with a deployment that is only accessible via HTTP CONNECT, because if the dev machine or test machine can directly connect to the Teleport cluster it will look successful when it is in fact, not.

[codingllama]

Withdrawing approval, as it seems we didn't quite hit the mark here yet. @atburke, let me know when you are ready for review again.

[johns-carta]

@atburke This code added to this PR works (I've done an end-to-end test) but you almost certainly don't want to integrate it as-is. I'm relatively sure authConnect shouldn't be the one creating the dialer & I don't feel like tracking down where the right place is but something that looks like this.

diff --git a/api/client/client.go b/api/client/client.go
index c2ec9efe6..c0f32325e 100644
--- a/api/client/client.go
+++ b/api/client/client.go
@@ -34,6 +34,7 @@ import (
 	"github.com/gravitational/teleport/api/types"
 	"github.com/gravitational/teleport/api/types/events"
 	"github.com/gravitational/teleport/api/utils"
+	utilproxy "github.com/gravitational/teleport/lib/utils/proxy"
 
 	"github.com/golang/protobuf/ptypes/empty"
 	"github.com/gravitational/trace"
@@ -284,6 +285,12 @@ type connectParams struct {
 // authConnect connects to the Teleport Auth Server directly.
 func authConnect(ctx context.Context, params connectParams) (*Client, error) {
 	dialer := NewDirectDialer(params.cfg.KeepAlivePeriod, params.cfg.DialTimeout)
+	proxyAddr := utilproxy.GetProxyAddress(params.addr)
+	if proxyAddr != "" {
+		dialer = ContextDialerFunc(func(ctx context.Context, network, _ string) (conn net.Conn, err error) {
+			return utilproxy.DialProxy(ctx, proxyAddr, params.addr)
+		})
+	}
 	clt := newClient(params.cfg, dialer, params.tlsConfig)
 	if err := clt.dialGRPC(ctx, params.addr); err != nil {
 		return nil, trace.Wrap(err, "failed to connect to addr %v as an auth server", params.addr)
diff --git a/lib/auth/clt.go b/lib/auth/clt.go
index 49a5f23b8..5bc6a672b 100644
--- a/lib/auth/clt.go
+++ b/lib/auth/clt.go
@@ -43,6 +43,7 @@ import (
 	"github.com/gravitational/teleport/lib/services"
 	"github.com/gravitational/teleport/lib/session"
 	"github.com/gravitational/teleport/lib/utils"
+	utilproxy "github.com/gravitational/teleport/lib/utils/proxy"
 
 	"github.com/gravitational/roundtrip"
 	"github.com/gravitational/trace"
@@ -131,7 +132,12 @@ func NewHTTPClient(cfg client.Config, tls *tls.Config, params ...roundtrip.Clien
 		contextDialer := client.NewDirectDialer(cfg.KeepAlivePeriod, cfg.DialTimeout)
 		dialer = client.ContextDialerFunc(func(ctx context.Context, network, _ string) (conn net.Conn, err error) {
 			for _, addr := range cfg.Addrs {
-				conn, err = contextDialer.DialContext(ctx, network, addr)
+				proxyAddr := utilproxy.GetProxyAddress(addr)
+				if proxyAddr == "" {
+					conn, err = contextDialer.DialContext(ctx, network, addr)
+				} else {
+					conn, err = utilproxy.DialProxy(ctx, proxyAddr, addr)
+				}
 				if err == nil {
 					return conn, nil
 				}
diff --git a/lib/utils/proxy/proxy.go b/lib/utils/proxy/proxy.go
index 6aba7c404..88e2c20dd 100644
--- a/lib/utils/proxy/proxy.go
+++ b/lib/utils/proxy/proxy.go
@@ -311,6 +311,10 @@ func dialProxy(ctx context.Context, proxyAddr string, addr string) (net.Conn, er
 	}, nil
 }
 
+func DialProxy(ctx context.Context, proxyAddr string, addr string) (net.Conn, error) {
+	return dialProxy(ctx, proxyAddr, addr)
+}
+
 func getProxyAddress(addr string) string {
 	envs := []string{
 		teleport.HTTPSProxy,
@@ -341,6 +345,10 @@ func getProxyAddress(addr string) string {
 	return ""
 }
 
+func GetProxyAddress(addr string) string {
+	return getProxyAddress(addr)
+}
+
 // bufferedConn is used when part of the data on a connection has already been
 // read by a *bufio.Reader. Reads will first try and read from the
 // *bufio.Reader and when everything has been read, reads will go to the

[russjones]

@atburke You have three things you want to test.

1. Regular Teleport with services running on different ports.
2. TLS routing.
3. NO_PROXY support. We have a customer that wants to send all HTTP traffic through their proxy but SSH traffic should not go through the proxy (so this is mode (1) above). So I imagine what you want to test is something like NO_PROXY=localhost:3023 (maybe also 3024, I forget?) will cause only HTTP traffic to be proxied, not SSH traffic.

[r0mant]

How can conf be nil here?

[atburke]

conf can be nil when the caller doesn't use proxy.WithTLSConfig() to use a custom tls config.

[r0mant]

To be honest, I'm not sure I fully understand this tlsConfig logic. Why are we setting NextProtos to ProtocolReverseTunnel here in what (I think) is supposed to be a generic proxy, but only in the case when tlsConfig wasn't passed in?

@smallinsky Can you take a look at these changes too pls - do you remember why we're setting ProtocolReverseTunnel here? Is this proxy supposed to be used by the agents only? Just want to make sure we don't break any scenarios.

Is this proxy is now used by both reverse tunnel agents and tsh, I wonder if we should make the TLS config mandatory and have the caller pass it appropriately.

[atburke]

ProtocolReverseTunnel is the default from before the tls config was injected. As long as it doesn't break anything, I think making tls config mandatory is the way to go.

[smallinsky]

do you remember why we're setting ProtocolReverseTunnel here?

The name of the dialer is misleading. It is used in TunnelAuthDialer and Reverse tunnel Agent call so under the hood it is Reverse Tunnel Dialer where a connection is established to Reverse tunnel service thus in case of TLSRouting mode the alpncommon.ProtocolReverseTunnel protocol is set.

[r0mant]

How can conf be nil here?

[r0mant]

Same q as above.

[r0mant]

Same q as above.