Re: [PATCH] Supply client connection and IDENT information to peer_cache_access ACL check.

I am sending a new patch.
However I believe my first patch is safer for squid-3.2 ...

On 08/10/2012 02:40 PM, Amos Jeffries wrote:
> On 10/08/2012 7:36 p.m., Tsantilas Christos wrote:
>> On 08/10/2012 06:02 AM, Amos Jeffries wrote:
>>> On 10/08/2012 6:54 a.m., Tsantilas Christos wrote:
>>>> Supply client connection and IDENT information to peer_cache_access ACL
>>>> check.
>>>>
>>>> Among other things, this enables SSL client certificate ACL checks
>>>> (user_cert and ca_cert) when making peering decisions
>>>>
>>> It would be better to do this inside the FilledChecklist constructor.
>>> That way all other access lists which pass in HttpRequest can make use
>>> of the details and we can remove duplicate code setting conn()
>>> elsewhere.
>>>
>>> I expect there will be complications from duplicate code with the
>>> assert() that conn_ is only set once. Or places needlessly sending in
>> Exactly.
>> I do not know if it is the right think, unless we make
>> ACLFilledCheckList::conn(ConnStateData *) private, or allow overwriting
>> ACLFilledCheckList::conn_
>> However if found only one case where the
>> ACLFilledCheckList::conn(ConnStateData *) is used, it is inside
>> client_side.cc
>
> Okay. It looks like the clientAclChecklistCreate() is largely useless
> once the conn and ident is pulled out of HttpRequest fields.

Well it sets the ident with a dash not an empty string. Is it possible
to have a valid "-" ident?

>
> If in doubt FilledChecklist::conn(X) can be made to return before the
> assert if the conn_ pointer is being set to its existing value.
ok.

>
>>> ident detail pulled explicitly from the HttpRequest by the caller.
>>> Cleaning those out and using the below would be a good improvement.
>> Well with a second view, setting the ACLFilledCheckList::rfc931 is not
>> required. The ACLIdent will check the
>> ACLFilledCheckList::conn_::clientConnection::rfc931 if the
>> ACLFilledCheckList::conn_ is set and ACLFilledCheckList::rfc931 is not
>> set.
>> I think it is enough and it is better because it allow passing custom
>> ident if needed.
>
> Okay. Excellent.
>
>> I believe the parameters we are passing to checklist need to be
>> reviewed.
>
> Yes they do. Do you have time for it?

Nop.

>
>> As an example look inside src/neighbors.cc near the code I am
>> changing in the patch I sent:
>> ACLFilledChecklist checklist(p->access, request, NULL);
>> checklist.src_addr = request->client_addr;
>> checklist.my_addr = request->my_addr;
>>
>> The checklist.src_addr and checklist.my_addr had already set inside
>> ACLFilledChecklist constructor. Also overwriting the
>> ACLFilledChecklist::src_addr member we are breaking the
>> use_indirect_client_addr feature. Is it normal? Are there cases where
>> this feature must not be used?
>
> Only one case. Inside follow_x_forwarded_for - and I believe that has
> been carefully audited already.
>
> All other places double-setting the src_addr and my_addr can be removed.
> This would actually explain some weird peer selection errors spotted
> recently.

I let it as is now. But I can remove them with a separate patch for
squid trunk if required.

>
>
>>>
>>> === modified file 'src/acl/FilledChecklist.cc'
>>> --- src/acl/FilledChecklist.cc 2012-06-28 18:26:44 +0000
>>> +++ src/acl/FilledChecklist.cc 2012-08-10 01:43:58 +0000
>>> @@ -184,11 +184,19 @@
>>> #endif /* FOLLOW_X_FORWARDED_FOR */
>>> src_addr = request->client_addr;
>>> my_addr = request->my_addr;
>>> +
>>> + if (request->clientConnectionManager.valid())
>>> + conn(request->clientConnectionManager.get());
>>> }
>>>
>>> #if USE_IDENT
>>> if (ident)
>>> xstrncpy(rfc931, ident, USER_IDENT_SZ);
>>> + else if (conn() != NULL) {
>>> + // client connection data may have been provided via
>>> HttpRequest
>>> + if (conn_->clientConnection != NULL &&
>>> conn_->clientConnection->rfc931[0])
>>> + xstrncpy(rfc931, conn_->clientConnection->rfc931,
>>> USER_IDENT_SZ);
>>> + }
>>> #endif
>>> }
>>>
>>>
>>>
>>> Amos
>>>
>
>