Re: Internal key management system

On Tue, 31 Mar 2020 at 09:36, Cary Huang <cary.huang@highgo.ca> wrote:
>
> 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
themismatch 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
willmake 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
careof copying "cluster_passphrase_command" param from old to new cluster after it has copied the encryption keys so
usersdon't have to do this step? If the expectation is for users to manually correct "cluster_passphrase_command" param
aftersuccessful pg_upgrade and key copy, then there should be a message to remind the users to do so. 

I think both the old cluster and the new cluster must be initialized
with the same passphrase at initdb. Specifying the different
passphrase command to the new cluster at initdb and changing it after
pg_upgrade doesn't make sense. Also I don't think we need to copy
cluster_passphrase_command same as other GUC parameters.

I've changed the patch so that pg_upgrade copies the crypto keys only
if both new and old cluster enable the key management. User must
specify the same passphrase command to both old and new cluster, which
is not cumbersome, I think. I also added the description about this to
the doc.

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

You're right. Fixed.

I've attached the new version patch.

Regards,

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