Allow upgrading from HTTP/1.1 to other protocols

[chtsanti]

Support admin-authorized HTTP Upgrade-driven (RFC 7230 Section 6.7)
protocol switching. The new http_upgrade_request_protocols configuration
directive allows admin to control what client Upgrade offer(s) are
forwarded to the server. Squid does not automatically check whether the
server selection matches one of the client offers, but the admin can
deny unacceptable server responses using http_reply_access.

By default, Squid still does not forward any Upgrade headers,
effectively blocking an upgrade attempt.

Squid itself does not understand the protocols being upgraded to and
participates in the upgraded communication only as a dumb TCP proxy.

This is a Measurement Factory project.

[yadij]

Squid itself does not understand the protocols being upgraded to and
participates in the upgraded communication only as a dumb TCP proxy.

This violates the RFC7230 requirement:

server MUST NOT switch protocols unless the received message semantics can be honored by the new protocol;

In other words, Squid has to understand the new protocol sufficiently to perform any handshake protocol and message translation from the HTTP request into other protocol native format for that request - including understanding whether tunnel is the correct thing to do. We simply cannot do that generically - such as the "OTHER" being added by this PR.

Even WebSocket/1 needs some translation, it is intentionally and explicitly not tolerant of HTTP/1 syntax violations in its handshake message. The Parser changes I/we have been doing have fixed a lot of those, but the handling of unknown and generic String headers and field-values has yet to happen. This PR in its current design would make it possible to DoS a WebSocket server behind Squid.

[chtsanti]

Squid itself does not understand the protocols being upgraded to and
participates in the upgraded communication only as a dumb TCP proxy.

This violates the RFC7230 requirement:

We are violating RFCs in other cases too for example when we are allowing to add or remove http headers, when we are ignoring Cache-Control etc.
Upgrading to other protocols by default is not enabled and uses acls.
We can add a related WARNING in http_upgrade_request_protocols documentation.

This PR in its current design would make it possible to DoS a WebSocket server behind Squid.

I do not believe that it is more dangerous than allowing CONNECT tunnels through squid.

[rousskov]

Squid itself does not understand the protocols being upgraded to and
participates in the upgraded communication only as a dumb TCP proxy.

This violates the RFC7230 requirement:

server MUST NOT switch protocols unless the received message semantics can be honored by the new protocol;

The quoted Squid behavior (i.e. "This") does not violate (and does not even relate to!) the quoted MUST: Before Squid switches protocols, the Squid admin has to tell Squid that the "received message" (e.g., a GET request) "semantics can be honored by the new protocol". (The origin server also confirms that admin's determination before the protocols are switched, but that is secondary).

In other words, Squid has to understand the new protocol sufficiently to perform any handshake protocol and message translation from the HTTP request into other protocol native format for that request - including understanding whether tunnel is the correct thing to do.

There is no such requirement in the RFC text you quoted. In other words, you are misrepresenting what that MUST actually requires. Not all protocols can be tunneled, of course. Squid knows that it can tunnel in each particular case because the admin told Squid that it can (and the origin server confirmed).

We simply cannot do that generically - such as the "OTHER" being added by this PR.

We are not doing this generically, not even in the OTHER case. In all cases, there is an explicit instruction from the admin. Needless to say, the admin may misconfigure Squid, but that is true for most Squid features.

Even WebSocket/1 needs some translation, it is intentionally and explicitly not tolerant of HTTP/1 syntax violations in its handshake message.

@yadij, it is not clear how your statement relates to this PR. Are you implying that this PR does not support any WebSocket/1 upgrades? Let's start with the basic/targeted case of a trusted client attempting an HTTP Upgrade to a trusted server behind Squid, with Squid admin permission (based on tests, knowledge of the application, etc.). Is this PR lacking something critical in that case?

@chtsanti, do WebSocket/1 upgrades work in your tests?

This PR in its current design would make it possible to DoS a WebSocket server behind Squid.

Since virtually any design would make it possible to DoS a WebSocket server behind Squid, you need to be a lot more specific in your criticism for Christos to be able to actually address your concerns. To simplify, let's start with the basic/targeted case of a trusted client attempting an HTTP Upgrade to a trusted server behind Squid, with a well-informed Squid admin permission. Does this PR make things worse in that case?

[chtsanti]

I am not an websockets expert so somethings from the following I am writing maybe are not accurate.

Even WebSocket/1 needs some translation, it is intentionally and explicitly not tolerant of HTTP/1 syntax violations in its handshake message.

@yadij, it is not clear how your statement relates to this PR. Are you implying that this PR does not support any WebSocket/1 upgrades? Let's start with the basic/targeted case of a trusted client attempting an HTTP Upgrade to a trusted server behind Squid, with Squid admin permission (based on tests, knowledge of the application, etc.). Is this PR lacking something critical in that case?

@chtsanti, do WebSocket/1 upgrades work in your tests?

My understanding is that @yadij refers to Websockets emulation protocol which is uses HTTP request/responses to transfer websockets packets. These requests must include between others the header "X-WebSocket-Version: wseb-1.0". I found very few information on web about this protocol. However looks that this is should work in squid and without this patch.

The websockets protocol described in RFC6455 and which is uses the upgrade mechanism works very well using this patch in my tests.

Simple tests someone can do using the following urls:
http://demos.kaazing.com/echo/index.html
https://www.websocket.org/echo.html

The first one uses the websockets emulation protocol when plain websockets is selected (I suppose to overcome possible proxy problems) but normal websockets if TLS is used. You can test TLS websockets using SSL bumping.

[rousskov]

@yadij, any objections to this going in? It looks like Christos is certain the code works for the targeted WebSocket use case. It is not clear whether you were talking about that case or something else in your original comment. If you have objections, please block this PR and clarify your remaining concerns. Otherwise, Christos should clear this PR for merge.

[rousskov]

I have not reviewed the code, but I accidentally noticed that it is using std::function. I want to make sure its overhead is justifiable. Please give me a day or two to check the corresponding code.

[chtsanti]

I have not reviewed the code, but I accidentally noticed that it is using std::function. I want to make sure its overhead is justifiable.

Could we use something like the following instead:
const MemberFilter ft = [item, ilen](const char *check, int checkLen) { ...}
bool found = strListIsMember_if(upgradeHeaderOut, ',', ref(ft));

Else we can use a pointer to standard C function instead or a class.

Please give me a day or two to check the corresponding code.

yep

[rousskov]

@chtsanti, I am still finding and fixing problems, but I have several questions that you may be able to help me with, parallelizing our work. Please see inlined change requests.

[rousskov]

Please fix two out of three issues we have discussed. Please see inlined change requests for details.

[rousskov]

I am content with the current PR state and do not plan doing further reviews or polishing.

My change request has been addressed.

[rousskov]

I polished PR description to fix a typo, emphasize what controls the admin has, and document that the default no-upgrades behavior is preserved.

[yadij]

No objections from me to current implementation.

[RoieRMeta]

Does this PR have any relation with running websockets via ssl_bump ?

[yadij]

Sorry got the previous reply backwards. A WebSockets client which is correctly using the opening handshake inside TLS would be decrypted and reach this feature for handling as WebSockets.

[rousskov]

Does this PR have any relation with running websockets via ssl_bump?

This PR adds a feature that allows Squid to support HTTP Upgrade, including "upgrade" to a WebSocket tunnel. The PR itself is unrelated to SslBump, but we hope that the feature works for bumped HTTPS connections just like it works for plain HTTP connections.

[RoieRMeta]

Are there plans to merge this feature to branch 4.x ?

[yadij]

Are there plans to merge this feature to branch 4.x ?

No. Squid-4 has been in production release for several years already and its merge criteria is restricted to changes such as bugs and security issues. This is a relatively large behavioural change that adds risk of adding stability problems to a few popular features.

Squid-5 is close to production ready and should be usable if this is a required feature.