Update External ACL helpers error handling and caching

The helper protocol for external ACLs [1] defines three possible return values:
   OK - Success. ACL test matches.
   ERR - Success. ACL test fails to match.
   BH - Failure. The helper encountered a problem.

The external acl helpers distributed with squid currently do not follow this
definition. For example, upon connection error, ERR is returned:

   $ ext_ldap_group_acl ... -d
   ext_ldap_group_acl: WARNING: could not bind to binddn 'Can't contact LDAP server'
   ERR

This does not allow to distinguish "no match" and "error" either and
therefore negative caches "ERR", also in the case of an error.

Moreover there are multiple problems inside squid when trying to handle BH
responses:
  - Squid-5 and Squid-4 retry requests for BH responses but crashes after the
    maximum retry number (currently 2) is reached.
  - If an external acl helper return always BH (eg because the LDAP server is
    down) squid sends infinitely new request to the helper.

This is a Measurement Factory project

diff --git a/helpers/defines.h b/helpers/defines.h
index 2f2d2615efbf8f4118170d83707a565e59612440..0f8e888550ab91aa7259553ba8888ee2be7324a3 100644 (file)
--- a/helpers/defines.h
+++ b/helpers/defines.h
@@ -51,9 +51,12 @@
 /* send ERR result to Squid with a string parameter. */
 #define SEND_ERR(x) fprintf(stdout, "ERR %s\n",x)
 
-/* send ERR result to Squid with a string parameter. */
+/* send BH result to Squid with a string parameter. */
 #define SEND_BH(x)  fprintf(stdout, "BH %s\n",x)
 
+/* constructs a message to Squid. */
+#define HLP_MSG(text)  "message=\"" text "\""
+
 /* send TT result to Squid with a string parameter. */
 #define SEND_TT(x)  fprintf(stdout, "TT %s\n",x)
 

diff --git a/helpers/external_acl/AD_group/ext_ad_group_acl.cc b/helpers/external_acl/AD_group/ext_ad_group_acl.cc
index d1a642aab05ce8bb95178ea48221c378321ab322..4cc6e8dbbd0c59942868840926b4a3c3fdabe09f 100644 (file)
--- a/helpers/external_acl/AD_group/ext_ad_group_acl.cc
+++ b/helpers/external_acl/AD_group/ext_ad_group_acl.cc
@@ -819,7 +819,7 @@ main(int argc, char *argv[])
                 if (strchr(buf, '\n') != NULL)
                     break;
             }
-            SEND_ERR("Invalid Request. Too Long.");
+            SEND_BH(HLP_MSG("Invalid Request. Too Long."));
             continue;
         }
         if ((p = strchr(buf, '\n')) != NULL)
@@ -830,7 +830,7 @@ main(int argc, char *argv[])
         debug("Got '%s' from Squid (length: %d).\n", buf, strlen(buf));
 
         if (buf[0] == '\0') {
-            SEND_ERR("Invalid Request. No Input.");
+            SEND_BH(HLP_MSG("Invalid Request. No Input."));
             continue;
         }
         username = strtok(buf, " ");
@@ -842,7 +842,7 @@ main(int argc, char *argv[])
         numberofgroups = n;
 
         if (NULL == username) {
-            SEND_ERR("Invalid Request. No Username.");
+            SEND_BH(HLP_MSG("Invalid Request. No Username."));
             continue;
         }
         rfc1738_unescape(username);

diff --git a/helpers/external_acl/LDAP_group/ext_ldap_group_acl.cc b/helpers/external_acl/LDAP_group/ext_ldap_group_acl.cc
index 01fad7a9302498e29aab138d96210b1b7ccf5c03..f61809720d990db6b688af1fb2a6fe301ab8bec7 100644 (file)
--- a/helpers/external_acl/LDAP_group/ext_ldap_group_acl.cc
+++ b/helpers/external_acl/LDAP_group/ext_ldap_group_acl.cc
@@ -466,13 +466,13 @@ main(int argc, char **argv)
                 if (strchr(buf, '\n') != NULL)
                     break;
             }
-            SEND_ERR("");
+            SEND_BH(HLP_MSG("Input too large"));
             continue;
         }
         user = strtok(buf, " \n");
         if (!user) {
             debug("%s: Invalid request: No Username given\n", argv[0]);
-            SEND_ERR("Invalid request. No Username");
+            SEND_BH(HLP_MSG("Invalid request. No Username"));
             continue;
         }
         rfc1738_unescape(user);
@@ -495,11 +495,12 @@ main(int argc, char **argv)
             extension_dn = strtok(NULL, " \n");
             if (!extension_dn) {
                 debug("%s: Invalid request: Extension DN configured, but none sent.\n", argv[0]);
-                SEND_ERR("Invalid Request. Extension DN required.");
+                SEND_BH(HLP_MSG("Invalid Request. Extension DN required"));
                 continue;
             }
             rfc1738_unescape(extension_dn);
         }
+        const char *broken = nullptr;
         while (!found && user && (group = strtok(NULL, " \n")) != NULL) {
             rfc1738_unescape(group);
 
@@ -509,6 +510,7 @@ recover:
                 if (strstr(ldapServer, "://") != NULL) {
                     rc = ldap_initialize(&ld, ldapServer);
                     if (rc != LDAP_SUCCESS) {
+                        broken = HLP_MSG("Unable to connect to LDAP server");
                         fprintf(stderr, "%s: ERROR: Unable to connect to LDAPURI:%s\n", argv[0], ldapServer);
                         break;
                     }
@@ -530,7 +532,8 @@ recover:
                     } else
 #endif
                         if ((ld = ldap_init(ldapServer, port)) == NULL) {
-                            fprintf(stderr, "ERROR: Unable to connect to LDAP server:%s port:%d\n", ldapServer, port);
+                            broken = HLP_MSG("Unable to connect to LDAP server");
+                            fprintf(stderr, "ERROR: %s:%s port:%d\n", broken, ldapServer, port);
                             break;
                         }
                 if (connect_timeout)
@@ -541,8 +544,8 @@ recover:
                     version = LDAP_VERSION3;
                 }
                 if (ldap_set_option(ld, LDAP_OPT_PROTOCOL_VERSION, &version) != LDAP_SUCCESS) {
-                    fprintf(stderr, "ERROR: Could not set LDAP_OPT_PROTOCOL_VERSION %d\n",
-                            version);
+                    broken = HLP_MSG("Could not set LDAP_OPT_PROTOCOL_VERSION");
+                    fprintf(stderr, "ERROR: %s %d\n", broken, version);
                     ldap_unbind(ld);
                     ld = NULL;
                     break;
@@ -553,7 +556,8 @@ recover:
                         fprintf(stderr, "FATAL: TLS requires LDAP version 3\n");
                         exit(1);
                     } else if (ldap_start_tls_s(ld, NULL, NULL) != LDAP_SUCCESS) {
-                        fprintf(stderr, "ERROR: Could not Activate TLS connection\n");
+                        broken = HLP_MSG("Could not Activate TLS connection");
+                        fprintf(stderr, "ERROR: %s\n", broken);
                         ldap_unbind(ld);
                         ld = NULL;
                         break;
@@ -570,7 +574,8 @@ recover:
                 if (binddn && bindpasswd && *binddn && *bindpasswd) {
                     rc = ldap_simple_bind_s(ld, binddn, bindpasswd);
                     if (rc != LDAP_SUCCESS) {
-                        fprintf(stderr, PROGRAM_NAME ": WARNING: could not bind to binddn '%s'\n", ldap_err2string(rc));
+                        broken = HLP_MSG("could not bind");
+                        fprintf(stderr, PROGRAM_NAME ": WARNING: %s to binddn '%s'\n", broken, ldap_err2string(rc));
                         ldap_unbind(ld);
                         ld = NULL;
                         break;
@@ -578,20 +583,24 @@ recover:
                 }
                 debug("Connected OK\n");
             }
-            if (searchLDAP(ld, group, user, extension_dn) == 0) {
+            int searchResult = searchLDAP(ld, group, user, extension_dn);
+            if (searchResult == 0) {
                 found = 1;
                 break;
-            } else {
+            } else if (searchResult < 0){
                 if (tryagain) {
                     tryagain = 0;
                     ldap_unbind(ld);
                     ld = NULL;
                     goto recover;
                 }
+                broken = HLP_MSG("LDAP search error");
             }
         }
         if (found)
             SEND_OK("");
+        else if (broken)
+            SEND_BH(broken);
         else {
             SEND_ERR("");
         }
@@ -713,7 +722,7 @@ searchLDAPGroup(LDAP * ld, char *group, char *member, char *extension_dn)
 
     if (build_filter(filter, sizeof(filter), searchfilter, member, group) != 0) {
         fprintf(stderr, PROGRAM_NAME ": ERROR: Failed to construct LDAP search filter. filter=\"%s\", user=\"%s\", group=\"%s\"\n", filter, member, group);
-        return 1;
+        return -1;
     }
     debug("group filter '%s', searchbase '%s'\n", filter, searchbase);
 
@@ -732,7 +741,7 @@ searchLDAPGroup(LDAP * ld, char *group, char *member, char *extension_dn)
             }
 #endif
             ldap_msgfree(res);
-            return 1;
+            return -1;
         }
     }
     entry = ldap_first_entry(ld, res);
@@ -779,7 +788,7 @@ searchLDAP(LDAP * ld, char *group, char *login, char *extension_dn)
                 }
 #endif
                 ldap_msgfree(res);
-                return 1;
+                return -1;
             }
         }
         entry = ldap_first_entry(ld, res);

diff --git a/helpers/external_acl/LM_group/ext_lm_group_acl.cc b/helpers/external_acl/LM_group/ext_lm_group_acl.cc
index fb73be3f837fa2a3b1aeb2ca4c821537e2591096..9135fd2d5ed8a2747f548229cab2dd3a74db75f4 100644 (file)
--- a/helpers/external_acl/LM_group/ext_lm_group_acl.cc
+++ b/helpers/external_acl/LM_group/ext_lm_group_acl.cc
@@ -561,7 +561,7 @@ main(int argc, char *argv[])
                 if (strchr(buf, '\n') != NULL)
                     break;
             }
-            SEND_ERR("Input Too Long.");
+            SEND_BH(HLP_MSG("Input Too Long."));
             continue;
         }
         if ((p = strchr(buf, '\n')) != NULL)
@@ -572,7 +572,7 @@ main(int argc, char *argv[])
         debug("Got '%s' from Squid (length: %d).\n", buf, strlen(buf));
 
         if (buf[0] == '\0') {
-            SEND_ERR("Invalid Request.");
+            SEND_BH(HLP_MSG("Invalid Request."));
             continue;
         }
         username = strtok(buf, " ");
@@ -583,7 +583,7 @@ main(int argc, char *argv[])
         groups[n] = NULL;
 
         if (NULL == username) {
-            SEND_ERR("Invalid Request. No Username.");
+            SEND_BH(HLP_MSG("Invalid Request. No Username."));
             continue;
         }
         rfc1738_unescape(username);

diff --git a/helpers/external_acl/SQL_session/ext_sql_session_acl.pl.in b/helpers/external_acl/SQL_session/ext_sql_session_acl.pl.in
index 7e28f276b819a8346ccf87db771a4c35dfc5d15c..7b34c730d5bac9900684b7294e0f7ef1558f53a9 100755 (executable)
--- a/helpers/external_acl/SQL_session/ext_sql_session_acl.pl.in
+++ b/helpers/external_acl/SQL_session/ext_sql_session_acl.pl.in
@@ -195,7 +195,7 @@ while (<>) {
 
     print(stderr "Received: Channel=".$cid.", UID='".$uid."'\n") if ($debug);
 
-    $status = $cid . " ERR message=\"database error\"";
+    $status = $cid . " BH message=\"database error\"";
     my $sth = query_db($uid) || next;
     print(stderr "Rows: ". $sth->rows()."\n") if ($debug);
     $status = $cid . " ERR message=\"unknown UID '".$uid."'\"";

diff --git a/helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc b/helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc
index a5ea8390d3423622e8dabf974bfc37ef16dad846..63609e443e0fd8ef0c13fba46135c316d2445b1e 100644 (file)
--- a/helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc
+++ b/helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc
@@ -1758,7 +1758,7 @@ MainSafe(int argc, char **argv)
         /* No space given, but group string is required --> ERR */
         if ((edui_conf.mode & EDUI_MODE_GROUP) && (p == NULL)) {
             debug("while() -> Search group is missing. (required)\n");
-            local_printfx("ERR message=\"(Search Group Required)\"\n");
+            local_printfx("BH message=\"(Search Group Required)\"\n");
             continue;
         }
         x = 0;
@@ -1801,7 +1801,7 @@ MainSafe(int argc, char **argv)
             if (x != LDAP_ERR_SUCCESS) {
                 /* Unable to bind */
                 debug("BindLDAP() -> %s (LDAP: %s)\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
-                local_printfx("ERR message=\"(BindLDAP: %s - %s)\"\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
+                local_printfx("BH message=\"(BindLDAP: %s - %s)\"\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
                 continue;
             } else
                 debug("BindLDAP(-, %s, %s, (LDAP_AUTH_TLS)) -> %s\n", edui_conf.dn, edui_conf.passwd, ErrLDAP(x));
@@ -1812,7 +1812,7 @@ MainSafe(int argc, char **argv)
                 if (x != LDAP_ERR_SUCCESS) {
                     /* Unable to bind */
                     debug("BindLDAP() -> %s (LDAP: %s)\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
-                    local_printfx("ERR message=\"(BindLDAP: %s - %s)\"\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
+                    local_printfx("BH message=\"(BindLDAP: %s - %s)\"\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
                     continue;
                 } else
                     debug("BindLDAP(-, %s, %s, (LDAP_AUTH_SIMPLE)) -> %s\n", edui_conf.dn, edui_conf.passwd, ErrLDAP(x));
@@ -1822,7 +1822,7 @@ MainSafe(int argc, char **argv)
                 if (x != LDAP_ERR_SUCCESS) {
                     /* Unable to bind */
                     debug("BindLDAP() -> %s (LDAP: %s)\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
-                    local_printfx("ERR message=\"(BindLDAP: %s - %s)\"\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
+                    local_printfx("BH message=\"(BindLDAP: %s - %s)\"\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
                     continue;
                 } else
                     debug("BindLDAP(-, -, -, (LDAP_AUTH_NONE)) -> %s\n", ErrLDAP(x));
@@ -1841,7 +1841,7 @@ MainSafe(int argc, char **argv)
             /* Everything failed --> ERR */
             debug("while() -> %s (LDAP: %s)\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
             CloseLDAP(&edui_ldap);
-            local_printfx("ERR message=\"(General Failure: %s)\"\n", ErrLDAP(x));
+            local_printfx("BH message=\"(General Failure: %s)\"\n", ErrLDAP(x));
             continue;
         }
         edui_ldap.err = -1;
@@ -1856,14 +1856,14 @@ MainSafe(int argc, char **argv)
                 x = ConvertIP(&edui_ldap, bufb);
                 if (x < 0) {
                     debug("ConvertIP() -> %s\n", ErrLDAP(x));
-                    local_printfx("ERR message=\"(ConvertIP: %s)\"\n", ErrLDAP(x));
+                    local_printfx("BH message=\"(ConvertIP: %s)\"\n", ErrLDAP(x));
                 } else {
                     edui_ldap.err = -1;
                     debug("ConvertIP(-, %s) -> Result[%d]: %s\n", bufb, x, edui_ldap.search_ip);
                     x = SearchFilterLDAP(&edui_ldap, bufa);
                     if (x < 0) {
                         debug("SearchFilterLDAP() -> %s\n", ErrLDAP(x));
-                        local_printfx("ERR message=\"(SearchFilterLDAP: %s)\"\n", ErrLDAP(x));
+                        local_printfx("BH message=\"(SearchFilterLDAP: %s)\"\n", ErrLDAP(x));
                     } else {
                         /* Do Search */
                         edui_ldap.err = -1;
@@ -1871,17 +1871,20 @@ MainSafe(int argc, char **argv)
                         x = SearchLDAP(&edui_ldap, edui_ldap.scope, edui_ldap.search_filter, (char **) &search_attrib);
                         if (x != LDAP_ERR_SUCCESS) {
                             debug("SearchLDAP() -> %s (LDAP: %s)\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
-                            local_printfx("ERR message=\"(SearchLDAP: %s)\"\n", ErrLDAP(x));
+                            local_printfx("BH message=\"(SearchLDAP: %s)\"\n", ErrLDAP(x));
                         } else {
                             edui_ldap.err = -1;
                             debug("SearchLDAP(-, %d, %s, -) -> %s\n", edui_conf.scope, edui_ldap.search_filter, ErrLDAP(x));
                             x = SearchIPLDAP(&edui_ldap);
-                            if (x != LDAP_ERR_SUCCESS) {
-                                debug("SearchIPLDAP() -> %s (LDAP: %s)\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
+                            if (x == LDAP_ERR_NOTFOUND) {
+                                debug("SearchIPLDAP() -> %s\n", ErrLDAP(x));
                                 local_printfx("ERR message=\"(SearchIPLDAP: %s)\"\n", ErrLDAP(x));
-                            } else {
+                            } else if (x == LDAP_ERR_SUCCESS) {
                                 debug("SearchIPLDAP(-, %s) -> %s\n", edui_ldap.userid, ErrLDAP(x));
                                 local_printfx("OK user=%s\n", edui_ldap.userid);            /* Got userid --> OK user=<userid> */
+                            } else {
+                                debug("SearchIPLDAP() -> %s (LDAP: %s)\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
+                                local_printfx("BH message=\"(SearchIPLDAP: %s)\"\n", ErrLDAP(x));
                             }
                         }
                         /* Clear for next query */
@@ -1890,38 +1893,41 @@ MainSafe(int argc, char **argv)
                 }
             } else {
                 debug("StringSplit() -> Error: %" PRIuSIZE "\n", i);
-                local_printfx("ERR message=\"(StringSplit Error %" PRIuSIZE ")\"\n", i);
+                local_printfx("BH message=\"(StringSplit Error %" PRIuSIZE ")\"\n", i);
             }
         } else {
             /* No group to match against, only an IP */
             x = ConvertIP(&edui_ldap, bufa);
             if (x < 0) {
                 debug("ConvertIP() -> %s\n", ErrLDAP(x));
-                local_printfx("ERR message=\"(ConvertIP: %s)\"\n", ErrLDAP(x));
+                local_printfx("BH message=\"(ConvertIP: %s)\"\n", ErrLDAP(x));
             } else {
                 debug("ConvertIP(-, %s) -> Result[%d]: %s\n", bufa, x, edui_ldap.search_ip);
                 /* Do search */
                 x = SearchFilterLDAP(&edui_ldap, NULL);
                 if (x < 0) {
                     debug("SearchFilterLDAP() -> %s\n", ErrLDAP(x));
-                    local_printfx("ERR message=\"(SearchFilterLDAP: %s)\"\n", ErrLDAP(x));
+                    local_printfx("BH message=\"(SearchFilterLDAP: %s)\"\n", ErrLDAP(x));
                 } else {
                     edui_ldap.err = -1;
                     debug("SearchFilterLDAP(-, NULL) -> Length: %u\n", x);
                     x = SearchLDAP(&edui_ldap, edui_ldap.scope, edui_ldap.search_filter, (char **) &search_attrib);
                     if (x != LDAP_ERR_SUCCESS) {
                         debug("SearchLDAP() -> %s (LDAP: %s)\n", ErrLDAP(x), ldap_err2string(x));
-                        local_printfx("ERR message=\"(SearchLDAP: %s)\"\n", ErrLDAP(x));
+                        local_printfx("BH message=\"(SearchLDAP: %s)\"\n", ErrLDAP(x));
                     } else {
                         edui_ldap.err = -1;
                         debug("SearchLDAP(-, %d, %s, -) -> %s\n", edui_conf.scope, edui_ldap.search_filter, ErrLDAP(x));
                         x = SearchIPLDAP(&edui_ldap);
-                        if (x != LDAP_ERR_SUCCESS) {
-                            debug("SearchIPLDAP() -> %s (LDAP: %s)\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
+                        if (x == LDAP_ERR_NOTFOUND) {
+                            debug("SearchIPLDAP() -> %s\n", ErrLDAP(x));
                             local_printfx("ERR message=\"(SearchIPLDAP: %s)\"\n", ErrLDAP(x));
-                        } else {
+                        } else if (x == LDAP_ERR_SUCCESS) {
                             debug("SearchIPLDAP(-, %s) -> %s\n", edui_ldap.userid, ErrLDAP(x));
                             local_printfx("OK user=%s\n", edui_ldap.userid);                /* Got a userid --> OK user=<userid> */
+                        } else if (x != LDAP_ERR_SUCCESS) {
+                            debug("SearchIPLDAP() -> %s (LDAP: %s)\n", ErrLDAP(x), ldap_err2string(edui_ldap.err));
+                            local_printfx("BH message=\"(SearchIPLDAP: %s)\"\n", ErrLDAP(x));
                         }
                     }
                 }

diff --git a/helpers/external_acl/file_userip/ext_file_userip_acl.cc b/helpers/external_acl/file_userip/ext_file_userip_acl.cc
index fffba64632b1567d358b26fcccd975f764ae61df..50b9937142fa17ab44e1c5b4a002a4cd41dc4d5a 100644 (file)
--- a/helpers/external_acl/file_userip/ext_file_userip_acl.cc
+++ b/helpers/external_acl/file_userip/ext_file_userip_acl.cc
@@ -265,7 +265,7 @@ main (int argc, char *argv[])
                 if (strchr(line, '\n') != NULL)
                     break;
             }
-            SEND_ERR("Input Too Large.");
+            SEND_BH(HLP_MSG("Input Too Large."));
             continue;
         }
         *cp = '\0';
@@ -273,7 +273,7 @@ main (int argc, char *argv[])
         username = strtok(NULL, " \t");
         if (!address || !username) {
             debug("%s: unable to read tokens\n", program_name);
-            SEND_ERR("Invalid Input.");
+            SEND_BH(HLP_MSG("Invalid Input."));
             continue;
         }
         rfc1738_unescape(address);

diff --git a/helpers/external_acl/time_quota/ext_time_quota_acl.cc b/helpers/external_acl/time_quota/ext_time_quota_acl.cc
index bfa1505349077a41173c16f7860eff09f17bcacd..76977a17cfa3cc121bc2912f17fdb5b2cd2661ad 100644 (file)
--- a/helpers/external_acl/time_quota/ext_time_quota_acl.cc
+++ b/helpers/external_acl/time_quota/ext_time_quota_acl.cc
@@ -452,7 +452,7 @@ int main(int argc, char **argv)
         // we expect the following line syntax: %LOGIN
         const char *user_key = strtok(request, " \n");
         if (!user_key) {
-            SEND_BH("message=\"User name missing\"");
+            SEND_BH(HLP_MSG("User name missing"));
             continue;
         }
         processActivity(user_key);

diff --git a/helpers/external_acl/unix_group/check_group.cc b/helpers/external_acl/unix_group/check_group.cc
index 4bb7d5003c8afe46b9e5f82a1cdcac10d86f9653..cd8c9c8452a49c678cce96f6238b52093202877d 100644 (file)
--- a/helpers/external_acl/unix_group/check_group.cc
+++ b/helpers/external_acl/unix_group/check_group.cc
@@ -198,12 +198,12 @@ main(int argc, char *argv[])
                 if (strchr(buf, '\n') != NULL)
                     break;
             }
-            SEND_ERR("Username Input too large.");
+            SEND_BH(HLP_MSG("Username Input too large."));
             continue;
         }
         *p = '\0';
         if ((p = strtok(buf, " ")) == NULL) {
-            SEND_ERR("No username given.");
+            SEND_BH(HLP_MSG("No username given."));
             continue;
         } else {
             user = p;

diff --git a/src/external_acl.cc b/src/external_acl.cc
index 99391954f8140713ab52528f02e78f5663db214f..308a961d17fab151b6e6a1edf90a837413a5b746 100644 (file)
--- a/src/external_acl.cc
+++ b/src/external_acl.cc
@@ -98,6 +98,8 @@ public:
 
     void trimCache();
 
+    bool maybeCacheable(const allow_t &) const;
+
     int ttl;
 
     int negative_ttl;
@@ -611,6 +613,21 @@ external_acl::trimCache()
     }
 }
 
+bool
+external_acl::maybeCacheable(const allow_t &result) const
+{
+    if (cache_size <= 0)
+        return false; // cache is disabled
+
+    if (result == ACCESS_DUNNO)
+        return false; // non-cacheable response
+
+    if ((result == ACCESS_ALLOWED ? ttl : negative_ttl) <= 0)
+        return false; // not caching this type of response
+
+    return true;
+}
+
 /******************************************************************
  * external acl type
  */
@@ -874,7 +891,7 @@ static void
 external_acl_cache_touch(external_acl * def, const ExternalACLEntryPointer &entry)
 {
     // this must not be done when nothing is being cached.
-    if (def->cache_size <= 0 || (def->ttl <= 0 && entry->result == 1) || (def->negative_ttl <= 0 && entry->result != 1))
+    if (!def->maybeCacheable(entry->result))
         return;
 
     dlinkDelete(&entry->lru, &def->lru_list);
@@ -1223,10 +1240,10 @@ makeExternalAclKey(ACLFilledChecklist * ch, external_acl_data * acl_data)
 static int
 external_acl_entry_expired(external_acl * def, const ExternalACLEntryPointer &entry)
 {
-    if (def->cache_size <= 0)
+    if (def->cache_size <= 0 || entry->result == ACCESS_DUNNO)
         return 1;
 
-    if (entry->date + (entry->result == 1 ? def->ttl : def->negative_ttl) < squid_curtime)
+    if (entry->date + (entry->result == ACCESS_ALLOWED ? def->ttl : def->negative_ttl) < squid_curtime)
         return 1;
     else
         return 0;
@@ -1235,11 +1252,11 @@ external_acl_entry_expired(external_acl * def, const ExternalACLEntryPointer &en
 static int
 external_acl_grace_expired(external_acl * def, const ExternalACLEntryPointer &entry)
 {
-    if (def->cache_size <= 0)
+    if (def->cache_size <= 0 || entry->result == ACCESS_DUNNO)
         return 1;
 
     int ttl;
-    ttl = entry->result == 1 ? def->ttl : def->negative_ttl;
+    ttl = entry->result == ACCESS_ALLOWED ? def->ttl : def->negative_ttl;
     ttl = (ttl * (100 - def->grace)) / 100;
 
     if (entry->date + ttl <= squid_curtime)
@@ -1253,9 +1270,13 @@ external_acl_cache_add(external_acl * def, const char *key, ExternalACLEntryData
 {
     ExternalACLEntryPointer entry;
 
-    // do not bother caching this result if TTL is going to expire it immediately
-    if (def->cache_size <= 0 || (def->ttl <= 0 && data.result == 1) || (def->negative_ttl <= 0 && data.result != 1)) {
+    if (!def->maybeCacheable(data.result)) {
         debugs(82,6, HERE);
+        if (data.result == ACCESS_DUNNO) {
+            const ExternalACLEntryPointer oldentry = static_cast<ExternalACLEntry *>(hash_lookup(def->cache, key));
+            if (oldentry != NULL)
+                external_acl_cache_delete(def, oldentry);
+        }
         entry = new ExternalACLEntry;
         entry->key = xstrdup(key);
         entry->update(data);
@@ -1347,13 +1368,15 @@ externalAclHandleReply(void *data, const Helper::Reply &reply)
     externalAclState *state = static_cast<externalAclState *>(data);
     externalAclState *next;
     ExternalACLEntryData entryData;
-    entryData.result = ACCESS_DENIED;
 
     debugs(82, 2, HERE << "reply=" << reply);
 
     if (reply.result == Helper::Okay)
         entryData.result = ACCESS_ALLOWED;
-    // XXX: handle other non-DENIED results better
+    else if (reply.result == Helper::Error)
+        entryData.result = ACCESS_DENIED;
+    else //BrokenHelper,TimedOut or Unknown. Should not cached.
+        entryData.result = ACCESS_DUNNO;
 
     // XXX: make entryData store a proper Helper::Reply object instead of copying.
 
@@ -1384,17 +1407,8 @@ externalAclHandleReply(void *data, const Helper::Reply &reply)
     dlinkDelete(&state->list, &state->def->queue);
 
     ExternalACLEntryPointer entry;
-    if (cbdataReferenceValid(state->def)) {
-        // only cache OK and ERR results.
-        if (reply.result == Helper::Okay || reply.result == Helper::Error)
-            entry = external_acl_cache_add(state->def, state->key, entryData);
-        else {
-            const ExternalACLEntryPointer oldentry = static_cast<ExternalACLEntry *>(hash_lookup(state->def->cache, state->key));
-
-            if (oldentry != NULL)
-                external_acl_cache_delete(state->def, oldentry);
-        }
-    }
+    if (cbdataReferenceValid(state->def))
+        entry = external_acl_cache_add(state->def, state->key, entryData);
 
     do {
         void *cbdata;