Fix #2308 - Mongrel should use X-Forwarded-For

Mongrel puppet code uses REMOTE_ADDR to set the ip address which will
be use to authenticate the client access.
Since mongrel is always used in a proxy mode with Puppet, REMOTE_ADDR
is always the address of the proxy (usually 127.0.0.1), which defeats
the purpose.
With this changeset, the mongrel code now uses the X-Forwarded-For
HTTP header value if it is passed over the REMOTE_ADDR.

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>

4 files changed, 33 insertions, 3 deletions

diff --git a/lib/puppet/network/http/mongrel/rest.rb b/lib/puppet/network/http/mongrel/rest.rb
index 2f49506c8..fe3f51d35 100644
--- a/lib/puppet/network/http/mongrel/rest.rb
+++ b/lib/puppet/network/http/mongrel/rest.rb
@@ -62,7 +62,7 @@ class Puppet::Network::HTTP::MongrelREST < Mongrel::HttpHandler
     def client_info(request)
         result = {}
         params = request.params
-        result[:ip] = params["REMOTE_ADDR"]
+        result[:ip] = params["HTTP_X_FORWARDED_FOR"] ? params["HTTP_X_FORWARDED_FOR"].split(',').last.strip : params["REMOTE_ADDR"]
 
         # JJM #906 The following dn.match regular expression is forgiving
         # enough to match the two Distinguished Name string contents
diff --git a/lib/puppet/network/http_server/mongrel.rb b/lib/puppet/network/http_server/mongrel.rb
index 924c11728..382b4dc58 100644
--- a/lib/puppet/network/http_server/mongrel.rb
+++ b/lib/puppet/network/http_server/mongrel.rb
@@ -118,7 +118,7 @@ module Puppet::Network
 
         def client_info(request)
             params = request.params
-            ip = params["REMOTE_ADDR"]
+            ip = params["HTTP_X_FORWARDED_FOR"] ? params["HTTP_X_FORWARDED_FOR"].split(',').last.strip : params["REMOTE_ADDR"]
             # JJM #906 The following dn.match regular expression is forgiving
             # enough to match the two Distinguished Name string contents
             # coming from Apache, Pound or other reverse SSL proxies.
diff --git a/spec/unit/network/http/mongrel/rest.rb b/spec/unit/network/http/mongrel/rest.rb
index 317fdaf8b..5a5d2cfec 100755
--- a/spec/unit/network/http/mongrel/rest.rb
+++ b/spec/unit/network/http/mongrel/rest.rb
@@ -152,6 +152,22 @@ describe "Puppet::Network::HTTP::MongrelREST" do
                 @handler.params(@request)[:ip].should == "ipaddress"
             end
 
+            it "should pass the client's provided X-Forwared-For value as the ip" do
+                @request.stubs(:params).returns("HTTP_X_FORWARDED_FOR" => "ipaddress")
+                @handler.params(@request)[:ip].should == "ipaddress"
+            end
+
+            it "should pass the client's provided X-Forwared-For first value as the ip" do
+                @request.stubs(:params).returns("HTTP_X_FORWARDED_FOR" => "ipproxy1,ipproxy2,ipaddress")
+                @handler.params(@request)[:ip].should == "ipaddress"
+            end
+
+            it "should pass the client's provided X-Forwared-For value as the ip instead of the REMOTE_ADDR" do
+                @request.stubs(:params).returns("REMOTE_ADDR" => "remote_addr")
+                @request.stubs(:params).returns("HTTP_X_FORWARDED_FOR" => "ipaddress")
+                @handler.params(@request)[:ip].should == "ipaddress"
+            end
+
             it "should use the :ssl_client_header to determine the parameter when looking for the certificate" do
                 Puppet.settings.stubs(:value).returns "eh"
                 Puppet.settings.expects(:value).with(:ssl_client_header).returns "myheader"
diff --git a/test/network/server/mongrel_test.rb b/test/network/server/mongrel_test.rb
index 80e9aa454..54bfb3978 100755
--- a/test/network/server/mongrel_test.rb
+++ b/test/network/server/mongrel_test.rb
@@ -29,7 +29,19 @@ class TestMongrelServer < PuppetTest::TestCase
         params[Puppet[:ssl_client_header]] = ""
         params[Puppet[:ssl_client_verify_header]] = "failure"
         info = nil
-        Resolv.expects(:getname).with(ip).returns("host.domain.com").times(3)
+        Resolv.expects(:getname).with(ip).returns("host.domain.com").times(4)
+        assert_nothing_raised("Could not call client_info") do
+            info = mongrel.send(:client_info, obj)
+        end
+        assert(! info.authenticated?, "Client info object was marked valid even though headers were missing")
+        assert_equal(ip, info.ip, "Did not copy over ip correctly")
+
+        assert_equal("host.domain.com", info.name, "Did not copy over hostname correctly")
+
+        # Now pass the X-Forwarded-For header and check it is preferred over REMOTE_ADDR
+        params["REMOTE_ADDR"] = '127.0.0.1'
+        params["HTTP_X_FORWARDED_FOR"] = ip
+        info = nil
         assert_nothing_raised("Could not call client_info") do
             info = mongrel.send(:client_info, obj)
         end
@@ -39,6 +51,8 @@ class TestMongrelServer < PuppetTest::TestCase
         assert_equal("host.domain.com", info.name, "Did not copy over hostname correctly")
 
         # Now add a valid auth header.
+        params["REMOTE_ADDR"] = ip
+        params["HTTP_X_FORWARDED_FOR"] = nil
         params[Puppet[:ssl_client_header]] = "/CN=host.domain.com"
         assert_nothing_raised("Could not call client_info") do
             info = mongrel.send(:client_info, obj)