Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

> Attached the draft version patch sets of per tablespace transparent
> data at rest encryption. The patch doesn't support full functionality,
> it includes:
>
> * Per tablespace encryption
> * Encryption and decryption buffer data when disk I/O.
> * 2 tier key hierarchy and key rotation
> * Temporary file encryption (based on the patch Antonin proposd)
> * System catalog encryption
> * Generic key management API and test module
> * Simple TAP test

I've checked your patch series to find out how to adjust [1] to make future
merge easier.

The biggest issue I see is that front-end applications won't be able to load
the KMGR plugin. We also used some sort of external library in the first
version of [1], but when I was trying to make the front-ends aware of
encryption, I found out that dfmgr.c cannot be linked to them w/o significant
rework. So I gave up and moved the encrypt_block() and decrypt_block()
functions to the core.

A few more notes regarding key management:

* InitializeKmgr()

** the function probably does not have to acquire KeyringControlLock, for
the same reasons that load_keyring_file() does not do (i.e. it's only called
by postmaster during startup)

** the lines

char *key = NULL;

as well as

/* Get the master key */
key = KmgrPluginGetKey(KmgrCtl->masterKeyId);

Assert(key != NULL);

should be enclosed in #ifdef USE_ASSERT_CHECKING - #endif, otherwise I
suppose (but haven't verified) compiler will produce warning that variable
is set but not used.

Actually ERROR might be more suitable for external (loadable) KMGR plugin,
but, as explained above, I'm not sure if such an approach is viable.

* KmgrPluginGetKey() only seems to deal with the master key, not with the
tablespace keys. So I suggest the name to contain the 'Master' word.

* KmgrPluginRemoveKey() seems to be unused.

* KeyringCreateKey() - I wondered why the key is returned encrypted. Actually
the only call of the function that I found is that in CreateTableSpace(),
and it does not use the return value at all. Shouldn't KeyringGetKey()
handle creation of the key if it does not exist yet?

* KeyringAddKey() seems to be unused.

* keyring size (kmgr.c):

/*
* Since we have encryption keys per tablspace, we expect this value is enough
* for most usecase.
*/
#define KMGR_KEYRING_SIZE 128

There's no guarantee that the number of tablespaces won't exceed any
(reasonably low) constant value. The KMGR module should be able to
allocate additional memory dynamically.

[1] https://commitfest.postgresql.org/23/2104/

--
Antonin Houska
Web: https://www.cybertec-postgresql.com