Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ensure Plugin#original_params is hides password fields #4952

Closed
wants to merge 2 commits into from
Closed

ensure Plugin#original_params is hides password fields #4952

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad4641b
ensure Plugin#original_params is obfuscated
jsvd Mar 31, 2016
b3d02b4
avoid reusing generic validation method during params securization
jsvd Mar 31, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion logstash-core/lib/logstash/config/mixin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def config_init(params)
# Keep a copy of the original config params so that we can later
# differentiate between explicit configuration and implicit (default)
# configuration.
@original_params = params.clone
original_params = params.clone

# store the plugin type, turns LogStash::Inputs::Base into 'input'
@plugin_type = self.class.ancestors.find { |a| a.name =~ /::Base$/ }.config_name
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about changing obfuscate to hide when talking about security, obfuscate has this idea of security through obscurity, what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have feelings either way. imo, it's correct in the sense of english (it can mean blur, complicate, etc). as for security by obscurity..this is opensource :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @purbon , an obfuscation implies that something can be reversed (an obfuscated password is something like HTTP basic, where the pass is base64 encoded). This is hidden or masked. I think this is an important distinction to make here if someone is reading this for a security review.

# of the parameters before storing them as an instance variable
self.class.validate_check_parameter_values(original_params)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am doing some testing for that, not sure if it could have some side effect in specific cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@original_params = original_params

@config = params
end # def config_init

Expand Down
4 changes: 4 additions & 0 deletions logstash-core/spec/logstash/config/mixin_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@
clone = subject.class.new(subject.params)
expect(clone.password.value).to(be == secret)
end

it "should obfuscate original_params" do
expect(subject.original_params['password']).to(be_a(LogStash::Util::Password))
end
end

describe "obsolete settings" do
Expand Down