diff --git a/contrib/pg_tde/expected/key_provider.out b/contrib/pg_tde/expected/key_provider.out index 8110100372452..f51c0f2885f15 100644 --- a/contrib/pg_tde/expected/key_provider.out +++ b/contrib/pg_tde/expected/key_provider.out @@ -252,7 +252,7 @@ WARNING: The WAL encryption feature is currently in beta and may be unstable. D (1 row) SELECT pg_tde_change_global_key_provider_file('global-provider','/tmp/global-provider-file-2'); -ERROR: could not fetch key "server-key" used as server key from modified key provider "global-provider": 0 +ERROR: key "server-key" used as server key not found in modified key provider "global-provider" -- Modifying key providers fails if new settings can't fetch existing database key SELECT pg_tde_add_global_key_provider_file('global-provider2', '/tmp/global-provider-file-1'); pg_tde_add_global_key_provider_file @@ -279,7 +279,7 @@ SELECT pg_tde_set_key_using_global_key_provider('database-key', 'global-provider \c :regress_database SELECT pg_tde_change_global_key_provider_file('global-provider2', '/tmp/global-provider-file-2'); -ERROR: could not fetch key "database-key" used by database "db_using_global_provider" from modified key provider "global-provider2": 0 +ERROR: key "database-key" used by database "db_using_global_provider" not found in key provider "global-provider2" DROP DATABASE db_using_global_provider; CREATE DATABASE db_using_database_provider; \c db_using_database_provider; @@ -303,7 +303,7 @@ SELECT pg_tde_set_key_using_database_key_provider('database-key', 'db-provider') (1 row) SELECT pg_tde_change_database_key_provider_file('db-provider', '/tmp/db-provider-file-2'); -ERROR: could not fetch key "database-key" used by database "db_using_database_provider" from modified key provider "db-provider": 0 +ERROR: key "database-key" used by database "db_using_database_provider" not found in key provider "db-provider" \c :regress_database DROP DATABASE db_using_database_provider; -- Deleting key providers fails if key name is NULL diff --git a/contrib/pg_tde/src/catalog/tde_principal_key.c b/contrib/pg_tde/src/catalog/tde_principal_key.c index 97c11526b5301..8e3342fa29e74 100644 --- a/contrib/pg_tde/src/catalog/tde_principal_key.c +++ b/contrib/pg_tde/src/catalog/tde_principal_key.c @@ -225,7 +225,7 @@ set_principal_key_with_keyring(const char *key_name, LWLock *lock_files = tde_lwlock_enc_keys(); bool already_has_key; GenericKeyring *new_keyring; - const KeyInfo *keyInfo = NULL; + KeyInfo *keyInfo = NULL; /* * Try to get principal key from cache. @@ -238,14 +238,15 @@ set_principal_key_with_keyring(const char *key_name, new_keyring = GetKeyProviderByName(provider_name, providerOid); { - KeyringReturnCode kr_ret; + KeyringReturnCode return_code; - keyInfo = KeyringGetKey(new_keyring, key_name, &kr_ret); + keyInfo = KeyringGetKey(new_keyring, key_name, &return_code); - if (kr_ret != KEYRING_CODE_SUCCESS) + if (return_code != KEYRING_CODE_SUCCESS) { ereport(ERROR, - errmsg("could not successfully query key provider \"%s\"", new_keyring->provider_name)); + errmsg("failed to retrieve principal key \"%s\" from key provider \"%s\"", key_name, new_keyring->provider_name), + errdetail("%s", KeyringErrorCodeToString(return_code))); } } @@ -289,6 +290,7 @@ set_principal_key_with_keyring(const char *key_name, LWLockRelease(lock_files); + pfree(keyInfo); pfree(new_keyring); pfree(new_principal_key); } @@ -303,7 +305,7 @@ xl_tde_perform_rotate_key(XLogPrincipalKeyRotate *xlrec) TDEPrincipalKey *new_principal_key; GenericKeyring *new_keyring; KeyInfo *keyInfo; - KeyringReturnCode kr_ret; + KeyringReturnCode return_code; LWLockAcquire(tde_lwlock_enc_keys(), LW_EXCLUSIVE); @@ -316,19 +318,20 @@ xl_tde_perform_rotate_key(XLogPrincipalKeyRotate *xlrec) } new_keyring = GetKeyProviderByID(xlrec->keyringId, xlrec->databaseId); - keyInfo = KeyringGetKey(new_keyring, xlrec->keyName, &kr_ret); + keyInfo = KeyringGetKey(new_keyring, xlrec->keyName, &return_code); - if (kr_ret != KEYRING_CODE_SUCCESS) + if (return_code != KEYRING_CODE_SUCCESS) { ereport(ERROR, - errmsg("failed to retrieve principal key from keyring provider: \"%s\"", new_keyring->provider_name), - errdetail("Error code: %d", kr_ret)); + errmsg("failed to retrieve principal key \"%s\" from key provider \"%s\"", xlrec->keyName, new_keyring->provider_name), + errdetail("%s", KeyringErrorCodeToString(return_code))); } /* The new key should be on keyring by this time */ if (keyInfo == NULL) { - ereport(ERROR, errmsg("failed to retrieve principal key from keyring for database %u.", xlrec->databaseId)); + ereport(ERROR, errmsg("failed to retrieve principal key \"%s\" from key provider \"%s\" for database %u", + xlrec->keyName, new_keyring->provider_name, xlrec->databaseId)); } new_principal_key = palloc_object(TDEPrincipalKey); @@ -347,6 +350,7 @@ xl_tde_perform_rotate_key(XLogPrincipalKeyRotate *xlrec) LWLockRelease(tde_lwlock_enc_keys()); + pfree(keyInfo); pfree(new_keyring); pfree(new_principal_key); } @@ -514,7 +518,8 @@ pg_tde_create_principal_key_internal(Oid providerOid, if (return_code != KEYRING_CODE_SUCCESS) ereport(ERROR, - errmsg("could not successfully query key provider \"%s\"", provider->provider_name)); + errmsg("failed to retrieve principal key \"%s\" from key provider \"%s\"", key_name, provider_name), + errdetail("%s", KeyringErrorCodeToString(return_code))); if (key_info != NULL) ereport(ERROR, @@ -939,11 +944,17 @@ get_principal_key_from_keyring(Oid dbOid) principalKeyInfo->data.name, principalKeyInfo->data.keyringId)); keyInfo = KeyringGetKey(keyring, principalKeyInfo->data.name, &keyring_ret); + + if (keyring_ret != KEYRING_CODE_SUCCESS) + ereport(ERROR, + errmsg("failed to retrieve principal key \"%s\" from key provider \"%s\"", principalKeyInfo->data.name, keyring->provider_name), + errdetail("%s", KeyringErrorCodeToString(keyring_ret))); + if (keyInfo == NULL) ereport(ERROR, errcode(ERRCODE_NO_DATA_FOUND), - errmsg("failed to retrieve principal key %s from keyring with ID %d", - principalKeyInfo->data.name, principalKeyInfo->data.keyringId)); + errmsg("key \"%s\" not found in key provider \"%s\"", + principalKeyInfo->data.name, keyring->provider_name)); if (!pg_tde_verify_principal_key_info(principalKeyInfo, &keyInfo->data)) ereport(ERROR, @@ -1184,11 +1195,21 @@ pg_tde_verify_provider_keys_in_use(GenericKeyring *modified_provider) KeyInfo *proposed_key; proposed_key = KeyringGetKey(modified_provider, key_name, &return_code); + + if (return_code != KEYRING_CODE_SUCCESS) + { + ereport(ERROR, + errmsg("failed to retreive \"%s\" key used as server key from modified key provider \"%s\"", + key_name, modified_provider->provider_name), + errdetail("%s", KeyringErrorCodeToString(return_code))); + } + if (!proposed_key) { ereport(ERROR, - errmsg("could not fetch key \"%s\" used as server key from modified key provider \"%s\": %d", - key_name, modified_provider->provider_name, return_code)); + errcode(ERRCODE_NO_DATA_FOUND), + errmsg("key \"%s\" used as server key not found in modified key provider \"%s\"", + key_name, modified_provider->provider_name)); } if (!pg_tde_verify_principal_key_info(existing_principal_key, &proposed_key->data)) @@ -1197,6 +1218,8 @@ pg_tde_verify_provider_keys_in_use(GenericKeyring *modified_provider) errmsg("key \"%s\" from modified key provider \"%s\" does not match existing server key", key_name, modified_provider->provider_name)); } + + pfree(proposed_key); } if (existing_principal_key) @@ -1219,11 +1242,20 @@ pg_tde_verify_provider_keys_in_use(GenericKeyring *modified_provider) KeyInfo *proposed_key; proposed_key = KeyringGetKey(modified_provider, key_name, &return_code); + if (return_code != KEYRING_CODE_SUCCESS) + { + ereport(ERROR, + errmsg("failed to retreive \"%s\" key used by database \"%s\" from key provider \"%s\"", + key_name, database->datname.data, modified_provider->provider_name), + errdetail("%s", KeyringErrorCodeToString(return_code))); + } + if (!proposed_key) { ereport(ERROR, - errmsg("could not fetch key \"%s\" used by database \"%s\" from modified key provider \"%s\": %d", - key_name, database->datname.data, modified_provider->provider_name, return_code)); + errcode(ERRCODE_NO_DATA_FOUND), + errmsg("key \"%s\" used by database \"%s\" not found in key provider \"%s\"", + key_name, database->datname.data, modified_provider->provider_name)); } if (!pg_tde_verify_principal_key_info(existing_principal_key, &proposed_key->data)) @@ -1232,6 +1264,8 @@ pg_tde_verify_provider_keys_in_use(GenericKeyring *modified_provider) 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)); } + + pfree(proposed_key); } if (existing_principal_key) diff --git a/contrib/pg_tde/src/include/keyring/keyring_api.h b/contrib/pg_tde/src/include/keyring/keyring_api.h index ab1add2dd8b20..8584386bfd37d 100644 --- a/contrib/pg_tde/src/include/keyring/keyring_api.h +++ b/contrib/pg_tde/src/include/keyring/keyring_api.h @@ -14,7 +14,9 @@ typedef enum ProviderType } ProviderType; #define TDE_KEY_NAME_LEN 256 -#define MAX_KEY_DATA_SIZE 32 /* maximum 256 bit encryption */ +#define KEY_DATA_SIZE_128 16 /* 128 bit encryption */ +#define KEY_DATA_SIZE_256 32 /* 256 bit encryption, not yet supported */ +#define MAX_KEY_DATA_SIZE KEY_DATA_SIZE_256 /* maximum 256 bit encryption */ #define INTERNAL_KEY_LEN 16 typedef struct KeyData @@ -35,7 +37,7 @@ typedef enum KeyringReturnCode KEYRING_CODE_INVALID_PROVIDER = 1, KEYRING_CODE_RESOURCE_NOT_AVAILABLE = 2, KEYRING_CODE_INVALID_RESPONSE = 5, - KEYRING_CODE_INVALID_KEY_SIZE = 6, + KEYRING_CODE_INVALID_KEY = 6, KEYRING_CODE_DATA_CORRUPTED = 7, } KeyringReturnCode; @@ -87,5 +89,7 @@ extern void RegisterKeyProviderType(const TDEKeyringRoutine *routine, ProviderTy extern KeyInfo *KeyringGetKey(GenericKeyring *keyring, const char *key_name, KeyringReturnCode *returnCode); extern KeyInfo *KeyringGenerateNewKeyAndStore(GenericKeyring *keyring, const char *key_name, unsigned key_len); extern void KeyringValidate(GenericKeyring *keyring); +extern bool ValidateKey(KeyInfo *key); +extern char *KeyringErrorCodeToString(KeyringReturnCode code); #endif /* KEYRING_API_H */ diff --git a/contrib/pg_tde/src/keyring/keyring_api.c b/contrib/pg_tde/src/keyring/keyring_api.c index 2cd1aad6c6728..11da816681f4d 100644 --- a/contrib/pg_tde/src/keyring/keyring_api.c +++ b/contrib/pg_tde/src/keyring/keyring_api.c @@ -100,16 +100,57 @@ RegisterKeyProviderType(const TDEKeyringRoutine *routine, ProviderType type) KeyInfo * KeyringGetKey(GenericKeyring *keyring, const char *key_name, KeyringReturnCode *returnCode) { + KeyInfo *key = NULL; RegisteredKeyProviderType *kp = find_key_provider_type(keyring->type); if (kp == NULL) { ereport(WARNING, - errmsg("Key provider of type %d not registered", keyring->type)); + errmsg("key provider of type %d not registered", keyring->type)); *returnCode = KEYRING_CODE_INVALID_PROVIDER; return NULL; } - return kp->routine->keyring_get_key(keyring, key_name, returnCode); + key = kp->routine->keyring_get_key(keyring, key_name, returnCode); + + if (*returnCode != KEYRING_CODE_SUCCESS || key == NULL) + return NULL; + + if (!ValidateKey(key)) + { + *returnCode = KEYRING_CODE_INVALID_KEY; + pfree(key); + return NULL; + } + + return key; +} + +bool +ValidateKey(KeyInfo *key) +{ + Assert(key != NULL); + + if (key->name[0] == '\0') + { + ereport(WARNING, errmsg("invalid key: name is empty")); + return false; + } + + if (key->data.len == 0) + { + ereport(WARNING, errmsg("invalid key: data length is zero")); + return false; + } + + /* For now we only support 128-bit keys */ + if (key->data.len != KEY_DATA_SIZE_128) + { + ereport(WARNING, + errmsg("invalid key: unsupported key length \"%u\"", key->data.len)); + return false; + } + + return true; } static void @@ -163,3 +204,25 @@ KeyringValidate(GenericKeyring *keyring) kp->routine->keyring_validate(keyring); } + +char * +KeyringErrorCodeToString(KeyringReturnCode code) +{ + switch (code) + { + case KEYRING_CODE_SUCCESS: + return "Success"; + case KEYRING_CODE_INVALID_PROVIDER: + return "Invalid key"; + case KEYRING_CODE_RESOURCE_NOT_AVAILABLE: + return "Resource not available"; + case KEYRING_CODE_INVALID_RESPONSE: + return "Invalid response from keyring provider"; + case KEYRING_CODE_INVALID_KEY: + return "Invalid key"; + case KEYRING_CODE_DATA_CORRUPTED: + return "Data corrupted"; + default: + return "Unknown error code"; + } +} diff --git a/contrib/pg_tde/src/keyring/keyring_kmip.c b/contrib/pg_tde/src/keyring/keyring_kmip.c index 74c70fc52f27a..df10fd56ced62 100644 --- a/contrib/pg_tde/src/keyring/keyring_kmip.c +++ b/contrib/pg_tde/src/keyring/keyring_kmip.c @@ -178,7 +178,7 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode if (key->data.len > sizeof(key->data.data)) { ereport(WARNING, errmsg("keyring provider returned invalid key size: %d", key->data.len)); - *return_code = KEYRING_CODE_INVALID_KEY_SIZE; + *return_code = KEYRING_CODE_INVALID_KEY; pfree(key); BIO_free_all(ctx.bio); SSL_CTX_free(ctx.ssl); diff --git a/contrib/pg_tde/src/keyring/keyring_vault.c b/contrib/pg_tde/src/keyring/keyring_vault.c index 74ab07c6c7bd0..130f77b32a4d7 100644 --- a/contrib/pg_tde/src/keyring/keyring_vault.c +++ b/contrib/pg_tde/src/keyring/keyring_vault.c @@ -220,7 +220,7 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode if (!curl_perform(vault_keyring, url, &str, &httpCode, NULL)) { - *return_code = KEYRING_CODE_INVALID_KEY_SIZE; + *return_code = KEYRING_CODE_INVALID_KEY; ereport(WARNING, errmsg("HTTP(S) request to keyring provider \"%s\" failed", vault_keyring->keyring.provider_name)); @@ -253,6 +253,15 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode goto cleanup; } + if (parse.key == NULL) + { + *return_code = KEYRING_CODE_INVALID_RESPONSE; + ereport(WARNING, + errmsg("HTTP(S) request to keyring provider \"%s\" returned no key", + vault_keyring->keyring.provider_name)); + goto cleanup; + } + responseKey = parse.key; #if KEYRING_DEBUG @@ -266,7 +275,7 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode if (key->data.len > MAX_KEY_DATA_SIZE) { - *return_code = KEYRING_CODE_INVALID_KEY_SIZE; + *return_code = KEYRING_CODE_INVALID_KEY; ereport(WARNING, errmsg("keyring provider \"%s\" returned invalid key size: %d", vault_keyring->keyring.provider_name, key->data.len)); diff --git a/contrib/pg_tde/t/expected/basic.out b/contrib/pg_tde/t/expected/basic.out index 587d8a7a31bf9..6f3fbc273c072 100644 --- a/contrib/pg_tde/t/expected/basic.out +++ b/contrib/pg_tde/t/expected/basic.out @@ -64,6 +64,6 @@ SELECT * FROM test_enc ORDER BY id; TABLEFILE FOUND: yes CONTAINS FOO (should be empty): SELECT pg_tde_verify_key() -psql::1: ERROR: failed to retrieve principal key test-db-key from keyring with ID 1 +psql::1: ERROR: key "test-db-key" not found in key provider "file-vault" DROP TABLE test_enc; DROP EXTENSION pg_tde; diff --git a/contrib/pg_tde/t/expected/change_key_provider.out b/contrib/pg_tde/t/expected/change_key_provider.out index e444fa7463e37..59de1281ae1a3 100644 --- a/contrib/pg_tde/t/expected/change_key_provider.out +++ b/contrib/pg_tde/t/expected/change_key_provider.out @@ -99,7 +99,7 @@ SELECT * FROM test_enc ORDER BY id; -- mv /tmp/change_key_provider_2.per /tmp/change_key_provider_1.per -- server restart SELECT pg_tde_verify_key(); -psql::1: ERROR: failed to retrieve principal key test-key from keyring with ID 1 +psql::1: ERROR: key "test-key" not found in key provider "file-vault" SELECT pg_tde_is_encrypted('test_enc'); pg_tde_is_encrypted --------------------- @@ -107,7 +107,7 @@ SELECT pg_tde_is_encrypted('test_enc'); (1 row) SELECT * FROM test_enc ORDER BY id; -psql::1: ERROR: failed to retrieve principal key test-key from keyring with ID 1 +psql::1: ERROR: key "test-key" not found in key provider "file-vault" SELECT pg_tde_change_database_key_provider_file('file-vault', '/tmp/change_key_provider_1.per'); pg_tde_change_database_key_provider_file ------------------------------------------ diff --git a/contrib/pg_tde/t/expected/key_validation.out b/contrib/pg_tde/t/expected/key_validation.out new file mode 100644 index 0000000000000..fe700ec77295e --- /dev/null +++ b/contrib/pg_tde/t/expected/key_validation.out @@ -0,0 +1,27 @@ +CREATE EXTENSION pg_tde; +SELECT pg_tde_add_database_key_provider_file('test-file-provider', '/tmp/pg_tde_test_key_validation.per'); + pg_tde_add_database_key_provider_file +--------------------------------------- + +(1 row) + +SELECT pg_tde_create_key_using_database_key_provider('key1', 'test-file-provider'); + pg_tde_create_key_using_database_key_provider +----------------------------------------------- + +(1 row) + +SELECT pg_tde_create_key_using_database_key_provider('key2', 'test-file-provider'); + pg_tde_create_key_using_database_key_provider +----------------------------------------------- + +(1 row) + +SELECT pg_tde_set_key_using_database_key_provider('key1', 'test-file-provider'); +psql::1: WARNING: invalid key: data length is zero +psql::1: ERROR: failed to retrieve principal key "key1" from key provider "test-file-provider" +DETAIL: Invalid key +SELECT pg_tde_set_key_using_database_key_provider('key2', 'test-file-provider'); +psql::1: WARNING: invalid key: unsupported key length "4294967295" +psql::1: ERROR: failed to retrieve principal key "key2" from key provider "test-file-provider" +DETAIL: Invalid key diff --git a/contrib/pg_tde/t/key_validation.pl b/contrib/pg_tde/t/key_validation.pl new file mode 100644 index 0000000000000..915d2e7bf5357 --- /dev/null +++ b/contrib/pg_tde/t/key_validation.pl @@ -0,0 +1,77 @@ +#!/usr/bin/perl + +use strict; +use warnings; +use File::Basename; +use Fcntl 'SEEK_CUR'; +use Test::More; +use lib 't'; +use pgtde; + +PGTDE::setup_files_dir(basename($0)); + +unlink('/tmp/pg_tde_test_key_validation.per'); + +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init; +$node->append_conf('postgresql.conf', "shared_preload_libraries = 'pg_tde'"); +$node->start; + +PGTDE::psql($node, 'postgres', 'CREATE EXTENSION pg_tde;'); +PGTDE::psql($node, 'postgres', + "SELECT pg_tde_add_database_key_provider_file('test-file-provider', '/tmp/pg_tde_test_key_validation.per');" +); +PGTDE::psql($node, 'postgres', + "SELECT pg_tde_create_key_using_database_key_provider('key1', 'test-file-provider');" +); +PGTDE::psql($node, 'postgres', + "SELECT pg_tde_create_key_using_database_key_provider('key2', 'test-file-provider');" +); + + +corrupt_key_file('/tmp/pg_tde_test_key_validation.per'); + + +PGTDE::psql($node, 'postgres', + "SELECT pg_tde_set_key_using_database_key_provider('key1', 'test-file-provider');" +); +PGTDE::psql($node, 'postgres', + "SELECT pg_tde_set_key_using_database_key_provider('key2', 'test-file-provider');" +); + +sub corrupt_key_file +{ + my ($keyfile) = @_; + + my $fh; + open($fh, '+<', $keyfile) + or BAIL_OUT("open failed: $!"); + binmode $fh; + + # Corrupt the first page of the key file by zeroing key data length. + # Offset is TDE_KEY_NAME_LEN + MAX_KEY_DATA_SIZE. See keyring_api.h for details. + sysseek($fh, 256 + 32, 0) + or BAIL_OUT("sysseek failed: $!"); + syswrite($fh, pack("L*", 0x00000000)) or BAIL_OUT("syswrite failed: $!"); + + # Corrupt the second page of the key file by setting incorrect key length. + # Offset is TDE_KEY_NAME_LEN + MAX_KEY_DATA_SIZE. See keyring_api.h for details. + sysseek($fh, 256 + 32, SEEK_CUR) + or BAIL_OUT("sysseek failed: $!"); + syswrite($fh, pack("L*", 0xFFFFFFFF)) or BAIL_OUT("syswrite failed: $!"); + + + close($fh) + or BAIL_OUT("close failed: $!"); +} + +$node->stop; + +# Compare the expected and out file +my $compare = PGTDE->compare_results(); + +is($compare, 0, + "Compare Files: $PGTDE::expected_filename_with_path and $PGTDE::out_filename_with_path files." +); + +done_testing();