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

[Amos Jeffries]

On 1/02/2017 5:49 a.m., Alex Rousskov wrote:
> 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.


Do you think we can compromise and call it USE_OPENSSL_OR_LIBRESSL ?


> 
> 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!
> 

So you wish de define a 3rd macro in addition to the other two

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

So a hypothetical patch is submitted to use, lets say a function
parameter is made const, in the LibreSSL but breaks when building with
OpenSSL. Is required to be wrapped in USE_OPENSSL.

 Can you spot the contradiction?


> 
>> 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!

I am following the autoconf guidelines for use of the AC_ARG_WITH() macro:
"
Some packages require, or can optionally use, other software packages
that are already installed. The user can give configure command line
options to specify which such external software to use. The options have
one of these forms:

     --with-package[=arg]
     --without-package

For example, --with-gnu-ld means work with the GNU linker instead of
some other linker.
"

The example about linker is for distinguishing between two different
linkers. They are both linkers and provide the same linking API.
Our usage is for libraries, the APIs have similarities but also differences.

Also note the name of the option follows the naming of the external
software. In our case we have two differently named external software.

1) OpenSSL is the name of one externally provided library / software.

2) LibreSSL is the name of a *different* library.

The point of difference is that LibreSSL does not work with all of the
code enabled by --with-openssl. Which is driving in this patch the
addition of --with-libressl.

If you want to convince anyone that LibreSSL is just a different name
for OpenSSL please take it up with the OpenSSL guys writing LibreSSL. It
is not our place to make (or un-make) that decision for them.



I am then following the naming convention documented in our Squid coding
guidelines for the USE_* macros and how they relate to the ./configure
option naming:

<http://wiki.squid-cache.org/SquidCodingGuidelines#Autoconf_Syntax_Guidelines>


If you will note, this patch was adding a --with-libressl option
*explicitly* to distinguish those builds from OpenSSL builds with the
auto-tools tests cannot.

It is not changing the meaning of the --with-openssl option in any way.


> 
> 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,

Er. False. You seem to be overlooking the fact that bug 2664 exists
because the APIs *have* diverged so much that one of them no longer
builds against code written for the other.

Likewise I brought up the fact that LibreSSL API goes way beyond
OpenSSL-compatible part as more evidence. Code written for LibreSSL
-ltls will not build against any version of OpenSSL.

It can be a bit unclear because Squid today mostly uses the overlapping
part of their APIs. But I dont believe that will always be the case, not
even in the near future.


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

That is one thing you should probably open a bug about with them. It
appears to be too late to prevent divergence.

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

Not quite, it is a bottleneck point. But all of the OpenSSL v1.1 API
changes are also points of divergence. Plus all the bits mentioned above.

LibreSSL was a full forking. It has just taken a while to be visible.

In regards to this being a planning for the future, we should IMO expect
the differences to keep on increasing. So making our long-term design
require that they been treated as equal is a fail.

The workaround of building --with-openssl to link against LibreSSL had
its time and that time is now over.


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

For now in the current Squid code yes. That is evident from the ||
additions in this patch.

However, I was under the impression that we were planning for the future
here. We also seem to agree that in future the LibreSSL is going to
diverge from OpenSSL**. That conflicts AFAICS with the idea of treating
LibreSSL as an alias for OpenSSL - which will by the way do exactly zero
for fixing the bug 2664.

** we seem to still differ on whether that has happened. But we agree it
will in future.

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

Not quite.

I believe the integrity of the code rests solely on which libraries are
used and tested by people. That will happen independently of whether
there is one macro around each code block or a pair/triplet/N-tuplet
with ||/&& expressions.

Fedora/RHEL/CentOS use OpenSSL and we get feedback on testing and
problems from them.

The various *BSD build with LibreSSL mostly now and increasing. We get
feedback on the LibreSSL parts from them.

So there is only minor need to use the macro naming to enforce "good
behaviour". We can arrange the BuildFarm and QA to do that enforcement.

Instead to be forward-looking we should plan to be flexible enough to
allow the different libraries to have their own unique code blocks where
necessary as they (further) diverge.

I am proposing this bug 2664 as the tipping point for LibreSSL
separating from OpenSSL. Or more correctly OpenSSL v1.1 diverging from
its shared base with LibreSSL.


> I believe that assumption is wrong and
> that low-level approach is too dangerous.

We have a precedent in the use of distro specific _SQUID_*_ macros. It
has worked fairly well for the past 30-odd years.


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

Large features is what we use the --enable-* macros for. That level
LibreSSL is not relevant. The macros wrapping those code blocks apply to
OpenSSL, GnuTLS or LibreSSL equally.

It would be lovely if we only had to use macros at that level. But until
TLS and no-longer-SSL libraries all share a universal API definition we
have to use per-library wrappers as well, and per-feature within that.

It is a PITA but these libraries *are* different.

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

I have to warn all the BSD people that 4.0.18 will probably not build on
their systems anymore unles they switch back to OpenSSL. So not to worry
or bother with complaints. Ditto for all the others voluntarily using
LibreSSL.

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


Amos