quiche: handle multiple cookies and multiple set-cookie headers (#14969)

Commit Message: fix some issues around duplicated http headers: Quiche doesn't coalesce duplicated headers, so we need to coalesce them in Envoy. Cookie headers after coalescing are semi-colon separated, while Set-Cookie headers shouldn't be coalesced. Risk Level: low Testing: added unit tests and integration tests. Part of #2557 Co-authored-by: Dan Zhang <[email protected]>
envoyproxy · Mar 2, 2021 · 3cc0ca0 · 3cc0ca0
1 parent 56eb1e2
commit 3cc0ca0
Show file tree

Hide file tree

Showing 9 changed files with 83 additions and 6 deletions.
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -68,6 +68,7 @@ Bug Fixes
 * filter_chain: fix filter chain matching with the server name as the case-insensitive way.
 * grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling` to false.
 * grpc_http_bridge: the downstream HTTP status is now correctly set for trailers-only responses from the upstream.
+* header map: pick the right delimiter to append multiple header values to the same key. Previouly header with multiple values are coalesced with ",", after this fix cookie headers should be coalesced with " ;". This doesn't affect Http1 or Http2 requests because these 2 codecs coalesce cookie headers before adding it to header map. To revert to the old behavior, set the runtime feature `envoy.reloadable_features.header_map_correctly_coalesce_cookies` to false.
 * http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false.
 * http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false.
 * jwt_authn: reject requests with a proper error if JWT has the wrong issuer when allow_missing is used. Before this change, the requests are accepted.

diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc
@@ -428,7 +428,10 @@ void HeaderMapImpl::appendCopy(const LowerCaseString& key, absl::string_view val
   // TODO(#9221): converge on and document a policy for coalescing multiple headers.
   auto entry = getExisting(key);
   if (!entry.empty()) {
-    const uint64_t added_size = appendToHeader(entry[0]->value(), value);
+    const std::string delimiter = (key == Http::Headers::get().Cookie ? "; " : ",");
+    const uint64_t added_size = header_map_correctly_coalesce_cookies_
+                                    ? appendToHeader(entry[0]->value(), value, delimiter)
+                                    : appendToHeader(entry[0]->value(), value);
     addSize(added_size);
   } else {
     addCopy(key, value);

diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h
@@ -329,6 +329,8 @@ class HeaderMapImpl : NonCopyable {
   HeaderList headers_;
   // This holds the internal byte size of the HeaderMap.
   uint64_t cached_byte_size_ = 0;
+  const bool header_map_correctly_coalesce_cookies_ = Runtime::runtimeFeatureEnabled(
+      "envoy.reloadable_features.header_map_correctly_coalesce_cookies");
 };
 
 /**

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -91,6 +91,7 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.unify_grpc_handling",
     "envoy.reloadable_features.upstream_http2_flood_checks",
     "envoy.restart_features.use_apple_api_for_dns_lookups",
+    "envoy.reloadable_features.header_map_correctly_coalesce_cookies",
 };
 
 // This is a section for officially sanctioned runtime features which are too

diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_utils.cc b/source/extensions/quic_listeners/quiche/envoy_quic_utils.cc
@@ -54,7 +54,8 @@ spdy::SpdyHeaderBlock envoyHeadersToSpdyHeaderBlock(const Http::HeaderMap& heade
   spdy::SpdyHeaderBlock header_block;
   headers.iterate([&header_block](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate {
     // The key-value pairs are copied.
-    header_block.insert({header.key().getStringView(), header.value().getStringView()});
+    header_block.AppendValueOrAddHeader(header.key().getStringView(),
+                                        header.value().getStringView());
     return Http::HeaderMap::Iterate::Continue;
   });
   return header_block;

diff --git a/source/extensions/quic_listeners/quiche/envoy_quic_utils.h b/source/extensions/quic_listeners/quiche/envoy_quic_utils.h
@@ -42,8 +42,15 @@ template <class T>
 std::unique_ptr<T> quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list) {
   auto headers = T::create();
   for (const auto& entry : header_list) {
-    // TODO(danzh): Avoid copy by referencing entry as header_list is already validated by QUIC.
-    headers->addCopy(Http::LowerCaseString(entry.first), entry.second);
+    auto key = Http::LowerCaseString(entry.first);
+    if (key != Http::Headers::get().Cookie) {
+      // TODO(danzh): Avoid copy by referencing entry as header_list is already validated by QUIC.
+      headers->addCopy(key, entry.second);
+    } else {
+      // QUICHE breaks "cookie" header into crumbs. Coalesce them by appending current one to
+      // existing one if there is any.
+      headers->appendCopy(key, entry.second);
+    }
   }
   return headers;
 }
@@ -54,8 +61,7 @@ std::unique_ptr<T> spdyHeaderBlockToEnvoyHeaders(const spdy::SpdyHeaderBlock& he
   for (auto entry : header_block) {
     // TODO(danzh): Avoid temporary strings and addCopy() with string_view.
     std::string key(entry.first);
-    std::string value(entry.second);
-    headers->addCopy(Http::LowerCaseString(key), value);
+    headers->addCopy(Http::LowerCaseString(key), entry.second);
   }
   return headers;
 }

diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc
@@ -741,6 +741,24 @@ TEST_P(HeaderMapImplTest, DoubleCookieAdd) {
   ASSERT_EQ(set_cookie_value[1]->value().getStringView(), "bar");
 }
 
+TEST_P(HeaderMapImplTest, AppendCookieHeadersWithSemicolon) {
+  if (!Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.header_map_correctly_coalesce_cookies")) {
+    return;
+  }
+  TestRequestHeaderMapImpl headers;
+  const std::string foo("foo=1");
+  const std::string bar("bar=2");
+  const LowerCaseString& cookie = Http::Headers::get().Cookie;
+  headers.addReference(cookie, foo);
+  headers.appendCopy(cookie, bar);
+  EXPECT_EQ(1UL, headers.size());
+
+  const auto cookie_value = headers.get(LowerCaseString("cookie"));
+  ASSERT_EQ(cookie_value.size(), 1);
+  ASSERT_EQ(cookie_value[0]->value().getStringView(), "foo=1; bar=2");
+}
+
 TEST_P(HeaderMapImplTest, DoubleInlineSet) {
   TestRequestHeaderMapImpl headers;
   headers.setReferenceKey(Headers::get().ContentType, "blah");

diff --git a/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc b/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc
@@ -183,6 +183,9 @@ TEST_P(EnvoyQuicServerStreamTest, GetRequestAndResponse) {
   request_headers.OnHeader(":authority", host_);
   request_headers.OnHeader(":method", "GET");
   request_headers.OnHeader(":path", "/");
+  // QUICHE stack doesn't coalesce Cookie headers for header compression optimization.
+  request_headers.OnHeader("cookie", "a=b");
+  request_headers.OnHeader("cookie", "c=d");
   request_headers.OnHeaderBlockEnd(/*uncompressed_header_bytes=*/0,
                                    /*compressed_header_bytes=*/0);
 
@@ -192,13 +195,22 @@ TEST_P(EnvoyQuicServerStreamTest, GetRequestAndResponse) {
         EXPECT_EQ(host_, headers->getHostValue());
         EXPECT_EQ("/", headers->getPathValue());
         EXPECT_EQ(Http::Headers::get().MethodValues.Get, headers->getMethodValue());
+        if (Runtime::runtimeFeatureEnabled(
+                "envoy.reloadable_features.header_map_correctly_coalesce_cookies")) {
+          // Verify that the duplicated headers are handled correctly before passing to stream
+          // decoder.
+          EXPECT_EQ("a=b; c=d",
+                    headers->get(Http::Headers::get().Cookie)[0]->value().getStringView());
+        }
       }));
   if (quic::VersionUsesHttp3(quic_version_.transport_version)) {
     EXPECT_CALL(stream_decoder_, decodeData(BufferStringEqual(""), /*end_stream=*/true));
     spdy::SpdyHeaderBlock spdy_headers;
     spdy_headers[":authority"] = host_;
     spdy_headers[":method"] = "GET";
     spdy_headers[":path"] = "/";
+    spdy_headers.AppendValueOrAddHeader("cookie", "a=b");
+    spdy_headers.AppendValueOrAddHeader("cookie", "c=d");
     std::string payload = spdyHeaderToHttp3StreamPayload(spdy_headers);
     quic::QuicStreamFrame frame(stream_id_, true, 0, payload);
     quic_stream_->OnStreamFrame(frame);

diff --git a/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc b/test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc
@@ -571,5 +571,38 @@ TEST_P(QuicHttpIntegrationTest, Reset101SwitchProtocolResponse) {
   EXPECT_FALSE(response->complete());
 }
 
+TEST_P(QuicHttpIntegrationTest, MultipleSetCookieAndCookieHeaders) {
+  initialize();
+
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto encoder_decoder =
+      codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "GET"},
+                                                                 {":path", "/dynamo/url"},
+                                                                 {":scheme", "http"},
+                                                                 {":authority", "host"},
+                                                                 {"cookie", "a=b"},
+                                                                 {"cookie", "c=d"}});
+  request_encoder_ = &encoder_decoder.first;
+  auto response = std::move(encoder_decoder.second);
+  codec_client_->sendData(*request_encoder_, 0, true);
+  waitForNextUpstreamRequest();
+  if (Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.header_map_correctly_coalesce_cookies")) {
+    EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Cookie)[0]->value(),
+              "a=b; c=d");
+  }
+
+  upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"},
+                                                                   {"set-cookie", "foo"},
+                                                                   {"set-cookie", "bar"}},
+                                   true);
+  response->waitForEndStream();
+  EXPECT_TRUE(response->complete());
+  const auto out = response->headers().get(Http::LowerCaseString("set-cookie"));
+  ASSERT_EQ(out.size(), 2);
+  ASSERT_EQ(out[0]->value().getStringView(), "foo");
+  ASSERT_EQ(out[1]->value().getStringView(), "bar");
+}
+
 } // namespace Quic
 } // namespace Envoy