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

[Amos Jeffries]

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


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


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


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.

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

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


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.


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

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

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


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_


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.


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.


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