Re: Internal key management system

On Tue, 27 Oct 2020 at 20:34, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
> On Tue, Oct 27, 2020 at 03:07:22PM +0800, Craig Ringer wrote:
> > On Mon, Oct 26, 2020 at 11:02 PM Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >
> >
> > TL;DR:
> >
> > * Important to check that key rotation is possible on a replica, i.e.
> > primary and standby can have different cluster passphrase and KEK
> > encrypting the same WAL and heap keys;
> > * with a HSM we can't read the key out, so a pluggable KEK operations
> > context or a configurable URI for the KEK is necessary
> > * I want the SQL key and SQL wrap/unwrap part in a separate patch, I
> > don't think it's fully baked and oppose its inclusion in its current
> > form
> > * Otherwise this looks good so far
> >
> > Explanation and argument for why below.
> >
> > > I've not been following very closely, but I definitely agree with the
> > > general feedback here (more on that below), but to this point- I do
> > > believe that was the intent, or at least I sure hope that it was. Being
> > > able to have user/role keys would certainly be good. Having a way for a
> > > user to log in and unlock their key would also be really nice.
> >
> > Right. AFAICS this is supposed to provide the foundation layer for
> > whole-cluster encryption, and it looks ok for that, caveat about HSMs
> > aside. I see nothing wrong with using a single key for heap (and one
> > for WAL, or even the same key). Finer grained and autovacuum etc
> > becomes seriously painful.
>
> You need to use separate keys for heap/index and WAL so you can
> replicate to another server that uses a different heap/index key, but
> the same WAL. You can then fail-over to the replica and change the WAL
> key to complete full key rotation. The replication protocol needs to
> decrypt, and the receiving end has to encrypt with a different
> heap/index key. This is the key rotation method this is planned. This
> is another good reason the keys should be in a separate directory so
> they can be easily copied or replaced.

I think it's better we decrypt WAL in the xlogreader layer, instead of
doing in replication protocol. That way, we also can support frontend
tools that need to read WAL such as pg_waldump and pg_rewind as well
as logical decoding.

>
> > I want to take a closer look at how the current implementation will
> > play with physical replication. I assume the WAL and heap keys have to
> > be constant for the full cluster lifetime, short of a dump and reload.
>
> The WAL key can change if you are willing to stop/start the server. You
> only read the WAL during crash recovery.

We might need to consider having multiple key generations, rather than
in-place rotation. If we simply update the WAL key in-place in the
primary, archive WALs restored via restore_command cannot be decrypted
in the replica. We might need to do generation management for WAL key
and provide the functionality to purge old WAL keys.

> >
> > SQL KEY
> > ----
> >
> > I'm not against the SQL key and wrap/unwrap functionality - quite the
> > contrary, I think it's really important to have something like it. But
> > is it appropriate to have a single, fixed-for-cluster-lifetime key for
> > this, one with no SQL-level access control over who can or cannot use
> > it, etc? The material encrypted with the key is user-exposed so key
> > rotation is an issue, but is not addressed here. And the interface
> > doesn't really solve the numerous potential problems with key material
> > leaks through logs and error messages.
> >
> > I just think that if we bake in the proposed user visible wrap/unwrap
> > interface now we're going to regret it later. How will it work when we
> > want to add user- or role- level access control for database-stored
> > keys? When we want to introduce a C-level API for extensions to work
> > directly with encrypted data like they can work currently with TOASTed
> > data, to prevent decrypted data from ever becoming SQL function
> > arguments subject to possible leakage and to allow implementation of
> > always-encrypted data types, etc?
> >
> > Most importantly - I don't think the SQL key adds anything really
> > crucial that we cannot do at the SQL level with an extension. An
> > extension "pg_wrap" could provide pg_wrap() and pg_unwrap() already,
> > using a single master key much like the SQL key proposed in this
> > patch. To store the master key it could:
>
> The idea of the SQL key was to give the boot key a use, but I am now
> seeing that the SQL key is just holding us back, and is preventing the
> boot testing that is a requirement. Maybe we just need to forget the
> SQL part and focus on the TDE usage now, and come back to the SQL part.
> I am also not 100% clear on the usefulness of the SQL key.

I agree to focus on the TDE usage now.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services