Re: Proposed patch for key managment

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