diff --git a/README.md b/README.md index 032e84c3..ea1c1cd2 100644 --- a/README.md +++ b/README.md @@ -259,6 +259,27 @@ set OCTOPUS_API_KEY="API-XXXXXXXXXXXXXXXXXXXXXXXXXXXXX" # replace with your API octopus.exe space list # should list all the spaces ``` +### Proxies + +The CLI honours the standard `HTTP_PROXY`, `HTTPS_PROXY` and `NO_PROXY` environment variables. + +To point the CLI at a proxy without affecting other tools, set `OCTOPUS_PROXY` (or the `ProxyUrl` config key, +which `OCTOPUS_PROXY` overrides). It applies to both http and https requests, and `NO_PROXY` still applies. +`http`, `https`, `socks5` and `socks5h` proxy urls are supported. + +```shell +export OCTOPUS_PROXY="http://proxy.example.com:3128" +``` + +Credentials can be embedded in the proxy url, or supplied separately with `OCTOPUS_PROXY_USERNAME` and +`OCTOPUS_PROXY_PASSWORD`. `OCTOPUS_PROXY_PASSWORD` needs `OCTOPUS_PROXY_USERNAME` alongside it; on its own +the CLI reports the mistake rather than connecting without the credentials. + +Prefer those two variables over embedding a password in the proxy url: they are never written to the CLI +config file, whereas `octopus config set ProxyUrl` stores whatever it is given in plain text, exactly as it +does for an API key. `octopus config list` and `octopus config get ProxyUrl` mask the password when they +display it. + ### go-octopusdeploy library The CLI depends heavily on the [go-octopusdeploy](https://github.com/OctopusDeploy/go-octopusdeploy) library, which manages diff --git a/go.mod b/go.mod index a46e3a7b..a89bf9c0 100644 --- a/go.mod +++ b/go.mod @@ -21,6 +21,7 @@ require ( github.com/spf13/viper v1.21.0 github.com/stretchr/testify v1.11.1 golang.org/x/exp v0.0.0-20230129154200-a960b3787bd2 + golang.org/x/net v0.57.0 golang.org/x/term v0.45.0 ) @@ -53,7 +54,6 @@ require ( github.com/subosito/gotenv v1.6.0 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/crypto v0.54.0 // indirect - golang.org/x/net v0.57.0 // indirect golang.org/x/sys v0.47.0 // indirect golang.org/x/text v0.40.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/pkg/apiclient/client_factory.go b/pkg/apiclient/client_factory.go index 93c73abe..ed411854 100644 --- a/pkg/apiclient/client_factory.go +++ b/pkg/apiclient/client_factory.go @@ -1,7 +1,6 @@ package apiclient import ( - "crypto/tls" "errors" "fmt" "net/url" @@ -121,13 +120,26 @@ func NewClientFactoryFromConfig(ask question.AskProvider) (ClientFactory, error) return nil, errs } - http.DefaultTransport.(*http.Transport).TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + // insecureSkipVerify is hardcoded true to preserve the behaviour this replaced, + // which set InsecureSkipVerify on the shared http.DefaultTransport: the CLI has + // never verified the Octopus server certificate, so --ignore-ssl-errors is + // effectively always on. That is a pre-existing security bug rather than + // something this proxy work introduces, and turning it off would break every + // user with a self-signed certificate, so it needs its own change with a way to + // opt out. Tracked separately; the setting is a parameter now so plumbing the + // real value through is all that is left. + transport, err := NewHttpTransport(ProxySettingsFromConfig(), true) + if err != nil { + return nil, err + } // The spinner is only wanted in interactive mode, but that is not settled // yet: this runs before cobra parses --no-prompt. The round-tripper decides // per request instead. + spinnerRoundTripper := NewSpinnerRoundTripper(ask) + spinnerRoundTripper.Next = transport httpClient := &http.Client{ - Transport: NewSpinnerRoundTripper(ask), + Transport: spinnerRoundTripper, } var credentials octopusApiClient.ICredential diff --git a/pkg/apiclient/proxy.go b/pkg/apiclient/proxy.go new file mode 100644 index 00000000..fc618e9c --- /dev/null +++ b/pkg/apiclient/proxy.go @@ -0,0 +1,167 @@ +package apiclient + +import ( + "crypto/tls" + "errors" + "fmt" + "net/http" + "net/url" + "os" + "strings" + + "github.com/OctopusDeploy/cli/pkg/constants" + "github.com/spf13/viper" + "golang.org/x/net/http/httpproxy" +) + +// ProxySettings is the CLI's proxy configuration. +// +// Url takes precedence over the standard HTTP_PROXY/HTTPS_PROXY variables and +// applies to both schemes; when it is empty those variables are used instead. +// NO_PROXY is honoured either way. http, https, socks5 and socks5h proxies are +// supported, all by net/http itself. +type ProxySettings struct { + Url string + Username string + Password string +} + +// ProxySettingsFromConfig reads the proxy settings from the viper config, which +// covers the ProxyUrl config file key and the OCTOPUS_PROXY environment variable. +// The credentials are deliberately read from the environment only, so that a +// proxy password is never written to the config file in plain text. +func ProxySettingsFromConfig() ProxySettings { + return ProxySettings{ + Url: viper.GetString(constants.ConfigProxyUrl), + Username: os.Getenv(constants.EnvOctopusProxyUsername), + Password: os.Getenv(constants.EnvOctopusProxyPassword), + } +} + +// ProxyFunc returns a function suitable for http.Transport.Proxy. +func (s ProxySettings) ProxyFunc() (func(*http.Request) (*url.URL, error), error) { + config := httpproxy.FromEnvironment() + if s.Url != "" { + // httpproxy silently ignores a proxy address it cannot parse, so parse it here + // to report a typo rather than quietly connecting directly. Hand httpproxy the + // normalized result rather than the raw string, so this parse is the only one + // that decides what the proxy is - otherwise the two copies of the rules could + // drift and we would accept a url that httpproxy then ignores. + parsed, err := parseProxyUrl(s.Url) + if err != nil { + return nil, err + } + config.HTTPProxy = parsed.String() + config.HTTPSProxy = parsed.String() + config.CGI = false + } + + proxyForUrl := config.ProxyFunc() + return func(request *http.Request) (*url.URL, error) { + proxyUrl, err := proxyForUrl(request.URL) + if err != nil || proxyUrl == nil { + return nil, err + } + // A password with no username cannot be sent, and dropping it silently leaves the + // user staring at a bare 407 from the proxy. Only complain once it actually + // matters, i.e. when a proxy is in play and the url carries no credentials of its + // own, so an unrelated stray variable never breaks a direct connection. + if s.Password != "" && s.Username == "" && proxyUrl.User == nil { + return nil, fmt.Errorf("%s is set but %s is empty, so the proxy credentials cannot be used", constants.EnvOctopusProxyPassword, constants.EnvOctopusProxyUsername) + } + return s.applyCredentials(proxyUrl), nil + }, nil +} + +// applyCredentials adds the configured proxy credentials, unless the proxy url +// already carries its own. +func (s ProxySettings) applyCredentials(proxyUrl *url.URL) *url.URL { + if s.Username == "" || proxyUrl.User != nil { + return proxyUrl + } + withCredentials := *proxyUrl + withCredentials.User = url.UserPassword(s.Username, s.Password) + return &withCredentials +} + +// NewHttpTransport returns the transport the CLI uses to talk to Octopus. It is +// a clone of http.DefaultTransport so the standard defaults are kept, with the +// proxy resolution replaced by ours. +func NewHttpTransport(settings ProxySettings, insecureSkipVerify bool) (*http.Transport, error) { + proxyFunc, err := settings.ProxyFunc() + if err != nil { + return nil, err + } + + transport := http.DefaultTransport.(*http.Transport).Clone() + transport.Proxy = proxyFunc + if insecureSkipVerify { + transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + } + return transport, nil +} + +// RedactProxyUrl removes the password from a proxy url so it can be displayed. +func RedactProxyUrl(rawUrl string) string { + if rawUrl == "" { + return "" + } + parsed, err := parseProxyUrl(rawUrl) + if err != nil { + return "***" // can't parse it, so we can't tell whether it holds a password + } + if parsed.User == nil { + return rawUrl + } + return parsed.Redacted() +} + +// parseProxyUrl mirrors how net/http parses a proxy address: a bare "host:port" +// is treated as http. +func parseProxyUrl(rawUrl string) (*url.URL, error) { + parsed, err := url.Parse(rawUrl) + if err != nil || parsed.Scheme == "" || parsed.Host == "" { + if withScheme, schemeErr := url.Parse("http://" + rawUrl); schemeErr == nil && withScheme.Host != "" { + return withScheme, nil + } + } + if err != nil { + return nil, invalidProxyUrlError(rawUrl, err) + } + if parsed.Host == "" { + return nil, fmt.Errorf("invalid proxy url '%s': no host specified", redactRawProxyUrl(rawUrl)) + } + return parsed, nil +} + +// invalidProxyUrlError reports a parse failure without echoing the password: both +// the raw string and url.Parse's own error message (a *url.Error, which repeats the +// whole url back) can carry one, and this error is printed to the terminal. +func invalidProxyUrlError(rawUrl string, err error) error { + var urlError *url.Error + if errors.As(err, &urlError) { + err = urlError.Err + } + return fmt.Errorf("invalid proxy url '%s': %w", redactRawProxyUrl(rawUrl), err) +} + +// redactRawProxyUrl masks the password in a proxy url that could not be parsed, so +// the rest of it is still recognisable in an error message. url.Redacted cannot be +// used here precisely because parsing is what failed. +func redactRawProxyUrl(rawUrl string) string { + scheme, rest := "", rawUrl + if separator := strings.Index(rawUrl, "://"); separator >= 0 { + scheme, rest = rawUrl[:separator+3], rawUrl[separator+3:] + } + + credentials := strings.LastIndex(rest, "@") + if credentials < 0 { + return rawUrl + } + + userInfo := rest[:credentials] + if password := strings.Index(userInfo, ":"); password >= 0 { + userInfo = userInfo[:password] + ":xxxxx" + } + return scheme + userInfo + "@" + rest[credentials+1:] +} diff --git a/pkg/apiclient/proxy_test.go b/pkg/apiclient/proxy_test.go new file mode 100644 index 00000000..22d219b0 --- /dev/null +++ b/pkg/apiclient/proxy_test.go @@ -0,0 +1,285 @@ +package apiclient_test + +import ( + "encoding/base64" + "net/http" + "net/http/httptest" + "testing" + + "github.com/OctopusDeploy/cli/pkg/apiclient" + "github.com/OctopusDeploy/cli/pkg/constants" + "github.com/spf13/viper" + "github.com/stretchr/testify/assert" +) + +const octopusUrl = "https://octopus.example.com/api/" + +// clearProxyEnvironment stops whatever the machine running the tests has configured +// from leaking into the expectations. +func clearProxyEnvironment(t *testing.T) { + t.Setenv("HTTP_PROXY", "") + t.Setenv("http_proxy", "") + t.Setenv("HTTPS_PROXY", "") + t.Setenv("https_proxy", "") + t.Setenv("NO_PROXY", "") + t.Setenv("no_proxy", "") +} + +func TestProxySettings_ProxyFunc(t *testing.T) { + tests := []struct { + name string + settings apiclient.ProxySettings + env map[string]string + requestUrl string + wantProxy string + }{ + { + name: "no proxy configured at all", + requestUrl: octopusUrl, + }, + { + name: "HTTPS_PROXY is honoured with no explicit configuration", + env: map[string]string{"HTTPS_PROXY": "http://envproxy:3128"}, + requestUrl: octopusUrl, + wantProxy: "http://envproxy:3128", + }, + { + name: "HTTP_PROXY is honoured for plain http requests", + env: map[string]string{"HTTP_PROXY": "http://envproxy:3128"}, + requestUrl: "http://octopus.example.com/api/", + wantProxy: "http://envproxy:3128", + }, + { + name: "HTTPS_PROXY does not apply to plain http requests", + env: map[string]string{"HTTPS_PROXY": "http://envproxy:3128"}, + requestUrl: "http://octopus.example.com/api/", + }, + { + name: "the configured proxy url wins over HTTPS_PROXY", + settings: apiclient.ProxySettings{Url: "http://configured:3128"}, + env: map[string]string{"HTTPS_PROXY": "http://envproxy:3128"}, + requestUrl: octopusUrl, + wantProxy: "http://configured:3128", + }, + { + name: "the configured proxy url applies to plain http requests too", + settings: apiclient.ProxySettings{Url: "http://configured:3128"}, + requestUrl: "http://octopus.example.com/api/", + wantProxy: "http://configured:3128", + }, + { + name: "a proxy url without a scheme is assumed to be http", + settings: apiclient.ProxySettings{Url: "configured:3128"}, + requestUrl: octopusUrl, + wantProxy: "http://configured:3128", + }, + { + name: "socks5 proxies are passed through to net/http", + settings: apiclient.ProxySettings{Url: "socks5://configured:1080"}, + requestUrl: octopusUrl, + wantProxy: "socks5://configured:1080", + }, + { + name: "NO_PROXY excludes the host from the configured proxy", + settings: apiclient.ProxySettings{Url: "http://configured:3128"}, + env: map[string]string{"NO_PROXY": "octopus.example.com"}, + requestUrl: octopusUrl, + }, + { + name: "NO_PROXY excludes the host from HTTPS_PROXY", + env: map[string]string{"HTTPS_PROXY": "http://envproxy:3128", "NO_PROXY": "octopus.example.com"}, + requestUrl: octopusUrl, + }, + { + name: "NO_PROXY leaves other hosts proxied", + settings: apiclient.ProxySettings{Url: "http://configured:3128"}, + env: map[string]string{"NO_PROXY": "internal.example.com"}, + requestUrl: octopusUrl, + wantProxy: "http://configured:3128", + }, + { + name: "loopback servers are never proxied", + settings: apiclient.ProxySettings{Url: "http://configured:3128"}, + requestUrl: "http://localhost:8065/api/", + }, + { + name: "credentials are added to the configured proxy url", + settings: apiclient.ProxySettings{Url: "http://configured:3128", Username: "octo", Password: "s3cret"}, + requestUrl: octopusUrl, + wantProxy: "http://octo:s3cret@configured:3128", + }, + { + name: "credentials are added to a proxy url taken from the environment", + settings: apiclient.ProxySettings{Username: "octo", Password: "s3cret"}, + env: map[string]string{"HTTPS_PROXY": "http://envproxy:3128"}, + requestUrl: octopusUrl, + wantProxy: "http://octo:s3cret@envproxy:3128", + }, + { + name: "credentials in the proxy url win over the environment", + settings: apiclient.ProxySettings{Url: "http://inurl:inurlpassword@configured:3128", Username: "octo", Password: "s3cret"}, + requestUrl: octopusUrl, + wantProxy: "http://inurl:inurlpassword@configured:3128", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearProxyEnvironment(t) + for key, value := range test.env { + t.Setenv(key, value) + } + + proxyFunc, err := test.settings.ProxyFunc() + if !assert.NoError(t, err) { + return + } + + request, err := http.NewRequest(http.MethodGet, test.requestUrl, nil) + if !assert.NoError(t, err) { + return + } + + proxyUrl, err := proxyFunc(request) + assert.NoError(t, err) + + if test.wantProxy == "" { + assert.Nil(t, proxyUrl) + return + } + if assert.NotNil(t, proxyUrl) { + assert.Equal(t, test.wantProxy, proxyUrl.String()) + } + }) + } +} + +func TestProxySettings_ProxyFuncRejectsAnInvalidProxyUrl(t *testing.T) { + clearProxyEnvironment(t) + + _, err := apiclient.ProxySettings{Url: "http://%zz:3128"}.ProxyFunc() + + assert.ErrorContains(t, err, "invalid proxy url") +} + +func TestProxySettings_ProxyFuncRejectsAPasswordWithNoUsername(t *testing.T) { + clearProxyEnvironment(t) + settings := apiclient.ProxySettings{Url: "http://configured:3128", Password: "s3cret"} + + proxyFunc, err := settings.ProxyFunc() + if !assert.NoError(t, err) { + return + } + + request, _ := http.NewRequest(http.MethodGet, octopusUrl, nil) + _, err = proxyFunc(request) + + assert.ErrorContains(t, err, constants.EnvOctopusProxyUsername) +} + +// A stray password is only a problem when a proxy would actually be used, so a +// direct connection must not be broken by one. +func TestProxySettings_ProxyFuncIgnoresAPasswordWithNoProxy(t *testing.T) { + clearProxyEnvironment(t) + settings := apiclient.ProxySettings{Password: "s3cret"} + + proxyFunc, err := settings.ProxyFunc() + if !assert.NoError(t, err) { + return + } + + request, _ := http.NewRequest(http.MethodGet, octopusUrl, nil) + proxyUrl, err := proxyFunc(request) + + assert.NoError(t, err) + assert.Nil(t, proxyUrl) +} + +// The error goes to the terminal (and CI logs), so it must not repeat the password +// back - neither from the raw string nor from url.Parse's own *url.Error message. +func TestProxySettings_ProxyFuncDoesNotEchoThePasswordOfAnInvalidProxyUrl(t *testing.T) { + clearProxyEnvironment(t) + + _, err := apiclient.ProxySettings{Url: "http://octo:s3cret@%zz:3128"}.ProxyFunc() + + if assert.ErrorContains(t, err, "invalid proxy url") { + assert.NotContains(t, err.Error(), "s3cret") + assert.Contains(t, err.Error(), "octo:xxxxx@") + } +} + +func TestProxySettingsFromConfig(t *testing.T) { + clearProxyEnvironment(t) + t.Setenv(constants.EnvOctopusProxyUsername, "octo") + t.Setenv(constants.EnvOctopusProxyPassword, "s3cret") + + viper.Set(constants.ConfigProxyUrl, "http://configured:3128") + t.Cleanup(func() { viper.Set(constants.ConfigProxyUrl, "") }) + + settings := apiclient.ProxySettingsFromConfig() + + assert.Equal(t, apiclient.ProxySettings{Url: "http://configured:3128", Username: "octo", Password: "s3cret"}, settings) +} + +func TestNewHttpTransport_SendsRequestsThroughTheProxy(t *testing.T) { + clearProxyEnvironment(t) + + var proxiedUrl, proxyAuthorization string + proxy := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + proxiedUrl = r.URL.String() + proxyAuthorization = r.Header.Get("Proxy-Authorization") + })) + defer proxy.Close() + + transport, err := apiclient.NewHttpTransport(apiclient.ProxySettings{Url: proxy.URL, Username: "octo", Password: "s3cret"}, false) + if !assert.NoError(t, err) { + return + } + + response, err := (&http.Client{Transport: transport}).Get("http://octopus.example.com/api/") + if !assert.NoError(t, err) { + return + } + defer response.Body.Close() + + assert.Equal(t, "http://octopus.example.com/api/", proxiedUrl) + assert.Equal(t, "Basic "+base64.StdEncoding.EncodeToString([]byte("octo:s3cret")), proxyAuthorization) +} + +// The CLI used to configure TLS by mutating the shared default transport, which +// affects every other user of it in the process. +func TestNewHttpTransport_LeavesTheDefaultTransportAlone(t *testing.T) { + clearProxyEnvironment(t) + + transport, err := apiclient.NewHttpTransport(apiclient.ProxySettings{}, true) + if !assert.NoError(t, err) { + return + } + + assert.True(t, transport.TLSClientConfig.InsecureSkipVerify) + if defaultTlsConfig := http.DefaultTransport.(*http.Transport).TLSClientConfig; defaultTlsConfig != nil { + assert.False(t, defaultTlsConfig.InsecureSkipVerify, "the shared default transport must keep verifying certificates") + } +} + +func TestRedactProxyUrl(t *testing.T) { + tests := []struct { + name string + rawUrl string + want string + }{ + {name: "empty", rawUrl: "", want: ""}, + {name: "no credentials", rawUrl: "http://proxy.example.com:3128", want: "http://proxy.example.com:3128"}, + {name: "no scheme", rawUrl: "proxy.example.com:3128", want: "proxy.example.com:3128"}, + {name: "username only", rawUrl: "http://octo@proxy.example.com:3128", want: "http://octo@proxy.example.com:3128"}, + {name: "username and password", rawUrl: "http://octo:s3cret@proxy.example.com:3128", want: "http://octo:xxxxx@proxy.example.com:3128"}, + {name: "unparseable", rawUrl: "http://octo:s3cret@%zz", want: "***"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.want, apiclient.RedactProxyUrl(test.rawUrl)) + assert.NotContains(t, apiclient.RedactProxyUrl(test.rawUrl), "s3cret") + }) + } +} diff --git a/pkg/cmd/config/get/get.go b/pkg/cmd/config/get/get.go index e76b11b0..21e2901e 100644 --- a/pkg/cmd/config/get/get.go +++ b/pkg/cmd/config/get/get.go @@ -3,8 +3,10 @@ package get import ( "fmt" "io" + "strings" "github.com/AlecAivazis/survey/v2" + "github.com/OctopusDeploy/cli/pkg/apiclient" "github.com/OctopusDeploy/cli/pkg/config" "github.com/OctopusDeploy/cli/pkg/constants" "github.com/OctopusDeploy/cli/pkg/factory" @@ -52,6 +54,12 @@ func getRun(isPromptEnabled bool, ask question.Asker, key string, out io.Writer) return fmt.Errorf("unable to get value for key: %s", key) } + // a proxy url can carry a password, and this output routinely ends up in a + // terminal recording or a support ticket. 'config list' redacts it the same way + if strings.EqualFold(key, constants.ConfigProxyUrl) { + value = apiclient.RedactProxyUrl(value) + } + fmt.Fprintln(out, value) return nil } @@ -65,7 +73,7 @@ func promptMissing(ask question.Asker) (string, error) { constants.ConfigOutputFormat, constants.ConfigShowOctopus, constants.ConfigEditor, - // constants.ConfigProxyUrl, + constants.ConfigProxyUrl, } var selectKey string diff --git a/pkg/cmd/config/list/list.go b/pkg/cmd/config/list/list.go index 51a1d630..ed9a1a2d 100644 --- a/pkg/cmd/config/list/list.go +++ b/pkg/cmd/config/list/list.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/MakeNowJust/heredoc/v2" + "github.com/OctopusDeploy/cli/pkg/apiclient" "github.com/OctopusDeploy/cli/pkg/constants" "github.com/OctopusDeploy/cli/pkg/factory" "github.com/OctopusDeploy/cli/pkg/output" @@ -43,12 +44,19 @@ func listRun(cmd *cobra.Command) error { configFile.Set(constants.ConfigAccessToken, "***") } + if configFile.IsSet(constants.ConfigProxyUrl) { + configFile.Set(constants.ConfigProxyUrl, apiclient.RedactProxyUrl(configFile.GetString(constants.ConfigProxyUrl))) + } + type ConfigData struct { + AccessToken string `json:"accesstoken"` ApiKey string `json:"apikey"` Editor string `json:"editor"` Host string `json:"host"` NoPrompt string `json:"noprompt"` OutputFormat string `json:"outputformat"` + ProxyUrl string `json:"proxyurl"` + ShowOctopus string `json:"showoctopus"` Space string `json:"space"` } @@ -62,14 +70,22 @@ func listRun(cmd *cobra.Command) error { configData := &ConfigData{} for _, key := range configFile.AllKeys() { switch strings.ToLower(key) { + // every 'octopus login' writes AccessToken, so without this case the json + // output hard-errors for anyone who has logged in + case strings.ToLower(constants.ConfigAccessToken): + configData.AccessToken = configFile.GetString(key) case strings.ToLower(constants.ConfigApiKey): configData.ApiKey = configFile.GetString(key) + case strings.ToLower(constants.ConfigShowOctopus): + configData.ShowOctopus = configFile.GetString(key) case strings.ToLower(constants.ConfigEditor): configData.Editor = configFile.GetString(key) case strings.ToLower(constants.ConfigUrl): configData.Host = configFile.GetString(key) case strings.ToLower(constants.ConfigNoPrompt): configData.NoPrompt = configFile.GetString(key) + case strings.ToLower(constants.ConfigProxyUrl): + configData.ProxyUrl = configFile.GetString(key) case strings.ToLower(constants.ConfigSpace): configData.Space = configFile.GetString(key) case strings.ToLower(constants.ConfigOutputFormat): diff --git a/pkg/cmd/config/set/set.go b/pkg/cmd/config/set/set.go index e27381af..a9d018ab 100644 --- a/pkg/cmd/config/set/set.go +++ b/pkg/cmd/config/set/set.go @@ -91,7 +91,7 @@ func promptMissing(ask question.Asker, key string) (string, string, error) { constants.ConfigOutputFormat, constants.ConfigShowOctopus, constants.ConfigEditor, - // constants.ConfigProxyUrl, + constants.ConfigProxyUrl, } if key == "" { diff --git a/pkg/cmd/login/login.go b/pkg/cmd/login/login.go index d6ed863d..03cc1d00 100644 --- a/pkg/cmd/login/login.go +++ b/pkg/cmd/login/login.go @@ -2,7 +2,6 @@ package login import ( "bytes" - "crypto/tls" "encoding/json" "errors" "fmt" @@ -121,17 +120,9 @@ func loginRun(cmd *cobra.Command, f factory.Factory, isPromptEnabled bool, ask q return err } - // The http client could be nil, in which case we just use the default one from http - if httpClient == nil { - httpClient = &http.Client{} - } - - if inputs.ignoreSslErrors { - if httpClient.Transport == nil { - httpClient.Transport = &http.Transport{} - } - - httpClient.Transport.(*http.Transport).TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + httpClient, err = ConfigureHttpClient(httpClient, inputs.ignoreSslErrors) + if err != nil { + return err } if inputs.apiKey != "" { @@ -153,6 +144,50 @@ func loginRun(cmd *cobra.Command, f factory.Factory, isPromptEnabled bool, ask q return nil } +// ConfigureHttpClient makes sure login talks to Octopus through the configured proxy. +func ConfigureHttpClient(httpClient *http.Client, ignoreSslErrors bool) (*http.Client, error) { + // the client is nil whenever the CLI has no usable configuration yet, which is the + // common case for login, so build a proxy-aware one rather than letting net/http + // fall back to its default + if httpClient == nil { + transport, err := apiclient.NewHttpTransport(apiclient.ProxySettingsFromConfig(), ignoreSslErrors) + if err != nil { + return nil, err + } + return &http.Client{Transport: transport}, nil + } + + // a client with no transport of its own would silently fall back to + // http.DefaultTransport, which knows nothing about the CLI's proxy settings and + // would drop --ignore-ssl-errors on the floor + if httpClient.Transport == nil { + transport, err := apiclient.NewHttpTransport(apiclient.ProxySettingsFromConfig(), ignoreSslErrors) + if err != nil { + return nil, err + } + httpClient.Transport = transport + return httpClient, nil + } + + // a configured client already carries a proxy-aware transport, so only the ssl + // override needs applying. Any other transport belongs to a caller (tests mock one + // in here) and is left alone. + // + // Two things worth knowing about this branch. It is a no-op while + // NewClientFactoryFromConfig hardcodes insecureSkipVerify to true - it only resets + // the connection pool - and becomes meaningful as soon as that is plumbed through. + // And it mutates the factory's shared client, so the override outlives the login + // probe: fine for a one-shot CLI, a trap for any longer-lived embedding. + if spinnerRoundTripper, ok := httpClient.Transport.(*apiclient.SpinnerRoundTripper); ok && ignoreSslErrors { + transport, err := apiclient.NewHttpTransport(apiclient.ProxySettingsFromConfig(), true) + if err != nil { + return nil, err + } + spinnerRoundTripper.Next = transport + } + return httpClient, nil +} + func loginWithApiKey(configProvider config.IConfigProvider, httpClient *http.Client, server string, apiKey string, cmd *cobra.Command) error { serverLink := output.Cyan(server) diff --git a/pkg/cmd/login/login_test.go b/pkg/cmd/login/login_test.go index 97910e22..2f4e7e4d 100644 --- a/pkg/cmd/login/login_test.go +++ b/pkg/cmd/login/login_test.go @@ -3,9 +3,11 @@ package login_test import ( "bytes" "errors" + "net/http" "testing" "github.com/AlecAivazis/survey/v2" + "github.com/OctopusDeploy/cli/pkg/apiclient" "github.com/OctopusDeploy/cli/pkg/cmd/login" cmdRoot "github.com/OctopusDeploy/cli/pkg/cmd/root" "github.com/OctopusDeploy/cli/pkg/constants" @@ -13,6 +15,7 @@ import ( "github.com/OctopusDeploy/cli/test/testutil" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/users" "github.com/spf13/cobra" + "github.com/spf13/viper" "github.com/stretchr/testify/assert" ) @@ -394,3 +397,58 @@ func TestLogin_OpenIdConnect(t *testing.T) { }) } } + +func TestConfigureHttpClient(t *testing.T) { + viper.Set(constants.ConfigProxyUrl, "http://configured:3128") + t.Cleanup(func() { viper.Set(constants.ConfigProxyUrl, "") }) + + t.Run("builds a proxy aware client when the CLI is not configured yet", func(t *testing.T) { + httpClient, err := login.ConfigureHttpClient(nil, false) + assert.NoError(t, err) + + request, _ := http.NewRequest("GET", "https://octopus.example.com/api/", nil) + proxyUrl, err := httpClient.Transport.(*http.Transport).Proxy(request) + assert.NoError(t, err) + assert.Equal(t, "http://configured:3128", proxyUrl.String()) + }) + + // the code this replaced supported a client with no transport, and dropping that + // leaves --ignore-ssl-errors doing nothing for any factory that returns a plain client + t.Run("gives a client with no transport a proxy aware one", func(t *testing.T) { + httpClient, err := login.ConfigureHttpClient(&http.Client{}, true) + assert.NoError(t, err) + + transport, ok := httpClient.Transport.(*http.Transport) + if !assert.True(t, ok, "expected an *http.Transport") { + return + } + assert.True(t, transport.TLSClientConfig.InsecureSkipVerify) + + request, _ := http.NewRequest("GET", "https://octopus.example.com/api/", nil) + proxyUrl, err := transport.Proxy(request) + assert.NoError(t, err) + assert.Equal(t, "http://configured:3128", proxyUrl.String()) + }) + + t.Run("applies the ssl override without discarding the spinner", func(t *testing.T) { + spinnerRoundTripper := apiclient.NewSpinnerRoundTripper(nil) + httpClient, err := login.ConfigureHttpClient(&http.Client{Transport: spinnerRoundTripper}, true) + assert.NoError(t, err) + + assert.Same(t, spinnerRoundTripper, httpClient.Transport) + assert.True(t, spinnerRoundTripper.Next.(*http.Transport).TLSClientConfig.InsecureSkipVerify) + }) + + // this used to be a type assertion onto *http.Transport, which panics for any + // client that wraps its transport + t.Run("leaves a transport it does not own alone", func(t *testing.T) { + mockClient := testutil.NewMockHttpClientWithTransport(testutil.RoundTripper(func(*http.Request) (*http.Response, error) { + return nil, nil + })) + + httpClient, err := login.ConfigureHttpClient(mockClient, true) + assert.NoError(t, err) + assert.Same(t, mockClient, httpClient) + assert.IsType(t, testutil.RoundTripper(nil), httpClient.Transport) + }) +} diff --git a/pkg/config/config.go b/pkg/config/config.go index 7b2b7999..45d4ff52 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -27,7 +27,7 @@ func setDefaults(v *viper.Viper) { v.SetDefault(constants.ConfigApiKey, "") v.SetDefault(constants.ConfigSpace, "") v.SetDefault(constants.ConfigNoPrompt, false) - // v.SetDefault(constants.ConfigProxyUrl, "") + v.SetDefault(constants.ConfigProxyUrl, "") v.SetDefault(constants.ConfigShowOctopus, true) v.SetDefault(constants.ConfigOutputFormat, "table") @@ -51,6 +51,9 @@ func bindEnvironment(v *viper.Viper) error { if err := v.BindEnv(constants.ConfigSpace, constants.EnvOctopusSpace); err != nil { return err } + if err := v.BindEnv(constants.ConfigProxyUrl, constants.EnvOctopusProxy); err != nil { + return err + } // Envs will take precedence in the specified order if err := v.BindEnv(constants.ConfigEditor, constants.EnvVisual, constants.EnvEditor); err != nil { return err diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go new file mode 100644 index 00000000..d4ec0027 --- /dev/null +++ b/pkg/config/config_test.go @@ -0,0 +1,26 @@ +package config_test + +import ( + "testing" + + "github.com/OctopusDeploy/cli/pkg/config" + "github.com/OctopusDeploy/cli/pkg/constants" + "github.com/spf13/viper" + "github.com/stretchr/testify/assert" +) + +func TestSetup_BindsTheProxyEnvironmentVariable(t *testing.T) { + t.Setenv(constants.EnvOctopusProxy, "http://envproxy:3128") + + v := viper.New() + assert.NoError(t, config.Setup(v)) + + assert.Equal(t, "http://envproxy:3128", v.GetString(constants.ConfigProxyUrl)) +} + +func TestSetup_DefaultsTheProxyToEmpty(t *testing.T) { + v := viper.New() + assert.NoError(t, config.Setup(v)) + + assert.Contains(t, v.AllKeys(), "proxyurl", "the proxy url must be a settable config key") +} diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index 39b2ccf0..a248ecf7 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -29,12 +29,12 @@ const ( // keys for key/value store config file const ( - ConfigUrl = "Url" - ConfigApiKey = "ApiKey" - ConfigAccessToken = "AccessToken" - ConfigSpace = "Space" - ConfigNoPrompt = "NoPrompt" - // ConfigProxyUrl = "ProxyUrl" + ConfigUrl = "Url" + ConfigApiKey = "ApiKey" + ConfigAccessToken = "AccessToken" + ConfigSpace = "Space" + ConfigNoPrompt = "NoPrompt" + ConfigProxyUrl = "ProxyUrl" ConfigEditor = "Editor" ConfigShowOctopus = "ShowOctopus" ConfigOutputFormat = "OutputFormat" @@ -45,9 +45,13 @@ const ( EnvOctopusApiKey = "OCTOPUS_API_KEY" EnvOctopusAccessToken = "OCTOPUS_ACCESS_TOKEN" EnvOctopusSpace = "OCTOPUS_SPACE" - EnvEditor = "EDITOR" - EnvVisual = "VISUAL" - EnvCI = "CI" + EnvOctopusProxy = "OCTOPUS_PROXY" + // Proxy credentials are environment-only; they are never stored in the config file + EnvOctopusProxyUsername = "OCTOPUS_PROXY_USERNAME" + EnvOctopusProxyPassword = "OCTOPUS_PROXY_PASSWORD" + EnvEditor = "EDITOR" + EnvVisual = "VISUAL" + EnvCI = "CI" ) const (