Ensure decrypted internal keys never swapped #185
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dAdAbird
force-pushed
the
no_keys_swapped
branch
2 times, most recently
from
d115b2e to
023928c
Compare
jeltz
requested changes
Apr 3, 2025
There was a problem hiding this comment.
I am not the biggest fan of this approach of heavy interaction between the cache and storage. Especially if we want to change from linear cache to hash map for example. But it is not like I have a better suggestion which does not involve writing our own allocator.
Plus I think the keys in TDESMgrRelationData still can be swapped to disk.
| DECLARE idx integer; | ||
| begin | ||
| for idx in 0..700 loop | ||
| EXECUTE format('CREATE TABLE t%s (c1 int) USING tde_heap', idx); |
There was a problem hiding this comment.
Not that it matters in tests but in real production code I would have done.
EXECUTE format('CREATE TABLE %I (c1 int) USING tde_heap', 't' || idx);
There was a problem hiding this comment.
So should I change it? I took it from the cache_alloc regression test. If to change, that it make sense to fix the regression test as well
There was a problem hiding this comment.
Up to you. I just wanted to share how real production code looks like.
dAdAbird
force-pushed
the
no_keys_swapped
branch
from
023928c to
6072c6c
Compare
We have already had a locked memory for internal keys. But we read and created keys into usually palloced memory before copying them to the locked memory. This commit changes the approach. Now, the user must allocate (acquire) a locked memory first and use it as a buffer for decryption/creating the key. This locked memory is kinda generic, so the user requests an amount of bytes (rather than objects). Although currently, during the key search, we assume that everything the memory contains only `RelKeyCacheRec` objects. With two exceptions: 1) server creates its own locked page to store the WAL write key; 2) `pg_tde_perform_rotate_key` cheekily allocates a space there for the keys re-encryption but releases this memory when it's done. In future, if we switch to the full _map files encryption, we can gulp (mmap) the whole file into that memory. Fixes PG-1445
dAdAbird
force-pushed
the
no_keys_swapped
branch
from
6072c6c to
6a8bd08
Compare
Smgr has its own cache, TDESMgrRelation, which stores the state and other. We use it to store the Internal key. Although we have to store a pointer to the key in the secure memory rather than a copy of the key. But when secure memory grows, it moves the object, therefore, the smgr pointer becomes invalid. Moreover, pointers become invalid even w/o growth. The simplest scenario:
```
CREATE TEMP TABLE articles (
id int CONSTRAINT articles_pkey PRIMARY KEY,
keywords text,
title text UNIQUE NOT NULL,
body text UNIQUE,
created date
);
SELECT id, keywords, title, body, created FROM articles GROUP BY id;
ERROR: invalid page in block 0 of relation base/16384/t0_32940
```
This commit removes the internal key pointer from smgr and replaces it with a GetSMGRRelationKey call. By the time of this call, the key would be in a cache. Though this will have performance drawbacks and should be benchmarked and re-evaluated
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have already had a locked memory for internal keys. But we read and created keys into usually palloced memory before copying them to the locked memory.
This commit changes the approach. Now, the user must allocate (acquire) a locked memory first and use it as a buffer for decryption/creating the key. This locked memory is kinda generic, so the user requests an amount of bytes (rather than objects). Although currently, during the key search, we assume that everything the memory contains only
RelKeyCacheRecobjects. With two exceptions: 1) server creates its own locked page to store the WAL write key;2)
pg_tde_perform_rotate_keycheekily allocates a space there for the keys re-encryption but releases this memory when it's done.In future, if we switch to the full _map files encryption, we can gulp (mmap) the whole file into that memory.
Fixes PG-1445