Thread: 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
Attachment
> 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
Attachment
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
Attachment
> 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
Attachment
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
Attachment
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
Attachment
> 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
Attachment
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