ensure Plugin#original_params is hides password fields #4952
Conversation
| @@ -142,6 +142,11 @@ def config_init(params) | |||
| instance_variable_set("@#{key}", value) | |||
| end | |||
|
|
|||
| # now that we know the parameters are valid, we can obfuscate the original copy | |||
| # of the parameters before storing them as an instance variable | |||
| self.class.validate_check_parameter_values(original_params) | |||
There was a problem hiding this comment.
Isn't this actually doing the validation twice? I saw https://github.com/elastic/logstash/blob/master/logstash-core/lib/logstash/config/mixin.rb#L252-L265 that is basically the validation method.
I know this fixes the problem, I'm just wondering here if we should have a diff method or some kind of alias that show we're actually running the change to hide the password.
what do you think?
There was a problem hiding this comment.
I struggled a bit with that, and I did a sample implementation of a method just to do that, but it's somewhat awkward and leads to some duplication of code.
I selected validate_check_parameter_values because it only takes care of validating arguments against their validators, so it doesn't populate default values, etc
There was a problem hiding this comment.
I am doing some testing for that, not sure if it could have some side effect in specific cases.
There was a problem hiding this comment.
I am OK with this fix event if we are actually validating twice the submitted values.
The actual problem is we should split the value assignment from the validation.
There was a problem hiding this comment.
I would say, we should add an issue to cleanup this deb, are you ok with
that?
On Thu, Mar 31, 2016 at 3:37 PM Pier-Hugues Pellerin <
notifications@github.com> wrote:
In logstash-core/lib/logstash/config/mixin.rb
#4952 (comment):@@ -142,6 +142,11 @@ def config_init(params)
instance_variable_set("@#{key}", value)
end
now that we know the parameters are valid, we can obfuscate the original copy
of the parameters before storing them as an instance variable
- self.class.validate_check_parameter_values(original_params)
I am OK with this fix event if we are actually validating twice the
submitted values.
The actual problem is we should split the value assignment from the
validation.—
You are receiving this because you commented.Reply to this email directly or view it on GitHub
https://github.com/elastic/logstash/pull/4952/files/ad4641b6fdec0a4073212578ed22f82da392fb6d#r58053907
|
LGTM |
|
refactored this so password hiding is done in a method created for this purpose alone. review welcome |
|
I prefer that refactor, thanks @jsvd, LGTM even more. |
jsvd
changed the title
|
LGTM, but I would so as @andrewvc also recommended s/obfuscation/hidden (or similar) |
|
LGTM pending a change to that comment regarding obfuscation. I tested this manually and saw the password properly masked in the logs. |
No description provided.