[squid-dev] [PATCH] External ACL helpers error handling & caching

[Christos Tsantilas]

On 11/01/2017 05:27 μμ, Amos Jeffries wrote:
> On 10/01/2017 12:49 a.m., Christos Tsantilas wrote:
>> 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 patch fixes the problems described above.
>>
>> This is a Measurement Factory project
>>
>
> Thank you for this long overdue fix.
>
> +1. Though if possible I would like one extra change...
>
> Please add the below method to class external_acl and use it to
> de-duplicate the complex if-conditions in external_acl_cache_touch and
> external_acl_cache_add about whether a response is non-cacheable:
>
> bool
> external_acl::maybeCacheable(const allow_t &result)
> {
>     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;
> }

The patch applied to squid-5 branch as r15005.
I am also attaching the patch for squid-3.5

>
>
> Could you also mention for the squid-dev record how much testing this
> patch has had.

Well, the fixes inside squid should be enough tested. It is tested with 
custom helpers and used various scenarios.

The  fixes in ext_ldap_group_acl helper should be enough tested too.

The other helpers which fixed with this patch did not tested, but the 
fixes looks safe.

I also checked a little the ext_kerberos_ldap_group_acl helper. Looks 
that it is ok, so no changes are required, but I did not run tests with 
this helper.


>
> Cheers
> Amos
>
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: SQUID-260-ext_ldap_group_acl-error-handling-squid-3.5-t3.patch
Type: text/x-patch
Size: 47874 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20170111/291f8516/attachment-0001.bin>