Hello,
Am I going crazy here? While working on the bump-ssl-server-first
project, we noticed that authentication does not seem to work right.
Squid debugging shows that a denied user is authenticated but Squid
allows access anyway. The attached patch is what I came up with. Please
review as I am not an ACL expert, and it seems strange to me that such a
big bug would remain unnoticed for so long!
Technical/commit details from the patch preamble:
When AuthenticateAcl() and aclMatchExternal() were converted to use
extended authentication ACL states (r11644 and r11645 dated 2011-08-14),
the result of those function calls was set as the current checklist
answer. This was incorrect because those functions do not make
allow/deny decisions. They only tell us whether the ACL part of the
allow/deny rule matches. If there is a match, the
ACCESS_ALLOWED/ACCESS_DENIED answer depends on whether it is an allow or
deny rule.
For example, "http_access deny BadGuys" should deny access when the
BadGuys ACL matches, but it was allowing access instead.
Thank you,
Alex.
Honor the "deny" part of "foobar deny ACL" options.
When AuthenticateAcl() and aclMatchExternal() were converted to use extended
authentication ACL states (r11644 and r11645 dated 2011-08-14), the result of
those function calls was set as the current checklist answer. This was
incorrect because those functions do not make allow/deny decisions. They only
tell us whether the ACL part of the allow/deny rule matches. If there is a
match, the ACCESS_ALLOWED/ACCESS_DENIED answer depends on whether it is an
allow or deny rule.
For example, "http_access deny BadGuys" should deny access when the BadGuys
ACL matches, but it was allowing access instead.
=== modified file 'src/auth/AclMaxUserIp.cc'
--- src/auth/AclMaxUserIp.cc 2012-01-20 18:55:04 +0000
+++ src/auth/AclMaxUserIp.cc 2012-03-09 19:43:34 +0000
@@ -134,41 +134,40 @@
/* remove _this_ ip, as it is the culprit for going over the limit */
authenticateAuthUserRequestRemoveIp(auth_user_request, src_addr);
debugs(28, 4, "aclMatchUserMaxIP: Denying access in strict mode");
} else {
/*
* non-strict - remove some/all of the cached entries
* ie to allow the user to move machines easily
*/
authenticateAuthUserRequestClearIp(auth_user_request);
debugs(28, 4, "aclMatchUserMaxIP: Denying access in non-strict mode - flushing the user ip cache");
}
return 1;
}
int
ACLMaxUserIP::match(ACLChecklist *cl)
{
ACLFilledChecklist *checklist = Filled(cl);
allow_t answer = AuthenticateAcl(checklist);
- checklist->currentAnswer(answer);
int ti;
// convert to tri-state ACL match 1,0,-1
switch (answer) {
case ACCESS_ALLOWED:
case ACCESS_AUTH_EXPIRED_OK:
// check for a match
ti = match(checklist->auth_user_request, checklist->src_addr);
checklist->auth_user_request = NULL;
return ti;
case ACCESS_DENIED:
case ACCESS_AUTH_EXPIRED_BAD:
return 0; // non-match
case ACCESS_DUNNO:
case ACCESS_AUTH_REQUIRED:
default:
return -1; // other
}
=== modified file 'src/auth/AclProxyAuth.cc'
--- src/auth/AclProxyAuth.cc 2012-01-20 18:55:04 +0000
+++ src/auth/AclProxyAuth.cc 2012-03-09 19:31:12 +0000
@@ -63,41 +63,40 @@
type_ = rhs.type_;
return *this;
}
char const *
ACLProxyAuth::typeString() const
{
return type_;
}
void
ACLProxyAuth::parse()
{
data->parse();
}
int
ACLProxyAuth::match(ACLChecklist *checklist)
{
allow_t answer = AuthenticateAcl(checklist);
- checklist->currentAnswer(answer);
// convert to tri-state ACL match 1,0,-1
switch (answer) {
case ACCESS_ALLOWED:
case ACCESS_AUTH_EXPIRED_OK:
// check for a match
return matchProxyAuth(checklist);
case ACCESS_DENIED:
case ACCESS_AUTH_EXPIRED_BAD:
return 0; // non-match
case ACCESS_DUNNO:
case ACCESS_AUTH_REQUIRED:
default:
return -1; // other
}
}
wordlist *
=== modified file 'src/external_acl.cc'
--- src/external_acl.cc 2012-01-20 18:55:04 +0000
+++ src/external_acl.cc 2012-03-09 19:42:42 +0000
@@ -844,41 +844,40 @@
debugs(82, DBG_IMPORTANT, "WARNING: external ACL '" << acl->def->name <<
"' queue overload. Using stale result. '" << key << "'.");
/* Fall thru to processing below */
}
}
}
}
external_acl_cache_touch(acl->def, entry);
external_acl_message = entry->message.termedBuf();
debugs(82, 2, HERE << acl->def->name << " = " << entry->result);
copyResultsFromEntry(ch->request, entry);
return entry->result;
}
int
ACLExternal::match(ACLChecklist *checklist)
{
allow_t answer = aclMatchExternal(data, Filled(checklist));
- checklist->currentAnswer(answer);
// convert to tri-state ACL match 1,0,-1
switch (answer) {
case ACCESS_ALLOWED:
case ACCESS_AUTH_EXPIRED_OK:
return 1; // match
case ACCESS_DENIED:
case ACCESS_AUTH_EXPIRED_BAD:
return 0; // non-match
case ACCESS_DUNNO:
case ACCESS_AUTH_REQUIRED:
default:
return -1; // other
}
}
wordlist *
ACLExternal::dump() const
