upstream: require opt-in for the x-envoy-original-dst-host header. (#…

…4046)

*Risk Level*: Low
*Testing*: bazel test //test/...
*Docs Changes*: Added
*Release Notes*: Added

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

(cherry picked from commit 3460595)
Signed-off-by: Matt Klein <mklein@lyft.com>

VERSION

@@ -1 +1 @@
-1.7.0
+1.7.1

api/envoy/api/v2/cds.proto

@@ -42,7 +42,7 @@ service ClusterDiscoveryService {
 // [#protodoc-title: Clusters]
 
 // Configuration for a single upstream cluster.
-// [#comment:next free field: 34]
+// [#comment:next free field: 35]
 message Cluster {
   // Supplies the name of the cluster which must be unique across all clusters.
   // The cluster name is used when emitting
@@ -373,6 +373,22 @@ message Cluster {
     DeprecatedV1 deprecated_v1 = 2 [deprecated = true];
   }
 
+  // Specific configuration for the
+  // :ref:`Original Destination <arch_overview_load_balancing_types_original_destination>`
+  // load balancing policy.
+  message OriginalDstLbConfig {
+    // When true, :ref:`x-envoy-orignal-dst-host
+    // <config_http_conn_man_headers_x-envoy-original-dst-host>` can be used to override destination
+    // address.
+    //
+    // .. attention::
+    //
+    //   This header isn't sanitized by default, so enabling this feature allows HTTP clients to
+    //   route traffic to arbitrary hosts and/or ports, which may have serious security
+    //   consequences.
+    bool use_http_header = 1;
+  }
+
   // Optional configuration for the load balancing algorithm selected by
   // LbPolicy. Currently only
   // :ref:`RING_HASH<envoy_api_enum_value_Cluster.LbPolicy.RING_HASH>`
@@ -383,6 +399,8 @@ message Cluster {
   oneof lb_config {
     // Optional configuration for the Ring Hash load balancing policy.
     RingHashLbConfig ring_hash_lb_config = 23;
+    // Optional configuration for the Original Destination load balancing policy.
+    OriginalDstLbConfig original_dst_lb_config = 34;
   }
 
   // Common configuration for all load balancer implementations.

docs/root/configuration/http_conn_man/headers.rst

@@ -98,6 +98,18 @@ the header value to *true*.
 
 This is a convenience to avoid having to parse and understand XFF.
 
+.. _config_http_conn_man_headers_x-envoy-original-dst-host:
+
+x-envoy-original-dst-host
+-------------------------
+
+The header used to override destination address when using the
+:ref:`Original Destination <arch_overview_load_balancing_types_original_destination>`
+load balancing policy.
+
+It is ignored, unless the use of it is enabled via
+:ref:`use_http_header <envoy_api_field_Cluster.OriginalDstLbConfig.use_http_header>`.
+
 .. _config_http_conn_man_headers_x-forwarded-client-cert:
 
 x-forwarded-client-cert

docs/root/intro/arch_overview/load_balancing.rst

@@ -126,7 +126,8 @@ be used with original destination clusters.
 
 Original destination host request header
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-Envoy can also pick up the original destination from a HTTP header called ``x-envoy-original-destination-host``. 
+Envoy can also pick up the original destination from a HTTP header called
+:ref:`x-envoy-orignal-dst-host <config_http_conn_man_headers_x-envoy-original-dst-host>`.
 Please note that fully resolved IP address should be passed in this header. For example if a request has to be
 routed to a host with IP address 10.195.16.237 at port 8888, the request header value should be set as
 ``10.195.16.237:8888``.

docs/root/intro/version_history.rst

@@ -1,6 +1,13 @@
 Version history
 ---------------
 
+1.7.1
+=====
+
+* upstream: require opt-in to use the :ref:`x-envoy-orignal-dst-host <config_http_conn_man_headers_x-envoy-original-dst-host>` header
+  for overriding destination address when using the :ref:`Original Destination <arch_overview_load_balancing_types_original_destination>`
+  load balancing policy.
+
 1.7.0
 ===============
 * access log: added ability to log response trailers.

include/envoy/upstream/upstream.h

@@ -458,6 +458,14 @@ class ClusterInfo {
   virtual const absl::optional<envoy::api::v2::Cluster::RingHashLbConfig>&
   lbRingHashConfig() const PURE;
 
+  /**
+   * @return const absl::optional<envoy::api::v2::Cluster::OriginalDstLbConfig>& the configuration
+   *         for the Original Destination load balancing policy, only used if type is set to
+   *         ORIGINAL_DST_LB.
+   */
+  virtual const absl::optional<envoy::api::v2::Cluster::OriginalDstLbConfig>&
+  lbOriginalDstConfig() const PURE;
+
   /**
    * @return Whether the cluster is currently in maintenance mode and should not be routed to.
    *         Different filters may handle this situation in different ways. The implementation

source/common/upstream/cluster_manager_impl.cc

@@ -877,7 +877,8 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::ClusterEntry(
     case LoadBalancerType::OriginalDst: {
       ASSERT(lb_factory_ == nullptr);
       lb_.reset(new OriginalDstCluster::LoadBalancer(
-          priority_set_, parent.parent_.active_clusters_.at(cluster->name())->cluster_));
+          priority_set_, parent.parent_.active_clusters_.at(cluster->name())->cluster_,
+          cluster->lbOriginalDstConfig()));
       break;
     }
     }

source/common/upstream/original_dst_cluster.cc

@@ -20,9 +20,11 @@ namespace Upstream {
 // OriginalDstCluster::LoadBalancer is never configured with any other type of cluster,
 // and throws an exception otherwise.
 
-OriginalDstCluster::LoadBalancer::LoadBalancer(PrioritySet& priority_set, ClusterSharedPtr& parent)
+OriginalDstCluster::LoadBalancer::LoadBalancer(
+    PrioritySet& priority_set, ClusterSharedPtr& parent,
+    const absl::optional<envoy::api::v2::Cluster::OriginalDstLbConfig>& config)
     : priority_set_(priority_set), parent_(std::static_pointer_cast<OriginalDstCluster>(parent)),
-      info_(parent->info()) {
+      info_(parent->info()), use_http_header_(config ? config.value().use_http_header() : false) {
   // priority_set_ is initially empty.
   priority_set_.addMemberUpdateCb(
       [this](uint32_t, const HostVector& hosts_added, const HostVector& hosts_removed) -> void {
@@ -44,7 +46,10 @@ HostConstSharedPtr OriginalDstCluster::LoadBalancer::chooseHost(LoadBalancerCont
   if (context) {
 
     // Check if override host header is present, if yes use it otherwise check local address.
-    Network::Address::InstanceConstSharedPtr dst_host = requestOverrideHost(context);
+    Network::Address::InstanceConstSharedPtr dst_host = nullptr;
+    if (use_http_header_) {
+      dst_host = requestOverrideHost(context);
+    }
     if (dst_host == nullptr) {
       const Network::Connection* connection = context->downstreamConnection();
       // The local address of the downstream connection is the original destination address,

source/common/upstream/original_dst_cluster.h

@@ -43,7 +43,8 @@ class OriginalDstCluster : public ClusterImplBase {
    */
   class LoadBalancer : public Upstream::LoadBalancer {
   public:
-    LoadBalancer(PrioritySet& priority_set, ClusterSharedPtr& parent);
+    LoadBalancer(PrioritySet& priority_set, ClusterSharedPtr& parent,
+                 const absl::optional<envoy::api::v2::Cluster::OriginalDstLbConfig>& config);
 
     // Upstream::LoadBalancer
     HostConstSharedPtr chooseHost(LoadBalancerContext* context) override;
@@ -97,6 +98,7 @@ class OriginalDstCluster : public ClusterImplBase {
     PrioritySet& priority_set_;                // Thread local priority set.
     std::weak_ptr<OriginalDstCluster> parent_; // Primary cluster managed by the main thread.
     ClusterInfoConstSharedPtr info_;
+    const bool use_http_header_;
     HostMap host_map_;
   };
 

source/common/upstream/upstream_impl.cc

@@ -276,7 +276,8 @@ ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config,
       resource_managers_(config, runtime, name_),
       maintenance_mode_runtime_key_(fmt::format("upstream.maintenance_mode.{}", name_)),
       source_address_(getSourceAddress(config, bind_config)),
-      lb_ring_hash_config_(envoy::api::v2::Cluster::RingHashLbConfig(config.ring_hash_lb_config())),
+      lb_ring_hash_config_(config.ring_hash_lb_config()),
+      lb_original_dst_config_(config.original_dst_lb_config()),
       ssl_context_manager_(ssl_context_manager), added_via_api_(added_via_api),
       lb_subset_(LoadBalancerSubsetInfoImpl(config.lb_subset_config())),
       metadata_(config.metadata()), common_lb_config_(config.common_lb_config()),

source/common/upstream/upstream_impl.h

@@ -338,6 +338,10 @@ class ClusterInfoImpl : public ClusterInfo,
   lbRingHashConfig() const override {
     return lb_ring_hash_config_;
   }
+  const absl::optional<envoy::api::v2::Cluster::OriginalDstLbConfig>&
+  lbOriginalDstConfig() const override {
+    return lb_original_dst_config_;
+  }
   bool maintenanceMode() const override;
   uint64_t maxRequestsPerConnection() const override { return max_requests_per_connection_; }
   const std::string& name() const override { return name_; }
@@ -397,6 +401,7 @@ class ClusterInfoImpl : public ClusterInfo,
   const Network::Address::InstanceConstSharedPtr source_address_;
   LoadBalancerType lb_type_;
   absl::optional<envoy::api::v2::Cluster::RingHashLbConfig> lb_ring_hash_config_;
+  absl::optional<envoy::api::v2::Cluster::OriginalDstLbConfig> lb_original_dst_config_;
   Ssl::ContextManager& ssl_context_manager_;
   const bool added_via_api_;
   LoadBalancerSubsetInfoImpl lb_subset_;