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

[jean-frederic clere]

On 01/11/2019 21:15, Yann Ylavic wrote:

Hi Jean-Frédéric,

sorry to dig up an old commit, I'm currently working on integrating
wstunnel into mod_proxy_http and wondering what is the use case for
upgrade=NONE. Please see below...


The upgrade=NONE was added to work around 
https://issues.jboss.org/browse/JBCS-291, I will dig again in the 
problem later today.




On Thu, Jul 13, 2017 at 2:58 PM jean-frederic clere  wrote:


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).


I see some possible use of upgrade=ANY (tunnel any protocol after the
upgrade, no restriction), but NONE is bypassing the check for the
client Upgrade header and act as if it were Websocket. Why would we
upgrade a connection if the client does not ask for it?



So how I have the following to proxy:

GET /jboss-websocket-hello/websocket/helloName HTTP/1.1

[snip]

Connection: keep-alive, Upgrade

[snip]

Upgrade: websocket

[snip]




HTTP/1.1 101
Upgrade: websocket
Connection: upgrade

[snip]



...,..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 above is some usual Websocket upgrade AFAICT, don't
upgrade=websocket or upgrade=any or even no upgrade at all work in
this case?

Regards,
Yann.




--
Cheers

Jean-Frederic

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

[Yann Ylavic]

Hi Jean-Frédéric,

sorry to dig up an old commit, I'm currently working on integrating
wstunnel into mod_proxy_http and wondering what is the use case for
upgrade=NONE. Please see below..

On Thu, Jul 13, 2017 at 2:58 PM jean-frederic clere  wrote:
>
> 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).

I see some possible use of upgrade=ANY (tunnel any protocol after the
upgrade, no restriction), but NONE is bypassing the check for the
client Upgrade header and act as if it were Websocket. Why would we
upgrade a connection if the client does not ask for it?

>
> So how I have the following to proxy:
> 
> GET /jboss-websocket-hello/websocket/helloName HTTP/1.1
[snip]
> Connection: keep-alive, Upgrade
[snip]
> Upgrade: websocket
[snip]
> 
>
> 
> HTTP/1.1 101
> Upgrade: websocket
> Connection: upgrade
[snip]
> 
>
> ...,..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 above is some usual Websocket upgrade AFAICT, don't
upgrade=websocket or upgrade=any or even no upgrade at all work in
this case?

Regards,
Yann.

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

[jean-frederic clere]

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:

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





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




...,..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
> 

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

[Jacob Champion]

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

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

[jean-frederic clere]

On 07/11/2017 02:36 PM, Yann Ylavic wrote:
> On Tue, Jul 11, 2017 at 1:41 PM,   wrote:
>> Author: jfclere
>> Date: Tue Jul 11 11:41:44 2017
>> New Revision: 1801594
>>
>> URL: http://svn.apache.org/viewvc?rev=1801594=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@httpd.apache.org/msg66204.html
> 

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,   wrote:
> Author: jfclere
> Date: Tue Jul 11 11:41:44 2017
> New Revision: 1801594
>
> URL: http://svn.apache.org/viewvc?rev=1801594=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@httpd.apache.org/msg66204.html