Subject: Moving forward with TDE

From: David Christensen <[email-masked]>
Date: Mon, 24 Oct 2022 11:29:19 -0500

Hi -hackers,

Working with Stephen, I am attempting to pick up some of the work that
was left off with TDE and the key management infrastructure.  I have
rebased Bruce's KMS/TDE patches as they existed on the
https://wiki.postgresql.org/wiki/Transparent_Data_Encryption wiki
page, which are enclosed in this email.

I would love to open a discussion about how to move forward and get
some of these features built out.  The historical threads here are
quite long and complicated; is there a "current state" other than the
wiki that reflects the general thinking on this feature?  Any major
developments in direction that would not be reflected in the code from
May 2021?

Thanks,

David

---

From: Aleksander Alekseev <[email-masked]>
Date: Thu, 3 Nov 2022 17:09:00 +0300

Hi David,

> page, which are enclosed in this email.

I'm happy to see that the TDE effort was picked up.

> developments in direction that would not be reflected in the code from

The patches seem to be well documented and decomposed in small pieces.
That's good.

Unless somebody in the community remembers open questions/issues with
TDE that were never addressed I suggest simply iterating with our
usual testing/reviewing process. For now I'm going to change the
status of the CF entry [1] to "Waiting for Author" since the patchset
doesn't pass the CI [2].

One limitation of the design described on the wiki I see is that it
seems to heavily rely on AES:

> We will use Advanced Encryption Standard (AES) [4]. We will offer three key length options (128, 192, and 256-bits) selected at initdb time with --file-encryption-method

(there doesn't seem to be any mention of the hash/MAC algorithms,
that's odd). In the future we should be able to add the support of
alternative algorithms. The reason is that the algorithms can become
weak every 20 years or so, and the preferred algorithms may also
depend on the region. This should NOT be implemented in this
particular patchset, but the design shouldn't prevent from
implementing this in the future.

[1]: https://commitfest.postgresql.org/40/3985/
[2]: http://cfbot.cputube.org/

-- 
Best regards,
Aleksander Alekseev

---

From: David Christensen <[email-masked]>
Date: Thu, 3 Nov 2022 17:06:23 -0500

> status of the CF entry [1] to "Waiting for Author" since the patchset

Thanks, enclosed is a new version that is rebased on HEAD and fixes a
bug that the new pg_control_init() test picked up.

Known issues (just discovered by me in testing the latest revision) is
that databases created from `template0` are not decrypting properly,
but `template1` works fine, so going to dig in on that soon.

> implementing this in the future.

Yes, we definitely are considering multiple algorithms support as part
of this effort.

Best,

David

---

From: Dilip Kumar <[email-masked]>
Date: Fri, 4 Nov 2022 14:12:19 +0530

> bug that the new pg_control_init() test picked up.

I was looking into the documentation patches 0001 and 0002, I think
the explanation is very clear.  I have a few questions/comments

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

How about the WAL_LOG method for creating a database? because in that
we get the new LSN for the pages in the new database, so do we
reencrypt, if yes then this documentation needs to be updated
otherwise we might need to add that code.

+changes its pages, it gets new LSNs, and hence new IV.  Using only the
+LSN and page number also avoids requiring pg_upgrade to preserve
+database oids, tablespace oids, and relfilenodes.

I think this line needs to be changed, because now we are already
preserving dbid/tbsid/relfilenode.  So even though we are not using
those in IV there is no point in saying we are avoiding that
requirement.

I will review the remaining patches soon.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

---

From: Jacob Champion <[email-masked]>
Date: Tue, 15 Nov 2022 11:07:54 -0800

> developments in direction that would not be reflected in the code from

I don't think the patchset here has incorporated the results of the
discussion [1] that happened at the end of 2021. For example, it looks
like AES-CTR is still in use for the pages, which I thought was
already determined to be insufficient.

The following next steps were proposed in that thread:

> 1. modify temporary file I/O to use a more centralized API
> 2. modify the existing cluster file encryption patch to use XTS with a
>    IV that uses more than the LSN
> 3. add XTS regression test code like CTR
> 4. create WAL encryption code using CTR

Does this patchset need review before those steps are taken (or was
there additional conversation/work that I missed)?

Thanks,
--Jacob

[1] https://www.postgresql.org/message-id/flat/20211013222648.GA373%40momjian.us

---

From: David Christensen <[email-masked]>
Date: Tue, 15 Nov 2022 13:39:27 -0600

> On Nov 15, 2022, at 1:08 PM, Jacob Champion <[email-masked]> wrote:
> 

Good to know about the next steps, thanks. 

> there additional conversation/work that I missed)?

This was just a refresh of the old patches on the wiki to work as written on HEAD. If there are known TODOs here this then that work is still needing to be done. 

I was going to take 2) and Stephen was going to work on 3); I am not sure about the other two but will review the thread you pointed to. Thanks for pointing that out. 

David

---

From: Jacob Champion <[email-masked]>
Date: Tue, 15 Nov 2022 12:08:40 -0800

> Good to know about the next steps, thanks.

You're welcome!

> I was going to take 2) and Stephen was going to work on 3); I am not sure about the other two but will review the thread you pointed to. Thanks for pointing that out.

I've attached the diffs I'm carrying to build this under meson (as
well as -Wshadow; my removal of the two variables probably needs some
scrutiny). It looks like the testcrypto executable will need
substantial changes after the common/hex.h revert.

--Jacob
From eb4b55f5d03e362cf340f322c0cefbf95f53657a Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email-masked]>
Date: Mon, 14 Nov 2022 16:02:57 -0800
Subject: [PATCH] WIP: fix meson

Note that the testcrypto executable needs to be rewritten.
---
 src/backend/bootstrap/bootstrap.c |  2 --
 src/backend/crypto/kmgr.c         |  1 -
 src/backend/crypto/meson.build    | 14 ++++++++++++++
 src/backend/meson.build           |  1 +
 src/bin/meson.build               |  1 +
 src/bin/pg_alterckey/meson.build  | 16 ++++++++++++++++
 src/common/meson.build            |  3 +++
 7 files changed, 35 insertions(+), 3 deletions(-)
 create mode 100644 src/backend/crypto/meson.build
 create mode 100644 src/bin/pg_alterckey/meson.build

diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index ffaf9c7cc4..07b2f15343 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -256,8 +256,6 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
 				break;
  			case 'K':
  				{
- 					int i;
- 
  					/* method 0/disabled cannot be specified */
  					for (i = DISABLED_ENCRYPTION_METHOD + 1;
  						 i < NUM_ENCRYPTION_METHODS; i++)
diff --git a/src/backend/crypto/kmgr.c b/src/backend/crypto/kmgr.c
index a83effbca8..b4248965f8 100644
--- a/src/backend/crypto/kmgr.c
+++ b/src/backend/crypto/kmgr.c
@@ -141,7 +141,6 @@ BootStrapKmgr(void)
 	if (bootstrap_old_key_datadir == NULL)
 	{
 		unsigned char *bootstrap_keys_wrap[KMGR_NUM_DATA_KEYS];
-		int			key_lens[KMGR_NUM_DATA_KEYS];
 		PgCipherCtx *cluster_key_ctx;
 
 		/* Create KEK encryption context */
diff --git a/src/backend/crypto/meson.build b/src/backend/crypto/meson.build
new file mode 100644
index 0000000000..2b72b26ebf
--- /dev/null
+++ b/src/backend/crypto/meson.build
@@ -0,0 +1,14 @@
+backend_sources += files(
+  'bufenc.c',
+  'kmgr.c',
+)
+
+install_data(
+  'ckey_aws.sh.sample',
+  'ckey_direct.sh.sample',
+  'ckey_passphrase.sh.sample',
+  'ckey_piv_nopin.sh.sample',
+  'ckey_piv_pin.sh.sample',
+  'ssl_passphrase.sh.sample',
+  install_dir: dir_data / 'auth_commands',
+)
diff --git a/src/backend/meson.build b/src/backend/meson.build
index 37562bae13..2162aef554 100644
--- a/src/backend/meson.build
+++ b/src/backend/meson.build
@@ -9,6 +9,7 @@ subdir('backup')
 subdir('bootstrap')
 subdir('catalog')
 subdir('commands')
+subdir('crypto')
 subdir('executor')
 subdir('foreign')
 subdir('jit')
diff --git a/src/bin/meson.build b/src/bin/meson.build
index 5fd5a9d2f9..8dd9133213 100644
--- a/src/bin/meson.build
+++ b/src/bin/meson.build
@@ -1,4 +1,5 @@
 subdir('initdb')
+subdir('pg_alterckey')
 subdir('pg_amcheck')
 subdir('pg_archivecleanup')
 subdir('pg_basebackup')
diff --git a/src/bin/pg_alterckey/meson.build b/src/bin/pg_alterckey/meson.build
new file mode 100644
index 0000000000..72ec8274c3
--- /dev/null
+++ b/src/bin/pg_alterckey/meson.build
@@ -0,0 +1,16 @@
+pg_alterckey_sources = files(
+  'pg_alterckey.c',
+)
+
+if host_system == 'windows'
+  pg_alterckey_sources += rc_bin_gen.process(win32ver_rc, extra_args: [
+    '--NAME', 'pg_alterckey',
+    '--FILEDESC', 'pg_alterckey - alter the cluster key',])
+endif
+
+pg_alterckey = executable('pg_alterckey',
+  pg_alterckey_sources,
+  dependencies: [frontend_code, libpq],
+  kwargs: default_bin_args,
+)
+bin_targets += pg_alterckey
diff --git a/src/common/meson.build b/src/common/meson.build
index 1c9b8a3a01..102b4d7b23 100644
--- a/src/common/meson.build
+++ b/src/common/meson.build
@@ -12,6 +12,7 @@ common_sources = files(
   'ip.c',
   'jsonapi.c',
   'keywords.c',
+  'kmgr_utils.c',
   'kwlookup.c',
   'link-canary.c',
   'md5_common.c',
@@ -34,12 +35,14 @@ common_sources = files(
 
 if ssl.found()
   common_sources += files(
+    'cipher_openssl.c',
     'cryptohash_openssl.c',
     'hmac_openssl.c',
     'protocol_openssl.c',
   )
 else
   common_sources += files(
+    'cipher.c',
     'cryptohash.c',
     'hmac.c',
     'md5.c',
-- 
2.25.1

---

From: David Christensen <[email-masked]>
Date: Thu, 17 Nov 2022 10:02:05 -0600

Hi Jacob,

Thanks, I've added this patch in my tree [1]. (For now, just adding
fixes and the like atop the original separate patches, but will
eventually get things winnowed down into probably the same 12 parts
the originals were reviewed in.

Best,

David

[1] https://github.com/pgguru/postgres/tree/tde

---

From: David Christensen <[email-masked]>
Date: Thu, 17 Nov 2022 10:34:48 -0600

Hi Dilip,

Thanks for the feedback here. I will review the docs changes and add to my tree.

Best,

David