After looking at this in more detail, I think the current behavior is actually correct for situations with 1 LB layer. In your case, where you effectively have N layers of LBs/proxies, you want to take the Nth-from-last element.
The behavior of AWS ELBs, at least, is to take any
X-Forwarded-For header they receive and add
, <actual address this request came from> to the end before forwarding the request.
This is actually consistent with the MDN page, I think that page is just written in a slightly misleading way. It says this:
X-Forwarded-For: <client>, <proxy1>, <proxy2>
…but this assumes all actors are trustworthy. Another way to describe the header is:
X-Forwarded-For: <lies the client made up>, <lies the device closest to the client made up>, <... maybe more lies ...>, <lies the device closest to the server made up>, <lies the device the device adjacent to the server made up>
That is, the client may send an
X-Forwarded-For header with an arbitrary number of made-up elements, and each trustworthy proxy will forward it unmodified, appending real information. When you parse this header, you should take the last element if you trust 1 adjacent device; the second-to-last element if you trust 2 adjacent devices; the third-to-last element if you trust 3 adjacent devices; and so on.
In a setup which uses
reset() instead of
end(), I believe you can spoof any client IP by making a request like this:
$ curl -H 'X-Forwarded-For: 126.96.36.199' https://phabricator.example.com/
I think you actually want to select the Nth-to-last element, e.g. something in this vein:
// Peel off the K8 virtual LB.
// Peel off the container network device layer.
// Peel off the hypervirtual metacontainer indirector.
// Walking backward from the point of termination, we've pierced
// through all the virtualization layers and the next address is
// a real client device.
$forwarded_for = end($forwarded_for);
I plan to simplify getting this right by providing some kind of
preamble_trust_x_forwarded_for($layers = 1) function, since this use case is broadly reasonable and if the original snippet was incorrect the current setup would make it difficult to guide installs on fixing.