Merge pull request #788 from element-hq/bbz/fix-x-forwarded-for-handling

Fix `X-Forwarded-For` handling with multiple values

Author: benbz

.github/element-docs-words.txt

@@ -8,8 +8,8 @@ dockerhub
 Gematik
 GHSA
 homeserver
-homeservers
 Homeserver
+homeservers
 Jetstack
 kubeconform
 kubernetes
@@ -22,6 +22,7 @@ postgres
 PostgreSQL
 pytest
 Pytest
+Referer
 SCIM
 shellcheck
 templatable

charts/matrix-stack/configs/haproxy/haproxy.cfg.tpl

@@ -26,6 +26,14 @@ defaults
 
   log global
 
+  # The Ingress Controller should appropriately set an X-Forwarded-For header
+  # We leave it alone if it has, but add in the source address in cases where it hasn't
+  # or the request hasn't come from the ingress controller (i.e. in-cluster)
+  option forwardfor if-none
+
+  # Set the RFC7239 `Forwarded` header
+  option forwarded
+
   # wait for 5s when connecting to a server
   timeout connect 5s
 
@@ -88,9 +96,9 @@ frontend http-blackhole
   # same as http log, with %Th (handshake time)
   log-format "%ci:%cp [%tr] %ft %b/%s %Th/%TR/%Tw/%Tc/%Tr/%Ta %ST %B %CC %CS %tsc %ac/%fc/%bc/%sc/%rc %sq/%bq %hr %hs %{+Q}r"
 
-  capture request header Host len 32
-  capture request header Referer len 200
-  capture request header User-Agent len 200
+  http-request capture hdr(host) len 32
+  http-request capture req.fhdr(x-forwarded-for) len 64
+  http-request capture req.fhdr(user-agent) len 200
 
   http-request deny content-type application/json string '{"errcode": "M_FORBIDDEN", "error": "Blocked"}'
 

charts/matrix-stack/configs/synapse/partial-haproxy.cfg.tpl

@@ -22,9 +22,6 @@ frontend startup
 frontend synapse-http-in
   bind *:8008
 
-  # same as http log, with %Th (handshake time)
-  log-format "%ci:%cp [%tr] %ft %b/%s %Th/%TR/%Tw/%Tc/%Tr/%Ta %ST %B %CC %CS %tsc %ac/%fc/%bc/%sc/%rc %sq/%bq %hr %hs %{+Q}r"
-
   # if we hit the maxconn on a server, and the queue timeout expires, we want
   # to avoid returning 503, since that will cause cloudflare to mark us down.
   #
@@ -35,24 +32,12 @@ frontend synapse-http-in
   #
   errorfile 503 /synapse/429.http
 
-  capture request header Host len 32
-  capture request header Referer len 200
-  capture request header User-Agent len 200
-
-  # before we change the 'src', stash it in a session variable
-  http-request set-var(sess.orig_src) src if !{ var(sess.orig_src) -m found }
-
-  # in case this is not the first request on the connection, restore the
-  # 'src' to the original, in case we fail to parse the x-f-f header.
-  http-request set-src var(sess.orig_src)
-
-  # Traditionally do this only for traffic from some limited IP addreses
-  # but the incoming router being what it is, means we have no fixed IP here.
-  http-request set-src hdr(x-forwarded-for)
+  # same as http log, with %Th (handshake time)
+  log-format "%ci:%cp [%tr] %ft %b/%s %Th/%TR/%Tw/%Tc/%Tr/%Ta %ST %B %CC %CS %tsc %ac/%fc/%bc/%sc/%rc %sq/%bq %hr %hs %{+Q}r"
 
-  # We always add a X-Forwarded-For header (clobbering any existing
-  # headers).
-  http-request set-header X-Forwarded-For %[src]
+  http-request capture hdr(host) len 32
+  http-request capture req.fhdr(x-forwarded-for) len 64
+  http-request capture req.fhdr(user-agent) len 200
 
   # Ingresses by definition run on both 80 & 443 and there's no customising of that
   # It is up to the ingress controller and any annotations provided to it whether

charts/matrix-stack/configs/well-known/partial-haproxy.cfg.tpl

@@ -13,6 +13,10 @@ frontend well-known-in
   # same as http log, with %Th (handshake time)
   log-format "%ci:%cp [%tr] %ft %b/%s %Th/%TR/%Tw/%Tc/%Tr/%Ta %ST %B %CC %CS %tsc %ac/%fc/%bc/%sc/%rc %sq/%bq %hr %hs %{+Q}r"
 
+  http-request capture hdr(host) len 32
+  http-request capture req.fhdr(x-forwarded-for) len 64
+  http-request capture req.fhdr(user-agent) len 200
+
   acl is_delete_put_post_method method DELETE POST PUT
   http-request deny status 405 if is_delete_put_post_method
 

newsfragments/788.changed.1.md

@@ -0,0 +1 @@
+Log the X-Forwarded-For header and stop logging the Referer header in HAProxy.

newsfragments/788.changed.2.md

@@ -0,0 +1,7 @@
+Correct the handling of multiple X-Forwarded-For headers to Synapse.
+
+This may have exhibit itself as requests being incorrectly rate-limited by Synapse.
+
+The source IP logged by HAProxy is now always the IP connecting to HAProxy rather than
+a value extracted from the X-Forwarded-For header (if present). This is usually an IP
+for the ingress controller.

newsfragments/788.changed.md

@@ -0,0 +1 @@
+Ensure consistent captured headers in HAProxy log lines, between all HTTP request processing HAProxy frontends.