Re: [PATCH] ACL to control TPROXY spoofing

On 21/12/2012 6:27 a.m., Alex Rousskov wrote:
> 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?

We can. In cf.data.pre set DEFAULT_DOC instead, and wrap the entire ACL
code setup in an if-nil check.

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

It would please, if you can work "tproxy" in specifically that would be
good.

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

It is probably a good thing that the changes to intercepted flag were
omitted. Since the NAT handling procedures and the TPROXY handling
procedures are mutually exclusive in the TCP socket options handling.
Long term the idea is to adjust the "intercepted" flag to that meaning,
but the individual inerception methods need to be further unwound before
that is completely safe to do.

Please add a new flags.interceptTproxy which is set on TPROXY traffic
regardless of flags.spoofClientIp being set.

Also, the cache_peer "no-tproxy" option needs documentation updates to
mention that it overrides this directive (it prevents *all* requests to
the peer being spoofed, regardless of whether this directive decides it
is okay).

Amos
Received on Fri Dec 21 2012 - 07:27:40 MST