Merge pull request envoyproxy#98 from jplevyak/release-1.2-java-h2c-fix

Do not 503 on Upgrade: h2c instead remove the header and ignore.

source/common/common/utility.cc

@@ -323,6 +323,33 @@ std::vector<absl::string_view> 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<absl::string_view> 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) {

source/common/common/utility.h

@@ -282,6 +282,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<absl::string_view> tokens_to_remove,
+                                  absl::string_view joiner, bool trim_tokens = true,
+                                  bool ignore_case = true);
+
   /**
    * Size-bounded string copying and concatenation
    */

source/common/http/headers.h

@@ -77,6 +77,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"};
@@ -105,11 +106,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;
 

source/common/http/http1/codec_impl.cc

@@ -4,6 +4,8 @@
 #include <memory>
 #include <string>
 
+#include "absl/strings/match.h"
+
 #include "envoy/buffer/buffer.h"
 #include "envoy/http/header_map.h"
 #include "envoy/network/connection.h"
@@ -450,8 +452,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_));

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 <iostream>
 #include <random>
 
 #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<std::string> to_remove;
+  std::set<absl::string_view> 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<int64_t>(state.iterations()) * input.size());
+  }
+}
+BENCHMARK(BM_RemoveTokensLong)->Range(8, 8 << 10);
+
 static void BM_IntervalSetInsert17(benchmark::State& state) {
   for (auto _ : state) {
     Envoy::IntervalSetImpl<size_t> interval_set;

test/common/common/utility_test.cc

@@ -368,6 +368,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<size_t> removals;
   removals.insert(3, 5);

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();