You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yavor Trapkov <tr...@netscape.net> on 2003/03/09 12:11:02 UTC
[patch] mod_auth_ldap doesn't effectively use the cache with "require
user User1 User2 .." directives
mod_auth_ldap doesn't effectively use the cache with "require user User1
User2
.." directives
What the module does:
- firstly, it checks if the whole string "User1 User2 .." matches the
CN of the
authenticated user and as this is a very rear situation it almost always
fails so each time we request a page, the WEB server sends a LDAP
query as this
is never cached as a negative result
- secondly, there is a loop that checks if every single entry in the list
matches the CN of the authenticated user
= it checks if this is a cached positive result
= and if not it sends a LDAP query
= this happens until it finds a match or the list finishes
if the authenticated user is the 100th one into the "require user User1
User2
.." list we are going to have 99 LDAP requests sent and then the cache will
return the positive answer.
In practise we don't really use the cache here, and such kind of
"require user
User1 User2 .. UserN" directives can send hundreds of LDAP queries
each time we load a web page if the list is long enough! This seems to
be able
to disable some LDAP servers (at least I've experienced this with NW
one). The
real trouble is when we use products like DreamWeaver with WebDAV aceess to
the web site - when opens the site, it makes an inventory sending
PROPFIND to
every object on the site, in such a cases even two UserNames into the
list are
enough to flood the LDAP server.
What I propose to be changed:
- firstly, to check all words into the list only against the cache and
not send
LDAP queries
- secondly, if a match is not found then check all words into the list
sending LDAP queries to the server
- at last, to check for the whole string "user1 user2 .." as this is very
rear case and in almost all cases gives a negative result
(even more sophisticated idea could be to assemble a single "OR"
query and send
to the server)
this requires spitting the util_ldap_cache_compare:
util_ldap_cache_only_compare - checks for a cache match
util_ldap_server_only_compare - sends a request to the LDAP server
A proposed patch follows, I haven't compiled it with httpd2.0, but I
successfully applied a similar one with the auth_ldap module for apache 1.3.
Best Regards
Yavor Trapkov
--- mod_auth_ldap.c.org 2003-03-09 08:15:12.000000000 +0100
+++ mod_auth_ldap.c 2003-03-09 10:34:57.000000000 +0100
@@ -548,30 +548,35 @@
return sec->auth_authoritative? HTTP_UNAUTHORIZED :
DECLINED;
}
/*
- * First do a whole-line compare, in case it's something like
- * require user Babs Jensen
+ * Now break apart the line and compare each word on it
against the cache
*/
- result = util_ldap_cache_compare(r, ldc, sec->url, req->dn,
sec->attribute, t);
- switch(result) {
- case LDAP_COMPARE_TRUE: {
- ap_log_rerror(APLOG_MARK,
APLOG_DEBUG|APLOG_NOERRNO, 0, r,
- "[%d] auth_ldap authorise: "
- "require user: authorisation
successful", getpid());
- return OK;
- }
- default: {
- ap_log_rerror(APLOG_MARK,
APLOG_DEBUG|APLOG_NOERRNO, 0, r,
- "[%d] auth_ldap authorise: require
user: "
- "authorisation failed [%s][%s]",
getpid(),
- ldc->reason, ldap_err2string(result));
+ while (t[0]) {
+ w = ap_getword_conf(r->pool, &t);
+ result = util_ldap_cache_only_compare(r, ldc, sec->url,
req->dn, sec->attribute, w);
+ switch(result) {
+ case LDAP_COMPARE_TRUE: {
+ ap_log_rerror(APLOG_MARK,
APLOG_DEBUG|APLOG_NOERRNO, 0, r,
+ "[%d] auth_ldap authorise: "
+ "require user: authorisation
successful", getpid());
+ return OK;
+ }
+ default: {
+ ap_log_rerror(APLOG_MARK,
APLOG_DEBUG|APLOG_NOERRNO, 0, r,
+ "[%d] auth_ldap authorise: "
+ "require user: authorisation
failed [%s][%s]",
+ getpid(), ldc->reason,
ldap_err2string(result));
+ }
}
}
/*
- * Now break apart the line and compare each word on it
+ * Now break apart the line and compare each word on it
against the LDAP server
*/
+ t = reqs[x].requirement;
+ w = ap_getword(r->pool, &t, ' ');
+
while (t[0]) {
w = ap_getword_conf(r->pool, &t);
- result = util_ldap_cache_compare(r, ldc, sec->url,
req->dn, sec->attribute, w);
+ result = util_ldap_server_only_compare(r, ldc,
sec->url, req->dn, sec->attribute, w);
switch(result) {
case LDAP_COMPARE_TRUE: {
ap_log_rerror(APLOG_MARK,
APLOG_DEBUG|APLOG_NOERRNO, 0, r,
@@ -587,6 +592,29 @@
}
}
}
+ /*
+ * Do a whole-line compare, in case it's something like
"require user Babs Jensen"
+ * as this is a very rear case, we leave it for the end
+ */
+ t = reqs[x].requirement;
+ w = ap_getword(r->pool, &t, ' ');
+
+ result = util_ldap_cache_compare(r, ldc, sec->url, req->dn,
sec->attribute, t);
+ switch(result) {
+ case LDAP_COMPARE_TRUE: {
+ ap_log_rerror(APLOG_MARK,
APLOG_DEBUG|APLOG_NOERRNO, 0, r,
+ "[%d] auth_ldap authorise: "
+ "require user: authorisation
successful", getpid());
+ return OK;
+ }
+ default: {
+ ap_log_rerror(APLOG_MARK,
APLOG_DEBUG|APLOG_NOERRNO, 0, r,
+ "[%d] auth_ldap authorise: require
user: "
+ "authorisation failed [%s][%s]",
getpid(),
+ ldc->reason, ldap_err2string(result));
+ }
+ }
+
}
else if (strcmp(w, "dn") == 0) {
if (req->dn == NULL || strlen(req->dn) == 0) {
--- util_ldap.c.org 2003-03-09 08:46:50.000000000 +0100
+++ util_ldap.c 2003-03-09 10:36:52.000000000 +0100
@@ -733,6 +733,145 @@
return result;
}
+LDAP_DECLARE(int) util_ldap_cache_only_compare(request_rec *r,
util_ldap_connection_t *ldc,
+ const char *url, const char *dn,
+ const char *attrib, const char *value)
+{
+ int result = 0;
+ util_url_node_t *curl;
+ util_url_node_t curnode;
+ util_compare_node_t *compare_nodep;
+ util_compare_node_t the_compare_node;
+ apr_time_t curtime;
+
+ util_ldap_state_t *st =
+ (util_ldap_state_t *)ap_get_module_config(r->server->module_config,
+ &ldap_module);
+
+ /* read lock this function */
+ LDAP_CACHE_LOCK_CREATE(st->pool);
+
+ /* get cache entry (or create one) */
+ LDAP_CACHE_WRLOCK();
+ curnode.url = url;
+ curl = util_ald_cache_fetch(util_ldap_cache, &curnode);
+ if (curl == NULL) {
+ curl = util_ald_create_caches(st, url);
+ }
+ LDAP_CACHE_UNLOCK();
+
+ if (curl) {
+ /* make a comparison to the cache */
+ LDAP_CACHE_RDLOCK();
+ curtime = apr_time_now();
+
+ the_compare_node.dn = (char *)dn;
+ the_compare_node.attrib = (char *)attrib;
+ the_compare_node.value = (char *)value;
+ the_compare_node.result = 0;
+
+ compare_nodep = util_ald_cache_fetch(curl->compare_cache,
&the_compare_node);
+
+ if (compare_nodep != NULL) {
+ /* found it... */
+ if (curtime - compare_nodep->lastcompare >
st->compare_cache_ttl) {
+ /* ...but it is too old */
+ util_ald_cache_remove(curl->compare_cache, compare_nodep);
+ }
+ else {
+ /* ...and it is good */
+ /* unlock this read lock */
+ LDAP_CACHE_UNLOCK();
+ if (LDAP_COMPARE_TRUE == compare_nodep->result) {
+ ldc->reason = "Comparison true (cached)";
+ return compare_nodep->result;
+ }
+ else if (LDAP_COMPARE_FALSE == compare_nodep->result) {
+ ldc->reason = "Comparison false (cached)";
+ return compare_nodep->result;
+ }
+ else if (LDAP_NO_SUCH_ATTRIBUTE == compare_nodep->result) {
+ ldc->reason = "Comparison no such attribute (cached)";
+ return compare_nodep->result;
+ }
+ else {
+ ldc->reason = "Comparison undefined (cached)";
+ return compare_nodep->result;
+ }
+ }
+ }
+ /* unlock this read lock */
+ LDAP_CACHE_UNLOCK();
+ }
+ ldc->reason = "Comparison false (cached)";
+ return result;
+}
+
+LDAP_DECLARE(int) util_ldap_server_only_compare(request_rec *r,
util_ldap_connection_t *ldc,
+ const char *url, const char *dn,
+ const char *attrib, const char *value)
+{
+ int result = 0;
+ util_url_node_t *curl;
+ util_url_node_t curnode;
+ util_compare_node_t *compare_nodep;
+ util_compare_node_t the_compare_node;
+ apr_time_t curtime;
+ int failures = 0;
+
+ util_ldap_state_t *st =
+ (util_ldap_state_t *)ap_get_module_config(r->server->module_config,
+ &ldap_module);
+
+ /* read lock this function */
+ LDAP_CACHE_LOCK_CREATE(st->pool);
+
+start_over:
+ if (failures++ > 10) {
+ /* too many failures */
+ return result;
+ }
+ if (LDAP_SUCCESS != (result = util_ldap_connection_open(r, ldc))) {
+ /* connect failed */
+ return result;
+ }
+
+ if ((result = ldap_compare_s(ldc->ldap, const_cast(dn),
const_cast(attrib), const_cast(value)))
+ == LDAP_SERVER_DOWN) {
+ /* connection failed - try again */
+ util_ldap_connection_close(ldc);
+ ldc->reason = "ldap_compare_s() failed with server down";
+ goto start_over;
+ }
+
+ ldc->reason = "Comparison complete";
+ if ((LDAP_COMPARE_TRUE == result) ||
+ (LDAP_COMPARE_FALSE == result) ||
+ (LDAP_NO_SUCH_ATTRIBUTE == result)) {
+ if (curl) {
+ /* compare completed; caching result */
+ LDAP_CACHE_WRLOCK();
+ the_compare_node.lastcompare = curtime;
+ the_compare_node.result = result;
+ util_ald_cache_insert(curl->compare_cache, &the_compare_node);
+ LDAP_CACHE_UNLOCK();
+ }
+ if (LDAP_COMPARE_TRUE == result) {
+ ldc->reason = "Comparison true (adding to cache)";
+ return LDAP_COMPARE_TRUE;
+ }
+ else if (LDAP_COMPARE_FALSE == result) {
+ ldc->reason = "Comparison false (adding to cache)";
+ return LDAP_COMPARE_FALSE;
+ }
+ else {
+ ldc->reason = "Comparison no such attribute (adding to cache)";
+ return LDAP_NO_SUCH_ATTRIBUTE;
+ }
+ }
+ return result;
+}
+
LDAP_DECLARE(int) util_ldap_cache_checkuserid(request_rec *r,
util_ldap_connection_t *ldc,
const char *url, const char *basedn, int
scope, char **attrs,
const char *filter, const char *bindpw,
const char **binddn,
Re: [patch] mod_auth_ldap doesn't effectively use the cache with
"require user User1 User2 .." directives
Posted by Graham Leggett <mi...@sharp.fm>.
Yavor Trapkov wrote:
> mod_auth_ldap doesn't effectively use the cache with "require user User1
> User2
> .." directives
Why not just use "require group"?
Regards,
Graham
--
-----------------------------------------
minfrin@sharp.fm "There's a moon
over Bourbon Street
tonight..."