Watcher: Add whitelist to HttpClient #36817

spinscale · 2018-12-19T08:35:53Z

This adds a configurable whitelist to the HTTP client in watcher. By
default it allows everything is does not, but there is a dynamically
configurable setting named "xpack.http.hosts.whitelist" that allows to
configure an array of urls, which can also contain simple regexes.

TODO:

fix the code TODO and cast properly, this was a two hour prototype so might more serve as a discussion base instead of being merged
Is a blacklist needed on top of a whitelist? No
Should we block certain things by default as mentioned in the issue? Not for now
Should we always whitelist slack/hipchat/pagerduty or allow to block this as well and tell the user he should whitelist it manually?
Add documentation

Closes #29937

This adds a configurable whitelist to the HTTP client in watcher. By default it allows everything is does not, but there is a dynamically configurable setting named "xpack.http.hosts.whitelist" that allows to configure an array of hosts, which can also contain simple regexes. TODO: - Should we match against a host list or really against URLs? Right now you cannot filter http vs https which might be quite the feature - fix the code TODO and cast properly - Is a blacklist needed on top of a whitelist? - Should we block certain things by default as mentioned in the issue? Closes elastic#29937

elasticmachine · 2018-12-19T08:35:54Z

Pinging @elastic/es-core-features

…tp-client

jakelandis · 2018-12-21T19:05:11Z

Should we match against a host list or really against URLs? Right now
you cannot filter http vs https which might be quite the feature

I think we should support the full URI and not just the host. Some large organizations do path based routing over dns for operational reasons (shared wildcard certs, SSO, etc), and they may to limit access to specific paths within a company's host. I think we should also support a URL with our with the http(s)://, for usability. w/r/t http vs. https allowance, I think this should be a standalone setting allow_insecure_http (or the like) and work towards making false the default.

Is a blacklist needed on top of a whitelist?

I don't think so. More of a personal opinion then anything ... but I have found when having both it is harder to understand which one to configure and how the precedence works for anything but the trivial case.

Should we block certain things by default as mentioned in the issue?

I think this gets tricky to get right and can lead to a false sense of increased security by providing these defaults. However, I think it may help, but the real answer is to require a white list.

…tp-client

spinscale · 2019-01-07T11:25:16Z

I changed the setting to be an URL like https://hooks.slack.com/messages*, this also simplified the code. There are a few todo's left, which I'd work on if the rest makes sense.. (plus documentation)

…tp-client

…whitelisted

spinscale · 2019-01-09T12:38:24Z

Removed the always whitelisting feature for slack/PD/pagerduty. If someone is using this feature, they could easily add the respective endpoints themselves. Documentation was added. TODOs were removed.

I think this is ready for a first review.

Jake: I intentionally did not introduce another allow_insecure_http parameter, as this can all be solved with the existing one. And if someone wants to add http based URLs, so be it.

hub-cap

lgtm

hub-cap · 2019-01-09T16:37:16Z

...ugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/common/http/HttpRequestTests.java

        }
    }

+    public void testToStringDoesNotContainAuthorizationheader() {


nice fix right here :)

jakelandis

a question and a couple nit picks, but LGTM as-is

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpClient.java

...lugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/common/http/HttpClientTests.java

jakelandis · 2019-01-09T18:54:40Z

...ugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/common/http/HttpRequestTests.java

        }
    }

+    public void testToStringDoesNotContainAuthorizationheader() {


…tp-client

spinscale · 2019-01-10T09:45:54Z

@elasticmachine retest this please

…tp-client

spinscale · 2019-01-10T12:20:11Z

applied all the requests changes. I also changed the setting from xpack.http.hosts.whitelist to xpack.http.whitelist as we are no longer whitelisting hosts. There is also no need to introduce this hierarchy.

jakelandis · 2019-01-10T17:35:23Z

still LGTM

hub-cap · 2019-01-10T18:07:23Z

still LGTM

same

spinscale added >enhancement WIP v7.0.0 :Data Management/Watcher labels Dec 19, 2018

spinscale requested review from hub-cap and jakelandis December 19, 2018 08:35

spinscale added 2 commits December 19, 2018 11:00

also sanitize auth headers in HttpRequest.toString()

62eea44

Merge branch 'master' into 1812-watcher-add-whitelist-blacklist-to-ht…

2151f4a

…tp-client

spinscale added 2 commits January 7, 2019 11:18

Merge branch 'master' into 1812-watcher-add-whitelist-blacklist-to-ht…

7a0ae13

…tp-client

allow to filter by URIs including protocol

68437f4

spinscale added 5 commits January 9, 2019 13:07

Merge branch 'master' into 1812-watcher-add-whitelist-blacklist-to-ht…

0175ea9

…tp-client

removed TODO

4725c0c

add documentation

42f4787

remove default white list, just mention in the docs those need to be …

e3a42da

…whitelisted

fix some docs

1ae5d1f

spinscale removed the WIP label Jan 9, 2019

hub-cap approved these changes Jan 9, 2019

View reviewed changes

jakelandis approved these changes Jan 9, 2019

View reviewed changes

spinscale added 2 commits January 10, 2019 09:48

Merge branch 'master' into 1812-watcher-add-whitelist-blacklist-to-ht…

3a48d73

…tp-client

incorporate review comments

5fe448a

spinscale added 4 commits January 10, 2019 11:09

Merge branch 'master' into 1812-watcher-add-whitelist-blacklist-to-ht…

8d324cb

…tp-client

remove unused imports

9648c4f

minor doc update

8b44a35

renamed xpack.http.hosts.whitelist to xpack.http.whitelist

2ba1abc

docs update

f8556ca

spinscale merged commit bbd0930 into elastic:master Jan 11, 2019

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Watcher: Add whitelist to HttpClient #36817

Watcher: Add whitelist to HttpClient #36817

Uh oh!

Conversation

spinscale commented Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 19, 2018

Uh oh!

jakelandis commented Dec 21, 2018

Uh oh!

spinscale commented Jan 7, 2019

Uh oh!

spinscale commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

hub-cap Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

jakelandis Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jakelandis Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

spinscale commented Jan 10, 2019

Uh oh!

spinscale commented Jan 10, 2019

Uh oh!

jakelandis commented Jan 10, 2019

Uh oh!

hub-cap commented Jan 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

spinscale commented Dec 19, 2018 •

edited

Loading

spinscale commented Jan 9, 2019 •

edited

Loading