From 3e4e0a3b2b479022cc20b5cd938aeaf197460ed3 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi Date: Fri, 13 Dec 2024 08:27:15 +0000 Subject: [PATCH] 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. --- expected/vault_v2_test.out | 17 ++++++++++++++++- expected/vault_v2_test_basic.out | 17 ++++++++++++++++- sql/vault_v2_test.inc | 11 +++++++++++ src/access/pg_tde_tdemap.c | 16 ++++++++++------ src/catalog/tde_principal_key.c | 8 +++++--- src/keyring/keyring_api.c | 2 ++ src/pg_tde_event_capture.c | 1 - 7 files changed, 60 insertions(+), 12 deletions(-) diff --git a/expected/vault_v2_test.out b/expected/vault_v2_test.out index 789a6ec7..b1037d72 100644 --- 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 index c3cc0204..5fcedd36 100644 --- 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 index 221e6390..70c71906 100644 --- 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 index a3cb8c5a..366e8716 100644 --- 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 index c3326c5a..5316cbe5 100644 --- 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 index 076779db..8fa56716 100644 --- 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 index 8d7794ae..1a7518f5 100644 --- 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))