From b1c9acbb40a7a941c98c322e9dfb33589f646262 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Fri, 30 Dec 2022 16:32:19 +0000 Subject: [PATCH 01/18] Set extra proxy headers in all `tsh` HTTP requests Before this commit, the `tsh` HTTP requests that had the extra headers were those that did not use `roundtrip`. This commit leverages `http.RoundTripper.RoundTrip` to ensure that all requests have the the extra headers. --- api/client/proxy/proxy.go | 31 +++++++---- api/client/webclient/webclient.go | 2 +- lib/client/api.go | 8 ++- lib/client/https_client.go | 31 +++-------- lib/client/https_client_test.go | 8 +-- lib/client/redirect.go | 8 ++- lib/client/weblogin.go | 92 +++++++++++++++++++++++-------- 7 files changed, 115 insertions(+), 65 deletions(-) diff --git a/api/client/proxy/proxy.go b/api/client/proxy/proxy.go index 7b95b3ba8a4df..59111e782a8ee 100644 --- a/api/client/proxy/proxy.go +++ b/api/client/proxy/proxy.go @@ -71,32 +71,39 @@ func parse(addr string) (*url.URL, error) { return addrURL, nil } -// HTTPFallbackRoundTripper is a wrapper for http.Transport that downgrades requests -// to plain HTTP when using a plain HTTP proxy at localhost. -type HTTPFallbackRoundTripper struct { +// HTTPRoundTripper is a wrapper for http.Transport that +// - adds extra HTTP headers to all requests, and +// - downgrades requests to plain HTTP when using a plain HTTP proxy at localhost. +type HTTPRoundTripper struct { *http.Transport + // extraHeaders is a map of extra HTTP headers to be included in requests. + extraHeaders map[string]string isProxyHTTPLocalhost bool } -// NewHTTPFallbackRoundTripper creates a new initialized HTTP fallback roundtripper. -func NewHTTPFallbackRoundTripper(transport *http.Transport, insecure bool) *HTTPFallbackRoundTripper { +// NewHTTPRoundTripper creates a new initialized HTTP roundtripper. +func NewHTTPRoundTripper(transport *http.Transport, extraHeaders map[string]string) *HTTPRoundTripper { proxyConfig := httpproxy.FromEnvironment() - rt := HTTPFallbackRoundTripper{ + rt := &HTTPRoundTripper{ Transport: transport, + extraHeaders: extraHeaders, isProxyHTTPLocalhost: strings.HasPrefix(proxyConfig.HTTPProxy, "http://localhost"), } - if rt.TLSClientConfig != nil { - rt.TLSClientConfig.InsecureSkipVerify = insecure - } - return &rt + return rt } // RoundTrip executes a single HTTP transaction. Part of the RoundTripper interface. -func (rt *HTTPFallbackRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { - tlsConfig := rt.Transport.TLSClientConfig +func (rt *HTTPRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + // Add extra HTTP headers. + for header, v := range rt.extraHeaders { + req.Header.Add(header, v) + } + // Use plain HTTP if proxying via http://localhost in insecure mode. + tlsConfig := rt.Transport.TLSClientConfig if rt.isProxyHTTPLocalhost && tlsConfig != nil && tlsConfig.InsecureSkipVerify { req.URL.Scheme = "http" } + return rt.Transport.RoundTrip(req) } diff --git a/api/client/webclient/webclient.go b/api/client/webclient/webclient.go index 8862fa254b5fb..f5226daed5ecd 100644 --- a/api/client/webclient/webclient.go +++ b/api/client/webclient/webclient.go @@ -103,7 +103,7 @@ func newWebClient(cfg *Config) (*http.Client, error) { } return &http.Client{ Transport: otelhttp.NewTransport( - proxy.NewHTTPFallbackRoundTripper(&transport, cfg.Insecure), + proxy.NewHTTPRoundTripper(&transport, nil), otelhttp.WithSpanNameFormatter(tracing.HTTPTransportFormatter), ), Timeout: cfg.Timeout, diff --git a/lib/client/api.go b/lib/client/api.go index f10be6954a734..094a929359f90 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -3344,7 +3344,12 @@ func makeProxySSHClient(ctx context.Context, tc *TeleportClient, sshConfig *ssh. if len(tc.JumpHosts) > 0 { sshProxyAddr = tc.JumpHosts[0].Addr.Addr // Check if JumpHost address is a proxy web address. - resp, err := webclient.Find(&webclient.Config{Context: ctx, ProxyAddr: sshProxyAddr, Insecure: tc.InsecureSkipVerify}) + resp, err := webclient.Find(&webclient.Config{ + Context: ctx, + ProxyAddr: sshProxyAddr, + Insecure: tc.InsecureSkipVerify, + ExtraHeaders: tc.ExtraProxyHeaders, + }) // If JumpHost address is a proxy web port and proxy supports TLSRouting dial proxy with TLSWrapper. if err == nil && resp.Proxy.TLSRoutingEnabled { log.Infof("Connecting to proxy=%v login=%q using TLS Routing JumpHost", sshProxyAddr, sshConfig.User) @@ -3830,6 +3835,7 @@ func (tc *TeleportClient) newSSHLogin(priv *keys.PrivateKey) (SSHLogin, error) { RouteToCluster: tc.SiteName, KubernetesCluster: tc.KubernetesCluster, AttestationStatement: attestationStatement, + ExtraHeaders: tc.ExtraProxyHeaders, }, nil } diff --git a/lib/client/https_client.go b/lib/client/https_client.go index 4e7f4c8e43e08..027e0e8a620aa 100644 --- a/lib/client/https_client.go +++ b/lib/client/https_client.go @@ -36,40 +36,27 @@ import ( "github.com/gravitational/teleport/lib/utils" ) -func NewInsecureWebClient() *http.Client { - // Because Teleport clients can't be configured (yet), they take the default - // list of cipher suites from Go. - tlsConfig := utils.TLSConfig(nil) - transport := http.Transport{ - TLSClientConfig: tlsConfig, - Proxy: func(req *http.Request) (*url.URL, error) { - return httpproxy.FromEnvironment().ProxyFunc()(req.URL) - }, - } +func newClient(insecure bool, pool *x509.CertPool, extraHeaders map[string]string) *http.Client { return &http.Client{ Transport: otelhttp.NewTransport( - apiproxy.NewHTTPFallbackRoundTripper(&transport, true /* insecure */), + apiproxy.NewHTTPRoundTripper(httpTransport(insecure, pool), extraHeaders), otelhttp.WithSpanNameFormatter(tracing.HTTPTransportFormatter), ), } } -func newClientWithPool(pool *x509.CertPool) *http.Client { +func httpTransport(insecure bool, pool *x509.CertPool) *http.Transport { // Because Teleport clients can't be configured (yet), they take the default // list of cipher suites from Go. tlsConfig := utils.TLSConfig(nil) + tlsConfig.InsecureSkipVerify = insecure tlsConfig.RootCAs = pool - return &http.Client{ - Transport: otelhttp.NewTransport( - &http.Transport{ - TLSClientConfig: tlsConfig, - Proxy: func(req *http.Request) (*url.URL, error) { - return httpproxy.FromEnvironment().ProxyFunc()(req.URL) - }, - }, - otelhttp.WithSpanNameFormatter(tracing.HTTPTransportFormatter), - ), + return &http.Transport{ + TLSClientConfig: tlsConfig, + Proxy: func(req *http.Request) (*url.URL, error) { + return httpproxy.FromEnvironment().ProxyFunc()(req.URL) + }, } } diff --git a/lib/client/https_client_test.go b/lib/client/https_client_test.go index 7482df7fd0ea8..1b4645d49c79d 100644 --- a/lib/client/https_client_test.go +++ b/lib/client/https_client_test.go @@ -24,7 +24,7 @@ import ( func TestNewInsecureWebClientHTTPProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") - client := NewInsecureWebClient() + client := newClient(true, nil, nil) //nolint:bodyclose // resp should be nil, so there will be no body to close. resp, err := client.Get("https://fakedomain.example.com") // Client should try to proxy through nonexistent server at localhost. @@ -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", "fakedomain.example.com") - client := NewInsecureWebClient() + client := newClient(true, nil, nil) //nolint:bodyclose // resp should be nil, so there will be no body to close. resp, err := client.Get("https://fakedomain.example.com") require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) @@ -48,7 +48,7 @@ func TestNewInsecureWebClientNoProxy(t *testing.T) { func TestNewClientWithPoolHTTPProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") - client := newClientWithPool(nil) + client := newClient(false, nil, nil) //nolint:bodyclose // resp should be nil, so there will be no body to close. resp, err := client.Get("https://fakedomain.example.com") // Client should try to proxy through nonexistent server at localhost. @@ -61,7 +61,7 @@ func TestNewClientWithPoolHTTPProxy(t *testing.T) { func TestNewClientWithPoolNoProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") t.Setenv("NO_PROXY", "fakedomain.example.com") - client := newClientWithPool(nil) + client := newClient(false, nil, nil) //nolint:bodyclose // resp should be nil, so there will be no body to close. resp, err := client.Get("https://fakedomain.example.com") require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) diff --git a/lib/client/redirect.go b/lib/client/redirect.go index 419d69ff0bb2a..8460992c35d85 100644 --- a/lib/client/redirect.go +++ b/lib/client/redirect.go @@ -91,7 +91,13 @@ type RedirectorConfig struct { // NewRedirector returns new local web server redirector func NewRedirector(ctx context.Context, login SSHLoginSSO, config *RedirectorConfig) (*Redirector, error) { - clt, proxyURL, err := initClient(login.ProxyAddr, login.Insecure, login.Pool) + cltConfig := ClientConfig{ + ProxyAddr: login.ProxyAddr, + Insecure: login.Insecure, + Pool: login.Pool, + ExtraHeaders: login.ExtraHeaders, + } + clt, proxyURL, err := initClient(cltConfig) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/client/weblogin.go b/lib/client/weblogin.go index 51687be3e5173..f6d4e63768c40 100644 --- a/lib/client/weblogin.go +++ b/lib/client/weblogin.go @@ -172,7 +172,7 @@ type SSHLogin struct { TTL time.Duration // Insecure turns off verification for x509 target proxy Insecure bool - // Pool is x509 cert pool to use for server certifcate verification + // Pool is x509 cert pool to use for server certificate verification Pool *x509.CertPool // Compatibility sets compatibility mode for SSH certificates Compatibility string @@ -184,6 +184,8 @@ type SSHLogin struct { KubernetesCluster string // AttestationStatement is an attestation statement. AttestationStatement *keys.AttestationStatement + // ExtraHeaders is a map of extra HTTP headers to be included in requests. + ExtraHeaders map[string]string } // SSHLoginSSO contains SSH login parameters for SSO login. @@ -249,39 +251,51 @@ type SSHLoginPasswordless struct { CustomPrompt wancli.LoginPrompt } +// ClientConfig contains the client configuration. +type ClientConfig struct { + // ProxyAddr is the target proxy address + ProxyAddr string + // Insecure turns off verification for x509 target proxy + Insecure bool + // Pool is x509 cert pool to use for server certificate verification + Pool *x509.CertPool + // ExtraHeaders is a map of extra HTTP headers to be included in requests. + ExtraHeaders map[string]string +} + // initClient creates a new client to the HTTPS web proxy. -func initClient(proxyAddr string, insecure bool, pool *x509.CertPool) (*WebClient, *url.URL, error) { +func initClient(config ClientConfig) (*WebClient, *url.URL, error) { log := logrus.WithFields(logrus.Fields{ trace.Component: teleport.ComponentClient, }) - log.Debugf("HTTPS client init(proxyAddr=%v, insecure=%v)", proxyAddr, insecure) - - // validate proxyAddr: - host, port, err := net.SplitHostPort(proxyAddr) + log.Debugf( + "HTTPS client init(proxyAddr=%v, insecure=%v, extraHeaders=%v)", + config.ProxyAddr, + config.Insecure, + config.ExtraHeaders, + ) + + // validate proxy address + host, port, err := net.SplitHostPort(config.ProxyAddr) if err != nil || host == "" || port == "" { if err != nil { log.Error(err) } - return nil, nil, trace.BadParameter("'%v' is not a valid proxy address", proxyAddr) + return nil, nil, trace.BadParameter("'%v' is not a valid proxy address", config.ProxyAddr) } - proxyAddr = "https://" + net.JoinHostPort(host, port) - u, err := url.Parse(proxyAddr) + config.ProxyAddr = "https://" + net.JoinHostPort(host, port) + u, err := url.Parse(config.ProxyAddr) if err != nil { - return nil, nil, trace.BadParameter("'%v' is not a valid proxy address", proxyAddr) + return nil, nil, trace.BadParameter("'%v' is not a valid proxy address", config.ProxyAddr) } - var opts []roundtrip.ClientParam - - if insecure { + if config.Insecure { // Skip https cert verification, print a warning that this is insecure. - fmt.Fprintf(os.Stderr, "WARNING: You are using insecure connection to Teleport proxy %v\n", proxyAddr) - opts = append(opts, roundtrip.HTTPClient(NewInsecureWebClient())) - } else if pool != nil { - // use custom set of trusted CAs - opts = append(opts, roundtrip.HTTPClient(newClientWithPool(pool))) + fmt.Fprintf(os.Stderr, "WARNING: You are using insecure connection to Teleport proxy %v\n", config.ProxyAddr) } - clt, err := NewWebClient(proxyAddr, opts...) + opt := roundtrip.HTTPClient(newClient(config.Insecure, config.Pool, config.ExtraHeaders)) + clt, err := NewWebClient(config.ProxyAddr, opt) if err != nil { return nil, nil, trace.Wrap(err) } @@ -360,7 +374,13 @@ func SSHAgentSSOLogin(ctx context.Context, login SSHLoginSSO, config *Redirector // SSHAgentLogin is used by tsh to fetch local user credentials. func SSHAgentLogin(ctx context.Context, login SSHLoginDirect) (*auth.SSHLoginResponse, error) { - clt, _, err := initClient(login.ProxyAddr, login.Insecure, login.Pool) + cltConfig := ClientConfig{ + ProxyAddr: login.ProxyAddr, + Insecure: login.Insecure, + Pool: login.Pool, + ExtraHeaders: login.ExtraHeaders, + } + clt, _, err := initClient(cltConfig) if err != nil { return nil, trace.Wrap(err) } @@ -395,7 +415,13 @@ func SSHAgentLogin(ctx context.Context, login SSHLoginDirect) (*auth.SSHLoginRes // // Returns the SSH certificate if authn is successful or an error. func SSHAgentPasswordlessLogin(ctx context.Context, login SSHLoginPasswordless) (*auth.SSHLoginResponse, error) { - webClient, webURL, err := initClient(login.ProxyAddr, login.Insecure, login.Pool) + cltConfig := ClientConfig{ + ProxyAddr: login.ProxyAddr, + Insecure: login.Insecure, + Pool: login.Pool, + ExtraHeaders: login.ExtraHeaders, + } + webClient, webURL, err := initClient(cltConfig) if err != nil { return nil, trace.Wrap(err) } @@ -466,7 +492,13 @@ func SSHAgentPasswordlessLogin(ctx context.Context, login SSHLoginPasswordless) // prompt the user to provide 2nd factor and pass the response to the proxy. // If the authentication succeeds, we will get a temporary certificate back. func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*auth.SSHLoginResponse, error) { - clt, _, err := initClient(login.ProxyAddr, login.Insecure, login.Pool) + cltConfig := ClientConfig{ + ProxyAddr: login.ProxyAddr, + Insecure: login.Insecure, + Pool: login.Pool, + ExtraHeaders: login.ExtraHeaders, + } + clt, _, err := initClient(cltConfig) if err != nil { return nil, trace.Wrap(err) } @@ -534,7 +566,13 @@ func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*auth.SSHLoginRes // HostCredentials is used to fetch host credentials for a node. func HostCredentials(ctx context.Context, proxyAddr string, insecure bool, req types.RegisterUsingTokenRequest) (*proto.Certs, error) { - clt, _, err := initClient(proxyAddr, insecure, nil) + cltConfig := ClientConfig{ + ProxyAddr: proxyAddr, + Insecure: insecure, + Pool: nil, + ExtraHeaders: nil, + } + clt, _, err := initClient(cltConfig) if err != nil { return nil, trace.Wrap(err) } @@ -554,7 +592,13 @@ func HostCredentials(ctx context.Context, proxyAddr string, insecure bool, req t // GetWebConfig is used by teleterm to fetch webconfig.js from proxies func GetWebConfig(ctx context.Context, proxyAddr string, insecure bool) (*webclient.WebConfig, error) { - clt, _, err := initClient(proxyAddr, insecure, nil) + cltConfig := ClientConfig{ + ProxyAddr: proxyAddr, + Insecure: insecure, + Pool: nil, + ExtraHeaders: nil, + } + clt, _, err := initClient(cltConfig) if err != nil { return nil, trace.Wrap(err) } From 860633824dfe38ffcec4f182288b6006ce78a0b7 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Wed, 4 Jan 2023 19:07:56 +0000 Subject: [PATCH 02/18] Update http round tripper docs Co-authored-by: Isaiah Becker-Mayer --- api/client/proxy/proxy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/client/proxy/proxy.go b/api/client/proxy/proxy.go index 59111e782a8ee..67f5bceb0414f 100644 --- a/api/client/proxy/proxy.go +++ b/api/client/proxy/proxy.go @@ -73,7 +73,7 @@ func parse(addr string) (*url.URL, error) { // HTTPRoundTripper is a wrapper for http.Transport that // - adds extra HTTP headers to all requests, and -// - downgrades requests to plain HTTP when using a plain HTTP proxy at localhost. +// - downgrades requests to plain HTTP when proxy is at localhost and the wrapped http.Transport has TLSClientConfig.InsecureSkipVerify set to true. type HTTPRoundTripper struct { *http.Transport // extraHeaders is a map of extra HTTP headers to be included in requests. From ff9c6a7fa02ddbaae46146cfa92cfa48a04021ad Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Wed, 4 Jan 2023 21:38:59 +0000 Subject: [PATCH 03/18] Inline config definition --- lib/client/redirect.go | 5 ++--- lib/client/weblogin.go | 25 ++++++++++--------------- 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/lib/client/redirect.go b/lib/client/redirect.go index 8460992c35d85..b433a70a64787 100644 --- a/lib/client/redirect.go +++ b/lib/client/redirect.go @@ -91,13 +91,12 @@ type RedirectorConfig struct { // NewRedirector returns new local web server redirector func NewRedirector(ctx context.Context, login SSHLoginSSO, config *RedirectorConfig) (*Redirector, error) { - cltConfig := ClientConfig{ + clt, proxyURL, err := initClient(ClientConfig{ ProxyAddr: login.ProxyAddr, Insecure: login.Insecure, Pool: login.Pool, ExtraHeaders: login.ExtraHeaders, - } - clt, proxyURL, err := initClient(cltConfig) + }) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/client/weblogin.go b/lib/client/weblogin.go index f6d4e63768c40..cba545ad12c2b 100644 --- a/lib/client/weblogin.go +++ b/lib/client/weblogin.go @@ -374,13 +374,12 @@ func SSHAgentSSOLogin(ctx context.Context, login SSHLoginSSO, config *Redirector // SSHAgentLogin is used by tsh to fetch local user credentials. func SSHAgentLogin(ctx context.Context, login SSHLoginDirect) (*auth.SSHLoginResponse, error) { - cltConfig := ClientConfig{ + clt, _, err := initClient(ClientConfig{ ProxyAddr: login.ProxyAddr, Insecure: login.Insecure, Pool: login.Pool, ExtraHeaders: login.ExtraHeaders, - } - clt, _, err := initClient(cltConfig) + }) if err != nil { return nil, trace.Wrap(err) } @@ -415,13 +414,12 @@ func SSHAgentLogin(ctx context.Context, login SSHLoginDirect) (*auth.SSHLoginRes // // Returns the SSH certificate if authn is successful or an error. func SSHAgentPasswordlessLogin(ctx context.Context, login SSHLoginPasswordless) (*auth.SSHLoginResponse, error) { - cltConfig := ClientConfig{ + webClient, webURL, err := initClient(ClientConfig{ ProxyAddr: login.ProxyAddr, Insecure: login.Insecure, Pool: login.Pool, ExtraHeaders: login.ExtraHeaders, - } - webClient, webURL, err := initClient(cltConfig) + }) if err != nil { return nil, trace.Wrap(err) } @@ -492,13 +490,12 @@ func SSHAgentPasswordlessLogin(ctx context.Context, login SSHLoginPasswordless) // prompt the user to provide 2nd factor and pass the response to the proxy. // If the authentication succeeds, we will get a temporary certificate back. func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*auth.SSHLoginResponse, error) { - cltConfig := ClientConfig{ + clt, _, err := initClient(ClientConfig{ ProxyAddr: login.ProxyAddr, Insecure: login.Insecure, Pool: login.Pool, ExtraHeaders: login.ExtraHeaders, - } - clt, _, err := initClient(cltConfig) + }) if err != nil { return nil, trace.Wrap(err) } @@ -566,13 +563,12 @@ func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*auth.SSHLoginRes // HostCredentials is used to fetch host credentials for a node. func HostCredentials(ctx context.Context, proxyAddr string, insecure bool, req types.RegisterUsingTokenRequest) (*proto.Certs, error) { - cltConfig := ClientConfig{ + clt, _, err := initClient(ClientConfig{ ProxyAddr: proxyAddr, Insecure: insecure, Pool: nil, ExtraHeaders: nil, - } - clt, _, err := initClient(cltConfig) + }) if err != nil { return nil, trace.Wrap(err) } @@ -592,13 +588,12 @@ func HostCredentials(ctx context.Context, proxyAddr string, insecure bool, req t // GetWebConfig is used by teleterm to fetch webconfig.js from proxies func GetWebConfig(ctx context.Context, proxyAddr string, insecure bool) (*webclient.WebConfig, error) { - cltConfig := ClientConfig{ + clt, _, err := initClient(ClientConfig{ ProxyAddr: proxyAddr, Insecure: insecure, Pool: nil, ExtraHeaders: nil, - } - clt, _, err := initClient(cltConfig) + }) if err != nil { return nil, trace.Wrap(err) } From 0107a14560cdfdbd8e18ca9f2a539886494f32bf Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Wed, 4 Jan 2023 21:55:16 +0000 Subject: [PATCH 04/18] Add `NewInsecureWebClient` back --- lib/client/https_client.go | 4 ++++ lib/client/https_client_test.go | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/client/https_client.go b/lib/client/https_client.go index 027e0e8a620aa..5c2f0ebf0bddb 100644 --- a/lib/client/https_client.go +++ b/lib/client/https_client.go @@ -36,6 +36,10 @@ import ( "github.com/gravitational/teleport/lib/utils" ) +func NewInsecureWebClient() *http.Client { + return newClient(true, nil, nil) +} + func newClient(insecure bool, pool *x509.CertPool, extraHeaders map[string]string) *http.Client { return &http.Client{ Transport: otelhttp.NewTransport( diff --git a/lib/client/https_client_test.go b/lib/client/https_client_test.go index 1b4645d49c79d..71f45831301bb 100644 --- a/lib/client/https_client_test.go +++ b/lib/client/https_client_test.go @@ -24,7 +24,7 @@ import ( func TestNewInsecureWebClientHTTPProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") - client := newClient(true, nil, nil) + client := NewInsecureWebClient() //nolint:bodyclose // resp should be nil, so there will be no body to close. resp, err := client.Get("https://fakedomain.example.com") // Client should try to proxy through nonexistent server at localhost. @@ -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", "fakedomain.example.com") - client := newClient(true, nil, nil) + client := NewInsecureWebClient() //nolint:bodyclose // resp should be nil, so there will be no body to close. resp, err := client.Get("https://fakedomain.example.com") require.Error(t, err, "GET unexpectedly succeeded: %+v", resp) From 57c40d82c1f78a6b2e7bbefba80e468c380df953 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Thu, 5 Jan 2023 10:04:51 +0000 Subject: [PATCH 05/18] Fix TestProxyAwareRoundTripper --- api/client/proxy/proxy_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/api/client/proxy/proxy_test.go b/api/client/proxy/proxy_test.go index 08f05bfbe9f3a..df65b2c4d38b3 100644 --- a/api/client/proxy/proxy_test.go +++ b/api/client/proxy/proxy_test.go @@ -182,12 +182,14 @@ func buildProxyAddr(addr, user, pass string) (string, error) { func TestProxyAwareRoundTripper(t *testing.T) { t.Setenv("HTTP_PROXY", "http://localhost:8888") transport := &http.Transport{ - TLSClientConfig: &tls.Config{}, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + }, Proxy: func(req *http.Request) (*url.URL, error) { return httpproxy.FromEnvironment().ProxyFunc()(req.URL) }, } - rt := NewHTTPFallbackRoundTripper(transport, true) + rt := NewHTTPRoundTripper(transport, nil) req, err := http.NewRequest(http.MethodGet, "https://localhost:9999", nil) require.NoError(t, err) // Don't care about response, only if the scheme changed. From c1514a4a27e3b1d02e98f4c1c2e7cbbbcf7fabf9 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Thu, 5 Jan 2023 10:14:12 +0000 Subject: [PATCH 06/18] Don't change config state --- lib/client/weblogin.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/client/weblogin.go b/lib/client/weblogin.go index cba545ad12c2b..ba9fdee9b88b1 100644 --- a/lib/client/weblogin.go +++ b/lib/client/weblogin.go @@ -283,19 +283,19 @@ func initClient(config ClientConfig) (*WebClient, *url.URL, error) { } return nil, nil, trace.BadParameter("'%v' is not a valid proxy address", config.ProxyAddr) } - config.ProxyAddr = "https://" + net.JoinHostPort(host, port) - u, err := url.Parse(config.ProxyAddr) + proxyAddr := "https://" + net.JoinHostPort(host, port) + u, err := url.Parse(proxyAddr) if err != nil { - return nil, nil, trace.BadParameter("'%v' is not a valid proxy address", config.ProxyAddr) + return nil, nil, trace.BadParameter("'%v' is not a valid proxy address", proxyAddr) } if config.Insecure { - // Skip https cert verification, print a warning that this is insecure. - fmt.Fprintf(os.Stderr, "WARNING: You are using insecure connection to Teleport proxy %v\n", config.ProxyAddr) + // Skipping https cert verification, print a warning that this is insecure. + fmt.Fprintf(os.Stderr, "WARNING: You are using insecure connection to Teleport proxy %v\n", proxyAddr) } opt := roundtrip.HTTPClient(newClient(config.Insecure, config.Pool, config.ExtraHeaders)) - clt, err := NewWebClient(config.ProxyAddr, opt) + clt, err := NewWebClient(proxyAddr, opt) if err != nil { return nil, nil, trace.Wrap(err) } From 90d67073a0da144adccb2d9d781810d23c616b30 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Thu, 5 Jan 2023 10:28:10 +0000 Subject: [PATCH 07/18] Remove unnecessary path var in TestPlainHttpFallback --- lib/client/weblogin_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/client/weblogin_test.go b/lib/client/weblogin_test.go index a562966a06e58..6f37cfd8036d8 100644 --- a/lib/client/weblogin_test.go +++ b/lib/client/weblogin_test.go @@ -39,13 +39,11 @@ import ( func TestPlainHttpFallback(t *testing.T) { testCases := []struct { desc string - path string handler http.HandlerFunc actionUnderTest func(ctx context.Context, addr string, insecure bool) error }{ { desc: "HostCredentials", - path: "/v1/webapi/host/credentials", handler: func(w http.ResponseWriter, r *http.Request) { if r.RequestURI != "/v1/webapi/host/credentials" { w.WriteHeader(http.StatusNotFound) From de5dc4deb1ef429e45bef4555b4e6341e301f434 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Thu, 5 Jan 2023 13:52:00 +0000 Subject: [PATCH 08/18] Remove ClientConfig struct --- lib/client/redirect.go | 7 +---- lib/client/weblogin.go | 66 ++++++++---------------------------------- 2 files changed, 13 insertions(+), 60 deletions(-) diff --git a/lib/client/redirect.go b/lib/client/redirect.go index b433a70a64787..cb92e5206e24a 100644 --- a/lib/client/redirect.go +++ b/lib/client/redirect.go @@ -91,12 +91,7 @@ type RedirectorConfig struct { // NewRedirector returns new local web server redirector func NewRedirector(ctx context.Context, login SSHLoginSSO, config *RedirectorConfig) (*Redirector, error) { - clt, proxyURL, err := initClient(ClientConfig{ - ProxyAddr: login.ProxyAddr, - Insecure: login.Insecure, - Pool: login.Pool, - ExtraHeaders: login.ExtraHeaders, - }) + clt, proxyURL, err := initClient(login.ProxyAddr, login.Insecure, login.Pool, login.ExtraHeaders) if err != nil { return nil, trace.Wrap(err) } diff --git a/lib/client/weblogin.go b/lib/client/weblogin.go index ba9fdee9b88b1..aa347516d1af5 100644 --- a/lib/client/weblogin.go +++ b/lib/client/weblogin.go @@ -251,50 +251,33 @@ type SSHLoginPasswordless struct { CustomPrompt wancli.LoginPrompt } -// ClientConfig contains the client configuration. -type ClientConfig struct { - // ProxyAddr is the target proxy address - ProxyAddr string - // Insecure turns off verification for x509 target proxy - Insecure bool - // Pool is x509 cert pool to use for server certificate verification - Pool *x509.CertPool - // ExtraHeaders is a map of extra HTTP headers to be included in requests. - ExtraHeaders map[string]string -} - // initClient creates a new client to the HTTPS web proxy. -func initClient(config ClientConfig) (*WebClient, *url.URL, error) { +func initClient(proxyAddr string, insecure bool, pool *x509.CertPool, extraHeaders map[string]string) (*WebClient, *url.URL, error) { log := logrus.WithFields(logrus.Fields{ trace.Component: teleport.ComponentClient, }) - log.Debugf( - "HTTPS client init(proxyAddr=%v, insecure=%v, extraHeaders=%v)", - config.ProxyAddr, - config.Insecure, - config.ExtraHeaders, - ) + log.Debugf("HTTPS client init(proxyAddr=%v, insecure=%v, extraHeaders=%v)", proxyAddr, insecure, extraHeaders) // validate proxy address - host, port, err := net.SplitHostPort(config.ProxyAddr) + host, port, err := net.SplitHostPort(proxyAddr) if err != nil || host == "" || port == "" { if err != nil { log.Error(err) } - return nil, nil, trace.BadParameter("'%v' is not a valid proxy address", config.ProxyAddr) + return nil, nil, trace.BadParameter("'%v' is not a valid proxy address", proxyAddr) } - proxyAddr := "https://" + net.JoinHostPort(host, port) + proxyAddr = "https://" + net.JoinHostPort(host, port) u, err := url.Parse(proxyAddr) if err != nil { return nil, nil, trace.BadParameter("'%v' is not a valid proxy address", proxyAddr) } - if config.Insecure { + if insecure { // Skipping https cert verification, print a warning that this is insecure. fmt.Fprintf(os.Stderr, "WARNING: You are using insecure connection to Teleport proxy %v\n", proxyAddr) } - opt := roundtrip.HTTPClient(newClient(config.Insecure, config.Pool, config.ExtraHeaders)) + opt := roundtrip.HTTPClient(newClient(insecure, pool, extraHeaders)) clt, err := NewWebClient(proxyAddr, opt) if err != nil { return nil, nil, trace.Wrap(err) @@ -374,12 +357,7 @@ func SSHAgentSSOLogin(ctx context.Context, login SSHLoginSSO, config *Redirector // SSHAgentLogin is used by tsh to fetch local user credentials. func SSHAgentLogin(ctx context.Context, login SSHLoginDirect) (*auth.SSHLoginResponse, error) { - clt, _, err := initClient(ClientConfig{ - ProxyAddr: login.ProxyAddr, - Insecure: login.Insecure, - Pool: login.Pool, - ExtraHeaders: login.ExtraHeaders, - }) + clt, _, err := initClient(login.ProxyAddr, login.Insecure, login.Pool, login.ExtraHeaders) if err != nil { return nil, trace.Wrap(err) } @@ -414,12 +392,7 @@ func SSHAgentLogin(ctx context.Context, login SSHLoginDirect) (*auth.SSHLoginRes // // Returns the SSH certificate if authn is successful or an error. func SSHAgentPasswordlessLogin(ctx context.Context, login SSHLoginPasswordless) (*auth.SSHLoginResponse, error) { - webClient, webURL, err := initClient(ClientConfig{ - ProxyAddr: login.ProxyAddr, - Insecure: login.Insecure, - Pool: login.Pool, - ExtraHeaders: login.ExtraHeaders, - }) + webClient, webURL, err := initClient(login.ProxyAddr, login.Insecure, login.Pool, login.ExtraHeaders) if err != nil { return nil, trace.Wrap(err) } @@ -490,12 +463,7 @@ func SSHAgentPasswordlessLogin(ctx context.Context, login SSHLoginPasswordless) // prompt the user to provide 2nd factor and pass the response to the proxy. // If the authentication succeeds, we will get a temporary certificate back. func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*auth.SSHLoginResponse, error) { - clt, _, err := initClient(ClientConfig{ - ProxyAddr: login.ProxyAddr, - Insecure: login.Insecure, - Pool: login.Pool, - ExtraHeaders: login.ExtraHeaders, - }) + clt, _, err := initClient(login.ProxyAddr, login.Insecure, login.Pool, login.ExtraHeaders) if err != nil { return nil, trace.Wrap(err) } @@ -563,12 +531,7 @@ func SSHAgentMFALogin(ctx context.Context, login SSHLoginMFA) (*auth.SSHLoginRes // HostCredentials is used to fetch host credentials for a node. func HostCredentials(ctx context.Context, proxyAddr string, insecure bool, req types.RegisterUsingTokenRequest) (*proto.Certs, error) { - clt, _, err := initClient(ClientConfig{ - ProxyAddr: proxyAddr, - Insecure: insecure, - Pool: nil, - ExtraHeaders: nil, - }) + clt, _, err := initClient(proxyAddr, insecure, nil, nil) if err != nil { return nil, trace.Wrap(err) } @@ -588,12 +551,7 @@ func HostCredentials(ctx context.Context, proxyAddr string, insecure bool, req t // GetWebConfig is used by teleterm to fetch webconfig.js from proxies func GetWebConfig(ctx context.Context, proxyAddr string, insecure bool) (*webclient.WebConfig, error) { - clt, _, err := initClient(ClientConfig{ - ProxyAddr: proxyAddr, - Insecure: insecure, - Pool: nil, - ExtraHeaders: nil, - }) + clt, _, err := initClient(proxyAddr, insecure, nil, nil) if err != nil { return nil, trace.Wrap(err) } From cbf721e109e61f5b0e70509256fbb65e15910b20 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Thu, 5 Jan 2023 15:38:40 +0000 Subject: [PATCH 09/18] Rename tests --- lib/client/https_client_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/client/https_client_test.go b/lib/client/https_client_test.go index 71f45831301bb..c37ad5207610a 100644 --- a/lib/client/https_client_test.go +++ b/lib/client/https_client_test.go @@ -46,7 +46,7 @@ func TestNewInsecureWebClientNoProxy(t *testing.T) { require.Contains(t, err.Error(), "no such host") } -func TestNewClientWithPoolHTTPProxy(t *testing.T) { +func TestNewSecureWebClientHTTPProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") client := newClient(false, nil, nil) //nolint:bodyclose // resp should be nil, so there will be no body to close. @@ -58,7 +58,7 @@ func TestNewClientWithPoolHTTPProxy(t *testing.T) { require.Contains(t, err.Error(), "no such host") } -func TestNewClientWithPoolNoProxy(t *testing.T) { +func TestNewSecureWebClientNoProxy(t *testing.T) { t.Setenv("HTTPS_PROXY", "fakeproxy.example.com:9999") t.Setenv("NO_PROXY", "fakedomain.example.com") client := newClient(false, nil, nil) From 1e188c3e6a20bdc0ec24717f45616193bee4976f Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Thu, 5 Jan 2023 15:58:17 +0000 Subject: [PATCH 10/18] Simplify TestPlainHttpFallback --- lib/client/weblogin_test.go | 86 +++++++++++++++---------------------- 1 file changed, 34 insertions(+), 52 deletions(-) diff --git a/lib/client/weblogin_test.go b/lib/client/weblogin_test.go index 6f37cfd8036d8..c18eaffb47d74 100644 --- a/lib/client/weblogin_test.go +++ b/lib/client/weblogin_test.go @@ -37,66 +37,48 @@ import ( ) func TestPlainHttpFallback(t *testing.T) { - testCases := []struct { - desc string - handler http.HandlerFunc - actionUnderTest func(ctx context.Context, addr string, insecure bool) error - }{ - { - desc: "HostCredentials", - handler: func(w http.ResponseWriter, r *http.Request) { - if r.RequestURI != "/v1/webapi/host/credentials" { - w.WriteHeader(http.StatusNotFound) - return - } - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(proto.Certs{}) - }, - actionUnderTest: func(ctx context.Context, addr string, insecure bool) error { - _, err := client.HostCredentials(ctx, addr, insecure, types.RegisterUsingTokenRequest{}) - return err - }, - }, + ctx := context.Background() + var handler http.HandlerFunc = func(w http.ResponseWriter, r *http.Request) { + if r.RequestURI != "/v1/webapi/host/credentials" { + w.WriteHeader(http.StatusNotFound) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(proto.Certs{}) } - for _, testCase := range testCases { - t.Run(testCase.desc, func(t *testing.T) { - ctx := context.Background() + t.Run("Allowed on insecure & loopback", func(t *testing.T) { + httpSvr := httptest.NewServer(handler) + defer httpSvr.Close() - t.Run("Allowed on insecure & loopback", func(t *testing.T) { - httpSvr := httptest.NewServer(testCase.handler) - defer httpSvr.Close() + _, err := client.HostCredentials(ctx, httpSvr.Listener.Addr().String(), true /* insecure */, types.RegisterUsingTokenRequest{}) + require.NoError(t, err) + }) - err := testCase.actionUnderTest(ctx, httpSvr.Listener.Addr().String(), true /* insecure */) - require.NoError(t, err) - }) + t.Run("Denied on secure", func(t *testing.T) { + httpSvr := httptest.NewServer(handler) + defer httpSvr.Close() - t.Run("Denied on secure", func(t *testing.T) { - httpSvr := httptest.NewServer(testCase.handler) - defer httpSvr.Close() + _, err := client.HostCredentials(ctx, httpSvr.Listener.Addr().String(), false /* secure */, types.RegisterUsingTokenRequest{}) + require.Error(t, err) + }) - err := testCase.actionUnderTest(ctx, httpSvr.Listener.Addr().String(), false /* secure */) - require.Error(t, err) - }) + t.Run("Denied on non-loopback", func(t *testing.T) { + nonLoopbackSvr := httptest.NewUnstartedServer(handler) - t.Run("Denied on non-loopback", func(t *testing.T) { - nonLoopbackSvr := httptest.NewUnstartedServer(testCase.handler) + // replace the test-supplied loopback listener with the first available + // non-loopback address + nonLoopbackSvr.Listener.Close() + l, err := net.Listen("tcp", "0.0.0.0:0") + require.NoError(t, err) + nonLoopbackSvr.Listener = l + nonLoopbackSvr.Start() + defer nonLoopbackSvr.Close() - // replace the test-supplied loopback listener with the first available - // non-loopback address - nonLoopbackSvr.Listener.Close() - l, err := net.Listen("tcp", "0.0.0.0:0") - require.NoError(t, err) - nonLoopbackSvr.Listener = l - nonLoopbackSvr.Start() - defer nonLoopbackSvr.Close() - - err = testCase.actionUnderTest(ctx, nonLoopbackSvr.Listener.Addr().String(), true /* insecure */) - require.Error(t, err) - }) - }) - } + _, err = client.HostCredentials(ctx, nonLoopbackSvr.Listener.Addr().String(), true /* insecure */, types.RegisterUsingTokenRequest{}) + require.Error(t, err) + }) } func TestSSHAgentPasswordlessLogin(t *testing.T) { From 9641dd13aaaf84e89e7b2093e1c1c127c7531fc0 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Fri, 6 Jan 2023 11:52:26 +0000 Subject: [PATCH 11/18] Add http roundtripper tests --- api/client/proxy/proxy.go | 25 ++-- lib/client/weblogin_test.go | 266 +++++++++++++++++++++++++++++++----- 2 files changed, 249 insertions(+), 42 deletions(-) diff --git a/api/client/proxy/proxy.go b/api/client/proxy/proxy.go index 67f5bceb0414f..bdf223da74b46 100644 --- a/api/client/proxy/proxy.go +++ b/api/client/proxy/proxy.go @@ -76,20 +76,24 @@ func parse(addr string) (*url.URL, error) { // - downgrades requests to plain HTTP when proxy is at localhost and the wrapped http.Transport has TLSClientConfig.InsecureSkipVerify set to true. type HTTPRoundTripper struct { *http.Transport + // downgrade indicates that requests to plain HTTP should be downgraded. + downgrade bool // extraHeaders is a map of extra HTTP headers to be included in requests. - extraHeaders map[string]string - isProxyHTTPLocalhost bool + extraHeaders map[string]string } // NewHTTPRoundTripper creates a new initialized HTTP roundtripper. func NewHTTPRoundTripper(transport *http.Transport, extraHeaders map[string]string) *HTTPRoundTripper { - proxyConfig := httpproxy.FromEnvironment() - rt := &HTTPRoundTripper{ - Transport: transport, - extraHeaders: extraHeaders, - isProxyHTTPLocalhost: strings.HasPrefix(proxyConfig.HTTPProxy, "http://localhost"), + // Should downgrade to plain HTTP if proxying via http://localhost in insecure mode. + isProxyHTTPLocalhost := strings.HasPrefix(httpproxy.FromEnvironment().HTTPProxy, "http://localhost") + insecure := transport.TLSClientConfig != nil && transport.TLSClientConfig.InsecureSkipVerify + downgrade := isProxyHTTPLocalhost && insecure + + return &HTTPRoundTripper{ + Transport: transport, + downgrade: downgrade, + extraHeaders: extraHeaders, } - return rt } // RoundTrip executes a single HTTP transaction. Part of the RoundTripper interface. @@ -99,9 +103,8 @@ func (rt *HTTPRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) req.Header.Add(header, v) } - // Use plain HTTP if proxying via http://localhost in insecure mode. - tlsConfig := rt.Transport.TLSClientConfig - if rt.isProxyHTTPLocalhost && tlsConfig != nil && tlsConfig.InsecureSkipVerify { + // Use plain HTTP if should downgrade. + if rt.downgrade { req.URL.Scheme = "http" } diff --git a/lib/client/weblogin_test.go b/lib/client/weblogin_test.go index c18eaffb47d74..a2ad145e837ff 100644 --- a/lib/client/weblogin_test.go +++ b/lib/client/weblogin_test.go @@ -19,9 +19,11 @@ package client_test import ( "context" "encoding/json" + "fmt" "net" "net/http" "net/http/httptest" + "net/url" "strings" "testing" "time" @@ -31,54 +33,256 @@ import ( "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/types" + "github.com/gravitational/teleport/lib/auth" wanlib "github.com/gravitational/teleport/lib/auth/webauthn" wancli "github.com/gravitational/teleport/lib/auth/webauthncli" "github.com/gravitational/teleport/lib/client" ) -func TestPlainHttpFallback(t *testing.T) { - ctx := context.Background() - var handler http.HandlerFunc = func(w http.ResponseWriter, r *http.Request) { - if r.RequestURI != "/v1/webapi/host/credentials" { - w.WriteHeader(http.StatusNotFound) - return - } - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(proto.Certs{}) +// TestHostCredentialsHttpFallback tests that HostCredentials requests (/v1/webapi/host/credentials/) +// fall back to HTTP only if the address is a loopback and the insecure mode was set. +func TestHostCredentialsHttpFallback(t *testing.T) { + testCases := []struct { + desc string + loopback bool + insecure bool + fallback bool + }{ + { + desc: "falls back to http if loopback and insecure", + loopback: true, + insecure: true, + fallback: true, + }, + { + desc: "does not fall back to http if loopback and secure", + loopback: true, + insecure: false, + fallback: false, + }, + { + desc: "does not fall back to http if non-loopback and insecure", + loopback: false, + insecure: true, + fallback: false, + }, } - t.Run("Allowed on insecure & loopback", func(t *testing.T) { - httpSvr := httptest.NewServer(handler) + for _, tc := range testCases { + // Start an http server (not https) so that the request only succeeds + // if the fallback occurs. + var handler http.HandlerFunc = func(w http.ResponseWriter, r *http.Request) { + handleRequest(w, r, "/v1/webapi/host/credentials", proto.Certs{}) + } + https := false + httpSvr, err := newServer(handler, tc.loopback, https) + require.NoError(t, err) defer httpSvr.Close() - _, err := client.HostCredentials(ctx, httpSvr.Listener.Addr().String(), true /* insecure */, types.RegisterUsingTokenRequest{}) - require.NoError(t, err) - }) + // Send the HostCredentials request. + ctx := context.Background() + _, err = client.HostCredentials(ctx, httpSvr.Listener.Addr().String(), tc.insecure, types.RegisterUsingTokenRequest{}) - t.Run("Denied on secure", func(t *testing.T) { - httpSvr := httptest.NewServer(handler) - defer httpSvr.Close() + // If it should fallback, then no error should occur. + if tc.fallback { + require.NoError(t, err) + } else { + require.Error(t, err) + } + } +} - _, err := client.HostCredentials(ctx, httpSvr.Listener.Addr().String(), false /* secure */, types.RegisterUsingTokenRequest{}) - require.Error(t, err) - }) +// TestHttpRoundTripperDowngrade tests that the round tripper downgrades https requests to http +// when HTTP_PROXY is set to "http://localhost:*" (i.e. there's an http proxy running on localhost). +func TestHttpRoundTripperDowngrade(t *testing.T) { + testCases := []struct { + desc string + insecure bool + setHttpProxy bool + shouldHitProxy bool + }{ + { + desc: "hits http proxy if insecure and localhost http proxy is set", + insecure: true, + setHttpProxy: true, + shouldHitProxy: true, + }, + { + desc: "does not hit http proxy if insecure and localhost http proxy is not set", + insecure: true, + setHttpProxy: false, + shouldHitProxy: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + newHandler := func(runningAtProxy bool) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if tc.shouldHitProxy { + // If the request should hit the proxy, then: + // - this handler is running at the proxy, and + // - the scheme should be http. + require.True(t, runningAtProxy) + require.Equal(t, "http", r.URL.Scheme) + } + + handleRequest(w, r, "/v1/webapi/ssh/certs", auth.SSHLoginResponse{}) + } + } + + // Start localhost http proxy. + runningAtProxy := true + loopback := true + tls := false + httpProxySrv, err := newServer(newHandler(runningAtProxy), loopback, tls) + require.NoError(t, err) + defer httpProxySrv.Close() + + // Start non-localhost https server. + runningAtProxy = false + loopback = false + tls = true + httpsSrv, err := newServer(newHandler(runningAtProxy), loopback, tls) + require.NoError(t, err) + defer httpsSrv.Close() + + if tc.setHttpProxy { + // url.Parse won't correctly parse an absolute URL without a scheme. + u, err := url.Parse("http://" + httpProxySrv.Listener.Addr().String()) + require.NoError(t, err) + _, port, err := net.SplitHostPort(u.Host) + require.NoError(t, err) - t.Run("Denied on non-loopback", func(t *testing.T) { - nonLoopbackSvr := httptest.NewUnstartedServer(handler) + // Set HTTP_PROXY to "http://localhost:*". + t.Setenv("HTTP_PROXY", fmt.Sprintf("http://localhost:%s", port)) + } + + var addr string + if tc.shouldHitProxy { + // If should hit the proxy, set the address to a fake one that won't resolve. + // This ensures that the request below fails if it doesn't hit the proxy. + addr = "unused.proxy.com:3000" + } else { + // If shouldn't hit the proxy, set the address to the https server. + addr = httpsSrv.Listener.Addr().String() + } + + // Send an SSHLoginDirect request. + ctx := context.Background() + login := client.SSHLoginDirect{ + SSHLogin: client.SSHLogin{ + ProxyAddr: addr, + Insecure: tc.insecure, + }, + } + _, err = client.SSHAgentLogin(ctx, login) + require.NoError(t, err) + }) + } +} + +// TestHttpRoundTripperExtraHeaders tests that the round tripper adds the extra headers set. +func TestHttpRoundTripperExtraHeaders(t *testing.T) { + testCases := []struct { + desc string + extraHeaders map[string]string + expectHeaders func(*testing.T, http.Header) + }{ + { + desc: "extra headers are added", + extraHeaders: map[string]string{ + "h1": "v1", + "h2": "v2", + }, + expectHeaders: func(t *testing.T, headers http.Header) { + require.Equal(t, []string{"v1"}, headers.Values("h1")) + require.Equal(t, []string{"v2"}, headers.Values("h2")) + }, + }, + { + desc: "extra headers do not overwrite existing headers", + extraHeaders: map[string]string{ + "h1": "v1", + "Content-Type": "v2", + }, + expectHeaders: func(t *testing.T, headers http.Header) { + require.Equal(t, []string{"v1"}, headers.Values("h1")) + require.Equal(t, []string{"application/json", "v2"}, headers.Values("Content-Type")) + }, + }, + } + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + var handler http.HandlerFunc = func(w http.ResponseWriter, r *http.Request) { + tc.expectHeaders(t, r.Header) + handleRequest(w, r, "/v1/webapi/ssh/certs", auth.SSHLoginResponse{}) + } + + // Start localhost https server. + // This requires insecure to be set so that the request succeeds. + loopback := true + tls := true + insecure := true + httpsSrv, err := newServer(handler, loopback, tls) + require.NoError(t, err) + defer httpsSrv.Close() + + // Set the address to the localhost https server. + addr := httpsSrv.Listener.Addr().String() + + // Send an SSHLoginDirect request. + ctx := context.Background() + login := client.SSHLoginDirect{ + SSHLogin: client.SSHLogin{ + ProxyAddr: addr, + Insecure: insecure, + ExtraHeaders: tc.extraHeaders, + }, + } + _, err = client.SSHAgentLogin(ctx, login) + require.NoError(t, err) + }) + } +} + +// newServer starts a new server that: +// - runs TLS if `https` +// - uses a loopback listener if `loopback` +func newServer(handler http.HandlerFunc, loopback bool, https bool) (*httptest.Server, error) { + srv := httptest.NewUnstartedServer(handler) + + if !loopback { // replace the test-supplied loopback listener with the first available // non-loopback address - nonLoopbackSvr.Listener.Close() + srv.Listener.Close() l, err := net.Listen("tcp", "0.0.0.0:0") - require.NoError(t, err) - nonLoopbackSvr.Listener = l - nonLoopbackSvr.Start() - defer nonLoopbackSvr.Close() + if err != nil { + return nil, err + } + srv.Listener = l + } - _, err = client.HostCredentials(ctx, nonLoopbackSvr.Listener.Addr().String(), true /* insecure */, types.RegisterUsingTokenRequest{}) - require.Error(t, err) - }) + if https { + srv.StartTLS() + } else { + srv.Start() + } + return srv, nil +} + +// handleRequest handles an http request so that it: +// - expects a certain `uriSuffix`, and +// - always returns the same `result`. +func handleRequest(w http.ResponseWriter, r *http.Request, uriSuffix string, result any) { + if !strings.HasSuffix(r.RequestURI, uriSuffix) { + w.WriteHeader(http.StatusNotFound) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(result) } func TestSSHAgentPasswordlessLogin(t *testing.T) { From 6441f5b9f939cc7cc1b3b45c4adb4f197d42fa3a Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Fri, 6 Jan 2023 12:37:51 +0000 Subject: [PATCH 12/18] Improve comment --- api/client/proxy/proxy.go | 6 +++--- lib/client/weblogin_test.go | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/api/client/proxy/proxy.go b/api/client/proxy/proxy.go index bdf223da74b46..74f336e772f80 100644 --- a/api/client/proxy/proxy.go +++ b/api/client/proxy/proxy.go @@ -76,10 +76,10 @@ func parse(addr string) (*url.URL, error) { // - downgrades requests to plain HTTP when proxy is at localhost and the wrapped http.Transport has TLSClientConfig.InsecureSkipVerify set to true. type HTTPRoundTripper struct { *http.Transport - // downgrade indicates that requests to plain HTTP should be downgraded. - downgrade bool // extraHeaders is a map of extra HTTP headers to be included in requests. extraHeaders map[string]string + // downgrade indicates that requests to plain HTTP should be downgraded. + downgrade bool } // NewHTTPRoundTripper creates a new initialized HTTP roundtripper. @@ -91,8 +91,8 @@ func NewHTTPRoundTripper(transport *http.Transport, extraHeaders map[string]stri return &HTTPRoundTripper{ Transport: transport, - downgrade: downgrade, extraHeaders: extraHeaders, + downgrade: downgrade, } } diff --git a/lib/client/weblogin_test.go b/lib/client/weblogin_test.go index a2ad145e837ff..87cc8e6d64a0f 100644 --- a/lib/client/weblogin_test.go +++ b/lib/client/weblogin_test.go @@ -83,7 +83,8 @@ func TestHostCredentialsHttpFallback(t *testing.T) { ctx := context.Background() _, err = client.HostCredentials(ctx, httpSvr.Listener.Addr().String(), tc.insecure, types.RegisterUsingTokenRequest{}) - // If it should fallback, then no error should occur. + // If it should fallback, then no error should occur + // as the request will hit the running http server. if tc.fallback { require.NoError(t, err) } else { @@ -162,7 +163,7 @@ func TestHttpRoundTripperDowngrade(t *testing.T) { if tc.shouldHitProxy { // If should hit the proxy, set the address to a fake one that won't resolve. // This ensures that the request below fails if it doesn't hit the proxy. - addr = "unused.proxy.com:3000" + addr = "fake.proxy.com:3000" } else { // If shouldn't hit the proxy, set the address to the https server. addr = httpsSrv.Listener.Addr().String() From 0c3a5dd8f1fb173b22a991176bd19a377168e066 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Sat, 7 Jan 2023 15:21:08 +0000 Subject: [PATCH 13/18] Remove optimization in RoundTrip --- api/client/proxy/proxy.go | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/api/client/proxy/proxy.go b/api/client/proxy/proxy.go index 74f336e772f80..c1e3da4322c46 100644 --- a/api/client/proxy/proxy.go +++ b/api/client/proxy/proxy.go @@ -78,21 +78,17 @@ type HTTPRoundTripper struct { *http.Transport // extraHeaders is a map of extra HTTP headers to be included in requests. extraHeaders map[string]string - // downgrade indicates that requests to plain HTTP should be downgraded. - downgrade bool + // isProxyHTTPLocalhost indicates that the HTTP_PROXY is at "http://localhost" + isProxyHTTPLocalhost bool } // NewHTTPRoundTripper creates a new initialized HTTP roundtripper. func NewHTTPRoundTripper(transport *http.Transport, extraHeaders map[string]string) *HTTPRoundTripper { - // Should downgrade to plain HTTP if proxying via http://localhost in insecure mode. - isProxyHTTPLocalhost := strings.HasPrefix(httpproxy.FromEnvironment().HTTPProxy, "http://localhost") - insecure := transport.TLSClientConfig != nil && transport.TLSClientConfig.InsecureSkipVerify - downgrade := isProxyHTTPLocalhost && insecure - + proxyConfig := httpproxy.FromEnvironment() return &HTTPRoundTripper{ - Transport: transport, - extraHeaders: extraHeaders, - downgrade: downgrade, + Transport: transport, + extraHeaders: extraHeaders, + isProxyHTTPLocalhost: strings.HasPrefix(proxyConfig.HTTPProxy, "http://localhost"), } } @@ -103,8 +99,9 @@ func (rt *HTTPRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) req.Header.Add(header, v) } - // Use plain HTTP if should downgrade. - if rt.downgrade { + // Use plain HTTP if proxying via http://localhost in insecure mode. + tlsConfig := rt.Transport.TLSClientConfig + if rt.isProxyHTTPLocalhost && tlsConfig != nil && tlsConfig.InsecureSkipVerify { req.URL.Scheme = "http" } From aa639b1b4d10f2af3f5f963249105e815497c7bc Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Sat, 7 Jan 2023 19:18:23 +0000 Subject: [PATCH 14/18] Improve TestHttpRoundTripperDowngrade --- lib/client/weblogin_test.go | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/lib/client/weblogin_test.go b/lib/client/weblogin_test.go index 87cc8e6d64a0f..d8bcbfcc8309c 100644 --- a/lib/client/weblogin_test.go +++ b/lib/client/weblogin_test.go @@ -98,19 +98,16 @@ func TestHostCredentialsHttpFallback(t *testing.T) { func TestHttpRoundTripperDowngrade(t *testing.T) { testCases := []struct { desc string - insecure bool setHttpProxy bool shouldHitProxy bool }{ { desc: "hits http proxy if insecure and localhost http proxy is set", - insecure: true, setHttpProxy: true, shouldHitProxy: true, }, { desc: "does not hit http proxy if insecure and localhost http proxy is not set", - insecure: true, setHttpProxy: false, shouldHitProxy: false, }, @@ -118,8 +115,9 @@ func TestHttpRoundTripperDowngrade(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - newHandler := func(runningAtProxy bool) http.HandlerFunc { + newHandler := func(runningAtProxy bool, wasHit *bool) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { + *wasHit = true if tc.shouldHitProxy { // If the request should hit the proxy, then: // - this handler is running at the proxy, and @@ -136,21 +134,23 @@ func TestHttpRoundTripperDowngrade(t *testing.T) { runningAtProxy := true loopback := true tls := false - httpProxySrv, err := newServer(newHandler(runningAtProxy), loopback, tls) + httpProxyWasHit := false + httpProxy, err := newServer(newHandler(runningAtProxy, &httpProxyWasHit), loopback, tls) require.NoError(t, err) - defer httpProxySrv.Close() + defer httpProxy.Close() // Start non-localhost https server. runningAtProxy = false loopback = false tls = true - httpsSrv, err := newServer(newHandler(runningAtProxy), loopback, tls) + httpsSrvWasHit := false + httpsSrv, err := newServer(newHandler(runningAtProxy, &httpsSrvWasHit), loopback, tls) require.NoError(t, err) defer httpsSrv.Close() if tc.setHttpProxy { // url.Parse won't correctly parse an absolute URL without a scheme. - u, err := url.Parse("http://" + httpProxySrv.Listener.Addr().String()) + u, err := url.Parse("http://" + httpProxy.Listener.Addr().String()) require.NoError(t, err) _, port, err := net.SplitHostPort(u.Host) require.NoError(t, err) @@ -159,26 +159,22 @@ func TestHttpRoundTripperDowngrade(t *testing.T) { t.Setenv("HTTP_PROXY", fmt.Sprintf("http://localhost:%s", port)) } - var addr string - if tc.shouldHitProxy { - // If should hit the proxy, set the address to a fake one that won't resolve. - // This ensures that the request below fails if it doesn't hit the proxy. - addr = "fake.proxy.com:3000" - } else { - // If shouldn't hit the proxy, set the address to the https server. - addr = httpsSrv.Listener.Addr().String() - } - // Send an SSHLoginDirect request. ctx := context.Background() login := client.SSHLoginDirect{ SSHLogin: client.SSHLogin{ - ProxyAddr: addr, - Insecure: tc.insecure, + // Always set ProxyAddr to the https server. + // If HTTP_PROXY was set above, the http proxy + // should be hit regardless. + ProxyAddr: httpsSrv.Listener.Addr().String(), + Insecure: true, }, } _, err = client.SSHAgentLogin(ctx, login) require.NoError(t, err) + + require.Equal(t, tc.shouldHitProxy, httpProxyWasHit) + require.Equal(t, !tc.shouldHitProxy, httpsSrvWasHit) }) } } From 451646b116a2c646071d235ec78cd9c3717d11d6 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Sun, 8 Jan 2023 11:14:26 +0000 Subject: [PATCH 15/18] Move TestHttpRoundTripper* tests to api/client/proxy --- api/client/proxy/proxy_test.go | 188 ++++++++++++++++++++++++++++++++ lib/client/weblogin_test.go | 190 ++------------------------------- 2 files changed, 199 insertions(+), 179 deletions(-) diff --git a/api/client/proxy/proxy_test.go b/api/client/proxy/proxy_test.go index df65b2c4d38b3..774fc7dce2c0f 100644 --- a/api/client/proxy/proxy_test.go +++ b/api/client/proxy/proxy_test.go @@ -17,13 +17,17 @@ limitations under the License. package proxy import ( + "context" "crypto/tls" "fmt" + "net" "net/http" + "net/http/httptest" "net/url" "strings" "testing" + "github.com/gravitational/roundtrip" "github.com/gravitational/trace" "github.com/stretchr/testify/require" "golang.org/x/net/http/httpproxy" @@ -199,6 +203,190 @@ func TestProxyAwareRoundTripper(t *testing.T) { require.Equal(t, "http", req.URL.Scheme) } +// TestHttpRoundTripperDowngrade tests that the round tripper downgrades https requests to http +// when HTTP_PROXY is set to "http://localhost:*" (i.e. there's an http proxy running on localhost). +func TestHttpRoundTripperDowngrade(t *testing.T) { + testCases := []struct { + desc string + setHttpProxy bool + shouldHitProxy bool + }{ + { + desc: "hits http proxy if insecure and localhost http proxy is set", + setHttpProxy: true, + shouldHitProxy: true, + }, + { + desc: "does not hit http proxy if insecure and localhost http proxy is not set", + setHttpProxy: false, + shouldHitProxy: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + newHandler := func(runningAtProxy bool, wasHit *bool) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + *wasHit = true + if tc.shouldHitProxy { + // If the request should hit the proxy, then: + // - this handler is running at the proxy, and + // - the scheme should be http. + require.True(t, runningAtProxy) + require.Equal(t, "http", r.URL.Scheme) + } + w.WriteHeader(http.StatusOK) + } + } + + // Start localhost http proxy. + runningAtProxy := true + loopback := true + https := false + httpProxyWasHit := false + httpProxy, err := newServer(newHandler(runningAtProxy, &httpProxyWasHit), loopback, https) + require.NoError(t, err) + defer httpProxy.Close() + + // Start non-localhost https server. + runningAtProxy = false + loopback = false + https = true + httpsSrvWasHit := false + httpsSrv, err := newServer(newHandler(runningAtProxy, &httpsSrvWasHit), loopback, https) + require.NoError(t, err) + defer httpsSrv.Close() + + if tc.setHttpProxy { + // url.Parse won't correctly parse an absolute URL without a scheme. + u, err := url.Parse("http://" + httpProxy.Listener.Addr().String()) + require.NoError(t, err) + _, port, err := net.SplitHostPort(u.Host) + require.NoError(t, err) + + // Set HTTP_PROXY to "http://localhost:*". + t.Setenv("HTTP_PROXY", fmt.Sprintf("http://localhost:%s", port)) + } + + // Always set addr to the https server. + // If HTTP_PROXY was set above, the http proxy should be hit regardless. + addr := httpsSrv.Listener.Addr().String() + clt := newClient(t, addr, nil) + + // Perform any request. + ctx := context.Background() + _, err = clt.PostJSON(ctx, clt.Endpoint("content"), nil) + require.NoError(t, err) + + // Validate that the correct server was hit. + require.Equal(t, tc.shouldHitProxy, httpProxyWasHit) + require.Equal(t, !tc.shouldHitProxy, httpsSrvWasHit) + }) + } +} + +// TestHttpRoundTripperExtraHeaders tests that the round tripper adds the extra headers set. +func TestHttpRoundTripperExtraHeaders(t *testing.T) { + testCases := []struct { + desc string + extraHeaders map[string]string + expectHeaders func(*testing.T, http.Header) + }{ + { + desc: "extra headers are added", + extraHeaders: map[string]string{ + "header1": "value1", + "header2": "value2", + }, + expectHeaders: func(t *testing.T, headers http.Header) { + require.Equal(t, []string{"value1"}, headers.Values("header1")) + require.Equal(t, []string{"value2"}, headers.Values("header2")) + }, + }, + { + desc: "extra headers do not overwrite existing headers", + extraHeaders: map[string]string{ + "header1": "value1", + "Content-Type": "value2", + }, + expectHeaders: func(t *testing.T, headers http.Header) { + require.Equal(t, []string{"value1"}, headers.Values("header1")) + require.Equal(t, []string{"application/json", "value2"}, headers.Values("Content-Type")) + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + var handler http.HandlerFunc = func(w http.ResponseWriter, r *http.Request) { + tc.expectHeaders(t, r.Header) + w.WriteHeader(http.StatusOK) + } + + // Start localhost https server. + loopback := true + tls := true + httpsSrv, err := newServer(handler, loopback, tls) + require.NoError(t, err) + defer httpsSrv.Close() + + // Set the address to the localhost https server. + addr := httpsSrv.Listener.Addr().String() + clt := newClient(t, addr, tc.extraHeaders) + + // Perform any request. + ctx := context.Background() + _, err = clt.PostJSON(ctx, clt.Endpoint("content"), nil) + require.NoError(t, err) + }) + } +} + +// newServer starts a new server that: +// - runs TLS if `https` +// - uses a loopback listener if `loopback` +func newServer(handler http.HandlerFunc, loopback bool, https bool) (*httptest.Server, error) { + srv := httptest.NewUnstartedServer(handler) + + if !loopback { + // replace the test-supplied loopback listener with the first available + // non-loopback address + srv.Listener.Close() + l, err := net.Listen("tcp", "0.0.0.0:0") + if err != nil { + return nil, err + } + srv.Listener = l + } + + if https { + srv.StartTLS() + } else { + srv.Start() + } + return srv, nil +} + +// newClient creates a new https roundtrip client. +func newClient(t *testing.T, addr string, extraHeaders map[string]string) *roundtrip.Client { + transport := &http.Transport{ + TLSClientConfig: &tls.Config{ + // Setting insecure ensures that https requests succeed. + InsecureSkipVerify: true, + }, + Proxy: func(req *http.Request) (*url.URL, error) { + return httpproxy.FromEnvironment().ProxyFunc()(req.URL) + }, + } + httpClient := &http.Client{ + Transport: NewHTTPRoundTripper(transport, extraHeaders), + } + opt := roundtrip.HTTPClient(httpClient) + clt, err := roundtrip.NewClient("https://"+addr, "v1", opt) + require.NoError(t, err) + return clt +} + func TestParse(t *testing.T) { successTests := []struct { name, addr, scheme, host, path string diff --git a/lib/client/weblogin_test.go b/lib/client/weblogin_test.go index d8bcbfcc8309c..ad2862f04c5e2 100644 --- a/lib/client/weblogin_test.go +++ b/lib/client/weblogin_test.go @@ -19,11 +19,9 @@ package client_test import ( "context" "encoding/json" - "fmt" "net" "net/http" "net/http/httptest" - "net/url" "strings" "testing" "time" @@ -33,7 +31,6 @@ import ( "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/types" - "github.com/gravitational/teleport/lib/auth" wanlib "github.com/gravitational/teleport/lib/auth/webauthn" wancli "github.com/gravitational/teleport/lib/auth/webauthncli" "github.com/gravitational/teleport/lib/client" @@ -72,10 +69,15 @@ func TestHostCredentialsHttpFallback(t *testing.T) { // Start an http server (not https) so that the request only succeeds // if the fallback occurs. var handler http.HandlerFunc = func(w http.ResponseWriter, r *http.Request) { - handleRequest(w, r, "/v1/webapi/host/credentials", proto.Certs{}) + if r.RequestURI != "/v1/webapi/host/credentials" { + w.WriteHeader(http.StatusNotFound) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(proto.Certs{}) } - https := false - httpSvr, err := newServer(handler, tc.loopback, https) + httpSvr, err := newServer(handler, tc.loopback) require.NoError(t, err) defer httpSvr.Close() @@ -93,161 +95,8 @@ func TestHostCredentialsHttpFallback(t *testing.T) { } } -// TestHttpRoundTripperDowngrade tests that the round tripper downgrades https requests to http -// when HTTP_PROXY is set to "http://localhost:*" (i.e. there's an http proxy running on localhost). -func TestHttpRoundTripperDowngrade(t *testing.T) { - testCases := []struct { - desc string - setHttpProxy bool - shouldHitProxy bool - }{ - { - desc: "hits http proxy if insecure and localhost http proxy is set", - setHttpProxy: true, - shouldHitProxy: true, - }, - { - desc: "does not hit http proxy if insecure and localhost http proxy is not set", - setHttpProxy: false, - shouldHitProxy: false, - }, - } - - for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - newHandler := func(runningAtProxy bool, wasHit *bool) http.HandlerFunc { - return func(w http.ResponseWriter, r *http.Request) { - *wasHit = true - if tc.shouldHitProxy { - // If the request should hit the proxy, then: - // - this handler is running at the proxy, and - // - the scheme should be http. - require.True(t, runningAtProxy) - require.Equal(t, "http", r.URL.Scheme) - } - - handleRequest(w, r, "/v1/webapi/ssh/certs", auth.SSHLoginResponse{}) - } - } - - // Start localhost http proxy. - runningAtProxy := true - loopback := true - tls := false - httpProxyWasHit := false - httpProxy, err := newServer(newHandler(runningAtProxy, &httpProxyWasHit), loopback, tls) - require.NoError(t, err) - defer httpProxy.Close() - - // Start non-localhost https server. - runningAtProxy = false - loopback = false - tls = true - httpsSrvWasHit := false - httpsSrv, err := newServer(newHandler(runningAtProxy, &httpsSrvWasHit), loopback, tls) - require.NoError(t, err) - defer httpsSrv.Close() - - if tc.setHttpProxy { - // url.Parse won't correctly parse an absolute URL without a scheme. - u, err := url.Parse("http://" + httpProxy.Listener.Addr().String()) - require.NoError(t, err) - _, port, err := net.SplitHostPort(u.Host) - require.NoError(t, err) - - // Set HTTP_PROXY to "http://localhost:*". - t.Setenv("HTTP_PROXY", fmt.Sprintf("http://localhost:%s", port)) - } - - // Send an SSHLoginDirect request. - ctx := context.Background() - login := client.SSHLoginDirect{ - SSHLogin: client.SSHLogin{ - // Always set ProxyAddr to the https server. - // If HTTP_PROXY was set above, the http proxy - // should be hit regardless. - ProxyAddr: httpsSrv.Listener.Addr().String(), - Insecure: true, - }, - } - _, err = client.SSHAgentLogin(ctx, login) - require.NoError(t, err) - - require.Equal(t, tc.shouldHitProxy, httpProxyWasHit) - require.Equal(t, !tc.shouldHitProxy, httpsSrvWasHit) - }) - } -} - -// TestHttpRoundTripperExtraHeaders tests that the round tripper adds the extra headers set. -func TestHttpRoundTripperExtraHeaders(t *testing.T) { - testCases := []struct { - desc string - extraHeaders map[string]string - expectHeaders func(*testing.T, http.Header) - }{ - { - desc: "extra headers are added", - extraHeaders: map[string]string{ - "h1": "v1", - "h2": "v2", - }, - expectHeaders: func(t *testing.T, headers http.Header) { - require.Equal(t, []string{"v1"}, headers.Values("h1")) - require.Equal(t, []string{"v2"}, headers.Values("h2")) - }, - }, - { - desc: "extra headers do not overwrite existing headers", - extraHeaders: map[string]string{ - "h1": "v1", - "Content-Type": "v2", - }, - expectHeaders: func(t *testing.T, headers http.Header) { - require.Equal(t, []string{"v1"}, headers.Values("h1")) - require.Equal(t, []string{"application/json", "v2"}, headers.Values("Content-Type")) - }, - }, - } - - for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - var handler http.HandlerFunc = func(w http.ResponseWriter, r *http.Request) { - tc.expectHeaders(t, r.Header) - handleRequest(w, r, "/v1/webapi/ssh/certs", auth.SSHLoginResponse{}) - } - - // Start localhost https server. - // This requires insecure to be set so that the request succeeds. - loopback := true - tls := true - insecure := true - httpsSrv, err := newServer(handler, loopback, tls) - require.NoError(t, err) - defer httpsSrv.Close() - - // Set the address to the localhost https server. - addr := httpsSrv.Listener.Addr().String() - - // Send an SSHLoginDirect request. - ctx := context.Background() - login := client.SSHLoginDirect{ - SSHLogin: client.SSHLogin{ - ProxyAddr: addr, - Insecure: insecure, - ExtraHeaders: tc.extraHeaders, - }, - } - _, err = client.SSHAgentLogin(ctx, login) - require.NoError(t, err) - }) - } -} - -// newServer starts a new server that: -// - runs TLS if `https` -// - uses a loopback listener if `loopback` -func newServer(handler http.HandlerFunc, loopback bool, https bool) (*httptest.Server, error) { +// newServer starts a new server that uses a loopback listener if `loopback`. +func newServer(handler http.HandlerFunc, loopback bool) (*httptest.Server, error) { srv := httptest.NewUnstartedServer(handler) if !loopback { @@ -261,27 +110,10 @@ func newServer(handler http.HandlerFunc, loopback bool, https bool) (*httptest.S srv.Listener = l } - if https { - srv.StartTLS() - } else { - srv.Start() - } + srv.Start() return srv, nil } -// handleRequest handles an http request so that it: -// - expects a certain `uriSuffix`, and -// - always returns the same `result`. -func handleRequest(w http.ResponseWriter, r *http.Request, uriSuffix string, result any) { - if !strings.HasSuffix(r.RequestURI, uriSuffix) { - w.WriteHeader(http.StatusNotFound) - return - } - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(result) -} - func TestSSHAgentPasswordlessLogin(t *testing.T) { silenceLogger(t) From 17ffc0b6dd88dbd3aee019d6e7629966cf8758bf Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Tue, 10 Jan 2023 10:24:47 +0000 Subject: [PATCH 16/18] Don't use roundtrip in roundtripper tests --- api/client/proxy/proxy_test.go | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/api/client/proxy/proxy_test.go b/api/client/proxy/proxy_test.go index 774fc7dce2c0f..e4cbcdfeae501 100644 --- a/api/client/proxy/proxy_test.go +++ b/api/client/proxy/proxy_test.go @@ -17,7 +17,6 @@ limitations under the License. package proxy import ( - "context" "crypto/tls" "fmt" "net" @@ -27,7 +26,6 @@ import ( "strings" "testing" - "github.com/gravitational/roundtrip" "github.com/gravitational/trace" "github.com/stretchr/testify/require" "golang.org/x/net/http/httpproxy" @@ -268,14 +266,14 @@ func TestHttpRoundTripperDowngrade(t *testing.T) { t.Setenv("HTTP_PROXY", fmt.Sprintf("http://localhost:%s", port)) } - // Always set addr to the https server. - // If HTTP_PROXY was set above, the http proxy should be hit regardless. - addr := httpsSrv.Listener.Addr().String() - clt := newClient(t, addr, nil) + clt := newClient(t, nil) // Perform any request. - ctx := context.Background() - _, err = clt.PostJSON(ctx, clt.Endpoint("content"), nil) + // Set addr to the https server. If HTTP_PROXY was set above, + // the http proxy should be hit regardless. + addr := httpsSrv.Listener.Addr().String() + url := "https://" + addr + "/v1/content" + _, err = clt.Post(url, "application/json", nil) require.NoError(t, err) // Validate that the correct server was hit. @@ -330,13 +328,13 @@ func TestHttpRoundTripperExtraHeaders(t *testing.T) { require.NoError(t, err) defer httpsSrv.Close() - // Set the address to the localhost https server. - addr := httpsSrv.Listener.Addr().String() - clt := newClient(t, addr, tc.extraHeaders) + clt := newClient(t, tc.extraHeaders) // Perform any request. - ctx := context.Background() - _, err = clt.PostJSON(ctx, clt.Endpoint("content"), nil) + // Set the address to the localhost https server. + addr := httpsSrv.Listener.Addr().String() + url := "https://" + addr + "/v1/content" + _, err = clt.Post(url, "application/json", nil) require.NoError(t, err) }) } @@ -368,7 +366,7 @@ func newServer(handler http.HandlerFunc, loopback bool, https bool) (*httptest.S } // newClient creates a new https roundtrip client. -func newClient(t *testing.T, addr string, extraHeaders map[string]string) *roundtrip.Client { +func newClient(t *testing.T, extraHeaders map[string]string) *http.Client { transport := &http.Transport{ TLSClientConfig: &tls.Config{ // Setting insecure ensures that https requests succeed. @@ -378,13 +376,9 @@ func newClient(t *testing.T, addr string, extraHeaders map[string]string) *round return httpproxy.FromEnvironment().ProxyFunc()(req.URL) }, } - httpClient := &http.Client{ + return &http.Client{ Transport: NewHTTPRoundTripper(transport, extraHeaders), } - opt := roundtrip.HTTPClient(httpClient) - clt, err := roundtrip.NewClient("https://"+addr, "v1", opt) - require.NoError(t, err) - return clt } func TestParse(t *testing.T) { From bb2420cb221a8c58af826f296cc484ecb62dc7c1 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Tue, 10 Jan 2023 11:40:35 +0000 Subject: [PATCH 17/18] Fix lint-api --- api/client/proxy/proxy_test.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/api/client/proxy/proxy_test.go b/api/client/proxy/proxy_test.go index e4cbcdfeae501..1231afa27154b 100644 --- a/api/client/proxy/proxy_test.go +++ b/api/client/proxy/proxy_test.go @@ -206,17 +206,17 @@ func TestProxyAwareRoundTripper(t *testing.T) { func TestHttpRoundTripperDowngrade(t *testing.T) { testCases := []struct { desc string - setHttpProxy bool + setHTTPProxy bool shouldHitProxy bool }{ { desc: "hits http proxy if insecure and localhost http proxy is set", - setHttpProxy: true, + setHTTPProxy: true, shouldHitProxy: true, }, { desc: "does not hit http proxy if insecure and localhost http proxy is not set", - setHttpProxy: false, + setHTTPProxy: false, shouldHitProxy: false, }, } @@ -255,7 +255,7 @@ func TestHttpRoundTripperDowngrade(t *testing.T) { require.NoError(t, err) defer httpsSrv.Close() - if tc.setHttpProxy { + if tc.setHTTPProxy { // url.Parse won't correctly parse an absolute URL without a scheme. u, err := url.Parse("http://" + httpProxy.Listener.Addr().String()) require.NoError(t, err) @@ -272,9 +272,7 @@ func TestHttpRoundTripperDowngrade(t *testing.T) { // Set addr to the https server. If HTTP_PROXY was set above, // the http proxy should be hit regardless. addr := httpsSrv.Listener.Addr().String() - url := "https://" + addr + "/v1/content" - _, err = clt.Post(url, "application/json", nil) - require.NoError(t, err) + request(t, clt, addr) // Validate that the correct server was hit. require.Equal(t, tc.shouldHitProxy, httpProxyWasHit) @@ -333,9 +331,7 @@ func TestHttpRoundTripperExtraHeaders(t *testing.T) { // Perform any request. // Set the address to the localhost https server. addr := httpsSrv.Listener.Addr().String() - url := "https://" + addr + "/v1/content" - _, err = clt.Post(url, "application/json", nil) - require.NoError(t, err) + request(t, clt, addr) }) } } @@ -381,6 +377,14 @@ func newClient(t *testing.T, extraHeaders map[string]string) *http.Client { } } +// request perform a POST request. +func request(t *testing.T, clt *http.Client, addr string) { + url := "https://" + addr + "/v1/content" + resp, err := clt.Post(url, "application/json", nil) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) +} + func TestParse(t *testing.T) { successTests := []struct { name, addr, scheme, host, path string From a584b5c8bab19a174bee04f1e260f81d7904a158 Mon Sep 17 00:00:00 2001 From: Vitor Enes Date: Tue, 10 Jan 2023 11:49:54 +0000 Subject: [PATCH 18/18] Consistent comments --- api/client/proxy/proxy_test.go | 4 ++-- lib/client/weblogin_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/client/proxy/proxy_test.go b/api/client/proxy/proxy_test.go index 1231afa27154b..939b530f91865 100644 --- a/api/client/proxy/proxy_test.go +++ b/api/client/proxy/proxy_test.go @@ -343,8 +343,8 @@ func newServer(handler http.HandlerFunc, loopback bool, https bool) (*httptest.S srv := httptest.NewUnstartedServer(handler) if !loopback { - // replace the test-supplied loopback listener with the first available - // non-loopback address + // Replace the test-supplied loopback listener with the first available + // non-loopback address. srv.Listener.Close() l, err := net.Listen("tcp", "0.0.0.0:0") if err != nil { diff --git a/lib/client/weblogin_test.go b/lib/client/weblogin_test.go index ad2862f04c5e2..071ebcc69f022 100644 --- a/lib/client/weblogin_test.go +++ b/lib/client/weblogin_test.go @@ -100,8 +100,8 @@ func newServer(handler http.HandlerFunc, loopback bool) (*httptest.Server, error srv := httptest.NewUnstartedServer(handler) if !loopback { - // replace the test-supplied loopback listener with the first available - // non-loopback address + // Replace the test-supplied loopback listener with the first available + // non-loopback address. srv.Listener.Close() l, err := net.Listen("tcp", "0.0.0.0:0") if err != nil {