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

On Sat, Jan  2, 2021 at 10:50:15AM +0100, Fabien COELHO wrote:
> > No, this isn't anything like the Kerberos model and there's no case
> > where the PG account won't have access to the DEK (data encryption key)
> > during normal operation (except with the possibility of off-loading to a
> > hardware crypto device which, while interesting, is definitely something
> > that's of very limited utility and which could be added on as a
> > capability later anyway.  This is also well understood by those who are
> > interested in this capability and it's perfectly acceptable that PG will
> > have, in memory, the DEK.
> 
> I'm sorry that I'm not very clear. My ranting is having a KEK "master key"
> in pg memory. I think I'm fine at this point with having DEKs available on
> the host to code/decode files. What I meant about kerberos is that the setup
> I was describing was making the database dependent on a remote host, which
> is somehow what keroberos does.

We bzero the master key once we are done with it in the server.  Why is
having the master key in memory for a short time while a problem while
having the data keys always in memory acceptable?  Aren't the data keys
more valuable, and hence they fact the master key is in memory for a
short time no additional risk?

> > The keys which are stored on disk with the proposed patch are encrypted
> > using a KEK which is external to PG- that's all part of the existing
> > patch and design.
> 
> Yep. My point is that the patch hardwires a cryptographic design with many
> assumptions, whereas I think it should design an API compatible with a
> larger class of designs, and possibly implement one with KEK and so on, I'm
> fine with that.

You are not addressing my complexity/fragility reasons for having a
single method.  As stated above, this feature has limited usefulness,
and if it breaks or you lose the KEK, your database server and backups
might be gone.

> > so it's unclear to me what the argument here is.
> 
> The argument is that professional cryptophers do wrong designs, and I do not
> see why you would do different. So instead of hardwiring choice that you
> think are THE good ones, ISTM that pg should design a reasonably flexible
> API, and also provide an implementation, obviously.

See above.

> > Further, we haven't even gotten to actual cluster encryption yet- this
> > is all just the key management side of things,
> 
> With hardwired choices about 1 KEK and 2 DEK that are debatable, see below.

Then let's debate them, since this patch isn't moving forward as long as
people are asking questions about the implementation.

> > > I'm not sure of the design of the key rotation stuff as I recall it from the
> > > patches I looked at, but the API design looks like the more pressing issue.
> > > First, the mere existence of an "master key" is a cryptographic choice which
> > > is debatable, because it creates issues if one must be able to change it,
> > > hence the whole "key rotation" stuff. ISTM that what core needs to know is
> > > that one particular key is changed by a new one and the underlying file is
> > > rewritten. It should not need to know anything about master keys and key
> > > derivation at all. It should need to know that the user wants to change file
> > > keys, though.
> > 
> > No, it's not debatable that a KEK is needed, I disagree entirely.
> 
> ISTM that there is cryptographic design which suits your needs and you seem
> to decide that it is the only possible way to do it It seems that you know.
> As a researcher, I know that I do not know:-) As a software engineer, I'm
> trying to suggest enabling more with the patch, including not hardwiring a
> three-key management scheme.

Well, your open API goal might work, but I am not comfortable
implementing that for reasons already explained, so what do we do?

> I'm advocating that you pg not decide for others what is best for them, but
> rather allow multiple choices and implementations through an API. Note that
> I'm not claiming that the 1 KEK + 2 DEK + AES… design is bad. I'm claiming
> that it is just one possible design, quite peculiar though, and that pg
> should not hardwire it, and it does not need to anyway.
> 
> The documentation included in the key management patch states: "Key zero is
> the key used to encrypt database heap and index files which are stored in
> the file system, plus temporary files created during database operation."
> Read as is, it suggests that the same key is used to encrypt many files.
> From a cryptographic point of view this looks like a bad idea. The mode used
> is unclear. If this is a stream cypher generated in counter mode, it could
> be a very bad idea. Hopefully this is not what is intended, but the included
> documentation is frankly misleading in that case. IMHO there should be one
> key per file. Also, the CTR mode should be avoided if possible because it
> has quite special properties, unless these properties are absolutely
> necessary.

We need CTR since the WAL and even data blocks are not encrypted in full
blocks.  We are using GCM for key validation and tamper detection.

What is the value of one key per file?  It means every CREATE TABLE
needs a new data key, and if we lose any key, we lose some data.  I
would rather walk away from this feature rather than implement this.

This gets to a general thread in this discussion of adding complexity
and fragility without a clear explanation of the security value of
adding it.  This is not new --- we created private voice meetings to
avoid the distraction of such suggestions, but now that the patch is
being considered, that isn't possible anymore.

> > From what I see in the patch, the DEKs cannot be changed, only the KEK
> which encodes de DEK. I cannot say I'm thrilled by such a property. If the
> KEK is lost, then probably the DEKs are lost as well. Changing the KEK does

Yes.

> not change anything about it, so that the attacker would be able to retrieve
> the contents of later/previous state of the database if they can retrieve a
> previous/later encoded version, even if the KEK is changed every day. This
> is a bad property which somehow provides a false sense of security. I'm not
> sure that it makes much sense to change a KEK without changing the
> associated DEK, security-wise.

Well, this is a common security feature.  You might have your KMS
compromised, but if you change your KEK before you are attacked, or if
someone leaves the organization, you get some security value.  However,
you are right that if you can access a DEK wrapped with an old KEK, you
can read the data keys, and the DEK will work with the new cluster.

> Anyway, I'd suggest to provide a script to re-encrypt a cluster, possibly
> while offline, with different DEKs. From an audit perspective, this could be
> a requirement.

We already have that documented in the wiki that it will use a standby
for key rotation, though I have heard someone suggest having LSN-based
keys that can change as transactions increment.

> I'd expect a very detailed README or documentation somewhere, but there are
> none in the committed/uncommitted patch.

For what?  They key management?  We don't have the data encryption part
done yet.

> In https://wiki.postgresql.org/wiki/Transparent_Data_Encryption, I see a lot
> of minute cryptographic design decision which are debatable, and a few which
> seem to be still opened, whereas other things are already in the process of
> being committed. I'm annoyed by the decision to reuse the same key for a lot
> of encryption, even if there are discussions about changing the iv in some
> way. If you have some clever way to build an iv, then please please change
> the key as well?

Why?  See above.  Adding complexity for unclear security value isn't
helpful.

> > All of this hand-waving really isn't helping.
> 
> I'm sorry that you feel that. I'm really trying to help improve the approach
> and design with my expertise. As I said in another mail, I'm no one, you can
> always chose to ignore me.

No, we can't.  People read this thread, conclude the implementation path
is unclear, and assume these issues should be resolved before moving
forward.  I am frankly leaning in the TDE-not-wanted direction based on
discussions here.   Frankly, I am happier with that than to try to push
this rock up hill for several months.

> > If it's a small addition to the current code then it'd be fantastic if
> > you'd propose a specific patch which adds what you're suggesting.
> 
> My point is to discuss an API.
> 
> Once there is some agreement the possible API outline, then implementations
> can be provided, but if people do not want an API in the first place, I'm
> not sure I can see the point of sending patches.

I think you are accurate here.  You can certainly propose an external
API, but I think any external API would have more negatives (complexity,
fragility) than security positives.  Any discussion that ignores the
negatives is unlikely to be adopted.

Perhaps someone proposes an external API, and I, and perhaps others,
complain about its negatives, so it isn't adopted.  So the internal key
management is rejected, the external API is rejected, so we do nothing.
I feel that is where we are now.  Marking this feature as not wanted
would at least eliminate discussions that will never lead to any value.
Another option would be to move forward without complete agreement.

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

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