Re: [PATCH] ACL to control TPROXY spoofing

On 12/19/2012 09:04 AM, Steve Hill wrote:
>
> The attached patch adds a "spoof" fast ACL to control whether TPROXY
> requests have their source IP address spoofed by Squid. The ACL
> defaults to allow (i.e. the current normal behaviour), but using an ACL
> that results in a deny result will disable spoofing for that request.
>
> Example config (disables spoofing for all requests):
> spoof deny all

Hi Steve,

> +DEFAULT_IF_NONE: allow all

Can we avoid the overhead of checking an obscure ACL in the common code
path if the list is not configured? That is, can we remove the above
default and check that Config.accessList.spoof is not nil instead?

> + acl_access* spoof;

Please document the new member using a doxygen ///< comment. Referring
to the squid.conf option name should be sufficient, but you may also add
"nil unless configured".

> +NAME: spoof
...
> +DOC_START
> + Allow client address spoofing based on defined access lists
> +
> + spoof allow|deny [!]aclname ...
> +
> + If there are no "spoof" lines present, the default is to "allow"
> + spoofing of any suitable request.
> +
> + This clause supports fast acl types.
> + See http://wiki.squid-cache.org/SquidFaq/SquidAcl for details.
> DOC_END

Please indicate (in the first paragraph) that the option applies to
certain connections only. Otherwise, we will see folks enabling this for
regular traffic thinking that Squid is going to spoof client IP address.

Please indicate that it is IP address being spoofed (not MAC or some
other address).

s/Allow client/Control client/

Th first paragraph is missing a period at the end.

Will a more specific name like "spoof_client_ip" be more appropriate?
Squid may spoofs a few other things (e.g., server certificates).

> The patch also does a bit of code-cleanup:
>
> 1. The flags.spoofClientIp flag was a general "this is a TPROXY request"
> flag, which was a bit confusing given the name of the flag. So the
> flags.spoofClientIp flag now only indicates whether we want to spoof the
> source IP or not.
>
> 2. The logic for requests handled by a "tproxy" port and those handled
> by an "intercept" port is pretty much identical, so the
> flags.intercepted flag is now used to generically indicate that the
> request has been intercepted either by "intercept" or "tproxy".

The above cleanup is missing from the patch but is probably required
because the patch alters flags.spoofClientIp semantics. Please be extra
careful with these changes because they affect already fragile code.

Please add a doxygen comment to flags.spoofClientIp declaration
explaining what it is now.

Thank you,

Alex.
Received on Thu Dec 20 2012 - 17:27:49 MST