Re: [PATCH] ACL to control TPROXY spoofing

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.

- for the sslBump fakeRequest please use HttpRequest::Pointer instead of
HttpRequest*X= HTTPMSGLOCK(...); and HTTPMSGUNLOCK(X).

Amos
Received on Mon Feb 25 2013 - 22:30:49 MST