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

On Thu, Apr 4, 2024 at 6:37 PM Michael Paquier <michael@paquier.xyz> wrote:
> From where did you pull the LibreSSL sources?  Directly from the
> OpenBSD tree?

I've been building LibreSSL Portable: https://github.com/libressl/portable

> Ah, right.  OpenSSL_add_all_algorithms() is documented as having no
> effect in 1.1.0.

I think it still has an effect, but it's unnecessary unless you need
some specific initialization other than the defaults -- for which we
now have OPENSSL_init_crypto(). Best I could tell, pgcrypto had no
such requirements (but extra eyes on that would be appreciated).

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

That would correspond to LibreSSL 3.3 if I'm not mistaken. Any
particular reason for 6.9 as the dividing line, and not something
later? And by "test in the buildfarm", do you mean across all
versions, or just what we support for PG17? (For the record, I don't
think there's any reason to drop older LibreSSL testing for earlier
branches.)

> This version is already not
> supported, and we had a number of issues with older versions and
> timestamps going backwards.

Speaking of which: for completeness I should note that LibreSSL 3.2
(OpenBSD 6.8) is failing src/test/ssl because of alternate error
messages. That failure exists on HEAD and is not introduced in this
patchset. LibreSSL 3.3, which passes, has the following changelog
note:

  * If x509_verify() fails, ensure that the error is set on both
    the x509_verify_ctx() and its store context to make some failures
    visible from SSL_get_verify_result().

So that would explain that. If we drop support for 3.2 and earlier
then there's nothing to be done anyway.

> -/* Define to 1 if you have the `CRYPTO_lock' function. */
> -#undef HAVE_CRYPTO_LOCK
>
> I'm happy to see that gone for good.

+1

> +      # 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?
> We don't have any more of their HAVE_* flags in the tree with this
> patch applied.

True, but they are required, and I needed something in there to reject
earlier builds. Daniel's suggested approach with _VERSION_NUMBER
should be able to replace this I think.

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

Good point.

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

It would be cool if there are more cleanups that can happen. I agree
we need to be careful around removal, though, especially now that we
know that LibreSSL testing is spotty... I will look more into these
later today.

Thanks,
--Jacob