From 5cfac300d7f49397e8f89415bc017941402021fb Mon Sep 17 00:00:00 2001 From: John Plevyak Date: Tue, 20 Aug 2019 15:02:18 -0700 Subject: [PATCH] Do not 503 on Upgrade: h2c instead remove the header and ignore. Signed-off-by: John Plevyak --- source/common/common/utility.cc | 27 +++++++++++++++++ source/common/common/utility.h | 15 +++++++++ source/common/http/headers.h | 3 ++ source/common/http/http1/codec_impl.cc | 28 +++++++++++++++-- test/common/common/utility_speed_test.cc | 23 ++++++++++++++ test/common/common/utility_test.cc | 25 +++++++++++++++ test/common/http/http1/codec_impl_test.cc | 37 +++++++++++++++++++++++ 7 files changed, 156 insertions(+), 2 deletions(-) diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index bb937f0dca06..eb6f5b2b8da5 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -338,6 +338,33 @@ std::vector StringUtil::splitToken(absl::string_view source, return absl::StrSplit(source, absl::ByAnyChar(delimiters), absl::SkipEmpty()); } +std::string StringUtil::removeTokens(absl::string_view source, absl::string_view delimiters, + std::set tokens_to_remove, + absl::string_view joiner, bool trim_tokens, bool ignore_case) { + const auto values = Envoy::StringUtil::splitToken(source, delimiters); + std::string new_value; + for (auto& v : values) { + absl::string_view token_for_compare = v; + absl::string_view token_for_result = v; + if (trim_tokens) { + token_for_result = token_for_compare = StringUtil::trim(v); + } + std::string lower_value; + if (ignore_case) { + lower_value = StringUtil::toLower(token_for_compare); + token_for_compare = lower_value; + } + if (tokens_to_remove.count(token_for_compare) != 0) { + continue; + } + if (!new_value.empty()) { + new_value.append(joiner.data(), joiner.size()); + } + new_value.append(token_for_result.data(), token_for_result.size()); + } + return new_value; +} + uint32_t StringUtil::itoa(char* out, size_t buffer_size, uint64_t i) { // The maximum size required for an unsigned 64-bit integer is 21 chars (including null). if (buffer_size < 21) { diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 5ab6e41f8b13..1506c4bf4b84 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -285,6 +285,21 @@ class StringUtil { absl::string_view delimiters, bool keep_empty_string = false); + /** + * Remove tokens from a delimiter-separated string view. + * @param source supplies the delimiter-separated string view. + * @param multi-delimiter supplies chars used to split the delimiter-separated string view. + * @param tokens_to_remove supplies a set of tokens which should not appear in the result. + * @param joiner contains a string used between tokens in the result. + * @param trim_tokens trims the the tokens before comparing to 'tokens_to_remove'. + * @param ignore_case compares the tokens in a case-insensitive way to tokens_to_remove'. + * @return string of the remaining joined tokens. + */ + static std::string removeTokens(absl::string_view source, absl::string_view delimiters, + std::set tokens_to_remove, + absl::string_view joiner, bool trim_tokens = true, + bool ignore_case = true); + /** * Size-bounded string copying and concatenation */ diff --git a/source/common/http/headers.h b/source/common/http/headers.h index 79c0b1cd153c..30daeebf8d81 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -76,6 +76,7 @@ class HeaderValues { const LowerCaseString GrpcAcceptEncoding{"grpc-accept-encoding"}; const LowerCaseString Host{":authority"}; const LowerCaseString HostLegacy{"host"}; + const LowerCaseString Http2Settings{"http2-settings"}; const LowerCaseString KeepAlive{"keep-alive"}; const LowerCaseString LastModified{"last-modified"}; const LowerCaseString Location{"location"}; @@ -104,11 +105,13 @@ class HeaderValues { struct { const std::string Close{"close"}; + const std::string Http2Settings{"http2-settings"}; const std::string KeepAlive{"keep-alive"}; const std::string Upgrade{"upgrade"}; } ConnectionValues; struct { + const std::string H2c{"h2c"}; const std::string WebSocket{"websocket"}; } UpgradeValues; diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 9adef3d928c3..c9279542eecb 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -4,6 +4,8 @@ #include #include +#include "absl/strings/match.h" + #include "envoy/buffer/buffer.h" #include "envoy/http/header_map.h" #include "envoy/network/connection.h" @@ -447,8 +449,30 @@ int ConnectionImpl::onHeadersCompleteBase() { protocol_ = Protocol::Http10; } if (Utility::isUpgrade(*current_header_map_)) { - ENVOY_CONN_LOG(trace, "codec entering upgrade mode.", connection_); - handling_upgrade_ = true; + // Ignore h2c upgrade requests until we support them. + // See https://github.com/envoyproxy/envoy/issues/7161 for details. + if (current_header_map_->Upgrade() && + absl::EqualsIgnoreCase(current_header_map_->Upgrade()->value().getStringView(), + Http::Headers::get().UpgradeValues.H2c)) { + ENVOY_CONN_LOG(trace, "removing unsupported h2c upgrade headers.", connection_); + current_header_map_->removeUpgrade(); + if (current_header_map_->Connection()) { + auto new_value = StringUtil::removeTokens( + current_header_map_->Connection()->value().getStringView(), ",", + {Http::Headers::get().ConnectionValues.Upgrade, + Http::Headers::get().ConnectionValues.Http2Settings}, + ","); + if (new_value.empty()) { + current_header_map_->removeConnection(); + } else { + current_header_map_->Connection()->value(new_value); + } + } + current_header_map_->remove(Headers::get().Http2Settings); + } else { + ENVOY_CONN_LOG(trace, "codec entering upgrade mode.", connection_); + handling_upgrade_ = true; + } } int rc = onHeadersComplete(std::move(current_header_map_)); diff --git a/test/common/common/utility_speed_test.cc b/test/common/common/utility_speed_test.cc index 69ed1b41ff4d..24c8e8751c40 100644 --- a/test/common/common/utility_speed_test.cc +++ b/test/common/common/utility_speed_test.cc @@ -1,6 +1,7 @@ // Note: this should be run with --compilation_mode=opt, and would benefit from a // quiescent system with disabled cstate power management. +#include #include #include "common/common/assert.h" @@ -206,6 +207,28 @@ static void BM_FindTokenValueNoSplit(benchmark::State& state) { } BENCHMARK(BM_FindTokenValueNoSplit); +static void BM_RemoveTokensLong(benchmark::State& state) { + auto size = state.range(0); + std::string input(size, ','); + std::vector to_remove; + std::set to_remove_set; + for (decltype(size) i = 0; i < size; i++) { + to_remove.push_back(std::to_string(i)); + } + for (int i = 0; i < size; i++) { + if (i & 1) { + to_remove_set.insert(to_remove[i]); + } + input.append(","); + input.append(to_remove[i]); + } + for (auto _ : state) { + Envoy::StringUtil::removeTokens(input, ",", to_remove_set, ","); + state.SetBytesProcessed(static_cast(state.iterations()) * input.size()); + } +} +BENCHMARK(BM_RemoveTokensLong)->Range(8, 8 << 10); + static void BM_IntervalSetInsert17(benchmark::State& state) { for (auto _ : state) { Envoy::IntervalSetImpl interval_set; diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index dbda67bbd13a..2f5958a6f418 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -395,6 +395,31 @@ TEST(StringUtil, StringViewSplit) { } } +TEST(StringUtil, StringViewRemoveTokens) { + // Basic cases. + EXPECT_EQ(StringUtil::removeTokens("", ",", {"two"}, ","), ""); + EXPECT_EQ(StringUtil::removeTokens("one", ",", {"two"}, ","), "one"); + EXPECT_EQ(StringUtil::removeTokens("one,two ", ",", {"two"}, ","), "one"); + EXPECT_EQ(StringUtil::removeTokens("one,two,three ", ",", {"two"}, ","), "one,three"); + EXPECT_EQ(StringUtil::removeTokens(" one , two , three ", ",", {"two"}, ","), "one,three"); + EXPECT_EQ(StringUtil::removeTokens(" one , two , three ", ",", {"three"}, ","), "one,two"); + EXPECT_EQ(StringUtil::removeTokens(" one , two , three ", ",", {"three"}, ", "), "one, two"); + EXPECT_EQ(StringUtil::removeTokens("one,two,three", ",", {"two", "three"}, ","), "one"); + EXPECT_EQ(StringUtil::removeTokens("one,two,three,four", ",", {"two", "three"}, ","), "one,four"); + // Ignore case. + EXPECT_EQ(StringUtil::removeTokens("One,Two,Three,Four", ",", {"two", "three"}, ","), "One,Four"); + // Longer joiner. + EXPECT_EQ(StringUtil::removeTokens("one,two,three,four", ",", {"two", "three"}, " , "), + "one , four"); + // Delimiters. + EXPECT_EQ(StringUtil::removeTokens("one,two;three ", ",;", {"two"}, ","), "one,three"); + // No trimming + EXPECT_EQ(StringUtil::removeTokens("one, two, three ", ",", {" two"}, ",", false), "one, three "); + // No ignore case. + EXPECT_EQ(StringUtil::removeTokens("one, Two, three", ",", {"two"}, ",", true, false), + "one,Two,three"); +} + TEST(StringUtil, removeCharacters) { IntervalSetImpl removals; removals.insert(3, 5); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 82e5c188788b..7f367ea5b544 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -632,6 +632,43 @@ TEST_F(Http1ServerConnectionImplTest, RequestWithTrailers) { EXPECT_EQ(0U, buffer.length()); } +TEST_F(Http1ServerConnectionImplTest, IgnoreUpgradeH2c) { + initialize(); + + TestHeaderMapImpl expected_headers{ + {":authority", "www.somewhere.com"}, {":path", "/"}, {":method", "GET"}}; + Buffer::OwnedImpl buffer( + "GET http://www.somewhere.com/ HTTP/1.1\r\nConnection: " + "Upgrade, HTTP2-Settings\r\nUpgrade: h2c\r\nHTTP2-Settings: token64\r\nHost: bah\r\n\r\n"); + expectHeadersTest(Protocol::Http11, true, buffer, expected_headers); +} + +TEST_F(Http1ServerConnectionImplTest, IgnoreUpgradeH2cClose) { + initialize(); + + TestHeaderMapImpl expected_headers{{":authority", "www.somewhere.com"}, + {":path", "/"}, + {":method", "GET"}, + {"connection", "Close"}}; + Buffer::OwnedImpl buffer("GET http://www.somewhere.com/ HTTP/1.1\r\nConnection: " + "Upgrade, Close, HTTP2-Settings\r\nUpgrade: h2c\r\nHTTP2-Settings: " + "token64\r\nHost: bah\r\n\r\n"); + expectHeadersTest(Protocol::Http11, true, buffer, expected_headers); +} + +TEST_F(Http1ServerConnectionImplTest, IgnoreUpgradeH2cCloseEtc) { + initialize(); + + TestHeaderMapImpl expected_headers{{":authority", "www.somewhere.com"}, + {":path", "/"}, + {":method", "GET"}, + {"connection", "Close,Etc"}}; + Buffer::OwnedImpl buffer("GET http://www.somewhere.com/ HTTP/1.1\r\nConnection: " + "Upgrade, Close, HTTP2-Settings, Etc\r\nUpgrade: h2c\r\nHTTP2-Settings: " + "token64\r\nHost: bah\r\n\r\n"); + expectHeadersTest(Protocol::Http11, true, buffer, expected_headers); +} + TEST_F(Http1ServerConnectionImplTest, UpgradeRequest) { initialize();