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.

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

Conversation

Jc2k
Copy link

@Jc2k Jc2k commented Nov 3, 2017

As discussed in logstash-plugins/logstash-input-http_poller#87, this PR lets a user configure whether or not to do eager authentication.

I have left the default as true, although manticore upstream makes a case for using false by default.

@@ -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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants