Thread: pgsql: Move SHA2 routines to a new generic API layer for crypto hashes
Move SHA2 routines to a new generic API layer for crypto hashes Two new routines to allocate a hash context and to free it are created, as these become necessary for the goal behind this refactoring: switch the all cryptohash implementations for OpenSSL to use EVP (for FIPS and also because upstream does not recommend the use of low-level cryptohash functions for 20 years). Note that OpenSSL hides the internals of cryptohash contexts since 1.1.0, so it is necessary to leave the allocation to OpenSSL itself, explaining the need for those two new routines. This part is going to require more work to properly track hash contexts with resource owners, but this not introduced here. Still, this refactoring makes the move possible. This reduces the number of routines for all SHA2 implementations from twelve (SHA{224,256,386,512} with init, update and final calls) to five (create, free, init, update and final calls) by incorporating the hash type directly into the hash context data. The new cryptohash routines are moved to a new file, called cryptohash.c for the fallback implementations, with SHA2 specifics becoming a part internal to src/common/. OpenSSL specifics are part of cryptohash_openssl.c. This infrastructure is usable for more hash types, like MD5 or HMAC. Any code paths using the internal SHA2 routines are adapted to report correctly errors, which are most of the changes of this commit. The zones mostly impacted are checksum manifests, libpq and SCRAM. Note that e21cbb4 was a first attempt to switch SHA2 to EVP, but it lacked the refactoring needed for libpq, as done here. This patch has been tested on Linux and Windows, with and without OpenSSL, and down to 1.0.1, the oldest version supported on HEAD. Author: Michael Paquier Reviewed-by: Daniel Gustafsson Discussion: https://postgr.es/m/20200924025314.GE7405@paquier.xyz Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/87ae9691d25379785f8c0f81b06a14818cfd8c56 Modified Files -------------- contrib/pgcrypto/internal-sha2.c | 188 +++++++--------------------- src/backend/libpq/auth-scram.c | 113 ++++++++++------- src/backend/replication/backup_manifest.c | 25 +++- src/backend/replication/basebackup.c | 24 ++-- src/backend/utils/adt/cryptohashes.c | 53 +++++--- src/bin/pg_verifybackup/parse_manifest.c | 15 ++- src/bin/pg_verifybackup/pg_verifybackup.c | 24 +++- src/common/Makefile | 6 +- src/common/checksum_helper.c | 79 +++++++++--- src/common/cryptohash.c | 190 ++++++++++++++++++++++++++++ src/common/cryptohash_openssl.c | 197 ++++++++++++++++++++++++++++++ src/common/scram-common.c | 175 ++++++++++++++++++-------- src/common/sha2.c | 23 +++- src/common/sha2_int.h | 91 ++++++++++++++ src/common/sha2_openssl.c | 102 ---------------- src/include/common/checksum_helper.h | 13 +- src/include/common/cryptohash.h | 40 ++++++ src/include/common/scram-common.h | 17 +-- src/include/common/sha2.h | 89 +------------- src/include/replication/backup_manifest.h | 3 +- src/interfaces/libpq/fe-auth-scram.c | 116 ++++++++++-------- src/tools/msvc/Mkvcbuild.pm | 3 +- src/tools/pgindent/typedefs.list | 2 + 23 files changed, 1039 insertions(+), 549 deletions(-)
On 2020/12/02 10:41, Michael Paquier wrote: > Move SHA2 routines to a new generic API layer for crypto hashes Thanks for improving this! I got the following compiler failure. ISTM that the variable "status" in both pg_cryptohash_update() and pg_cryptohash_final() needs to be initialized with 0. Patch attached. Thought? cryptohash_openssl.c: In function ‘pg_cryptohash_update’: cryptohash_openssl.c:144:5: error: ‘status’ may be used uninitialized in this function [-Werror=maybe-uninitialized] if (status <= 0) ^ cryptohash_openssl.c: In function ‘pg_cryptohash_final’: cryptohash_openssl.c:179:5: error: ‘status’ may be used uninitialized in this function [-Werror=maybe-uninitialized] if (status <= 0) ^ cc1: all warnings being treated as errors make[2]: *** [cryptohash_openssl.o] Error 1 <builtin>: recipe for target 'cryptohash_openssl.o' failed Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Attachment
Re: pgsql: Move SHA2 routines to a new generic API layer for crypto hashes
From
Michael Paquier
Date:
On Wed, Dec 02, 2020 at 11:42:58AM +0900, Fujii Masao wrote: > On 2020/12/02 10:41, Michael Paquier wrote: >> Move SHA2 routines to a new generic API layer for crypto hashes > > Thanks for improving this! > I got the following compiler failure. ISTM that the variable "status" > in both pg_cryptohash_update() and pg_cryptohash_final() needs > to be initialized with 0. Patch attached. Thought? Neither gcc nor clang complain on my end, even with -Werror=maybe-uninitialized, because they understand that the status will be set anyway as we have no default in the switch? :) You are right, this is bad practice for those code paths, so let's fix it. Which version are you using? I'd like to be able to reproduce that and double-check if there are more missing spots. -- Michael
Attachment
Re: pgsql: Move SHA2 routines to a new generic API layer for crypto hashes
From
Michael Paquier
Date:
On Wed, Dec 02, 2020 at 12:14:33PM +0900, Michael Paquier wrote: > Neither gcc nor clang complain on my end, even with > -Werror=maybe-uninitialized, because they understand that the status > will be set anyway as we have no default in the switch? :) > > You are right, this is bad practice for those code paths, so let's fix > it. Which version are you using? I'd like to be able to reproduce > that and double-check if there are more missing spots. Hmm. Forget that. I can see the warnings once I switched to -O2, which is what I guess you are doing. Some animals are also showing that. I'll fix in a minute. Thanks for the report! -- Michael
Attachment
On 2020/12/02 12:21, Michael Paquier wrote: > On Wed, Dec 02, 2020 at 12:14:33PM +0900, Michael Paquier wrote: >> Neither gcc nor clang complain on my end, even with >> -Werror=maybe-uninitialized, because they understand that the status >> will be set anyway as we have no default in the switch? :) >> >> You are right, this is bad practice for those code paths, so let's fix >> it. Which version are you using? I'd like to be able to reproduce >> that and double-check if there are more missing spots. > > Hmm. Forget that. I can see the warnings once I switched to -O2, > which is what I guess you are doing. Some animals are also showing > that. I'll fix in a minute. Thanks for the report! Thanks! FWIW, I got that failure when I built PostgreSQL in github actions using yml file [1]. The default setting -O2 is used there. Since -Werror is used, I got the compiler failure than warning. [1] https://github.com/MasaoFujii/my-bin/blob/master/data/build.yml Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION