Re: Proposed patch for key managment

> On 2 Dec 2020, at 22:38, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
> Attached is a patch for key management, which will eventually be part of
> cluster file encryption (CFE), called TDE (Transparent Data Encryption)
> by Oracle.

The attackvector protected against seems to be operating systems users
(*without* any legitimate database access) gaining access to the data files.
Such an attacker would also gain access to postgresql.conf and therefore may
have the cluster passphrase command in case it's stored there. That would
imply the attacker is likely (or perhaps better phrased "not entirely
unlikely") to be able to run that command and decrypt the data *iff* there is
no interactive/2fa aspect to the passphrase command. Is that an accurate
interpretation? Do Oracle TDE et.al use a similar setup where an database
restart require manual intervention?

Admittedly I haven't read the other threads on the topic so if it's discussed
somewhere else then I'd appreciate a whacking with a cluestick.

A few other comments from skimming the patch:

+ is data encryption keys, specifically keys zero and one. Key zero is
+ the key uses to encrypt database heap and index files which are stored in
".. is the key used to .."?

+ matches the initdb-supplied passphrase. If this check fails, and the
+ the server will refuse to start.
Seems like something is missing, perhaps just s/and the//?

+ <ulink url="https://tools.ietf.org/html/rfc2315">RFC2315</ulink>.</simpara>
Tiny nitpick: turns out that RFC 2315 (with a space) is the correct spelling.

+ * are read-only. All wrapping and unwrapping key routines depends on
+ * the openssl library for now.
OpenSSL is a name so it should be cased as OpenSSL in text like this.

#include <openssl/ssl.h>
+#include <openssl/conf.h>
Why do we need this header in be-secure-openssl.c? There are no other changes
to that file?

+/* Define to 1 if you have the `OPENSSL_init_crypto' function. */
+#undef HAVE_OPENSSL_INIT_CRYPTO
This seems to be unused?

KmgrSaveCryptoKeys doesn't seem to be explicitly clamping down permissions on
the generated file, is that something we want for belt-and-suspender level
paranoia around keys? The same goes for read_one_keyfile etc.

Also, KmgrSaveCryptoKeys assumes that keys are stored in plain files which is
true for OpenSSL but won't necessarily be true for other crypto libraries.
Would it make sense to have an API in be-kmgr.c with the OpenSSL specifics in
be-kmgr-openssl.c like how the TLS backend support is written? We have spent a
lot effort in making the backend not assume a particular TLS library, it seems
a shame to step away from that here with a tight coupling. (yes, I am biased)

The same goes for src/common/cipher.c which wraps calls in ifdefs, why not just
have an thin wrapper API which cipher-openssl.c implements?

+ case 'K':
+ file_encryption_keylen = atoi(optarg);
+ break;
atoi() will return zero on failing to parse the keylen, where 0 implies
"disabled". Wouldn't it make sense to parse this with code which can't
silently fall back on disabling in case of users mistyping?

+ * Skip cryptographic keys. It's generally not good idea to copy the
".. not *a* good idea .."

+ pg_log_fatal("cluser passphrase referenced %%R, but -R not specified");
s/cluser/cluster/ (there are few copy-pasteos of that elsewhere too)

+ elog(ERROR, "too many cryptographic kes");
s/kes/keys/

+#ifndef CIPHER_H
+#define CIPHER_H
The risk for headerguard collision with non-PG headers seem quite high, maybe
PG_CIPHER_H would be better?

I will try to dive in a bit deeper over the holidays.

cheers ./daniel