External ACL helpers error handling & 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 doesn't 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 is 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 retries 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/src/acl/external/AD_group/ext_ad_group_acl.cc b/src/acl/external/AD_group/ext_ad_group_acl.cc
index 86ed031222d09a362aa67139eaa715dfaeb16981..18e7cf4f2089c392d278500e2e8d102e8ffd38a6 100644 (file)
--- a/src/acl/external/AD_group/ext_ad_group_acl.cc
+++ b/src/acl/external/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/src/acl/external/LDAP_group/ext_ldap_group_acl.cc b/src/acl/external/LDAP_group/ext_ldap_group_acl.cc
index 92e9168e4848b684a6d15142ccf4db03284dafd0..bb1265efb973957715f677519ab6ce06fd90b262 100644 (file)
--- a/src/acl/external/LDAP_group/ext_ldap_group_acl.cc
+++ b/src/acl/external/LDAP_group/ext_ldap_group_acl.cc
@@ -471,13 +471,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);
@@ -500,11 +500,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);
 
@@ -514,6 +515,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;
                     }
@@ -535,7 +537,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)
@@ -546,8 +549,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;
@@ -558,7 +561,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;
@@ -575,7 +579,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;
@@ -583,20 +588,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("");
         }
@@ -722,14 +731,14 @@ searchLDAPGroup(LDAP * ld, const char *group, const char *member, const char *ex
     if (!build_filter(filter, searchfilter, member, group)) {
         std::cerr << PROGRAM_NAME  << ": ERROR: Failed to construct LDAP search filter. filter=\"" <<
                   filter.c_str() << "\", user=\"" << member << "\", group=\"" << group << "\"" << std::endl;
-        return 1;
+        return -1;
     }
     debug("group filter '%s', searchbase '%s'\n", filter.c_str(), searchbase.c_str());
 
     rc = ldap_search_s(ld, searchbase.c_str(), searchscope, filter.c_str(), searchattr, 1, &res);
     LdapResult ldapRes(res, ldap_msgfree);
     if (!ldap_search_ok(rc))
-        return 1;
+        return -1;
 
     return ldap_first_entry(ld, ldapRes.get()) ? 0 : 1;
 }
@@ -764,7 +773,7 @@ searchLDAP(LDAP * ld, char *group, char *login, char *extension_dn)
         rc = ldap_search_s(ld, searchbase.c_str(), searchscope, filter.c_str(), searchattr, 1, &res);
         LdapResult ldapRes(res, ldap_msgfree);
         if (!ldap_search_ok(rc))
-            return 1;
+            return -1;
         entry = ldap_first_entry(ld, ldapRes.get());
         if (!entry) {
             std::cerr << PROGRAM_NAME << ": WARNING: User '" << login <<

diff --git a/src/acl/external/LM_group/ext_lm_group_acl.cc b/src/acl/external/LM_group/ext_lm_group_acl.cc
index 4188e044812fd4af1f2557552df9f570513d730e..7d2cc8a084450feef78106d8d1823ad54377f19d 100644 (file)
--- a/src/acl/external/LM_group/ext_lm_group_acl.cc
+++ b/src/acl/external/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/src/acl/external/SQL_session/ext_sql_session_acl.pl.in b/src/acl/external/SQL_session/ext_sql_session_acl.pl.in
index 7e28f276b819a8346ccf87db771a4c35dfc5d15c..7b34c730d5bac9900684b7294e0f7ef1558f53a9 100755 (executable)
--- a/src/acl/external/SQL_session/ext_sql_session_acl.pl.in
+++ b/src/acl/external/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/src/acl/external/eDirectory_userip/ext_edirectory_userip_acl.cc b/src/acl/external/eDirectory_userip/ext_edirectory_userip_acl.cc
index a6a775e786bc3ad7be99c8f1eab7a5275799f61a..68dcd53142ae93c3815f6a6a7d7c29596dc39218 100644 (file)
--- a/src/acl/external/eDirectory_userip/ext_edirectory_userip_acl.cc
+++ b/src/acl/external/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/src/acl/external/file_userip/ext_file_userip_acl.cc b/src/acl/external/file_userip/ext_file_userip_acl.cc
index 73fd4ec968bb00750edd0cc500440c30ac3e75d7..e0c73e5ba7b93a1362323b8f7fb49f0808b5bd15 100644 (file)
--- a/src/acl/external/file_userip/ext_file_userip_acl.cc
+++ b/src/acl/external/file_userip/ext_file_userip_acl.cc
@@ -266,7 +266,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';
@@ -274,7 +274,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/src/acl/external/time_quota/ext_time_quota_acl.cc b/src/acl/external/time_quota/ext_time_quota_acl.cc
index 5531d689408d577ffec2c21f68db120c65cc84bd..3c7ec6933ddd0cc6ac67855fd024f740ed6cc92b 100644 (file)
--- a/src/acl/external/time_quota/ext_time_quota_acl.cc
+++ b/src/acl/external/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/src/acl/external/unix_group/check_group.cc b/src/acl/external/unix_group/check_group.cc
index a95d46d20f894aaa0459ccf34597899a9b4075ac..b7fdf598c76cd3acf79abe5ff28210d515da766c 100644 (file)
--- a/src/acl/external/unix_group/check_group.cc
+++ b/src/acl/external/unix_group/check_group.cc
@@ -203,12 +203,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 d7b01e597a6518afc6dddc6211e686282e876494..c8c08fee81f705d914b22ae744b9f4fa71dfac83 100644 (file)
--- a/src/external_acl.cc
+++ b/src/external_acl.cc
@@ -85,6 +85,8 @@ public:
 
     void trimCache();
 
+    bool maybeCacheable(const allow_t &) const;
+
     int ttl;
 
     int negative_ttl;
@@ -445,6 +447,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
  */
@@ -716,7 +733,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);
@@ -782,10 +799,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;
@@ -794,11 +811,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)
@@ -812,9 +829,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) {
+            if (const ExternalACLEntryPointer oldentry = static_cast<ExternalACLEntry *>(hash_lookup(def->cache, key)))
+                external_acl_cache_delete(def, oldentry);
+        }
         entry = new ExternalACLEntry;
         entry->key = xstrdup(key);
         entry->update(data);
@@ -916,13 +937,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.
 
@@ -953,17 +976,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;

diff --git a/src/helper.cc b/src/helper.cc
index 39f9c2a32e5b40798617754e401a505735399851..ae8355c711444ddb24373b4f0045d4d8cbc38e4c 100644 (file)
--- a/src/helper.cc
+++ b/src/helper.cc
@@ -896,18 +896,19 @@ helperReturnBuffer(helper_server * srv, helper * hlp, char * msg, size_t msgSize
         if (!msgEnd)
             return; // We are waiting for more data.
 
-        HLPCB *callback = r->request.callback;
-        r->request.callback = nullptr;
-
         bool retry = false;
-        void *cbdata = nullptr;
-        if (cbdataReferenceValidDone(r->request.data, &cbdata)) {
+        if (cbdataReferenceValid(r->request.data)) {
             r->reply.finalize();
             if (r->reply.result == Helper::BrokenHelper && r->request.retries < MAX_RETRIES) {
                 debugs(84, DBG_IMPORTANT, "ERROR: helper: " << r->reply << ", attempt #" << (r->request.retries + 1) << " of 2");
                 retry = true;
-            } else
+            } else {
+                HLPCB *callback = r->request.callback;
+                r->request.callback = nullptr;
+                void *cbdata = nullptr;
+                cbdataReferenceValidDone(r->request.data, &cbdata);
                 callback(cbdata, r->reply);
+            }
         }
 
         -- srv->stats.pending;

diff --git a/src/helper/protocol_defines.h b/src/helper/protocol_defines.h
index 0599e588f7f14275d675792f4b53c7860ffdf49b..e1f481f5b4de01acd5dd6ff6699250f935bea098 100644 (file)
--- a/src/helper/protocol_defines.h
+++ b/src/helper/protocol_defines.h
@@ -53,9 +53,12 @@
 /* send ERR result to Squid with a string parameter. */
 #define SEND_ERR(x) std::cout << "ERR " << x << std::endl
 
-/* send ERR result to Squid with a string parameter. */
+/* send BH result to Squid with a string parameter. */
 #define SEND_BH(x)  std::cout << "BH " << x << std::endl
 
+/* constructs a message to Squid. */
+#define HLP_MSG(text)  "message=\"" text "\""
+
 /* send TT result to Squid with a string parameter. */
 #define SEND_TT(x)  std::cout << "TT " << x << std::endl