Thread: OpenSSL 3.0.0 compatibility
Since OpenSSL is now releasing 3.0.0-alpha versions, I took a look at using it with postgres to see what awaits us. As it is now shipping in releases (with GA planned for Q4), users will probably soon start to test against it so I wanted to be prepared. Regarding the deprecations, we can either set preprocessor directives or use compiler flags to silence the warning and do nothing (for now), or we could update to the new API. We probably want to different things for master vs back-branches, but as an illustration of what the latter could look like I've implemented this in 0001. SSL_CTX_load_verify_locations and X509_STORE_load_locations are deprecated and replaced by individual calls to load files and directories. These are quite straightforward, and are implemented like how we handle the TLS protocol API. DH_check has been discouraged to use for quite some time, and is now marked deprecated without a 1:1 replacement. The OpenSSL docs recommends using the EVP API, which is also done in 0001. For now I just stuck it in with version gated ifdefs, something cleaner than that can clearly be done. 0001 is clearly not proposed for review/commit yet, it's included in case someone else is interested as well. OpenSSL also deprecates DES keys in 3.0.0, which cause our password callback tests to fail with the cryptic error "fetch failed", as the test suite keys are encrypted with DES. 0002 fixes this by changing to AES256 (randomly chosen among the ciphers supported in 1.0.1+ and likely to be around), and could be applied already today as there is nothing 3.0.0 specific about it. On top of DES keys there are also a lot of deprecations or low-level functions which breaks pgcrypto in quite a few ways. I haven't tackled these yet, but it looks like we have to do the EVP rewrite there. cheers ./daniel
Attachment
On Fri, 2020-05-29 at 00:16 +0200, Daniel Gustafsson wrote: > Since OpenSSL is now releasing 3.0.0-alpha versions, I took a look at using it > with postgres to see what awaits us. As it is now shipping in releases (with > GA planned for Q4), users will probably soon start to test against it so I > wanted to be prepared. > > Regarding the deprecations, we can either set preprocessor directives or use > compiler flags to silence the warning and do nothing (for now), or we could > update to the new API. We probably want to different things for master vs > back-branches, but as an illustration of what the latter could look like I've > implemented this in 0001. An important question will be: if we convert to functions that are not deprecated, what is the earliest OpenSSL version we can support? Yours, Laurenz Albe
> On 29 May 2020, at 08:06, Laurenz Albe <laurenz.albe@cybertec.at> wrote: >> Regarding the deprecations, we can either set preprocessor directives or use >> compiler flags to silence the warning and do nothing (for now), or we could >> update to the new API. We probably want to different things for master vs >> back-branches, but as an illustration of what the latter could look like I've >> implemented this in 0001. > > An important question will be: if we convert to functions that are not deprecated, > what is the earliest OpenSSL version we can support? The replacement functions for _locations calls are introduced together with the deprecation in 3.0.0, so there is no overlap. For pgcrypto, that remains to be seen once it attempted, but ideally all the way down to 1.0.1. cheers ./daniel
On 2020-05-29 00:16, Daniel Gustafsson wrote: > Regarding the deprecations, we can either set preprocessor directives or use > compiler flags to silence the warning and do nothing (for now), or we could > update to the new API. We probably want to different things for master vs > back-branches, but as an illustration of what the latter could look like I've > implemented this in 0001. I think we should set OPENSSL_API_COMPAT=10001, and move that along with whatever our oldest supported release is going forward. That declares our intention, it will silence the deprecation warnings, and IIUC, if the deprecated stuff actually gets removed, you get a clean compiler error that your API level is too low. > OpenSSL also deprecates DES keys in 3.0.0, which cause our password callback > tests to fail with the cryptic error "fetch failed", as the test suite keys are > encrypted with DES. 0002 fixes this by changing to AES256 (randomly chosen > among the ciphers supported in 1.0.1+ and likely to be around), and could be > applied already today as there is nothing 3.0.0 specific about it. It appears you can load a "legacy provider" to support these keys. What if someone made a key using 0.9.* with an older PostgreSQL version and keeps using the same key? I'm wondering about the implications in practice here. Obviously moving off legacy crypto is good in general. There is also the question of what to do with the test suites in the back branches. Maybe we will want some user-exposed option about which providers to load, since that also affects whether the FIPS provider gets loaded. What's the plan there? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On 29 May 2020, at 13:34, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > On 2020-05-29 00:16, Daniel Gustafsson wrote: >> Regarding the deprecations, we can either set preprocessor directives or use >> compiler flags to silence the warning and do nothing (for now), or we could >> update to the new API. We probably want to different things for master vs >> back-branches, but as an illustration of what the latter could look like I've >> implemented this in 0001. > > I think we should set OPENSSL_API_COMPAT=10001, and move that along with whatever our oldest supported release is goingforward. That declares our intention, it will silence the deprecation warnings, and IIUC, if the deprecated stuff actuallygets removed, you get a clean compiler error that your API level is too low. I think I know what you mean but just to clarify: I master, back-branches or all of the above? Considering how little effort it is to not use the deprecated API's I'm not entirely convinced, but I don't have too strong opinions there. >> OpenSSL also deprecates DES keys in 3.0.0, which cause our password callback >> tests to fail with the cryptic error "fetch failed", as the test suite keys are >> encrypted with DES. 0002 fixes this by changing to AES256 (randomly chosen >> among the ciphers supported in 1.0.1+ and likely to be around), and could be >> applied already today as there is nothing 3.0.0 specific about it. > > It appears you can load a "legacy provider" to support these keys. What if someone made a key using 0.9.* with an olderPostgreSQL version and keeps using the same key? I'm wondering about the implications in practice here. Obviouslymoving off legacy crypto is good in general. If they do, then that key will stop working with any OpenSSL 3 enabled software unless the legacy provider has been loaded. My understanding is that users can load these in openssl.conf, so maybe it's mostly a documentation patch for us? Iff key loading fails with an errormessage that indicates that the algorithm couldn't be fetched (ie fetch failed), we could add an errhint on algorithm use. Currently it's easy to believe that it's the key file that couldn't be fetched, as the error message from OpenSSL is quite cryptic and expects the user to understand OpenSSL terminology. This could happen already in pre-3 versions, so maybe it's worthwhile to do? > There is also the question of what to do with the test suites in the back branches. If we don't want to change the testdata in the backbranches, we could add a SKIP section for the password key tests iff OpenSSL is 3.0.0+? > Maybe we will want some user-exposed option about which providers to load, since that also affects whether the FIPS providergets loaded. What's the plan there? This again boils down to if we want to load providers, or if we want to punt that to openssl.conf completely. Since we will support olders versions for a long time still, maybe punting will render the cleanest code? AFAICT, if care is taken with openssl.conf one could already load providers such that postgres will operate in FIPS mode due to nothing non-FIPS being available. For libpq I'm not sure if there is anything to do, as we don't mandate any cipher use (SSL_CTX_set_cipher_list will simply fail IIUC). For pgcrypto however it's another story, but there we'd need to rewrite it to not use low-level APIs first since the use of those aren't FIPS compliant. Once done, we could have an option for FIPS mode and pass the "fips=yes" property when loading ciphers to make sure the right version is loaded if multiple ones are available. cheers ./daniel
On 2020-05-29 14:45, Daniel Gustafsson wrote: >> I think we should set OPENSSL_API_COMPAT=10001, and move that along with whatever our oldest supported release is goingforward. That declares our intention, it will silence the deprecation warnings, and IIUC, if the deprecated stuff actuallygets removed, you get a clean compiler error that your API level is too low. > > I think I know what you mean but just to clarify: I master, back-branches or > all of the above? 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. My proposal would be to introduce OPENSSL_API_COMPAT=10001 into master after the 13/14 branching, along with any other changes to make it compile cleanly against OpenSSL 3.0.0. Once that has survived some scrutiny from the buildfarm and also from folks building against LibreSSL etc., it should probably be backpatched into PG13. In the immediate future, I wouldn't bother about the older branches (<=PG12) at all. As long as they still compile, users can just disable deprecation warnings, and we may add some patches to that effect at some point, but it's not like OpenSSL 3.0.0 will be adopted into production builds any time soon. > Considering how little effort it is to not use the deprecated API's I'm not > entirely convinced, but I don't have too strong opinions there. Well, in the case like X509_STORE_load_locations(), the solution is in either case to write a wrapper. It doesn't matter if we write the wrapper or OpenSSL writes the wrapper. Only OpenSSL has already written the wrapper and has created a well-defined way to declare that you want to use the wrapper, so I'd just take that. In any case, using OPENSSL_API_COMPAT is also good just for our own documentation, so we can keep track of what version we claim to support in different branches. > If they do, then that key will stop working with any OpenSSL 3 enabled software > unless the legacy provider has been loaded. My understanding is that users can > load these in openssl.conf, so maybe it's mostly a documentation patch for us? Yes, it looks like that should work, so no additional work required from us. >> There is also the question of what to do with the test suites in the back branches. > > If we don't want to change the testdata in the backbranches, we could add a > SKIP section for the password key tests iff OpenSSL is 3.0.0+? I suggest to update the test data in PG13+, since we require OpenSSL 1.0.1 there. For the older branches, I would look into changing the test driver setup so that it loads a custom openssl.cnf that loads the legacy providers. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 5/28/20 6:16 PM, Daniel Gustafsson wrote: > > OpenSSL also deprecates DES keys in 3.0.0, which cause our password callback > tests to fail with the cryptic error "fetch failed", as the test suite keys are > encrypted with DES. 0002 fixes this by changing to AES256 (randomly chosen > among the ciphers supported in 1.0.1+ and likely to be around), and could be > applied already today as there is nothing 3.0.0 specific about it. > +1 for applying this forthwith. The key in my recent commit 896fcdb230 is encrypted with AES256. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, May 30, 2020 at 11:29:11AM +0200, Peter Eisentraut wrote: > 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. 593d4e4 claims that we only support OpenSSL >= 0.9.8, meaning that down to PG 10 we have this requirement, and that PG 9.6 and 9.5 should be able to work with OpenSSL 0.9.7 and 0.9.6, but little effort has been put in testing these. > My proposal would be to introduce OPENSSL_API_COMPAT=10001 into master after > the 13/14 branching, along with any other changes to make it compile cleanly > against OpenSSL 3.0.0. Once that has survived some scrutiny from the > buildfarm and also from folks building against LibreSSL etc., it should > probably be backpatched into PG13. In the immediate future, I wouldn't > bother about the older branches (<=PG12) at all. As long as they still > compile, users can just disable deprecation warnings, and we may add some > patches to that effect at some point, but it's not like OpenSSL 3.0.0 will > be adopted into production builds any time soon. Please note that I actually may have to bother about 12 and OpenSSL 3.0.0 as 1.0.2 deprecation is visibly accelerating a move to newer versions at least in my close neighborhood. We are not there yet of course, so doing this work with 14 and 13 sounds fine to me for now. -- Michael
Attachment
On 2020-05-30 14:34, Andrew Dunstan wrote: > > On 5/28/20 6:16 PM, Daniel Gustafsson wrote: >> >> OpenSSL also deprecates DES keys in 3.0.0, which cause our password callback >> tests to fail with the cryptic error "fetch failed", as the test suite keys are >> encrypted with DES. 0002 fixes this by changing to AES256 (randomly chosen >> among the ciphers supported in 1.0.1+ and likely to be around), and could be >> applied already today as there is nothing 3.0.0 specific about it. >> > > +1 for applying this forthwith. The key in my recent commit 896fcdb230 > is encrypted with AES256. I don't see anything in that commit about how to regenerate those files, such as a makefile rule. Is that missing? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020-05-31 04:52, Michael Paquier wrote: > 593d4e4 claims that we only support OpenSSL >= 0.9.8, meaning that > down to PG 10 we have this requirement, and that PG 9.6 and 9.5 should > be able to work with OpenSSL 0.9.7 and 0.9.6, but little effort has > been put in testing these. Then we can stick a OPENSSL_API_COMPAT=908 into at least PG10, 11, and 12, and probably also into PG9.5 and 9.6 without harm. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On 30 May 2020, at 11:29, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > On 2020-05-29 14:45, Daniel Gustafsson wrote: >>> I think we should set OPENSSL_API_COMPAT=10001, and move that along with whatever our oldest supported release is goingforward. That declares our intention, it will silence the deprecation warnings, and IIUC, if the deprecated stuff actuallygets removed, you get a clean compiler error that your API level is too low. >> I think I know what you mean but just to clarify: I master, back-branches or >> all of the above? > > I'm not sure. I don't have a good sense of what OpenSSL versions we claim to support in branches older than PG13. Wemade a conscious decision for 1.0.1 in PG13, but I seem to recall that that discussion also revealed that the version assumptionsbefore that were quite inconsistent. Code in PG12 and before makes references to OpenSSL as old as 0.9.6. ButOpenSSL 3.0.0 will reject a compat level older than 0.9.8. > > My proposal would be to introduce OPENSSL_API_COMPAT=10001 into master after the 13/14 branching, along with any otherchanges to make it compile cleanly against OpenSSL 3.0.0. Once that has survived some scrutiny from the buildfarm andalso from folks building against LibreSSL etc., it should probably be backpatched into PG13. In the immediate future,I wouldn't bother about the older branches (<=PG12) at all. As long as they still compile, users can just disabledeprecation warnings, and we may add some patches to that effect at some point, but it's not like OpenSSL 3.0.0 willbe adopted into production builds any time soon. > >> Considering how little effort it is to not use the deprecated API's I'm not >> entirely convinced, but I don't have too strong opinions there. > > Well, in the case like X509_STORE_load_locations(), the solution is in either case to write a wrapper. It doesn't matterif we write the wrapper or OpenSSL writes the wrapper. Only OpenSSL has already written the wrapper and has createda well-defined way to declare that you want to use the wrapper, so I'd just take that. I'll buy that argument. > In any case, using OPENSSL_API_COMPAT is also good just for our own documentation, so we can keep track of what versionwe claim to support in different branches. Good point. >>> There is also the question of what to do with the test suites in the back branches. >> If we don't want to change the testdata in the backbranches, we could add a >> SKIP section for the password key tests iff OpenSSL is 3.0.0+? > > I suggest to update the test data in PG13+, since we require OpenSSL 1.0.1 there. For the older branches, I would lookinto changing the test driver setup so that it loads a custom openssl.cnf that loads the legacy providers. Ok, I'll roll a patch along these lines for master for ~ the 13/14 branch time and then we'll see how we deal with PG13 once the dust has settled not only on our side but for OpenSSL. cheers ./daniel
On 6/1/20 4:33 AM, Peter Eisentraut wrote:
> On 2020-05-30 14:34, Andrew Dunstan wrote:
>>
>> On 5/28/20 6:16 PM, Daniel Gustafsson wrote:
>>>
>>> OpenSSL also deprecates DES keys in 3.0.0, which cause our password
>>> callback
>>> tests to fail with the cryptic error "fetch failed", as the test
>>> suite keys are
>>> encrypted with DES. 0002 fixes this by changing to AES256 (randomly
>>> chosen
>>> among the ciphers supported in 1.0.1+ and likely to be around), and
>>> could be
>>> applied already today as there is nothing 3.0.0 specific about it.
>>>
>>
>> +1 for applying this forthwith. The key in my recent commit 896fcdb230
>> is encrypted with AES256.
>
> I don't see anything in that commit about how to regenerate those
> files, such as a makefile rule. Is that missing?
You missed these comments in the test file:
# self-signed cert was generated like this:
# system('openssl req -new -x509 -days 10000 -nodes -out server.crt
-keyout server.ckey -subj "/CN=localhost"');
# add the cleartext passphrase to the key, remove the unprotected key
# system("openssl rsa -aes256 -in server.ckey -out server.key -passout
pass:$clearpass");
# unlink "server.ckey";
If you want I can add a rule for it to the Makefile, although who knows
what commands will actually apply when the certificate runs out?
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On 1 Jun 2020, at 13:58, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote: > If you want I can add a rule for it to the Makefile, although who knows > what commands will actually apply when the certificate runs out? Being able to easily regenerate the testdata, regardless of expiration status, has proven very helpful for me when implementing support for new TLS backends. +1 for adding it to the Makefile. cheers ./daniel
On 6/1/20 8:03 AM, Daniel Gustafsson wrote: >> On 1 Jun 2020, at 13:58, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote: >> If you want I can add a rule for it to the Makefile, although who knows >> what commands will actually apply when the certificate runs out? > Being able to easily regenerate the testdata, regardless of expiration status, > has proven very helpful for me when implementing support for new TLS backends. > +1 for adding it to the Makefile. > OK, here's a patch. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 6/1/20 8:03 AM, Daniel Gustafsson wrote:
>> +1 for adding it to the Makefile.
> OK, here's a patch.
Likewise +1 for having it in the makefile. But now you have two copies,
the other being in comments in the test script. The latter should go
away, as we surely won't remember to maintain it. (You could replace it
with a pointer to the makefile rules if you want.)
regards, tom lane
On 2020-06-01 15:23, Andrew Dunstan wrote: > > On 6/1/20 8:03 AM, Daniel Gustafsson wrote: >>> On 1 Jun 2020, at 13:58, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote: >>> If you want I can add a rule for it to the Makefile, although who knows >>> what commands will actually apply when the certificate runs out? >> Being able to easily regenerate the testdata, regardless of expiration status, >> has proven very helpful for me when implementing support for new TLS backends. >> +1 for adding it to the Makefile. >> > OK, here's a patch. In src/test/ssl/ we have targets sslfiles and sslfiles-clean, and here we have ssl-files and ssl-files-clean. Let's keep that consistent. Or, why not actually use the generated files from src/test/ssl/ instead of making another set? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 6/2/20 4:57 AM, Peter Eisentraut wrote: > On 2020-06-01 15:23, Andrew Dunstan wrote: >> >> On 6/1/20 8:03 AM, Daniel Gustafsson wrote: >>>> On 1 Jun 2020, at 13:58, Andrew Dunstan >>>> <andrew.dunstan@2ndquadrant.com> wrote: >>>> If you want I can add a rule for it to the Makefile, although who >>>> knows >>>> what commands will actually apply when the certificate runs out? >>> Being able to easily regenerate the testdata, regardless of >>> expiration status, >>> has proven very helpful for me when implementing support for new TLS >>> backends. >>> +1 for adding it to the Makefile. >>> >> OK, here's a patch. > > In src/test/ssl/ we have targets sslfiles and sslfiles-clean, and here > we have ssl-files and ssl-files-clean. Let's keep that consistent. > > Or, why not actually use the generated files from src/test/ssl/ > instead of making another set? Honestly, I think we've spent plenty of time on this already. I don't see a problem with each module having its own certificate(s) - that makes them more self-contained - nor any great need to have the targets named the same. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Jun 02, 2020 at 02:45:11PM -0400, Andrew Dunstan wrote: > Honestly, I think we've spent plenty of time on this already. I don't > see a problem with each module having its own certificate(s) - that > makes them more self-contained - nor any great need to have the targets > named the same. Yeah, I don't see much point in combining both of them as those modules have different assumptions behind the files built. Now I agree with Peter's point to use the same Makefile rule names in both files so as it gets easier to grep for all instances. So, src/test/ssl/ being the oldest one, ssl_passphrase_callback should just do s/ssl-files/sslfiles/. -- Michael
Attachment
On Mon, Jun 01, 2020 at 10:44:17AM +0200, Peter Eisentraut wrote: > Then we can stick a OPENSSL_API_COMPAT=908 into at least PG10, 11, and 12, > and probably also into PG9.5 and 9.6 without harm. FWIW, I am fine that for PG >= 10, and I don't think that it is worth bothering with PG <= 9.6. -- Michael
Attachment
On 2020-05-30 11:29, Peter Eisentraut wrote: > I suggest to update the test data in PG13+, since we require OpenSSL > 1.0.1 there. For the older branches, I would look into changing the > test driver setup so that it loads a custom openssl.cnf that loads the > legacy providers. I have pushed your 0002 patch (the one that updates the test data) to master. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020-05-30 11:29, Peter Eisentraut wrote: > My proposal would be to introduce OPENSSL_API_COMPAT=10001 into master > after the 13/14 branching, along with any other changes to make it > compile cleanly against OpenSSL 3.0.0. Once that has survived some > scrutiny from the buildfarm and also from folks building against > LibreSSL etc., it should probably be backpatched into PG13. In the > immediate future, I wouldn't bother about the older branches (<=PG12) at > all. As long as they still compile, users can just disable deprecation > warnings, and we may add some patches to that effect at some point, but > it's not like OpenSSL 3.0.0 will be adopted into production builds any > time soon. Trying to move this along, where would be a good place to define OPENSSL_API_COMPAT? The only place that's shared between frontend and backend code is c.h. The attached patch does it that way. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> Trying to move this along, where would be a good place to define
> OPENSSL_API_COMPAT? The only place that's shared between frontend and
> backend code is c.h. The attached patch does it that way.
pg_config_manual.h, perhaps?
regards, tom lane
> On 7 Jul 2020, at 19:53, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >> Trying to move this along, Thanks, this has stalled a bit on my TODO. >> where would be a good place to define >> OPENSSL_API_COMPAT? The only place that's shared between frontend and >> backend code is c.h. The attached patch does it that way. > > pg_config_manual.h, perhaps? I don't have a strong preference. When starting hacking on this I went for the quick and simple option of adding it to CFLAGS in configure.in for the time being since I wasn't sure where to put it. A slightly more complicated problem arise when trying to run the pgcrypto regress tests, and make it run the tests for the now deprecated ciphers, as they require the legacy provider to be loaded via the openssl configuration file. As was mentioned upthread, this requires us to inject our own openssl.cnf in OPENSSL_CONF, load the legacy provider there and then from that file include the system openssl.cnf (or override the system one completely during testing which doesn't seem like a good idea). Hacking this up in a crude PoC I added a REGRESS_ENV option in pgxs.mk which then pgcrypto/Makefile could use to set an OPENSSL_CONF, which in turn ends with a .include=<path> for my system config. This enables pgcrypto to load the now deprecated ciphers, but even as PoC's goes this is awfully brittle and a significant amount of bricks shy. Actually running the tests with the legacy provider loaded yields a fair number of errors like these, and somewhere around there I ran out of time for now as the CF started. - decrypt ----------------------------- - Lets try a longer message. + decrypt +---------------------------------------------------------- + Lets try a longer messag\177\177\177\177\177\177\177\177 Memorizing the "cannot load cipher" errors in an alternative output and documenting how to use old ciphers in pgcrypto together with OpenSSL 3.0.0+ might be the least bad option? Anyone else have any good ideas on how to get this into the testrunner? cheers ./daniel
On Tue, Jul 7, 2020 at 1:46 PM Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > Trying to move this along, where would be a good place to define > OPENSSL_API_COMPAT? The only place that's shared between frontend and > backend code is c.h. The attached patch does it that way. So, if we go this way, does that mean that we're not going to pursue removing dependencies on the deprecated interfaces? I wonder if we really ought to be doing that too, with preprocessor conditionals. Otherwise, aren't we putting ourselves in an uncomfortable situation when the deprecated stuff eventually goes away upstream? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2020-07-08 16:51, Robert Haas wrote: > On Tue, Jul 7, 2020 at 1:46 PM Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> Trying to move this along, where would be a good place to define >> OPENSSL_API_COMPAT? The only place that's shared between frontend and >> backend code is c.h. The attached patch does it that way. > > So, if we go this way, does that mean that we're not going to pursue > removing dependencies on the deprecated interfaces? I wonder if we > really ought to be doing that too, with preprocessor conditionals. > Otherwise, aren't we putting ourselves in an uncomfortable situation > when the deprecated stuff eventually goes away upstream? I don't think there is a rush. The 3.0.0 alphas still support interfaces deprecated in 0.9.8 (released 2005). AFAICT, nothing tagged under this API compatibility scheme has ever been removed. If they started doing so, they would presumably do it step by step at the tail end, which would still give us several steps before it catches up with us. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020-07-07 22:52, Daniel Gustafsson wrote: > Actually running the tests with the legacy provider loaded yields a fair number > of errors like these, and somewhere around there I ran out of time for now as > the CF started. > > - decrypt > ----------------------------- > - Lets try a longer message. > + decrypt > +---------------------------------------------------------- > + Lets try a longer messag\177\177\177\177\177\177\177\177 > > Memorizing the "cannot load cipher" errors in an alternative output and > documenting how to use old ciphers in pgcrypto together with OpenSSL 3.0.0+ > might be the least bad option? Anyone else have any good ideas on how to get > this into the testrunner? I think an alternative test output file would be a legitimate solution in the short and mid term. However, as you mention, and looking at the test output, this might also require a bit of work making the handling of these new various error conditions more robust. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2020-07-07 22:52, Daniel Gustafsson wrote: >>> where would be a good place to define >>> OPENSSL_API_COMPAT? The only place that's shared between frontend and >>> backend code is c.h. The attached patch does it that way. >> pg_config_manual.h, perhaps? > I don't have a strong preference. When starting hacking on this I went for the > quick and simple option of adding it to CFLAGS in configure.in for the time > being since I wasn't sure where to put it. Actually, it would be most formally correct to set it using AC_DEFINE in configure.in, so that configure tests see it. It doesn't look like we currently have any configure tests that would really be affected, but it seems sensible to do it there anyway. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>> where would be a good place to define
>>> OPENSSL_API_COMPAT?
> Actually, it would be most formally correct to set it using AC_DEFINE in
> configure.in, so that configure tests see it.
Yeah, very good point.
regards, tom lane
On 2020-07-13 15:38, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes: >>>> where would be a good place to define >>>> OPENSSL_API_COMPAT? > >> Actually, it would be most formally correct to set it using AC_DEFINE in >> configure.in, so that configure tests see it. > > Yeah, very good point. New patch done that way. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Thu, Jul 16, 2020 at 10:58:58AM +0200, Peter Eisentraut wrote: > if test "$with_openssl" = yes ; then > dnl Order matters! > + AC_DEFINE(OPENSSL_API_COMPAT, [10001], > + [Define to the OpenSSL API version in use. This avoids > deprecation warnings from newer OpenSSL versions.]) > if test "$PORTNAME" != "win32"; then I think that you should additionally mention the version number directly in the description, so as when support for 1.0.1 gets removed it is possible to grep for it, and then adjust the number and the description. -- Michael
Attachment
On 2020-07-16 13:45, Michael Paquier wrote: > On Thu, Jul 16, 2020 at 10:58:58AM +0200, Peter Eisentraut wrote: >> if test "$with_openssl" = yes ; then >> dnl Order matters! >> + AC_DEFINE(OPENSSL_API_COMPAT, [10001], >> + [Define to the OpenSSL API version in use. This avoids >> deprecation warnings from newer OpenSSL versions.]) >> if test "$PORTNAME" != "win32"; then > > I think that you should additionally mention the version number > directly in the description, so as when support for 1.0.1 gets removed > it is possible to grep for it, and then adjust the number and the > description. Good point. I have committed it with that adjustment. Also, I had the format of the version number wrong, so I changed that, too. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Jul 19, 2020 at 04:29:54PM +0200, Peter Eisentraut wrote: > Good point. I have committed it with that adjustment. Also, I had the > format of the version number wrong, so I changed that, too. Thanks. I was not paying much attention to the format, but what you have committed is in line with the upstream docs: https://www.openssl.org/docs/manmaster/man7/OPENSSL_API_COMPAT.html So we are all good. -- Michael
Attachment
On Fri, May 29, 2020 at 12:16:47AM +0200, Daniel Gustafsson wrote: > SSL_CTX_load_verify_locations and X509_STORE_load_locations are deprecated and > replaced by individual calls to load files and directories. These are quite > straightforward, and are implemented like how we handle the TLS protocol API. > DH_check has been discouraged to use for quite some time, and is now marked > deprecated without a 1:1 replacement. The OpenSSL docs recommends using the > EVP API, which is also done in 0001. For now I just stuck it in with version > gated ifdefs, something cleaner than that can clearly be done. 0001 is clearly > not proposed for review/commit yet, it's included in case someone else is > interested as well. Leaving the problems with pgcrypto aside for now, we have also two parts involving DH_check() deprecation and the load-file APIs for the backend. From what I can see, the new APIs to load files are new as of 3.0.0, but these are not marked as deprecated yet, so I am not sure that it is worth having now one extra API compatibility layer just for that now as proposed in cert_openssl.c. Most of the the EVP counterparts, though, are much older than 1.0.1, except for EVP_PKEY_param_check() introduced in 1.1.1 :( By the way, in the previous patch, EVP_PKEY_CTX_new_from_pkey() was getting used but it is new as of 3.0. We could just use EVP_PKEY_CTX_new() which does the same work (see crypto/evp/pmeth_lib.c as we have no library context of engine to pass down), and is much older, but it does not reduce the diffs. Then the actual problem is EVP_PKEY_param_check(), new as of 1.1.1, which looks to be the expected replacement for DH_check(). It seems to me that it would not be a bad thing to switch to the EVP APIs on HEAD to be prepared for the future, but I would switch to EVP_PKEY_CTX_new() instead of EVP_PKEY_CTX_new_from_pkey() and add a configure check to see if EVP_PKEY_param_check() is defined or not. OPENSSL_VERSION_NUMBER cannot be used because of LibreSSL overriding it, and I guess that OPENSSL_VERSION_MAJOR, as used in the original patch, would not work with LibreSSL either. Any thoughts? -- Michael
Attachment
> On 17 Aug 2020, at 06:12, Michael Paquier <michael@paquier.xyz> wrote: > Leaving the problems with pgcrypto aside for now Returning to this subject, I decided to take a stab at fixing pgcrypto since it wasn't working. Since we support ciphers that are now deprecated, we have no other choice than to load the legacy provider. The other problem was that the cipher context padding must be explicitly set, whereas in previous versions relying on the default worked fine. EVP_CIPHER_CTX_set_padding always returns 1 so thats why it isn't checking the returnvalue as the other nearby initialization calls. To avoid problems with the by LibreSSL overloaded OPENSSL_VERSION_NUMBER macro (which too is deprecated in 3.0.0), I used the new macro which is only set in 3.0.0. Not sure if that's considered acceptable or if we should invent our own version macro in autoconf. For the main SSL tests, the incorrect password test has a new errormessage which is added in 0002. With these two all SSL tests pass for me in 1.0.1 through 3.0.0-alpha6 (tested on a mix of Debian and macOS). Thoughts on these? cheers ./daniel
Attachment
On Fri, Sep 18, 2020 at 04:11:13PM +0200, Daniel Gustafsson wrote: > Since we support ciphers that are now deprecated, we have no other choice than > to load the legacy provider. Ah, thanks. I actually tried something similar to that when I had my mind on it by loading the legacy providers, but missed the padding part. Nice find. > The other problem was that the cipher context > padding must be explicitly set, whereas in previous versions relying on the > default worked fine. EVP_CIPHER_CTX_set_padding always returns 1 so thats why > it isn't checking the returnvalue as the other nearby initialization calls. It seems to me that it would be a good idea to still check for the return value of EVP_CIPHER_CTX_set_padding() and just return with a PXE_CIPHER_INIT. By looking at the upstream code, it is true that it always returns true for <= 1.1.1, but that's not the case for 3.0.0. Some code paths of upstream also check after it. Also, what's the impact with disabling the padding for <= 1.1.1? This part of the upstream code is still a bit obscure to me.. > To avoid problems with the by LibreSSL overloaded OPENSSL_VERSION_NUMBER macro > (which too is deprecated in 3.0.0), I used the new macro which is only set in > 3.0.0. Not sure if that's considered acceptable or if we should invent our own > version macro in autoconf. OSSL_PROVIDER_load() is new as of 3.0.0, so using a configure switch similarly as what we do for the other functions should be more consistent and enough, no? > For the main SSL tests, the incorrect password test has a new errormessage > which is added in 0002. Hmm. I am linking to a build of alpha6 here, but I still see the error being reported as a bad decrypt for this test. Interesting. -- Michael
Attachment
> On 19 Sep 2020, at 04:11, Michael Paquier <michael@paquier.xyz> wrote: > On Fri, Sep 18, 2020 at 04:11:13PM +0200, Daniel Gustafsson wrote: >> The other problem was that the cipher context >> padding must be explicitly set, whereas in previous versions relying on the >> default worked fine. EVP_CIPHER_CTX_set_padding always returns 1 so thats why >> it isn't checking the returnvalue as the other nearby initialization calls. > > It seems to me that it would be a good idea to still check for the > return value of EVP_CIPHER_CTX_set_padding() and just return with > a PXE_CIPHER_INIT. By looking at the upstream code, it is true that > it always returns true for <= 1.1.1, but that's not the case for > 3.0.0. Some code paths of upstream also check after it. Thats a good point, it's now provider dependent and can thus fail in case the provider isn't supplying the functionality. We've already loaded a provider where we know the call is supported, but it's of course better to check. Fixed in the attached v2. I was only reading the docs and not the code, and they haven't been updated to reflect this. I'll open a PR to the OpenSSL devs to fix that. > Also, what's the impact with disabling the padding for <= 1.1.1? This > part of the upstream code is still a bit obscure to me.. I'm far from fluent in OpenSSL, but AFAICT it has to do with the new provider API. The default value for padding is unchanged, but it needs to be propaged into the provider to be set in the context where the old code picked it up automatically. The relevant OpenSSL commit (83f68df32f0067ee7b0) which changes the docs to say the function should be called doesn't contain enough information to explain why. >> To avoid problems with the by LibreSSL overloaded OPENSSL_VERSION_NUMBER macro >> (which too is deprecated in 3.0.0), I used the new macro which is only set in >> 3.0.0. Not sure if that's considered acceptable or if we should invent our own >> version macro in autoconf. > > OSSL_PROVIDER_load() is new as of 3.0.0, so using a configure switch > similarly as what we do for the other functions should be more > consistent and enough, no? Good point, fixed in the attached. >> For the main SSL tests, the incorrect password test has a new errormessage >> which is added in 0002. > > Hmm. I am linking to a build of alpha6 here, but I still see the > error being reported as a bad decrypt for this test. Interesting. Turns out it was coming from when I tested against OpenSSL git HEAD, so it may come in alpha7 (or whatever the next version will be). Let's disregard this for now until dust settles, I've dropped the patch from the series. cheers ./daniel
Attachment
On 2020-09-18 16:11, Daniel Gustafsson wrote: > Since we support ciphers that are now deprecated, we have no other choice than > to load the legacy provider. Well, we could just have deprecated ciphers fail, unless the user loads the legacy provider in the OS configuration. There might be an argument that that is more proper. As a more extreme analogy, what if OpenSSL remove a cipher from the legacy provider? Are we then obliged to reimplement it manually for the purpose of pgcrypto? Probably not. The code you wrote to load the necessary providers is small enough that I think it's fine, but it's worth pondering this question briefly. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On 22 Sep 2020, at 11:37, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > On 2020-09-18 16:11, Daniel Gustafsson wrote: >> Since we support ciphers that are now deprecated, we have no other choice than >> to load the legacy provider. > > Well, we could just have deprecated ciphers fail, unless the user loads the legacy provider in the OS configuration. Theremight be an argument that that is more proper. Thats a fair point. The issue I have with that is that we'd impose a system wide loading of the legacy provider, impacting other consumers of libssl as well. > As a more extreme analogy, what if OpenSSL remove a cipher from the legacy provider? Are we then obliged to reimplementit manually for the purpose of pgcrypto? Probably not. I don't think we have made any such guarantees of support, especially since it's in contrib/. That doesn't mean that some users wont expect it though. Another option would be to follow OpenSSL's deprecations and mark these ciphers as deprecated such that we can remove them in case they indeed get removed from libcypto. That would give users a heads-up that they should have a migration plan for if that time comes. > The code you wrote to load the necessary providers is small enough that I think it's fine, but it's worth pondering thisquestion briefly. Absolutely. cheers ./daniel
On Mon, Sep 21, 2020 at 08:18:42PM +0200, Daniel Gustafsson wrote: > I'm far from fluent in OpenSSL, but AFAICT it has to do with the new provider > API. The default value for padding is unchanged, but it needs to be propaged > into the provider to be set in the context where the old code picked it up > automatically. The relevant OpenSSL commit (83f68df32f0067ee7b0) which changes > the docs to say the function should be called doesn't contain enough > information to explain why. Hmm. Perhaps we should consider making this part conditional on 3.0.0? But I don't see an actual reason why we cannot do it unconditionally either. This needs careful testing for sure. > Turns out it was coming from when I tested against OpenSSL git HEAD, so it may > come in alpha7 (or whatever the next version will be). Let's disregard this > for now until dust settles, I've dropped the patch from the series. Yeah. I have just tried that on the latest HEAD at e771249 and I could reproduce what you saw. It smells to me like a regression introduced by upstream, as per 9a30f40c and c2150f7. I'd rather wait for 3.0.0 to be GA before concluding here. If it proves to be reproducible with their golden version as a bug (or even not as a bug), we will need to have a workaround in any case. -- Michael
Attachment
On Tue, Sep 22, 2020 at 02:01:18PM +0200, Daniel Gustafsson wrote: > Another option would be to follow OpenSSL's deprecations and mark these ciphers > as deprecated such that we can remove them in case they indeed get removed from > libcypto. That would give users a heads-up that they should have a migration > plan for if that time comes. Does that mean a deprecation note in the docs as well as a WARNING when attempting to use those ciphers in pgcryto with the version of OpenSSL marking such ciphers as deprecated? I would assume that we should do both, rather than only one of them to bring more visibility to the user. -- Michael
Attachment
> On 23 Sep 2020, at 10:19, Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Sep 22, 2020 at 02:01:18PM +0200, Daniel Gustafsson wrote: >> Another option would be to follow OpenSSL's deprecations and mark these ciphers >> as deprecated such that we can remove them in case they indeed get removed from >> libcypto. That would give users a heads-up that they should have a migration >> plan for if that time comes. > > Does that mean a deprecation note in the docs as well as a WARNING > when attempting to use those ciphers in pgcryto with the version of > OpenSSL marking such ciphers as deprecated? I would assume that we > should do both, rather than only one of them to bring more visibility > to the user. We generally don't issue WARNINGs for deprecated functionality do we? The only one I can see is for GLOBAL TEMPORARY in temp table creation. The relevant errcode ERRCODE_WARNING_DEPRECATED_FEATURE is also not used anywhere. I'd expect it to just be a note in the documentation, with a prominent placement in the release notes, if we decide to do something like this. cheers ./daniel
> On 22 Sep 2020, at 14:01, Daniel Gustafsson <daniel@yesql.se> wrote: > >> On 22 Sep 2020, at 11:37, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: >> >> On 2020-09-18 16:11, Daniel Gustafsson wrote: >>> Since we support ciphers that are now deprecated, we have no other choice than >>> to load the legacy provider. >> >> Well, we could just have deprecated ciphers fail, unless the user loads the legacy provider in the OS configuration. There might be an argument that that is more proper. > > Thats a fair point. The issue I have with that is that we'd impose a system > wide loading of the legacy provider, impacting other consumers of libssl as > well. After another round of thinking on this, somewhat spurred on by findings in the the nearby FIPS thread, I'm coming around to this idea. Looking at SCRAM+FIPS made me realize that we can't enforce FIPS mode in pgcrypto via the OpenSSL configuration file, since pgcrypto doesn't load the config. The automatic initialization in 1.1.0+ will however load the config file, so not doing it for older versions makes pgcrypto inconsistent. Thus, I think we should make sure that pgcrypto loads the configuratio for all OpenSSL versions, and defer the provider decision in 3.0.0 to the users. This makes the patch minimally intrusive while making pgcrypto behave consistently (and giving 3.0.0 users the option to not run legacy). The attached adds config loading to pgcrypto for < 1.1.0 and a doc notice for enabling the legacy provider in 3.0.0. This will require an alternative output file for non-legacy configs, but that should wait until 3.0.0 is GA since the returned error messages have changed over course of development and may not be set in stone just yet. cheers ./daniel
Attachment
On Tue, Sep 29, 2020 at 12:25:05PM +0200, Daniel Gustafsson wrote: > The attached adds config loading to pgcrypto for < 1.1.0 and a doc notice for > enabling the legacy provider in 3.0.0. This will require an alternative output > file for non-legacy configs, but that should wait until 3.0.0 is GA since the > returned error messages have changed over course of development and may not be > set in stone just yet. FWIW, testing with 3.0.0-alpha9 dev (2d84089), I can see that the error we have in our SSL tests when using a wrong password in the private PEM key leads now to "PEM lib" instead of "bad decrypt". Upthread, we had "nested asn1 error": https://www.postgresql.org/message-id/9CE70AF4-E1A0-4D24-86FA-4C3067077897@yesql.se It looks like not everything is sorted out there yet. pgcrypto is also throwing new errors. Daniel, what if we let this patch aside until upstream has sorted out their stuff? -- Michael
Attachment
> On 26 Nov 2020, at 09:08, Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Sep 29, 2020 at 12:25:05PM +0200, Daniel Gustafsson wrote: >> The attached adds config loading to pgcrypto for < 1.1.0 and a doc notice for >> enabling the legacy provider in 3.0.0. This will require an alternative output >> file for non-legacy configs, but that should wait until 3.0.0 is GA since the >> returned error messages have changed over course of development and may not be >> set in stone just yet. > > FWIW, testing with 3.0.0-alpha9 dev (2d84089), I can see that the > error we have in our SSL tests when using a wrong password in the > private PEM key leads now to "PEM lib" instead of "bad decrypt". > > Upthread, we had "nested asn1 error": > https://www.postgresql.org/message-id/9CE70AF4-E1A0-4D24-86FA-4C3067077897@yesql.se > It looks like not everything is sorted out there yet. > > pgcrypto is also throwing new errors. Daniel, what if we let this > patch aside until upstream has sorted out their stuff? Well, the patch as it stands isn't changing any expected output at all, and only adds a docs notice for OpenSSL 3.0.0 conformance. The gist of the patch is to ensure that all supported versions of OpenSSL are initialized equally as currently < 1.1.0 are bypassing the local openssl config, where 1.1.0+ isn't. So I still think this patch is worth considering. Regarding test output: it's clear that we'll need to revisit this as the dust settles on OpenSSL 3.0.0, but as you say there is no use in doing anything until it has. According to their tracker they are, at this time of writing, 64% complete on the milestone to reach beta readiness [0] (which I believe started counting on alpha7). cheers ./daniel [0] https://github.com/openssl/openssl/milestone/17
On 12.03.21 00:22, Daniel Gustafsson wrote:
>> On 12 Mar 2021, at 00:04, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>>
>> On 11.03.21 11:41, Daniel Gustafsson wrote:
>>> Then there are a few where we get padding back where we really should have
>>> ended up with the "Cipher cannot be initialized" error since DES is in the
>>> legacy provider:
>>> select decrypt_iv(decode('50735067b073bb93', 'hex'), '0123456', 'abcd', 'des');
>>> - decrypt_iv
>>> -------------
>>> - foo
>>> + decrypt_iv
>>> +----------------------------------
>>> + \177\177\177\177\177\177\177\177
>>> (1 row)
>>
>> The attached patch appears to address these cases.
>
> +1, males a lot of sense. This removes said errors when running without the
> legacy provider enabled, and all tests still pass with it enabled.
I have committed this to master. I see that the commit fest entry has
been withdrawn in the meantime. I suppose we'll come back to this,
including possible backpatching, when OpenSSL 3.0.0 is in beta.
On 12.03.21 08:51, Peter Eisentraut wrote: > > On 11.03.21 11:41, Daniel Gustafsson wrote: >> .. and apply the padding changes as proposed in a patch upthread like >> this (these >> work for all OpenSSL versions I've tested, and I'm rather more puzzled >> as to >> why we got away with not having them in the past): > > Yes, before proceeding with this, we should probably understand why > these changes are effective and why they haven't been required in the past. I took another look at this with openssl-3.0.0-beta1. The issue with the garbled padding output is still there. What I found is that pgcrypto has been using the encryption and decryption API slightly incorrectly. You are supposed to call EVP_DecryptUpdate() followed by EVP_DecryptFinal_ex() (and similarly for encryption), but pgcrypto doesn't do the second one. (To be fair, this API was added to OpenSSL after pgcrypto first appeared.) The "final" functions take care of the padding. We have been getting away with it like this because we do the padding manually elsewhere. But apparently, something has changed in OpenSSL 3.0.0 in that if padding is enabled in OpenSSL, EVP_DecryptUpdate() doesn't flush the last normal block until the "final" function is called. Your proposed fix was to explicitly disable padding, and then this problem goes away. You can still call the "final" functions, but they won't do anything, except check that there is no more data left, but we already check that elsewhere. Another option is to throw out our own padding code and let OpenSSL handle it. See attached demo patch. But that breaks the non-OpenSSL code in internal.c, so we'd have to re-add the padding code there. So this isn't quite as straightforward an option. (At least, with the patch we can confirm that the OpenSSL padding works consistently with our own implementation.) So I think your proposed patch is sound and a good short-term and low-risk solution.
Attachment
> On 3 Jul 2021, at 17:00, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > > On 12.03.21 08:51, Peter Eisentraut wrote: >> On 11.03.21 11:41, Daniel Gustafsson wrote: >>> .. and apply the padding changes as proposed in a patch upthread like this (these >>> work for all OpenSSL versions I've tested, and I'm rather more puzzled as to >>> why we got away with not having them in the past): >> Yes, before proceeding with this, we should probably understand why these changes are effective and why they haven't beenrequired in the past. > > I took another look at this with openssl-3.0.0-beta1. The issue with the garbled padding output is still there. WhatI found is that pgcrypto has been using the encryption and decryption API slightly incorrectly. You are supposed tocall EVP_DecryptUpdate() followed by EVP_DecryptFinal_ex() (and similarly for encryption), but pgcrypto doesn't do thesecond one. (To be fair, this API was added to OpenSSL after pgcrypto first appeared.) The "final" functions take careof the padding. We have been getting away with it like this because we do the padding manually elsewhere. That does make a lot of sense, following the code and API docs I concur with your findings. > ..apparently, something has changed in OpenSSL 3.0.0 in that if padding is enabled in OpenSSL, EVP_DecryptUpdate() doesn'tflush the last normal block until the "final" function is called. Skimming the code I wasn't able to find something off the cuff, but there has been work done to postpone/move padding for constant time operations so maybe it's related to that. > Your proposed fix was to explicitly disable padding, and then this problem goes away. You can still call the "final" functions,but they won't do anything, except check that there is no more data left, but we already check that elsewhere. In earlier versions, _Final also closed the context to ensure nothing can leak from there, but I'm not sure if 1.0.1 constitutes as earlier. Still calling it from the finish function seems like a good idea though. > Another option is to throw out our own padding code and let OpenSSL handle it. See attached demo patch. But that breaksthe non-OpenSSL code in internal.c, so we'd have to re-add the padding code there. So this isn't quite as straightforwardan option. While the PX cleanup is nice, since we can't get rid of all the padding we simply shift the complexity to another place where I'd be wary of introducing bugs into quite stable code. > (At least, with the patch we can confirm that the OpenSSL padding works consistently with our own implementation.) +1 > So I think your proposed patch is sound and a good short-term and low-risk solution The attached 0001 disables the padding. I've tested this with OpenSSL 1.0.1, 1.0.2, 1.1.1 and Git HEAD at e278127cbfa2709d. Another aspect of OpenSSL 3 compatibility is that of legacy cipher support, and as we concluded upthread it's best to leave that to the user to define in openssl.cnf. The attached 0002 adds alternative output files for 3.0.0 installations without the legacy provider loaded, as well as adds a note in the pgcrypto docs to enable it in case DES is needed. It does annoy me a bit that we don't load the openssl.cnf file for 1.0.1 if we start mentioning it in the docs for other versions, but it's probably not worth the effort to fix it given the lack of complaints so far (it needs a call to OPENSSL_config(NULL); guarded to HAVE_ macros for 1.0.1). -- Daniel Gustafsson https://vmware.com/
Attachment
On Tue, Jul 20, 2021 at 01:23:42AM +0200, Daniel Gustafsson wrote: > Another aspect of OpenSSL 3 compatibility is that of legacy cipher support, and > as we concluded upthread it's best to leave that to the user to define in > openssl.cnf. The attached 0002 adds alternative output files for 3.0.0 > installations without the legacy provider loaded, as well as adds a note in the > pgcrypto docs to enable it in case DES is needed. It does annoy me a bit that > we don't load the openssl.cnf file for 1.0.1 if we start mentioning it in the > docs for other versions, but it's probably not worth the effort to fix it given > the lack of complaints so far (it needs a call to OPENSSL_config(NULL); guarded > to HAVE_ macros for 1.0.1). Sounds sensible as a whole. Another thing I can notice is that OpenSSL 3.0.0beta1 has taken care of the issue causing diffs in the tests of src/test/ssl/. So once pgcrypto is addressed, it looks like there is nothing left for this thread. -- Michael
Attachment
> On 20 Jul 2021, at 09:54, Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Jul 20, 2021 at 01:23:42AM +0200, Daniel Gustafsson wrote: >> Another aspect of OpenSSL 3 compatibility is that of legacy cipher support, and >> as we concluded upthread it's best to leave that to the user to define in >> openssl.cnf. The attached 0002 adds alternative output files for 3.0.0 >> installations without the legacy provider loaded, as well as adds a note in the >> pgcrypto docs to enable it in case DES is needed. It does annoy me a bit that >> we don't load the openssl.cnf file for 1.0.1 if we start mentioning it in the >> docs for other versions, but it's probably not worth the effort to fix it given >> the lack of complaints so far (it needs a call to OPENSSL_config(NULL); guarded >> to HAVE_ macros for 1.0.1). > > Sounds sensible as a whole. Thanks for reviewing! > Another thing I can notice is that > OpenSSL 3.0.0beta1 has taken care of the issue causing diffs in the > tests of src/test/ssl/. So once pgcrypto is addressed, it looks like > there is nothing left for this thread. That's a good point, I forgot to bring that up. -- Daniel Gustafsson https://vmware.com/
On 20.07.21 01:23, Daniel Gustafsson wrote: >> So I think your proposed patch is sound and a good short-term and low-risk solution > The attached 0001 disables the padding. I've tested this with OpenSSL 1.0.1, > 1.0.2, 1.1.1 and Git HEAD at e278127cbfa2709d. > > Another aspect of OpenSSL 3 compatibility is that of legacy cipher support, and > as we concluded upthread it's best to leave that to the user to define in > openssl.cnf. The attached 0002 adds alternative output files for 3.0.0 > installations without the legacy provider loaded, as well as adds a note in the > pgcrypto docs to enable it in case DES is needed. It does annoy me a bit that > we don't load the openssl.cnf file for 1.0.1 if we start mentioning it in the > docs for other versions, but it's probably not worth the effort to fix it given > the lack of complaints so far (it needs a call to OPENSSL_config(NULL); guarded > to HAVE_ macros for 1.0.1). Are you going to commit these?
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> Are you going to commit these?
Note that with release wraps scheduled for Monday, we are probably
already past the time when it'd be wise to push anything that has
a significant chance of introducing portability issues. There's
just not much time to deal with it if the buildfarm shows problems.
So unless you intend this as HEAD-only, I'd counsel waiting for the
release window to pass.
regards, tom lane
> On 6 Aug 2021, at 21:01, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes: >> Are you going to commit these? Absolutely, a combination of unplanned home renovations and vacations changed my plans a bit recently. > Note that with release wraps scheduled for Monday, we are probably > already past the time when it'd be wise to push anything that has > a significant chance of introducing portability issues. There's > just not much time to deal with it if the buildfarm shows problems. > So unless you intend this as HEAD-only, I'd counsel waiting for the > release window to pass. Until there is an animal running OpenSSL 3.0.0 in the buildfarm I think this should be HEAD only. Further down the line we need to support OpenSSL 3 in all backbranches IMO since they are all equally likely to be compiled against it, but not until we can regularly test against it in the farm. -- Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
> Until there is an animal running OpenSSL 3.0.0 in the buildfarm I think this
> should be HEAD only. Further down the line we need to support OpenSSL 3 in all
> backbranches IMO since they are all equally likely to be compiled against it,
> but not until we can regularly test against it in the farm.
Works for me.
regards, tom lane
> On 6 Aug 2021, at 21:17, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <daniel@yesql.se> writes: >> Until there is an animal running OpenSSL 3.0.0 in the buildfarm I think this >> should be HEAD only. Further down the line we need to support OpenSSL 3 in all >> backbranches IMO since they are all equally likely to be compiled against it, >> but not until we can regularly test against it in the farm. > > Works for me. These have now been committed, when OpenSSL 3.0.0 ships and there is coverage in the buildfarm I’ll revisit this for the backbranches. -- Daniel Gustafsson https://vmware.com/
> On 10 Aug 2021, at 15:27, Daniel Gustafsson <daniel@yesql.se> wrote: > These have now been committed, when OpenSSL 3.0.0 ships and there is coverage > in the buildfarm I’ll revisit this for the backbranches. As an update to this, I’ve tested the tree frozen for the upcoming 3.0.0 release (scheduled for today AFAIK) and postgres still builds and tests clean with the patches that were applied. -- Daniel Gustafsson https://vmware.com/
On Tue, Sep 07, 2021 at 02:04:23PM +0200, Daniel Gustafsson wrote: > On 10 Aug 2021, at 15:27, Daniel Gustafsson <daniel@yesql.se> wrote: > >> These have now been committed, when OpenSSL 3.0.0 ships and there is coverage >> in the buildfarm I’ll revisit this for the backbranches. > > As an update to this, I’ve tested the tree frozen for the upcoming 3.0.0 > release (scheduled for today AFAIK) and postgres still builds and tests clean > with the patches that were applied. I think that the time to do a backpatch of 318df8 has come. caiman, that runs Fedora 35, has just failed: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=caiman&dt=2021-09-22%2006%3A28%3A00 Here is a diff: @@ -8,168 +8,88 @@ decode('0000000000000000', 'hex'), decode('0000000000000000', 'hex'), 'bf-ecb/pad:none'), 'hex'); - encode ------------------- - 4ef997456198dd78 -(1 row) - +ERROR: encrypt error: Cipher cannot be initialized ? And if I look at the list of packages at the top of Fedora, I see an update to OpenSSL 3.0.0: https://fedora.pkgs.org/rawhide/fedora-aarch64/openssl-libs-3.0.0-1.fc36.aarch64.rpm.html So the coverage is here. HEAD passes, not the stabele branches. At least for 14 it would be nice to do that before the release of next week. -- Michael
Attachment
> On 22 Sep 2021, at 09:49, Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Sep 07, 2021 at 02:04:23PM +0200, Daniel Gustafsson wrote: >> On 10 Aug 2021, at 15:27, Daniel Gustafsson <daniel@yesql.se> wrote: >> >>> These have now been committed, when OpenSSL 3.0.0 ships and there is coverage >>> in the buildfarm I’ll revisit this for the backbranches. >> >> As an update to this, I’ve tested the tree frozen for the upcoming 3.0.0 >> release (scheduled for today AFAIK) and postgres still builds and tests clean >> with the patches that were applied. > > I think that the time to do a backpatch of 318df8 has come. caiman, > that runs Fedora 35, has just failed: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=caiman&dt=2021-09-22%2006%3A28%3A00 > > Here is a diff: > @@ -8,168 +8,88 @@ > decode('0000000000000000', 'hex'), > decode('0000000000000000', 'hex'), > 'bf-ecb/pad:none'), 'hex'); > - encode > ------------------- > - 4ef997456198dd78 > -(1 row) > - > +ERROR: encrypt error: Cipher cannot be initialized ? That particular error stems from the legacy provider not being enabled in openssl.cnf, so for this we need to backpatch 72bbff4cd as well. > So the coverage is here. HEAD passes, not the stabele branches. At > least for 14 it would be nice to do that before the release of next > week. Agreed, I will go ahead and prep backpatches for 318df8 and 72bbff4cd. -- Daniel Gustafsson https://vmware.com/
> On 22 Sep 2021, at 10:06, Daniel Gustafsson <daniel@yesql.se> wrote:
> Agreed, I will go ahead and prep backpatches for 318df8 and 72bbff4cd.
These commits are enough to keep 14 happy, and I intend to apply them tomorrow
after another round of testing and caffeine.
For the 13- backbranches we also need to backport 22e1943f1 ("pgcrypto: Check
for error return of px_cipher_decrypt()" by Peter E) in order to avoid
incorrect results for decrypt tests on disallowed ciphers. Does anyone have
any concerns about applying this to backbranches?
13 and older will, when compiled against OpenSSL 3.0.0, produce a fair amount
of compiler warnings on usage of depreceted functionality but there is really
anything we can do as suppressing that is beyond the scope of a backpatchable
fix IMHO.
--
Daniel Gustafsson https://vmware.com/
On Thu, Sep 23, 2021 at 2:51 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> For the 13- backbranches we also need to backport 22e1943f1 ("pgcrypto: Check
> for error return of px_cipher_decrypt()" by Peter E) in order to avoid
> incorrect results for decrypt tests on disallowed ciphers. Does anyone have
> any concerns about applying this to backbranches?
To me it looks like it would be more concerning if we did not apply it
to back-branches.
--
Robert Haas
EDB: http://www.enterprisedb.com
On 23.09.21 20:51, Daniel Gustafsson wrote:
> For the 13- backbranches we also need to backport 22e1943f1 ("pgcrypto: Check
> for error return of px_cipher_decrypt()" by Peter E) in order to avoid
> incorrect results for decrypt tests on disallowed ciphers. Does anyone have
> any concerns about applying this to backbranches?
This should be backpatched as a bug fix.
> 13 and older will, when compiled against OpenSSL 3.0.0, produce a fair amount
> of compiler warnings on usage of depreceted functionality but there is really
> anything we can do as suppressing that is beyond the scope of a backpatchable
> fix IMHO.
Right, that's just a matter of adjusting the compiler warnings.
Earlier in this thread, I had suggested backpatching the
OPENSSL_API_COMPAT definition to PG13, but now I'm thinking I wouldn't
bother, since that still wouldn't help with anything older.
> On 23 Sep 2021, at 23:26, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> On 23.09.21 20:51, Daniel Gustafsson wrote:
>> For the 13- backbranches we also need to backport 22e1943f1 ("pgcrypto: Check
>> for error return of px_cipher_decrypt()" by Peter E) in order to avoid
>> incorrect results for decrypt tests on disallowed ciphers. Does anyone have
>> any concerns about applying this to backbranches?
>
> This should be backpatched as a bug fix.
Thanks for confirming, I will go ahead and do that.
--
Daniel Gustafsson https://vmware.com/
> On 23 Sep 2021, at 23:41, Daniel Gustafsson <daniel@yesql.se> wrote: > Thanks for confirming, I will go ahead and do that. These have now been pushed to 14 through to 10 ahead of next week releases, I will keep an eye on caiman as it builds these branches. The OpenSSL support in 9.6 pgcrypto isn't using the EVP API (committed in 5ff4a67f63) so it's a bit trickier to get green, but I'll take a stab at that when my fever goes down a bit. -- Daniel Gustafsson https://vmware.com/
On Sat, Sep 25, 2021 at 11:55:03AM +0200, Daniel Gustafsson wrote: > These have now been pushed to 14 through to 10 ahead of next week releases, I > will keep an eye on caiman as it builds these branches. The OpenSSL support in > 9.6 pgcrypto isn't using the EVP API (committed in 5ff4a67f63) so it's a bit > trickier to get green, Thanks! As 9.6 will be EOL'd in a couple of weeks, is that really worth the effort though? It sounds risky to me to introduce an invasive change as that would increase the risk of bugs for existing users. So my vote would be to just let this one go. > but I'll take a stab at that when my fever goes down a bit. Ouch. Take care! -- Michael
Attachment
> On 25 Sep 2021, at 12:03, Michael Paquier <michael@paquier.xyz> wrote: > As 9.6 will be EOL'd in a couple of weeks, is that really > worth the effort though? It sounds risky to me to introduce an > invasive change as that would increase the risk of bugs for existing > users. So my vote would be to just let this one go. Agreed, if it's not a simple fix it's unlikely to be worth it. -- Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
>> On 25 Sep 2021, at 12:03, Michael Paquier <michael@paquier.xyz> wrote:
>> As 9.6 will be EOL'd in a couple of weeks, is that really
>> worth the effort though? It sounds risky to me to introduce an
>> invasive change as that would increase the risk of bugs for existing
>> users. So my vote would be to just let this one go.
> Agreed, if it's not a simple fix it's unlikely to be worth it.
Yeah, there will be no second chance to get 9.6.last right,
so I'd vote against touching it for this.
regards, tom lane
> On 25 Sep 2021, at 15:45, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <daniel@yesql.se> writes: >>> On 25 Sep 2021, at 12:03, Michael Paquier <michael@paquier.xyz> wrote: >>> As 9.6 will be EOL'd in a couple of weeks, is that really >>> worth the effort though? It sounds risky to me to introduce an >>> invasive change as that would increase the risk of bugs for existing >>> users. So my vote would be to just let this one go. > >> Agreed, if it's not a simple fix it's unlikely to be worth it. > > Yeah, there will be no second chance to get 9.6.last right, > so I'd vote against touching it for this. Fair point. Should we perhaps instead include a note in the pgcrypto docs for 9.6 that 3.0.0 isn't supported and leave it at that? -- Daniel Gustafsson https://vmware.com/
On Wed, 29 Jun 2022 at 10:55, Daniel Gustafsson <daniel@yesql.se> wrote: > These have now been pushed to 14 through to 10 ahead of next week releases I upgraded my OS to Ubuntu 22.04 and it seems that "Define OPENSSL_API_COMPAT" commit was never backported (4d3db13621be64fbac2faf7c01c4879d20885c1b). I now get various deprecation warnings when compiling PG13 on Ubuntu 22.04, because of OpenSSL 3.0. Was this simply forgotten, or is there a reason why it wasn't backported?
> On 29 Jun 2022, at 11:02, Jelte Fennema <postgres@jeltef.nl> wrote: > > On Wed, 29 Jun 2022 at 10:55, Daniel Gustafsson <daniel@yesql.se> wrote: >> These have now been pushed to 14 through to 10 ahead of next week releases > > I upgraded my OS to Ubuntu 22.04 and it seems that "Define > OPENSSL_API_COMPAT" commit was never backported > (4d3db13621be64fbac2faf7c01c4879d20885c1b). I now get various > deprecation warnings when compiling PG13 on Ubuntu 22.04, because of > OpenSSL 3.0. Was this simply forgotten, or is there a reason why it > wasn't backported? See upthread in ef5c7896-20cb-843f-e91e-0ee5f7fd932e@enterprisedb.com, below is the relevant portion: >> 13 and older will, when compiled against OpenSSL 3.0.0, produce a fair amount >> of compiler warnings on usage of depreceted functionality but there is really >> anything we can do as suppressing that is beyond the scope of a backpatchable >> fix IMHO. > > Right, that's just a matter of adjusting the compiler warnings. > > Earlier in this thread, I had suggested backpatching the OPENSSL_API_COMPAT definition to PG13, but now I'm thinking Iwouldn't bother, since that still wouldn't help with anything older. -- Daniel Gustafsson https://vmware.com/
> See upthread in ef5c7896-20cb-843f-e91e-0ee5f7fd932e@enterprisedb.com I saw that section, but I thought that only applied before you backpatched the actual fixes to PG13 and below. I mean there's no reason anymore not to compile those older versions with OpenSSL 3.0, right? If so, it seems confusing for the build to spit out warnings that indicate the contrary.
> On 29 Jun 2022, at 11:44, Jelte Fennema <postgres@jeltef.nl> wrote: > >> See upthread in ef5c7896-20cb-843f-e91e-0ee5f7fd932e@enterprisedb.com > > I saw that section, but I thought that only applied before you > backpatched the actual fixes to PG13 and below. I mean there's no > reason anymore not to compile those older versions with OpenSSL 3.0, > right? If so, it seems confusing for the build to spit out warnings > that indicate the contrary. The project isn't automatically fixing compiler warnings or library deprecation warnings in back-branches. I guess one could make the argument for this case given how widespread OpenSSL 3.0, but it comes with a significant testing effort to ensure that all back-branches behave correctly with all version of OpenSSL so it's not for free (it should be, but with OpenSSL I would personally not trust that). Also, PG12 and below had 0.9.8 as minimum version. -- Daniel Gustafsson https://vmware.com/