PG-1661 Validate key coming from key providers

Check that key that we retreived from key provider is valid.

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 modified 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 modified key provider "db-provider"
 \c :regress_database
 DROP DATABASE db_using_database_provider;
 -- Deleting key providers fails if key name is NULL

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),
+				errmsg("failed to retrieve principal key \"%s\" from key provider \"%s\"", xlrec->keyName, new_keyring->provider_name),
-				errdetail("Error code: %d", kr_ret));
+				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",
+				errmsg("key \"%s\" not found in key provider \"%s\"",
-					   principalKeyInfo->data.name, principalKeyInfo->data.keyringId));
+					   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",
+					errcode(ERRCODE_NO_DATA_FOUND),
-						   key_name, modified_provider->provider_name, return_code));
+					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 modified 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",
+						errcode(ERRCODE_NO_DATA_FOUND),
-							   key_name, database->datname.data, modified_provider->provider_name, return_code));
+						errmsg("key \"%s\" used by database \"%s\" not found in modified 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)

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 */

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";
+	}
+}

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);

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));
@@ -266,7 +266,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));

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:<stdin>:1: ERROR:  failed to retrieve principal key test-db-key from keyring with ID 1
+psql:<stdin>:1: ERROR:  key "test-db-key" not found in key provider "file-vault"
 DROP TABLE test_enc;
 DROP EXTENSION pg_tde;

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:<stdin>:1: ERROR:  failed to retrieve principal key test-key from keyring with ID 1
+psql:<stdin>: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:<stdin>:1: ERROR:  failed to retrieve principal key test-key from keyring with ID 1
+psql:<stdin>: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 
 ------------------------------------------

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:<stdin>:1: WARNING:  invalid key: data length is zero
+psql:<stdin>: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:<stdin>:1: WARNING:  invalid key: unsupported key length "4294967295"
+psql:<stdin>:1: ERROR:  failed to retrieve principal key "key2" from key provider "test-file-provider"
+DETAIL:  Invalid key

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();