From bee3f3da2b40fdee17665506bd541b3012901a64 Mon Sep 17 00:00:00 2001
From: Bruce Toll <btoll@dhsus.com>
Date: Tue, 26 May 2020 11:04:26 -0400
Subject: [PATCH] Add reverse-proxy-client-ip wt_config.xml option

Add a reverse-proxy-client-ip option to wt_config.xml that provides
finer control over the http headers recognized when the
behind-reverse-proxy option is enabled.

When the reverse-proxy-client-ip option is set true, Wt will consider
http "Client-IP" headers in determining the clientAddress() for a
session. Otherwise, the "Client-IP" headers will be ignored.

The default setting for reverse-proxy-client-ip is false. This is
a change from earlier versions of Wt which behaved as if the option was
set to true.

Additional tests for the new option are included in test.http.
---
 src/Wt/Http/Request.C            |  3 +-
 src/Wt/WEnvironment.C            |  3 +-
 src/http/ProxyReply.C            |  2 +-
 src/web/Configuration.C          | 13 +++++
 src/web/Configuration.h          |  3 +
 src/web/WebRequest.C             | 16 ++++--
 src/web/WebRequest.h             |  2 +-
 src/web/WebSession.C             |  3 +-
 test/http/HttpClientServerTest.C | 97 +++++++++++++++++++++++++++++++-
 wt_config.xml.in                 | 11 ++++
 10 files changed, 141 insertions(+), 12 deletions(-)

diff --git a/src/Wt/Http/Request.C b/src/Wt/Http/Request.C
index 17ab28ea..95f8bfc3 100644
--- a/src/Wt/Http/Request.C
+++ b/src/Wt/Http/Request.C
@@ -226,7 +226,8 @@ std::string Request::clientAddress() const
 
   WServer *server = WServer::instance();
   const bool behindReverseProxy = server && server->configuration().behindReverseProxy();
-  return request_->clientAddress(behindReverseProxy);
+  const bool reverseProxyClientIp = server && server->configuration().reverseProxyClientIp();
+  return request_->clientAddress(behindReverseProxy, reverseProxyClientIp);
 }
 
 WSslInfo *Request::sslInfo() const
diff --git a/src/Wt/WEnvironment.C b/src/Wt/WEnvironment.C
index 1aef48dd..b7db129d 100644
--- a/src/Wt/WEnvironment.C
+++ b/src/Wt/WEnvironment.C
@@ -183,7 +183,8 @@ void WEnvironment::init(const WebRequest& request)
       host_ += ":" + request.serverPort();
   }
 
-  clientAddress_ = request.clientAddress(conf.behindReverseProxy());
+  clientAddress_ = request.clientAddress(conf.behindReverseProxy(),
+      conf.reverseProxyClientIp());
 
   const char *cookie = request.headerValue("Cookie");
   doesCookies_ = cookie;
diff --git a/src/http/ProxyReply.C b/src/http/ProxyReply.C
index 76463ac0..f8ac88c9 100644
--- a/src/http/ProxyReply.C
+++ b/src/http/ProxyReply.C
@@ -275,7 +275,7 @@ void ProxyReply::assembleRequestHeaders()
         LOG_SECURE("wthttp is not behind a reverse proxy, dropping " << it->name.str() << " header");
       }
     } else if (it->name.iequals("X-Forwarded-For") ||
-               it->name.iequals("Client-IP")) {
+               (wtConfiguration.reverseProxyClientIp() && it->name.iequals("Client-IP"))) {
       if (wtConfiguration.behindReverseProxy()) {
         forwardedFor = it->value.str() + ", ";
       }
diff --git a/src/web/Configuration.C b/src/web/Configuration.C
index 75b65b6a..d7f07a78 100644
--- a/src/web/Configuration.C
+++ b/src/web/Configuration.C
@@ -214,6 +214,7 @@ void Configuration::reset()
   properties_.clear();
   xhtmlMimeType_ = false;
   behindReverseProxy_ = false;
+  reverseProxyClientIp_ = false;
   redirectMsg_ = "Load basic HTML";
   serializedEvents_ = false;
   webSockets_ = false;
@@ -398,6 +399,12 @@ bool Configuration::behindReverseProxy() const
   return behindReverseProxy_;
 }
 
+bool Configuration::reverseProxyClientIp() const
+{
+  READ_LOCK;
+  return reverseProxyClientIp_;
+}
+
 std::string Configuration::redirectMessage() const
 {
   READ_LOCK;
@@ -872,6 +879,11 @@ void Configuration::setBehindReverseProxy(bool enabled)
   behindReverseProxy_ = enabled;
 }
 
+void Configuration::setReverseProxyClientIp(bool enabled)
+{
+  reverseProxyClientIp_ = enabled;
+}
+
 void Configuration::readApplicationSettings(xml_node<> *app)
 {
   xml_node<> *sess = singleChildElement(app, "session-management");
@@ -978,6 +990,7 @@ void Configuration::readApplicationSettings(xml_node<> *app)
   redirectMsg_ = singleChildElementValue(app, "redirect-message", redirectMsg_);
 
   setBoolean(app, "behind-reverse-proxy", behindReverseProxy_);
+  setBoolean(app, "reverse-proxy-client-ip", reverseProxyClientIp_);
   setBoolean(app, "strict-event-serialization", serializedEvents_);
   setBoolean(app, "web-sockets", webSockets_);
 
diff --git a/src/web/Configuration.h b/src/web/Configuration.h
index 718c2ec3..f90639a3 100644
--- a/src/web/Configuration.h
+++ b/src/web/Configuration.h
@@ -208,6 +208,7 @@ public:
   void setAppRoot(const std::string& path);
   std::string appRoot() const;
   bool behindReverseProxy() const;
+  bool reverseProxyClientIp() const;
   std::string redirectMessage() const;
   bool serializedEvents() const;
   bool webSockets() const;
@@ -234,6 +235,7 @@ public:
   void setUseSlashExceptionForInternalPaths(bool enabled);
   void setNeedReadBodyBeforeResponse(bool needed);
   void setBehindReverseProxy(bool enabled);
+  void setReverseProxyClientIp(bool enabled);
 
   std::string generateSessionId();
   bool registerSessionId(const std::string& oldId, const std::string& newId);
@@ -288,6 +290,7 @@ private:
   PropertyMap     properties_;
   bool            xhtmlMimeType_;
   bool            behindReverseProxy_;
+  bool            reverseProxyClientIp_;
   std::string     redirectMsg_;
   bool            serializedEvents_;
   bool		  webSockets_;
diff --git a/src/web/WebRequest.C b/src/web/WebRequest.C
index 679bce60..5f5073db 100644
--- a/src/web/WebRequest.C
+++ b/src/web/WebRequest.C
@@ -365,7 +365,8 @@ const std::vector<std::pair<std::string, std::string> > &WebRequest::urlParams()
   return urlParams_;
 }
 
-std::string WebRequest::clientAddress(const bool behindReverseProxy) const
+std::string WebRequest::clientAddress(const bool behindReverseProxy,
+    const bool reverseProxyClientIp) const
 {
   std::string result;
 
@@ -373,12 +374,15 @@ std::string WebRequest::clientAddress(const bool behindReverseProxy) const
    * Determine client address, taking into account proxies
    */
   if (behindReverseProxy) {
-    std::string clientIp = str(headerValue("Client-IP"));
-    boost::trim(clientIp);
-
     std::vector<std::string> ips;
-    if (!clientIp.empty())
-      boost::split(ips, clientIp, boost::is_any_of(","));
+
+    if (reverseProxyClientIp) {
+      std::string clientIp = str(headerValue("Client-IP"));
+      boost::trim(clientIp);
+
+      if (!clientIp.empty())
+        boost::split(ips, clientIp, boost::is_any_of(","));
+    }
 
     std::string forwardedFor = str(headerValue("X-Forwarded-For"));
     boost::trim(forwardedFor);
diff --git a/src/web/WebRequest.h b/src/web/WebRequest.h
index 61864a7c..84ba645e 100644
--- a/src/web/WebRequest.h
+++ b/src/web/WebRequest.h
@@ -197,7 +197,7 @@ public:
 
   virtual const std::vector<std::pair<std::string, std::string> >& urlParams() const;
 
-  std::string clientAddress(bool behindReverseProxy) const;
+  std::string clientAddress(bool behindReverseProxy, bool reverseProxyClientIp) const;
 
 protected:
   const EntryPoint *entryPoint_;
diff --git a/src/web/WebSession.C b/src/web/WebSession.C
index 198889af..bba93f33 100644
--- a/src/web/WebSession.C
+++ b/src/web/WebSession.C
@@ -2340,7 +2340,8 @@ void WebSession::notify(const WEvent& event)
 	}
 
       std::string ca = handler.request()->clientAddress(
-            controller_->configuration().behindReverseProxy());
+            controller_->configuration().behindReverseProxy(),
+            controller_->configuration().reverseProxyClientIp());
 
       if (ca != env_->clientAddress()) {
 	bool isInvalid = sessionIdCookie_.empty();
diff --git a/test/http/HttpClientServerTest.C b/test/http/HttpClientServerTest.C
index 084a465f..206cdc87 100644
--- a/test/http/HttpClientServerTest.C
+++ b/test/http/HttpClientServerTest.C
@@ -9,10 +9,12 @@
 
 #include <boost/test/unit_test.hpp>
 
+#include <web/Configuration.h>
 #include <Wt/WResource.h>
 #include <Wt/WServer.h>
 #include <Wt/WIOService.h>
 #include <Wt/Http/Client.h>
+#include <Wt/Http/Message.h>
 #include <Wt/Http/Response.h>
 #include <Wt/Http/ResponseContinuation.h>
 #include <Wt/Http/Request.h>
@@ -34,6 +36,7 @@ namespace {
 	delaySendingBody_(false),
 	haveEverMoreData_(false),
 	haveRandomMoreData_(false),
+	clientAddressTest_(false),
 	aborted_(0)
     { }
 
@@ -57,6 +60,10 @@ namespace {
       haveRandomMoreData_ = true;
     }
 
+    void clientAddressTest() {
+      clientAddressTest_ = true;
+    }
+
     int abortedCount() const {
       return aborted_;
     }
@@ -66,6 +73,8 @@ namespace {
     {
       if (continuation_)
 	handleWithContinuation(request, response);
+      else if (clientAddressTest_)
+	handleClientAddress(request, response);
       else
 	handleSimple(request, response);
     }
@@ -80,6 +89,7 @@ namespace {
     bool delaySendingBody_;
     bool haveEverMoreData_;
     bool haveRandomMoreData_;
+    bool clientAddressTest_;
     int aborted_;
 
     void handleSimple(const Http::Request& request,
@@ -104,8 +114,16 @@ namespace {
 	  c->waitForMoreData();
       }
     }
+
+    void handleClientAddress(const Http::Request& request,
+			     Http::Response& response)
+    {
+      response.setStatus(200);
+      response.out() << request.clientAddress();
+    }
   };
 
+
   class Server : public WServer
   {
   public:
@@ -327,11 +345,88 @@ BOOST_AUTO_TEST_CASE( http_client_server_test5 )
   }
 }
 
-#endif // WT_THREADED
+BOOST_AUTO_TEST_CASE( client_address_default_not_behind_reverse_proxy )
+{
+  Server server;
+  server.resource().clientAddressTest();
+
+  if (server.start()) {
+    Client client;
+
+    /*
+     * NOTE: Need to use public IP address or clientAddress() will ignore
+     *       The addresses are not accessed and I believe are Google DNS
+     */
+    std::vector<Http::Message::Header> headers;
+    headers.push_back(Http::Message::Header("X-Forwarded-For", "8.8.8.8"));
+    headers.push_back(Http::Message::Header("Client-IP", "8.8.4.4"));
 
+    client.get("http://" + server.address() + "/test", headers);
+    client.waitDone();
 
+    BOOST_REQUIRE(!client.err());
+    BOOST_REQUIRE(client.message().status() == 200);
 
+    // should ignore "Client-IP" and "X-Forwarded-For" headers if not behindReverseProxy
+    BOOST_REQUIRE(client.message().body() == "127.0.0.1");
+  }
+}
 
+BOOST_AUTO_TEST_CASE( client_address_behind_reverse_proxy_without_reverse_proxy_client_ip )
+{
+  Server server;
+  server.resource().clientAddressTest();
+  server.configuration().setBehindReverseProxy(true);
 
+  if (server.start()) {
+    Client client;
 
+    /*
+     * NOTE: Need to use public IP address or clientAddress() will ignore
+     *       The addresses are not accessed and I believe are Google DNS
+     */
+    std::vector<Http::Message::Header> headers;
+    headers.push_back(Http::Message::Header("X-Forwarded-For", "8.8.8.8"));
+    headers.push_back(Http::Message::Header("Client-IP", "8.8.4.4"));
 
+    client.get("http://" + server.address() + "/test", headers);
+    client.waitDone();
+
+    BOOST_REQUIRE(!client.err());
+    BOOST_REQUIRE(client.message().status() == 200);
+
+    // should use "X-Forwarded-For" header if behindReverseProxy without reverseProxyClientIp
+    BOOST_REQUIRE(client.message().body() == "8.8.8.8");
+  }
+}
+
+BOOST_AUTO_TEST_CASE( client_address_behind_reverse_proxy_with_reverse_proxy_client_ip )
+{
+  Server server;
+  server.resource().clientAddressTest();
+  server.configuration().setBehindReverseProxy(true);
+  server.configuration().setReverseProxyClientIp(true);
+
+  if (server.start()) {
+    Client client;
+
+    /*
+     * NOTE: Need to use public IP address or clientAddress() will ignore
+     *       The addresses are not accessed and I believe are Google DNS
+     */
+    std::vector<Http::Message::Header> headers;
+    headers.push_back(Http::Message::Header("X-Forwarded-For", "8.8.8.8"));
+    headers.push_back(Http::Message::Header("Client-IP", "8.8.4.4"));
+
+    client.get("http://" + server.address() + "/test", headers);
+    client.waitDone();
+
+    BOOST_REQUIRE(!client.err());
+    BOOST_REQUIRE(client.message().status() == 200);
+
+    // should use "Client-IP" header if behindReverseProxy with reverseProxyClientIp
+    BOOST_REQUIRE(client.message().body() == "8.8.4.4");
+  }
+}
+
+#endif // WT_THREADED
diff --git a/wt_config.xml.in b/wt_config.xml.in
index 817ca88f..813dcaad 100644
--- a/wt_config.xml.in
+++ b/wt_config.xml.in
@@ -404,6 +404,17 @@
 	  -->
 	<behind-reverse-proxy>false</behind-reverse-proxy>
 
+	<!-- Whether HTTP Client-IP headers should be processed
+
+	   If behind-reverse-proxy is enabled, this option can be used
+	   to enable processing of addresses in Client-IP headers, as
+	   well as X-Forwarded-For headers.
+
+	   If your reverse proxy does not use Client-IP headers, you
+	   should leave this at its default setting of false.
+	  -->
+	<reverse-proxy-client-ip>false</reverse-proxy-client-ip>
+
 	<!-- Whether inline CSS is allowed.
 
            Some Wt widgets will insert CSS rules in the the inline
-- 
2.23.2

