From d3095694cb5e78c20a73005d8319d9657e470349 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Wed, 19 Aug 2026 11:47:51 +1000 Subject: [PATCH 01/10] feat: explicit proxy configuration for the CLI Standard HTTP_PROXY/HTTPS_PROXY/NO_PROXY already worked for API traffic because the transport chain ends at http.DefaultTransport, but `octopus login --ignore-ssl-errors` built a bare http.Transport that dropped proxy support (and panicked when the client already had one). Adds an OCTOPUS_PROXY environment variable and matching ProxyUrl config key, which override HTTP_PROXY/HTTPS_PROXY for both schemes while still honouring NO_PROXY. Credentials may be embedded in the url or supplied via OCTOPUS_PROXY_USERNAME/OCTOPUS_PROXY_PASSWORD, which are read from the environment only so a password is never written to the config file, and are redacted in `config list`. socks5 comes free from net/http. Refs #49 Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 16 +++ go.mod | 2 +- pkg/apiclient/client_factory.go | 10 +- pkg/apiclient/proxy.go | 122 ++++++++++++++++ pkg/apiclient/proxy_test.go | 239 ++++++++++++++++++++++++++++++++ pkg/cmd/config/get/get.go | 2 +- pkg/cmd/config/list/list.go | 8 ++ pkg/cmd/config/set/set.go | 2 +- pkg/cmd/login/login.go | 41 ++++-- pkg/cmd/login/login_test.go | 40 ++++++ pkg/config/config.go | 5 +- pkg/config/config_test.go | 26 ++++ pkg/constants/constants.go | 22 +-- 13 files changed, 507 insertions(+), 28 deletions(-) create mode 100644 pkg/apiclient/proxy.go create mode 100644 pkg/apiclient/proxy_test.go create mode 100644 pkg/config/config_test.go diff --git a/README.md b/README.md index 032e84c3..0799d2ef 100644 --- a/README.md +++ b/README.md @@ -259,6 +259,22 @@ 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`. Credentials are read from the environment only, so a proxy password is never +written to the CLI config file. + ### 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..1ca3fd6a 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,18 @@ func NewClientFactoryFromConfig(ask question.AskProvider) (ClientFactory, error) return nil, errs } - http.DefaultTransport.(*http.Transport).TLSClientConfig = &tls.Config{InsecureSkipVerify: true} + 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..4bd90b59 --- /dev/null +++ b/pkg/apiclient/proxy.go @@ -0,0 +1,122 @@ +package apiclient + +import ( + "crypto/tls" + "fmt" + "net/http" + "net/url" + "os" + + "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 validate it here + // to report a typo rather than quietly connecting directly. + if _, err := parseProxyUrl(s.Url); err != nil { + return nil, err + } + config.HTTPProxy = s.Url + config.HTTPSProxy = s.Url + 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 + } + 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, fmt.Errorf("invalid proxy url '%s': %w", rawUrl, err) + } + if parsed.Host == "" { + return nil, fmt.Errorf("invalid proxy url '%s': no host specified", rawUrl) + } + return parsed, nil +} diff --git a/pkg/apiclient/proxy_test.go b/pkg/apiclient/proxy_test.go new file mode 100644 index 00000000..68603da2 --- /dev/null +++ b/pkg/apiclient/proxy_test.go @@ -0,0 +1,239 @@ +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 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..b23b3b3f 100644 --- a/pkg/cmd/config/get/get.go +++ b/pkg/cmd/config/get/get.go @@ -65,7 +65,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..8e64b168 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,17 @@ 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 { ApiKey string `json:"apikey"` Editor string `json:"editor"` Host string `json:"host"` NoPrompt string `json:"noprompt"` OutputFormat string `json:"outputformat"` + ProxyUrl string `json:"proxyurl"` Space string `json:"space"` } @@ -70,6 +76,8 @@ func listRun(cmd *cobra.Command) error { 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..79ade32d 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,32 @@ 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 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. + 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..918a9722 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,40 @@ 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()) + }) + + 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 ( From 1b96899d108eba59b4af073645ee1b2e9cb4dd8e Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:07:17 +1000 Subject: [PATCH 02/10] fix: keep the proxy password out of parse error messages An invalid OCTOPUS_PROXY/ProxyUrl was reported by interpolating the raw string into the error, and by wrapping url.Parse's *url.Error, which repeats the whole url again. Both paths printed an embedded password to the terminal and to CI logs. Redact the userinfo and unwrap the *url.Error before reporting. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/apiclient/proxy.go | 38 +++++++++++++++++++++++++++++++++++-- pkg/apiclient/proxy_test.go | 13 +++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/pkg/apiclient/proxy.go b/pkg/apiclient/proxy.go index 4bd90b59..4155637f 100644 --- a/pkg/apiclient/proxy.go +++ b/pkg/apiclient/proxy.go @@ -2,10 +2,12 @@ package apiclient import ( "crypto/tls" + "errors" "fmt" "net/http" "net/url" "os" + "strings" "github.com/OctopusDeploy/cli/pkg/constants" "github.com/spf13/viper" @@ -113,10 +115,42 @@ func parseProxyUrl(rawUrl string) (*url.URL, error) { } } if err != nil { - return nil, fmt.Errorf("invalid proxy url '%s': %w", rawUrl, err) + return nil, invalidProxyUrlError(rawUrl, err) } if parsed.Host == "" { - return nil, fmt.Errorf("invalid proxy url '%s': no host specified", rawUrl) + 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 index 68603da2..c203eee4 100644 --- a/pkg/apiclient/proxy_test.go +++ b/pkg/apiclient/proxy_test.go @@ -162,6 +162,19 @@ func TestProxySettings_ProxyFuncRejectsAnInvalidProxyUrl(t *testing.T) { assert.ErrorContains(t, err, "invalid proxy url") } +// 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") From 1b92f89b940eedb4127dc84cc24d9292d5e12283 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:07:37 +1000 Subject: [PATCH 03/10] refactor: make one parse authoritative for the proxy url parseProxyUrl validated the configured url and then threw the result away, leaving httpproxy to parse the raw string again with its own copy of the same rules. Pass the normalized url through instead, so the two cannot drift into accepting here and ignoring there. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/apiclient/proxy.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/pkg/apiclient/proxy.go b/pkg/apiclient/proxy.go index 4155637f..7a9a55f1 100644 --- a/pkg/apiclient/proxy.go +++ b/pkg/apiclient/proxy.go @@ -42,13 +42,17 @@ func ProxySettingsFromConfig() ProxySettings { 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 validate it here - // to report a typo rather than quietly connecting directly. - if _, err := parseProxyUrl(s.Url); err != nil { + // 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 = s.Url - config.HTTPSProxy = s.Url + config.HTTPProxy = parsed.String() + config.HTTPSProxy = parsed.String() config.CGI = false } From a5ee1404370e41a035a0c3a30e32c50b9c924dc0 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:08:02 +1000 Subject: [PATCH 04/10] fix: report a proxy password set without a username OCTOPUS_PROXY_PASSWORD with no OCTOPUS_PROXY_USERNAME (unset, or a typo'd variable name) dropped the credentials silently and the user got a bare 407 from the proxy with no hint that the CLI had ignored them. Fail with a clear message instead, and only when a proxy is actually resolved and carries no credentials of its own, so a stray variable cannot break a direct connection. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/apiclient/proxy.go | 7 +++++++ pkg/apiclient/proxy_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/pkg/apiclient/proxy.go b/pkg/apiclient/proxy.go index 7a9a55f1..fc618e9c 100644 --- a/pkg/apiclient/proxy.go +++ b/pkg/apiclient/proxy.go @@ -62,6 +62,13 @@ func (s ProxySettings) ProxyFunc() (func(*http.Request) (*url.URL, error), error 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 } diff --git a/pkg/apiclient/proxy_test.go b/pkg/apiclient/proxy_test.go index c203eee4..22d219b0 100644 --- a/pkg/apiclient/proxy_test.go +++ b/pkg/apiclient/proxy_test.go @@ -162,6 +162,39 @@ func TestProxySettings_ProxyFuncRejectsAnInvalidProxyUrl(t *testing.T) { 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) { From 44924886d66943ff77b51d9b90bf7ccf8d16865f Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:08:45 +1000 Subject: [PATCH 05/10] fix: keep supporting a login client with no transport The old code applied the ssl override to any non-nil client, building a transport when the client had none. The rewrite only handled *SpinnerRoundTripper, so a factory returning a plain &http.Client{} got --ignore-ssl-errors silently ignored and no proxy. Build the proxy-aware transport for that case too, and note in the comments that the spinner branch is a no-op while the factory hardcodes insecureSkipVerify, and that it mutates the factory's shared client. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/login/login.go | 18 ++++++++++++++++++ pkg/cmd/login/login_test.go | 18 ++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/pkg/cmd/login/login.go b/pkg/cmd/login/login.go index 79ade32d..03cc1d00 100644 --- a/pkg/cmd/login/login.go +++ b/pkg/cmd/login/login.go @@ -157,9 +157,27 @@ func ConfigureHttpClient(httpClient *http.Client, ignoreSslErrors bool) (*http.C 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 { diff --git a/pkg/cmd/login/login_test.go b/pkg/cmd/login/login_test.go index 918a9722..2f4e7e4d 100644 --- a/pkg/cmd/login/login_test.go +++ b/pkg/cmd/login/login_test.go @@ -412,6 +412,24 @@ func TestConfigureHttpClient(t *testing.T) { 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) From 0ac4906a4eede5d26dc79be1024f0bd7b2fff5e1 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:09:07 +1000 Subject: [PATCH 06/10] fix: redact the proxy password in 'config get ProxyUrl' config get printed the stored ProxyUrl raw, including any embedded user:password, which contradicted the redaction 'config list' applies to the same key. ProxyUrl is new in this change, so nothing depends on the raw value being readable back; the file itself is still there for anyone who needs it. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/config/get/get.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/cmd/config/get/get.go b/pkg/cmd/config/get/get.go index b23b3b3f..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 } From 82f320ddd802743e6751c9a146f1e16b72747813 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:09:57 +1000 Subject: [PATCH 07/10] fix: stop 'config list -f json' erroring on a logged-in config Both login paths always write AccessToken, and ShowOctopus is settable, but neither had a case in the output switch, so any config file containing them fell through to "the key '%s' is not a supported config option" and printed nothing. AccessToken is masked above already, so it lists as *** like ApiKey. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/config/list/list.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/cmd/config/list/list.go b/pkg/cmd/config/list/list.go index 8e64b168..ed9a1a2d 100644 --- a/pkg/cmd/config/list/list.go +++ b/pkg/cmd/config/list/list.go @@ -49,12 +49,14 @@ func listRun(cmd *cobra.Command) error { } 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"` } @@ -68,8 +70,14 @@ 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): From aaaf8a6f57582a9331f4b1355ef8e51dc4f289bc Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:10:12 +1000 Subject: [PATCH 08/10] docs: record why insecureSkipVerify is hardcoded at the call site The reviewer is right that the CLI never verifies the Octopus certificate, but that predates this change and flipping it here would break self-signed installs with no way to opt out. Say so at the call site so the next reader does not have to rediscover it. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/apiclient/client_factory.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/apiclient/client_factory.go b/pkg/apiclient/client_factory.go index 1ca3fd6a..ed411854 100644 --- a/pkg/apiclient/client_factory.go +++ b/pkg/apiclient/client_factory.go @@ -120,6 +120,14 @@ func NewClientFactoryFromConfig(ask question.AskProvider) (ClientFactory, error) return nil, errs } + // 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 From 9dde12d03e49378021668f397f727147c4041cf2 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:10:34 +1000 Subject: [PATCH 09/10] docs: be precise about where a proxy password is stored The previous wording implied no proxy password can reach the config file, but one embedded in ProxyUrl does. Say which of the two is stored, that display is masked, and that the password variable needs the username variable. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 0799d2ef..ea1c1cd2 100644 --- a/README.md +++ b/README.md @@ -272,8 +272,13 @@ 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`. Credentials are read from the environment only, so a proxy password is never -written to the CLI config file. +`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 From 884abe2d3840f37701effde61d70d5245b31080b Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 14 Sep 2026 22:47:20 +1000 Subject: [PATCH 10/10] fix: verify the octopus server certificate by default The CLI set InsecureSkipVerify on the shared http.DefaultTransport unconditionally, so it never verified the Octopus Server's TLS certificate: --ignore-ssl-errors was effectively always on and anything on the network path could read the API key sent with every request. Verification is now on, and turning it off is an explicit choice: the new IgnoreSslErrors config key, its OCTOPUS_IGNORE_SSL_ERRORS environment variable, or 'octopus login --ignore-ssl-errors' for a single login. login honours the config key as well as its own flag so it is not the odd command out. This is a deliberate behaviour change. Anyone relying on the old behaviour, typically a self-signed certificate, now gets a certificate error until they add the CA to the trust store or opt out. 'config set' rejects a non-boolean IgnoreSslErrors value rather than storing something viper would read back as false, and the key is listed by 'config list -f json' and offered by the 'config get'/'config set' pickers. Covered by TestNewClientFactoryFromConfig_TlsVerification, TestConfigureHttpClient's two new subtests, and the two new config.Setup tests. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 21 +++++++++++ pkg/apiclient/client_factory.go | 16 ++++---- pkg/apiclient/client_factory_test.go | 55 ++++++++++++++++++++++++++++ pkg/apiclient/proxy.go | 8 ++++ pkg/cmd/config/get/get.go | 1 + pkg/cmd/config/list/list.go | 21 ++++++----- pkg/cmd/config/set/set.go | 20 +++++++++- pkg/cmd/login/login.go | 15 +++++--- pkg/cmd/login/login_test.go | 32 ++++++++++++++++ pkg/config/config.go | 4 ++ pkg/config/config_test.go | 17 +++++++++ pkg/constants/constants.go | 28 ++++++++------ 12 files changed, 201 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index ea1c1cd2..f2f86fcd 100644 --- a/README.md +++ b/README.md @@ -280,6 +280,27 @@ config file, whereas `octopus config set ProxyUrl` stores whatever it is given i does for an API key. `octopus config list` and `octopus config get ProxyUrl` mask the password when they display it. +### TLS certificate verification + +The CLI verifies the Octopus Server's TLS certificate. If the server presents a certificate this machine +cannot verify — a self-signed certificate, or one from an internal CA that is not in the trust store — the +preferred fix is to add that CA to the machine's trust store. + +Failing that, verification can be turned off explicitly: + +```shell +export OCTOPUS_IGNORE_SSL_ERRORS=true # or: octopus config set IgnoreSslErrors true +octopus login --ignore-ssl-errors # one login, rather than a standing setting +``` + +Only do this on a network path you trust. With verification off, anything positioned between the CLI and +the server can read the API key or access token sent with every request. + +> **Behaviour change:** CLI versions before this one disabled certificate verification unconditionally, so +> this setting had no effect and the warning above applied to every invocation. If the CLI starts failing +> with a certificate error after upgrading, that is the verification now working; fix the trust store or +> opt out with the setting above. + ### go-octopusdeploy library The CLI depends heavily on the [go-octopusdeploy](https://github.com/OctopusDeploy/go-octopusdeploy) library, which manages diff --git a/pkg/apiclient/client_factory.go b/pkg/apiclient/client_factory.go index ed411854..91fe133a 100644 --- a/pkg/apiclient/client_factory.go +++ b/pkg/apiclient/client_factory.go @@ -120,15 +120,13 @@ func NewClientFactoryFromConfig(ask question.AskProvider) (ClientFactory, error) return nil, errs } - // 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) + // The code this replaced set InsecureSkipVerify on the shared http.DefaultTransport + // unconditionally, so the CLI never verified the Octopus server certificate and any + // MITM on the path could read the API key sent with every request. Verification is + // now on unless the user opts out, which is a deliberate behaviour change: anyone + // relying on the old behaviour (typically a self-signed certificate) has to say so + // with OCTOPUS_IGNORE_SSL_ERRORS or 'octopus config set IgnoreSslErrors true'. + transport, err := NewHttpTransport(ProxySettingsFromConfig(), IgnoreSslErrorsFromConfig()) if err != nil { return nil, err } diff --git a/pkg/apiclient/client_factory_test.go b/pkg/apiclient/client_factory_test.go index 4cbc542e..19cc0208 100644 --- a/pkg/apiclient/client_factory_test.go +++ b/pkg/apiclient/client_factory_test.go @@ -1,11 +1,14 @@ package apiclient_test import ( + "net/http" "testing" "github.com/OctopusDeploy/cli/pkg/apiclient" + "github.com/OctopusDeploy/cli/pkg/constants" "github.com/OctopusDeploy/cli/test/testutil" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/client" + "github.com/spf13/viper" "github.com/stretchr/testify/assert" ) @@ -67,3 +70,55 @@ func TestNewClientFactory_WhenHostAndAccessTokenAreSupplied_ReturnsClientFactory testutil.RequireSuccess(t, err) assert.NotNil(t, factory) } + +// The code this replaced set InsecureSkipVerify on the shared http.DefaultTransport +// unconditionally, so the CLI never verified the Octopus server certificate. Verification +// is on by default now and the user has to opt out of it explicitly. +func TestNewClientFactoryFromConfig_TlsVerification(t *testing.T) { + tests := []struct { + name string + ignoreSslErrors any + wantInsecureSkipVerify bool + }{ + {name: "verifies by default", ignoreSslErrors: nil, wantInsecureSkipVerify: false}, + {name: "verifies when the opt out is false", ignoreSslErrors: false, wantInsecureSkipVerify: false}, + {name: "skips verification when opted out", ignoreSslErrors: true, wantInsecureSkipVerify: true}, + {name: "skips verification when opted out via a string", ignoreSslErrors: "true", wantInsecureSkipVerify: true}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + viper.Set(constants.ConfigUrl, hostUrl) + viper.Set(constants.ConfigApiKey, apiKey) + viper.Set(constants.ConfigIgnoreSslErrors, test.ignoreSslErrors) + t.Cleanup(func() { + viper.Set(constants.ConfigUrl, "") + viper.Set(constants.ConfigApiKey, "") + viper.Set(constants.ConfigIgnoreSslErrors, nil) + }) + + factory, err := apiclient.NewClientFactoryFromConfig(qa) + testutil.RequireSuccess(t, err) + + httpClient, err := factory.GetHttpClient() + testutil.RequireSuccess(t, err) + + spinnerRoundTripper, ok := httpClient.Transport.(*apiclient.SpinnerRoundTripper) + if !assert.True(t, ok, "expected a *apiclient.SpinnerRoundTripper") { + return + } + transport, ok := spinnerRoundTripper.Next.(*http.Transport) + if !assert.True(t, ok, "expected the spinner to wrap an *http.Transport") { + return + } + + if test.wantInsecureSkipVerify { + if assert.NotNil(t, transport.TLSClientConfig) { + assert.True(t, transport.TLSClientConfig.InsecureSkipVerify) + } + } else if transport.TLSClientConfig != nil { + assert.False(t, transport.TLSClientConfig.InsecureSkipVerify, "the CLI must verify the Octopus server certificate unless the user opts out") + } + }) + } +} diff --git a/pkg/apiclient/proxy.go b/pkg/apiclient/proxy.go index fc618e9c..936be37b 100644 --- a/pkg/apiclient/proxy.go +++ b/pkg/apiclient/proxy.go @@ -38,6 +38,14 @@ func ProxySettingsFromConfig() ProxySettings { } } +// IgnoreSslErrorsFromConfig reports whether the user has opted out of verifying the +// Octopus server's TLS certificate, via the IgnoreSslErrors config file key or the +// OCTOPUS_IGNORE_SSL_ERRORS environment variable. It defaults to false: anything that +// is not a recognised truthy value leaves verification on, which is the safe direction. +func IgnoreSslErrorsFromConfig() bool { + return viper.GetBool(constants.ConfigIgnoreSslErrors) +} + // ProxyFunc returns a function suitable for http.Transport.Proxy. func (s ProxySettings) ProxyFunc() (func(*http.Request) (*url.URL, error), error) { config := httpproxy.FromEnvironment() diff --git a/pkg/cmd/config/get/get.go b/pkg/cmd/config/get/get.go index 21e2901e..1d8774cc 100644 --- a/pkg/cmd/config/get/get.go +++ b/pkg/cmd/config/get/get.go @@ -74,6 +74,7 @@ func promptMissing(ask question.Asker) (string, error) { constants.ConfigShowOctopus, constants.ConfigEditor, constants.ConfigProxyUrl, + constants.ConfigIgnoreSslErrors, } var selectKey string diff --git a/pkg/cmd/config/list/list.go b/pkg/cmd/config/list/list.go index ed9a1a2d..8ee0b5fa 100644 --- a/pkg/cmd/config/list/list.go +++ b/pkg/cmd/config/list/list.go @@ -49,15 +49,16 @@ func listRun(cmd *cobra.Command) error { } 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"` + AccessToken string `json:"accesstoken"` + ApiKey string `json:"apikey"` + Editor string `json:"editor"` + Host string `json:"host"` + IgnoreSslErrors string `json:"ignoresslerrors"` + NoPrompt string `json:"noprompt"` + OutputFormat string `json:"outputformat"` + ProxyUrl string `json:"proxyurl"` + ShowOctopus string `json:"showoctopus"` + Space string `json:"space"` } outputFormat, _ := cmd.Flags().GetString(constants.FlagOutputFormat) @@ -84,6 +85,8 @@ func listRun(cmd *cobra.Command) error { configData.Host = configFile.GetString(key) case strings.ToLower(constants.ConfigNoPrompt): configData.NoPrompt = configFile.GetString(key) + case strings.ToLower(constants.ConfigIgnoreSslErrors): + configData.IgnoreSslErrors = configFile.GetString(key) case strings.ToLower(constants.ConfigProxyUrl): configData.ProxyUrl = configFile.GetString(key) case strings.ToLower(constants.ConfigSpace): diff --git a/pkg/cmd/config/set/set.go b/pkg/cmd/config/set/set.go index a9d018ab..ae635177 100644 --- a/pkg/cmd/config/set/set.go +++ b/pkg/cmd/config/set/set.go @@ -67,10 +67,14 @@ func setRun(isPromptEnabled bool, ask question.Asker, key string, value string) key = k } key = strings.ToLower(key) - if key == strings.ToLower(constants.ConfigNoPrompt) { + // IgnoreSslErrors turns off certificate verification, so a value that is not + // plainly true or false must be rejected rather than quietly stored: viper would + // read 'yes' back as false, and a user who thought they had turned verification + // off is better served by an error than by a setting that does nothing + if boolKey := boolConfigKey(key); boolKey != "" { boolValue, err := strconv.ParseBool(value) if err != nil { - return fmt.Errorf("the provided value %s is not valid for NoPrompt, please use true of false", value) + return fmt.Errorf("the provided value %s is not valid for %s, please use true of false", value, boolKey) } localViper.Set(key, boolValue) } else { @@ -82,6 +86,17 @@ func setRun(isPromptEnabled bool, ask question.Asker, key string, value string) return nil } +// boolConfigKey returns the display name of the config key if it only accepts a +// boolean value, and an empty string otherwise. +func boolConfigKey(lowercaseKey string) string { + for _, key := range []string{constants.ConfigNoPrompt, constants.ConfigIgnoreSslErrors} { + if lowercaseKey == strings.ToLower(key) { + return key + } + } + return "" +} + func promptMissing(ask question.Asker, key string) (string, string, error) { keys := []string{ constants.ConfigApiKey, @@ -92,6 +107,7 @@ func promptMissing(ask question.Asker, key string) (string, string, error) { constants.ConfigShowOctopus, constants.ConfigEditor, constants.ConfigProxyUrl, + constants.ConfigIgnoreSslErrors, } if key == "" { diff --git a/pkg/cmd/login/login.go b/pkg/cmd/login/login.go index 03cc1d00..5b9911f0 100644 --- a/pkg/cmd/login/login.go +++ b/pkg/cmd/login/login.go @@ -145,7 +145,14 @@ func loginRun(cmd *cobra.Command, f factory.Factory, isPromptEnabled bool, ask q } // ConfigureHttpClient makes sure login talks to Octopus through the configured proxy. +// +// ignoreSslErrors is the --ignore-ssl-errors flag, a one-off opt out of verifying the +// Octopus server certificate. The IgnoreSslErrors config key (and its environment +// variable) is the standing opt out that every other command reads, so it is honoured +// here too - otherwise login would be the only command that still verified. func ConfigureHttpClient(httpClient *http.Client, ignoreSslErrors bool) (*http.Client, error) { + ignoreSslErrors = ignoreSslErrors || apiclient.IgnoreSslErrorsFromConfig() + // 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 @@ -173,11 +180,9 @@ func ConfigureHttpClient(httpClient *http.Client, ignoreSslErrors bool) (*http.C // 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. + // Note that this mutates the factory's shared client rather than cloning it, so + // --ignore-ssl-errors outlives the login probe and applies to every later request + // in the process: 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 { diff --git a/pkg/cmd/login/login_test.go b/pkg/cmd/login/login_test.go index 2f4e7e4d..e2391369 100644 --- a/pkg/cmd/login/login_test.go +++ b/pkg/cmd/login/login_test.go @@ -430,6 +430,38 @@ func TestConfigureHttpClient(t *testing.T) { assert.Equal(t, "http://configured:3128", proxyUrl.String()) }) + // without the flag, and without the config key, login has to keep verifying + t.Run("verifies the server certificate by default", func(t *testing.T) { + httpClient, err := login.ConfigureHttpClient(nil, false) + assert.NoError(t, err) + + transport, ok := httpClient.Transport.(*http.Transport) + if !assert.True(t, ok, "expected an *http.Transport") { + return + } + if transport.TLSClientConfig != nil { + assert.False(t, transport.TLSClientConfig.InsecureSkipVerify) + } + }) + + // the config key is the standing opt out the rest of the CLI reads; login would be + // the odd one out if only its own flag could turn verification off + t.Run("honours the IgnoreSslErrors config key without the flag", func(t *testing.T) { + viper.Set(constants.ConfigIgnoreSslErrors, true) + t.Cleanup(func() { viper.Set(constants.ConfigIgnoreSslErrors, nil) }) + + httpClient, err := login.ConfigureHttpClient(nil, false) + assert.NoError(t, err) + + transport, ok := httpClient.Transport.(*http.Transport) + if !assert.True(t, ok, "expected an *http.Transport") { + return + } + if assert.NotNil(t, transport.TLSClientConfig) { + assert.True(t, transport.TLSClientConfig.InsecureSkipVerify) + } + }) + 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) diff --git a/pkg/config/config.go b/pkg/config/config.go index 45d4ff52..2e6d737c 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -28,6 +28,7 @@ func setDefaults(v *viper.Viper) { v.SetDefault(constants.ConfigSpace, "") v.SetDefault(constants.ConfigNoPrompt, false) v.SetDefault(constants.ConfigProxyUrl, "") + v.SetDefault(constants.ConfigIgnoreSslErrors, false) v.SetDefault(constants.ConfigShowOctopus, true) v.SetDefault(constants.ConfigOutputFormat, "table") @@ -54,6 +55,9 @@ func bindEnvironment(v *viper.Viper) error { if err := v.BindEnv(constants.ConfigProxyUrl, constants.EnvOctopusProxy); err != nil { return err } + if err := v.BindEnv(constants.ConfigIgnoreSslErrors, constants.EnvOctopusIgnoreSslErrors); 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 index d4ec0027..4ab1f9d3 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -24,3 +24,20 @@ func TestSetup_DefaultsTheProxyToEmpty(t *testing.T) { assert.Contains(t, v.AllKeys(), "proxyurl", "the proxy url must be a settable config key") } + +func TestSetup_DefaultsToVerifyingTheServerCertificate(t *testing.T) { + v := viper.New() + assert.NoError(t, config.Setup(v)) + + assert.False(t, v.GetBool(constants.ConfigIgnoreSslErrors)) + assert.Contains(t, v.AllKeys(), "ignoresslerrors", "ignoring ssl errors must be a settable config key") +} + +func TestSetup_BindsTheIgnoreSslErrorsEnvironmentVariable(t *testing.T) { + t.Setenv(constants.EnvOctopusIgnoreSslErrors, "true") + + v := viper.New() + assert.NoError(t, config.Setup(v)) + + assert.True(t, v.GetBool(constants.ConfigIgnoreSslErrors)) +} diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index a248ecf7..442dad5e 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -29,15 +29,16 @@ const ( // keys for key/value store config file const ( - ConfigUrl = "Url" - ConfigApiKey = "ApiKey" - ConfigAccessToken = "AccessToken" - ConfigSpace = "Space" - ConfigNoPrompt = "NoPrompt" - ConfigProxyUrl = "ProxyUrl" - ConfigEditor = "Editor" - ConfigShowOctopus = "ShowOctopus" - ConfigOutputFormat = "OutputFormat" + ConfigUrl = "Url" + ConfigApiKey = "ApiKey" + ConfigAccessToken = "AccessToken" + ConfigSpace = "Space" + ConfigNoPrompt = "NoPrompt" + ConfigProxyUrl = "ProxyUrl" + ConfigEditor = "Editor" + ConfigShowOctopus = "ShowOctopus" + ConfigOutputFormat = "OutputFormat" + ConfigIgnoreSslErrors = "IgnoreSslErrors" ) const ( @@ -49,9 +50,12 @@ const ( // 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" + // Opts out of verifying the Octopus server's TLS certificate. Off by default; + // only set it when the server presents a certificate the machine cannot verify. + EnvOctopusIgnoreSslErrors = "OCTOPUS_IGNORE_SSL_ERRORS" + EnvEditor = "EDITOR" + EnvVisual = "VISUAL" + EnvCI = "CI" ) const (