Cutting support for OpenSSL 1.0.1 and 1.0.2 in 17~?
Hi all,
$Subject says it all. Based on an analysis of the code, I can note
the following when it comes to the removal of 1.0.1:
- A lot of code related to channel binding is now simplified as
X509_get_signature_nid() always exists, mostly around its CFLAGS.
- This removes the comments related to 1.0.1e introduced by 74242c2.
Then for 1.0.2, the following flags can be gone:
HAVE_ASN1_STRING_GET0_DATA
HAVE_BIO_GET_DATA
HAVE_BIO_METH_NEW
HAVE_HMAC_CTX_FREE
HAVE_HMAC_CTX_NEW
It would be nice to remove CRYPTO_lock(), but from what I can see the
function still exists in LibreSSL, meaning that libpq locking wouldn't
be thread-safe if these function's paths are removed.
Another related question is how much do we care about builds with
LibreSSL with MSVC? This patch sets takes the simple path of assuming
that this has never been really tested, and if you look at the MSVC
scripts on HEAD we rely on a version number from OpenSSL, which is not
something LibreSSL copes nicely with already, as documented in
configure.ac.
OpenSSL 1.0.2 has been EOLd at the end of 2019, and 1.0.1 had its last
minor release in September 2019, so with Postgres 17 targetted in
September/October 2024, we would be five years behind that.
Last comes the buildfarm, and I suspect that a few animals are
building with 1.0.2. Among what I have spotted:
- wobbegong and vulpes, on Fedora 27, though I could not find
references about the version used there.
- bonito, on Fedora 29.
- SUSE 12 SP{4,5} have 1.0.2 as their newer version.
- butterflyfish may not like that, if I recall correctly, as it should
have 1.0.2.
So, it seems to me that 1.0.1 would be a rather silent move for the
buildfarm, and 1.0.2 could lead to some noise. Note as well that
1.1.0 support has been stopped by upstream at the same time as 1.0.1,
with a last minor release in September 2019, though that feels like a
huge jump at this stage. On a code basis, removing 1.0.1 leads to the
most cleanup. The removal of 1.0.2 would be nice, but the tweaks
needed for LibreSSL make it less appealing.
Attached are two patches to remove support for 1.0.1 and 1.0.2 for
now, kept separated for clarity, to be considered as something to do
at the beginning of the v17 cycle. 0001 is in a rather good shape
seen from here.
Now, for 0002 and the removal of 1.0.2, I am seeing two things once
OPENSSL_API_COMPAT is bumped from 0x10002000L to 0x10100000L:
- be-secure-openssl.c requires an extra openssl/bn.h, which is not a
big deal, from what I get.
- Much more interesting: OpenSSL_add_all_algorithms() has two macros
that get removed, see include/openssl/evp.h:
# ifdef OPENSSL_LOAD_CONF
# define OpenSSL_add_all_algorithms() \
OPENSSL_init_crypto(OPENSSL_INIT_ADD_ALL_CIPHERS \
| OPENSSL_INIT_ADD_ALL_DIGESTS \
| OPENSSL_INIT_LOAD_CONFIG, NULL)
# else
# define OpenSSL_add_all_algorithms() \
OPENSSL_init_crypto(OPENSSL_INIT_ADD_ALL_CIPHERS \
| OPENSSL_INIT_ADD_ALL_DIGESTS, NULL)
# endif
The part where I am not quite sure of what to do is about
OPENSSL_LOAD_CONF. We could call directly OPENSSL_init_crypto() and
add OPENSSL_INIT_LOAD_CONFIG if building with OPENSSL_LOAD_CONF, but
it feels a bit ugly to copy-paste this code from OpenSSL itself.
Note that patch 0002 still has OPENSSL_API_COMPAT at 0x10002000L.
OPENSSL_init_crypto() looks to be in LibreSSL, and it is new in
OpenSSL 1.1.0, so switching the minimum to OpenSSL 1.1.0 should not
require a new cflags on this one.
Thoughts?
--
Michael
On 24 May 2023, at 10:22, Michael Paquier <michael@paquier.xyz> wrote:
The removal of 1.0.2 would be nice, but the tweaks
needed for LibreSSL make it less appealing.
1.0.2 is also an LTS version available commercially for premium support
customers of OpenSSL (1.1.1 will become an LTS version as well), with 1.0.2zh
slated for release next week. This raises the likelyhood of Postgres
installations using 1.0.2 in production still, and for some time to come.
--
Daniel Gustafsson
On Wed, May 24, 2023 at 11:36:56AM +0200, Daniel Gustafsson wrote:
1.0.2 is also an LTS version available commercially for premium support
customers of OpenSSL (1.1.1 will become an LTS version as well), with 1.0.2zh
slated for release next week. This raises the likelyhood of Postgres
installations using 1.0.2 in production still, and for some time to come.
Good point. Indeed, that makes it pretty clear that not dropping
1.0.2 would be the best option for the time being, so 0001 would be
enough.
I am wondering if we should worry about having a buildfarm member that
could test these binaries, though, in case they have compatibility
issues.. But it would be harder to debug without the code at hand, as
well.
--
Michael
On 24 May 2023, at 11:52, Michael Paquier <michael@paquier.xyz> wrote:
On Wed, May 24, 2023 at 11:36:56AM +0200, Daniel Gustafsson wrote:
1.0.2 is also an LTS version available commercially for premium support
customers of OpenSSL (1.1.1 will become an LTS version as well), with 1.0.2zh
slated for release next week. This raises the likelyhood of Postgres
installations using 1.0.2 in production still, and for some time to come.Good point. Indeed, that makes it pretty clear that not dropping
1.0.2 would be the best option for the time being, so 0001 would be
enough.
I think thats the right move re 1.0.2 support. 1.0.2 is also the version in
RHEL7 which is in ELS until 2026.
When we moved the goalposts to 1.0.1 (commit 7b283d0e1d1) we referred to RHEL6
using 1.0.1, and RHEL6 goes out of ELS in late June 2024 seems possible to drop
1.0.1 support during v17. I haven't studied the patch yet but I'll have a look
at it.
I am wondering if we should worry about having a buildfarm member that
could test these binaries, though, in case they have compatibility
issues.. But it would be harder to debug without the code at hand, as
well.
It would be nice it the OpenSSL project could grant us an LTS license for a
buildfarm animal to ensure compatibility but I have no idea how realistic that
is (or how much the LTS version of 1.0.2 has diverged from the last available
public 1.0.2 version).
--
Daniel Gustafsson
Daniel Gustafsson <daniel@yesql.se> writes:
It would be nice it the OpenSSL project could grant us an LTS license for a
buildfarm animal to ensure compatibility but I have no idea how realistic that
is (or how much the LTS version of 1.0.2 has diverged from the last available
public 1.0.2 version).
Surely the answer must be "not much". The entire point of an LTS
version is to not have to change dusty calling applications.
We had definitely better have some animals still using 1.0.2, but
I don't see much reason to think that the last public release
wouldn't be good enough.
regards, tom lane
On Wed, May 24, 2023 at 01:03:04PM +0200, Daniel Gustafsson wrote:
When we moved the goalposts to 1.0.1 (commit 7b283d0e1d1) we referred to RHEL6
using 1.0.1, and RHEL6 goes out of ELS in late June 2024 seems possible to drop
1.0.1 support during v17. I haven't studied the patch yet but I'll have a look
at it.
Great, thanks for the help.
--
Michael
On 25 May 2023, at 00:18, Michael Paquier <michael@paquier.xyz> wrote:
On Wed, May 24, 2023 at 01:03:04PM +0200, Daniel Gustafsson wrote:
When we moved the goalposts to 1.0.1 (commit 7b283d0e1d1) we referred to RHEL6
using 1.0.1, and RHEL6 goes out of ELS in late June 2024 seems possible to drop
1.0.1 support during v17. I haven't studied the patch yet but I'll have a look
at it.
Patch looks to be in pretty good shape, just a few minor comments:
-#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH
+#ifdef USE_OPENSSL
Since this is only calling the pgtls abstraction API and not directly into the
SSL implementation we should use USE_SSL here instead. Same for the
corresponding hunks in the frontend code.
+ * Note that if we don't support channel binding if we're not using SSL at
+ * all, we would not have advertised the PLUS variant in the first place.
Seems like a word fell off when merging these sentences. This should probably
be "..support channel binding, or if we're no.." or something similar.-#if defined(USE_OPENSSL) && (defined(HAVE_X509_GET_SIGNATURE_NID) || defined(HAVE_X509_GET_SIGNATURE_INFO))
-#define HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
+#ifdef USE_OPENSSL
extern char *pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len);
#endif
No need for any guard at all now is there? All supported SSL implementations
have it, and I doubt we'd accept a new one that doesn't (or which can't
implement the function and error out).- # Functions introduced in OpenSSL 1.0.2. LibreSSL does not have
- # SSL_CTX_set_cert_cb().
- AC_CHECK_FUNCS([X509_get_signature_nid SSL_CTX_set_cert_cb])
+ # LibreSSL does not have SSL_CTX_set_cert_cb().
+ AC_CHECK_FUNCS([SSL_CTX_set_cert_cb])
The comment about when the function was introduced is still interesting and
should remain IMHO.The changes to the Windows buildsystem worry me a bit, but they don't move the
goalposts in either direction wrt to LibreSSL on Windows so for the purpose of
this patch we don't need to do more. Longer term we should either make
LibreSSL work on Windows builds, or explicitly not support it on Windows.
--
Daniel Gustafsson
On 24 May 2023, at 16:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:
We had definitely better have some animals still using 1.0.2, but
I don't see much reason to think that the last public release
wouldn't be good enough.
There are still RHEL7 animals like chub who use 1.0.2 so I'm not worried. We
might want to consider displaying the OpenSSL version number during configure
(meson already does it) in all branches to make it easier to figure out which
versions we have coverage for?
--
Daniel Gustafsson
On Thu, May 25, 2023 at 10:27:02AM +0200, Daniel Gustafsson wrote:
There are still RHEL7 animals like chub who use 1.0.2 so I'm not worried. We
might want to consider displaying the OpenSSL version number during configure
(meson already does it) in all branches to make it easier to figure out which
versions we have coverage for?
+1.
--
MichaelOn Thu, May 25, 2023 at 10:16:27AM +0200, Daniel Gustafsson wrote:
-#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH
+#ifdef USE_OPENSSL
Since this is only calling the pgtls abstraction API and not directly into the
SSL implementation we should use USE_SSL here instead. Same for the
corresponding hunks in the frontend code.
Makes sense, done.
+ * Note that if we don't support channel binding if we're not using SSL at + * all, we would not have advertised the PLUS variant in the first place. Seems like a word fell off when merging these sentences. This should probably be "..support channel binding, or if we're no.." or something similar.
Indeed, something has been eaten when merging these lines.
-#if defined(USE_OPENSSL) && (defined(HAVE_X509_GET_SIGNATURE_NID) || defined(HAVE_X509_GET_SIGNATURE_INFO)) -#define HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH +#ifdef USE_OPENSSL extern char *pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len); #endif No need for any guard at all now is there? All supported SSL implementations have it, and I doubt we'd accept a new one that doesn't (or which can't implement the function and error out).
Yup. It looks that you are right. A build without SSL is not
complaining once removed in libpq-int.h or libpq-be.h.
- # Functions introduced in OpenSSL 1.0.2. LibreSSL does not have - # SSL_CTX_set_cert_cb(). - AC_CHECK_FUNCS([X509_get_signature_nid SSL_CTX_set_cert_cb]) + # LibreSSL does not have SSL_CTX_set_cert_cb(). + AC_CHECK_FUNCS([SSL_CTX_set_cert_cb]) The comment about when the function was introduced is still interesting and should remain IMHO.
Okay. Kept as well in meson.build.
The changes to the Windows buildsystem worry me a bit, but they don't move the
goalposts in either direction wrt to LibreSSL on Windows so for the purpose of
this patch we don't need to do more. Longer term we should either make
LibreSSL work on Windows builds, or explicitly not support it on Windows.
Yes, I was wondering what to do about that in the long term. With the
argument that the MSVC scripts may be gone over meson, it is not
really appealing to dig into that.
Something that was missing in 0001 is the bump of OPENSSL_API_COMPAT
in meson.build. This was in 0002.
Please find attached an updated patch only for the removal of 1.0.1.
Thanks for the review.
--
Michael
Attachments:
v2-0001-Remove-support-for-OpenSSL-1.0.1.patchtext/x-diff; charset=us-asciiDownload+37-104
On 26 May 2023, at 04:08, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, May 25, 2023 at 10:16:27AM +0200, Daniel Gustafsson wrote:
The changes to the Windows buildsystem worry me a bit, but they don't move the
goalposts in either direction wrt to LibreSSL on Windows so for the purpose of
this patch we don't need to do more. Longer term we should either make
LibreSSL work on Windows builds, or explicitly not support it on Windows.Yes, I was wondering what to do about that in the long term. With the
argument that the MSVC scripts may be gone over meson, it is not
really appealing to dig into that.
Yeah, let's revisit this during the v17 cycle when the future of these scripts
will become clearer.
Something that was missing in 0001 is the bump of OPENSSL_API_COMPAT
in meson.build. This was in 0002.Please find attached an updated patch only for the removal of 1.0.1.
Thanks for the review.
LGTM.
--
Daniel Gustafsson
On Thu, May 25, 2023 at 7:09 PM Michael Paquier <michael@paquier.xyz> wrote:
Please find attached an updated patch only for the removal of 1.0.1.
Thanks for the review.
Nice! Sorry about the new complications with LibreSSL. :(
- # Functions introduced in OpenSSL 1.0.2. LibreSSL does not have + # Function introduced in OpenSSL 1.0.2. LibreSSL does not have # SSL_CTX_set_cert_cb(). - AC_CHECK_FUNCS([X509_get_signature_nid SSL_CTX_set_cert_cb]) + AC_CHECK_FUNCS([SSL_CTX_set_cert_cb])
Can X509_get_signature_nid be moved to the required section up above?
As it is now, anyone configuring with -Dssl=auto can still pick up a
1.0.1 build, which Meson accepts, and then the build fails downstream.
If we require the function instead, Meson will ignore 1.0.1 (or, for
-Dssl=openssl, complain before we compile).
t/001_ssltests.pl has a reference to 1.0.1 that can probably be
entirely deleted:
# ... (Nor for OpenSSL
# 1.0.1, but that's old enough that accommodating it isn't worth the cost.)
Thanks,
--Jacob
On Fri, May 26, 2023 at 09:10:17AM -0700, Jacob Champion wrote:
Can X509_get_signature_nid be moved to the required section up above?
As it is now, anyone configuring with -Dssl=auto can still pick up a
1.0.1 build, which Meson accepts, and then the build fails downstream.
If we require the function instead, Meson will ignore 1.0.1 (or, for
-Dssl=openssl, complain before we compile).
Yes, I was wondering a bit if something more should be marked as
required, but just saw more value in removing all references to this
function. Making the build fail straight when setting up things is OK
by me, but I am not convinced that X509_get_signature_nid() would be
the right choice for the job, as it is an OpenSSL artifact originally,
AFAIK.
The same problem exists with OpenSSL 1.0.0 on HEAD when building with
meson? CRYPTO_new_ex_data() and SSL_new() exist there.
t/001_ssltests.pl has a reference to 1.0.1 that can probably be
entirely deleted:# ... (Nor for OpenSSL
# 1.0.1, but that's old enough that accommodating it isn't worth the cost.)
Not touching that is intentional. It sounded useful to me as an
historic reference for LibreSSL ;)
--
Michael
On 27 May 2023, at 04:02, Michael Paquier <michael@paquier.xyz> wrote:
Making the build fail straight when setting up things is OK
by me, but I am not convinced that X509_get_signature_nid() would be
the right choice for the job, as it is an OpenSSL artifact originally,
AFAIK.
I think we should avoid the is-defined-in dance and just pull out the version
numbers for comparisons. While it's true that LibreSSL doesn't play well with
OpenSSL versions, they do define their own which can be checked for to
distinguish the libraries.
--
Daniel Gustafsson
On Fri, Jun 02, 2023 at 10:35:43AM +0200, Daniel Gustafsson wrote:
I think we should avoid the is-defined-in dance and just pull out the version
numbers for comparisons. While it's true that LibreSSL doesn't play well with
OpenSSL versions, they do define their own which can be checked for to
distinguish the libraries.
Agreed. How about tackling that in a separate patch? At this stage,
I would like to not care about ./configure and do it only with meson,
but there is value in reporting the version number in ./configure to
see the version coverage in the buildfarm.
--
Michael
On 2 Jun 2023, at 23:21, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Jun 02, 2023 at 10:35:43AM +0200, Daniel Gustafsson wrote:
I think we should avoid the is-defined-in dance and just pull out the version
numbers for comparisons. While it's true that LibreSSL doesn't play well with
OpenSSL versions, they do define their own which can be checked for to
distinguish the libraries.Agreed. How about tackling that in a separate patch?
Absolutely, let's keep these goalposts in place and deal with that separately.
--
Daniel Gustafsson
On Fri, Jun 02, 2023 at 11:23:19PM +0200, Daniel Gustafsson wrote:
Absolutely, let's keep these goalposts in place and deal with that separately.
I have not gone back to this part yet, though I plan to do so. As we
are at the beginning of the development cycle, I have applied the
patch to remove support for 1.0.1 for now on HEAD. Let's see what the
buildfarm tells.
--
Michael
On Mon, Jul 3, 2023 at 4:26 PM Michael Paquier <michael@paquier.xyz> wrote:
I have not gone back to this part yet, though I plan to do so. As we
are at the beginning of the development cycle, I have applied the
patch to remove support for 1.0.1 for now on HEAD. Let's see what the
buildfarm tells.
curculio (OpenBSD 5.9) is failing with "undefined reference to
`X509_get_signature_nid'", but that's OK, Mikael already supplied a
modern OpenBSD system to replace it (schnauzer, which is green) and he
was planning to shut curculio down (see Direct I/O thread where that
came up because its GCC 4.2 compiler doesn't understand our stack
alignment directives; it will also break comprehensively when I push
the nearby all-supported-computers-have-locale_t patch from the
check_strxfrm_bug thread).
On 3 Jul 2023, at 20:40, Thomas Munro <thomas.munro@gmail.com> wrote:
On Mon, Jul 3, 2023 at 4:26 PM Michael Paquier <michael@paquier.xyz> wrote:
I have not gone back to this part yet, though I plan to do so. As we
are at the beginning of the development cycle, I have applied the
patch to remove support for 1.0.1 for now on HEAD. Let's see what the
buildfarm tells.curculio (OpenBSD 5.9) is failing with "undefined reference to
`X509_get_signature_nid'", but that's OK, Mikael already supplied a
modern OpenBSD system to replace it
Thanks for the report! OpenBSD 5.9 was released in 2016 and is thus well over
5 years EOL, so I agree that it doesn't warrant a code change from us to
support this.
--
Daniel Gustafsson
On 2023-07-03 20:53, Daniel Gustafsson wrote:
curculio (OpenBSD 5.9) is failing with "undefined reference to
`X509_get_signature_nid'", but that's OK, Mikael already supplied a
modern OpenBSD system to replace itThanks for the report! OpenBSD 5.9 was released in 2016 and is thus well over
5 years EOL, so I agree that it doesn't warrant a code change from us to
support this.
I have retired curculio now. So it will stop reporting in from now on.
/Mikael
