[squid-dev] [PATCH] TLS NPN updates

[Amos Jeffries]

On 17/12/2015 4:34 a.m., Alex Rousskov wrote:
> On 12/15/2015 10:19 PM, Amos Jeffries wrote:
>> This patch is shuffling the TLS NPN gadgetry into libsecurity.
>>
>> It would be a non-audit commit except that ...
>>
>> * there is a new config option added (tls-no-npn) to fully disable NPN
>> on selected peers or ports.
> 
> Please document (in the commit message) why it is sometimes useful to
> disable NPN.
> 
> 
>> * ICAPS connections are setting that option by default to prevent NPN
>> wrongly advertising them as HTTPS connections.
> 
> You could add code to advertise ICAP/1.0 connections correctly instead.
> However, that should have been done when NPN support was added and can
> be considered out of this patch scope, so I certainly do not insist on
> this addition.

I considered that. However NPN is already deprecated as a feature and
only available in aging OpenSSL anyway. I am instead hoping to get ALPN
implemented properly at some point in this process and look towards drop
the NPN.

Along those lines, is there anything you can do to help push the ICAP
people towards getting a formal ALPN assignment for ICAPS ?

> 
>> +        /// do not use the TLS NPN extension on these connections
>> +        bool noTlsNpn;
> 
> Or use it, depending on the data member value. "Whether to use ..." is
> the neutral description variant.

Okay.

> 
> Please do not add negative ("no-something") booleans to code. It is
> difficult to read double negations in initializations
> (noSomething=false) and expressions (!noSomething). It is usually
> possible to rename to doSomething (and adjust code) or, if necessary, to
> avoidSomething (without code adjustments) to mitigate those code
> comprehension problems.

Changing it.

But FTR we do not use or need double negations like that. This and the
other flag options are used to short-circuit logics ie. "if
(flags.noFoo) return;".  Or in the case of the config parse/dump using
"if (flags.noFoo)" 'flags is true' semantics.

Making these to positive flags just causes the code to use if(!X)
constructs.

I'm okay either way, it just seemed cleaner code this way around.


> 
> Negative options are borderline OK in configuration files (because we
> can add the positive "yes-something" version), although they break the
> option=value convention by essentially placing the value in front of the
> option.
> 
> 
>> If there are no objections to those two small changes I would like to
>> fast-track this patch and apply in ~48hrs.
> 
> My suggestions above are not blocking objections.
> 

Thank you for the review. I have applied as trunk rev.14447.

(PS, and followed up with polishing rev.14448 that changes the
noDefaultCa flag to match).

Amos