PG-1923 WIP 256-bit encryption (take 2) #587
Conversation
It's a draft. Only Principal and WAL keys work so far
It does make sense to rewrite old files on server start (otherwise it'll slow down random read/write). But reading all _keys files and checking the magic number could slow the server start, especially if there are a lot of databases with encrypted tables. Moreover, we'd rewrite files once but would have to scan and read them on every start... That's why this commit introduces new suffixes to the filenames in the new format. That way, we would only scan the dir and read file names, instead of opening and reading each _keys file.
dAdAbird
force-pushed
the
32_byte_keys_take2
branch
from
c012cde to
0b8794c
Compare
| AesEcbEncrypt(EVP_CIPHER_CTX **ctxPtr, const unsigned char *key, int key_len, const unsigned char *in, int in_len, unsigned char *out) | ||
| { | ||
| int out_len; | ||
| const EVP_CIPHER *cipher = key_len == 32 ? cipher_ctr_ecb_256 : cipher_ctr_ecb; |
There was a problem hiding this comment.
As for me it needs better validation here, or may be even accept some enum value here instead of key_len as we always expect discrete values.
| NULL /* show_hook */ | ||
| ); | ||
|
|
||
| DefineCustomEnumVariable("pg_tde.cipher", /* name */ |
There was a problem hiding this comment.
I'm pretty sure that over time we will have request for multi-tenant cipher configuration. Not sure, but maybe it worth to think how to simplify our future. Maybe call it pg_tde.default_cipher?
There was a problem hiding this comment.
Maybe, I'm not sure. It might be confusing right now - if that's a default then what else could be set. Besides the real default is "aes128" - which is if the GUC isn't defined :)
But a good point, need to think about that
| } | ||
|
|
||
| void | ||
| pg_tde_update_wal_keys_file(void) |
There was a problem hiding this comment.
I think something like migrate fits better to this function name rather than update
|
|
||
| typedef struct WalKeyFileEntry | ||
| { | ||
| uint32 key_len; |
There was a problem hiding this comment.
I wonder if it's worth to store cipher type here as well for future needs?
| } | ||
| pg_tde_save_server_key(principalKey, false); | ||
| TDEXLogSmgrInitWrite(true); | ||
| TDEXLogSmgrInitWrite(true, 16); |
There was a problem hiding this comment.
It's pinned here just for time being?
There was a problem hiding this comment.
yes, I haven't look at tools yet, so it's a stub just to compile. The basebackup and other (?) tools that creates key, probably would have to have it's own flag for the key size/cipher
This is a draft and needs further improvements and refactoring. I just want feedback on the general direction.
Cheers