WAL keyring improvements #238

dAdAbird · 2024-07-10T11:04:23Z

Rename database key rotation functions to make room for the global space ones.
Now, during the first start, we would create a default temporary key provider for the global space. A user can (and should) create their own key provider afterwards. This allows use the same codepath and internal interfaces for the keyring management across databases and the global space.
Now need to cache the principal key for the global space as we use it only at the server start to decrypt internal key. Then internal key persists in the memory cache.

Fixes https://perconadev.atlassian.net/browse/PG-835, https://perconadev.atlassian.net/browse/PG-833

dutow · 2024-07-11T07:11:32Z

README.md

@@ -65,15 +65,15 @@ FUNCTION pg_tde_add_key_provider_file(
 SELECT pg_tde_add_key_provider_file('file','/tmp/pgkeyring');
 ```
 **Note: The `File` provided is intended for development and stores the keys unencrypted in the specified data file.**
-6. Set the principal key for the database using the `pg_tde_set_principal_key` function.
+6. Set the principal key for the database using the `pg_tde_set_database_key` function.


Now actually looking at this change, I don't think this is a good idea. We just agreed to name "master key" to "principal key" in the beta, and then we would rename all public occurrences again to "database key". Documentation / internal code would still say principal key, but the functions that actually deal with the keys would all be using the database key name.

Yeah, I had the same thoughts tbh. Should we have pg_tde_set_principal_db_key/ pg_tde_set_principal_global_space_key etc. instead?
The renaming is a one part and the other is creating respective functions for global spaces. But then something like pg_tde_set_principal_global_space_key becomes ridiculously long. Having just ...principal_global_key (w/o space) looks a bit confusing. And I'm starting to consider having pg_tde_set_principal_key() with the enum option DATABASE, GLOBAL_SPACE. But I'm not sure.

On another note, not related to this commit, but it might be a right time to do something about that:
Looking at pg_tde--1.0.sql we have inconsistency with the functions naming there. Most time they are prefixed with pg_tde_ but there are pg_tdeam_handler, pg_td2eam_handler, and pgtde_is_encrypted which don't follow that convention

dutow · 2024-07-24T08:12:47Z

documentation/docs/functions.md

@@ -33,43 +33,43 @@ where:

 All parameters can be either strings, or JSON objects [referencing remote parameters](external-parameters.md).

-## pg_tde_set_principal_key
+## pg_tde_set_database_key


database_principal_key?

dutow · 2024-07-24T08:13:01Z

documentation/docs/functions.md


 ```sql
-SELECT pg_tde_set_principal_key('name-of-the-principal-key', 'provider-name');
+SELECT pg_tde_set_database_key('name-of-the-principal-key', 'provider-name');


dutow · 2024-07-24T08:31:01Z

src/catalog/tde_principal_key.c

+    ensure_new_key = PG_GETARG_BOOL(2);
+
+
+    ereport(LOG, (errmsg("Rotating principal key to [%s : %s] for the database", new_principal_key_name, new_provider_name)));


for the cluster, or maybe explicitly mention global key?

dutow · 2024-07-24T08:34:36Z

src/catalog/tde_keyring.c

@@ -499,7 +511,23 @@ get_keyring_infofile_path(char* resPath, Oid dbOid, Oid spcOid)
 }

 Datum
-pg_tde_add_key_provider_internal(PG_FUNCTION_ARGS)
+pg_tde_add_database_key_provider_internal(PG_FUNCTION_ARGS)


We could avoid all this code duplication with a simple helper function, only 1 line is different in the two variants.

dutow · 2024-07-25T07:58:10Z

src/catalog/tde_keyring.c

+	Oid dbOid = MyDatabaseId;
+	Oid spcOid = MyDatabaseTableSpace;
+
+#ifdef PERCONA_FORK


Do we really need this ifdef here?

Yep, you're right, we can get rid of it: cf02b52

dutow · 2024-07-25T07:59:06Z

src/catalog/tde_principal_key.c


+#ifdef PERCONA_FORK


Same question, do we need this ifdef?

... to make room for global tablespace's ones. Also chage signature of the key rotation func

- Move internal keys to the own cache (separate from db's one) - Drop the support of multiple principal keys for the global space

dutow

Looks good, please make sure to use squash and merge, so we don't get this long commit history into the main branch.

- Rename database key rotation functions to make room for the global space ones. - Now, during the first start, we would create a default temporary key provider for the global space. A user can (and should) create their own key provider afterwards. This allows use the same codepath and internal interfaces for the keyring management across databases and the global space. - Now need to cache the principal key for the global space as we use it only at the server start to decrypt internal key. Then internal key persists in the memory cache. Fixes https://perconadev.atlassian.net/browse/PG-835, https://perconadev.atlassian.net/browse/PG-833

dutow reviewed Jul 11, 2024

View reviewed changes

dAdAbird force-pushed the xlog_keyring branch 4 times, most recently from 672d012 to be3c77f Compare July 16, 2024 15:37

dAdAbird marked this pull request as ready for review July 18, 2024 13:52

dAdAbird requested review from dutow and codeforall July 18, 2024 13:54

dAdAbird force-pushed the xlog_keyring branch from 8d960ae to 8862b16 Compare July 23, 2024 12:30

dutow reviewed Jul 24, 2024

View reviewed changes

dAdAbird force-pushed the xlog_keyring branch 2 times, most recently from 68189fe to a5378e8 Compare July 24, 2024 17:34

dutow reviewed Jul 25, 2024

View reviewed changes

dAdAbird added 12 commits July 26, 2024 14:30

Rename keyring UI functions

8c43faf

... to make room for global tablespace's ones. Also chage signature of the key rotation func

Global space keys cache refactoring

ea16bf2

- Move internal keys to the own cache (separate from db's one) - Drop the support of multiple principal keys for the global space

Use common keyring and key rotation

380f78f

Get rid of now obsolete interfaces

795cf10

No need in a pricipal key cache for the gobal space

4392fb8

Get rid off redundant actions + comments

432db6c

Rename UI keyring functions

6839b09

Rename internal functions

4fe68fc

Fix default global principal key info

eefdfbb

Simplify UI w/ overloaded funcs

5fb83e6

Get rid of redundant checks

e8e779e

Fix rebase issues

f657ccb

dAdAbird force-pushed the xlog_keyring branch from cf02b52 to f657ccb Compare July 26, 2024 12:04

dAdAbird requested a review from dutow July 26, 2024 15:13

dutow approved these changes Jul 26, 2024

View reviewed changes

dAdAbird merged commit 02cd531 into percona:main Jul 30, 2024
10 checks passed

dAdAbird deleted the xlog_keyring branch July 30, 2024 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WAL keyring improvements #238

WAL keyring improvements #238

dAdAbird commented Jul 10, 2024 •

edited

Loading

dutow Jul 11, 2024

dAdAbird Jul 11, 2024

dAdAbird Jul 11, 2024

dutow Jul 24, 2024

dutow Jul 24, 2024

dutow Jul 24, 2024

dutow Jul 24, 2024

dutow Jul 25, 2024

dAdAbird Jul 26, 2024

dutow Jul 25, 2024

dutow left a comment

		ensure_new_key = PG_GETARG_BOOL(2);


		ereport(LOG, (errmsg("Rotating principal key to [%s : %s] for the database", new_principal_key_name, new_provider_name)));


		#ifdef PERCONA_FORK

WAL keyring improvements #238

WAL keyring improvements #238

Conversation

dAdAbird commented Jul 10, 2024 • edited Loading

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

dAdAbird commented Jul 10, 2024 •

edited

Loading