Re: Cutting support for OpenSSL 1.0.1 and 1.0.2 in 17~?

> On 5 Apr 2024, at 03:37, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> On Thu, Apr 04, 2024 at 11:03:35AM -0700, Jacob Champion wrote:

>> v3 does that by putting back checks for symbols that aren't part of
>> LibreSSL (tested back to 2.7, which is where the 1.1.x APIs started to
>> arrive).
>
> From where did you pull the LibreSSL sources? Directly from the
> OpenBSD tree?
>
>> It also makes adjustments for the new OPENSSL_API_COMPAT
>> version, getting rid of OpenSSL_add_all_algorithms() and adding a
>> missing header.
>
> Ah, right. OpenSSL_add_all_algorithms() is documented as having no
> effect in 1.1.0.

This API was deprecated and made into a no-op in OpenBSD 6.4 which corresponds
to LibreSSL 2.8.3.

>> This patch has a deficiency where 1.1.0 itself isn't actually rejected
>> at configure time; Daniel's working on an explicit check for the
>> OPENSSL/LIBRESSL_VERSION_NUMBER that should fix that up. There's an
>> open question about which version we should pin for LibreSSL, which
>> should ultimately come down to which versions of OpenBSD we want PG17
>> to support.
>
> I would be OK to draw a line to what we test in the buildfarm if it
> comes to that, down to OpenBSD 6.9. This version is already not
> supported, and we had a number of issues with older versions and
> timestamps going backwards.

There is a member on 6.8 as well, and while 6.8 work fine the tests all fail
due to the error messages being different. Rather than adding alternate output
for an EOL version of OpenBSD (which currently don't even run the ssl checks in
the BF) I think using 6.9 as the minimum makes sense.

> + # Functions introduced in OpenSSL 1.1.0/LibreSSL 2.7.0.
> + ['OPENSSL_init_ssl', {'required': true}],
> + ['BIO_meth_new', {'required': true}],
> + ['ASN1_STRING_get0_data', {'required': true}],
> + ['HMAC_CTX_new', {'required': true}],
> + ['HMAC_CTX_free', {'required': true}],
>
> These should be removed to save cycles in ./configure and meson, no?

Correct, they are removed in favor of a compile test for OpenSSL version.

> - cdata.set('OPENSSL_API_COMPAT', '0x10002000L',
> + cdata.set('OPENSSL_API_COMPAT', '0x10100000L',
>
> Seems to me that this should also document in meson.build why 1.1.0 is
> chosen, same as ./configure.

Done.

> It seems to me that src/common/protocol_openssl.c could be cleaned up;
> I see SSL_CTX_set_min_proto_version and SSL_CTX_set_max_proto_version
> listed in LibreSSL (looking at some past version of
> https://github.com/libressl/libressl.git that I still have around).

Both SSL_CTX_set_min_proto_version and SSL_CTX_set_max_proto_version are
available in at least OpenBSD 6.3 which is LibreSSL 2.7.5. With this we can
thus remove the whole file.

> There is an extra thing in pg_strong_random.c once we cut OpenSSL <
> 1.1.1.. Do we still need pg_strong_random_init() and its RAND_poll()
> when it comes to LibreSSL? This is a sensitive area, so we should be
> careful.

Re-reading the thread which added this comment, and the OpenSSL docs and code,
I'm leaning towards leaving this in. The overhead is marginal and fork safety
has been broken at least once in OpenSSL since 1.1.1:

https://github.com/openssl/openssl/issues/12377

That particular bug was thankfully caught before it shipped, but mitigating the
risk is this cheap enough that is seems reasonable to keep this in.

Attached is a WIP patch to get more eyes on it, the Meson test for 1.1.1 fails
on Windows in CI which I will investigate next.

--
Daniel Gustafsson