Re: [mongrel2] [patch] Add REMOTE_ADDR and REMOTE_PORT headers to
mongrel2 messages
- From:
- Dalton Barreto
- Date:
- 2011-03-11 @ 14:22
2011/3/10 Loic d'Anterroches <loic@ceondo.com>:
> Hello,
>
>>> please find attached a patch to add REMOTE_ADDR and REMOTE_PORT to the
>>> hearders. At the moment you have a dup, in the sense that the
>>> REMOTE_ADDR is the same as the x-forwarded-for header.
>>>
>>> The patch is a diff done with "fossil diff". I am not sure if this is
>>> easy to use. My first patch for Mongrel2, yeah!
>>>
>>
>> Great! Congratulations! The patch applies cleanly here and everything
>> works like expected!
>> I think we could remove the REMOTE_ADDR as it is indeed a dup.
>>
>> Would you mind producing a new patch without the REMOTE_ADDR part?
>>
>> Let's wait Zed to take the final step and possibly merge this into the
mainline.
>
> In fact my patch and Mongrel2 mainline are wrong per description of the
> non official x-forwarded-for header:
> http://en.wikipedia.org/wiki/X-Forwarded-For
>
> If we consider that Mongrel2 is a "smart" proxy simply dispatching the
> request to our handlers, we should have:
>
> X-Forwarded-For: client1, proxy1, proxy2, remote_addr_trad
> REMOTE_ADDR: ip.of.mongrel2
>
> and remote_addr_trad being the effective address of the client.
>
> So, in connection.c we should test if the x-forwarded-for header is
> already set, if yes, add the client address at the end of the list, if
> not do like now and put remote_addr as the ip of mongrel2.
>
> Adding the ip of mongrel2 as the remote_addr could be interesting if one
> decide to forward deeper the request at the handler level and update
> accordingly the x-forwarded-for header, but when you update, you are
> connected to mongrel2 using zmq, so you know the ip.
>
Actually x-forwarded-for is fragile, as the very first client
can send the wrong IP address since the first connection.
REMOTE_ADDR seems to be the address of the "last-hop" of the request, that is,
the machine that is mediately before you in the request chain.
If we imagine a perfect world, where no clients are forging this
header, the IP of the original
client would always be x-forwarded-for[0], but obviously we can not
think this way.
Anyway, thanks for your clarifications. For now I'm using
x-forwarded-for[0] (even knowing that this value can be wrong), but
I'll keep an eye on this.
If your second patch gets into mainline I will start using REMOTE_ADDR
directly. Since mongrel2 lowers all headers names
coming from the client it becomes very difficult (impossible?) to
forge the value of REMOTE_ADDR. With your patch applied
a simple test gives me this:
curl -H "REMOTE_ADDR: wrong IP" http://127.0.0.1:8888/py
{
"REMOTE_PORT": "57647",
"PATTERN": "/py",
"x-forwarded-for": "127.0.0.1",
"remote_addr": "wrong IP",
"METHOD": "GET",
"accept": "*/*",
"user-agent": "curl/7.20.0 (i686-pc-linux-gnu) libcurl/7.20.0
GnuTLS/2.10.2 zlib/1.2.3 libssh2/1.2.2",
"host": "127.0.0.1:8888",
"VERSION": "HTTP/1.1",
"PATH": "/py",
"REMOTE_ADDR": "127.0.0.1",
"URI": "/py"
}
Since we know all uppercase header are mongrel2 headers, I can trust
the value of REMOTE_ADDR but not remote_addr.
Applications running with my monrel2 adapter will get the right value
when they do environ['REMOTE_ADDR'].
> So, at the moment, only the remote_port information is "right" in the
> patch.
>
If we update REMOTE_ADDR to be ip.of.mongrel2 in some cases we should
also adjust REMOTE_PORT accordingly.
--
Dalton Barreto
http://daltonmatos.wordpress.com
Re: [mongrel2] [patch] Add REMOTE_ADDR and REMOTE_PORT headers to
mongrel2 messages
- From:
- Zed A. Shaw
- Date:
- 2011-03-14 @ 16:58
On Fri, Mar 11, 2011 at 11:22:29AM -0300, Dkalton Barreto wrote:
> 2011/3/10 Loic d'Anterroches <loic@ceondo.com>:
> Actually x-forwarded-for is fragile, as the very first client
> can send the wrong IP address since the first connection.
I haven't tested this yet, but mongrel2 looks like it's adding an extra
x-forwarded-for header. Since the remote client can add one, you'd get
two in that case. Let's say the client sends:
X-Forwarded-For: blah, blah, 127.0.0.1
Then mongrel2 adds the "real" one with:
// add the x-forwarded-for header
Request_set(conn->req, bstrcpy(&HTTP_X_FORWARDED_FOR),
blk2bstr(conn->remote, IPADDR_SIZE), 1);
Which would be the last-hop. So, there is a security problem here
because a lot of the frameworks just assume that this is going to be one
header.
Probably what needs to happen is some kind of change on this...
> REMOTE_ADDR seems to be the address of the "last-hop" of the request, that is,
> the machine that is mediately before you in the request chain.
>...
> Since we know all uppercase header are mongrel2 headers, I can trust
> the value of REMOTE_ADDR but not remote_addr.
> Applications running with my monrel2 adapter will get the right value
> when they do environ['REMOTE_ADDR'].
Right, so I'll check out the patch today and see if I can apply it, and
I think this might need another modification because so many frameworks
rely on x-forwarded-for as if it comes from the webserver reliably.
--
Zed A. Shaw
http://zedshaw.com/
Re: [mongrel2] [patch] Add REMOTE_ADDR and REMOTE_PORT headers to
mongrel2 messages
- From:
- Loic d'Anterroches
- Date:
- 2011-03-15 @ 08:09
On 2011-03-14 17:58, Zed A. Shaw wrote:
> On Fri, Mar 11, 2011 at 11:22:29AM -0300, Dkalton Barreto wrote:
>> 2011/3/10 Loic d'Anterroches <loic@ceondo.com>:
>> Actually x-forwarded-for is fragile, as the very first client
>> can send the wrong IP address since the first connection.
>
> I haven't tested this yet, but mongrel2 looks like it's adding an extra
> x-forwarded-for header. Since the remote client can add one, you'd get
> two in that case. Let's say the client sends:
>
> X-Forwarded-For: blah, blah, 127.0.0.1
>
> Then mongrel2 adds the "real" one with:
>
> // add the x-forwarded-for header
> Request_set(conn->req, bstrcpy(&HTTP_X_FORWARDED_FOR),
> blk2bstr(conn->remote, IPADDR_SIZE), 1);
>
> Which would be the last-hop. So, there is a security problem here
> because a lot of the frameworks just assume that this is going to be one
> header.
>
> Probably what needs to happen is some kind of change on this...
>
>> REMOTE_ADDR seems to be the address of the "last-hop" of the request, that is,
>> the machine that is mediately before you in the request chain.
>> ...
>> Since we know all uppercase header are mongrel2 headers, I can trust
>> the value of REMOTE_ADDR but not remote_addr.
>> Applications running with my monrel2 adapter will get the right value
>> when they do environ['REMOTE_ADDR'].
>
> Right, so I'll check out the patch today and see if I can apply it, and
> I think this might need another modification because so many frameworks
> rely on x-forwarded-for as if it comes from the webserver reliably.
Yes, this is what I said. In the way I wrote the patch, the last hop of
X-Forwarded-For is reliably set to the IP of the client without
discarding what is provided by the client. Per "RFC" - there are no RFCs
on the subject only squid way of doing things, this is the way to go.
But as I wrote somewhere in the discussion, you have the "right way" and
the "practical right way". I am a practical coder, so... This is why I
added the REMOTE_ADDR as uppercase header to basically tell "Here you
can trust the full header".
loïc
Re: [mongrel2] [patch] Add REMOTE_ADDR and REMOTE_PORT headers to
mongrel2 messages
- From:
- Dalton Barreto
- Date:
- 2011-03-16 @ 01:37
2011/3/15 Loic d'Anterroches <loic@ceondo.com>:
> Yes, this is what I said. In the way I wrote the patch, the last hop of
> X-Forwarded-For is reliably set to the IP of the client without
> discarding what is provided by the client. Per "RFC" - there are no RFCs
> on the subject only squid way of doing things, this is the way to go.
> But as I wrote somewhere in the discussion, you have the "right way" and
> the "practical right way". I am a practical coder, so... This is why I
> added the REMOTE_ADDR as uppercase header to basically tell "Here you
> can trust the full header".
>
I agree with the idea of having REMOTE_ADDR separately. This way even
if x-forwarded-for is spoofed the handler will have a way to get the
correct value.
About x-forwarded-for, it's up to the handler to modify it (if your
handler is actually a gateway between mongrel2 and a known framework)
or just do not trust it and use REMOTE_ADDR directly.
This patch will be of great help! Hope to see it applied soon.
Thanks.
--
Dalton Barreto
http://daltonmatos.wordpress.com
[new patch] Add REMOTE_ADDR and REMOTE_PORT headers to mongrel2 messages
- From:
- Loic d'Anterroches
- Date:
- 2011-03-10 @ 18:35
Hello,
here is a new patch. I am correctly updating the x-forwarded-for header
as needed. I kept the remote_addr too as it is what normally people
expect at the script level.
To test:
curl -H "X-forwarded-for: titi, tata" http://localhost:6767/handlertest/
then in the headers you correctly get:
(
[PATH] => /handlertest/
[user-agent] => curl/7.19.7 (i486-pc-linux-gnu) libcurl/7.19.7
OpenSSL/0.9.8k zlib/1.2.3.3 libidn/1.15
[host] => localhost:6767
[REMOTE_PORT] => 46945
[accept] => */*
[x-forwarded-for] => titi, tata, 127.0.0.1
[REMOTE_ADDR] => 127.0.0.1
[METHOD] => GET
[VERSION] => HTTP/1.1
[URI] => /handlertest/
[PATTERN] => /handlertest
)
loïc
And yes, I am having fun at understanding the code of Mongrel2.
On 2011-03-10 18:01, Loic d'Anterroches wrote:
> Hello,
>
>>> please find attached a patch to add REMOTE_ADDR and REMOTE_PORT to the
>>> hearders. At the moment you have a dup, in the sense that the
>>> REMOTE_ADDR is the same as the x-forwarded-for header.
>>>
>>> The patch is a diff done with "fossil diff". I am not sure if this is
>>> easy to use. My first patch for Mongrel2, yeah!
>>>
>>
>> Great! Congratulations! The patch applies cleanly here and everything
>> works like expected!
>> I think we could remove the REMOTE_ADDR as it is indeed a dup.
>>
>> Would you mind producing a new patch without the REMOTE_ADDR part?
>>
>> Let's wait Zed to take the final step and possibly merge this into the
mainline.
>
> In fact my patch and Mongrel2 mainline are wrong per description of the
> non official x-forwarded-for header:
> http://en.wikipedia.org/wiki/X-Forwarded-For
>
> If we consider that Mongrel2 is a "smart" proxy simply dispatching the
> request to our handlers, we should have:
>
> X-Forwarded-For: client1, proxy1, proxy2, remote_addr_trad
> REMOTE_ADDR: ip.of.mongrel2
>
> and remote_addr_trad being the effective address of the client.
>
> So, in connection.c we should test if the x-forwarded-for header is
> already set, if yes, add the client address at the end of the list, if
> not do like now and put remote_addr as the ip of mongrel2.
>
> Adding the ip of mongrel2 as the remote_addr could be interesting if one
> decide to forward deeper the request at the handler level and update
> accordingly the x-forwarded-for header, but when you update, you are
> connected to mongrel2 using zmq, so you know the ip.
>
> So, at the moment, only the remote_port information is "right" in the
> patch.
>
> loïc
--
Dr Loïc d'Anterroches
Founder Céondo Ltd
w: www.ceondo.com | e: loic@ceondo.com
t: +44 (0)207 183 0016 | f: +44 (0)207 183 0124
Céondo Ltd
Dalton House
60 Windsor Avenue
London
SW19 2RR / United Kingdom