8000 [PG-1583, PG-1600] Do not unnecessarily try to fetch the principal key by AndersAstrand · Pull Request #330 · percona/postgres · GitHub
[go: up one dir, main page]
Skip to content

[PG-1583, PG-1600] Do not unnecessarily try to fetch the principal key #330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 14, 2025
Merged

[PG-1583, PG-1600] Do not unnecessarily try to fetch the principal key #330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a8f4a38
Use an enum instead of bool for encryption status
AndersAstrand May 13, 2025
5816caa
Do not use principal key to check encryption status
AndersAstrand May 13, 2025
1a04d5d
Defer decryption of relation keys
AndersAstrand May 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions contrib/pg_tde/src/access/pg_tde_tdemap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions contrib/pg_tde/src/common/pg_tde_utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand All @@ -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 */
Expand Down
1 change: 1 addition & 0 deletions contrib/pg_tde/src/include/access/pg_tde_tdemap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
85 changes: 68 additions & 17 deletions contrib/pg_tde/src/smgr/pg_tde_smgr.c
View file
Open in desktop
B41A F438
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,18 @@
#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,

/* This is an encrypted relation, but we haven't loaded the key yet. */
RELATION_KEY_NOT_AVAILABLE = 2,
} TDEMgrRelationDataEncryptionStatus;

typedef struct TDESMgrRelationData
{
/* parent data */
Expand All @@ -19,14 +31,24 @@ 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;

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)
{
Expand Down Expand Up @@ -58,6 +80,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;
}
}
Expand All @@ -70,18 +93,26 @@ 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->encrypted_relation)
if (tdereln->encryption_status == RELATION_NOT_ENCRYPTED)
{
mdwritev(reln, forknum, blocknum, buffers, nblocks, skipFsync);
}
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;
Expand All @@ -102,6 +133,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)
{
Expand All @@ -119,7 +155,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);
}
}
Expand All @@ -129,18 +165,26 @@ tde_mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
const void *buffer, bool skipFsync)
{
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);
}
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);
Expand All @@ -156,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->encrypted_relation)
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)
{
Expand Down Expand Up @@ -233,37 +284,37 @@ 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;
}
}
}

/*
* 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->encrypted_relation = true;
tdereln->relKey = *key;
tdereln->encryption_status = RELATION_KEY_NOT_AVAILABLE;
}
else
{
tdereln->encrypted_relation = false;
tdereln->encryption_status = RELATION_NOT_ENCRYPTED;
}
}

Expand Down
8 changes: 7 additions & 1 deletion contrib/pg_tde/t/001_basic.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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;');
Expand Down
4 changes: 3 additions & 1 deletion contrib/pg_tde/t/expected/001_basic.out
View file
Open in desktop
Orig 10000 inal file line number Diff line number Diff line change
Expand Up @@ -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:<stdin>: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
Expand Down Expand Up @@ -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:<stdin>:1: ERROR: failed to retrieve principal key test-db-key from keyring with ID 1
DROP TABLE test_enc;
DROP EXTENSION pg_tde;
12 changes: 10 additions & 2 deletions contrib/pg_tde/t/expected/010_change_key_provider.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,11 @@ SELECT * FROM test_enc ORDER BY id;
SELECT pg_tde_verify_key();
psql:<stdin>:1: ERROR: failed to retrieve principal key test-key from keyring with ID 1
SELECT pg_tde_is_encrypted('test_enc');
psql:<stdin>: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:<stdin>: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
Expand Down Expand Up @@ -191,7 +195,11 @@ SELECT pg_tde_change_database_key_provider_file('file-vault', '/tmp/change_key_p
SELECT pg_tde_verify_key();
psql:<stdin>: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:<stdin>: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:<stdin>: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;
Expand Down
2 changes: 1 addition & 1 deletion src/bin/pg_checksums/pg_checksums.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
0