[squid-dev] [PATCH] Crypto-NG: Use Security::PeerOptions for listening port TLS settings

[Amos Jeffries]

On 1/07/2015 10:11 a.m., Alex Rousskov wrote:
> On 06/30/2015 09:27 AM, Amos Jeffries wrote:
> 
>> * Change the tls_outgoing_options default value from "disable" which did
>> not parse previously (now does). To setting TLS/1.0 minimum version with
>> SSLv3 disabled.
> 
>>  NAME: tls_outgoing_options
> ...
>> -DEFAULT: disable
>> +DEFAULT: min-version=1.0 options=NO_SSLv3
> 
> 
> Does this change mean that Squid can no longer talk to SSLv3-only sites
> unless the admin manually adjusts tls_outgoing_options to include SSLv3
> support?

Yes. As per your request earlier I am not removing SSLv3 entirely, just
reducing it to default-OFF inline with current BCP requirements.

> 
>> * Fix tls-min-version=1.N handling not to alter stored options= config
>> string. Now updates the binary representation in parsedOptions directly.
> 
> Can you describe this bug from the admin point of view? That is, what
> Squid trunk behavior was broken that this patch fixes?

The first patch which is in trunk appends the NO_* settings to the
textual sslOptions member. Which:
a) has already been pre-parsed so the setting would have no effect, and
b) on dumping the config would output both tls-min-version= and
tls-options= (containing tls-min-version textual representation) to the
cachemgr report info. Reloading the resulting config dump into Squid
would cause sslOptions to have the NO_TLS* options set twice, and so on
for each dump/reload loop.
 By setting the parsedOptions value directly we ensure the setting
actually works on first load, the dumper will only dump out what the
user conigured for tls-options=, and that tls-min-versions=1.N and
tls-options= interacte properly (tls-min-version= sets absolute floor on
the allowed TLS version, tls-options= disables higher ones piecemeal as
needed).


> 
>> -        ssl_error = ERR_get_error();
>> +        int ssl_error = ERR_get_error();
> 
> Minor: You can make all(?) of the redeclared ssl_error variables
> constant. Just something I accidentally noticed -- I did not review the
> whole patch.
> 

Seems yes. Done.

Thanks for what you could do.

Amos