Re: [pve-devel] [PATCH qemu] pbs: add master key support

[Stefan Reiter <s.reiter@proxmox.com>]

Patch looks good in general, but the added file does not follow our 
formatting for the other patches. I'd prefer to keep them the same, or 
at least applicable with 'git am'.

Since one of us is going to have to rebase anyway, I can also send along 
a fixed up version with v2 of my 5.2 series if you want.

On 08/02/2021 14:08, Fabian Grünbichler wrote:
> this requires a new enough libproxmox-backup-qemu0, and allows querying
> from the PVE side to avoid QMP calls with unsupported parameters.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>      requires versioned build and runtime dep on libproxmox-backup-qemu with changed API for masterkey support
> 
>   .../pve/0059-pbs-backup-add-masterkey.patch   | 96 +++++++++++++++++++
>   debian/patches/series                         |  1 +
>   2 files changed, 97 insertions(+)
>   create mode 100644 debian/patches/pve/0059-pbs-backup-add-masterkey.patch
> 
> diff --git a/debian/patches/pve/0059-pbs-backup-add-masterkey.patch b/debian/patches/pve/0059-pbs-backup-add-masterkey.patch
> new file mode 100644
> index 0000000..12fce05
> --- /dev/null
> +++ b/debian/patches/pve/0059-pbs-backup-add-masterkey.patch
> @@ -0,0 +1,96 @@
> +Index: qemu/pve-backup.c
> +===================================================================
> +--- qemu.orig/pve-backup.c
> ++++ qemu/pve-backup.c
> +@@ -539,6 +539,8 @@ typedef struct QmpBackupTask {
> +     const char *keyfile;
> +     bool has_key_password;
> +     const char *key_password;
> ++    bool has_master_keyfile;
> ++    const char *master_keyfile;
> +     bool has_backup_id;
> +     const char *backup_id;
> +     bool has_backup_time;
> +@@ -710,6 +712,7 @@ static void coroutine_fn pvebackup_co_pr
> +             task->has_password ? task->password : NULL,
> +             task->has_keyfile ? task->keyfile : NULL,
> +             task->has_key_password ? task->key_password : NULL,
> ++            task->has_master_keyfile ? task->master_keyfile : NULL,
> +             task->has_compress ? task->compress : true,
> +             task->has_encrypt ? task->encrypt : task->has_keyfile,
> +             task->has_fingerprint ? task->fingerprint : NULL,
> +@@ -989,6 +992,7 @@ UuidInfo *qmp_backup(
> +     bool has_password, const char *password,
> +     bool has_keyfile, const char *keyfile,
> +     bool has_key_password, const char *key_password,
> ++    bool has_master_keyfile, const char *master_keyfile,
> +     bool has_fingerprint, const char *fingerprint,
> +     bool has_backup_id, const char *backup_id,
> +     bool has_backup_time, int64_t backup_time,
> +@@ -1009,6 +1013,8 @@ UuidInfo *qmp_backup(
> +         .keyfile = keyfile,
> +         .has_key_password = has_key_password,
> +         .key_password = key_password,
> ++        .has_master_keyfile = has_master_keyfile,
> ++        .master_keyfile = master_keyfile,
> +         .has_fingerprint = has_fingerprint,
> +         .fingerprint = fingerprint,
> +         .has_backup_id = has_backup_id,
> +@@ -1131,5 +1137,6 @@ ProxmoxSupportStatus *qmp_query_proxmox_
> +     ret->pbs_dirty_bitmap = true;
> +     ret->query_bitmap_info = true;
> +     ret->pbs_dirty_bitmap_migration = true;
> ++    ret->pbs_masterkey = true;
> +     return ret;
> + }
> +Index: qemu/block/monitor/block-hmp-cmds.c
> +===================================================================
> +--- qemu.orig/block/monitor/block-hmp-cmds.c
> ++++ qemu/block/monitor/block-hmp-cmds.c
> +@@ -1035,6 +1035,7 @@ void hmp_backup(Monitor *mon, const QDic
> +         false, NULL, // PBS password
> +         false, NULL, // PBS keyfile
> +         false, NULL, // PBS key_password
> ++        false, NULL, // PBS master_keyfile
> +         false, NULL, // PBS fingerprint
> +         false, NULL, // PBS backup-id
> +         false, 0, // PBS backup-time
> +Index: qemu/qapi/block-core.json
> +===================================================================
> +--- qemu.orig/qapi/block-core.json
> ++++ qemu/qapi/block-core.json
> +@@ -827,6 +827,8 @@
> + #
> + # @key-password: password for keyfile (optional for format 'pbs')
> + #
> ++# @master_keyfile: PEM-formatted master public keyfile (optional for format 'pbs')
> ++#

please use master-keyfile with a dash

> + # @fingerprint: server cert fingerprint (optional for format 'pbs')
> + #
> + # @backup-id: backup ID (required for format 'pbs')
> +@@ -846,6 +848,7 @@
> +                                     '*password': 'str',
> +                                     '*keyfile': 'str',
> +                                     '*key-password': 'str',
> ++                                    '*master_keyfile': 'str',

here too

Upstream seems to use _ in some places, but at least keep it consistent 
in our code ;)

> +                                     '*fingerprint': 'str',
> +                                     '*backup-id': 'str',
> +                                     '*backup-time': 'int',
> +@@ -895,6 +898,9 @@
> + #                              migration cap if this is false/unset may lead
> + #                              to crashes on migration!
> + #
> ++# @pbs-masterkey: True if the QMP backup call supports the 'master_keyfile'
> ++#                 parameter.
> ++#
> + # @pbs-library-version: Running version of libproxmox-backup-qemu0 library.
> + #
> + ##
> +@@ -902,6 +908,7 @@
> +   'data': { 'pbs-dirty-bitmap': 'bool',
> +             'query-bitmap-info': 'bool',
> +             'pbs-dirty-bitmap-migration': 'bool',
> ++            'pbs-masterkey': 'bool',
> +             'pbs-library-version': 'str' } }
> +
> + ##
> diff --git a/debian/patches/series b/debian/patches/series
> index 1ef7185..433efda 100644
> --- a/debian/patches/series
> +++ b/debian/patches/series
> @@ -59,3 +59,4 @@ pve/0055-PVE-Migrate-dirty-bitmap-state-via-savevm.patch
>   pve/0056-migration-block-dirty-bitmap-migrate-other-bitmaps-e.patch
>   pve/0057-PVE-fix-aborting-multiple-CREATED-jobs-in-sequential.patch
>   pve/0058-PVE-fall-back-to-open-iscsi-initiatorname.patch
> +pve/0059-pbs-backup-add-masterkey.patch
>