envoyproxy · mattklein123 · May 24, 2022 · Apr 19, 2022 · Apr 20, 2022 · Apr 20, 2022
diff --git a/api/envoy/extensions/filters/network/thrift_proxy/v3/thrift_proxy.proto b/api/envoy/extensions/filters/network/thrift_proxy/v3/thrift_proxy.proto
@@ -73,7 +73,7 @@ message Trds {
   string route_config_name = 2;
 }
 
-// [#next-free-field: 9]
+// [#next-free-field: 10]
 message ThriftProxy {
   option (udpa.annotations.versioning).previous_message_type =
       "envoy.config.filter.network.thrift_proxy.v2alpha1.ThriftProxy";
@@ -112,6 +112,10 @@ message ThriftProxy {
 
   // Optional maximum requests for a single downstream connection. If not specified, there is no limit.
   google.protobuf.UInt32Value max_requests_per_connection = 7;
+
+  // If set to true, Envoy will treat header keys in Thrift as case-sensitive instead of converting
+  // them to lower case as per the default behavior.
+  bool header_keys_case_sensitive = 9;
 }
 
 // ThriftFilter configures a Thrift filter.

diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -42,6 +42,7 @@ New Features
 * access_log: added new access_log command operators to retrieve upstream connection information: ``%UPSTREAM_PROTOCOL%``, ``%UPSTREAM_PEER_SUBJECT%``, ``%UPSTREAM_PEER_ISSUER%``, ``%UPSTREAM_TLS_SESSION_ID%``, ``%UPSTREAM_TLS_CIPHER%``, ``%UPSTREAM_TLS_VERSION%``, ``%UPSTREAM_PEER_CERT_V_START%``, ``%UPSTREAM_PEER_CERT_V_END%`` and ``%UPSTREAM_PEER_CERT%``.
 * dns_resolver: added :ref:`include_unroutable_families<envoy_v3_api_field_extensions.network.dns_resolver.apple.v3.AppleDnsResolverConfig.include_unroutable_families>` to the Apple DNS resolver.
 * ext_proc: added support for per-route :ref:`grpc_service <envoy_v3_api_field_extensions.filters.http.ext_proc.v3.ExtProcOverrides.grpc_service>`.
+* thrift: add support for case-sensitive header keys.
 * thrift: added flag to router to control downstream local close. :ref:`close_downstream_on_upstream_error <envoy_v3_api_field_extensions.filters.network.thrift_proxy.router.v3.Router.close_downstream_on_upstream_error>`.
 
 Deprecated

@@ -136,7 +136,8 @@ ConfigImpl::ConfigImpl(
       stats_(ThriftFilterStats::generateStats(stats_prefix_, context_.scope())),
       transport_(lookupTransport(config.transport())), proto_(lookupProtocol(config.protocol())),
       payload_passthrough_(config.payload_passthrough()),
-      max_requests_per_connection_(config.max_requests_per_connection().value()) {
+      max_requests_per_connection_(config.max_requests_per_connection().value()),
+      header_keys_case_sensitive_(config.header_keys_case_sensitive()) {
 
   if (config.thrift_filters().empty()) {
     ENVOY_LOG(debug, "using default router filter");

@@ -87,6 +87,7 @@ class ConfigImpl : public Config,
   Router::Config& routerConfig() override { return *this; }
   bool payloadPassthrough() const override { return payload_passthrough_; }
   uint64_t maxRequestsPerConnection() const override { return max_requests_per_connection_; }
+  bool headerKeysCaseSensitive() const override { return header_keys_case_sensitive_; }
 
 private:
   void processFilter(
@@ -103,6 +104,7 @@ class ConfigImpl : public Config,
   const bool payload_passthrough_;
 
   const uint64_t max_requests_per_connection_{};
+  const bool header_keys_case_sensitive_;
 };
 
 } // namespace ThriftProxy

@@ -208,6 +208,10 @@ bool ConnectionManager::passthroughEnabled() const {
   return (*rpcs_.begin())->passthroughSupported();
 }
 
+bool ConnectionManager::headerKeysCaseSensitive() const {
+  return config_.headerKeysCaseSensitive();
+}
+
 bool ConnectionManager::ResponseDecoder::onData(Buffer::Instance& data) {
   upstream_buffer_.move(data);
 
@@ -316,6 +320,10 @@ bool ConnectionManager::ResponseDecoder::passthroughEnabled() const {
   return parent_.parent_.passthroughEnabled();
 }
 
+bool ConnectionManager::ResponseDecoder::headerKeysCaseSensitive() const {
+  return parent_.parent_.headerKeysCaseSensitive();
+}
+
 void ConnectionManager::ActiveRpcDecoderFilter::continueDecoding() {
   const FilterStatus status = parent_.applyDecoderFilters(this);
   if (status == FilterStatus::Continue) {

@@ -41,6 +41,7 @@ class Config {
   virtual Router::Config& routerConfig() PURE;
   virtual bool payloadPassthrough() const PURE;
   virtual uint64_t maxRequestsPerConnection() const PURE;
+  virtual bool headerKeysCaseSensitive() const PURE;
 };
 
 /**
@@ -68,6 +69,7 @@ class ConnectionManager : public Network::ReadFilter,
   // DecoderCallbacks
   DecoderEventHandler& newDecoderEventHandler() override;
   bool passthroughEnabled() const override;
+  bool headerKeysCaseSensitive() const override;
 
 private:
   struct ActiveRpc;
@@ -93,6 +95,7 @@ class ConnectionManager : public Network::ReadFilter,
     // DecoderCallbacks
     DecoderEventHandler& newDecoderEventHandler() override { return *this; }
     bool passthroughEnabled() const override;
+    bool headerKeysCaseSensitive() const override;
 
     ActiveRpc& parent_;
     DecoderPtr decoder_;

@@ -436,7 +436,7 @@ FilterStatus Decoder::onData(Buffer::Instance& data, bool& buffer_underflow) {
   if (!frame_started_) {
     // Look for start of next frame.
     if (!metadata_) {
-      metadata_ = std::make_shared<MessageMetadata>();
+      metadata_ = std::make_shared<MessageMetadata>(callbacks_.headerKeysCaseSensitive());
     }
 
     if (!transport_.decodeFrameStart(data, *metadata_)) {

@@ -196,6 +196,11 @@ class DecoderCallbacks {
    * @return True if payload passthrough is enabled and is supported by filter chain.
    */
   virtual bool passthroughEnabled() const PURE;
+
+  /**
+   * @return True if payload header keys should be treated as case-sensitive.
+   */
+  virtual bool headerKeysCaseSensitive() const PURE;
 };
 
 /**

@@ -3,6 +3,7 @@
 #include <limits>
 
 #include "envoy/common/exception.h"
+#include "envoy/http/header_formatter.h"
 
 #include "source/common/buffer/buffer_impl.h"
 #include "source/extensions/filters/network/thrift_proxy/buffer_helper.h"
@@ -146,11 +147,18 @@ bool HeaderTransportImpl::decodeFrameStart(Buffer::Instance& buffer, MessageMeta
       throw EnvoyException(absl::StrCat("invalid header transport header count ", num_headers));
     }
 
+    Envoy::Http::StatefulHeaderKeyFormatterOptRef formatter = metadata.headers().formatter();
     while (num_headers-- > 0) {
       std::string key_string = drainVarString(buffer, header_size, "header key");
       // LowerCaseString doesn't allow '\0', '\n', and '\r'.
       key_string =
           absl::StrReplaceAll(key_string, {{std::string(1, '\0'), ""}, {"\n", ""}, {"\r", ""}});
+
+      if (formatter) {
+        // TODO(rgs1): do we also want to preserve the removed characters from above?
+        formatter->processKey(key_string);
+      }
+
       const Http::LowerCaseString key = Http::LowerCaseString(key_string);
       const std::string value = drainVarString(buffer, header_size, "header value");
       metadata.headers().addCopy(key, value);
@@ -212,11 +220,15 @@ void HeaderTransportImpl::encodeFrame(Buffer::Instance& buffer, const MessageMet
     // Num headers
     BufferHelper::writeVarIntI32(header_buffer, static_cast<int32_t>(headers.size()));
 
-    headers.iterate([&header_buffer](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate {
-      writeVarString(header_buffer, header.key().getStringView());
-      writeVarString(header_buffer, header.value().getStringView());
-      return Http::HeaderMap::Iterate::Continue;
-    });
+    const auto formatter = headers.formatter();
+
+    headers.iterate(
+        [&header_buffer, formatter](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate {
+          const auto header_key = header.key().getStringView();
+          writeVarString(header_buffer, formatter ? formatter->format(header_key) : header_key);
+          writeVarString(header_buffer, header.value().getStringView());
+          return Http::HeaderMap::Iterate::Continue;
+        });
   }
 
   uint64_t header_size = header_buffer.length();

@@ -7,6 +7,7 @@
 #include <string>
 
 #include "envoy/buffer/buffer.h"
+#include "envoy/http/header_formatter.h"
 
 #include "source/common/common/macros.h"
 #include "source/common/http/header_map_impl.h"
@@ -19,6 +20,29 @@ namespace Envoy {
 namespace Extensions {
 namespace NetworkFilters {
 namespace ThriftProxy {
+namespace {
+
+// This is copied from the PreserveCaseHeaderFormatter extension, because it'd be weird for
+// ThriftProxy to depend on an optional HTTP extension and it's not that much code anyway.
+class PreserveCaseHeaderFormatter : public Envoy::Http::StatefulHeaderKeyFormatter {
+public:
+  PreserveCaseHeaderFormatter() = default;
+
+  // Envoy::Http::StatefulHeaderKeyFormatter
+  std::string format(absl::string_view key) const override {
+    const auto remembered_key_itr = original_header_keys_.find(key);
+    return remembered_key_itr != original_header_keys_.end() ? *remembered_key_itr
+                                                             : std::string(key);
+  }
+  void processKey(absl::string_view key) override { original_header_keys_.emplace(key); }
+  void setReasonPhrase(absl::string_view) override {}
+  absl::string_view getReasonPhrase() const override { return ""; }
+
+private:
+  StringUtil::CaseUnorderedSet original_header_keys_;
+};
+
+} // namespace
 
 /**
  * MessageMetadata encapsulates metadata about Thrift messages. The various fields are considered
@@ -28,7 +52,13 @@ namespace ThriftProxy {
  */
 class MessageMetadata {
 public:
-  MessageMetadata() = default;
+  MessageMetadata(bool case_sensitive = false) {
+    auto headers = Http::RequestHeaderMapImpl::create();
+    if (case_sensitive) {
+      headers->setFormatter(std::make_unique<PreserveCaseHeaderFormatter>());
+    }
+    headers_ = std::move(headers);
+  }
 
   std::shared_ptr<MessageMetadata> clone() const {
     auto copy = std::make_shared<MessageMetadata>();
@@ -189,7 +219,7 @@ class MessageMetadata {
   absl::optional<int32_t> seq_id_{};
   absl::optional<MessageType> msg_type_{};
   absl::optional<ReplyType> reply_type_{};
-  Http::HeaderMapPtr headers_{Http::RequestHeaderMapImpl::create()};
+  Http::HeaderMapPtr headers_;
   absl::optional<AppExceptionType> app_ex_type_;
   absl::optional<std::string> app_ex_msg_;
   bool protocol_upgrade_message_{false};

@@ -75,6 +75,7 @@ struct NullResponseDecoder : public DecoderCallbacks, public ProtocolConverter {
   // DecoderCallbacks
   DecoderEventHandler& newDecoderEventHandler() override { return *this; }
   bool passthroughEnabled() const override { return true; }
+  bool headerKeysCaseSensitive() const override { return false; }
 
   DecoderPtr decoder_;
   Buffer::OwnedImpl response_buffer_;

@@ -248,6 +248,7 @@ class ThriftObjectImpl : public ThriftObject,
     return FilterStatus::Continue;
   }
   bool passthroughEnabled() const override { return false; }
+  bool headerKeysCaseSensitive() const override { return false; }
 
   // ThriftObject
   bool onData(Buffer::Instance& buffer) override;

@@ -436,22 +436,25 @@ TEST(HeaderTransportTest, InvalidInfoBlock) {
   }
 }
 
-TEST(HeaderTransportTest, InfoBlock) {
+MessageMetadata testInfoBlock(bool case_sensitive = false) {
   HeaderTransportImpl transport;
   Buffer::OwnedImpl buffer;
-  MessageMetadata metadata;
+  MessageMetadata metadata(case_sensitive);
+
   metadata.headers().addCopy(Http::LowerCaseString("not"), "empty");
 
+  const char* key = case_sensitive ? "Key" : "key";
+  const char* value = case_sensitive ? "Value" : "value";
   buffer.writeBEInt<int32_t>(200);
   buffer.writeBEInt<int16_t>(0x0FFF);
   buffer.writeBEInt<int16_t>(0);
   buffer.writeBEInt<int32_t>(1);  // sequence number
   buffer.writeBEInt<int16_t>(38); // size 152
   addSeq(buffer, {0, 0, 1, 3}); // 0 = binary proto, 0 = num transforms, 1 = key value, 3 = num kvs
-  buffer.writeByte(3);
-  buffer.add("key");
-  buffer.writeByte(5);
-  buffer.add("value");
+  buffer.writeByte(std::strlen(key));
+  buffer.add(key);
+  buffer.writeByte(std::strlen(value));
+  buffer.add(value);
   buffer.writeByte(4);
   buffer.add("key2");
   addSeq(buffer, {0x80, 0x01}); // var int 128
@@ -462,7 +465,7 @@ TEST(HeaderTransportTest, InfoBlock) {
 
   Http::TestRequestHeaderMapImpl expected_headers;
   expected_headers.addCopy(Http::LowerCaseString("not"), "empty");
-  expected_headers.addCopy(Http::LowerCaseString("key"), "value");
+  expected_headers.addCopy(Http::LowerCaseString("key"), value);
   expected_headers.addCopy(Http::LowerCaseString("key2"), std::string(128, 'x'));
   expected_headers.addCopy(Http::LowerCaseString(""), "");
 
@@ -471,6 +474,26 @@ TEST(HeaderTransportTest, InfoBlock) {
 
   EXPECT_EQ(expected_headers, metadata.headers());
   EXPECT_EQ(buffer.length(), 0);
+
+  return metadata;
+}
+
+TEST(HeaderTransportTest, InfoBlock) { testInfoBlock(); }
+
+TEST(HeaderTransportTest, InfoBlockCaseSensitive) {
+  auto metadata = testInfoBlock(true /* case-sensitive */);
+  HeaderTransportImpl transport;
+  Buffer::OwnedImpl buffer;
+  Buffer::OwnedImpl msg;
+  msg.add("fake message");
+  transport.encodeFrame(buffer, metadata, msg);
+  EXPECT_EQ(0, msg.length());
+  EXPECT_EQ(std::string("\0\0\0\xBA\xF\xFF\0\0\0\0\0\x1\0)\0\0\x1\x4\x3not\x5"
+                        "empty\x3Key\x5Value\x4key2\x80\x1xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+                        "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+                        "xxxxxxxxxxxxx\0\0\0\0\0fake message",
+                        190),
+            buffer.toString());
 }
 
 TEST(HeaderTransportTest, DecodeFrameEnd) {

@@ -136,6 +136,7 @@ class MockDecoderCallbacks : public DecoderCallbacks {
   // ThriftProxy::DecoderCallbacks
   MOCK_METHOD(DecoderEventHandler&, newDecoderEventHandler, ());
   MOCK_METHOD(bool, passthroughEnabled, (), (const));
+  MOCK_METHOD(bool, headerKeysCaseSensitive, (), (const));
 };
 
 class MockDecoderEventHandler : public DecoderEventHandler {