Do not 503 on Upgrade: h2c instead remove the header and ignore. (#7981)

Description: When a request comes in on http1 with "upgrade: h2c", the current behavior is to 503.  Instead we should ignore the upgrade and remove the header and continue with the request as http1.
Risk Level: Medium
Testing: Unit test. Hand test with ithub.com/rdsubhas/java-istio client server locally.
Docs Changes: N/A
Release Notes:  http1: ignore and remove Upgrade: h2c.
Fixes istio/istio#16391

Signed-off-by: John Plevyak <[email protected]>

docs/root/intro/version_history.rst

@@ -30,6 +30,7 @@ Version history
 * http: changed Envoy to forward existing x-forwarded-proto from upstream trusted proxies. Guarded by `envoy.reloadable_features.trusted_forwarded_proto` which defaults true.
 * http: added the ability to configure the behavior of the server response header, via the :ref:`server_header_transformation<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.server_header_transformation>` field.
 * http: added the ability to :ref:`merge adjacent slashes<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.merge_slashes>` in the path.
+* http: remove h2c upgrade headers for HTTP/1 as h2c upgrades are currently not supported.
 * listeners: added :ref:`continue_on_listener_filters_timeout <envoy_api_field_Listener.continue_on_listener_filters_timeout>` to configure whether a listener will still create a connection when listener filters time out.
 * listeners: added :ref:`HTTP inspector listener filter <config_listener_filters_http_inspector>`.
 * lua: extended `httpCall()` and `respond()` APIs to accept headers with entry values that can be a string or table of strings.

source/common/common/utility.cc

@@ -324,6 +324,16 @@ 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,
+                                     const CaseUnorderedSet& tokens_to_remove,
+                                     absl::string_view joiner) {
+  auto values = Envoy::StringUtil::splitToken(source, delimiters);
+  std::for_each(values.begin(), values.end(), [](auto& v) { v = StringUtil::trim(v); });
+  auto end = std::remove_if(values.begin(), values.end(),
+                            [&](absl::string_view t) { return tokens_to_remove.count(t) != 0; });
+  return absl::StrJoin(values.begin(), end, joiner);
+}
+
 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

@@ -144,6 +144,35 @@ class DateUtil {
  */
 class StringUtil {
 public:
+  /**
+   * Callable struct that returns the result of string comparison ignoring case.
+   * @param lhs supplies the first string view.
+   * @param rhs supplies the second string view.
+   * @return true if strings are semantically the same and false otherwise.
+   */
+  struct CaseInsensitiveCompare {
+    // Enable heterogeneous lookup (https://abseil.io/tips/144)
+    using is_transparent = void;
+    bool operator()(absl::string_view lhs, absl::string_view rhs) const;
+  };
+
+  /**
+   * Callable struct that returns the hash representation of a case-insensitive string_view input.
+   * @param key supplies the string view.
+   * @return uint64_t hash representation of the supplied string view.
+   */
+  struct CaseInsensitiveHash {
+    // Enable heterogeneous lookup (https://abseil.io/tips/144)
+    using is_transparent = void;
+    uint64_t operator()(absl::string_view key) const;
+  };
+
+  /**
+   * Definition of unordered set of case-insensitive std::string.
+   */
+  using CaseUnorderedSet =
+      absl::flat_hash_set<std::string, CaseInsensitiveHash, CaseInsensitiveCompare>;
+
   static const char WhitespaceChars[];
 
   /**
@@ -281,6 +310,20 @@ class StringUtil {
                                                    absl::string_view delimiters,
                                                    bool keep_empty_string = false);
 
+  /**
+   * Remove tokens from a delimiter-separated string view. The tokens are trimmed before
+   * they are compared ignoring case with the elements of 'tokens_to_remove'. The output is
+   * built from the trimmed tokens preserving case.
+   * @param source supplies the delimiter-separated string view.
+   * @param multi-delimiters 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.
+   * @return string of the remaining joined tokens.
+   */
+  static std::string removeTokens(absl::string_view source, absl::string_view delimiters,
+                                  const CaseUnorderedSet& tokens_to_remove,
+                                  absl::string_view joiner);
+
   /**
    * Size-bounded string copying and concatenation
    */
@@ -332,35 +375,6 @@ class StringUtil {
    */
   static std::string toLower(absl::string_view s);
 
-  /**
-   * Callable struct that returns the result of string comparison ignoring case.
-   * @param lhs supplies the first string view.
-   * @param rhs supplies the second string view.
-   * @return true if strings are semantically the same and false otherwise.
-   */
-  struct CaseInsensitiveCompare {
-    // Enable heterogeneous lookup (https://abseil.io/tips/144)
-    using is_transparent = void;
-    bool operator()(absl::string_view lhs, absl::string_view rhs) const;
-  };
-
-  /**
-   * Callable struct that returns the hash representation of a case-insensitive string_view input.
-   * @param key supplies the string view.
-   * @return uint64_t hash representation of the supplied string view.
-   */
-  struct CaseInsensitiveHash {
-    // Enable heterogeneous lookup (https://abseil.io/tips/144)
-    using is_transparent = void;
-    uint64_t operator()(absl::string_view key) const;
-  };
-
-  /**
-   * Definition of unordered set of case-insensitive std::string.
-   */
-  using CaseUnorderedSet =
-      absl::flat_hash_set<std::string, CaseInsensitiveHash, CaseInsensitiveCompare>;
-
   /**
    * Removes all the character indices from str contained in the interval-set.
    * @param str the string containing the characters to be removed.

source/common/http/headers.h

@@ -125,6 +125,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"};
@@ -153,11 +154,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

@@ -21,6 +21,15 @@
 namespace Envoy {
 namespace Http {
 namespace Http1 {
+namespace {
+
+const StringUtil::CaseUnorderedSet& caseUnorderdSetContainingUpgradeAndHttp2Settings() {
+  CONSTRUCT_ON_FIRST_USE(StringUtil::CaseUnorderedSet,
+                         Http::Headers::get().ConnectionValues.Upgrade,
+                         Http::Headers::get().ConnectionValues.Http2Settings);
+}
+
+} // namespace
 
 const std::string StreamEncoderImpl::CRLF = "\r\n";
 const std::string StreamEncoderImpl::LAST_CHUNK = "0\r\n\r\n";
@@ -469,8 +478,28 @@ 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()) {
+        const auto& tokens_to_remove = caseUnorderdSetContainingUpgradeAndHttp2Settings();
+        std::string new_value = StringUtil::removeTokens(
+            current_header_map_->Connection()->value().getStringView(), ",", tokens_to_remove, ",");
+        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

@@ -206,6 +206,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;
+  StringUtil::CaseUnorderedSet 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,28 @@ 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 ", ",", {"two", "one"}, ","), "");
+  EXPECT_EQ(StringUtil::removeTokens("one,two ", ",", {"one"}, ","), "two");
+  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");
+}
+
 TEST(StringUtil, removeCharacters) {
   IntervalSetImpl<size_t> removals;
   removals.insert(3, 5);

test/common/http/http1/codec_impl_test.cc

@@ -720,6 +720,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();