Re: Internal key management system

On Wed, 5 Feb 2020 at 22:28, Sehrope Sarkuni <sehrope@jackdb.com> wrote:
>
> On Sat, Feb 1, 2020 at 7:02 PM Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote:
> > On Sun, 2 Feb 2020 at 00:37, Sehrope Sarkuni <sehrope@jackdb.com> wrote:
> > >
> > > On Fri, Jan 31, 2020 at 1:21 AM Masahiko Sawada
> > > <masahiko.sawada@2ndquadrant.com> wrote:
> > > > On Thu, 30 Jan 2020 at 20:36, Sehrope Sarkuni <sehrope@jackdb.com> wrote:
> > > > > That
> > > > > would allow the internal usage to have a fixed output length of
> > > > > LEN(IV) + LEN(HMAC) + LEN(DATA) = 16 + 32 + 64 = 112 bytes.
> > > >
> > > > Probably you meant LEN(DATA) is 32? DATA will be an encryption key for
> > > > AES256 (master key) internally generated.
> > >
> > > No it should be 64-bytes. That way we can have separate 32-byte
> > > encryption key (for AES256) and 32-byte MAC key (for HMAC-SHA256).
> > >
> > > While it's common to reuse the same 32-byte key for both AES256 and an
> > > HMAC-SHA256 and there aren't any known issues with doing so, when
> > > designing something from scratch it's more secure to use entirely
> > > separate keys.
> >
> > The HMAC key you mentioned above is not the same as the HMAC key
> > derived from the user provided passphrase, right? That is, individual
> > key needs to have its IV and HMAC key. Given that the HMAC key used
> > for HMAC(IV || ENCRYPT(KEY, IV, DATA)) is the latter key (derived from
> > passphrase), what will be the former key used for?
>
> It's not derived from the passphrase, it's unlocked by the passphrase (along with the master encryption key). The
serverwill have 64-bytes of random data, saved encrypted in pg_control, which can be treated as two separate 32-byte
keys,let's call them master_encryption_key and master_mac_key. The 64-bytes is unlocked by decrypting it with the user
passphraseat startup (which itself would be split into a pair of encryption and MAC keys to do the unlocking). 
>
> The wrap and unwrap operations would use both keys:
>
> wrap(plain_text, encryption_key, mac_key) {
>     // Generate random IV:
>     iv = pg_strong_random(16);
>     // Encrypt:
>     cipher_text = encrypt_aes256_cbc(encryption_key, iv, plain_text);
>     // Compute MAC on all inputs:
>     mac = hmac_sha256(mac_key, encryption_key || iv || cipher_text);
>     // Concat user facing pieces together
>     wrapped = mac || iv || cipher_text;
>     return wrapped;
> }
>
> unwrap(wrapped, encryption_key, mac_key) {
>     // Split wrapped into its pieces:
>     actual_mac = wrapped.slice(0, 32);
>     iv = wrapped.slice(0 + 32, 16);
>     cipher_text = wrapped.slice(0 + 32 + 16);
>     // Compute MAC on all inputs:
>     expected_mac = hmac_sha256(mac_key, encryption_key || iv || cipher_text);
>     // Compare MAC vs value in wrapped:
>     if (expected_mac != actual_mac) { return Error("MAC does not match"); }
>     // MAC matches so decrypt:
>     plain_text = decrypt_aes256_cbc(encryption_key, iv, cipher_text);
>     return plain_text;
> }
>
> Every input to the encryption operation, including the encryption key, must be included into the HMAC calculation. If
youuse the same key for both encryption and MAC that's not required as it's already part of the MAC process as the key.
Usingseparate keys requires explicitly adding in the encryption key into the MAC input to ensure that it the correct
keyprior to decryption in the unwrap operation. Any additional parts of the wrapped output (ex: a "version" byte for
thealgos or padding choices) should also be included. 
>
> The wrap / unwrap above would be used with the encryption and mac keys derived from the user passphrase to unlock the
master_encryption_keyand master_mac_key from pg_control. Then those would be used by the higher level functions: 
>
> pg_kmgr_wrap(plain_text) {
>     return wrap(plain_text, master_encryption_key, master_mac_key);
> }
>
> pg_kmgr_unwrap(wrapped) {
>     return unwrap(wrapped, master_encryption_key, master_mac_key);
> }

Thank you for explaining the details. I had missed something.

Attached updated patch incorporated all comments I got so far. The changes are:

* Renamed data_encryption_cipher to key_management_cipher
* Renamed pg_kmgr_wrap and pg_kmgr_unwrap to pg_wrap_key and pg_unwrap_key
* Changed wrap and unwrap procedure based on the comments
* Removed the restriction of requiring the input key being a multiple
of 16 bytes.
* Created a context dedicated to wrap and unwrap data

Documentation and regression tests are still missing.

Regarding key rotation, currently we allow online key rotation by
doing pg_rotate_encryption_key after changing
cluster_passphrase_command and loading. But if the server crashed
during key rotation it might require the old passphrase in spite of
the passphrase command in postgresql.conf having been changed. We need
to deal with it but I'm not sure the best approach. Possibly having a
new frontend tool that changes the key offline would be a safe
approach.

Regards,


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