properly handle 'proxy-connection: close' header #6749

Mythra · 2019-04-29T22:29:03Z

Description:

currently envoy is only checking for the: connection: close header
and not the: proxy-connection: close header. this presents a problem
with HPE, because HPE marks a connection as closed when you specify
proxy-connection: close.

this can cause problems in a scenario where a load balancer is
forwarding traffic to envoy which does not support proxy-connection
header. in this case the load balancer will try to keep forwarding
data onto envoy regardless of the fact that HPE think it's closed.
thus causing a 400. this is a common scenario since proxy-connection
is not standard.

this commit fixes that by properly sending connection: close back
in a response whenever we receive a: proxy-connection: close. this
should properly get the proxy to close the connection as long as it
is spec compliant.

Risk Level: Medium

Testing:
ontop of the unit tests the most basic full test you can run is:

Compile Envoy with this patch.
Supply it with: https://gist.github.com/SecurityInsanity/bd0807d7b8a4a96101106e9aab3a8518
(this is the most simple config I could think of).
Start a netcat session with the local envoy:

$ nc localhost 8443
GET /health_check HTTP/1.1
Host: localhost
Proxy-Connection: close

You should get back a response with: connection: close.

Docs Changes: None
Release Notes: None

Signed-off-by: Eric [email protected]

mattklein123 · 2019-04-30T05:52:20Z

AFAIK proxy-connection is not a spec header so I'm unclear if we should support it? @alyssawilk @PiotrSikora any thoughts here?

alyssawilk · 2019-04-30T13:19:09Z

From what I read

it's basically a "bug" in HTTP/1 which is supposed to be stripped for HTTP/2. Unless it's commonly supported by other OSS proxies I'd be inclined to relegate closing to a filter, though I'd be fine stripping it for HTTP/2. I think this PR as-is will result in goaway-drain for H2 which I think we definitely want to avoid.

@securityinsanity can you give a bit more detail where you're running into this?

Mythra · 2019-04-30T13:32:26Z

It is a non-standard header, and essentially only supported in certain pieces of software (unfortunately http-parser supports it, which does all HTTP Parsing in envoy).

To give a background as to what we ran into:

All of the connections to envoy are long-lived (through an AWS ALB), and reused.
A spammer would send: Proxy-Connection: close in a request. http-parser would mark the stream as closed. Unfortunately, neither envoy or AWS considered the connection closed. Because neither support Proxy-Connection currently.
The next request that would be sent (regardless of from who) on that connection would immediately 400, since HTTP Parser considers the stream closed.

Essentially a spam bot was able to cause random requests to 400 (which has caused tons of problems).

I'm happy to add in a config guard for HTTP/1.1 only, (if we want to strip it for HTTP/2). The main reason we filed this as a PR instead of developing our own filter is that we figured since Http-Parser supported it, and Envoy is built on that it should support it too. That being said it is unstandard so if it doesn't want to be supported we understand.

mattklein123 · 2019-04-30T16:29:09Z

@securityinsanity thanks for the detailed notes. I guess since http_parser supports this we should probably also support it at the higher layer. Agree with @alyssawilk though that we should only do this for HTTP/1.1. I see that this header was already known to Envoy, is it already being stripped for HTTP/2?

Mythra · 2019-04-30T16:32:24Z

@mattklein123 I don't see anywhere: ProxyConnection is used (besides what I added in this PR):

./test/common/http/conn_manager_impl_test.cc
2141:TEST_F(HttpConnectionManagerImplTest, DisconnectOnProxyConnectionDisconnect) {

./test/common/upstream/health_checker_impl_test.cc
1447:TEST_F(HttpHealthCheckerImplTest, ProxyConnectionClose) {

./source/common/http/conn_manager_impl.cc
686:  if (!state_.saw_connection_close_ && request_headers_->ProxyConnection() &&
687:      absl::EqualsIgnoreCase(request_headers_->ProxyConnection()->value().getStringView(),

./source/common/http/conn_manager_utility.cc
78:  request_headers.removeProxyConnection();
362:  response_headers.removeProxyConnection();

./source/common/http/headers.h
89:  const LowerCaseString ProxyConnection{"proxy-connection"};

./source/common/upstream/health_checker_impl.cc
270:      (response_headers_->ProxyConnection() &&
271:       absl::EqualsIgnoreCase(response_headers_->ProxyConnection()->value().getStringView(),

./include/envoy/http/header_map.h
317:  HEADER_FUNC(ProxyConnection)                                                                     \

./test/common/http/http1/conn_pool_test.cc
584:TEST_F(Http1ConnPoolImplTest, ProxyConnectionCloseHeader) {

./source/common/http/http1/conn_pool.cc
282:  if (!saw_close_header_ && headers->ProxyConnection() &&
283:      absl::EqualsIgnoreCase(headers->ProxyConnection()->value().getStringView(),

I'm happy to add the behaviour to this PR though. I'll get going on that now.

mattklein123 · 2019-04-30T16:35:23Z

Sorry I meant it's already in header_map.h. I'm not sure how/why it got there.

Mythra · 2019-04-30T16:39:16Z

Ah that I'm not sure about, git blame shows me you touched it last, though bringing it up does show:

./source/common/http/conn_manager_utility.cc
78:  request_headers.removeProxyConnection();
362:  response_headers.removeProxyConnection();

I'm unfamiliar with that part of the codebase, is that perhaps stripping the headers for HTTP/2?

alyssawilk · 2019-04-30T18:17:06Z

The mutate call strips ProxyConnection for all response headers right before writing them to the client, so it sounds like we do what's "necessary". +1 to what Matt said that if http_parser thinks the connection is closed, Envoy should too - thanks for digging into this.

I think you can scope the effect of your change pretty easily by exempting the close if protocol is Protocol::Http2

Mythra · 2019-04-30T18:25:16Z

Sounds good, I'll do that and add in some tests when I get back from lunch.

Mythra · 2019-04-30T20:28:41Z

Looks like coverage failed again, and release failed for: //test/extensions/filters/http/fault:fault_filter_integration_test timing out. This passes locally however. Is there a way for me to retrigger the tests to get them to pass (and are these known problem areas?)

alyssawilk · 2019-04-30T20:31:06Z

Coverage is borked right now -we're working on it and hopefully we'll have it sorted soon, but Matt also made it so it won't block merges in the interim. So don't worry about getting that passing, and I'll check out the code change regardless of coverage status tomorrow morning :-)

…

On Tue, Apr 30, 2019 at 4:29 PM Eric ***@***.***> wrote: Looks like coverage failed again, and release failed for: //test/extensions/filters/http/fault:fault_filter_integration_test timing out. This passes locally however. Is there a way for me to retrigger the tests to get them to pass (and are these known problem areas?) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6749 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AELALPI4OKN7DQ2CZZX4DKDPTCTZHANCNFSM4HJHG3AQ> .

Mythra · 2019-04-30T20:32:13Z

Thanks! Very much appreciated.

alyssawilk

This is looking great! Good catch on fixing up the connection pool and health checker as well.

FYI I believe we fixed coverage with #6761 so a master merge should get the build green now.

alyssawilk · 2019-05-01T12:54:56Z

source/common/http/conn_manager_impl.cc

@@ -683,6 +683,12 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
                             Http::Headers::get().ConnectionValues.Close)) {
    state_.saw_connection_close_ = true;
  }
+  if (protocol != Protocol::Http2 && !state_.saw_connection_close_ &&


So I believe from code inspection this will correctly remove the ProxyConnection header (which you dug up earlier) and later append the Connection: Close header, but I would like to make sure we have a regression test which both includes closing the connection (which you have) and making sure we swap headers (which I don't think this PR has?) to future-proof against various bits of code moving around.

You can either try to tweak your unit test to make sure the ProxyConnection is removed and Connection is added, or add an integration test to test/integration/http_integration.cc which does
auto response = sendRequestAndWaitForResponse(default_request_headers_, 0, proxy_connection_close, 0) and
EXPECT_THAT(response->headers(),
HeaderValueOf(Headers::get().ProxyConnection,
Http::Headers::get().ConnectionValues.Closed));

Also optional, but it might be worth commenting that we support this non-standard header because http_parser does and we want to have consistent state.

I don't think we'll ever remove support once we add it even if we switch off of http_parser but it'd be nice to track.

I believe I took care of this by adding in the extra EXPECT statement down below which seemed easier than writing an integration test. Although now re-reading this I'm unsure if you wanted to test sending a Proxy-Connection: close and it getting replaced?

alyssawilk · 2019-05-01T13:13:09Z

source/common/upstream/health_checker_impl.cc

@@ -264,9 +264,12 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onResponseComplete() {
    break;
  }

-  if ((response_headers_->Connection() &&
+  if ((response_headers_->Connection() && protocol_ != Http::Protocol::Http2 &&


I was going to suggest a release note for this (as I assumed it changed the behavior for any H2 which was sending an H1 header) but according to the H2 spec the connection header is entirely illegal* and it turns out nghttp2 rejects it so this is a no-op.

https://http2.github.io/http2-spec/#rfc.section.8.1.2.2

I'd be inclined to leave this as-is without the Http::Protocol::Http2 check just on the theory we should keep the code simpler and not special case something illegal.

alyssawilk · 2019-05-01T13:13:25Z

source/common/upstream/health_checker_impl.cc

       absl::EqualsIgnoreCase(response_headers_->Connection()->value().getStringView(),
                              Http::Headers::get().ConnectionValues.Close)) ||
+      (response_headers_->ProxyConnection() &&


I do think we want a "not H2" check here though.

currently envoy is only checking for the: `connection: close` header and not the: `proxy-connection: close` header. this presents a problem with HPE, because HPE marks a connection as closed when you specify `proxy-connection: close`. this can cause problems in a scenario where a load balancer is forwarding traffic to envoy which does not support `proxy-connection` header. in this case the load balancer will try to keep forwarding data onto envoy regardless of the fact that HPE think it's closed. thus causing a 400. this is a common scenario since `proxy-connection` is not standard. this commit fixes that by properly sending `connection: close` back in a response whenever we receive a: `proxy-connection: close`. this should properly get the proxy to close the connection as long as it is spec compliant. --- extra tests to validate: ontop of the unit tests the most basic full test you can run is: 1. Compile Envoy with this patch. 2. Supply it with: https://gist.github.com/SecurityInsanity/bd0807d7b8a4a96101106e9aab3a8518 (this is the most simple config I could think of). 3. Start a netcat session with the local envoy: ``` $ nc localhost 8443 GET /health_check HTTP/1.1 Host: localhost Proxy-Connection: close ``` You should get back a response with: `connection: close`. Signed-off-by: Eric <[email protected]>

Signed-off-by: Eric <[email protected]>

alyssawilk

Yep - while I'm always a fan of integration tests, I'm fine with you covering header removal in the unit test :-)

@PiotrSikora any thoughts from your end? If not I'm inclined to bump this to medium risk (just because it affects the HTTP data plane) and merge it later today.

PiotrSikora

Thanks for digging into this, @securityinsanity! That's a pretty nasty way to reject legitimate requests.

This PR looks great, if we want to keep the fix within Envoy's codebase, but honestly, I'd much rather see this fixed in HTTP Parser, and simply ignore (i.e. pass-through) Proxy-Connection in Envoy, instead of adding support for it, since no legitimate clients should be using this header nowadays, and other modern proxies don't have any special support for this header.

PiotrSikora · 2019-05-01T18:56:40Z

test/common/http/http1/conn_pool_test.cc

+  conn_pool_.test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected);
+  callbacks.outer_encoder_->encodeHeaders(TestHeaderMapImpl{}, true);
+
+  // Response with 'connection: close' which should cause the connection to go away.


Nit: proxy-connection: close.

Signed-off-by: Eric <[email protected]>

Mythra · 2019-05-01T19:40:30Z

FWIW on the legitimate clients front, our AWS Support rep did mention that many Microsoft products will send this header to get certain behaviors out of web servers. Unfortunately, they didn't expand on what "many" was. It does seem to line up (see the SQL Server Protocol for example: https://docs.microsoft.com/en-us/openspecs/sql_server_protocols/ms-ssas8/c2b9f128-76f3-48f9-b23d-6bf78516f842 ). Unsure if that's an actual use case envoy/nodejs cares about (it's certainly not one that impacts us, we just want our 400s to stop heh), or if Microsoft all sends "Connection" headers. Just figured I'd mention it for context.

PiotrSikora · 2019-05-01T20:04:35Z

It seems that SQL Server sends Proxy-Connection: Keep-Alive to keep the connection open, which is the default behavior in HTTP/1.1, so ignoring it is perfectly safe.

Mythra · 2019-05-01T20:10:38Z

@PiotrSikora would you be opposed to perhaps merging this now, and then I can file an issue on http-parser, and we can then remove it after http-parser has fixed it? It is completely blocking most things we do here, and while I understand that's not your problem it could be a good interim solution.

PiotrSikora · 2019-05-02T00:13:56Z

I'm not against merging this PR, but it shouldn't be necessary (long-term), and if the consensus is that this header should be simply ignored, then we're adding workaround for http-parser and changing behavior that we're going to revert in O(weeks), which isn't great.

I'll defer the decision to @alyssawilk and/or @mattklein123.

mattklein123 · 2019-05-02T16:28:10Z

I don't have a strong opinion. I'm OK with merging since that is going to be a lot faster than trying to fix http_parser unless we just pull the plug and fork it until we switch to something new (because I'm sure they are going to want a config option). @alyssawilk @envoyproxy/maintainers WDYT? We should at the very least open an http_parser issue and link here @securityinsanity

Mythra · 2019-05-02T16:37:24Z

Thanks Matt, I've created: nodejs/http-parser#477 which is fairly bare-bones, but can hopefully kick off the discussion with the http-parser maintainers.

currently envoy is only checking for the: connection: close header and not the: proxy-connection: close header. this presents a problem with HPE, because HPE marks a connection as closed when you specify proxy-connection: close. this can cause problems in a scenario where a load balancer is forwarding traffic to envoy which does not support proxy-connection header. in this case the load balancer will try to keep forwarding data onto envoy regardless of the fact that HPE think it's closed. thus causing a 400. this is a common scenario since proxy-connection is not standard. this commit fixes that by properly sending connection: close back in a response whenever we receive a: proxy-connection: close. this should properly get the proxy to close the connection as long as it is spec compliant. Risk Level: Medium Testing: unit tests, manual integration tests. Docs Changes: None Release Notes: None Signed-off-by: Eric <[email protected]> Signed-off-by: Jeff Piazza <[email protected]>

mattklein123 assigned alyssawilk and PiotrSikora Apr 30, 2019

alyssawilk reviewed May 1, 2019

View reviewed changes

Eric added 2 commits May 1, 2019 07:29

scope proxy-connection close to HTTP1/HTTP1.1

20e9c6c

Signed-off-by: Eric <[email protected]>

alyssawilk added the waiting label May 1, 2019

add test + comment to proxyconnection

a44fa74

Signed-off-by: Eric <[email protected]>

Mythra force-pushed the closed-connection branch from 6487bd9 to a44fa74 Compare May 1, 2019 15:20

repokitteh-read-only bot removed the waiting label May 1, 2019

alyssawilk previously approved these changes May 1, 2019

View reviewed changes

PiotrSikora reviewed May 1, 2019

View reviewed changes

fix comment to properly reflect

d99e034

Signed-off-by: Eric <[email protected]>

Mythra dismissed alyssawilk’s stale review via d99e034 May 1, 2019 19:28

Mythra mentioned this pull request May 2, 2019

Option to Drop support for Proxy-Connection nodejs/http-parser#477

Open

alyssawilk approved these changes May 2, 2019

View reviewed changes

alyssawilk merged commit 8c39944 into envoyproxy:master May 2, 2019

Mythra deleted the closed-connection branch May 2, 2019 19:01

Mythra mentioned this pull request Aug 14, 2019

Stable release management #7915

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

properly handle 'proxy-connection: close' header #6749

properly handle 'proxy-connection: close' header #6749

Mythra commented Apr 29, 2019 •

edited by alyssawilk

Loading

mattklein123 commented Apr 30, 2019

alyssawilk commented Apr 30, 2019

Mythra commented Apr 30, 2019

mattklein123 commented Apr 30, 2019

Mythra commented Apr 30, 2019

mattklein123 commented Apr 30, 2019

Mythra commented Apr 30, 2019

alyssawilk commented Apr 30, 2019

Mythra commented Apr 30, 2019

Mythra commented Apr 30, 2019

alyssawilk commented Apr 30, 2019 via email

Mythra commented Apr 30, 2019

alyssawilk left a comment

alyssawilk May 1, 2019

alyssawilk May 1, 2019

Mythra May 1, 2019

alyssawilk May 1, 2019

alyssawilk May 1, 2019

alyssawilk left a comment

PiotrSikora left a comment

PiotrSikora May 1, 2019

Mythra May 1, 2019

Mythra commented May 1, 2019

PiotrSikora commented May 1, 2019

Mythra commented May 1, 2019

PiotrSikora commented May 2, 2019

mattklein123 commented May 2, 2019

Mythra commented May 2, 2019

properly handle 'proxy-connection: close' header #6749

properly handle 'proxy-connection: close' header #6749

Conversation

Mythra commented Apr 29, 2019 • edited by alyssawilk Loading

mattklein123 commented Apr 30, 2019

alyssawilk commented Apr 30, 2019

Mythra commented Apr 30, 2019

mattklein123 commented Apr 30, 2019

Mythra commented Apr 30, 2019

mattklein123 commented Apr 30, 2019

Mythra commented Apr 30, 2019

alyssawilk commented Apr 30, 2019

Mythra commented Apr 30, 2019

Mythra commented Apr 30, 2019

alyssawilk commented Apr 30, 2019 via email

Mythra commented Apr 30, 2019

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk May 1, 2019

Choose a reason for hiding this comment

alyssawilk May 1, 2019

Choose a reason for hiding this comment

Mythra May 1, 2019

Choose a reason for hiding this comment

alyssawilk May 1, 2019

Choose a reason for hiding this comment

alyssawilk May 1, 2019

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

PiotrSikora left a comment

Choose a reason for hiding this comment

PiotrSikora May 1, 2019

Choose a reason for hiding this comment

Mythra May 1, 2019

Choose a reason for hiding this comment

Mythra commented May 1, 2019

PiotrSikora commented May 1, 2019

Mythra commented May 1, 2019

PiotrSikora commented May 2, 2019

mattklein123 commented May 2, 2019

Mythra commented May 2, 2019

Mythra commented Apr 29, 2019 •

edited by alyssawilk

Loading