PG-1254, PG-1253, PG-1250: Improve diagnostic messages

* Event triggers dumped a debug log message with the LOG level. This commit completely removes it, this is a debug functionality for development, it can be simply re-added temporarily during event trigger development * Many, but not all FATAL messages are now ERROR messages. A few messages dealing with data corruption are still FATAL. * Some of these also got a TODO comment, as it looks like those would result in a corrupted internal key file, resulting in data loss. These parts should be improved. * Key generation on keyring was missing some error messages, and also displayed some of the messages as WARNING instead of ERROR. This is now all fixed, functions like pg_tde_set_principal key only display one specific error message explaining the problem.
percona · Dec 13, 2024 · 3e4e0a3 · 3e4e0a3
1 parent 69f8337
commit 3e4e0a3
Show file tree

Hide file tree

Showing 7 changed files with 60 additions and 12 deletions.
diff --git a/expected/vault_v2_test.out b/expected/vault_v2_test.out
@@ -2,12 +2,27 @@
 \i sql/vault_v2_test.inc
 CREATE EXTENSION pg_tde;
 \getenv root_token ROOT_TOKEN
-SELECT pg_tde_add_key_provider_vault_v2('vault-v2',:'root_token','http://127.0.0.1:8200','secret',NULL);
+SELECT pg_tde_add_key_provider_vault_v2('vault-incorrect',:'root_token','http://127.0.0.1:8200','DUMMY-TOKEN',NULL);
  pg_tde_add_key_provider_vault_v2 
 ----------------------------------
                                 1
 (1 row)
 
+-- FAILS
+SELECT pg_tde_set_principal_key('vault-v2-principal-key','vault-incorrect');
+psql:sql/vault_v2_test.inc:7: ERROR:  Failed to store key on keyring. Please check the keyring configuration.
+CREATE TABLE test_enc(
+	  id SERIAL,
+	  k INTEGER DEFAULT '0' NOT NULL,
+	  PRIMARY KEY (id)
+	) USING :tde_am;
+psql:sql/vault_v2_test.inc:13: ERROR:  failed to retrieve principal key. Create one using pg_tde_set_principal_key before using encrypted tables.
+SELECT pg_tde_add_key_provider_vault_v2('vault-v2',:'root_token','http://127.0.0.1:8200','secret',NULL);
+ pg_tde_add_key_provider_vault_v2 
+----------------------------------
+                                2
+(1 row)
+
 SELECT pg_tde_set_principal_key('vault-v2-principal-key','vault-v2');
  pg_tde_set_principal_key 
 --------------------------

diff --git a/expected/vault_v2_test_basic.out b/expected/vault_v2_test_basic.out
@@ -2,12 +2,27 @@
 \i sql/vault_v2_test.inc
 CREATE EXTENSION pg_tde;
 \getenv root_token ROOT_TOKEN
-SELECT pg_tde_add_key_provider_vault_v2('vault-v2',:'root_token','http://127.0.0.1:8200','secret',NULL);
+SELECT pg_tde_add_key_provider_vault_v2('vault-incorrect',:'root_token','http://127.0.0.1:8200','DUMMY-TOKEN',NULL);
  pg_tde_add_key_provider_vault_v2 
 ----------------------------------
                                 1
 (1 row)
 
+-- FAILS
+SELECT pg_tde_set_principal_key('vault-v2-principal-key','vault-incorrect');
+psql:sql/vault_v2_test.inc:7: ERROR:  Failed to store key on keyring. Please check the keyring configuration.
+CREATE TABLE test_enc(
+	  id SERIAL,
+	  k INTEGER DEFAULT '0' NOT NULL,
+	  PRIMARY KEY (id)
+	) USING :tde_am;
+psql:sql/vault_v2_test.inc:13: ERROR:  failed to retrieve principal key. Create one using pg_tde_set_principal_key before using encrypted tables.
+SELECT pg_tde_add_key_provider_vault_v2('vault-v2',:'root_token','http://127.0.0.1:8200','secret',NULL);
+ pg_tde_add_key_provider_vault_v2 
+----------------------------------
+                                2
+(1 row)
+
 SELECT pg_tde_set_principal_key('vault-v2-principal-key','vault-v2');
  pg_tde_set_principal_key 
 --------------------------

diff --git a/sql/vault_v2_test.inc b/sql/vault_v2_test.inc
@@ -1,6 +1,17 @@
 CREATE EXTENSION pg_tde;
 
 \getenv root_token ROOT_TOKEN
+
+SELECT pg_tde_add_key_provider_vault_v2('vault-incorrect',:'root_token','http://127.0.0.1:8200','DUMMY-TOKEN',NULL);
+-- FAILS
+SELECT pg_tde_set_principal_key('vault-v2-principal-key','vault-incorrect');
+
+CREATE TABLE test_enc(
+	  id SERIAL,
+	  k INTEGER DEFAULT '0' NOT NULL,
+	  PRIMARY KEY (id)
+	) USING :tde_am;
+
 SELECT pg_tde_add_key_provider_vault_v2('vault-v2',:'root_token','http://127.0.0.1:8200','secret',NULL);
 SELECT pg_tde_set_principal_key('vault-v2-principal-key','vault-v2');
 

diff --git a/src/access/pg_tde_tdemap.c b/src/access/pg_tde_tdemap.c
@@ -177,7 +177,7 @@ pg_tde_create_key_map_entry(const RelFileLocator *newrlocator, uint32 entry_type
 	if (!RAND_bytes(int_key.key, INTERNAL_KEY_LEN))
 	{
 		LWLockRelease(lock_pk);
-		ereport(FATAL,
+		ereport(ERROR,
 				(errcode(ERRCODE_INTERNAL_ERROR),
 				 errmsg("could not generate internal key for relation \"%s\": %s",
 						"TODO", ERR_error_string(ERR_get_error(), NULL))));
@@ -439,8 +439,9 @@ pg_tde_write_one_map_entry(int fd, const RelFileLocator *rlocator, uint32 flags,
 	{
 		char db_map_path[MAXPGPATH] = {0};
 
+		// TODO: this seems like a bad idea?
 		pg_tde_set_db_file_paths(rlocator->dbOid, db_map_path, NULL);
-		ereport(FATAL,
+		ereport(ERROR,
 				(errcode_for_file_access(),
 				 errmsg("could not write tde map file \"%s\": %m",
 						db_map_path)));
@@ -449,6 +450,7 @@ pg_tde_write_one_map_entry(int fd, const RelFileLocator *rlocator, uint32 flags,
 	{
 		char db_map_path[MAXPGPATH] = {0};
 
+		// TODO: this seems like a bad idea?
 		pg_tde_set_db_file_paths(rlocator->dbOid, db_map_path, NULL);
 		ereport(data_sync_elevel(ERROR),
 				(errcode_for_file_access(),
@@ -500,7 +502,8 @@ pg_tde_write_one_keydata(int fd, int32 key_index, RelKeyData *enc_rel_key_data)
 	/* TODO: pgstat_report_wait_start / pgstat_report_wait_end */
 	if (pg_pwrite(fd, &enc_rel_key_data->internal_key, INTERNAL_KEY_DAT_LEN, curr_pos) != INTERNAL_KEY_DAT_LEN)
 	{
-		ereport(FATAL,
+		// TODO: what now? File is corrupted
+		ereport(ERROR,
 				(errcode_for_file_access(),
 				 errmsg("could not write tde key data file: %m")));
 	}
@@ -1022,10 +1025,11 @@ pg_tde_process_map_entry(const RelFileLocator *rlocator, uint32 key_type, char *
 
 		if (curr_pos == -1)
 		{
-			ereport(FATAL,
+			ereport(ERROR,
 					(errcode_for_file_access(),
 					 errmsg("could not seek in tde map file \"%s\": %m",
 							db_map_path)));
+			return curr_pos;
 		}
 	}
 	else
@@ -1117,7 +1121,7 @@ tde_decrypt_rel_key(TDEPrincipalKey *principal_key, RelKeyData *enc_rel_key_data
  * Open and Validate File Header [pg_tde.*]:
  * 		header: {Format Version, Principal Key Name}
  *
- * Returns the file descriptor in case of a success. Otherwise, fatal error
+ * Returns the file descriptor in case of a success. Otherwise, error
  * is raised.
  *
  * Also, it sets the is_new_file to true if the file is just created. This is
@@ -1160,7 +1164,7 @@ pg_tde_open_file(char *tde_filename, TDEPrincipalKeyInfo *principal_key_info, bo
 /*
  * Open a TDE file [pg_tde.*]:
  *
- * Returns the file descriptor in case of a success. Otherwise, fatal error
+ * Returns the file descriptor in case of a success. Otherwise, error
  * is raised except when ignore_missing is true and the file does not exit.
  */
 static int

diff --git a/src/catalog/tde_principal_key.c b/src/catalog/tde_principal_key.c
@@ -262,7 +262,7 @@ set_principal_key_with_keyring(const char *key_name, GenericKeyring *keyring,
 		keyInfo = load_latest_versioned_key_name(&principalKey->keyInfo, keyring, ensure_new_key);
 
 		if (keyInfo == NULL)
-			keyInfo = KeyringGenerateNewKeyAndStore(keyring, principalKey->keyInfo.keyId.versioned_name, INTERNAL_KEY_LEN, false);
+			keyInfo = KeyringGenerateNewKeyAndStore(keyring, principalKey->keyInfo.keyId.versioned_name, INTERNAL_KEY_LEN, true);
 
 		if (keyInfo == NULL)
 		{
@@ -424,7 +424,7 @@ RotatePrincipalKey(TDEPrincipalKey *current_key, const char *new_key_name, const
 	keyInfo = load_latest_versioned_key_name(&new_principal_key.keyInfo, keyring, ensure_new_key);
 
 	if (keyInfo == NULL)
-		keyInfo = KeyringGenerateNewKeyAndStore(keyring, new_principal_key.keyInfo.keyId.versioned_name, INTERNAL_KEY_LEN, false);
+		keyInfo = KeyringGenerateNewKeyAndStore(keyring, new_principal_key.keyInfo.keyId.versioned_name, INTERNAL_KEY_LEN, true);
 
 	if (keyInfo == NULL)
 	{
@@ -496,9 +496,10 @@ load_latest_versioned_key_name(TDEPrincipalKeyInfo *principal_key_info, GenericK
 		 */
 		if (kr_ret != KEYRING_CODE_SUCCESS && kr_ret != KEYRING_CODE_RESOURCE_NOT_AVAILABLE)
 		{
-			ereport(FATAL,
+			ereport(ERROR,
 					(errmsg("failed to retrieve principal key from keyring provider :\"%s\"", keyring->provider_name),
 					 errdetail("Error code: %d", kr_ret)));
+			return NULL;
 		}
 		if (keyInfo == NULL)
 		{
@@ -531,6 +532,7 @@ load_latest_versioned_key_name(TDEPrincipalKeyInfo *principal_key_info, GenericK
 		{
 			ereport(ERROR,
 					(errmsg("failed to retrieve principal key. %d versions already exist", MAX_PRINCIPAL_KEY_VERSION_NUM)));
+			return NULL;
 		}
 	}
 	return NULL;				/* Just to keep compiler quite */

diff --git a/src/keyring/keyring_api.c b/src/keyring/keyring_api.c
@@ -164,6 +164,8 @@ KeyringGenerateNewKeyAndStore(GenericKeyring *keyring, const char *key_name, uns
 	if (KeyringStoreKey(keyring, key, throw_error) != KEYRING_CODE_SUCCESS)
 	{
 		pfree(key);
+		ereport(ereport_level,
+			(errmsg("Failed to store key on keyring. Please check the keyring configuration.")));
 		return NULL;
 	}
 	return key;

diff --git a/src/pg_tde_event_capture.c b/src/pg_tde_event_capture.c
@@ -68,7 +68,6 @@ pg_tde_ddl_command_start_capture(PG_FUNCTION_ARGS)
 	trigdata = (EventTriggerData *) fcinfo->context;
 	parsetree = trigdata->parsetree;
 
-	elog(LOG, "EVENT TRIGGER (%s) %s", trigdata->event, nodeToString(parsetree));
 	reset_current_tde_create_event();
 
 	if (IsA(parsetree, IndexStmt))