Framework for multi-tenancy support #121

codeforall · 2024-02-21T17:02:31Z

The PR aims to build multi-tenancy support 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.

Few Notes:

A few test cases might fail with PR, but that is because of the file location of the test file vault and not because of any functionality issue. We can take care of that as a separate commit after discussion.
Currently, the PR only creates a framework and implements a mechanism to install a key provider and master keys for a database. Altering, Deleting, and handling master-key dependency on key provider will be taken care of as a separate PR
Unifying master-key-info with keymap file is also a TODO item for the next PR

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_key_provider(provider_type VARCHAR(10), provider_name VARCHAR(128), options JSON) - pg_tde_add_key_provider_file(provider_name VARCHAR(128), file_path TEXT) - pg_tde_add_key_provider_vault_v2(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 VARCHAR(255), provider_name VARCHAR(255)); 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.

pg_tde--1.0.sql

dutow · 2024-02-21T18:00:31Z

sql/move_large_tuples.sql

@@ -1,6 +1,9 @@
 -- test pg_tde_move_encrypted_data()
 CREATE EXTENSION pg_tde;

+SELECT pg_tde_add_key_provider_file('file-valut','/tmp/pg_tde_test_keyring.per');


Here and in other files: valut looks like a typo

dutow · 2024-02-21T18:01:38Z

sql/toast_extended_storage.sql

@@ -1,6 +1,9 @@
 -- test https://github.com/Percona-Lab/pg_tde/issues/63
 CREATE EXTENSION pg_tde;

+SELECT pg_tde_add_key_provider_file('file-valut','/tmp/pg_tde_test_keyring.per');
+--SELECT pg_tde_set_master_key('test-db-master-key','file-valut');


Why is this commented out?

Right now we need to figure out a way to delete the files created by extension if the extension is deleted. So I have intentionally commented out these lines to take care of that. IMHO we can handle this as a separate PR

dutow · 2024-02-21T18:06:35Z

src/catalog/tde_keyring.c

+	keyring_name = TextDatumGetCString(datum);
+
+	option_datum = heap_getattr(tuple, PG_TDE_KEY_PROVIDER_OPTIONS_ATTRNUM, tupDesc, &isnull);
+	/*keyring_options = TextDatumGetCString(option_datum);*/


Why is this commented out?

dutow · 2024-02-21T18:08:31Z

src/catalog/tde_keyring.c

+	FileKeyring *file_keyring = palloc0(sizeof(FileKeyring));
+	/*
+	Datum file_type;
+	file_type = DirectFunctionCall2(json_object_field_text, keyring_options, CStringGetTextDatum(FILE_KEYRING_TYPE_KEY));


Why the duplicated commented out code?

dutow · 2024-02-21T18:21:08Z

src/include/catalog/tde_keyring.h

+{
+	ProviderType type; /* Must be the first field */
+	Oid keyId;
+	char keyName[128];


Shouldn't we add a define for 128?

dutow · 2024-02-21T18:21:45Z

src/include/catalog/tde_master_key.h

+{
+	Oid databaseId;
+	uint32 keyVersion;
+    Oid keyringId;


indentation

dutow · 2024-02-21T18:22:44Z

src/include/keyring/keyring_api.h

 } keyName;

 typedef struct keyData
 {
-	unsigned char data[32]; // maximum 256 bit encryption
+	unsigned char data[MAX_KEY_DATA_SIZE]; /* maximum 256 bit encryption */


I don't think we need this comment at both places

dutow · 2024-02-21T18:23:20Z

src/include/keyring/keyring_api.h

+extern KeyringReturnCodes KeyringStoreKey(GenericKeyring *keyring, keyInfo *key, bool throw_error);
+extern keyInfo *KeyringGetKey(GenericKeyring *keyring, const char *key_name, bool throw_error, KeyringReturnCodes *returnCode);
+
+extern keyInfo *keyringGenerateNewKeyAndStore(GenericKeyring *keyring, const char *key_name, unsigned key_len, bool throw_error);


KeyringGenerate to match the other functions?

Not fixed, the k is still small, this file now has functions with different naming conventions

dutow · 2024-02-21T18:26:24Z

src/keyring/keyring_file.c

+			errmsg("Failed to open keyring file %s :%m", file_keyring->file_name)));
+        return KEYRING_CODE_RESOURCE_NOT_ACCESSABLE;
+    }
+	/* Write key to the end of file */


Shouldn't this function also report an error if the key already exists in the file?

You are right! Committed the fix

dAdAbird

It also makes to run pgindent on new files.

dAdAbird · 2024-02-26T09:11:09Z

pg_tde--1.0.sql

@@ -18,9 +74,12 @@ RETURNS boolean
 AS 'MODULE_PATHNAME'
 LANGUAGE C;

+CREATE FUNCTION pg_tde_set_master_key(master_key_name VARCHAR(255), provider_name VARCHAR(255))


In percona_tde.pg_tde_key_provider, provider_name len is 256 but here is 255

Thanks for pointing it out. I've updated the catalog definition.

src/catalog/tde_master_key.c

dAdAbird · 2024-02-26T11:28:06Z

src/catalog/tde_master_key.c

+ * push the master key for current database to the shared memory cache
+ */
+static void
+push_master_key_to_cache(TDEMasterKey *masterKey)


As a TODO for the future: set a limit on the cache size (and add eviction policy). As I understand, now cache might grow indefinitely

That is a correct observation. At the same time, we would only have one cache entry for each database, so I am not expecting the cache to grow insanely huge, So IMHO we can take care of this at a later stage. For now I have added a TODO note on the function.

Co-authored-by: Andrew Pogrebnoi <[email protected]>

Adjusting provider_name length in pg_tde_key_provider catalog definition and adding a TODO note for implementing an eviction policy for the master key cache.

This commit enhances the extension by adding a new mechanism to facilitate cleanup or setup procedures when the extension is installed in a database. The core addition is a function "pg_tde_extension_initialize" invoked upon executing the database's 'CREATE EXTENSION' command. To streamline future development and ensure extensibility, the commit introduces a callback registration mechanism. This enables any module to specify a callback function (registered using on_ext_install() ) to be invoked during extension creation. As of this commit, the callback functionality is explicitly utilized by the master key module to handle the cleanup of the master key information file. This file might persist in the database directory if the extension had been previously deleted in the same database. This enhancement paves the way for a more modular and maintainable extension architecture, allowing individual modules to manage their specific setup and cleanup tasks seamlessly." Commit also contains the test case adjustments.

dutow · 2024-02-27T18:09:36Z

src/catalog/tde_keyring.c

+	keyring = load_keyring_provider_options(provider_type, option_datum);
+	if (keyring)
+	{
+		strncpy(keyring->keyName, keyring_name, sizeof(keyring->keyName));


We are missing a null check for keyring_name

Catalog definition ensures that we don't get NULL keyring name

dutow · 2024-02-27T18:12:15Z

src/catalog/tde_keyring.c

+	Datum file_path;
+	FileKeyring *file_keyring = palloc0(sizeof(FileKeyring));
+	file_path = DirectFunctionCall2(json_object_field_text, keyring_options, CStringGetTextDatum(FILE_KEYRING_PATH_KEY));
+	/* TODO check NULL  and verify type */


Shouldn't be there an issue about this?

Adjusted the function definition to ensure we do not get a NULL JSON value

dutow · 2024-02-27T18:14:29Z

src/catalog/tde_master_key.c

+    Assert(keyring != NULL);
+
+    masterKeyInfo = palloc(sizeof(TDEMasterKeyInfo));
+    masterKeyInfo->keyId = 0;


@codeforall ^ still a question

dutow · 2024-02-27T18:14:56Z

src/catalog/tde_master_key.c

+
+    masterKey = palloc(sizeof(TDEMasterKey));
+    masterKey->databaseId = MyDatabaseId;
+    masterKey->keyVersion = 1;


@codeforall ^

dutow · 2024-02-27T18:17:27Z

src/include/keyring/keyring_api.h

+extern KeyringReturnCodes KeyringStoreKey(GenericKeyring *keyring, keyInfo *key, bool throw_error);
+extern keyInfo *KeyringGetKey(GenericKeyring *keyring, const char *key_name, bool throw_error, KeyringReturnCodes *returnCode);
+
+extern keyInfo *keyringGenerateNewKeyAndStore(GenericKeyring *keyring, const char *key_name, unsigned key_len, bool throw_error);


Not fixed, the k is still small, this file now has functions with different naming conventions

dutow · 2024-02-27T18:18:28Z

src/keyring/keyring_file.c

+get_key_by_name(GenericKeyring* keyring, const char* key_name, bool throw_error, KeyringReturnCodes *returnCode)
+{
+	keyInfo* key = NULL;
+    File file = -1;


indentation (at multiple places in this file)

I tried to fix as many as I could find. It seems like my editor needs some setting fixes as everything appears correctly indented.

dutow · 2024-02-27T18:19:27Z

src/keyring/keyring_file.c


-	datafile = keyringParseStringParam(dataO);
+static keyInfo*
+get_key_by_name(GenericKeyring* keyring, const char* key_name, bool throw_error, KeyringReturnCodes *returnCode)


And this function also mixes naming styles (key_name and returnCode)

dutow

I see no changes for keyring vault. Will it continue to work after these changes?

dutow · 2024-02-28T07:06:58Z

sql/multi_insert.sql

@@ -2,6 +2,9 @@
 -- 
 CREATE EXTENSION pg_tde;

+SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per');


I realized an issue with these changes: now the tests are hardcoded to only run with keyring file, previously we ran them both with keyring file and vault. With these changes, effectively we no longer execute vault tests.

If we go in this direction, we have to duplicate all test files, and later when we add additional providers, copy and copy them again and again. I don't think this is a good approach, but I do not see how we can do it differently, as the tests would require SQL commands before execution.

Commit also contains the fix for review comments and a test case for Vault-V2 type key provider

codeforall added 5 commits February 21, 2024 15:14

Adjusting test cases

e8b1381

codeforall mentioned this pull request Feb 21, 2024

Implement interfaces and framework for multi tenancy #118

Closed

dutow reviewed Feb 21, 2024

View reviewed changes

Taking care of review comments and some code cleanups

7e4b41f

dAdAbird reviewed Feb 26, 2024

View reviewed changes

codeforall and others added 5 commits February 26, 2024 21:05

Fix typo in src/catalog/tde_master_key.c

2a82f57

Co-authored-by: Andrew Pogrebnoi <[email protected]>

Update comment in src/catalog/tde_master_key.c

d49c679

Co-authored-by: Andrew Pogrebnoi <[email protected]>

Taking care of review comments

ce0ce5c

Adjusting provider_name length in pg_tde_key_provider catalog definition and adding a TODO note for implementing an eviction policy for the master key cache.

Fixing the tap test case

5f31b50

dutow reviewed Feb 27, 2024

View reviewed changes

dutow reviewed Feb 28, 2024

View reviewed changes

codeforall added 3 commits February 28, 2024 16:23

Adjusting Vault-V2 key provider to use new keyring architecture

fc7bedf

Commit also contains the fix for review comments and a test case for Vault-V2 type key provider

Fixing a silly typo

52e9d24

Disabling vault test case and adjusting perf test workflow

cdcba4d

dutow approved these changes Feb 28, 2024

View reviewed changes

codeforall merged commit 210c95c into percona:main Feb 28, 2024
8 checks passed

codeforall mentioned this pull request Feb 28, 2024

Provide SQL interface for Key management #108

Closed

codeforall linked an issue Feb 28, 2024 that may be closed by this pull request

Provide SQL interface for Key management #108

Closed

codeforall mentioned this pull request Mar 6, 2024

Keyring API Code Needs A Major Rewrite #109

Closed

Framework for multi-tenancy support #121

Framework for multi-tenancy support #121

Conversation

codeforall commented Feb 21, 2024

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 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

dutow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment