From a8f4a382004685efb067ef3cfd27e0e4f071e523 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20=C3=85strand?= Date: Tue, 13 May 2025 13:35:10 +0200 Subject: [PATCH 1/3] Use an enum instead of bool for encryption status Replace the boolean used in the smgr hooks. Currently we only have two states still. This is in preparation for adding a third state to indicate that the table is encrypted, but the key has not yet been loaded. --- contrib/pg_tde/src/smgr/pg_tde_smgr.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/contrib/pg_tde/src/smgr/pg_tde_smgr.c b/contrib/pg_tde/src/smgr/pg_tde_smgr.c index e72f5b195efef..fe2cddd7c0d60 100644 --- a/contrib/pg_tde/src/smgr/pg_tde_smgr.c +++ b/contrib/pg_tde/src/smgr/pg_tde_smgr.c @@ -7,6 +7,15 @@ #include "access/pg_tde_tdemap.h" #include "pg_tde_event_capture.h" +typedef enum TDEMgrRelationDataEncryptionStatus +{ + /* This is a plaintext relation */ + RELATION_NOT_ENCRYPTED = 0, + + /* This is an encrypted relation, and we have the key available. */ + RELATION_KEY_AVAILABLE = 1, +} TDEMgrRelationDataEncryptionStatus; + typedef struct TDESMgrRelationData { /* parent data */ @@ -19,7 +28,7 @@ typedef struct TDESMgrRelationData int md_num_open_segs[MAX_FORKNUM + 1]; struct _MdfdVec *md_seg_fds[MAX_FORKNUM + 1]; - bool encrypted_relation; + TDEMgrRelationDataEncryptionStatus encryption_status; InternalKey relKey; } TDESMgrRelationData; @@ -72,7 +81,7 @@ tde_mdwritev(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, TDESMgrRelation tdereln = (TDESMgrRelation) reln; InternalKey *int_key = &tdereln->relKey; - if (!tdereln->encrypted_relation) + if (tdereln->encryption_status == RELATION_NOT_ENCRYPTED) { mdwritev(reln, forknum, blocknum, buffers, nblocks, skipFsync); } @@ -131,7 +140,7 @@ tde_mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, TDESMgrRelation tdereln = (TDESMgrRelation) reln; InternalKey *int_key = &tdereln->relKey; - if (!tdereln->encrypted_relation) + if (tdereln->encryption_status == RELATION_NOT_ENCRYPTED) { mdextend(reln, forknum, blocknum, buffer, skipFsync); } @@ -160,7 +169,7 @@ tde_mdreadv(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, mdreadv(reln, forknum, blocknum, buffers, nblocks); - if (!tdereln->encrypted_relation) + if (tdereln->encryption_status == RELATION_NOT_ENCRYPTED) return; for (int i = 0; i < nblocks; ++i) @@ -233,12 +242,12 @@ tde_mdcreate(RelFileLocator relold, SMgrRelation reln, ForkNumber forknum, bool if (key) { - tdereln->encrypted_relation = true; + tdereln->encryption_status = RELATION_KEY_AVAILABLE; tdereln->relKey = *key; } else { - tdereln->encrypted_relation = false; + tdereln->encryption_status = RELATION_NOT_ENCRYPTED; } } } @@ -258,12 +267,12 @@ tde_mdopen(SMgrRelation reln) if (key) { - tdereln->encrypted_relation = true; + tdereln->encryption_status = RELATION_KEY_AVAILABLE; tdereln->relKey = *key; } else { - tdereln->encrypted_relation = false; + tdereln->encryption_status = RELATION_NOT_ENCRYPTED; } } From 5816caa61a793185f6d6b67eda54312960b03df1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20=C3=85strand?= Date: Tue, 13 May 2025 15:30:24 +0200 Subject: [PATCH 2/3] Do not use principal key to check encryption status In many cases it's completely unnecessary to have the principal key in order to know if a relation is encrypted or not. This simplifies cases where principal key provider is not available for any reason. The unencrypted key should most likely be cached, but this commit does not do that. --- contrib/pg_tde/src/access/pg_tde_tdemap.c | 32 +++++++++++++++++++ contrib/pg_tde/src/common/pg_tde_utils.c | 6 ++-- .../pg_tde/src/include/access/pg_tde_tdemap.h | 1 + contrib/pg_tde/src/smgr/pg_tde_smgr.c | 8 ++++- .../t/expected/010_change_key_provider.out | 12 +++++-- src/bin/pg_checksums/pg_checksums.c | 2 +- 6 files changed, 54 insertions(+), 7 deletions(-) diff --git a/contrib/pg_tde/src/access/pg_tde_tdemap.c b/contrib/pg_tde/src/access/pg_tde_tdemap.c index b870683618436..59a3856e2ca1a 100644 --- a/contrib/pg_tde/src/access/pg_tde_tdemap.c +++ b/contrib/pg_tde/src/access/pg_tde_tdemap.c @@ -1074,6 +1074,38 @@ pg_tde_get_principal_key_info(Oid dbOid) return signed_key_info; } +/* + * Figures out whether a relation is encrypted or not, but without trying to + * decrypt the key if it is. This also means that this function cannot push the + * key to cache. + */ +bool +IsSMGRRelationEncrypted(RelFileLocatorBackend rel) +{ + bool result; + TDEMapEntry map_entry; + char db_map_path[MAXPGPATH]; + + Assert(rel.locator.relNumber != InvalidRelFileNumber); + + if (RelFileLocatorBackendIsTemp(rel)) + return pg_tde_get_key_from_cache(&rel.locator, TDE_KEY_TYPE_SMGR) != NULL; + else if (pg_tde_get_key_from_cache(&rel.locator, TDE_KEY_TYPE_SMGR)) + return true; + + pg_tde_set_db_file_path(rel.locator.dbOid, db_map_path); + + if (access(db_map_path, F_OK) == -1) + return false; + + LWLockAcquire(tde_lwlock_enc_keys(), LW_SHARED); + + result = pg_tde_find_map_entry(&rel.locator, TDE_KEY_TYPE_SMGR, db_map_path, &map_entry); + + LWLockRelease(tde_lwlock_enc_keys()); + return result; +} + /* * Returns TDE key for a given relation. * First it looks in a cache. If nothing found in the cache, it reads data from diff --git a/contrib/pg_tde/src/common/pg_tde_utils.c b/contrib/pg_tde/src/common/pg_tde_utils.c index d7ed21af75b2a..91a9c8128e26e 100644 --- a/contrib/pg_tde/src/common/pg_tde_utils.c +++ b/contrib/pg_tde/src/common/pg_tde_utils.c @@ -29,7 +29,7 @@ pg_tde_is_encrypted(PG_FUNCTION_ARGS) LOCKMODE lockmode = AccessShareLock; Relation rel = relation_open(relationOid, lockmode); RelFileLocatorBackend rlocator = {.locator = rel->rd_locator,.backend = rel->rd_backend}; - InternalKey *key; + bool result; if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind)) { @@ -42,11 +42,11 @@ pg_tde_is_encrypted(PG_FUNCTION_ARGS) errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("we cannot check if temporary relations from other backends are encrypted")); - key = GetSMGRRelationKey(rlocator); + result = IsSMGRRelationEncrypted(rlocator); relation_close(rel, lockmode); - PG_RETURN_BOOL(key != NULL); + PG_RETURN_BOOL(result); } #endif /* !FRONTEND */ diff --git a/contrib/pg_tde/src/include/access/pg_tde_tdemap.h b/contrib/pg_tde/src/include/access/pg_tde_tdemap.h index 0955569b00cdc..70104d00474da 100644 --- a/contrib/pg_tde/src/include/access/pg_tde_tdemap.h +++ b/contrib/pg_tde/src/include/access/pg_tde_tdemap.h @@ -109,6 +109,7 @@ pg_tde_set_db_file_path(Oid dbOid, char *path) join_path_components(path, pg_tde_get_data_dir(), psprintf(PG_TDE_MAP_FILENAME, dbOid)); } +extern bool IsSMGRRelationEncrypted(RelFileLocatorBackend rel); extern InternalKey *GetSMGRRelationKey(RelFileLocatorBackend rel); extern int pg_tde_count_relations(Oid dbOid); diff --git a/contrib/pg_tde/src/smgr/pg_tde_smgr.c b/contrib/pg_tde/src/smgr/pg_tde_smgr.c index fe2cddd7c0d60..715203b01faad 100644 --- a/contrib/pg_tde/src/smgr/pg_tde_smgr.c +++ b/contrib/pg_tde/src/smgr/pg_tde_smgr.c @@ -67,6 +67,7 @@ tde_smgr_should_encrypt(const RelFileLocatorBackend *smgr_rlocator, RelFileLocat .backend = smgr_rlocator->backend, }; + /* Actually get the key here to ensure result is cached. */ return GetSMGRRelationKey(old_smgr_locator) != 0; } } @@ -111,6 +112,11 @@ tde_mdwritev(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, } } +/* + * The current transaction might already be commited when this function is + * called, so do not call any code that uses ereport(ERROR) or otherwise tries + * to abort the transaction. + */ static void tde_mdunlink(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo) { @@ -128,7 +134,7 @@ tde_mdunlink(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo) */ if (forknum == MAIN_FORKNUM || forknum == InvalidForkNumber) { - if (!RelFileLocatorBackendIsTemp(rlocator) && GetSMGRRelationKey(rlocator)) + if (!RelFileLocatorBackendIsTemp(rlocator) && IsSMGRRelationEncrypted(rlocator)) pg_tde_free_key_map_entry(&rlocator.locator); } } diff --git a/contrib/pg_tde/t/expected/010_change_key_provider.out b/contrib/pg_tde/t/expected/010_change_key_provider.out index b162e7703de3a..e744203167269 100644 --- a/contrib/pg_tde/t/expected/010_change_key_provider.out +++ b/contrib/pg_tde/t/expected/010_change_key_provider.out @@ -121,7 +121,11 @@ SELECT * FROM test_enc ORDER BY id; SELECT pg_tde_verify_key(); psql::1: ERROR: failed to retrieve principal key test-key from keyring with ID 1 SELECT pg_tde_is_encrypted('test_enc'); -psql::1: ERROR: failed to retrieve principal key test-key from keyring with ID 1 + pg_tde_is_encrypted +--------------------- + t +(1 row) + SELECT * FROM test_enc ORDER BY id; psql::1: ERROR: failed to retrieve principal key test-key from keyring with ID 1 -- mv /tmp/change_key_provider_2.per /tmp/change_key_provider_3.per @@ -191,7 +195,11 @@ SELECT pg_tde_change_database_key_provider_file('file-vault', '/tmp/change_key_p SELECT pg_tde_verify_key(); psql::1: ERROR: Failed to verify principal key header for key test-key, incorrect principal key or corrupted key file SELECT pg_tde_is_encrypted('test_enc'); -psql::1: ERROR: Failed to verify principal key header for key test-key, incorrect principal key or corrupted key file + pg_tde_is_encrypted +--------------------- + t +(1 row) + SELECT * FROM test_enc ORDER BY id; psql::1: ERROR: Failed to verify principal key header for key test-key, incorrect principal key or corrupted key file CREATE TABLE test_enc2 (id serial, k integer, PRIMARY KEY (id)) USING tde_heap; diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index 833b07a970971..73c8f320a60ad 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -141,7 +141,7 @@ is_pg_tde_encypted(Oid spcOid, Oid dbOid, RelFileNumber relNumber) RelFileLocator locator = {.spcOid = spcOid, .dbOid = dbOid,.relNumber = relNumber}; RelFileLocatorBackend rlocator = {.locator = locator,.backend = INVALID_PROC_NUMBER}; - return GetSMGRRelationKey(rlocator) != NULL; + return IsSMGRRelationEncrypted(rlocator); } #endif From 1a04d5d62d542748c3ed7d691127ac29143e5f50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20=C3=85strand?= Date: Tue, 13 May 2025 15:33:28 +0200 Subject: [PATCH 3/3] Defer decryption of relation keys Don't decrypt relation keys until they're actually needed. In some cases tde_mdopen() is called after the transaction is committed which means that we're not longer able to abort the transaction if we fail when we fetch the principal key. This happens, for example, if dropping an encrypted table. Previously this would cause postmaster to panic if it didn't have access to the principal key. --- contrib/pg_tde/src/smgr/pg_tde_smgr.c | 54 ++++++++++++++++++++----- contrib/pg_tde/t/001_basic.pl | 8 +++- contrib/pg_tde/t/expected/001_basic.out | 4 +- 3 files changed, 55 insertions(+), 11 deletions(-) diff --git a/contrib/pg_tde/src/smgr/pg_tde_smgr.c b/contrib/pg_tde/src/smgr/pg_tde_smgr.c index 715203b01faad..83ee67be668e2 100644 --- a/contrib/pg_tde/src/smgr/pg_tde_smgr.c +++ b/contrib/pg_tde/src/smgr/pg_tde_smgr.c @@ -14,6 +14,9 @@ typedef enum TDEMgrRelationDataEncryptionStatus /* This is an encrypted relation, and we have the key available. */ RELATION_KEY_AVAILABLE = 1, + + /* This is an encrypted relation, but we haven't loaded the key yet. */ + RELATION_KEY_NOT_AVAILABLE = 2, } TDEMgrRelationDataEncryptionStatus; typedef struct TDESMgrRelationData @@ -36,6 +39,16 @@ typedef TDESMgrRelationData *TDESMgrRelation; static void CalcBlockIv(ForkNumber forknum, BlockNumber bn, const unsigned char *base_iv, unsigned char *iv); +static bool +tde_smgr_is_encrypted(const RelFileLocatorBackend *smgr_rlocator) +{ + /* Do not try to encrypt/decrypt catalog tables */ + if (IsCatalogRelationOid(smgr_rlocator->locator.relNumber)) + return false; + + return IsSMGRRelationEncrypted(*smgr_rlocator); +} + static InternalKey * tde_smgr_get_key(const RelFileLocatorBackend *smgr_rlocator) { @@ -80,7 +93,6 @@ tde_mdwritev(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, const void **buffers, BlockNumber nblocks, bool skipFsync) { TDESMgrRelation tdereln = (TDESMgrRelation) reln; - InternalKey *int_key = &tdereln->relKey; if (tdereln->encryption_status == RELATION_NOT_ENCRYPTED) { @@ -88,10 +100,19 @@ tde_mdwritev(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, } else { + InternalKey *int_key; unsigned char *local_blocks = palloc(BLCKSZ * (nblocks + 1)); unsigned char *local_blocks_aligned = (unsigned char *) TYPEALIGN(PG_IO_ALIGN_SIZE, local_blocks); void **local_buffers = palloc_array(void *, nblocks); + if (tdereln->encryption_status == RELATION_KEY_NOT_AVAILABLE) + { + tdereln->relKey = *tde_smgr_get_key(&reln->smgr_rlocator); + tdereln->encryption_status = RELATION_KEY_AVAILABLE; + } + + int_key = &tdereln->relKey; + for (int i = 0; i < nblocks; ++i) { BlockNumber bn = blocknum + i; @@ -144,7 +165,6 @@ tde_mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, const void *buffer, bool skipFsync) { TDESMgrRelation tdereln = (TDESMgrRelation) reln; - InternalKey *int_key = &tdereln->relKey; if (tdereln->encryption_status == RELATION_NOT_ENCRYPTED) { @@ -152,10 +172,19 @@ tde_mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, } else { + InternalKey *int_key; unsigned char *local_blocks = palloc(BLCKSZ * (1 + 1)); unsigned char *local_blocks_aligned = (unsigned char *) TYPEALIGN(PG_IO_ALIGN_SIZE, local_blocks); unsigned char iv[16]; + if (tdereln->encryption_status == RELATION_KEY_NOT_AVAILABLE) + { + tdereln->relKey = *tde_smgr_get_key(&reln->smgr_rlocator); + tdereln->encryption_status = RELATION_KEY_AVAILABLE; + } + + int_key = &tdereln->relKey; + CalcBlockIv(forknum, blocknum, int_key->base_iv, iv); AesEncrypt(int_key->key, iv, ((unsigned char *) buffer), BLCKSZ, local_blocks_aligned); @@ -171,12 +200,19 @@ tde_mdreadv(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, void **buffers, BlockNumber nblocks) { TDESMgrRelation tdereln = (TDESMgrRelation) reln; - InternalKey *int_key = &tdereln->relKey; + InternalKey *int_key; mdreadv(reln, forknum, blocknum, buffers, nblocks); if (tdereln->encryption_status == RELATION_NOT_ENCRYPTED) return; + else if (tdereln->encryption_status == RELATION_KEY_NOT_AVAILABLE) + { + tdereln->relKey = *tde_smgr_get_key(&reln->smgr_rlocator); + tdereln->encryption_status = RELATION_KEY_AVAILABLE; + } + + int_key = &tdereln->relKey; for (int i = 0; i < nblocks; ++i) { @@ -260,21 +296,21 @@ tde_mdcreate(RelFileLocator relold, SMgrRelation reln, ForkNumber forknum, bool /* * mdopen() -- Initialize newly-opened relation. + * + * The current transaction might already be commited when this function is + * called, so do not call any code that uses ereport(ERROR) or otherwise tries + * to abort the transaction. */ static void tde_mdopen(SMgrRelation reln) { TDESMgrRelation tdereln = (TDESMgrRelation) reln; - InternalKey *key; mdopen(reln); - key = tde_smgr_get_key(&reln->smgr_rlocator); - - if (key) + if (tde_smgr_is_encrypted(&reln->smgr_rlocator)) { - tdereln->encryption_status = RELATION_KEY_AVAILABLE; - tdereln->relKey = *key; + tdereln->encryption_status = RELATION_KEY_NOT_AVAILABLE; } else { diff --git a/contrib/pg_tde/t/001_basic.pl b/contrib/pg_tde/t/001_basic.pl index fc6e495101955..611e4b077e670 100644 --- a/contrib/pg_tde/t/001_basic.pl +++ b/contrib/pg_tde/t/001_basic.pl @@ -24,7 +24,7 @@ ); PGTDE::psql($node, 'postgres', - "SELECT pg_tde_add_database_key_provider_file('file-vault', '/tmp/pg_tde_test_keyring.per');" + "SELECT pg_tde_add_database_key_provider_file('file-vault', '/tmp/pg_tde_test_001_basic.per');" ); PGTDE::psql($node, 'postgres', @@ -56,6 +56,12 @@ $strings .= `strings $tablefile | grep foo`; PGTDE::append_to_result_file($strings); + +# An encrypted table can be dropped even if we don't have access to the principal key. +$node->stop; +unlink('/tmp/pg_tde_test_001_basic.per'); +$node->start; +PGTDE::psql($node, 'postgres', 'SELECT pg_tde_verify_key()'); PGTDE::psql($node, 'postgres', 'DROP TABLE test_enc;'); PGTDE::psql($node, 'postgres', 'DROP EXTENSION pg_tde;'); diff --git a/contrib/pg_tde/t/expected/001_basic.out b/contrib/pg_tde/t/expected/001_basic.out index ff9315540cd75..fc7e373f111cc 100644 --- a/contrib/pg_tde/t/expected/001_basic.out +++ b/contrib/pg_tde/t/expected/001_basic.out @@ -8,7 +8,7 @@ SELECT extname, extversion FROM pg_extension WHERE extname = 'pg_tde'; CREATE TABLE test_enc (id SERIAL, k INTEGER, PRIMARY KEY (id)) USING tde_heap; psql::1: ERROR: principal key not configured HINT: create one using pg_tde_set_key before using encrypted tables -SELECT pg_tde_add_database_key_provider_file('file-vault', '/tmp/pg_tde_test_keyring.per'); +SELECT pg_tde_add_database_key_provider_file('file-vault', '/tmp/pg_tde_test_001_basic.per'); pg_tde_add_database_key_provider_file --------------------------------------- 1 @@ -39,5 +39,7 @@ SELECT * FROM test_enc ORDER BY id; TABLEFILE FOUND: yes CONTAINS FOO (should be empty): +SELECT pg_tde_verify_key() +psql::1: ERROR: failed to retrieve principal key test-db-key from keyring with ID 1 DROP TABLE test_enc; DROP EXTENSION pg_tde;