From 3e0639c390b817e059d4ea3d383a6d2f5306c3d2 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] 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 715203b01fa..83ee67be668 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 fc6e4951019..611e4b077e6 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', ); 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 @@ my $strings = 'CONTAINS FOO (should be empty): '; $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 ff9315540cd..fc7e373f111 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;