the realip module and multiple X-Forwarded-For lines
marty.stitt at gmail.com
Thu Jul 17 18:58:53 MSD 2008
thank you very much for reviewing what i came up with. yes, my coding style
is very much like a guy who hasn't done anything with C since 1994, and who
wasn't trying or expecting to make something like an nginx-committer would
typical time pressures of my job and being a newbie to the internals of
On Wed, Jul 16, 2008 at 11:46 PM, Evan Miller <emmiller at gmail.com> wrote:
> Martin Stitt wrote:
>> we are currently using nginx v0.5.35 in our production environment.
>> fast, lean and stable. thanks!
>> and, we have run into one problem recently.
>> i've devised what i believe is a fix. the attached a patch file shows
>> what i've done. my hope is that it could be helpful to share this
>> patch, and that i might get some feedback, such as: "yes, your fix
>> seems workable", or "you've overlooked this whole other situation...".
>> i developed this patch against latest stable release, v0.6.32 and have
>> been running it on a staging machine. hoping to get it proofed enough
>> to move into production soon. it was designed to just minimally
>> attend to the specific problem i am having. i realize it's not a
>> general-coverage type of solution (more on this below).
>> our web server has recently had an appliance known as "netscaler" put
>> in front of it. an overview:
>> as a result, we now need to count on nginx's realip module to derive
>> the remote_addr value based on the X-Forwarded-For headers that come
>> through. we want a remote_addr value that reflects the client's ip
>> number (or their most outward proxy) as much as possible to make
>> logging meaningful, and so that we can derive location info through
>> the geo module.
>> some incoming HTTP headers have a single X-Forwarded-For line, with a
>> single value:
>> X-Forwarded-For: 184.108.40.206 <http://220.127.116.11>
>> or a single line with a set of comma separated values:
>> X-Forwarded-For: 18.104.22.168 <http://22.214.171.124>, 126.96.36.199 <
>> both of these cases are handled ok.
>> when multiple values exist the realip module always takes the
>> rightmost ip number. this seems to work ok based on the emperical
>> evidence i see from a tcpdump.
>> the problem comes in when a header contains multiple X-Forwarded-For
>> GET /stylesheets/search.css?1210024315 HTTP/1.0
>> Accept: */*
>> Referer: http://www.somewhere.com/com/usa/en/searches
>> Accept-Language: en-us
>> UA-CPU: x86
>> If-Modified-Since: Mon, 05 May 2008 21:51:55 GMT; length=4732
>> User-Agent: Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; .NET CLR
>> 1.1.4322; .NET CLR 2.0.50727; InfoPath.1; .NET CLR 3.0.04506.30; .NET CLR
>> Host: www.somewhere.com <http://www.somewhere.com>
>> X-Forwarded-For: 10.1.1.69 <http://10.1.1.69>
>> Cache-Control: max-age=259200
>> Connection: keep-alive
>> X-Forwarded-For: 188.8.131.52 <http://184.108.40.206>
>> the behavior i see in both v0.5.35 and v0.6.32 is that the value from
>> the first-encountered X-Forwarded-For line is used and any remaining
>> ones seem to be ignored. so, for the above case, our remote_addr ends
>> up being the non-public value of 10.1.1.69 <http://10.1.1.69>.
>> after some searching about how multiple lines of the same type are
>> supposed to be handled, i found this:
>> which contains:
>> Multiple message-header fields with the same field-name MAY be
>> present in a message if and only if the entire field-value for that
>> header field is defined as a comma-separated list [i.e.,
>> #(values)]. It MUST be possible to combine the multiple header
>> fields into one "field-name: field-value" pair, without changing the
>> semantics of the message, by appending each subsequent field-value
>> to the first, each separated by a comma. The order in which header
>> fields with the same field-name are received is therefore
>> significant to the interpretation of the combined field value, and
>> thus a proxy MUST NOT change the order of these field values when a
>> message is forwarded.
>> so, for that header above, if i could have the values from the
>> multiple lines coalesced, it would be as if i received a single line
>> like this:
>> X-Forwarded-For: 10.1.1.69 <http://10.1.1.69>, 220.127.116.11 <
>> i have developed a coding change that makes nginx accumulate the
>> values for any and all X-Forwarded-For lines within an incoming
>> header. i found that this is almost enough let the realip module
>> operate properly.
>> once this coding change was put in place, i could proceed with
>> testing. i then discovered a bug within the realip module that makes
>> it fail to replace the ip address in some cases. a string was not
>> being properly zero-terminated so, just depending on what came after
>> it in memory, the replacement would work sometimes, but not others. i
>> made another coding change to correct this.
>> the attached patch file modifies two source files:
>> the changes to ngx_http_request.c have to do with accumulating
>> the values from any and all X-Forwarded-For lines.
>> the changes to ngx_http_realip_module.c have to do with making sure
>> the ip replacement decision is made with a zero-terminated string.
>> when i wrote the value-accumualation code within ngx_http_request.c, i
>> ended up needing a local buffer. i'm allocating that buffer's memory
>> from the request pool. my belief is that i don't need to explicitly
>> free this memory since the entire request-based pool will be dropped
>> once the request is fully processed. i do not see any free calls
>> being made for any of the other allocations made against this
>> request-based pool. is my presumption correct?
>> from my reading of that rfc2616 text, i believe that nginx needs to
>> apply this type of value coalescing to any and all cases of multiple
>> header lines of the same type. my modification only applies this
>> treatment for the X-Forwarded-For case. i did this to reduce the
>> amount of testing required.
>> i hope this info is helpful and that, if i'm overlooking something,
>> someone might raise a warning to me.
>> thanks in advance...
>> martin stitt
> Why don't you just make r->headers_in.x_forwarded_for point to the last
> X-F-F header instead of the first? Won't that achieve what you want without
> munging headers?
> Some comments on the coding style FWIW:
> 1. The "max size" seems a little brittle. You should probably see how much
> space is needed for all the IP addresses, allocate that amount, then create
> the new string. Maybe in the header loop save references to the
> X-Forwarded-For headers in an array. Then outside the loop iterate over your
> x_forwarded_for array once to get the length, again to copy the values.
> Check out src/core/ngx_array.c for array usage.
> 2. I think the logged messages here should be at level "debug", not "info".
> 3. As a matter of patch politesse, you should probably change the mjs_
> prefixes and get rid of the commented-out code.
> 4. Good catch with ngx_inet_addr.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the nginx