Only proxy whitelisted request headers to ES server upstream #6896

ycombinator · 2016-04-13T14:47:03Z

Besides the unit test included in this PR, I've tested this fix functionally in a couple of different ways:

As described in ES Proxy passing origin header #6484 (comment), I set http.cors.enabled: true in the Elasticsearch config via esvm. Kibana loads up as expected.
Looked at HTTP packets in Wireshark. There is no Origin header in the request that is sent to Elasticsearch:

…tream

w33ble · 2016-04-13T22:40:00Z

I checked this out, it works, everything looks good, except for 1 thing. I think we need to be using a header whitelist instead of a blacklist like this is done currently. Additionally, that whitelist should be modifiable via the kibana.yml file, and then this PR can also close #6221

Seems like everyone else is in agreement on that, so I'm going to ask you to update this PR.

As for passing in a default whitelist, the theory goes that the only header that we might need to forward is Authorization, but even that probably isn't required; we should definitely get a broader consensus. Since we should lean on the side of more restriction by default, having an empty whitelist for now is probably sufficient.

…ation

ycombinator · 2016-04-14T00:18:00Z

@w33ble I've updated the PR to use a whitelist approach as we discussed on Zoom and you summarized in your comment above. Using the default whitelist (no headers), here's what I see with Wireshark, going from the Kibana server to Elasticsearch:

I've also confirmed that the issue in #6484 goes away (because origin is not a whitelisted header by default). Interestingly, however, if I do whitelist only origin, the issue in #6484 does not happen! So I experimented a bit more and found out that it is the combination of the origin and user-agent headers that causes the issue in #6484. Huh.

ycombinator · 2016-04-14T00:22:31Z

With the configurable request header whitelist in place, this PR probably also resolves #6221, pending discussion on whether the default whitelist should be empty (as currently implemented by this PR) or contain certain headers (like authorization). /cc @skearns64 @rashidkpc @panda01

w33ble · 2016-04-14T00:46:34Z

src/plugins/elasticsearch/lib/map_uri.js

+      return header.trim().toLowerCase();
+    });
+
+    return _.pick(originalHeaders, headersToKeepNormalized);


I'm not sure if originalHeaders will always come back lower-case or not. I think it's probably best to normalize them as well when filtering the list.

Good call. Fixing.

w33ble · 2016-04-14T00:54:34Z

Security doesn't work without the authorization header being passed. Auto-login via some proxy has been a really big ask lately as well - I think we should probably include that header in the default list. Of course, this is still pending conversation tomorrow.

Setting the config value makes it all work though:

# array syntax
elasticsearch.requestHeaders: [ 'authorization' ]

# or list syntax, both do the same thing
elasticsearch.requestHeaders: 
  - authorization

w33ble · 2016-04-14T01:07:30Z

Pending that one change, and our discussion about the default values tomorrow, this LGTM.

I'd recommend passing to 1 more person for a second approval, since this is pretty important code. @panda01 is probably a decent candidate, since he's assigned to #6221

epixa · 2016-04-15T19:55:05Z

config/kibana.yml

@@ -61,6 +61,11 @@
 # must be a positive integer.
 # elasticsearch.requestTimeout: 30000

+# List of Kibana client-side headers to send to Elasticsearch. To send *no* client-side
+# headers, set this value to nothing/blank (i.e. elasticsearch.requestHeadersWhitelist: )
+# or the empty list (i.e. elasticsearch.requestHeadersWhitelist: [])


Let's give only one option for this: empty array []. Is there any situation where offering an alternative would be beneficial?

Not particularly. I think as a user its intuitive for blank values to mean nothing (in addition to [] also meaning list with nothing in it) so I documented the alternative. I'm happy to only document the [] option (even though the blank would still work but might be less confusing to the user if we don't actually document it).

If nothing else, for consistency sake I think we should stick to []. I'm sure there are other configurations in kibana.yml that technically would allow an empty value, but we don't advertise it as such. Your call though.

Sounds good to me. I'll document only [] here.

ycombinator · 2016-04-19T09:05:46Z

@epixa I've made the changes you suggested and this is ready for review again. Thanks!

epixa · 2016-04-19T20:17:26Z

LGTM

ycombinator · 2016-04-22T17:09:27Z

Labeling this as a breaking change because, after this change, we pass (by default) fewer headers upstream to Elasticsearch. So if there's an HTTP intermediary between Kibana and Elasticsearch relying on certain headers, they might break.

`eui@83.0.0` ⏩ `83.1.0` --- ## [`83.1.0`](https://github.com/elastic/eui/tree/v83.1.0) - Added `placeholder` prop to `EuiInlineEdit` ([#6883](elastic/eui#6883)) - Added `sparkles` glyph to `EuiIcon` ([#6898](elastic/eui#6898)) **Bug fixes** - Fixed Safari-only bug for single-line row `EuiDataGrid`s, where cell actions on hover would overlap instead of pushing content to the left ([#6881](elastic/eui#6881)) - Fixed `EuiButton` not correctly merging in passed `className`s with its base `.euiButton` class ([#6887](elastic/eui#6887)) - Fixed `EuiIcon` not correctly passing the `style` prop custom `img` icons ([#6888](elastic/eui#6888)) - Fixed multiple components with child props (e.g. `buttonProps`, `iconProps`, etc.) unsetting EUI's Emotion styling if custom `css` was passed to the child props object ([#6896](elastic/eui#6896)) **CSS-in-JS conversions** - Converted `EuiHeader` and `EuiHeaderLogo` to Emotion ([#6878](elastic/eui#6878)) - Removed Sass variables `$euiHeaderDarkBackgroundColor`, `$euiHeaderBorderColor`, and `$euiHeaderBreadcrumbColor` ([#6878](elastic/eui#6878)) - Removed Sass mixin `@euiHeaderDarkTheme` ([#6878](elastic/eui#6878))

ycombinator added 2 commits April 13, 2016 07:18

Filter out origin header before proxying request to ES server upstream

9b268cf

Disable automatic passthrough of request/response headers to/from ups…

1ed0a25

…tream

ycombinator assigned w33ble Apr 13, 2016

ycombinator added the review label Apr 13, 2016

ycombinator mentioned this pull request Apr 13, 2016

Pass incoming request headers to Elasticsearch #6221

Closed

w33ble assigned ycombinator and unassigned w33ble Apr 13, 2016

w33ble added updates_needed and removed review labels Apr 13, 2016

ycombinator added 3 commits April 13, 2016 16:50

Filter headers using whitelist, not blacklist

b102e26

Adding elasticsearch.requestHeaders option to kibana.yml and document…

b635b2c

…ation

Do not send x-forwarded-* headers

e56e98f

ycombinator added the review label Apr 14, 2016

ycombinator assigned w33ble and unassigned ycombinator Apr 14, 2016

ycombinator removed the updates_needed label Apr 14, 2016

ycombinator changed the title ~~Filter out origin request header before proxying request to ES server upstream~~ Only proxy whitelisted request headers to ES server upstream Apr 14, 2016

w33ble reviewed Apr 14, 2016
View reviewed changes

w33ble assigned ycombinator and unassigned w33ble Apr 14, 2016

ycombinator added 2 commits April 13, 2016 18:26

Reformatting

ad5d772

Normalizing original request headers as well, just in case

5cca6f7

ycombinator assigned w33ble and unassigned ycombinator and w33ble Apr 14, 2016

epixa reviewed Apr 15, 2016
View reviewed changes

epixa assigned ycombinator and unassigned epixa Apr 15, 2016

ycombinator added 3 commits April 16, 2016 05:49

Use es6 export default syntax

89dfbee

Only document one option for sending no headers

65ddf96

Adding back x-forwarded-* headers

028db3f

ycombinator assigned epixa and unassigned ycombinator Apr 19, 2016

epixa assigned ycombinator and unassigned epixa Apr 19, 2016

ycombinator added v5.0.0-alpha2 v5.0.0 and removed review labels Apr 19, 2016

ycombinator merged commit aa2f65f into elastic:master Apr 19, 2016

ycombinator deleted the gh-6484 branch April 19, 2016 21:31

ycombinator added the release_note:breaking label Apr 22, 2016

w33ble mentioned this pull request Jul 22, 2016

[Backport] Only proxy whitelisted request headers to ES server upstream #7800

Closed

w33ble added a commit to w33ble/kibana that referenced this pull request Jul 22, 2016

backport elastic#6896 to 4.6

3939ab1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only proxy whitelisted request headers to ES server upstream #6896

Only proxy whitelisted request headers to ES server upstream #6896

ycombinator commented Apr 13, 2016

w33ble commented Apr 13, 2016

ycombinator commented Apr 14, 2016

ycombinator commented Apr 14, 2016

w33ble Apr 14, 2016

ycombinator Apr 14, 2016

w33ble commented Apr 14, 2016

w33ble commented Apr 14, 2016

epixa Apr 15, 2016 •

edited

Loading

ycombinator Apr 15, 2016

epixa Apr 15, 2016

ycombinator Apr 15, 2016

ycombinator commented Apr 19, 2016

epixa commented Apr 19, 2016

ycombinator commented Apr 22, 2016 •

edited

Loading

Only proxy whitelisted request headers to ES server upstream #6896

Only proxy whitelisted request headers to ES server upstream #6896

Conversation

ycombinator commented Apr 13, 2016

w33ble commented Apr 13, 2016

ycombinator commented Apr 14, 2016

ycombinator commented Apr 14, 2016

w33ble Apr 14, 2016

Choose a reason for hiding this comment

ycombinator Apr 14, 2016

Choose a reason for hiding this comment

w33ble commented Apr 14, 2016

w33ble commented Apr 14, 2016

epixa Apr 15, 2016 • edited Loading

Choose a reason for hiding this comment

ycombinator Apr 15, 2016

Choose a reason for hiding this comment

epixa Apr 15, 2016

Choose a reason for hiding this comment

ycombinator Apr 15, 2016

Choose a reason for hiding this comment

ycombinator commented Apr 19, 2016

epixa commented Apr 19, 2016

ycombinator commented Apr 22, 2016 • edited Loading

epixa Apr 15, 2016 •

edited

Loading

ycombinator commented Apr 22, 2016 •

edited

Loading