Thread

Commits

pgcrypto: Remove non-OpenSSL support
- db7d1a7b0530 15.0 landed

remove internal support in pgcrypto?

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-08-24T09:13:44Z

During the discussion on OpenSSL 3.0.0 support in pgcrypto [0], I 
started to wonder whether the "internal" code variants in pgcrypto (the 
ones that implement the ciphers themselves instead of using OpenSSL) are 
more trouble than they are worth.  As discussed there, keeping this adds 
some amount of complexity in the code that could otherwise easily be 
done away with.

Historically, this made some sense.  OpenSSL support and pgcrypto came 
into PostgreSQL at around the same time.  So it was probably reasonable 
for pgcrypto not to rely exclusively on OpenSSL being available.  But 
today, building PostgreSQL for production without some kind of SSL 
support seems rare, and then nevertheless requiring cryptographic 
hashing and encryption support from pgcrypto seems unreasonable.

So I'm tempted to suggest that we remove the built-in, non-OpenSSL 
cipher and hash implementations in pgcrypto (basically INT_SRCS in 
pgcrypto/Makefile), and then also pursue the simplifications in the 
OpenSSL code paths described in [0].

Thoughts?

(Some thoughts from those pursuing NSS support would also be useful.)


[0]: 
https://www.postgresql.org/message-id/b1a62889-bb45-e5e0-d138-7a370a0a334f@enterprisedb.com

Re: remove internal support in pgcrypto?

Michael Paquier <michael@paquier.xyz> — 2021-08-24T10:28:42Z

On Tue, Aug 24, 2021 at 11:13:44AM +0200, Peter Eisentraut wrote:
> So I'm tempted to suggest that we remove the built-in, non-OpenSSL cipher
> and hash implementations in pgcrypto (basically INT_SRCS in
> pgcrypto/Makefile), and then also pursue the simplifications in the OpenSSL
> code paths described in [0].

+1 to remove the internal parts.  And the options of the non-OpenSSL
code paths are more limited than the OpenSSL ones, with md5, sha1 and
sha2.

NSS has support for most of the hash implementations pgcrypto makes
use of, as far as I recall.
--
Michael

Re: remove internal support in pgcrypto?

Daniel Gustafsson <daniel@yesql.se> — 2021-08-24T12:38:05Z

> On 24 Aug 2021, at 11:13, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

> So I'm tempted to suggest that we remove the built-in, non-OpenSSL cipher and hash implementations in pgcrypto (basically INT_SRCS in pgcrypto/Makefile), and then also pursue the simplifications in the OpenSSL code paths described in [0].

+1

> Thoughts?

With src/common/cryptohash_*.c and contrib/pgcrypto we have two abstractions
for hashing ciphers, should we perhaps retire hashing from pgcrypto altogether
and pull across what we feel is useful to core (AES and 3DES and..)?  There is
already significant overlap, and allowing core to only support certain ciphers
when compiled with OpenSSL isn’t any different from doing it in pgcrypto
really.

> (Some thoughts from those pursuing NSS support would also be useful.)

Blowfish and CAST5 are not available in NSS.  I've used the internal Blowfish
implementation as a fallback in the NSS patch and left CAST5 as not supported.
This proposal would mean that Blowfish too wasn’t supported in NSS builds, but
I personally don’t see that as a dealbreaker.

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

Re: remove internal support in pgcrypto?

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-10-30T12:11:53Z

On 24.08.21 11:13, Peter Eisentraut wrote:
> So I'm tempted to suggest that we remove the built-in, non-OpenSSL 
> cipher and hash implementations in pgcrypto (basically INT_SRCS in 
> pgcrypto/Makefile), and then also pursue the simplifications in the 
> OpenSSL code paths described in [0].

Here is a patch for this.

Re: remove internal support in pgcrypto?

Andrew Dunstan <andrew@dunslane.net> — 2021-11-01T13:48:32Z

On 8/24/21 08:38, Daniel Gustafsson wrote:
>> On 24 Aug 2021, at 11:13, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>> So I'm tempted to suggest that we remove the built-in, non-OpenSSL cipher and hash implementations in pgcrypto (basically INT_SRCS in pgcrypto/Makefile), and then also pursue the simplifications in the OpenSSL code paths described in [0].
> +1
>
>> Thoughts?
> With src/common/cryptohash_*.c and contrib/pgcrypto we have two abstractions
> for hashing ciphers, should we perhaps retire hashing from pgcrypto altogether
> and pull across what we feel is useful to core (AES and 3DES and..)?  There is
> already significant overlap, and allowing core to only support certain ciphers
> when compiled with OpenSSL isn’t any different from doing it in pgcrypto
> really.
>
>> (Some thoughts from those pursuing NSS support would also be useful.)
> Blowfish and CAST5 are not available in NSS.  I've used the internal Blowfish
> implementation as a fallback in the NSS patch and left CAST5 as not supported.
> This proposal would mean that Blowfish too wasn’t supported in NSS builds, but
> I personally don’t see that as a dealbreaker.
>

Maybe it would be worth creating a non-core extension for things like
this that we are ripping out? I have no idea how many people might be
using them.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: remove internal support in pgcrypto?

Daniel Gustafsson <daniel@yesql.se> — 2021-11-02T10:06:46Z

> On 30 Oct 2021, at 14:11, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> 
> On 24.08.21 11:13, Peter Eisentraut wrote:
>> So I'm tempted to suggest that we remove the built-in, non-OpenSSL cipher and hash implementations in pgcrypto (basically INT_SRCS in pgcrypto/Makefile), and then also pursue the simplifications in the OpenSSL code paths described in [0].
> 
> Here is a patch for this.

+1 on this patch, it does what it says on the tin and lays good foundations for
further work on modernizing pgcrypto.  If anything, maybe the hard OpenSSL
requirement should be advertised earlier in the documentation?

Should we consider bumping the version number of the module?  While it's true
that everyone will have to recompile anyways, and there are no changes in
exposed functionality, it might be an easier sell for those it without OpenSSL
if the version nunber indicates a change.

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

Re: remove internal support in pgcrypto?

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

> On 30 Oct 2021, at 14:11, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> 
> On 24.08.21 11:13, Peter Eisentraut wrote:
>> So I'm tempted to suggest that we remove the built-in, non-OpenSSL cipher and hash implementations in pgcrypto (basically INT_SRCS in pgcrypto/Makefile), and then also pursue the simplifications in the OpenSSL code paths described in [0].
> 
> Here is a patch for this.

This patch doesn't work on Windows, which I think is because it pulls in
pgcrypto even in builds without OpenSSL.  Poking at that led me to realize that
we can simplify even more with this.  The conditonal source includes can go
away and be replaced with a simple OBJS clause, and with that the special hacks
in Mkvcbuild.pm to overcome that.

Attached is a diff on top of your patch to do the above.  I haven't tested it
on Windows yet, but if you think it's in the right direction we'll take it for
a spin in a CI with/without OpenSSL.

Now, *if* we merge the NSS patch this does introduce special cases again which
this rips out.  I prefer to try and fix them in that patch to keep avoiding the
need for them rather than keep them on speculation for a patch which hasn't
been decided on.

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

Re: remove internal support in pgcrypto?

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-11-03T15:06:34Z

On 03.11.21 11:16, Daniel Gustafsson wrote:
>> On 30 Oct 2021, at 14:11, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>>
>> On 24.08.21 11:13, Peter Eisentraut wrote:
>>> So I'm tempted to suggest that we remove the built-in, non-OpenSSL cipher and hash implementations in pgcrypto (basically INT_SRCS in pgcrypto/Makefile), and then also pursue the simplifications in the OpenSSL code paths described in [0].
>>
>> Here is a patch for this.
> 
> This patch doesn't work on Windows, which I think is because it pulls in
> pgcrypto even in builds without OpenSSL.  Poking at that led me to realize that
> we can simplify even more with this.  The conditonal source includes can go
> away and be replaced with a simple OBJS clause, and with that the special hacks
> in Mkvcbuild.pm to overcome that.
> 
> Attached is a diff on top of your patch to do the above.  I haven't tested it
> on Windows yet, but if you think it's in the right direction we'll take it for
> a spin in a CI with/without OpenSSL.

Here is a consolidated patch.  I have tested it locally, so it should be 
okay on Windows.

> Now, *if* we merge the NSS patch this does introduce special cases again which
> this rips out.  I prefer to try and fix them in that patch to keep avoiding the
> need for them rather than keep them on speculation for a patch which hasn't
> been decided on.

Okay, I wasn't sure about the preferred way forward here.  I'm content 
with the approach you have chosen.

Re: remove internal support in pgcrypto?

Daniel Gustafsson <daniel@yesql.se> — 2021-11-03T20:10:25Z

> On 3 Nov 2021, at 16:06, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> 
> On 03.11.21 11:16, Daniel Gustafsson wrote:
>>> On 30 Oct 2021, at 14:11, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>>> 
>>> On 24.08.21 11:13, Peter Eisentraut wrote:
>>>> So I'm tempted to suggest that we remove the built-in, non-OpenSSL cipher and hash implementations in pgcrypto (basically INT_SRCS in pgcrypto/Makefile), and then also pursue the simplifications in the OpenSSL code paths described in [0].
>>> 
>>> Here is a patch for this.
>> This patch doesn't work on Windows, which I think is because it pulls in
>> pgcrypto even in builds without OpenSSL.  Poking at that led me to realize that
>> we can simplify even more with this.  The conditonal source includes can go
>> away and be replaced with a simple OBJS clause, and with that the special hacks
>> in Mkvcbuild.pm to overcome that.
>> Attached is a diff on top of your patch to do the above.  I haven't tested it
>> on Windows yet, but if you think it's in the right direction we'll take it for
>> a spin in a CI with/without OpenSSL.
> 
> Here is a consolidated patch.  I have tested it locally, so it should be okay on Windows.

I don't think this bit is correct, as OSSL_TESTS have been removed from the Makefie:

-			  $config->{openssl}
-			  ? GetTests("OSSL_TESTS", $m)
-			  : GetTests("INT_TESTS",  $m);
+			  GetTests("OSSL_TESTS", $m);

I think we need something like the (untested) diff below:

diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index e3a323b8bf..fc2406b2be 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -729,13 +729,10 @@ sub fetchTests
                        # pgcrypto is special since the tests depend on the
                        # configuration of the build

-                       my $cftests =
-                         GetTests("OSSL_TESTS", $m);
                        my $pgptests =
                          $config->{zlib}
                          ? GetTests("ZLIB_TST",     $m)
                          : GetTests("ZLIB_OFF_TST", $m);
-                       $t =~ s/\$\(CF_TESTS\)/$cftests/;
                        $t =~ s/\$\(CF_PGP_TESTS\)/$pgptests/;
                }
        }

>> Now, *if* we merge the NSS patch this does introduce special cases again which
>> this rips out.  I prefer to try and fix them in that patch to keep avoiding the
>> need for them rather than keep them on speculation for a patch which hasn't
>> been decided on.
> 
> Okay, I wasn't sure about the preferred way forward here.  I'm content with the approach you have chosen.

I'm honestly not sure either; but as the NSS patch author, if I break it I get
to keep both pieces =)

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

Re: remove internal support in pgcrypto?

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-11-05T14:04:26Z

On 03.11.21 21:10, Daniel Gustafsson wrote:
>> Here is a consolidated patch.  I have tested it locally, so it should be okay on Windows.
> 
> I don't think this bit is correct, as OSSL_TESTS have been removed from the Makefie:
> 
> -			  $config->{openssl}
> -			  ? GetTests("OSSL_TESTS", $m)
> -			  : GetTests("INT_TESTS",  $m);
> +			  GetTests("OSSL_TESTS", $m);
> 
> I think we need something like the (untested) diff below:

Committed with that.  Thanks.

Re: remove internal support in pgcrypto?

Daniel Gustafsson <daniel@yesql.se> — 2021-11-05T14:12:00Z

> On 5 Nov 2021, at 15:04, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> 
> On 03.11.21 21:10, Daniel Gustafsson wrote:
>>> Here is a consolidated patch.  I have tested it locally, so it should be okay on Windows.
>> I don't think this bit is correct, as OSSL_TESTS have been removed from the Makefie:
>> -			  $config->{openssl}
>> -			  ? GetTests("OSSL_TESTS", $m)
>> -			  : GetTests("INT_TESTS",  $m);
>> +			  GetTests("OSSL_TESTS", $m);
>> I think we need something like the (untested) diff below:
> 
> Committed with that.  Thanks.

Great! I guess I have some rebasing ahead of me then.

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