From 5d3a4506d03def516db7087cf29cf8d4891c3ded Mon Sep 17 00:00:00 2001 From: Hamid Akhtar Date: Sat, 16 Mar 2024 01:35:11 +0500 Subject: [PATCH 01/10] Initial version of the key rotation functionality without the xlog functionality. This patch includes some minor refactoring as well as changes the return type of the set key function which now returns a boolean. --- expected/insert_update_delete.out | 2 +- expected/keyprovider_dependency.out | 2 +- expected/move_large_tuples.out | 2 +- expected/multi_insert.out | 2 +- expected/non_sorted_off_compact.out | 2 +- expected/pgtde_is_encrypted.out | 2 +- expected/toast_decrypt.out | 2 +- expected/toast_extended_storage.out | 2 +- expected/trigger_on_view.out | 2 +- expected/update_compare_indexes.out | 2 +- expected/vault_v2_test.out | 2 +- pg_tde--1.0.sql | 4 +- src/access/pg_tde_tdemap.c | 141 +++++++++++++++++++-------- src/catalog/tde_keyring.c | 7 ++ src/catalog/tde_master_key.c | 89 ++++++++++++++--- src/include/access/pg_tde_tdemap.h | 8 +- src/include/catalog/tde_master_key.h | 11 ++- src/keyring/keyring_file.c | 7 +- 18 files changed, 217 insertions(+), 72 deletions(-) diff --git a/expected/insert_update_delete.out b/expected/insert_update_delete.out index ea7b9d42..956e0a24 100644 --- a/expected/insert_update_delete.out +++ b/expected/insert_update_delete.out @@ -8,7 +8,7 @@ SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per') SELECT pg_tde_set_master_key('test-db-master-key','file-vault'); pg_tde_set_master_key ----------------------- - + t (1 row) CREATE TABLE albums ( diff --git a/expected/keyprovider_dependency.out b/expected/keyprovider_dependency.out index 683945ab..33cd8b0d 100644 --- a/expected/keyprovider_dependency.out +++ b/expected/keyprovider_dependency.out @@ -20,7 +20,7 @@ SELECT pg_tde_add_key_provider_vault_v2('V2-vault','vault-token','percona.com/va SELECT pg_tde_set_master_key('test-db-master-key','mk-file'); pg_tde_set_master_key ----------------------- - + t (1 row) -- Try dropping the in-use key provider diff --git a/expected/move_large_tuples.out b/expected/move_large_tuples.out index c58eec9a..780d9c67 100644 --- a/expected/move_large_tuples.out +++ b/expected/move_large_tuples.out @@ -9,7 +9,7 @@ SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per') SELECT pg_tde_set_master_key('test-db-master-key','file-vault'); pg_tde_set_master_key ----------------------- - + t (1 row) CREATE TABLE sbtest2( diff --git a/expected/multi_insert.out b/expected/multi_insert.out index 3282c26c..11f5ea56 100644 --- a/expected/multi_insert.out +++ b/expected/multi_insert.out @@ -10,7 +10,7 @@ SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per') SELECT pg_tde_set_master_key('test-db-master-key','file-vault'); pg_tde_set_master_key ----------------------- - + t (1 row) CREATE TABLE albums ( diff --git a/expected/non_sorted_off_compact.out b/expected/non_sorted_off_compact.out index b8440f2a..972d7e84 100644 --- a/expected/non_sorted_off_compact.out +++ b/expected/non_sorted_off_compact.out @@ -10,7 +10,7 @@ SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per') SELECT pg_tde_set_master_key('test-db-master-key','file-vault'); pg_tde_set_master_key ----------------------- - + t (1 row) DROP TABLE IF EXISTS sbtest1; diff --git a/expected/pgtde_is_encrypted.out b/expected/pgtde_is_encrypted.out index 77966708..be395fb4 100644 --- a/expected/pgtde_is_encrypted.out +++ b/expected/pgtde_is_encrypted.out @@ -8,7 +8,7 @@ SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per') SELECT pg_tde_set_master_key('test-db-master-key','file-vault'); pg_tde_set_master_key ----------------------- - + t (1 row) CREATE TABLE test_enc( diff --git a/expected/toast_decrypt.out b/expected/toast_decrypt.out index 715893c8..06907dc3 100644 --- a/expected/toast_decrypt.out +++ b/expected/toast_decrypt.out @@ -8,7 +8,7 @@ SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per') SELECT pg_tde_set_master_key('test-db-master-key','file-vault'); pg_tde_set_master_key ----------------------- - + t (1 row) CREATE TABLE src (f1 TEXT STORAGE EXTERNAL) USING pg_tde; diff --git a/expected/toast_extended_storage.out b/expected/toast_extended_storage.out index e3cb91f9..4e4c1c82 100644 --- a/expected/toast_extended_storage.out +++ b/expected/toast_extended_storage.out @@ -9,7 +9,7 @@ SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per') SELECT pg_tde_set_master_key('test-db-master-key','file-vault'); pg_tde_set_master_key ----------------------- - + t (1 row) CREATE TEMP TABLE src (f1 text) USING pg_tde; diff --git a/expected/trigger_on_view.out b/expected/trigger_on_view.out index d81105fb..6750c603 100644 --- a/expected/trigger_on_view.out +++ b/expected/trigger_on_view.out @@ -8,7 +8,7 @@ SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per') SELECT pg_tde_set_master_key('test-db-master-key','file-vault'); pg_tde_set_master_key ----------------------- - + t (1 row) -- diff --git a/expected/update_compare_indexes.out b/expected/update_compare_indexes.out index d4b18069..c84d297e 100644 --- a/expected/update_compare_indexes.out +++ b/expected/update_compare_indexes.out @@ -8,7 +8,7 @@ SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per') SELECT pg_tde_set_master_key('test-db-master-key','file-vault'); pg_tde_set_master_key ----------------------- - + t (1 row) DROP TABLE IF EXISTS pvactst; diff --git a/expected/vault_v2_test.out b/expected/vault_v2_test.out index fac26c58..4eec680d 100644 --- a/expected/vault_v2_test.out +++ b/expected/vault_v2_test.out @@ -9,7 +9,7 @@ SELECT pg_tde_add_key_provider_vault_v2('vault-v2',:'root_token','http://127.0.0 SELECT pg_tde_set_master_key('vault-v2-master-key','vault-v2'); pg_tde_set_master_key ----------------------- - + t (1 row) CREATE TABLE test_enc( diff --git a/pg_tde--1.0.sql b/pg_tde--1.0.sql index 1048b8af..49c377b1 100644 --- a/pg_tde--1.0.sql +++ b/pg_tde--1.0.sql @@ -81,13 +81,13 @@ RETURNS boolean AS $$ SELECT amname = 'pg_tde' FROM pg_class INNER JOIN pg_am ON pg_am.oid = pg_class.relam WHERE relname = table_name $$ LANGUAGE SQL; -CREATE FUNCTION pg_tde_rotate_key(key_name VARCHAR) +CREATE FUNCTION pg_tde_rotate_key(new_master_key_name VARCHAR(255), new_provider_name VARCHAR(255)) RETURNS boolean AS 'MODULE_PATHNAME' LANGUAGE C; CREATE FUNCTION pg_tde_set_master_key(master_key_name VARCHAR(255), provider_name VARCHAR(255)) -RETURNS VOID +RETURNS boolean AS 'MODULE_PATHNAME' LANGUAGE C; diff --git a/src/access/pg_tde_tdemap.c b/src/access/pg_tde_tdemap.c index fb495a33..a15fcb67 100644 --- a/src/access/pg_tde_tdemap.c +++ b/src/access/pg_tde_tdemap.c @@ -87,13 +87,12 @@ static File pg_tde_file_header_read(char *tde_filename, File tde_file, TDEFileHe static RelKeyData* tde_create_rel_key(const RelFileLocator *rlocator, InternalKey *key, TDEMasterKeyInfo *master_key_info); static RelKeyData *tde_encrypt_rel_key(TDEMasterKey *master_key, RelKeyData *rel_key_data, const RelFileLocator *rlocator); static RelKeyData *tde_decrypt_rel_key(TDEMasterKey *master_key, RelKeyData *enc_rel_key_data, const RelFileLocator *rlocator); -static bool pg_tde_perform_rotate_key(const char *new_master_key_name); static void pg_tde_set_db_file_paths(Oid dbOid); static File pg_tde_open_file(char *tde_filename, TDEMasterKeyInfo *master_key_info, bool should_fill_info, int fileFlags, bool *is_new_file, off_t *offset); static int32 pg_tde_write_map_entry(const RelFileLocator *rlocator, char *db_map_path, TDEMasterKeyInfo *master_key_info); -static int32 pg_tde_write_one_map_entry(File map_file, const RelFileLocator *rlocator, int flags, int32 key_index, TDEMapEntry *map_entry, off_t *offset); +static off_t pg_tde_write_one_map_entry(File map_file, const RelFileLocator *rlocator, int flags, int32 key_index, TDEMapEntry *map_entry, off_t *offset); static int32 pg_tde_process_map_entry(const RelFileLocator *rlocator, char *db_map_path, off_t *offset, bool should_delete); static bool pg_tde_read_one_map_entry(File map_file, const RelFileLocator *rlocator, int flags, TDEMapEntry *map_entry, off_t *offset); @@ -115,7 +114,7 @@ pg_tde_create_key_map_entry(const RelFileLocator *newrlocator, Relation rel) TDEMasterKey *master_key; XLogRelKey xlrec; - master_key = GetMasterKey(newrlocator->dbOid); + master_key = GetMasterKey(); if (master_key == NULL) { ereport(ERROR, @@ -561,9 +560,11 @@ pg_tde_write_map_entry(const RelFileLocator *rlocator, char *db_map_path, TDEMas * Based on the given arguments, creates and write the entry into the key * map file. */ -int32 +off_t pg_tde_write_one_map_entry(File map_file, const RelFileLocator *rlocator, int flags, int32 key_index, TDEMapEntry *map_entry, off_t *offset) { + int bytes_written = 0; + Assert(map_entry); /* Fill in the map entry structure */ @@ -571,8 +572,10 @@ pg_tde_write_one_map_entry(File map_file, const RelFileLocator *rlocator, int fl map_entry->flags = flags; map_entry->key_index = key_index; + bytes_written = FileWrite(map_file, map_entry, MAP_ENTRY_SIZE, *offset, WAIT_EVENT_DATA_FILE_WRITE); + /* Add the entry to the file */ - if (FileWrite(map_file, map_entry, MAP_ENTRY_SIZE, *offset, WAIT_EVENT_DATA_FILE_WRITE) != MAP_ENTRY_SIZE) + if (bytes_written != MAP_ENTRY_SIZE) { ereport(FATAL, (errcode_for_file_access(), @@ -580,7 +583,7 @@ pg_tde_write_one_map_entry(File map_file, const RelFileLocator *rlocator, int fl db_map_path))); } - return key_index; + return (*offset + bytes_written); } /* @@ -929,7 +932,7 @@ pg_tde_get_key_from_file(const RelFileLocator *rlocator) Assert(rlocator); /* Get/generate a master, create the key for relation and get the encrypted key with bytes to write */ - master_key = GetMasterKey(rlocator->dbOid); + master_key = GetMasterKey(); if (master_key == NULL) { ereport(ERROR, @@ -949,45 +952,105 @@ pg_tde_get_key_from_file(const RelFileLocator *rlocator) return rel_key_data; } -PG_FUNCTION_INFO_V1(pg_tde_rotate_key); -Datum -pg_tde_rotate_key(PG_FUNCTION_ARGS) -{ - const char *new_key; - bool ret; - - if (PG_ARGISNULL(0)) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("new master key name cannot be NULL"))); - - new_key = TextDatumGetCString(PG_GETARG_DATUM(0)); - ret = pg_tde_perform_rotate_key(new_key); - PG_RETURN_BOOL(ret); -} - /* * TODO: * - How do we get the old key name and the key itself? * - We need to specify this for a current or all databases? */ bool -pg_tde_perform_rotate_key(const char *new_master_key_name) +pg_tde_perform_rotate_key(TDEMasterKey *master_key, TDEMasterKey *new_master_key) { - /* - * Implementation: - * - Get names of the new and old master keys; either via arguments or function calls - * - Open old map file - * - Open old keydata file - * - Open new map file - * - Open new keydata file - * - Read old map entry using its index. - * - Write to new map file using its index. - * - Read keydata from old file - * - Decrypt it using the old master key - * - Encrypt it using the new master key - * - Write to new keydata file - */ +#define OLD_MASTER_KEY 0 +#define NEW_MASTER_KEY 1 +#define MASTER_KEY_COUNT 2 + + off_t curr_pos[MASTER_KEY_COUNT] = {0}; + off_t prev_pos[MASTER_KEY_COUNT] = {0}; + int32 key_index[MASTER_KEY_COUNT] = {0}; + RelKeyData *rel_key_data[MASTER_KEY_COUNT]; + RelKeyData *enc_rel_key_data[MASTER_KEY_COUNT]; + File m_file[MASTER_KEY_COUNT] = {-1}; + File k_file[MASTER_KEY_COUNT] = {-1}; + char m_path[MASTER_KEY_COUNT][MAXPGPATH]; + char k_path[MASTER_KEY_COUNT][MAXPGPATH]; + TDEMapEntry map_entry; + RelFileLocator rloc; + bool found = false; + off_t read_pos_tmp = 0; + bool is_new_file; + /* Set the file paths */ + pg_tde_set_db_file_paths(master_key->keyInfo.databaseId); + + /* Let's update the pathnames in the local variable for ease of use/readability */ + strncpy(m_path[OLD_MASTER_KEY], db_map_path, MAXPGPATH); + strncpy(k_path[OLD_MASTER_KEY], db_keydata_path, MAXPGPATH); + + /* Set the file name and postfix it with .r to identify it as part of a key rotation process. */ + snprintf(m_path[NEW_MASTER_KEY], MAXPGPATH, "%s.r", m_path[OLD_MASTER_KEY]); + snprintf(k_path[NEW_MASTER_KEY], MAXPGPATH, "%s.r", k_path[OLD_MASTER_KEY]); + + /* Open both files in read only mode. */ + m_file[OLD_MASTER_KEY] = pg_tde_open_file(m_path[OLD_MASTER_KEY], &master_key->keyInfo, false, O_RDONLY, &is_new_file, &curr_pos[OLD_MASTER_KEY]); + k_file[OLD_MASTER_KEY] = pg_tde_open_file(k_path[OLD_MASTER_KEY], &master_key->keyInfo, false, O_RDONLY, &is_new_file, &read_pos_tmp); + + /* Create both files, truncate if the rotate file already exits */ + m_file[NEW_MASTER_KEY] = pg_tde_open_file(m_path[NEW_MASTER_KEY], &new_master_key->keyInfo, false, O_RDWR | O_CREAT | O_TRUNC, &is_new_file, &curr_pos[NEW_MASTER_KEY]); + k_file[NEW_MASTER_KEY] = pg_tde_open_file(k_path[NEW_MASTER_KEY], &new_master_key->keyInfo, false, O_RDWR | O_CREAT | O_TRUNC, &is_new_file, &read_pos_tmp); + + /* Read all entries until EOF */ + for(key_index[OLD_MASTER_KEY] = 0; ; key_index[OLD_MASTER_KEY]++) + { + prev_pos[OLD_MASTER_KEY] = curr_pos[OLD_MASTER_KEY]; + found = pg_tde_read_one_map_entry(m_file[OLD_MASTER_KEY], NULL, MAP_ENTRY_VALID, &map_entry, &curr_pos[OLD_MASTER_KEY]); + + /* We either reach EOF */ + if (prev_pos[OLD_MASTER_KEY] == curr_pos[OLD_MASTER_KEY]) + break; + + /* We didn't find a valid entry */ + if (found == false) + continue; + + /* Set the relNumber of rlocator. Ignore the tablespace Oid since we only place our files under the default. */ + rloc.relNumber = map_entry.relNumber; + rloc.dbOid = master_key->keyInfo.databaseId; + rloc.spcOid = DEFAULTTABLESPACE_OID; + + /* Let's get the decrypted key and re-encrypt it with the new key. */ + enc_rel_key_data[OLD_MASTER_KEY] = pg_tde_read_one_keydata(k_file[OLD_MASTER_KEY], key_index[OLD_MASTER_KEY], master_key); + + /* Decrypt and re-encrypt keys */ + rel_key_data[OLD_MASTER_KEY] = tde_decrypt_rel_key(master_key, enc_rel_key_data[OLD_MASTER_KEY], &rloc); + enc_rel_key_data[NEW_MASTER_KEY] = tde_encrypt_rel_key(new_master_key, rel_key_data[OLD_MASTER_KEY], &rloc); + + /* Write the given entry at the location pointed by prev_pos */ + prev_pos[NEW_MASTER_KEY] = curr_pos[NEW_MASTER_KEY]; + curr_pos[NEW_MASTER_KEY] = pg_tde_write_one_map_entry(m_file[NEW_MASTER_KEY], &rloc, MAP_ENTRY_VALID, key_index[NEW_MASTER_KEY], &map_entry, &prev_pos[NEW_MASTER_KEY]); + pg_tde_write_one_keydata(k_file[NEW_MASTER_KEY], key_index[NEW_MASTER_KEY], enc_rel_key_data[NEW_MASTER_KEY]); + + /* Increment the key index for the new master key */ + key_index[NEW_MASTER_KEY]++; + } + + /* Close open files */ + FileClose(m_file[OLD_MASTER_KEY]); + FileClose(k_file[OLD_MASTER_KEY]); + FileClose(m_file[NEW_MASTER_KEY]); + FileClose(k_file[NEW_MASTER_KEY]); + + /* Remove old files */ + durable_unlink(m_path[OLD_MASTER_KEY], LOG); + durable_unlink(k_path[OLD_MASTER_KEY], LOG); + + /* Rename the new files to required filenames */ + durable_rename(m_path[NEW_MASTER_KEY], m_path[OLD_MASTER_KEY], LOG); + durable_rename(k_path[NEW_MASTER_KEY], k_path[OLD_MASTER_KEY], LOG); + + /* TODO: Remove the existing ones from cache etc. */ return true; + +#undef OLD_MASTER_KEY +#undef NEW_MASTER_KEY +#undef MASTER_KEY_COUNT } diff --git a/src/catalog/tde_keyring.c b/src/catalog/tde_keyring.c index 57e65559..afd8128c 100644 --- a/src/catalog/tde_keyring.c +++ b/src/catalog/tde_keyring.c @@ -164,6 +164,13 @@ GetKeyProviderByName(const char *provider_name) heap_endscan(scan); relation_close(kp_table_relation, AccessShareLock); + if (keyring == NULL) + { + ereport(ERROR, + (errmsg("Key provider \"%s\" does not exists", provider_name), + errhint("Use create_key_provider interface to create the key provider"))); + } + return keyring; } diff --git a/src/catalog/tde_master_key.c b/src/catalog/tde_master_key.c index 07331bc3..5aa6d4e0 100644 --- a/src/catalog/tde_master_key.c +++ b/src/catalog/tde_master_key.c @@ -29,6 +29,8 @@ #include "access/pg_tde_tdemap.h" +#define DEFAULT_MASTER_KEY_VERSION 1 + typedef struct TdeMasterKeySharedState { LWLock *Lock; @@ -191,7 +193,7 @@ create_master_key_info(TDEMasterKey *master_key, GenericKeyring *keyring) masterKeyInfo = palloc(sizeof(TDEMasterKeyInfo)); masterKeyInfo->databaseId = MyDatabaseId; masterKeyInfo->tablespaceId = MyDatabaseTableSpace; - masterKeyInfo->keyId.version = 1; + masterKeyInfo->keyId.version = DEFAULT_MASTER_KEY_VERSION; gettimeofday(&masterKeyInfo->creationTime, NULL); strncpy(masterKeyInfo->keyId.name, master_key->keyInfo.keyId.name, MASTER_KEY_NAME_LEN); masterKeyInfo->keyringId = keyring->key_id; @@ -215,13 +217,14 @@ save_master_key_info(TDEMasterKeyInfo *master_key_info) * throws an error. */ TDEMasterKey * -GetMasterKey(Oid dbOid) +GetMasterKey(void) { TDEMasterKey *masterKey = NULL; TDEMasterKeyInfo *masterKeyInfo = NULL; GenericKeyring *keyring = NULL; const keyInfo *keyInfo = NULL; KeyringReturnCodes keyring_ret; + Oid dbOid = MyDatabaseId; masterKey = get_master_key_from_cache(dbOid, true); if (masterKey) @@ -326,7 +329,7 @@ set_master_key_with_keyring(const char *key_name, GenericKeyring *keyring) masterKey = palloc(sizeof(TDEMasterKey)); masterKey->keyInfo.databaseId = MyDatabaseId; - masterKey->keyInfo.keyId.version = 1; + masterKey->keyInfo.keyId.version = DEFAULT_MASTER_KEY_VERSION; masterKey->keyInfo.keyringId = keyring->key_id; strncpy(masterKey->keyInfo.keyId.name, key_name, TDE_KEY_NAME_LEN); /* We need to get the key from keyring */ @@ -373,18 +376,63 @@ set_master_key_with_keyring(const char *key_name, GenericKeyring *keyring) return masterKey; } -TDEMasterKey * +bool SetMasterKey(const char *key_name, const char *provider_name) { - GenericKeyring *keyring = GetKeyProviderByName(provider_name); - if (keyring == NULL) + TDEMasterKey *master_key = set_master_key_with_keyring(key_name, GetKeyProviderByName(provider_name)); + + return (master_key != NULL); +} + +bool +RotateMasterKey(const char *new_key_name, const char *new_provider_name) +{ + TDEMasterKey *master_key = GetMasterKey(); + TDEMasterKey new_master_key; + const keyInfo *keyInfo = NULL; + KeyringReturnCodes keyring_ret; + GenericKeyring *keyring; + + /* + * Let's set everything the same as the older master key and + * update only the required attributes. + * */ + memcpy(&new_master_key, master_key, sizeof(TDEMasterKey)); + + if (new_key_name == NULL) + { + new_master_key.keyInfo.keyId.version++; + } + else + { + strncpy(new_master_key.keyInfo.keyId.name, new_key_name, sizeof(new_master_key.keyInfo.keyId.name)); + new_master_key.keyInfo.keyId.version = DEFAULT_MASTER_KEY_VERSION; + + if (new_provider_name != NULL) + { + new_master_key.keyInfo.keyringId = GetKeyProviderByName(new_provider_name)->key_id; + } + } + + /* We need a valid keyring structure */ + keyring = GetKeyProviderByID(new_master_key.keyInfo.keyringId); + + /* Retrieve or generate new key */ + keyInfo = KeyringGetKey(keyring, new_key_name, false, &keyring_ret); + + if (keyInfo == NULL) + keyInfo = KeyringGenerateNewKeyAndStore(keyring, new_key_name, INTERNAL_KEY_LEN, false); + + if (keyInfo == NULL) { ereport(ERROR, - (errmsg("Key provider \"%s\" does not exists", provider_name), - errhint("Use create_key_provider interface to create the key provider"))); - return NULL; + (errmsg("failed to retrieve master key"))); } - return set_master_key_with_keyring(key_name, keyring); + + new_master_key.keyLength = keyInfo->data.len; + memcpy(new_master_key.keyData, keyInfo->data.data, keyInfo->data.len); + + return pg_tde_perform_rotate_key(master_key, &new_master_key); } /* @@ -535,8 +583,25 @@ Datum pg_tde_set_master_key(PG_FUNCTION_ARGS) { char *master_key_name = text_to_cstring(PG_GETARG_TEXT_PP(0)); char *provider_name = text_to_cstring(PG_GETARG_TEXT_PP(1)); + bool ret; ereport(LOG, (errmsg("Setting master key [%s : %s] for the database", master_key_name, provider_name))); - SetMasterKey(master_key_name, provider_name); - PG_RETURN_NULL(); + ret = SetMasterKey(master_key_name, provider_name); + PG_RETURN_BOOL(ret); +} + +/* + * SQL interface for key rotation + */ +PG_FUNCTION_INFO_V1(pg_tde_rotate_key); +Datum +pg_tde_rotate_key(PG_FUNCTION_ARGS) +{ + char *new_master_key_name = text_to_cstring(PG_GETARG_TEXT_PP(0)); + char *new_provider_name = text_to_cstring(PG_GETARG_TEXT_PP(1)); + bool ret; + + ereport(LOG, (errmsg("Rotating master key to [%s : %s] for the database", new_master_key_name, new_provider_name))); + ret = RotateMasterKey(new_master_key_name, new_provider_name); + PG_RETURN_BOOL(ret); } diff --git a/src/include/access/pg_tde_tdemap.h b/src/include/access/pg_tde_tdemap.h index ddd5450f..ff2db9f0 100644 --- a/src/include/access/pg_tde_tdemap.h +++ b/src/include/access/pg_tde_tdemap.h @@ -46,16 +46,20 @@ typedef struct XLogRelKey RelKeyData relKey; } XLogRelKey; +extern void pg_tde_create_key_map_entry(const RelFileLocator *newrlocator, Relation rel); extern void pg_tde_write_key_map_entry(const RelFileLocator *rlocator, RelKeyData *enc_rel_key_data, TDEMasterKeyInfo *master_key_info); extern void pg_tde_delete_key_map_entry(const RelFileLocator *rlocator); extern void pg_tde_free_key_map_entry(const RelFileLocator *rlocator, off_t offset); -extern void pg_tde_create_key_map_entry(const RelFileLocator *newrlocator, Relation rel); + extern RelKeyData *pg_tde_get_key_from_fork(const RelFileLocator *rlocator); extern RelKeyData *GetRelationKey(RelFileLocator rel); + extern void pg_tde_cleanup_path_vars(void); extern void pg_tde_delete_tde_files(Oid dbOid); -extern bool pg_tde_save_master_key(TDEMasterKeyInfo *master_key_info); + extern TDEMasterKeyInfo *pg_tde_get_master_key(Oid dbOid); +extern bool pg_tde_save_master_key(TDEMasterKeyInfo *master_key_info); +extern bool pg_tde_perform_rotate_key(TDEMasterKey *master_key, TDEMasterKey *new_master_key); const char * tde_sprint_key(InternalKey *k); diff --git a/src/include/catalog/tde_master_key.h b/src/include/catalog/tde_master_key.h index b133ec5a..d51c1bf7 100644 --- a/src/include/catalog/tde_master_key.h +++ b/src/include/catalog/tde_master_key.h @@ -48,10 +48,13 @@ typedef struct XLogMasterKeyCleanup } XLogMasterKeyCleanup; extern void InitializeMasterKeyInfo(void); -extern TDEMasterKey* GetMasterKey(Oid dbOid); -extern TDEMasterKey* SetMasterKey(const char* key_name, const char* provider_name); -extern Oid GetMasterKeyProviderId(void); -extern bool save_master_key_info(TDEMasterKeyInfo *masterKeyInfo); extern void cleanup_master_key_info(Oid databaseId, Oid tablespaceId); +extern bool save_master_key_info(TDEMasterKeyInfo *masterKeyInfo); + +extern Oid GetMasterKeyProviderId(void); +extern TDEMasterKey* GetMasterKey(void); +extern bool SetMasterKey(const char *key_name, const char *provider_name); +extern bool RotateMasterKey(const char *new_key_name, const char *new_provider_name); + #endif /*PG_TDE_MASTER_KEY_H*/ diff --git a/src/keyring/keyring_file.c b/src/keyring/keyring_file.c index a717a2c0..3c5ba417 100644 --- a/src/keyring/keyring_file.c +++ b/src/keyring/keyring_file.c @@ -42,6 +42,8 @@ get_key_by_name(GenericKeyring* keyring, const char* key_name, bool throw_error, keyInfo* key = NULL; File file = -1; FileKeyring* file_keyring = (FileKeyring*)keyring; + off_t bytes_read = 0; + off_t curr_pos = 0; file = PathNameOpenFile(file_keyring->file_name, O_CREAT | O_RDWR | PG_BINARY); if (file < 0) @@ -55,8 +57,9 @@ get_key_by_name(GenericKeyring* keyring, const char* key_name, bool throw_error, key = palloc(sizeof(keyInfo)); while(true) { - off_t bytes_read = 0; - bytes_read = FileRead(file, key, sizeof(keyInfo), 0, WAIT_EVENT_DATA_FILE_READ); + bytes_read = FileRead(file, key, sizeof(keyInfo), curr_pos, WAIT_EVENT_DATA_FILE_READ); + curr_pos += bytes_read; + if (bytes_read == 0 ) { pfree(key); From f50f3ff6ee22f63b56ebeef6fb78698f7368665d Mon Sep 17 00:00:00 2001 From: Hamid Akhtar Date: Sat, 16 Mar 2024 01:36:01 +0500 Subject: [PATCH 02/10] Master key versioning implemented by Usama. New field of versioned_name is added. Co-authored-by: Muhammad Usama --- src/catalog/tde_master_key.c | 59 +++++++++++++++++++++++----- src/include/catalog/tde_master_key.h | 3 ++ 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/src/catalog/tde_master_key.c b/src/catalog/tde_master_key.c index 5aa6d4e0..d5b33217 100644 --- a/src/catalog/tde_master_key.c +++ b/src/catalog/tde_master_key.c @@ -71,7 +71,7 @@ static TDEMasterKeyInfo *create_master_key_info(TDEMasterKey *master_key, Generi static inline dshash_table *get_master_key_Hash(void); static TDEMasterKey *get_master_key_from_cache(Oid dbOid, bool acquire_lock); static void push_master_key_to_cache(TDEMasterKey *masterKey); -static TDEMasterKey *set_master_key_with_keyring(const char *key_name, GenericKeyring *keyring); +static TDEMasterKey *set_master_key_with_keyring(const char *key_name, GenericKeyring *keyring, bool ensure_new_key); static const TDEShmemSetupRoutine master_key_info_shmem_routine = { .init_shared_state = initialize_shared_state, @@ -249,7 +249,7 @@ GetMasterKey(void) return NULL; } - keyInfo = KeyringGetKey(keyring, masterKeyInfo->keyId.name, false, &keyring_ret); + keyInfo = KeyringGetKey(keyring, masterKeyInfo->keyId.versioned_name, false, &keyring_ret); if (keyInfo == NULL) { ereport(ERROR, @@ -283,7 +283,7 @@ GetMasterKey(void) */ static TDEMasterKey * -set_master_key_with_keyring(const char *key_name, GenericKeyring *keyring) +set_master_key_with_keyring(const char *key_name, GenericKeyring *keyring, bool ensure_new_key) { TDEMasterKey *masterKey = NULL; TDEMasterKeyInfo *masterKeyInfo = NULL; @@ -332,11 +332,30 @@ set_master_key_with_keyring(const char *key_name, GenericKeyring *keyring) masterKey->keyInfo.keyId.version = DEFAULT_MASTER_KEY_VERSION; masterKey->keyInfo.keyringId = keyring->key_id; strncpy(masterKey->keyInfo.keyId.name, key_name, TDE_KEY_NAME_LEN); + strncpy(masterKey->keyInfo.keyId.versioned_name, key_name, TDE_KEY_NAME_LEN); + /* We need to get the key from keyring */ + while (true) + { + keyInfo = KeyringGetKey(keyring, masterKey->keyInfo.keyId.versioned_name, false, &keyring_ret); + if (keyInfo == NULL || ensure_new_key == false) + break; + + masterKey->keyInfo.keyId.version++; + + snprintf(masterKey->keyInfo.keyId.versioned_name, TDE_KEY_NAME_LEN, "%s_%d", key_name, masterKey->keyInfo.keyId.version); + if (masterKey->keyInfo.keyId.version > MAX_MASTER_KEY_VERSION_NUM) + { + LWLockRelease(shared_state->Lock); + ereport(ERROR, + (errmsg("Failed to generate new key name"))); + break; + } + /* TODO: check if the key was not present or there was a problem with key provider*/ + } - keyInfo = KeyringGetKey(keyring, key_name, false, &keyring_ret); - if (keyInfo == NULL) /* TODO: check if the key was not present or there was a problem with key provider*/ - keyInfo = KeyringGenerateNewKeyAndStore(keyring, key_name, INTERNAL_KEY_LEN, false); + if (keyInfo == NULL) + keyInfo = KeyringGenerateNewKeyAndStore(keyring, masterKey->keyInfo.keyId.versioned_name, INTERNAL_KEY_LEN, false); if (keyInfo == NULL) { @@ -379,7 +398,7 @@ set_master_key_with_keyring(const char *key_name, GenericKeyring *keyring) bool SetMasterKey(const char *key_name, const char *provider_name) { - TDEMasterKey *master_key = set_master_key_with_keyring(key_name, GetKeyProviderByName(provider_name)); + TDEMasterKey *master_key = set_master_key_with_keyring(key_name, GetKeyProviderByName(provider_name), false); return (master_key != NULL); } @@ -392,6 +411,7 @@ RotateMasterKey(const char *new_key_name, const char *new_provider_name) const keyInfo *keyInfo = NULL; KeyringReturnCodes keyring_ret; GenericKeyring *keyring; + bool ensure_new_key = true; /* * Let's set everything the same as the older master key and @@ -406,6 +426,8 @@ RotateMasterKey(const char *new_key_name, const char *new_provider_name) else { strncpy(new_master_key.keyInfo.keyId.name, new_key_name, sizeof(new_master_key.keyInfo.keyId.name)); + strncpy(new_master_key.keyInfo.keyId.versioned_name, new_key_name, sizeof(new_master_key.keyInfo.keyId.name)); + new_master_key.keyInfo.keyId.version = DEFAULT_MASTER_KEY_VERSION; if (new_provider_name != NULL) @@ -417,11 +439,28 @@ RotateMasterKey(const char *new_key_name, const char *new_provider_name) /* We need a valid keyring structure */ keyring = GetKeyProviderByID(new_master_key.keyInfo.keyringId); - /* Retrieve or generate new key */ - keyInfo = KeyringGetKey(keyring, new_key_name, false, &keyring_ret); + + /* We need to get the key from keyring */ + while (true) + { + keyInfo = KeyringGetKey(keyring, new_master_key.keyInfo.keyId.versioned_name, false, &keyring_ret); + if (keyInfo == NULL || ensure_new_key == false) + break; + + new_master_key.keyInfo.keyId.version++; + + snprintf(new_master_key.keyInfo.keyId.versioned_name, TDE_KEY_NAME_LEN, "%s_%d", new_key_name, new_master_key.keyInfo.keyId.version); + + if (new_master_key.keyInfo.keyId.version > MAX_MASTER_KEY_VERSION_NUM) + { + ereport(ERROR, + (errmsg("Failed to generate new key name"))); + break; + } + } if (keyInfo == NULL) - keyInfo = KeyringGenerateNewKeyAndStore(keyring, new_key_name, INTERNAL_KEY_LEN, false); + keyInfo = KeyringGenerateNewKeyAndStore(keyring, new_master_key.keyInfo.keyId.versioned_name, INTERNAL_KEY_LEN, false); if (keyInfo == NULL) { diff --git a/src/include/catalog/tde_master_key.h b/src/include/catalog/tde_master_key.h index d51c1bf7..208fc890 100644 --- a/src/include/catalog/tde_master_key.h +++ b/src/include/catalog/tde_master_key.h @@ -17,11 +17,14 @@ #include "nodes/pg_list.h" #define MASTER_KEY_NAME_LEN TDE_KEY_NAME_LEN +#define MAX_MASTER_KEY_VERSION_NUM 1000 +#define DEFAULUT_MASTER_KEY_VERSION 1 typedef struct TDEMasterKeyId { uint32 version; char name[MASTER_KEY_NAME_LEN]; + char versioned_name[MASTER_KEY_NAME_LEN + 4]; } TDEMasterKeyId; typedef struct TDEMasterKeyInfo From d0d824f94db9b6f4a704ac6445b5ba71da097197 Mon Sep 17 00:00:00 2001 From: Hamid Akhtar Date: Sat, 16 Mar 2024 02:05:17 +0500 Subject: [PATCH 03/10] Fixing issue where regression was failing because of key name versioning. --- src/catalog/tde_master_key.c | 32 +++++--------------------------- 1 file changed, 5 insertions(+), 27 deletions(-) diff --git a/src/catalog/tde_master_key.c b/src/catalog/tde_master_key.c index d5b33217..7c2392bc 100644 --- a/src/catalog/tde_master_key.c +++ b/src/catalog/tde_master_key.c @@ -67,7 +67,6 @@ static int required_locks_count(void); static void shared_memory_shutdown(int code, Datum arg); static void master_key_startup_cleanup(int tde_tbl_count, void *arg); -static TDEMasterKeyInfo *create_master_key_info(TDEMasterKey *master_key, GenericKeyring *keyring); static inline dshash_table *get_master_key_Hash(void); static TDEMasterKey *get_master_key_from_cache(Oid dbOid, bool acquire_lock); static void push_master_key_to_cache(TDEMasterKey *masterKey); @@ -182,25 +181,6 @@ shared_memory_shutdown(int code, Datum arg) masterKeyLocalState.sharedMasterKeyState = NULL; } -static TDEMasterKeyInfo * -create_master_key_info(TDEMasterKey *master_key, GenericKeyring *keyring) -{ - TDEMasterKeyInfo *masterKeyInfo = NULL; - - Assert(master_key != NULL); - Assert(keyring != NULL); - - masterKeyInfo = palloc(sizeof(TDEMasterKeyInfo)); - masterKeyInfo->databaseId = MyDatabaseId; - masterKeyInfo->tablespaceId = MyDatabaseTableSpace; - masterKeyInfo->keyId.version = DEFAULT_MASTER_KEY_VERSION; - gettimeofday(&masterKeyInfo->creationTime, NULL); - strncpy(masterKeyInfo->keyId.name, master_key->keyInfo.keyId.name, MASTER_KEY_NAME_LEN); - masterKeyInfo->keyringId = keyring->key_id; - - return masterKeyInfo; -} - bool save_master_key_info(TDEMasterKeyInfo *master_key_info) { @@ -253,7 +233,7 @@ GetMasterKey(void) if (keyInfo == NULL) { ereport(ERROR, - (errmsg("failed to retrieve master key from keyring"))); + (errmsg("failed to retrieve master key from keyring %s", masterKeyInfo->keyId.versioned_name))); return NULL; } @@ -286,7 +266,6 @@ static TDEMasterKey * set_master_key_with_keyring(const char *key_name, GenericKeyring *keyring, bool ensure_new_key) { TDEMasterKey *masterKey = NULL; - TDEMasterKeyInfo *masterKeyInfo = NULL; TdeMasterKeySharedState *shared_state = masterKeyLocalState.sharedMasterKeyState; Oid dbOid = MyDatabaseId; @@ -304,8 +283,7 @@ set_master_key_with_keyring(const char *key_name, GenericKeyring *keyring, bool return NULL; } /* Check if valid master key info exists in the file. There is no need for a lock here as the key might be in the file and not in the cache, but it must be in the file if it's in the cache and we check the cache under the lock later. */ - masterKeyInfo = pg_tde_get_master_key(dbOid); - if (masterKeyInfo) + if (pg_tde_get_master_key(dbOid)) { ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT), @@ -333,6 +311,7 @@ set_master_key_with_keyring(const char *key_name, GenericKeyring *keyring, bool masterKey->keyInfo.keyringId = keyring->key_id; strncpy(masterKey->keyInfo.keyId.name, key_name, TDE_KEY_NAME_LEN); strncpy(masterKey->keyInfo.keyId.versioned_name, key_name, TDE_KEY_NAME_LEN); + gettimeofday(&masterKey->keyInfo.creationTime, NULL); /* We need to get the key from keyring */ while (true) @@ -367,12 +346,11 @@ set_master_key_with_keyring(const char *key_name, GenericKeyring *keyring, bool masterKey->keyLength = keyInfo->data.len; memcpy(masterKey->keyData, keyInfo->data.data, keyInfo->data.len); - masterKeyInfo = create_master_key_info(masterKey, keyring); - save_master_key_info(masterKeyInfo); + save_master_key_info(&masterKey->keyInfo); /* XLog the new key*/ XLogBeginInsert(); - XLogRegisterData((char *) masterKeyInfo, sizeof(TDEMasterKeyInfo)); + XLogRegisterData((char *) &masterKey->keyInfo, sizeof(TDEMasterKeyInfo)); XLogInsert(RM_TDERMGR_ID, XLOG_TDE_ADD_MASTER_KEY); push_master_key_to_cache(masterKey); From 00fc9ef5c6e91cc4ed413f7bc99ad6a27db0767e Mon Sep 17 00:00:00 2001 From: Hamid Akhtar Date: Sun, 17 Mar 2024 03:32:52 +0500 Subject: [PATCH 04/10] Completing changes for master key rotation. The functionality is feature complete and dev-tested. As part of this commit, XLog for key rotation is implemented and verified that during streaming replication, when master key is rotated, the key map and the data file on primary and standby are binary equivalent when the Xlog record is replayed on the standby. --- src/access/pg_tde_tdemap.c | 144 +++++++++++++++++++++++---- src/access/pg_tde_xlog.c | 12 +++ src/catalog/tde_master_key.c | 2 +- src/include/access/pg_tde_tdemap.h | 1 + src/include/access/pg_tde_xlog.h | 2 + src/include/catalog/tde_master_key.h | 11 +- 6 files changed, 150 insertions(+), 22 deletions(-) diff --git a/src/access/pg_tde_tdemap.c b/src/access/pg_tde_tdemap.c index a15fcb67..0b3530c5 100644 --- a/src/access/pg_tde_tdemap.c +++ b/src/access/pg_tde_tdemap.c @@ -102,6 +102,9 @@ static RelKeyData* pg_tde_get_key_from_file(const RelFileLocator *rlocator); static RelKeyData* pg_tde_read_keydata(char *db_keydata_path, int32 key_index, TDEMasterKey *master_key); static RelKeyData* pg_tde_read_one_keydata(File keydata_file, int32 key_index, TDEMasterKey *master_key); +static File keyrotation_init_file(TDEMasterKeyInfo *new_master_key_info, char *rotated_filename, char *filename, bool *is_new_file, off_t *curr_pos); +static void finalize_key_rotation(char *m_path_old, char *k_path_old, char *m_path_new, char *k_path_new); + /* * Generate an encrypted key for the relation and store it in the keymap file. */ @@ -135,8 +138,8 @@ pg_tde_create_key_map_entry(const RelFileLocator *newrlocator, Relation rel) rel_key_data = tde_create_rel_key(newrlocator, &int_key, &master_key->keyInfo); enc_rel_key_data = tde_encrypt_rel_key(master_key, rel_key_data, newrlocator); - /* - * XLOG internal key + /* + * XLOG internal key */ xlrec.rlocator = *newrlocator; xlrec.relKey = *enc_rel_key_data; @@ -953,9 +956,39 @@ pg_tde_get_key_from_file(const RelFileLocator *rlocator) } /* - * TODO: - * - How do we get the old key name and the key itself? - * - We need to specify this for a current or all databases? + * Accepts the unrotated filename and returns the rotation temp + * filename. Both the strings are expected to be of the size + * MAXPGPATH. + * + * No error checking by this function. + */ +File +keyrotation_init_file(TDEMasterKeyInfo *new_master_key_info, char *rotated_filename, char *filename, bool *is_new_file, off_t *curr_pos) +{ + /* Set the new filenames for the key rotation process - temporary at the moment */ + snprintf(rotated_filename, MAXPGPATH, "%s.r", filename); + + /* Create file, truncate if the rotate file already exits */ + return pg_tde_open_file(rotated_filename, new_master_key_info, false, O_RDWR | O_CREAT | O_TRUNC, is_new_file, curr_pos); +} + +/* + * Do the final steps in the key rotation. + */ +void +finalize_key_rotation(char *m_path_old, char *k_path_old, char *m_path_new, char *k_path_new) +{ + /* Remove old files */ + durable_unlink(m_path_old, LOG); + durable_unlink(k_path_old, LOG); + + /* Rename the new files to required filenames */ + durable_rename(m_path_new, m_path_old, LOG); + durable_rename(k_path_new, k_path_old, LOG); +} + +/* + * Rotate keys and generates the WAL record for it. */ bool pg_tde_perform_rotate_key(TDEMasterKey *master_key, TDEMasterKey *new_master_key) @@ -978,6 +1011,10 @@ pg_tde_perform_rotate_key(TDEMasterKey *master_key, TDEMasterKey *new_master_key bool found = false; off_t read_pos_tmp = 0; bool is_new_file; + off_t map_size; + off_t keydata_size; + XLogMasterKeyRotate *xlrec; + off_t xlrec_size; /* Set the file paths */ pg_tde_set_db_file_paths(master_key->keyInfo.databaseId); @@ -986,17 +1023,12 @@ pg_tde_perform_rotate_key(TDEMasterKey *master_key, TDEMasterKey *new_master_key strncpy(m_path[OLD_MASTER_KEY], db_map_path, MAXPGPATH); strncpy(k_path[OLD_MASTER_KEY], db_keydata_path, MAXPGPATH); - /* Set the file name and postfix it with .r to identify it as part of a key rotation process. */ - snprintf(m_path[NEW_MASTER_KEY], MAXPGPATH, "%s.r", m_path[OLD_MASTER_KEY]); - snprintf(k_path[NEW_MASTER_KEY], MAXPGPATH, "%s.r", k_path[OLD_MASTER_KEY]); - - /* Open both files in read only mode. */ + /* Open both files in read only mode. We don't need to track the current position of the keydata file. We always use the key index */ m_file[OLD_MASTER_KEY] = pg_tde_open_file(m_path[OLD_MASTER_KEY], &master_key->keyInfo, false, O_RDONLY, &is_new_file, &curr_pos[OLD_MASTER_KEY]); k_file[OLD_MASTER_KEY] = pg_tde_open_file(k_path[OLD_MASTER_KEY], &master_key->keyInfo, false, O_RDONLY, &is_new_file, &read_pos_tmp); - /* Create both files, truncate if the rotate file already exits */ - m_file[NEW_MASTER_KEY] = pg_tde_open_file(m_path[NEW_MASTER_KEY], &new_master_key->keyInfo, false, O_RDWR | O_CREAT | O_TRUNC, &is_new_file, &curr_pos[NEW_MASTER_KEY]); - k_file[NEW_MASTER_KEY] = pg_tde_open_file(k_path[NEW_MASTER_KEY], &new_master_key->keyInfo, false, O_RDWR | O_CREAT | O_TRUNC, &is_new_file, &read_pos_tmp); + m_file[NEW_MASTER_KEY] = keyrotation_init_file(&new_master_key->keyInfo, m_path[NEW_MASTER_KEY], m_path[OLD_MASTER_KEY], &is_new_file, &curr_pos[NEW_MASTER_KEY]); + k_file[NEW_MASTER_KEY] = keyrotation_init_file(&new_master_key->keyInfo, k_path[NEW_MASTER_KEY], k_path[OLD_MASTER_KEY], &is_new_file, &read_pos_tmp); /* Read all entries until EOF */ for(key_index[OLD_MASTER_KEY] = 0; ; key_index[OLD_MASTER_KEY]++) @@ -1033,19 +1065,40 @@ pg_tde_perform_rotate_key(TDEMasterKey *master_key, TDEMasterKey *new_master_key key_index[NEW_MASTER_KEY]++; } - /* Close open files */ + /* Close unrotated files */ FileClose(m_file[OLD_MASTER_KEY]); FileClose(k_file[OLD_MASTER_KEY]); + + /* Let's calculate sizes */ + map_size = FileSize(m_file[NEW_MASTER_KEY]); + keydata_size = FileSize(k_file[NEW_MASTER_KEY]); + xlrec_size = map_size + keydata_size + SizeoOfXLogMasterKeyRotate; + + /* palloc and fill in the structure */ + xlrec = (XLogMasterKeyRotate *) palloc(xlrec_size); + + xlrec->databaseId = master_key->keyInfo.databaseId; + xlrec->map_size = map_size; + xlrec->keydata_size = keydata_size; + + FileRead(m_file[NEW_MASTER_KEY], xlrec->buff, xlrec->map_size, 0, WAIT_EVENT_DATA_FILE_READ); + FileRead(k_file[NEW_MASTER_KEY], &xlrec->buff[xlrec->map_size], xlrec->keydata_size, 0, WAIT_EVENT_DATA_FILE_READ); + + /* Close the files */ FileClose(m_file[NEW_MASTER_KEY]); FileClose(k_file[NEW_MASTER_KEY]); - /* Remove old files */ - durable_unlink(m_path[OLD_MASTER_KEY], LOG); - durable_unlink(k_path[OLD_MASTER_KEY], LOG); + /* Insert the XLog record */ + XLogBeginInsert(); + XLogRegisterData((char *) xlrec, xlrec_size); + XLogInsert(RM_TDERMGR_ID, XLOG_TDE_ROTATE_KEY); - /* Rename the new files to required filenames */ - durable_rename(m_path[NEW_MASTER_KEY], m_path[OLD_MASTER_KEY], LOG); - durable_rename(k_path[NEW_MASTER_KEY], k_path[OLD_MASTER_KEY], LOG); + /* Do the final steps */ + finalize_key_rotation(m_path[OLD_MASTER_KEY], k_path[OLD_MASTER_KEY], + m_path[NEW_MASTER_KEY], k_path[NEW_MASTER_KEY]); + + /* Free up the palloc'ed data */ + pfree(xlrec); /* TODO: Remove the existing ones from cache etc. */ return true; @@ -1054,3 +1107,54 @@ pg_tde_perform_rotate_key(TDEMasterKey *master_key, TDEMasterKey *new_master_key #undef NEW_MASTER_KEY #undef MASTER_KEY_COUNT } + +/* + * Rotate keys on a standby. + */ +bool +xl_tde_perform_rotate_key(XLogMasterKeyRotate *xlrec) +{ + TDEFileHeader *fheader; + char m_path_new[MAXPGPATH]; + char k_path_new[MAXPGPATH]; + File m_file_new; + File k_file_new; + bool is_new_file; + off_t curr_pos = 0; + off_t read_pos_tmp = 0; + + /* Let's get the header. Buff should start with the map file header. */ + fheader = (TDEFileHeader *) xlrec->buff; + + ereport(LOG, (errmsg("xl_tde_rotate => %s", fheader->master_key_info.keyId.name))); + + /* Set the file paths */ + pg_tde_set_db_file_paths(fheader->master_key_info.databaseId); + + /* Initialize the new files and set the names */ + m_file_new = keyrotation_init_file(&fheader->master_key_info, m_path_new, db_map_path, &is_new_file, &curr_pos); + k_file_new = keyrotation_init_file(&fheader->master_key_info, k_path_new, db_keydata_path, &is_new_file, &read_pos_tmp); + + if (FileWrite(m_file_new, xlrec->buff, xlrec->map_size, 0, WAIT_EVENT_DATA_FILE_WRITE) != xlrec->map_size) + { + ereport(WARNING, + (errcode_for_file_access(), + errmsg("Could not write tde file \"%s\": %m", + m_path_new))); + } + + if (FileWrite(k_file_new, &xlrec->buff[xlrec->map_size], xlrec->keydata_size, 0, WAIT_EVENT_DATA_FILE_WRITE) != xlrec->keydata_size) + { + ereport(WARNING, + (errcode_for_file_access(), + errmsg("Could not write tde file \"%s\": %m", + k_path_new))); + } + + FileClose(m_file_new); + FileClose(k_file_new); + + finalize_key_rotation(db_map_path, db_keydata_path, m_path_new, k_path_new); + + return true; +} \ No newline at end of file diff --git a/src/access/pg_tde_xlog.c b/src/access/pg_tde_xlog.c index 5d530a77..b87344e9 100644 --- a/src/access/pg_tde_xlog.c +++ b/src/access/pg_tde_xlog.c @@ -46,6 +46,12 @@ pg_tde_rmgr_redo(XLogReaderState *record) cleanup_master_key_info(xlrec->databaseId, xlrec->tablespaceId); } + else if (info == XLOG_TDE_ROTATE_KEY) + { + XLogMasterKeyRotate *xlrec = (XLogMasterKeyRotate *) XLogRecGetData(record); + + xl_tde_perform_rotate_key(xlrec); + } else { elog(PANIC, "pg_tde_redo: unknown op code %u", info); @@ -75,6 +81,12 @@ pg_tde_rmgr_desc(StringInfo buf, XLogReaderState *record) appendStringInfo(buf, "cleanup tde master key info for db %u/%u", xlrec->databaseId, xlrec->tablespaceId); } + if (info == XLOG_TDE_ROTATE_KEY) + { + XLogMasterKeyRotate *xlrec = (XLogMasterKeyRotate *) XLogRecGetData(record); + + appendStringInfo(buf, "rotate master key for %u", xlrec->databaseId); + } } const char * diff --git a/src/catalog/tde_master_key.c b/src/catalog/tde_master_key.c index 7c2392bc..4c57bac4 100644 --- a/src/catalog/tde_master_key.c +++ b/src/catalog/tde_master_key.c @@ -233,7 +233,7 @@ GetMasterKey(void) if (keyInfo == NULL) { ereport(ERROR, - (errmsg("failed to retrieve master key from keyring %s", masterKeyInfo->keyId.versioned_name))); + (errmsg("failed to retrieve master key \"%s\" from keyring.", masterKeyInfo->keyId.versioned_name))); return NULL; } diff --git a/src/include/access/pg_tde_tdemap.h b/src/include/access/pg_tde_tdemap.h index ff2db9f0..b358e4d1 100644 --- a/src/include/access/pg_tde_tdemap.h +++ b/src/include/access/pg_tde_tdemap.h @@ -60,6 +60,7 @@ extern void pg_tde_delete_tde_files(Oid dbOid); extern TDEMasterKeyInfo *pg_tde_get_master_key(Oid dbOid); extern bool pg_tde_save_master_key(TDEMasterKeyInfo *master_key_info); extern bool pg_tde_perform_rotate_key(TDEMasterKey *master_key, TDEMasterKey *new_master_key); +extern bool xl_tde_perform_rotate_key(XLogMasterKeyRotate *xlrec); const char * tde_sprint_key(InternalKey *k); diff --git a/src/include/access/pg_tde_xlog.h b/src/include/access/pg_tde_xlog.h index 7c75857b..bc32c979 100644 --- a/src/include/access/pg_tde_xlog.h +++ b/src/include/access/pg_tde_xlog.h @@ -15,6 +15,8 @@ #define XLOG_TDE_ADD_RELATION_KEY 0x00 #define XLOG_TDE_ADD_MASTER_KEY 0x10 #define XLOG_TDE_CLEAN_MASTER_KEY 0x20 +#define XLOG_TDE_ROTATE_KEY 0x30 + /* TODO: ID has to be registedred and changed: https://wiki.postgresql.org/wiki/CustomWALResourceManagers */ #define RM_TDERMGR_ID RM_EXPERIMENTAL_ID #define RM_TDERMGR_NAME "test_pg_tde_custom_rmgr" diff --git a/src/include/catalog/tde_master_key.h b/src/include/catalog/tde_master_key.h index 208fc890..adb418f2 100644 --- a/src/include/catalog/tde_master_key.h +++ b/src/include/catalog/tde_master_key.h @@ -18,7 +18,6 @@ #define MASTER_KEY_NAME_LEN TDE_KEY_NAME_LEN #define MAX_MASTER_KEY_VERSION_NUM 1000 -#define DEFAULUT_MASTER_KEY_VERSION 1 typedef struct TDEMasterKeyId { @@ -44,6 +43,16 @@ typedef struct TDEMasterKey uint32 keyLength; } TDEMasterKey; +typedef struct XLogMasterKeyRotate +{ + Oid databaseId; + off_t map_size; + off_t keydata_size; + char buff[FLEXIBLE_ARRAY_MEMBER]; +} XLogMasterKeyRotate; + +#define SizeoOfXLogMasterKeyRotate offsetof(XLogMasterKeyRotate, buff) + typedef struct XLogMasterKeyCleanup { Oid databaseId; From 9d889976b930548e4351d6defff564c5ed3edae5 Mon Sep 17 00:00:00 2001 From: Hamid Akhtar Date: Tue, 19 Mar 2024 17:52:38 +0500 Subject: [PATCH 05/10] Resolving a couple of feedback coments on the PR. durable_.* now throws an error. --- src/access/pg_tde_tdemap.c | 8 ++++---- src/catalog/tde_master_key.c | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/access/pg_tde_tdemap.c b/src/access/pg_tde_tdemap.c index 0b3530c5..3f7eb583 100644 --- a/src/access/pg_tde_tdemap.c +++ b/src/access/pg_tde_tdemap.c @@ -979,12 +979,12 @@ void finalize_key_rotation(char *m_path_old, char *k_path_old, char *m_path_new, char *k_path_new) { /* Remove old files */ - durable_unlink(m_path_old, LOG); - durable_unlink(k_path_old, LOG); + durable_unlink(m_path_old, ERROR); + durable_unlink(k_path_old, ERROR); /* Rename the new files to required filenames */ - durable_rename(m_path_new, m_path_old, LOG); - durable_rename(k_path_new, k_path_old, LOG); + durable_rename(m_path_new, m_path_old, ERROR); + durable_rename(k_path_new, k_path_old, ERROR); } /* diff --git a/src/catalog/tde_master_key.c b/src/catalog/tde_master_key.c index 4c57bac4..b5dd530c 100644 --- a/src/catalog/tde_master_key.c +++ b/src/catalog/tde_master_key.c @@ -432,7 +432,7 @@ RotateMasterKey(const char *new_key_name, const char *new_provider_name) if (new_master_key.keyInfo.keyId.version > MAX_MASTER_KEY_VERSION_NUM) { ereport(ERROR, - (errmsg("Failed to generate new key name"))); + (errmsg("failed to retrieve master key"))); break; } } @@ -443,7 +443,7 @@ RotateMasterKey(const char *new_key_name, const char *new_provider_name) if (keyInfo == NULL) { ereport(ERROR, - (errmsg("failed to retrieve master key"))); + (errmsg("Failed to generate new key name"))); } new_master_key.keyLength = keyInfo->data.len; From fa4a9df783d0fc9ca32f74d3102026825c5b5738 Mon Sep 17 00:00:00 2001 From: Muhammad Usama Date: Wed, 20 Mar 2024 17:43:58 +0500 Subject: [PATCH 06/10] Few updates around master key versioning. The commit contains the following changes. Moved the logic for getting the latest available master key name to a separate function. Changed the base name for the master key. Now it always contains the version number postfix. Added a SQL interface to allow users to specify if they need to ensure the key during set and rotate master key operations. Also making a few adjustments in file keyring file handling. --- pg_tde--1.0.sql | 6 +- src/catalog/tde_master_key.c | 140 ++++++++++++++++----------- src/include/catalog/tde_master_key.h | 6 +- src/keyring/keyring_file.c | 30 +++--- src/keyring/keyring_vault.c | 2 + 5 files changed, 111 insertions(+), 73 deletions(-) diff --git a/pg_tde--1.0.sql b/pg_tde--1.0.sql index 49c377b1..2108dc94 100644 --- a/pg_tde--1.0.sql +++ b/pg_tde--1.0.sql @@ -81,12 +81,12 @@ RETURNS boolean AS $$ SELECT amname = 'pg_tde' FROM pg_class INNER JOIN pg_am ON pg_am.oid = pg_class.relam WHERE relname = table_name $$ LANGUAGE SQL; -CREATE FUNCTION pg_tde_rotate_key(new_master_key_name VARCHAR(255), new_provider_name VARCHAR(255)) +CREATE FUNCTION pg_tde_rotate_key(new_master_key_name VARCHAR(255) DEFAULT NULL, new_provider_name VARCHAR(255) DEFAULT NULL, ensure_new_key BOOLEAN DEFAULT TRUE) RETURNS boolean AS 'MODULE_PATHNAME' LANGUAGE C; -CREATE FUNCTION pg_tde_set_master_key(master_key_name VARCHAR(255), provider_name VARCHAR(255)) +CREATE FUNCTION pg_tde_set_master_key(master_key_name VARCHAR(255), provider_name VARCHAR(255), ensure_new_key BOOLEAN DEFAULT FALSE) RETURNS boolean AS 'MODULE_PATHNAME' LANGUAGE C; @@ -102,4 +102,4 @@ CREATE ACCESS METHOD pg_tde TYPE TABLE HANDLER pg_tdeam_handler; COMMENT ON ACCESS METHOD pg_tde IS 'pg_tde table access method'; -- Per database extension initialization -SELECT pg_tde_extension_initialize(); \ No newline at end of file +SELECT pg_tde_extension_initialize(); diff --git a/src/catalog/tde_master_key.c b/src/catalog/tde_master_key.c index b5dd530c..1e197239 100644 --- a/src/catalog/tde_master_key.c +++ b/src/catalog/tde_master_key.c @@ -66,6 +66,7 @@ static Size required_shared_mem_size(void); static int required_locks_count(void); static void shared_memory_shutdown(int code, Datum arg); static void master_key_startup_cleanup(int tde_tbl_count, void *arg); +static keyInfo *load_latest_versioned_key_name(TDEMasterKeyInfo *mastere_key_info, GenericKeyring *keyring, bool ensure_new_key); static inline dshash_table *get_master_key_Hash(void); static TDEMasterKey *get_master_key_from_cache(Oid dbOid, bool acquire_lock); @@ -303,35 +304,15 @@ set_master_key_with_keyring(const char *key_name, GenericKeyring *keyring, bool if (!masterKey) { const keyInfo *keyInfo = NULL; - KeyringReturnCodes keyring_ret; masterKey = palloc(sizeof(TDEMasterKey)); masterKey->keyInfo.databaseId = MyDatabaseId; masterKey->keyInfo.keyId.version = DEFAULT_MASTER_KEY_VERSION; masterKey->keyInfo.keyringId = keyring->key_id; strncpy(masterKey->keyInfo.keyId.name, key_name, TDE_KEY_NAME_LEN); - strncpy(masterKey->keyInfo.keyId.versioned_name, key_name, TDE_KEY_NAME_LEN); gettimeofday(&masterKey->keyInfo.creationTime, NULL); - /* We need to get the key from keyring */ - while (true) - { - keyInfo = KeyringGetKey(keyring, masterKey->keyInfo.keyId.versioned_name, false, &keyring_ret); - if (keyInfo == NULL || ensure_new_key == false) - break; - - masterKey->keyInfo.keyId.version++; - - snprintf(masterKey->keyInfo.keyId.versioned_name, TDE_KEY_NAME_LEN, "%s_%d", key_name, masterKey->keyInfo.keyId.version); - if (masterKey->keyInfo.keyId.version > MAX_MASTER_KEY_VERSION_NUM) - { - LWLockRelease(shared_state->Lock); - ereport(ERROR, - (errmsg("Failed to generate new key name"))); - break; - } - /* TODO: check if the key was not present or there was a problem with key provider*/ - } + keyInfo = load_latest_versioned_key_name(&masterKey->keyInfo, keyring, ensure_new_key); if (keyInfo == NULL) keyInfo = KeyringGenerateNewKeyAndStore(keyring, masterKey->keyInfo.keyId.versioned_name, INTERNAL_KEY_LEN, false); @@ -374,22 +355,20 @@ set_master_key_with_keyring(const char *key_name, GenericKeyring *keyring, bool } bool -SetMasterKey(const char *key_name, const char *provider_name) +SetMasterKey(const char *key_name, const char *provider_name, bool ensure_new_key) { - TDEMasterKey *master_key = set_master_key_with_keyring(key_name, GetKeyProviderByName(provider_name), false); + TDEMasterKey *master_key = set_master_key_with_keyring(key_name, GetKeyProviderByName(provider_name), ensure_new_key); return (master_key != NULL); } bool -RotateMasterKey(const char *new_key_name, const char *new_provider_name) +RotateMasterKey(const char *new_key_name, const char *new_provider_name, bool ensure_new_key) { TDEMasterKey *master_key = GetMasterKey(); TDEMasterKey new_master_key; const keyInfo *keyInfo = NULL; - KeyringReturnCodes keyring_ret; GenericKeyring *keyring; - bool ensure_new_key = true; /* * Let's set everything the same as the older master key and @@ -404,8 +383,6 @@ RotateMasterKey(const char *new_key_name, const char *new_provider_name) else { strncpy(new_master_key.keyInfo.keyId.name, new_key_name, sizeof(new_master_key.keyInfo.keyId.name)); - strncpy(new_master_key.keyInfo.keyId.versioned_name, new_key_name, sizeof(new_master_key.keyInfo.keyId.name)); - new_master_key.keyInfo.keyId.version = DEFAULT_MASTER_KEY_VERSION; if (new_provider_name != NULL) @@ -417,25 +394,7 @@ RotateMasterKey(const char *new_key_name, const char *new_provider_name) /* We need a valid keyring structure */ keyring = GetKeyProviderByID(new_master_key.keyInfo.keyringId); - - /* We need to get the key from keyring */ - while (true) - { - keyInfo = KeyringGetKey(keyring, new_master_key.keyInfo.keyId.versioned_name, false, &keyring_ret); - if (keyInfo == NULL || ensure_new_key == false) - break; - - new_master_key.keyInfo.keyId.version++; - - snprintf(new_master_key.keyInfo.keyId.versioned_name, TDE_KEY_NAME_LEN, "%s_%d", new_key_name, new_master_key.keyInfo.keyId.version); - - if (new_master_key.keyInfo.keyId.version > MAX_MASTER_KEY_VERSION_NUM) - { - ereport(ERROR, - (errmsg("failed to retrieve master key"))); - break; - } - } + keyInfo = load_latest_versioned_key_name(&new_master_key.keyInfo, keyring, ensure_new_key); if (keyInfo == NULL) keyInfo = KeyringGenerateNewKeyAndStore(keyring, new_master_key.keyInfo.keyId.versioned_name, INTERNAL_KEY_LEN, false); @@ -452,6 +411,70 @@ RotateMasterKey(const char *new_key_name, const char *new_provider_name) return pg_tde_perform_rotate_key(master_key, &new_master_key); } +/* +* Load the latest versioned key name for the master key +* If ensure_new_key is true, then we will keep on incrementing the version number +* till we get a key name that is not present in the keyring +*/ +static keyInfo * +load_latest_versioned_key_name(TDEMasterKeyInfo *mastere_key_info, GenericKeyring *keyring, bool ensure_new_key) +{ + KeyringReturnCodes kr_ret; + keyInfo *keyInfo = NULL; + int base_version = mastere_key_info->keyId.version; + Assert(mastere_key_info != NULL); + Assert(keyring != NULL); + Assert(strlen(mastere_key_info->keyId.name) > 0); + /* Start with the passed in version number + * We expect the name and the version number are already properly initialized + * and contain the correct values + */ + snprintf(mastere_key_info->keyId.versioned_name, TDE_KEY_NAME_LEN, + "%s_%d", mastere_key_info->keyId.name, mastere_key_info->keyId.version); + + while (true) + { + keyInfo = KeyringGetKey(keyring, mastere_key_info->keyId.versioned_name, false, &kr_ret); + if (kr_ret != KEYRING_CODE_SUCCESS) + { + ereport(ERROR, + (errmsg("failed to retrieve master key from keyring provider :\"%s\"", keyring->provider_name), + errdetail("Error code: %d", kr_ret))); + } + if (keyInfo == NULL) + { + if (ensure_new_key == false) + { + /* + * If ensure_key is false and we are not at the base version, + * We should return the last existent version. + */ + if (base_version < mastere_key_info->keyId.version) + { + /* Not optimal but keep the things simple */ + mastere_key_info->keyId.version -= 1; + snprintf(mastere_key_info->keyId.versioned_name, TDE_KEY_NAME_LEN, + "%s_%d", mastere_key_info->keyId.name, mastere_key_info->keyId.version); + keyInfo = KeyringGetKey(keyring, mastere_key_info->keyId.versioned_name, false, &kr_ret); + } + } + return keyInfo; + } + + mastere_key_info->keyId.version++; + snprintf(mastere_key_info->keyId.versioned_name, TDE_KEY_NAME_LEN, "%s_%d", mastere_key_info->keyId.name, mastere_key_info->keyId.version); + + /* + * Not really required. Just to break the infinite loop in case the key provider is not behaving sane. + */ + if (mastere_key_info->keyId.version > MAX_MASTER_KEY_VERSION_NUM) + { + ereport(ERROR, + (errmsg("failed to retrieve master key. %d versions already exist", MAX_MASTER_KEY_VERSION_NUM))); + } + } + return NULL; /* Just to keep compiler quite */ +} /* * Returns the provider ID of the keyring that holds the master key * Return InvalidOid if the master key is not set for the database @@ -600,11 +623,12 @@ Datum pg_tde_set_master_key(PG_FUNCTION_ARGS) { char *master_key_name = text_to_cstring(PG_GETARG_TEXT_PP(0)); char *provider_name = text_to_cstring(PG_GETARG_TEXT_PP(1)); + bool ensure_new_key = PG_GETARG_BOOL(2); bool ret; ereport(LOG, (errmsg("Setting master key [%s : %s] for the database", master_key_name, provider_name))); - ret = SetMasterKey(master_key_name, provider_name); - PG_RETURN_BOOL(ret); + ret = SetMasterKey(master_key_name, provider_name, ensure_new_key); + PG_RETURN_BOOL(ret); } /* @@ -614,11 +638,19 @@ PG_FUNCTION_INFO_V1(pg_tde_rotate_key); Datum pg_tde_rotate_key(PG_FUNCTION_ARGS) { - char *new_master_key_name = text_to_cstring(PG_GETARG_TEXT_PP(0)); - char *new_provider_name = text_to_cstring(PG_GETARG_TEXT_PP(1)); - bool ret; + char *new_master_key_name = NULL; + char *new_provider_name = NULL; + bool ensure_new_key; + bool ret; + + if (!PG_ARGISNULL(0)) + new_master_key_name = text_to_cstring(PG_GETARG_TEXT_PP(0)); + if (!PG_ARGISNULL(1)) + new_provider_name = text_to_cstring(PG_GETARG_TEXT_PP(1)); + ensure_new_key = PG_GETARG_BOOL(2); + ereport(LOG, (errmsg("Rotating master key to [%s : %s] for the database", new_master_key_name, new_provider_name))); - ret = RotateMasterKey(new_master_key_name, new_provider_name); - PG_RETURN_BOOL(ret); + ret = RotateMasterKey(new_master_key_name, new_provider_name, ensure_new_key); + PG_RETURN_BOOL(ret); } diff --git a/src/include/catalog/tde_master_key.h b/src/include/catalog/tde_master_key.h index adb418f2..efc79d15 100644 --- a/src/include/catalog/tde_master_key.h +++ b/src/include/catalog/tde_master_key.h @@ -17,7 +17,7 @@ #include "nodes/pg_list.h" #define MASTER_KEY_NAME_LEN TDE_KEY_NAME_LEN -#define MAX_MASTER_KEY_VERSION_NUM 1000 +#define MAX_MASTER_KEY_VERSION_NUM 100000 typedef struct TDEMasterKeyId { @@ -66,7 +66,7 @@ extern bool save_master_key_info(TDEMasterKeyInfo *masterKeyInfo); extern Oid GetMasterKeyProviderId(void); extern TDEMasterKey* GetMasterKey(void); -extern bool SetMasterKey(const char *key_name, const char *provider_name); -extern bool RotateMasterKey(const char *new_key_name, const char *new_provider_name); +extern bool SetMasterKey(const char *key_name, const char *provider_name, bool ensure_new_key); +extern bool RotateMasterKey(const char *new_key_name, const char *new_provider_name, bool ensure_new_key); #endif /*PG_TDE_MASTER_KEY_H*/ diff --git a/src/keyring/keyring_file.c b/src/keyring/keyring_file.c index 3c5ba417..26a9c73e 100644 --- a/src/keyring/keyring_file.c +++ b/src/keyring/keyring_file.c @@ -45,14 +45,11 @@ get_key_by_name(GenericKeyring* keyring, const char* key_name, bool throw_error, off_t bytes_read = 0; off_t curr_pos = 0; - file = PathNameOpenFile(file_keyring->file_name, O_CREAT | O_RDWR | PG_BINARY); + *return_code = KEYRING_CODE_SUCCESS; + + file = PathNameOpenFile(file_keyring->file_name, PG_BINARY); if (file < 0) - { - *return_code = KEYRING_CODE_RESOURCE_NOT_ACCESSABLE; - ereport(throw_error ? ERROR : NOTICE, - (errmsg("Failed to open keyring file :%s %m", file_keyring->file_name))); return NULL; - } key = palloc(sizeof(keyInfo)); while(true) @@ -62,19 +59,24 @@ get_key_by_name(GenericKeyring* keyring, const char* key_name, bool throw_error, if (bytes_read == 0 ) { + /* + * Empty keyring file is considered as a valid keyring file that has no keys + */ + FileClose(file); pfree(key); - *return_code = KEYRING_CODE_RESOURCE_NOT_AVAILABLE; return NULL; } if (bytes_read != sizeof(keyInfo)) { + FileClose(file); pfree(key); /* Corrupt file */ *return_code = KEYRING_CODE_DATA_CORRUPTED; ereport(throw_error?ERROR:WARNING, (errcode_for_file_access(), errmsg("keyring file \"%s\" is corrupted: %m", - file_keyring->file_name))); + file_keyring->file_name), + errdetail("invalid key size %d expected %d", bytes_read, sizeof(keyInfo)))); return NULL; } if (strncasecmp(key->name.name, key_name, sizeof(key->name.name)) == 0) @@ -83,7 +85,6 @@ get_key_by_name(GenericKeyring* keyring, const char* key_name, bool throw_error, return key; } } - *return_code = KEYRING_CODE_SUCCESS; FileClose(file); pfree(key); return NULL; @@ -93,7 +94,8 @@ static KeyringReturnCodes set_key_by_name(GenericKeyring* keyring, keyInfo *key, bool throw_error) { off_t bytes_written = 0; - File file; + off_t curr_pos = 0; + File file; FileKeyring* file_keyring = (FileKeyring*)keyring; keyInfo *existing_key; KeyringReturnCodes return_code = KEYRING_CODE_SUCCESS; @@ -118,9 +120,11 @@ set_key_by_name(GenericKeyring* keyring, keyInfo *key, bool throw_error) return KEYRING_CODE_RESOURCE_NOT_ACCESSABLE; } /* Write key to the end of file */ - lseek(file, 0, SEEK_END); - bytes_written = FileWrite(file, key, sizeof(keyInfo), 0, WAIT_EVENT_DATA_FILE_WRITE); - if (bytes_written != sizeof(keyInfo)) + curr_pos = FileSize(file); + ereport(NOTICE, + (errmsg("Writing key to file %s at offset %ld", file_keyring->file_name, curr_pos))); + bytes_written = FileWrite(file, key, sizeof(keyInfo), curr_pos, WAIT_EVENT_DATA_FILE_WRITE); + if (bytes_written != sizeof(keyInfo)) { FileClose(file); ereport(throw_error?ERROR:WARNING, diff --git a/src/keyring/keyring_vault.c b/src/keyring/keyring_vault.c index 9a9b9849..ef38fa70 100644 --- a/src/keyring/keyring_vault.c +++ b/src/keyring/keyring_vault.c @@ -185,6 +185,8 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, bool throw_error, const char* responseKey; + *return_code = KEYRING_CODE_SUCCESS; + get_keyring_vault_url(vault_keyring, key_name, url, sizeof(url)); if (!curl_perform(vault_keyring, url, &str, &httpCode, NULL)) From 3b04d48fe3b856eda71813af96319ee5deca56f2 Mon Sep 17 00:00:00 2001 From: Muhammad Usama Date: Wed, 20 Mar 2024 18:49:09 +0500 Subject: [PATCH 07/10] Removing left out NOTICE message from previous commit --- src/keyring/keyring_file.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/keyring/keyring_file.c b/src/keyring/keyring_file.c index 26a9c73e..620051b7 100644 --- a/src/keyring/keyring_file.c +++ b/src/keyring/keyring_file.c @@ -121,8 +121,6 @@ set_key_by_name(GenericKeyring* keyring, keyInfo *key, bool throw_error) } /* Write key to the end of file */ curr_pos = FileSize(file); - ereport(NOTICE, - (errmsg("Writing key to file %s at offset %ld", file_keyring->file_name, curr_pos))); bytes_written = FileWrite(file, key, sizeof(keyInfo), curr_pos, WAIT_EVENT_DATA_FILE_WRITE); if (bytes_written != sizeof(keyInfo)) { From 6c7e3396f2185d251c3a8c983ff816fa45dc4570 Mon Sep 17 00:00:00 2001 From: Muhammad Usama Date: Wed, 20 Mar 2024 19:16:01 +0500 Subject: [PATCH 08/10] Fix vault-v2 test case vault-v2 returns 404 (KEYRING_CODE_RESOURCE_NOT_AVAILABLE) when key is not found so it should not be treated as an error --- src/catalog/tde_master_key.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/catalog/tde_master_key.c b/src/catalog/tde_master_key.c index 1e197239..41a0864c 100644 --- a/src/catalog/tde_master_key.c +++ b/src/catalog/tde_master_key.c @@ -435,7 +435,8 @@ load_latest_versioned_key_name(TDEMasterKeyInfo *mastere_key_info, GenericKeyrin while (true) { keyInfo = KeyringGetKey(keyring, mastere_key_info->keyId.versioned_name, false, &kr_ret); - if (kr_ret != KEYRING_CODE_SUCCESS) + /* vault-v2 returns 404 (KEYRING_CODE_RESOURCE_NOT_AVAILABLE) when key is not found */ + if (kr_ret != KEYRING_CODE_SUCCESS && kr_ret != KEYRING_CODE_RESOURCE_NOT_AVAILABLE) { ereport(ERROR, (errmsg("failed to retrieve master key from keyring provider :\"%s\"", keyring->provider_name), From c5434798cc72a69ac194f4f0eab25e727eb9fff3 Mon Sep 17 00:00:00 2001 From: Muhammad Usama Date: Wed, 20 Mar 2024 19:39:19 +0500 Subject: [PATCH 09/10] Delete master-key cache entry as part of key rotation Commit also adds a key rotation tap test case --- meson.build | 1 + src/catalog/tde_master_key.c | 25 +++++---- t/002_rotate_key.pl | 103 ++++++++++++++++++++++++++++++++++ t/expected/002_rotate_key.out | 25 +++++++++ 4 files changed, 144 insertions(+), 10 deletions(-) create mode 100644 t/002_rotate_key.pl create mode 100644 t/expected/002_rotate_key.out diff --git a/meson.build b/meson.build index 94ba75e4..50ded6d8 100644 --- a/meson.build +++ b/meson.build @@ -84,6 +84,7 @@ tests += { 'tap': { 'tests': [ 't/001_basic.pl', + 't/002_rotate_key.pl', ], }, } diff --git a/src/catalog/tde_master_key.c b/src/catalog/tde_master_key.c index 41a0864c..aa683e9d 100644 --- a/src/catalog/tde_master_key.c +++ b/src/catalog/tde_master_key.c @@ -67,7 +67,7 @@ static int required_locks_count(void); static void shared_memory_shutdown(int code, Datum arg); static void master_key_startup_cleanup(int tde_tbl_count, void *arg); static keyInfo *load_latest_versioned_key_name(TDEMasterKeyInfo *mastere_key_info, GenericKeyring *keyring, bool ensure_new_key); - +static void clear_master_key_cache(Oid databaseId, Oid tablespaceId) ; static inline dshash_table *get_master_key_Hash(void); static TDEMasterKey *get_master_key_from_cache(Oid dbOid, bool acquire_lock); static void push_master_key_to_cache(TDEMasterKey *masterKey); @@ -407,7 +407,7 @@ RotateMasterKey(const char *new_key_name, const char *new_provider_name, bool en new_master_key.keyLength = keyInfo->data.len; memcpy(new_master_key.keyData, keyInfo->data.data, keyInfo->data.len); - + clear_master_key_cache(MyDatabaseId, MyDatabaseTableSpace); return pg_tde_perform_rotate_key(master_key, &new_master_key); } @@ -594,6 +594,19 @@ master_key_startup_cleanup(int tde_tbl_count, void* arg) void cleanup_master_key_info(Oid databaseId, Oid tablespaceId) +{ + clear_master_key_cache(databaseId, tablespaceId); + /* + * TODO: Although should never happen. Still verify if any table in the + * database is using tde + */ + + /* Remove the tde files */ + pg_tde_delete_tde_files(databaseId); +} + +static void +clear_master_key_cache(Oid databaseId, Oid tablespaceId) { TDEMasterKey *cache_entry; @@ -604,14 +617,6 @@ cleanup_master_key_info(Oid databaseId, Oid tablespaceId) { dshash_delete_entry(get_master_key_Hash(), cache_entry); } - - /* - * TODO: Although should never happen. Still verify if any table in the - * database is using tde - */ - - /* Remove the tde files */ - pg_tde_delete_tde_files(databaseId); } /* diff --git a/t/002_rotate_key.pl b/t/002_rotate_key.pl new file mode 100644 index 00000000..b51dd32f --- /dev/null +++ b/t/002_rotate_key.pl @@ -0,0 +1,103 @@ +#!/usr/bin/perl + +use strict; +use warnings; +use File::Basename; +use File::Compare; +use File::Copy; +use Test::More; +use lib 't'; +use pgtde; + +# Get file name and CREATE out file name and dirs WHERE requried +PGTDE::setup_files_dir(basename($0)); + +# CREATE new PostgreSQL node and do initdb +my $node = PGTDE->pgtde_init_pg(); +my $pgdata = $node->data_dir; + +# UPDATE postgresql.conf to include/load pg_tde library +open my $conf, '>>', "$pgdata/postgresql.conf"; +print $conf "shared_preload_libraries = 'pg_tde'\n"; +close $conf; + +# Start server +my $rt_value = $node->start; +ok($rt_value == 1, "Start Server"); + +# CREATE EXTENSION and change out file permissions +my ($cmdret, $stdout, $stderr) = $node->psql('postgres', 'CREATE EXTENSION pg_tde;', extra_params => ['-a']); +ok($cmdret == 0, "CREATE PGTDE EXTENSION"); +PGTDE::append_to_file($stdout); + + +$rt_value = $node->psql('postgres', 'CREATE TABLE test_enc(id SERIAL,k INTEGER,PRIMARY KEY (id)) USING pg_tde;', extra_params => ['-a']); +ok($rt_value == 3, "Failing query"); + + +# Restart the server +PGTDE::append_to_file("-- server restart"); +$node->stop(); + +$rt_value = $node->start(); +ok($rt_value == 1, "Restart Server"); + +$rt_value = $node->psql('postgres', "SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per');", extra_params => ['-a']); +$rt_value = $node->psql('postgres', "SELECT pg_tde_add_key_provider_file('file-2','/tmp/pg_tde_test_keyring_2.per');", extra_params => ['-a']); +$rt_value = $node->psql('postgres', "SELECT pg_tde_set_master_key('test-db-master-key','file-vault');", extra_params => ['-a']); + +$stdout = $node->safe_psql('postgres', 'CREATE TABLE test_enc(id SERIAL,k INTEGER,PRIMARY KEY (id)) USING pg_tde;', extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +$stdout = $node->safe_psql('postgres', 'INSERT INTO test_enc (k) VALUES (5),(6);', extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +$stdout = $node->safe_psql('postgres', 'SELECT * FROM test_enc ORDER BY id ASC;', extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +#rotate key +PGTDE::append_to_file("-- ROTATE KEY pg_tde_rotate_key('rotated-master-key','file-2');"); +$rt_value = $node->psql('postgres', "SELECT pg_tde_rotate_key('rotated-master-key','file-2');", extra_params => ['-a']); +$stdout = $node->safe_psql('postgres', 'SELECT * FROM test_enc ORDER BY id ASC;', extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +# Restart the server +PGTDE::append_to_file("-- server restart"); +$rt_value = $node->stop(); +$rt_value = $node->start(); + +$stdout = $node->safe_psql('postgres', 'SELECT * FROM test_enc ORDER BY id ASC;', extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +#Again rotate key +PGTDE::append_to_file("-- ROTATE KEY pg_tde_rotate_key();"); +$rt_value = $node->psql('postgres', "SELECT pg_tde_rotate_key();", extra_params => ['-a']); +$stdout = $node->safe_psql('postgres', 'SELECT * FROM test_enc ORDER BY id ASC;', extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +# Restart the server +PGTDE::append_to_file("-- server restart"); +$rt_value = $node->stop(); +$rt_value = $node->start(); + +$stdout = $node->safe_psql('postgres', 'SELECT * FROM test_enc ORDER BY id ASC;', extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +$stdout = $node->safe_psql('postgres', 'DROP TABLE test_enc;', extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +# DROP EXTENSION +$stdout = $node->safe_psql('postgres', 'DROP EXTENSION pg_tde;', extra_params => ['-a']); +ok($cmdret == 0, "DROP PGTDE EXTENSION"); +PGTDE::append_to_file($stdout); +# Stop the server +$node->stop(); + +# compare the expected and out file +my $compare = PGTDE->compare_results(); + +# Test/check if expected and result/out file match. If Yes, test passes. +is($compare,0,"Compare Files: $PGTDE::expected_filename_with_path and $PGTDE::out_filename_with_path files."); + +# Done testing for this testcase file. +done_testing(); diff --git a/t/expected/002_rotate_key.out b/t/expected/002_rotate_key.out new file mode 100644 index 00000000..a1730f4a --- /dev/null +++ b/t/expected/002_rotate_key.out @@ -0,0 +1,25 @@ +CREATE EXTENSION pg_tde; +-- server restart +CREATE TABLE test_enc(id SERIAL,k INTEGER,PRIMARY KEY (id)) USING pg_tde; +INSERT INTO test_enc (k) VALUES (5),(6); +SELECT * FROM test_enc ORDER BY id ASC; +1|5 +2|6 +-- ROTATE KEY pg_tde_rotate_key('rotated-master-key','file-2'); +SELECT * FROM test_enc ORDER BY id ASC; +1|5 +2|6 +-- server restart +SELECT * FROM test_enc ORDER BY id ASC; +1|5 +2|6 +-- ROTATE KEY pg_tde_rotate_key(); +SELECT * FROM test_enc ORDER BY id ASC; +1|5 +2|6 +-- server restart +SELECT * FROM test_enc ORDER BY id ASC; +1|5 +2|6 +DROP TABLE test_enc; +DROP EXTENSION pg_tde; From f9bd8fa8ce46e5f5acb3b1a470e576c36271ad22 Mon Sep 17 00:00:00 2001 From: Hamid Akhtar Date: Wed, 20 Mar 2024 23:00:31 +0500 Subject: [PATCH 10/10] Ensuring that we are clearing master key cache during xlog redo as well. --- src/access/pg_tde_tdemap.c | 10 ++++------ src/catalog/tde_master_key.c | 14 ++++++++++++++ src/include/access/pg_tde_tdemap.h | 2 +- src/include/catalog/tde_master_key.h | 1 + src/keyring/keyring_file.c | 2 +- 5 files changed, 21 insertions(+), 8 deletions(-) diff --git a/src/access/pg_tde_tdemap.c b/src/access/pg_tde_tdemap.c index 3f7eb583..84fc8c20 100644 --- a/src/access/pg_tde_tdemap.c +++ b/src/access/pg_tde_tdemap.c @@ -1112,7 +1112,7 @@ pg_tde_perform_rotate_key(TDEMasterKey *master_key, TDEMasterKey *new_master_key * Rotate keys on a standby. */ bool -xl_tde_perform_rotate_key(XLogMasterKeyRotate *xlrec) +pg_tde_write_map_keydata_files(off_t map_size, char *m_file_data, off_t keydata_size, char *k_file_data) { TDEFileHeader *fheader; char m_path_new[MAXPGPATH]; @@ -1124,9 +1124,7 @@ xl_tde_perform_rotate_key(XLogMasterKeyRotate *xlrec) off_t read_pos_tmp = 0; /* Let's get the header. Buff should start with the map file header. */ - fheader = (TDEFileHeader *) xlrec->buff; - - ereport(LOG, (errmsg("xl_tde_rotate => %s", fheader->master_key_info.keyId.name))); + fheader = (TDEFileHeader *) m_file_data; /* Set the file paths */ pg_tde_set_db_file_paths(fheader->master_key_info.databaseId); @@ -1135,7 +1133,7 @@ xl_tde_perform_rotate_key(XLogMasterKeyRotate *xlrec) m_file_new = keyrotation_init_file(&fheader->master_key_info, m_path_new, db_map_path, &is_new_file, &curr_pos); k_file_new = keyrotation_init_file(&fheader->master_key_info, k_path_new, db_keydata_path, &is_new_file, &read_pos_tmp); - if (FileWrite(m_file_new, xlrec->buff, xlrec->map_size, 0, WAIT_EVENT_DATA_FILE_WRITE) != xlrec->map_size) + if (FileWrite(m_file_new, m_file_data, map_size, 0, WAIT_EVENT_DATA_FILE_WRITE) != map_size) { ereport(WARNING, (errcode_for_file_access(), @@ -1143,7 +1141,7 @@ xl_tde_perform_rotate_key(XLogMasterKeyRotate *xlrec) m_path_new))); } - if (FileWrite(k_file_new, &xlrec->buff[xlrec->map_size], xlrec->keydata_size, 0, WAIT_EVENT_DATA_FILE_WRITE) != xlrec->keydata_size) + if (FileWrite(k_file_new, k_file_data, keydata_size, 0, WAIT_EVENT_DATA_FILE_WRITE) != keydata_size) { ereport(WARNING, (errcode_for_file_access(), diff --git a/src/catalog/tde_master_key.c b/src/catalog/tde_master_key.c index aa683e9d..d8517524 100644 --- a/src/catalog/tde_master_key.c +++ b/src/catalog/tde_master_key.c @@ -411,6 +411,20 @@ RotateMasterKey(const char *new_key_name, const char *new_provider_name, bool en return pg_tde_perform_rotate_key(master_key, &new_master_key); } +/* + * Rotate keys on a standby. + */ +bool +xl_tde_perform_rotate_key(XLogMasterKeyRotate *xlrec) +{ + bool ret; + + ret = pg_tde_write_map_keydata_files(xlrec->map_size, xlrec->buff, xlrec->keydata_size, &xlrec->buff[xlrec->map_size]); + clear_master_key_cache(MyDatabaseId, MyDatabaseTableSpace); + + return ret; +} + /* * Load the latest versioned key name for the master key * If ensure_new_key is true, then we will keep on incrementing the version number diff --git a/src/include/access/pg_tde_tdemap.h b/src/include/access/pg_tde_tdemap.h index b358e4d1..c6ff9083 100644 --- a/src/include/access/pg_tde_tdemap.h +++ b/src/include/access/pg_tde_tdemap.h @@ -60,7 +60,7 @@ extern void pg_tde_delete_tde_files(Oid dbOid); extern TDEMasterKeyInfo *pg_tde_get_master_key(Oid dbOid); extern bool pg_tde_save_master_key(TDEMasterKeyInfo *master_key_info); extern bool pg_tde_perform_rotate_key(TDEMasterKey *master_key, TDEMasterKey *new_master_key); -extern bool xl_tde_perform_rotate_key(XLogMasterKeyRotate *xlrec); +extern bool pg_tde_write_map_keydata_files(off_t map_size, char *m_file_data, off_t keydata_size, char *k_file_data); const char * tde_sprint_key(InternalKey *k); diff --git a/src/include/catalog/tde_master_key.h b/src/include/catalog/tde_master_key.h index efc79d15..700f48b2 100644 --- a/src/include/catalog/tde_master_key.h +++ b/src/include/catalog/tde_master_key.h @@ -68,5 +68,6 @@ extern Oid GetMasterKeyProviderId(void); extern TDEMasterKey* GetMasterKey(void); extern bool SetMasterKey(const char *key_name, const char *provider_name, bool ensure_new_key); extern bool RotateMasterKey(const char *new_key_name, const char *new_provider_name, bool ensure_new_key); +extern bool xl_tde_perform_rotate_key(XLogMasterKeyRotate *xlrec); #endif /*PG_TDE_MASTER_KEY_H*/ diff --git a/src/keyring/keyring_file.c b/src/keyring/keyring_file.c index 620051b7..812e9fab 100644 --- a/src/keyring/keyring_file.c +++ b/src/keyring/keyring_file.c @@ -76,7 +76,7 @@ get_key_by_name(GenericKeyring* keyring, const char* key_name, bool throw_error, (errcode_for_file_access(), errmsg("keyring file \"%s\" is corrupted: %m", file_keyring->file_name), - errdetail("invalid key size %d expected %d", bytes_read, sizeof(keyInfo)))); + errdetail("invalid key size %lu expected %lu", bytes_read, sizeof(keyInfo)))); return NULL; } if (strncasecmp(key->name.name, key_name, sizeof(key->name.name)) == 0)