diff mbox series

[12/13] qemu-img: implement key management

Message ID 20190814202219.1870-13-mlevitsk@redhat.com
State New
Headers show
Series RFC: luks/encrypted qcow2 key management | expand

Commit Message

Maxim Levitsky Aug. 14, 2019, 8:22 p.m. UTC
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/crypto.c   |  16 ++++++
 block/crypto.h   |   3 +
 qemu-img-cmds.hx |  13 +++++
 qemu-img.c       | 140 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 172 insertions(+)

Comments

Max Reitz Aug. 20, 2019, 6:29 p.m. UTC | #1
On 14.08.19 22:22, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/crypto.c   |  16 ++++++
>  block/crypto.h   |   3 +
>  qemu-img-cmds.hx |  13 +++++
>  qemu-img.c       | 140 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 172 insertions(+)

Yes, this seems a bit weird.  Putting it under amend seems like the
natural thing if that works; if not, I think it should be a single
qemu-img subcommand instead of two.

Max
Maxim Levitsky Aug. 21, 2019, 10:33 p.m. UTC | #2
On Tue, 2019-08-20 at 20:29 +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  block/crypto.c   |  16 ++++++
> >  block/crypto.h   |   3 +
> >  qemu-img-cmds.hx |  13 +++++
> >  qemu-img.c       | 140 +++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 172 insertions(+)
> 
> Yes, this seems a bit weird.  Putting it under amend seems like the
> natural thing if that works; if not, I think it should be a single
> qemu-img subcommand instead of two.
> 
> Max
> 
Agree, thats why RFC.

Best regards,
	Maxim Levitsky
Daniel P. Berrangé Aug. 22, 2019, 11:32 a.m. UTC | #3
On Tue, Aug 20, 2019 at 08:29:55PM +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  block/crypto.c   |  16 ++++++
> >  block/crypto.h   |   3 +
> >  qemu-img-cmds.hx |  13 +++++
> >  qemu-img.c       | 140 +++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 172 insertions(+)
> 
> Yes, this seems a bit weird.  Putting it under amend seems like the
> natural thing if that works; if not, I think it should be a single
> qemu-img subcommand instead of two.

I'm not convinced by overloading two distinct operations on to one
sub-command - doesn't seem to give an obvious benefit to overload
them & IME experiance overloading results in harder to understand
commands due to having distinct args to each command.


Regards,
Daniel
Max Reitz Aug. 22, 2019, 2:42 p.m. UTC | #4
On 22.08.19 13:32, Daniel P. Berrangé wrote:
> On Tue, Aug 20, 2019 at 08:29:55PM +0200, Max Reitz wrote:
>> On 14.08.19 22:22, Maxim Levitsky wrote:
>>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>>> ---
>>>  block/crypto.c   |  16 ++++++
>>>  block/crypto.h   |   3 +
>>>  qemu-img-cmds.hx |  13 +++++
>>>  qemu-img.c       | 140 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 172 insertions(+)
>>
>> Yes, this seems a bit weird.  Putting it under amend seems like the
>> natural thing if that works; if not, I think it should be a single
>> qemu-img subcommand instead of two.
> 
> I'm not convinced by overloading two distinct operations on to one
> sub-command - doesn't seem to give an obvious benefit to overload
> them & IME experiance overloading results in harder to understand
> commands due to having distinct args to each command.

Because it suits the qemu-img interface we currently have.  For example,
we have a single subcommand for internal snapshot management (“qemu-img
snapshot”), so I think it makes sense to have a single subcommand for
encrypted image management.

Max
Maxim Levitsky Aug. 25, 2019, 5:04 p.m. UTC | #5
On Thu, 2019-08-22 at 16:42 +0200, Max Reitz wrote:
> On 22.08.19 13:32, Daniel P. Berrangé wrote:
> > On Tue, Aug 20, 2019 at 08:29:55PM +0200, Max Reitz wrote:
> > > On 14.08.19 22:22, Maxim Levitsky wrote:
> > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > > ---
> > > >  block/crypto.c   |  16 ++++++
> > > >  block/crypto.h   |   3 +
> > > >  qemu-img-cmds.hx |  13 +++++
> > > >  qemu-img.c       | 140 +++++++++++++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 172 insertions(+)
> > > 
> > > Yes, this seems a bit weird.  Putting it under amend seems like the
> > > natural thing if that works; if not, I think it should be a single
> > > qemu-img subcommand instead of two.
> > 
> > I'm not convinced by overloading two distinct operations on to one
> > sub-command - doesn't seem to give an obvious benefit to overload
> > them & IME experiance overloading results in harder to understand
> > commands due to having distinct args to each command.
> 
> Because it suits the qemu-img interface we currently have.  For example,
> we have a single subcommand for internal snapshot management (“qemu-img
> snapshot”), so I think it makes sense to have a single subcommand for
> encrypted image management.

I personally don't care, other that I do thing that the best here is to use
the amend interface.

> 
> Max
> 

Best regards,
	Maxim Levitsky
diff mbox series

Patch

diff --git a/block/crypto.c b/block/crypto.c
index 415b6db041..2fcdf9dd39 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -203,6 +203,22 @@  block_crypto_create_opts_init(QDict *opts, Error **errp)
     return ret;
 }
 
+QCryptoEncryptionSetupOptions *
+block_crypto_setup_opts_init(QDict *opts, Error **errp)
+{
+    Visitor *v;
+    QCryptoEncryptionSetupOptions *ret;
+
+    v = qobject_input_visitor_new_flat_confused(opts, errp);
+    if (!v) {
+        return NULL;
+    }
+
+    visit_type_QCryptoEncryptionSetupOptions(v, NULL, &ret, errp);
+
+    visit_free(v);
+    return ret;
+}
 
 static int block_crypto_open_generic(QCryptoBlockFormat format,
                                      QemuOptsList *opts_spec,
diff --git a/block/crypto.h b/block/crypto.h
index b935695e79..ece4d64aef 100644
--- a/block/crypto.h
+++ b/block/crypto.h
@@ -94,4 +94,7 @@  block_crypto_create_opts_init(QDict *opts, Error **errp);
 QCryptoBlockOpenOptions *
 block_crypto_open_opts_init(QDict *opts, Error **errp);
 
+QCryptoEncryptionSetupOptions *
+block_crypto_setup_opts_init(QDict *opts, Error **errp);
+
 #endif /* BLOCK_CRYPTO_H */
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 1c93e6d185..7816a0adfb 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -19,6 +19,18 @@  STEXI
 @item amend [--object @var{objectdef}] [--image-opts] [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
 ETEXI
 
+DEF("add_encryption_key", img_add_encryption_key,
+    "add_encryption_key [--object objectdef] [--image-opts] [--force] -U --keydef key_definition filename")
+STEXI
+@item add_encryption_key [--object @var{objectdef}] [--image-opts] [--force] -U --keydef @var{key_definition} @var{filename}
+ETEXI
+
+DEF("erase_encryption_key", img_erase_encryption_key,
+    "erase_encryption_key [--object objectdef] [--image-opts] [--force] -U --keydef key_definition filename")
+STEXI
+@item erase_encryption_key [--object @var{objectdef}] [--image-opts] [--force] -U --keydef @var{key_definition} @var{filename}
+ETEXI
+
 DEF("bench", img_bench,
     "bench [-c count] [-d depth] [-f fmt] [--flush-interval=flush_interval] [-n] [--no-drain] [-o offset] [--pattern=pattern] [-q] [-s buffer_size] [-S step_size] [-t cache] [-w] [-U] filename")
 STEXI
@@ -97,6 +109,7 @@  STEXI
 @item resize [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--preallocation=@var{prealloc}] [-q] [--shrink] @var{filename} [+ | -]@var{size}
 ETEXI
 
+
 STEXI
 @end table
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index 79983772de..bc6cd60df1 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -47,6 +47,7 @@ 
 #include "block/blockjob.h"
 #include "block/qapi.h"
 #include "crypto/init.h"
+#include "block/crypto.h"
 #include "trace/control.h"
 
 #define QEMU_IMG_VERSION "qemu-img version " QEMU_FULL_VERSION \
@@ -70,6 +71,8 @@  enum {
     OPTION_PREALLOCATION = 265,
     OPTION_SHRINK = 266,
     OPTION_SALVAGE = 267,
+    OPTION_FORCE = 268,
+    OPTION_KEYDEF = 269,
 };
 
 typedef enum OutputFormat {
@@ -223,6 +226,14 @@  static QemuOptsList qemu_source_opts = {
     },
 };
 
+static QemuOptsList keydef_opts = {
+    .name = "encryption_opts",
+    .head = QTAILQ_HEAD_INITIALIZER(keydef_opts.head),
+    .desc = {
+        { }
+    },
+};
+
 static int GCC_FMT_ATTR(2, 3) qprintf(bool quiet, const char *fmt, ...)
 {
     int ret = 0;
@@ -4997,6 +5008,135 @@  out:
     return ret;
 }
 
+
+static QemuOptsList keydef_options_list = {
+    .name = "encryption",
+    .head = QTAILQ_HEAD_INITIALIZER(keydef_options_list.head),
+    .desc = {
+        { }
+    },
+};
+
+static int setup_encryption(int argc, char **argv,
+        enum BlkSetupEncryptionAction action)
+{
+    static const struct option long_options[] = {
+        {"help", no_argument, 0, 'h'},
+        {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+        {"object", required_argument, 0, OPTION_OBJECT},
+        {"force", no_argument, 0, OPTION_FORCE},
+        {"force-share", no_argument, 0, 'U'},
+        {"keydef", required_argument, 0, OPTION_KEYDEF},
+        {0, 0, 0, 0}
+    };
+
+    BlockBackend *blk = NULL;
+    const char *filename = NULL;
+    bool force_share = false;
+    QemuOpts *keydef_opts = NULL;
+    bool image_opts = false;
+    Error *local_err = NULL;
+    QDict *keydef_dict = NULL;
+    QCryptoEncryptionSetupOptions *qcrypto_options = NULL;
+    bool force = false;
+
+    int ret = 1;
+    int c;
+
+    while ((c = getopt_long(argc, argv, "hU:", long_options, NULL)) != -1) {
+        switch (c) {
+        case '?':
+        case 'h':
+            help();
+            break;
+        case 'U':
+            force_share = true;
+            break;
+
+        case OPTION_KEYDEF:
+            if (keydef_opts) {
+                error_report("Only single --keydef argument is allowed.");
+                goto out;
+            }
+
+            keydef_opts = qemu_opts_parse_noisily(&keydef_options_list,
+                                                  optarg, false);
+            if (!keydef_opts) {
+                goto out;
+            }
+            break;
+
+        case OPTION_OBJECT: {
+            QemuOpts *object_opts = qemu_opts_parse_noisily(&qemu_object_opts,
+                                                  optarg, true);
+            if (!object_opts) {
+                goto out;
+            }
+            break;
+        }
+        case OPTION_IMAGE_OPTS:
+            image_opts = true;
+            break;
+        case OPTION_FORCE:
+            force = true;
+            break;
+        break;
+        }
+    }
+
+    if (qemu_opts_foreach(&qemu_object_opts,
+                          user_creatable_add_opts_foreach,
+                          NULL, &error_fatal)) {
+        goto out;
+    }
+
+    if (argc - optind > 1) {
+        error_report("At most one filename argument is allowed.");
+        goto out;
+    } else if (argc - optind == 1) {
+        filename = argv[optind];
+    } else {
+        error_report("filename is required");
+        goto out;
+
+    }
+
+    blk = img_open(image_opts, filename, NULL, BDRV_O_RDWR,
+                   false, false, force_share);
+    if (!blk) {
+        goto out;
+    }
+
+    keydef_dict = qemu_opts_to_qdict(keydef_opts, NULL);
+    qcrypto_options = block_crypto_setup_opts_init(keydef_dict, &local_err);
+    if (!qcrypto_options) {
+        error_report_err(local_err);
+        goto out;
+    }
+
+    if (blk_setup_encryption(blk, action, qcrypto_options, force, &local_err)) {
+        error_report_err(local_err);
+        goto out;
+    }
+    ret = 0;
+out:
+    qemu_opts_del(keydef_opts);
+    qobject_unref(keydef_dict);
+    qapi_free_QCryptoEncryptionSetupOptions(qcrypto_options);
+    blk_unref(blk);
+    return ret;
+}
+
+static int img_add_encryption_key(int argc, char **argv)
+{
+    return setup_encryption(argc, argv, BLK_UPDATE_ENCRYPTION);
+}
+
+static int img_erase_encryption_key(int argc, char **argv)
+{
+    return setup_encryption(argc, argv, BLK_ERASE_ENCRYPTION);
+}
+
 static const img_cmd_t img_cmds[] = {
 #define DEF(option, callback, arg_string)        \
     { option, callback },