Обсуждение: Removal of support for OpenSSL 0.9.8 and 1.0.0

Hi all,

So, I have been looking at what we could clean up by removing support
for OpenSSL 0.9.8 and 1.0.0.  Here are my notes:
1) SSL_get_current_compression exists before 0.9.8, and we don't
actually make use of its configure check.  So I think that it could
just be removed, as per patch 0001.
2) SSL_clear_options exists since 0.9.8, so we should not even need the
configure checks.  Still, it is defined as a macro from 0.9.8 to
1.0.2, and then it has switched to a function in 1.1.0, so we fail to
detect it on past versions of OpenSSL (LibreSSL has forked at the
point of 1.0.1g, so it uses only a macro).  There is an extra take
though.  Daniel has mentioned that here:
https://www.postgresql.org/message-id/98F7F99E-1129-41D8-B86B-FE3B1E286881@yesql.se
Note also that a364dfa has also added a tweak in fe-secure-openssl.c
for cases where we don't have SSL_clear_options().  This refers to
NetBSD 5.1.  Peter, do you recall which version of LibreSSL was
involved here?  From a lookup at the code of LibreSSL, the function
has always been around as a macro.  Anyway, 0002 is more subject to
discussions regarding this last point.

Then comes the actual changes across the major versions:
1) SSL_CTX_set_options, which has been added in 0.9.8f, so this could
get removed in be-secure-openssl.c.
2) These functions are new as of 1.0.2:
X509_get_signature_nid
3) These functions are new as of 1.1.0:
- SSL_CTX_set_min_proto_version, SSL_CTX_set_max_proto_version (still
for the fallback functions we have it sounds better to keep the extra
checks on the TLSvXX definitions.)
- BIO_meth_new
- BIO_get_data
- OPENSSL_init_ssl
- ASN1_STRING_get0_data
From the point of view of the code, the cleanup is not actually that
amazing I am afraid, a jump directly to 1.1.0 would remove much more
because the breakages were wider when we integrated it.  Anyway, those
cleanups are part of 0003.  I thought that this would have resulted in
more cleanup :(

I think that 0001 is a fix we need to do, 0002 is debatable still
LibreSSL should support it and we fail to detect SSL_clear_options
properly, and 0003 does not really much additional value.  Or we put
into the balance for 0003 the argument that we can use TLSv1.2 for all
configurations, which is safer but we have the configuration to
enforce it.

Thoughts?
--
Michael

> On 5 Dec 2019, at 09:32, Michael Paquier <michael@paquier.xyz> wrote:

> From the point of view of the code, the cleanup is not actually that
> amazing I am afraid, a jump directly to 1.1.0 would remove much more
> because the breakages were wider when we integrated it.  Anyway, those
> cleanups are part of 0003.  I thought that this would have resulted in
> more cleanup :(

While expected, it's still disappointing.  Until we can drop 1.0.2 there isn't
too much to gain, and that will likely be reasonably far into the future given
that it's the final version that can run the FIPS module.

> I think that 0001 is a fix we need to do

+1

> 0002 is debatable still LibreSSL should support it and we fail to detect
> SSL_clear_options properly,


LibreSSL has SSL_clear_options in all versions, as does OpenSSL even at the
level of support we have as of now.  The only issue with 0002 is support for
older NetBSD releases, as is documented in the comment and commit message.

NetBSD 5.x shipped a custom OpenSSL identified as 0.9.9, which is a version of
their own invention.  NetBSD 6.0 (which shipped in October 2012) ships 1.0.1u,
which has SSL_clear_options as well as SSL_OP_NO_COMPRESSION.  So, this patch
is not really debateable if we are dropping support for 0.9.8 and 1.0.0.

opossum is the only animal in the buildfarm on NetBSD 5, and it has been silent
for close to a year now (coypu is on NetBSD 8).  Requiring opossum to build
without OpenSSL (if/when) it comes back seems a reasonable ask.  NetBSD 5.x has
been EOL for quite some time too.

+1 on applying this instead of trying to fix the autoconf check.

> 0003 does not really much additional value.  Or we put
> into the balance for 0003 the argument that we can use TLSv1.2 for all
> configurations, which is safer but we have the configuration to
> enforce it.

I think the TLSv1.2 argument is the most compelling one, the changes are a wash
in terms of code maintainability.  Raising the minimum supported version
doesn't really have any downsides though, and should be quite uncontroversial
(and as noted in the other thread, prariedog and gaur are ready for the change
so buildfarm breakage should be limited to an animal that doesnt build
anymore). Overall, +1.

cheers ./daniel

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 5 Dec 2019, at 09:32, Michael Paquier <michael@paquier.xyz> wrote:
>> From the point of view of the code, the cleanup is not actually that
>> amazing I am afraid, a jump directly to 1.1.0 would remove much more
>> because the breakages were wider when we integrated it.  Anyway, those
>> cleanups are part of 0003.  I thought that this would have resulted in
>> more cleanup :(

> While expected, it's still disappointing.  Until we can drop 1.0.2 there isn't
> too much to gain, and that will likely be reasonably far into the future given
> that it's the final version that can run the FIPS module.

Yeah; also as mentioned in the other thread, 1.0.1 is still in use
in RHEL 6, so it's hard to consider dropping that for at least another
year.  I concur with the conclusion that we can stop worrying about
NetBSD 5, though.

I see nothing to object to in this patch set.

            regards, tom lane

On Thu, Dec 05, 2019 at 10:38:55AM -0500, Tom Lane wrote:
> Yeah; also as mentioned in the other thread, 1.0.1 is still in use
> in RHEL 6, so it's hard to consider dropping that for at least another
> year.  I concur with the conclusion that we can stop worrying about
> NetBSD 5, though.

Thanks.  Another argument in favor of dropping 1.0.0 and 0.9.8 is that
it is a pain to check an OpenSSL patch across that many versions,
multiplied by the number of Postgres branches in need of patching :)

> I see nothing to object to in this patch set.

I have applied 0001 on HEAD for now as that's a simple cleanup (I
would not backpatch that though).  For 0002, I would prefer be sure
that we reach the right conclusion.  My take is to:
1) Apply 0002 only on HEAD to remove the check for clear_options.
2) Apply something like Daniel's patch in [1] for REL_12_STABLE and
REL_11_STABLE as we care about this routine since e3bdb2d to not mess
up with past versions of NetBSD which worked previously on our
released branches.  (The patch looks fine at quick glance and I
haven't tested it yet)

[1]: https://www.postgresql.org/message-id/3C636E88-44C7-40C6-ABA3-1B236E0A74DE@yesql.se
--
Michael

On Fri, Dec 06, 2019 at 10:33:23AM +0900, Michael Paquier wrote:
> Thanks.  Another argument in favor of dropping 1.0.0 and 0.9.8 is that
> it is a pain to check an OpenSSL patch across that many versions,
> multiplied by the number of Postgres branches in need of patching :)

I have done nothing for 0003 yet.  Let's wait a bit and see if others
have more arguments in favor of it or not.

> I have applied 0001 on HEAD for now as that's a simple cleanup (I
> would not backpatch that though).  For 0002, I would prefer be sure
> that we reach the right conclusion.  My take is to:
> 1) Apply 0002 only on HEAD to remove the check for clear_options.
> 2) Apply something like Daniel's patch in [1] for REL_12_STABLE and
> REL_11_STABLE as we care about this routine since e3bdb2d to not mess
> up with past versions of NetBSD which worked previously on our
> released branches.  (The patch looks fine at quick glance and I
> haven't tested it yet)

0002 is now applied, and did things as described in the above
paragraph.
--
Michael

> On 6 Dec 2019, at 02:33, Michael Paquier <michael@paquier.xyz> wrote:

> Another argument in favor of dropping 1.0.0 and 0.9.8 is that
> it is a pain to check an OpenSSL patch across that many versions,
> multiplied by the number of Postgres branches in need of patching :)

That is indeed a very good argument.

cheers ./daniel

On Fri, Dec 06, 2019 at 09:21:55AM +0100, Daniel Gustafsson wrote:
> On 6 Dec 2019, at 02:33, Michael Paquier <michael@paquier.xyz> wrote:
>> Another argument in favor of dropping 1.0.0 and 0.9.8 is that
>> it is a pain to check an OpenSSL patch across that many versions,
>> multiplied by the number of Postgres branches in need of patching :)
>
> That is indeed a very good argument.

Sorry for letting this thread down for a couple of weeks, but I was
hesitating to apply the last patch of the series as the cleanup of the
code related to OpenSSL 0.9.8 and 1.0.0 is not that much.  An extra
argument in favor of the removal is that this can allow more shaving
of past Python versions, as proposed by Peter here:
https://www.postgresql.org/message-id/98b69261-298c-13d2-f34d-836fd9c29b21@2ndquadrant.com

So, let's do it.  I don't think that I'll be able to do anything this
week about it, but that should be fine by the end of next week.  Are
there any objections or comments?

For now, please note that I have added an entry in the CF app:
https://commitfest.postgresql.org/26/2413/
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> Sorry for letting this thread down for a couple of weeks, but I was
> hesitating to apply the last patch of the series as the cleanup of the
> code related to OpenSSL 0.9.8 and 1.0.0 is not that much.  An extra
> argument in favor of the removal is that this can allow more shaving
> of past Python versions, as proposed by Peter here:
> https://www.postgresql.org/message-id/98b69261-298c-13d2-f34d-836fd9c29b21@2ndquadrant.com

> So, let's do it.

FWIW, I'm not sure I see why there's a connection between moving up
the minimum Python version and minimum OpenSSL version.  Nobody is
installing bleeding-edge Postgres on RHEL5, not even me ;-), so I
don't especially buy Peter's line of reasoning.

I'm perfectly okay with doing both things in HEAD, I just don't
see that doing one is an argument for or against doing the other.

            regards, tom lane

Michael Paquier <michael@paquier.xyz> writes:
> For now, please note that I have added an entry in the CF app:
> https://commitfest.postgresql.org/26/2413/

BTW, the referenced patch only removes the configure check for
SSL_get_current_compression, which is fine, but is that even
moving any goalposts?  The proposed commit message says the
function exists down to 0.9.8, which is already our minimum.
There's nothing to debate here if that statement is accurate.

            regards, tom lane

On Thu, Jan 02, 2020 at 09:30:42AM -0500, Tom Lane wrote:
> BTW, the referenced patch only removes the configure check for
> SSL_get_current_compression, which is fine, but is that even
> moving any goalposts?  The proposed commit message says the
> function exists down to 0.9.8, which is already our minimum.
> There's nothing to debate here if that statement is accurate.

Oops, sorry for the confusion.  There are three patches in the set.
0001 has been already applied as of 28f4bba, and 0002 as of 7d0bcb0
(backpatched with a different fix from Daniel to allow the build to
still work).  The actual patch I am proposing to finish merging is
0003 as posted here, which is the remaining piece:
https://www.postgresql.org/message-id/20191205083252.GE5064@paquier.xyz
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Jan 02, 2020 at 09:30:42AM -0500, Tom Lane wrote:
>> BTW, the referenced patch only removes the configure check for
>> SSL_get_current_compression

> The actual patch I am proposing to finish merging is
> 0003 as posted here, which is the remaining piece:
> https://www.postgresql.org/message-id/20191205083252.GE5064@paquier.xyz

Ah.  The CF app doesn't understand that (and hence the cfbot ditto),
so you might want to repost just the currently-proposed patch to get
the cfbot to try it.

            regards, tom lane

On Thu, Jan 02, 2020 at 11:45:37PM -0500, Tom Lane wrote:
> Ah.  The CF app doesn't understand that (and hence the cfbot ditto),
> so you might want to repost just the currently-proposed patch to get
> the cfbot to try it.

Yes, let's do that.  Here you go with a v2.  While on it, I have
noticed in the docs a mention to OpenSSL 1.0.0 regarding our
sslcompression parameter in libpq, so a paragraph can be removed.
--
Michael

> On 3 Jan 2020, at 07:49, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Thu, Jan 02, 2020 at 11:45:37PM -0500, Tom Lane wrote:
>> Ah.  The CF app doesn't understand that (and hence the cfbot ditto),
>> so you might want to repost just the currently-proposed patch to get
>> the cfbot to try it.
> 
> Yes, let's do that.  Here you go with a v2.  While on it, I have
> noticed in the docs a mention to OpenSSL 1.0.0 regarding our
> sslcompression parameter in libpq, so a paragraph can be removed.

LGTM, switching to ready for committer.

cheers ./daniel

On Thu, Jan 02, 2020 at 09:22:47AM -0500, Tom Lane wrote:
> FWIW, I'm not sure I see why there's a connection between moving up
> the minimum Python version and minimum OpenSSL version.  Nobody is
> installing bleeding-edge Postgres on RHEL5, not even me ;-), so I
> don't especially buy Peter's line of reasoning.

It seems to me that the line of reasoning was to consider RHEL5 in the
garbage for all our dependencies, in a consistent way.

> I'm perfectly okay with doing both things in HEAD, I just don't
> see that doing one is an argument for or against doing the other.

Yes, right.  That would be the case if we had direct dependencies
between both, but that has never been the case AFAIK.
--
Michael

On Fri, Jan 03, 2020 at 10:57:54PM +0100, Daniel Gustafsson wrote:
> LGTM, switching to ready for committer.

Thanks Daniel.  I have looked at that stuff again, and committed the
patch.
--
Michael