Keyrotation and key name versioning implementation #144

hqakhtar · 2024-03-15T20:39:36Z

The initial version of the key rotation functionality without the xlog functionality.

This patch includes some minor refactoring as well as changes to the return type of the set key function, which now returns a boolean.

Master key versioning implemented by Usama. A new field of versioned_name is added.

Co-authored-by: Muhammad Usama [email protected]

xlog functionality. This patch includes some minor refactoring as well as changes the return type of the set key function which now returns a boolean.

versioned_name is added. Co-authored-by: Muhammad Usama <[email protected]>

…ing.

feature complete and dev-tested. As part of this commit, XLog for key rotation is implemented and verified that during streaming replication, when master key is rotated, the key map and the data file on primary and standby are binary equivalent when the Xlog record is replayed on the standby.

src/access/pg_tde_tdemap.c

dAdAbird · 2024-03-17T20:08:41Z

src/catalog/tde_master_key.c

+    while (true)
+    {
+        keyInfo = KeyringGetKey(keyring, new_master_key.keyInfo.keyId.versioned_name, false, &keyring_ret);
+        if (keyInfo == NULL || ensure_new_key == false)


ensure_new_key is always true, do we need || ensure_new_key == false here?

We will add a SQL interface ( A new argument to set key function) after this PR gets in. So for now it is set to always true.

src/catalog/tde_master_key.c

dAdAbird · 2024-03-17T20:53:23Z

src/catalog/tde_master_key.c

+    while (true)
+    {
+        keyInfo = KeyringGetKey(keyring, new_master_key.keyInfo.keyId.versioned_name, false, &keyring_ret);
+        if (keyInfo == NULL || ensure_new_key == false)


The code in L422-447 looks like it is generalized to be shared with set_master_key_with_keyring(). But the code is duplicated in both funcions and it is much more complicated than it has to be. For example here in addition to redundant check || ensure_new_key == false we also don't need L440 check if (keyInfo == NULL) as this is the only way to reach the code after the loop.
In set_master_key_with_keyring() the loop is redundant by it self.
As for me either code in these function should be simplified or this common code has to be moved to the separate function and reused. Otherwise it is really confusing.

BTW, should we always generate a new key during rotation? Can't a user set a key with the "new name" which is already exists in a keyring and we use it? And generate a new key only if a provided new name doesn't exists or the name is the same as the name of the current key (is such a case we generate a new version of the key). I'm just asking because I'm not sure what is the supposed logic in that regard.
Can we have a comment w/ these rules?

We should have functionality to use an existing key, for example when multiple databases use the same master key. One rotation should generate a new version, the others just should use the latest.

Basically, the loop appears redundant because of the missing SQL interface. The idea is that if users want to use the existing key ( for whatever reason), they call the set key function with mastrer-key name (using ensure_new_key=false) that already exists in the key provider. Secondly, if the user intends to use the latest version of the key when the key name already exists in the key provider, then they can do so by passing ensure_new_key=true.
This PR just adds the infrastructure for enabling the above.

dAdAbird · 2024-03-17T21:12:49Z

src/access/pg_tde_tdemap.c

 */
 bool
-pg_tde_perform_rotate_key(const char *new_master_key_name)
+pg_tde_perform_rotate_key(TDEMasterKey *master_key, TDEMasterKey *new_master_key)


Do we need locks in this function? Or should the caller have locks?
I imagine it can be call by different processes which can rewrite/unlink files at the same time.

dutow · 2024-03-18T08:02:53Z

src/catalog/tde_master_key.c

+
+            masterKey->keyInfo.keyId.version++;
+
+            snprintf(masterKey->keyInfo.keyId.versioned_name, TDE_KEY_NAME_LEN, "%s_%d", key_name, masterKey->keyInfo.keyId.version);


Setting this variable here seems out of place - shouldn't it be part of keyring internals?

dutow · 2024-03-18T08:04:55Z

src/catalog/tde_master_key.c

+        while (true)
+        {
+            keyInfo = KeyringGetKey(keyring, masterKey->keyInfo.keyId.versioned_name, false, &keyring_ret);
+            if (keyInfo == NULL || ensure_new_key == false)


This condition seems strange to me - if ensure_new_key is false we always return the first version of the key, not the last?

dutow · 2024-03-18T08:07:53Z

src/catalog/tde_master_key.c

+    while (true)
+    {
+        keyInfo = KeyringGetKey(keyring, new_master_key.keyInfo.keyId.versioned_name, false, &keyring_ret);
+        if (keyInfo == NULL || ensure_new_key == false)


We should have functionality to use an existing key, for example when multiple databases use the same master key. One rotation should generate a new version, the others just should use the latest.

dutow · 2024-03-18T08:10:13Z

src/catalog/tde_master_key.c

        /* We need to get the key from keyring */
+        while (true)
+        {
+            keyInfo = KeyringGetKey(keyring, masterKey->keyInfo.keyId.versioned_name, false, &keyring_ret);


I see now why you introduced versioned_name, but this is a hack and not a good solution. Instead of this, KeyringGetKey should receive keyInfo as a parameter, and deal with the name construction internally.

IMHO the key version is the function of master_key, not the keyring. And keyring should only be concerned about key-name and if that key-name contains the version number appended to it so be it. Also managing the version as part of master key naming allows the keyring to get-set master key without constructing the key name every time and that is good for performance as well.

dutow · 2024-03-18T08:12:42Z

pg_tde--1.0.sql

@@ -81,13 +81,13 @@ 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_rotate_key(key_name VARCHAR)
+CREATE FUNCTION pg_tde_rotate_key(new_master_key_name VARCHAR(255), new_provider_name VARCHAR(255))


99% of the time, users just want to rotate the key, and that's it. We should have an overload without any arguments.

We also need a function that just updates the file to the latest already existing key, instead of creating a new one.

…ws an error.

The commit contains the following changes. Moved the logic for getting the latest available master key name to a separate function. Changed the base name for the master key. Now it always contains the version number postfix. Added a SQL interface to allow users to specify if they need to ensure the key during set and rotate master key operations. Also making a few adjustments in file keyring file handling.

vault-v2 returns 404 (KEYRING_CODE_RESOURCE_NOT_AVAILABLE) when key is not found so it should not be treated as an error

Commit also adds a key rotation tap test case

hqakhtar · 2024-03-20T18:07:43Z

As agreed during the weekly meeting, the discussion points are now pretty much resolved by the latest commits from @codeforall and @EngineeredVirus, barring the locking issue, which will come as a separate PR for the whole structure, not just key rotation.

Hamid Akhtar and others added 4 commits March 16, 2024 01:35

Initial version of the key rotation functionality without the

5d3a450

xlog functionality. This patch includes some minor refactoring as well as changes the return type of the set key function which now returns a boolean.

Master key versioning implemented by Usama. New field of

f50f3ff

versioned_name is added. Co-authored-by: Muhammad Usama <[email protected]>

Fixing issue where regression was failing because of key name version…

d0d824f

…ing.

dAdAbird reviewed Mar 17, 2024

View reviewed changes

dutow reviewed Mar 18, 2024

View reviewed changes

Hamid Akhtar and others added 6 commits March 19, 2024 17:52

Resolving a couple of feedback coments on the PR. durable_.* now thro…

9d88997

…ws an error.

Removing left out NOTICE message from previous commit

3b04d48

Fix vault-v2 test case

6c7e339

vault-v2 returns 404 (KEYRING_CODE_RESOURCE_NOT_AVAILABLE) when key is not found so it should not be treated as an error

Delete master-key cache entry as part of key rotation

c543479

Commit also adds a key rotation tap test case

Ensuring that we are clearing master key cache during xlog redo as well.

f9bd8fa

hqakhtar merged commit 661c209 into percona:main Mar 20, 2024
10 checks passed

codeforall linked an issue Mar 21, 2024 that may be closed by this pull request

Implement Master Key Rotation #111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keyrotation and key name versioning implementation #144

Keyrotation and key name versioning implementation #144

hqakhtar commented Mar 15, 2024

dAdAbird Mar 17, 2024

codeforall Mar 18, 2024

dAdAbird Mar 17, 2024

dAdAbird Mar 17, 2024

dutow Mar 18, 2024

codeforall Mar 18, 2024

dAdAbird Mar 17, 2024

dutow Mar 18, 2024

dutow Mar 18, 2024

dutow Mar 18, 2024

dutow Mar 18, 2024

codeforall Mar 18, 2024

dutow Mar 18, 2024

hqakhtar commented Mar 20, 2024


		masterKey->keyInfo.keyId.version++;

		snprintf(masterKey->keyInfo.keyId.versioned_name, TDE_KEY_NAME_LEN, "%s_%d", key_name, masterKey->keyInfo.keyId.version);

Keyrotation and key name versioning implementation #144

Keyrotation and key name versioning implementation #144

Conversation

hqakhtar commented Mar 15, 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

hqakhtar commented Mar 20, 2024