Key Map Implementation - Replacing TDE Forks #113
Conversation
There was a problem hiding this comment.
I added some comments.
My main issue is: the master key name handling seems to be completely incorrect.
Generally, we have two names:
- A generic name,
master-key - A specific version,
master-key-X, e.g.master-key-13
An entire TDE file always has to be encrypted entirely with the same key version, otherwise we won't be able to decrypt it correctly.
This means that:
- If the TDE file doesn't exists, we have to use the generic name (
master-key), lookup the latest master key versionmaster-key-XusinggetLatestKey/generateKey, and then save the name of the specific key it gives us (with the version number) to the file. - When adding a key to an existing file, we have to lookup the specific master key name stored in the file (
master-key-X), usekeyringGetKeyto retrieve it, and encrypt the new internal key using that specific master key - When reading an existing file, similarly we have to use
keyringGetKeyto retrieve the specific version specified in the file - When rotating the master key, we read the file using the existing earlier key as above, but then generate a new version using
generateKey, and then rewrite the entire file using the new version.
src/encryption/enc_aes.c
Outdated
| * short lifespan until it is written to disk. | ||
| */ | ||
| void | ||
| AesEncryptKey(const keyInfo *master_key_info, RelKeysData *rel_key_data, RelKeysData **p_enc_rel_key_data, size_t *enc_key_bytes) |
There was a problem hiding this comment.
This function shouldn't be in this file: this is where we have code that only deals with encryption and openssl. The code that connects this to postgres codo is in enc_tuple.c
There was a problem hiding this comment.
It's essentially our interface to the SSL functions. Hence, I put these two functions here.
There was a problem hiding this comment.
No, this function already uses the functions defined in enc_aes.h. The idea is that enc_aes uses OpenSSL directly, and doesn't depend on postgres/pg_tde headers - see the includes above, it had no entries related to these, before this PR.
Anything that depends on postgres/pg_tde headers is implemented in enc_tuple.h.
the old "tde" fork architecture. This new architecture implements a two file pair with: (1) Map File (2) Key Data File Both files contain a header that contains the name of the master key that was to encrypt the data keys and a file version. The file version is set to PG_TDE_FILEMAGIC at the moment and it can be used to differiate between different file format versions in case we change the structure later on. The map file is a list of relNumber, flags and key index. - relNumber is the Oid of the associated relation. - Flags define if the map entry is free or in use. - Key index points to the starting position of the key in the key data file. The flags play a pivotal role in avoiding the file to grow infinitely. When a relation is either deleted or a transaction is aborted, the entry map entry is marked as MAP_ENTRY_FREE. Any next transaction requiring to store its relation key will pick the first entry with flag set to MAP_ENTRY_FREE. The key data file is simply a list of keys. No flags are needed as the validity is identified by the map file. Writing to the file is performed using FileWrite function. This avoids any locking in the key data file. Pending: - Implementation of key rotation - Locking of file during key rotation or map entry - Review of fflush calls - Review of the WAL
src/encryption/enc_aes.c
Outdated
| * short lifespan until it is written to disk. | ||
| */ | ||
| void | ||
| AesEncryptKey(const keyInfo *master_key_info, RelKeysData *rel_key_data, RelKeysData **p_enc_rel_key_data, size_t *enc_key_bytes) |
There was a problem hiding this comment.
No, this function already uses the functions defined in enc_aes.h. The idea is that enc_aes uses OpenSSL directly, and doesn't depend on postgres/pg_tde headers - see the includes above, it had no entries related to these, before this PR.
Anything that depends on postgres/pg_tde headers is implemented in enc_tuple.h.
Moving the key encryption/decryption functions to the enc_tuple file and renaming the files according to the functionality.
and when redo-ing the log. Also, updated the handling of master key to accomodate versioning.
src/keyring/keyring_api.c
Outdated
| * doGenerateKey should be false and doRaiseError should be set to indicate | ||
| * that master key is expected but could not be accessed. | ||
| */ | ||
| const keyInfo* getMasterKey(const char* internalName, int doGenerateKey, int doRaiseError) |
There was a problem hiding this comment.
I think doGenerateKey, doRaiseError should be bool not int - in a sake of the interface clarity.
src/access/pg_tde_tdemap.c
Outdated
| rel_key_data = tde_create_rel_key(rlocator, key, master_key_info); | ||
| enc_rel_key_data = tde_encrypt_rel_key(master_key_info, rel_key_data); | ||
|
|
||
| /* Get the file paths */ |
There was a problem hiding this comment.
| /* Get the file paths */ | |
| /* Set the file paths */ |
| /* | ||
| * Add the encyrpted key to the key map data file structure. | ||
| */ | ||
| pg_tde_write_key_map_entry(newrlocator, enc_rel_key_data, master_key_info); |
There was a problem hiding this comment.
Better to add a new internal key to the memory cache as well, as there is a big chances that the key will be used (on INSERT) right after the relation creation. This is how it was before and I think it makes sense from the performance perspective
This PR has two commits with one commit including some refactoring, and the other commit includes implementation of the key map architecture. Unfortunately, since most of this code was written back in December, it is very hard to recreate a separate PR for the refactoring part. Therefore, for ease, I've created a separate commit for it.