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
