Sorry,
I did a mistake I send an intermediate patch. This one is the
correct. The comments in my previous mail are correct (I hope).
I am marking the new patch as v4.
On 06/10/2011 07:58 PM, Tsantilas Christos wrote:
Hi all,
The problem has been discussed here:
http://www.squid-cache.org/mail-archive/squid-dev/201105/0116.html>
This patch add the method ACLChecklist::reusableFastCheck which is
similar to the ACLChecklist::fastCheck method but it can be called many
times, and each time called a new independed access list check done.
Also I found some problems in ACLChecklist::fastCheck which the current
patch does not try to solve them, but we may take care of.
1) Documentation of ::fastCheck does not match implementation
--------------------------------------------------------------
Currently the documentation of the ACLChecklist::fastCheck does not
match the implementation. From the documentation:
* If there is no access list to check the default is to return DENIED.
...
* \retval 1/true Access Allowed
* \retval 0/false Access Denied
But in the case no access list exist (accessList = NULL) we have inside
::fastCheck():
currentAnswer(ACCESS_DENIED);
while (accessList) {
....
}
return currentAnswer() == ACCESS_DENIED;
In the case no access list defined, it return true ("access allowed").
We should fix the ::fastCheck or its documentation.
2) Bug in ::fastCheck() method
-------------------------------
Now, when the ::fastCheck() called for a second time or third time, the
accessList is NULL always:
- if not match because list exhausted in first call
- if match because of the "cbdataReferenceDone(accessList);" line
(src/acl/Checklist.cc line 350).
So a second call of the ::fastCheck() will return access allowed, even
if the initial decision was access denied...
This is not normal (I think it is a bug).
I propose to replace the old ::fastCheck() method with the new
::reusableFastCheck() code so the fastCheck() calls have simpler
semantics and are always reusable.
New ACLChecklist method to make ACLChecklist objects reusable
This patch investigates the ACLChecklist::reusableFastCheck method which
is similar to the ACLChecklist::fastCheck method.
The new methd does not change the internal state of the ACLChecklist
object and each execution run an indepeneded access list check.
=== modified file 'src/acl/Checklist.cc'
--- src/acl/Checklist.cc 2009-03-08 19:41:27 +0000
+++ src/acl/Checklist.cc 2011-06-10 17:04:54 +0000
@@ -352,40 +352,63 @@
}
/*
* Reference the next access entry
*/
const acl_access *A = accessList;
assert (A);
accessList = cbdataReference(A->next);
cbdataReferenceDone(A);
}
debugs(28, 5, "aclCheckFast: no matches, returning: " << (currentAnswer() == ACCESS_DENIED));
PROF_stop(aclCheckFast);
return currentAnswer() == ACCESS_DENIED;
}
+bool
+ACLChecklist::reusableFastCheck()
+{
+ currentAnswer(ACCESS_DENIED);
+ debugs(28, 5, "reusableCheckFast: list: " << accessList);
+ const acl_access *acl = cbdataReference(accessList);
+ while (acl != NULL && cbdataReferenceValid(acl)) {
+ currentAnswer(acl->allow);
+ if (matchAclListFast(acl->aclList)) {
+ cbdataReferenceDone(acl);
+ return currentAnswer() == ACCESS_ALLOWED;
+ }
+
+ /*
+ * Reference the next access entry
+ */
+ const acl_access *A = acl;
+ acl = cbdataReference(acl->next);
+ cbdataReferenceDone(A);
+ }
+ debugs(28, 5, "reusableCheckFast: no matches, returning: " << (currentAnswer() == ACCESS_DENIED));
+ return currentAnswer() == ACCESS_DENIED;
+}
bool
ACLChecklist::checking() const
{
return checking_;
}
void
ACLChecklist::checking (bool const newValue)
{
checking_ = newValue;
}
bool
ACLChecklist::callerGone()
{
return !cbdataReferenceValid(callback_data);
}
bool
=== modified file 'src/acl/Checklist.h'
--- src/acl/Checklist.h 2009-04-07 13:51:57 +0000
+++ src/acl/Checklist.h 2011-06-10 12:42:35 +0000
@@ -96,40 +96,48 @@
void nonBlockingCheck(PF * callback, void *callback_data);
/**
* Trigger a blocking access check for a set of *_access options.
*
* ACLs which cannot be satisfied directly from available data are ignored.
* This means any proxy_auth, external_acl, DNS lookups, Ident lookups etc
* which have not already been performed and cached will not be checked.
*
* If there is no access list to check the default is to return DENIED.
* However callers should perform their own check and default based on local
* knowledge of the ACL usage rather than depend on this default.
* That will also save on work setting up ACLChecklist fields for a no-op.
*
* \retval 1/true Access Allowed
* \retval 0/false Access Denied
*/
int fastCheck();
/**
+ * Trigger a blocking access check for a given access list
+ * The behavior and the result is similar to the ::fastCheck method,
+ * but this method does not change the internal state of the ACLChecklist
+ * object and each execution run an indepeneded access list check.
+ */
+ bool reusableFastCheck();
+
+ /**
* Trigger a blocking access check for a single ACL line (a AND b AND c).
*
* ACLs which cannot be satisfied directly from available data are ignored.
* This means any proxy_auth, external_acl, DNS lookups, Ident lookups etc
* which have not already been performed and cached will not be checked.
*
* \retval 1/true Access Allowed
* \retval 0/false Access Denied
*/
bool matchAclListFast(const ACLList * list);
/**
* Attempt to check the current checklist against current data.
* This is the core routine behind all ACL test routines.
* As much as possible of current tests are performed immediately
* and the result is maybe delayed to wait for async lookups.
*
* When all tests are done callback is presented with one of:
* - ACCESS_ALLOWED Access explicitly Allowed
* - ACCESS_DENIED Access explicitly Denied
=== modified file 'src/ssl/support.cc'
--- src/ssl/support.cc 2011-05-13 07:59:19 +0000
+++ src/ssl/support.cc 2011-06-10 16:09:21 +0000
@@ -226,41 +226,41 @@
debugs(83, 2, "SQUID_X509_V_ERR_DOMAIN_MISMATCH: Certificate " << buffer << " does not match domainname " << server);
ok = 0;
error_no = SQUID_X509_V_ERR_DOMAIN_MISMATCH;
if (check)
Filled(check)->ssl_error = SQUID_X509_V_ERR_DOMAIN_MISMATCH;
}
}
} else {
error_no = ctx->error;
if (const char *err_descr = Ssl::GetErrorDescr(ctx->error))
debugs(83, 5, err_descr << ": " << buffer);
else
debugs(83, 1, "SSL unknown certificate error " << ctx->error << " in " << buffer);
if (check)
Filled(check)->ssl_error = ctx->error;
}
if (!ok && check) {
- if (check->fastCheck()) {
+ if (check->reusableFastCheck()) {
debugs(83, 3, "bypassing SSL error " << ctx->error << " in " << buffer);
ok = 1;
} else {
debugs(83, 5, "confirming SSL error " << ctx->error);
}
}
if (!dont_verify_domain && server) {}
if (error_no != SSL_ERROR_NONE && !SSL_get_ex_data(ssl, ssl_ex_index_ssl_error_detail) ) {
Ssl::ErrorDetail *errDetail = new Ssl::ErrorDetail(error_no, peer_cert);
if (!SSL_set_ex_data(ssl, ssl_ex_index_ssl_error_detail, errDetail)) {
debugs(83, 2, "Failed to set Ssl::ErrorDetail in ssl_verify_cb: Certificate " << buffer);
delete errDetail;
}
}
return ok;
}
