Implement interfaces and framework for multi tenancy #118

codeforall · 2024-02-15T11:27:33Z

The PR aims towards building support for multi tenancy and contains multiple commits for the following.

Establishing a Framework for Master Key.
New Shared Cache Management Framework.
Adding catalog table for managing key providers.
Revamping the Keyring API Interface and Integrating Master Key.

Individual commit messages contain a more detailed description of changes.

This commit introduces a user catalog table, percona_tde.pg_tde_key_provider, within the percona_tde schema, as part of the pg_tde extension. The purpose of this table is to store essential provider information. The catalog accommodates various key providers, present and future, utilizing a JSON type options field to capture provider-specific details. To facilitate the creation of key providers, the commit introduces new SQL interfaces: - pg_tde_add_file_wallet(provider_name VARCHAR(128), file_path TEXT) - pg_tde_add_wallet(provider_type VARCHAR(10), provider_name VARCHAR(128), options JSON) - pg_tde_add_vault_v2_wallet(provider_name VARCHAR(128), vault_token TEXT, vault_url TEXT, vault_mount_path TEXT, vault_ca_path TEXT) Additionally, the commit implements the C interface for catalog interaction, detailed in the 'tde_keyring.h' file. These changes lay the foundation for implementing multi-tenancy in pg_tde by eliminating the necessity of a 'keyring.json' file for configuring a cluster-wide key provider. With this enhancement, each database can have its dedicated key provider, added via SQL interface, removing the need for DBA intervention in TDE setup."

Up until now, pg_tde relied on a hard-coded master key name, primarily for proof-of-concept purposes. This commit introduces a more robust infrastructure for configuring the master key and managing a dynamic shared memory-based master-key cache to enhance accessibility. For user interaction, a new SQL interface is provided: - pg_tde_set_master_key(master_key_name text, provider_name text); This interface enables users to set a master key for a specific database and make further enhancements toward implementing the multi-tenancy. In addition to the public SQL interface, the commit optimizes the internal master-key API. It introduces straightforward Get and Set functions, handling locking, retrieval, caching, and assignment of a master key for a database seamlessly. The commit also introduces a unified internal interface for requesting and utilizing shared memory, contributing to a more cohesive and efficient master key and cache management system.

This commit unifies the master-key and key-provider modules with the core of pg_tde, marking a significant evolution in the architecture. As part of this integration, the keyring API undergoes substantial changes to enhance flexibility and remove unnecessary components such as the key cache. As a result of the keyring refactoring, the file keyring is also rewritten, offering a template for implementing additional key providers for the extension. The modifications make the keyring API more pluggable, streamlining interactions and paving the way for future enhancements.

…rements This commit addresses PostgreSQL core's requirement for upfront information regarding the number of locks needed by the extension. Given the connection between locks and the shared memory interface, a new callback routine is introduced. This routine allows modules to specify the number of locks they require. In addition to this functionality, the commit includes code cleanups and adjustments to nomenclature for improved clarity and consistency.

dutow

I only added two minor comments/questions related to the public interface

dutow · 2024-02-20T18:26:07Z

src/keyring/keyring_api.c

 	{
-		case PROVIDER_FILE:
-		keyringFilePreloadCache();


Are you sure we don't need the preload function? For some keyrings, such as file, it makes sense to read everything at startup

Since we would only require one master key at any time for a database as no version number gets appended with the key name so IMHO reading only one required key should be enough.

dutow · 2024-02-20T18:27:16Z

pg_tde--1.0.sql

+LANGUAGE SQL;
+
+
+CREATE OR REPLACE FUNCTION pg_tde_add_wallet(provider_type VARCHAR(10), provider_name VARCHAR(128), options JSON)


This wallet name seems strange to me, I would keep calling them provider

Makes sense. I have changed the function names as you suggested in the latest (#121) PR

dAdAbird · 2024-02-20T11:24:13Z

pg_tde--1.0.sql

+CREATE OR REPLACE FUNCTION pg_tde_set_master_key(provider_type VARCHAR(10), provider_name VARCHAR(128), options JSON)
+RETURNS INT
+AS $$
+    INSERT INTO percona_tde.pg_tde_key_provider (keyring_type, provider_name, options) VALUES (provider_type, provider_name, options) RETURNING provider_id;
+$$
+LANGUAGE SQL;
+
+
+CREATE OR REPLACE FUNCTION pg_tde_add_wallet(provider_type VARCHAR(10), provider_name VARCHAR(128), options JSON)
+RETURNS INT
+AS $$
+    INSERT INTO percona_tde.pg_tde_key_provider (keyring_type, provider_name, options) VALUES (provider_type, provider_name, options) RETURNING provider_id;
+$$
+LANGUAGE SQL;


What is the difference between those functions?

The function was left as an oversight. fixed in the latest (#121) PR

dAdAbird · 2024-02-20T17:25:08Z

pg_tde--1.0.sql

@@ -13,9 +64,12 @@ RETURNS boolean
 AS $$ SELECT amname = 'pg_tde' FROM pg_class INNER JOIN pg_am ON pg_am.oid = pg_class.relam WHERE relname = table_name $$
 LANGUAGE SQL;

+CREATE FUNCTION pg_tde_set_master_key(master_key_name text, provider_name text)


Given that #define TDE_KEY_NAME_LEN 256, should master_key_name be VARCHAR(256)?

Fixed in (#121) PR

dAdAbird · 2024-02-21T08:43:13Z

pg_tde--1.0.sql

+RETURNS INT
+AS $$
+    SELECT pg_tde_add_wallet('file', provider_name,
+                json_object('type' VALUE 'file', 'path' VALUE file_path));
+$$
+LANGUAGE SQL;
+
+CREATE OR REPLACE FUNCTION pg_tde_add_vault_v2_wallet(provider_name VARCHAR(128),
+                                                        valut_token TEXT,
+                                                        valut_url TEXT,
+                                                        valut_mount_path TEXT,
+                                                        valut_ca_path TEXT)
+RETURNS INT
+AS $$
+    SELECT pg_tde_add_wallet('vault-v2', provider_name,
+                            json_object('type' VALUE 'vault-v2',
+                            'url' VALUE valut_url,
+                            'token' VALUE valut_token,
+                            'mountPath' VALUE valut_mount_path,
+                            'caPath' VALUE valut_ca_path));
+$$
+LANGUAGE SQL;


Should be comments that these relations structure must be in sync with catalog/tde_keyring.c's macros

Fixed in (#121) PR

dAdAbird · 2024-02-21T08:46:48Z

src/include/pg_tde_shmem.h

+#include "lib/dshash.h"
+#include "utils/dsa.h"
+
+typedef struct TDEShmemSetupRoutine


Need comments on interface's functions - what should be implemented (what interface expects from them)

I have added a few comments in shared memory .h file in the latest PR

dAdAbird · 2024-02-21T09:21:05Z

src/pg_tde.c


-	RequestAddinShmemSpace(keyringCacheMemorySize());
+	RequestNamedLWLockTranche("pg_tde_tranche", required_locks);


Should "pg_tde_tranche" be a macros?

dAdAbird · 2024-02-21T09:38:26Z

src/catalog/tde_master_key.c

+    masterKey = get_master_key_from_cache();
+    if (!masterKey)


But what should be done if masterKey exists here? It must be an error as in L361-370?
I think either get_master_key_from_cache() and get_master_key_info() checks in L361-380 should be under the lock or they should be duplicated after we acquired the lock (if we want to have cheap pre-check before locking - in that case it make sense to make a macros for that code) otherwise function will have unpredictable for the caller behaviour.

You are right. I have fixed it in #121

dutow · 2024-02-21T09:46:31Z

src/keyring/keyring_file.c

+			errmsg("Failed to open keyring file %s :%m", file_keyring->file_name)));
+        return KEYRING_CODE_RESOURCE_NOT_ACCESSABLE;
+    }
+    bytes_written = FileWrite(file, key, sizeof(keyInfo), 0, WAIT_EVENT_DATA_FILE_WRITE);


Doesn't this overwrite the file with a single key again and again, if we have multiple master keys?

The previous implementation also overwrote the file, but dumped the entire cache, not just one key.

Thanks for pointing it out. Fixed in #121

dAdAbird · 2024-02-26T10:44:16Z

Do we need this PR anymore (having #121)?

codeforall · 2024-02-27T13:01:44Z

There is a new PR for the same. Closing this draft

codeforall added 4 commits February 15, 2024 15:44

dutow reviewed Feb 20, 2024

View reviewed changes

dAdAbird reviewed Feb 21, 2024

View reviewed changes

dutow reviewed Feb 21, 2024

View reviewed changes

codeforall closed this Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement interfaces and framework for multi tenancy #118

Implement interfaces and framework for multi tenancy #118

codeforall commented Feb 15, 2024

dutow left a comment

dutow Feb 20, 2024

codeforall Feb 21, 2024

dutow Feb 20, 2024

codeforall Feb 21, 2024

dAdAbird Feb 20, 2024

codeforall Feb 21, 2024

dAdAbird Feb 20, 2024

codeforall Feb 21, 2024

dAdAbird Feb 21, 2024

codeforall Feb 21, 2024

dAdAbird Feb 21, 2024

codeforall Feb 21, 2024

dAdAbird Feb 21, 2024

codeforall Feb 22, 2024

dAdAbird Feb 21, 2024

codeforall Feb 21, 2024

dutow Feb 21, 2024

codeforall Feb 21, 2024

dAdAbird commented Feb 26, 2024

codeforall commented Feb 27, 2024

		LANGUAGE SQL;


		CREATE OR REPLACE FUNCTION pg_tde_add_wallet(provider_type VARCHAR(10), provider_name VARCHAR(128), options JSON)


		RequestAddinShmemSpace(keyringCacheMemorySize());
		RequestNamedLWLockTranche("pg_tde_tranche", required_locks);

Implement interfaces and framework for multi tenancy #118

Implement interfaces and framework for multi tenancy #118

Conversation

codeforall commented Feb 15, 2024

dutow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dAdAbird commented Feb 26, 2024

codeforall commented Feb 27, 2024