[4.x] Pass all headers in callWithRequest

[epixa]

To make the behavior consistent with the main proxy, plugin requests through the callWithRequest service should send all client headers to ES rather than none.

This behavior is going to be reversed in 5.0 in favor of a configurable header whitelist, but in the meantime it might as well be consistent. We can't change the proxy behavior in 4.x because it's a backwards compatibility break, so this is the next best option.

Related to #6221

[epixa]

/cc @tbragin @skearns64 @sherry-ger @ycombinator

[epixa]

Closed by #7592

[lukasolson]

While I'm all for consistency, callWithRequest is not a proxy, so I don't think it makes sense to forward ALL the headers. We're noticing more and more headers that we shouldn't be forwarding (origin, content-length) and when 5.0 hits, now we've trained users into thinking that by default, all headers are forwarded, instead of a specified list.

I read through #6221 and it was stated that backporting the headers whitelist would be a breaking change because some people might be relying on all headers being sent through in the proxy. Well... this is even more of a breaking change for anyone using callWithRequest and expecting only the auth header to be forwarded, because there is no way to configure which headers are forwarded.

[epixa]

But while there are Kibana installs that do not leverage callWithRequest (all purely open source installs), there are no Kibana installs that only use callWithRequest. So changing the behavior of callWithRequest makes Kibana consistent, while not doing anything ensures that Kibana is not consistent with shield.

On Jul 8, 2016, at 7:20 PM, Lukas Olson notifications@github.com wrote:

While I'm all for consistency, callWithRequest is not a proxy, so I don't think it makes sense to forward ALL the headers. We're noticing more and more headers that we shouldn't be forwarding (origin, content-length) and when 5.0 hits, now we've trained users into thinking that by default, all headers are forwarded, instead of a specified list.

I read through #6221 and it was stated that backporting the headers whitelist would be a breaking change because some people might be relying on all headers being sent through in the proxy. Well... this is even more of a breaking change for anyone using callWithRequest and expecting only the auth header to be forwarded, because there is no way to configure which headers are forwarded.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub, or mute the thread.

[lukasolson]

Regardless of whether or not it makes it consistent, it's a breaking change, and it is causing problems because callWithRequest is actually not a proxy.

My suggestion is that we either backport the whitelist behavior for callWithRequest only, or we rigorously go through each header that browsers might be sending and ask ourselves, does it make sense to forward this header? We'll also have to do some rigorous testing around ALL of the places that callWithRequest is currently used.

[ycombinator]

I think trying to figure out which browser headers to forward or not is error-prone. I think implementing the whitelist as we did in 5.0 is the safer choice.

[epixa]

I don't understand. What behavior is breaking exactly?

On Jul 8, 2016, at 7:38 PM, Lukas Olson notifications@github.com wrote:

Regardless of whether or not it makes it consistent, it's a breaking change, and it is causing problems because callWithRequest is actually not a proxy.

My suggestion is that we either backport the whitelist behavior for callWithRequest only, or we rigorously go through each header that browsers might be sending and ask ourselves, does it make sense to forward this header? We'll also have to do some rigorous testing around ALL of the places that callWithRequest is currently used.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub, or mute the thread.

[ycombinator]

The implementation in this PR passes through all headers, including Content-Length. As the request made from the Kibana server -> Elasticsearch is not the same as the one made from the browser -> Kibana server, their content lengths will almost always differ.

That's just one specific example but generally speaking, as @lukasolson said, callWithRequest is a not a proxy so we shouldn't try to make it behave like one, when it comes to forwarding headers.

[epixa]

Even with the whitelisting behavior in master, this sounds like a separate bug. That said, I agree that the effect of allowing all headers in 4.x is way worse with this. We should revert it.

On Jul 8, 2016, at 7:53 PM, Shaunak Kashyap notifications@github.com wrote:

The implementation in this PR passes through all headers, including Content-Length. As the request made from the Kibana server -> Elasticsearch is not the same as the one made from the browser -> Kibana server, their content lengths will almost always differ.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub, or mute the thread.