Separate key creation and usage #428

AndersAstrand · 2025-06-12T22:01:47Z

This adds a function to explicitly create keys at key providers. It also removes the "create if exists" semantics as that seems quite useless in the real world. If customers want it it's easy to add later.

The documentation is not updated in this PR since I ran out of time and needed to submit the code changes for review.

I will update the documentation in a separate PR.

codecov-commenter · 2025-06-12T22:05:38Z

Codecov Report

Attention: Patch coverage is 83.78378% with 6 lines in your changes missing coverage. Please review.

Project coverage is 84.66%. Comparing base (086b342) to head (1eff35c).
Report is 3 commits behind head on TDE_REL_17_STABLE.

❌ Your project status has failed because the head coverage (84.66%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@                  Coverage Diff                  @@
##           TDE_REL_17_STABLE     #428      +/-   ##
=====================================================
- Coverage              84.76%   84.66%   -0.10%     
=====================================================
  Files                     21       21              
  Lines                   2566     2589      +23     
  Branches                 393      402       +9     
=====================================================
+ Hits                    2175     2192      +17     
- Misses                   311      316       +5     
- Partials                  80       81       +1

Components	Coverage Δ
access	`81.11% <ø> (ø)`
catalog	`88.22% <83.78%> (-0.40%)`	⬇️
common	`77.77% <ø> (ø)`
encryption	`73.45% <ø> (ø)`
keyring	`72.88% <ø> (ø)`
src	`91.44% <ø> (ø)`
smgr	`94.88% <ø> (+0.02%)`	⬆️
transam	`∅ <ø> (∅)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dutow · 2025-06-12T22:21:10Z

contrib/pg_tde/sql/access_control.sql

@@ -1,3 +1,5 @@
+\! rm -f '/tmp/pg_tde_test_keyring.per'


Even if we remove the file, the previous keys are still in the cache - shouldn't that cause issues? It seems a bit strange that create_key succeeds, even if we have a key named exactly that in the in memory cache. (multiple tests create test-db-key, but we never delete it)

My main concern with this, are you sure that we are using the correct key in this situation? The affected tests that reuse the same keyname don't restart the server. create_key won't update the cache, that usually happens when we retrieve the key. So it is possible that we store a new random generated key in the file, but we keep using the old key from the cache.

No, I just realized we still push it to the cache in set, so that part should still work - but this still seems like a possibly problematic situation.

I don't understand what the problem is. The files are removed before the providers are even created, so there is nothing to be in cache. The providers doesn't even exist when these files are removed.

The sql tests are reusing the same postgres instance - so even if we delete these files, the in memory cache is still there, we aren't doing a complete reset.

Thinking about it more this is probably fine, but we are still running tests in a not entirely realistic way with this.

Hmm ok I get what you mean, but each regress file does recreate the extension, and thus runs clear_principal_key_cache(dbOid) at start of each script. so it's probably fine.

I agree that we should maybe think about how that stuff works a little bit more though!

dutow · 2025-06-13T09:57:45Z

contrib/pg_tde/src/catalog/tde_principal_key.c

+	if (providerOid == GLOBAL_DATA_TDE_OID && !superuser())
+		ereport(ERROR,
+				errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				errmsg("must be superuser to access global key providers"));


I'm not 100% sure that access is the correct word there - create maybe?

This is the same message as we use for the other places where we check this. Maybe "use" is a better word than "access"?

Maybe. Access is just confusing in this context to me, because it suggests a read only operation, and this isn't that, it always writes a new key.

jeltz

I think the change is generally good but this did me dislike the local vs global a bit more due to have long that makes the function names. Also had a few comments.

jeltz · 2025-06-13T10:15:22Z

contrib/pg_tde/src/catalog/tde_principal_key.c

@@ -447,15 +445,99 @@ clear_principal_key_cache(Oid databaseId)
 * SQL interface to set principal key
 */

+Datum
+pg_tde_create_key_using_database_key_provider(PG_FUNCTION_ARGS)


I think I would prefer to call it generate to differentiate it from commands which creates objects inside pg_tde.

Hmm... I think we should be better at what words we use. But to me this definitely feels like we're creating something.

We don't have any other user facing functions named "create" afaik, so I don't think there is something to differentiate from.

I also prefer create, as we are creating keys on the providers, generate wouldn't be that explicit about this.

I disagree but seems like I am outvoted. :) I think generate makes the usage more clear.

jeltz · 2025-06-13T10:18:37Z

contrib/pg_tde/src/catalog/tde_principal_key.c

 	}

-	if (strlen(key_name) >= sizeof(keyInfo->name))


Don't we actually still need this check due to our file format? Imagine if a key exists which they have created themselves which is longer than the field size in our file format?

Oh. You're completely right. I forgot that these names are stored in the tdemap header. Thank you!

contrib/pg_tde/src/catalog/tde_principal_key.c

AndersAstrand · 2025-06-13T10:26:38Z

I think the change is generally good but this did me dislike the local vs global a bit more due to have long that makes the function names. Also had a few comments.

I agree about the database/global thing, but I don't think we'll have time to change that unfortunately :(

Also move a couple of checks to the calling function.

To ensure the tests are always run from the same state we remove any key provider files so that pg_tde_add_database/global_key_provider_file() always creates a new file.

Add new functions pg_tde_create_key_using_database/global_key_provider() to create keys instead of key creation being a side effect of setting the key. Also remove support for "create if not exists" semantics as any user should know what keys their key provider contains.

AndersAstrand · 2025-06-13T11:13:35Z

New version pushed!

jeltz

This PR should really include documentation changes too but I am approving it since we are so close to the freeze. Decide yourself if you merge with our without documentation changes.

AndersAstrand requested review from dutow and dAdAbird as code owners June 12, 2025 22:01

dutow reviewed Jun 12, 2025

View reviewed changes

dutow approved these changes Jun 13, 2025

View reviewed changes

jeltz requested changes Jun 13, 2025

View reviewed changes

AndersAstrand added 3 commits June 13, 2025 13:04

Clean up errors from set key functions

c1e9b01

Also move a couple of checks to the calling function.

Remove key provider files in regress tests

073f365

To ensure the tests are always run from the same state we remove any key provider files so that pg_tde_add_database/global_key_provider_file() always creates a new file.

AndersAstrand force-pushed the tde/separate-key-creation-and-usage branch from 73c9ea5 to 1eff35c Compare June 13, 2025 11:12

jeltz approved these changes Jun 13, 2025

View reviewed changes

AndersAstrand merged commit ceff90e into percona:TDE_REL_17_STABLE Jun 13, 2025
17 checks passed

AndersAstrand deleted the tde/separate-key-creation-and-usage branch June 13, 2025 12:21

Separate key creation and usage #428

Separate key creation and usage #428

Uh oh!

Conversation

AndersAstrand commented Jun 12, 2025

Uh oh!

codecov-commenter commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndersAstrand Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeltz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndersAstrand Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndersAstrand Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndersAstrand commented Jun 13, 2025

Uh oh!

AndersAstrand commented Jun 13, 2025

Uh oh!

jeltz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jun 12, 2025 •

edited

Loading

AndersAstrand Jun 13, 2025 •

edited

Loading

AndersAstrand Jun 13, 2025 •

edited

Loading

AndersAstrand Jun 13, 2025 •

edited

Loading