Re: svn commit: r1801594 - in /httpd/httpd/trunk: docs/manual/mod/mod_proxy_wstunnel.xml modules/proxy/mod_proxy_wstunnel.c

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1801594 - in /httpd/httpd/trunk: docs/manual/mod/mod_proxy_wstunnel.xml modules/proxy/mod_proxy_wstunnel.c

Yann Ylavic
On Tue, Jul 11, 2017 at 1:41 PM,  <[hidden email]> wrote:

> Author: jfclere
> Date: Tue Jul 11 11:41:44 2017
> New Revision: 1801594
>
> URL: http://svn.apache.org/viewvc?rev=1801594&view=rev
> Log:
>
> Add logic to read the Upgrade header and use it in the response.
> Use we you are proxying to a server that has multiple upgrade on the same IP/Port.
> PR 61142

I think it's quite hazardous to use/allow ANY and would prefer the
upgrade_method (worker->s->upgrade) to be a list of acceptable
protocols.

The admin surely knows which protocol(s) the backend supports, the
issue being that otherwise most backends will ignore the Upgrade and
hence the connection will continue in normal HTTP (tunneled w/o any
protocol checking).

IMO the Upgrade handling should be part of mod_proxy_http (not
_wstunnel) and depend on whether or not the backend accepted it.

It was already discussed in [1], well, I can't say that the idea was
unanimous at that time...
The proposed patch there is probably outdated now, but I still find it
safer than what we have now.

Regards,
Yann.

[1] https://www.mail-archive.com/dev@.../msg66204.html
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1801594 - in /httpd/httpd/trunk: docs/manual/mod/mod_proxy_wstunnel.xml modules/proxy/mod_proxy_wstunnel.c

Jean-frederic Clere-3
On 07/11/2017 02:36 PM, Yann Ylavic wrote:

> On Tue, Jul 11, 2017 at 1:41 PM,  <[hidden email]> wrote:
>> Author: jfclere
>> Date: Tue Jul 11 11:41:44 2017
>> New Revision: 1801594
>>
>> URL: http://svn.apache.org/viewvc?rev=1801594&view=rev
>> Log:
>>
>> Add logic to read the Upgrade header and use it in the response.
>> Use we you are proxying to a server that has multiple upgrade on the same IP/Port.
>> PR 61142
>
> I think it's quite hazardous to use/allow ANY and would prefer the
> upgrade_method (worker->s->upgrade) to be a list of acceptable
> protocols.

Probably...

>
> The admin surely knows which protocol(s) the backend supports, the
> issue being that otherwise most backends will ignore the Upgrade and
> hence the connection will continue in normal HTTP (tunneled w/o any
> protocol checking).
>
> IMO the Upgrade handling should be part of mod_proxy_http (not
> _wstunnel) and depend on whether or not the backend accepted it.

Correct upgrade belongs to the HTTP protocol.

Cheers

Jean-Frederic

>
> It was already discussed in [1], well, I can't say that the idea was
> unanimous at that time...
> The proposed patch there is probably outdated now, but I still find it
> safer than what we have now.
>
> Regards,
> Yann.
>
> [1] https://www.mail-archive.com/dev@.../msg66204.html
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1801594 - in /httpd/httpd/trunk: docs/manual/mod/mod_proxy_wstunnel.xml modules/proxy/mod_proxy_wstunnel.c

Jacob Champion-2
In reply to this post by Yann Ylavic
On 07/11/2017 05:36 AM, Yann Ylavic wrote:
> I think it's quite hazardous to use/allow ANY and would prefer the
> upgrade_method (worker->s->upgrade) to be a list of acceptable
> protocols.

I think both ANY *and* NONE are dangerous. Both of them turn
proxy_wstunnel into a generic TCP forwarder (and NONE does so without
any opt-in on the client's part).

> The admin surely knows which protocol(s) the backend supports, the
> issue being that otherwise most backends will ignore the Upgrade and
> hence the connection will continue in normal HTTP (tunneled w/o any
> protocol checking).

+1. Even once we implement the protocol list, we should still
double-check that the protocol is actually upgraded before we start
forwarding back and forth.

> IMO the Upgrade handling should be part of mod_proxy_http (not
> _wstunnel) and depend on whether or not the backend accepted it.

This I don't necessarily agree with as much... for now, Upgrade handling
belongs where it's needed, and if there are duplicate pieces of code, we
probably need to pull them into the core, not a different proxy module.

> It was already discussed in [1], well, I can't say that the idea was
> unanimous at that time...

Yeah, I don't understand the turn that conversation took. We're talking
about a feature that can be used for reverse-proxying, and there's
nothing to CONNECT to.

--Jacob
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1801594 - in /httpd/httpd/trunk: docs/manual/mod/mod_proxy_wstunnel.xml modules/proxy/mod_proxy_wstunnel.c

Jean-frederic Clere-3
On 07/12/2017 07:25 PM, Jacob Champion wrote:
> On 07/11/2017 05:36 AM, Yann Ylavic wrote:
>> I think it's quite hazardous to use/allow ANY and would prefer the
>> upgrade_method (worker->s->upgrade) to be a list of acceptable protocols.
>
> I think both ANY *and* NONE are dangerous. Both of them turn
> proxy_wstunnel into a generic TCP forwarder (and NONE does so without
> any opt-in on the client's part).

So how I have the following to proxy:
<request>
GET /jboss-websocket-hello/websocket/helloName HTTP/1.1

Host: localhost:8080

User-Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:53.0)
Gecko/20100101 Firefox/53.0

Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8

Accept-Language: en-US,en;q=0.7,ca;q=0.3

Accept-Encoding: gzip, deflate

Sec-WebSocket-Version: 13

Origin: http://localhost:8080

Sec-WebSocket-Extensions: permessage-deflate

Sec-WebSocket-Key: CMVwRmu3A0Ozj0og8cnrlA==

Connection: keep-alive, Upgrade

Pragma: no-cache

Cache-Control: no-cache

Upgrade: websocket


</request>

<response>
HTTP/1.1 101

Upgrade: websocket

Connection: upgrade

Sec-WebSocket-Accept: p4OcxSGQGdGqMJi7cxMnp8Sjrxc=

Sec-WebSocket-Extensions: permessage-deflate

Date: Thu, 13 Jul 2017 12:47:45 GMT


</response>

...,..9e...,.$.H...W(I-.QH+..U(OM*.O.N-QH.K)...+..
Tomcat web socket stuff...

So yes the HTTP/1.1 really needs to upgrade and NONE is just a work-around.

>
>> The admin surely knows which protocol(s) the backend supports, the
>> issue being that otherwise most backends will ignore the Upgrade and
>> hence the connection will continue in normal HTTP (tunneled w/o any
>> protocol checking).
>
> +1. Even once we implement the protocol list, we should still
> double-check that the protocol is actually upgraded before we start
> forwarding back and forth.

Actually the tunnel allows nearly everything.

Cheers

Jean-Frederic

>
>> IMO the Upgrade handling should be part of mod_proxy_http (not
>> _wstunnel) and depend on whether or not the backend accepted it.
>
> This I don't necessarily agree with as much... for now, Upgrade handling
> belongs where it's needed, and if there are duplicate pieces of code, we
> probably need to pull them into the core, not a different proxy module.
>
>> It was already discussed in [1], well, I can't say that the idea was
>> unanimous at that time...
>
> Yeah, I don't understand the turn that conversation took. We're talking
> about a feature that can be used for reverse-proxying, and there's
> nothing to CONNECT to.
>
> --Jacob
>

Loading...