-----Original Message-----
From: Masahiko Sawada <sawada.m...@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 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
