[squid-dev] [PATCH] Bug 4662 adding --with-libressl build option

[Alex Rousskov]

On 01/31/2017 08:20 AM, Amos Jeffries wrote:
> On 31/01/2017 7:04 a.m., Alex Rousskov wrote:
>> On 01/29/2017 04:26 AM, Amos Jeffries wrote:
>>> This is I think all we need to do code-wise to resolve the Bug 4662
>>> issues with LibreSSL being incompatible with OpenSSL 1.1.

>> I do not think these changes should be committed. As you probably know
>> from earlier communication, I think we should avoid using both
>> USE_OPENSSL and USE_LIBRESSL in the code if LibreSSL is [treated as] a
>> replacement for OpenSSL. I have suggested several ways to avoid the
>> dangerous and needless repetition of (USE_OPENSSL || USE_LIBRESSL)
>> conditions, and we even seemed to agree on one of those solutions.


> IMO we only agreed on the HAVE_* macro usage for determining whether the
> 1.1 API was in use. Which is included in this patch.

I do not limit "feature tests" to "1.1 API". Any feature is eligible. In
fact, it may be a good idea to test individual features rather than
bundle many of them into a single API version test (that will become
obsolete as parts of that API are going to change). However, those
details are probably not very important when resolving the core
disagreement here.


> I don't think the repetition of (USE_OPENSSL || USE_LIBRESSL) is either
> needless or dangerous.

The needless part is not a matter of opinion. It is a fact -- you do not
need to repeat the (USE_OPENSSL || USE_LIBRESSL) test. You may use a
single USE_FOOBAR macro to avoid this code repetition. The exact
spelling of FOOBAR is a separate question.

Whether code duplication is bad, dangerous, unwanted, etc. is certainly
a matter of opinion and subject to context, but I doubt you can convince
me that it is not in this case, especially as we have started using
USE_OPENSSL macro in complex expressions involving GnuTLS.


> No more than USE_OPENSSL was in the same spot.

Repeating (a || b) condition many times is bad. Using a single (c)
condition many times is not. We should not be arguing about that basic
programming principle!


> Fixing the sheer number of uses is not in scope.
> 
> Keep it simple. 

I believe we can simply continue to use USE_OPENSSL almost everywhere it
is used today. No "fixing" is needed in this context except changing
what you think that macro should stand for.


> Only one macro per library. --with-foo sets USE_FOO.

For you, the libraries provided by OpenSSL and LibreSSL project are two
different "libraries". For me, they are two slightly different flavors
of the same "library". IMO, the developers should not be forced to think
which flavor is in use now or which flavors are supported today, except
in very few low-level cases where that information might be needed. With
feature tests, the number of such cases might be zero.

In other words, you seem to abstract at the level of file name spelling.
I abstract at the level of APIs. Naturally, it is difficult to agree on
a single scheme that accommodates both approaches!

Needless to say, the two projects may eventually diverge their APIs so
that one is no longer a flavor of the other. This day has not come yet,
and one could hope that LibreSSL folks would either avoid this
divergence or have the decency to stop using "OPENSSL" in their diverged
API names.


> NP: I am only adding the "|| USE_LIBRESSL" parts in this patch because I
> happen to know that the existing code is being used with LibreSSL and
> found to be working. Future code will have to prove itself separately
> for each library, OR at the submitters discretion (passing audit and QA
> also) list the librares believed to be safe with it.

I believe the approach you have described is inferior to treating both
OpenSSL and LibreSSL as providing the same overall API while avoiding
(or treating specially) the rare parts of the API that differ. Today,
there is only one such known exception in Squid context -- the
OPENSSL_VERSION_NUMBER macro. We seem to agree that we should not be
using that part of the API anyway, so there will be no exceptions at all
after OPENSSL_VERSION_NUMBER is replaced with feature tests.

Your approach appears to be based on the assumption that enabling or
disabling individual code pieces dealing with OpenSSL/LibreSSL APIs
based on "known to be used and seemingly working" test will preserve the
overall integrity of the code. I believe that assumption is wrong and
that low-level approach is too dangerous. IMO, the decision whether to
support LibreSSL in Squid should be made on a much higher level than an
individual piece of code. Levels such as "whole Squid" or "non-bumping
Squid" would be more appropriate. Exceptions might be made for small,
isolated functionality, but they should be carefully considered
exceptions, not the rule.

The considerations in the above two paragraphs are orthogonal to
avoiding code duplication but avoiding code duplication is easier when
LibreSSL support decisions are made at a level higher than individual
preprocessor statements.


> It is not a blocker, but I would like to avoid declaring LibreSSL as no
> longer supported by v4 since the fix is not exactly hard.

1. You do not have to declare anything with regard to LibreSSL support,
   especially at the v4 branch level. An open bug report is sufficient.
   IIRC, we have not declared LibreSSL as officially supported either.

2. The difficulty of changing Squid code lines is an important factor,
   but there are other factors to consider. Those other factors make
   the proposed "not hard" fix undesirable IMO.

Alex.