Separate key creation and usage #428
Conversation
Codecov ReportAttention: Patch coverage is
❌ 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
🚀 New features to boost your workflow:
|
| @@ -1,3 +1,5 @@ | |||
| \! rm -f '/tmp/pg_tde_test_keyring.per' | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
| if (providerOid == GLOBAL_DATA_TDE_OID && !superuser()) | ||
| ereport(ERROR, | ||
| errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), | ||
| errmsg("must be superuser to access global key providers")); |
There was a problem hiding this comment.
I'm not 100% sure that access is the correct word there - create maybe?
There was a problem hiding this comment.
This is the same message as we use for the other places where we check this. Maybe "use" is a better word than "access"?
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
I think I would prefer to call it generate to differentiate it from commands which creates objects inside pg_tde.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I also prefer create, as we are creating keys on the providers, generate wouldn't be that explicit about this.
There was a problem hiding this comment.
I disagree but seems like I am outvoted. :) I think generate makes the usage more clear.
| } | ||
|
|
||
| if (strlen(key_name) >= sizeof(keyInfo->name)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Oh. You're completely right. I forgot that these names are stored in the tdemap header. Thank you!
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.
73c9ea5 to
1eff35c
Compare
|
New version pushed! |
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.