From 4b8ec02efc4076683810b7aad527a42f2397a5cc Mon Sep 17 00:00:00 2001 From: Vaughn Dice Date: Tue, 26 Mar 2019 15:47:35 -0600 Subject: [PATCH] feat(*): mask sensitive values in console output --- docs/content/authoring-bundles.md | 7 ++ examples/azure-mysql-wordpress/porter.yaml | 15 ++- pkg/config/manifest.go | 43 +++++++- pkg/config/manifest_test.go | 115 +++++++++++++++++++++ pkg/context/context.go | 46 ++++++++- pkg/mixin/runner_test.go | 33 ++++++ pkg/porter/build.go | 1 + pkg/porter/parameters.go | 1 + pkg/porter/run.go | 3 + pkg/porter/run_test.go | 3 + 10 files changed, 257 insertions(+), 10 deletions(-) diff --git a/docs/content/authoring-bundles.md b/docs/content/authoring-bundles.md index 5c610d879d..93ef5e7bdd 100644 --- a/docs/content/authoring-bundles.md +++ b/docs/content/authoring-bundles.md @@ -63,6 +63,9 @@ parameters: - name: mysql_user type: string default: azureuser +- name: mysql_password + type: string + sensitive: true - name: database_name type: string default: "wordpress" @@ -76,6 +79,7 @@ parameters: * `destination`: The destination in the bundle to define the parameter. * `env`: The name for the environment variable. Defaults to the name of the parameter in upper case. * `path`: The path for the file. Required for file paths, there is no default. +* `sensitive`: Designate this parameter's value as sensitive, for masking in console output. ## Credentials @@ -85,6 +89,8 @@ you to pass in sensitive data when you execute the bundle, such as passwords or When the bundle is executed, for example when you run `duffle install`, the installer will look on your local system for the named credential and then place the value or file found in the bundle as either an environment variable or file. +By default, all credential values are considered sensitive and will be masked in console output. + ```yaml credentials: - name: SUBSCRIPTION_ID @@ -133,6 +139,7 @@ install: * `MIXIN`: The name of the mixin that will handle this step. In the example above, `helm` is the mixin. * `outputs`: Any outputs provided by the steps. The `name` is required but the rest of the the schema for the output is specific to the mixin. In the example above, the mixin will make the Kubernetes secret data available as outputs. +By default, all output values are considered sensitive and will be masked in console output. ## Dependencies diff --git a/examples/azure-mysql-wordpress/porter.yaml b/examples/azure-mysql-wordpress/porter.yaml index cc29a60bcd..44c0601b3c 100644 --- a/examples/azure-mysql-wordpress/porter.yaml +++ b/examples/azure-mysql-wordpress/porter.yaml @@ -25,7 +25,7 @@ parameters: - name: mysql_password type: string - default: "!Th1s1s4p4ss!" + sensitive: true - name: database_name type: string @@ -73,9 +73,16 @@ install: source: bundle.parameters.mysql_password externalDatabase.database: source: bundle.parameters.database_name + uninstall: - - azure: - description: "Uninstall Wordpress" - name: porter-ci-wordpress + # TODO: enable once the porter-azure mixin supports this action + # - azure: + # description: "Uninstall Mysql" + # name: mysql-azure-porter-demo-wordpress + - helm: + description: "Helm Uninstall Wordpress" + purge: true + releases: + - "porter-ci-wordpress" diff --git a/pkg/config/manifest.go b/pkg/config/manifest.go index 4ed13e56e3..20a865cada 100644 --- a/pkg/config/manifest.go +++ b/pkg/config/manifest.go @@ -19,8 +19,9 @@ import ( type Manifest struct { // path where the manifest was loaded, used to resolve local bundle references - path string - outputs map[string]string + path string + outputs map[string]string + sensitiveValues []string Name string `yaml:"name,omitempty"` Description string `yaml:"description,omitempty"` @@ -48,6 +49,7 @@ type ParameterDefinition struct { MaxLength *int `yaml:"maxLength,omitempty"` Metadata ParameterMetadata `yaml:"metadata,omitempty"` Destination *Location `yaml:"destination,omitempty"` + Sensitive bool `yaml:"sensitive"` } type CredentialDefinition struct { @@ -97,6 +99,10 @@ func (d *Dependency) resolveValue(key string) (interface{}, error) { switch sourceType { case "outputs": replacement = d.m.outputs[sourceName] + // Porter considers all outputs as sensitive + if replacement != nil { + d.m.sensitiveValues = append(d.m.sensitiveValues, reflect.ValueOf(replacement).String()) + } case "parameters": for _, param := range d.m.Parameters { if param.Name == sourceName { @@ -113,6 +119,10 @@ func (d *Dependency) resolveValue(key string) (interface{}, error) { "unknown parameter definition, no environment variable or path specified", ) } + // if replacement has been set and parameter is designated sensitive, add to list of sensitive values + if replacement != nil && param.Sensitive { + d.m.sensitiveValues = append(d.m.sensitiveValues, reflect.ValueOf(replacement).String()) + } } } default: @@ -272,6 +282,13 @@ func (m *Manifest) Validate() error { return result } +func (m *Manifest) GetSensitiveValues() []string { + if m.sensitiveValues == nil { + return []string{} + } + return m.sensitiveValues +} + func (m *Manifest) GetSteps(action Action) (Steps, error) { var steps Steps switch action { @@ -671,7 +688,7 @@ func (m *Manifest) Struct(val reflect.Value) error { return nil } -// Struct implements reflectwalk's StructWalker so that we can skip private fields +// StructField implements reflectwalk's StructWalker so that we can skip private fields func (m *Manifest) StructField(field reflect.StructField, val reflect.Value) error { isUnexported := func() bool { return field.PkgPath != "" @@ -709,6 +726,10 @@ func (m *Manifest) resolveValue(key string) (interface{}, error) { "unknown parameter definition, no environment variable or path specified", ) } + // if replacement has been set and parameter is designated sensitive, add to list of sensitive values + if replacement != nil && param.Sensitive { + m.sensitiveValues = append(m.sensitiveValues, reflect.ValueOf(replacement).String()) + } } } case "credentials": @@ -723,16 +744,30 @@ func (m *Manifest) resolveValue(key string) (interface{}, error) { "unknown credential definition, no environment variable or path specified", ) } + + // if replacement has been set, add to list of sensitive values + if replacement != nil { + m.sensitiveValues = append(m.sensitiveValues, reflect.ValueOf(replacement).String()) + } } } case "outputs": if o, exists := m.outputs[sourceName]; exists { replacement = o + // Porter considers all outputs as sensitive + if replacement != nil { + m.sensitiveValues = append(m.sensitiveValues, reflect.ValueOf(replacement).String()) + } } case "dependencies": for _, dep := range m.Dependencies { if dep.Name == sourceName { - return dep.resolveValue(key) + replacement, err := dep.resolveValue(key) + // Retrieve updated list of sensitive values from dependency and add to our list + for _, val := range dep.m.GetSensitiveValues() { + m.sensitiveValues = append(m.sensitiveValues, val) + } + return replacement, err } } default: diff --git a/pkg/config/manifest_test.go b/pkg/config/manifest_test.go index 4143da3a11..0d33cc19e4 100644 --- a/pkg/config/manifest_test.go +++ b/pkg/config/manifest_test.go @@ -242,6 +242,121 @@ func TestResolveArray(t *testing.T) { assert.Equal(t, "Ralpha", args[0]) } +func TestResolveSensitiveParameter(t *testing.T) { + m := &Manifest{ + Parameters: []ParameterDefinition{ + { + Name: "sensitive_param", + Sensitive: true, + }, + { + Name: "regular_param", + }, + }, + } + + os.Setenv("SENSITIVE_PARAM", "deliciou$dubonnet") + os.Setenv("REGULAR_PARAM", "regular param value") + s := &Step{ + Data: map[string]interface{}{ + "description": "a test step", + "Arguments": []string{ + "source: bundle.parameters.sensitive_param", + "source: bundle.parameters.regular_param", + }, + }, + } + + // Prior to resolving step values, this method should return an empty string array + assert.Equal(t, m.GetSensitiveValues(), []string{}) + + err := m.ResolveStep(s) + require.NoError(t, err) + args, ok := s.Data["Arguments"].([]string) + assert.True(t, ok) + assert.Equal(t, 2, len(args)) + assert.Equal(t, "deliciou$dubonnet", args[0]) + assert.Equal(t, "regular param value", args[1]) + + // There should now be one sensitive value tracked under the manifest + assert.Equal(t, []string{"deliciou$dubonnet"}, m.GetSensitiveValues()) +} + +func TestResolveCredential(t *testing.T) { + m := &Manifest{ + Credentials: []CredentialDefinition{ + { + Name: "password", + EnvironmentVariable: "PASSWORD", + }, + }, + } + + os.Setenv("PASSWORD", "deliciou$dubonnet") + s := &Step{ + Data: map[string]interface{}{ + "description": "a test step", + "Arguments": []string{ + "source: bundle.credentials.password", + }, + }, + } + + // Prior to resolving step values, this method should return an empty string array + assert.Equal(t, m.GetSensitiveValues(), []string{}) + + err := m.ResolveStep(s) + require.NoError(t, err) + args, ok := s.Data["Arguments"].([]string) + assert.True(t, ok) + assert.Equal(t, "deliciou$dubonnet", args[0]) + + // There should now be a sensitive value tracked under the manifest + assert.Equal(t, []string{"deliciou$dubonnet"}, m.GetSensitiveValues()) +} + +func TestResolveOutputs(t *testing.T) { + m := &Manifest{ + outputs: map[string]string{ + "output": "output_value", + }, + Dependencies: []*Dependency{ + &Dependency{ + Name: "dep", + m: &Manifest{ + outputs: map[string]string{ + "dep_output": "dep_output_value", + }, + }, + }, + }, + } + + s := &Step{ + Data: map[string]interface{}{ + "description": "a test step", + "Arguments": []string{ + "source: bundle.outputs.output", + "source: bundle.dependencies.dep.outputs.dep_output", + }, + }, + } + + // Prior to resolving step values, this method should return an empty string array + assert.Equal(t, m.GetSensitiveValues(), []string{}) + + err := m.ResolveStep(s) + require.NoError(t, err) + args, ok := s.Data["Arguments"].([]string) + assert.True(t, ok) + assert.Equal(t, 2, len(args)) + assert.Equal(t, "output_value", args[0]) + assert.Equal(t, "dep_output_value", args[1]) + + // There should now be a sensitive value tracked under the manifest + assert.Equal(t, []string{"output_value", "dep_output_value"}, m.GetSensitiveValues()) +} + func TestResolveInMainDict(t *testing.T) { c := NewTestConfig(t) c.SetupPorterHome() diff --git a/pkg/context/context.go b/pkg/context/context.go index c6025674eb..dd2ded5cc0 100644 --- a/pkg/context/context.go +++ b/pkg/context/context.go @@ -2,6 +2,7 @@ package context import ( "bufio" + "bytes" "fmt" "io" "os" @@ -24,15 +25,43 @@ type Context struct { NewCommand CommandBuilder } +// CensoredWriter is a writer wrapping the provided io.Writer with logic to censor certain values +type CensoredWriter struct { + writer io.Writer + sensitiveValues []string +} + +// NewCensoredWriter returns a new CensoredWriter +func NewCensoredWriter(writer io.Writer) *CensoredWriter { + return &CensoredWriter{writer: writer, sensitiveValues: []string{}} +} + +// SetSensitiveValues sets values needing masking for an CensoredWriter +func (cw *CensoredWriter) SetSensitiveValues(vals []string) { + cw.sensitiveValues = vals +} + +// Write implements io.Writer's Write method, performing necessary auditing while doing so +func (cw *CensoredWriter) Write(b []byte) (int, error) { + auditedBytes := b + for _, val := range cw.sensitiveValues { + auditedBytes = bytes.Replace(auditedBytes, []byte(val), []byte("*******"), -1) + } + + _, err := cw.writer.Write(auditedBytes) + return len(b), err +} + func New() *Context { // Default to respecting the PORTER_DEBUG env variable, the cli will override if --debug is set otherwise _, debug := os.LookupEnv("PORTER_DEBUG") + return &Context{ Debug: debug, FileSystem: &afero.Afero{Fs: afero.NewOsFs()}, In: os.Stdin, - Out: os.Stdout, - Err: os.Stderr, + Out: NewCensoredWriter(os.Stdout), + Err: NewCensoredWriter(os.Stderr), NewCommand: exec.Command, } } @@ -109,3 +138,16 @@ func (c *Context) WriteOutput(lines []string) error { } return nil } + +// SetSensitiveValues sets the sensitive values needing masking on output/err streams +func (c *Context) SetSensitiveValues(vals []string) { + if len(vals) > 0 { + out := NewCensoredWriter(os.Stdout) + out.SetSensitiveValues(vals) + c.Out = out + + err := NewCensoredWriter(os.Stderr) + err.SetSensitiveValues(vals) + c.Err = err + } +} diff --git a/pkg/mixin/runner_test.go b/pkg/mixin/runner_test.go index b0fe07f952..423c51c57e 100644 --- a/pkg/mixin/runner_test.go +++ b/pkg/mixin/runner_test.go @@ -2,10 +2,12 @@ package mixin import ( "bytes" + "io" "path/filepath" "testing" "github.com/deislabs/porter/pkg/context" + "github.com/deislabs/porter/pkg/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -42,6 +44,7 @@ func TestRunner_Validate_MissingExecutable(t *testing.T) { } func TestRunner_Run(t *testing.T) { + // Provide a way for tests to capture stdout output := &bytes.Buffer{} binDir := context.NewTestContext(t).FindBinDir() @@ -62,3 +65,33 @@ func TestRunner_Run(t *testing.T) { assert.NoError(t, err) assert.Contains(t, string(output.Bytes()), "Hello World") } + +func TestRunner_RunWithMaskedOutput(t *testing.T) { + // Provide a way for tests to capture stdout + output := &bytes.Buffer{} + + // Copy output to the test log simultaneously, use go test -v to see the output + aggOutput := io.MultiWriter(output, test.Logger{T: t}) + + // Supply an CensoredWriter with values needing masking + censoredWriter := context.NewCensoredWriter(aggOutput) + censoredWriter.SetSensitiveValues([]string{"World"}) + + binDir := context.NewTestContext(t).FindBinDir() + + // I'm not using the TestRunner because I want to use the current filesystem, not an isolated one + r := NewRunner("exec", filepath.Join(binDir, "mixins/exec"), false) + r.Command = "install" + r.File = "testdata/exec_input.yaml" + + // Capture the output + r.Out = censoredWriter + r.Err = censoredWriter + + err := r.Validate() + require.NoError(t, err) + + err = r.Run() + assert.NoError(t, err) + assert.Contains(t, string(output.Bytes()), "Hello *******") +} diff --git a/pkg/porter/build.go b/pkg/porter/build.go index 0b48313060..d694d0ee06 100644 --- a/pkg/porter/build.go +++ b/pkg/porter/build.go @@ -320,6 +320,7 @@ func (p *Porter) generateBundleParameters() map[string]ParameterDefinition { MaxValue: param.MaxValue, MinLength: param.MinLength, MaxLength: param.MaxLength, + Sensitive: param.Sensitive, } // If the default is empty, set required to true. diff --git a/pkg/porter/parameters.go b/pkg/porter/parameters.go index 378fcf5c0b..af5c6d84c5 100644 --- a/pkg/porter/parameters.go +++ b/pkg/porter/parameters.go @@ -19,6 +19,7 @@ type ParameterDefinition struct { MaxLength *int `json:"maxLength,omitempty" toml:"maxLength,omitempty"` Metadata ParameterMetadata `json:"metadata,omitempty" toml:"metadata,omitempty"` Destination *Location `json:"destination,omitemtpty" toml:"destination,omitempty"` + Sensitive bool `json:"sensitive" toml:"sensitive"` } // ParameterMetadata contains metadata for a parameter definition. diff --git a/pkg/porter/run.go b/pkg/porter/run.go index 4f64d65ff1..3aab4590de 100644 --- a/pkg/porter/run.go +++ b/pkg/porter/run.go @@ -113,6 +113,9 @@ func (p *Porter) Run(opts RunOptions) error { if err != nil { return errors.Wrap(err, "unable to resolve sourced values") } + // Hand over values needing masking in context output streams + p.Context.SetSensitiveValues(p.Manifest.GetSensitiveValues()) + runner := p.loadRunner(step, opts.parsedAction, mixinsDir) err = runner.Validate() diff --git a/pkg/porter/run_test.go b/pkg/porter/run_test.go index 147f876689..fe20c3d3f6 100644 --- a/pkg/porter/run_test.go +++ b/pkg/porter/run_test.go @@ -70,3 +70,6 @@ func TestActionInput_MarshalYAML(t *testing.T) { wantYaml := "install:\n- exec:\n command: echo hi\n" assert.Equal(t, wantYaml, string(b)) } + +// TODO: We need a fully-fledged run unit test for testing all sensitive value variants +// credentials, parameters where sensitive: true, outputs \ No newline at end of file