Re: Proposed patch for key managment

On Tue, Dec 15, 2020 at 10:36:56AM +0800, Neil Chen wrote:
> Hi, Bruce  
>
> I read your question and here are some of my thoughts.
>
>
>         Why is KmgrShmemData a struct, when it only has a single member? 
> Are
>         all shared memory areas structs?
>
>  
> Yes, all areas created by ShmemInitStruct() are structs. I think the
> significance of using struct is that it delimits an "area"  to store the KMS
> related things, which makes our memory management more clear and unified.

OK, thanks, that helps.

>         What testing infrastructure should this have?
>
>         There are a few shell script I should include to show how to create
>         commands.  Where should they be stored?  /contrib module?
>
>  
>
>         Are people okay with having the feature enabled, but invisible
>         since the docs and --help output are missing? When we enable
>         ssl_passphrase_command to prompt from the terminal, some of the
>         command-line options will be useful.
>
>  
> Since our implementation is not in contrib, I don't think we should put the
> script there. Maybe we can refer to postgresql.conf.sample?  

Uh, the script are 20-60 lines long --- I am attaching them to this
email. Plus, when we allow user prompting for the SSL passphrase, we
will have another script, or maybe three mor if people want to use a
Yubikey to unlock the SSL passphrase.

> Actually, I wonder whether we can add the UDK(user data encryption key) to the
> first version of KMS, which can be provided to plug-ins such as pgsodium. In
> this way, users can at least use it to a certain extent.

I don't thinK I want to get into that at this point. It can be done
later.

> Also, I have tested some KMS functionalities by adding very simple TDE logic.
> In the meantime, I found some confusion in the following places:

OK, I know Cybertec has a TDE patch too.

> 1. Previously, we added a variable bootstrap_keys_wrap that is used for
> encryption during initdb. However, since we save the "wrapped" key, we need to
> use a global KEK that can be accessed in boot mode to unwrap it before use... I
> don't know if that's good. To make it simple, I modified the
> bootstrap_keys_wrap to store the "unwrapped" key so that the encryption
> function can get it correctly. (The variable name should be changed
> accordingly).

I see what you are saying. We store the wrapped in bootstrap mode, but
the unwrapped in normal mode. There is also the case of when we copy
the keys from an old cluster. I will work on a patch tomorrow and
report back here.

> 2. I tried to add support for AES_CTR mode, and the code for encrypting buffer
> blocks. In the process I found that in pg_cipher_ctx_create, the key length is
> declared as "byte". However, in the CryptoKey structure, the length is stored
> as "bit", which leads me to use a form similar to Key->klen / 8 when I call
> this function. Maybe we should unify the two to avoid unnecessary confusion.

Agreed. I will look at that too tomorrow.

> 3. This one is not a problem/bug. I have some doubts about the length of data
> encryption blocks. For the page, we do not encrypt the PageHeaderData, which is
> 192 bit long. By default, the page size is 8K(65536 bits), so the length of the
> data we want to encrypt is 65344 bits. This number can't be divisible by 128
> and 192 keys and 256 bits long keys. Will this cause a problem?

Since we are using CTR mode for everything, it should be fine. We
encrypt WAL as it is written.

> Here is a simple patch that I wrote with references to Sawada's previous TDE
> works, hope it can help you.

Thanks. I will work on the items you mentioned. Can you look at the
Cybertec patch and then our wiki to see what needs to be done next?

https://wiki.postgresql.org/wiki/Transparent_Data_Encryption

Thanks for getting a proof-of-concept patch out there. :-)

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee