Re: Internal key management system

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

> The main issue I have so far is that I don't think the SQL key
> actually fits well with the current proposal. Its proposed interface
> and use cases are incomplete, it doesn't fully address key leak risks,
> there's no user access control, etc. Also the SQL key part could be
> implemented on top of the base cluster encryption part, I don't see
> why it actually has to integrate with the whole-cluster key management
> directly.

Agreed.  Maybe we should just focus on the TDE use now.  I do think the
current patch is not commitable since, because there are no defined
keys, there is no way to validate the boot-time password.  The no-key
case should be an unsupported configuration.  Maybe we need to just
create one key just to verify the boot password.

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

> OTHER TRANSPARENT ENCRYPTION USE CASES
> ----
> 
> Does this patch get in the way of supporting other kinds of
> transparent encryption that are frequently requested and are in use on
> other systems already?
> 
> I don't think so. Whole-cluster encryption is quite separate and the
> proposed patch doesn't seem to do anything that'd make table-, row- or
> column-level encryption, per-user key management, etc any harder.

I think those all are very different and will require more user-level
features that what is being done here.

> Specific use cases I looked at:
> 
> * Finer grained keying than whole-cluster for transparent
> encryption-at-rest. As soon as we have relations that require user
> session supplied information to allow the backend to read the relation
> we get into a real mess with autovacuum, logical decoding, etc. So if
> anyone wants to implement that sorts of thing they're probably going
> to want to do so separately to block-level whole-cluster encryption,
> in a way that preserves the normal page and page item structure and
> encrypts the row data only.

Agreed.

> * Client-driver-assisted transparently encrypted
> at-rest-and-in-transit data, where the database engine doesn't have
> the encrypt/decrypt keys at all. Again in this case they're going to
> have to do that at the row level or column level, not the block
> (relfilenode extents and WAL) level, otherwise we can't provide
> autovacuum etc.

Yes, this is all going to have to be user-level.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

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