Strip and re-author forwarding headers in Falcon::Middleware::Proxy

lib/falcon/middleware/proxy.rb

@@ -38,6 +38,34 @@ class Proxy < Protocol::HTTP::Middleware
 			VIA = "via"
 			CONNECTION = "connection"
 
+			# Forwarding headers which carry trust-sensitive information about the
+			# original client (their address and the request scheme). Because Falcon
+			# acts as the trust boundary, any client-supplied values are untrustworthy,
+			# so we strip every inbound forwarding header and author our own below from
+			# connection-level facts.
+			#
+			# We emit both the modern RFC 7239 {FORWARDED} header and the legacy
+			# `x-forwarded-for` / `x-forwarded-proto` headers, because many downstream
+			# consumers still read the legacy ones. Notably Rack's
+			# `Rack::Request#forwarded_for` (used by Rails' `ActionDispatch::RemoteIp`)
+			# only prefers `Forwarded` and falls back to `X-Forwarded-For`; older Rack
+			# (< 3) and a lot of application code read `X-Forwarded-For` directly.
+			#
+			# We also strip underscore variants of the same header names because Rack
+			# normalizes both `x-forwarded-for` and `x_forwarded_for` to the same
+			# `HTTP_X_FORWARDED_FOR` CGI environment key.
+			FORWARDING_HEADERS = [
+				FORWARDED,
+				X_FORWARDED_FOR,
+				X_FORWARDED_PROTO,
+				"x-forwarded-host",
+				"x-forwarded-port",
+				"x_forwarded_for",
+				"x_forwarded_proto",
+				"x_forwarded_host",
+				"x_forwarded_port",
+			]
+
 			# HTTP hop headers which *should* not be passed through the proxy.
 			HOP_HEADERS = [
 				"connection",
@@ -101,10 +129,13 @@ def prepare_headers(headers)
 			end
 
 			# Prepare the request to be proxied to the specified host.
-			# In particular, we set appropriate {VIA}, {FORWARDED}, {X_FORWARDED_FOR} and {X_FORWARDED_PROTO} headers.
+			#
+			# Falcon acts as the trust boundary, so we strip any client-supplied
+			# {FORWARDING_HEADERS} and author our own from connection-level facts: the
+			# RFC 7239 {FORWARDED} header plus the legacy {X_FORWARDED_FOR} /
+			# {X_FORWARDED_PROTO} headers, along with an appended {VIA} header. This
+			# prevents a client from spoofing the forwarded address or scheme.
 			def prepare_request(request, host)
-				forwarded = []
-
 				Console.debug(self) do |buffer|
 					buffer.puts "Request authority: #{request.authority}"
 					buffer.puts "Host authority: #{host.authority}"
@@ -115,9 +146,17 @@ def prepare_request(request, host)
 				# The authority of the request must match the authority of the endpoint we are proxying to, otherwise SNI and other things won't work correctly.
 				request.authority = host.authority
 
+				# Discard inbound hop-by-hop headers before authoring trusted forwarding
+				# headers, so client-supplied Connection tokens can't remove headers we add.
+				self.prepare_headers(request.headers)
+				# Discard any inbound forwarding headers so a client can't spoof them; we author our own below from connection-level facts.
+				request.headers.extract(FORWARDING_HEADERS)
+
+				forwarded = []
+
 				if address = request.remote_address
 					request.headers.add(X_FORWARDED_FOR, address.ip_address)
-					forwarded << "for=#{address.ip_address}"
+					forwarded << "for=#{forwarded_node(address)}"
 				end
 
 				if scheme = request.scheme
@@ -131,11 +170,21 @@ def prepare_request(request, host)
 
 				request.headers.add(VIA, "#{request.version} #{self.class}")
 
-				self.prepare_headers(request.headers)
-
 				return request
 			end
 
+			# Format a remote address as an RFC 7239 `for=` node identifier.
+			# IPv6 addresses must be enclosed in square brackets and quoted.
+			# @parameter address [Addrinfo] The remote address of the client.
+			# @returns [String] The node identifier for use in a {FORWARDED} header.
+			def forwarded_node(address)
+				if address.ipv6?
+					"\"[#{address.ip_address}]\""
+				else
+					address.ip_address
+				end
+			end
+
 			# Proxy the request if the authority matches a specific host.
 			# @parameter request [Protocol::HTTP::Request]
 			def call(request)

test/falcon/middleware/proxy.rb

@@ -27,6 +27,8 @@ def proxy_for(**options)
 
 	let(:headers) {Protocol::HTTP::Headers["accept" => "*/*"]}
 
+	let(:host) {proxy_for(authority: "www.google.com", endpoint: Async::HTTP::Endpoint.parse("https://www.google.com"))}
+
 	it "removes proxy authorization by default" do
 		headers = Protocol::HTTP::Headers[
 			"authorization" => "Bearer application",
@@ -47,11 +49,106 @@ def proxy_for(**options)
 
 		expect(response).not.to be(:failure?)
 
+		expect(request.headers["forwarded"]).to be == ["for=127.0.0.1;proto=https"]
 		expect(request.headers["x-forwarded-for"]).to be == ["127.0.0.1"]
 
 		proxy.close
 	end
 
+	it "authors a forwarded header from the connection" do
+		request = Protocol::HTTP::Request.new("https", "www.google.com", "GET", "/", nil, headers, nil)
+		expect(request).to receive(:remote_address).and_return(Addrinfo.ip("127.0.0.1"))
+
+		proxy.prepare_request(request, host)
+
+		expect(request.headers["forwarded"]).to be == ["for=127.0.0.1;proto=https"]
+		expect(request.headers["x-forwarded-for"]).to be == ["127.0.0.1"]
+		expect(request.headers["x-forwarded-proto"]).to be == ["https"]
+		expect(request.headers["via"]).not.to be_nil
+	end
+
+	it "strips client-supplied forwarding headers so they can't be spoofed" do
+		headers = Protocol::HTTP::Headers[
+			"x-forwarded-for" => "1.2.3.4",
+			"x-forwarded-proto" => "https",
+			"x-forwarded-host" => "evil.example.com",
+			"x-forwarded-port" => "8443",
+			"forwarded" => "for=9.9.9.9;proto=https",
+		]
+		request = Protocol::HTTP::Request.new("http", "www.google.com", "GET", "/", nil, headers, nil)
+		expect(request).to receive(:remote_address).and_return(Addrinfo.ip("127.0.0.1"))
+
+		proxy.prepare_request(request, host)
+
+		# The spoofed values are stripped and replaced with Falcon's own.
+		expect(request.headers["x-forwarded-for"]).to be == ["127.0.0.1"]
+		expect(request.headers["x-forwarded-proto"]).to be == ["http"]
+		expect(request.headers["forwarded"]).to be == ["for=127.0.0.1;proto=http"]
+
+		# `x-forwarded-host` and `x-forwarded-port` are stripped and not re-authored, so they can't be spoofed.
+		expect(request.headers["x-forwarded-host"]).to be_nil
+		expect(request.headers["x-forwarded-port"]).to be_nil
+	end
+
+	it "strips underscore forwarding headers that collide with rack environment keys" do
+		headers = Protocol::HTTP::Headers[
+			"x_forwarded_for" => "1.2.3.4",
+			"x_forwarded_proto" => "https",
+			"x_forwarded_host" => "evil.example.com",
+			"x_forwarded_port" => "8443",
+		]
+		request = Protocol::HTTP::Request.new("http", "www.google.com", "GET", "/", nil, headers, nil)
+		expect(request).to receive(:remote_address).and_return(Addrinfo.ip("127.0.0.1"))
+		proxy.prepare_request(request, host)
+		expect(request.headers["x_forwarded_for"]).to be_nil
+		expect(request.headers["x_forwarded_proto"]).to be_nil
+		expect(request.headers["x_forwarded_host"]).to be_nil
+		expect(request.headers["x_forwarded_port"]).to be_nil
+		expect(request.headers["x-forwarded-for"]).to be == ["127.0.0.1"]
+		expect(request.headers["x-forwarded-proto"]).to be == ["http"]
+		expect(request.headers["forwarded"]).to be == ["for=127.0.0.1;proto=http"]
+	end
+	it "doesn't let connection tokens strip authored forwarding headers" do
+		headers = Protocol::HTTP::Headers[
+			"connection" => "x-forwarded-for, x-forwarded-proto, forwarded, via",
+		]
+		request = Protocol::HTTP::Request.new("http", "www.google.com", "GET", "/", nil, headers, nil)
+		expect(request).to receive(:remote_address).and_return(Addrinfo.ip("127.0.0.1"))
+		proxy.prepare_request(request, host)
+		expect(request.headers["connection"]).to be_nil
+		expect(request.headers["x-forwarded-for"]).to be == ["127.0.0.1"]
+		expect(request.headers["x-forwarded-proto"]).to be == ["http"]
+		expect(request.headers["forwarded"]).to be == ["for=127.0.0.1;proto=http"]
+		expect(request.headers["via"]).not.to be_nil
+	end
+	it "formats IPv6 addresses according to RFC 7239" do
+		request = Protocol::HTTP::Request.new("https", "www.google.com", "GET", "/", nil, headers, nil)
+		expect(request).to receive(:remote_address).and_return(Addrinfo.ip("::1"))
+
+		proxy.prepare_request(request, host)
+
+		# RFC 7239 requires IPv6 to be bracketed and quoted in `Forwarded`...
+		expect(request.headers["forwarded"]).to be == ["for=\"[::1]\";proto=https"]
+		# ...but `X-Forwarded-For` carries the bare address.
+		expect(request.headers["x-forwarded-for"]).to be == ["::1"]
+	end
+
+	it "omits the client address when the remote address is unavailable" do
+		request = Protocol::HTTP::Request.new("https", "www.google.com", "GET", "/", nil, headers, nil)
+		expect(request).to receive(:remote_address).and_return(nil)
+
+		proxy.prepare_request(request, host)
+
+		# With no remote address there is nothing to author, so neither the legacy
+		# `x-forwarded-for` nor a `for=` element in `forwarded` is emitted.
+		expect(request.headers["x-forwarded-for"]).to be_nil
+		expect(request.headers["forwarded"]).to be == ["proto=https"]
+
+		# The scheme and via are still authored from connection-level facts.
+		expect(request.headers["x-forwarded-proto"]).to be == ["https"]
+		expect(request.headers["via"]).not.to be_nil
+	end
+
 	it "defers if no host is available" do
 		request = Protocol::HTTP::Request.new("www.groogle.com", "GET", "/", nil, headers, nil)