Re: Internal key management system

Hi

I had a look on kms_v9 patch and have some comments

--> pg_upgrade.c

keys are copied correctly, but as pg_upgrade progresses further, it will try to start the new_cluster from "issue_warnings_and_set_wal_level()" function, which is called after key copy. The new cluster will fail to start due to the mismatch between cluster_passphrase_command and the newly copied keys. This causes pg_upgrade to always finish with failure. We could move "copy_master_encryption_key()" to be called after "issue_warnings_and_set_wal_level()" and this will make pg_upgrade to finish with success, but user will still have to manually correct the "cluster_passphrase_command" param on the new cluster in order for it to start up correctly. Should pg_upgrade also take care of copying "cluster_passphrase_command" param from old to new cluster after it has copied the encryption keys so users don't have to do this step? If the expectation is for users to manually correct "cluster_passphrase_command" param after successful pg_upgrade and key copy, then there should be a message to remind the users to do so. 

-->Kmgr.c

+ /*

+ * If there is only temporary directory, it means that the previous

+ * rotation failed after wrapping the all internal keys by the new

+ * passphrase.  Therefore we use the new cluster passphrase.

+ */

+ if (stat(KMGR_DIR, &st) != 0)

+ {

+ ereport(DEBUG1,

+ (errmsg("both directories %s and %s exist, use the newly wrapped keys",

+ KMGR_DIR, KMGR_TMP_DIR)));

I think the error message should say "there is only temporary directory exist" instead of "both directories exist"

thanks!

Cary Huang

-------------

HighGo Software Inc. (Canada)

mailto:cary(dot)huang(at)highgo(dot)ca

http://www.highgo.ca

---- On Wed, 25 Mar 2020 01:51:08 -0700 Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote ----

On Tue, 24 Mar 2020 at 23:15, Bruce Momjian <mailto:bruce(at)momjian(dot)us> wrote:
>
> On Tue, Mar 24, 2020 at 02:29:57PM +0900, Masahiko Sawada wrote:
> > That seems to work fine.
> >
> > So we will have pg_cryptokeys within PGDATA and each key is stored
> > into separate file named the key id such as "sql", "tde-wal" and
> > "tde-block". I'll update the patch and post.
>
> Yes, that makes sense to me.
>

I've attached the updated patch. With the patch, we have three
internal keys: SQL key, TDE-block key and TDE-wal key. Only SQL key
can be used so far to wrap and unwrap user secret via pg_wrap and
pg_unwrap SQL functions. Each keys is saved to the single file located
at pg_cryptokeys. After initdb with enabling key manager, the
pg_cryptokeys directory has the following files:

$ ll data/pg_cryptokeys
total 12K
-rw------- 1 masahiko staff 132 Mar 25 15:45 0000
-rw------- 1 masahiko staff 132 Mar 25 15:45 0001
-rw------- 1 masahiko staff 132 Mar 25 15:45 0002

I used the integer id rather than string id to make the code simple.

When cluster passphrase rotation, we update all keys atomically using
temporary directory as follows:

1. Derive the new passphrase
2. Wrap all internal keys with the new passphrase
3. Save all internal keys to the temp directory
4. Remove the original directory, pg_cryptokeys
5. Rename the temp directory to pg_cryptokeys

In case of failure during rotation, pg_cyrptokeys and
pg_cyrptokeys_tmp can be left in an incomplete state. We recover it by
checking if the temporary directory exists and the wrapped keys in the
temporary directory are valid.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services