[squid-dev] [PATCH] TLS NPN updates

[Alex Rousskov]

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.


> +        /// 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.

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.


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,

Alex.