From 5bd61b57d63ae86fc246531d3a483c15ee0dcd57 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sun, 25 May 2014 04:40:20 +0000 Subject: http: remove xftrust options This has long been considered a mistake and not documented for very long. I considered removing X-Forwarded-Proto and X-Forwarded-SSL handling, too, so rack.url_scheme is always "http", but that might lead to compatibility issues in rare apps if Rack::Request#scheme is not used. --- ext/unicorn_http/unicorn_http.rl | 70 ++++++++++------------------------- lib/unicorn/configurator.rb | 13 ------- lib/unicorn/http_server.rb | 8 ---- t/t0016-trust-x-forwarded-false.sh | 30 --------------- t/t0017-trust-x-forwarded-true.sh | 30 --------------- test/unit/test_http_parser_xftrust.rb | 38 ------------------- 6 files changed, 20 insertions(+), 169 deletions(-) delete mode 100755 t/t0016-trust-x-forwarded-false.sh delete mode 100755 t/t0017-trust-x-forwarded-true.sh delete mode 100644 test/unit/test_http_parser_xftrust.rb diff --git a/ext/unicorn_http/unicorn_http.rl b/ext/unicorn_http/unicorn_http.rl index 6293033..ecdadb0 100644 --- a/ext/unicorn_http/unicorn_http.rl +++ b/ext/unicorn_http/unicorn_http.rl @@ -29,12 +29,6 @@ void init_unicorn_httpdate(void); /* all of these flags need to be set for keepalive to be supported */ #define UH_FL_KEEPALIVE (UH_FL_KAVERSION | UH_FL_REQEOF | UH_FL_HASHEADER) -/* - * whether or not to trust X-Forwarded-Proto and X-Forwarded-SSL when - * setting rack.url_scheme - */ -static VALUE trust_x_forward = Qtrue; - static unsigned long keepalive_requests = 100; /* same as nginx */ /* @@ -58,31 +52,6 @@ static VALUE set_ka_req(VALUE self, VALUE val) return ka_req(self); } -/* - * Sets whether or not the parser will trust X-Forwarded-Proto and - * X-Forwarded-SSL headers and set "rack.url_scheme" to "https" accordingly. - * Rainbows!/Zbatery installations facing untrusted clients directly - * should set this to +false+ - */ -static VALUE set_xftrust(VALUE self, VALUE val) -{ - if (Qtrue == val || Qfalse == val) - trust_x_forward = val; - else - rb_raise(rb_eTypeError, "must be true or false"); - - return val; -} - -/* - * returns whether or not the parser will trust X-Forwarded-Proto and - * X-Forwarded-SSL headers and set "rack.url_scheme" to "https" accordingly - */ -static VALUE xftrust(VALUE self) -{ - return trust_x_forward; -} - static size_t MAX_HEADER_LEN = 1024 * (80 + 32); /* same as Mongrel */ /* this is only intended for use with Rainbows! */ @@ -491,26 +460,29 @@ static void set_url_scheme(VALUE env, VALUE *server_port) VALUE scheme = rb_hash_aref(env, g_rack_url_scheme); if (NIL_P(scheme)) { - if (trust_x_forward == Qfalse) { - scheme = g_http; + /* + * would anybody be horribly opposed to removing the X-Forwarded-SSL + * and X-Forwarded-Proto handling from this parser? We've had it + * forever and nobody has said anything against it, either. + * Anyways, please send comments to our public mailing list: + * unicorn-public@bogomips.org (no HTML mail, no subscription necessary) + */ + scheme = rb_hash_aref(env, g_http_x_forwarded_ssl); + if (!NIL_P(scheme) && STR_CSTR_EQ(scheme, "on")) { + *server_port = g_port_443; + scheme = g_https; } else { - scheme = rb_hash_aref(env, g_http_x_forwarded_ssl); - if (!NIL_P(scheme) && STR_CSTR_EQ(scheme, "on")) { - *server_port = g_port_443; - scheme = g_https; + scheme = rb_hash_aref(env, g_http_x_forwarded_proto); + if (NIL_P(scheme)) { + scheme = g_http; } else { - scheme = rb_hash_aref(env, g_http_x_forwarded_proto); - if (NIL_P(scheme)) { - scheme = g_http; + long len = RSTRING_LEN(scheme); + if (len >= 5 && !memcmp(RSTRING_PTR(scheme), "https", 5)) { + if (len != 5) + scheme = g_https; + *server_port = g_port_443; } else { - long len = RSTRING_LEN(scheme); - if (len >= 5 && !memcmp(RSTRING_PTR(scheme), "https", 5)) { - if (len != 5) - scheme = g_https; - *server_port = g_port_443; - } else { - scheme = g_http; - } + scheme = g_http; } } } @@ -1018,8 +990,6 @@ void Init_unicorn_http(void) rb_define_singleton_method(cHttpParser, "keepalive_requests", ka_req, 0); rb_define_singleton_method(cHttpParser, "keepalive_requests=", set_ka_req, 1); - rb_define_singleton_method(cHttpParser, "trust_x_forwarded=", set_xftrust, 1); - rb_define_singleton_method(cHttpParser, "trust_x_forwarded?", xftrust, 0); rb_define_singleton_method(cHttpParser, "max_header_len=", set_maxhdrlen, 1); init_common_fields(); diff --git a/lib/unicorn/configurator.rb b/lib/unicorn/configurator.rb index 9406223..5962418 100644 --- a/lib/unicorn/configurator.rb +++ b/lib/unicorn/configurator.rb @@ -48,7 +48,6 @@ class Unicorn::Configurator :check_client_connection => false, :rewindable_input => true, # for Rack 2.x: (Rack::VERSION[0] <= 1), :client_body_buffer_size => Unicorn::Const::MAX_BODY, - :trust_x_forwarded => true, } #:startdoc: @@ -558,18 +557,6 @@ class Unicorn::Configurator set[:user] = [ user, group ] end - # Sets whether or not the parser will trust X-Forwarded-Proto and - # X-Forwarded-SSL headers and set "rack.url_scheme" to "https" accordingly. - # Rainbows!/Zbatery installations facing untrusted clients directly - # should set this to +false+. This is +true+ by default as Unicorn - # is designed to only sit behind trusted nginx proxies. - # - # This has never been publically documented and is subject to removal - # in future releases. - def trust_x_forwarded(bool) # :nodoc: - set_bool(:trust_x_forwarded, bool) - end - # expands "unix:path/to/foo" to a socket relative to the current path # expands pathnames of sockets if relative to "~" or "~username" # expands "*:port and ":port" to "0.0.0.0:port" diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb index a0ca302..819a0a8 100644 --- a/lib/unicorn/http_server.rb +++ b/lib/unicorn/http_server.rb @@ -369,14 +369,6 @@ class Unicorn::HttpServer Unicorn::TeeInput.client_body_buffer_size = bytes end - def trust_x_forwarded - Unicorn::HttpParser.trust_x_forwarded? - end - - def trust_x_forwarded=(bool) - Unicorn::HttpParser.trust_x_forwarded = bool - end - def check_client_connection Unicorn::HttpRequest.check_client_connection end diff --git a/t/t0016-trust-x-forwarded-false.sh b/t/t0016-trust-x-forwarded-false.sh deleted file mode 100755 index 3163690..0000000 --- a/t/t0016-trust-x-forwarded-false.sh +++ /dev/null @@ -1,30 +0,0 @@ -#!/bin/sh -. ./test-lib.sh -t_plan 5 "trust_x_forwarded=false configuration test" - -t_begin "setup and start" && { - unicorn_setup - echo "trust_x_forwarded false" >> $unicorn_config - unicorn -D -c $unicorn_config env.ru - unicorn_wait_start -} - -t_begin "spoofed request with X-Forwarded-Proto does not trigger" && { - curl -H 'X-Forwarded-Proto: https' http://$listen/ | \ - grep -F '"rack.url_scheme"=>"http"' -} - -t_begin "spoofed request with X-Forwarded-SSL does not trigger" && { - curl -H 'X-Forwarded-SSL: on' http://$listen/ | \ - grep -F '"rack.url_scheme"=>"http"' -} - -t_begin "killing succeeds" && { - kill $unicorn_pid -} - -t_begin "check stderr has no errors" && { - check_stderr -} - -t_done diff --git a/t/t0017-trust-x-forwarded-true.sh b/t/t0017-trust-x-forwarded-true.sh deleted file mode 100755 index 11103c5..0000000 --- a/t/t0017-trust-x-forwarded-true.sh +++ /dev/null @@ -1,30 +0,0 @@ -#!/bin/sh -. ./test-lib.sh -t_plan 5 "trust_x_forwarded=true configuration test" - -t_begin "setup and start" && { - unicorn_setup - echo "trust_x_forwarded true " >> $unicorn_config - unicorn -D -c $unicorn_config env.ru - unicorn_wait_start -} - -t_begin "spoofed request with X-Forwarded-Proto sets 'https'" && { - curl -H 'X-Forwarded-Proto: https' http://$listen/ | \ - grep -F '"rack.url_scheme"=>"https"' -} - -t_begin "spoofed request with X-Forwarded-SSL sets 'https'" && { - curl -H 'X-Forwarded-SSL: on' http://$listen/ | \ - grep -F '"rack.url_scheme"=>"https"' -} - -t_begin "killing succeeds" && { - kill $unicorn_pid -} - -t_begin "check stderr has no errors" && { - check_stderr -} - -t_done diff --git a/test/unit/test_http_parser_xftrust.rb b/test/unit/test_http_parser_xftrust.rb deleted file mode 100644 index db8cfa9..0000000 --- a/test/unit/test_http_parser_xftrust.rb +++ /dev/null @@ -1,38 +0,0 @@ -# -*- encoding: binary -*- -require 'test/test_helper' - -include Unicorn - -class HttpParserXFTrustTest < Test::Unit::TestCase - def setup - assert HttpParser.trust_x_forwarded? - end - - def test_xf_trust_false_xfp - HttpParser.trust_x_forwarded = false - parser = HttpParser.new - parser.buf << "GET / HTTP/1.1\r\nHost: foo:\r\n" \ - "X-Forwarded-Proto: https\r\n\r\n" - env = parser.parse - assert_kind_of Hash, env - assert_equal 'foo', env['SERVER_NAME'] - assert_equal '80', env['SERVER_PORT'] - assert_equal 'http', env['rack.url_scheme'] - end - - def test_xf_trust_false_xfs - HttpParser.trust_x_forwarded = false - parser = HttpParser.new - parser.buf << "GET / HTTP/1.1\r\nHost: foo:\r\n" \ - "X-Forwarded-SSL: on\r\n\r\n" - env = parser.parse - assert_kind_of Hash, env - assert_equal 'foo', env['SERVER_NAME'] - assert_equal '80', env['SERVER_PORT'] - assert_equal 'http', env['rack.url_scheme'] - end - - def teardown - HttpParser.trust_x_forwarded = true - end -end -- cgit v1.2.3-24-ge0c7