Re: Proposed patch for key managment

On Mon, Dec 14, 2020 at 10:19:02PM -0500, Bruce Momjian wrote:
> On Tue, Dec 15, 2020 at 10:05:49AM +0900, Michael Paquier wrote:
>> - The HMAC split is terrible, as mentioned upthread. I think that you
>> would need to extract this stuff as a separate patch, and not add more
>> mess to the existing mess (SCRAM uses its own HMAC with SHA256, which
>> is already wrong). We can and should have a fallback implementation,
>> because that's easy to do. And we need to have the fallback
>> implementation depend on the contents of cryptohash.c (in a
>> src/common/hmac.c), while the OpenSSL portion requires a
>> hmac_openssl.c where you can choose the hash type based on
>> pg_cryptohash_type. So ossl_HMAC_SHA512() does not do the right
>> thing. APIs flexible enough to allow a new SSL library to plug into
>> this stuff are required.
>> - Not much a fan of the changes done in cryptohash.c for the resource
>> owners as well. It also feels like this could be thought as something
>> directly for resowner.c.
>> - The cipher split also should be done in its own patch, and reviewed
>> on its own. There is a mix of dependencies between non-OpenSSL and
>> OpenSSL code paths, making the pluggin of a new SSL library harder to
>> do. In short, this requires proper API design, which is not something
>> we have here. There are also no changes in pgcrypto for that stuff.
>
> I am going to need someone to help me make these changes. I don't feel
> I know enough about the crypto API to do it, and it will take me 1+ week
> to learn it.

I think that designing a correct set of APIs that can be plugged with
any SSL library is the correct move in the long term. I have on my
agenda to clean up HMAC as SCRAM uses that with SHA256 and you would
use that with SHA512. Daniel has mentioned that he has been touching
this area, and I also got a patch halfly done though pgcrypto needs
some extra thoughts. So this is still WIP but you could reuse that
here.

> Uh, I got this code from pg_resetwal.c, which does something similar to
> pg_altercpass.

Yeah, that's actually the part where it is a bad idea to just copy
this pattern. pg_resetwal should not do that in the long term in my
opinion, but nobody has come to clean up this stuff. (- -;)

>> +#ifdef USE_OPENSSL
>> + ctx = (PgCipherCtx *) palloc0(sizeof(PgCipherCtx));
>> +
>> + ctx->encctx = ossl_cipher_ctx_create(cipher, key, klen, true);
>> + ctx->decctx = ossl_cipher_ctx_create(cipher, key, klen, false);
>> +#endif
>> There is a cipher_openssl.c and a cipher.c that includes USE_OPENSSL.
>> This requires a cleaner split IMO. We should avoid as much as
>> possible OpenSSL-specific code paths in files part of src/common/ when
>> not building with OpenSSL. So this is now a mixed bag of
>> dependencies.
>
> Again, I need help here.

My take would be to try to sort out the HMAC part first, then look at
the ciphers. One step at a time.
--
Michael