Re: [PATCH] ACL to control TPROXY spoofing

On 26/02/2013 11:30 a.m., Amos Jeffries wrote:
> On 26/02/2013 5:34 a.m., Steve Hill wrote:
>> Ok, I've had time to clean this patch up... I'm not sure how half my
>> patch went missing the last time I sent it - I was obviously having a
>> bad day. :)
>>
>> The attached patch adds a "spoof_client_ip" 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_client_ip deny all
>>
>> I've implemented the changes suggested by both Alex and Amos.
>>
>> 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. TPROXY requests now all set flags.interceptTproxy, irrespective of
>> whether there is going to be any address spoofing.
>>
>
> Hi Steve,
>
>
> I still have the question about whether it is necessary to have this
> as a full-blown ACL test, or if a flag on the http_port is sufficient.
> - ACLs are more flexible, but a Fast-ACL lookup only goes halfway
> towards supporting all the Slow-ACL things people will be tempted to
> use there.
> - The main use-cases supporting this as to noted will simply be "deny
> all" or "allow all".
> - Using this directive makes TPROXY into an equivalent of NAT, and
> since there is now NAT in both IPv4 and IPv6 firewalls no benefits
> over just using NAT rules in the firewall to begin with.
>
> I'm on the fence for this, but definitely want to see a clear use-case
> need before we add yet another ACL lookup to Squid.
>
>
> Back to the audit:
>
> 1) 3.3 is closed to new features now. This will have to be targeted at
> 3.HEAD for merge.
>
> 2) Kinkie and I have since been doing some more code polishing towards
> 3.4 release. Amongst this was a small rationalization of the PortCfg
> mode flags including the spoofClientIp one which is now
> PortCfg::flags::tproxyIntercept. Several of the chunks of your patch
> doing the re-naming can be removed when targeting 3.HEAD.
>
> 3) in src/client_side.cc
>
> - the spoofing decision should probably be made inside
> tcpOutgoingAddress() where a lot more transaction state has been
> determined.
> ** this opens up a third implementation posibility - that we extend
> tcp_outgoing_address directive with a flag to signal that the IP
> listed may be selected for use even when TPROXY is used.

Err. Forget the above request, it won't work.

>
> - for the sslBump fakeRequest please use HttpRequest::Pointer instead
> of HttpRequest*X= HTTPMSGLOCK(...); and HTTPMSGUNLOCK(X).
>
> Amos
Received on Mon Feb 25 2013 - 22:48:14 MST