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
