[PG-1637] Remove any existing keys for plaintext tables on create #388
Conversation
This fixes a bug where an encrypted relation could have encryption_status set to plaintext in a recovery scenario where the key has not yet been created.
There are crash scenarios where keys are left behind in the key file even though the OID for the table goes unused. This meant that we could have keys laying around for newly created plaintext relations after OID wraparound. Simply removing any existing keys when plaintext relation forks are created seems appriopriate. Also move creation of pg_tde data dir to library init. This directory is used by the SMgr which is loaded regardless of whether any database are yet to create extension or not.
Codecov ReportAttention: Patch coverage is
❌ Your project status has failed because the head coverage (85.44%) 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 #388 +/- ##
=====================================================
+ Coverage 85.40% 85.44% +0.03%
=====================================================
Files 22 22
Lines 2603 2610 +7
Branches 394 394
=====================================================
+ Hits 2223 2230 +7
Misses 304 304
Partials 76 76
🚀 New features to boost your workflow:
|
| @@ -93,6 +93,7 @@ _PG_init(void) | |||
|
|
|||
| check_percona_api_version(); | |||
|
|
|||
| pg_tde_init_data_dir(); | |||
There was a problem hiding this comment.
what was the reason to move it here? I'm not objecting it, just curious
There was a problem hiding this comment.
As I mentioned in the commit message, this directory is used by the SMgr, and the SMgr is loaded and used regardless of whether any database have run CREATE EXTENSION. So the directory should be created regardless.
Practically it was because the call to pg_free_key_map_entry() crashed when the directory didn't exist :).
There was a problem hiding this comment.
Yes, sorry, I missed it. Thanks
|
|
||
| if (key) | ||
| if (unlikely(key)) |
There was a problem hiding this comment.
When can this even happen? Is it a thing which only happens if there are leftover keys or we are doing a recovery/replay on the replica? This branch should be explained.
I have to think about this. I was worried about the recovery case, but there will be a WAL record for creating the key after this function in called, so it shouldn't be an issue. That would also remove the need for the branch you asked about I'm pretty sure. I need to verify though. I also need to understand exactly how and when the forks are created et.c. because my understanding of this is a little hazy. |
Some crash scenarios can leave keys behind in the relation key file for OIDs that are unused. If these OIDs are later used when creating a non-encrypted relation the key has to be removed or we will do the wrong thing since the existence of a key makes us assume the relation is encrypted.
This means that if an encrypted relation is created in the same scenario it will use the key that was previously generated but unused. This is probably ok.