Re: Proposed patch for key managment

On Tue, Dec 15, 2020 at 10:05:49AM +0900, Michael Paquier wrote:
> 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.

Uh, pg_altercpass is a new file I wrote and almost every block has a
comment. I added a few more, but can you be more specific?

> - There are no changes to src/tools/msvc/. Seeing the diffs in
> src/common/, this stuff breaks Windows builds.

OK, done in patch at URL.

> - 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 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.

OK, I will try that.

> > 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?

I have posted some of the scripts. They involved complex shell
scripting that I doubt the average user can do. The scripts are for
prompting for a passphrase from the user's terminal, or using the
Yubikey, with our without typing a pin. I can put them in the docs and
then users can copy them into a file. Is that the preferred method?

> > 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?

The point is that the command-line options will be active, but will not
be documented. It will not do anything on its own unless you specify
that command-line option. I can comment-out the command-line options
from being active but the code it going to look very messy.

> > 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.

Well, the patches are changing frequently. I am attaching a version to
this email.

> +/*
> + * 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.

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

> @@ -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.

Oh, good point. Fixed.

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

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee