From: Christos Tsantilas Date: Fri, 27 Jan 2017 02:26:04 +0000 (+1300) Subject: Update External ACL helpers error handling and caching X-Git-Tag: SQUID_3_5_24~7 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2331dc22f188d18e092a3b6921e3cb3130c5a213;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/helpers/defines.h b/helpers/defines.h index 2f2d2615ef..0f8e888550 100644 --- 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 d1a642aab0..4cc6e8dbbd 100644 --- 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 01fad7a930..f61809720d 100644 --- 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 fb73be3f83..9135fd2d5e 100644 --- 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 7e28f276b8..7b34c730d5 100755 --- 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 a5ea8390d3..63609e443e 100644 --- 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= */ + } 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= */ + } 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 fffba64632..50b9937142 100644 --- 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 bfa1505349..76977a17cf 100644 --- 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 4bb7d5003c..cd8c9c8452 100644 --- 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 99391954f8..308a961d17 100644 --- 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(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(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(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;