Обсуждение: Remove deprecation warnings when compiling PG ~13 with OpenSSL 3.0~

Hi all,
(adding Daniel in CC.)

Compiling Postgres up to 13 with OpenSSL 3.0 leads to a couple of
compilation warnings with what OpenSSL considers as deprecated, like:
sha2_openssl.c: In function pg_sha384_init
sha2_openssl.c:70:9: warning: SHA384_Init is deprecated =
Since OpenSSL 3.0 [-Wdeprecated-declarations]
   70 |         SHA384_Init((SHA512_CTX *) ctx);
      |         ^~~~~~~~~~~
/usr/include/openssl/sha.h:119:27: note: declared here
  119 | OSSL_DEPRECATEDIN_3_0 int SHA384_Init(SHA512_CTX *c);

I was looking at the code of OpenSSL to see if there would be a way to
silenced these, and found about OPENSSL_SUPPRESS_DEPRECATED.

I have been annoyed by these in the past when doing backpatches, as
this creates some noise, and the only place where this counts is
sha2_openssl.c.  Thoughts about doing something like the attached for
~13?
--
Michael

Hi,

On 2023-06-21 11:53:44 +0900, Michael Paquier wrote:
> Compiling Postgres up to 13 with OpenSSL 3.0 leads to a couple of
> compilation warnings with what OpenSSL considers as deprecated, like:
> sha2_openssl.c: In function pg_sha384_init
> sha2_openssl.c:70:9: warning: SHA384_Init is deprecated =
> Since OpenSSL 3.0 [-Wdeprecated-declarations]
>    70 |         SHA384_Init((SHA512_CTX *) ctx);
>       |         ^~~~~~~~~~~
> /usr/include/openssl/sha.h:119:27: note: declared here
>   119 | OSSL_DEPRECATEDIN_3_0 int SHA384_Init(SHA512_CTX *c);
> 
> I was looking at the code of OpenSSL to see if there would be a way to
> silenced these, and found about OPENSSL_SUPPRESS_DEPRECATED.
> 
> I have been annoyed by these in the past when doing backpatches, as
> this creates some noise, and the only place where this counts is
> sha2_openssl.c.  Thoughts about doing something like the attached for
> ~13?

Wouldn't the proper fix be to backpatch 4d3db13621b? Just suppressing all
deprecations doesn't strike me as particularly wise, especially because we've
chosen a different path for 14+?

Greetings,

Andres Freund

> On 21 Jun 2023, at 07:44, Andres Freund <andres@anarazel.de> wrote:
> On 2023-06-21 11:53:44 +0900, Michael Paquier wrote:

>> I have been annoyed by these in the past when doing backpatches, as
>> this creates some noise, and the only place where this counts is
>> sha2_openssl.c.  Thoughts about doing something like the attached for
>> ~13?
> 
> Wouldn't the proper fix be to backpatch 4d3db13621b?

Agreed, I'd be more inclined to go with OPENSSL_API_COMPAT.  If we still get
warnings with that set then I feel those warrant special consideration rather
than a blanket suppression.

--
Daniel Gustafsson

On Wed, Jun 21, 2023 at 09:16:38AM +0200, Daniel Gustafsson wrote:
> Agreed, I'd be more inclined to go with OPENSSL_API_COMPAT.  If we still get
> warnings with that set then I feel those warrant special consideration rather
> than a blanket suppression.

4d3db136 seems to be OK on REL_13_STABLE with a direct cherry-pick.
REL_11_STABLE and REL_12_STABLE require two different changes:
- pg_config.h.win32 needs to list OPENSSL_API_COMPAT.
- Solution.pm needs an extra #define OPENSSL_API_COMPAT in
GenerateFiles() whose value can be retrieved from configure.in like in
13~.

Anything I am missing perhaps?
--
Michael

On 21.06.23 09:43, Michael Paquier wrote:
> On Wed, Jun 21, 2023 at 09:16:38AM +0200, Daniel Gustafsson wrote:
>> Agreed, I'd be more inclined to go with OPENSSL_API_COMPAT.  If we still get
>> warnings with that set then I feel those warrant special consideration rather
>> than a blanket suppression.
> 
> 4d3db136 seems to be OK on REL_13_STABLE with a direct cherry-pick.
> REL_11_STABLE and REL_12_STABLE require two different changes:
> - pg_config.h.win32 needs to list OPENSSL_API_COMPAT.
> - Solution.pm needs an extra #define OPENSSL_API_COMPAT in
> GenerateFiles() whose value can be retrieved from configure.in like in
> 13~.
> 
> Anything I am missing perhaps?

Backpatching the OPENSSL_API_COMPAT change would set the minimum OpenSSL 
version to 1.0.1, which is newer than what was so far required in those 
branches.  That is the reason we didn't do this.

Hi,

On 2023-06-21 10:11:33 +0200, Peter Eisentraut wrote:
> On 21.06.23 09:43, Michael Paquier wrote:
> > On Wed, Jun 21, 2023 at 09:16:38AM +0200, Daniel Gustafsson wrote:
> > > Agreed, I'd be more inclined to go with OPENSSL_API_COMPAT.  If we still get
> > > warnings with that set then I feel those warrant special consideration rather
> > > than a blanket suppression.
> > 
> > 4d3db136 seems to be OK on REL_13_STABLE with a direct cherry-pick.
> > REL_11_STABLE and REL_12_STABLE require two different changes:
> > - pg_config.h.win32 needs to list OPENSSL_API_COMPAT.
> > - Solution.pm needs an extra #define OPENSSL_API_COMPAT in
> > GenerateFiles() whose value can be retrieved from configure.in like in
> > 13~.
> > 
> > Anything I am missing perhaps?
> 
> Backpatching the OPENSSL_API_COMPAT change would set the minimum OpenSSL
> version to 1.0.1, which is newer than what was so far required in those
> branches.  That is the reason we didn't do this.

What's the problem with just setting a different version in those branches?

Greetings,

Andres Freund

On Wed, Jun 21, 2023 at 10:11:33AM +0200, Peter Eisentraut wrote:
> Backpatching the OPENSSL_API_COMPAT change would set the minimum OpenSSL
> version to 1.0.1, which is newer than what was so far required in those
> branches.  That is the reason we didn't do this.

Looking at the relevant thread from 2020, this was still at the point
where we did not consider supporting 3.0 for all the stable branches
because 3.0 was in alpha:
https://www.postgresql.org/message-id/3d4afcfc-0930-1389-b9f7-59bdf11fb125@2ndquadrant.com

However, recent fixes like cab553a have made that possible, and we do
build with OpenSSL 3.0 across the whole set of stable branches.
Regarding the versions of OpenSSL supported:
- REL_13_STABLE requires 1.0.1 since 7b283d0e1.
- REL_12_STABLE and REL_11_STABLE require 0.9.8.

For 0.9.8, OPENSSL_API_COMPAT needs to be set at 0x00908000L (see
upstream's CHANGES.md).  So I don't see a reason not to do as
suggested by Andres?

I have tested the attached patches across 11~13 with various versions
of OpenSSL (OPENSSL_API_COMPAT exists since 1.1.0), and this is
working here.  Note that I don't have a MSVC environment at hand to
test this change on Windows, still `perl -cw Solution.pm` is OK with
it.

What do you think about the attached patch set (one for each branch)?
--
Michael

> On 22 Jun 2023, at 01:53, Michael Paquier <michael@paquier.xyz> wrote:

> I have tested the attached patches across 11~13 with various versions
> of OpenSSL (OPENSSL_API_COMPAT exists since 1.1.0), and this is
> working here.  Note that I don't have a MSVC environment at hand to
> test this change on Windows, still `perl -cw Solution.pm` is OK with
> it.

These patches LGTM from reading, but I think the Discussion link in the commit
messages should refer to this thread as well.

--
Daniel Gustafsson

On Thu, Jun 22, 2023 at 10:02:58AM +0200, Daniel Gustafsson wrote:
> These patches LGTM from reading,

Thanks for double-checking.

> but I think the Discussion link in the commit
> messages should refer to this thread as well.

Of course.
--
Michael

On 22.06.23 01:53, Michael Paquier wrote:
> Looking at the relevant thread from 2020, this was still at the point
> where we did not consider supporting 3.0 for all the stable branches
> because 3.0 was in alpha:
> https://www.postgresql.org/message-id/3d4afcfc-0930-1389-b9f7-59bdf11fb125@2ndquadrant.com
> 
> However, recent fixes like cab553a have made that possible, and we do
> build with OpenSSL 3.0 across the whole set of stable branches.
> Regarding the versions of OpenSSL supported:
> - REL_13_STABLE requires 1.0.1 since 7b283d0e1.
> - REL_12_STABLE and REL_11_STABLE require 0.9.8.
> 
> For 0.9.8, OPENSSL_API_COMPAT needs to be set at 0x00908000L (see
> upstream's CHANGES.md).  So I don't see a reason not to do as
> suggested by Andres?

The message linked to above also says:

 > I'm not sure.  I don't have a good sense of what OpenSSL versions we
 > claim to support in branches older than PG13.  We made a conscious
 > decision for 1.0.1 in PG13, but I seem to recall that that discussion
 > also revealed that the version assumptions before that were quite
 > inconsistent.  Code in PG12 and before makes references to OpenSSL as
 > old as 0.9.6.  But OpenSSL 3.0.0 will reject a compat level older than
 > 0.9.8.

On Thu, Jun 22, 2023 at 08:08:54PM +0200, Peter Eisentraut wrote:
> The message linked to above also says:
>
>> I'm not sure.  I don't have a good sense of what OpenSSL versions we
>> claim to support in branches older than PG13.  We made a conscious
>> decision for 1.0.1 in PG13, but I seem to recall that that discussion
>> also revealed that the version assumptions before that were quite
>> inconsistent.  Code in PG12 and before makes references to OpenSSL as
>> old as 0.9.6.  But OpenSSL 3.0.0 will reject a compat level older than
>> 0.9.8.

Well, I highly doubt that anybody has tried to compile Postgres 12
with OpenSSL 0.9.7 for a few years.  If they attempt to do so, the
compilation fails:
<command-line>: note: this is the location of the previous definition
In file included from ../../src/include/common/scram-common.h:16,
                 from scram-common.c:23:
../../src/include/common/sha2.h:73:9: error: unknown type name ‘SHA256_CTX’
   73 | typedef SHA256_CTX pg_sha256_ctx;

One reason is that SHA256_CTX is defined in OpenSSL 0.9.8
crypto/sha/sha.h, but this exists only in fips-1.0 in OpenSSL 0.9.7,
while we rely on SHA256_CTX in src/common/ since SCRAM exists.

Also, note that the documentation claims that the minimum version of
OpenSSL supported is 0.9.8, which is something that commit 9b7cd59 has
done, impacting Postgres 10~.  So your argument looks incorrect to me?

Honestly, I see no reason to not move on with this and remove these
deprecation warnings as proposed by the last patches sent.  (I have
run builds with 0.9.8, FWIW.)
--
Michael

On 23.06.23 00:22, Michael Paquier wrote:
> Also, note that the documentation claims that the minimum version of
> OpenSSL supported is 0.9.8, which is something that commit 9b7cd59 has
> done, impacting Postgres 10~.  So your argument looks incorrect to me?

Considering that, yes.

On Fri, Jun 23, 2023 at 10:41:06PM +0200, Peter Eisentraut wrote:
> Considering that, yes.

Thanks, applied to 11~13, then.
--
Michael