Skip to content

PG-1651 Do not try the fetch key from old provider when changing provider settings #395

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 1 commit into from
Jun 6, 2025
Merged

PG-1651 Do not try the fetch key from old provider when changing provider settings #395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 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 @@ -696,9 +696,9 @@ pg_tde_count_relations(Oid dbOid)
}

bool
pg_tde_verify_principal_key_info(TDESignedPrincipalKeyInfo *signed_key_info, const TDEPrincipalKey *principal_key)
pg_tde_verify_principal_key_info(TDESignedPrincipalKeyInfo *signed_key_info, const KeyData *principal_key_data)
{
return AesGcmDecrypt(principal_key->keyData,
return AesGcmDecrypt(principal_key_data->data,
signed_key_info->sign_iv, MAP_ENTRY_IV_SIZE,
(unsigned char *) &signed_key_info->data, sizeof(signed_key_info->data),
NULL, 0,
Expand Down
42 changes: 22 additions & 20 deletions contrib/pg_tde/src/catalog/tde_principal_key.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,9 +677,7 @@ pg_tde_get_key_info(PG_FUNCTION_ARGS, Oid dbOid)
#endif /* FRONTEND */

/*
* Gets principal key form the keyring.
*
* Caller should hold an exclusive tde_lwlock_enc_keys lock
Copy link
Member

Choose a reason for hiding this comment

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

what's the reason for removing this comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because the assert is there and protects the caller.

* Get principal key form the keyring.
*/
static TDEPrincipalKey *
get_principal_key_from_keyring(Oid dbOid)
Expand Down Expand Up @@ -710,6 +708,12 @@ get_principal_key_from_keyring(Oid dbOid)
errmsg("failed to retrieve principal key %s from keyring with ID %d",
principalKeyInfo->data.name, principalKeyInfo->data.keyringId));

if (!pg_tde_verify_principal_key_info(principalKeyInfo, &keyInfo->data))
ereport(ERROR,
errcode(ERRCODE_DATA_CORRUPTED),
errmsg("Failed to verify principal key header for key %s, incorrect principal key or corrupted key file",
principalKeyInfo->data.name));

principalKey = palloc_object(TDEPrincipalKey);

principalKey->keyInfo = principalKeyInfo->data;
Expand All @@ -718,12 +722,6 @@ get_principal_key_from_keyring(Oid dbOid)

Assert(dbOid == principalKey->keyInfo.databaseId);

if (!pg_tde_verify_principal_key_info(principalKeyInfo, principalKey))
ereport(ERROR,
errcode(ERRCODE_DATA_CORRUPTED),
errmsg("Failed to verify principal key header for key %s, incorrect principal key or corrupted key file",
principalKeyInfo->data.name));

pfree(keyInfo);
pfree(keyring);
pfree(principalKeyInfo);
Expand Down Expand Up @@ -929,7 +927,7 @@ pg_tde_is_provider_used(Oid databaseOid, Oid providerId)
void
pg_tde_verify_provider_keys_in_use(GenericKeyring *modified_provider)
{
TDEPrincipalKey *existing_principal_key;
TDESignedPrincipalKeyInfo *existing_principal_key;
HeapTuple tuple;
SysScanDesc scan;
Relation rel;
Expand All @@ -940,11 +938,11 @@ pg_tde_verify_provider_keys_in_use(GenericKeyring *modified_provider)
LWLockAcquire(tde_lwlock_enc_keys(), LW_EXCLUSIVE);

/* Check the server key that is used for WAL encryption */
existing_principal_key = GetPrincipalKeyNoDefault(GLOBAL_DATA_TDE_OID, LW_EXCLUSIVE);
existing_principal_key = pg_tde_get_principal_key_info(GLOBAL_DATA_TDE_OID);
if (existing_principal_key != NULL &&
existing_principal_key->keyInfo.keyringId == modified_provider->keyring_id)
existing_principal_key->data.keyringId == modified_provider->keyring_id)
{
char *key_name = existing_principal_key->keyInfo.name;
char *key_name = existing_principal_key->data.name;
KeyringReturnCodes return_code;
KeyInfo *proposed_key;

Expand All @@ -956,15 +954,17 @@ pg_tde_verify_provider_keys_in_use(GenericKeyring *modified_provider)
key_name, modified_provider->provider_name, return_code));
}

if (proposed_key->data.len != existing_principal_key->keyLength ||
memcmp(proposed_key->data.data, existing_principal_key->keyData, proposed_key->data.len) != 0)
if (!pg_tde_verify_principal_key_info(existing_principal_key, &proposed_key->data))
{
ereport(ERROR,
errmsg("key \"%s\" from modified key provider \"%s\" does not match existing server key",
key_name, modified_provider->provider_name));
}
}

if (existing_principal_key)
pfree(existing_principal_key);

/* Check all databases for usage of keys from this key provider. */
rel = table_open(DatabaseRelationId, AccessShareLock);
scan = systable_beginscan(rel, 0, false, NULL, 0, NULL);
Expand All @@ -973,11 +973,11 @@ pg_tde_verify_provider_keys_in_use(GenericKeyring *modified_provider)
{
Form_pg_database database = (Form_pg_database) GETSTRUCT(tuple);

existing_principal_key = GetPrincipalKeyNoDefault(database->oid, LW_EXCLUSIVE);
existing_principal_key = pg_tde_get_principal_key_info(database->oid);
if (existing_principal_key != NULL &&
existing_principal_key->keyInfo.keyringId == modified_provider->keyring_id)
existing_principal_key->data.keyringId == modified_provider->keyring_id)
{
char *key_name = existing_principal_key->keyInfo.name;
char *key_name = existing_principal_key->data.name;
KeyringReturnCodes return_code;
KeyInfo *proposed_key;

Expand All @@ -989,14 +989,16 @@ pg_tde_verify_provider_keys_in_use(GenericKeyring *modified_provider)
key_name, database->datname.data, modified_provider->provider_name, return_code));
}

if (proposed_key->data.len != existing_principal_key->keyLength ||
memcmp(proposed_key->data.data, existing_principal_key->keyData, proposed_key->data.len) != 0)
if (!pg_tde_verify_principal_key_info(existing_principal_key, &proposed_key->data))
{
ereport(ERROR,
errmsg("key \"%s\" from modified key provider \"%s\" does not match existing key used by database \"%s\"",
key_name, modified_provider->provider_name, database->datname.data));
}
}

if (existing_principal_key)
pfree(existing_principal_key);
}
systable_endscan(scan);
table_close(rel, AccessShareLock);
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 @@ -103,7 +103,7 @@ extern int pg_tde_count_relations(Oid dbOid);
extern void pg_tde_delete_tde_files(Oid dbOid);

extern TDESignedPrincipalKeyInfo *pg_tde_get_principal_key_info(Oid dbOid);
extern bool pg_tde_verify_principal_key_info(TDESignedPrincipalKeyInfo *signed_key_info, const TDEPrincipalKey *principal_key);
extern bool pg_tde_verify_principal_key_info(TDESignedPrincipalKeyInfo *signed_key_info, const KeyData *principal_key_data);
extern void pg_tde_save_principal_key(const TDEPrincipalKey *principal_key, bool write_xlog);
extern void pg_tde_save_principal_key_redo(const TDESignedPrincipalKeyInfo *signed_key_info);
extern void pg_tde_perform_rotate_key(TDEPrincipalKey *principal_key, TDEPrincipalKey *new_principal_key, bool write_xlog);
Expand Down
29 changes: 24 additions & 5 deletions contrib/pg_tde/t/change_key_provider.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@

unlink('/tmp/change_key_provider_1.per');
unlink('/tmp/change_key_provider_2.per');
unlink('/tmp/change_key_provider_3.per');
unlink('/tmp/change_key_provider_4.per');

my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
Expand All @@ -26,7 +24,7 @@
"SELECT pg_tde_add_database_key_provider_file('file-vault', '/tmp/change_key_provider_1.per');"
);
PGTDE::psql($node, 'postgres',
"SELECT pg_tde_list_all_database_key_providers();");
"SELECT * FROM pg_tde_list_all_database_key_providers();");
PGTDE::psql($node, 'postgres',
"SELECT pg_tde_set_key_using_database_key_provider('test-key', 'file-vault');"
);
Expand All @@ -48,7 +46,7 @@
"SELECT pg_tde_change_database_key_provider_file('file-vault', '/tmp/change_key_provider_2.per');"
);
PGTDE::psql($node, 'postgres',
"SELECT pg_tde_list_all_database_key_providers();");
"SELECT * FROM pg_tde_list_all_database_key_providers();");

PGTDE::psql($node, 'postgres', "SELECT pg_tde_verify_key();");
PGTDE::psql($node, 'postgres', "SELECT pg_tde_is_encrypted('test_enc');");
Expand All @@ -57,7 +55,28 @@
PGTDE::append_to_result_file("-- server restart");
$node->restart;

# Verify
PGTDE::psql($node, 'postgres', "SELECT pg_tde_verify_key();");
PGTDE::psql($node, 'postgres', "SELECT pg_tde_is_encrypted('test_enc');");
PGTDE::psql($node, 'postgres', 'SELECT * FROM test_enc ORDER BY id;');

# Move file, restart and then change provider
PGTDE::append_to_result_file(
"-- mv /tmp/change_key_provider_2.per /tmp/change_key_provider_1.per");
move('/tmp/change_key_provider_2.per', '/tmp/change_key_provider_1.per');

PGTDE::append_to_result_file("-- server restart");
$node->restart;

PGTDE::psql($node, 'postgres', "SELECT pg_tde_verify_key();");
PGTDE::psql($node, 'postgres', "SELECT pg_tde_is_encrypted('test_enc');");
PGTDE::psql($node, 'postgres', 'SELECT * FROM test_enc ORDER BY id;');

PGTDE::psql($node, 'postgres',
"SELECT pg_tde_change_database_key_provider_file('file-vault', '/tmp/change_key_provider_1.per');"
);
PGTDE::psql($node, 'postgres',
"SELECT * FROM pg_tde_list_all_database_key_providers();");

PGTDE::psql($node, 'postgres', "SELECT pg_tde_verify_key();");
PGTDE::psql($node, 'postgres', "SELECT pg_tde_is_encrypted('test_enc');");
PGTDE::psql($node, 'postgres', 'SELECT * FROM test_enc ORDER BY id;');
Expand Down
59 changes: 51 additions & 8 deletions contrib/pg_tde/t/expected/change_key_provider.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ SELECT pg_tde_add_database_key_provider_file('file-vault', '/tmp/change_key_prov

(1 row)

SELECT pg_tde_list_all_database_key_providers();
pg_tde_list_all_database_key_providers
-----------------------------------------------------------------------
(1,file-vault,file,"{""path"" : ""/tmp/change_key_provider_1.per""}")
SELECT * FROM pg_tde_list_all_database_key_providers();
id | provider_name | provider_type | options
----+---------------+---------------+---------------------------------------------
1 | file-vault | file | {"path" : "/tmp/change_key_provider_1.per"}
(1 row)

SELECT pg_tde_set_key_using_database_key_provider('test-key', 'file-vault');
Expand Down Expand Up @@ -45,10 +45,10 @@ SELECT pg_tde_change_database_key_provider_file('file-vault', '/tmp/change_key_p

(1 row)

SELECT pg_tde_list_all_database_key_providers();
pg_tde_list_all_database_key_providers
-----------------------------------------------------------------------
(1,file-vault,file,"{""path"" : ""/tmp/change_key_provider_2.per""}")
SELECT * FROM pg_tde_list_all_database_key_providers();
id | provider_name | provider_type | options
----+---------------+---------------+---------------------------------------------
1 | file-vault | file | {"path" : "/tmp/change_key_provider_2.per"}
(1 row)

SELECT pg_tde_verify_key();
Expand Down Expand Up @@ -90,5 +90,48 @@ SELECT * FROM test_enc ORDER BY id;
2 | 6
(2 rows)

-- mv /tmp/change_key_provider_2.per /tmp/change_key_provider_1.per
-- server restart
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');
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
SELECT pg_tde_change_database_key_provider_file('file-vault', '/tmp/change_key_provider_1.per');
pg_tde_change_database_key_provider_file
------------------------------------------

(1 row)

SELECT * FROM pg_tde_list_all_database_key_providers();
id | provider_name | provider_type | options
----+---------------+---------------+---------------------------------------------
1 | file-vault | file | {"path" : "/tmp/change_key_provider_1.per"}
(1 row)

SELECT pg_tde_verify_key();
pg_tde_verify_key
-------------------

(1 row)

SELECT pg_tde_is_encrypted('test_enc');
pg_tde_is_encrypted
---------------------
t
(1 row)

SELECT * FROM test_enc ORDER BY id;
id | k
----+---
1 | 5
2 | 6
(2 rows)

DROP EXTENSION pg_tde CASCADE;
psql:<stdin>:1: NOTICE: drop cascades to table test_enc