feat(*): mask sensitive values in console output #251
Conversation
ghost
assigned 
| @@ -63,6 +63,9 @@ parameters: | |||
| - name: mysql_user | |||
| type: string | |||
| default: azureuser | |||
| - name: mysql_password | |||
| type: string | |||
| sensitive: true | |||
| } | ||
| 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 |
There was a problem hiding this comment.
I think I see why you put this here, but it really will need to be refactored or improved later. It's problematic that we need to remember to do this after calling dep.resolveValue and points to a leaky abstraction with how we are either managing our dependencies or how we are collecting and managing these sensitive values I think.
|
|
||
| err = r.Run() | ||
| assert.NoError(t, err) | ||
| assert.Contains(t, string(output.Bytes()), "Hello *******") |
pkg/porter/run.go
Outdated
| @@ -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()) | |||
There was a problem hiding this comment.
Ok, so this is in reference to the comment above about refactoring. What do you think about having p.Context.AppendSensitiveValue("poop")? Then as the manifest finds sensitive words, instead of the manifest tracking them in a slice, and having to coordinate passing it back to the context here (and then also after resolving the dependency's values), it could append to them as it finds them?
You are closer to this, so let me know if you think that would help reduce complexity or not.
There was a problem hiding this comment.
In talking about this offline, I believe we came to the consensus that we're good with revisiting this area of the code after some near-term refactoring.
vdice
force-pushed
the
feat/mask-values
branch
2 times, most recently
from
e40656d to
4b8ec02
Compare
vdice
force-pushed
the
feat/mask-values
branch
2 times, most recently
from
b4a52ae to
8b84627
Compare
There was a problem hiding this comment.
LGTM but since @jeremyrickard just nuked those functions in his new PR, let's have you to work on getting this merged and his nicely.
* jsonschema-lock is finally merged and we can get rid of our fork * Allow numbers in bundle.json again * Add support for params of type array * Add ValidateSchema function * Add output validation * Validate parameter defaults * Validate param regardless of if override or default * Fix retrieval of outputs when characters in the result id are in the output name * Add support for params of object type in ConvertValue (getporter#251) * Use alternate canonical json library * Bump docker libraries to newer version Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
* jsonschema-lock is finally merged and we can get rid of our fork * Allow numbers in bundle.json again * Add support for params of type array * Add ValidateSchema function * Add output validation * Validate parameter defaults * Validate param regardless of if override or default * Fix retrieval of outputs when characters in the result id are in the output name * Add support for params of object type in ConvertValue (getporter#251) * Use alternate canonical json library * Bump docker libraries to newer version Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Mask values in console output if value is:
Implements #132