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

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.

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.

>> 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?

> 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.

>>
>> === 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
>>
Received on Fri Aug 10 2012 - 11:40:15 MDT