[squid-dev] [PATCH] Non-HTTP bypass

Tue Oct 21 13:29:07 UTC 2014

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Sorry I didn't get back to you on this earier.

Please hold off on this until the Parser-NG patch lands. You may
want/need to redesign parts of this based on parser changes already in
that branch, or which will follow in the next stages.
In fact this tunneling behaviour was to *be* one of the following stages.

Some hints follow ...

On 16/10/2014 7:30 a.m., Tsantilas Christos wrote:
> 
> Intercepting proxies often receive non-HTTP connections. Squid cannot
> currently deal with such connections well because it assumes that a
> given port receives HTTP, FTP, or HTTPS traffic exclusively. This patch
> allows Squid to tunnel unexpected connections instead of terminating
> them with an error.
> 
> This patch:
>    -Defines an unexpected connection as a connection that resulted in a
> Squid error during first request parsing. Which errors trigger tunneling
> behavior is configurable by the admin using ACLs.

The new parser will give a definitive fail answer to identify non-HTTP
traffic. Consisting of hp->parse() call returning false, and
hp->request_parse_status == Http::scBadRequest.

You can see the relevant case at the top of client_side.cc
parseHttpRequest() where it emits "error:invalid-request".

I have also just committed the flexible transferProtocol member for
ConnStateData. With this proposed feature we will need to add a
AnyP::PROTO_TCP and set the traffic type to be that when bypassing
opaque data through the proxy.

> 
>   - Adds "on_first_request_error", a new ACL-driven squid.conf directive
> that can be used to establish a blind TCP tunnel which relays all bytes
> from/to the intercepted connection to/from the intended destination
> address. See the sketch above.
> The on_first_request_error directive supports fast ACLs only.
> 

What a nasty name for an access list.

I believe tcp_tunnel or tunnel_transparent would be more descriptive of
the test is *checking* for. Particularly when we fix the missing CONNECT
via peers issue.

The "only checked on first request failure" detail can be left in the
directive documentation. I also suspect that is a false statement since
anything we do with a CONNECT body that fails will need a matching check
applied.

ssl-bump calls its version of tunnelling "ssl_bump splice". So if we are
continuing with the splice vs tunnel terminology the syntax would look
something like this:

 tcp_tunnel deny localhost
 tcp_tunnel splice all

After this change goes in there will undoubtedly be requests to extend
it so that we do not waste time waiting for a parse error and just allow
a transparent port to tunnel selected connections right from accept().
So lets be prepared for that with the right naming and scope from the start.

>   - Adds "squid_error", a new ACL type to match transactions that
> triggered a given Squid error. Squid error IDs are used to configure one
> or more errors to match. This is similar to the existing ssl_error ACL
> type but works with Squid-generated errors rather than SSL library errors.

Note that what you are calling an "error" is just the presented payload
page ID. It is not necessarily one of the registered codes in the
template list since deny_info attached to any ACL can change it and load
custom pages.

> 
>   - Adds "ERR_WRONG_PROTOCOL", a new Squid error triggered for http_port
> connections that start with something that lacks even basic HTTP request
> structure. This error is triggered by the HTTP request parser, and
> probably only when/after the current parsing code detects an error.

There is nothing wrong about the protocol. It is the administrators
"bad" decision to intercept that port which is the problem.

 ERR_UNSUP_PROTOCOL is more correct and even matches the actual template
message you picked.

Also, what HTTP status code are you intending to use for this template?
since scBadRequest maps to ERR_INVALID_REQUEST already.

> 
>   Adds "request_start_timeout", a new squid.conf directive to trigger a
> new Squid ERR_REQUEST_START_TIMEOUT error if no bytes are received from
> the client on a newly established http_port connection during the
> configured time period. Applies to all http_ports (for now).

That was a separate discussion and should be a separate patch IMO.
Please separate the two features.

I know that hanging client connections is *possibly* a sign that they
are trying to use some protocol where the server handshakes first.

However, the times when that was even halfway reliable are long gone.
Browsers implemented "happy eyeballs", Alex implemented standby
connections, IPv4 people block ICMP packet-too-big, etc ... new
connections with long timeout are a very common error in otherwise
perfectly normal HTTP traffic. It is invalid criteria to base a
determination of the "unsupported protocol" status.

Also, allowing a client to bypass the proxy simply by hanging their
connection for some seconds/minutes before using it is an extremely
hazardous action to allow. I would be surprised if any sane admin wanted
this to happen.

In short, I want to see this added separately with a default action on
this timeout to be connection close / TCP_RST or the usual squid error
page (no checking for bypass until someone can provide a rock-solid
reason for it and way to do so reliably).

> 
> No support for tunneling through cache_peers is included. Configurations
> that direct outgoing traffic through a peer may break Squid.
> 
> Configuration sketch:
> 
>    # define what Squid errors indicate receiving non-HTTP traffic:
>    acl foreignProtocol squid_error ERR_WRONG_PROTOCOL ERR_TOO_BIG

TOO_BIG is not correct for this. It appears if the URL field or the mime
headers portions are larger than admin *configured* limits - regardless
of how valid they are in HTTP syntax.

There are a handful of situations where the too-big request happens so
routinely that status codes 414 and 431 got standardized to help
browsers auto-crop their request details and retry.

> 
>    # define what Squid errors indicate receiving nothing:
>    acl serverTalksFirstProtocol squid_error ERR_REQUEST_START_TIMEOUT
> 
>    # tunnel everything that does not look like HTTP:
>    on_first_request_error tunnel foreignProtocol
> 
>    # tunnel if we think the client waits for the server to talk first:
>    on_first_request_error tunnel serverTalksFirstProtocol
> 
>    # in all other error cases, just send an HTTP "error page" response:
>    on_first_request_error respond all
> 
>    # Configure how long to wait for the first byte on the incoming
>    # connection before raising an ERR_REQUEST_START_TIMEOUT error.
>    request_start_timeout 5 seconds
> 
> For more informations please read patch preamble.
> 
> This is a Measurement Factory project
> 

I've had a quick look through the patch. Two major things stood out:

1)  The timeout change being unrelated to the transparent proxy changes,
as mentioned above.

2) All changes in src/tunnel.cc seem to be needless.
 - tunnelStartShovelling() should *always* be the entrypoint to begin
transmit on a tunnel in any direction. At that point there is maybe
client data to send to server ...
 - The socket may not even permit one whole TCP_RCV_BUF worth of bytes
to be written in a single action. comm_write can handle that just fine.
If comm_write is unable to handle all the available conn->in.buf in one
comm_write() call then that would be a bug in comm to fix separate from
all this.
 There is simply no reason to add an extra if
(preReadClientData.length()) conditional in *every* packet I/O cycle.

Cheers
Amos

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (MingW32)

iQEcBAEBAgAGBQJURl+iAAoJELJo5wb/XPRjI2MIAJqSAbdiX08FKrCeXaPn1ZCP
kYSDcLVTBH9FQyHlGwXLlefh9YTaHnCSx+XLGrvqPFOWQtQ7RkECzQD0/VgxxxSt
tkm5Mrt2lNk/jvu153nctfraVvfWWz+yqF6sbcYv9Ufdr/QhVggob3c6kNIYido4
XFvvAqzaeV/g8FkylAcuCnnxIGLQcyzmUmsDg64shIKPx5nzUARxVkrDyHhjYPqU
D+lf4QMoyYuIJOytU++PdL2gkyEfVAtdLWZqfeaxZN89lRkpPGmNWRNOYjW+RqeY
WXGn0ulRlHuJpTOEsB98nGBd/sV51e49OnGo4cNAKXHI3IXK/49b9L4hqWj+rAY=
=4FE+
-----END PGP SIGNATURE-----