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

Let user set whether to do eager authentication #32

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Let user set whether to do eager authentication #32

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion lib/logstash/plugin_mixins/http_client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,12 @@ def setup_http_client_config

# Password to use for HTTP auth
config :password, :validate => :password

# Whether to instruct Manticore to present basic auth credentials on initial
Copy link
Contributor

Choose a reason for hiding this comment

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

This verbiage is unclear You should only use this if you have a specific need for it, as it may be a security concern otherwise.. It makes it sound like setting this option (and you would only set it to false) would be a security concern.

I personally don't understand what the security concern here is. If there is a fear of a MITM, then SSL certificates should protect against that.

Copy link
Author

Choose a reason for hiding this comment

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

Personally I agree, but that is pretty much a quote from the manticore changelog entry that introduced the eager setting, so I felt I had to defer to them on their library’s settings given we are just passing them through...

I would hate to gloss over some security concern they had but will change it to something less worrying if you want?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in favor of a warning but only if we know why and can make it concrete.

I don't mean to be discouraging. This is a good patch, but I'd rather not
scare people unless we can explain why, especially since eager auth is the
default behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume, by the way, that the security risk is more for people using Apache HC as a generic HTTP client against a variety of endpoints. Some of which don't need passwords sent.

Perhaps we should add a line saying: "The default value true, sends HTTP auth headers to all URLs configured, whether the password is required via an HTTP 401 challenge or not".

# request, rather than being challenged for them. You should only use this
# if you have a specific need for it, as it may be a security concern otherwise.
# Currently defaults to true for backwards compatibility.
config :eager, :validate => :boolean, :default => true
end

public
Expand Down Expand Up @@ -132,7 +138,7 @@ def client_config
c[:auth] = {
:user => @user,
:password => @password.value,
:eager => true
:eager => @eager
}
end

Expand Down