Re: Internal key management system

On Wed, Oct 28, 2020 at 12:02 PM Craig Ringer <craig(dot)ringer(at)enterprisedb(dot)com>
wrote:

> On Wed, Oct 28, 2020 at 9:43 AM Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >
>
>> I don't know much about how to hook into that stuff so if you have an
>> idea, I am all ears.
>
>
> Yeah, I have a reasonable idea. The main thing will be to re-read the
> patch and put it into more concrete terms, which I'll try to find time for
> soon. I need to find time to craft a proper demo that uses a virtual hsm,
> and can also demonstrate how to use the host TPM or a Yubikey using the
> simple openssl engine interfaces or a URI.
>

Do you have this in a public git tree anywhere? If not please consider
using "git format-patch -v 1 -1" or similar to generate it, so I can "git
am" the patch.

A few comments on the patch as I read through. Some will be addressed by
the quick PoC I'm preparing for pluggable key derivation, some won't. In no
particular order:

* The term KEK only appears in the docs; where it appears in the sources
it's lower case. I suggest making "KEK" grep-able in the sources.
* BootStrapKmgr() says to call once on "system install" . I suggest
"initdb".
* The jumble of #ifdef FRONTEND in src/common/kmgr_utils.c shouldn't remain
in the final patch if possible. This may require some "static inline"
wrappers or helpers.
* PgKeyWrapCtx.key is unused and should probably be deleted
* HMAC support should go into PgCipherCtx so that we can have a single
PgCipherCtx that supports cipher ops, cipher+HMAC ops, or just HMAC ops
* PgKeyWrapCtx.cipherctx should then be supplemented with a hmacctx. It
should be legal to set cipherctx and hmacctx to the same value, since in
some cases the it won't be easy to initialize the backing implementation
separately for key and HMAC.

The patch I've been hacking together will look like this, though I haven't
got far along with it yet:

* Give each PgKeyWrapCtx a 'key_name' field to identify it
* Extract default passphrase based KEK creation into separate function that
returns
a new PgKeyWrapCtx for the KEK, currently called
kmgr_create_kek_context_from_cluster_passphrase()
* BootStrapKmgr() and pg_rotate_cluster_passphrase() call
kmgr_create_kek_context_from_cluster_passphrase()
instead of doing their own ctx creation
* [TODO] Replace kmgr_verify_passphrase() with kmgr_verify_ctx(...)
which takes a PgKeyWrapCtx instead of constructing its own from a
passphrase
* [TODO] In InitializeKmgr() use kmgr_verify_ctx() instead of explicit
passphrase fetch
* [TODO] Teach PgCipherCtx about HMAC operations
* [TODO] replace PgKeyWrapCtx.mackey with another PgCipherCtx
* [TODO] add PgKeyWrapCtx.teardown_cb callback to be called before free
* [TODO] add a kmgr_create_kek_context() that checks for a hook/plugin
or other means of loading a non-default means of getting a KEK
PgKeyWrapContext, and calls
kmgr_create_kek_context_from_cluster_passphrase()
by default
* [TODO] replace calls to kmgr_create_kek_context_from_cluster_passphrase()
with calls to kmgr_create_kek_context()

That should hide the details of HMAC operations and of KEK creation from
kmgr_* .

Then via a TBD configuration mechanism we'd be able to select a method of
creating the PgKeyWrapCtx for the KEK and its contained PgCipherCtx
implementations for cipher and HMAC operations, then use that without
caring about how it works internally.

The key manager no longer has to care if the KEK was created by reading a
password from a command and deriving the KEK and HMAC. Or whether it's
actually backed by an OpenSSL engine that delegates to PKCS#11. kmgr ops
can just request the KEK context and use it.

FORK?
----

One possible problem with this is that we should not assume we can perform
KEK operations in postmaster children, since there's no guarantee we can
use whatever sits behind a PgCipherCtx after fork(). But AFAICS the KEK
doesn't live beyond the various kmgr_ operations as it is, so there's no
reason it should ever have to be carried over a fork() anyway.

CONFIGURING SOURCE OF KEK
---

Re the configuration mechanism: the usual way Pg does things is do provide
a global foo_hook_type foo_hook. The foo() function checks for foo_hook and
calls it if it's non-null, otherwise it calls the default implementation in
standard_foo(). A hook may choose to override standard_foo() completely, or
take its own actions before or after. The hook is installed by an extension
loaded in shared_preload_libraries.

There are a couple of issues with using this method for kmgr:

* the kmgr appears to need to be able to work in frontend code (?)
* for key rotation we need to be able to change KEKs, and possibly KEK
acquisition methods, at runtime

so I'm inclined to handle this a bit like we do for logical decoding output
plugins instead. Use a normal Pg extension library with PG_MODULE_MAGIC,
but dlsym() a different entrypoint. Have that entrypoint populate a struct
of API function pointers. The kmgr can use that struct to request KEK
loading. If no kmgr plugin is configured, use the default API struct that
does KEK loading based on password.

When re-keying, we'd (re)load the kmgr KEK library, possibly a different
one to that used at startup, or if the user switched to the default method
we'd use the default API struct.

To the user this would probably look like

kmgr_plugin = 'kmgr_openssl_engine'
kmgr_openssl_engine.key_uri = 'pkcs11:foo;bar;baz'

or however else we feel like spelling it.

Reasonable?