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
>>>
>
>
=== modified file 'src/acl/FilledChecklist.cc'
--- src/acl/FilledChecklist.cc 2012-06-28 18:26:44 +0000
+++ src/acl/FilledChecklist.cc 2012-08-11 09:50:27 +0000
@@ -72,40 +72,42 @@
cbdataReferenceDone(conn_);
#if USE_SSL
cbdataReferenceDone(sslErrors);
#endif
debugs(28, 4, HERE << "ACLFilledChecklist destroyed " << this);
}
ConnStateData *
ACLFilledChecklist::conn() const
{
return conn_;
}
void
ACLFilledChecklist::conn(ConnStateData *aConn)
{
+ if (conn() == aConn)
+ return;
assert (conn() == NULL);
conn_ = cbdataReference(aConn);
}
int
ACLFilledChecklist::fd() const
{
return (conn_ != NULL && conn_->clientConnection != NULL) ? conn_->clientConnection->fd : fd_;
}
void
ACLFilledChecklist::fd(int aDescriptor)
{
assert(!conn() || conn()->clientConnection == NULL || conn()->clientConnection->fd == aDescriptor);
fd_ = aDescriptor;
}
bool
ACLFilledChecklist::destinationDomainChecked() const
{
@@ -167,28 +169,31 @@
sourceDomainChecked_(false)
{
my_addr.SetEmpty();
src_addr.SetEmpty();
dst_addr.SetEmpty();
rfc931[0] = '\0';
// cbdataReferenceDone() is in either fastCheck() or the destructor
if (A)
accessList = cbdataReference(A);
if (http_request != NULL) {
request = HTTPMSGLOCK(http_request);
#if FOLLOW_X_FORWARDED_FOR
if (Config.onoff.acl_uses_indirect_client)
src_addr = request->indirect_client_addr;
else
#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);
#endif
}
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2012-08-06 17:21:57 +0000
+++ src/client_side.cc 2012-08-12 08:06:44 +0000
@@ -3705,41 +3705,40 @@
if (!sslServerBump) {
assert(port->signingCert.get());
certProperties.signWithX509.resetAndLock(port->signingCert.get());
if (port->signPkey.get())
certProperties.signWithPkey.resetAndLock(port->signPkey.get());
certProperties.signAlgorithm = Ssl::algSignTrusted;
return;
}
// In case of an error while connecting to the secure server, use a fake
// trusted certificate, with no mimicked fields and no adaptation
// algorithms. There is nothing we can mimic so we want to minimize the
// number of warnings the user will have to see to get to the error page.
assert(sslServerBump->entry);
if (sslServerBump->entry->isEmpty()) {
if (X509 *mimicCert = sslServerBump->serverCert.get())
certProperties.mimicCert.resetAndLock(mimicCert);
ACLFilledChecklist checklist(NULL, sslServerBump->request,
clientConnection != NULL ? clientConnection->rfc931 : dash_str);
- checklist.conn(this);
checklist.sslErrors = cbdataReference(sslServerBump->sslErrors);
for (sslproxy_cert_adapt *ca = Config.ssl_client.cert_adapt; ca != NULL; ca = ca->next) {
// If the algorithm already set, then ignore it.
if ((ca->alg == Ssl::algSetCommonName && certProperties.setCommonName) ||
(ca->alg == Ssl::algSetValidAfter && certProperties.setValidAfter) ||
(ca->alg == Ssl::algSetValidBefore && certProperties.setValidBefore) )
continue;
if (ca->aclList && checklist.fastCheck(ca->aclList) == ACCESS_ALLOWED) {
const char *alg = Ssl::CertAdaptAlgorithmStr[ca->alg];
const char *param = ca->param;
// For parameterless CN adaptation, use hostname from the
// CONNECT request.
if (ca->alg == Ssl::algSetCommonName) {
if (!param)
param = sslConnectHostOrIp.termedBuf();
certProperties.commonName = param;
certProperties.setCommonName = true;
@@ -4211,50 +4210,40 @@
*/
debugs(33, DBG_IMPORTANT, "varyEvaluateMatch: Oops. Not a Vary match on second attempt, '" <<
entry->mem_obj->url << "' '" << vary << "'");
return VARY_CANCEL;
}
}
}
ACLFilledChecklist *
clientAclChecklistCreate(const acl_access * acl, ClientHttpRequest * http)
{
ConnStateData * conn = http->getConn();
ACLFilledChecklist *ch = new ACLFilledChecklist(acl, http->request,
cbdataReferenceValid(conn) && conn != NULL && conn->clientConnection != NULL ? conn->clientConnection->rfc931 : dash_str);
/*
* hack for ident ACL. It needs to get full addresses, and a place to store
* the ident result on persistent connections...
*/
/* connection oriented auth also needs these two lines for it's operation. */
- /*
- * Internal requests do not have a connection reference, because: A) their
- * byte count may be transformed before being applied to an outbound
- * connection B) they are internal - any limiting on them should be done on
- * the server end.
- */
-
- if (conn != NULL)
- ch->conn(conn); /* unreferenced in FilledCheckList.cc */
-
return ch;
}
CBDATA_CLASS_INIT(ConnStateData);
ConnStateData::ConnStateData() :
AsyncJob("ConnStateData"),
#if USE_SSL
sslBumpMode(Ssl::bumpEnd),
switchedToHttps_(false),
sslServerBump(NULL),
#endif
stoppedSending_(NULL),
stoppedReceiving_(NULL)
{
pinning.pinned = false;
pinning.auth = false;
}
bool
=== modified file 'src/neighbors.cc'
--- src/neighbors.cc 2012-08-06 17:41:08 +0000
+++ src/neighbors.cc 2012-08-11 09:51:35 +0000
@@ -177,51 +177,40 @@
bool do_ping = false;
for (d = p->peer_domain; d; d = d->next) {
if (0 == matchDomainName(request->GetHost(), d->domain)) {
do_ping = d->do_ping;
break;
}
do_ping = !d->do_ping;
}
if (p->peer_domain && !do_ping)
return false;
if (p->access == NULL)
return do_ping;
ACLFilledChecklist checklist(p->access, request, NULL);
checklist.src_addr = request->client_addr;
checklist.my_addr = request->my_addr;
-#if 0 && USE_IDENT
- /*
- * this is currently broken because 'request->user_ident' has been
- * moved to conn->rfc931 and we don't have access to the parent
- * ConnStateData here.
- */
- if (request->user_ident[0])
- xstrncpy(checklist.rfc931, request->user_ident, USER_IDENT_SZ);
-
-#endif
-
return (checklist.fastCheck() == ACCESS_ALLOWED);
}
/* Return TRUE if it is okay to send an ICP request to this peer. */
static int
peerWouldBePinged(const peer * p, HttpRequest * request)
{
if (p->icp.port == 0)
return 0;
if (p->options.no_query)
return 0;
if (p->options.mcast_responder)
return 0;
if (p->n_addresses == 0)
return 0;
if (p->options.background_ping && (squid_curtime - p->stats.last_query < Config.backgroundPingRate))
