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

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 <sawada(dot)mshk(at)gmail(dot)com> 于2019年10月31日周四 下午10:25写道：

> On Fri, Sep 6, 2019 at 3:34 PM Smith, Peter <peters(at)fast(dot)au(dot)fujitsu(dot)com>
> wrote:
> >
> > -----Original Message-----
> > From: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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 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] 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
>