From: Christos Tsantilas <chtsanti@users.sourceforge.net>
Date: Thu, 26 Jan 2017 17:46:17 +0000 (+1300)
Subject: Update External ACL helpers error handling and caching
X-Git-Tag: SQUID_4_0_18~13
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=7686cf0fee811e59b320bc09d915802c0bf6e876;p=thirdparty%2Fsquid.git

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/src/acl/external/AD_group/ext_ad_group_acl.cc b/src/acl/external/AD_group/ext_ad_group_acl.cc
index 86ed031222..18e7cf4f20 100644
--- 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 92e9168e48..6bf5a5e906 100644
--- 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 4188e04481..7d2cc8a084 100644
--- 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 7e28f276b8..7b34c730d5 100755
--- 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 a6a775e786..68dcd53142 100644
--- 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 73fd4ec968..e0c73e5ba7 100644
--- 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 5531d68940..3c7ec6933d 100644
--- 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 a95d46d20f..b7fdf598c7 100644
--- 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 d7b01e597a..c8c08fee81 100644
--- 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 39f9c2a32e..02f4f53b48 100644
--- 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
-                callback(cbdata, r->reply);
+            } else {
+                HLPCB *callback = r->request.callback;
+                r->request.callback = nullptr;
+                void *cbdata = nullptr;
+                if (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 0599e588f7..e1f481f5b4 100644
--- 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