Skip to content

[PG-1637] Remove any existing keys for tables on create #388

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
Jun 9, 2025
Merged

[PG-1637] Remove any existing keys for tables on create #388

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
89f4ada
Make tde_mdcreate more strict in it's behavior
AndersAstrand Jun 5, 2025
36766f5
Move WAL logging of relation key creation to smgr
AndersAstrand Jun 9, 2025
45303eb
Delete relation keys before create
AndersAstrand Jun 9, 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
16 changes: 1 addition & 15 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 @@ -76,13 +76,10 @@ static void finalize_key_rotation(const char *path_old, const char *path_new);
static int pg_tde_open_file_write(const char *tde_filename, const TDESignedPrincipalKeyInfo *signed_key_info, bool truncate, off_t *curr_pos);

void
pg_tde_save_smgr_key(RelFileLocator rel, const InternalKey *rel_key_data, bool write_xlog)
pg_tde_save_smgr_key(RelFileLocator rel, const InternalKey *rel_key_data)
{
TDEPrincipalKey *principal_key;
LWLock *lock_pk = tde_lwlock_enc_keys();
XLogRelKey xlrec = {
.rlocator = rel,
};

LWLockAcquire(lock_pk, LW_EXCLUSIVE);
principal_key = GetPrincipalKey(rel.dbOid, LW_EXCLUSIVE);
Expand All @@ -95,17 +92,6 @@ pg_tde_save_smgr_key(RelFileLocator rel, const InternalKey *rel_key_data, bool w

pg_tde_write_key_map_entry(&rel, rel_key_data, principal_key);
LWLockRelease(lock_pk);

if (write_xlog)
{
/*
* It is fine to write the to WAL after writing to the file since we
* have not WAL logged the SMGR CREATE event either.
*/
XLogBeginInsert();
XLogRegisterData((char *) &xlrec, sizeof(xlrec));
XLogInsert(RM_TDERMGR_ID, XLOG_TDE_ADD_RELATION_KEY);
}
}

const char *
Expand Down
6 changes: 6 additions & 0 deletions contrib/pg_tde/src/access/pg_tde_xlog.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ tdeheap_rmgr_redo(XLogReaderState *record)

pg_tde_save_principal_key_redo(mkey);
}
else if (info == XLOG_TDE_REMOVE_RELATION_KEY)
{
XLogRelKey *xlrec = (XLogRelKey *) XLogRecGetData(record);

tde_smgr_delete_key_redo(&xlrec->rlocator);
}
else if (info == XLOG_TDE_ROTATE_PRINCIPAL_KEY)
{
XLogPrincipalKeyRotate *xlrec = (XLogPrincipalKeyRotate *) XLogRecGetData(record);
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -93,7 +93,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 void pg_tde_save_smgr_key(RelFileLocator rel, const InternalKey *key, bool write_xlog);
extern void pg_tde_save_smgr_key(RelFileLocator rel, const InternalKey *key);
extern bool pg_tde_has_smgr_key(RelFileLocator rel);
extern InternalKey *pg_tde_get_smgr_key(RelFileLocator rel);
extern void pg_tde_free_key_map_entry(RelFileLocator rel);
Expand Down
1 change: 1 addition & 0 deletions contrib/pg_tde/src/include/access/pg_tde_xlog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#define XLOG_TDE_ROTATE_PRINCIPAL_KEY 0x20
#define XLOG_TDE_WRITE_KEY_PROVIDER 0x30
#define XLOG_TDE_INSTALL_EXTENSION 0x40
#define XLOG_TDE_REMOVE_RELATION_KEY 0x50

/* ID 140 is registered for Percona TDE extension: https://wiki.postgresql.org/wiki/CustomWALResourceManagers */
#define RM_TDERMGR_ID 140
Expand Down
1 change: 1 addition & 0 deletions contrib/pg_tde/src/include/smgr/pg_tde_smgr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

extern void RegisterStorageMgr(void);
extern void tde_smgr_create_key_redo(const RelFileLocator *rlocator);
extern void tde_smgr_delete_key_redo(const RelFileLocator *rlocator);
extern bool tde_smgr_rel_is_encrypted(SMgrRelation reln);

#endif /* PG_TDE_SMGR_H */
3 changes: 1 addition & 2 deletions contrib/pg_tde/src/pg_tde.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ _PG_init(void)

check_percona_api_version();

pg_tde_init_data_dir();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was the reason to move it here? I'm not objecting it, just curious

Copy link
Collaborator Author

@AndersAstrand AndersAstrand Jun 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the commit message, this directory is used by the SMgr, and the SMgr is loaded and used regardless of whether any database have run CREATE EXTENSION. So the directory should be created regardless.

Practically it was because the call to pg_free_key_map_entry() crashed when the directory didn't exist :).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry, I missed it. Thanks

AesInit();
TdeGucInit();
TdeEventCaptureInit();
Expand All @@ -113,8 +114,6 @@ _PG_init(void)
static void
extension_install(Oid databaseId)
{
/* Initialize the TDE dir */
pg_tde_init_data_dir();
key_provider_startup_cleanup(databaseId);
principal_key_startup_cleanup(databaseId);
}
Expand Down
99 changes: 79 additions & 20 deletions contrib/pg_tde/src/smgr/pg_tde_smgr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
#include "smgr/pg_tde_smgr.h"
#include "storage/smgr.h"
#include "storage/md.h"
#include "access/xloginsert.h"
#include "catalog/catalog.h"
#include "access/pg_tde_xlog.h"
#include "encryption/enc_aes.h"
#include "encryption/enc_tde.h"
#include "access/pg_tde_tdemap.h"
Expand Down Expand Up @@ -77,11 +79,23 @@ tde_smgr_create_key(const RelFileLocatorBackend *smgr_rlocator)
if (RelFileLocatorBackendIsTemp(*smgr_rlocator))
tde_smgr_save_temp_key(&smgr_rlocator->locator, key);
else
pg_tde_save_smgr_key(smgr_rlocator->locator, key, true);
pg_tde_save_smgr_key(smgr_rlocator->locator, key);

return key;
}

static void
tde_smgr_log_create_key(const RelFileLocatorBackend *smgr_rlocator)
{
XLogRelKey xlrec = {
.rlocator = smgr_rlocator->locator,
};

XLogBeginInsert();
XLogRegisterData((char *) &xlrec, sizeof(xlrec));
XLogInsert(RM_TDERMGR_ID, XLOG_TDE_ADD_RELATION_KEY);
}

void
tde_smgr_create_key_redo(const RelFileLocator *rlocator)
{
Expand All @@ -92,7 +106,27 @@ tde_smgr_create_key_redo(const RelFileLocator *rlocator)

pg_tde_generate_internal_key(&key, TDE_KEY_TYPE_SMGR);

pg_tde_save_smgr_key(*rlocator, &key, false);
pg_tde_save_smgr_key(*rlocator, &key);
}

static void
tde_smgr_delete_key(const RelFileLocatorBackend *smgr_rlocator)
{
XLogRelKey xlrec = {
.rlocator = smgr_rlocator->locator,
};

pg_tde_free_key_map_entry(smgr_rlocator->locator);

XLogBeginInsert();
XLogRegisterData((char *) &xlrec, sizeof(xlrec));
XLogInsert(RM_TDERMGR_ID, XLOG_TDE_REMOVE_RELATION_KEY);
}

void
tde_smgr_delete_key_redo(const RelFileLocator *rlocator)
{
pg_tde_free_key_map_entry(*rlocator);
}

static bool
Expand Down Expand Up @@ -322,6 +356,7 @@ static void
tde_mdcreate(RelFileLocator relold, SMgrRelation reln, ForkNumber forknum, bool isRedo)
{
TDESMgrRelation *tdereln = (TDESMgrRelation *) reln;
InternalKey *key;

/* Copied from mdcreate() in md.c */
if (isRedo && tdereln->md_num_open_segs[forknum] > 0)
Expand All @@ -334,36 +369,60 @@ tde_mdcreate(RelFileLocator relold, SMgrRelation reln, ForkNumber forknum, bool

mdcreate(relold, reln, forknum, isRedo);

if (forknum == MAIN_FORKNUM || forknum == INIT_FORKNUM)
if (forknum != MAIN_FORKNUM)
{
/*
* Only create keys when creating the main/init fork. Other forks can
* be created later, even during tde creation events. We definitely do
* Only create keys when creating the main fork. Other forks can be
* created later, even during tde creation events. We definitely do
* not want to create keys then, even later, when we encrypt all
* forks!
*
* Later calls then decide to encrypt or not based on the existence of
* the key.
*/
return;
}

if (!isRedo)
{
/*
* If we have a key for this relation already, we need to remove it.
* This can happen if OID is re-used after a crash left a key for a
* non-existing relation in the key file.
*
* Since event triggers do not fire on the standby or in recovery we
* do not try to generate any new keys and instead trust the xlog.
* If we're in redo, a separate WAL record will make sure the key is
* removed.
*/
InternalKey *key = tde_smgr_get_key(&reln->smgr_rlocator);
tde_smgr_delete_key(&reln->smgr_rlocator);
}

if (!isRedo && !key && tde_smgr_should_encrypt(&reln->smgr_rlocator, &relold))
key = tde_smgr_create_key(&reln->smgr_rlocator);
if (!tde_smgr_should_encrypt(&reln->smgr_rlocator, &relold))
{
tdereln->encryption_status = RELATION_NOT_ENCRYPTED;
return;
}

if (key)
{
tdereln->encryption_status = RELATION_KEY_AVAILABLE;
tdereln->relKey = *key;
pfree(key);
}
else
{
tdereln->encryption_status = RELATION_NOT_ENCRYPTED;
}
if (isRedo)
{
/*
* If we're in redo, the WAL record for creating the key has already
* happened and we can just fetch it.
*/
key = tde_smgr_get_key(&reln->smgr_rlocator);

Assert(key);
if (!key)
elog(ERROR, "could not get key when creating encrypted relation");
}
else
{
key = tde_smgr_create_key(&reln->smgr_rlocator);
tde_smgr_log_create_key(&reln->smgr_rlocator);
}

tdereln->encryption_status = RELATION_KEY_AVAILABLE;
tdereln->relKey = *key;
pfree(key);
}

/*
Expand Down