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

[Alex Rousskov]

Executive summary:

* Still no agreement on whether or how to rename the primary SSL guard.
* Possibly an agreement to continue using a single primary SSL guard??
* Clarification that --with-libressl itself is a relatively minor issue.
* A firm veto on adding support for the 3rd SSL API.
  Whether that veto applies to bug 4662 depends on the fix,
  but a correct fix will not add support for the 3rd API IMO.

Details below.

On 02/01/2017 06:51 AM, Amos Jeffries wrote:

> Do you think we can compromise and call it USE_OPENSSL_OR_LIBRESSL ?

I would not view that name as a compromise: It still enumerates specific
libraries instead of naming the API used by the code it protects. You
can try to argue that it avoids code duplication but the value of this
literal/meaningless avoidance is very small. In other words, this
solution would preserve 95% of the bad properties of the original
proposal while giving me a superficial 5% satisfaction of removing
spaces from (a || b).

[rant]Compromises in design and development are rarely a good idea. Two
correct designs are rarely on the same curve. Adding them up and
dividing by two usually gives a solution that is worse than either of
the alternatives. Please do not misinterpret this general statement as
an implication that there are two correct designs here; I have not seen
them (or I would not be objecting).[/rant]


I would like to capitalize on your willingness to use a single macro.

This macro should name the associated API. I think it is a bad idea to
call the associated API "OpenSSL or LibreSSL" when we know that

* there are other libraries that implement the same API (leading to
USE_OPENSSL_OR_LIBRESSL_OR_FOO_OR_BAR)

* LibreSSL provides more than one API
(leading to USE_OPENSSL_OR_LIBRESSL_PART_PROVIDING_OPENSSL_API)

If we do not add --with-libressl, then USE_OPENSSL would be a natural
choice given the current code. In case we decide to add --with-libressl,
I can offer the following alternatives and remain open to other suggestions:

  * USING_OPENSSL
  * USING_OPENSSL_API
  * USE_OPENSSL_API
  * USE_OPENSSL_FLAVORS
  * USE_ANY_OPENSSL
  * HAVE_OPENSSL_API


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

I wish we add no new macros until they become necessary. You are adding
macros. I am content with what we already have for this high-level
context: A single USE_OPENSSL macro.

The existing USE_OPENSSL macro is not the problem. The lack of an
explicit --with-libressl option is not the problem. The problem is that
Squid relies on the OPENSSL_VERSION_NUMBER (which LibreSSL has changed
in an incompatible way). This is a minor compatibility problem that has
several correct solutions, none of which require changing most Squid
code dealing with the OpenSSL API. Here are a few sketches:

A. Define SQUID_OPENSSL_VERSION to match available API version (in
OpenSSL terms) and use that instead of raw OPENSSL_VERSION_NUMBER.

B. Replace raw OPENSSL_VERSION_NUMBER uses with macros that guard the
corresponding features (without adding feature tests).

C. Replace OPENSSL_VERSION_NUMBER with feature tests.

Solution C is the best/cleanest but also requires more work. I am not
sure, but I suspect that all solutions can be implemented without adding
a new --with-* option for every OpenSSL API implementation (LibreSSL,
BoringSSL, etc.).


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

There is no contradiction if you interpret USE_OPENSSL correctly.

Yes, there could be other macros/guards inside USE_OPENSSL code,
including, if really necessary USE_LIBRESSL set by --with-libressl. In
the latter case, USE_OPENSSL itself (i.e., the primary guard for the
code using OpenSSL API) would have to be renamed to USING_OPENSSL_API or
some such. Needless to say, those other macros/guards have to be
confined to a few exceptional cases.


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

I rest my case! We should be following autoconf guidelines when
implementing a high-level design. We should not follow autoconf
guidelines to come up with or understand a high-level design. Again and
again, you are obsessed with letters such as those used to spell the
USE_OPENSSL macro. We should be obsessed with that macro meaning instead.


> The example about linker is for distinguishing between two different
> linkers. They are both linkers and provide the same linking API.

The last part is incorrect or misleading. In the scope of ./configure
usage, the GNU linker interface differs from the minimal interface used
by default (and probably results in different linked binaries). This is
why we have --with-gnu-ld in the first place. If all linkers were the
same or differed only in minor exceptional cases, there would be no need
for such an option -- the differences would be detected internally (like
the existing ./configure code that detects such minor differences among
GNU ld linkers, for example).

The same logic applies to Squid. As long as we keep using the same API,
we can have a single macro guarding that use (and possibly a single
--with-openssl option to set that macro, but that is secondary). Yes,
there will be minor exceptions where Squid would need to look at more
than that single macro. We already have them, and there will be more.
However, those _exceptions_ do not affect the overall design. Adding "||
USE_LIBRESSL" to that single macro, in any shape of form, does.


> The point of difference is that LibreSSL does not work with all of the
> code enabled by --with-openssl.

LibreSSL does not work with some of the _current_ code enabled by
--with-openssl because we use OPENSSL_VERSION_NUMBER macro
[incorrectly]. We can fix that minor, localized problem instead of
introducing a full-fledged support for a third SSL API.


> Which is driving in this patch the addition of --with-libressl.

Please note that the addition of --with-libressl is a minor problem. I
am focusing on a much bigger problem: The addition of "|| USE_LIBRESSL"
or equivalent guards to SSL-related code. If you do not see the
difference between the two problems, then you are still thinking in
terms of letters, not overall design.

If we have to add --with-libressl, we will add --with-libressl. Adding
it adds development and administration costs, so we should not add it
just because we can -- we should try to find a better solution to
accommodate LibreSSL idiosyncrasies. However, it might turn out that
--with-libressl is the best solution! If we add it, we should still use
a single primary macro to guard SSL code that uses OpenSSL API.


> If you want to convince anyone that LibreSSL is just a different name
> for OpenSSL

Why would I want to convince anyone that LibreSSL is just a different
name for OpenSSL? It is obviously not true! LibreSSL provides (among
other things) an alternative implementation of OpenSSL API. Squid does
not care about the implementation differences, but that does not make
LibreSSL project less valuable or distinct. I have no beef with LibreSSL
(except that they screw up OPENSSL_VERSION_NUMBER value, but we can work
around that).


> this patch was adding a --with-libressl option
> *explicitly* to distinguish those builds from OpenSSL builds with the
> auto-tools tests cannot.

I recommend that we discuss --with-libressl addition later, after we
agree regarding the primary guard macro. --with-libressl addition is a
relatively minor issue. If you insist on discussing it now, then please
indicate whether it is possible to identify LibreSSL [headers] by other
means? Perhaps they define some macro that is unique to them? The answer
has to be limited to public headers that are named the same as OpenSSL
headers, of course. Or perhaps we can test (at preprocessor time) for
the value of some macro shared between OpenSSL and LibreSSL.


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

You are mistaking a difference in versioning numbers for API divergence.
I would not be surprised if LibreSSL considers specific
OPENSSL_VERSION_NUMBER values to be outside the OpenSSL API, but even if
they do not, it is not important for Squid. We have no other indications
that LibreSSL does not work with Squid, so I expect the APIs to be
essentially the same. We just need to accommodate a few exceptions.


I can approach the same problem from another angle: If you manage to
convince me that LibreSSL does not provide an OpenSSL API, then I will
have to block any patch claiming to add LibreSSL support to Squid right
now. We simply cannot afford to support a third SSL API right now!


> Likewise I brought up the fact that LibreSSL API goes way beyond
> OpenSSL-compatible part as more evidence.

I have already said that the existence of additional API(s) provided by
LibreSSL only proves my point. I will not repeat those arguments here.


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

There is no long-term requirement to treat them as equal. Such a
requirement would be absurd IMO -- we cannot predict that future, and I
agree that the number of exceptions is likely to grow.

However, as soon as LibreSSL is no longer providing an OpenSSL API (with
exceptions that we can manage), we must stop supporting LibreSSL
(unless, of course, by that time we have adequate resources to take on
an additional SSL API).


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

FWIW, I prefer to ship quality code instead of relying on users to find
holes in its overall integrity. Needless to say, I cannot find all the
bugs before shipping, but I can avoid designs that I know will introduce
bugs that I would not be able to look for.


> That will happen independently of whether
> there is one macro around each code block or a pair/triplet/N-tuplet
> with ||/&& expressions.

These are not the two designs we are comparing in this context. We are
comparing a design where there is

* one set of OpenSSL-related code pieces

with a design where there is a complex mix of three non-trivial sets:

* one set of pure OpenSSL-related code pieces
* one set of pure LibreSSL-related code pieces
* one set of "any OpenSSL flavor"-related code pieces

We cannot rely on tests to validate the correctness of such a complex
mix. We should avoid such a complex mix as too costly for the Project.
If necessary, we can prohibit LibreSSL support to avoid it. I do not see
any reason to prohibit LibreSSL support today unless you insist that
this mix is necessary for LibreSSL support.


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

Yes, as long as those unique code blocks are small well-hidden
exceptions. We cannot afford to support a third SSL API right now, but
we may be able to afford to cover a few exceptions.


> I am proposing this bug 2664 as the tipping point for LibreSSL
> separating from OpenSSL.

I would have to block patches adding support for a 3rd SSL API due to
lack of Project resources to properly support a 3rd API combined with
wide availability of the two already supported APIs. It would be nice to
support more (e.g., Curl supports more than 10!) but we simply cannot
afford and do not desperately need more APIs.

Again, I do not think we are at the point where we have to stop
supporting LibreSSL, but if you propose a design that essentially treats
LibreSSL as a separate/third API, I would be too horrified to look the
other way while you are committing it.


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

Agreed in general, with s/per-library/per-API/ correction, but we cannot
properly support more than two SSL APIs at the moment. Any library that
does not provide OpenSSL or GnuTLS API (with minor exceptions that we
can manage) is inadmissible today IMO.


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

There are other ways to avoid those complaints. Those complaints do not
justify committing wrong design when better designs are available. We
cannot be hostage to folks using unsupported libraries and discovering
that they are, well, unsupported. And even if we decide that we are
hostages, there are other ways to escape our captivity in this case.

Alex.