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

[Tsantilas Christos]

On 10/21/2014 04:29 PM, Amos Jeffries wrote:
> -----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.

OK.

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

Please see my comments bellow....
If required I will make my comments in parser-ng patch discussion too....

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

Yep I saw this change.
I will remake the patch after your patch applied to trunk.


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

it is not so bad :-)

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

The tcp_tunnel is not a bad name.
Lets see if Alex has a different opinion on see...


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

It is the error we are recording to request::errType or the error passed 
directly to FillledChecklist::requestErrorType

The HttpRequest::errType set using the HttpRequest::detailError() method 
which will not update with new error code the errType member it it is 
already set.
So I believe that the deny_info can not change/adjust the 
HttpRequest::errType. This is always the first error.
Am I missing something?

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

Actually still I am confused about the correct errors...
Probably the ERR_UNSUP_PROTOCOL is an error triggered where we can read 
the protocol, but we are not support it. For example a request in the 
form:
     GET rtsp://destodomain.com/path/ RTSP/1.1

The wrong protocol menas a completely foreign protocol, for example a 
sequence of binary chars...
But still I am not sure if we need to separate these cases....

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

Bad request may include a variety of errors in an HTTP request. But not 
an unknown protocol.

Do you think that these cases can be handled (and needed) in new 
parser-ng patch?


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

Does it make sense this feature with out the non-http bypass feature?

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

Yes! this is true.
Also the browsers may open connections just in case they need them in 
the future.
It is not a prof but it is an indication. Just for special applications.
It can be combined with other acls (ip addresses)

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

Yes. But we should not consider clients always as enemies.
There are cases where there is trust between admins and users.

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

It is an example.
I will remove it.


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

I will review the related changes. I am to tired now to make clear 
thoughts on this...



>
> Cheers
> Amos
>