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

From: Michael Paquier <[email-masked]>
Date: Mon, 15 Apr 2024 14:04:12 +0900

On Fri, Apr 12, 2024 at 02:42:57PM +0200, Daniel Gustafsson wrote:
>> On 10 Apr 2024, at 09:31, Peter Eisentraut <[email-masked]> wrote:
>> 2. Move to 1.1.1.  I understand this has to do with the fork-safety of pg_strong_random(), and it's not an API change but a behavior change. Let's make this association clearer in the code.  For example, add a version check or assertion about this into pg_strong_random() itself.

> 0005 moves the fork safety init inline with calling pg_strong_random, and
> removes it for everyone else.  This allows 1.1.0 to be supported as we
> effectively are at the 1.1.0 API level, at the cost of calls for strong random
> being slower on 1.1.0.  An unscientific guess based on packaged OpenSSL
> versions and the EOL and ELS/LTS status of 1.1.0, is that the number of
> production installs of postgres 17 using OpenSSL 1.1.0 is close to zero.

It is only necessary to call RAND_poll once after forking.  Wouldn't
it be OK to use a static flag and use the initialization once?

>> * src/port/pg_strong_random.c
>> 
>> I would prefer to remove pg_strong_random_init() if it's no longer
>> useful.  I mean, if we leave it as is, and we are not removing any
>> callers, then we are effectively continuing to support OpenSSL
>> <1.1.1, right?
> 
> The attached removes pg_strong_random_init and instead calls it explicitly for
> 1.1.0 users by checking the OpenSSL version.
> 
> Is the attached split in line with how you were thinking about it?

If I may, 0001 looks sensible here.  The bits from 0003 and 0004 could
be applied before 0002, as well.

--- a/src/backend/postmaster/fork_process.c
+++ b/src/backend/postmaster/fork_process.c
@@ -110,9 +110,6 @@ fork_process(void)
 				close(fd);
 			}
 		}
-
-		/* do post-fork initialization for random number generation */
-		pg_strong_random_init();

Perhaps you intented this diff to be in 0005 rather than in 0002?
With 0002 applied, only support for 1.0.2 is removed, not 1.1.0 yet.

 pg_strong_random(void *buf, size_t len)
 {
 	int			i;
 
+#if (OPENSSL_VERSION_NUMBER <= 0x10100000L)
+	/*
+	 * Make sure processes do not share OpenSSL randomness state.  This is not
+	 * requred on LibreSSL and no longer required in OpenSSL 1.1.1 and later
+	 * versions.
+	 */
+	RAND_poll();
+#endif

s/requred/required/.  Rather than calling always RAND_poll(), this
could use a static flag to call it only once when pg_strong_random is
called for the first time.  I would not mind seeing this part entirely
gone with the removal of support for 1.1.0.
--
Michael

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 15 Apr 2024 11:07:05 +0200

> On 15 Apr 2024, at 07:04, Michael Paquier <[email-masked]> wrote:

> be applied before 0002, as well.

Agreed, once we are in post-freeze I think those three are mostly ready to go.

> With 0002 applied, only support for 1.0.2 is removed, not 1.1.0 yet.

Yes, nice catch, that was a mistake in splitting up the patch into multiple
pieces, it should be in the 0005 patch for strong random. Fixed.

> s/requred/required/.

Fixed.

> Rather than calling always RAND_poll(), this
> called for the first time.

Agreed, we can good that. Fixed.

> I would not mind seeing this part entirely

If we want to keep autoconf from checking versions and just check compatibility
(with our code) then we will remain at 1.1.0 compatibility.  The only 1.1.1 API
we use is not present in LibreSSL so we can't really place a hard restriction
on that.  It might be that keeping it for now, and removing it later during the
v18 cycle as we modernize our OpenSSL code (which I hope to find time to work
on) and make use of newer 1.1.1 API:s.  That way we can keep our autoconf/meson
checks consistent across library checks.  If we end up with no new API:s to
check for by the time the last commitfest of v18 rolls around, we can revisit
the decision then.

--
Daniel Gustafsson

---

From: Michael Paquier <[email-masked]>
Date: Tue, 16 Apr 2024 08:03:41 +0900

> Agreed, once we are in post-freeze I think those three are mostly

Is there a point to wait for 0001, 0003 and 0004, though, and
shouldn't these three be backpatched?  0001 is certainly OK as a
doc-only change to be consistent across the board without waiting 5
more years.  0003 and 0004 are conditional and depend on if
SSL_R_VERSION_TOO_LOW and SSL_OP_NO_CLIENT_RENEGOTIATION are defined
at compile-time.  0003 is much more important, and note that
01e6f1a842f4 has been backpatched all the way down.  0004 is nice,
still not strongly mandatory.

+#if (OPENSSL_VERSION_NUMBER <= 0x10100000L)
+	static bool rand_initialized = false;

This does not look right.  At the top of upstream's branch
OpenSSL_1_1_0-stable, OPENSSL_VERSION_NUMBER is 0x101000d0L, so the
initialization would be missed for any version in the 1.1.0 series
except the GA one without this code being compiled, no?

> check for by the time the last commitfest of v18 rolls around, we can revisit

Okay, fine by me.
--
Michael

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 16 Apr 2024 10:17:26 +0200

> On 16 Apr 2024, at 01:03, Michael Paquier <[email-masked]> wrote:
> 

I forgot (and didn't check) that we backpatched 01e6f1a842f4, with that in mind
I agree that we should backpatch 0003 as well to put LibreSSL on par as much as
we can.  0004 is a fix for the LibreSSL support, not adding anything new, so
pushing that to master now makes sense.  Unless objections are raised I'll push
0001, 0003 and 0004 shortly.  0002 and 0005 can hopefully be addressed in the
July commitfest.

> except the GA one without this code being compiled, no?

Meh, I was clearly not caffeinated as that's a thinko with a typo attached to
it.  The check should be "< 0x10101000" to catch any version prior to 1.1.1.
Fixed.

--
Daniel Gustafsson

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 18 Apr 2024 12:53:43 +0200

> 0001, 0003 and 0004 shortly.  0002 and 0005 can hopefully be addressed in the

Review of the latest batch:

* v9-0001-Doc-Use-past-tense-for-things-which-happened-in-t.patch

Ok

8 v9-0002-Remove-support-for-OpenSSL-1.0.2.patch

Ok, but maybe make the punctuation consistent here:

+      # Function introduced in OpenSSL 1.0.2, not in LibreSSL.
+      ['SSL_CTX_set_cert_cb'],
+
+      # Function introduced in OpenSSL 1.1.1, not in LibreSSL
        ['X509_get_signature_info'],

* v9-0003-Support-disallowing-SSL-renegotiation-in-LibreSSL.patch

ok

* v9-0004-Support-SSL_R_VERSION_TOO_LOW-on-LibreSSL.patch

Seems ok, but the reason isn't clear to me.  Are there LibreSSL versions 
that have SSL_R_VERSION_TOO_LOW but not SSL_R_VERSION_TOO_HIGH?  Maybe 
this could be explained better.

Also, "OpenSSL 7.2" in the commit message probably meant "OpenBSD"?

* v9-0005-Remove-pg_strong_random-initialization.patch

I don't understand the reason for this phrase in the commit message: 
"1.1.1 is being increasingly phased out from production use".  Did you 
mean 1.1.0 there?

Conditionally sticking the RAND_poll() into pg_strong_random(), does 
that have the effect we want?  It wouldn't reinitialize after a fork, 
AFAICT.

If everything is addressed, I agree that 0001, 0003, and 0004 can go 
into PG17, the rest later.

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 18 Apr 2024 22:26:33 +0200

> On 18 Apr 2024, at 12:53, Peter Eisentraut <[email-masked]> wrote:

> Review of the latest batch:

Thanks for reviewing!

Fixed.

> Seems ok, but the reason isn't clear to me.  Are there LibreSSL versions that have SSL_R_VERSION_TOO_LOW but not SSL_R_VERSION_TOO_HIGH?  Maybe this could be explained better.

LibreSSL doesn't support SSL_R_VERSION_TOO_HIGH at all, they only support
_TOO_LOW starting with the OpenBSD 7.2 release.  I've expanded the commit
message to document this.

> Also, "OpenSSL 7.2" in the commit message probably meant "OpenBSD"?

Ah yes, fixed.

> I don't understand the reason for this phrase in the commit message: "1.1.1 is being increasingly phased out from production use".  Did you mean 1.1.0 there?

Correct, I got lost among the version numbers it seems. Fixed.

> Conditionally sticking the RAND_poll() into pg_strong_random(), does that have the effect we want?  It wouldn't reinitialize after a fork, AFAICT.

No I think you're right, my previous version would have worked (but was ugly)
but this doesn't guarantee that.  Thinking more about it maybe it's best to
just keep the init function and have a version check for 1.1.0 there, making it
an empty no-op for all other cases.  When we move past 1.1.0 due to a new API
requirement we can blow it all away.

> If everything is addressed, I agree that 0001, 0003, and 0004 can go into PG17, the rest later.

Agreed, 0002 and 0005 are clearly for the v18 cycle.

--
Daniel Gustafsson

---

From: Michael Paquier <[email-masked]>
Date: Fri, 19 Apr 2024 14:37:35 +0900

> If everything is addressed, I agree that 0001, 0003, and 0004 can go into

About the PG17 bits, would you agree about a backpatch?  Or perhaps
you disagree?
--
Michael

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 19 Apr 2024 08:55:32 +0200

> On 19 Apr 2024, at 07:37, Michael Paquier <[email-masked]> wrote:
> 

If we want to 0001 can be baclpatched to v15, 0004 to v13 and 0003 all the way.
I don't have strong opinions either way.

--
Daniel Gustafsson

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 19 Apr 2024 10:06:26 +0200

> About the PG17 bits, would you agree about a backpatch?  Or perhaps

I don't think any of these need to be backpatched, at least right now.

0001 is just a cosmetic documentation tweak, has no reason to be 
backpatched.

0003 adds new functionality for LibreSSL.  While the code looks 
straightforward, we have little knowledge about how it works in 
practice.  How is the buildfarm coverage of LibreSSL (with SSL tests 
enabled!)?  If people are keen on this, it might be better to get it 
into PG17 and at least let to go through a few months of beta testing.

0004 effectively just enhances an error message for LibreSSL; there is 
little reason to backpatch this.

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 23 Apr 2024 22:08:13 +0200

> On 19 Apr 2024, at 10:06, Peter Eisentraut <[email-masked]> wrote:
> 
> 0001 is just a cosmetic documentation tweak, has no reason to be backpatched.
> 
> 0003 adds new functionality for LibreSSL.  While the code looks straightforward, we have little knowledge about how it works in practice.  How is the buildfarm coverage of LibreSSL (with SSL tests enabled!)?  If people are keen on this, it might be better to get it into PG17 and at least let to go through a few months of beta testing.
> 
> 0004 effectively just enhances an error message for LibreSSL; there is little reason to backpatch this.

Hearing no objections to this plan (and the posted v10), I'll go ahead with
0001, 0003 and 0004 into v17 tomorrow.

--
Daniel Gustafsson

---

From: Michael Paquier <[email-masked]>
Date: Wed, 24 Apr 2024 07:20:27 +0900

> 0001, 0003 and 0004 into v17 tomorrow.

WFM, thanks.
--
Michael

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 24 Apr 2024 13:31:12 +0200

> On 24 Apr 2024, at 00:20, Michael Paquier <[email-masked]> wrote:
> 

Done.  Attached are the two remaining patches, rebased over HEAD, for removing
support for OpenSSL 1.0.2 in v18. Parking them in the commitfest for now.

--
Daniel Gustafsson

---

From: Michael Paquier <[email-masked]>
Date: Thu, 25 Apr 2024 12:49:16 +0900

> support for OpenSSL 1.0.2 in v18. Parking them in the commitfest for now.

You have mentioned once upthread the documentation of PQinitOpenSSL():
   However, this is unnecessary when using <productname>OpenSSL</productname>
   version 1.1.0 or later, as duplicate initializations are no longer problematic.

With 1.0.2's removal in place, this could be simplified more and the
patch does not touch it.  This relates also to pq_init_crypto_lib,
which is gone with 0001.  Of course, it is not possible to just remove
PQinitOpenSSL() or old application may fail linking.  Removing
pq_init_crypto_lib reduces any confusion around this part of the
initialization.

0002 looks OK here.
--
Michael

---

From: Daniel Gustafsson <[email-masked]>
Date: Sat, 27 Apr 2024 20:32:19 +0200

> On 25 Apr 2024, at 05:49, Michael Paquier <[email-masked]> wrote:
> 

That's a good point, there is potential for more code removal here.  The
attached 0001 takes a stab at it while it's fresh in mind, I'll revisit before
the July CF to see if there is more that can be done.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Sat, 27 Apr 2024 20:33:55 +0200

> On 27 Apr 2024, at 20:32, Daniel Gustafsson <[email-masked]> wrote:

> the July CF to see if there is more that can be done.

..and again with the attachment. Not enough coffee.

--
Daniel Gustafsson

---

From: Michael Paquier <[email-masked]>
Date: Wed, 1 May 2024 13:21:32 +0900

> ..and again with the attachment. Not enough coffee.

My remark was originally about pq_init_crypto_lib that does the
locking initialization, and your new patch a bit more, as of:

-    /* This stuff need be done only once. */
-    if (!SSL_initialized)
-    {
-#ifdef HAVE_OPENSSL_INIT_SSL
-        OPENSSL_init_ssl(OPENSSL_INIT_LOAD_CONFIG, NULL);
-#else
-        OPENSSL_config(NULL);
-        SSL_library_init();
-        SSL_load_error_strings();
-#endif
-        SSL_initialized = true;
-    }

OPENSSL_init_ssl() has replaced SSL_library_init(), marked as
deprecated, and even this step is mentioned as not required anymore
with 1.1.0~:
https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_ssl.html

Same with OPENSSL_init_crypto(), replacing OPENSSL_config(), again not
required in 1.1.0~:
https://www.openssl.org/docs/man1.1.1/man3/OPENSSL_init_crypto.html

SSL_load_error_strings() is recommended as not to use in 1.1.0,
replaced by the others:
https://www.openssl.org/docs/man3.2/man3/SSL_load_error_strings.html

While OpenSSL will be able to cope with that, how much of that applies
to LibreSSL?  SSL_load_error_strings(), OPENSSL_init_ssl(),
OPENSSL_CONFIG() are OK based on the docs:
https://man.archlinux.org/man/extra/libressl/libressl-OPENSSL_config.3.en
https://man.archlinux.org/man/extra/libressl/libressl-OPENSSL_init_ssl.3.en
https://man.archlinux.org/man/extra/libressl/libressl-ERR_load_crypto_strings.3.en

So +1 to remove all this code after a closer lookup.  I would
recommend to update the documentation of PQinitSSL and PQinitOpenSSL
to tell that these become useless and are deprecated.

    ERR_clear_error();
-
#ifdef USE_RESOWNER_FOR_HMAC

Some noise diff.
--
Michael

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 3 May 2024 10:39:15 +0200

> On 1 May 2024, at 06:21, Michael Paquier <[email-masked]> wrote:

> ...
> So +1 to remove all this code after a closer lookup.

Thanks for review.

>  I would

They are no-ops when linking against v18, but writing an extension which
targets all supported versions of postgres along with their respective
supported OpenSSL versions make them still required, or am I missing something?

> #ifdef USE_RESOWNER_FOR_HMAC

Will fix with a new rebase when once the tree has settled down from the
post-freeze fixups in this area.

--
Daniel Gustafsson

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 3 May 2024 21:02:12 +0200

> supported OpenSSL versions make them still required, or am I missing something?

I don't think extensions come into play here, since this is libpq, so 
only the shared library interface compatibility matters.

---

From: Tom Lane <[email-masked]>
Date: Fri, 03 May 2024 15:21:33 -0400

> only the shared library interface compatibility matters.

Yeah, server-side extensions don't really seem to be at hazard,
but doesn't the argument apply to client-side applications and
libraries that want to work across different PG/OpenSSL versions?

			regards, tom lane

---

From: Daniel Gustafsson <[email-masked]>
Date: Sat, 4 May 2024 10:08:48 +0200

> On 3 May 2024, at 21:21, Tom Lane <[email-masked]> wrote:
> 

Right, I was using "extension" a bit carelessly, what I meant was client-side
applications using libpq. 

--
Daniel Gustafsson

---

From: Michael Paquier <[email-masked]>
Date: Tue, 7 May 2024 08:31:29 +0900

> supported OpenSSL versions make them still required, or am I missing something?

Yeah, that depends on how much version you expect your application to
work on.  Still it seems to me that there's value in mentioning that
if your application does not care about anything older than OpenSSL 
1.1.0, like PG 18 assuming that this patch is merged, then these calls
are pointless for HEAD.  The routine definitions would be around only
for the .so compatibility.
--
Michael

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 7 May 2024 12:36:24 +0200

> On 7 May 2024, at 01:31, Michael Paquier <[email-masked]> wrote:
> 

Fair enough.  I've taken a stab at documenting that the functions are
deprecated, while at the same time documenting when and how they can be used
(and be useful).  The attached also removes one additional comment in the
testcode which is now obsolete (since removing 1.0.1 support really), and fixes
the spurious whitespace you detected upthread.

--
Daniel Gustafsson

---

From: Michael Paquier <[email-masked]>
Date: Wed, 8 May 2024 09:45:24 +0900

> the spurious whitespace you detected upthread.

+       This function is deprecated and only present for backwards compatibility,
+       it does nothing.
[...]
+       <xref linkend="libpq-PQinitSSL"/> and <xref linkend="libpq-PQinitOpenSSL"/>
+       are maintained for backwards compatibility, but are no longer required
+       since <productname>PostgreSQL</productname> 18.

LGTM, thanks for doing this!
--
Michael

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 11 Jul 2024 22:22:50 +0100

> the spurious whitespace you detected upthread.

The 0001 patch removes the functions pgtls_init_library() and 
pgtls_init() but keeps the declarations in libpq-int.h.  This should be 
fixed.