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

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@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

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