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.

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;

}

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