From c4053375461a3fcc12852813952e69d8815b0be5 Mon Sep 17 00:00:00 2001 From: Andreas Karlsson Date: Thu, 2 Apr 2026 20:32:28 +0200 Subject: [PATCH 1/2] Do not return NULL when failing to parse a key provider config The bad habit of returning NULL when we fail to parse a value from the key provider has a high risk of hiding bugs and data corrupt by just making it seem like the key provider never existed in the first place. --- src/catalog/tde_keyring.c | 56 +++++++++++++++------------------------ 1 file changed, 21 insertions(+), 35 deletions(-) diff --git a/src/catalog/tde_keyring.c b/src/catalog/tde_keyring.c index 6ed8b5c6..148d273b 100644 --- a/src/catalog/tde_keyring.c +++ b/src/catalog/tde_keyring.c @@ -540,13 +540,6 @@ check_provider_record(KeyringProviderRecord *provider_record) /* Validate that the provider record can be properly parsed. */ provider = load_keyring_provider_from_record(provider_record); - if (provider == NULL) - { - ereport(ERROR, - errcode(ERRCODE_DATA_EXCEPTION), - errmsg("Invalid provider options.")); - } - KeyringValidate(provider); if (provider->keyring_id != 0) @@ -801,16 +794,13 @@ scan_key_provider_file(ProviderScanType scanType, void *scanKey, Oid dbOid) { GenericKeyring *keyring = load_keyring_provider_from_record(&provider); - if (keyring) - { #ifndef FRONTEND - providers_list = lappend(providers_list, keyring); + providers_list = lappend(providers_list, keyring); #else - if (providers_list == NULL) - providers_list = palloc0_object(SimplePtrList); - simple_ptr_list_append(providers_list, keyring); + if (providers_list == NULL) + providers_list = palloc0_object(SimplePtrList); + simple_ptr_list_append(providers_list, keyring); #endif - } } } CloseTransientFile(fd); @@ -825,14 +815,11 @@ load_keyring_provider_from_record(KeyringProviderRecord *provider) keyring = load_keyring_provider_options(provider->provider_type, provider->options); - if (keyring) - { - keyring->keyring_id = provider->provider_id; - memcpy(keyring->provider_name, provider->provider_name, sizeof(keyring->provider_name)); - keyring->type = provider->provider_type; - memcpy(keyring->options, provider->options, sizeof(keyring->options)); - debug_print_kerying(keyring); - } + keyring->keyring_id = provider->provider_id; + memcpy(keyring->provider_name, provider->provider_name, sizeof(keyring->provider_name)); + keyring->type = provider->provider_type; + memcpy(keyring->options, provider->options, sizeof(keyring->options)); + debug_print_kerying(keyring); return keyring; } @@ -849,7 +836,9 @@ load_keyring_provider_options(ProviderType provider_type, char *keyring_options) case KMIP_KEY_PROVIDER: return (GenericKeyring *) load_kmip_keyring_provider_options(keyring_options); default: - return NULL; + ereport(ERROR, + errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("unknown key provider type: %d", provider_type)); } } @@ -865,12 +854,11 @@ load_file_keyring_provider_options(char *keyring_options) if (file_keyring->file_name == NULL || file_keyring->file_name[0] == '\0') { - ereport(WARNING, + free_keyring((GenericKeyring *) file_keyring); + + ereport(ERROR, errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("file path is missing in the keyring options")); - - free_keyring((GenericKeyring *) file_keyring); - return NULL; } return file_keyring; @@ -891,15 +879,14 @@ load_vaultV2_keyring_provider_options(char *keyring_options) vaultV2_keyring->vault_url == NULL || vaultV2_keyring->vault_url[0] == '\0' || vaultV2_keyring->vault_mount_path == NULL || vaultV2_keyring->vault_mount_path[0] == '\0') { - ereport(WARNING, + free_keyring((GenericKeyring *) vaultV2_keyring); + + ereport(ERROR, errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("missing in the keyring options:%s%s%s", (vaultV2_keyring->vault_token_path != NULL && vaultV2_keyring->vault_token_path[0] != '\0') ? "" : " tokenPath", (vaultV2_keyring->vault_url != NULL && vaultV2_keyring->vault_url[0] != '\0') ? "" : " url", (vaultV2_keyring->vault_mount_path != NULL && vaultV2_keyring->vault_mount_path[0] != '\0') ? "" : " mountPath")); - - free_keyring((GenericKeyring *) vaultV2_keyring); - return NULL; } /* TODO: the vault_token mem should be protected from paging to the swap */ @@ -924,7 +911,9 @@ load_kmip_keyring_provider_options(char *keyring_options) kmip_keyring->kmip_cert_path == NULL || kmip_keyring->kmip_cert_path[0] == '\0' || kmip_keyring->kmip_key_path == NULL || kmip_keyring->kmip_key_path[0] == '\0') { - ereport(WARNING, + free_keyring((GenericKeyring *) kmip_keyring); + + ereport(ERROR, errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("missing in the keyring options:%s%s%s%s%s", (kmip_keyring->kmip_host != NULL && kmip_keyring->kmip_host[0] != '\0') ? "" : " host", @@ -932,9 +921,6 @@ load_kmip_keyring_provider_options(char *keyring_options) (kmip_keyring->kmip_ca_path != NULL && kmip_keyring->kmip_ca_path[0] != '\0') ? "" : " caPath", (kmip_keyring->kmip_cert_path != NULL && kmip_keyring->kmip_cert_path[0] != '\0') ? "" : " certPath", (kmip_keyring->kmip_key_path != NULL && kmip_keyring->kmip_key_path[0] != '\0') ? "" : " keyPath")); - - free_keyring((GenericKeyring *) kmip_keyring); - return NULL; } return kmip_keyring; From c0055ea8f97d3062150f7e1032b7ff050b36d549 Mon Sep 17 00:00:00 2001 From: Andreas Karlsson Date: Thu, 2 Apr 2026 20:47:53 +0200 Subject: [PATCH 2/2] Merge two functions in tde_keyring.c These two functions being separate made the code harder to read. --- src/catalog/tde_keyring.c | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/src/catalog/tde_keyring.c b/src/catalog/tde_keyring.c index 148d273b..3110d03a 100644 --- a/src/catalog/tde_keyring.c +++ b/src/catalog/tde_keyring.c @@ -54,7 +54,6 @@ static bool fetch_next_key_provider(int fd, off_t *curr_pos, KeyringProviderReco static inline void get_keyring_infofile_path(char *resPath, Oid dbOid); static FileKeyring *load_file_keyring_provider_options(char *keyring_options); static GenericKeyring *load_keyring_provider_from_record(KeyringProviderRecord *provider); -static GenericKeyring *load_keyring_provider_options(ProviderType provider_type, char *keyring_options); static KmipKeyring *load_kmip_keyring_provider_options(char *keyring_options); static VaultV2Keyring *load_vaultV2_keyring_provider_options(char *keyring_options); static int open_keyring_infofile(Oid dbOid, int flags); @@ -813,33 +812,30 @@ load_keyring_provider_from_record(KeyringProviderRecord *provider) { GenericKeyring *keyring; - keyring = load_keyring_provider_options(provider->provider_type, provider->options); - - keyring->keyring_id = provider->provider_id; - memcpy(keyring->provider_name, provider->provider_name, sizeof(keyring->provider_name)); - keyring->type = provider->provider_type; - memcpy(keyring->options, provider->options, sizeof(keyring->options)); - debug_print_kerying(keyring); - - return keyring; -} - -static GenericKeyring * -load_keyring_provider_options(ProviderType provider_type, char *keyring_options) -{ - switch (provider_type) + switch (provider->provider_type) { case FILE_KEY_PROVIDER: - return (GenericKeyring *) load_file_keyring_provider_options(keyring_options); + keyring = (GenericKeyring *) load_file_keyring_provider_options(provider->options); + break; case VAULT_V2_KEY_PROVIDER: - return (GenericKeyring *) load_vaultV2_keyring_provider_options(keyring_options); + keyring = (GenericKeyring *) load_vaultV2_keyring_provider_options(provider->options); + break; case KMIP_KEY_PROVIDER: - return (GenericKeyring *) load_kmip_keyring_provider_options(keyring_options); + keyring = (GenericKeyring *) load_kmip_keyring_provider_options(provider->options); + break; default: ereport(ERROR, errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("unknown key provider type: %d", provider_type)); + errmsg("unknown key provider type: %d", provider->provider_type)); } + + keyring->keyring_id = provider->provider_id; + memcpy(keyring->provider_name, provider->provider_name, sizeof(keyring->provider_name)); + keyring->type = provider->provider_type; + memcpy(keyring->options, provider->options, sizeof(keyring->options)); + debug_print_kerying(keyring); + + return keyring; } static FileKeyring *