Re: Proposed patch for key managment

On Fri, Dec 04, 2020 at 09:08:03PM -0500, Bruce Momjian wrote:
> Here is an updated patch to handle the new hash API introduced by
> commit 87ae9691d2.

+ if (!ossl_initialized)
+ {
+#ifdef HAVE_OPENSSL_INIT_SSL
+ OPENSSL_init_ssl(OPENSSL_INIT_LOAD_CONFIG, NULL);
+#else
+ OPENSSL_config(NULL);
+ SSL_library_init();
+ SSL_load_error_strings();
+#endif
+ ossl_initialized = true;
This is a duplicate of what's done in be-secure-openssl.c, and it does
not strike me as a good idea to do that potentially twice.

git diff --check complains.

+extern bool pg_HMAC_SHA512(const uint8 *key,
+ const uint8 *in, int inlen,
+ uint8 *out);
I think that the split done in this patch makes the HMAC handling in
the core code messier:
- SCRAM makes use of HMAC internally, and we should try to use the
HMAC of OpenSSL if building with it even for SCRAM.
- For the first reason, I think that we should also have a fallback
implementation.
- This API layer should not depend directly on the SHA2 used (SCRAM
uses SHA256 with HMAC).
FWIW, I got plans to work on that once I am done with the business
around MD5 and OpenSSL.

The refactoring done with the ciphers moved from pgcrypto to
src/common/ should be a separate patch. In short, it would be good to
rework this patch and split it into pieces that are independently
useful. This would make the review much easier as well.
--
Michael