Re: [PATCH] %>la for intercepted connections

On 09/07/2011 09:38 PM, Amos Jeffries wrote:
> On 08/09/11 02:21, Tsantilas Christos wrote:
>> On 09/07/2011 05:03 AM, Amos Jeffries wrote:
>>> + case LFT_LOCAL_LISTENING_IP:
>>> + if (al->cache.port == NULL) {
>>> + break; // dash
>>> + } else if (al->cache.port->s.IsAnyAddr()) {
>>> + if (al->tcpClient != NULL &&
>>> + !(al->request != NULL && (al->request->flags.spoof_client_ip ||
>>> al->request->flags.intercepted))) {
>>> + out = al->tcpClient->local.NtoA(tmp, sizeof(tmp));
>>> + } // else dash.
>>> + } else {
>>> + out = al->cache.port->s.NtoA(tmp, sizeof(tmp));
>>> + }
>>> + break;

>> My original code was not good, I agree...
>> I have in my mind something like this:
>> case LFT_LOCAL_LISTENING_IP:
>> + if ((al->request->flags.spoof_client_ip ||
>> al->request->flags.intercepted) && al->cache.port) {
>> + if (!al->cache.port->s.IsAnyAddr())
>> + out = al->cache.port->s.NtoA(tmp, sizeof(tmp));
>> + } else if (al->tcpClient != NULL)
>> + out = al->tcpClient->local.NtoA(tmp, sizeof(tmp));
>> + break;
>>
>> If it is an intercepted connection use the listening address (if it is
>> available), else use the IP address used by the client to connect to
>> squid. Is it OK?

> That seems okay. Confusing to read, but is slightly more efficient than
> what I suggested with the same cases covered.

To be honest, neither version is readable to an uninitiated. May I
suggest giving each condition a name and adding comments? For example
(with wrong names/comments, I am sure),

    // avoid logging a dash if we have reliable info
    const bool interceptedAtKnownPort =
        (al->request->flags.spoof_client_ip ||
         al->request->flags.intercepted) && al->cache.port);
    if (interceptedAtKnownPort) {
        const bool portAddressConfigured =
            !al->cache.port->s.IsAnyAddr()
        if (portAddressConfigured)
        ... // log listening address
    ...

Another option would be to add methods to al->request and al->cache.port
objects instead of local variables. That way, the same potentially
useful condition can be reused throughout the code.

> For the record these are the permutations we seek to cover...
>
>
> Scenario 1: client 192.168.0.3 connects to google (74.125.237.81). Gets
> intercepted into Squid.
>
> 1a) squid.conf: http_port 3129 intercept|tproxy
>
> tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
> tcpClient->local == 74.125.237.81:80 (%>la:%>lp)
> al->cache.port->s.local == 0.0.0.0:3129 (%la:%lp) [log "-"]
>
>
> 1b) squid.conf: http_port 192.168.0.1:3129 intercept|tproxy
>
> tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
> tcpClient->local == 74.125.237.81:80 (%>la:%>lp)
> al->cache.port->s.local == 192.168.0.1:3129 (%la:%lp) [log 192...]
>
>
> Scenario 2: client 192.168.0.3 connects to Squid asking for
> http://google.com
>
> 2a) squid.conf: http_port 3128 [accel]
>
> tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
> tcpClient->local == 192.168.0.1:3128 (%>la:%>lp)
> al->cache.port->s.local == 0.0.0.0:3128 (%la:%lp) [log 192...]
>
> 2b) squid.conf: http_port 192.168.0.1:3128 [accel]
>
> tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
> tcpClient->local == 192.168.0.1:3128 (%>la:%>lp)
> al->cache.port->s.local == 192.168.0.1:3128 (%la:%lp) [log 192...]
>
>
> Senario 3: squid generates an internal request.
>
> tcpClient == NULL (%>a:%>p,%>la:%>lp) [log "-"]
> al->cache.port == NULL (%la:%lp) [log "-"]

Perhaps the above list should be added to the commit message in case we
need to reshuffle this code later? It sounds rather valuable to me.

Thank you,

Alex.
Received on Thu Sep 08 2011 - 04:19:11 MDT