Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Sehrope Sarkuni]

On Fri, Jan 31, 2020 at 1:21 AM Masahiko Sawada
 wrote:
> On Thu, 30 Jan 2020 at 20:36, Sehrope Sarkuni  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.

> > For the user facing piece, padding would enabled to support arbitrary
> > input data lengths. That would make the output length grow by up to
> > 16-bytes (rounding the data length up to the AES block size) plus one
> > more byte if a version field is added.
>
> I think the length of padding also needs to be added to the output.
> Anyway, in the first version the same methods of wrapping/unwrapping
> key are used for both internal use and user facing function. And user
> input key needs to be a multiple of 16 bytes value.

A separate length field does not need to be added as the
padding-enabled output will already include it at the end[1]. This
would be handled automatically by the OpenSSL encryption / decryption
operations (if it's enabled):

[1]: https://en.wikipedia.org/wiki/Padding_(cryptography)#PKCS#5_and_PKCS#7

> BTW I think this topic is better to be discussed on a separate thread
> as the scope no longer includes TDE. I'll start a new thread for
> introducing internal KMS.

Good idea.

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Masahiko Sawada]

On Thu, 30 Jan 2020 at 20:36, Sehrope Sarkuni  wrote:
>
> On Wed, Jan 29, 2020 at 3:43 AM Masahiko Sawada
>  wrote:
> > 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.
>
> Yes I'm suggesting not using the key wrap functions and instead using
> the regular EVP_aes_256_cbc with a random IV per invocation. For
> internal usage (e.g. the encrypted key) it does not need padding as we
> know the input value would always be a multiple of 16-bytes.

That makes sense.

> 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.

>
> For the user facing piece, padding would enabled to support arbitrary
> input data lengths. That would make the output length grow by up to
> 16-bytes (rounding the data length up to the AES block size) plus one
> more byte if a version field is added.

I think the length of padding also needs to be added to the output.
Anyway, in the first version the same methods of wrapping/unwrapping
key are used for both internal use and user facing function. And user
input key needs to be a multiple of 16 bytes value.

>
> > 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.
>
> I'd keep the patch smaller and the functions internal to the KMS for
> now. Maybe address it again after the patch is complete as it'll be
> easier to see overlaps that could be combined.

Agreed.

BTW I think this topic is better to be discussed on a separate thread
as the scope no longer includes TDE. I'll start a new thread for
introducing internal KMS.

Regards,

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

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Sehrope Sarkuni]

On Wed, Jan 29, 2020 at 3:43 AM Masahiko Sawada
 wrote:
> 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.

Yes I'm suggesting not using the key wrap functions and instead using
the regular EVP_aes_256_cbc with a random IV per invocation. For
internal usage (e.g. the encrypted key) it does not need padding as we
know the input value would always be a multiple of 16-bytes. That
would allow the internal usage to have a fixed output length of
LEN(IV) + LEN(HMAC) + LEN(DATA) = 16 + 32 + 64 = 112 bytes.

For the user facing piece, padding would enabled to support arbitrary
input data lengths. That would make the output length grow by up to
16-bytes (rounding the data length up to the AES block size) plus one
more byte if a version field is added.

> 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.

I'd keep the patch smaller and the functions internal to the KMS for
now. Maybe address it again after the patch is complete as it'll be
easier to see overlaps that could be combined.

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Masahiko Sawada]

On Sun, 26 Jan 2020 at 01:35, Sehrope Sarkuni  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 | 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Sehrope Sarkuni]

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.

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.

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?

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.

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.

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.

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.

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

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);

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)));

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

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 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[cary huang]

On Mon, Jan 6, 2020 at 4:43 AM Masahiko Sawada <
masahiko.saw...@2ndquadrant.com> wrote:

> On Sat, 4 Jan 2020 at 15:11, cary huang  wrote:
> >>
> >> Hello Sawada and all
> >>
> >> I would like to elaborate more on Sehrope and Sawada's discussion on
> passing NULL IV in "pg_cipher_encrypt/decrypt" functions during
> kmgr_wrap_key and kmgr_unwrap_key routines in kmgr_utils.c. Openssl
> implements key wrap according to RFC3394 as Sawada mentioned and passing
> NULL will make openssl to use default IV, which equals to A6A6A6A6A6A6A6A6.
> I have confirmed this on my side; A key wrapped with "NULL" IV can be
> unwrapped successfully with IV=A6A6A6A6A6A6A6A6, and unwrap will fail if IV
> is set to anything else other than NULL or A6A6A6A6A6A6A6A6.
> >>
>
> >Sehrope also suggested me not to use the fixed IV in order to avoid
> >getting the same result from the same value. I'm researching it now.
> >Also, currently it's using key wrap algorithm[1] but it accepts only
> >multiple of 8 bytes as input. Since it's not good for some cases it's
> >better to use key wrap with padding algorithm[2] instead, which seems
> >available in OpenSSL 1.1.0 or later.
>

Since the current kmgr only supports AES128 and AES256, the master key
generated by kmgr during bootstrap will always be multiple of 8. I believe
AES keys in general must always be in multiple of 8. I have not done enough
research as to which encryption algorithm will involve keys not in multiple
of 8 so I think with the current key_wrap_algorithm is fine. With the key
wrap algorithm defined in RFC3394. The IV is used only as a "initial value"
and it has to be static; either we randomize one or we use the default
A6A6A6... by passing NULL, It is different from the CTR block cipher mode
which has been selected to encrypt WAL and buffer. In CTR mode, each block
requires a different and unique IV as input in order to be secured; and we
have agreed to use segment IDs as IVs. For this reason, I think the current
key wrap implementation is fine. The least we can do is to generate a IV
during bootstrap and store it in control file, and this generated IV will
be used for all key wrapping / unwrapping purposes instead of using the
default one.

Best regards
Cary Huang
HighGo Software Canada

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Masahiko Sawada]

On Sat, 4 Jan 2020 at 15:11, cary huang  wrote:
>
> Hello Sawada and all
>
> I would like to elaborate more on Sehrope and Sawada's discussion on passing 
> NULL IV in "pg_cipher_encrypt/decrypt" functions during kmgr_wrap_key and 
> kmgr_unwrap_key routines in kmgr_utils.c. Openssl implements key wrap 
> according to RFC3394 as Sawada mentioned and passing NULL will make openssl 
> to use default IV, which equals to A6A6A6A6A6A6A6A6. I have confirmed this on 
> my side; A key wrapped with "NULL" IV can be unwrapped successfully with 
> IV=A6A6A6A6A6A6A6A6, and unwrap will fail if IV is set to anything else other 
> than NULL or A6A6A6A6A6A6A6A6.
>

Sehrope also suggested me not to use the fixed IV in order to avoid
getting the same result from the same value. I'm researching it now.
Also, currently it's using key wrap algorithm[1] but it accepts only
multiple of 8 bytes as input. Since it's not good for some cases it's
better to use key wrap with padding algorithm[2] instead, which seems
available in OpenSSL 1.1.0 or later.

> I would like to provide some comments on the encryption and decryption 
> routines provided by cipher_openssl.c in which cipher.c and kmgr_utils.c are 
> using. I see that "ossl_cipher_encrypt" calls "EVP_EncryptInit_ex" and 
> "EVP_EncryptUpdate" only to complete the encryption. Same thing applies to 
> decryption routines. According to my past experience with openssl and the 
> usages online, it is highly recommended to use "init-update-final" cycle to 
> complete the encryption and I see that the "final" part (EVP_EncryptFinal) is 
> missing. This call will properly handle the last block of data especially 
> when padding is taken into account. The functions still works now because the 
> input is encryption key and its size is multiple of each cipher block and no 
> padding is used. I think it will be safer to use the proper 
> "init-update-final" cycle for encryption/decryption

Agreed.

>
> According to openssl EVP documentation, "EVP_EncryptUpdate" can be called 
> multiple times at different offset to the input data to be encrypted. I see 
> that "pg_cipher_encrypt" only calls "EVP_EncryptUpdate" once, which makes me 
> assume that the application should invoke "pg_cipher_encrypt" multiple times 
> until the entire data block is encrypted? I am asking because if we were to 
> use "EVP_EncryptFinal" to complete the encryption cycle, then it is better to 
> let "pg_cipher_encrypt" to figure out how many times "EVP_EncryptUpdate" 
> should be called and finalize it with "EVP_EncryptFinal" at last block.

IIUC EVP_EncryptUpdate can encrypt the entire data block.
EVP_EncryptFinal_ex encrypts any data that remains in a partial block.

>
> Lastly, I think we are missing a cleanup routine that calls 
> "EVP_CIPHER_CTX_free()" to free up the EVP_CIPHER_CTX when encryption is done.

Right.

While reading pgcrypto code I thought that it's better to change the
cryptographic code (cipher.c) so that pgcrypto can use them instead of
having duplicated code. I'm trying to change it so.

[1] https://tools.ietf.org/html/rfc3394
[2] https://tools.ietf.org/html/rfc5649

Regards,

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

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[cary huang]

Hello Sawada and all

I would like to elaborate more on Sehrope and Sawada's discussion on passing 
NULL IV in "pg_cipher_encrypt/decrypt" functions during kmgr_wrap_key and 
kmgr_unwrap_key routines in kmgr_utils.c. Openssl implements key wrap according 
to RFC3394 as Sawada mentioned and passing NULL will make openssl to use 
default IV, which equals to A6A6A6A6A6A6A6A6. I have confirmed this on my side; 
A key wrapped with "NULL" IV can be unwrapped successfully with 
IV=A6A6A6A6A6A6A6A6, and unwrap will fail if IV is set to anything else other 
than NULL or A6A6A6A6A6A6A6A6.

I would like to provide some comments on the encryption and decryption routines 
provided by cipher_openssl.c in which cipher.c and kmgr_utils.c are using. I 
see that "ossl_cipher_encrypt" calls "EVP_EncryptInit_ex" and 
"EVP_EncryptUpdate" only to complete the encryption. Same thing applies to 
decryption routines. According to my past experience with openssl and the 
usages online, it is highly recommended to use "init-update-final" cycle to 
complete the encryption and I see that the "final" part (EVP_EncryptFinal) is 
missing. This call will properly handle the last block of data especially when 
padding is taken into account. The functions still works now because the input 
is encryption key and its size is multiple of each cipher block and no padding 
is used. I think it will be safer to use the proper "init-update-final" cycle 
for encryption/decryption

According to openssl EVP documentation, "EVP_EncryptUpdate" can be called 
multiple times at different offset to the input data to be encrypted. I see 
that "pg_cipher_encrypt" only calls "EVP_EncryptUpdate" once, which makes me 
assume that the application should invoke "pg_cipher_encrypt" multiple times 
until the entire data block is encrypted? I am asking because if we were to use 
"EVP_EncryptFinal" to complete the encryption cycle, then it is better to let 
"pg_cipher_encrypt" to figure out how many times "EVP_EncryptUpdate" should be 
called and finalize it with "EVP_EncryptFinal" at last block.

Lastly, I think we are missing a cleanup routine that calls 
"EVP_CIPHER_CTX_free()" to free up the EVP_CIPHER_CTX when encryption is done. 

Thank you

Cary Huang
HighGo Software Canada

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Ibrar Ahmed]

On Tue, Dec 31, 2019 at 1:05 PM Masahiko Sawada 
wrote:

> On Sun, Dec 1, 2019 at 12:03 PM Michael Paquier 
> wrote:
> >
> > On Fri, Nov 01, 2019 at 09:38:37AM +0900, Moon, Insung wrote:
> > > Of course, I may not have written the excellent quality code
> > > correctly, so I will make an interim report if possible.
> >
> > The last patch has rotten, and does not apply anymore.  A rebase would
> > be nice, so I am switching the patch as waiting on author, and moved
> > it to next CF.
> >
>
> We have discussed in off-list and weekly voice meeting for several
> months that the purpose of this feature and the target for PG13 and we
> concluded to step back and to focus on only internal key management
> system for PG13. Transparent data encryption support is now the target
> for PG14 or later. Key management system is an important
> infrastructure for TDE but it can work independent of TDE. The plan
> for PG13 we discussed is to introduce the internal key management
> system that has one encryption key for whole database cluster and make
> it have some interface to get encryption keys that are managed inside
> PostgreSQL database in order to integrate it with other components
> such as pgcrypto.
>
> Idea is to get something encrypted and decrypted without ever knowing
> the actual key that was used to encrypt it. The attached patch has two
> APIs to wrap and unwrap the secret by the encryption key stored inside
> database cluster. user generate a secret key locally and send it to
> PostgreSQL server to wrap it using by pg_kmgr_wrap() and save it
> somewhere. Then user can use the saved and wrapped secret key to
> encrypt and decrypt user data by something like:
>
> INSERT INTO tbl VALUES (pg_encrypt('user data', pg_kmgr_unwrap('x'));
>
> Where 'x' is the result of pg_kmgr_wrap function.
>
> I've attached the KMS patch. It requires openssl library. What the
> patch does is simple and is not changed much from the previous version
> patch that includes KMS and TDE; we generate one encryption key called
> master key for whole database cluster at initdb time, which is stored
> in pg_control and wrapped by key encryption key(KEK) derived from
> user-provided passphrase. When postmaster starts up it verifies the
> correctness of passphrase provided by user using hmac key which is
> also derived from user-provided passphrase. The server won't start if
> the passphrase is incorrect. Once the passphrase is verified the
> master key is loaded to the shared buffer and is active.
>
> I added two options to initdb: --cluster-passphrase-command and -e
> that takes a passphrase command and a cipher algorithm (currently only
> aes-128 and aes-256) respectively. The internal KMS is enabled by
> executing initdb with those options as follows:
>
> $ initdb -D data --cluster-passphrase-command="echo 'password'" -e aes-256
>
> I believe the internal KMS would be useful several use cases but I'd
> like to have discussion around the integration with pgcrypto because
> pgcrypto would be the first user of the KMS and pgcrypto can be more
> powerful with the KMS. I'll register this KMS patch to the next Commit
> Fest.
>
> It is already there "KMS - Internal key management system" (
https://commitfest.postgresql.org/26/2196/).

I really appreciate peoples (CC-ing) who participated in off-list
> discussion/meeting for many inputs, suggestions and reviewing codes.
>
> Regards,
>
>
> --
> Masahiko Sawada  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>


-- 
Ibrar Ahmed

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Masahiko Sawada]

On Sun, Dec 1, 2019 at 12:03 PM Michael Paquier  wrote:
>
> On Fri, Nov 01, 2019 at 09:38:37AM +0900, Moon, Insung wrote:
> > Of course, I may not have written the excellent quality code
> > correctly, so I will make an interim report if possible.
>
> The last patch has rotten, and does not apply anymore.  A rebase would
> be nice, so I am switching the patch as waiting on author, and moved
> it to next CF.
>

We have discussed in off-list and weekly voice meeting for several
months that the purpose of this feature and the target for PG13 and we
concluded to step back and to focus on only internal key management
system for PG13. Transparent data encryption support is now the target
for PG14 or later. Key management system is an important
infrastructure for TDE but it can work independent of TDE. The plan
for PG13 we discussed is to introduce the internal key management
system that has one encryption key for whole database cluster and make
it have some interface to get encryption keys that are managed inside
PostgreSQL database in order to integrate it with other components
such as pgcrypto.

Idea is to get something encrypted and decrypted without ever knowing
the actual key that was used to encrypt it. The attached patch has two
APIs to wrap and unwrap the secret by the encryption key stored inside
database cluster. user generate a secret key locally and send it to
PostgreSQL server to wrap it using by pg_kmgr_wrap() and save it
somewhere. Then user can use the saved and wrapped secret key to
encrypt and decrypt user data by something like:

INSERT INTO tbl VALUES (pg_encrypt('user data', pg_kmgr_unwrap('x'));

Where 'x' is the result of pg_kmgr_wrap function.

I've attached the KMS patch. It requires openssl library. What the
patch does is simple and is not changed much from the previous version
patch that includes KMS and TDE; we generate one encryption key called
master key for whole database cluster at initdb time, which is stored
in pg_control and wrapped by key encryption key(KEK) derived from
user-provided passphrase. When postmaster starts up it verifies the
correctness of passphrase provided by user using hmac key which is
also derived from user-provided passphrase. The server won't start if
the passphrase is incorrect. Once the passphrase is verified the
master key is loaded to the shared buffer and is active.

I added two options to initdb: --cluster-passphrase-command and -e
that takes a passphrase command and a cipher algorithm (currently only
aes-128 and aes-256) respectively. The internal KMS is enabled by
executing initdb with those options as follows:

$ initdb -D data --cluster-passphrase-command="echo 'password'" -e aes-256

I believe the internal KMS would be useful several use cases but I'd
like to have discussion around the integration with pgcrypto because
pgcrypto would be the first user of the KMS and pgcrypto can be more
powerful with the KMS. I'll register this KMS patch to the next Commit
Fest.

I really appreciate peoples (CC-ing) who participated in off-list
discussion/meeting for many inputs, suggestions and reviewing codes.

Regards,


--
Masahiko Sawada  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/configure b/configure
index 3d9bd0bdf8..19f927cfb8 100755
--- a/configure
+++ b/configure
@@ -12111,7 +12111,7 @@ done
   # defines OPENSSL_VERSION_NUMBER to claim version 2.0.0, even though it
   # doesn't have these OpenSSL 1.1.0 functions. So check for individual
   # functions.
-  for ac_func in OPENSSL_init_ssl BIO_get_data BIO_meth_new ASN1_STRING_get0_data
+  for ac_func in OPENSSL_init_ssl OPENSSL_init_crypto BIO_get_data BIO_meth_new ASN1_STRING_get0_data
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/configure.in b/configure.in
index 34aea0b4ac..0a94ea37c5 100644
--- a/configure.in
+++ b/configure.in
@@ -1193,7 +1193,7 @@ if test "$with_openssl" = yes ; then
   # defines OPENSSL_VERSION_NUMBER to claim version 2.0.0, even though it
   # doesn't have these OpenSSL 1.1.0 functions. So check for individual
   # functions.
-  AC_CHECK_FUNCS([OPENSSL_init_ssl BIO_get_data BIO_meth_new ASN1_STRING_get0_data])
+  AC_CHECK_FUNCS([OPENSSL_init_ssl OPENSSL_init_crypto BIO_get_data BIO_meth_new ASN1_STRING_get0_data])
   # OpenSSL versions before 1.1.0 required setting callback functions, for
   # thread-safety. In 1.1.0, it's no longer required, and CRYPTO_lock()
   # function was removed.
diff --git a/src/backend/Makefile b/src/backend/Makefile
index b0d2be7ee0..64ce50474f 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -21,7 +21,7 @@ SUBDIRS = access bootstrap catalog parser commands executor foreign lib libpq \
 	main nodes optimizer partitioning port postmaster \
 	regex replication rewrite \
 	statistics storage tcop tsearch utils $(top_builddir)/src/timezone \
-	jit
+	jit crypto
 
 include 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Michael Paquier]

On Fri, Nov 01, 2019 at 09:38:37AM +0900, Moon, Insung wrote:
> Of course, I may not have written the excellent quality code
> correctly, so I will make an interim report if possible.

The last patch has rotten, and does not apply anymore.  A rebase would
be nice, so I am switching the patch as waiting on author, and moved
it to next CF.

The discussion has gone long around..
--
Michael


signature.asc
Description: PGP signature

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Antonin Houska]

Robert Haas  wrote:

> On Sat, Nov 2, 2019 at 8:23 AM Antonin Houska  wrote:
> > Change to hint bits does not result in LSN change in the case I described 
> > here
> >
> > https://www.postgresql.org/message-id/28452.1572443058%40antos
> >
> > but I consider this a bug (BTW, I discovered this problem when thinking 
> > about
> > the use of LSN as encryption IV). Do you mean any other case? If LSN does 
> > not
> > get changed, then the related full-page image WAL record is not guaranteed 
> > to
> > be on disk during crash recovery. Thus if page checksum is invalid due to
> > torn-page write, there's now WAL record to fix the page.
> 
> I thought the idea was that the first change to hint bits after a
> given checkpoint produced an FPI, but subsequent changes within the
> same checkpoint cycle do not.

Got it, this is what happens in XLogSaveBufferForHint().

Perhaps we can fix it by issuing XLOG_NOOP record in the cases that produce no
FPI. Of course only if the encryption is enabled.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[shawn wang]

Hi hackers,
By arrange, I will complete the modification of the front-end tool to
support TDE.
Now I have completed the modification of the pg_waldump, pg_resetwal, and
pg_rewind tools.
My design:
1. Add two options,  -D and -c, to the front-end tools. You can use -c to
get a password of the user to generate kek; use the -D option to get
cluster encryption, walkey, and relkey.
2. pg_waldump adds wal decryption function
3. pg_rewind adds wal decryption function
4. pg_resetwal adds wal encryption

Regards,

--
Shawn Wang

Masahiko Sawada  于2019年10月31日周四 下午10:25写道：

> On Fri, Sep 6, 2019 at 3:34 PM Smith, Peter 
> wrote:
> >
> > -Original Message-
> > From: Masahiko Sawada  Sent: Thursday, 15 August
> 2019 7:10 PM
> >
> > > BTW I've created PoC patch for cluster encryption feature. Attached
> patch set has done some items of TODO list and some of them can be used
> even for finer granularity encryption. Anyway, the implemented components
> are followings:
> >
> > Hello Sawada-san,
> >
> > I guess your original patch code may be getting a bit out-dated by the
> ongoing TDE discussions, but I have done some code review of it anyway.
> >
> > Hopefully a few comments below can still be of use going forward:
> >
> > ---
> >
> > REVIEW COMMENTS
> >
> > * src/backend/storage/encryption/enc_cipher.c – For functions
> EncryptionCipherValue/String maybe should log warnings for unexpected
> values instead of silently assigning to default 0/”off”.
> >
> > * src/backend/storage/encryption/enc_cipher.c – For function
> EncryptionCipherString, purpose of returning ”unknown” if unclear because
> that will map back to “off” again anyway via EncryptionCipherValue. Why not
> just return "off" (with warning logged).
> >
> > * src/include/storage/enc_common.h – Typo in comment: "Encrypton".
> >
> > * src/include/storage/encryption.h - The macro DataEncryptionEnabled may
> be better to be using enum TDE_ENCRYPTION_OFF instead of magic number 0
> >
> > * src/backend/storage/encryption/kmgr.c - Function BootStrapKmgr will
> report error if USE_OPENSSL is not defined. The check seems premature
> because it would fail even if the user is not using encryption. Shouldn't
> the lack of openssl be OK when user is not using TDE at all (i.e. when
> encryption is "none")?
> >
> > * src/backend/storage/encryption/kmgr.c - In function BootStrapMgr
> suggest better to check if (bootstrap_data_encryption_cipher ==
> TDE_ENCRYPTION_OFF) using enum instead of the magic number 0.
> >
> > * src/backend/storage/encryption/kmgr.c - The function
> run_cluster_passphrase_command function seems mostly a clone of an existing
> run_ssl_passphrase_command function. Is it possible to refactor to share
> the common code?
> >
> > * src/backend/storage/encryption/kmgr.c - The function
> derive_encryption_key declares a char key_len. Why char? It seems int
> everywhere else.
> >
> > * src/backend/bootstrap/bootstrap.c - Suggest better if variable
> declaration bootstrap_data_encryption_cipher = 0 uses enum
> TDE_ENCRYPTION_OFF instead of magic number 0
> >
> > * src/backend/utils/misc/guc.c - It looks like the default value for GUC
> variable data_encryption_cipher is AES128. Wouldn't "off" be the more
> appropriate default value? Otherwise it seems inconsistent with the logic
> of initdb (which insists that the -e option is mandatory if you wanted any
> encryption).
> >
> > * src/backend/utils/misc/guc.c - There is a missing entry in the
> config_group_names[]. The patch changed the config_group[] in guc_tables.h,
> so I think there needs to be a matching item in the config_group_names.
> >
> > * src/bin/initdb/initdb.c - The function check_encryption_cipher would
> disallow an encryption value of "none". Although maybe it is not very
> useful to say -e none, it does seem inconsistent to reject it, given that
> "none" was a valid value for the GUC variable data_encryption_cipher.
> >
> > * contrib/bloom/blinsert.c - In function btbuildempty the arguments for
> PageEncryptionInPlace seem in the wrong order (forknum should be 2nd).
> >
> > * src/backend/access/hash/hashpage.c - In function _hash_alloc_buckets
> the arguments for PageEncryptionInPlace seem in the wrong order (forknum
> should be 2nd).
> >
> > * src/backend/access/spgist/spginsert.c - In function spgbuildempty the
> arguments for PageEncryptionInPlace seem in the wrong order (forknum should
> be 2nd). This error looks repeated 3X.
> >
> > * in multiple files - The encryption enums have equivalent strings
> ("off", "aes-128", "aes-256") but they are scattered as string literals in
> many places (e.g. pg_controldata.c, initdb.c, guc.c, enc_cipher.c). Suggest
> it would be better to declare those as string constants in one place only.em
> >
>
> Thank you for reviewing this patch.
>
> I've updated TDE patches. These patches implements key system, buffer
> encryption and WAL encryption. Please refer to ToDo of cluster-wide
> encryption for more details of design and components. It lacks
> 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Sat, Nov  2, 2019 at 01:34:41PM +0100, Antonin Houska wrote:
> Robert Haas  wrote:
> 
> > On Tue, Aug 6, 2019 at 10:36 AM Bruce Momjian  wrote:
> 
> > Seems reasonable (not that I am an encryption expert).
> > 
> > > For WAL, we effectively create a 16MB bitstream, though we can create it
> > > in parts as needed.  (Creating it in parts is easier in CTR mode.)  The
> > > nonce is the segment number, but each 16-byte chunk uses a different
> > > counter.  Therefore, even if you are encrypting the same 8k page several
> > > times in the WAL, the 8k page would be different because of the LSN (and
> > > other changes), and the bitstream you encrypt/XOR it with would be
> > > different because the counter would be different for that offset in the
> > > WAL.
> > 
> > But, if you encrypt the same WAL page several times, the LSN won't
> > change, because a WAL page doesn't have an LSN on it, and if it did,
> > it wouldn't be changing, because an LSN is just a position within the
> > WAL stream, so any given byte on any given WAL page always has the
> > same LSN, whatever it is.
> > 
> > And if the counter value changed on re-encryption, I don't see how
> > we'd know what counter value to use when decrypting.  There's no way
> > for the code that is decrypting to know how many times the page got
> > rewritten as it was being filled.
> > 
> > Please correct me if I'm being stupid here.
> 
> In my implementation (I haven't checked whether Masahiko Sawada changed this
> in his patch) I avoided repeated encryption of different data using the same
> key+IV by omitting the unused part of the WAL page from encryption. Already
> written records can be encrypted repeatedly because they do not change.

Right.  Even though AES with CTR generates encryption bit patterns in
16-byte chunks, you only XOR the bytes you have written.  So, if the WAL
record is 167 bytes, you generate 11 16-byte patterns, but you only XOR
the first seven bytes of the 11th 16-byte block.  CTR is not like CBC
which has to encrypt in 16-byte chunks.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Masahiko Sawada]

On Sat, 2 Nov 2019 at 21:33, Antonin Houska  wrote:

> Robert Haas  wrote:
>
> > On Tue, Aug 6, 2019 at 10:36 AM Bruce Momjian  wrote:
>
> > Seems reasonable (not that I am an encryption expert).
> >
> > > For WAL, we effectively create a 16MB bitstream, though we can create
> it
> > > in parts as needed.  (Creating it in parts is easier in CTR mode.)  The
> > > nonce is the segment number, but each 16-byte chunk uses a different
> > > counter.  Therefore, even if you are encrypting the same 8k page
> several
> > > times in the WAL, the 8k page would be different because of the LSN
> (and
> > > other changes), and the bitstream you encrypt/XOR it with would be
> > > different because the counter would be different for that offset in the
> > > WAL.
> >
> > But, if you encrypt the same WAL page several times, the LSN won't
> > change, because a WAL page doesn't have an LSN on it, and if it did,
> > it wouldn't be changing, because an LSN is just a position within the
> > WAL stream, so any given byte on any given WAL page always has the
> > same LSN, whatever it is.
> >
> > And if the counter value changed on re-encryption, I don't see how
> > we'd know what counter value to use when decrypting.  There's no way
> > for the code that is decrypting to know how many times the page got
> > rewritten as it was being filled.
> >
> > Please correct me if I'm being stupid here.
>
> In my implementation (I haven't checked whether Masahiko Sawada changed
> this
> in his patch) I avoided repeated encryption of different data using the
> same
> key+IV by omitting the unused part of the WAL page from encryption. Already
> written records can be encrypted repeatedly because they do not change.
>
>
Yeah my patch doesn't change this part. IV for WAL encryption consists of
the segment file number, page offset within segment file and the counter
for CTR cipher mode.

Regards,

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

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Antonin Houska]

Robert Haas  wrote:

> On Tue, Aug 6, 2019 at 10:36 AM Bruce Momjian  wrote:

> Seems reasonable (not that I am an encryption expert).
> 
> > For WAL, we effectively create a 16MB bitstream, though we can create it
> > in parts as needed.  (Creating it in parts is easier in CTR mode.)  The
> > nonce is the segment number, but each 16-byte chunk uses a different
> > counter.  Therefore, even if you are encrypting the same 8k page several
> > times in the WAL, the 8k page would be different because of the LSN (and
> > other changes), and the bitstream you encrypt/XOR it with would be
> > different because the counter would be different for that offset in the
> > WAL.
> 
> But, if you encrypt the same WAL page several times, the LSN won't
> change, because a WAL page doesn't have an LSN on it, and if it did,
> it wouldn't be changing, because an LSN is just a position within the
> WAL stream, so any given byte on any given WAL page always has the
> same LSN, whatever it is.
> 
> And if the counter value changed on re-encryption, I don't see how
> we'd know what counter value to use when decrypting.  There's no way
> for the code that is decrypting to know how many times the page got
> rewritten as it was being filled.
> 
> Please correct me if I'm being stupid here.

In my implementation (I haven't checked whether Masahiko Sawada changed this
in his patch) I avoided repeated encryption of different data using the same
key+IV by omitting the unused part of the WAL page from encryption. Already
written records can be encrypted repeatedly because they do not change.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Antonin Houska]

Robert Haas  wrote:

> On Mon, Aug 5, 2019 at 8:44 PM Bruce Momjian  wrote:
> > Right.  The 8k page LSN changes each time the page is modified, and the
> > is part of the page nonce.
> 
> What about hint bit changes?
> 
> I think even with wal_log_hints=on, it's not the case that *every*
> change to hint bits results in an LSN change.

Change to hint bits does not result in LSN change in the case I described here

https://www.postgresql.org/message-id/28452.1572443058%40antos

but I consider this a bug (BTW, I discovered this problem when thinking about
the use of LSN as encryption IV). Do you mean any other case? If LSN does not
get changed, then the related full-page image WAL record is not guaranteed to
be on disk during crash recovery. Thus if page checksum is invalid due to
torn-page write, there's now WAL record to fix the page.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Robert Haas]

On Tue, Aug 6, 2019 at 10:36 AM Bruce Momjian  wrote:
> OK, I think you are missing something.   Let me go over the details.
> First, I think we are all agreed we are using CTR for heap/index pages,
> and for WAL, because CTR allows byte granularity, it is faster, and
> might be more secure.
>
> So, to write 8k heap/index pages, we use the agreed-on LSN/page-number
> to encrypt each page.  In CTR mode, we do that by creating an 8k bit
> stream, which is created in 16-byte chunks with AES by incrementing the
> counter used for each 16-byte chunk.  Wee then XOR the bits with what we
> want to encrypt, and skip the LSN and CRC parts of the page.

Seems reasonable (not that I am an encryption expert).

> For WAL, we effectively create a 16MB bitstream, though we can create it
> in parts as needed.  (Creating it in parts is easier in CTR mode.)  The
> nonce is the segment number, but each 16-byte chunk uses a different
> counter.  Therefore, even if you are encrypting the same 8k page several
> times in the WAL, the 8k page would be different because of the LSN (and
> other changes), and the bitstream you encrypt/XOR it with would be
> different because the counter would be different for that offset in the
> WAL.

But, if you encrypt the same WAL page several times, the LSN won't
change, because a WAL page doesn't have an LSN on it, and if it did,
it wouldn't be changing, because an LSN is just a position within the
WAL stream, so any given byte on any given WAL page always has the
same LSN, whatever it is.

And if the counter value changed on re-encryption, I don't see how
we'd know what counter value to use when decrypting.  There's no way
for the code that is decrypting to know how many times the page got
rewritten as it was being filled.

Please correct me if I'm being stupid here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Robert Haas]

On Mon, Aug 5, 2019 at 8:44 PM Bruce Momjian  wrote:
> Right.  The 8k page LSN changes each time the page is modified, and the
> is part of the page nonce.

What about hint bit changes?

I think even with wal_log_hints=on, it's not the case that *every*
change to hint bits results in an LSN change.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Moon, Insung]

Hello.

On Thu, Oct 31, 2019 at 11:25 PM Masahiko Sawada  wrote:
>
> On Fri, Sep 6, 2019 at 3:34 PM Smith, Peter  
> wrote:
> >
> > -Original Message-
> > From: Masahiko Sawada  Sent: Thursday, 15 August 
> > 2019 7:10 PM
> >
> > > BTW I've created PoC patch for cluster encryption feature. Attached patch 
> > > set has done some items of TODO list and some of them can be used even 
> > > for finer granularity encryption. Anyway, the implemented components are 
> > > followings:
> >
> > Hello Sawada-san,
> >
> > I guess your original patch code may be getting a bit out-dated by the 
> > ongoing TDE discussions, but I have done some code review of it anyway.
> >
> > Hopefully a few comments below can still be of use going forward:
> >
> > ---
> >
> > REVIEW COMMENTS
> >
> > * src/backend/storage/encryption/enc_cipher.c – For functions 
> > EncryptionCipherValue/String maybe should log warnings for unexpected 
> > values instead of silently assigning to default 0/”off”.
> >
> > * src/backend/storage/encryption/enc_cipher.c – For function 
> > EncryptionCipherString, purpose of returning ”unknown” if unclear because 
> > that will map back to “off” again anyway via EncryptionCipherValue. Why not 
> > just return "off" (with warning logged).
> >
> > * src/include/storage/enc_common.h – Typo in comment: "Encrypton".
> >
> > * src/include/storage/encryption.h - The macro DataEncryptionEnabled may be 
> > better to be using enum TDE_ENCRYPTION_OFF instead of magic number 0
> >
> > * src/backend/storage/encryption/kmgr.c - Function BootStrapKmgr will 
> > report error if USE_OPENSSL is not defined. The check seems premature 
> > because it would fail even if the user is not using encryption. Shouldn't 
> > the lack of openssl be OK when user is not using TDE at all (i.e. when 
> > encryption is "none")?
> >
> > * src/backend/storage/encryption/kmgr.c - In function BootStrapMgr suggest 
> > better to check if (bootstrap_data_encryption_cipher == TDE_ENCRYPTION_OFF) 
> > using enum instead of the magic number 0.
> >
> > * src/backend/storage/encryption/kmgr.c - The function 
> > run_cluster_passphrase_command function seems mostly a clone of an existing 
> > run_ssl_passphrase_command function. Is it possible to refactor to share 
> > the common code?
> >
> > * src/backend/storage/encryption/kmgr.c - The function 
> > derive_encryption_key declares a char key_len. Why char? It seems int 
> > everywhere else.
> >
> > * src/backend/bootstrap/bootstrap.c - Suggest better if variable 
> > declaration bootstrap_data_encryption_cipher = 0 uses enum 
> > TDE_ENCRYPTION_OFF instead of magic number 0
> >
> > * src/backend/utils/misc/guc.c - It looks like the default value for GUC 
> > variable data_encryption_cipher is AES128. Wouldn't "off" be the more 
> > appropriate default value? Otherwise it seems inconsistent with the logic 
> > of initdb (which insists that the -e option is mandatory if you wanted any 
> > encryption).
> >
> > * src/backend/utils/misc/guc.c - There is a missing entry in the 
> > config_group_names[]. The patch changed the config_group[] in guc_tables.h, 
> > so I think there needs to be a matching item in the config_group_names.
> >
> > * src/bin/initdb/initdb.c - The function check_encryption_cipher would 
> > disallow an encryption value of "none". Although maybe it is not very 
> > useful to say -e none, it does seem inconsistent to reject it, given that 
> > "none" was a valid value for the GUC variable data_encryption_cipher.
> >
> > * contrib/bloom/blinsert.c - In function btbuildempty the arguments for 
> > PageEncryptionInPlace seem in the wrong order (forknum should be 2nd).
> >
> > * src/backend/access/hash/hashpage.c - In function _hash_alloc_buckets the 
> > arguments for PageEncryptionInPlace seem in the wrong order (forknum should 
> > be 2nd).
> >
> > * src/backend/access/spgist/spginsert.c - In function spgbuildempty the 
> > arguments for PageEncryptionInPlace seem in the wrong order (forknum should 
> > be 2nd). This error looks repeated 3X.
> >
> > * in multiple files - The encryption enums have equivalent strings ("off", 
> > "aes-128", "aes-256") but they are scattered as string literals in many 
> > places (e.g. pg_controldata.c, initdb.c, guc.c, enc_cipher.c). Suggest it 
> > would be better to declare those as string constants in one place only.em
> >
>
> Thank you for reviewing this patch.
>
> I've updated TDE patches. These patches implements key system, buffer
> encryption and WAL encryption. Please refer to ToDo of cluster-wide
> encryption for more details of design and components. It lacks
> temporary file encryption and front end tools encryption. For
> temporary file encryption, we are discussing which files should be
> encrypted on other thread and I thought that temporary file encryption
> might be related to that. So I'm currently studying the temporary
> encryption patch that Antonin already submitted[1] but some changes
> might be needed based on that 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Masahiko Sawada]

On Fri, Sep 6, 2019 at 3:34 PM Smith, Peter  wrote:
>
> -Original Message-
> From: Masahiko Sawada  Sent: Thursday, 15 August 2019 
> 7:10 PM
>
> > BTW I've created PoC patch for cluster encryption feature. Attached patch 
> > set has done some items of TODO list and some of them can be used even for 
> > finer granularity encryption. Anyway, the implemented components are 
> > followings:
>
> Hello Sawada-san,
>
> I guess your original patch code may be getting a bit out-dated by the 
> ongoing TDE discussions, but I have done some code review of it anyway.
>
> Hopefully a few comments below can still be of use going forward:
>
> ---
>
> REVIEW COMMENTS
>
> * src/backend/storage/encryption/enc_cipher.c – For functions 
> EncryptionCipherValue/String maybe should log warnings for unexpected values 
> instead of silently assigning to default 0/”off”.
>
> * src/backend/storage/encryption/enc_cipher.c – For function 
> EncryptionCipherString, purpose of returning ”unknown” if unclear because 
> that will map back to “off” again anyway via EncryptionCipherValue. Why not 
> just return "off" (with warning logged).
>
> * src/include/storage/enc_common.h – Typo in comment: "Encrypton".
>
> * src/include/storage/encryption.h - The macro DataEncryptionEnabled may be 
> better to be using enum TDE_ENCRYPTION_OFF instead of magic number 0
>
> * src/backend/storage/encryption/kmgr.c - Function BootStrapKmgr will report 
> error if USE_OPENSSL is not defined. The check seems premature because it 
> would fail even if the user is not using encryption. Shouldn't the lack of 
> openssl be OK when user is not using TDE at all (i.e. when encryption is 
> "none")?
>
> * src/backend/storage/encryption/kmgr.c - In function BootStrapMgr suggest 
> better to check if (bootstrap_data_encryption_cipher == TDE_ENCRYPTION_OFF) 
> using enum instead of the magic number 0.
>
> * src/backend/storage/encryption/kmgr.c - The function 
> run_cluster_passphrase_command function seems mostly a clone of an existing 
> run_ssl_passphrase_command function. Is it possible to refactor to share the 
> common code?
>
> * src/backend/storage/encryption/kmgr.c - The function derive_encryption_key 
> declares a char key_len. Why char? It seems int everywhere else.
>
> * src/backend/bootstrap/bootstrap.c - Suggest better if variable declaration 
> bootstrap_data_encryption_cipher = 0 uses enum TDE_ENCRYPTION_OFF instead of 
> magic number 0
>
> * src/backend/utils/misc/guc.c - It looks like the default value for GUC 
> variable data_encryption_cipher is AES128. Wouldn't "off" be the more 
> appropriate default value? Otherwise it seems inconsistent with the logic of 
> initdb (which insists that the -e option is mandatory if you wanted any 
> encryption).
>
> * src/backend/utils/misc/guc.c - There is a missing entry in the 
> config_group_names[]. The patch changed the config_group[] in guc_tables.h, 
> so I think there needs to be a matching item in the config_group_names.
>
> * src/bin/initdb/initdb.c - The function check_encryption_cipher would 
> disallow an encryption value of "none". Although maybe it is not very useful 
> to say -e none, it does seem inconsistent to reject it, given that "none" was 
> a valid value for the GUC variable data_encryption_cipher.
>
> * contrib/bloom/blinsert.c - In function btbuildempty the arguments for 
> PageEncryptionInPlace seem in the wrong order (forknum should be 2nd).
>
> * src/backend/access/hash/hashpage.c - In function _hash_alloc_buckets the 
> arguments for PageEncryptionInPlace seem in the wrong order (forknum should 
> be 2nd).
>
> * src/backend/access/spgist/spginsert.c - In function spgbuildempty the 
> arguments for PageEncryptionInPlace seem in the wrong order (forknum should 
> be 2nd). This error looks repeated 3X.
>
> * in multiple files - The encryption enums have equivalent strings ("off", 
> "aes-128", "aes-256") but they are scattered as string literals in many 
> places (e.g. pg_controldata.c, initdb.c, guc.c, enc_cipher.c). Suggest it 
> would be better to declare those as string constants in one place only.em
>

Thank you for reviewing this patch.

I've updated TDE patches. These patches implements key system, buffer
encryption and WAL encryption. Please refer to ToDo of cluster-wide
encryption for more details of design and components. It lacks
temporary file encryption and front end tools encryption. For
temporary file encryption, we are discussing which files should be
encrypted on other thread and I thought that temporary file encryption
might be related to that. So I'm currently studying the temporary
encryption patch that Antonin already submitted[1] but some changes
might be needed based on that discussion. For frontend tool support,
Shawn will share his patch that is built on my patch.

I haven't changed the usage of this feature. Please refer to the
email[2] for how to setup encrypted database cluster.

[1] 

RE: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Smith, Peter]

-Original Message-
From: Masahiko Sawada  Sent: Thursday, 15 August 2019 
7:10 PM

> BTW I've created PoC patch for cluster encryption feature. Attached patch set 
> has done some items of TODO list and some of them can be used even for finer 
> granularity encryption. Anyway, the implemented components are followings:

Hello Sawada-san,

I guess your original patch code may be getting a bit out-dated by the ongoing 
TDE discussions, but I have done some code review of it anyway. 

Hopefully a few comments below can still be of use going forward:

---

REVIEW COMMENTS

* src/backend/storage/encryption/enc_cipher.c – For functions 
EncryptionCipherValue/String maybe should log warnings for unexpected values 
instead of silently assigning to default 0/”off”.

* src/backend/storage/encryption/enc_cipher.c – For function 
EncryptionCipherString, purpose of returning ”unknown” if unclear because that 
will map back to “off” again anyway via EncryptionCipherValue. Why not just 
return "off" (with warning logged).

* src/include/storage/enc_common.h – Typo in comment: "Encrypton".

* src/include/storage/encryption.h - The macro DataEncryptionEnabled may be 
better to be using enum TDE_ENCRYPTION_OFF instead of magic number 0

* src/backend/storage/encryption/kmgr.c - Function BootStrapKmgr will report 
error if USE_OPENSSL is not defined. The check seems premature because it would 
fail even if the user is not using encryption. Shouldn't the lack of openssl be 
OK when user is not using TDE at all (i.e. when encryption is "none")?

* src/backend/storage/encryption/kmgr.c - In function BootStrapMgr suggest 
better to check if (bootstrap_data_encryption_cipher == TDE_ENCRYPTION_OFF) 
using enum instead of the magic number 0.

* src/backend/storage/encryption/kmgr.c - The function 
run_cluster_passphrase_command function seems mostly a clone of an existing 
run_ssl_passphrase_command function. Is it possible to refactor to share the 
common code?

* src/backend/storage/encryption/kmgr.c - The function derive_encryption_key 
declares a char key_len. Why char? It seems int everywhere else.

* src/backend/bootstrap/bootstrap.c - Suggest better if variable declaration 
bootstrap_data_encryption_cipher = 0 uses enum TDE_ENCRYPTION_OFF instead of 
magic number 0

* src/backend/utils/misc/guc.c - It looks like the default value for GUC 
variable data_encryption_cipher is AES128. Wouldn't "off" be the more 
appropriate default value? Otherwise it seems inconsistent with the logic of 
initdb (which insists that the -e option is mandatory if you wanted any 
encryption).

* src/backend/utils/misc/guc.c - There is a missing entry in the 
config_group_names[]. The patch changed the config_group[] in guc_tables.h, so 
I think there needs to be a matching item in the config_group_names.

* src/bin/initdb/initdb.c - The function check_encryption_cipher would disallow 
an encryption value of "none". Although maybe it is not very useful to say -e 
none, it does seem inconsistent to reject it, given that "none" was a valid 
value for the GUC variable data_encryption_cipher.

* contrib/bloom/blinsert.c - In function btbuildempty the arguments for 
PageEncryptionInPlace seem in the wrong order (forknum should be 2nd).

* src/backend/access/hash/hashpage.c - In function _hash_alloc_buckets the 
arguments for PageEncryptionInPlace seem in the wrong order (forknum should be 
2nd).

* src/backend/access/spgist/spginsert.c - In function spgbuildempty the 
arguments for PageEncryptionInPlace seem in the wrong order (forknum should be 
2nd). This error looks repeated 3X.

* in multiple files - The encryption enums have equivalent strings ("off", 
"aes-128", "aes-256") but they are scattered as string literals in many places 
(e.g. pg_controldata.c, initdb.c, guc.c, enc_cipher.c). Suggest it would be 
better to declare those as string constants in one place only.

---

Kind Regards,

Peter Smith
Fujitsu Australia

RE: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Smith, Peter]

Greetings, 

(Apologies for any naïve thoughts below. Please correct my misunderstandings)

I am trying to understand the background for the ideas proposed and/or already 
decided, but it is increasingly difficult to follow.

I’ve been watching the TDE list for several months and over that time there 
have been dozens of different ideas floated; Each of them have their own good 
points; Some are conflicting;

IMO any TDE implementation will be a trade-off between a number of factors:
* Design – e.g. Simple v Complex solution
* Secureness – e.g. Acceptance that a simpler solution may not handle every 
possible threat
* Cost/Feasibility – e.g. How hard will TDE be to implement/maintain. 
* User expectations - e.g. What is the “threat model” the end user actually 
wants to protect against
* User expectations – e.g. Comparison with other products
* Completeness – e.g. Acknowledgement that first implementation may not meet 
the end-goal.
* Future proof – e.g. ability to evolve in future TDE versions (with minimal 
re-write of what came before)
* Usability – e.g. Online/offline considerations
* Usability – e.g. Will a more complex solution end up being too difficult to 
actually use/administer
* etc…

New TDE ideas keep popping up all the time. The discussion sometimes has become 
mired in technical details; I’m losing sight of the bigger picture.

Would it be possible to share a *tabulation* for all the TDE components; Each 
component may be a number of design choices (options); And have brief lists of 
Pros/Cons for each of those options so that each can be concisely summarised on 
their respective merits.

I think this would be of a great help to understand how we got to where we are 
now, as well as helping to focus on how to proceed.

For example,

=
Component: TDKEY
* Option: use derived keys; List of Pros/Cons
* Option: use random keys; List of Pros/Cons
* Option: use keys from some external source and encrypted by MDKEY; List of 
Pros/Cons
* Option: use same TKEY for all tables/tablespaces; List of Pros/Cons
* Option: … 
* Option: …
* => Decision (i.e. the least-worst compromise/combination of the possible 
options)
=

~

Postscript: 

After writing this, I recalled recently reading a mail from Antonin 
https://www.postgresql.org/message-id/44057.1565977657%40antos which says 
pretty much the same thing!

Also, I recognise that there is an offline shared Googledoc which already 
includes some of this information, but I think it would be valuable if it could 
be formatted as Pros/Cons summary table and shared on the Wiki page for 
everybody to see.


Kind Regards,
---
Peter Smith
Fujitsu Australia

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Joe Conway]

On 8/26/19 2:53 AM, Masahiko Sawada wrote:
> I guess that this depends on the number of encryption keys we use. If
> we have encryption keys per tablespace or database the number of keys
> would be at most several dozen or several hundred. It's enough to have
> them in flat-file format on the disk and to load them to the hash
> table on the shared memory. We would not need a complex mechanism.
> OTOH if we have keys per tables, we would need to consider indexes and
> buffering as they might not fit in the memory.

Master key(s) need to be kept in memory, but derived keys (using KDF)
would be calculated at time of use, I would think.

>> Some kind of flat-file based approach with a temp file and renaming of
>> files using durable_rename(), like what we used to do with
>> pg_shadow/authid, and now do with replorigin_checkpoint and such?
> 
> The PoC patch I created does that for the keyring file. When key
> rotation, the correspond WAL contains all re-encrypted keys with the
> master key identifier, and the recovery restores the keyring file. One
> good point of this approach is that external tools and startup process
> read it easier. It doesn't require backend codes such as system cache
> and heap functions.

That sounds like a good approach.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Masahiko Sawada]

On Mon, Aug 26, 2019 at 7:49 PM Joe Conway  wrote:
>
> On 8/26/19 2:53 AM, Masahiko Sawada wrote:
> > I guess that this depends on the number of encryption keys we use. If
> > we have encryption keys per tablespace or database the number of keys
> > would be at most several dozen or several hundred. It's enough to have
> > them in flat-file format on the disk and to load them to the hash
> > table on the shared memory. We would not need a complex mechanism.
> > OTOH if we have keys per tables, we would need to consider indexes and
> > buffering as they might not fit in the memory.
>
> Master key(s) need to be kept in memory, but derived keys (using KDF)
> would be calculated at time of use, I would think.

Yes, we can do that and the PoC patch does so. I'm rather concerned
the salt and info to derive keys. We would need at least info, which
could be OID perhaps, for each keys. Also these data need to be
accessible by both frontend tool and startup process. If the info is
very small data, say 4 byte of OID, we could have all of them on the
memory even if we have keys per tables.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Masahiko Sawada]

On Fri, Aug 23, 2019 at 11:35 PM Stephen Frost  wrote:
>
> Greetings,
>
> * Bruce Momjian (br...@momjian.us) wrote:
> > On Fri, Aug 23, 2019 at 07:45:22AM -0400, Stephen Frost wrote:
> > > Having listed out the feature set of each of the other major databases
> > > when it comes to TDE is exactly how we objectively look at what is being
> > > done in the industry, and that then gives us an understanding of what
> > > users (and auditors) coming from other platforms will expect.
> > >
> > > I entirely agree that we shouldn't just copy N feature from X other
> > > database system unless we feel that's the best approach, but when every
> > > other database system out there has capability Y for the general feature
> > > X that we're thinking about implementing, we should be questioning an
> > > approach which doesn't include that.
> >
> > Agreed.  The features of other databases are a clear source for what we
> > should consider and run through the useful/reasonable filter.
>
> Following on from that- when other databases don't have something that
> we're thinking about implementing, maybe we should be contemplating if
> it really makes sense as a requirement for us.
>
> Specifically in this case- I went back and tried to figure out what
> other database systems have an "encrypt EVERYTHING" option.  I didn't
> have much luck finding one though.  So I think we need to ask ourselves-
> the "check box" that we're trying to check off with TDE, do the other
> database system check that box?  If so, then it looks like the "check
> box" isn't actually "encrypt EVERYTHING", it's more along the lines of
> "make sure all regular user data is encrypted automatically" or some
> such, and that's a very different requirement, which seems to be
> answered by the other systems by having a KMS + tablespace/database
> level encryption.  We certainly shouldn't be putting a lot of effort
> into building something that is either overkill or won't be interesting
> to users due to limitations like "have to take the entire cluster
> offline to re-key it".
>
> Now, that KMS has to be encrypted using a master key, of course, and we
> have to make sure that it is able to survive across a crash, and it'd
> sure be nice if it was indexed.  One option for such a KMS would be
> something entirely external (which could potentially just be another PG
> database or something) but it'd be nice if we had something built-in.
> We might also want it to be replicated (or maybe we don't, as was
> discussed on the call, to allow for a replica to use an independent set
> of keys- of course that leads to issues with pg_rewind and such though).

I think most user would expect the physical standby server uses the
same key as the primary server's one, at least for the master key.
Otherwise they would need to use different keys every time of fail
over. Even for WAL encryption keys, since it's common to fetch
archived WAL files that are produced by the primary server by
restore_command using scp the standby server needs to use the same
keys or at least know it. In logical replication, I think that since
we would sent unencrypted data and encrypt it on the subscriber that
is initiated as a different database cluster we can use the different
keys on both sides.

> Anything built-in does seem like it'd be a fair bit of work to get it to
> address those requirements, but that does seem to be what the other
> database systems have done.  Unfortunately, their documentation doesn't
> seem to really say exactly what they've done to address that.

I guess that this depends on the number of encryption keys we use. If
we have encryption keys per tablespace or database the number of keys
would be at most several dozen or several hundred. It's enough to have
them in flat-file format on the disk and to load them to the hash
table on the shared memory. We would not need a complex mechanism.
OTOH if we have keys per tables, we would need to consider indexes and
buffering as they might not fit in the memory.

> A couple random ideas that probably won't work, but I'll put them out
> there for others to shoot down-
>
> Some kind of 2-phase WAL pass, where we do WAL replay for the
> non-encrypted bits first (which would include the KMS) and then go back
> and WAL replay the encrypted stuff.  Seems terrible.
>
> An independent WAL for the KMS only.  Ugh, do we need another walwriter
> then?  and buffers, and lots of other stuff.
>
> Some kind of flat-file based approach with a temp file and renaming of
> files using durable_rename(), like what we used to do with
> pg_shadow/authid, and now do with replorigin_checkpoint and such?

The PoC patch I created does that for the keyring file. When key
rotation, the correspond WAL contains all re-encrypted keys with the
master key identifier, and the recovery restores the keyring file. One
good point of this approach is that external tools and startup process
read it easier. It doesn't require backend codes such as system cache
and 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Moon, Insung]

Dear Hackers.


> Specifically in this case- I went back and tried to figure out what
> other database systems have an "encrypt EVERYTHING" option.  I didn't
> have much luck finding one though.  So I think we need to ask ourselves-
> the "check box" that we're trying to check off with TDE, do the other
> database system check that box?  If so, then it looks like the "check
> box" isn't actually "encrypt EVERYTHING", it's more along the lines of
> "make sure all regular user data is encrypted automatically" or some
> such, and that's a very different requirement, which seems to be
> answered by the other systems by having a KMS + tablespace/database
> level encryption.  We certainly shouldn't be putting a lot of effort
> into building something that is either overkill or won't be interesting
> to users due to limitations like "have to take the entire cluster
> offline to re-key it".
>
> Now, that KMS has to be encrypted using a master key, of course, and we
> have to make sure that it is able to survive across a crash, and it'd
> sure be nice if it was indexed.

Sorry, Does KMS here mean key Management System(or Service)?
I may be mistaken, but I know that KMS is managing cryptographic keys.
In other words, I kept the master key(or KEK) in KMS( not kept to
PostgreSQL server-side),
and PostgreSQL fetched the master key from KMS, and then encrypt or
decrypt it on the PostgreSQL server-side.
Of course, some KMS supports encryption function,
which is the function to encrypt plain text inside KMS. Is this
project aiming to use this function?


>
> A couple random ideas that probably won't work, but I'll put them out
> there for others to shoot down-
>
> Some kind of 2-phase WAL pass, where we do WAL replay for the
> non-encrypted bits first (which would include the KMS) and then go back
> and WAL replay the encrypted stuff.  Seems terrible.

Sorry, Can you tell me an example what is the 2-phase WAL pass?
I know that WAL read process is decrypted WAL data when
reading an encrypted WAL page(per-page encrypt) or
WAL record(per-record encrypt) and then replay.
Is this a different case?

Best Regards.
Moon.

>
> An independent WAL for the KMS only.  Ugh, do we need another walwriter
> then?  and buffers, and lots of other stuff.
>
> Some kind of flat-file based approach with a temp file and renaming of
> files using durable_rename(), like what we used to do with
> pg_shadow/authid, and now do with replorigin_checkpoint and such?
>
> Something else?
>
> Thoughts?
>
> Thanks!
>
> Stephen

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Moon, Insung]

Dear Hackers.
It's been a long time since I sent a mail.

On Sat, Aug 24, 2019 at 9:27 AM Bruce Momjian  wrote:

> On Fri, Aug 23, 2019 at 10:35:17AM -0400, Stephen Frost wrote:
> > > Agreed.  The features of other databases are a clear source for what we
> > > should consider and run through the useful/reasonable filter.
> >
> > Following on from that- when other databases don't have something that
> > we're thinking about implementing, maybe we should be contemplating if
> > it really makes sense as a requirement for us.
>
> Yes, that's a good point.
>
> > Specifically in this case- I went back and tried to figure out what
> > other database systems have an "encrypt EVERYTHING" option.  I didn't
> > have much luck finding one though.  So I think we need to ask ourselves-
> > the "check box" that we're trying to check off with TDE, do the other
> > database system check that box?  If so, then it looks like the "check
> > box" isn't actually "encrypt EVERYTHING", it's more along the lines of
> > "make sure all regular user data is encrypted automatically" or some
> > such, and that's a very different requirement, which seems to be
> > answered by the other systems by having a KMS + tablespace/database
> > level encryption.  We certainly shouldn't be putting a lot of effort
> > into building something that is either overkill or won't be interesting
> > to users due to limitations like "have to take the entire cluster
> > offline to re-key it".
>
> Well, I think they might do that to reduce encryption overhead.  I think
> tests have shown that is not an issue, but we will need to test further.
> I am not sure of the downside of encrypting everything, since it leaks
> the least information and has a minimal user API and code impact.  What
> is the value of encrypting only the user rows?  Better key control?
>

Maybe my think can be very wrong. Please understand.

I think that the value of encrypting with granularity rather than
encrypting of all clusters. Maybe it is advantageous to manageability.

Of course, encrypting of all clusters is an excellent choice because
it has minimal impact on the code, and perhaps simply to implement and
management APIs.

But what about Database user or DBA? I thought of the example below.

Suppose we have a system with multiple tenants
(Tenant here means table, tablespace, or database.) in one database cluster.
(I think it's similar to a cloud service. I think this is going to be a
common case in the future.)

We need to encrypt for tenant A and not need encryption for tenant B.
In this case, is there a reason to encrypt until tenant B?
It is a great advantage that the user, which is a characteristic of TDE,
is encrypted without considering encryption.
But there is no reason to encrypt even a tenant that does not require
encryption.
And another example, in terms of key management, I thought to encrypt with
granularity was a good choice (especially key rotation).

A special situation where A and B tenants need encryption and A tenant
needs to
rotate the key once every three months. And B tenant needs to rotate the
key once a year.
( Of course, maybe it is a very rare case.)

If we encrypt all clusters and do not manage of granularity encryption keys
by tenants.
And when they run to A tenant key rotated, the B tenant is also rotated
together,
which can cause operational discomfort.

Of course, encrypting of all clusters and creating the managing of
granularity keys for each tenant will solve the problem.
But if we are implementing this part, I think it's the same as
the implementation of granular encryption.

Best Regards.
Moon.


>
> > Now, that KMS has to be encrypted using a master key, of course, and we
> > have to make sure that it is able to survive across a crash, and it'd
> > sure be nice if it was indexed.  One option for such a KMS would be
> > something entirely external (which could potentially just be another PG
> > database or something) but it'd be nice if we had something built-in.
> > We might also want it to be replicated (or maybe we don't, as was
> > discussed on the call, to allow for a replica to use an independent set
> > of keys- of course that leads to issues with pg_rewind and such though).
>
> I think the replica could use a different key for the relations, but the
> WAL key would have to be the same.
>
> > Anything built-in does seem like it'd be a fair bit of work to get it to
> > address those requirements, but that does seem to be what the other
> > database systems have done.  Unfortunately, their documentation doesn't
> > seem to really say exactly what they've done to address that.
>
> I do like they pgcrypto key support to be per-database so pg_dump will
> dump the data encrypted, and with its locked keys.
>
> --
>   Bruce Momjian  http://momjian.us
>   EnterpriseDB http://enterprisedb.com
>
> + As you are, so once was I.  As I am, so you will be. +
> +  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Fri, Aug 23, 2019 at 10:04:13PM -0400, Stephen Frost wrote:
> > Well, I think they might do that to reduce encryption overhead.  I think
> > tests have shown that is not an issue, but we will need to test further.
> 
> I seriously doubt that's why and I don't think there's actually much
> value in trying to figure out the "why" here- the question is, do those
> systems answer the check-box requirement that was brought up on the call
> as the justification for this feature?  If so, then clearly not
> everything is required to be encrypted and we shouldn't be stressing
> over trying to do that.

We will stress in trying _not_ to encrypt everything.

> > I am not sure of the downside of encrypting everything, since it leaks
> > the least information and has a minimal user API and code impact.  What
> > is the value of encrypting only the user rows?  Better key control?
> 
> Yes, better key control, and better user API, and avoiding having an

Uh, there is no user API for all-cluster encryption except for the
administrator.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Stephen Frost]

Greetings,

* Bruce Momjian (br...@momjian.us) wrote:
> On Fri, Aug 23, 2019 at 10:35:17AM -0400, Stephen Frost wrote:
> > Following on from that- when other databases don't have something that
> > we're thinking about implementing, maybe we should be contemplating if
> > it really makes sense as a requirement for us.
> 
> Yes, that's a good point.
> 
> > Specifically in this case- I went back and tried to figure out what
> > other database systems have an "encrypt EVERYTHING" option.  I didn't
> > have much luck finding one though.  So I think we need to ask ourselves-
> > the "check box" that we're trying to check off with TDE, do the other
> > database system check that box?  If so, then it looks like the "check
> > box" isn't actually "encrypt EVERYTHING", it's more along the lines of
> > "make sure all regular user data is encrypted automatically" or some
> > such, and that's a very different requirement, which seems to be
> > answered by the other systems by having a KMS + tablespace/database
> > level encryption.  We certainly shouldn't be putting a lot of effort
> > into building something that is either overkill or won't be interesting
> > to users due to limitations like "have to take the entire cluster
> > offline to re-key it".
> 
> Well, I think they might do that to reduce encryption overhead.  I think
> tests have shown that is not an issue, but we will need to test further.

I seriously doubt that's why and I don't think there's actually much
value in trying to figure out the "why" here- the question is, do those
systems answer the check-box requirement that was brought up on the call
as the justification for this feature?  If so, then clearly not
everything is required to be encrypted and we shouldn't be stressing
over trying to do that.

> I am not sure of the downside of encrypting everything, since it leaks
> the least information and has a minimal user API and code impact.  What
> is the value of encrypting only the user rows?  Better key control?

Yes, better key control, and better user API, and avoiding having an
implementation that isn't actually what people either expect or want.  I
don't agree at all that this distinction has a "minimal user API
impact"- much of the reason we were throwing out the idea of having a
proper KMS for the "bulk data encryption", at least from what I gathered
on the call, is because of the issues around having to try and bootstrap
a fully encrypted system and deal with crash recovery and hypothesized
leaks.  If we can accept that it's alright for some data to be
unencrypted, then that certainly makes life easier for us, and from what
it looks like, that's pretty typical in industry.  I daresay it seems
likely that could get us all the way to table-level encryption of whole
tuples as discussed elsewhere.  I had a further side-chat with Sehrope
where I believe I explained why the concern regarding tids and ordering
isn't actually valid too, would be great if we could discuss that at
some point as well.  I'd be happy to chat with you about it first and
then if we agree, write up the discussion for the list as well.

> > Now, that KMS has to be encrypted using a master key, of course, and we
> > have to make sure that it is able to survive across a crash, and it'd
> > sure be nice if it was indexed.  One option for such a KMS would be
> > something entirely external (which could potentially just be another PG
> > database or something) but it'd be nice if we had something built-in.
> > We might also want it to be replicated (or maybe we don't, as was
> > discussed on the call, to allow for a replica to use an independent set
> > of keys- of course that leads to issues with pg_rewind and such though).
> 
> I think the replica could use a different key for the relations, but the
> WAL key would have to be the same.

This depends on how the WAL is sent to the replica-- if it's sent
unencrypted then the replica could have a different key, at least
potentially.  There are some very interesting questions around pg_rewind
support and archive_mode = always, but that's pretty far down the road
and we may have to tell the users that they have to make some choices
about if they want to have support for those features.

> > Anything built-in does seem like it'd be a fair bit of work to get it to
> > address those requirements, but that does seem to be what the other
> > database systems have done.  Unfortunately, their documentation doesn't
> > seem to really say exactly what they've done to address that.
> 
> I do like they pgcrypto key support to be per-database so pg_dump will
> dump the data encrypted, and with its locked keys.

Yes, a built-in KMS would also need pg_dump support.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Fri, Aug 23, 2019 at 10:35:17AM -0400, Stephen Frost wrote:
> > Agreed.  The features of other databases are a clear source for what we
> > should consider and run through the useful/reasonable filter.
> 
> Following on from that- when other databases don't have something that
> we're thinking about implementing, maybe we should be contemplating if
> it really makes sense as a requirement for us.

Yes, that's a good point.

> Specifically in this case- I went back and tried to figure out what
> other database systems have an "encrypt EVERYTHING" option.  I didn't
> have much luck finding one though.  So I think we need to ask ourselves-
> the "check box" that we're trying to check off with TDE, do the other
> database system check that box?  If so, then it looks like the "check
> box" isn't actually "encrypt EVERYTHING", it's more along the lines of
> "make sure all regular user data is encrypted automatically" or some
> such, and that's a very different requirement, which seems to be
> answered by the other systems by having a KMS + tablespace/database
> level encryption.  We certainly shouldn't be putting a lot of effort
> into building something that is either overkill or won't be interesting
> to users due to limitations like "have to take the entire cluster
> offline to re-key it".

Well, I think they might do that to reduce encryption overhead.  I think
tests have shown that is not an issue, but we will need to test further.
I am not sure of the downside of encrypting everything, since it leaks
the least information and has a minimal user API and code impact.  What
is the value of encrypting only the user rows?  Better key control?

> Now, that KMS has to be encrypted using a master key, of course, and we
> have to make sure that it is able to survive across a crash, and it'd
> sure be nice if it was indexed.  One option for such a KMS would be
> something entirely external (which could potentially just be another PG
> database or something) but it'd be nice if we had something built-in.
> We might also want it to be replicated (or maybe we don't, as was
> discussed on the call, to allow for a replica to use an independent set
> of keys- of course that leads to issues with pg_rewind and such though).

I think the replica could use a different key for the relations, but the
WAL key would have to be the same.

> Anything built-in does seem like it'd be a fair bit of work to get it to
> address those requirements, but that does seem to be what the other
> database systems have done.  Unfortunately, their documentation doesn't
> seem to really say exactly what they've done to address that.

I do like they pgcrypto key support to be per-database so pg_dump will
dump the data encrypted, and with its locked keys.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Stephen Frost]

Greetings,

* Bruce Momjian (br...@momjian.us) wrote:
> On Fri, Aug 23, 2019 at 07:45:22AM -0400, Stephen Frost wrote:
> > Having listed out the feature set of each of the other major databases
> > when it comes to TDE is exactly how we objectively look at what is being
> > done in the industry, and that then gives us an understanding of what
> > users (and auditors) coming from other platforms will expect.
> > 
> > I entirely agree that we shouldn't just copy N feature from X other
> > database system unless we feel that's the best approach, but when every
> > other database system out there has capability Y for the general feature
> > X that we're thinking about implementing, we should be questioning an
> > approach which doesn't include that.
> 
> Agreed.  The features of other databases are a clear source for what we
> should consider and run through the useful/reasonable filter.

Following on from that- when other databases don't have something that
we're thinking about implementing, maybe we should be contemplating if
it really makes sense as a requirement for us.

Specifically in this case- I went back and tried to figure out what
other database systems have an "encrypt EVERYTHING" option.  I didn't
have much luck finding one though.  So I think we need to ask ourselves-
the "check box" that we're trying to check off with TDE, do the other
database system check that box?  If so, then it looks like the "check
box" isn't actually "encrypt EVERYTHING", it's more along the lines of
"make sure all regular user data is encrypted automatically" or some
such, and that's a very different requirement, which seems to be
answered by the other systems by having a KMS + tablespace/database
level encryption.  We certainly shouldn't be putting a lot of effort
into building something that is either overkill or won't be interesting
to users due to limitations like "have to take the entire cluster
offline to re-key it".

Now, that KMS has to be encrypted using a master key, of course, and we
have to make sure that it is able to survive across a crash, and it'd
sure be nice if it was indexed.  One option for such a KMS would be
something entirely external (which could potentially just be another PG
database or something) but it'd be nice if we had something built-in.
We might also want it to be replicated (or maybe we don't, as was
discussed on the call, to allow for a replica to use an independent set
of keys- of course that leads to issues with pg_rewind and such though).

Anything built-in does seem like it'd be a fair bit of work to get it to
address those requirements, but that does seem to be what the other
database systems have done.  Unfortunately, their documentation doesn't
seem to really say exactly what they've done to address that.

A couple random ideas that probably won't work, but I'll put them out
there for others to shoot down-

Some kind of 2-phase WAL pass, where we do WAL replay for the
non-encrypted bits first (which would include the KMS) and then go back
and WAL replay the encrypted stuff.  Seems terrible.

An independent WAL for the KMS only.  Ugh, do we need another walwriter
then?  and buffers, and lots of other stuff.

Some kind of flat-file based approach with a temp file and renaming of
files using durable_rename(), like what we used to do with
pg_shadow/authid, and now do with replorigin_checkpoint and such?

Something else?

Thoughts?

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Fri, Aug 23, 2019 at 07:45:22AM -0400, Stephen Frost wrote:
> Greetings,
> 
> * Bruce Momjian (br...@momjian.us) wrote:
> > On Sat, Aug 17, 2019 at 01:52:17PM -0400, Stephen Frost wrote:
> > > Being PostgreSQL, I would expect us to shoot for as much flexibility as
> > > we possible, similar to what we've done for our ACL system where we
> > > support down to a column-level (and row level with RLS).
> > > 
> > > That's our target end-goal.  Having an incremental plan to get there
> > > where we start with something simpler and then work towards a more
> > > complicated implementation is fine- but that base, as I've said multiple
> > > times and as supported by what we see other database systems have,
> > > should include some kind of key store with support for multiple keys and
> > > a way to encrypt something less than the entire system.  Every other
> > > database system that we consider at all comparable has at least that.
> > 
> > Well, we don't blindly copy features from other databases.  The features
> > has to be useful for our users and reasonable to implement in Postgres. 
> > This is been the criteria for every other Postgres features I have seen
> > developed.
> 
> Having listed out the feature set of each of the other major databases
> when it comes to TDE is exactly how we objectively look at what is being
> done in the industry, and that then gives us an understanding of what
> users (and auditors) coming from other platforms will expect.
> 
> I entirely agree that we shouldn't just copy N feature from X other
> database system unless we feel that's the best approach, but when every
> other database system out there has capability Y for the general feature
> X that we're thinking about implementing, we should be questioning an
> approach which doesn't include that.

Agreed.  The features of other databases are a clear source for what we
should consider and run through the useful/reasonable filter.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Stephen Frost]

Greetings,

* Bruce Momjian (br...@momjian.us) wrote:
> On Sat, Aug 17, 2019 at 01:52:17PM -0400, Stephen Frost wrote:
> > Being PostgreSQL, I would expect us to shoot for as much flexibility as
> > we possible, similar to what we've done for our ACL system where we
> > support down to a column-level (and row level with RLS).
> > 
> > That's our target end-goal.  Having an incremental plan to get there
> > where we start with something simpler and then work towards a more
> > complicated implementation is fine- but that base, as I've said multiple
> > times and as supported by what we see other database systems have,
> > should include some kind of key store with support for multiple keys and
> > a way to encrypt something less than the entire system.  Every other
> > database system that we consider at all comparable has at least that.
> 
> Well, we don't blindly copy features from other databases.  The features
> has to be useful for our users and reasonable to implement in Postgres. 
> This is been the criteria for every other Postgres features I have seen
> developed.

Having listed out the feature set of each of the other major databases
when it comes to TDE is exactly how we objectively look at what is being
done in the industry, and that then gives us an understanding of what
users (and auditors) coming from other platforms will expect.

I entirely agree that we shouldn't just copy N feature from X other
database system unless we feel that's the best approach, but when every
other database system out there has capability Y for the general feature
X that we're thinking about implementing, we should be questioning an
approach which doesn't include that.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Sat, Aug 17, 2019 at 01:52:17PM -0400, Stephen Frost wrote:
> Being PostgreSQL, I would expect us to shoot for as much flexibility as
> we possible, similar to what we've done for our ACL system where we
> support down to a column-level (and row level with RLS).
> 
> That's our target end-goal.  Having an incremental plan to get there
> where we start with something simpler and then work towards a more
> complicated implementation is fine- but that base, as I've said multiple
> times and as supported by what we see other database systems have,
> should include some kind of key store with support for multiple keys and
> a way to encrypt something less than the entire system.  Every other
> database system that we consider at all comparable has at least that.

Well, we don't blindly copy features from other databases.  The features
has to be useful for our users and reasonable to implement in Postgres. 
This is been the criteria for every other Postgres features I have seen
developed.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Ahsan Hadi]

I have shared a calendar invite for TDE/KMS weekly meeting with the members
who expressed interest of joining the meeting in this chain. Hopefully I
haven't missed anyone.

I am not aware of everyone's timezone but I have tried to setup a time
that's not very inconvenient. It won't be ideal for everyone as we are
dealing with multiple timezone but do let me know It is too bizarre for you
and I will try to find another slot.

I will share a zoom link for the meeting on the invite in due course.

-- Ahsan


On Mon, Aug 19, 2019 at 9:26 AM Ahsan Hadi  wrote:

>
>
> On Mon, 19 Aug 2019 at 6:23 AM, Smith, Peter 
> wrote:
>
>> > From: Ibrar Ahmed  Sent: Sunday, 18 August 2019
>> 2:43 AM
>> > +1 for voice call, bruce we usually have a weekly TDE call. I will
>> include you in
>>
>> If you don't mind, please also add me to that TDE call list.
>>
>
> Sure will do.
>
>
>> Thanks/Regards,
>> ---
>> Peter Smith
>> Fujitsu Australia
>>
>

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Joe Conway]

On 8/19/19 8:51 AM, Ahsan Hadi wrote:
> I have shared a calendar invite for TDE/KMS weekly meeting with the
> members who expressed interest of joining the meeting in this chain.
> Hopefully I haven't missed anyone.
> 
> I am not aware of everyone's timezone but I have tried to setup a time
> that's not very inconvenient. It won't be ideal for everyone as we are
> dealing with multiple timezone but do let me know It is too bizarre for
> you and I will try to find another slot.    
> 
> I will share a zoom link for the meeting on the invite in due course.


Please add me as well. I would like to join when I can.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Ahsan Hadi]

On Mon, 19 Aug 2019 at 6:23 AM, Smith, Peter 
wrote:

> > From: Ibrar Ahmed  Sent: Sunday, 18 August 2019
> 2:43 AM
> > +1 for voice call, bruce we usually have a weekly TDE call. I will
> include you in
>
> If you don't mind, please also add me to that TDE call list.
>

Sure will do.


> Thanks/Regards,
> ---
> Peter Smith
> Fujitsu Australia
>

RE: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Smith, Peter]

> -Original Message-
> From: Stephen Frost  Sent: Friday, 16 August 2019 11:01 AM

> Having direct integration with a KMS would certainly be valuable, and I don't 
> see a reason to deny users that option if someone would like to spend time
> implementing it- in addition to a simpler mechanism such as a passphrase 
> command, which I believe is what was being suggested here.

Yes. We recently made an internal PoC for FEP to enable it to reach out to AWS 
KMS whenever the MKEY was rotated or TDKEY was created. This was achieved by 
inserting some hooks in our TDE code - these hooks were implemented by a 
contrib-module loaded by the shared_preload_libraries GUC variable. So when no 
special "tdekey_aws" module was loaded, our TDE functionality simply reverts to 
its default (random) MDEK/TDEK keys. 

Even if OSS community chooses not to implement any KMS integration, the TDE 
design could consider providing hooks in a few appropriate places to make it 
easy for people who may need to add their own later.

Regards,
---
Peter Smith
Fujitsu Australia

RE: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Smith, Peter]

> From: Ibrar Ahmed  Sent: Sunday, 18 August 2019 2:43 AM
> +1 for voice call, bruce we usually have a weekly TDE call. I will include 
> you in

If you don't mind, please also add me to that TDE call list.

Thanks/Regards,
---
Peter Smith
Fujitsu Australia

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Peter Eisentraut]

On 2019-08-17 08:16, Antonin Houska wrote:
> One problem that occurs to me is that PG may need to send some sort of
> credentials to the KMS. If it runs a separate process to execute the command,
> it needs to pass those credentials to it. Whether it does so via parameters or
> environment variables, both can be seen by other users.

You could do it via stdin or a file, perhaps.

Where would the PostgreSQL server ultimately get the KMS credentials from?

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

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Stephen Frost]

Greetings,

On Sat, Aug 17, 2019 at 18:30 Ahsan Hadi  wrote:

> The current calendar entry for TDE weekly call will not work for EST
> timezone. I will change the invite so we can accommodate people from
> multiple time zones.
>

I appreciate the thought but at least for my part, I already have regular
conference calls after midnight to support Asian and Australian time zones,
so I’m willing to work to support whatever has already been worked out.  (I
also won’t complain about a time that’s more convenient for everyone, of
course.)

Thanks!

Stephen

>

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Ahsan Hadi]

The current calendar entry for TDE weekly call will not work for EST
timezone. I will change the invite so we can accommodate people from
multiple time zones.

Stay tuned.


On Sun, 18 Aug 2019 at 2:29 AM, Sehrope Sarkuni  wrote:

> On Sat, Aug 17, 2019 at 12:43 PM Ibrar Ahmed 
> wrote:
>
>> +1 for voice call, bruce we usually have a weekly TDE call.
>>
>
> Please add me to the call as well. Thanks!
>
> Regards,
> -- Sehrope Sarkuni
> Founder & CEO | JackDB, Inc. | https://www.jackdb.com/
>
>

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Sehrope Sarkuni]

On Sat, Aug 17, 2019 at 12:43 PM Ibrar Ahmed  wrote:

> +1 for voice call, bruce we usually have a weekly TDE call.
>

Please add me to the call as well. Thanks!

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Stephen Frost]

Greetings,

* Bruce Momjian (br...@momjian.us) wrote:
> I will state whet I have already told some people privately, that for
> this feature, we have many people understanding 40% of the problem, but
> thinking they understand 90%.  I do agree we should plan for our
> eventual full feature set, but I can't figure out what that feature set
> looks like beyond full-cluster encryption, and no one is addressing my
> concerns to move that forward.  Vague complains that they don't like the
> process are not changing that.

I don't particularly care for these "40%" and "90%" characterizations
and I'm concerned that some might also, reasonably, find that to be a
way to dismiss the opinions and comments from anyone who isn't in the
clearly subjective "90%" crowd.

Regarding what the eventual feature-set is, I believe it's abundently
clear where we want to eventually go and it's surprising to me that it's
unclear- we should be aiming for parity with the other major database
vendors when it comes to TDE.  That's pretty clear and straight-forward
to define, as well, and as facts:

Oracle:
- Supports column-level and tablespace-level.
- Has a Master Encryption Key (MEK), and table keys.
- Supports having the MEK be external to the database system.
- For tablespaces, can also use an external key store with
  different keys for different tablespaces.
- Supports Triple-DES and AES (128, 192, 256 bit)
- Supports a NOMAC parameter to improve performance.
- Has a mechanism for changing the keys/algorithms for tables
  with encrypted columns.

SQL Server:
- Supports database-level encryption
- Has a Instance master key and a per-database master key
- Includes a key store for having other keys
- Provides a function-based approach for encrypting at a column level
  (imagine pgcrypto, but where the key can be pulled from a key-store in
  the database which has to be unlocked)

DB2:
- Supports a Master Key and a Data Encryption Key
- Support encryption at a per-database level

Sybase:
- Supports a key encryption key
- Supports column level encryption with column encryption keys

MySQL:
- Supports a master encryption key
- Supports having the master key in an external data store which speaks
  Oasis KMIP
- Supports per-tablespace encryption
- Supports per-table encryption

Every one of the database systems above uses at least a two-tier system
(SQL server seems to possibly support three-tier) where there is a MEK
and then multiple keys under the MEK to allow partial encryption of the
system, at *least* at a database or tablespace level but a number go
down to column-level, either directly or using a function-based approach
with a key store.

Every one has some kind of key store, and a number support an external
key store.

There is not one that uses a single key or which requires that the
enctire instance be encrypted.

Being PostgreSQL, I would expect us to shoot for as much flexibility as
we possible, similar to what we've done for our ACL system where we
support down to a column-level (and row level with RLS).

That's our target end-goal.  Having an incremental plan to get there
where we start with something simpler and then work towards a more
complicated implementation is fine- but that base, as I've said multiple
times and as supported by what we see other database systems have,
should include some kind of key store with support for multiple keys and
a way to encrypt something less than the entire system.  Every other
database system that we consider at all comparable has at least that.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Stephen Frost]

Greetings,

* Bruce Momjian (br...@momjian.us) wrote:
> On Sat, Aug 17, 2019 at 08:16:06AM +0200, Antonin Houska wrote:
> > Bruce Momjian  wrote:
> > 
> > > On Thu, Aug 15, 2019 at 09:01:05PM -0400, Stephen Frost wrote:
> > > > * Bruce Momjian (br...@momjian.us) wrote:
> > > > > Why would it not be simpler to have the cluster_passphrase_command run
> > > > > whatever command-line program it wants?  If you don't want to use a
> > > > > shell command, create an executable and call that.
> > > > 
> > > > Having direct integration with a KMS would certainly be valuable, and I
> > > > don't see a reason to deny users that option if someone would like to
> > > > spend time implementing it- in addition to a simpler mechanism such as a
> > > > passphrase command, which I believe is what was being suggested here.
> > > 
> > > OK,  I am just trying to see why we would not use the
> > > cluster_passphrase_command-like interface to do that.
> > 
> > One problem that occurs to me is that PG may need to send some sort of
> > credentials to the KMS. If it runs a separate process to execute the 
> > command,
> > it needs to pass those credentials to it. Whether it does so via parameters 
> > or
> > environment variables, both can be seen by other users.
> 
> Yes, that would be a good reason to use an external library, if we can't
> figure out a clean API like opening a pipe into the command-line tool
> and piping in the secret.

Having to install something additional to make that whole mechanism
happen would also be less than ideal, imv.  That includes even something
as install-X and then configure passphrase_command.  Our experience with
archive_command shows that it really isn't a very good approach, even
when everything can be passed in on a command line.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Stephen Frost]

Greetings,

* Ibrar Ahmed (ibrar.ah...@gmail.com) wrote:
> On Sat, Aug 17, 2019 at 3:04 AM Bruce Momjian  wrote:
> > +1 for voice call, bruce we usually have a weekly TDE call. I will include
> you in
> that call.  Currently, in that group are

> moon_insung...@lab.ntt.co.jp,
> sawada.m...@gmail.com,
> shawn.w...@highgo.ca,
> ahsan.h...@highgo.ca,
> ibrar.ah...@gmail.com
> 
> I am able to do that for others as well.

If you could add me to that call, I'll do my best to attend.

(If it's a gmail calendar invite, please send to frost.stephen.p @
gmail).

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Ibrar Ahmed]

On Sat, Aug 17, 2019 at 3:04 AM Bruce Momjian  wrote:

> On Fri, Aug 16, 2019 at 07:47:37PM +0200, Antonin Houska wrote:
> > Bruce Momjian  wrote:
> >
> > > I have seen no one present a clear description of how anything beyond
> > > all-cluster encryption would work or be secure.  Wishing that were not
> > > the case doesn't change things.
> >
> > Since this email thread has grown a lot and is difficult to follow, it
> might
> > help if we summarized various approaches on the wiki, with their pros and
> > cons, and included some links to the corresponding emails in the
> > archive. There might be people who would like think about the problems
> but
> > don't have time to read the whole thread. Overview of the pending
> problems of
> > particular approaches might be useful for newcomers, but also for people
> who
> > followed only part of the discussion. I mean an overview of the storage
> > problems; the key management seems to be less controversial.
> >
> > If you think it makes sense, I can spend some time next week on the
> > research. However I'd need at least an outline of the approaches proposed
> > because I also missed some parts of the thread.
>
> I suggest we schedule a voice call and I will go over all the issues and
> explain why I came to the conclusions listed.  It is hard to know what
> level of detail to explain that in an email, beyond what I have already
> posted on this thread.  The only other options is to read all the emails
> _I_ sent on the thread to get an idea.
>
> +1 for voice call, bruce we usually have a weekly TDE call. I will include
you in
that call.  Currently, in that group are
moon_insung...@lab.ntt.co.jp,
sawada.m...@gmail.com,
shawn.w...@highgo.ca,
ahsan.h...@highgo.ca,
ibrar.ah...@gmail.com

I am able to do that for others as well.
>
> --
>   Bruce Momjian  http://momjian.us
>   EnterpriseDB http://enterprisedb.com
>
> + As you are, so once was I.  As I am, so you will be. +
> +  Ancient Roman grave inscription +
>


-- 
Ibrar Ahmed

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Sat, Aug 17, 2019 at 08:16:06AM +0200, Antonin Houska wrote:
> Bruce Momjian  wrote:
> 
> > On Thu, Aug 15, 2019 at 09:01:05PM -0400, Stephen Frost wrote:
> > > * Bruce Momjian (br...@momjian.us) wrote:
> > > > Why would it not be simpler to have the cluster_passphrase_command run
> > > > whatever command-line program it wants?  If you don't want to use a
> > > > shell command, create an executable and call that.
> > > 
> > > Having direct integration with a KMS would certainly be valuable, and I
> > > don't see a reason to deny users that option if someone would like to
> > > spend time implementing it- in addition to a simpler mechanism such as a
> > > passphrase command, which I believe is what was being suggested here.
> > 
> > OK,  I am just trying to see why we would not use the
> > cluster_passphrase_command-like interface to do that.
> 
> One problem that occurs to me is that PG may need to send some sort of
> credentials to the KMS. If it runs a separate process to execute the command,
> it needs to pass those credentials to it. Whether it does so via parameters or
> environment variables, both can be seen by other users.

Yes, that would be a good reason to use an external library, if we can't
figure out a clean API like opening a pipe into the command-line tool
and piping in the secret.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Fri, Aug 16, 2019 at 06:04:39PM -0400, Bruce Momjian wrote:
> I suggest we schedule a voice call and I will go over all the issues and
> explain why I came to the conclusions listed.  It is hard to know what
> level of detail to explain that in an email, beyond what I have already
> posted on this thread.  The only other options is to read all the emails
> _I_ sent on the thread to get an idea.
> 
> I am able to do that for others as well.  

Also, people can certainly ask questions on this list, and I can answer
them, or I can do a Skype/Zoom/IRC chat/call if people want.  The points
of complexity are really the amount of data encrypted, the number of
keys and who controls them, and performance overhead.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Antonin Houska]

Bruce Momjian  wrote:

> On Thu, Aug 15, 2019 at 09:01:05PM -0400, Stephen Frost wrote:
> > * Bruce Momjian (br...@momjian.us) wrote:
> > > Why would it not be simpler to have the cluster_passphrase_command run
> > > whatever command-line program it wants?  If you don't want to use a
> > > shell command, create an executable and call that.
> > 
> > Having direct integration with a KMS would certainly be valuable, and I
> > don't see a reason to deny users that option if someone would like to
> > spend time implementing it- in addition to a simpler mechanism such as a
> > passphrase command, which I believe is what was being suggested here.
> 
> OK,  I am just trying to see why we would not use the
> cluster_passphrase_command-like interface to do that.

One problem that occurs to me is that PG may need to send some sort of
credentials to the KMS. If it runs a separate process to execute the command,
it needs to pass those credentials to it. Whether it does so via parameters or
environment variables, both can be seen by other users.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Fri, Aug 16, 2019 at 07:47:37PM +0200, Antonin Houska wrote:
> Bruce Momjian  wrote:
> 
> > I have seen no one present a clear description of how anything beyond
> > all-cluster encryption would work or be secure.  Wishing that were not
> > the case doesn't change things.
> 
> Since this email thread has grown a lot and is difficult to follow, it might
> help if we summarized various approaches on the wiki, with their pros and
> cons, and included some links to the corresponding emails in the
> archive. There might be people who would like think about the problems but
> don't have time to read the whole thread. Overview of the pending problems of
> particular approaches might be useful for newcomers, but also for people who
> followed only part of the discussion. I mean an overview of the storage
> problems; the key management seems to be less controversial.
> 
> If you think it makes sense, I can spend some time next week on the
> research. However I'd need at least an outline of the approaches proposed
> because I also missed some parts of the thread.

I suggest we schedule a voice call and I will go over all the issues and
explain why I came to the conclusions listed.  It is hard to know what
level of detail to explain that in an email, beyond what I have already
posted on this thread.  The only other options is to read all the emails
_I_ sent on the thread to get an idea.

I am able to do that for others as well.  

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Antonin Houska]

Bruce Momjian  wrote:

> I have seen no one present a clear description of how anything beyond
> all-cluster encryption would work or be secure.  Wishing that were not
> the case doesn't change things.

Since this email thread has grown a lot and is difficult to follow, it might
help if we summarized various approaches on the wiki, with their pros and
cons, and included some links to the corresponding emails in the
archive. There might be people who would like think about the problems but
don't have time to read the whole thread. Overview of the pending problems of
particular approaches might be useful for newcomers, but also for people who
followed only part of the discussion. I mean an overview of the storage
problems; the key management seems to be less controversial.

If you think it makes sense, I can spend some time next week on the
research. However I'd need at least an outline of the approaches proposed
because I also missed some parts of the thread.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Fri, Aug 16, 2019 at 05:58:59PM +0500, Ibrar Ahmed wrote:
> 
> 
> On Thu, Aug 15, 2019 at 8:21 PM Bruce Momjian  wrote:
> 
> On Thu, Aug 15, 2019 at 11:24:46AM +0200, Antonin Houska wrote:
> > > I think there are several directions we can go after all-cluster
> > > encryption,
> >
> > I think I misunderstood. What you summarize in
> >
> > https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#
> TODO_for_Full-Cluster_Encryption
> >
> 
> Do we have any status of TODO's, what has been done and what left? It's much
> better if we have a link of discussion of each item.

I think some are done and some are in process, but I don't have a good
handle on that yet.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Thu, Aug 15, 2019 at 09:01:05PM -0400, Stephen Frost wrote:
> * Bruce Momjian (br...@momjian.us) wrote:
> > I assume you are talking about my option #1.  I can see if you only need
> > a few tables encrypted, e.g., credit card numbers, it can be excessive
> > to encrypt the entire cluster.  (I think you would need to encrypt
> > pg_statistic too.)
> 
> Or we would need a seperate encrypted pg_statistic, or a way to encrypt
> certain entries inside pg_statistic.

Yes.

> > The tricky part will be WAL --- if we encrypt all of WAL, the per-table
> > overhead might be minimal compared to the WAL encryption overhead.  The
> > better solution would be to add a flag to WAL records to indicate
> > encrypted entries, but you would then leak when an encryption change
> > happens and WAL record length.  (FYI, numeric values have different
> > lengths, as do character strings.)  I assume we would still use a single
> > key for all tables/indexes, and one for WAL, plus key rotation
> > requirements.
> 
> I don't think the fact that a change was done to an encrypted blob is an
> actual 'leak'- anyone can tell that by looking at the at the encrypted
> data before and after.  Further, the actual change would be encrypted,
> right?  Length of data is necessary to include in the vast majority of
> cases that the data is being dealt with and so I'm not sure that it
> makes sense for us to be worrying about that as a leak, unless you have
> a specific recommendation from a well known source discussing that
> concern..?

Yes, it is a minor negative, but we would need to see some performance
reason to have that minor negative, and I have already stated why I
think there might be no performance reason to do so.  Masahiko Sawada
talk at PGCon 2019 supports that conclusion:

https://www.youtube.com/watch?v=TXKoo2SNMzk

> > I personally would like to see full cluster implemented first to find
> > out exactly what the overhead is.  As I stated earlier, the overhead of
> > determining which things to encrypt, both in code complexity, user
> > interface, and processing overhead, might not be worth it.
> 
> I disagree with this and feel that the overhead that's being discussed
> here (user interface, figuring out if we should encrypt it or not,
> processing overhead for those determinations) is along the lines of
> UNLOGGED tables, yet there wasn't any question about if that was a valid
> or useful feature to implement.  The biggest challenge here is really

We implemented UNLOGGED tables because where was a clear performance win
to doing so.  I have not seen any measurements for encryption,
particularly when WAL is considered.

> around key management and I agree that's difficult but it's also really
> important and something that we need to be thinking about- and thinking
> about how to work with multiple keys and not just one.  Building in an
> assumption that we will only ever work with one key would make this
> capability nothing more than DBA-managed filesystem-level encryption

Agreed, that's all it is.

> (though even there different tablespaces could have different keys...)
> and I worry would make later work to support multiple keys more
> difficult and less likely to actually happen.  It's also not clear to me
> why we aren't building in *some* mechanism to work with multiple keys
> from the start as part of the initial design.

Well, every time I look at multiple keys, I go over exactly what that
means and how it behaves, but get no feedback on how to address the
problems.

> > I can see why you would think that encrypting less would be easier than
> > encrypting more, but security boundaries are hard to construct, and
> > anything that requires a user API, even more so.
> 
> I'm not sure I'm follwing here- I'm pretty sure everyone understands
> that selective encryption will require more work to implement, in part
> because an API needs to be put in place and we need to deal with
> multiple keys, etc.  I don't think anyone thinks that'll be "easier".

Uh, I thought Masahiko Sawada stated but, but looking back, I don't see
it, so I must be wrong.

> > > > > At least it should be clear how [2] will retrieve the master key 
> > > > > because [1]
> > > > > should not do it in a differnt way. (The GUC 
> > > > > cluster_passphrase_command
> > > > > mentioned in [3] seems viable, although I think [1] uses approach 
> > > > > which is
> > > > > more convenient if the passphrase should be read from console.)
> > > 
> > > I think that we can also provide a way to pass encryption key directly
> > > to postmaster rather than using passphrase. Since it's common that
> > > user stores keys in KMS it's useful if we can do that.
> > 
> > Why would it not be simpler to have the cluster_passphrase_command run
> > whatever command-line program it wants?  If you don't want to use a
> > shell command, create an executable and call that.
> 
> Having direct integration with a KMS would certainly be valuable, and I
> don't see a 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Ibrar Ahmed]

On Thu, Aug 15, 2019 at 8:21 PM Bruce Momjian  wrote:

> On Thu, Aug 15, 2019 at 11:24:46AM +0200, Antonin Houska wrote:
> > > I think there are several directions we can go after all-cluster
> > > encryption,
> >
> > I think I misunderstood. What you summarize in
> >
> >
> https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#TODO_for_Full-Cluster_Encryption
> >
>
Do we have any status of TODO's, what has been done and what left? It's
much better if we have a link of discussion of each item.



> > does include
> >
> >
> https://www.postgresql.org/message-id/CAD21AoBjrbxvaMpTApX1cEsO=8N=nc2xVZPB0d9e-VjJ=ya...@mail.gmail.com
> >
> > i.e. per-tablespace keys, right? Then the collaboration should be easier
> than
> > I thought.
>
> No, there is a single tables/indexes key and a WAL key, plus keys for
> rotation.  I explained why per-tablespace keys don't add much value.
>
> --
>   Bruce Momjian  http://momjian.us
>   EnterpriseDB http://enterprisedb.com
>
> + As you are, so once was I.  As I am, so you will be. +
> +  Ancient Roman grave inscription +
>


-- 
Ibrar Ahmed

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Masahiko Sawada]

On Fri, Aug 16, 2019 at 10:01 AM Stephen Frost  wrote:
>
> Greetings,
>
> * Bruce Momjian (br...@momjian.us) wrote:
> > On Thu, Aug 15, 2019 at 06:10:24PM +0900, Masahiko Sawada wrote:
> > > On Thu, Aug 15, 2019 at 10:19 AM Bruce Momjian  wrote:
> > > >
> > > > On Wed, Aug 14, 2019 at 04:36:35PM +0200, Antonin Houska wrote:
> > > > > I can work on it right away but don't know where to start.
> > > >
> > > > I think the big open question is whether there will be acceptance of an
> > > > all-cluster encyption feature.  I guess if no one objects, we can move
> > > > forward.
> > >
> > > I still feel that we need to have per table/tablespace keys although
> > > it might not be the first implementation. I think the safeness of both
> > > table/tablespace level and cluster level would be almost the same but
> > > the former would have an advantage in terms of operation and
> > > performance.
> >
> > I assume you are talking about my option #1.  I can see if you only need
> > a few tables encrypted, e.g., credit card numbers, it can be excessive
> > to encrypt the entire cluster.  (I think you would need to encrypt
> > pg_statistic too.)
>
> Or we would need a seperate encrypted pg_statistic, or a way to encrypt
> certain entries inside pg_statistic.

I think we also need to encrypt other system catalogs. For instance
pg_procs might also have sensitive data in prosrc column. So I think
it's better to encrypt all system catalogs rather than picking up some
catalogs since it would not be a big overhead. Since system catalogs
are created during CREATE DATABASE by copying files tablespace level
or database level encryption would be well suited with system catalog
encryption.

>
> > The tricky part will be WAL --- if we encrypt all of WAL, the per-table
> > overhead might be minimal compared to the WAL encryption overhead.  The
> > better solution would be to add a flag to WAL records to indicate
> > encrypted entries, but you would then leak when an encryption change
> > happens and WAL record length.  (FYI, numeric values have different
> > lengths, as do character strings.)  I assume we would still use a single
> > key for all tables/indexes, and one for WAL, plus key rotation
> > requirements.
>
> I don't think the fact that a change was done to an encrypted blob is an
> actual 'leak'- anyone can tell that by looking at the at the encrypted
> data before and after.  Further, the actual change would be encrypted,
> right?  Length of data is necessary to include in the vast majority of
> cases that the data is being dealt with and so I'm not sure that it
> makes sense for us to be worrying about that as a leak, unless you have
> a specific recommendation from a well known source discussing that
> concern..?
>
> > I personally would like to see full cluster implemented first to find
> > out exactly what the overhead is.  As I stated earlier, the overhead of
> > determining which things to encrypt, both in code complexity, user
> > interface, and processing overhead, might not be worth it.
>
> I disagree with this and feel that the overhead that's being discussed
> here (user interface, figuring out if we should encrypt it or not,
> processing overhead for those determinations) is along the lines of
> UNLOGGED tables, yet there wasn't any question about if that was a valid
> or useful feature to implement.  The biggest challenge here is really
> around key management and I agree that's difficult but it's also really
> important and something that we need to be thinking about- and thinking
> about how to work with multiple keys and not just one.  Building in an
> assumption that we will only ever work with one key would make this
> capability nothing more than DBA-managed filesystem-level encryption
> (though even there different tablespaces could have different keys...)
> and I worry would make later work to support multiple keys more
> difficult and less likely to actually happen.  It's also not clear to me
> why we aren't building in *some* mechanism to work with multiple keys
> from the start as part of the initial design.
>
> > I can see why you would think that encrypting less would be easier than
> > encrypting more, but security boundaries are hard to construct, and
> > anything that requires a user API, even more so.
>
> I'm not sure I'm follwing here- I'm pretty sure everyone understands
> that selective encryption will require more work to implement, in part
> because an API needs to be put in place and we need to deal with
> multiple keys, etc.  I don't think anyone thinks that'll be "easier".
>
> > > > > At least it should be clear how [2] will retrieve the master key 
> > > > > because [1]
> > > > > should not do it in a differnt way. (The GUC 
> > > > > cluster_passphrase_command
> > > > > mentioned in [3] seems viable, although I think [1] uses approach 
> > > > > which is
> > > > > more convenient if the passphrase should be read from console.)
> > >
> > > I think that we can also provide a way to 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Stephen Frost]

Greetings,

* Bruce Momjian (br...@momjian.us) wrote:
> On Thu, Aug 15, 2019 at 06:10:24PM +0900, Masahiko Sawada wrote:
> > On Thu, Aug 15, 2019 at 10:19 AM Bruce Momjian  wrote:
> > >
> > > On Wed, Aug 14, 2019 at 04:36:35PM +0200, Antonin Houska wrote:
> > > > I can work on it right away but don't know where to start.
> > >
> > > I think the big open question is whether there will be acceptance of an
> > > all-cluster encyption feature.  I guess if no one objects, we can move
> > > forward.
> > 
> > I still feel that we need to have per table/tablespace keys although
> > it might not be the first implementation. I think the safeness of both
> > table/tablespace level and cluster level would be almost the same but
> > the former would have an advantage in terms of operation and
> > performance.
> 
> I assume you are talking about my option #1.  I can see if you only need
> a few tables encrypted, e.g., credit card numbers, it can be excessive
> to encrypt the entire cluster.  (I think you would need to encrypt
> pg_statistic too.)

Or we would need a seperate encrypted pg_statistic, or a way to encrypt
certain entries inside pg_statistic.

> The tricky part will be WAL --- if we encrypt all of WAL, the per-table
> overhead might be minimal compared to the WAL encryption overhead.  The
> better solution would be to add a flag to WAL records to indicate
> encrypted entries, but you would then leak when an encryption change
> happens and WAL record length.  (FYI, numeric values have different
> lengths, as do character strings.)  I assume we would still use a single
> key for all tables/indexes, and one for WAL, plus key rotation
> requirements.

I don't think the fact that a change was done to an encrypted blob is an
actual 'leak'- anyone can tell that by looking at the at the encrypted
data before and after.  Further, the actual change would be encrypted,
right?  Length of data is necessary to include in the vast majority of
cases that the data is being dealt with and so I'm not sure that it
makes sense for us to be worrying about that as a leak, unless you have
a specific recommendation from a well known source discussing that
concern..?

> I personally would like to see full cluster implemented first to find
> out exactly what the overhead is.  As I stated earlier, the overhead of
> determining which things to encrypt, both in code complexity, user
> interface, and processing overhead, might not be worth it.

I disagree with this and feel that the overhead that's being discussed
here (user interface, figuring out if we should encrypt it or not,
processing overhead for those determinations) is along the lines of
UNLOGGED tables, yet there wasn't any question about if that was a valid
or useful feature to implement.  The biggest challenge here is really
around key management and I agree that's difficult but it's also really
important and something that we need to be thinking about- and thinking
about how to work with multiple keys and not just one.  Building in an
assumption that we will only ever work with one key would make this
capability nothing more than DBA-managed filesystem-level encryption
(though even there different tablespaces could have different keys...)
and I worry would make later work to support multiple keys more
difficult and less likely to actually happen.  It's also not clear to me
why we aren't building in *some* mechanism to work with multiple keys
from the start as part of the initial design.

> I can see why you would think that encrypting less would be easier than
> encrypting more, but security boundaries are hard to construct, and
> anything that requires a user API, even more so.

I'm not sure I'm follwing here- I'm pretty sure everyone understands
that selective encryption will require more work to implement, in part
because an API needs to be put in place and we need to deal with
multiple keys, etc.  I don't think anyone thinks that'll be "easier".

> > > > At least it should be clear how [2] will retrieve the master key 
> > > > because [1]
> > > > should not do it in a differnt way. (The GUC cluster_passphrase_command
> > > > mentioned in [3] seems viable, although I think [1] uses approach which 
> > > > is
> > > > more convenient if the passphrase should be read from console.)
> > 
> > I think that we can also provide a way to pass encryption key directly
> > to postmaster rather than using passphrase. Since it's common that
> > user stores keys in KMS it's useful if we can do that.
> 
> Why would it not be simpler to have the cluster_passphrase_command run
> whatever command-line program it wants?  If you don't want to use a
> shell command, create an executable and call that.

Having direct integration with a KMS would certainly be valuable, and I
don't see a reason to deny users that option if someone would like to
spend time implementing it- in addition to a simpler mechanism such as a
passphrase command, which I believe is what was being suggested here.

> > > > 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Stephen Frost]

Greetings,

* Bruce Momjian (br...@momjian.us) wrote:
> On Thu, Aug 15, 2019 at 11:24:46AM +0200, Antonin Houska wrote:
> > > I think there are several directions we can go after all-cluster
> > > encryption,
> > 
> > I think I misunderstood. What you summarize in
> > 
> > https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#TODO_for_Full-Cluster_Encryption
> > 
> > does include
> > 
> > https://www.postgresql.org/message-id/CAD21AoBjrbxvaMpTApX1cEsO=8N=nc2xVZPB0d9e-VjJ=ya...@mail.gmail.com
> > 
> > i.e. per-tablespace keys, right? Then the collaboration should be easier 
> > than
> > I thought.
> 
> No, there is a single tables/indexes key and a WAL key, plus keys for
> rotation.  I explained why per-tablespace keys don't add much value.

Nothing in the discussion that I've seen, at least, has changed my
opinion that tablespace-based keys *would* add significant value,
particularly if it'd be difficult to support per-table keys.  Of course,
if we can get per-table keys without too much difficulty then that would
be better.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Thu, Aug 15, 2019 at 11:24:46AM +0200, Antonin Houska wrote:
> > I think there are several directions we can go after all-cluster
> > encryption,
> 
> I think I misunderstood. What you summarize in
> 
> https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#TODO_for_Full-Cluster_Encryption
> 
> does include
> 
> https://www.postgresql.org/message-id/CAD21AoBjrbxvaMpTApX1cEsO=8N=nc2xVZPB0d9e-VjJ=ya...@mail.gmail.com
> 
> i.e. per-tablespace keys, right? Then the collaboration should be easier than
> I thought.

No, there is a single tables/indexes key and a WAL key, plus keys for
rotation.  I explained why per-tablespace keys don't add much value.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Thu, Aug 15, 2019 at 06:10:24PM +0900, Masahiko Sawada wrote:
> On Thu, Aug 15, 2019 at 10:19 AM Bruce Momjian  wrote:
> >
> > On Wed, Aug 14, 2019 at 04:36:35PM +0200, Antonin Houska wrote:
> > > I can work on it right away but don't know where to start.
> >
> > I think the big open question is whether there will be acceptance of an
> > all-cluster encyption feature.  I guess if no one objects, we can move
> > forward.
> >
> 
> I still feel that we need to have per table/tablespace keys although
> it might not be the first implementation. I think the safeness of both
> table/tablespace level and cluster level would be almost the same but
> the former would have an advantage in terms of operation and
> performance.

I assume you are talking about my option #1.  I can see if you only need
a few tables encrypted, e.g., credit card numbers, it can be excessive
to encrypt the entire cluster.  (I think you would need to encrypt
pg_statistic too.)

The tricky part will be WAL --- if we encrypt all of WAL, the per-table
overhead might be minimal compared to the WAL encryption overhead.  The
better solution would be to add a flag to WAL records to indicate
encrypted entries, but you would then leak when an encryption change
happens and WAL record length.  (FYI, numeric values have different
lengths, as do character strings.)  I assume we would still use a single
key for all tables/indexes, and one for WAL, plus key rotation
requirements.

I personally would like to see full cluster implemented first to find
out exactly what the overhead is.  As I stated earlier, the overhead of
determining which things to encrypt, both in code complexity, user
interface, and processing overhead, might not be worth it.

I can see why you would think that encrypting less would be easier than
encrypting more, but security boundaries are hard to construct, and
anything that requires a user API, even more so.

> > > At least it should be clear how [2] will retrieve the master key because 
> > > [1]
> > > should not do it in a differnt way. (The GUC cluster_passphrase_command
> > > mentioned in [3] seems viable, although I think [1] uses approach which is
> > > more convenient if the passphrase should be read from console.)
> 
> I think that we can also provide a way to pass encryption key directly
> to postmaster rather than using passphrase. Since it's common that
> user stores keys in KMS it's useful if we can do that.

Why would it not be simpler to have the cluster_passphrase_command run
whatever command-line program it wants?  If you don't want to use a
shell command, create an executable and call that.

> > > Rotation of
> > > the master key is another thing that both versions of the feature should 
> > > do in
> > > the same way. And of course, the fronend applications need consistent 
> > > approach
> > > too.
> >
> > I don't see the value of an external library for key storage.
> 
> I think that big benefit is that PostgreSQL can seamlessly work with
> external services such as KMS. For instance, when key rotation,
> PostgreSQL can register new key to KMS and use it, and it can remove
> keys when it no longer necessary. That is, it can enable PostgreSQL to
> not only not only getting key from KMS but also registering and
> removing keys. And we also can decrypt MDEK in KMS instead of doing in
> PostgreSQL which is more safety. In addition, once someone create the
> plugin library of an external services individual projects don't need
> to create that.

I think the big win for an external library is when you don't want the
overhead of calling an external program.  For example, we certainly
would not want to call an external program while processing a query.  Do
we have any such requirements for encryption, especially since we only
are going to allow offline mode for encryption mode changes and key
rotation in the first version?

> BTW I've created PoC patch for cluster encryption feature. Attached
> patch set has done some items of TODO list and some of them can be
> used even for finer granularity encryption. Anyway, the implemented
> components are followings:

Nice, thanks.

> * Initialization stuff (initdb support). initdb has new command line
> options: --enc-cipher and --cluster-passphrase-command. --enc-cipher
> option accepts either aes-128 or aes-256 values while
> --cluster-passphrase-command accepts an arbitrary command. ControlFile
> has an integer indicating cluster encryption support, 'off', 'aes-128'
> or 'aes-256'.

Nice.  If we get agreement we want to do this for PG 13, we can start
applying these patches.

> * 3-tier encryption keys. During initdb we create KEK and MDEK and
> write the meta data file(global/pg_kmgr file). When postmaster startup
> it reads the kmgr file, verifies the passphrase using HMAC, unwraps
> MDEK and derives TDEK and WDEK from MDEK. Currently MDEK, TDEK and
> WDEK are stored into shared memory as this is still PoC but we also
> can have them in process local memory.

Uh, I thought 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Wed, Aug 14, 2019 at 09:19:44PM -0400, Bruce Momjian wrote:
> I think there are several directions we can go after all-cluster
> encryption, and it does matter because we would want minimal API
> breakage.  The options are:
> 
> 1)  Allow per-table encyption control to limit encryption overhead,
> though all of WAL still needs to be encrypted;  we could add a
> per-record encyption flag to WAL records to avoid that.
> 
> 2)  Allow user-controlled keys, which are always unlocked, and encrypt
> WAL with one key
> 
> 3)  Encrypt only the user-data portion of pages with user-controlled
> keys.  FREEZE and crash recovery work since only the user data is
> encrypted.  WAL is not encrypted, except for the user-data portion
> 
...
> I don't think #3 is viable since there is too much information leakage,
> particularly for indexes because the tid is visible.

Thinking some more, it might be possible to encrypt the index tid and
for crash recovery and the freeze part of vacuum to work, which might be
sufficient to allow the user keys to remain locked.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Antonin Houska]

Bruce Momjian  wrote:

> On Wed, Aug 14, 2019 at 04:36:35PM +0200, Antonin Houska wrote:
> > I can work on it right away but don't know where to start.
> 
> I think the big open question is whether there will be acceptance of an
> all-cluster encyption feature.  I guess if no one objects, we can move
> forward.
> 
> > First, I think we should use a code repository to integrate [1] and [2]
> > instead of sending diffs back and forth. That would force us to resolve
> > conflicts soon and help to avoid duplicate work. The diffs would be created
> > only whe we need to post the next patch version to pgsql-hackers for review,
> > otherwise the discussions of details can take place elsewhere.
> 
> Well, we can do that, or just follow the TODO list and apply items as we
> complete them.  We have found that doing everything in one big patch is
> just too hard to review and get accepted.
> 
> > The most difficult problem I see now regarding the collaboration is 
> > agreement
> > on the key management user interface. The Full-Cluster Encryption feature 
> > [1]
> > should not add configuration variables or even tools that the next, more
> > sophisticated version [2] deprecates immediately. Part of the problem is 
> > that
> 
> Yes, the all-cluster encryption feature has _no_ SQL-level API to
> control it, just a GUC variable that you can use SHOW to see the
> encryption mode.
> 
> > [2] puts all (key management related) interaction of postgres with the
> > environment into an external library. As I pointed out in my response to 
> > [2],
> > this will not work for frontend applications (e.g. pg_waldump). I think the
> > key management UI for [2] needs to be designed first even if PG 13 should
> > adopt only [1].
> 
> I think there are several directions we can go after all-cluster
> encryption,

I think I misunderstood. What you summarize in

https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#TODO_for_Full-Cluster_Encryption

does include

https://www.postgresql.org/message-id/CAD21AoBjrbxvaMpTApX1cEsO=8N=nc2xVZPB0d9e-VjJ=ya...@mail.gmail.com

i.e. per-tablespace keys, right? Then the collaboration should be easier than
I thought.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Masahiko Sawada]

On Thu, Aug 15, 2019 at 10:19 AM Bruce Momjian  wrote:
>
> On Wed, Aug 14, 2019 at 04:36:35PM +0200, Antonin Houska wrote:
> > I can work on it right away but don't know where to start.
>
> I think the big open question is whether there will be acceptance of an
> all-cluster encyption feature.  I guess if no one objects, we can move
> forward.
>

I still feel that we need to have per table/tablespace keys although
it might not be the first implementation. I think the safeness of both
table/tablespace level and cluster level would be almost the same but
the former would have an advantage in terms of operation and
performance.

> > First, I think we should use a code repository to integrate [1] and [2]
> > instead of sending diffs back and forth. That would force us to resolve
> > conflicts soon and help to avoid duplicate work. The diffs would be created
> > only whe we need to post the next patch version to pgsql-hackers for review,
> > otherwise the discussions of details can take place elsewhere.
>
> Well, we can do that, or just follow the TODO list and apply items as we
> complete them.  We have found that doing everything in one big patch is
> just too hard to review and get accepted.
>
> > The most difficult problem I see now regarding the collaboration is 
> > agreement
> > on the key management user interface. The Full-Cluster Encryption feature 
> > [1]
> > should not add configuration variables or even tools that the next, more
> > sophisticated version [2] deprecates immediately. Part of the problem is 
> > that
>
> Yes, the all-cluster encryption feature has _no_ SQL-level API to
> control it, just a GUC variable that you can use SHOW to see the
> encryption mode.
>
> > [2] puts all (key management related) interaction of postgres with the
> > environment into an external library. As I pointed out in my response to 
> > [2],
> > this will not work for frontend applications (e.g. pg_waldump). I think the
> > key management UI for [2] needs to be designed first even if PG 13 should
> > adopt only [1].
>
> I think there are several directions we can go after all-cluster
> encryption, and it does matter because we would want minimal API
> breakage.  The options are:
>
> 1)  Allow per-table encyption control to limit encryption overhead,
> though all of WAL still needs to be encrypted;  we could add a
> per-record encyption flag to WAL records to avoid that.
>
> 2)  Allow user-controlled keys, which are always unlocked, and encrypt
> WAL with one key
>
> 3)  Encrypt only the user-data portion of pages with user-controlled
> keys.  FREEZE and crash recovery work since only the user data is
> encrypted.  WAL is not encrypted, except for the user-data portion
>
> I think once we implement all-cluster encryption, there will be little
> value to #1 unless we find that page encryption is a big performance
> hit, which I think is unlikely based on performance tests so far.
>
> I don't think #2 has much value since the keys have to always be
> unlocked to allow freeze and crash recovery.
>
> I don't think #3 is viable since there is too much information leakage,
> particularly for indexes because the tid is visible.
>
> Now, if someone says they still want 2 & 3, which has happened many
> times, explain how these issues can be reasonable addressed.
>
> I frankly think we will implement all-cluster encryption, and nothing
> else.  I think the next big encryption feature after that will be
> client-side encryption support, which can be done now but is complex;
> it needs to be easier.
>
> > At least it should be clear how [2] will retrieve the master key because [1]
> > should not do it in a differnt way. (The GUC cluster_passphrase_command
> > mentioned in [3] seems viable, although I think [1] uses approach which is
> > more convenient if the passphrase should be read from console.)

I think that we can also provide a way to pass encryption key directly
to postmaster rather than using passphrase. Since it's common that
user stores keys in KMS it's useful if we can do that.

> > Rotation of
> > the master key is another thing that both versions of the feature should do 
> > in
> > the same way. And of course, the fronend applications need consistent 
> > approach
> > too.
>
> I don't see the value of an external library for key storage.

I think that big benefit is that PostgreSQL can seamlessly work with
external services such as KMS. For instance, when key rotation,
PostgreSQL can register new key to KMS and use it, and it can remove
keys when it no longer necessary. That is, it can enable PostgreSQL to
not only not only getting key from KMS but also registering and
removing keys. And we also can decrypt MDEK in KMS instead of doing in
PostgreSQL which is more safety. In addition, once someone create the
plugin library of an external services individual projects don't need
to create that.


BTW I've created PoC patch for cluster encryption feature. Attached
patch set has done some items of TODO list and 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Wed, Aug 14, 2019 at 04:36:35PM +0200, Antonin Houska wrote:
> I can work on it right away but don't know where to start.

I think the big open question is whether there will be acceptance of an
all-cluster encyption feature.  I guess if no one objects, we can move
forward.

> First, I think we should use a code repository to integrate [1] and [2]
> instead of sending diffs back and forth. That would force us to resolve
> conflicts soon and help to avoid duplicate work. The diffs would be created
> only whe we need to post the next patch version to pgsql-hackers for review,
> otherwise the discussions of details can take place elsewhere.

Well, we can do that, or just follow the TODO list and apply items as we
complete them.  We have found that doing everything in one big patch is
just too hard to review and get accepted.

> The most difficult problem I see now regarding the collaboration is agreement
> on the key management user interface. The Full-Cluster Encryption feature [1]
> should not add configuration variables or even tools that the next, more
> sophisticated version [2] deprecates immediately. Part of the problem is that

Yes, the all-cluster encryption feature has _no_ SQL-level API to
control it, just a GUC variable that you can use SHOW to see the
encryption mode.

> [2] puts all (key management related) interaction of postgres with the
> environment into an external library. As I pointed out in my response to [2],
> this will not work for frontend applications (e.g. pg_waldump). I think the
> key management UI for [2] needs to be designed first even if PG 13 should
> adopt only [1].

I think there are several directions we can go after all-cluster
encryption, and it does matter because we would want minimal API
breakage.  The options are:

1)  Allow per-table encyption control to limit encryption overhead,
though all of WAL still needs to be encrypted;  we could add a
per-record encyption flag to WAL records to avoid that.

2)  Allow user-controlled keys, which are always unlocked, and encrypt
WAL with one key

3)  Encrypt only the user-data portion of pages with user-controlled
keys.  FREEZE and crash recovery work since only the user data is
encrypted.  WAL is not encrypted, except for the user-data portion

I think once we implement all-cluster encryption, there will be little
value to #1 unless we find that page encryption is a big performance
hit, which I think is unlikely based on performance tests so far.

I don't think #2 has much value since the keys have to always be
unlocked to allow freeze and crash recovery.

I don't think #3 is viable since there is too much information leakage,
particularly for indexes because the tid is visible.

Now, if someone says they still want 2 & 3, which has happened many
times, explain how these issues can be reasonable addressed.

I frankly think we will implement all-cluster encryption, and nothing
else.  I think the next big encryption feature after that will be
client-side encryption support, which can be done now but is complex; 
it needs to be easier.

> At least it should be clear how [2] will retrieve the master key because [1]
> should not do it in a differnt way. (The GUC cluster_passphrase_command
> mentioned in [3] seems viable, although I think [1] uses approach which is
> more convenient if the passphrase should be read from console.) Rotation of
> the master key is another thing that both versions of the feature should do in
> the same way. And of course, the fronend applications need consistent approach
> too.

I don't see the value of an external library for key storage.

> I'm not too happy to start another (potentially long) discussion in this
> already huge thread, but I think the UI stuff belongs to the -hackers list
> rather than to an offline discussion.

I think the big question is whether we do anything, or just decide we
can't agree and stop.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Antonin Houska]

Bruce Momjian  wrote:

> On Sat, Aug 10, 2019 at 08:06:17AM -0400, Bruce Momjian wrote:
> > So, I just had an indea if we use separate encryption keys for
> > heap/index and for WAL --- we already know we will have an offline tool
> > that can rotate the passphrase or encryption keys.  If we allow the
> > encryption keys to be rotated independently, we can create a standby,
> > and immediately rotate its heap/index encryption key.  We can then start
> > streaming replication.  When we promote the standby to primary, we can
> > then shut it down and rotate the WAL encryption key --- the new primary
> > would then have no shared keys with the old primary.
> 
> To help move this forward, I created a new wiki TDE section titled "TODO
> for Full-Cluster Encryption" and marked some unresolved items with
> question marks:
> 
>   
> https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#TODO_for_Full-Cluster_Encryption
> 
> I have also updated some of the other text to match conclusions we have
> made.
> 
> I know some of the items are done, but if we have agreement on moving
> forward, I can help with some of the missing code.  This looks doable
> for PG 13 if we start soon.

I can work on it right away but don't know where to start.

First, I think we should use a code repository to integrate [1] and [2]
instead of sending diffs back and forth. That would force us to resolve
conflicts soon and help to avoid duplicate work. The diffs would be created
only whe we need to post the next patch version to pgsql-hackers for review,
otherwise the discussions of details can take place elsewhere.

A separate branch can be created for the Full-Cluster Encryption at some point
- there are probably multiple branching strategies.

The most difficult problem I see now regarding the collaboration is agreement
on the key management user interface. The Full-Cluster Encryption feature [1]
should not add configuration variables or even tools that the next, more
sophisticated version [2] deprecates immediately. Part of the problem is that
[2] puts all (key management related) interaction of postgres with the
environment into an external library. As I pointed out in my response to [2],
this will not work for frontend applications (e.g. pg_waldump). I think the
key management UI for [2] needs to be designed first even if PG 13 should
adopt only [1].

At least it should be clear how [2] will retrieve the master key because [1]
should not do it in a differnt way. (The GUC cluster_passphrase_command
mentioned in [3] seems viable, although I think [1] uses approach which is
more convenient if the passphrase should be read from console.) Rotation of
the master key is another thing that both versions of the feature should do in
the same way. And of course, the fronend applications need consistent approach
too.

I'm not too happy to start another (potentially long) discussion in this
already huge thread, but I think the UI stuff belongs to the -hackers list
rather than to an offline discussion.


[1] https://commitfest.postgresql.org/23/2104/

[2] 
https://www.postgresql.org/message-id/CAD21AoBjrbxvaMpTApX1cEsO%3D8N%3Dnc2xVZPB0d9e-VjJ%3DYaRnw%40mail.gmail.com

[3]
https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#TODO_for_Full-Cluster_Encryption

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

RE: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Smith, Peter]

On Sat, Aug 10, 2019 at 1:19 AM Tomas Vondra  
wrote: 

> We can of course support "forced" re-encryption, but I think it's acceptable 
> if that's fairly expensive as long as it can be throttled and executed in the 
> background (kinda similar to the patch to enable checksums in the background).

As an alternative way to provide for a "forced" re-encryption couldn't you just 
run pg_dumpall + psql?

Regards,
--
Peter Smith
Fujitsu Australia

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Masahiko Sawada]

On Sat, Aug 10, 2019 at 12:18 AM Tomas Vondra
 wrote:
>
> On Fri, Aug 09, 2019 at 11:51:23PM +0900, Masahiko Sawada wrote:
> >On Fri, Aug 9, 2019 at 10:25 AM Bruce Momjian  wrote:
> >>
> >> On Thu, Aug  8, 2019 at 06:31:42PM -0400, Stephen Frost wrote:
> >> > > >Crash recovery doesn't happen "all the time" and neither does vacuum
> >> > > >freeze, and autovacuum processes are independent of individual client
> >> > > >backends- we don't need to (and shouldn't) have the keys in shared
> >> > > >memory.
> >> > >
> >> > > Don't people do physical replication / HA pretty much all the time?
> >> >
> >> > Strictly speaking, that isn't actually crash recovery, it's physical
> >> > replication / HA, and while those are certainly nice to have it's no
> >> > guarantee that they're required or that you'd want to have the same keys
> >> > for them- conceptually, at least, you could have WAL with one key that
> >> > both sides know and then different keys for the actual data files, if we
> >> > go with the approach where the WAL is encrypted with one key and then
> >> > otherwise is plaintext.
> >>
> >> Uh, yes, you could have two encryption keys in the data directory, one
> >> for heap/indexes, one for WAL, both unlocked with the same passphrase,
> >> but what would be the value in that?
> >>
> >> > > >>That might allow crash recovery and the freeze part of VACUUM FREEZE 
> >> > > >>to
> >> > > >>work.  (I don't think we could vacuum since we couldn't read the 
> >> > > >>index
> >> > > >>pages to find the matching rows since the index values would be 
> >> > > >>encrypted
> >> > > >>too.  We might be able to not encrypt the tid in the index typle.)
> >> > > >
> >> > > >Why do we need the indexed values to vacuum the index..?  We don't
> >> > > >today, as I recall.  We would need the tids though, yes.
> >> > >
> >> > > Well, we also do collect statistics on the data, for example. But even
> >> > > if we assume we wouldn't do that for encrypted indexes (which seems 
> >> > > like
> >> > > a pretty bad idea to me), you'd probably end up leaking information
> >> > > about ordering of the values. Which is generally a pretty serious
> >> > > information leak, AFAICS.
> >> >
> >> > I agree entirely that order information would be bad to leak- but this
> >> > is all new ground here and we haven't actually sorted out what such a
> >> > partially encrypted btree would look like.  We don't actually have to
> >> > have the down-links in the tree be unencrypted to allow vacuuming of
> >> > leaf pages, after all.
> >>
> >> Agreed, but I think we kind of know that the value in cluster-wide
> >> encryption is different from multi-key encryption --- both have their
> >> value, but right now cluster-wide is the easiest and simplest, and
> >> probably meets more user needs than multi-key encryption.  If others
> >> want to start scoping out what multi-key encryption would look like, we
> >> can discuss it.  I personally would like to focus on cluster-wide
> >> encryption for PG 13.
> >
> >I agree that cluster-wide is more simpler but I'm not sure that it
> >meets real needs from users. One example is re-encryption; when the
> >key leakage happens, in cluster-wide encryption we end up with doing
> >re-encrypt whole database regardless the amount of user sensitive data
> >in database. I think it's a big constraint for users because it's
> >common that the amount of data such as master table that needs to be
> >encrypted doesn't account for a large potion of database. That's one
> >reason why I think more fine granularity encryption such as
> >table/tablespace is required.
> >
>
> TBH I think it's mostly pointless to design for key leakage.
>
> My understanding it that all this work is motivated by the assumption that
> Bob can obtain access to the data directory (say, a backup of it). So if
> he also manages to get access to the encryption key, we probably have to
> assume he already has access to current snapshot of the data directory,
> which means any re-encryption is pretty futile.
>
> What we can (and should) optimize for is key rotation, but as that only
> changes the master key and not the actual encryption keys, the overhead is
> pretty low.
>
> We can of course support "forced" re-encryption, but I think it's
> acceptable if that's fairly expensive as long as it can be throttled and
> executed in the background (kinda similar to the patch to enable checksums
> in the background).

I'm not sure that we can ignore the risk of MDEK leakage. Once MDEK is
leaked for whatever reason all that is left for attacker is to steal
data. User who realized that MDEK is leaked will have to re-encrypt
data. Even if the data is already stolen user will want to re-encrypt
data to protect further attacks. KEK rotation is futile in this case.

>
> >And in terms of feature development we would implement
> >fine-granularity encryption in the future even if the first step is
> >cluster-wide encryption? And both TDEs encrypt the same kind of
> 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Sat, Aug 10, 2019 at 08:06:17AM -0400, Bruce Momjian wrote:
> So, I just had an indea if we use separate encryption keys for
> heap/index and for WAL --- we already know we will have an offline tool
> that can rotate the passphrase or encryption keys.  If we allow the
> encryption keys to be rotated independently, we can create a standby,
> and immediately rotate its heap/index encryption key.  We can then start
> streaming replication.  When we promote the standby to primary, we can
> then shut it down and rotate the WAL encryption key --- the new primary
> would then have no shared keys with the old primary.

To help move this forward, I created a new wiki TDE section titled "TODO
for Full-Cluster Encryption" and marked some unresolved items with
question marks:


https://wiki.postgresql.org/wiki/Transparent_Data_Encryption#TODO_for_Full-Cluster_Encryption

I have also updated some of the other text to match conclusions we have
made.

I know some of the items are done, but if we have agreement on moving
forward, I can help with some of the missing code.  This looks doable
for PG 13 if we start soon.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Wed, Aug  7, 2019 at 08:56:18AM -0400, Sehrope Sarkuni wrote:
> On Mon, Aug 5, 2019 at 9:02 PM Bruce Momjian  wrote:
> I don't think we want to add a MAC at this point since the MAC for 8k
> pages seems unattainable.
> 
> Even without a per-page MAC, a MAC at some level for WAL has its own benefits
> such as perfect corruption detection. It could be per-record, per-N-records,
> per-checkpoint, or per-file. The current WAL file format already handles
> arbitrary gaps so there is significantly more flexibility in adding it vs
> pages. I'm not saying it should be a requirement but, unlike pages, I would 
> not
> rule it out just yet as it may not be that complicated.

FYI, the WAL already has a CRC that detects corruption and
parially-written records (which are ignored and stop the reading of
WAL).

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Wed, Jul 31, 2019 at 09:43:00AM -0400, Sehrope Sarkuni wrote:
> On Wed, Jul 31, 2019 at 2:32 AM Masahiko Sawada  wrote:
> For WAL encryption,  before flushing WAL we encrypt whole 8k WAL page
> and then write only the encrypted data of the new WAL record using
> pg_pwrite() rather than write whole encrypted page. So each time we
> encrypt 8k WAL page we end up with encrypting different data with the
> same key+nonce but since we don't write to the disk other than space
> where we actually wrote WAL records it's not a problem. Is that right?
> 
> Ah, this is what I was referring to in my previous mail. I'm not familiar with
> how the writes happen yet (reading up...) but, yes, we would need to ensure
> that encrypted data is not written more than once (i.e. no writing of encrypt
> (zero) followed by writing of encrypt(non-zero) at the same spot).

No one replied to this comment, so I will state here that we never write
zeros to the WAL and go back and write something else --- the WAL is
append-only.  We might do that for heap/index pages, but they would get
a new LSN (and hence a new IV) when that happens.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Tue, Jul 30, 2019 at 04:48:31PM -0400, Bruce Momjian wrote:
> I am not even clear if pg_upgrade preserving relfilenode is possible ---
> when we wrap the relfilenode counter, does it start at 1 or at the
> first-user-relation-oid?  If the former, it could conflict with oids
> assigned to new system tables in later major releases.  Tying the
> preservation of relations to two restrictions seems risky.

For the curious, when relfilenode wraps, it starts at
FirstNormalObjectId, because GetNewRelFileNode eventually calls
GetNewObjectId(), so the concern above is wrong, though this is not an
issue anymore.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Thu, Jul 25, 2019 at 11:30:55PM -0400, Alvaro Herrera wrote:
> On 2019-Jul-25, Alvaro Herrera wrote:
> > On the other hand if the Key and IV are reused between messages then
> > the same plaintext will lead to the same ciphertext, so you can
> > potentially decrypt a message using a sufficiently large corpus of known
> > matching plaintext/ciphertext pairs, even without ever recovering the
> > key.
> 
> Actually the attack being described presumes that you know *both the*
> *unencrypted data and the encrypted data* for a certain key/IV pair,
> and only then you can decrypt some other data.  It doesn't follow that
> you can decrypt data just because somebody reused the IV for a second
> page ... I haven't seen any literature referenced that explains what
> this attack is.

I never addressed this exact comment.  If someone can guess at some
known heap/index format markers at specific offsets in a page, they can
XOR that with the encrypted data to get the encryption bit stream.  They
could then use that encrypted bit stream to XOR against another
encrypted page at the same offsets and with the same key/IV to see
unenrypted user data if it exists at the same page offsets.  (The
all-zero empty space is a huge known format marker area.)

This is why CTR is so sensitive to reuse of the key/IV settings for
encrypting different data.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Wed, Jul 10, 2019 at 08:07:49PM -0400, Bruce Momjian wrote:
> On Thu, Jul 11, 2019 at 12:18:47AM +0200, Tomas Vondra wrote:
> > On Wed, Jul 10, 2019 at 06:04:30PM -0400, Stephen Frost wrote:
> > > Greetings,
> > > 
> > > * Tomas Vondra (tomas.von...@2ndquadrant.com) wrote:
> > > > On Wed, Jul 10, 2019 at 04:11:21PM -0400, Alvaro Herrera wrote:
> > > > >On 2019-Jul-10, Bruce Momjian wrote:
> > > > >
> > > > >>Uh, what if a transaction modifies page 0 and page 1 of the same table
> > > > >>--- don't those pages have the same LSN.
> > > > >
> > > > >No, because WAL being a physical change log, each page gets its own
> > > > >WAL record with its own LSN.
> > > > >
> > > > 
> > > > What if you have wal_log_hints=off? AFAIK that won't change the page 
> > > > LSN.
> > > 
> > > Alvaro suggested elsewhere that we require checksums for these, which
> > > would also force wal_log_hints to be on, and therefore the LSN would
> > > change.
> > > 
> > 
> > Oh, I see - yes, that would solve the hint bits issue. Not sure we want
> > to combine the features like this, though, as it increases the costs of
> > TDE. But maybe it's the best solution.
> 
> Uh, why can't we just force log_hint_bits for encrypted tables?  Why
> would we need to use checksums as well?

When we were considering CBC mode for heap/index pages, a change of a
hint bit would change all later 16-byte encrypted blocks.  Now that we
are using CTR mode, a change of a hint bit will only change a bit on the
stored page.

Someone could compare the old and new pages and see that a bit was
changed.  This would make log_hint_bits less of a requirement with CTR
mode, though leaking hit bit changes is probably not ideal.  Perhaps
log_hint_bits should be a recommendation and not a requirement.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Antonin Houska]

Masahiko Sawada  wrote:

> Attached the draft version patch sets of per tablespace transparent
> data at rest encryption. The patch doesn't support full functionality,
> it includes:
> 
> * Per tablespace encryption
> * Encryption and decryption buffer data when disk I/O.
> * 2 tier key hierarchy and key rotation
> * Temporary file encryption (based on the patch Antonin proposd)
> * System catalog encryption
> * Generic key management API and test module
> * Simple TAP test

I've checked your patch series to find out how to adjust [1] to make future
merge easier.

The biggest issue I see is that front-end applications won't be able to load
the KMGR plugin. We also used some sort of external library in the first
version of [1], but when I was trying to make the front-ends aware of
encryption, I found out that dfmgr.c cannot be linked to them w/o significant
rework. So I gave up and moved the encrypt_block() and decrypt_block()
functions to the core.

A few more notes regarding key management:

* InitializeKmgr()

  ** the function probably does not have to acquire KeyringControlLock, for
  the same reasons that load_keyring_file() does not do (i.e. it's only called
  by postmaster during startup)

  ** the lines

char *key = NULL;

 as well as

/* Get the master key */
key = KmgrPluginGetKey(KmgrCtl->masterKeyId);

Assert(key != NULL);

  should be enclosed in #ifdef USE_ASSERT_CHECKING - #endif, otherwise I
  suppose (but haven't verified) compiler will produce warning that variable
  is set but not used.

  Actually ERROR might be more suitable for external (loadable) KMGR plugin,
  but, as explained above, I'm not sure if such an approach is viable.

* KmgrPluginGetKey() only seems to deal with the master key, not with the
  tablespace keys. So I suggest the name to contain the 'Master' word.

* KmgrPluginRemoveKey() seems to be unused.

* KeyringCreateKey() - I wondered why the key is returned encrypted. Actually
  the only call of the function that I found is that in CreateTableSpace(),
  and it does not use the return value at all. Shouldn't KeyringGetKey()
  handle creation of the key if it does not exist yet?

* KeyringAddKey() seems to be unused.

* keyring size (kmgr.c):

/*
 * Since we have encryption keys per tablspace, we expect this value is 
enough
 * for most usecase.
 */
#define KMGR_KEYRING_SIZE 128

There's no guarantee that the number of tablespaces won't exceed any
(reasonably low) constant value. The KMGR module should be able to
allocate additional memory dynamically.


[1] https://commitfest.postgresql.org/23/2104/

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Wed, Aug  7, 2019 at 08:56:18AM -0400, Sehrope Sarkuni wrote:
> On Mon, Aug 5, 2019 at 9:02 PM Bruce Momjian  wrote:
> I was thinking the WAL would use the same key since the nonce is unique
> between the two.  What value is there in using a different key?

> Never having to worry about overlap in Key + IV usage is main advantage. While
> it's possible to structure IVs to avoid that from happening, it's much easier
> to completely avoid that situation by ensuring different parts of an
> application are using separate derived keys.

Now that we are considering a different encryption key for heap/index
files and WAL, so there is no chance of overlap, it seems we can go back
to using a non-zero IV rather than derived keys.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Fri, Aug  9, 2019 at 10:54:51PM -0400, Bruce Momjian wrote:
> On Thu, Aug  8, 2019 at 10:17:53PM -0400, Sehrope Sarkuni wrote:
> > On Thu, Aug 8, 2019 at 2:16 PM Bruce Momjian  wrote:
> > 
> > On Wed, Aug  7, 2019 at 08:56:18AM -0400, Sehrope Sarkuni wrote:
> > > Simplest approach for derived keys would be to use immutable 
> > attributes
> > of the
> > > WAL files as an input to the key derivation. Something like HKDF(MDEK,
> > "WAL:" |
> > 
> > So, I am thinking we should use "WAL:" for WAL and "REL:" for heap/index
> > files.
> > 
> > 
> > Sounds good. Any unique convention is fine. Main thing to keep in mind is 
> > that
> > they're directly tied to the master key so it's not possible to rotate them
> > without changing the master key.
> 
> A recent email talked about using two different encryption keys for
> heap/index and WAL, which allows for future features, and allows for key
> rotation of the two independently.  (I already stated how hard key
> rotation would be with WAL and pg_rewind.)

So, I just had an indea if we use separate encryption keys for
heap/index and for WAL --- we already know we will have an offline tool
that can rotate the passphrase or encryption keys.  If we allow the
encryption keys to be rotated independently, we can create a standby,
and immediately rotate its heap/index encryption key.  We can then start
streaming replication.  When we promote the standby to primary, we can
then shut it down and rotate the WAL encryption key --- the new primary
would then have no shared keys with the old primary.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Thu, Aug  8, 2019 at 10:17:53PM -0400, Sehrope Sarkuni wrote:
> On Thu, Aug 8, 2019 at 2:16 PM Bruce Momjian  wrote:
> 
> On Wed, Aug  7, 2019 at 08:56:18AM -0400, Sehrope Sarkuni wrote:
> > Simplest approach for derived keys would be to use immutable attributes
> of the
> > WAL files as an input to the key derivation. Something like HKDF(MDEK,
> "WAL:" |
> 
> So, I am thinking we should use "WAL:" for WAL and "REL:" for heap/index
> files.
> 
> 
> Sounds good. Any unique convention is fine. Main thing to keep in mind is that
> they're directly tied to the master key so it's not possible to rotate them
> without changing the master key.

A recent email talked about using two different encryption keys for
heap/index and WAL, which allows for future features, and allows for key
rotation of the two independently.  (I already stated how hard key
rotation would be with WAL and pg_rewind.)

> This is in contrast to saving a WDEK key to a file (similar to how the MDEK 
> key
> would be saved) and unlocking it with the MDEK. That has more moving parts but
> would allow that key to be independent of the MDEK. In a later message Stephen
> refers to an example of a replica receiving encrypted WAL and applying it with
> a different MDEK for the page buffers. That's doable with an independent WDEK.

I assumed we would call a command on boot to get a passphrase, which
would unlock the encryption keys.  Is there more being described above?

> > | timeline_id || wal_segment_num) should be fine for this as it is:
> 
> I considered using the timeline in the nonce, but then remembered that
> in timeline switch, we _copy_ the part of the old WAL up to the timeline
> switch to the new timeline;  see:
> 
>     https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/
> 
> backend/access/transam/xlog.c;h=f55352385732c6b0124eff5265462f3883fe7435;hb
> =HEAD#l5502
> 
>    * Initialize the starting WAL segment for the new timeline. If the
> switch
>    * happens in the middle of a segment, copy data from the last WAL
> segment
>    * of the old timeline up to the switch point, to the starting WAL
> segment
>    * on the new timeline.
> 
> We would need to decrypt/encrypt to do the copy, and just wasn't sure of
> the value of the timeline in the nonce.  One value to it is that if
> there some WAL that generated after the timeline switch in the old
> primary that isn't transfered, there would be potentially new data
> encrypted with the same key/nonce in the new primary, but if that WAL is
> not used, odds are it is gone/destroyed/inaccessible, or it would have
> been used during the switchover, so it didn't seem worth worrying about.
> 
> One _big_ reason to add the timeline is if you had a standby that you
> recovered and rolled forward only to a specific transaction, then
> continued running it as a new primary.  In that case, you would have
> different WAL encrypted with the same key/nonce, but that sounds like
> the same as promoting two standbys, and we should just document not to
> do it.
> 
> Maybe we need to consider this further.
> 
> 
> Good points. Yes, anything short of generating a new key at promotion time 
> will
> have these issues. If we're not going to do that, no point in adding the
> timeline id if it does not change anything. I had thought only the combo was
> unique but sounds like the segment number is unique on its own. One thing I
> like about a unique per-file key is that it simplifies the IV generation (i.e.
> can start at zero).

Uh, well, the segment number is unique, _except_ for a timeline switch,
where the segment number exists in the old timeline file, and in the new
timeline file, though the new timeline file has more WAL because it has
writes only in the new timeline.  So, in a way, the WAL data is the same
in the old and new timeline files, e.g. 00010001 and
00020001, but 00010001 stops at the
timeline switch and 00020001 has more WAL data that
represents cluster activity since the WAL switch, though the two files
are both 16MB.

> What about discarding the rest of the WAL file at promotion and skipping to a
> new file? With a random per-file key in the first page header would ensure 
> that
> going forward all WAL data is encrypted differently. Combine that with
> independent WAL and MDEK keys and everything would be different between two
> replicas promoted from the same point.

Yes, we could do that, but I am hesitant to change the WAL format just
for this, unless there is value to the random number.  We already
discussed that the LSN could be duplicated in different heap/index
files, so we would still have issues.

> > A unique WDEK per WAL file that is derived from the segment number would
> not
> > have that problem. A unique key per-file means the IVs can all start at

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Fri, Aug  9, 2019 at 05:01:47PM +0200, Tomas Vondra wrote:
> I know there were proposals to keep it encrypted in shared buffers, but
> I'm not sure that's what we'll end up doing (I have not followed the
> recent discussion all that closely, though).

There is no plan to encrypt shared buffers.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Thu, Aug  8, 2019 at 10:34:26PM -0400, Sehrope Sarkuni wrote:
> On Thu, Aug 8, 2019 at 6:31 PM Stephen Frost  wrote:
> 
> Strictly speaking, that isn't actually crash recovery, it's physical
> replication / HA, and while those are certainly nice to have it's no
> guarantee that they're required or that you'd want to have the same keys
> for them- conceptually, at least, you could have WAL with one key that
> both sides know and then different keys for the actual data files, if we
> go with the approach where the WAL is encrypted with one key and then
> otherwise is plaintext.
> 
> 
> I like the idea of separating the WAL key from the rest of the data files. 
> It'd all be unlocked by the MDEK and you'd still need derived keys per
> WAL-file, but disconnecting all that from the data files solves a lot of the
> problems with promoted replicas.
> 
> This would complicate cloning a replica as using a different MDEK would 
> involve
> decrypting / encrypting everything rather than just copying the files. Even if
> that's not baked in a first version, the separation allows for eventually
> supporting that.

OK, I can get behind that idea.  One cool idea would be for the WAL on
primary and standbys to use the same WAL key, but to use different
heap/index keys.  When the standby is promoted, there would be a way for
the WAL to start using a new encryption key, and the heap/index would
already be using its own encryption key.

Setting up such a system seems complicated.  The big problem is that the
base backup would use the primary key, unless we allowed  pg_basebackup
to decrypt/encrypt with a new heap/index key.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Fri, Aug  9, 2019 at 11:51:23PM +0900, Masahiko Sawada wrote:
> I agree that cluster-wide is more simpler but I'm not sure that it
> meets real needs from users. One example is re-encryption; when the
> key leakage happens, in cluster-wide encryption we end up with doing
> re-encrypt whole database regardless the amount of user sensitive data
> in database. I think it's a big constraint for users because it's
> common that the amount of data such as master table that needs to be
> encrypted doesn't account for a large potion of database. That's one
> reason why I think more fine granularity encryption such as
> table/tablespace is required.
> 
> And in terms of feature development we would implement
> fine-granularity encryption in the future even if the first step is
> cluster-wide encryption? And both TDEs encrypt the same kind of
> database objects (i.e. only  tables , indexes and WAL)? If so, how
> does users  use them depending on cases?
> 
> I imagined the case where we had the cluster-wide encryption as the
> first TDE feature. We will enable TDE at initdb time by specifying the
> command-line parameter for TDE. Then TDE is enabled in cluster wide
> and all tables/indexes and WAL are automatically encrypted. Then, if
> we want to implement the more fine granularity encryption how can we
> make users use it? WAL encryption and tables/index encryption are
> enabled at the same time but we want to enable encryption for
> particular tables/indexes after initdb. If the cluster-wide encryption
> is something like a short-cut of encrypting all tables/indexes, I
> personally think that implementing the more fine granularity one first
> and then using it to achieve the more coarse granularity would be more
> easier.

I don't know how to move this thread forward, so I am going to try to
explain how I view it.  People want feature X and feature Y.  Feature X
seems straight-forward to implement, use, and seems secure.  Feature Y
is none of those, so far.

When I explain that feature X is the direction we should go in for PG
13, people give more reasons they want feature Y, but don't give any
details about how the problems of implemention, use, and security can be
addressed.

I just don't see that continuing to discuss feature Y with no details in
how to implement it really helps, so I have no reply to these ideas. 
People can talk about whatever feature they want on these lists, but I
have no way to help discussions that don't address facts.

I will close with what I have already stated, that what people want, and
what we can reasonably accomplish, are two different things.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Tomas Vondra]

On Fri, Aug 09, 2019 at 11:51:23PM +0900, Masahiko Sawada wrote:

On Fri, Aug 9, 2019 at 10:25 AM Bruce Momjian  wrote:


On Thu, Aug  8, 2019 at 06:31:42PM -0400, Stephen Frost wrote:
> > >Crash recovery doesn't happen "all the time" and neither does vacuum
> > >freeze, and autovacuum processes are independent of individual client
> > >backends- we don't need to (and shouldn't) have the keys in shared
> > >memory.
> >
> > Don't people do physical replication / HA pretty much all the time?
>
> Strictly speaking, that isn't actually crash recovery, it's physical
> replication / HA, and while those are certainly nice to have it's no
> guarantee that they're required or that you'd want to have the same keys
> for them- conceptually, at least, you could have WAL with one key that
> both sides know and then different keys for the actual data files, if we
> go with the approach where the WAL is encrypted with one key and then
> otherwise is plaintext.

Uh, yes, you could have two encryption keys in the data directory, one
for heap/indexes, one for WAL, both unlocked with the same passphrase,
but what would be the value in that?

> > >>That might allow crash recovery and the freeze part of VACUUM FREEZE to
> > >>work.  (I don't think we could vacuum since we couldn't read the index
> > >>pages to find the matching rows since the index values would be encrypted
> > >>too.  We might be able to not encrypt the tid in the index typle.)
> > >
> > >Why do we need the indexed values to vacuum the index..?  We don't
> > >today, as I recall.  We would need the tids though, yes.
> >
> > Well, we also do collect statistics on the data, for example. But even
> > if we assume we wouldn't do that for encrypted indexes (which seems like
> > a pretty bad idea to me), you'd probably end up leaking information
> > about ordering of the values. Which is generally a pretty serious
> > information leak, AFAICS.
>
> I agree entirely that order information would be bad to leak- but this
> is all new ground here and we haven't actually sorted out what such a
> partially encrypted btree would look like.  We don't actually have to
> have the down-links in the tree be unencrypted to allow vacuuming of
> leaf pages, after all.

Agreed, but I think we kind of know that the value in cluster-wide
encryption is different from multi-key encryption --- both have their
value, but right now cluster-wide is the easiest and simplest, and
probably meets more user needs than multi-key encryption.  If others
want to start scoping out what multi-key encryption would look like, we
can discuss it.  I personally would like to focus on cluster-wide
encryption for PG 13.


I agree that cluster-wide is more simpler but I'm not sure that it
meets real needs from users. One example is re-encryption; when the
key leakage happens, in cluster-wide encryption we end up with doing
re-encrypt whole database regardless the amount of user sensitive data
in database. I think it's a big constraint for users because it's
common that the amount of data such as master table that needs to be
encrypted doesn't account for a large potion of database. That's one
reason why I think more fine granularity encryption such as
table/tablespace is required.



TBH I think it's mostly pointless to design for key leakage.

My understanding it that all this work is motivated by the assumption that
Bob can obtain access to the data directory (say, a backup of it). So if
he also manages to get access to the encryption key, we probably have to
assume he already has access to current snapshot of the data directory,
which means any re-encryption is pretty futile.

What we can (and should) optimize for is key rotation, but as that only
changes the master key and not the actual encryption keys, the overhead is
pretty low.

We can of course support "forced" re-encryption, but I think it's
acceptable if that's fairly expensive as long as it can be throttled and
executed in the background (kinda similar to the patch to enable checksums
in the background).


And in terms of feature development we would implement
fine-granularity encryption in the future even if the first step is
cluster-wide encryption? And both TDEs encrypt the same kind of
database objects (i.e. only  tables , indexes and WAL)? If so, how
does users  use them depending on cases?

I imagined the case where we had the cluster-wide encryption as the
first TDE feature. We will enable TDE at initdb time by specifying the
command-line parameter for TDE. Then TDE is enabled in cluster wide
and all tables/indexes and WAL are automatically encrypted. Then, if
we want to implement the more fine granularity encryption how can we
make users use it? WAL encryption and tables/index encryption are
enabled at the same time but we want to enable encryption for
particular tables/indexes after initdb. If the cluster-wide encryption
is something like a short-cut of encrypting all tables/indexes, I
personally think that implementing the more fine 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Tomas Vondra]

On Thu, Aug 08, 2019 at 06:31:42PM -0400, Stephen Frost wrote:

Greetings,

* Tomas Vondra (tomas.von...@2ndquadrant.com) wrote:

On Thu, Aug 08, 2019 at 03:07:59PM -0400, Stephen Frost wrote:
>* Bruce Momjian (br...@momjian.us) wrote:
>>On Tue, Jul  9, 2019 at 11:09:01AM -0400, Bruce Momjian wrote:
>>> On Tue, Jul  9, 2019 at 10:59:12AM -0400, Stephen Frost wrote:
>>> > * Bruce Momjian (br...@momjian.us) wrote:
>>> > I agree that all of that isn't necessary for an initial implementation,
>>> > I was rather trying to lay out how we could improve on this in the
>>> > future and why having the keying done at a tablespace level makes sense
>>> > initially because we can then potentially move forward with further
>>> > segregation to improve the situation.  I do believe it's also useful in
>>> > its own right, to be clear, just not as nice since a compromised backend
>>> > could still get access to data in shared buffers that it really
>>> > shouldn't be able to, even broadly, see.
>>>
>>> I think TDE is feature of questionable value at best and the idea that
>>> we would fundmentally change the internals of Postgres to add more
>>> features to it seems very unlikely.  I realize we have to discuss it so
>>> we don't block reasonable future feature development.
>>
>>I have a new crazy idea.  I know we concluded that allowing multiple
>>independent keys, e.g., per user, per table, didn't make sense since
>>they have to be unlocked all the time, e.g., for crash recovery and
>>vacuum freeze.
>
>I'm a bit confused as I never agreed that made any sense and I continue
>to feel that it doesn't make sense to have one key for everything.
>
>Crash recovery doesn't happen "all the time" and neither does vacuum
>freeze, and autovacuum processes are independent of individual client
>backends- we don't need to (and shouldn't) have the keys in shared
>memory.

Don't people do physical replication / HA pretty much all the time?


Strictly speaking, that isn't actually crash recovery, it's physical
replication / HA, and while those are certainly nice to have it's no
guarantee that they're required or that you'd want to have the same keys
for them- conceptually, at least, you could have WAL with one key that
both sides know and then different keys for the actual data files, if we
go with the approach where the WAL is encrypted with one key and then
otherwise is plaintext.



Uh? IMHO not breaking physical replication / HA should be pretty much
required for any new feature, unless it's somehow obviously clear that
it's not needed for that particular feature. I very much doubt we can make
that conclusion for encrypted instances (at least I don't see why it would
be the case in general).

One reason is that those features are also used for backups, which I hope
we both agree is not an optional feature. Maybe it's possible to modify
pg_basebackup to re-encrypt all the data, but to do that it clearly needs
to know all encryption keys (although not necessarily on the same side).


>>However, that assumes that all heap/index pages are encrypted, and all
>>of WAL.  What if we encrypted only the user-data part of the page, i.e.,
>>tuple data.  We left xmin/xmax unencrypted, and only stored the
>>encrypted part of that data in WAL, and didn't encrypt any more of WAL.
>
>This is pretty much what Alvaro was suggesting a while ago, isn't it..?
>Have just the user data be encrypted in the table and in the WAL stream.

It's also moving us much closer to pgcrypto-style encryption ...


Yes, it is, and there's good parts and bad parts to that, to be sure.


>>That might allow crash recovery and the freeze part of VACUUM FREEZE to
>>work.  (I don't think we could vacuum since we couldn't read the index
>>pages to find the matching rows since the index values would be encrypted
>>too.  We might be able to not encrypt the tid in the index typle.)
>
>Why do we need the indexed values to vacuum the index..?  We don't
>today, as I recall.  We would need the tids though, yes.

Well, we also do collect statistics on the data, for example. But even
if we assume we wouldn't do that for encrypted indexes (which seems like
a pretty bad idea to me), you'd probably end up leaking information
about ordering of the values. Which is generally a pretty serious
information leak, AFAICS.


I agree entirely that order information would be bad to leak- but this
is all new ground here and we haven't actually sorted out what such a
partially encrypted btree would look like.  We don't actually have to
have the down-links in the tree be unencrypted to allow vacuuming of
leaf pages, after all.



Well, I'm not all that familiar with the btree code, but I still think you
can deduce an awful amount of information from having the leaf pages alone
(not sure if we could deduce a total order, but presumably yes).


>>Is this something considering in version one of this feature?  Probably
>>not, but later?  Never?  Would the information leakage be too great,
>>particularly 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Masahiko Sawada]

On Fri, Aug 9, 2019 at 10:25 AM Bruce Momjian  wrote:
>
> On Thu, Aug  8, 2019 at 06:31:42PM -0400, Stephen Frost wrote:
> > > >Crash recovery doesn't happen "all the time" and neither does vacuum
> > > >freeze, and autovacuum processes are independent of individual client
> > > >backends- we don't need to (and shouldn't) have the keys in shared
> > > >memory.
> > >
> > > Don't people do physical replication / HA pretty much all the time?
> >
> > Strictly speaking, that isn't actually crash recovery, it's physical
> > replication / HA, and while those are certainly nice to have it's no
> > guarantee that they're required or that you'd want to have the same keys
> > for them- conceptually, at least, you could have WAL with one key that
> > both sides know and then different keys for the actual data files, if we
> > go with the approach where the WAL is encrypted with one key and then
> > otherwise is plaintext.
>
> Uh, yes, you could have two encryption keys in the data directory, one
> for heap/indexes, one for WAL, both unlocked with the same passphrase,
> but what would be the value in that?
>
> > > >>That might allow crash recovery and the freeze part of VACUUM FREEZE to
> > > >>work.  (I don't think we could vacuum since we couldn't read the index
> > > >>pages to find the matching rows since the index values would be 
> > > >>encrypted
> > > >>too.  We might be able to not encrypt the tid in the index typle.)
> > > >
> > > >Why do we need the indexed values to vacuum the index..?  We don't
> > > >today, as I recall.  We would need the tids though, yes.
> > >
> > > Well, we also do collect statistics on the data, for example. But even
> > > if we assume we wouldn't do that for encrypted indexes (which seems like
> > > a pretty bad idea to me), you'd probably end up leaking information
> > > about ordering of the values. Which is generally a pretty serious
> > > information leak, AFAICS.
> >
> > I agree entirely that order information would be bad to leak- but this
> > is all new ground here and we haven't actually sorted out what such a
> > partially encrypted btree would look like.  We don't actually have to
> > have the down-links in the tree be unencrypted to allow vacuuming of
> > leaf pages, after all.
>
> Agreed, but I think we kind of know that the value in cluster-wide
> encryption is different from multi-key encryption --- both have their
> value, but right now cluster-wide is the easiest and simplest, and
> probably meets more user needs than multi-key encryption.  If others
> want to start scoping out what multi-key encryption would look like, we
> can discuss it.  I personally would like to focus on cluster-wide
> encryption for PG 13.

I agree that cluster-wide is more simpler but I'm not sure that it
meets real needs from users. One example is re-encryption; when the
key leakage happens, in cluster-wide encryption we end up with doing
re-encrypt whole database regardless the amount of user sensitive data
in database. I think it's a big constraint for users because it's
common that the amount of data such as master table that needs to be
encrypted doesn't account for a large potion of database. That's one
reason why I think more fine granularity encryption such as
table/tablespace is required.

And in terms of feature development we would implement
fine-granularity encryption in the future even if the first step is
cluster-wide encryption? And both TDEs encrypt the same kind of
database objects (i.e. only  tables , indexes and WAL)? If so, how
does users  use them depending on cases?

I imagined the case where we had the cluster-wide encryption as the
first TDE feature. We will enable TDE at initdb time by specifying the
command-line parameter for TDE. Then TDE is enabled in cluster wide
and all tables/indexes and WAL are automatically encrypted. Then, if
we want to implement the more fine granularity encryption how can we
make users use it? WAL encryption and tables/index encryption are
enabled at the same time but we want to enable encryption for
particular tables/indexes after initdb. If the cluster-wide encryption
is something like a short-cut of encrypting all tables/indexes, I
personally think that implementing the more fine granularity one first
and then using it to achieve the more coarse granularity would be more
easier.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Sehrope Sarkuni]

On Thu, Aug 8, 2019 at 6:31 PM Stephen Frost  wrote:

> Strictly speaking, that isn't actually crash recovery, it's physical
> replication / HA, and while those are certainly nice to have it's no
> guarantee that they're required or that you'd want to have the same keys
> for them- conceptually, at least, you could have WAL with one key that
> both sides know and then different keys for the actual data files, if we
> go with the approach where the WAL is encrypted with one key and then
> otherwise is plaintext.
>

I like the idea of separating the WAL key from the rest of the data files.
It'd all be unlocked by the MDEK and you'd still need derived keys per
WAL-file, but disconnecting all that from the data files solves a lot of
the problems with promoted replicas.

This would complicate cloning a replica as using a different MDEK would
involve decrypting / encrypting everything rather than just copying the
files. Even if that's not baked in a first version, the separation allows
for eventually supporting that.

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Sehrope Sarkuni]

On Thu, Aug 8, 2019 at 2:16 PM Bruce Momjian  wrote:

> On Wed, Aug  7, 2019 at 08:56:18AM -0400, Sehrope Sarkuni wrote:
> > Simplest approach for derived keys would be to use immutable attributes
> of the
> > WAL files as an input to the key derivation. Something like HKDF(MDEK,
> "WAL:" |
>
> So, I am thinking we should use "WAL:" for WAL and "REL:" for heap/index
> files.
>

Sounds good. Any unique convention is fine. Main thing to keep in mind is
that they're directly tied to the master key so it's not possible to rotate
them without changing the master key.

This is in contrast to saving a WDEK key to a file (similar to how the MDEK
key would be saved) and unlocking it with the MDEK. That has more moving
parts but would allow that key to be independent of the MDEK. In a later
message Stephen refers to an example of a replica receiving encrypted WAL
and applying it with a different MDEK for the page buffers. That's doable
with an independent WDEK.


> > | timeline_id || wal_segment_num) should be fine for this as it is:
>
> I considered using the timeline in the nonce, but then remembered that
> in timeline switch, we _copy_ the part of the old WAL up to the timeline
> switch to the new timeline;  see:
>
>
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/transam/xlog.c;h=f55352385732c6b0124eff5265462f3883fe7435;hb=HEAD#l5502
>
>* Initialize the starting WAL segment for the new timeline. If the
> switch
>* happens in the middle of a segment, copy data from the last WAL
> segment
>* of the old timeline up to the switch point, to the starting WAL
> segment
>* on the new timeline.
>
> We would need to decrypt/encrypt to do the copy, and just wasn't sure of
> the value of the timeline in the nonce.  One value to it is that if
> there some WAL that generated after the timeline switch in the old
> primary that isn't transfered, there would be potentially new data
> encrypted with the same key/nonce in the new primary, but if that WAL is
> not used, odds are it is gone/destroyed/inaccessible, or it would have
> been used during the switchover, so it didn't seem worth worrying about.
>
> One _big_ reason to add the timeline is if you had a standby that you
> recovered and rolled forward only to a specific transaction, then
> continued running it as a new primary.  In that case, you would have
> different WAL encrypted with the same key/nonce, but that sounds like
> the same as promoting two standbys, and we should just document not to
> do it.
>
> Maybe we need to consider this further.
>

Good points. Yes, anything short of generating a new key at promotion time
will have these issues. If we're not going to do that, no point in adding
the timeline id if it does not change anything. I had thought only the
combo was unique but sounds like the segment number is unique on its own.
One thing I like about a unique per-file key is that it simplifies the IV
generation (i.e. can start at zero).

What about discarding the rest of the WAL file at promotion and skipping to
a new file? With a random per-file key in the first page header would
ensure that going forward all WAL data is encrypted differently. Combine
that with independent WAL and MDEK keys and everything would be different
between two replicas promoted from the same point.


> > A unique WDEK per WAL file that is derived from the segment number would
> not
> > have that problem. A unique key per-file means the IVs can all start at
> zero
> > and the each file can be treated as one encrypted stream. Any encryption/
> > decryption code would only need to touch the write/read callsites.
>
> So, I am now wondering when we should be using a non-zero nonce to
> start, and when we should be using derived keys.   Should we add the
> page-number to the derived key for heap/index files too and just use the
> LSN for nonce, or add the LSN to the derived key too?
>

The main cost of using multiple keys is that you need to derive or unlock
them for each usage.

A per-type, per-relation, or per-file derived key with the same
non-repeating guarantees for the IV (ex: LSN + Page Number) is as secure
but allows for caching all needed derived keys in memory (it's one per open
file descriptor).

Having page-level derived keys incorporating the LSN + Page Number and
starting the per-page IV at zero works, but you'd have to perform an HKDF
for each page read or write. A cache of those derived keys would be much
larger (32-bytes per page) so presumably you're not going to have them all
cached or maybe not bother with any caching,


> > Even without a per-page MAC, a MAC at some level for WAL has its own
> benefits
> > such as perfect corruption detection. It could be per-record,
> per-N-records,
> > per-checkpoint, or per-file. The current WAL file format already handles
> > arbitrary gaps so there is significantly more flexibility in adding it vs
> > pages. I'm not saying it should be a requirement but, unlike pages, I
> would not

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Thu, Aug  8, 2019 at 06:31:42PM -0400, Stephen Frost wrote:
> > >Crash recovery doesn't happen "all the time" and neither does vacuum
> > >freeze, and autovacuum processes are independent of individual client
> > >backends- we don't need to (and shouldn't) have the keys in shared
> > >memory.
> > 
> > Don't people do physical replication / HA pretty much all the time?
> 
> Strictly speaking, that isn't actually crash recovery, it's physical
> replication / HA, and while those are certainly nice to have it's no
> guarantee that they're required or that you'd want to have the same keys
> for them- conceptually, at least, you could have WAL with one key that
> both sides know and then different keys for the actual data files, if we
> go with the approach where the WAL is encrypted with one key and then
> otherwise is plaintext.

Uh, yes, you could have two encryption keys in the data directory, one
for heap/indexes, one for WAL, both unlocked with the same passphrase,
but what would be the value in that?

> > >>That might allow crash recovery and the freeze part of VACUUM FREEZE to
> > >>work.  (I don't think we could vacuum since we couldn't read the index
> > >>pages to find the matching rows since the index values would be encrypted
> > >>too.  We might be able to not encrypt the tid in the index typle.)
> > >
> > >Why do we need the indexed values to vacuum the index..?  We don't
> > >today, as I recall.  We would need the tids though, yes.
> > 
> > Well, we also do collect statistics on the data, for example. But even
> > if we assume we wouldn't do that for encrypted indexes (which seems like
> > a pretty bad idea to me), you'd probably end up leaking information
> > about ordering of the values. Which is generally a pretty serious
> > information leak, AFAICS.
> 
> I agree entirely that order information would be bad to leak- but this
> is all new ground here and we haven't actually sorted out what such a
> partially encrypted btree would look like.  We don't actually have to
> have the down-links in the tree be unencrypted to allow vacuuming of
> leaf pages, after all.

Agreed, but I think we kind of know that the value in cluster-wide
encryption is different from multi-key encryption --- both have their
value, but right now cluster-wide is the easiest and simplest, and
probably meets more user needs than multi-key encryption.  If others
want to start scoping out what multi-key encryption would look like, we
can discuss it.  I personally would like to focus on cluster-wide
encryption for PG 13.

> > >>Is this something considering in version one of this feature?  Probably
> > >>not, but later?  Never?  Would the information leakage be too great,
> > >>particularly from indexes?
> > >
> > >What would be leaking from the indexes..?  That an encrypted blob in the
> > >index pointed to a given tid?  Wouldn't someone be able to see that same
> > >information by looking directly at the relation too?
> > 
> > Ordering of values, for example. Depending on how exactly the data is
> > encrypted we might also be leaking information about which values are
> > equal, etc. It also seems quite a bit more expensive to use such index.
> 
> Using an encrypted index isn't going to be free.  It's not clear that
> this would be much more expensive than if the entire index is encrypted,
> or that people would actually be unhappy if there was such an additional
> expense if it meant that they could have vacuum run without the keys.

Yes, I think it is information leakage that is always going to make
multi-key unable to fulfill all the features of cluster-wide encryption.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Thu, Aug  8, 2019 at 03:07:59PM -0400, Stephen Frost wrote:
> Greetings,
> 
> * Bruce Momjian (br...@momjian.us) wrote:
> > On Tue, Jul  9, 2019 at 11:09:01AM -0400, Bruce Momjian wrote:
> > > On Tue, Jul  9, 2019 at 10:59:12AM -0400, Stephen Frost wrote:
> > > > * Bruce Momjian (br...@momjian.us) wrote:
> > > > I agree that all of that isn't necessary for an initial implementation,
> > > > I was rather trying to lay out how we could improve on this in the
> > > > future and why having the keying done at a tablespace level makes sense
> > > > initially because we can then potentially move forward with further
> > > > segregation to improve the situation.  I do believe it's also useful in
> > > > its own right, to be clear, just not as nice since a compromised backend
> > > > could still get access to data in shared buffers that it really
> > > > shouldn't be able to, even broadly, see.
> > > 
> > > I think TDE is feature of questionable value at best and the idea that
> > > we would fundmentally change the internals of Postgres to add more
> > > features to it seems very unlikely.  I realize we have to discuss it so
> > > we don't block reasonable future feature development.
> > 
> > I have a new crazy idea.  I know we concluded that allowing multiple
> > independent keys, e.g., per user, per table, didn't make sense since
> > they have to be unlocked all the time, e.g., for crash recovery and
> > vacuum freeze.
> 
> I'm a bit confused as I never agreed that made any sense and I continue
> to feel that it doesn't make sense to have one key for everything.

I clearly explained why multiple keys, while desirable, have many
negatives.  If you want to address my replies, we can go over them
again.  What people want, and what we can reasonably accomplish, are two
different things.

> Crash recovery doesn't happen "all the time" and neither does vacuum
> freeze, and autovacuum processes are independent of individual client
> backends- we don't need to (and shouldn't) have the keys in shared
> memory.

Uh, I just don't know what that would look like, honestly.  I am trying
to get us toward something that is easily implemented and easy to
control.
 
> > However, that assumes that all heap/index pages are encrypted, and all
> > of WAL.  What if we encrypted only the user-data part of the page, i.e.,
> > tuple data.  We left xmin/xmax unencrypted, and only stored the
> > encrypted part of that data in WAL, and didn't encrypt any more of WAL. 
> 
> This is pretty much what Alvaro was suggesting a while ago, isn't it..?
> Have just the user data be encrypted in the table and in the WAL stream.

Well, I think he was saying that to reduce the overhead of encryption. 
I didn't see it as a way of allowing recovery and vacuum freeze.  My
exact reply was:

> Well, you would need to decide what WAL information needs to be secured.
> Is the fact an insert was performed on a table a security issue?
> Depends on your risks.  My point is that almost anything you do beyond
> cluster-level encryption either adds complexity that is bug-prone or
> fragile, or adds unacceptable overhead, or leaks security information.

> > That might allow crash recovery and the freeze part of VACUUM FREEZE to
> > work.  (I don't think we could vacuum since we couldn't read the index
> > pages to find the matching rows since the index values would be encrypted
> > too.  We might be able to not encrypt the tid in the index typle.)
> 
> Why do we need the indexed values to vacuum the index..?  We don't
> today, as I recall.  We would need the tids though, yes.

Uh, well, if we are doing index cleaning by doing a sequential scan of
the index, which I think we have done for many years, I think just
looking at the tids should work.  However, I don't know if we ever
adjust index entries, like re-balance the trees.

> > Is this something considering in version one of this feature?  Probably
> > not, but later?  Never?  Would the information leakage be too great,
> > particularly from indexes?
> 
> What would be leaking from the indexes..?  That an encrypted blob in the
> index pointed to a given tid?  Wouldn't someone be able to see that same
> information by looking directly at the relation too?

Well, I assume we would encrypt the heap and its indexes.  For example,
if there is an employee table, and there is an index on the employee
last name and employee salary, it would be trivial to get a list of
employee salaries sorted by last name by just joining the tids, though
you would not know the last names.  That seems like an information leak
to me.  Plus, which tables were updated would be visible in WAL.  And we
would have issues with system tables, pg_statistics, and lots of other
complexity.

I can see value in eventually doing this, perhaps before we perform
cluster-wide encryption, but doing it without cluster-wide encryption
seems like it would leak too much information to be useful.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB  

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Stephen Frost]

Greetings,

* Tomas Vondra (tomas.von...@2ndquadrant.com) wrote:
> On Thu, Aug 08, 2019 at 03:07:59PM -0400, Stephen Frost wrote:
> >* Bruce Momjian (br...@momjian.us) wrote:
> >>On Tue, Jul  9, 2019 at 11:09:01AM -0400, Bruce Momjian wrote:
> >>> On Tue, Jul  9, 2019 at 10:59:12AM -0400, Stephen Frost wrote:
> >>> > * Bruce Momjian (br...@momjian.us) wrote:
> >>> > I agree that all of that isn't necessary for an initial implementation,
> >>> > I was rather trying to lay out how we could improve on this in the
> >>> > future and why having the keying done at a tablespace level makes sense
> >>> > initially because we can then potentially move forward with further
> >>> > segregation to improve the situation.  I do believe it's also useful in
> >>> > its own right, to be clear, just not as nice since a compromised backend
> >>> > could still get access to data in shared buffers that it really
> >>> > shouldn't be able to, even broadly, see.
> >>>
> >>> I think TDE is feature of questionable value at best and the idea that
> >>> we would fundmentally change the internals of Postgres to add more
> >>> features to it seems very unlikely.  I realize we have to discuss it so
> >>> we don't block reasonable future feature development.
> >>
> >>I have a new crazy idea.  I know we concluded that allowing multiple
> >>independent keys, e.g., per user, per table, didn't make sense since
> >>they have to be unlocked all the time, e.g., for crash recovery and
> >>vacuum freeze.
> >
> >I'm a bit confused as I never agreed that made any sense and I continue
> >to feel that it doesn't make sense to have one key for everything.
> >
> >Crash recovery doesn't happen "all the time" and neither does vacuum
> >freeze, and autovacuum processes are independent of individual client
> >backends- we don't need to (and shouldn't) have the keys in shared
> >memory.
> 
> Don't people do physical replication / HA pretty much all the time?

Strictly speaking, that isn't actually crash recovery, it's physical
replication / HA, and while those are certainly nice to have it's no
guarantee that they're required or that you'd want to have the same keys
for them- conceptually, at least, you could have WAL with one key that
both sides know and then different keys for the actual data files, if we
go with the approach where the WAL is encrypted with one key and then
otherwise is plaintext.

> >>However, that assumes that all heap/index pages are encrypted, and all
> >>of WAL.  What if we encrypted only the user-data part of the page, i.e.,
> >>tuple data.  We left xmin/xmax unencrypted, and only stored the
> >>encrypted part of that data in WAL, and didn't encrypt any more of WAL.
> >
> >This is pretty much what Alvaro was suggesting a while ago, isn't it..?
> >Have just the user data be encrypted in the table and in the WAL stream.
> 
> It's also moving us much closer to pgcrypto-style encryption ...

Yes, it is, and there's good parts and bad parts to that, to be sure.

> >>That might allow crash recovery and the freeze part of VACUUM FREEZE to
> >>work.  (I don't think we could vacuum since we couldn't read the index
> >>pages to find the matching rows since the index values would be encrypted
> >>too.  We might be able to not encrypt the tid in the index typle.)
> >
> >Why do we need the indexed values to vacuum the index..?  We don't
> >today, as I recall.  We would need the tids though, yes.
> 
> Well, we also do collect statistics on the data, for example. But even
> if we assume we wouldn't do that for encrypted indexes (which seems like
> a pretty bad idea to me), you'd probably end up leaking information
> about ordering of the values. Which is generally a pretty serious
> information leak, AFAICS.

I agree entirely that order information would be bad to leak- but this
is all new ground here and we haven't actually sorted out what such a
partially encrypted btree would look like.  We don't actually have to
have the down-links in the tree be unencrypted to allow vacuuming of
leaf pages, after all.

> >>Is this something considering in version one of this feature?  Probably
> >>not, but later?  Never?  Would the information leakage be too great,
> >>particularly from indexes?
> >
> >What would be leaking from the indexes..?  That an encrypted blob in the
> >index pointed to a given tid?  Wouldn't someone be able to see that same
> >information by looking directly at the relation too?
> 
> Ordering of values, for example. Depending on how exactly the data is
> encrypted we might also be leaking information about which values are
> equal, etc. It also seems quite a bit more expensive to use such index.

Using an encrypted index isn't going to be free.  It's not clear that
this would be much more expensive than if the entire index is encrypted,
or that people would actually be unhappy if there was such an additional
expense if it meant that they could have vacuum run without the keys.

Thanks,

Stephen


signature.asc
Description: PGP 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Tomas Vondra]

On Thu, Aug 08, 2019 at 03:07:59PM -0400, Stephen Frost wrote:

Greetings,

* Bruce Momjian (br...@momjian.us) wrote:

On Tue, Jul  9, 2019 at 11:09:01AM -0400, Bruce Momjian wrote:
> On Tue, Jul  9, 2019 at 10:59:12AM -0400, Stephen Frost wrote:
> > * Bruce Momjian (br...@momjian.us) wrote:
> > I agree that all of that isn't necessary for an initial implementation,
> > I was rather trying to lay out how we could improve on this in the
> > future and why having the keying done at a tablespace level makes sense
> > initially because we can then potentially move forward with further
> > segregation to improve the situation.  I do believe it's also useful in
> > its own right, to be clear, just not as nice since a compromised backend
> > could still get access to data in shared buffers that it really
> > shouldn't be able to, even broadly, see.
>
> I think TDE is feature of questionable value at best and the idea that
> we would fundmentally change the internals of Postgres to add more
> features to it seems very unlikely.  I realize we have to discuss it so
> we don't block reasonable future feature development.

I have a new crazy idea.  I know we concluded that allowing multiple
independent keys, e.g., per user, per table, didn't make sense since
they have to be unlocked all the time, e.g., for crash recovery and
vacuum freeze.


I'm a bit confused as I never agreed that made any sense and I continue
to feel that it doesn't make sense to have one key for everything.

Crash recovery doesn't happen "all the time" and neither does vacuum
freeze, and autovacuum processes are independent of individual client
backends- we don't need to (and shouldn't) have the keys in shared
memory.



Don't people do physical replication / HA pretty much all the time?



However, that assumes that all heap/index pages are encrypted, and all
of WAL.  What if we encrypted only the user-data part of the page, i.e.,
tuple data.  We left xmin/xmax unencrypted, and only stored the
encrypted part of that data in WAL, and didn't encrypt any more of WAL.


This is pretty much what Alvaro was suggesting a while ago, isn't it..?
Have just the user data be encrypted in the table and in the WAL stream.



It's also moving us much closer to pgcrypto-style encryption ...


That might allow crash recovery and the freeze part of VACUUM FREEZE to
work.  (I don't think we could vacuum since we couldn't read the index
pages to find the matching rows since the index values would be encrypted
too.  We might be able to not encrypt the tid in the index typle.)


Why do we need the indexed values to vacuum the index..?  We don't
today, as I recall.  We would need the tids though, yes.



Well, we also do collect statistics on the data, for example. But even
if we assume we wouldn't do that for encrypted indexes (which seems like
a pretty bad idea to me), you'd probably end up leaking information
about ordering of the values. Which is generally a pretty serious
information leak, AFAICS.


Is this something considering in version one of this feature?  Probably
not, but later?  Never?  Would the information leakage be too great,
particularly from indexes?


What would be leaking from the indexes..?  That an encrypted blob in the
index pointed to a given tid?  Wouldn't someone be able to see that same
information by looking directly at the relation too?



Ordering of values, for example. Depending on how exactly the data is
encrypted we might also be leaking information about which values are
equal, etc. It also seems quite a bit more expensive to use such index.


regards

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

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Stephen Frost]

Greetings,

* Bruce Momjian (br...@momjian.us) wrote:
> On Tue, Jul  9, 2019 at 11:09:01AM -0400, Bruce Momjian wrote:
> > On Tue, Jul  9, 2019 at 10:59:12AM -0400, Stephen Frost wrote:
> > > * Bruce Momjian (br...@momjian.us) wrote:
> > > I agree that all of that isn't necessary for an initial implementation,
> > > I was rather trying to lay out how we could improve on this in the
> > > future and why having the keying done at a tablespace level makes sense
> > > initially because we can then potentially move forward with further
> > > segregation to improve the situation.  I do believe it's also useful in
> > > its own right, to be clear, just not as nice since a compromised backend
> > > could still get access to data in shared buffers that it really
> > > shouldn't be able to, even broadly, see.
> > 
> > I think TDE is feature of questionable value at best and the idea that
> > we would fundmentally change the internals of Postgres to add more
> > features to it seems very unlikely.  I realize we have to discuss it so
> > we don't block reasonable future feature development.
> 
> I have a new crazy idea.  I know we concluded that allowing multiple
> independent keys, e.g., per user, per table, didn't make sense since
> they have to be unlocked all the time, e.g., for crash recovery and
> vacuum freeze.

I'm a bit confused as I never agreed that made any sense and I continue
to feel that it doesn't make sense to have one key for everything.

Crash recovery doesn't happen "all the time" and neither does vacuum
freeze, and autovacuum processes are independent of individual client
backends- we don't need to (and shouldn't) have the keys in shared
memory.

> However, that assumes that all heap/index pages are encrypted, and all
> of WAL.  What if we encrypted only the user-data part of the page, i.e.,
> tuple data.  We left xmin/xmax unencrypted, and only stored the
> encrypted part of that data in WAL, and didn't encrypt any more of WAL. 

This is pretty much what Alvaro was suggesting a while ago, isn't it..?
Have just the user data be encrypted in the table and in the WAL stream.

> That might allow crash recovery and the freeze part of VACUUM FREEZE to
> work.  (I don't think we could vacuum since we couldn't read the index
> pages to find the matching rows since the index values would be encrypted
> too.  We might be able to not encrypt the tid in the index typle.)

Why do we need the indexed values to vacuum the index..?  We don't
today, as I recall.  We would need the tids though, yes.

> Is this something considering in version one of this feature?  Probably
> not, but later?  Never?  Would the information leakage be too great,
> particularly from indexes?

What would be leaking from the indexes..?  That an encrypted blob in the
index pointed to a given tid?  Wouldn't someone be able to see that same
information by looking directly at the relation too?

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Tue, Jul  9, 2019 at 11:09:01AM -0400, Bruce Momjian wrote:
> On Tue, Jul  9, 2019 at 10:59:12AM -0400, Stephen Frost wrote:
> > * Bruce Momjian (br...@momjian.us) wrote:
> > I agree that all of that isn't necessary for an initial implementation,
> > I was rather trying to lay out how we could improve on this in the
> > future and why having the keying done at a tablespace level makes sense
> > initially because we can then potentially move forward with further
> > segregation to improve the situation.  I do believe it's also useful in
> > its own right, to be clear, just not as nice since a compromised backend
> > could still get access to data in shared buffers that it really
> > shouldn't be able to, even broadly, see.
> 
> I think TDE is feature of questionable value at best and the idea that
> we would fundmentally change the internals of Postgres to add more
> features to it seems very unlikely.  I realize we have to discuss it so
> we don't block reasonable future feature development.

I have a new crazy idea.  I know we concluded that allowing multiple
independent keys, e.g., per user, per table, didn't make sense since
they have to be unlocked all the time, e.g., for crash recovery and
vacuum freeze.

However, that assumes that all heap/index pages are encrypted, and all
of WAL.  What if we encrypted only the user-data part of the page, i.e.,
tuple data.  We left xmin/xmax unencrypted, and only stored the
encrypted part of that data in WAL, and didn't encrypt any more of WAL. 
That might allow crash recovery and the freeze part of VACUUM FREEZE to
work.  (I don't think we could vacuum since we couldn't read the index
pages to find the matching rows since the index values would be encrypted
too.  We might be able to not encrypt the tid in the index typle.)

Is this something considering in version one of this feature?  Probably
not, but later?  Never?  Would the information leakage be too great,
particularly from indexes?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Wed, Aug  7, 2019 at 07:40:05PM -0400, Sehrope Sarkuni wrote:
> With the provisos above, yes I think that would work though I don't think it's
> a good idea. Better to start off using the functions directly and then look
> into optimizing only if they're a bottleneck. As a first pass I'd break it up
> as separate writes with the encryption happening at write time. If that works
> fine there's no need to complicate things further.

OK, sounds like a plan!

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Wed, Aug  7, 2019 at 08:56:18AM -0400, Sehrope Sarkuni wrote:
> On Mon, Aug 5, 2019 at 9:02 PM Bruce Momjian  wrote:
> 
> On Wed, Jul 31, 2019 at 09:25:01AM -0400, Sehrope Sarkuni wrote:
> > Even if we do not include a separate per-relation salt or things like
> > relfilenode when generating a derived key, we can still include other
> types of
> > immutable attributes. For example the fork type could be included to
> eventually
> > allow multiple forks for the same relation to be encrypted with the same
> IV =
> > LSN + Page Number as the derived key per-fork would be distinct.
> 
> Yes, the fork number could be useful in this case.  I was thinking we
> would just leave the extra bits as zeros and we can then set it to '1'
> or something else for a different fork.
> 
> 
> Key derivation has more flexibility as you're not limited by the number of
> unused bits in the IV.

Understood, though I was not aware of the usefulness of key derivation
until this thread.

> > WAL encryption should not use the same key as page encryption so there's
> no
> > need to design the IV to try to avoid matching the page IVs. Even a 
> basic
> > derivation with a single fixed WDEK = HKDF(MDEK, "WAL") and TDEK = HKDF
> (MDEK,
> > "PAGE") would ensure separate keys. That's the the literal string "WAL"
> or
> > "PAGE" being added as a salt to generate the respective keys, all that
> matters
> > is they're different.
> 
> I was thinking the WAL would use the same key since the nonce is unique
> between the two.  What value is there in using a different key?
> 
> 
> Never having to worry about overlap in Key + IV usage is main advantage. While
> it's possible to structure IVs to avoid that from happening, it's much easier
> to completely avoid that situation by ensuring different parts of an
> application are using separate derived keys.

Understood.

> > Ideally WAL encryption would generating new derived keys as part of the
> WAL
> > stream. The WAL stream is not fixed so you have the luxury of being able
> to add
> > a "Use new random salt XZY going forward" records. Forcing generation of
> a new
> > salt/key upon promotion of a replica would ensure that at least the WAL
> is
> > unique going forward. Could also generate a new upon server startup,
> after
> 
> Ah, yes, good point, and using a derived key would make that easier.
> The tricky part is what to use to create the new derived key, unless we
> generate a random number and store that somewhere in the data directory,
> but that might lead to fragility, so I am worried. 
> 
> 
> Simplest approach for derived keys would be to use immutable attributes of the
> WAL files as an input to the key derivation. Something like HKDF(MDEK, "WAL:" 
> |

So, I am thinking we should use "WAL:" for WAL and "REL:" for heap/index
files.

> | timeline_id || wal_segment_num) should be fine for this as it is:

I considered using the timeline in the nonce, but then remembered that
in timeline switch, we _copy_ the part of the old WAL up to the timeline
switch to the new timeline;  see:


https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/transam/xlog.c;h=f55352385732c6b0124eff5265462f3883fe7435;hb=HEAD#l5502

   * Initialize the starting WAL segment for the new timeline. If the switch
   * happens in the middle of a segment, copy data from the last WAL segment
   * of the old timeline up to the switch point, to the starting WAL segment
   * on the new timeline.

We would need to decrypt/encrypt to do the copy, and just wasn't sure of
the value of the timeline in the nonce.  One value to it is that if
there some WAL that generated after the timeline switch in the old
primary that isn't transfered, there would be potentially new data
encrypted with the same key/nonce in the new primary, but if that WAL is
not used, odds are it is gone/destroyed/inaccessible, or it would have
been used during the switchover, so it didn't seem worth worrying about.

One _big_ reason to add the timeline is if you had a standby that you
recovered and rolled forward only to a specific transaction, then
continued running it as a new primary.  In that case, you would have
different WAL encrypted with the same key/nonce, but that sounds like
the same as promoting two standbys, and we should just document not to
do it.

Maybe we need to consider this further.

> * Unique per WAL file
> * Known prior to writing to a given WAL file
> * Known prior to reading a given WAL file
> * Does not require any additional persistence

Agreed.

> We have pg_rewind,
> which allows to make the WAL go backwards.  What is the value in doing
> this?
> 
> 
> Good point re: pg_rewind. Having key rotation records in the stream would
> complicate that as you'd have to jump back / forward to figure out which key 
> to
> use. It's doable but much more 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Sehrope Sarkuni]

On Wed, Aug 7, 2019 at 1:39 PM Bruce Momjian  wrote:

> On Wed, Aug  7, 2019 at 11:41:51AM -0400, Sehrope Sarkuni wrote:
> > On Wed, Aug 7, 2019 at 7:19 AM Bruce Momjian  wrote:
> >
> > On Wed, Aug  7, 2019 at 05:13:31PM +0900, Masahiko Sawada wrote:
> > > I understood. IIUC in your approach postgres processes encrypt WAL
> > > records when inserting to the WAL buffer. So WAL data is encrypted
> > > even on the WAL buffer.
> >
> >
> > I was originally thinking of not encrypting the shared WAL buffers but
> that may
> > have issues. If the buffers are already encrypted and contiguous in
> shared
> > memory, it's possible to write out many via a single pg_pwrite(...) call
> as is
> > currently done in XLogWrite(...).
>
> The shared buffers will not be encrypted --- they are encrypted only
> when being written to storage.  We felt encrypting shared buffers will
> be too much overhead, for little gain.  I don't know if we will encrypt
> while writing to the WAL buffers or while writing the WAL buffers to
> the file system.
>

My mistake on the wording. By "shared WAL buffers" I meant the shared
memory used for WAL buffers, XLogCtl->pages. Not the shared buffers for
pages.


> > If they're not encrypted you'd need to do more work in that critical
> section.
> > That'd involve allocating a commensurate amount of memory to hold the
> encrypted
> > pages and then encrypting them all prior to the single pg_pwrite(...)
> call.
> > Reusing one buffer is possible but it would require encrypting and
> writing the
> > pages one by one. Both of those seem like a bad idea.
>
> Well, right now the 8k pages is part of the WAL stream, so I don't know
> it would be any more overhead than other WAL writes.


The total work is the same but when it happens, memory usage, or number of
syscalls could change.

Right now the XLogWrite(...) code can write many WAL pages at once via a
single call to pg_pwrite(...):
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/transam/xlog.c;h=f55352385732c6b0124eff5265462f3883fe7435;hb=HEAD#l2491

If the blocks are not encrypted then you either need to allocate and
encrypt everything (could be up to wal_buffers max size) to do it as one
write, or encrypt chunks of WAL and do multiple writes. I'm not sure how
big an issue this would be in practice as it'd be workload specific.


> I am hoping we can
> generate the encryption bit stream in chunks earlier so we can just to
> the XOR was we are writing the data to the WAL buffers.
>

For pure CTR that sounds doable as it'd be the same as doing an XOR with
encrypted zero. Anything with a built-in MAC like GCM would not work though
(I'm not proposing we use that, just keeping it in mind).

You'd also increase your memory requirements (one allocation for the
encrypted stream and one for the encrypted data right?).


> > Better to pay the encryption cost at the time of WAL record creation and
> keep
> > the writing process as fast and simple as possible.
>
> Yes, I don't think we know at the time of WAL record creation what
> _offset_ the records will have when then are written to WAL, so I am
> thinking we need to do it later, and as I said, I am hoping we can
> generate the encryption bit stream earlier.
>
> > > It works but I think the implementation might be complex; For
> example
> > > using openssl, we would use EVP functions to encrypt data by
> > > AES-256-CTR. We would need to make IV and pass it to them and these
> > > functions however don't manage the counter value of nonce as long
> as I
> > > didn't miss. That is, we need to calculate the correct counter
> value
> > > for each encryption and pass it to EVP functions. Suppose we
> encrypt
> > > 20 bytes of WAL. The first 16 bytes is encrypted with nonce of
> > > (segment_number, 0) and the next 4 bytes is encrypted with nonce of
> > > (segment_number, 1). After that suppose we encrypt 12 bytes of
> WAL. We
> > > cannot use nonce of (segment_number, 2) but should use nonce of
> > > (segment_number , 1). Therefore we would need 4 bytes padding and
> to
> > > encrypt it and then to throw that 4 bytes away .
> >
> > Since we want to have per-byte control over encryption, for both
> > heap/index pages (skip LSN and CRC), and WAL (encrypt to the last
> byte),
> > I assumed we would need to generate a bit stream of a specified size
> and
> > do the XOR ourselves against the data.  I assume ssh does this, so we
> > would have to study the method.
> >
> >
> > The lower level non-EVP OpenSSL functions allow specifying the offset
> within
> > the 16-byte AES block from which the encrypt/decrypt should proceed.
> It's the
> > "num" parameter of their encrypt/decrypt functions. For a continuous
> encrypted
> > stream such as a WAL file, a "pread(...)" of a possibly non-16-byte
> aligned
> > section would involve determining the 16-byte counter (byte_offset / 16)
> and
> > the intra-block 

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Wed, Aug  7, 2019 at 11:41:51AM -0400, Sehrope Sarkuni wrote:
> On Wed, Aug 7, 2019 at 7:19 AM Bruce Momjian  wrote:
> 
> On Wed, Aug  7, 2019 at 05:13:31PM +0900, Masahiko Sawada wrote:
> > I understood. IIUC in your approach postgres processes encrypt WAL
> > records when inserting to the WAL buffer. So WAL data is encrypted
> > even on the WAL buffer.
> 
> 
> I was originally thinking of not encrypting the shared WAL buffers but that 
> may
> have issues. If the buffers are already encrypted and contiguous in shared
> memory, it's possible to write out many via a single pg_pwrite(...) call as is
> currently done in XLogWrite(...).

The shared buffers will not be encrypted --- they are encrypted only
when being written to storage.  We felt encrypting shared buffers will
be too much overhead, for little gain.  I don't know if we will encrypt
while writing to the WAL buffers or while writing the WAL buffers to
the file system.

> If they're not encrypted you'd need to do more work in that critical section.
> That'd involve allocating a commensurate amount of memory to hold the 
> encrypted
> pages and then encrypting them all prior to the single pg_pwrite(...) call.
> Reusing one buffer is possible but it would require encrypting and writing the
> pages one by one. Both of those seem like a bad idea.

Well, right now the 8k pages is part of the WAL stream, so I don't know
it would be any more overhead than other WAL writes.  I am hoping we can
generate the encryption bit stream in chunks earlier so we can just to
the XOR was we are writing the data to the WAL buffers.

> Better to pay the encryption cost at the time of WAL record creation and keep
> the writing process as fast and simple as possible.

Yes, I don't think we know at the time of WAL record creation what
_offset_ the records will have when then are written to WAL, so I am
thinking we need to do it later, and as I said, I am hoping we can
generate the encryption bit stream earlier.

> > It works but I think the implementation might be complex; For example
> > using openssl, we would use EVP functions to encrypt data by
> > AES-256-CTR. We would need to make IV and pass it to them and these
> > functions however don't manage the counter value of nonce as long as I
> > didn't miss. That is, we need to calculate the correct counter value
> > for each encryption and pass it to EVP functions. Suppose we encrypt
> > 20 bytes of WAL. The first 16 bytes is encrypted with nonce of
> > (segment_number, 0) and the next 4 bytes is encrypted with nonce of
> > (segment_number, 1). After that suppose we encrypt 12 bytes of WAL. We
> > cannot use nonce of (segment_number, 2) but should use nonce of
> > (segment_number , 1). Therefore we would need 4 bytes padding and to
> > encrypt it and then to throw that 4 bytes away .
> 
> Since we want to have per-byte control over encryption, for both
> heap/index pages (skip LSN and CRC), and WAL (encrypt to the last byte),
> I assumed we would need to generate a bit stream of a specified size and
> do the XOR ourselves against the data.  I assume ssh does this, so we
> would have to study the method.
> 
> 
> The lower level non-EVP OpenSSL functions allow specifying the offset within
> the 16-byte AES block from which the encrypt/decrypt should proceed. It's the
> "num" parameter of their encrypt/decrypt functions. For a continuous encrypted
> stream such as a WAL file, a "pread(...)" of a possibly non-16-byte aligned
> section would involve determining the 16-byte counter (byte_offset / 16) and
> the intra-block offset (byte_offset % 16). I'm not sure how one handles
> initializing the internal encrypted counter and that might be one more step
> that would need be done. But it's definitely possible to read / write less 
> than
> a block via those APIs (not the EVP ones).
> 
> I don't think the EVP functions have parameters for the intra-block offset but
> you can mimic it by initializing the IV/block counter and then skipping over
> the intra-block offset by either reading or writing a dummy partial block. The
> EVP read and write functions both deal with individual bytes so once you've
> seeked to your desired offset you can read or write the real individual bytes.

Can we generate the bit stream in 1MB chunks or something and just XOR
as needed?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Sehrope Sarkuni]

On Wed, Aug 7, 2019 at 7:19 AM Bruce Momjian  wrote:

> On Wed, Aug  7, 2019 at 05:13:31PM +0900, Masahiko Sawada wrote:
> > I understood. IIUC in your approach postgres processes encrypt WAL
> > records when inserting to the WAL buffer. So WAL data is encrypted
> > even on the WAL buffer.
>

I was originally thinking of not encrypting the shared WAL buffers but that
may have issues. If the buffers are already encrypted and contiguous in
shared memory, it's possible to write out many via a single pg_pwrite(...)
call as is currently done in XLogWrite(...).

If they're not encrypted you'd need to do more work in that critical
section. That'd involve allocating a commensurate amount of memory to hold
the encrypted pages and then encrypting them all prior to the single
pg_pwrite(...) call. Reusing one buffer is possible but it would require
encrypting and writing the pages one by one. Both of those seem like a bad
idea.

Better to pay the encryption cost at the time of WAL record creation and
keep the writing process as fast and simple as possible.


> > It works but I think the implementation might be complex; For example
> > using openssl, we would use EVP functions to encrypt data by
> > AES-256-CTR. We would need to make IV and pass it to them and these
> > functions however don't manage the counter value of nonce as long as I
> > didn't miss. That is, we need to calculate the correct counter value
> > for each encryption and pass it to EVP functions. Suppose we encrypt
> > 20 bytes of WAL. The first 16 bytes is encrypted with nonce of
> > (segment_number, 0) and the next 4 bytes is encrypted with nonce of
> > (segment_number, 1). After that suppose we encrypt 12 bytes of WAL. We
> > cannot use nonce of (segment_number, 2) but should use nonce of
> > (segment_number , 1). Therefore we would need 4 bytes padding and to
> > encrypt it and then to throw that 4 bytes away .
>
> Since we want to have per-byte control over encryption, for both
> heap/index pages (skip LSN and CRC), and WAL (encrypt to the last byte),
> I assumed we would need to generate a bit stream of a specified size and
> do the XOR ourselves against the data.  I assume ssh does this, so we
> would have to study the method.
>

The lower level non-EVP OpenSSL functions allow specifying the offset
within the 16-byte AES block from which the encrypt/decrypt should proceed.
It's the "num" parameter of their encrypt/decrypt functions. For a
continuous encrypted stream such as a WAL file, a "pread(...)" of a
possibly non-16-byte aligned section would involve determining the 16-byte
counter (byte_offset / 16) and the intra-block offset (byte_offset % 16).
I'm not sure how one handles initializing the internal encrypted counter
and that might be one more step that would need be done. But it's
definitely possible to read / write less than a block via those APIs (not
the EVP ones).

I don't think the EVP functions have parameters for the intra-block offset
but you can mimic it by initializing the IV/block counter and then skipping
over the intra-block offset by either reading or writing a dummy partial
block. The EVP read and write functions both deal with individual bytes so
once you've seeked to your desired offset you can read or write the real
individual bytes.

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Sehrope Sarkuni]

On Mon, Aug 5, 2019 at 9:02 PM Bruce Momjian  wrote:

> On Wed, Jul 31, 2019 at 09:25:01AM -0400, Sehrope Sarkuni wrote:
> > Even if we do not include a separate per-relation salt or things like
> > relfilenode when generating a derived key, we can still include other
> types of
> > immutable attributes. For example the fork type could be included to
> eventually
> > allow multiple forks for the same relation to be encrypted with the same
> IV =
> > LSN + Page Number as the derived key per-fork would be distinct.
>
> Yes, the fork number could be useful in this case.  I was thinking we
> would just leave the extra bits as zeros and we can then set it to '1'
> or something else for a different fork.
>

Key derivation has more flexibility as you're not limited by the number of
unused bits in the IV.


> > WAL encryption should not use the same key as page encryption so there's
> no
> > need to design the IV to try to avoid matching the page IVs. Even a basic
> > derivation with a single fixed WDEK = HKDF(MDEK, "WAL") and TDEK =
> HKDF(MDEK,
> > "PAGE") would ensure separate keys. That's the the literal string "WAL"
> or
> > "PAGE" being added as a salt to generate the respective keys, all that
> matters
> > is they're different.
>
> I was thinking the WAL would use the same key since the nonce is unique
> between the two.  What value is there in using a different key?
>

Never having to worry about overlap in Key + IV usage is main advantage.
While it's possible to structure IVs to avoid that from happening, it's
much easier to completely avoid that situation by ensuring different parts
of an application are using separate derived keys.


> > Ideally WAL encryption would generating new derived keys as part of the
> WAL
> > stream. The WAL stream is not fixed so you have the luxury of being able
> to add
> > a "Use new random salt XZY going forward" records. Forcing generation of
> a new
> > salt/key upon promotion of a replica would ensure that at least the WAL
> is
> > unique going forward. Could also generate a new upon server startup,
> after
>
> Ah, yes, good point, and using a derived key would make that easier.
> The tricky part is what to use to create the new derived key, unless we
> generate a random number and store that somewhere in the data directory,
> but that might lead to fragility, so I am worried.


Simplest approach for derived keys would be to use immutable attributes of
the WAL files as an input to the key derivation. Something like HKDF(MDEK,
"WAL:" || timeline_id || wal_segment_num) should be fine for this as it is:

* Unique per WAL file
* Known prior to writing to a given WAL file
* Known prior to reading a given WAL file
* Does not require any additional persistence

We have pg_rewind,
> which allows to make the WAL go backwards.  What is the value in doing
> this?
>

Good point re: pg_rewind. Having key rotation records in the stream would
complicate that as you'd have to jump back / forward to figure out which
key to use. It's doable but much more complicated.

A unique WDEK per WAL file that is derived from the segment number would
not have that problem. A unique key per-file means the IVs can all start at
zero and the each file can be treated as one encrypted stream. Any
encryption/decryption code would only need to touch the write/read
callsites.


> > every N bytes, or a new one for each new WAL file. There's much more
> > flexibility compared to page encryption.
> >
> > As WAL is a single continuous stream, we can start the IV for each
> derived WAL
> > key from zero. There's no need to complicate it further as Key + IV will
> never
> > be reused.
>
> Uh, you want a new random key for each WAL file?  I was going to use the
> WAL segment number as the nonce, which is always increasing, and easily
> determined.  The file is 16MB.
>

Ideally yes as it would allow for multiple replicas promoted off the same
primary to immediately diverge as each would have its own keys. I don't
consider it a requirement but if it's possible without significant added
complexity I say that's a win.

I'm still reading up on the file and record format to understand how
complex that would be. Though given your point re: pg_rewind and the lack
of handling for page encryption divergence when promoting multiple
replicas, I doubt the complexity will be worth it.


> > If WAL is always written as full pages we need to ensure that the empty
> parts
> > of the page are actual zeros and not "encrypted zeroes". Otherwise an
> XOR of
> > the empty section of the first write of a page against a subsequent one
> would
> > give you the plain text.
>
> Right, I think we need the segment number as part of the nonce for WAL.
>

+1 to using segment number but it's better as a derived key instead of
coming up with new IV constructs and reusing the MDEK.


> > The non-fixed size of the WAL allows for the addition of a MAC though
> I'm not
> > sure yet the best way to incorporate it. It could be part of each
> encrypted

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Bruce Momjian]

On Wed, Aug  7, 2019 at 05:13:31PM +0900, Masahiko Sawada wrote:
> I understood. IIUC in your approach postgres processes encrypt WAL
> records when inserting to the WAL buffer. So WAL data is encrypted
> even on the WAL buffer.
> 
> It works but I think the implementation might be complex; For example
> using openssl, we would use EVP functions to encrypt data by
> AES-256-CTR. We would need to make IV and pass it to them and these
> functions however don't manage the counter value of nonce as long as I
> didn't miss. That is, we need to calculate the correct counter value
> for each encryption and pass it to EVP functions. Suppose we encrypt
> 20 bytes of WAL. The first 16 bytes is encrypted with nonce of
> (segment_number, 0) and the next 4 bytes is encrypted with nonce of
> (segment_number, 1). After that suppose we encrypt 12 bytes of WAL. We
> cannot use nonce of (segment_number, 2) but should use nonce of
> (segment_number , 1). Therefore we would need 4 bytes padding and to
> encrypt it and then to throw that 4 bytes away .

Since we want to have per-byte control over encryption, for both
heap/index pages (skip LSN and CRC), and WAL (encrypt to the last byte),
I assumed we would need to generate a bit stream of a specified size and
do the XOR ourselves against the data.  I assume ssh does this, so we
would have to study the method.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Masahiko Sawada]

On Wed, Aug 7, 2019 at 2:55 AM Bruce Momjian  wrote:
>
> On Wed, Aug  7, 2019 at 12:31:58AM +0900, Masahiko Sawada wrote:
> > Well, so you mean that for example we encrypt only 100 bytes WAL
> > record when append 100 bytes WAL records?
> >
> > For WAL encryption, if we encrypt the entire 8k WAL page and write the
> > entire page, the encrypted-and-written page will contain 100 bytes WAL
> > record data and (8192-100) bytes garbage (omitted WAL page header for
> > simplify), although WAL data on WAL buffer is still not encrypted
> > state. And then if we append 200 bytes again, the
> > encrypted-and-written page will contain 300 bytes WAL record data and
> > (8192-300)bytes garbage, data on WAL buffer is still not encrypted
> > state though.
> >
> > In this case I think the first 100 bytes of two 8k WAL pages are the
> > same because we encrypted both from the beginning of the page with the
> > counter = 0. But the next 200 bytes are different; it's (encrypted)
> > garbage in the former case but it's (encrypted) WAL record data in the
> > latter case. I think that's a problem.
> >
> > On the other hand, if we encrypt 8k WAL page with the different
> > counter of nonce after append 200 byes WAL record, the first 100 byte
> > (and of course the entire 8k page also) will be different. However
> > since it's the same thing doing as changing already-flushed WAL record
> > on the disk it's bad.
> >
> > Also, if we encrypt only append data instead of entire 8k page, we
> > would need to have the information  in somewhere about how much byte
> > the WAL page has valid values. Otherwise reading WAL would not work
> > fine.
>
> OK, onlist reply.  We are going to encrypt the _entire_ WAL stream as we
> write it, which is possible with CTR.  If we write 200 bytes of WAL, we
> encrypt/XOR 200 bytes of WAL.  If we write 10k of WAL, and 8k of that is
> an 8k page, we encrypt the entire 10k of WAL --- we don't care if there
> is an 8k page in there or not.
>
> CTR mode creates a bit stream for the first 16 bytes with nonce of
> (segment_number, counter = 0), and the next 16 bytes with
> (segment_number, counter = 1), etc.  We only XOR using the parts of the
> bit stream we want to use.  We don't care what the WAL content is --- we
> just XOR it with the stream with the matching counter for that part of
> the WAL.
>
> It is true we are encrypting the same 8k page in the heap/index page,
> and in WAL, with different key/nonce combinations, which I think is
> secure.  What is insecure is to encrypt two different pieces of data
> with the same key/nonce combination.
>

I understood. IIUC in your approach postgres processes encrypt WAL
records when inserting to the WAL buffer. So WAL data is encrypted
even on the WAL buffer.

It works but I think the implementation might be complex; For example
using openssl, we would use EVP functions to encrypt data by
AES-256-CTR. We would need to make IV and pass it to them and these
functions however don't manage the counter value of nonce as long as I
didn't miss. That is, we need to calculate the correct counter value
for each encryption and pass it to EVP functions. Suppose we encrypt
20 bytes of WAL. The first 16 bytes is encrypted with nonce of
(segment_number, 0) and the next 4 bytes is encrypted with nonce of
(segment_number, 1). After that suppose we encrypt 12 bytes of WAL. We
cannot use nonce of (segment_number, 2) but should use nonce of
(segment_number , 1). Therefore we would need 4 bytes padding and to
encrypt it and then to throw that 4 bytes away .


Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center