From 72459fbea9fd124e7d716812198ebc55dcb14866 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Mon, 7 Feb 2022 15:21:09 -0800 Subject: [PATCH 01/29] Add http proxy to web clients --- api/client/webclient/webclient.go | 1 + lib/client/https_client.go | 2 ++ 2 files changed, 3 insertions(+) diff --git a/api/client/webclient/webclient.go b/api/client/webclient/webclient.go index b82e16f7ac9fe..d877609c04240 100644 --- a/api/client/webclient/webclient.go +++ b/api/client/webclient/webclient.go @@ -46,6 +46,7 @@ func newWebClient(insecure bool, pool *x509.CertPool) *http.Client { RootCAs: pool, InsecureSkipVerify: insecure, }, + Proxy: http.ProxyFromEnvironment, }, } } diff --git a/lib/client/https_client.go b/lib/client/https_client.go index 4d1c94807dcec..8a95b3cf47497 100644 --- a/lib/client/https_client.go +++ b/lib/client/https_client.go @@ -41,6 +41,7 @@ func NewInsecureWebClient() *http.Client { return &http.Client{ Transport: &http.Transport{ TLSClientConfig: tlsConfig, + Proxy: http.ProxyFromEnvironment, }, } } @@ -54,6 +55,7 @@ func newClientWithPool(pool *x509.CertPool) *http.Client { return &http.Client{ Transport: &http.Transport{ TLSClientConfig: tlsConfig, + Proxy: http.ProxyFromEnvironment, }, } } From 05fa8b509e151ce67fe0bd70748a23d0ad627270 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Tue, 8 Feb 2022 12:45:15 -0800 Subject: [PATCH 02/29] Add tests --- api/client/webclient/webclient_test.go | 10 ++++++ lib/client/https_client_test.go | 42 ++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) create mode 100644 lib/client/https_client_test.go diff --git a/api/client/webclient/webclient_test.go b/api/client/webclient/webclient_test.go index 556d6cab96478..d3c9ba34fee70 100644 --- a/api/client/webclient/webclient_test.go +++ b/api/client/webclient/webclient_test.go @@ -22,6 +22,7 @@ import ( "net" "net/http" "net/http/httptest" + "os" "testing" "github.com/stretchr/testify/require" @@ -289,3 +290,12 @@ func TestExtract(t *testing.T) { }) } } + +func TestNewWebClientRespectHTTPProxy(t *testing.T) { + os.Setenv("HTTPS_PROXY", "localhost:9999") + defer os.Unsetenv("HTTPS_PROXY") + client := newWebClient(false, nil) + _, err := client.Get("https://example.com") + // Client should try to proxy through nonexistent server at localhost. + require.Error(t, err) +} diff --git a/lib/client/https_client_test.go b/lib/client/https_client_test.go new file mode 100644 index 0000000000000..96bf5d1036117 --- /dev/null +++ b/lib/client/https_client_test.go @@ -0,0 +1,42 @@ +/* +Copyright 2022 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 client + +import ( + "os" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestNewInsecureWebClientHTTPProxy(t *testing.T) { + os.Setenv("HTTPS_PROXY", "localhost:9999") + defer os.Unsetenv("HTTPS_PROXY") + client := NewInsecureWebClient() + _, err := client.Get("https://example.com") + // Client should try to proxy through nonexistent server at localhost. + require.Error(t, err) +} + +func TestNewClientWithPoolHTTPProxy(t *testing.T) { + os.Setenv("HTTPS_PROXY", "localhost:9999") + defer os.Unsetenv("HTTPS_PROXY") + client := newClientWithPool(nil) + _, err := client.Get("https://example.com") + // Client should try to proxy through nonexistent server at localhost. + require.Error(t, err) +} From 37cd7861908e5391ca10933327ccf25a60bc71f0 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Tue, 8 Feb 2022 16:24:13 -0800 Subject: [PATCH 03/29] Reduce arg ambiguity --- api/client/webclient/webclient_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/client/webclient/webclient_test.go b/api/client/webclient/webclient_test.go index d3c9ba34fee70..5075fda39a292 100644 --- a/api/client/webclient/webclient_test.go +++ b/api/client/webclient/webclient_test.go @@ -294,7 +294,7 @@ func TestExtract(t *testing.T) { func TestNewWebClientRespectHTTPProxy(t *testing.T) { os.Setenv("HTTPS_PROXY", "localhost:9999") defer os.Unsetenv("HTTPS_PROXY") - client := newWebClient(false, nil) + client := newWebClient(false /* insecure */, nil /* pool */) _, err := client.Get("https://example.com") // Client should try to proxy through nonexistent server at localhost. require.Error(t, err) From e2df5954be28ca435fd3a5b03c7f9c5e87660db4 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Tue, 8 Feb 2022 16:41:08 -0800 Subject: [PATCH 04/29] Make error assertions more specific --- api/client/webclient/webclient_test.go | 2 ++ lib/client/https_client_test.go | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/api/client/webclient/webclient_test.go b/api/client/webclient/webclient_test.go index 5075fda39a292..ae3b78232687f 100644 --- a/api/client/webclient/webclient_test.go +++ b/api/client/webclient/webclient_test.go @@ -298,4 +298,6 @@ func TestNewWebClientRespectHTTPProxy(t *testing.T) { _, err := client.Get("https://example.com") // Client should try to proxy through nonexistent server at localhost. require.Error(t, err) + require.Contains(t, err.Error(), "proxyconnect") + require.Contains(t, err.Error(), "connection refused") } diff --git a/lib/client/https_client_test.go b/lib/client/https_client_test.go index 96bf5d1036117..e8ba11315392d 100644 --- a/lib/client/https_client_test.go +++ b/lib/client/https_client_test.go @@ -30,6 +30,8 @@ func TestNewInsecureWebClientHTTPProxy(t *testing.T) { _, err := client.Get("https://example.com") // Client should try to proxy through nonexistent server at localhost. require.Error(t, err) + require.Contains(t, err.Error(), "proxyconnect") + require.Contains(t, err.Error(), "connection refused") } func TestNewClientWithPoolHTTPProxy(t *testing.T) { @@ -39,4 +41,6 @@ func TestNewClientWithPoolHTTPProxy(t *testing.T) { _, err := client.Get("https://example.com") // Client should try to proxy through nonexistent server at localhost. require.Error(t, err) + require.Contains(t, err.Error(), "proxyconnect") + require.Contains(t, err.Error(), "connection refused") } From 8281dfc6731dddb5a2f6a0a751cc01d71d0d042b Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Wed, 9 Feb 2022 12:13:53 -0800 Subject: [PATCH 05/29] Fix linting --- api/client/webclient/webclient_test.go | 11 +++++++---- lib/client/https_client_test.go | 21 ++++++++++++++------- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/api/client/webclient/webclient_test.go b/api/client/webclient/webclient_test.go index ae3b78232687f..f5b36f08596ea 100644 --- a/api/client/webclient/webclient_test.go +++ b/api/client/webclient/webclient_test.go @@ -22,7 +22,6 @@ import ( "net" "net/http" "net/http/httptest" - "os" "testing" "github.com/stretchr/testify/require" @@ -292,10 +291,14 @@ func TestExtract(t *testing.T) { } func TestNewWebClientRespectHTTPProxy(t *testing.T) { - os.Setenv("HTTPS_PROXY", "localhost:9999") - defer os.Unsetenv("HTTPS_PROXY") + t.Setenv("HTTPS_PROXY", "localhost:9999") client := newWebClient(false /* insecure */, nil /* pool */) - _, err := client.Get("https://example.com") + resp, err := client.Get("https://example.com") + defer func() { + if resp != nil { + resp.Body.Close() + } + }() // Client should try to proxy through nonexistent server at localhost. require.Error(t, err) require.Contains(t, err.Error(), "proxyconnect") diff --git a/lib/client/https_client_test.go b/lib/client/https_client_test.go index e8ba11315392d..446695287c9c4 100644 --- a/lib/client/https_client_test.go +++ b/lib/client/https_client_test.go @@ -17,17 +17,20 @@ limitations under the License. package client import ( - "os" "testing" "github.com/stretchr/testify/require" ) func TestNewInsecureWebClientHTTPProxy(t *testing.T) { - os.Setenv("HTTPS_PROXY", "localhost:9999") - defer os.Unsetenv("HTTPS_PROXY") + t.Setenv("HTTPS_PROXY", "localhost:9999") client := NewInsecureWebClient() - _, err := client.Get("https://example.com") + resp, err := client.Get("https://example.com") + defer func() { + if resp != nil { + resp.Body.Close() + } + }() // Client should try to proxy through nonexistent server at localhost. require.Error(t, err) require.Contains(t, err.Error(), "proxyconnect") @@ -35,10 +38,14 @@ func TestNewInsecureWebClientHTTPProxy(t *testing.T) { } func TestNewClientWithPoolHTTPProxy(t *testing.T) { - os.Setenv("HTTPS_PROXY", "localhost:9999") - defer os.Unsetenv("HTTPS_PROXY") + t.Setenv("HTTPS_PROXY", "localhost:9999") client := newClientWithPool(nil) - _, err := client.Get("https://example.com") + resp, err := client.Get("https://example.com") + defer func() { + if resp != nil { + resp.Body.Close() + } + }() // Client should try to proxy through nonexistent server at localhost. require.Error(t, err) require.Contains(t, err.Error(), "proxyconnect") From 700abc27d0e6047980e9d96e90c81a3aa88e6747 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Wed, 9 Feb 2022 12:41:49 -0800 Subject: [PATCH 06/29] Add messages to assertions --- api/client/webclient/webclient_test.go | 2 +- lib/client/https_client_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/client/webclient/webclient_test.go b/api/client/webclient/webclient_test.go index f5b36f08596ea..75fe4bcb8f584 100644 --- a/api/client/webclient/webclient_test.go +++ b/api/client/webclient/webclient_test.go @@ -300,7 +300,7 @@ func TestNewWebClientRespectHTTPProxy(t *testing.T) { } }() // Client should try to proxy through nonexistent server at localhost. - require.Error(t, err) + require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") require.Contains(t, err.Error(), "connection refused") } diff --git a/lib/client/https_client_test.go b/lib/client/https_client_test.go index 446695287c9c4..3fac6f0440d3b 100644 --- a/lib/client/https_client_test.go +++ b/lib/client/https_client_test.go @@ -32,7 +32,7 @@ func TestNewInsecureWebClientHTTPProxy(t *testing.T) { } }() // Client should try to proxy through nonexistent server at localhost. - require.Error(t, err) + require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") require.Contains(t, err.Error(), "connection refused") } @@ -47,7 +47,7 @@ func TestNewClientWithPoolHTTPProxy(t *testing.T) { } }() // Client should try to proxy through nonexistent server at localhost. - require.Error(t, err) + require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") require.Contains(t, err.Error(), "connection refused") } From d41122a6f20e90c306f6d4ce2b83d573174f3f8d Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Wed, 9 Feb 2022 13:22:01 -0800 Subject: [PATCH 07/29] Change fake proxy address in tests --- api/client/webclient/webclient_test.go | 4 ++-- lib/client/https_client_test.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/api/client/webclient/webclient_test.go b/api/client/webclient/webclient_test.go index 75fe4bcb8f584..d7148542d7988 100644 --- a/api/client/webclient/webclient_test.go +++ b/api/client/webclient/webclient_test.go @@ -291,7 +291,7 @@ func TestExtract(t *testing.T) { } func TestNewWebClientRespectHTTPProxy(t *testing.T) { - t.Setenv("HTTPS_PROXY", "localhost:9999") + t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") client := newWebClient(false /* insecure */, nil /* pool */) resp, err := client.Get("https://example.com") defer func() { @@ -302,5 +302,5 @@ func TestNewWebClientRespectHTTPProxy(t *testing.T) { // Client should try to proxy through nonexistent server at localhost. require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") - require.Contains(t, err.Error(), "connection refused") + require.Contains(t, err.Error(), "no such host") } diff --git a/lib/client/https_client_test.go b/lib/client/https_client_test.go index 3fac6f0440d3b..2bb93cb88396f 100644 --- a/lib/client/https_client_test.go +++ b/lib/client/https_client_test.go @@ -23,7 +23,7 @@ import ( ) func TestNewInsecureWebClientHTTPProxy(t *testing.T) { - t.Setenv("HTTPS_PROXY", "localhost:9999") + t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") client := NewInsecureWebClient() resp, err := client.Get("https://example.com") defer func() { @@ -34,11 +34,11 @@ func TestNewInsecureWebClientHTTPProxy(t *testing.T) { // Client should try to proxy through nonexistent server at localhost. require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") - require.Contains(t, err.Error(), "connection refused") + require.Contains(t, err.Error(), "no such host") } func TestNewClientWithPoolHTTPProxy(t *testing.T) { - t.Setenv("HTTPS_PROXY", "localhost:9999") + t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") client := newClientWithPool(nil) resp, err := client.Get("https://example.com") defer func() { @@ -49,5 +49,5 @@ func TestNewClientWithPoolHTTPProxy(t *testing.T) { // Client should try to proxy through nonexistent server at localhost. require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") - require.Contains(t, err.Error(), "connection refused") + require.Contains(t, err.Error(), "no such host") } From d225e71efce9f310eeb5962e9794682f1e20a5f7 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Thu, 10 Feb 2022 15:07:01 -0800 Subject: [PATCH 08/29] Add http proxy support for ssh --- lib/client/api.go | 40 +++++++-------------- lib/utils/proxy/proxy.go | 78 ++++++++++++++++++++++++++-------------- 2 files changed, 64 insertions(+), 54 deletions(-) diff --git a/lib/client/api.go b/lib/client/api.go index dadf89ae0e3c7..6a5a6a9db97f4 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -68,6 +68,7 @@ import ( "github.com/gravitational/teleport/lib/tlsca" "github.com/gravitational/teleport/lib/utils" "github.com/gravitational/teleport/lib/utils/agentconn" + "github.com/gravitational/teleport/lib/utils/proxy" "github.com/gravitational/trace" @@ -2198,37 +2199,20 @@ func (tc *TeleportClient) connectToProxy(ctx context.Context) (*ProxyClient, err }, nil } -func makeProxySSHClientWithTLSWrapper(tc *TeleportClient, sshConfig *ssh.ClientConfig) (*ssh.Client, error) { - cfg := tc.Config - clientTLSConf, err := tc.loadTLSConfig() - if err != nil { - return nil, trace.Wrap(err) - } - - clientTLSConf.NextProtos = []string{string(alpncommon.ProtocolProxySSH)} - clientTLSConf.InsecureSkipVerify = cfg.InsecureSkipVerify - - tlsConn, err := tls.Dial("tcp", cfg.WebProxyAddr, clientTLSConf) - if err != nil { - return nil, trace.Wrap(err, "failed to dial tls %v", cfg.WebProxyAddr) - } - c, chans, reqs, err := ssh.NewClientConn(tlsConn, cfg.WebProxyAddr, sshConfig) - if err != nil { - // tlsConn is closed inside ssh.NewClientConn function - return nil, trace.Wrap(err, "failed to authenticate with proxy %v", cfg.WebProxyAddr) - } - return ssh.NewClient(c, chans, reqs), nil -} - func makeProxySSHClient(tc *TeleportClient, sshConfig *ssh.ClientConfig) (*ssh.Client, error) { + var opts []proxy.DialerOptionFunc if tc.Config.TLSRoutingEnabled { - return makeProxySSHClientWithTLSWrapper(tc, sshConfig) - } - client, err := ssh.Dial("tcp", tc.Config.SSHProxyAddr, sshConfig) - if err != nil { - return nil, trace.Wrap(err, "failed to authenticate with proxy %v", tc.Config.SSHProxyAddr) + tlsConfig, err := tc.loadTLSConfig() + if err != nil { + return nil, trace.Wrap(err) + } + tlsConfig.NextProtos = []string{string(alpncommon.ProtocolProxySSH)} + tlsConfig.InsecureSkipVerify = tc.Config.InsecureSkipVerify + opts = append(opts, proxy.WithALPNDialer(), proxy.WithTLSConfig(tlsConfig)) } - return client, nil + dialer := proxy.DialerFromEnvironment(tc.Config.WebProxyAddr, opts...) + client, err := dialer.Dial("tcp", tc.Config.SSHProxyAddr, sshConfig) + return client, trace.Wrap(err, "failed to authenticate with proxy %v", tc.Config.SSHProxyAddr) } func (tc *TeleportClient) rootClusterName() (string, error) { diff --git a/lib/utils/proxy/proxy.go b/lib/utils/proxy/proxy.go index 9ef5d2ac57fee..6aba7c404287f 100644 --- a/lib/utils/proxy/proxy.go +++ b/lib/utils/proxy/proxy.go @@ -57,7 +57,7 @@ func dialWithDeadline(network string, addr string, config *ssh.ClientConfig) (*s // dialALPNWithDeadline allows connecting to Teleport in single-port mode. SSH protocol is wrapped into // TLS connection where TLS ALPN protocol is set to ProtocolReverseTunnel allowing ALPN Proxy to route the // incoming connection to ReverseTunnel proxy service. -func dialALPNWithDeadline(network string, addr string, config *ssh.ClientConfig) (*ssh.Client, error) { +func dialALPNWithDeadline(network string, addr string, config *ssh.ClientConfig, tlsConfig *tls.Config) (*ssh.Client, error) { dialer := &net.Dialer{ Timeout: config.Timeout, } @@ -65,11 +65,15 @@ func dialALPNWithDeadline(network string, addr string, config *ssh.ClientConfig) if err != nil { return nil, trace.Wrap(err) } - tlsConn, err := tls.DialWithDialer(dialer, network, addr, &tls.Config{ - NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, - InsecureSkipVerify: lib.IsInsecureDevMode(), - ServerName: address.Host(), - }) + conf := tlsConfig.Clone() + if conf == nil { + conf = &tls.Config{ + NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, + InsecureSkipVerify: lib.IsInsecureDevMode(), + } + } + conf.ServerName = address.Host() + tlsConn, err := tls.DialWithDialer(dialer, network, addr, conf) if err != nil { return nil, trace.Wrap(err) } @@ -86,13 +90,14 @@ type Dialer interface { } type directDial struct { - useTLS bool + useTLS bool + tlsConfig *tls.Config } // Dial calls ssh.Dial directly. func (d directDial) Dial(network string, addr string, config *ssh.ClientConfig) (*ssh.Client, error) { if d.useTLS { - client, err := dialALPNWithDeadline(network, addr, config) + client, err := dialALPNWithDeadline(network, addr, config, d.tlsConfig) if err != nil { return nil, trace.Wrap(err) } @@ -112,11 +117,15 @@ func (d directDial) DialTimeout(network, address string, timeout time.Duration) if err != nil { return nil, trace.Wrap(err) } - tlsConn, err := tls.Dial("tcp", address, &tls.Config{ - NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, - InsecureSkipVerify: lib.IsInsecureDevMode(), - ServerName: addr.Host(), - }) + conf := d.tlsConfig.Clone() + if conf == nil { + conf = &tls.Config{ + NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, + InsecureSkipVerify: lib.IsInsecureDevMode(), + } + } + conf.ServerName = addr.Host() + tlsConn, err := tls.Dial("tcp", address, conf) if err != nil { return nil, trace.Wrap(err) } @@ -132,6 +141,7 @@ func (d directDial) DialTimeout(network, address string, timeout time.Duration) type proxyDial struct { proxyHost string useTLS bool + tlsConfig *tls.Config } // DialTimeout acts like Dial but takes a timeout. @@ -152,11 +162,15 @@ func (d proxyDial) DialTimeout(network, address string, timeout time.Duration) ( if err != nil { return nil, trace.Wrap(err) } - conn = tls.Client(conn, &tls.Config{ - NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, - InsecureSkipVerify: lib.IsInsecureDevMode(), - ServerName: address.Host(), - }) + conf := d.tlsConfig.Clone() + if conf == nil { + conf = &tls.Config{ + NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, + InsecureSkipVerify: lib.IsInsecureDevMode(), + } + } + conf.ServerName = address.Host() + conn = tls.Client(conn, conf) } return conn, nil } @@ -177,11 +191,15 @@ func (d proxyDial) Dial(network string, addr string, config *ssh.ClientConfig) ( if err != nil { return nil, trace.Wrap(err) } - pconn = tls.Client(pconn, &tls.Config{ - NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, - InsecureSkipVerify: lib.IsInsecureDevMode(), - ServerName: address.Host(), - }) + conf := d.tlsConfig.Clone() + if conf == nil { + conf = &tls.Config{ + NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, + InsecureSkipVerify: lib.IsInsecureDevMode(), + } + } + conf.ServerName = address.Host() + pconn = tls.Client(pconn, conf) } // Do the same as ssh.Dial but pass in proxy connection. @@ -196,7 +214,8 @@ func (d proxyDial) Dial(network string, addr string, config *ssh.ClientConfig) ( } type dialerOptions struct { - dialTLS bool + dialTLS bool + tlsConfig *tls.Config } // DialerOptionFunc allows setting options as functional arguments to DialerFromEnvironment @@ -209,6 +228,13 @@ func WithALPNDialer() DialerOptionFunc { } } +// WithTLSConfig creates a dialer that uses a specific tls config. +func WithTLSConfig(tlsConfig *tls.Config) DialerOptionFunc { + return func(options *dialerOptions) { + options.tlsConfig = tlsConfig + } +} + // DialerFromEnvironment returns a Dial function. If the https_proxy or http_proxy // environment variable are set, it returns a function that will dial through // said proxy server. If neither variable is set, it will connect to the SSH @@ -226,10 +252,10 @@ func DialerFromEnvironment(addr string, opts ...DialerOptionFunc) Dialer { // otherwise return a proxy dialer. if proxyAddr == "" { log.Debugf("No proxy set in environment, returning direct dialer.") - return directDial{useTLS: options.dialTLS} + return directDial{useTLS: options.dialTLS, tlsConfig: options.tlsConfig} } log.Debugf("Found proxy %q in environment, returning proxy dialer.", proxyAddr) - return proxyDial{proxyHost: proxyAddr, useTLS: options.dialTLS} + return proxyDial{proxyHost: proxyAddr, useTLS: options.dialTLS, tlsConfig: options.tlsConfig} } type DirectDialerOptFunc func(dial *directDial) From 810d958b90744e2101f77af63e6252f75301add0 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Tue, 15 Feb 2022 13:40:30 -0800 Subject: [PATCH 09/29] Move proxy utils to api --- {lib => api}/utils/proxy/noproxy.go | 0 api/utils/proxy/proxy.go | 144 +++++++++++++++++++++++++ {lib => api}/utils/proxy/proxy_test.go | 2 +- lib/utils/proxy/proxy.go | 125 +-------------------- 4 files changed, 149 insertions(+), 122 deletions(-) rename {lib => api}/utils/proxy/noproxy.go (100%) create mode 100644 api/utils/proxy/proxy.go rename {lib => api}/utils/proxy/proxy_test.go (98%) diff --git a/lib/utils/proxy/noproxy.go b/api/utils/proxy/noproxy.go similarity index 100% rename from lib/utils/proxy/noproxy.go rename to api/utils/proxy/noproxy.go diff --git a/api/utils/proxy/proxy.go b/api/utils/proxy/proxy.go new file mode 100644 index 0000000000000..631be761fd70a --- /dev/null +++ b/api/utils/proxy/proxy.go @@ -0,0 +1,144 @@ +/* +Copyright 2022 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 proxy + +import ( + "bufio" + "context" + "net" + "net/http" + "net/url" + "os" + "strings" + + "github.com/gravitational/teleport" + "github.com/gravitational/trace" + "github.com/siddontang/go/log" +) + +func DialProxy(ctx context.Context, proxyAddr string, addr string) (net.Conn, error) { + var d net.Dialer + conn, err := d.DialContext(ctx, "tcp", proxyAddr) + if err != nil { + log.Warnf("Unable to dial to proxy: %v: %v.", proxyAddr, err) + return nil, trace.ConvertSystemError(err) + } + + connectReq := &http.Request{ + Method: http.MethodConnect, + URL: &url.URL{Opaque: addr}, + Host: addr, + Header: make(http.Header), + } + err = connectReq.Write(conn) + if err != nil { + log.Warnf("Unable to write to proxy: %v.", err) + return nil, trace.Wrap(err) + } + + // Read in the response. http.ReadResponse will read in the status line, mime + // headers, and potentially part of the response body. the body itself will + // not be read, but kept around so it can be read later. + br := bufio.NewReader(conn) + // Per the above comment, we're only using ReadResponse to check the status + // and then hand off the underlying connection to the caller. + // resp.Body.Close() would drain conn and close it, we don't need to do it + // here. Disabling bodyclose linter for this edge case. + //nolint:bodyclose + resp, err := http.ReadResponse(br, connectReq) + if err != nil { + conn.Close() + log.Warnf("Unable to read response: %v.", err) + return nil, trace.Wrap(err) + } + if resp.StatusCode != http.StatusOK { + conn.Close() + return nil, trace.BadParameter("unable to proxy connection: %v", resp.Status) + } + + // Return a bufferedConn that wraps a net.Conn and a *bufio.Reader. this + // needs to be done because http.ReadResponse will buffer part of the + // response body in the *bufio.Reader that was passed in. reads must first + // come from anything buffered, then from the underlying connection otherwise + // data will be lost. + return &bufferedConn{ + Conn: conn, + reader: br, + }, nil +} + +func GetProxyAddress(addr string) string { + envs := []string{ + teleport.HTTPSProxy, + strings.ToLower(teleport.HTTPSProxy), + teleport.HTTPProxy, + strings.ToLower(teleport.HTTPProxy), + } + + for _, v := range envs { + envAddr := os.Getenv(v) + if envAddr == "" { + continue + } + proxyAddr, err := parse(envAddr) + if err != nil { + log.Debugf("Unable to parse environment variable %q: %q.", v, envAddr) + continue + } + log.Debugf("Successfully parsed environment variable %q: %q to %q.", v, envAddr, proxyAddr) + if !useProxy(addr) { + log.Debugf("Matched NO_PROXY override for %q: %q, going to ignore proxy variable.", v, envAddr) + return "" + } + return proxyAddr + } + + log.Debugf("No valid environment variables found.") + return "" +} + +// 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 +// underlying connection. +type bufferedConn struct { + net.Conn + reader *bufio.Reader +} + +// Read first reads from the *bufio.Reader any data that has already been +// buffered. Once all buffered data has been read, reads go to the net.Conn. +func (bc *bufferedConn) Read(b []byte) (n int, err error) { + if bc.reader.Buffered() > 0 { + return bc.reader.Read(b) + } + return bc.Conn.Read(b) +} + +// parse will extract the host:port of the proxy to dial to. If the +// value is not prefixed by "http", then it will prepend "http" and try. +func parse(addr string) (string, error) { + proxyurl, err := url.Parse(addr) + if err != nil || !strings.HasPrefix(proxyurl.Scheme, "http") { + proxyurl, err = url.Parse("http://" + addr) + if err != nil { + return "", trace.Wrap(err) + } + } + + return proxyurl.Host, nil +} diff --git a/lib/utils/proxy/proxy_test.go b/api/utils/proxy/proxy_test.go similarity index 98% rename from lib/utils/proxy/proxy_test.go rename to api/utils/proxy/proxy_test.go index a5c986acd89c5..1067c31d91ef4 100644 --- a/lib/utils/proxy/proxy_test.go +++ b/api/utils/proxy/proxy_test.go @@ -96,7 +96,7 @@ func TestGetProxyAddress(t *testing.T) { for _, env := range tt.env { t.Setenv(env.name, env.val) } - p := getProxyAddress(tt.targetAddr) + p := GetProxyAddress(tt.targetAddr) require.Equal(t, tt.proxyAddr, p) }) } diff --git a/lib/utils/proxy/proxy.go b/lib/utils/proxy/proxy.go index 6aba7c404287f..bc6f56560f5f6 100644 --- a/lib/utils/proxy/proxy.go +++ b/lib/utils/proxy/proxy.go @@ -16,19 +16,15 @@ limitations under the License. package proxy import ( - "bufio" "context" "crypto/tls" "net" - "net/http" - "net/url" - "os" - "strings" "time" "github.com/gravitational/trace" "github.com/gravitational/teleport" + apiproxy "github.com/gravitational/teleport/api/utils/proxy" "github.com/gravitational/teleport/api/utils/sshutils" "github.com/gravitational/teleport/lib" alpncommon "github.com/gravitational/teleport/lib/srv/alpnproxy/common" @@ -153,7 +149,7 @@ func (d proxyDial) DialTimeout(network, address string, timeout time.Duration) ( defer cancel() ctx = timeoutCtx } - conn, err := dialProxy(ctx, d.proxyHost, address) + conn, err := apiproxy.DialProxy(ctx, d.proxyHost, address) if err != nil { return nil, trace.Wrap(err) } @@ -179,7 +175,7 @@ func (d proxyDial) DialTimeout(network, address string, timeout time.Duration) ( // SSH connection. func (d proxyDial) Dial(network string, addr string, config *ssh.ClientConfig) (*ssh.Client, error) { // Build a proxy connection first. - pconn, err := dialProxy(context.Background(), d.proxyHost, addr) + pconn, err := apiproxy.DialProxy(context.Background(), d.proxyHost, addr) if err != nil { return nil, trace.Wrap(err) } @@ -241,7 +237,7 @@ func WithTLSConfig(tlsConfig *tls.Config) DialerOptionFunc { // server directly. func DialerFromEnvironment(addr string, opts ...DialerOptionFunc) Dialer { // Try and get proxy addr from the environment. - proxyAddr := getProxyAddress(addr) + proxyAddr := apiproxy.GetProxyAddress(addr) var options dialerOptions for _, opt := range opts { @@ -259,116 +255,3 @@ func DialerFromEnvironment(addr string, opts ...DialerOptionFunc) Dialer { } type DirectDialerOptFunc func(dial *directDial) - -func dialProxy(ctx context.Context, proxyAddr string, addr string) (net.Conn, error) { - var d net.Dialer - conn, err := d.DialContext(ctx, "tcp", proxyAddr) - if err != nil { - log.Warnf("Unable to dial to proxy: %v: %v.", proxyAddr, err) - return nil, trace.ConvertSystemError(err) - } - - connectReq := &http.Request{ - Method: http.MethodConnect, - URL: &url.URL{Opaque: addr}, - Host: addr, - Header: make(http.Header), - } - err = connectReq.Write(conn) - if err != nil { - log.Warnf("Unable to write to proxy: %v.", err) - return nil, trace.Wrap(err) - } - - // Read in the response. http.ReadResponse will read in the status line, mime - // headers, and potentially part of the response body. the body itself will - // not be read, but kept around so it can be read later. - br := bufio.NewReader(conn) - // Per the above comment, we're only using ReadResponse to check the status - // and then hand off the underlying connection to the caller. - // resp.Body.Close() would drain conn and close it, we don't need to do it - // here. Disabling bodyclose linter for this edge case. - //nolint:bodyclose - resp, err := http.ReadResponse(br, connectReq) - if err != nil { - conn.Close() - log.Warnf("Unable to read response: %v.", err) - return nil, trace.Wrap(err) - } - if resp.StatusCode != http.StatusOK { - conn.Close() - return nil, trace.BadParameter("unable to proxy connection: %v", resp.Status) - } - - // Return a bufferedConn that wraps a net.Conn and a *bufio.Reader. this - // needs to be done because http.ReadResponse will buffer part of the - // response body in the *bufio.Reader that was passed in. reads must first - // come from anything buffered, then from the underlying connection otherwise - // data will be lost. - return &bufferedConn{ - Conn: conn, - reader: br, - }, nil -} - -func getProxyAddress(addr string) string { - envs := []string{ - teleport.HTTPSProxy, - strings.ToLower(teleport.HTTPSProxy), - teleport.HTTPProxy, - strings.ToLower(teleport.HTTPProxy), - } - - for _, v := range envs { - envAddr := os.Getenv(v) - if envAddr == "" { - continue - } - proxyAddr, err := parse(envAddr) - if err != nil { - log.Debugf("Unable to parse environment variable %q: %q.", v, envAddr) - continue - } - log.Debugf("Successfully parsed environment variable %q: %q to %q.", v, envAddr, proxyAddr) - if !useProxy(addr) { - log.Debugf("Matched NO_PROXY override for %q: %q, going to ignore proxy variable.", v, envAddr) - return "" - } - return proxyAddr - } - - log.Debugf("No valid environment variables found.") - return "" -} - -// 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 -// underlying connection. -type bufferedConn struct { - net.Conn - reader *bufio.Reader -} - -// Read first reads from the *bufio.Reader any data that has already been -// buffered. Once all buffered data has been read, reads go to the net.Conn. -func (bc *bufferedConn) Read(b []byte) (n int, err error) { - if bc.reader.Buffered() > 0 { - return bc.reader.Read(b) - } - return bc.Conn.Read(b) -} - -// parse will extract the host:port of the proxy to dial to. If the -// value is not prefixed by "http", then it will prepend "http" and try. -func parse(addr string) (string, error) { - proxyurl, err := url.Parse(addr) - if err != nil || !strings.HasPrefix(proxyurl.Scheme, "http") { - proxyurl, err = url.Parse("http://" + addr) - if err != nil { - return "", trace.Wrap(err) - } - } - - return proxyurl.Host, nil -} From dd5e2328f80846af49c2ab4870780477cf67fc3c Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Tue, 15 Feb 2022 14:59:55 -0800 Subject: [PATCH 10/29] Add proxy-aware context dialer --- api/client/client.go | 2 +- api/client/contextdialer.go | 11 +++++++++++ api/{utils/proxy => client}/noproxy.go | 2 +- api/{utils/proxy => client}/proxy.go | 10 ++++++---- api/{utils/proxy => client}/proxy_test.go | 2 +- lib/auth/clt.go | 2 +- lib/utils/proxy/proxy.go | 8 ++++---- 7 files changed, 25 insertions(+), 12 deletions(-) rename api/{utils/proxy => client}/noproxy.go (99%) rename api/{utils/proxy => client}/proxy.go (94%) rename api/{utils/proxy => client}/proxy_test.go (99%) diff --git a/api/client/client.go b/api/client/client.go index c2ec9efe65bcc..84dc97e666815 100644 --- a/api/client/client.go +++ b/api/client/client.go @@ -283,7 +283,7 @@ 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) + dialer := NewDialer(params.cfg.KeepAlivePeriod, params.cfg.DialTimeout) 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/api/client/contextdialer.go b/api/client/contextdialer.go index 2c2d08e29e894..62da334808e5a 100644 --- a/api/client/contextdialer.go +++ b/api/client/contextdialer.go @@ -52,6 +52,17 @@ func NewDirectDialer(keepAlivePeriod, dialTimeout time.Duration) ContextDialer { } } +// NewDialer makes a new dialer that connects to an Auth server either directly or via an HTTP proxy/ +func NewDialer(keepAlivePeriod, dialTimeout time.Duration) ContextDialer { + dialer := NewDirectDialer(keepAlivePeriod, dialTimeout) + return ContextDialerFunc(func(ctx context.Context, network, addr string) (net.Conn, error) { + if proxyAddr := GetProxyAddress(addr); proxyAddr != "" { + return DialProxy(ctx, proxyAddr, addr, dialer) + } + return dialer.DialContext(ctx, network, addr) + }) +} + // NewProxyDialer makes a dialer to connect to an Auth server through the SSH reverse tunnel on the proxy. // The dialer will ping the web client to discover the tunnel proxy address on each dial. func NewProxyDialer(ssh ssh.ClientConfig, keepAlivePeriod, dialTimeout time.Duration, discoveryAddr string, insecure bool) ContextDialer { diff --git a/api/utils/proxy/noproxy.go b/api/client/noproxy.go similarity index 99% rename from api/utils/proxy/noproxy.go rename to api/client/noproxy.go index d9dcdb5f3d8a9..159ad56b8d382 100644 --- a/api/utils/proxy/noproxy.go +++ b/api/client/noproxy.go @@ -7,7 +7,7 @@ // This is the low-level Transport implementation of RoundTripper. // The high-level interface is in client.go. -package proxy +package client import ( "os" diff --git a/api/utils/proxy/proxy.go b/api/client/proxy.go similarity index 94% rename from api/utils/proxy/proxy.go rename to api/client/proxy.go index 631be761fd70a..3fc54104ec5a6 100644 --- a/api/utils/proxy/proxy.go +++ b/api/client/proxy.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package proxy +package client import ( "bufio" @@ -30,9 +30,11 @@ import ( "github.com/siddontang/go/log" ) -func DialProxy(ctx context.Context, proxyAddr string, addr string) (net.Conn, error) { - var d net.Dialer - conn, err := d.DialContext(ctx, "tcp", proxyAddr) +func DialProxy(ctx context.Context, proxyAddr, addr string, dialer ContextDialer) (net.Conn, error) { + if dialer == nil { + dialer = &net.Dialer{} + } + conn, err := dialer.DialContext(ctx, "tcp", proxyAddr) if err != nil { log.Warnf("Unable to dial to proxy: %v: %v.", proxyAddr, err) return nil, trace.ConvertSystemError(err) diff --git a/api/utils/proxy/proxy_test.go b/api/client/proxy_test.go similarity index 99% rename from api/utils/proxy/proxy_test.go rename to api/client/proxy_test.go index 1067c31d91ef4..d1d781f137fdd 100644 --- a/api/utils/proxy/proxy_test.go +++ b/api/client/proxy_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package proxy +package client import ( "fmt" diff --git a/lib/auth/clt.go b/lib/auth/clt.go index 49a5f23b83845..e74f3654f7db5 100644 --- a/lib/auth/clt.go +++ b/lib/auth/clt.go @@ -128,7 +128,7 @@ func NewHTTPClient(cfg client.Config, tls *tls.Config, params ...roundtrip.Clien if len(cfg.Addrs) == 0 { return nil, trace.BadParameter("no addresses to dial") } - contextDialer := client.NewDirectDialer(cfg.KeepAlivePeriod, cfg.DialTimeout) + contextDialer := client.NewDialer(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) diff --git a/lib/utils/proxy/proxy.go b/lib/utils/proxy/proxy.go index bc6f56560f5f6..683e625144e58 100644 --- a/lib/utils/proxy/proxy.go +++ b/lib/utils/proxy/proxy.go @@ -24,7 +24,7 @@ import ( "github.com/gravitational/trace" "github.com/gravitational/teleport" - apiproxy "github.com/gravitational/teleport/api/utils/proxy" + apiclient "github.com/gravitational/teleport/api/client" "github.com/gravitational/teleport/api/utils/sshutils" "github.com/gravitational/teleport/lib" alpncommon "github.com/gravitational/teleport/lib/srv/alpnproxy/common" @@ -149,7 +149,7 @@ func (d proxyDial) DialTimeout(network, address string, timeout time.Duration) ( defer cancel() ctx = timeoutCtx } - conn, err := apiproxy.DialProxy(ctx, d.proxyHost, address) + conn, err := apiclient.DialProxy(ctx, d.proxyHost, address, nil) if err != nil { return nil, trace.Wrap(err) } @@ -175,7 +175,7 @@ func (d proxyDial) DialTimeout(network, address string, timeout time.Duration) ( // SSH connection. func (d proxyDial) Dial(network string, addr string, config *ssh.ClientConfig) (*ssh.Client, error) { // Build a proxy connection first. - pconn, err := apiproxy.DialProxy(context.Background(), d.proxyHost, addr) + pconn, err := apiclient.DialProxy(context.Background(), d.proxyHost, addr, nil) if err != nil { return nil, trace.Wrap(err) } @@ -237,7 +237,7 @@ func WithTLSConfig(tlsConfig *tls.Config) DialerOptionFunc { // server directly. func DialerFromEnvironment(addr string, opts ...DialerOptionFunc) Dialer { // Try and get proxy addr from the environment. - proxyAddr := apiproxy.GetProxyAddress(addr) + proxyAddr := apiclient.GetProxyAddress(addr) var options dialerOptions for _, opt := range opts { From 7981450ce8a5e835be190a121eb06b3611e9802a Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Wed, 16 Feb 2022 15:31:26 -0800 Subject: [PATCH 11/29] Fix incorrect address in makeProxySSHClient --- api/client/contextdialer.go | 3 ++- lib/client/api.go | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/api/client/contextdialer.go b/api/client/contextdialer.go index 62da334808e5a..0797ec1a7a64e 100644 --- a/api/client/contextdialer.go +++ b/api/client/contextdialer.go @@ -52,7 +52,8 @@ func NewDirectDialer(keepAlivePeriod, dialTimeout time.Duration) ContextDialer { } } -// NewDialer makes a new dialer that connects to an Auth server either directly or via an HTTP proxy/ +// NewDialer makes a new dialer that connects to an Auth server either directly or via an HTTP proxy, depending +// on the environment. func NewDialer(keepAlivePeriod, dialTimeout time.Duration) ContextDialer { dialer := NewDirectDialer(keepAlivePeriod, dialTimeout) return ContextDialerFunc(func(ctx context.Context, network, addr string) (net.Conn, error) { diff --git a/lib/client/api.go b/lib/client/api.go index 6a5a6a9db97f4..09e80be1ce0a4 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -2201,6 +2201,7 @@ func (tc *TeleportClient) connectToProxy(ctx context.Context) (*ProxyClient, err func makeProxySSHClient(tc *TeleportClient, sshConfig *ssh.ClientConfig) (*ssh.Client, error) { var opts []proxy.DialerOptionFunc + addr := tc.Config.SSHProxyAddr if tc.Config.TLSRoutingEnabled { tlsConfig, err := tc.loadTLSConfig() if err != nil { @@ -2208,10 +2209,11 @@ func makeProxySSHClient(tc *TeleportClient, sshConfig *ssh.ClientConfig) (*ssh.C } tlsConfig.NextProtos = []string{string(alpncommon.ProtocolProxySSH)} tlsConfig.InsecureSkipVerify = tc.Config.InsecureSkipVerify + addr = tc.Config.WebProxyAddr opts = append(opts, proxy.WithALPNDialer(), proxy.WithTLSConfig(tlsConfig)) } - dialer := proxy.DialerFromEnvironment(tc.Config.WebProxyAddr, opts...) - client, err := dialer.Dial("tcp", tc.Config.SSHProxyAddr, sshConfig) + dialer := proxy.DialerFromEnvironment(addr, opts...) + client, err := dialer.Dial("tcp", addr, sshConfig) return client, trace.Wrap(err, "failed to authenticate with proxy %v", tc.Config.SSHProxyAddr) } From ad44ccf0b5d3a095e095e5e40cdcc2866f5e69e3 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Tue, 1 Mar 2022 14:48:01 -0800 Subject: [PATCH 12/29] Add docs --- api/client/proxy.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/api/client/proxy.go b/api/client/proxy.go index 3fc54104ec5a6..d7ee2d9431800 100644 --- a/api/client/proxy.go +++ b/api/client/proxy.go @@ -30,6 +30,7 @@ import ( "github.com/siddontang/go/log" ) +// DialProxy creates a connection to a server via an HTTP Proxy. func DialProxy(ctx context.Context, proxyAddr, addr string, dialer ContextDialer) (net.Conn, error) { if dialer == nil { dialer = &net.Dialer{} @@ -83,6 +84,7 @@ func DialProxy(ctx context.Context, proxyAddr, addr string, dialer ContextDialer }, nil } +// GetProxyAddress gets the HTTP proxy address to use for a given address, if any. func GetProxyAddress(addr string) string { envs := []string{ teleport.HTTPSProxy, From de6c7155e6d80d9774f03b86c47e6841f9081a53 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Tue, 1 Mar 2022 15:11:03 -0800 Subject: [PATCH 13/29] Add NO_PROXY tests --- api/client/webclient/webclient_test.go | 10 ++++++++++ lib/client/https_client_test.go | 21 +++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/api/client/webclient/webclient_test.go b/api/client/webclient/webclient_test.go index d7148542d7988..fd9d81fab02d8 100644 --- a/api/client/webclient/webclient_test.go +++ b/api/client/webclient/webclient_test.go @@ -304,3 +304,13 @@ func TestNewWebClientRespectHTTPProxy(t *testing.T) { require.Contains(t, err.Error(), "proxyconnect") require.Contains(t, err.Error(), "no such host") } + +func TestNewWebClientNoProxy(t *testing.T) { + t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") + t.Setenv("NO_PROXY", "example.com") + client := newWebClient(false /* insecure */, nil /* pool */) + resp, err := client.Get("https://example.com") + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) +} diff --git a/lib/client/https_client_test.go b/lib/client/https_client_test.go index 2bb93cb88396f..b0cced4188890 100644 --- a/lib/client/https_client_test.go +++ b/lib/client/https_client_test.go @@ -17,6 +17,7 @@ limitations under the License. package client import ( + "net/http" "testing" "github.com/stretchr/testify/require" @@ -37,6 +38,16 @@ func TestNewInsecureWebClientHTTPProxy(t *testing.T) { require.Contains(t, err.Error(), "no such host") } +func TestNewInsecureWebClientNoProxy(t *testing.T) { + t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") + t.Setenv("NO_PROXY", "example.com") + client := NewInsecureWebClient() + resp, err := client.Get("https://example.com") + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) +} + func TestNewClientWithPoolHTTPProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") client := newClientWithPool(nil) @@ -51,3 +62,13 @@ func TestNewClientWithPoolHTTPProxy(t *testing.T) { require.Contains(t, err.Error(), "proxyconnect") require.Contains(t, err.Error(), "no such host") } + +func TestNewClientWithPoolNoProxy(t *testing.T) { + t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") + t.Setenv("NO_PROXY", "example.com") + client := newClientWithPool(nil) + resp, err := client.Get("https://example.com") + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) +} From df6c5e2875da4913b622290bdc0b478a0872ca37 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Tue, 1 Mar 2022 16:00:36 -0800 Subject: [PATCH 14/29] Simplify bodyclose workaround --- api/client/webclient/webclient_test.go | 7 ++----- lib/client/https_client_test.go | 14 ++++---------- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/api/client/webclient/webclient_test.go b/api/client/webclient/webclient_test.go index fd9d81fab02d8..963b6b5f2156c 100644 --- a/api/client/webclient/webclient_test.go +++ b/api/client/webclient/webclient_test.go @@ -293,12 +293,9 @@ func TestExtract(t *testing.T) { func TestNewWebClientRespectHTTPProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") client := newWebClient(false /* insecure */, nil /* pool */) + // resp should be nil, so there will be no body to close. + //nolint:bodyclose resp, err := client.Get("https://example.com") - defer func() { - if resp != nil { - resp.Body.Close() - } - }() // Client should try to proxy through nonexistent server at localhost. require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") diff --git a/lib/client/https_client_test.go b/lib/client/https_client_test.go index b0cced4188890..ea5fac1a89a4d 100644 --- a/lib/client/https_client_test.go +++ b/lib/client/https_client_test.go @@ -26,12 +26,9 @@ import ( func TestNewInsecureWebClientHTTPProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") client := NewInsecureWebClient() + // resp should be nil, so there will be no body to close. + //nolint:bodyclose resp, err := client.Get("https://example.com") - defer func() { - if resp != nil { - resp.Body.Close() - } - }() // Client should try to proxy through nonexistent server at localhost. require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") @@ -51,12 +48,9 @@ func TestNewInsecureWebClientNoProxy(t *testing.T) { func TestNewClientWithPoolHTTPProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") client := newClientWithPool(nil) + // resp should be nil, so there will be no body to close. + //nolint:bodyclose resp, err := client.Get("https://example.com") - defer func() { - if resp != nil { - resp.Body.Close() - } - }() // Client should try to proxy through nonexistent server at localhost. require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") From bde8fb2c7e7728c9b83a8a65432d26f23b9e1ff9 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Wed, 2 Mar 2022 16:08:50 -0800 Subject: [PATCH 15/29] Stop caching env vars --- api/client/webclient/webclient.go | 5 ++++- lib/client/https_client.go | 9 +++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/api/client/webclient/webclient.go b/api/client/webclient/webclient.go index 1b684c0969218..53a496587f5b8 100644 --- a/api/client/webclient/webclient.go +++ b/api/client/webclient/webclient.go @@ -33,6 +33,7 @@ import ( "github.com/gravitational/teleport/api/constants" "github.com/gravitational/teleport/api/defaults" "github.com/gravitational/teleport/api/utils" + "golang.org/x/net/http/httpproxy" "github.com/gravitational/trace" log "github.com/sirupsen/logrus" @@ -46,7 +47,9 @@ func newWebClient(insecure bool, pool *x509.CertPool) *http.Client { RootCAs: pool, InsecureSkipVerify: insecure, }, - Proxy: http.ProxyFromEnvironment, + Proxy: func(req *http.Request) (*url.URL, error) { + return httpproxy.FromEnvironment().ProxyFunc()(req.URL) + }, }, } } diff --git a/lib/client/https_client.go b/lib/client/https_client.go index 8a95b3cf47497..c9f01aead2cb0 100644 --- a/lib/client/https_client.go +++ b/lib/client/https_client.go @@ -27,6 +27,7 @@ import ( apiutils "github.com/gravitational/teleport/api/utils" "github.com/gravitational/teleport/lib/httplib" "github.com/gravitational/teleport/lib/utils" + "golang.org/x/net/http/httpproxy" "github.com/gravitational/roundtrip" "github.com/gravitational/trace" @@ -41,7 +42,9 @@ func NewInsecureWebClient() *http.Client { return &http.Client{ Transport: &http.Transport{ TLSClientConfig: tlsConfig, - Proxy: http.ProxyFromEnvironment, + Proxy: func(req *http.Request) (*url.URL, error) { + return httpproxy.FromEnvironment().ProxyFunc()(req.URL) + }, }, } } @@ -55,7 +58,9 @@ func newClientWithPool(pool *x509.CertPool) *http.Client { return &http.Client{ Transport: &http.Transport{ TLSClientConfig: tlsConfig, - Proxy: http.ProxyFromEnvironment, + Proxy: func(req *http.Request) (*url.URL, error) { + return httpproxy.FromEnvironment().ProxyFunc()(req.URL) + }, }, } } From bbc3af7b1b653b22fe558e04cbf2efb59acf29a7 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Thu, 3 Mar 2022 14:05:05 -0800 Subject: [PATCH 16/29] Split makeProxySSHClient --- lib/client/api.go | 32 +++++++++++++++++++------------- lib/utils/proxy/proxy.go | 3 +-- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/lib/client/api.go b/lib/client/api.go index 285fac1476940..3ddf07a1f48cc 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -2234,21 +2234,27 @@ func (tc *TeleportClient) connectToProxy(ctx context.Context) (*ProxyClient, err } func makeProxySSHClient(tc *TeleportClient, sshConfig *ssh.ClientConfig) (*ssh.Client, error) { - var opts []proxy.DialerOptionFunc - addr := tc.Config.SSHProxyAddr if tc.Config.TLSRoutingEnabled { - tlsConfig, err := tc.loadTLSConfig() - if err != nil { - return nil, trace.Wrap(err) - } - tlsConfig.NextProtos = []string{string(alpncommon.ProtocolProxySSH)} - tlsConfig.InsecureSkipVerify = tc.Config.InsecureSkipVerify - addr = tc.Config.WebProxyAddr - opts = append(opts, proxy.WithALPNDialer(), proxy.WithTLSConfig(tlsConfig)) + return makeProxySSHClientWithTLSWrapper(tc, sshConfig) } - dialer := proxy.DialerFromEnvironment(addr, opts...) - client, err := dialer.Dial("tcp", addr, sshConfig) - return client, trace.Wrap(err, "failed to authenticate with proxy %v", tc.Config.SSHProxyAddr) + return makeProxySSHClientDirect(tc, sshConfig) +} + +func makeProxySSHClientDirect(tc *TeleportClient, sshConfig *ssh.ClientConfig) (*ssh.Client, error) { + dialer := proxy.DialerFromEnvironment(tc.Config.SSHProxyAddr) + return dialer.Dial("tcp", tc.Config.SSHProxyAddr, sshConfig) +} + +func makeProxySSHClientWithTLSWrapper(tc *TeleportClient, sshConfig *ssh.ClientConfig) (*ssh.Client, error) { + tlsConfig, err := tc.loadTLSConfig() + if err != nil { + return nil, trace.Wrap(err) + } + tlsConfig.NextProtos = []string{string(alpncommon.ProtocolProxySSH)} + tlsConfig.InsecureSkipVerify = tc.Config.InsecureSkipVerify + + dialer := proxy.DialerFromEnvironment(tc.Config.WebProxyAddr, proxy.WithALPNDialer(), proxy.WithTLSConfig(tlsConfig)) + return dialer.Dial("tcp", tc.Config.WebProxyAddr, sshConfig) } func (tc *TeleportClient) rootClusterName() (string, error) { diff --git a/lib/utils/proxy/proxy.go b/lib/utils/proxy/proxy.go index 1075034e85805..c9f60900c62ed 100644 --- a/lib/utils/proxy/proxy.go +++ b/lib/utils/proxy/proxy.go @@ -26,7 +26,6 @@ import ( "github.com/gravitational/teleport" apiclient "github.com/gravitational/teleport/api/client" "github.com/gravitational/teleport/api/utils/sshutils" - "github.com/gravitational/teleport/lib" alpncommon "github.com/gravitational/teleport/lib/srv/alpnproxy/common" "github.com/gravitational/teleport/lib/utils" @@ -200,7 +199,7 @@ func (d proxyDial) Dial(network string, addr string, config *ssh.ClientConfig) ( if conf == nil { conf = &tls.Config{ NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, - InsecureSkipVerify: lib.IsInsecureDevMode(), + InsecureSkipVerify: d.insecure, } } conf.ServerName = address.Host() From 79e3f3f8b9709c89dcbe99322c12b3076d764085 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Thu, 10 Mar 2022 14:46:38 -0800 Subject: [PATCH 17/29] Remove leftover teleport imports from api --- api/client/noproxy.go | 4 ++-- api/client/proxy.go | 10 +++++----- api/client/proxy_test.go | 7 ------- api/constants/constants.go | 12 ++++++++++++ api/go.mod | 1 + api/go.sum | 2 ++ constants.go | 12 ------------ 7 files changed, 22 insertions(+), 26 deletions(-) diff --git a/api/client/noproxy.go b/api/client/noproxy.go index 159ad56b8d382..a6115d20c6a5a 100644 --- a/api/client/noproxy.go +++ b/api/client/noproxy.go @@ -13,7 +13,7 @@ import ( "os" "strings" - "github.com/gravitational/teleport" + "github.com/gravitational/teleport/api/constants" ) // useProxy reports whether requests to addr should use a proxy, @@ -24,7 +24,7 @@ func useProxy(addr string) bool { return true } var noProxy string - for _, env := range []string{teleport.NoProxy, strings.ToLower(teleport.NoProxy)} { + for _, env := range []string{constants.NoProxy, strings.ToLower(constants.NoProxy)} { noProxy = os.Getenv(env) if noProxy != "" { break diff --git a/api/client/proxy.go b/api/client/proxy.go index d7ee2d9431800..b42c45fe56cf3 100644 --- a/api/client/proxy.go +++ b/api/client/proxy.go @@ -25,7 +25,7 @@ import ( "os" "strings" - "github.com/gravitational/teleport" + "github.com/gravitational/teleport/api/constants" "github.com/gravitational/trace" "github.com/siddontang/go/log" ) @@ -87,10 +87,10 @@ func DialProxy(ctx context.Context, proxyAddr, addr string, dialer ContextDialer // GetProxyAddress gets the HTTP proxy address to use for a given address, if any. func GetProxyAddress(addr string) string { envs := []string{ - teleport.HTTPSProxy, - strings.ToLower(teleport.HTTPSProxy), - teleport.HTTPProxy, - strings.ToLower(teleport.HTTPProxy), + constants.HTTPSProxy, + strings.ToLower(constants.HTTPSProxy), + constants.HTTPProxy, + strings.ToLower(constants.HTTPProxy), } for _, v := range envs { diff --git a/api/client/proxy_test.go b/api/client/proxy_test.go index d1d781f137fdd..29cffc12e1b4f 100644 --- a/api/client/proxy_test.go +++ b/api/client/proxy_test.go @@ -18,18 +18,11 @@ package client import ( "fmt" - "os" "testing" - "github.com/gravitational/teleport/lib/utils" "github.com/stretchr/testify/require" ) -func TestMain(m *testing.M) { - utils.InitLoggerForTests() - os.Exit(m.Run()) -} - func TestGetProxyAddress(t *testing.T) { type env struct { name string diff --git a/api/constants/constants.go b/api/constants/constants.go index fbb1214b8493a..0e54e83e62b6c 100644 --- a/api/constants/constants.go +++ b/api/constants/constants.go @@ -189,3 +189,15 @@ const ( // KubeTeleportProxyALPNPrefix is a SNI Kubernetes prefix used for distinguishing the Kubernetes HTTP traffic. KubeTeleportProxyALPNPrefix = "kube-teleport-proxy-alpn." ) + +const ( + // HTTPSProxy is an environment variable pointing to a HTTPS proxy. + HTTPSProxy = "HTTPS_PROXY" + + // HTTPProxy is an environment variable pointing to a HTTP proxy. + HTTPProxy = "HTTP_PROXY" + + // NoProxy is an environment variable matching the cases + // when HTTPS_PROXY or HTTP_PROXY is ignored + NoProxy = "NO_PROXY" +) diff --git a/api/go.mod b/api/go.mod index 3a7210c771f25..a81a73769382d 100644 --- a/api/go.mod +++ b/api/go.mod @@ -8,6 +8,7 @@ require ( github.com/google/go-cmp v0.5.4 github.com/gravitational/trace v1.1.17 github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect + github.com/siddontang/go v0.0.0-20180604090527-bdc77568d726 github.com/sirupsen/logrus v1.8.1 github.com/stretchr/testify v1.7.0 golang.org/x/crypto v0.0.0-20220126234351-aa10faf2a1f8 diff --git a/api/go.sum b/api/go.sum index 72c1f18217730..69ab03da359fa 100644 --- a/api/go.sum +++ b/api/go.sum @@ -61,6 +61,8 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA= github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ= +github.com/siddontang/go v0.0.0-20180604090527-bdc77568d726 h1:xT+JlYxNGqyT+XcU8iUrN18JYed2TvG9yN5ULG2jATM= +github.com/siddontang/go v0.0.0-20180604090527-bdc77568d726/go.mod h1:3yhqj7WBBfRhbBlzyOC3gUxftwsU0u8gqevxwIHQpMw= github.com/sirupsen/logrus v1.8.1 h1:dJKuHgqk1NNQlqoA6BTlM1Wf9DOH3NBjQyu0h9+AZZE= github.com/sirupsen/logrus v1.8.1/go.mod h1:yWOB1SBYBC5VeMP7gHvWumXLIWorT60ONWic61uBYv0= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= diff --git a/constants.go b/constants.go index 246cdc8809a78..13bf738f230b4 100644 --- a/constants.go +++ b/constants.go @@ -61,18 +61,6 @@ const ( HTTPNextProtoTLS = "http/1.1" ) -const ( - // HTTPSProxy is an environment variable pointing to a HTTPS proxy. - HTTPSProxy = "HTTPS_PROXY" - - // HTTPProxy is an environment variable pointing to a HTTP proxy. - HTTPProxy = "HTTP_PROXY" - - // NoProxy is an environment variable matching the cases - // when HTTPS_PROXY or HTTP_PROXY is ignored - NoProxy = "NO_PROXY" -) - const ( // TOTPValidityPeriod is the number of seconds a TOTP token is valid. TOTPValidityPeriod uint = 30 From fcf43eea14cce4e6d25b20935f09fe0dbc7b6a74 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Fri, 11 Mar 2022 14:27:52 -0800 Subject: [PATCH 18/29] Make dialALPNWithDeadline a member of directDial --- lib/utils/proxy/proxy.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/utils/proxy/proxy.go b/lib/utils/proxy/proxy.go index c9f60900c62ed..f128fc223ebc5 100644 --- a/lib/utils/proxy/proxy.go +++ b/lib/utils/proxy/proxy.go @@ -52,7 +52,7 @@ func dialWithDeadline(network string, addr string, config *ssh.ClientConfig) (*s // dialALPNWithDeadline allows connecting to Teleport in single-port mode. SSH protocol is wrapped into // TLS connection where TLS ALPN protocol is set to ProtocolReverseTunnel allowing ALPN Proxy to route the // incoming connection to ReverseTunnel proxy service. -func dialALPNWithDeadline(network string, addr string, config *ssh.ClientConfig, insecure bool, tlsConfig *tls.Config) (*ssh.Client, error) { +func (d directDial) dialALPNWithDeadline(network string, addr string, config *ssh.ClientConfig) (*ssh.Client, error) { dialer := &net.Dialer{ Timeout: config.Timeout, } @@ -60,14 +60,14 @@ func dialALPNWithDeadline(network string, addr string, config *ssh.ClientConfig, if err != nil { return nil, trace.Wrap(err) } - conf := tlsConfig.Clone() + conf := d.tlsConfig.Clone() if conf == nil { conf = &tls.Config{ NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, } } conf.ServerName = address.Host() - conf.InsecureSkipVerify = insecure + conf.InsecureSkipVerify = d.insecure tlsConn, err := tls.DialWithDialer(dialer, network, addr, conf) if err != nil { return nil, trace.Wrap(err) @@ -96,7 +96,7 @@ type directDial struct { // Dial calls ssh.Dial directly. func (d directDial) Dial(network string, addr string, config *ssh.ClientConfig) (*ssh.Client, error) { if d.tlsRoutingEnabled { - client, err := dialALPNWithDeadline(network, addr, config, d.insecure, d.tlsConfig) + client, err := d.dialALPNWithDeadline(network, addr, config) if err != nil { return nil, trace.Wrap(err) } From e24d3d0451be0671dd4fadfef8e47a2df87f6121 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Fri, 11 Mar 2022 14:30:59 -0800 Subject: [PATCH 19/29] Add DialProxyWithDialer for custom dialers --- api/client/contextdialer.go | 2 +- api/client/proxy.go | 10 ++++++---- lib/utils/proxy/proxy.go | 4 ++-- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/api/client/contextdialer.go b/api/client/contextdialer.go index 0797ec1a7a64e..5c3725c5a1631 100644 --- a/api/client/contextdialer.go +++ b/api/client/contextdialer.go @@ -58,7 +58,7 @@ func NewDialer(keepAlivePeriod, dialTimeout time.Duration) ContextDialer { dialer := NewDirectDialer(keepAlivePeriod, dialTimeout) return ContextDialerFunc(func(ctx context.Context, network, addr string) (net.Conn, error) { if proxyAddr := GetProxyAddress(addr); proxyAddr != "" { - return DialProxy(ctx, proxyAddr, addr, dialer) + return DialProxyWithDialer(ctx, proxyAddr, addr, dialer) } return dialer.DialContext(ctx, network, addr) }) diff --git a/api/client/proxy.go b/api/client/proxy.go index b42c45fe56cf3..42d448c8e9040 100644 --- a/api/client/proxy.go +++ b/api/client/proxy.go @@ -31,10 +31,12 @@ import ( ) // DialProxy creates a connection to a server via an HTTP Proxy. -func DialProxy(ctx context.Context, proxyAddr, addr string, dialer ContextDialer) (net.Conn, error) { - if dialer == nil { - dialer = &net.Dialer{} - } +func DialProxy(ctx context.Context, proxyAddr, addr string) (net.Conn, error) { + return DialProxyWithDialer(ctx, proxyAddr, addr, &net.Dialer{}) +} + +// DialProxyWithDialer creates a connection to a server via an HTTP Proxy using a specified dialer. +func DialProxyWithDialer(ctx context.Context, proxyAddr, addr string, dialer ContextDialer) (net.Conn, error) { conn, err := dialer.DialContext(ctx, "tcp", proxyAddr) if err != nil { log.Warnf("Unable to dial to proxy: %v: %v.", proxyAddr, err) diff --git a/lib/utils/proxy/proxy.go b/lib/utils/proxy/proxy.go index f128fc223ebc5..19fe1972a996c 100644 --- a/lib/utils/proxy/proxy.go +++ b/lib/utils/proxy/proxy.go @@ -157,7 +157,7 @@ func (d proxyDial) DialTimeout(network, address string, timeout time.Duration) ( defer cancel() ctx = timeoutCtx } - conn, err := apiclient.DialProxy(ctx, d.proxyHost, address, nil) + conn, err := apiclient.DialProxy(ctx, d.proxyHost, address) if err != nil { return nil, trace.Wrap(err) } @@ -183,7 +183,7 @@ func (d proxyDial) DialTimeout(network, address string, timeout time.Duration) ( // SSH connection. func (d proxyDial) Dial(network string, addr string, config *ssh.ClientConfig) (*ssh.Client, error) { // Build a proxy connection first. - pconn, err := apiclient.DialProxy(context.Background(), d.proxyHost, addr, nil) + pconn, err := apiclient.DialProxy(context.Background(), d.proxyHost, addr) if err != nil { return nil, trace.Wrap(err) } From 749d8896c736ae3d8d5482b4ba1c514ea35e6946 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Mon, 14 Mar 2022 10:32:35 -0700 Subject: [PATCH 20/29] Make tlsConfig mandatory --- lib/client/api.go | 4 +- lib/reversetunnel/agent.go | 6 ++- lib/reversetunnel/transport.go | 6 ++- lib/utils/proxy/proxy.go | 72 +++++++++++----------------------- 4 files changed, 34 insertions(+), 54 deletions(-) diff --git a/lib/client/api.go b/lib/client/api.go index 3ddf07a1f48cc..4c19cd4a3609e 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -2251,9 +2251,7 @@ func makeProxySSHClientWithTLSWrapper(tc *TeleportClient, sshConfig *ssh.ClientC return nil, trace.Wrap(err) } tlsConfig.NextProtos = []string{string(alpncommon.ProtocolProxySSH)} - tlsConfig.InsecureSkipVerify = tc.Config.InsecureSkipVerify - - dialer := proxy.DialerFromEnvironment(tc.Config.WebProxyAddr, proxy.WithALPNDialer(), proxy.WithTLSConfig(tlsConfig)) + dialer := proxy.DialerFromEnvironment(tc.Config.WebProxyAddr, proxy.WithALPNDialer(tlsConfig)) return dialer.Dial("tcp", tc.Config.WebProxyAddr, sshConfig) } diff --git a/lib/reversetunnel/agent.go b/lib/reversetunnel/agent.go index 22bebc91c6d54..8f040c6974b3a 100644 --- a/lib/reversetunnel/agent.go +++ b/lib/reversetunnel/agent.go @@ -22,6 +22,7 @@ package reversetunnel import ( "context" + "crypto/tls" "fmt" "sync" "time" @@ -35,6 +36,7 @@ import ( "github.com/gravitational/teleport/lib" "github.com/gravitational/teleport/lib/auth" "github.com/gravitational/teleport/lib/reversetunnel/track" + alpncommon "github.com/gravitational/teleport/lib/srv/alpnproxy/common" "github.com/gravitational/teleport/lib/sshutils" "github.com/gravitational/teleport/lib/utils" "github.com/gravitational/teleport/lib/utils/proxy" @@ -283,7 +285,9 @@ func (a *Agent) connect() (conn *ssh.Client, err error) { } if a.reverseTunnelDetails != nil && a.reverseTunnelDetails.TLSRoutingEnabled { - opts = append(opts, proxy.WithALPNDialer()) + opts = append(opts, proxy.WithALPNDialer(&tls.Config{ + NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, + })) } for _, authMethod := range a.authMethods { diff --git a/lib/reversetunnel/transport.go b/lib/reversetunnel/transport.go index 73b593690ba49..65237ca5514e4 100644 --- a/lib/reversetunnel/transport.go +++ b/lib/reversetunnel/transport.go @@ -18,6 +18,7 @@ package reversetunnel import ( "context" + "crypto/tls" "encoding/json" "fmt" "io" @@ -34,6 +35,7 @@ import ( "github.com/gravitational/teleport/api/utils/sshutils" "github.com/gravitational/teleport/lib/auth" "github.com/gravitational/teleport/lib/events" + alpncommon "github.com/gravitational/teleport/lib/srv/alpnproxy/common" "github.com/gravitational/teleport/lib/utils" "github.com/gravitational/teleport/lib/utils/proxy" @@ -97,7 +99,9 @@ func (t *TunnelAuthDialer) DialContext(ctx context.Context, _, _ string) (net.Co // address thus the ping call will always fail. t.Log.Debugf("Failed to ping web proxy %q addr: %v", addr.Addr, err) } else if resp.Proxy.TLSRoutingEnabled { - opts = append(opts, proxy.WithALPNDialer()) + opts = append(opts, proxy.WithALPNDialer(&tls.Config{ + NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, + })) } dialer := proxy.DialerFromEnvironment(addr.Addr, opts...) diff --git a/lib/utils/proxy/proxy.go b/lib/utils/proxy/proxy.go index 19fe1972a996c..078d0d062e147 100644 --- a/lib/utils/proxy/proxy.go +++ b/lib/utils/proxy/proxy.go @@ -26,7 +26,6 @@ import ( "github.com/gravitational/teleport" apiclient "github.com/gravitational/teleport/api/client" "github.com/gravitational/teleport/api/utils/sshutils" - alpncommon "github.com/gravitational/teleport/lib/srv/alpnproxy/common" "github.com/gravitational/teleport/lib/utils" "golang.org/x/crypto/ssh" @@ -61,11 +60,6 @@ func (d directDial) dialALPNWithDeadline(network string, addr string, config *ss return nil, trace.Wrap(err) } conf := d.tlsConfig.Clone() - if conf == nil { - conf = &tls.Config{ - NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, - } - } conf.ServerName = address.Host() conf.InsecureSkipVerify = d.insecure tlsConn, err := tls.DialWithDialer(dialer, network, addr, conf) @@ -85,17 +79,20 @@ type Dialer interface { } type directDial struct { - // tlsRoutingEnabled indicates that proxy is running in TLSRouting mode. - tlsRoutingEnabled bool // insecure is whether to skip certificate validation. insecure bool // tlsConfig is the TLS config to use. tlsConfig *tls.Config } +// tlsRoutingEnabled indicates that proxy is running in TLSRouting mode. +func (d directDial) tlsRoutingEnabled() bool { + return d.tlsConfig != nil +} + // Dial calls ssh.Dial directly. func (d directDial) Dial(network string, addr string, config *ssh.ClientConfig) (*ssh.Client, error) { - if d.tlsRoutingEnabled { + if d.tlsRoutingEnabled() { client, err := d.dialALPNWithDeadline(network, addr, config) if err != nil { return nil, trace.Wrap(err) @@ -111,17 +108,12 @@ func (d directDial) Dial(network string, addr string, config *ssh.ClientConfig) // DialTimeout acts like Dial but takes a timeout. func (d directDial) DialTimeout(network, address string, timeout time.Duration) (net.Conn, error) { - if d.tlsRoutingEnabled { + if d.tlsRoutingEnabled() { addr, err := utils.ParseAddr(address) if err != nil { return nil, trace.Wrap(err) } conf := d.tlsConfig.Clone() - if conf == nil { - conf = &tls.Config{ - NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, - } - } conf.ServerName = addr.Host() conf.InsecureSkipVerify = d.insecure tlsConn, err := tls.Dial("tcp", address, conf) @@ -140,14 +132,17 @@ func (d directDial) DialTimeout(network, address string, timeout time.Duration) type proxyDial struct { // proxyHost is the HTTPS proxy address. proxyHost string - // tlsRoutingEnabled indicates that proxy is running in TLSRouting mode. - tlsRoutingEnabled bool // insecure is whether to skip certificate validation. insecure bool // tlsConfig is the TLS config to use. tlsConfig *tls.Config } +// tlsRoutingEnabled indicates that proxy is running in TLSRouting mode. +func (d proxyDial) tlsRoutingEnabled() bool { + return d.tlsConfig != nil +} + // DialTimeout acts like Dial but takes a timeout. func (d proxyDial) DialTimeout(network, address string, timeout time.Duration) (net.Conn, error) { // Build a proxy connection first. @@ -161,17 +156,12 @@ func (d proxyDial) DialTimeout(network, address string, timeout time.Duration) ( if err != nil { return nil, trace.Wrap(err) } - if d.tlsRoutingEnabled { + if d.tlsRoutingEnabled() { address, err := utils.ParseAddr(address) if err != nil { return nil, trace.Wrap(err) } conf := d.tlsConfig.Clone() - if conf == nil { - conf = &tls.Config{ - NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, - } - } conf.ServerName = address.Host() conf.InsecureSkipVerify = d.insecure conn = tls.Client(conn, conf) @@ -190,19 +180,14 @@ func (d proxyDial) Dial(network string, addr string, config *ssh.ClientConfig) ( if config.Timeout > 0 { pconn.SetReadDeadline(time.Now().Add(config.Timeout)) } - if d.tlsRoutingEnabled { + if d.tlsRoutingEnabled() { address, err := utils.ParseAddr(addr) if err != nil { return nil, trace.Wrap(err) } conf := d.tlsConfig.Clone() - if conf == nil { - conf = &tls.Config{ - NextProtos: []string{string(alpncommon.ProtocolReverseTunnel)}, - InsecureSkipVerify: d.insecure, - } - } conf.ServerName = address.Host() + conf.InsecureSkipVerify = d.insecure pconn = tls.Client(pconn, conf) } @@ -218,11 +203,9 @@ func (d proxyDial) Dial(network string, addr string, config *ssh.ClientConfig) ( } type dialerOptions struct { - // tlsRoutingEnabled indicates that proxy is running in TLSRouting mode. - tlsRoutingEnabled bool // insecureSkipTLSVerify is whether to skip certificate validation. insecureSkipTLSVerify bool - // tlsConfig is the TLS config to use. + // tlsConfig is the TLS config to use for TLS routing. tlsConfig *tls.Config } @@ -230,9 +213,9 @@ type dialerOptions struct { type DialerOptionFunc func(options *dialerOptions) // WithALPNDialer creates a dialer that allows to Teleport running in single-port mode. -func WithALPNDialer() DialerOptionFunc { +func WithALPNDialer(tlsConfig *tls.Config) DialerOptionFunc { return func(options *dialerOptions) { - options.tlsRoutingEnabled = true + options.tlsConfig = tlsConfig } } @@ -243,13 +226,6 @@ func WithInsecureSkipTLSVerify(insecure bool) DialerOptionFunc { } } -// WithTLSConfig creates a dialer that uses a specific tls config. -func WithTLSConfig(tlsConfig *tls.Config) DialerOptionFunc { - return func(options *dialerOptions) { - options.tlsConfig = tlsConfig - } -} - // DialerFromEnvironment returns a Dial function. If the https_proxy or http_proxy // environment variable are set, it returns a function that will dial through // said proxy server. If neither variable is set, it will connect to the SSH @@ -268,17 +244,15 @@ func DialerFromEnvironment(addr string, opts ...DialerOptionFunc) Dialer { if proxyAddr == "" { log.Debugf("No proxy set in environment, returning direct dialer.") return directDial{ - tlsRoutingEnabled: options.tlsRoutingEnabled, - insecure: options.insecureSkipTLSVerify, - tlsConfig: options.tlsConfig, + insecure: options.insecureSkipTLSVerify, + tlsConfig: options.tlsConfig, } } log.Debugf("Found proxy %q in environment, returning proxy dialer.", proxyAddr) return proxyDial{ - proxyHost: proxyAddr, - tlsRoutingEnabled: options.tlsRoutingEnabled, - insecure: options.insecureSkipTLSVerify, - tlsConfig: options.tlsConfig, + proxyHost: proxyAddr, + insecure: options.insecureSkipTLSVerify, + tlsConfig: options.tlsConfig, } } From 4e2a20aa79f58de4cfce0d500d8ccd044e192be4 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Mon, 14 Mar 2022 12:02:42 -0700 Subject: [PATCH 21/29] Bring back tlsRoutingEnabled flag --- lib/utils/proxy/proxy.go | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/lib/utils/proxy/proxy.go b/lib/utils/proxy/proxy.go index 078d0d062e147..12961ca9beee8 100644 --- a/lib/utils/proxy/proxy.go +++ b/lib/utils/proxy/proxy.go @@ -81,18 +81,15 @@ type Dialer interface { type directDial struct { // insecure is whether to skip certificate validation. insecure bool + // tlsRoutingEnabled indicates that proxy is running in TLSRouting mode. + tlsRoutingEnabled bool // tlsConfig is the TLS config to use. tlsConfig *tls.Config } -// tlsRoutingEnabled indicates that proxy is running in TLSRouting mode. -func (d directDial) tlsRoutingEnabled() bool { - return d.tlsConfig != nil -} - // Dial calls ssh.Dial directly. func (d directDial) Dial(network string, addr string, config *ssh.ClientConfig) (*ssh.Client, error) { - if d.tlsRoutingEnabled() { + if d.tlsRoutingEnabled { client, err := d.dialALPNWithDeadline(network, addr, config) if err != nil { return nil, trace.Wrap(err) @@ -108,7 +105,7 @@ func (d directDial) Dial(network string, addr string, config *ssh.ClientConfig) // DialTimeout acts like Dial but takes a timeout. func (d directDial) DialTimeout(network, address string, timeout time.Duration) (net.Conn, error) { - if d.tlsRoutingEnabled() { + if d.tlsRoutingEnabled { addr, err := utils.ParseAddr(address) if err != nil { return nil, trace.Wrap(err) @@ -134,15 +131,12 @@ type proxyDial struct { proxyHost string // insecure is whether to skip certificate validation. insecure bool + // tlsRoutingEnabled indicates that proxy is running in TLSRouting mode. + tlsRoutingEnabled bool // tlsConfig is the TLS config to use. tlsConfig *tls.Config } -// tlsRoutingEnabled indicates that proxy is running in TLSRouting mode. -func (d proxyDial) tlsRoutingEnabled() bool { - return d.tlsConfig != nil -} - // DialTimeout acts like Dial but takes a timeout. func (d proxyDial) DialTimeout(network, address string, timeout time.Duration) (net.Conn, error) { // Build a proxy connection first. @@ -156,7 +150,7 @@ func (d proxyDial) DialTimeout(network, address string, timeout time.Duration) ( if err != nil { return nil, trace.Wrap(err) } - if d.tlsRoutingEnabled() { + if d.tlsRoutingEnabled { address, err := utils.ParseAddr(address) if err != nil { return nil, trace.Wrap(err) @@ -180,7 +174,7 @@ func (d proxyDial) Dial(network string, addr string, config *ssh.ClientConfig) ( if config.Timeout > 0 { pconn.SetReadDeadline(time.Now().Add(config.Timeout)) } - if d.tlsRoutingEnabled() { + if d.tlsRoutingEnabled { address, err := utils.ParseAddr(addr) if err != nil { return nil, trace.Wrap(err) @@ -205,6 +199,8 @@ func (d proxyDial) Dial(network string, addr string, config *ssh.ClientConfig) ( type dialerOptions struct { // insecureSkipTLSVerify is whether to skip certificate validation. insecureSkipTLSVerify bool + // tlsRoutingEnabled indicates that proxy is running in TLSRouting mode. + tlsRoutingEnabled bool // tlsConfig is the TLS config to use for TLS routing. tlsConfig *tls.Config } @@ -215,6 +211,7 @@ type DialerOptionFunc func(options *dialerOptions) // WithALPNDialer creates a dialer that allows to Teleport running in single-port mode. func WithALPNDialer(tlsConfig *tls.Config) DialerOptionFunc { return func(options *dialerOptions) { + options.tlsRoutingEnabled = true options.tlsConfig = tlsConfig } } @@ -244,15 +241,17 @@ func DialerFromEnvironment(addr string, opts ...DialerOptionFunc) Dialer { if proxyAddr == "" { log.Debugf("No proxy set in environment, returning direct dialer.") return directDial{ - insecure: options.insecureSkipTLSVerify, - tlsConfig: options.tlsConfig, + insecure: options.insecureSkipTLSVerify, + tlsRoutingEnabled: options.tlsRoutingEnabled, + tlsConfig: options.tlsConfig, } } log.Debugf("Found proxy %q in environment, returning proxy dialer.", proxyAddr) return proxyDial{ - proxyHost: proxyAddr, - insecure: options.insecureSkipTLSVerify, - tlsConfig: options.tlsConfig, + proxyHost: proxyAddr, + insecure: options.insecureSkipTLSVerify, + tlsRoutingEnabled: options.tlsRoutingEnabled, + tlsConfig: options.tlsConfig, } } From cad2d6cd66ad692b7804c050fd3dfdd44932580f Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Tue, 15 Mar 2022 11:24:29 -0700 Subject: [PATCH 22/29] Move tls config configuration to its own function --- lib/utils/proxy/proxy.go | 50 ++++++++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/lib/utils/proxy/proxy.go b/lib/utils/proxy/proxy.go index 12961ca9beee8..90c8226cced13 100644 --- a/lib/utils/proxy/proxy.go +++ b/lib/utils/proxy/proxy.go @@ -59,9 +59,10 @@ func (d directDial) dialALPNWithDeadline(network string, addr string, config *ss if err != nil { return nil, trace.Wrap(err) } - conf := d.tlsConfig.Clone() - conf.ServerName = address.Host() - conf.InsecureSkipVerify = d.insecure + conf, err := d.getTLSConfig(address) + if err != nil { + return nil, trace.Wrap(err) + } tlsConn, err := tls.DialWithDialer(dialer, network, addr, conf) if err != nil { return nil, trace.Wrap(err) @@ -87,6 +88,17 @@ type directDial struct { tlsConfig *tls.Config } +// getTLSConfig configures the dialers TLS config for a specified address. +func (d directDial) getTLSConfig(addr *utils.NetAddr) (*tls.Config, error) { + if d.tlsConfig == nil { + return nil, trace.BadParameter("TLS config was nil") + } + tlsConfig := d.tlsConfig.Clone() + tlsConfig.ServerName = addr.Host() + tlsConfig.InsecureSkipVerify = d.insecure + return tlsConfig, nil +} + // Dial calls ssh.Dial directly. func (d directDial) Dial(network string, addr string, config *ssh.ClientConfig) (*ssh.Client, error) { if d.tlsRoutingEnabled { @@ -110,9 +122,10 @@ func (d directDial) DialTimeout(network, address string, timeout time.Duration) if err != nil { return nil, trace.Wrap(err) } - conf := d.tlsConfig.Clone() - conf.ServerName = addr.Host() - conf.InsecureSkipVerify = d.insecure + conf, err := d.getTLSConfig(addr) + if err != nil { + return nil, trace.Wrap(err) + } tlsConn, err := tls.Dial("tcp", address, conf) if err != nil { return nil, trace.Wrap(err) @@ -137,6 +150,17 @@ type proxyDial struct { tlsConfig *tls.Config } +// getTLSConfig configures the dialers TLS config for a specified address. +func (d proxyDial) getTLSConfig(addr *utils.NetAddr) (*tls.Config, error) { + if d.tlsConfig == nil { + return nil, trace.BadParameter("TLS config was nil") + } + tlsConfig := d.tlsConfig.Clone() + tlsConfig.ServerName = addr.Host() + tlsConfig.InsecureSkipVerify = d.insecure + return tlsConfig, nil +} + // DialTimeout acts like Dial but takes a timeout. func (d proxyDial) DialTimeout(network, address string, timeout time.Duration) (net.Conn, error) { // Build a proxy connection first. @@ -155,9 +179,10 @@ func (d proxyDial) DialTimeout(network, address string, timeout time.Duration) ( if err != nil { return nil, trace.Wrap(err) } - conf := d.tlsConfig.Clone() - conf.ServerName = address.Host() - conf.InsecureSkipVerify = d.insecure + conf, err := d.getTLSConfig(address) + if err != nil { + return nil, trace.Wrap(err) + } conn = tls.Client(conn, conf) } return conn, nil @@ -179,9 +204,10 @@ func (d proxyDial) Dial(network string, addr string, config *ssh.ClientConfig) ( if err != nil { return nil, trace.Wrap(err) } - conf := d.tlsConfig.Clone() - conf.ServerName = address.Host() - conf.InsecureSkipVerify = d.insecure + conf, err := d.getTLSConfig(address) + if err != nil { + return nil, trace.Wrap(err) + } pconn = tls.Client(pconn, conf) } From df2853e10474ab428a60693692d9113421a2b56a Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Tue, 22 Mar 2022 10:09:12 -0700 Subject: [PATCH 23/29] Remove siddontang logger from proxy.go --- api/client/proxy.go | 2 +- api/go.mod | 1 - api/go.sum | 2 -- 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/api/client/proxy.go b/api/client/proxy.go index 42d448c8e9040..630066be045e8 100644 --- a/api/client/proxy.go +++ b/api/client/proxy.go @@ -27,7 +27,7 @@ import ( "github.com/gravitational/teleport/api/constants" "github.com/gravitational/trace" - "github.com/siddontang/go/log" + log "github.com/sirupsen/logrus" ) // DialProxy creates a connection to a server via an HTTP Proxy. diff --git a/api/go.mod b/api/go.mod index a81a73769382d..3a7210c771f25 100644 --- a/api/go.mod +++ b/api/go.mod @@ -8,7 +8,6 @@ require ( github.com/google/go-cmp v0.5.4 github.com/gravitational/trace v1.1.17 github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect - github.com/siddontang/go v0.0.0-20180604090527-bdc77568d726 github.com/sirupsen/logrus v1.8.1 github.com/stretchr/testify v1.7.0 golang.org/x/crypto v0.0.0-20220126234351-aa10faf2a1f8 diff --git a/api/go.sum b/api/go.sum index 69ab03da359fa..72c1f18217730 100644 --- a/api/go.sum +++ b/api/go.sum @@ -61,8 +61,6 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA= github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ= -github.com/siddontang/go v0.0.0-20180604090527-bdc77568d726 h1:xT+JlYxNGqyT+XcU8iUrN18JYed2TvG9yN5ULG2jATM= -github.com/siddontang/go v0.0.0-20180604090527-bdc77568d726/go.mod h1:3yhqj7WBBfRhbBlzyOC3gUxftwsU0u8gqevxwIHQpMw= github.com/sirupsen/logrus v1.8.1 h1:dJKuHgqk1NNQlqoA6BTlM1Wf9DOH3NBjQyu0h9+AZZE= github.com/sirupsen/logrus v1.8.1/go.mod h1:yWOB1SBYBC5VeMP7gHvWumXLIWorT60ONWic61uBYv0= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= From 23fdd82541c4de8dd6d31bb33d202f7195ef7e68 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Tue, 22 Mar 2022 15:20:00 -0700 Subject: [PATCH 24/29] Address review comments --- api/client/contextdialer.go | 8 ++++---- api/client/proxy.go | 4 ++-- api/client/webclient/webclient_test.go | 13 +++++++------ lib/client/https_client_test.go | 25 +++++++++++++------------ 4 files changed, 26 insertions(+), 24 deletions(-) diff --git a/api/client/contextdialer.go b/api/client/contextdialer.go index 5c3725c5a1631..b07af84097e9a 100644 --- a/api/client/contextdialer.go +++ b/api/client/contextdialer.go @@ -44,8 +44,8 @@ func (f ContextDialerFunc) DialContext(ctx context.Context, network, addr string return f(ctx, network, addr) } -// NewDirectDialer makes a new dialer to connect directly to an Auth server. -func NewDirectDialer(keepAlivePeriod, dialTimeout time.Duration) ContextDialer { +// newDirectDialer makes a new dialer to connect directly to an Auth server. +func newDirectDialer(keepAlivePeriod, dialTimeout time.Duration) ContextDialer { return &net.Dialer{ Timeout: dialTimeout, KeepAlive: keepAlivePeriod, @@ -55,8 +55,8 @@ func NewDirectDialer(keepAlivePeriod, dialTimeout time.Duration) ContextDialer { // NewDialer makes a new dialer that connects to an Auth server either directly or via an HTTP proxy, depending // on the environment. func NewDialer(keepAlivePeriod, dialTimeout time.Duration) ContextDialer { - dialer := NewDirectDialer(keepAlivePeriod, dialTimeout) return ContextDialerFunc(func(ctx context.Context, network, addr string) (net.Conn, error) { + dialer := newDirectDialer(keepAlivePeriod, dialTimeout) if proxyAddr := GetProxyAddress(addr); proxyAddr != "" { return DialProxyWithDialer(ctx, proxyAddr, addr, dialer) } @@ -84,7 +84,7 @@ func NewProxyDialer(ssh ssh.ClientConfig, keepAlivePeriod, dialTimeout time.Dura // newTunnelDialer makes a dialer to connect to an Auth server through the SSH reverse tunnel on the proxy. func newTunnelDialer(ssh ssh.ClientConfig, keepAlivePeriod, dialTimeout time.Duration) ContextDialer { - dialer := NewDirectDialer(keepAlivePeriod, dialTimeout) + dialer := newDirectDialer(keepAlivePeriod, dialTimeout) return ContextDialerFunc(func(ctx context.Context, network, addr string) (conn net.Conn, err error) { conn, err = dialer.DialContext(ctx, network, addr) if err != nil { diff --git a/api/client/proxy.go b/api/client/proxy.go index 630066be045e8..9d33a1ab23730 100644 --- a/api/client/proxy.go +++ b/api/client/proxy.go @@ -49,8 +49,8 @@ func DialProxyWithDialer(ctx context.Context, proxyAddr, addr string, dialer Con Host: addr, Header: make(http.Header), } - err = connectReq.Write(conn) - if err != nil { + + if err := connectReq.Write(conn); err != nil { log.Warnf("Unable to write to proxy: %v.", err) return nil, trace.Wrap(err) } diff --git a/api/client/webclient/webclient_test.go b/api/client/webclient/webclient_test.go index 963b6b5f2156c..2e54d6f97838a 100644 --- a/api/client/webclient/webclient_test.go +++ b/api/client/webclient/webclient_test.go @@ -295,19 +295,20 @@ func TestNewWebClientRespectHTTPProxy(t *testing.T) { client := newWebClient(false /* insecure */, nil /* pool */) // resp should be nil, so there will be no body to close. //nolint:bodyclose - resp, err := client.Get("https://example.com") + resp, err := client.Get("https://fakedomain.example.com") // Client should try to proxy through nonexistent server at localhost. require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") - require.Contains(t, err.Error(), "no such host") + require.Contains(t, err.Error(), "lookup fakeproxy.example.com: no such host") } func TestNewWebClientNoProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") t.Setenv("NO_PROXY", "example.com") client := newWebClient(false /* insecure */, nil /* pool */) - resp, err := client.Get("https://example.com") - require.NoError(t, err) - defer resp.Body.Close() - require.Equal(t, http.StatusOK, resp.StatusCode) + //nolint:bodyclose + resp, err := client.Get("https://fakedomain.example.com") + require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) + require.NotContains(t, err.Error(), "proxyconnect") + require.Contains(t, err.Error(), "lookup fakedomain.example.com: no such host") } diff --git a/lib/client/https_client_test.go b/lib/client/https_client_test.go index ea5fac1a89a4d..4987692287625 100644 --- a/lib/client/https_client_test.go +++ b/lib/client/https_client_test.go @@ -17,7 +17,6 @@ limitations under the License. package client import ( - "net/http" "testing" "github.com/stretchr/testify/require" @@ -32,17 +31,18 @@ func TestNewInsecureWebClientHTTPProxy(t *testing.T) { // Client should try to proxy through nonexistent server at localhost. require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") - require.Contains(t, err.Error(), "no such host") + require.Contains(t, err.Error(), "lookup fakeproxy.example.com: no such host") } func TestNewInsecureWebClientNoProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") t.Setenv("NO_PROXY", "example.com") client := NewInsecureWebClient() - resp, err := client.Get("https://example.com") - require.NoError(t, err) - defer resp.Body.Close() - require.Equal(t, http.StatusOK, resp.StatusCode) + //nolint:bodyclose + resp, err := client.Get("https://fakedomain.example.com") + require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) + require.NotContains(t, err.Error(), "proxyconnect") + require.Contains(t, err.Error(), "lookup fakedomain.example.com: no such host") } func TestNewClientWithPoolHTTPProxy(t *testing.T) { @@ -50,19 +50,20 @@ func TestNewClientWithPoolHTTPProxy(t *testing.T) { client := newClientWithPool(nil) // resp should be nil, so there will be no body to close. //nolint:bodyclose - resp, err := client.Get("https://example.com") + resp, err := client.Get("https://fakedomain.example.com") // Client should try to proxy through nonexistent server at localhost. require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") - require.Contains(t, err.Error(), "no such host") + require.Contains(t, err.Error(), "lookup fakeproxy.example.com: no such host") } func TestNewClientWithPoolNoProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") t.Setenv("NO_PROXY", "example.com") client := newClientWithPool(nil) - resp, err := client.Get("https://example.com") - require.NoError(t, err) - defer resp.Body.Close() - require.Equal(t, http.StatusOK, resp.StatusCode) + //nolint:bodyclose + resp, err := client.Get("https://fakedomain.example.com") + require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) + require.NotContains(t, err.Error(), "proxyconnect") + require.Contains(t, err.Error(), "lookup fakedomain.example.com: no such host") } From a94ce4b1b000913408e7568f237978669e68925f Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Tue, 22 Mar 2022 15:28:40 -0700 Subject: [PATCH 25/29] Fix error message checks in tests --- api/client/webclient/webclient_test.go | 6 ++++-- lib/client/https_client_test.go | 12 ++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/api/client/webclient/webclient_test.go b/api/client/webclient/webclient_test.go index 2e54d6f97838a..0a7ef992560b3 100644 --- a/api/client/webclient/webclient_test.go +++ b/api/client/webclient/webclient_test.go @@ -299,7 +299,8 @@ func TestNewWebClientRespectHTTPProxy(t *testing.T) { // Client should try to proxy through nonexistent server at localhost. require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") - require.Contains(t, err.Error(), "lookup fakeproxy.example.com: no such host") + require.Contains(t, err.Error(), "lookup fakeproxy.example.com") + require.Contains(t, err.Error(), "no such host") } func TestNewWebClientNoProxy(t *testing.T) { @@ -310,5 +311,6 @@ func TestNewWebClientNoProxy(t *testing.T) { resp, err := client.Get("https://fakedomain.example.com") require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.NotContains(t, err.Error(), "proxyconnect") - require.Contains(t, err.Error(), "lookup fakedomain.example.com: no such host") + require.Contains(t, err.Error(), "lookup fakedomain.example.com") + require.Contains(t, err.Error(), "no such host") } diff --git a/lib/client/https_client_test.go b/lib/client/https_client_test.go index 4987692287625..d5b6c246e61e8 100644 --- a/lib/client/https_client_test.go +++ b/lib/client/https_client_test.go @@ -31,7 +31,8 @@ func TestNewInsecureWebClientHTTPProxy(t *testing.T) { // Client should try to proxy through nonexistent server at localhost. require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") - require.Contains(t, err.Error(), "lookup fakeproxy.example.com: no such host") + require.Contains(t, err.Error(), "lookup fakeproxy.example.com") + require.Contains(t, err.Error(), "no such host") } func TestNewInsecureWebClientNoProxy(t *testing.T) { @@ -42,7 +43,8 @@ func TestNewInsecureWebClientNoProxy(t *testing.T) { resp, err := client.Get("https://fakedomain.example.com") require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.NotContains(t, err.Error(), "proxyconnect") - require.Contains(t, err.Error(), "lookup fakedomain.example.com: no such host") + require.Contains(t, err.Error(), "lookup fakedomain.example.com") + require.Contains(t, err.Error(), "no such host") } func TestNewClientWithPoolHTTPProxy(t *testing.T) { @@ -54,7 +56,8 @@ func TestNewClientWithPoolHTTPProxy(t *testing.T) { // Client should try to proxy through nonexistent server at localhost. require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") - require.Contains(t, err.Error(), "lookup fakeproxy.example.com: no such host") + require.Contains(t, err.Error(), "lookup fakeproxy.example.com") + require.Contains(t, err.Error(), "no such host") } func TestNewClientWithPoolNoProxy(t *testing.T) { @@ -65,5 +68,6 @@ func TestNewClientWithPoolNoProxy(t *testing.T) { resp, err := client.Get("https://fakedomain.example.com") require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.NotContains(t, err.Error(), "proxyconnect") - require.Contains(t, err.Error(), "lookup fakedomain.example.com: no such host") + require.Contains(t, err.Error(), "lookup fakedomain.example.com") + require.Contains(t, err.Error(), "no such host") } From dd2cb235772f3c7b315a65afe83da1f4c813d84b Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Tue, 22 Mar 2022 15:58:14 -0700 Subject: [PATCH 26/29] Fix missed url update --- lib/client/https_client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/client/https_client_test.go b/lib/client/https_client_test.go index d5b6c246e61e8..7d8935d1b2ce8 100644 --- a/lib/client/https_client_test.go +++ b/lib/client/https_client_test.go @@ -27,7 +27,7 @@ func TestNewInsecureWebClientHTTPProxy(t *testing.T) { client := NewInsecureWebClient() // resp should be nil, so there will be no body to close. //nolint:bodyclose - resp, err := client.Get("https://example.com") + resp, err := client.Get("https://fakedomain.example.com") // Client should try to proxy through nonexistent server at localhost. require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) require.Contains(t, err.Error(), "proxyconnect") From 2815f4c49cb9147a8fadb1673f8fb0c41f245e33 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Tue, 22 Mar 2022 15:59:30 -0700 Subject: [PATCH 27/29] Fix no_proxy value in tests --- api/client/webclient/webclient_test.go | 2 +- lib/client/https_client_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/client/webclient/webclient_test.go b/api/client/webclient/webclient_test.go index 0a7ef992560b3..5e8caf6da0c56 100644 --- a/api/client/webclient/webclient_test.go +++ b/api/client/webclient/webclient_test.go @@ -305,7 +305,7 @@ func TestNewWebClientRespectHTTPProxy(t *testing.T) { func TestNewWebClientNoProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") - t.Setenv("NO_PROXY", "example.com") + t.Setenv("NO_PROXY", "fakedomain.example.com") client := newWebClient(false /* insecure */, nil /* pool */) //nolint:bodyclose resp, err := client.Get("https://fakedomain.example.com") diff --git a/lib/client/https_client_test.go b/lib/client/https_client_test.go index 7d8935d1b2ce8..66adb430ef035 100644 --- a/lib/client/https_client_test.go +++ b/lib/client/https_client_test.go @@ -37,7 +37,7 @@ func TestNewInsecureWebClientHTTPProxy(t *testing.T) { func TestNewInsecureWebClientNoProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") - t.Setenv("NO_PROXY", "example.com") + t.Setenv("NO_PROXY", "fakedomain.example.com") client := NewInsecureWebClient() //nolint:bodyclose resp, err := client.Get("https://fakedomain.example.com") @@ -62,7 +62,7 @@ func TestNewClientWithPoolHTTPProxy(t *testing.T) { func TestNewClientWithPoolNoProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") - t.Setenv("NO_PROXY", "example.com") + t.Setenv("NO_PROXY", "fakedomain.example.com") client := newClientWithPool(nil) //nolint:bodyclose resp, err := client.Get("https://fakedomain.example.com") From b343ff8bda9a17e2604910b22510c25a3796ea4e Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Wed, 23 Mar 2022 10:31:33 -0700 Subject: [PATCH 28/29] Tweak newWebClient arg description --- api/client/webclient/webclient_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/api/client/webclient/webclient_test.go b/api/client/webclient/webclient_test.go index 5e8caf6da0c56..9632a957410a0 100644 --- a/api/client/webclient/webclient_test.go +++ b/api/client/webclient/webclient_test.go @@ -292,7 +292,8 @@ func TestExtract(t *testing.T) { func TestNewWebClientRespectHTTPProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") - client := newWebClient(false /* insecure */, nil /* pool */) + // Insecure client with no pool. + client := newWebClient(false, nil) // resp should be nil, so there will be no body to close. //nolint:bodyclose resp, err := client.Get("https://fakedomain.example.com") @@ -306,7 +307,8 @@ func TestNewWebClientRespectHTTPProxy(t *testing.T) { func TestNewWebClientNoProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") t.Setenv("NO_PROXY", "fakedomain.example.com") - client := newWebClient(false /* insecure */, nil /* pool */) + // Insecure client with no pool. + client := newWebClient(false, nil) //nolint:bodyclose resp, err := client.Get("https://fakedomain.example.com") require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) From b6dff620d5c4a0e8bbad262307bb6a763a4958f9 Mon Sep 17 00:00:00 2001 From: Andrew Burke Date: Wed, 23 Mar 2022 10:58:04 -0700 Subject: [PATCH 29/29] Fix newWebClient signature in tests --- api/client/webclient/webclient_test.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/api/client/webclient/webclient_test.go b/api/client/webclient/webclient_test.go index b2dce27685359..b908a9777a718 100644 --- a/api/client/webclient/webclient_test.go +++ b/api/client/webclient/webclient_test.go @@ -293,8 +293,11 @@ func TestExtract(t *testing.T) { func TestNewWebClientRespectHTTPProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") - // Insecure client with no pool. - client := newWebClient(false, nil) + client, err := newWebClient(&Config{ + Context: context.Background(), + ProxyAddr: "localhost:3080", + }) + require.NoError(t, err) // resp should be nil, so there will be no body to close. //nolint:bodyclose resp, err := client.Get("https://fakedomain.example.com") @@ -308,8 +311,11 @@ func TestNewWebClientRespectHTTPProxy(t *testing.T) { func TestNewWebClientNoProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") t.Setenv("NO_PROXY", "fakedomain.example.com") - // Insecure client with no pool. - client := newWebClient(false, nil) + client, err := newWebClient(&Config{ + Context: context.Background(), + ProxyAddr: "localhost:3080", + }) + require.NoError(t, err) //nolint:bodyclose resp, err := client.Get("https://fakedomain.example.com") require.Error(t, err, "GET unexpectedly succeeded: %+v", resp)