From 157230de396d17ac71ebf9c27112377463f3dc15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20=C3=85strand?= Date: Wed, 16 Apr 2025 13:29:01 +0200 Subject: [PATCH] PG-1419 Validate key provider access This adds some validation to make sure we can access the key provider when it's created to make the user experience a little nicer. The actual access validation is very rudimentary for now but can easily be expanded. --- .../pg_tde/documentation/docs/architecture.md | 2 - contrib/pg_tde/expected/key_provider.out | 3 ++ contrib/pg_tde/expected/key_provider_1.out | 3 ++ contrib/pg_tde/expected/kmip_test.out | 3 ++ contrib/pg_tde/expected/vault_v2_test.out | 3 ++ contrib/pg_tde/sql/key_provider.sql | 3 ++ contrib/pg_tde/sql/kmip_test.sql | 3 ++ contrib/pg_tde/sql/vault_v2_test.sql | 3 ++ contrib/pg_tde/src/catalog/tde_keyring.c | 2 + .../pg_tde/src/include/keyring/keyring_api.h | 2 + contrib/pg_tde/src/keyring/keyring_api.c | 13 +++++++ contrib/pg_tde/src/keyring/keyring_file.c | 20 +++++++++- contrib/pg_tde/src/keyring/keyring_kmip.c | 16 +++++++- contrib/pg_tde/src/keyring/keyring_vault.c | 39 ++++++++++++++++++- 14 files changed, 110 insertions(+), 5 deletions(-) diff --git a/contrib/pg_tde/documentation/docs/architecture.md b/contrib/pg_tde/documentation/docs/architecture.md index 3be3f4f78ab..f7cd70e4805 100644 --- a/contrib/pg_tde/documentation/docs/architecture.md +++ b/contrib/pg_tde/documentation/docs/architecture.md @@ -206,8 +206,6 @@ To add a database specific provider: pg_tde_add_database_key_provider_('provider_name', ... details ...) ``` -Note that in these functions do not verify the parameters. For that, see `pg_tde_verify_key`. - ### Changing providers To change a value of a global provider: diff --git a/contrib/pg_tde/expected/key_provider.out b/contrib/pg_tde/expected/key_provider.out index 202555ae9da..51f45f3eb22 100644 --- a/contrib/pg_tde/expected/key_provider.out +++ b/contrib/pg_tde/expected/key_provider.out @@ -160,4 +160,7 @@ SELECT id, provider_name FROM pg_tde_list_all_global_key_providers(); -1 | file-keyring (1 row) +-- Creating a file key provider fails if we can't open or create the file +SELECT pg_tde_add_database_key_provider_file('will-not-work','/cant-create-file-in-root.per'); +ERROR: Failed to open keyring file /cant-create-file-in-root.per: Permission denied DROP EXTENSION pg_tde; diff --git a/contrib/pg_tde/expected/key_provider_1.out b/contrib/pg_tde/expected/key_provider_1.out index a583cc3dad1..4b072d6f35f 100644 --- a/contrib/pg_tde/expected/key_provider_1.out +++ b/contrib/pg_tde/expected/key_provider_1.out @@ -164,4 +164,7 @@ SELECT id, provider_name FROM pg_tde_list_all_global_key_providers(); -2 | file-keyring (2 rows) +-- Creating a file key provider fails if we can't open or create the file +SELECT pg_tde_add_database_key_provider_file('will-not-work','/cant-create-file-in-root.per'); +ERROR: Failed to open keyring file /cant-create-file-in-root.per: Permission denied DROP EXTENSION pg_tde; diff --git a/contrib/pg_tde/expected/kmip_test.out b/contrib/pg_tde/expected/kmip_test.out index 9b4949ece00..3d905bbaeed 100644 --- a/contrib/pg_tde/expected/kmip_test.out +++ b/contrib/pg_tde/expected/kmip_test.out @@ -34,4 +34,7 @@ SELECT pg_tde_verify_key(); (1 row) DROP TABLE test_enc; +-- Creating provider fails if we can't connect to kmip server +SELECT pg_tde_add_database_key_provider_kmip('will-not-work','127.0.0.1', 61, '/tmp/server_certificate.pem', '/tmp/client_key_jane_doe.pem'); +ERROR: SSL error: BIO_do_connect failed DROP EXTENSION pg_tde; diff --git a/contrib/pg_tde/expected/vault_v2_test.out b/contrib/pg_tde/expected/vault_v2_test.out index 06a2fd71949..b4ff2a1ffdf 100644 --- a/contrib/pg_tde/expected/vault_v2_test.out +++ b/contrib/pg_tde/expected/vault_v2_test.out @@ -51,4 +51,7 @@ SELECT pg_tde_verify_key(); (1 row) DROP TABLE test_enc; +-- Creating provider fails if we can't connect to vault +SELECT pg_tde_add_database_key_provider_vault_v2('will-not-work', :'root_token', 'http://127.0.0.1:61', 'secret', NULL); +ERROR: HTTP(S) request to keyring provider "will-not-work" failed DROP EXTENSION pg_tde; diff --git a/contrib/pg_tde/sql/key_provider.sql b/contrib/pg_tde/sql/key_provider.sql index 76eb036850a..78fcc71de83 100644 --- a/contrib/pg_tde/sql/key_provider.sql +++ b/contrib/pg_tde/sql/key_provider.sql @@ -52,4 +52,7 @@ SELECT id, provider_name FROM pg_tde_list_all_global_key_providers(); SELECT pg_tde_delete_global_key_provider('file-keyring2'); SELECT id, provider_name FROM pg_tde_list_all_global_key_providers(); +-- Creating a file key provider fails if we can't open or create the file +SELECT pg_tde_add_database_key_provider_file('will-not-work','/cant-create-file-in-root.per'); + DROP EXTENSION pg_tde; diff --git a/contrib/pg_tde/sql/kmip_test.sql b/contrib/pg_tde/sql/kmip_test.sql index 4b647ba1129..d47467e5fda 100644 --- a/contrib/pg_tde/sql/kmip_test.sql +++ b/contrib/pg_tde/sql/kmip_test.sql @@ -19,4 +19,7 @@ SELECT pg_tde_verify_key(); DROP TABLE test_enc; +-- Creating provider fails if we can't connect to kmip server +SELECT pg_tde_add_database_key_provider_kmip('will-not-work','127.0.0.1', 61, '/tmp/server_certificate.pem', '/tmp/client_key_jane_doe.pem'); + DROP EXTENSION pg_tde; diff --git a/contrib/pg_tde/sql/vault_v2_test.sql b/contrib/pg_tde/sql/vault_v2_test.sql index 4e8b92c97aa..205aa111853 100644 --- a/contrib/pg_tde/sql/vault_v2_test.sql +++ b/contrib/pg_tde/sql/vault_v2_test.sql @@ -31,4 +31,7 @@ SELECT pg_tde_verify_key(); DROP TABLE test_enc; +-- Creating provider fails if we can't connect to vault +SELECT pg_tde_add_database_key_provider_vault_v2('will-not-work', :'root_token', 'http://127.0.0.1:61', 'secret', NULL); + DROP EXTENSION pg_tde; diff --git a/contrib/pg_tde/src/catalog/tde_keyring.c b/contrib/pg_tde/src/catalog/tde_keyring.c index a0c844726f7..9e04ad04f36 100644 --- a/contrib/pg_tde/src/catalog/tde_keyring.c +++ b/contrib/pg_tde/src/catalog/tde_keyring.c @@ -472,6 +472,8 @@ check_provider_record(KeyringProviderRecord *provider_record) errmsg("Invalid provider options.")); } + KeyringValidate(provider); + pfree(provider); } diff --git a/contrib/pg_tde/src/include/keyring/keyring_api.h b/contrib/pg_tde/src/include/keyring/keyring_api.h index 3e3819d3b91..89be8282f85 100644 --- a/contrib/pg_tde/src/include/keyring/keyring_api.h +++ b/contrib/pg_tde/src/include/keyring/keyring_api.h @@ -61,6 +61,7 @@ typedef struct TDEKeyringRoutine { KeyInfo *(*keyring_get_key) (GenericKeyring *keyring, const char *key_name, KeyringReturnCodes *returnCode); void (*keyring_store_key) (GenericKeyring *keyring, KeyInfo *key); + void (*keyring_validate) (GenericKeyring *keyring); } TDEKeyringRoutine; typedef struct FileKeyring @@ -91,5 +92,6 @@ extern void RegisterKeyProviderType(const TDEKeyringRoutine *routine, ProviderTy extern KeyInfo *KeyringGetKey(GenericKeyring *keyring, const char *key_name, KeyringReturnCodes *returnCode); extern KeyInfo *KeyringGenerateNewKeyAndStore(GenericKeyring *keyring, const char *key_name, unsigned key_len); +extern void KeyringValidate(GenericKeyring *keyring); #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 766d75dc43f..f6d9785eef7 100644 --- a/contrib/pg_tde/src/keyring/keyring_api.c +++ b/contrib/pg_tde/src/keyring/keyring_api.c @@ -74,6 +74,7 @@ RegisterKeyProviderType(const TDEKeyringRoutine *routine, ProviderType type) Assert(routine != NULL); Assert(routine->keyring_get_key != NULL); Assert(routine->keyring_store_key != NULL); + Assert(routine->keyring_validate != NULL); kp = find_key_provider_type(type); if (kp) @@ -148,3 +149,15 @@ KeyringGenerateNewKeyAndStore(GenericKeyring *keyring, const char *key_name, uns return key; } + +void +KeyringValidate(GenericKeyring *keyring) +{ + RegisteredKeyProviderType *kp = find_key_provider_type(keyring->type); + + if (kp == NULL) + ereport(ERROR, + errmsg("Key provider of type %d not registered", keyring->type)); + + kp->routine->keyring_validate(keyring); +} diff --git a/contrib/pg_tde/src/keyring/keyring_file.c b/contrib/pg_tde/src/keyring/keyring_file.c index 22214d24b2e..5d1f83c1349 100644 --- a/contrib/pg_tde/src/keyring/keyring_file.c +++ b/contrib/pg_tde/src/keyring/keyring_file.c @@ -28,10 +28,12 @@ static KeyInfo *get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCodes *return_code); static void set_key_by_name(GenericKeyring *keyring, KeyInfo *key); +static void validate(GenericKeyring *keyring); const TDEKeyringRoutine keyringFileRoutine = { .keyring_get_key = get_key_by_name, - .keyring_store_key = set_key_by_name + .keyring_store_key = set_key_by_name, + .keyring_validate = validate, }; void @@ -143,3 +145,19 @@ set_key_by_name(GenericKeyring *keyring, KeyInfo *key) } close(fd); } + +static void +validate(GenericKeyring *keyring) +{ + FileKeyring *file_keyring = (FileKeyring *) keyring; + int fd = BasicOpenFile(file_keyring->file_name, O_CREAT | O_RDWR | PG_BINARY); + + if (fd < 0) + { + ereport(ERROR, + errcode_for_file_access(), + errmsg("Failed to open keyring file %s: %m", file_keyring->file_name)); + } + + close(fd); +} diff --git a/contrib/pg_tde/src/keyring/keyring_kmip.c b/contrib/pg_tde/src/keyring/keyring_kmip.c index fbceedd223c..86b438a9208 100644 --- a/contrib/pg_tde/src/keyring/keyring_kmip.c +++ b/contrib/pg_tde/src/keyring/keyring_kmip.c @@ -26,10 +26,12 @@ static void set_key_by_name(GenericKeyring *keyring, KeyInfo *key); static KeyInfo *get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCodes *return_code); +static void validate(GenericKeyring *keyring); const TDEKeyringRoutine keyringKmipRoutine = { .keyring_get_key = get_key_by_name, - .keyring_store_key = set_key_by_name + .keyring_store_key = set_key_by_name, + .keyring_validate = validate, }; void @@ -204,3 +206,15 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode return key; } + +static void +validate(GenericKeyring *keyring) +{ + KmipKeyring *kmip_keyring = (KmipKeyring *) keyring; + KmipCtx ctx; + + kmipSslConnect(&ctx, kmip_keyring, true); + + 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 c302e5d550b..03a92423719 100644 --- a/contrib/pg_tde/src/keyring/keyring_vault.c +++ b/contrib/pg_tde/src/keyring/keyring_vault.c @@ -70,10 +70,12 @@ static bool curl_perform(VaultV2Keyring *keyring, const char *url, CurlString *o static void set_key_by_name(GenericKeyring *keyring, KeyInfo *key); static KeyInfo *get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCodes *return_code); +static void validate(GenericKeyring *keyring); const TDEKeyringRoutine keyringVaultV2Routine = { .keyring_get_key = get_key_by_name, - .keyring_store_key = set_key_by_name + .keyring_store_key = set_key_by_name, + .keyring_validate = validate, }; void @@ -300,6 +302,41 @@ cleanup: return key; } +static void +validate(GenericKeyring *keyring) +{ + VaultV2Keyring *vault_keyring = (VaultV2Keyring *) keyring; + char url[VAULT_URL_MAX_LEN]; + CurlString str; + long httpCode = 0; + + /* + * Validate connection by listing available keys at the root level of the + * mount point + */ + snprintf(url, VAULT_URL_MAX_LEN, "%s/v1/%s/metadata/?list=true", + vault_keyring->vault_url, vault_keyring->vault_mount_path); + + if (!curl_perform(vault_keyring, url, &str, &httpCode, NULL)) + { + ereport(ERROR, + errmsg("HTTP(S) request to keyring provider \"%s\" failed", + vault_keyring->keyring.provider_name)); + } + + /* If the mount point doesn't have any secrets yet, we'll get a 404. */ + if (httpCode != 200 && httpCode != 404) + { + ereport(ERROR, + errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("Listing secrets of \"%s\" at mountpoint \"%s\" failed", + vault_keyring->vault_url, vault_keyring->vault_mount_path)); + } + + if (str.ptr != NULL) + pfree(str.ptr); +} + /* * JSON parser routines *