diff --git a/cmd/config/config.go b/cmd/config/config.go index 186faf74..ad186feb 100644 --- a/cmd/config/config.go +++ b/cmd/config/config.go @@ -103,7 +103,7 @@ func run(service config.Service) func(*cobra.Command, []string) error { return newErr(err.Error()) } - configJSON, err := json.Marshal(conf) + configJSON, err := json.Marshal(conf.Redacted()) if err != nil { return newErr(err.Error()) } diff --git a/internal/config/config.go b/internal/config/config.go index b6f72004..72b2fd80 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "strconv" "github.com/mitchellh/go-homedir" @@ -16,6 +17,10 @@ import ( const Filename = ".ldcli-config.yml" +// RedactedValue is rendered in place of a sensitive configuration value. It is only ever +// substituted into command output; the value stored in the config file is left untouched. +const RedactedValue = "[REDACTED]" + type ReadFile func(name string) ([]byte, error) // Config represents the data stored in the config file. @@ -47,6 +52,37 @@ func New(filename string, readFile ReadFile) (Config, error) { return c, nil } +// Redacted returns a copy of the Config with sensitive values replaced by RedactedValue, for use +// anywhere a Config is rendered to output. An unset sensitive value is left empty so that it stays +// elided by omitempty rather than being reported as a value that is present but hidden. +func (c Config) Redacted() Config { + if c.AccessToken != "" { + c.AccessToken = RedactedValue + } + + return c +} + +// configKeyPattern describes the shape of every supported configuration key: lowercase words +// joined by single hyphens. +var configKeyPattern = regexp.MustCompile(`^[a-z0-9]+(-[a-z0-9]+)*$`) + +// maxConfigKeyLength bounds what is treated as key-shaped. The longest supported key is well +// under this, while an access token is well over it. +const maxConfigKeyLength = 32 + +// safeKeyForError returns key when it has the shape of a configuration key, and RedactedValue +// otherwise. An unrecognized key is echoed back to help the user spot a typo, but the argument in +// that position is not always a key: transposing `--set ` puts the value there, and +// for access-token that value is a secret. Anything that is not key-shaped is therefore elided. +func safeKeyForError(key string) string { + if len(key) <= maxConfigKeyLength && configKeyPattern.MatchString(key) { + return key + } + + return RedactedValue +} + // Update validates the updating fields and sets them on the Config. It returns the updated fields // in addition to the Config. func (c Config) Update(kvs []string) (Config, []string, error) { @@ -59,7 +95,7 @@ func (c Config) Update(kvs []string) (Config, []string, error) { // TODO: move this list to this package? _, ok := cliflags.AllFlagsHelp()[kvs[i]] if !ok { - return Config{}, updatedFields, errors.NewError(fmt.Sprintf("%s is not a valid configuration option", kvs[i])) + return Config{}, updatedFields, errors.NewError(fmt.Sprintf("%s is not a valid configuration option", safeKeyForError(kvs[i]))) } } @@ -107,7 +143,7 @@ func (c Config) Update(kvs []string) (Config, []string, error) { func (c Config) Remove(key string) (Config, error) { _, ok := cliflags.AllFlagsHelp()[key] if !ok { - return Config{}, errors.NewError(fmt.Sprintf("%s is not a valid configuration option", key)) + return Config{}, errors.NewError(fmt.Sprintf("%s is not a valid configuration option", safeKeyForError(key))) } return c, nil diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 46b9ef31..3078d1cc 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,6 +1,7 @@ package config_test import ( + "encoding/json" "errors" "net/http" "testing" @@ -10,6 +11,7 @@ import ( "gopkg.in/yaml.v3" "github.com/launchdarkly/ldcli/internal/config" + "github.com/launchdarkly/ldcli/internal/output" "github.com/launchdarkly/ldcli/internal/resources" ) @@ -149,3 +151,103 @@ func TestRemove(t *testing.T) { assert.EqualError(t, err, "invalid is not a valid configuration option") }) } + +func TestRedacted(t *testing.T) { + t.Run("replaces a set access token", func(t *testing.T) { + c := config.Config{AccessToken: "test-access-token"} + + redacted := c.Redacted() + + assert.Equal(t, config.RedactedValue, redacted.AccessToken) + }) + + t.Run("leaves an unset access token empty so omitempty still elides it", func(t *testing.T) { + c := config.Config{Project: "test-project"} + + redacted := c.Redacted() + + assert.Equal(t, "", redacted.AccessToken) + + configJSON, err := json.Marshal(redacted) + require.NoError(t, err) + assert.NotContains(t, string(configJSON), "access-token") + }) + + t.Run("leaves non-sensitive values alone", func(t *testing.T) { + optOut := true + c := config.Config{ + AccessToken: "test-access-token", + AnalyticsOptOut: &optOut, + BaseURI: "http://test.com", + DevStreamURI: "http://stream.test.com", + Environment: "test-environment", + Flag: "test-flag", + Output: "json", + Project: "test-project", + } + + redacted := c.Redacted() + + expected := c + expected.AccessToken = config.RedactedValue + assert.Equal(t, expected, redacted) + }) + + t.Run("does not mutate the receiver", func(t *testing.T) { + c := config.Config{AccessToken: "test-access-token"} + + _ = c.Redacted() + + assert.Equal(t, "test-access-token", c.AccessToken) + }) +} + +// TestRedactedOutput covers the rendering paths that `config --list` feeds the redacted Config +// into, so that neither the plaintext nor the JSON representation can reveal the token. +func TestRedactedOutput(t *testing.T) { + c := config.Config{ + AccessToken: "test-access-token", + Project: "test-project", + } + + configJSON, err := json.Marshal(c.Redacted()) + require.NoError(t, err) + + for _, outputKind := range []string{"json", "plaintext"} { + t.Run(outputKind, func(t *testing.T) { + out, err := output.CmdOutputSingular(outputKind, configJSON, output.ConfigPlaintextOutputFn) + + require.NoError(t, err) + assert.NotContains(t, out, "test-access-token") + assert.Contains(t, out, config.RedactedValue) + assert.Contains(t, out, "test-project") + }) + } +} + +// TestErrorDoesNotEchoNonKeyArguments covers the `--set`/`--unset` validation errors, which echo +// back the argument they rejected. A transposed `--set access-token` puts a secret in the +// position a key was expected in, so only key-shaped arguments are echoed. +func TestErrorDoesNotEchoNonKeyArguments(t *testing.T) { + const token = "api-2c2f9f1e-0b1a-4a3a-9d3f-000000000000" + + t.Run("a key-shaped typo is still echoed", func(t *testing.T) { + _, _, err := config.Config{}.Update([]string{"projct", "test-project"}) + + assert.EqualError(t, err, "projct is not a valid configuration option") + }) + + t.Run("a transposed --set does not echo the value", func(t *testing.T) { + _, _, err := config.Config{}.Update([]string{token, "access-token"}) + + assert.EqualError(t, err, config.RedactedValue+" is not a valid configuration option") + assert.NotContains(t, err.Error(), token) + }) + + t.Run("--unset does not echo a non-key argument", func(t *testing.T) { + _, err := config.Config{}.Remove(token) + + assert.EqualError(t, err, config.RedactedValue+" is not a valid configuration option") + assert.NotContains(t, err.Error(), token) + }) +}