Thread: remove internal support in pgcrypto?
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
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
Attachment
> 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 (basicallyINT_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/
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.
Attachment
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 (basicallyINT_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
> 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 (basicallyINT_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/
> 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 (basicallyINT_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/
Attachment
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 (basicallyINT_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.
Attachment
> 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
(basicallyINT_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/
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.
> 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/