Subject: Re: Moving forward with TDE [PATCH v3]

From: David Christensen <[email-masked]>
Date: Tue, 31 Oct 2023 16:23:17 -0500

Greetings,

I am including an updated version of this patch series; it has been rebased
onto 6ec62b7799 and reworked somewhat.

The patches are as follows:

0001 - doc updates
0002 - Basic key management and cipher support
0003 - Backend-related changes to support heap encryption
0004 - modifications to bin tools and programs to manage key rotation and
add other knowledge
0005 - Encrypted/authenticated WAL

These are very broad strokes at this point and should be split up a bit
more to make things more granular and easier to review, but I wanted to get
this update out.

Of note, the encryption supported in this release as exposed to the
heap-level is AES-XTS-128 and AES-XTS-256; there is built-in support for
CTR and GCM, however based on other discussions related how to store the
additional authenticated data on the page, GCM has been removed from
the list of supported ciphers.  This could certainly be enabled in the
future, however the other pieces that this patchset provides would enable
TDE without the additional block size/storage concerns.

Best,

David

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 31 Oct 2023 17:30:18 -0400

On Tue, Oct 31, 2023 at 04:23:17PM -0500, David Christensen wrote:
> 0004 - modifications to bin tools and programs to manage key rotation and add
> other knowledge
> These are very broad strokes at this point and should be split up a bit more to
> make things more granular and easier to review, but I wanted to get this update
> out.

This lacks temp table file encryption, right?

-- 
  Bruce Momjian  <[email-masked]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

---

From: Andres Freund <[email-masked]>
Date: Thu, 2 Nov 2023 19:32:28 -0700

Hi,

> more to make things more granular and easier to review, but I wanted to get

Yes, particularly 0003 really needs to be split - as is it's not easily
reviewable.

> From 327e86d52be1df8de9c3a324cb06b85ba5db9604 Mon Sep 17 00:00:00 2001
> From: David Christensen <[email-masked]>
> Date: Fri, 29 Sep 2023 15:16:00 -0400
> Subject: [PATCH v3 5/5] Add encrypted/authenticated WAL
>
> When using an encrypted cluster, we need to ensure that the WAL is also
> encrypted. While we could go with an page-based approach, we use instead a
> per-record approach, using GCM for the encryption method and storing the AuthTag
> in the xl_crc field.
>
> We change the xl_crc field to instead be a union struct, with a compile-time
> adjustable size to allow us to customize the number of bytes allocated to the
> GCM authtag.  This allows us to easily adjust the size of bytes needed to
> support our authentication.  (Testing has included up to 12, but leaving at this
> point to 4 due to keeping the size of the WAL records the same.)

Ugh, that'll be quite a bit of overhead in some workloads... You can't really
use such a build for non-encrypted workloads, making this a not very
deployable path...

> @@ -905,20 +905,28 @@ XLogInsertRecord(XLogRecData *rdata,
>  	{
>  		/*
>  		 * Now that xl_prev has been filled in, calculate CRC of the record
> -		 * header.
> +		 * header.  If we are using encrypted WAL, this CRC is overwritten by
> +		 * the authentication tag, so just zero
>  		 */
> -		rdata_crc = rechdr->xl_crc;
> -		COMP_CRC32C(rdata_crc, rechdr, offsetof(XLogRecord, xl_crc));
> -		FIN_CRC32C(rdata_crc);
> -		rechdr->xl_crc = rdata_crc;
> +		if (!encrypt_wal)
> +		{
> +			rdata_crc = rechdr->xl_integrity.crc;
> +			COMP_CRC32C(rdata_crc, rechdr, offsetof(XLogRecord, xl_integrity.crc));
> +			FIN_CRC32C(rdata_crc);
> +			rechdr->xl_integrity.crc = rdata_crc;
> +		}
> +		else
> +			memset(&rechdr->xl_integrity, 0, sizeof(rechdr->xl_integrity));

Why aren't you encrypting most of the data here?  Just as for CRC computation,
encrypting a large record in XLOG_BLCKSZ

>   * XLogRecordDataHeaderLong structs all begin with a single 'id' byte. It's
>   * used to distinguish between block references, and the main data structs.
>   */
> +
> +#define XL_AUTHTAG_SIZE 4
> +#define XL_HEADER_PAD 2
> +
>  typedef struct XLogRecord
>  {
>  	uint32		xl_tot_len;		/* total len of entire record */
> @@ -45,14 +49,16 @@ typedef struct XLogRecord
>  	XLogRecPtr	xl_prev;		/* ptr to previous record in log */
>  	uint8		xl_info;		/* flag bits, see below */
>  	RmgrId		xl_rmid;		/* resource manager for this record */
> -	/* 2 bytes of padding here, initialize to zero */
> -	pg_crc32c	xl_crc;			/* CRC for this record */
> -
> +	uint8       xl_pad[XL_HEADER_PAD];		/* required alignment padding */

What does "required" mean here? And why is this defined in a separate define?
And why do we need the explicit field here at all? The union will still
provide sufficient alignment for a uint32.

It also doesn't seem right to remove the comment about needing to zero out the
space.

> From 7557acf60f52da4a86fd9f902bab4804c028dd4b Mon Sep 17 00:00:00 2001
> From: David Christensen <[email-masked]>
> Date: Tue, 31 Oct 2023 15:24:02 -0400
> Subject: [PATCH v3 3/5] Backend-related changes

> --- a/src/backend/access/heap/rewriteheap.c
> +++ b/src/backend/access/heap/rewriteheap.c
> @@ -323,6 +323,11 @@ end_heap_rewrite(RewriteState state)
>  						state->rs_buffer,
>  						true);
>
> +		PageEncryptInplace(state->rs_buffer, MAIN_FORKNUM,
> +						   RelationIsPermanent(state->rs_new_rel),
> +						   state->rs_blockno,
> +						   RelationGetSmgr(state->rs_new_rel)->smgr_rlocator.locator.relNumber
> +			);
>  		PageSetChecksumInplace(state->rs_buffer, state->rs_blockno);
>
>  		smgrextend(RelationGetSmgr(state->rs_new_rel), MAIN_FORKNUM,

I don't think it's ok to have to make such changes in a bunch of places. I
think we need to add an abstraction layer between smgr and code like this
first.  Luckily I think Heikki was working abstracting away some of these
direct smgr* uses...

> +Architecture
> +------------
> +
> +Fundamentally, cluster file encryption must store data in a file system
> +in such a way that the keys required to decrypt the file system data can
> +only be accessed using somewhere outside of the file system itself.  The
> +external requirement can be someone typing in a passphrase, getting a
> +key from a key management server (KMS), or decrypting a key stored in
> +the file system using a hardware security module (HSM).  The current
> +architecture supports all of these methods, and includes sample scripts
> +for them.
> +
> +The simplest method for accessing data keys using some external
> +requirement would be to retrieve all data encryption keys from a KMS.
> +However, retrieved keys would still need to be verified as valid.  This
> +method also introduces unacceptable complexity for simpler use-cases,
> +like user-supplied passphrases or HSM usage.  External key rotation
> +would also be very hard since it would require re-encrypting all the
> +file system data with the new externally-stored keys.
> +
> +For these reason, a two-tiered architecture is used, which uses two
> +types of encryption keys: a key encryption key (KEK) and data encryption
> +keys (DEK). The KEK should not be present unencrypted in the file system
> +--- it should be supplied the user, stored externally (e.g., in a KMS)

*by* the user?

> +or stored in the file system encrypted with a HSM (e.g., PIV device).
> +The DEK is used to encrypt database files and is stored in the same file
> +system as the database but is encrypted using the KEK.  Because the DEK
> +is encrypted, its storage in the file system is no more of a security
> +weakness and the storage of the encrypted database files in the same
> +file system.

As is this paragraph doesn't really follow from the prior paragraph for
me. That a KMS would be hard to use isn't obviously related to splitting the
KEK and the DEK.

> +Implementation
> +--------------
> +
> +To enable cluster file encryption, the initdb option
> +--cluster-key-command must be used, which specifies a command to
> +retrieve the KEK.

FWIW, I think "cluster file encryption" is somewhat ambiguous. That could also
mean encrypting on the file system level or such.

> initdb records the cluster_key_command in
> +postgresql.conf.  Every time the KEK is needed, the command is run and
> +must return 64 hex characters which are decoded into the KEK.  The
> +command is called twice during initdb, and every time the server starts.
> +initdb also sets the encryption method in controldata during server
> +bootstrap.
> +
> +initdb runs "postgres --boot", which calls function
> +kmgr.c::BootStrapKmgr(), which calls the cluster key command.  The
> +cluster key command returns a KEK which is used to encrypt random bytes
> +for each DEK and writes them to the file system by
> +kmgr.c::KmgrWriteCryptoKeys() (unless --copy-encryption-keys is used).
> +Currently the DEK files are 0 and 1 and are stored in
> +$PGDATA/pg_cryptokeys/live.  The wrapped DEK files use Key Wrapping with
> +Padding which verifies the validity of the KEK.
> +
> +initdb also does a non-boot backend start which calls
> +kmgr.c::InitializeKmgr(), which calls the cluster key command a second
> +time.  This decrypts/unwraps the DEK keys and stores them in the shared
> +memory structure KmgrShmem. This step also happens every time the server
> +starts. Later patches will use the keys stored in KmgrShmem to
> +encrypt/decrypt database files.  KmgrShmem is erased via
> +explicit_bzero() on server shutdown.

I think this encodes too many details of how initdb works today. It seems
likely that nobody adding or removing a restart will think of updating this
file - nor should they have to.  I'd just say that initdb starts/stops
postgres multiple times.

> +Initialization Vector
> +---------------------
> +
> +Nonce means "number used once". An Initialization Vector (IV) is a
> +specific type of nonce. That is, unique but not necessarily random or
> +secret, as specified by the NIST
> +(https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38a.pdf).
> +To generate unique IVs, the NIST recommends two methods:
> +
> +	The first method is to apply the forward cipher function, under
> +	the same key that is used for the encryption of the plaintext,
> +	to a nonce. The nonce must be a data block that is unique to
> +	each execution of the encryption operation. For example, the
> +	nonce may be a counter, as described in Appendix B, or a message
> +	number. The second method is to generate a random data block
> +	using a FIPS-approved random number generator.
> +
> +We will use the first method to generate IVs. That is, select nonce
> +carefully and use a cipher with the key to make it unique enough to use
> +as an IV. The nonce selection for buffer encryption and WAL encryption
> +are described below.
> +
> +If the IV was used more than once with the same key (and we only use one
> +data encryption key), changes in the unencrypted data would be visible
> +in the encrypted data.
> +
> +IV for Heap/Index Encryption
> +- - - - - - - - - - - - - -
> +
> +To create the 16-byte IV needed by AES for each page version, we will
> +use the page LSN (8 bytes) and page number (4 bytes).

I still am quite quite unconvinced that using the LSN as a nonce is a good
design decision.

- LSNs can end up being reused after crash restarts
- the LSN does not contain the timeline ID, if a standby is promoted, two
  systems can be using the same LSNs
- The LSN does *NOT* actually change every time a page is modified. Even with
  wal_log_hint_bits, only the first hint bit modification to a page in a
  checkpoint cycles will cause WAL writes - and changing that would have
  a quite substantial overhead.

>  The LSN is ideal for use in the IV because it is +always increasing, and is
> changed every time a page is updated.  The +same LSN is never used for two
> relations with different page contents.

As mentioned above, this is not true - the LSN does *NOT* change every time
the page is updated.

> +However, the same LSN can be used in multiple pages in the same relation
> +--- this can happen when a heap update expires an old tuple and adds a
> +new tuple to another page.  By adding the page number to the IV, we keep
> +the IV unique.

There's many other ways that can happen.

> +CREATE DATABASE can be run with two different strategies: FILE_COPY or
> +WAL_LOG.  If using WAL_LOG, the heap/index files are automatically
> +rewritten with new LSNs as part of the copy operation and will get new
> +IVs automatically.
> +
> +This approach still works with the older FILE_COPY stragegy; by not
> +using the database id in the IV, CREATE DATABASE can copy the heap/index
> +files from the old database to a new one without decryption/encryption.
> +Both page copies are valid.  Once a database changes its pages, it gets
> +new LSNs, and hence new IV.
> +
> +As part of WAL logging, every change of a WAL-logged page gets a new
> +LSN, and therefore a new IV automatically.
> +
> +However, the LSN must then be visible on encrypted pages, so we will not
> +encrypt the LSN on the page. We will also not encrypt the CRC so
> +pg_checksums can still check pages offline without access to the keys.

s/crc/checksum/? The data-page-level checksum isn't a CRC.

> +Non-Permanent Relations
> +- - - - - - - - - - - -
> +
> +To avoid the overhead of generating WAL for non-permanent (unlogged and
> +temporary) relations, we assign fake LSNs that are derived from a
> +counter via xlog.c::GetFakeLSNForUnloggedRel().  (GiST also uses this
> +counter for LSNs.)  We also set a bit in the IV so the use of the same
> +value for WAL (real) and fake LSNs will still generate unique IVs.  Only
> +main forks are encrypted, not init, vm, or fsm files.

Why aren't other forks encrypted? This seems like a very odd design to me.

> +Hint Bits
> +- - - - -
> +
> +For hint bit changes, the LSN normally doesn't change, which is a
> +problem.  By enabling wal_log_hints, you get full page writes to the WAL
> +after the first hint bit change of the checkpoint.  This is useful for
> +two reasons.  First, it generates a new LSN, which is needed for the IV
> +to be secure.  Second, full page images protect against torn pages,
> +which is an even bigger requirement for encryption because the new LSN
> +is re-encrypting the entire page, not just the hint bit changes.  You
> +can safely lose the hint bit changes, but you need to use the same LSN
> +to decrypt the entire page, so a torn page with an LSN change cannot be
> +decrypted.  To prevent this, wal_log_hints guarantees that the
> +pre-hint-bit version (and previous LSN version) of the page is restored.
> +
> +However, if a hint-bit-modified page is written to the file system
> +during a checkpoint, and there is a later hint bit change switching the
> +same page from clean to dirty during the same checkpoint, we need a new
> +LSN, and wal_log_hints doesn't give us a new LSN here.  The fix for this
> +is to update the page LSN by writing a dummy WAL record via
> +xloginsert.c::LSNForEncryption() in such cases.

Ugh, so that's really the plan. That's a substantial overhead in some common
scenarios.

...

Greetings,

Andres Freund

---

From: Stephen Frost <[email-masked]>
Date: Mon, 6 Nov 2023 09:56:37 -0500

Greetings,

Thanks for your feedback on this.

> I still am quite quite unconvinced that using the LSN as a nonce is a good

This is a really important part of the overall path to moving this
forward, so I wanted to jump to it and have a specific discussion
around this.  I agree that there are downsides to using the LSN, some of
which we could possibly address (eg: include the timeline ID in the IV),
but others that would be harder to deal with.

The question then is- what's the alternative?

One approach would be to change the page format to include space for an
explicit nonce.  I don't see the community accepting such a new page
format as the only format we support though as that would mean no
pg_upgrade support along with wasted space if TDE isn't being used.
Ideally, we'd instead be able to support multiple page formats where
users could decide when they create their cluster what features they
want- and luckily we even have such an effort underway with patches
posted for review [1].  Certainly, with the base page-special-feature
patch, we could have an option for users to choose that they want a
better nonce than the LSN, or we could bundle that assumption in with,
say, the authenticated-encryption feature (if you want authenticated
encryption, then you want more from the encryption system than the
basics, and therefore we presume you also want a better nonce than the
LSN).

Another approach would be a separate fork, but that then has a number of
downsides too- every write has to touch that too, and a single page of
nonces would cover a pretty large number of pages also.

Ultimately, I agree with you that the LSN isn't perfect and we really
shouldn't be calling it 'ideal' as it isn't, and we can work to fix that
language in the patch, but the lack of any alternative being proposed
that might be acceptable makes this feel a bit like rock management [2].

My apologies if there's something good that's already been specifically
pushed and I just missed it; if so, a link to that suggestion and
discussion would be greatly appreciated.

Thanks again!

Stephen

[1]: https://commitfest.postgresql.org/45/3986/
[2]: https://en.wikipedia.org/wiki/Wikipedia:Bring_me_a_rock ; though
that isn't great for a quick summary (which I tried to find on an
isolated page somewhere and didn't).

The gist is, without a suggestion of things to try, we're left
to our own devices to try and figure out things which might be
successful, only to have those turned down too when we come back with
them, see [1] for what feels like an example of this.  Given your
feedback overall, which I'm very thankful for, I'm hopeful that you see
that this is, indeed, a useful feature that people are asking for and
therefore are willing to spend some time on it, but if the feedback is
that nothing on the page is acceptable to use for the nonce, we can't
put the nonce somewhere else, and we can't change the page format, then
everything else is just moving deck chairs around on the titanic that
has been this effort.

---

From: Bruce Momjian <[email-masked]>
Date: Mon, 6 Nov 2023 11:04:04 -0500

> mean encrypting on the file system level or such.

We could call it:

*  cluster data file encryption
*  cluster data encryption
*  database cluster encryption

---

From: Andres Freund <[email-masked]>
Date: Tue, 7 Nov 2023 15:49:32 -0800

Hi,

> pg_upgrade support along with wasted space if TDE isn't being used.

Right.

> posted for review [1].

I think there are some details wrong with that patch - IMO the existing macros
should just continue to work as-is and instead the places that want the more
narrow definition should be moved to the new macros and it changes places that
should continue to use compile time constants - but it doesn't seem like a
fundamentally bad idea to me.  I certainly like it much better than making the
page size runtime configurable.

(I'll try to reply with the above points to [1])

> Certainly, with the base page-special-feature patch, we could have an option
> for users to choose that they want a better nonce than the LSN, or we could
> bundle that assumption in with, say, the authenticated-encryption feature
> (if you want authenticated encryption, then you want more from the
> encryption system than the basics, and therefore we presume you also want a
> better nonce than the LSN).

I don't think we should support using the LSN as a nonce if we have an
alternative. The cost and complexity overhead is just not worth it.  Yes,
it'll be harder for users to migrate to encryption, but adding complexity
elsewhere in the system to get an inferior result isn't worth it.

> nonces would cover a pretty large number of pages also.

Yea, the costs of doing so is nontrivial. If you were trying to implement
encryption on the smgr level - which I doubt you should but am not certain
about - my suggestion would be to interleave pages with metadata like nonces
and AEAD with the data pages. I.e. one metadata page would be followed by
  (BLCKSZ - SizeOfPageHeaderData) / (sizeof(nonce) + sizeof(AEAD))
pages containing actual relation data.  That way you still get decent locality
during scans and writes.

Relation forks were a mistake, we shouldn't use them in more places.

I think it'd be much better if we also encrypted forks, rather than just the
main fork...

Greetings,

Andres Freund

---

From: Peter Smith <[email-masked]>
Date: Mon, 22 Jan 2024 17:17:21 +1100

2024-01 Commitfest.

Hi, This patch has a CF status of "Needs Review" [1], but it seems
there were CFbot test failures last time it was run [2]. Please have a
look and post an updated version if necessary.

======
[1] https://commitfest.postgresql.org/46/3985/
[2] https://cirrus-ci.com/task/5498215743619072

Kind Regards,
Peter Smith.

---

From: vignesh C <[email-masked]>
Date: Thu, 1 Feb 2024 20:47:39 +0530

> look and post an updated version if necessary.

The patch which you submitted has been awaiting your attention for
quite some time now.  As such, we have moved it to "Returned with
Feedback" and removed it from the reviewing queue. Depending on
timing, this may be reversible.  Kindly address the feedback you have
received, and resubmit the patch to the next CommitFest.

Regards,
Vignesh