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

[Tsantilas Christos]

I made all requested changes/fixes.
The patch also ported to latest trunk.


On 01/12/2015 06:46 PM, Amos Jeffries wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 31/12/2014 11:40 p.m., Tsantilas Christos wrote:
>> On 12/31/2014 09:54 AM, Alex Rousskov wrote:
>>> On 12/30/2014 06:19 PM, Amos Jeffries wrote:
>>>> On 31/12/2014 7:30 a.m., Alex Rousskov wrote:
>>>
>>>>> Amos, if on_first_request_error is converted into on_error
>>>>> with a first_request (or similar) ACL, would you continue to
>>>>> block this frequently-requested bypass feature?
>>>
>>>
>>>> Mosly I am waiting to see an updated patch. The blocker was on
>>>> waiting for Parser-NG request handling re-write which is now
>>>> in.
>>>
>>> I am interpreting your answer as a "no" and ask Christos to
>>> update the patch to better integrate with the new parsing code.
>>> AFAIK, the only integration is in detecting an "HTTP request
>>> parsing has failed" condition, which is hopefully easy.
>>
>> OK I will post an updated patch. If the problem is only the name
>> then we can easily change it.
>>
>
> It seems the updated patch submission did not get through to me for
> review. Sorry about that and the delay its caused. Here is the review:
>
>
> * Regarding "ERR_WRONG_PROTOCOL"
>   Please call this ERR_PROTOCOL_UNKNOWN. There is no clear "wrong" to
> be fixed.

Fixed.

>
>
> * add the copyright blurb from scripts/boilerplate.h to the top of all
> new source code files.
>   - please see the latest version of other ERR_ templates for how to
> copyright the new template.

Yep. fixed.

>
>
> in src/acl/SquidError.h:
> * dont add dead code: //virtual bool requiresRequest() ...

line removed.

>
>
> in src/acl/SquidErrorData.cc:
> * please bump the debugs level in ACLSquidErrorData::match() to
> something deeper (eg 3 or 4). We dont need a high level log entry on
> every attempted value match.

fixed

>
> * ACLSquidErrorData::parse() should be able to have token declared n
> the while() scope, like so:
>    while (char *token = ...) {

fixed

>
> * debugs error message missing "FATAL: " prefix in
> ACLSquidErrorData::parse().

>   - Also, this is a relatively minor config issue for which the
> self_destruct() can be skipped when doing -k parse (if global variable
> opt_parse_cfg_only is set non-0). Though leave the FATAL debugs
> message displaying.

I add the "FATAL: " prefix and  self_destruct called only when 
opt_parse_cfg_only is not set.

>
>
> in src/cf.data.pre:
> * instead of "DEFAULT: none" please use "DEFAULT_DOC: ..." to state a
> one-line about what happens if there are none of these directive lines
> configured.
>   eg. DEFAULT_DOC: Respond with an error message to unidentifiable traffic.
>

OK. I left "DEFAULT: none", looks that other directives have both 
"DEFAULT: none" and DEFAULT_DOC

>
> in src/client_side.cc:
> * dont add HERE in new debugs() in clientTunnelOnError()

OK.

>
> * fix \retval documentation on Squid_SSL_accept()
>   - "\retval" takes two parameters: VALUE MEANING_OF_VALUE

fixed

>
> * clientPeekAndSpliceSSL() has the ConnStateData object (conn)
> available with its conn->clientConnection.
>   - IF (only if) the "fd" being closed is supposed to be the client
> connection rather than other FD, then please use
> conn->clientConnection->close() to close it cleanly.

Yes the client connection is closed here. I am using 
conn->clientConnection->close() now.

>
>
> in src/client_side.h:
> * issues with \retval again for spliceOnError()
>
> * grammar in docs for preservedClientData:   s/as is/as-is/
>
> * lowercase 'True' in docs for receivedFirstByte_

ok.

>
>
> in src/http/one/RequestParser.cc:
> * accepting the full range of valid method characters is important,
> the new implementation will make us start to fail some coadvisor
> testing for special cased methods like the dot (.) method, and WebDAV
> extension methods with hyphens or underscores, etc.
>   NP: I am not aware of any other characters being used, but if the
> mechanism to test is easily extended we can fill others in later.

You are right.
Now I am accepting as valid chars any printable ascii. I hope it is OK.

>
>
> in src/servers/HttpServer.cc:
> * missing body to two if-statements in buildHttpRequest().
>    - if (!clientTunnelOnError(...)) {/* missing code ?? */}
>   please fix or document why nothing is done.

It is OK as is.
We may want to call clientProcessRequestFinished(), because maybe in the 
future executes code which is required. But not it is not required.

Regards,
    Christos



>
>
> HTH
> Amos
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.22 (MingW32)
>
> iQEcBAEBAgAGBQJUs/paAAoJELJo5wb/XPRjf3sIAOC85ezF2m2OvqbLSxDwGoO1
> dkLmHHGqQv1X5qcyWzDXlS2XFP67zNCwcCiuLnT5nqsF4tcGTAZ3Phi9+LQTlR18
> H67Qp6uSJLzKLeVCdjEEbvAOPtJ8EldkkhlMFOEaKzhVKqNStqXjg8SUOkYJz/V7
> Ouo8G/Re0StVp3w4WtHND0zDGh6Dupw4B+E7EBfxMu61MjqXB4WzAcgzIqIQeT8/
> phJT4ObSYyOvHEPRIeAUGrMrryr5wwmKTynhB6xbT9Q4Kks0bzirdjhaiKVffIdm
> mMTHKuUbJesouAGZSTueXBBU9AobpRGVSecfDiLfFTJL7Fy417jIKtfqDsnsHUU=
> =wIBa
> -----END PGP SIGNATURE-----
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: trunk-non-HTTP-bypass-v8.patch
Type: text/x-patch
Size: 94416 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150113/caecd58b/attachment-0001.bin>