Re: Proposed patch for key managment

On Wed, Dec 9, 2020 at 10:34:59PM +0100, Daniel Gustafsson wrote:
> > 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?

I have updated the docs to say "read" access more clearly:

The purpose of cluster file encryption is to prevent users with read
access to the directories used to store database files from being able to
access the data stored in those files. For example, when using cluster
file encryption, users who have read access to the cluster directories
for backup purposes will not be able to read the data stored in the
these files.

I already said "read access" in the later part of the paragraph, but
obviously it needs to be mentioned early too. If we need more text
here, please let me know.

As far as someone hijacking the passphrase command, that is a serious
risk. No matter how you do the authentication, even TFA, you are going
to have someone able to grab that passphrase as it comes out of the
script and passes to the server. It might help to store the script and
postgres binaries in a more secure location than the data, but I am not
sure how realistic that is. I can if you are using a NAS for data store
and have local binaries and passphrase script, that would be more secure
than putting the script on the NAS. Is that something we should explain?
Should we explicity say that there is no protection against write
access? What can someone with write access to PGDATA do if
postgresql.conf is not stored there? I don't know.

> 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 .."?

Fixed, thanks.

> + 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//?

Fixed.

> + <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.

Fixed.

> + * 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.

Fixed, and fixed "depends".

> #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?

Not sure, removed, since it compiles fine without it.
>
> +/* Define to 1 if you have the `OPENSSL_init_crypto' function. */
> +#undef HAVE_OPENSSL_INIT_CRYPTO
> This seems to be unused?

Agreed, removed.

> 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.

Well, KmgrSaveCryptoKeys is calling BasicOpenFile, which is calling
BasicOpenFilePerm(fileName, fileFlags, pg_file_create_mode). The
question is whether we should honor the pg_file_create_mode specified on
the data directory, or make it tighter for these keys. The file is
already owner-rw-only by default:

$ ls -l
drwx------ 2 postgres postgres 4096 Dec 10 13:13 live/
$ cd live/
$ ls -l
total 8
-rw------- 1 postgres postgres 356 Dec 10 13:13 0
-rw------- 1 postgres postgres 356 Dec 10 13:13 1

If the key files were mixed in with a bunch of other files of lesser
value, like with Apache, I could see not honoring it, but for this case,
since all the files are of equal value, I think just honoring it makes
sense.

> 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)

I have no idea, sorry. I am mostly using the excellent routines
Sawada-san used in the original patch.

> 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?

Same. I am open to it, sure, but I would need a patch.

> + 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?

Good question, I found 43 references to atoi(optarg) in our code, so it
seems they all should be fixed, no? Not sure I want to be different here.

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

Thank, fixed

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

All fixed, thanks.

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

Fixed.
>
> +#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?

Agreed, fixed.

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

Thanks. The diff URL has all the updates.

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee