-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Good article on x-forwarded-for
parsing
#29
Comments
What a fantastic article! It speaks to my soul. 😭 I definitely want to link it in the docs, at least. Thanks so much for forwarding it! And thanks @adam-p for writing it. ❤️ It also gives me some food for thought. One thing the post calls out is
which is something I was feeling the last time I was doing significant work on this library: remote_ip/lib/remote_ip/options.ex Lines 33 to 63 in 4826fc4
There's definitely an upgrade path we could follow to get people to specify their headers explicitly. Plus, maybe replace the They also opened the issue at sebest/xff#15 pointing out that their list of private IPs is incomplete, which is strictly true for remote_ip as well (the list originally came from the Rails implementation). Migrating that might be more of a pain, though. |
You're welcome! I wanted the post to be useful to people just like you, so I'm super happy you found it. This is among the best libraries I've seen. I really liked your algorithm document. You've already found the post, so I'll only add a couple of things...
Do you treat the latter two like a comma-separated list? (I think you do, but I have never tried to read Elixir.) They shouldn't be, but I guess there's no big downside. If the user has configured the lib to use one of those headers then they shouldn't be spoofable (a trusted reverse proxy set it) and treating it like a list-of-one is fine, and if the header is spoofable, then treating it like a list is the least of the user's problems. (It'd be nice to give an error or something to indicate to the user that something is wrong, but... again, if it's spoofable then it probably won't be spoofed with comma-separated values. Why would an attacker bother doing that?) After writing the post I created a "reference implementation" in Go that you might be interested in: https://github.com/realclientip/realclientip-go I would really like to be able to point to implementations in other languages, so if yours ends up similar I'd be happy to add a stub realclientip-elixir repo to the organization that points to yours. (I mean, you'd also be welcome to move your repo under the org, but I doubt you will want to.) |
@ajvondrak @adam-p I don’t have time to look this over now, but I’d be happy to provide an analysis of both codebases to see where they differ. |
@ajvondrak, the short version is that What follows is mostly pulled from @adam-p's documentation and the implementation. I believe that it would be possible to do this with multiple plugs (for the use of single strategies). I have some thoughts about how this could be implemented, and it would depend on the Before I get into my thoughts, let me cover the strategies in
Here’s my thoughts on how this could work in Elixir:
This is extremely doable. |
Sorry for the tangent, but when you get a moment could you explain this to me:
The order of multiple XFF or Forwarded headers obviously matters (and surely doesn't ever get reordered, right?), but how does the order of other headers matter? (An answer I can see being possible is: Instead of looking through a list of headers names in order of preference, you go through the actual headers and take the first that matches something in the preference list. Is that happening?) |
That would need to be something that would be raised with the makers of Cowboy or other HTTP servers, unfortunately. It appears that Erlang maps are sorted by key, meaning that they are stable, but unordered. 1> maps:put(a, 2, #{z => 1}).
#{a => 2,z => 1} Keyword lists would be better (those are tuple lists), but would end up being a fairly major API change for Cowboy &c. But that’s only if the order of the headers matters. According to Cowboy documentation, RFC7230 3.2.2 indicates that the order of headers with different names does not matter. So, I would bet that Cowboy (but I can’t find the code offhand):
It might have some optimizations for headers that are only supposed to appear once (e.g., |
@adam-p Sort of. It's true that the Elixir's [
{"accept", "*/*"},
{"x-forwarded-for", "1.1.1.1, 2.2.2.2"},
{"host", "localhost:4000"},
{"x-client-ip", "3.3.3.3"},
{"user-agent", "curl/7.77.0"},
] We take this list and select just the headers whose names appear in the plug RemoteIp, headers: ["x-forwarded-for", "x-client-ip"] or equivalently plug RemoteIp, headers: ["x-client-ip", "x-forwarded-for"] Either way, we'd whittle the above [
{"x-forwarded-for", "1.1.1.1, 2.2.2.2"},
{"x-client-ip", "3.3.3.3"},
] Each header is parsed for a list of IPs, then those lists are concatenated together, still respecting their relative ordering. Thus: [
{1, 1, 1, 1},
{2, 2, 2, 2},
{3, 3, 3, 3},
] So, this list is taken to represent a network path like:
However, as I was trying to convey in the docs, there are several things "off" about this. Really, it's based on the faulty premise that the order of Elixir's
Then there's also the matter of how multiple headers with the same name get collapsed into comma-separated values. Suppose we had
This could come in to the [
{"x-forwarded-for", "1.1.1.1, 3.3.3.3"},
{"x-client-ip", "2.2.2.2"},
] or [
{"x-client-ip", "2.2.2.2"},
{"x-forwarded-for", "1.1.1.1, 3.3.3.3"},
] Neither order gives us a list of parsed IPs that goes Ultimately, if we can't rely on the relative ordering of separate headers, the I guess users could set their own priority after a fashion by calling the plugin multiple times: plug RemoteIp, headers: ["x-lo-priority"]
plug RemoteIp, headers: ["x-hi-priority"]
Although interactions might get weird here when skipping proxy IPs. Especially if the two plugins are configured with different values for their |
@adam-p Yes, and I followed pretty much the same line of reasoning, lol. However, now that v1.0 has custom parsers, it'd be pretty easy to make one that expects just a single IP and use that as the default for So many ideas flowing from this thread! 🤩 |
@halostatue Thanks for the thorough breakdown in #29 (comment) 💜 It'd be interesting to offer all these different strategies. This library was always pretty opinionated about the algorithm, although everything else around it has become super configurable. So I guess making the algorithm configurable is kind of the next frontier, lol. Definitely going to be thinking on this. Want to get my thoughts organized enough to start spawning some follow-up tickets. |
(I revisited your algorithm document just now and I see that the answer to my question was there. Sorry!) @ajvondrak IMO, putting the values from different headers into a single ordered list doesn't seem right. Even ignoring the random header ordering. That's just not what the different headers mean -- they're not meant to be a single order list altogether. However... If the user has configured a single header (which really is what they should do), then it's okay. If you're using a strictly rightmost algorithm AND the user's reverse proxies are replacing any headers that are configured to be looked at AND XFF is being appended properly AND For example:
You get one of these two lists:
In either case, the rightmost is the same. Now let's say the server is behind Cloudflare and one more internal reverse proxy:
Same result in either case: RightmostTrustedRangeStrategy Different result between cases: Strict rightmost, LeftmostNonPrivateStrategy, RightmostTrustedCountStrategy, RightmostNonPrivateStrategy (Of course, some of those aren't correct in this case no matter what. But the point is that they'll randomly give different answers.) So... the current approach is okay for most cases, but a) it feels weird to me, and b) it falls apart if you start handling different algorithms/strategies (and maybe if the reverse proxy configuration gets complex). |
https://adam-p.ca/blog/2022/03/x-forwarded-for/
I’m going to be looking at this in substantially more depth, but this is probably worth looking at for this library as well to see if it’s worth adding references to the documentation.
The text was updated successfully, but these errors were encountered: