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

On Sun, 26 Jan 2020 at 01:35, Sehrope Sarkuni <sehrope@jackdb.com> wrote:
>
> Hi,
>
> I took a look at this patch. With some additions I think the feature
> itself is useful but the patch needs more work. It also doesn't have
> any of its own automated tests yet so the testing below was done
> manually.
>
> The attached file, kms_v2.patch, is a rebased version of the
> kms_v1.patch that fixes some bit rot. It sorts some of the Makefile
> additions but otherwise is the original patch. This version applies
> cleanly on master and passes make check.


Thank you for the comments and updating the patch.

>
> I don't have a Windows machine to test it, but I think the Windows
> build files for these changes are missing. The updated
> src/common/Makefile has a comment to coordinate updates to
> Mkvcbuild.pm but I don't see kmgr_utils.c or cipher_openssl.c
> referenced anywhere in there.

Will support Windows building.

>
> The patch adds "pg_kmgr" to the list of files to skip in
> pg_checksums.c but there's no additional "pg_kmgr" file written to the
> data directory. Perhaps that's from a prior version that saved data to
> its own file?

Right, it's unnecessary. Will remove it.

>
> The constant AES128_KEY_LEN is defined in cipher.c but it's not used
> anywhere. RE: AES-128, not sure the value of even supporting it for
> this feature (v.s. just supporting AES-256). Unlike something like
> table data encryption, I'd expect a KMS to be used much less
> frequently so any performance boost of AES-128 vs AES-256 would be
> meaningless.

Ok. I agree to support only AES256 for this feature.

> The functions pg_cipher_encrypt(...), pg_cipher_decrypt(...), and
> pg_compute_HMAC(...) return true if OpenSSL is not configured. Should
> that be false? The ctx init functions all return false when not
> configured. I don't think that code path would ever be reached as you
> would not have a valid context but seems more consistent to have them
> all return false.

Agreed.

>
> There's a comment referring to "Encryption keys (TDEK and WDEK)
> length" but this feature is only for a KMS so that should be renamed.
>

Will remove it.

> The passphrase is hashed to split it into two 32-byte keys but the min
> length is only 8-bytes:
>
>     #define KMGR_MIN_PASSPHRASE_LEN 8
>
> ... that should be at least 64-bytes to reflect how it's being used
> downstream. Depending on the format of the passphrase commands output
> it should be even longer (ex: binary data in hex should really be
> double that). The overall min should be 64-byte but maybe add a note
> to the docs to explain how the output will be used and the expected
> amount of entropy.

Agreed.

>
> In pg_kmgr_wrap(...) it checks that the input is a multiple of 8 bytes:
>
>     if (datalen % 8 != 0)
>         ereport(ERROR,
>             (errmsg("input data must be multiple of 8 bytes")));
>
> ...but after testing it, the OpenSSL key wrap functions it invokes
> require a multiple of 16-bytes (block size of AES). Otherwise you get
> a generic error:
>
> # SELECT pg_kmgr_wrap('abcd1234'::bytea);
> ERROR:  could not wrap the given secret

Thank you for testing it. I will follow your suggestion described below.

>
> In ossl_compute_HMAC(...) it refers to AES256_KEY_LEN. Should be
> SHA256_HMAC_KEY_LEN (they're both 32-bytes but naming is wrong)
>
>     return HMAC(EVP_sha256(), key, AES256_KEY_LEN, data,
>         (uint32) data_size, result, (uint32 *) result_size);

Will fix.

>
> In pg_rotate_encryption_key(...) the error message for short
> passphrases should be "at least %d bytes":
>
>     if (passlen < KMGR_MIN_PASSPHRASE_LEN)
>         ereport(ERROR,
>             (errmsg("passphrase must be more than %d bytes",
>             KMGR_MIN_PASSPHRASE_LEN)));

Agreed. Will fix.

>
> Rotating the passphrase via "SELECT pg_rotate_encryption_key()" and
> restarting the server worked (good). Having the server attempt to
> start with invalid output from the command gives an error "FATAL:
> cluster passphrase does not match expected passphrase" (good).
>
> Round tripping via wrap/unwrap works (good!):
>
> # SELECT convert_from(pg_kmgr_unwrap(pg_kmgr_wrap('abcd1234abcd1234'::bytea)),
> 'utf8');
>    convert_from
> ------------------
>  abcd1234abcd1234
> (1 row)
>
> Trying to unwrap gibberish fails (also good!):
>
> # SELECT pg_kmgr_unwrap('\x123456789012345678901234567890123456789012345678');
> ERROR:  could not unwrap the given secret
>

Thank you for testing!

> The pg_kmgr_wrap/unwrap functions use EVP_aes_256_wrap()[1] which
> implements RFC 5649[2] with the default IVs so they always return the
> same value for the same input:
>
> # SELECT x, pg_kmgr_wrap('abcd1234abcd1234abcd1234') FROM
> generate_series(1,5) x;
>  x |                            pg_kmgr_wrap
> ---+--------------------------------------------------------------------
>  1 | \x51041d1fe52916fd15f456c2b67108473d9bf536795e2b6d4db81c065c8cd688
>  2 | \x51041d1fe52916fd15f456c2b67108473d9bf536795e2b6d4db81c065c8cd688
>  3 | \x51041d1fe52916fd15f456c2b67108473d9bf536795e2b6d4db81c065c8cd688
>  4 | \x51041d1fe52916fd15f456c2b67108473d9bf536795e2b6d4db81c065c8cd688
>  5 | \x51041d1fe52916fd15f456c2b67108473d9bf536795e2b6d4db81c065c8cd688
> (5 rows)
>
> The IVs should be randomized so that repeated wrap operations give
> distinct results. To do that, the output format needs to include the
> randomized IV. It need not be secret but it needs to be included in
> the wrapped output. Related, IIUC, the wrapping mechanism of RFC5649
> does provide some integrity checking but it's only 64-bits (v.s. say
> 256-bits for a full HMAC-SHA-256).
>
> Rather than use EVP_aes_256_wrap() with its defaults, we can generate
> a random IV and have the output be "IV || ENCRYPT(KEY, IV, DATA) ||
> HMAC(IV || ENCRYPT(KEY, IV, DATA))". For a fixed length internal input
> (ex: the KEK encrypted key stored in pg_control) there's no need for
> padding as we're dealing with multiples of 16-bytes (ex: KEK encrypted
> enc-key / mac-key would be 64-bytes).
>
> It'd also be useful if the user level wrap/unwrap API allowed for
> arbitrary sized inputs (not just multiples of 16-byte). Having the
> output be in a standard format (i.e. matching OpenSSL's
> EVP_aes_256_wrap API) is nice, but as it's meant to be an opaque
> interface I think it's fine if the output is not usable outside the
> database. I don't see anyone using the wrapped data directly as it's
> random bytes without the key. The primary contract for the interface:
> "data == unwrap(wrap(data))". This would require enabling padding
> which would round up the size of the output to the next 16-bytes.
> Adding a prefix byte for a "version" would be nice too as it could be
> used to infer the specific cipher/mac combo (Ex: v1 would be
> AES256/HMAC-SHA256). I don't think the added size is an issue as
> again, the output is opaque. Similar things can also be accomplished
> by combining the 16-byte only version with pgcrypto but like this it'd
> be usable out of the box without additional extensions.
>

Thank you for the suggestion. I like your suggestion. We can do an
integrity check of the user input wrapped key by using HMAC when
unwrapping. Regarding the output format you meant to use aes-256
rather than aes-256-key-wrap? I think that DATA in the output is the
user input key so it still must be multiples of 16-bytes if we use
aes-256-key-wrap.

BTW regarding the implementation of cipher function using opensssl in
the src/common I'm concerned whether we should integrate it with the
openssl.c in pgcrypto. Since pgcrypto with openssl currently supports
aes, des and bf etc the cipher function code in this patch also has
similar functionality. Similarly when we introduced SCRAM we moved
sha2 functions from pgcrypto to src/common. I thought we move all
cipher functions in pgcrypto to src/common but it might be overkill
because the internal KMS will use only aes with only 256 key length as
of now.

Regards,

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