http: making the behavior of the response Server header configurable (#…

…8014) Default behavior remains unchanged, but now Envoy can override, override iff there's no server header from upstream, or always leave the server header (or lack thereof) unmodified. Risk Level: low (config guarded change) Testing: new unit tests Docs Changes: n/a Release Notes: inline Fixes #6716 Signed-off-by: Alyssa Wilk <[email protected]>
envoyproxy · Aug 28, 2019 · b8966cb · b8966cb
1 parent f80188e
commit b8966cb
Show file tree

Hide file tree

Showing 13 changed files with 207 additions and 2 deletions.
diff --git a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto
@@ -25,7 +25,7 @@ import "gogoproto/gogo.proto";
 // [#protodoc-title: HTTP connection manager]
 // HTTP connection manager :ref:`configuration overview <config_http_conn_man>`.
 
-// [#comment:next free field: 34]
+// [#comment:next free field: 35]
 message HttpConnectionManager {
   enum CodecType {
     option (gogoproto.goproto_enum_prefix) = false;
@@ -144,6 +144,24 @@ message HttpConnectionManager {
   // header in responses. If not set, the default is *envoy*.
   string server_name = 10;
 
+  enum ServerHeaderTransformation {
+    option (gogoproto.goproto_enum_prefix) = false;
+
+    // Overwrite any Server header with the contents of server_name.
+    OVERWRITE = 0;
+    // If no Server header is present, append Server server_name
+    // If a Server header is present, pass it through.
+    APPEND_IF_ABSENT = 1;
+    // Pass through the value of the server header, and do not append a header
+    // if none is present.
+    PASS_THROUGH = 2;
+  }
+  // Defines the action to be applied to the Server header on the response path.
+  // By default, Envoy will overwrite the header with the value specified in
+  // server_name.
+  ServerHeaderTransformation server_header_transformation = 34
+      [(validate.rules).enum.defined_only = true];
+
   // The maximum request headers size for incoming connections.
   // If unconfigured, the default max request headers allowed is 60 KiB.
   // Requests that exceed this limit will receive a 431 response.

diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst
@@ -27,6 +27,8 @@ Version history
 * grpc-json: added support for :ref:`ignoring unknown query parameters<envoy_api_field_config.filter.http.transcoder.v2.GrpcJsonTranscoder.ignore_unknown_query_parameters>`.
 * header to metadata: added :ref:`PROTOBUF_VALUE <envoy_api_enum_value_config.filter.http.header_to_metadata.v2.Config.ValueType.PROTOBUF_VALUE>` and :ref:`ValueEncode <envoy_api_enum_config.filter.http.header_to_metadata.v2.Config.ValueEncode>` to support protobuf Value and Base64 encoding.
 * http: added the ability to reject HTTP/1.1 requests with invalid HTTP header values, using the runtime feature `envoy.reloadable_features.strict_header_validation`.
+* 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: changed Envoy to forward existing x-forwarded-proto from downstream trusted proxies. Guarded by `envoy.reloadable_features.trusted_forwarded_proto` which defaults true.
 * 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.
 * 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.

diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h
@@ -1,6 +1,7 @@
 #pragma once
 
 #include "envoy/config/config_provider.h"
+#include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.h"
 #include "envoy/http/filter.h"
 #include "envoy/router/rds.h"
 #include "envoy/stats/scope.h"
@@ -170,6 +171,9 @@ class DefaultInternalAddressConfig : public Http::InternalAddressConfig {
  */
 class ConnectionManagerConfig {
 public:
+  using HttpConnectionManagerProto =
+      envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager;
+
   virtual ~ConnectionManagerConfig() = default;
 
   /**
@@ -265,6 +269,11 @@ class ConnectionManagerConfig {
    */
   virtual const std::string& serverName() PURE;
 
+  /**
+   * @return ServerHeaderTransformation the transformation to apply to Server response headers.
+   */
+  virtual HttpConnectionManagerProto::ServerHeaderTransformation serverHeaderTransformation() PURE;
+
   /**
    * @return ConnectionManagerStats& the stats to write to.
    */

diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc
@@ -1353,7 +1353,12 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte
   // Base headers.
   connection_manager_.config_.dateProvider().setDateHeader(headers);
   // Following setReference() is safe because serverName() is constant for the life of the listener.
-  headers.insertServer().value().setReference(connection_manager_.config_.serverName());
+  const auto transformation = connection_manager_.config_.serverHeaderTransformation();
+  if (transformation == ConnectionManagerConfig::HttpConnectionManagerProto::OVERWRITE ||
+      (transformation == ConnectionManagerConfig::HttpConnectionManagerProto::APPEND_IF_ABSENT &&
+       headers.Server() == nullptr)) {
+    headers.insertServer().value().setReference(connection_manager_.config_.serverName());
+  }
   ConnectionManagerUtility::mutateResponseHeaders(headers, request_headers_.get(),
                                                   connection_manager_.config_.via());
 

diff --git a/source/extensions/filters/network/http_connection_manager/BUILD b/source/extensions/filters/network/http_connection_manager/BUILD
@@ -40,5 +40,6 @@ envoy_cc_library(
         "//source/common/router:scoped_rds_lib",
         "//source/extensions/filters/network:well_known_names",
         "//source/extensions/filters/network/common:factory_base_lib",
+        "@envoy_api//envoy/config/filter/network/http_connection_manager/v2:http_connection_manager_cc",
     ],
 )
diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc
@@ -286,6 +286,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
     access_logs_.push_back(current_access_log);
   }
 
+  server_transformation_ = config.server_header_transformation();
+
   if (!config.server_name().empty()) {
     server_name_ = config.server_name();
   } else {

diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h
@@ -116,6 +116,9 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
     return scoped_routes_config_provider_.get();
   }
   const std::string& serverName() override { return server_name_; }
+  HttpConnectionManagerProto::ServerHeaderTransformation serverHeaderTransformation() override {
+    return server_transformation_;
+  }
   Http::ConnectionManagerStats& stats() override { return stats_; }
   Http::ConnectionManagerTracingStats& tracingStats() override { return tracing_stats_; }
   bool useRemoteAddress() override { return use_remote_address_; }
@@ -166,6 +169,8 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
   CodecType codec_type_;
   const Http::Http2Settings http2_settings_;
   const Http::Http1Settings http1_settings_;
+  HttpConnectionManagerProto::ServerHeaderTransformation server_transformation_{
+      HttpConnectionManagerProto::OVERWRITE};
   std::string server_name_;
   Http::TracingConnectionManagerConfigPtr tracing_config_;
   absl::optional<std::string> user_agent_;

diff --git a/source/server/http/admin.h b/source/server/http/admin.h
@@ -121,6 +121,9 @@ class AdminImpl : public Admin,
     return &scoped_route_config_provider_;
   }
   const std::string& serverName() override { return Http::DefaultServerString::get(); }
+  HttpConnectionManagerProto::ServerHeaderTransformation serverHeaderTransformation() override {
+    return HttpConnectionManagerProto::OVERWRITE;
+  }
   Http::ConnectionManagerStats& stats() override { return stats_; }
   Http::ConnectionManagerTracingStats& tracingStats() override { return tracing_stats_; }
   bool useRemoteAddress() override { return true; }

diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc
@@ -91,6 +91,9 @@ class FuzzConfig : public ConnectionManagerConfig {
     return &scoped_route_config_provider_;
   }
   const std::string& serverName() override { return server_name_; }
+  HttpConnectionManagerProto::ServerHeaderTransformation serverHeaderTransformation() override {
+    return server_transformation_;
+  }
   ConnectionManagerStats& stats() override { return stats_; }
   ConnectionManagerTracingStats& tracingStats() override { return tracing_stats_; }
   bool useRemoteAddress() override { return use_remote_address_; }
@@ -124,6 +127,8 @@ class FuzzConfig : public ConnectionManagerConfig {
   ConnectionManagerImplHelper::RouteConfigProvider route_config_provider_;
   ConnectionManagerImplHelper::ScopedRouteConfigProvider scoped_route_config_provider_;
   std::string server_name_;
+  HttpConnectionManagerProto::ServerHeaderTransformation server_transformation_{
+      HttpConnectionManagerProto::OVERWRITE};
   Stats::IsolatedStoreImpl fake_stats_;
   ConnectionManagerStats stats_;
   ConnectionManagerTracingStats tracing_stats_;

diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc
@@ -227,6 +227,21 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan
     conn_manager_->onData(fake_input, false);
   }
 
+  HeaderMap* sendResponseHeaders(HeaderMapPtr&& response_headers) {
+    HeaderMap* altered_response_headers = nullptr;
+
+    EXPECT_CALL(*encoder_filters_[0], encodeHeaders(_, _))
+        .WillOnce(Invoke([&](HeaderMap& headers, bool) -> FilterHeadersStatus {
+          altered_response_headers = &headers;
+          return FilterHeadersStatus::Continue;
+        }));
+    EXPECT_CALL(*encoder_filters_[1], encodeHeaders(_, false))
+        .WillOnce(Return(FilterHeadersStatus::Continue));
+    EXPECT_CALL(response_encoder_, encodeHeaders(_, false));
+    decoder_filters_[0]->callbacks_->encodeHeaders(std::move(response_headers), false);
+    return altered_response_headers;
+  }
+
   void expectOnDestroy() {
     for (auto filter : decoder_filters_) {
       EXPECT_CALL(*filter, onDestroy());
@@ -261,6 +276,9 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan
     return &scoped_route_config_provider_;
   }
   const std::string& serverName() override { return server_name_; }
+  HttpConnectionManagerProto::ServerHeaderTransformation serverHeaderTransformation() override {
+    return server_transformation_;
+  }
   ConnectionManagerStats& stats() override { return stats_; }
   ConnectionManagerTracingStats& tracingStats() override { return tracing_stats_; }
   bool useRemoteAddress() override { return use_remote_address_; }
@@ -301,6 +319,8 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan
   NiceMock<Network::MockDrainDecision> drain_close_;
   std::unique_ptr<ConnectionManagerImpl> conn_manager_;
   std::string server_name_;
+  HttpConnectionManagerProto::ServerHeaderTransformation server_transformation_{
+      HttpConnectionManagerProto::OVERWRITE};
   Network::Address::Ipv4Instance local_address_{"127.0.0.1"};
   bool use_remote_address_{true};
   Http::DefaultInternalAddressConfig internal_address_config_;
@@ -537,6 +557,65 @@ TEST_F(HttpConnectionManagerImplTest, PauseResume100Continue) {
   decoder_filters_[1]->callbacks_->encodeHeaders(std::move(response_headers), false);
 }
 
+// By default, Envoy will set the server header to the server name, here "custom-value"
+TEST_F(HttpConnectionManagerImplTest, ServerHeaderOverwritten) {
+  setup(false, "custom-value", false);
+  setUpEncoderAndDecoder(false, false);
+
+  sendRequestHeadersAndData();
+  const HeaderMap* altered_headers = sendResponseHeaders(
+      HeaderMapPtr{new TestHeaderMapImpl{{":status", "200"}, {"server", "foo"}}});
+  EXPECT_EQ("custom-value", altered_headers->Server()->value().getStringView());
+}
+
+// When configured APPEND_IF_ABSENT if the server header is present it will be retained.
+TEST_F(HttpConnectionManagerImplTest, ServerHeaderAppendPresent) {
+  server_transformation_ = HttpConnectionManagerProto::APPEND_IF_ABSENT;
+  setup(false, "custom-value", false);
+  setUpEncoderAndDecoder(false, false);
+
+  sendRequestHeadersAndData();
+  const HeaderMap* altered_headers = sendResponseHeaders(
+      HeaderMapPtr{new TestHeaderMapImpl{{":status", "200"}, {"server", "foo"}}});
+  EXPECT_EQ("foo", altered_headers->Server()->value().getStringView());
+}
+
+// When configured APPEND_IF_ABSENT if the server header is absent the server name will be set.
+TEST_F(HttpConnectionManagerImplTest, ServerHeaderAppendAbsent) {
+  server_transformation_ = HttpConnectionManagerProto::APPEND_IF_ABSENT;
+  setup(false, "custom-value", false);
+  setUpEncoderAndDecoder(false, false);
+
+  sendRequestHeadersAndData();
+  const HeaderMap* altered_headers =
+      sendResponseHeaders(HeaderMapPtr{new TestHeaderMapImpl{{":status", "200"}}});
+  EXPECT_EQ("custom-value", altered_headers->Server()->value().getStringView());
+}
+
+// When configured PASS_THROUGH, the server name will pass through.
+TEST_F(HttpConnectionManagerImplTest, ServerHeaderPassthroughPresent) {
+  server_transformation_ = HttpConnectionManagerProto::PASS_THROUGH;
+  setup(false, "custom-value", false);
+  setUpEncoderAndDecoder(false, false);
+
+  sendRequestHeadersAndData();
+  const HeaderMap* altered_headers = sendResponseHeaders(
+      HeaderMapPtr{new TestHeaderMapImpl{{":status", "200"}, {"server", "foo"}}});
+  EXPECT_EQ("foo", altered_headers->Server()->value().getStringView());
+}
+
+// When configured PASS_THROUGH, the server header will not be added if absent.
+TEST_F(HttpConnectionManagerImplTest, ServerHeaderPassthroughAbsent) {
+  server_transformation_ = HttpConnectionManagerProto::PASS_THROUGH;
+  setup(false, "custom-value", false);
+  setUpEncoderAndDecoder(false, false);
+
+  sendRequestHeadersAndData();
+  const HeaderMap* altered_headers =
+      sendResponseHeaders(HeaderMapPtr{new TestHeaderMapImpl{{":status", "200"}}});
+  EXPECT_TRUE(altered_headers->Server() == nullptr);
+}
+
 TEST_F(HttpConnectionManagerImplTest, InvalidPathWithDualFilter) {
   InSequence s;
   setup(false, "");

diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc
@@ -64,6 +64,8 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig {
   MOCK_METHOD0(routeConfigProvider, Router::RouteConfigProvider*());
   MOCK_METHOD0(scopedRouteConfigProvider, Config::ConfigProvider*());
   MOCK_METHOD0(serverName, const std::string&());
+  MOCK_METHOD0(serverHeaderTransformation,
+               HttpConnectionManagerProto::ServerHeaderTransformation());
   MOCK_METHOD0(stats, ConnectionManagerStats&());
   MOCK_METHOD0(tracingStats, ConnectionManagerTracingStats&());
   MOCK_METHOD0(useRemoteAddress, bool());

diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc
@@ -237,6 +237,8 @@ stat_prefix: router
               ContainerEq(config.tracingConfig()->request_headers_for_tags_));
   EXPECT_EQ(*context_.local_info_.address_, config.localAddress());
   EXPECT_EQ("foo", config.serverName());
+  EXPECT_EQ(HttpConnectionManagerConfig::HttpConnectionManagerProto::OVERWRITE,
+            config.serverHeaderTransformation());
   EXPECT_EQ(5 * 60 * 1000, config.streamIdleTimeout().count());
 }
 
@@ -388,6 +390,66 @@ TEST_F(HttpConnectionManagerConfigTest, DisabledStreamIdleTimeout) {
   EXPECT_EQ(0, config.streamIdleTimeout().count());
 }
 
+TEST_F(HttpConnectionManagerConfigTest, ServerOverwrite) {
+  const std::string yaml_string = R"EOF(
+  stat_prefix: ingress_http
+  server_header_transformation: OVERWRITE
+  route_config:
+    name: local_route
+  http_filters:
+  - name: envoy.router
+  )EOF";
+
+  EXPECT_CALL(context_.runtime_loader_.snapshot_, featureEnabled(_, An<uint64_t>()))
+      .WillOnce(Invoke(&context_.runtime_loader_.snapshot_,
+                       &Runtime::MockSnapshot::featureEnabledDefault));
+  HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
+                                     date_provider_, route_config_provider_manager_,
+                                     scoped_routes_config_provider_manager_);
+  EXPECT_EQ(HttpConnectionManagerConfig::HttpConnectionManagerProto::OVERWRITE,
+            config.serverHeaderTransformation());
+}
+
+TEST_F(HttpConnectionManagerConfigTest, ServerAppendIfAbsent) {
+  const std::string yaml_string = R"EOF(
+  stat_prefix: ingress_http
+  server_header_transformation: APPEND_IF_ABSENT
+  route_config:
+    name: local_route
+  http_filters:
+  - name: envoy.router
+  )EOF";
+
+  EXPECT_CALL(context_.runtime_loader_.snapshot_, featureEnabled(_, An<uint64_t>()))
+      .WillOnce(Invoke(&context_.runtime_loader_.snapshot_,
+                       &Runtime::MockSnapshot::featureEnabledDefault));
+  HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
+                                     date_provider_, route_config_provider_manager_,
+                                     scoped_routes_config_provider_manager_);
+  EXPECT_EQ(HttpConnectionManagerConfig::HttpConnectionManagerProto::APPEND_IF_ABSENT,
+            config.serverHeaderTransformation());
+}
+
+TEST_F(HttpConnectionManagerConfigTest, ServerPassThrough) {
+  const std::string yaml_string = R"EOF(
+  stat_prefix: ingress_http
+  server_header_transformation: PASS_THROUGH
+  route_config:
+    name: local_route
+  http_filters:
+  - name: envoy.router
+  )EOF";
+
+  EXPECT_CALL(context_.runtime_loader_.snapshot_, featureEnabled(_, An<uint64_t>()))
+      .WillOnce(Invoke(&context_.runtime_loader_.snapshot_,
+                       &Runtime::MockSnapshot::featureEnabledDefault));
+  HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
+                                     date_provider_, route_config_provider_manager_,
+                                     scoped_routes_config_provider_manager_);
+  EXPECT_EQ(HttpConnectionManagerConfig::HttpConnectionManagerProto::PASS_THROUGH,
+            config.serverHeaderTransformation());
+}
+
 // Validated that by default we don't normalize paths
 // unless set build flag path_normalization_by_default=true
 TEST_F(HttpConnectionManagerConfigTest, NormalizePathDefault) {

diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc
@@ -21,6 +21,7 @@
 
 #include "extensions/transport_sockets/tls/context_config_impl.h"
 
+#include "test/mocks/http/mocks.h"
 #include "test/mocks/runtime/mocks.h"
 #include "test/mocks/server/mocks.h"
 #include "test/test_common/environment.h"
@@ -89,6 +90,17 @@ class AdminFilterTest : public testing::TestWithParam<Network::Address::IpVersio
   Http::TestHeaderMapImpl request_headers_;
 };
 
+// Check default implementations the admin class picks up.
+TEST_P(AdminFilterTest, MiscFunctions) {
+  EXPECT_EQ(false, admin_.preserveExternalRequestId());
+  Http::MockFilterChainFactoryCallbacks mock_filter_chain_factory_callbacks;
+  EXPECT_EQ(false,
+            admin_.createUpgradeFilterChain("", nullptr, mock_filter_chain_factory_callbacks));
+  EXPECT_TRUE(nullptr != admin_.scopedRouteConfigProvider());
+  EXPECT_EQ(Http::ConnectionManagerConfig::HttpConnectionManagerProto::OVERWRITE,
+            admin_.serverHeaderTransformation());
+}
+
 INSTANTIATE_TEST_SUITE_P(IpVersions, AdminStatsTest,
                          testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
                          TestUtility::ipTestParamsToString);