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

-----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 and
someof 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
codereview 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
forunexpected values instead of silently assigning to default 0/”off”.
 

* src/backend/storage/encryption/enc_cipher.c – For function EncryptionCipherString, purpose of returning ”unknown” if
unclearbecause 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_OFF
insteadof magic number 0
 

* src/backend/storage/encryption/kmgr.c - Function BootStrapKmgr will report error if USE_OPENSSL is not defined. The
checkseems premature because it would fail even if the user is not using encryption. Shouldn't the lack of openssl be
OKwhen 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
anexisting 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 uses
enumTDE_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". Although
maybeit is not very useful to say -e none, it does seem inconsistent to reject it, given that "none" was a valid value
forthe GUC variable data_encryption_cipher.
 

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

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

* src/backend/access/spgist/spginsert.c - In function spgbuildempty the arguments for PageEncryptionInPlace seem in the
wrongorder (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
asstring literals in many places (e.g. pg_controldata.c, initdb.c, guc.c, enc_cipher.c). Suggest it would be better to
declarethose as string constants in one place only.
 

---

Kind Regards,

Peter Smith
Fujitsu Australia