Re: Proposed patch for key managment - Mailing list pgsql-hackers

On Mon, Dec 14, 2020 at 06:06:15PM -0500, Bruce Momjian wrote:
> I am getting close to applying these patches, probably this week.  The
> patches are cumulative:
>
>     https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
>     https://github.com/bmomjian/postgres/compare/key...bmomjian:key-alter.diff

I strongly object to a commit based on the current state of the patch.
Based on my lookup of the patches you are referring to, I see a couple
of things that should be splitted up and refactored properly before
thinking about the core part of the patch (FWIW, I don't have much
thoughts to offer about the core part because I haven't really thought
about it, but it does not prevent to do a correct refactoring).  Here
are some notes:
- The code lacks a lot of comments IMO.  Why is retrieve_passphrase()
doing what it does?  It seems to me that pg_altercpass needs a large
brushup.
- There are no changes to src/tools/msvc/.  Seeing the diffs in
src/common/, this stuff breaks Windows builds.
- 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 do have a few questions:

That looks like a lot of things to sort out as well.

>     Can anyone test this on Windows, particularly -R handling?
>
>     What testing infrastructure should this have?

Using TAP tests would be adapted for those two points.

>     There are a few shell script I should include to show how to create
>     commands.  Where should they be stored?  /contrib module?

Why are these needed.  Is it a matter of documentation?

>     Are people okay with having the feature enabled, but invisible
>     since the docs and --help output are missing?  When we enable
>     ssl_passphrase_command to prompt from the terminal, some of the
>     command-line options will be useful.

You are suggesting to enable the feature by default, make it invisible
to the users and leave it undocumented?  Is there something I missing
here?

>     Do people like the command-letter choices?
>
>     I called the alter passphrase utility pg_altercpass.  I could
>     have called it pg_clusterpass, but I wanted to highlight it is
>     only for changing the passphrase, not for creating them.

I think that you should attach such patches directly to the emails
sent to pgsql-hackers, if those github links disappear for some
reason, it would become impossible to refer to see what has been
discussed here.

+/*
+ * We have to use postgres.h not postgres_fe.h here, because there's so much
+ * backend-only stuff in the kmgr include files we need.  But we need a
+ * frontend-ish environment otherwise.  Hence this ugly hack.
+ */
+#define FRONTEND 1
+
+#include "postgres.h"
I would advise to really understand why this happens and split up the
backend and frontend parts cleanly.  This style ought to be avoided as
much as possible.

@@ -95,9 +101,9 @@ pg_cryptohash_create(pg_cryptohash_type type)

    if (state->evpctx == NULL)
    {
+#ifndef FRONTEND
        explicit_bzero(state, sizeof(pg_cryptohash_state));
    explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
-#ifndef FRONTEND
I think that this change is incorrect.  Any clean up of memory should
be done for the frontend *and* the backend.

+#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.
--
Michael