Horton

Thread

Commits

  1. Define OPENSSL_API_COMPAT

  2. Add alternative output for OpenSSL 3 without legacy loaded

  3. Disable OpenSSL EVP digest padding in pgcrypto

  4. pgcrypto: Check for error return of px_cipher_decrypt()

  5. OpenSSL 3.0.0 compatibility in tests

  6. Make ssl certificate for ssl_passphrase_callback test via Makefile

  7. Provide a TLS init hook

  1. OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2020-05-28T22:16:47Z

    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

  2. Re: OpenSSL 3.0.0 compatibility

    Laurenz Albe <laurenz.albe@cybertec.at> — 2020-05-29T06:06:30Z

    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

  3. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2020-05-29T07:04:38Z

    > 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

  4. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-05-29T11:34:49Z

    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

  5. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2020-05-29T12:45:44Z

    > 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 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.

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 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.

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 provider gets 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

  6. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-05-30T09:29:11Z

    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 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.
> 
> 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

  7. Re: OpenSSL 3.0.0 compatibility

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-05-30T12:34:37Z

    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

  8. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2020-05-31T02:52:15Z

    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

  9. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-06-01T08:33:12Z

    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

  10. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-06-01T08:44:17Z

    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

  11. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2020-06-01T08:49:04Z

    > 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 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.
>> 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.

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 version we 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 look into 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

  12. Re: OpenSSL 3.0.0 compatibility

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-06-01T11:58:21Z

    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

  13. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2020-06-01T12:03:38Z

    > 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

  14. Re: OpenSSL 3.0.0 compatibility

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-06-01T13:23:24Z

    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

  15. Re: OpenSSL 3.0.0 compatibility

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-01T14:00:05Z

    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

  16. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-06-02T08:57:40Z

    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

  17. Re: OpenSSL 3.0.0 compatibility

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-06-02T18:45:11Z

    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

  18. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2020-06-03T05:26:07Z

    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

  19. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2020-06-05T07:31:50Z

    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

  20. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-06-05T09:21:43Z

    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

  21. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-07-07T17:45:58Z

    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

  22. Re: OpenSSL 3.0.0 compatibility

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-07-07T17:53:11Z

    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

  23. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2020-07-07T20:52:47Z

    > 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

  24. Re: OpenSSL 3.0.0 compatibility

    Robert Haas <robertmhaas@gmail.com> — 2020-07-08T14:51:36Z

    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

  25. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-07-08T16:03:48Z

    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

  26. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-07-09T06:43:24Z

    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

  27. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-07-13T07:40:25Z

    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

  28. Re: OpenSSL 3.0.0 compatibility

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-07-13T13:38:51Z

    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

  29. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-07-16T08:58:58Z

    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

  30. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2020-07-16T11:45:55Z

    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

  31. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-07-19T14:29:54Z

    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

  32. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2020-07-21T05:32:31Z

    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

  33. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2020-08-17T04:12:42Z

    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

  34. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2020-09-18T14:11:13Z

    > 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

  35. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2020-09-19T02:11:48Z

    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

  36. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2020-09-21T18:18:42Z

    > 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

  37. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-09-22T09:37:57Z

    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

  38. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2020-09-22T12:01:18Z

    > 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.

> 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.

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 this question briefly.

Absolutely.

cheers ./daniel

  39. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2020-09-23T08:17:00Z

    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

  40. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2020-09-23T08:19:55Z

    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

  41. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2020-09-23T08:44:20Z

    > 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

  42. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2020-09-29T10:25:05Z

    > 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

  43. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2020-11-26T08:08:58Z

    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

  44. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2020-11-30T13:05:21Z

    > 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

  45. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2021-03-03T13:55:41Z

    This thread is still in the commit fest, but I don't see any actual 
proposed patch still pending.  Most of the activity has moved into other 
threads.

Could you update the status of this CF entry, and perhaps also on the 
status of OpenSSL compatibility in general?

  46. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2021-03-04T08:15:05Z

    On Wed, Mar 03, 2021 at 02:55:41PM +0100, Peter Eisentraut wrote:
> This thread is still in the commit fest, but I don't see any actual proposed
> patch still pending.  Most of the activity has moved into other threads.
> 
> Could you update the status of this CF entry, and perhaps also on the status
> of OpenSSL compatibility in general?

3.0.0 has released an alpha 12 on the 18th of February, so their stuff
is not quite close to GA yet.

I have not looked closely, but my guess is that it would take a couple
of extra months at least to see a release.  What if we just waited and
revisited this stuff during the next dev cycle, once there is at least
a beta, meaning mostly stable APIs?

Daniel, what do you think?
--
Michael

  47. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2021-03-10T08:23:15Z

    > On 3 Mar 2021, at 14:55, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> 
> This thread is still in the commit fest, but I don't see any actual proposed patch still pending.  Most of the activity has moved into other threads.

The doc changes in the patch proposed on 29/9 still stands, although I see that
it had an off by one in mentioning MD5 when it should be MD4 et.al; so
something more like the below.

diff --git a/doc/src/sgml/pgcrypto.sgml b/doc/src/sgml/pgcrypto.sgml
index b6bb23de0f..d45464c7ea 100644
--- a/doc/src/sgml/pgcrypto.sgml
+++ b/doc/src/sgml/pgcrypto.sgml
@@ -1234,6 +1234,12 @@ gen_random_uuid() returns uuid
     </tgroup>
    </table>

+   <para>
+    When compiled against <productname>OpenSSL</productname> 3.0.0, the legacy
+    provider must be activated in the system <filename>openssl.cnf</filename>
+    configuration file in order to use older ciphers like DES and Blowfish.
+   </para>
+
    <para>

> Could you update the status of this CF entry, and perhaps also on the status of OpenSSL compatibility in general?

Let's just wait for 3.0.0 to ship before we do anything.

--
Daniel Gustafsson		https://vmware.com/

  48. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-03-11T10:03:21Z

    On 10.03.21 09:23, Daniel Gustafsson wrote:
>> On 3 Mar 2021, at 14:55, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>>
>> This thread is still in the commit fest, but I don't see any actual proposed patch still pending.  Most of the activity has moved into other threads.
> 
> The doc changes in the patch proposed on 29/9 still stands, although I see that
> it had an off by one in mentioning MD5 when it should be MD4 et.al; so
> something more like the below.
> 
> diff --git a/doc/src/sgml/pgcrypto.sgml b/doc/src/sgml/pgcrypto.sgml
> index b6bb23de0f..d45464c7ea 100644
> --- a/doc/src/sgml/pgcrypto.sgml
> +++ b/doc/src/sgml/pgcrypto.sgml
> @@ -1234,6 +1234,12 @@ gen_random_uuid() returns uuid
>       </tgroup>
>      </table>
> 
> +   <para>
> +    When compiled against <productname>OpenSSL</productname> 3.0.0, the legacy
> +    provider must be activated in the system <filename>openssl.cnf</filename>
> +    configuration file in order to use older ciphers like DES and Blowfish.
> +   </para>
> +
>      <para>

I tested the current master with openssl-3.0.0-alpha12.

Everything builds cleanly.

The ssl tests fail with a small error message difference that must have 
been introduced recently, because I think this was never reported before:

--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -449,7 +449,7 @@
  test_connect_fails(
     $common_connstr,
     "user=ssltestuser sslcert=ssl/client.crt 
sslkey=ssl/client-encrypted-pem_tmp.key sslpassword='wrong'",
-   qr!\Qprivate key file "ssl/client-encrypted-pem_tmp.key": bad 
decrypt\E!,
+   qr!\Qprivate key file "ssl/client-encrypted-pem_tmp.key":\E (bad 
decrypt|PEM lib)!,
     "certificate authorization fails with correct client cert and wrong 
password in encrypted PEM format"
  );

The pgcrypto tests fail all over the place.  Some of these failures are 
quite likely because of the disabled legacy provider, but some appear to 
be due to bad error handling.

Then I tried enabling the legacy provider in openssl.cnf.  This caused 
pg_strong_random() to fail, which causes initdb to fail, like this:

PANIC:  could not generate secret authorization token

I tried patching around in pg_strong_random.c to use the /dev/urandom 
variant as a workaround, but apparently that doesn't work.  You get all 
kinds of scary make check failures from md5 and sha256 calls.

So, we knew pgcrypto was in trouble with openssl 3.0.0, but can someone 
else get its tests to pass with some kind of openssl.cnf configuration?

  49. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2021-03-11T10:41:22Z

    > On 11 Mar 2021, at 11:03, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

> The ssl tests fail with a small error message difference that must have been introduced recently, because I think this was never reported before:

This was mentioned by Michael on 26/11, it was earlier in the 3.0.0 cycle
reported as "nested asn.1 error".  Waiting for 3.0.0 to go beta or GA before
changing saves us from having to change again should they update, but tests
will fail for anyone testing out new OpenSSL versions.

> The pgcrypto tests fail all over the place.  Some of these failures are quite likely because of the disabled legacy provider, but some appear to be due to bad error handling.

The below ones are likely due to the provider not being loaded, but as you say
they are probably cases of poor error handling as they fail in various places
while probably being due to the same root cause:

+ERROR:  encrypt error: Key was too big

+ERROR:  encrypt error: Cipher cannot be initialized ?

+ERROR:  Wrong key or corrupt data

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)

> Then I tried enabling the legacy provider in openssl.cnf.  This caused pg_strong_random() to fail, which causes initdb to fail, like this:

Huh?  Enabling the legacy provider shouldn't affect the CRNG.  I see no such
failure, and haven't in any alpha version tested.  How did you change the
openssl config?  If one can break pg_strong_random with a config change then we
need defensive coding to cope with that.

> So, we knew pgcrypto was in trouble with openssl 3.0.0, but can someone else get its tests to pass with some kind of openssl.cnf configuration?

If I enable the legacy provider in openssl.cnf like this:

[openssl_init]
providers = provider_sect

[provider_sect]
default = default_sect
legacy = legacy_sect

[default_sect]
activate = 1

[legacy_sect]
activate = 1

.. 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):

diff --git a/contrib/pgcrypto/openssl.c b/contrib/pgcrypto/openssl.c
index ed8e74a2b9..e236b0d79c 100644
--- a/contrib/pgcrypto/openssl.c
+++ b/contrib/pgcrypto/openssl.c
@@ -379,6 +379,8 @@ gen_ossl_decrypt(PX_Cipher *c, const uint8 *data, unsigned dlen,
        {
                if (!EVP_DecryptInit_ex(od->evp_ctx, od->evp_ciph, NULL, NULL, NULL))
                        return PXE_CIPHER_INIT;
+               if (!EVP_CIPHER_CTX_set_padding(od->evp_ctx, 0))
+                       return PXE_CIPHER_INIT;
                if (!EVP_CIPHER_CTX_set_key_length(od->evp_ctx, od->klen))
                        return PXE_CIPHER_INIT;
                if (!EVP_DecryptInit_ex(od->evp_ctx, NULL, NULL, od->key, od->iv))
@@ -403,6 +405,8 @@ gen_ossl_encrypt(PX_Cipher *c, const uint8 *data, unsigned dlen,
        {
                if (!EVP_EncryptInit_ex(od->evp_ctx, od->evp_ciph, NULL, NULL, NULL))
                        return PXE_CIPHER_INIT;
+               if (!EVP_CIPHER_CTX_set_padding(od->evp_ctx, 0))
+                       return PXE_CIPHER_INIT;
                if (!EVP_CIPHER_CTX_set_key_length(od->evp_ctx, od->klen))
                        return PXE_CIPHER_INIT;
                if (!EVP_EncryptInit_ex(od->evp_ctx, NULL, NULL, od->key, od->iv))

.. then all the pgcrypto tests pass for me as well as "make check", with a single
SSL test failing on the above mentioned PEM lib error message.

Did you build OpenSSL with anything non-standard?

--
Daniel Gustafsson		https://vmware.com/

  50. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2021-03-11T13:08:20Z

    On Thu, Mar 11, 2021 at 11:41:22AM +0100, 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):

No objections from here to disable the padding and tighten a bit the
error checks on the amount of data encrypted or decrypted based on
the block size.  This indeed works correctly down to OpenSSL 1.0.1 as
far as I have tested, so let's extract this part first, and figure the
rest after there is a beta.
--
Michael

  51. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-03-11T23:04:12Z

    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.

  52. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2021-03-11T23:22:45Z

    > 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.

--
Daniel Gustafsson		https://vmware.com/

  53. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-03-12T07:51:39Z

    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.

  54. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-03-23T10:52:22Z

    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.

  55. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-07-03T15:00:42Z

    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.

  56. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2021-07-19T23:23:42Z

    > 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 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.

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't flush 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 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.

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/

  57. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2021-07-20T07:54:47Z

    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

  58. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2021-07-20T21:55:26Z

    > 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/

  59. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-08-06T18:41:22Z

    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?

  60. Re: OpenSSL 3.0.0 compatibility

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-06T19:01:57Z

    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

  61. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2021-08-06T19:14:15Z

    > 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/

  62. Re: OpenSSL 3.0.0 compatibility

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-06T19:17:28Z

    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

  63. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2021-08-10T13:27:18Z

    > 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/

  64. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2021-09-07T12:04:23Z

    > 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/

  65. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2021-09-22T07:49:10Z

    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

  66. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2021-09-22T08:06:26Z

    > 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/

  67. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2021-09-23T18:51:08Z

    > 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/

  68. Re: OpenSSL 3.0.0 compatibility

    Robert Haas <robertmhaas@gmail.com> — 2021-09-23T19:54:54Z

    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

  69. Re: OpenSSL 3.0.0 compatibility

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-09-23T21:26:46Z

    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.

  70. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2021-09-23T21:41:26Z

    > 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/

  71. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2021-09-25T09:55:03Z

    > 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/

  72. Re: OpenSSL 3.0.0 compatibility

    Michael Paquier <michael@paquier.xyz> — 2021-09-25T10:03:29Z

    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

  73. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2021-09-25T10:07:43Z

    > 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/

  74. Re: OpenSSL 3.0.0 compatibility

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-09-25T13:45:29Z

    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

  75. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2021-09-25T18:34:44Z

    > 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/

  76. Re: OpenSSL 3.0.0 compatibility

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2022-06-29T09:02:56Z

    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?

  77. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2022-06-29T09:25:59Z

    > 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 I wouldn't bother, since that still wouldn't help with anything older.

--
Daniel Gustafsson		https://vmware.com/

  78. Re: OpenSSL 3.0.0 compatibility

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2022-06-29T09:44:00Z

    > 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.

  79. Re: OpenSSL 3.0.0 compatibility

    Daniel Gustafsson <daniel@yesql.se> — 2022-06-29T10:59:32Z

    > 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/