Re: Key management with tests - Mailing list pgsql-hackers

On Fri, Jan 29, 2021 at 05:40:37PM -0500, Stephen Frost wrote:
> I hope it's pretty clear that I'm also very much in support of both this
> effort with the KMS and of TDE in general- TDE is specifically,

Yes, thanks.  I know we have privately talked about this recently, but
it is nice to have it in public like this.

> repeatedly, called out as a capability whose lack is blocking PG from
> being able to be used for certain use-cases that it would otherwise be
> well suited for, and that's really unfortunate.

So, below, I am going to copy two doc paragraphs from the patch:

  The purpose of cluster file encryption is to prevent users with read
  access to the directories used to store database files and write-ahead
  log files from being able to access the data stored in those files.
  For example, when using cluster file encryption, users who have read
  access to the cluster directories for backup purposes will not be able
  to decrypt the data stored in these files.  It also protects against
  decrypted data access after media theft.

  File system write access can allow for unauthorized file system data
  decryption if the writes can be used to weaken the system's security
  and this weakened system is later supplied with externally-stored keys.
  This also does not protect from users who have read access to system
  memory.  This also does not detect or protect against users with write
  access from removing or modifying database files.

Given what I said above, is the value of this feature for compliance, or
for actual additional security?  If it just compliance, are we willing
to add all of this code just for that, even if it has limited security
value?  We should answer this question now, and if we don't want it,
let's document that so users know and can consider alternatives.

FYI, I don't think we can detect or protect against writers modifying
the data files --- even if we could do it on a block level, they could
remove trailing pages (might cause index lookup failures) or copy
pages from other tables at the same offset.  Therefore, I think we can
only offer viewing security, not modification detection/prevention.

> I appreciate the recent discussion and reviews of the KMS in particular,
> and of the patches which have been sent enabling TDE based on the KMS
> patches.  Having them be relatively independent seems to be an ongoing

I was thinking some more and I have received productive feedback from at
least eight people on the key management patch, which is very good.

> concern and perhaps we should figure out a way to more clearly put them
> together.  That is- the KMS patches have been posted on one thread, and
> TDE PoC patches which use the KMS patches have been on another thread,
> leading some to not realize that there's already been TDE PoC work done
> based on the KMS patches.  Seems like it might make sense to get one
> patch set which goes all the way from the KMS and includes the TDE PoC,
> even if they don't all go in at once.

Uh, it is worse than that.  Some people saw comments about the TDE PoC
patch (e.g., buffer pins) and thought they were related to the KMS
patch, so they thought the KMS patch wasn't ready.  Now, I am not saying
the KMS patch is ready, but comments on the TDE PoC patch are unrelated
to the KMS patch being ready.

I think the TDE PoC was a big positive because it showed the KMS patch
being used for the actual use-case we are planning, so it was truly a
proof-of-concept.

> I'm happy to go look over the KMS patches again if that'd be helpful and
> to comment on the TDE PoC.  I can also spend some time trying to improve

I think we eventually need a full review of the TDE PoC, combined with
the Cybertec patch, and the wiki, to get them all aligned.  However, as
I said already, let's get the KMS patch approved, even if we don't apply
it now, so we know we are on an approved foundation.

> on each, as I've already done.  A few of the larger concerns that I have
> revolve around how to store integrity information (I've tried to find a
> way to make room for such information in our existing page layout and,
> perhaps unsuprisingly, it's far from trivial to do so in a way that will
> avoid breaking the existing page layout, or where the same set of
> binaries could work on both unencrypted pages and encrypted pages with
> integrity validation information, and that's a problem that we really

As stated above, I think we only need a byte or two for the hint bit
counter (used in the IV), as I don't think the GCM verification bytes
will add any additional security, and I bet we can find a byte or two. 
We do need a separate discussion on this, either here or privately.

> should consider trying to solve...), and how to automate key rotation
> (one of the nice things about Bruce's approach to storing the keys is
> that we're leveraging the filesystem as an index- it's easy to see how
> we might extend the key-per-file approach to allow us to, say, have a
> different key for every 32GB of LSN, but if we tried to put all of the
> keys into a single file then we'd have to figure out an indexing
> solution for it which would allow us to find the key we need to decrypt
> a given page...).  I tend to agree with Bruce that we need to take

Yeah, yuck on that plan.  I was very happy how the per-version directory
worked with scripts that needed to store matching state.

> these things in steps, getting each piece implemented as we go.  Maybe
> we can do that in a separate repo for a time and then bring it all
> together, as a few on this thread have voiced, but there's no doubt that
> this is a large project and it's hard to see how we could possibly
> commit all of it at once.

I was putting stuff in a git tree/URL;  you can see it here:

   https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
   https://github.com/postgres/postgres/compare/master...bmomjian:key.patch
   https://github.com/postgres/postgres/compare/master...bmomjian:key

However, people wanted persistent patches attached, so I started doing that.
Attached is the current patch set.

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

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