Re: Internal key management system

Hi

 

I took a step back
today and started to think about the purpose of internal KMS and what it is
supposed to do, and how it compares to external KMS. Both are intended to
manage the life cycle of encryptions keys including their generation,
protection, storage and rotation. External KMS, on the other hand, is a more
centralized but expensive way to manage encryption life cycles and many
deployment actually starts with internal KMS and later migrate to external one.

 

Anyhow, the design
and implementation of internal KMS should circle around these stages of key
lifecycle.

 

1. Key Generation -  Yes, internal KMS module generates keys
using pseudo random function, but only the keys for TDE and  SQL level keys. Users cannot request new
key generation

2. Key Protection - Yes,
internal KMS wrap all keys with KEK and HMAC hash derived from a cluster
passphrase

3. Key Storage - Yes, the
wrapped keys are stored in the cluster

4. Key Rotation - Yes, internal
KMS has a SQL method to swap out cluster passphrase, which rotates the KEK
and HMAC key

 

I am
saying this, because I want to make sure we can all agree on the scope of
internal KMS. Without clear scope, this KMS development will seem to go on
forever.

 

In this
patch, the internal KMS exposes pg_encrypt() and pg_decrypt() (was pg_wrap()
and pg_unwrap() before) to the user to turn a clear text password into some
sort of key material based on the SQL level key generated at initdb. This is
used so the user does not have to provide clear text password to
pgp_sym_encrypt() provided by pgcrypto extension. The intention is good, I
understand, but I don't think it is within the scope of KMS and it is
definitely not within the scope of TDE either.

 

Even
if the password can be passed into pgp_sym_encrypt() securely by using pg_decrypt() function, the pgp_sym_encrypt() still will have to take
this password and derive into an encryption key using algorithms that internal
KMS does not manage currently. This kind of defeats the purpose of internal
KMS. So simply using pg_encrypt() and pg_decrypt() is not really a solution to
pgcrypto's limitation. This should be in another topic/project that is aimed to
improve pgcrypto by integrating it with the internal KMS, similar to TDE where
it also has to integrate with the internal KMS later.

 

So
for internal KMS, the only cryptographic functions needed for now is
kmgr_wrap_key() and kmgr_unwrap_key() to encapsulate and restore the
encryptions keys to satisfy the "key protection" life cycle stage. I
don't think pg_encrypt() and pg_decrypt() should be part of internal KMS.

 

Anyways, I have also
reviewed the patch and have a few comments below:

 

(1)

The ciphering
algorithm in my opinion should contain the algorithm name, key length and block
cipher mode, which is similar to openssl's definition.

 

Instead of defining
a cipher as  PG_CIPHER_AES_CBC, and have
key length as separate parameter, I would define them as

#define
PG_CIPHER_AES128_CBC 0

#define
PG_CIPHER_AES256_CBC 1

#define
PG_CIPHER_AES128_CTR 2

#define
PG_CIPHER_AES256_CTR 3

 

I know
PG_CIPHER_AES128_CTR and PG_CIPHER_AES256_CTR are not being used now as these
are for the TDE in future, but might as well list them here because this KMS is
made to work specifically for TDE as I understand.

-----------------------------------------------------------------------------------------------------------

/*

 * Supported symmetric encryption algorithm.
These identifiers are passed

 * to pg_cipher_ctx_create() function, and then
actual encryption

 * implementations need to initialize their
context of the given encryption

 * algorithm.

 */

#define
PG_CIPHER_AES_CBC                        0

#define
PG_MAX_CIPHER_ID                        1

-----------------------------------------------------------------------------------------------------------

 

(2)

If the cipher
algorithms are defined like (1), then there is no need to pass key length as
argument to ossl_cipher_ctx_create() function because it already knows the key
length based on the cipher definition. Less argument the better.

 

-----------------------------------------------------------------------------------------------------------

PgCipherCtx *

pg_cipher_ctx_create(int
cipher, uint8 *key, int klen)

{

PgCipherCtx
*ctx = NULL;

 

if
(cipher >= PG_MAX_CIPHER_ID)

return
NULL;

 

#ifdef USE_OPENSSL

ctx
= (PgCipherCtx *) palloc0(sizeof(PgCipherCtx));

 

ctx->encctx
= ossl_cipher_ctx_create(cipher, key, klen, true);

ctx->decctx
= ossl_cipher_ctx_create(cipher, key, klen, false);

#endif

 

return
ctx;

}

-----------------------------------------------------------------------------------------------------------

Cary Huang

-------------

HighGo Software Inc. (Canada)

mailto:cary(dot)huang(at)highgo(dot)ca

http://www.highgo.ca

---- On Sun, 31 May 2020 23:58:31 -0700 Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote ----

On Sat, 30 May 2020 at 04:20, Robert Haas <mailto:robertmhaas(at)gmail(dot)com> wrote:
>
> On Fri, May 29, 2020 at 1:50 AM Masahiko Sawada
> <mailto:masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
> > However, this usage has a downside that user secret can be logged to
> > server logs when log_statement = 'all' or an error happens. To deal
> > with this issue I've created a PoC patch on top of the key manager
> > patch which adds a libpq function PQencrypt() to encrypt data and new
> > psql meta-command named \encrypt in order to encrypt data while
> > eliminating the possibility of the user data being logged.
> > PQencrypt() just calls pg_encrypt() via PQfn(). Using this command the
> > above example can become as follows:
>
> If PQfn() calls aren't currently logged, that's probably more of an
> oversight due to the feature being almost dead than something upon
> which we want to rely.

Agreed.

The patch includes pg_encrypt() and pg_decrypt() SQL functions
inspired by Always Encryption but these functions are interfaces of
the key manager to make it work independently from TDE and are
actually not necessary in terms of TDE. Perhaps it's better to
consider whether it's worth having them after introducing TDE.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services