On Wed, 3 Jun 2020 at 08:30, Cary Huang <cary.hu...@highgo.ca> wrote: > > 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. > >
Yes, the internal KMS is not an alternative to external KMS such as AWS KMS, SafeNet Key Secure, and Vault, but a PostgreSQL internal component that can work with these external solutions (via cluster_passphrase_command). It's the same position as our other management components such as bufmgr, smgr, and lmgr. I agree with this scope. It manages only encryption keys used by PostgreSQL. > > 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. > I agree that neither pg_encrypt() nor pg_decrypt() is within the scope of KMS and TDE. That's why I've split the patch, and that's why I renamed to pg_encrypt() and pg_decrypt() to clarify the purpose of these functions is not key management. Key wrapping and unwrapping is one of the usages of these functions. I think we can use the internal KMS for several purposes. It can manage encryption keys not only for cluster-wide TDE but also, for example, for column-level TDE and encryption SQL functions. pg_encrypt() and pg_decrypt() are one example of the usage of the internal KMS. Originally since we thought KMS and TDE are not introduced at the same release, the idea is come up with so that users can use KMS functionality with some interface. Therefore these SQL functions are not within the scope of KMS and it should be fine with introducing the internal KMS having 0 keys. > 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. Yeah, when using pgcrypto, user must manage their encryption keys. The internal KMS doesn't help that because it manages only keys internally used. What pg_encrypt() and pg_decrypt() can help is only to hide the password from server logs. > 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. > Agreed. > 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. > Agreed. > > 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 > Agreed. I was concerned that we will end up having many IDs in the future for example when porting pgcrypto functions into core but I'm okay with that change. > > > 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. Agreed. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
