diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 7bdf89436a8d8..a895be623c55f 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -106,31 +106,6 @@ class HeaderString { */ bool find(const char* str) const { return strstr(c_str(), str); } - /** - * HeaderString is in token list form, each token separated by commas or whitespace, - * see https://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.1 for more information, - * header field value's case sensitivity depends on each header. - * @return whether contains token in case insensitive manner. - */ - bool caseInsensitiveContains(const char* token) const { - // Avoid dead loop if token argument is empty. - const int n = strlen(token); - if (n == 0) { - return false; - } - - // Find token substring, skip if it's partial of other token. - const char* tokens = c_str(); - for (const char* p = tokens; (p = strcasestr(p, token)); p += n) { - if ((p == tokens || *(p - 1) == ' ' || *(p - 1) == ',') && - (*(p + n) == '\0' || *(p + n) == ' ' || *(p + n) == ',')) { - return true; - } - } - - return false; - } - /** * Set the value of the string by copying data into it. This overwrites any existing string. */ diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index bc14af4821352..d3115d85701e4 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -23,9 +23,9 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest ConnectionManagerConfig& config, const Router::Config& route_config, Runtime::RandomGenerator& random, Runtime::Loader& runtime, const LocalInfo::LocalInfo& local_info) { - // If this is a WebSocket Upgrade request, do not remove the Connection and Upgrade headers, + // If this is a Upgrade request, do not remove the Connection and Upgrade headers, // as we forward them verbatim to the upstream hosts. - if (protocol == Protocol::Http11 && Utility::isWebSocketUpgradeRequest(request_headers)) { + if (protocol == Protocol::Http11 && Utility::isUpgrade(request_headers)) { // The current WebSocket implementation re-uses the HTTP1 codec to send upgrade headers to // the upstream host. This adds the "transfer-encoding: chunked" request header if the stream // has not ended and content-length does not exist. In HTTP1.1, if transfer-encoding and @@ -45,6 +45,7 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest request_headers.removeEnvoyInternalRequest(); request_headers.removeKeepAlive(); request_headers.removeProxyConnection(); + // TODO(alyssawilk) handle this with current and new websocket here and below. request_headers.removeTransferEncoding(); // If we are "using remote address" this means that we create/append to XFF with our immediate @@ -303,7 +304,19 @@ void ConnectionManagerUtility::mutateXfccRequestHeader(Http::HeaderMap& request_ void ConnectionManagerUtility::mutateResponseHeaders(Http::HeaderMap& response_headers, const Http::HeaderMap& request_headers, const std::string& via) { - response_headers.removeConnection(); + if (Utility::isUpgrade(request_headers) && Utility::isUpgrade(response_headers)) { + // As in mutateRequestHeaders, Upgrade responses have special handling. + // + // Unlike mutateRequestHeaders there is no explicit protocol check. If Envoy is proxying an + // upgrade response it has already passed the protocol checks. + const bool no_body = + (!response_headers.TransferEncoding() && !response_headers.ContentLength()); + if (no_body) { + response_headers.insertContentLength().value(uint64_t(0)); + } + } else { + response_headers.removeConnection(); + } response_headers.removeTransferEncoding(); if (request_headers.EnvoyForceTrace() && request_headers.RequestId()) { diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index afd0510dbf77c..1bfc43ef727ea 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -201,15 +201,18 @@ uint64_t Utility::getResponseStatus(const HeaderMap& headers) { return response_code; } -bool Utility::isWebSocketUpgradeRequest(const HeaderMap& headers) { +bool Utility::isUpgrade(const HeaderMap& headers) { // In firefox the "Connection" request header value is "keep-alive, Upgrade", // we should check if it contains the "Upgrade" token. return (headers.Connection() && headers.Upgrade() && - headers.Connection()->value().caseInsensitiveContains( - Http::Headers::get().ConnectionValues.Upgrade.c_str()) && - (0 == StringUtil::caseInsensitiveCompare( - headers.Upgrade()->value().c_str(), - Http::Headers::get().UpgradeValues.WebSocket.c_str()))); + Envoy::StringUtil::caseFindToken(headers.Connection()->value().getStringView(), ",", + Http::Headers::get().ConnectionValues.Upgrade.c_str())); +} + +bool Utility::isWebSocketUpgradeRequest(const HeaderMap& headers) { + return (isUpgrade(headers) && (0 == StringUtil::caseInsensitiveCompare( + headers.Upgrade()->value().c_str(), + Http::Headers::get().UpgradeValues.WebSocket.c_str()))); } Http2Settings diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 2e00748d6b698..8a11ae5129f2b 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -93,6 +93,14 @@ std::string makeSetCookieValue(const std::string& key, const std::string& value, */ uint64_t getResponseStatus(const HeaderMap& headers); +/** + * Determine whether these headers are a valid Upgrade request or response. + * This function returns true if the following HTTP headers and values are present: + * - Connection: Upgrade + * - Upgrade: [any value] + */ +bool isUpgrade(const HeaderMap& headers); + /** * Determine whether this is a WebSocket Upgrade request. * This function returns true if the following HTTP headers and values are present: diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 1d6978c736016..ff8265821a116 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -548,17 +548,74 @@ TEST_F(ConnectionManagerUtilityTest, RemoveConnectionUpgradeForHttp2Requests) { // Test cleaning response headers. TEST_F(ConnectionManagerUtilityTest, MutateResponseHeaders) { TestHeaderMapImpl response_headers{ - {"connection", "foo"}, {"transfer-encoding", "foo"}, {"custom_header", "foo"}}; + {"connection", "foo"}, {"transfer-encoding", "foo"}, {"custom_header", "custom_value"}}; TestHeaderMapImpl request_headers{{"x-request-id", "request-id"}}; ConnectionManagerUtility::mutateResponseHeaders(response_headers, request_headers, ""); EXPECT_EQ(1UL, response_headers.size()); - EXPECT_EQ("foo", response_headers.get_("custom_header")); + EXPECT_EQ("custom_value", response_headers.get_("custom_header")); EXPECT_FALSE(response_headers.has("x-request-id")); EXPECT_FALSE(response_headers.has(Headers::get().Via)); } +// Make sure we don't remove connection headers on all Upgrade responses. +TEST_F(ConnectionManagerUtilityTest, DoNotRemoveConnectionUpgradeForWebSocketResponses) { + TestHeaderMapImpl request_headers{{"connection", "UpGrAdE"}, {"upgrade", "foo"}}; + TestHeaderMapImpl response_headers{ + {"connection", "upgrade"}, {"transfer-encoding", "foo"}, {"upgrade", "bar"}}; + EXPECT_TRUE(Utility::isUpgrade(request_headers)); + EXPECT_TRUE(Utility::isUpgrade(response_headers)); + ConnectionManagerUtility::mutateResponseHeaders(response_headers, request_headers, ""); + + EXPECT_EQ(2UL, response_headers.size()) << response_headers; + EXPECT_EQ("upgrade", response_headers.get_("connection")); + EXPECT_EQ("bar", response_headers.get_("upgrade")); +} + +TEST_F(ConnectionManagerUtilityTest, ClearUpgradeHeadersForNonUpgradeRequests) { + // Test clearing non-upgrade request and response headers + { + TestHeaderMapImpl request_headers{{"x-request-id", "request-id"}}; + TestHeaderMapImpl response_headers{ + {"connection", "foo"}, {"transfer-encoding", "bar"}, {"custom_header", "custom_value"}}; + EXPECT_FALSE(Utility::isUpgrade(request_headers)); + EXPECT_FALSE(Utility::isUpgrade(response_headers)); + ConnectionManagerUtility::mutateResponseHeaders(response_headers, request_headers, ""); + + EXPECT_EQ(1UL, response_headers.size()) << response_headers; + EXPECT_EQ("custom_value", response_headers.get_("custom_header")); + } + + // Test with the request headers not valid upgrade headers + { + TestHeaderMapImpl request_headers{{"upgrade", "foo"}}; + TestHeaderMapImpl response_headers{{"connection", "upgrade"}, + {"transfer-encoding", "eep"}, + {"upgrade", "foo"}, + {"custom_header", "custom_value"}}; + EXPECT_FALSE(Utility::isUpgrade(request_headers)); + EXPECT_TRUE(Utility::isUpgrade(response_headers)); + ConnectionManagerUtility::mutateResponseHeaders(response_headers, request_headers, ""); + + EXPECT_EQ(2UL, response_headers.size()) << response_headers; + EXPECT_EQ("custom_value", response_headers.get_("custom_header")); + EXPECT_EQ("foo", response_headers.get_("upgrade")); + } + + // Test with the response headers not valid upgrade headers + { + TestHeaderMapImpl request_headers{{"connection", "UpGrAdE"}, {"upgrade", "foo"}}; + TestHeaderMapImpl response_headers{{"transfer-encoding", "foo"}, {"upgrade", "bar"}}; + EXPECT_TRUE(Utility::isUpgrade(request_headers)); + EXPECT_FALSE(Utility::isUpgrade(response_headers)); + ConnectionManagerUtility::mutateResponseHeaders(response_headers, request_headers, ""); + + EXPECT_EQ(1UL, response_headers.size()) << response_headers; + EXPECT_EQ("bar", response_headers.get_("upgrade")); + } +} + // Test that we correctly return x-request-id if we were requested to force a trace. TEST_F(ConnectionManagerUtilityTest, MutateResponseHeadersReturnXRequestId) { TestHeaderMapImpl response_headers; diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 141e2ff4ffee4..5b0ce1a7092ad 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -261,31 +261,6 @@ TEST(HeaderStringTest, All) { EXPECT_EQ(HeaderString::Type::Reference, string.type()); } - // caseInsensitiveContains - { - const std::string static_string("keep-alive, Upgrade, close"); - HeaderString string(static_string); - EXPECT_TRUE(string.caseInsensitiveContains("keep-alive")); - EXPECT_TRUE(string.caseInsensitiveContains("Keep-alive")); - EXPECT_TRUE(string.caseInsensitiveContains("Upgrade")); - EXPECT_TRUE(string.caseInsensitiveContains("upgrade")); - EXPECT_TRUE(string.caseInsensitiveContains("close")); - EXPECT_TRUE(string.caseInsensitiveContains("Close")); - EXPECT_FALSE(string.caseInsensitiveContains("")); - EXPECT_FALSE(string.caseInsensitiveContains("keep")); - EXPECT_FALSE(string.caseInsensitiveContains("alive")); - EXPECT_FALSE(string.caseInsensitiveContains("grade")); - - const std::string small("close"); - string.setCopy(small.c_str(), small.size()); - EXPECT_FALSE(string.caseInsensitiveContains("keep-alive")); - - const std::string empty(""); - string.setCopy(empty.c_str(), empty.size()); - EXPECT_FALSE(string.caseInsensitiveContains("keep-alive")); - EXPECT_FALSE(string.caseInsensitiveContains("")); - } - // getString { std::string static_string("HELLO"); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index cd04127fbd27e..c50be78887abe 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -46,6 +46,8 @@ TEST(HttpUtility, isWebSocketUpgradeRequest) { EXPECT_FALSE(Utility::isWebSocketUpgradeRequest(TestHeaderMapImpl{{"upgrade", "websocket"}})); EXPECT_FALSE(Utility::isWebSocketUpgradeRequest( TestHeaderMapImpl{{"Connection", "close"}, {"Upgrade", "websocket"}})); + EXPECT_FALSE(Utility::isUpgrade( + TestHeaderMapImpl{{"Connection", "IsNotAnUpgrade"}, {"Upgrade", "websocket"}})); EXPECT_TRUE(Utility::isWebSocketUpgradeRequest( TestHeaderMapImpl{{"Connection", "upgrade"}, {"Upgrade", "websocket"}})); @@ -55,6 +57,23 @@ TEST(HttpUtility, isWebSocketUpgradeRequest) { TestHeaderMapImpl{{"connection", "Upgrade"}, {"upgrade", "WebSocket"}})); } +TEST(HttpUtility, isUpgrade) { + EXPECT_FALSE(Utility::isUpgrade(TestHeaderMapImpl{})); + EXPECT_FALSE(Utility::isUpgrade(TestHeaderMapImpl{{"connection", "upgrade"}})); + EXPECT_FALSE(Utility::isUpgrade(TestHeaderMapImpl{{"upgrade", "foo"}})); + EXPECT_FALSE(Utility::isUpgrade(TestHeaderMapImpl{{"Connection", "close"}, {"Upgrade", "foo"}})); + EXPECT_FALSE( + Utility::isUpgrade(TestHeaderMapImpl{{"Connection", "IsNotAnUpgrade"}, {"Upgrade", "foo"}})); + EXPECT_FALSE(Utility::isUpgrade( + TestHeaderMapImpl{{"Connection", "Is Not An Upgrade"}, {"Upgrade", "foo"}})); + + EXPECT_TRUE(Utility::isUpgrade(TestHeaderMapImpl{{"Connection", "upgrade"}, {"Upgrade", "foo"}})); + EXPECT_TRUE(Utility::isUpgrade(TestHeaderMapImpl{{"connection", "upgrade"}, {"upgrade", "foo"}})); + EXPECT_TRUE(Utility::isUpgrade(TestHeaderMapImpl{{"connection", "Upgrade"}, {"upgrade", "FoO"}})); + EXPECT_TRUE(Utility::isUpgrade( + TestHeaderMapImpl{{"connection", "keep-alive, Upgrade"}, {"upgrade", "FOO"}})); +} + TEST(HttpUtility, appendXff) { { TestHeaderMapImpl headers; diff --git a/test/integration/websocket_integration_test.cc b/test/integration/websocket_integration_test.cc index c06440d66e141..00c155a79bd83 100644 --- a/test/integration/websocket_integration_test.cc +++ b/test/integration/websocket_integration_test.cc @@ -71,6 +71,13 @@ void WebsocketIntegrationTest::validateFinalDownstreamData(const std::string& re EXPECT_EQ(received_data, expected_data); } +void WebsocketIntegrationTest::validateFinalUpstreamData(const std::string& received_data, + const std::string& expected_data) { + std::regex extra_response_headers("x-request-id:.*\r\n"); + std::string stripped_data = std::regex_replace(received_data, extra_response_headers, ""); + EXPECT_EQ(expected_data, stripped_data); +} + TEST_P(WebsocketIntegrationTest, WebSocketConnectionDownstreamDisconnect) { config_helper_.addConfigModifier(setRouteUsingWebsocket(nullptr)); initialize(); @@ -296,4 +303,64 @@ TEST_P(WebsocketIntegrationTest, WebSocketLogging) { ip_port_regex, ip_port_regex, ip_port_regex))); } +TEST_P(WebsocketIntegrationTest, BidirectionalChunkedData) { + config_helper_.addConfigModifier(setRouteUsingWebsocket(nullptr)); + initialize(); + const std::string upgrade_req_str = "GET /websocket/test HTTP/1.1\r\nHost: host\r\nconnection: " + "keep-alive, Upgrade\r\nupgrade: Websocket\r\n" + "transfer-encoding: chunked\r\n\r\n" + "3\r\n123\r\n0\r\n\r\n" + "SomeWebSocketPayload"; + + // Upgrade, send initial data and wait for it to be received. + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("http")); + tcp_client->write(upgrade_req_str); + FakeRawConnectionPtr fake_upstream_connection = fake_upstreams_[0]->waitForRawConnection(); + // TODO(alyssawilk) We should be able to wait for SomeWebSocketPayload but it + // is not flushed immediately. + const std::string data = + fake_upstream_connection->waitForData(FakeRawConnection::waitForInexactMatch("\r\n\r\n")); + + // Finish the upgrade. + const std::string upgrade_resp_str = + "HTTP/1.1 101 Switching Protocols\r\nconnection: Upgrade\r\nupgrade: Websocket\r\n" + "transfer-encoding: chunked\r\n\r\n" + "4\r\nabcd\r\n0\r\n" + "SomeWebsocketResponsePayload"; + fake_upstream_connection->write(upgrade_resp_str); + tcp_client->waitForData("Payload", false); + + // Verify bidirectional data still works. + tcp_client->write("FinalClientPayload"); + std::string final_data = fake_upstream_connection->waitForData( + FakeRawConnection::waitForInexactMatch("FinalClientPayload")); + fake_upstream_connection->write("FinalServerPayload"); + tcp_client->waitForData("FinalServerPayload", false); + + // Clean up. + tcp_client->close(); + fake_upstream_connection->waitForDisconnect(); + + // TODO(alyssawilk) the current stack is stripping chunked encoding, then + // adding back the chunked encoding header without actually chunk encoding. + // Data about HTTP vs websocket data boundaries is therefore lost. Fix by + // actually chunk encoding. + const std::string old_style_modified_payload = "GET /websocket HTTP/1.1\r\n" + "host: host\r\n" + "connection: keep-alive, Upgrade\r\n" + "upgrade: Websocket\r\n" + "x-forwarded-proto: http\r\n" + "x-envoy-original-path: /websocket/test\r\n" + "transfer-encoding: chunked\r\n\r\n" + "123SomeWebSocketPayloadFinalClientPayload"; + validateFinalUpstreamData(final_data, old_style_modified_payload); + + const std::string modified_downstream_payload = + "HTTP/1.1 101 Switching Protocols\r\nconnection: Upgrade\r\nupgrade: Websocket\r\n" + "transfer-encoding: chunked\r\n\r\n" + "4\r\nabcd\r\n0\r\n" + "SomeWebsocketResponsePayloadFinalServerPayload"; + validateFinalDownstreamData(tcp_client->data(), modified_downstream_payload); +} + } // namespace Envoy diff --git a/test/integration/websocket_integration_test.h b/test/integration/websocket_integration_test.h index d3ac3cb94f75d..468e9582348ec 100644 --- a/test/integration/websocket_integration_test.h +++ b/test/integration/websocket_integration_test.h @@ -17,6 +17,8 @@ class WebsocketIntegrationTest : public HttpIntegrationTest, void validateInitialDownstreamData(const std::string& received_data); void validateFinalDownstreamData(const std::string& received_data, const std::string& expected_data); + void validateFinalUpstreamData(const std::string& received_data, + const std::string& expected_data); const std::string upgrade_req_str_ = "GET /websocket/test HTTP/1.1\r\nHost: host\r\nConnection: " "keep-alive, Upgrade\r\nUpgrade: websocket\r\n\r\n";