fix: Support tcp connection rejection & rate-limiting when Proxy protocol is enabled

acceptance-tests/access_control_test.go

@@ -140,4 +140,42 @@ var _ = Describe("Access Control", func() {
 		By("Allowing TCP connections from non-blocklisted CIDRs (request from 127.0.0.1 on HAProxy VM)")
 		expectTestServer200(http.Get("http://127.0.0.1:11000"))
 	})
+
+	It("Rejects IPs in TCP-layer blocklisted CIDRs when proxy protocol is enabled", func() {
+		haproxyBackendPort := 12000
+
+		opsfileTCPBlocklistWithProxyProtocol := opsfileTCPBlocklist + `
+# Enable Proxy Protocol
+- type: replace
+  path: /instance_groups/name=haproxy/jobs/name=haproxy/properties/ha_proxy/accept_proxy?
+  value: true
+`
+		haproxyInfo, _ := deployHAProxy(baseManifestVars{
+			haproxyBackendPort:    haproxyBackendPort,
+			haproxyBackendServers: []string{"127.0.0.1"},
+			deploymentName:        deploymentNameForTestNode(),
+		}, []string{opsfileTCPBlocklistWithProxyProtocol}, map[string]interface{}{
+			// traffic from test runner appears to come from this CIDR block
+			"cidr_blocklist_tcp": []string{"10.1.1.1/32"},
+		}, true)
+
+		closeLocalServer, localPort := startDefaultTestServer()
+		defer closeLocalServer()
+
+		closeBackendTunnel := setupTunnelFromHaproxyToTestServer(haproxyInfo, haproxyBackendPort, localPort)
+		defer closeBackendTunnel()
+
+		By("Denying TCP connections from blocklisted CIDRs via Proxy Protocol header (source IP 10.1.1.1)")
+		// Send a request with a Proxy Protocol header advertising a source IP in the blocked 10.0.0.0/8 CIDR.
+		// With accept_proxy enabled, HAProxy parses the PROXY header and uses the advertised source IP
+		// for the tcp-request session phase ACL evaluation — so this must be rejected.
+		err := performProxyProtocolRequestWithSourceIP(haproxyInfo.PublicIP, 80, "/", "10.1.1.1")
+		Expect(err).To(HaveOccurred())
+		Expect(errors.Is(err, syscall.ECONNRESET)).To(BeTrue())
+
+		By("Allowing TCP connections from non-blocklisted CIDRs via Proxy Protocol header (source IP 192.168.1.1)")
+		// Send a request with a Proxy Protocol header advertising a source IP outside the blocked CIDR.
+		err = performProxyProtocolRequestWithSourceIP(haproxyInfo.PublicIP, 80, "/", "192.168.1.1")
+		Expect(err).NotTo(HaveOccurred())
+	})
 })

acceptance-tests/proxy_protocol_test.go

@@ -138,6 +138,10 @@ var _ = Describe("Proxy Protocol", func() {
 })
 
 func performProxyProtocolRequest(ip string, port int, endpoint string) error {
+	return performProxyProtocolRequestWithSourceIP(ip, port, endpoint, "10.1.1.1")
+}
+
+func performProxyProtocolRequestWithSourceIP(ip string, port int, endpoint string, sourceIP string) error {
 	// Create a connection to the HAProxy instance
 	conn, err := net.Dial("tcp", net.JoinHostPort(ip, strconv.Itoa(port)))
 	if err != nil {
@@ -152,7 +156,7 @@ func performProxyProtocolRequest(ip string, port int, endpoint string) error {
 		Command:           proxyproto.PROXY,
 		TransportProtocol: proxyproto.TCPv4,
 		SourceAddr: &net.TCPAddr{
-			IP:   net.ParseIP("10.1.1.1"),
+			IP:   net.ParseIP(sourceIP),
 			Port: 1000,
 		},
 		DestinationAddr: &net.TCPAddr{

acceptance-tests/rate_limit_test.go

@@ -165,6 +165,61 @@ var _ = Describe("Rate-Limiting", func() {
 		Expect(successfulRequestCount).To(Equal(connLimit))
 	})
 
+	It("Connection Based Limiting Works with Proxy Protocol enabled", func() {
+		connLimit := 5
+		opsfileConnRateLimitWithProxyProtocol := fmt.Sprintf(`---
+# Enable Proxy Protocol
+- type: replace
+  path: /instance_groups/name=haproxy/jobs/name=haproxy/properties/ha_proxy/accept_proxy?
+  value: true
+- type: replace
+  path: /instance_groups/name=haproxy/jobs/name=haproxy/properties/ha_proxy/connections_rate_limit?/connections
+  value: %d
+- type: replace
+  path: /instance_groups/name=haproxy/jobs/name=haproxy/properties/ha_proxy/connections_rate_limit/window_size?
+  value: 100s
+- type: replace
+  path: /instance_groups/name=haproxy/jobs/name=haproxy/properties/ha_proxy/connections_rate_limit/table_size?
+  value: 100
+- type: replace
+  path: /instance_groups/name=haproxy/jobs/name=haproxy/properties/ha_proxy/connections_rate_limit/block?
+  value: true
+`, connLimit)
+
+		haproxyBackendPort := 12000
+		haproxyInfo, _ := deployHAProxy(baseManifestVars{
+			haproxyBackendPort:    haproxyBackendPort,
+			haproxyBackendServers: []string{"127.0.0.1"},
+			deploymentName:        deploymentNameForTestNode(),
+		}, []string{opsfileConnRateLimitWithProxyProtocol}, map[string]interface{}{}, true)
+
+		closeLocalServer, localPort := startDefaultTestServer()
+		defer closeLocalServer()
+
+		closeTunnel := setupTunnelFromHaproxyToTestServer(haproxyInfo, haproxyBackendPort, localPort)
+		defer closeTunnel()
+
+		By("Sending requests via Proxy Protocol, each on a new TCP connection, expecting connection rate limiting to apply")
+		testRequestCount := int(float64(connLimit) * 1.5)
+		firstFailure := -1
+		successfulRequestCount := 0
+		for i := 0; i < testRequestCount; i++ {
+			err := performProxyProtocolRequest(haproxyInfo.PublicIP, 80, "/foo")
+			if err == nil {
+				successfulRequestCount++
+			} else {
+				if firstFailure == -1 {
+					firstFailure = i
+				}
+			}
+		}
+
+		By("The first connection should fail after we've sent the amount of connections specified in the Connection Rate Limit")
+		Expect(firstFailure).To(Equal(connLimit))
+		By("The total amount of successful connections per time window should equal the Connection Rate Limit")
+		Expect(successfulRequestCount).To(Equal(connLimit))
+	})
+
 	It("Both types of rate limiting work in parallel", func() {
 		requestLimit := 5
 		connLimit := 6 // needs to be higher than request limit for this test

jobs/haproxy/templates/haproxy.config.erb

@@ -39,6 +39,11 @@ tcp_accept_proxy = accept_proxy
 if p("ha_proxy.disable_tcp_accept_proxy")
   tcp_accept_proxy = ""
 end
+# When proxy protocol is active, the real client IP is only known after the
+# proxy header is parsed (session phase), so reject must use tcp-request session.
+# Without proxy protocol, tcp-request connection is sufficient.
+tcp_request_phase = p("ha_proxy.accept_proxy") ? "session" : "connection"
+
 # }}}
 # Global SSL Flags {{{
 ssl_flags = ""
@@ -429,12 +434,12 @@ frontend http-in
     <%= format_indented_multiline_config(p("ha_proxy.frontend_config")) %>
   <%- end -%>
     acl layer4_block src -f /var/vcap/jobs/haproxy/config/blocklist_cidrs_tcp.txt
-    tcp-request connection reject if layer4_block
+    tcp-request <%= tcp_request_phase %> reject if layer4_block
   <%- if_p("ha_proxy.connections_rate_limit.table_size", "ha_proxy.connections_rate_limit.window_size") do -%>
-    tcp-request connection track-sc0 src table st_tcp_conn_rate
+    tcp-request <%= tcp_request_phase %> track-sc0 src table st_tcp_conn_rate
     <%- if_p("ha_proxy.connections_rate_limit.block", "ha_proxy.connections_rate_limit.connections") do |block, connections| -%>
       <%-if block -%>
-    tcp-request connection reject if { sc_conn_rate(0) gt <%= connections %> }
+    tcp-request <%= tcp_request_phase %> reject if { sc_conn_rate(0) gt <%= connections %> }
       <%- end -%>
     <%- end -%>
   <%- end -%>
@@ -563,12 +568,12 @@ frontend https-in
     <%= format_indented_multiline_config(p("ha_proxy.frontend_config")) %>
   <%- end -%>
     acl layer4_block src -f /var/vcap/jobs/haproxy/config/blocklist_cidrs_tcp.txt
-    tcp-request connection reject if layer4_block
+    tcp-request <%= tcp_request_phase %> reject if layer4_block
   <%- if_p("ha_proxy.connections_rate_limit.table_size", "ha_proxy.connections_rate_limit.window_size") do -%>
-    tcp-request connection track-sc0 src table st_tcp_conn_rate
+    tcp-request <%= tcp_request_phase %> track-sc0 src table st_tcp_conn_rate
     <%- if_p("ha_proxy.connections_rate_limit.block", "ha_proxy.connections_rate_limit.connections") do |block, connections| -%>
       <%-if block -%>
-    tcp-request connection reject if { sc_conn_rate(0) gt <%= connections %> }
+    tcp-request <%= tcp_request_phase %> reject if { sc_conn_rate(0) gt <%= connections %> }
       <%- end -%>
     <%- end -%>
   <%- end -%>

spec/haproxy/templates/haproxy_config/frontend_http_spec.rb

@@ -153,6 +153,41 @@
         expect(frontend_http).to include('tcp-request connection reject if layer4_block')
       end
     end
+
+    context 'when ha_proxy.accept_proxy set to true' do
+      context 'when ha_proxy.cidr_blocklist_tcp is provided' do
+        let(:properties) do
+          { 'accept_proxy' => true, 'cidr_blocklist_tcp' => ['172.168.4.1/32', '10.2.0.0/16'] }
+        end
+
+        it 'sets the correct acl and connection reject rules' do
+          expect(frontend_http).to include('acl layer4_block src -f /var/vcap/jobs/haproxy/config/blocklist_cidrs_tcp.txt')
+          expect(frontend_http).to include('tcp-request session reject if layer4_block')
+        end
+      end
+
+      context 'when ha_proxy.cidr_blocklist_tcp is not provided' do
+        let(:properties) do
+          { 'accept_proxy' => true }
+        end
+
+        it 'still includes the layer4_block acl and reject rule (always present)' do
+          expect(frontend_http).to include('acl layer4_block src -f /var/vcap/jobs/haproxy/config/blocklist_cidrs_tcp.txt')
+          expect(frontend_http).to include('tcp-request session reject if layer4_block')
+        end
+      end
+
+      context 'when ha_proxy.cidr_blocklist_tcp is an empty array' do
+        let(:properties) do
+          { 'accept_proxy' => true, 'cidr_blocklist_tcp' => [] }
+        end
+
+        it 'still includes the layer4_block acl and reject rule (always present)' do
+          expect(frontend_http).to include('acl layer4_block src -f /var/vcap/jobs/haproxy/config/blocklist_cidrs_tcp.txt')
+          expect(frontend_http).to include('tcp-request session reject if layer4_block')
+        end
+      end
+    end
   end
 
   context 'when ha_proxy.block_all is provided' do

spec/haproxy/templates/haproxy_config/frontend_https_spec.rb

@@ -585,6 +585,41 @@
         expect(frontend_https).to include('tcp-request connection reject if layer4_block')
       end
     end
+
+    context 'when ha_proxy.accept_proxy set to true' do
+      context 'when ha_proxy.cidr_blocklist_tcp is provided' do
+        let(:properties) do
+          default_properties.merge({ 'accept_proxy' => true, 'cidr_blocklist_tcp' => ['172.168.4.1/32', '10.2.0.0/16'] })
+        end
+
+        it 'sets the correct acl and connection reject rules' do
+          expect(frontend_https).to include('acl layer4_block src -f /var/vcap/jobs/haproxy/config/blocklist_cidrs_tcp.txt')
+          expect(frontend_https).to include('tcp-request session reject if layer4_block')
+        end
+      end
+
+      context 'when ha_proxy.cidr_blocklist_tcp is not provided' do
+        let(:properties) do
+          default_properties.merge({ 'accept_proxy' => true })
+        end
+
+        it 'still includes the layer4_block acl and reject rule (always present)' do
+          expect(frontend_https).to include('acl layer4_block src -f /var/vcap/jobs/haproxy/config/blocklist_cidrs_tcp.txt')
+          expect(frontend_https).to include('tcp-request session reject if layer4_block')
+        end
+      end
+
+      context 'when ha_proxy.cidr_blocklist_tcp is an empty array' do
+        let(:properties) do
+          default_properties.merge({ 'accept_proxy' => true, 'cidr_blocklist_tcp' => [] })
+        end
+
+        it 'still includes the layer4_block acl and reject rule (always present)' do
+          expect(frontend_https).to include('acl layer4_block src -f /var/vcap/jobs/haproxy/config/blocklist_cidrs_tcp.txt')
+          expect(frontend_https).to include('tcp-request session reject if layer4_block')
+        end
+      end
+    end
   end
 
   context 'when ha_proxy.block_all is provided' do

spec/haproxy/templates/haproxy_config/rate_limit_spec.rb

@@ -88,17 +88,41 @@
       expect(frontend_https).to include('tcp-request connection track-sc0 src table st_tcp_conn_rate')
     end
 
+    context 'when proxy protocol used' do
+      let(:properties) do
+        temp_properties.deep_merge({ 'accept_proxy' => true })
+      end
+
+      it 'tracks connections in stick tables' do
+        expect(frontend_http).to include('tcp-request session track-sc0 src table st_tcp_conn_rate')
+        expect(frontend_https).to include('tcp-request session track-sc0 src table st_tcp_conn_rate')
+      end
+    end
+
     context 'when "connections" and "block" are also provided' do
       let(:properties) do
         temp_properties.deep_merge({ 'connections_rate_limit' => { 'connections' => '5', 'block' => 'true' } })
       end
 
-      it 'adds http-request deny condition to http-in and https-in frontends' do
+      it 'adds tcp-request connection reject to http-in and https-in frontends' do
         expect(frontend_http).to include('tcp-request connection reject if { sc_conn_rate(0) gt 5 }')
         expect(frontend_http).to include('tcp-request connection track-sc0 src table st_tcp_conn_rate')
         expect(frontend_https).to include('tcp-request connection reject if { sc_conn_rate(0) gt 5 }')
         expect(frontend_https).to include('tcp-request connection track-sc0 src table st_tcp_conn_rate')
       end
     end
+
+    context 'when proxy protocol used and "connections" and "block" are also provided' do
+      let(:properties) do
+        temp_properties.deep_merge({ 'accept_proxy' => true, 'connections_rate_limit' => { 'connections' => '5', 'block' => 'true' } })
+      end
+
+      it 'adds tcp-request session reject to http-in and https-in frontends' do
+        expect(frontend_http).to include('tcp-request session reject if { sc_conn_rate(0) gt 5 }')
+        expect(frontend_http).to include('tcp-request session track-sc0 src table st_tcp_conn_rate')
+        expect(frontend_https).to include('tcp-request session reject if { sc_conn_rate(0) gt 5 }')
+        expect(frontend_https).to include('tcp-request session track-sc0 src table st_tcp_conn_rate')
+      end
+    end
   end
 end