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

Hello.

On Thu, Oct 31, 2019 at 11:25 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Sep 6, 2019 at 3:34 PM Smith, Peter <peters@fast.au.fujitsu.com> wrote:
> >
> > -----Original Message-----
> > From: Masahiko Sawada <sawada.mshk@gmail.com> 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
andsome 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
somecode 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
warningsfor unexpected values instead of silently assigning to default 0/”off”. 
> >
> > * src/backend/storage/encryption/enc_cipher.c – For function EncryptionCipherString, purpose of returning ”unknown”
ifunclear because that will map back to “off” again anyway via EncryptionCipherValue. Why not just return "off" (with
warninglogged). 
> >
> > * 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_OFFinstead of magic number 0 
> >
> > * src/backend/storage/encryption/kmgr.c - Function BootStrapKmgr will report error if USE_OPENSSL is not defined.
Thecheck seems premature because it would fail even if the user is not using encryption. Shouldn't the lack of openssl
beOK 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
ofan 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
seemsint everywhere else. 
> >
> > * src/backend/bootstrap/bootstrap.c - Suggest better if variable declaration bootstrap_data_encryption_cipher = 0
usesenum 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
insiststhat 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".
Althoughmaybe it is not very useful to say -e none, it does seem inconsistent to reject it, given that "none" was a
validvalue 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
inthe wrong order (forknum should be 2nd). 
> >
> > * src/backend/access/spgist/spginsert.c - In function spgbuildempty the arguments for PageEncryptionInPlace seem in
thewrong 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
scatteredas string literals in many places (e.g. pg_controldata.c, initdb.c, guc.c, enc_cipher.c). Suggest it would be
betterto 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.

Thank you for sharing the patch!

Currently, I'm checking the temporary file while watching Antonin's patch.
The discussion started about a month ago but did not provide a precise method.
The changes expected so far are
For temporary files (files generated by "file/buffile.c", except
logical/reorderbuffer.c"), currently overwriting situation was not
detected yet,
Like Antonin's opinion, the structure of “file/buffile.c” is very
likely to be overwritten. To solve this, we need to suppose the IV
value is changed.
The simplest method for IV values is to add a small header for each
page previously proposed by  Sawada-san and me and record the number
of overwrites in the header.
In other words, when overwriting is performed, the header value is
increased, and the IV value is changed.
Perhaps this part will require more discussion.

I'm researching such a method, but I want to spend more time on the
potential side effects.
If possible, I would like to make an effort to fix this part using the
patch shared by  Sawada-san.

There was also a reasonable opinion that it would be better to use the
structure of “file/buffile.c” for temporary files that created in
“logical/reorderbuffer.c”
This part was created in Antonin's patch before, and if there is no
problem, I would like to use it.

Of course, I may not have written the excellent quality code
correctly, so I will make an interim report if possible.

Best regards.
Moon.

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] https://www.postgresql.org/message-id/7082.1562337694%40localhost
> [2]
https://www.postgresql.org/message-id/CAD21AoBc-o%3DKZ%3DBPB5wWVNnBepqe8yqVs_D3eAd3Tr%3DX%3DtTGpQ%40mail.gmail.com
>
> Regards,
>
> --
> Masahiko Sawada