http2: changing to using envoy header sanitization

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk · Jun 10, 2024 · 792568e · 792568e
1 parent b3b2c1a
commit 792568e
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 5 deletions.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -85,6 +85,9 @@ minor_behavior_changes:
 - area: http
   change: |
     Removed runtime guard ``envoy.reloadable_features.refresh_rtt_after_request`` and legacy code path.
+- area: http
+  change: |
+    Changing HTTP/2 semi-colon prefixed headers to being sanitized by Envoy code rather than nghttp2. Should be a functional no-op but guarded by ``envoy.reloadable_features.sanitize_http2_headers_without_nghttp2``
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*

diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc
@@ -201,11 +201,18 @@ bool HeaderUtility::headerValueIsValid(const absl::string_view header_value) {
                                                              http2::adapter::ObsTextOption::kAllow);
 }
 
-bool HeaderUtility::headerNameIsValid(const absl::string_view header_key) {
+bool HeaderUtility::headerNameIsValid(absl::string_view header_key) {
   if (!header_key.empty() && header_key[0] == ':') {
-    // For HTTP/2 pseudo header, use the HTTP/2 semantics for checking validity
-    return nghttp2_check_header_name(reinterpret_cast<const uint8_t*>(header_key.data()),
-                                     header_key.size()) != 0;
+    if (!Runtime::runtimeFeatureEnabled(
+            "envoy.reloadable_features.sanitize_http2_headers_without_nghttp2")) {
+      // For HTTP/2 pseudo header, use the HTTP/2 semantics for checking validity
+      return nghttp2_check_header_name(reinterpret_cast<const uint8_t*>(header_key.data()),
+                                       header_key.size()) != 0;
+    }
+    header_key.remove_prefix(1);
+    if (header_key.empty()) {
+      return false;
+    }
   }
   // For all other header use HTTP/1 semantics. The only difference from HTTP/2 is that
   // uppercase characters are allowed. This allows HTTP filters to add header with mixed

diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h
@@ -140,7 +140,7 @@ class HeaderUtility {
    * http://tools.ietf.org/html/rfc7230#section-3.2
    * @return bool true if the header name is valid, according to the aforementioned RFC.
    */
-  static bool headerNameIsValid(const absl::string_view header_key);
+  static bool headerNameIsValid(absl::string_view header_key);
 
   /**
    * Checks if header name contains underscore characters.

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -85,6 +85,7 @@ RUNTIME_GUARD(envoy_reloadable_features_quic_receive_ecn);
 // @danzh2010 or @RyanTheOptimist before removing.
 RUNTIME_GUARD(envoy_reloadable_features_quic_send_server_preferred_address_to_all_clients);
 RUNTIME_GUARD(envoy_reloadable_features_quic_upstream_reads_fixed_number_packets);
+RUNTIME_GUARD(envoy_reloadable_features_sanitize_http2_headers_without_nghttp2);
 RUNTIME_GUARD(envoy_reloadable_features_sanitize_te);
 RUNTIME_GUARD(envoy_reloadable_features_send_header_raw_value);
 RUNTIME_GUARD(envoy_reloadable_features_send_local_reply_when_no_buffer_and_upstream_request);

diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc
@@ -1265,6 +1265,28 @@ TEST(HeaderAddTest, HeaderAdd) {
   });
 }
 
+void headerNameIsValid() {
+  // Isn't valid but passes legacy checks.
+  EXPECT_FALSE(HeaderUtility::headerNameIsValid(":"));
+  EXPECT_FALSE(HeaderUtility::headerNameIsValid("::"));
+  EXPECT_FALSE(HeaderUtility::headerNameIsValid("\n"));
+  EXPECT_FALSE(HeaderUtility::headerNameIsValid(":\n"));
+  EXPECT_FALSE(HeaderUtility::headerNameIsValid("asd\n"));
+
+  EXPECT_TRUE(HeaderUtility::headerNameIsValid("asd"));
+  // Not actually valid but passes legacy Envoy checks.
+  EXPECT_TRUE(HeaderUtility::headerNameIsValid(":asd"));
+}
+
+TEST(HeaderIsValidTest, HeaderNameIsValid) { headerNameIsValid(); }
+
+TEST(HeaderIsValidTest, HeaderNameIsValidLegacy) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues(
+      {{"envoy.reloadable_features.sanitize_http2_headers_without_nghttp2", "false"}});
+  headerNameIsValid();
+}
+
 TEST(HeaderIsValidTest, HeaderNameContainsUnderscore) {
   EXPECT_FALSE(HeaderUtility::headerNameContainsUnderscore("cookie"));
   EXPECT_FALSE(HeaderUtility::headerNameContainsUnderscore("x-something"));