Proof-of-concept: Protect decrypted relation keys #446
Conversation
Using openssl's secure allocators for these ensures they are never swapped to disk or present in a core dump. Using ResourceOwner for them ensures we don't leak memory when transactions are aborted. This commit is mostly a poc for only keeping encrypted keys in the smgr cache, as that memory is not protected.
Codecov ReportAttention: Patch coverage is
❌ Your project status has failed because the head coverage (84.55%) 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 #446 +/- ##
=====================================================
- Coverage 84.66% 84.55% -0.12%
=====================================================
Files 21 22 +1
Lines 2589 2674 +85
Branches 402 408 +6
=====================================================
+ Hits 2192 2261 +69
- Misses 316 327 +11
- Partials 81 86 +5
🚀 New features to boost your workflow:
|
|
(also ignore the infinite recursion I realized in the shower that I left in there :D ) |
|
Did some preliminary performance testing and I see about a 0.4% slowdown compared to our current version as measured in how many queries sysbench can perform during 20 seconds on my laptop using the oltp_read_write test. To me this could just be noise in the testing, but indicates that this is not insanely slow at least. |
| Assert(principal_key); | ||
|
|
||
| *rel_key_data = map_entry->enc_key; | ||
|
|
||
| if (!AesGcmDecrypt(principal_key->keyData, | ||
| map_entry->entry_iv, MAP_ENTRY_IV_SIZE, | ||
| (unsigned char *) map_entry, offsetof(TDEMapEntry, enc_key), | ||
| (uint8 *) &rlocator, sizeof(RelFileLocator), |
There was a problem hiding this comment.
The change to uint8 seems very arbitrary. Doesn't PostgreSQL use unsigned char everywhere for this?
There was a problem hiding this comment.
That said I prefer uint8 over unsigned char, but that is not what I think we or PostgreSQL are using. So now you introduced a new way. Also it does seem you have not changed AesGcmDecrypt.
There was a problem hiding this comment.
The changes in pg_tde_tdemap.c was just to support the way I calculated aad in the other file, so that rotations wouldn't break everything. They are very PoC.
There was a problem hiding this comment.
The postgres code uses uint8 about as commonly as unsigned char. I picked the one I like :D. The type wasn't the point there at all but instead to use the full rlocator as aad.
For WAL keys we currently use a fake rlocator that's the same for all of them so they need something else (probably start_lsn).
EDIT: excluding contrib/ unsigned char seems to be used about 66% of the time.
What we win? Decrypted key material is protected. That's the whole win. How we free them (ResourceOwners vs smgr_close) is kind of irrelevant for that. EDIT: A secondary win is moving away from the horrible InternalKey structure. But that's secondary :D |
I just finished this up yesterday and have not yet run any performance tests at all. Maybe this is way too slow and not a viable solution at all. I will run performance tests on monday. But any comments on the approach is highly appreciated either way 🙂
The main idea is that the relation cache only contains still encrypted keys, and that any decrypted keys are put in memory that's allocated using openssl's secure allocators to make sure they're never swapped or present in a core dump et.c.
It uses ResourceOwner to ensure we're not leaking memory even though we use a custom allocator.
WAL keys were ignored in this PoC, but they of course need the same treatment if we move forward with this.