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

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

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

I believe the parameters we are passing to checklist need to be
reviewed. 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?

>
>
> === 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 - 07:36:44 MDT