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

From Antonin Houska
Subject Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)
Date
Msg-id 10542.1565600196@linux-edt6
Whole thread Raw
In response to Re: [Proposal] Table-level Transparent Data Encryption (TDE) and KeyManagement Service (KMS)  (Masahiko Sawada <sawada.mshk@gmail.com>)
List pgsql-hackers
Masahiko Sawada <sawada.mshk@gmail.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



pgsql-hackers by date:

Previous
From: Kohei KaiGai
Date:
Subject: Asymmetric partition-wise JOIN
Next
From: Andrey Borodin
Date:
Subject: Do not check unlogged indexes on standby