[PATCH v3 00/14] LUKS: encryption slot management using amend interface

Maxim Levitsky posted 14 patches 4 years ago
Test docker-mingw@fedora passed
Test checkpatch failed
Test asan passed
Test docker-quick@centos7 passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200503184324.12506-1-mlevitsk@redhat.com
Maintainers: Eric Blake <eblake@redhat.com>, Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Markus Armbruster <armbru@redhat.com>, John Snow <jsnow@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>
There is a newer version of this series
block.c                          |   4 +-
block/Makefile.objs              |   2 +-
block/amend.c                    | 108 ++++++++
block/crypto.c                   | 203 ++++++++++++++--
block/crypto.h                   |  37 +++
block/qcow2.c                    | 306 +++++++++++++----------
crypto/block-luks.c              | 406 ++++++++++++++++++++++++++++++-
crypto/block.c                   |  29 +++
crypto/blockpriv.h               |   8 +
docs/tools/qemu-img.rst          |   5 +-
include/block/block.h            |   1 +
include/block/block_int.h        |  24 +-
include/crypto/block.h           |  22 ++
qapi/block-core.json             |  68 ++++++
qapi/crypto.json                 |  75 +++++-
qapi/job.json                    |   4 +-
qemu-img-cmds.hx                 |   4 +-
qemu-img.c                       |  44 +++-
tests/qemu-iotests/049.out       | 102 ++++----
tests/qemu-iotests/061.out       |  12 +-
tests/qemu-iotests/079.out       |  18 +-
tests/qemu-iotests/082.out       | 176 ++++----------
tests/qemu-iotests/085.out       |  38 +--
tests/qemu-iotests/087.out       |   6 +-
tests/qemu-iotests/115.out       |   2 +-
tests/qemu-iotests/121.out       |   4 +-
tests/qemu-iotests/125.out       | 192 +++++++--------
tests/qemu-iotests/134.out       |   2 +-
tests/qemu-iotests/144.out       |   4 +-
tests/qemu-iotests/158.out       |   4 +-
tests/qemu-iotests/182.out       |   2 +-
tests/qemu-iotests/185.out       |   8 +-
tests/qemu-iotests/188.out       |   2 +-
tests/qemu-iotests/189.out       |   4 +-
tests/qemu-iotests/198.out       |   4 +-
tests/qemu-iotests/243.out       |  16 +-
tests/qemu-iotests/250.out       |   2 +-
tests/qemu-iotests/255.out       |   8 +-
tests/qemu-iotests/263.out       |   4 +-
tests/qemu-iotests/274.out       |  46 ++--
tests/qemu-iotests/280.out       |   2 +-
tests/qemu-iotests/284.out       |   6 +-
tests/qemu-iotests/300           | 207 ++++++++++++++++
tests/qemu-iotests/300.out       |  99 ++++++++
tests/qemu-iotests/301           |  90 +++++++
tests/qemu-iotests/301.out       |  30 +++
tests/qemu-iotests/302           | 278 +++++++++++++++++++++
tests/qemu-iotests/302.out       |  40 +++
tests/qemu-iotests/303           | 233 ++++++++++++++++++
tests/qemu-iotests/303.out       |  33 +++
tests/qemu-iotests/common.filter |   6 +-
tests/qemu-iotests/group         |   5 +
52 files changed, 2503 insertions(+), 532 deletions(-)
create mode 100644 block/amend.c
create mode 100755 tests/qemu-iotests/300
create mode 100644 tests/qemu-iotests/300.out
create mode 100755 tests/qemu-iotests/301
create mode 100644 tests/qemu-iotests/301.out
create mode 100755 tests/qemu-iotests/302
create mode 100644 tests/qemu-iotests/302.out
create mode 100755 tests/qemu-iotests/303
create mode 100644 tests/qemu-iotests/303.out
[PATCH v3 00/14] LUKS: encryption slot management using amend interface
Posted by Maxim Levitsky 4 years ago
Hi!
Here is the updated series of my patches, incorporating all the feedback I received.

This implements the API interface that we agreed upon except that I merged the
LUKSKeyslotActive/LUKSKeyslotInactive union into a struct because otherwise
I need nested unions which are not supported currently by QAPI parser.
This didn't change the API and thus once support for nested unions is there,
it can always be implemented in backward compatible way.

I hope that this series will finally be considered for merging, since I am somewhat running
out of time to finish this task.

Patches are strictly divided by topic to 3 groups, and each group depends on former groups.

* Patches 1,2 implement qcrypto generic amend interface, including definition
  of structs used in crypto.json and implement this in luks crypto driver
  Nothing is exposed to the user at this stage

* Patches 3-9 use the code from patches 1,2 to implement qemu-img amend based encryption slot management
  for luks and for qcow2, and add a bunch of iotests to cover that.

* Patches 10-13 add x-blockdev-amend (I'll drop the -x prefix if you like), and wire it
  to luks and qcow2 driver to implement qmp based encryption slot management also using
  the code from patches 1,2, and also add a bunch of iotests to cover this.

Tested with -raw,-qcow2 and -luks iotests and 'make check'

V3: rebased, addressed most of the review feedback.
For now I kept the slot bitmap code since I am not sure that replacing it will be better.

Best regards,
        Maxim Levitsky

clone of "luks-keymgmnt-v2"

Maxim Levitsky (14):
  qcrypto/core: add generic infrastructure for crypto options amendment
  qcrypto/luks: implement encryption key management
  block/amend: add 'force' option
  block/amend: separate amend and create options for qemu-img
  block/amend: refactor qcow2 amend options
  block/crypto: rename two functions
  block/crypto: implement the encryption key management
  block/qcow2: extend qemu-img amend interface with crypto options
  iotests: filter few more luks specific create options
  iotests: qemu-img tests for luks key management
  block/core: add generic infrastructure for x-blockdev-amend qmp
    command
  block/crypto: implement blockdev-amend
  block/qcow2: implement blockdev-amend
  iotests: add tests for blockdev-amend

 block.c                          |   4 +-
 block/Makefile.objs              |   2 +-
 block/amend.c                    | 108 ++++++++
 block/crypto.c                   | 203 ++++++++++++++--
 block/crypto.h                   |  37 +++
 block/qcow2.c                    | 306 +++++++++++++----------
 crypto/block-luks.c              | 406 ++++++++++++++++++++++++++++++-
 crypto/block.c                   |  29 +++
 crypto/blockpriv.h               |   8 +
 docs/tools/qemu-img.rst          |   5 +-
 include/block/block.h            |   1 +
 include/block/block_int.h        |  24 +-
 include/crypto/block.h           |  22 ++
 qapi/block-core.json             |  68 ++++++
 qapi/crypto.json                 |  75 +++++-
 qapi/job.json                    |   4 +-
 qemu-img-cmds.hx                 |   4 +-
 qemu-img.c                       |  44 +++-
 tests/qemu-iotests/049.out       | 102 ++++----
 tests/qemu-iotests/061.out       |  12 +-
 tests/qemu-iotests/079.out       |  18 +-
 tests/qemu-iotests/082.out       | 176 ++++----------
 tests/qemu-iotests/085.out       |  38 +--
 tests/qemu-iotests/087.out       |   6 +-
 tests/qemu-iotests/115.out       |   2 +-
 tests/qemu-iotests/121.out       |   4 +-
 tests/qemu-iotests/125.out       | 192 +++++++--------
 tests/qemu-iotests/134.out       |   2 +-
 tests/qemu-iotests/144.out       |   4 +-
 tests/qemu-iotests/158.out       |   4 +-
 tests/qemu-iotests/182.out       |   2 +-
 tests/qemu-iotests/185.out       |   8 +-
 tests/qemu-iotests/188.out       |   2 +-
 tests/qemu-iotests/189.out       |   4 +-
 tests/qemu-iotests/198.out       |   4 +-
 tests/qemu-iotests/243.out       |  16 +-
 tests/qemu-iotests/250.out       |   2 +-
 tests/qemu-iotests/255.out       |   8 +-
 tests/qemu-iotests/263.out       |   4 +-
 tests/qemu-iotests/274.out       |  46 ++--
 tests/qemu-iotests/280.out       |   2 +-
 tests/qemu-iotests/284.out       |   6 +-
 tests/qemu-iotests/300           | 207 ++++++++++++++++
 tests/qemu-iotests/300.out       |  99 ++++++++
 tests/qemu-iotests/301           |  90 +++++++
 tests/qemu-iotests/301.out       |  30 +++
 tests/qemu-iotests/302           | 278 +++++++++++++++++++++
 tests/qemu-iotests/302.out       |  40 +++
 tests/qemu-iotests/303           | 233 ++++++++++++++++++
 tests/qemu-iotests/303.out       |  33 +++
 tests/qemu-iotests/common.filter |   6 +-
 tests/qemu-iotests/group         |   5 +
 52 files changed, 2503 insertions(+), 532 deletions(-)
 create mode 100644 block/amend.c
 create mode 100755 tests/qemu-iotests/300
 create mode 100644 tests/qemu-iotests/300.out
 create mode 100755 tests/qemu-iotests/301
 create mode 100644 tests/qemu-iotests/301.out
 create mode 100755 tests/qemu-iotests/302
 create mode 100644 tests/qemu-iotests/302.out
 create mode 100755 tests/qemu-iotests/303
 create mode 100644 tests/qemu-iotests/303.out

-- 
2.17.2


Re: [PATCH v3 00/14] LUKS: encryption slot management using amend interface
Posted by no-reply@patchew.org 4 years ago
Patchew URL: https://patchew.org/QEMU/20200503184324.12506-1-mlevitsk@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200503184324.12506-1-mlevitsk@redhat.com
Subject: [PATCH v3 00/14] LUKS: encryption slot management using amend interface
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
5d8eee3 iotests: add tests for blockdev-amend
73b4eb1 block/qcow2: implement blockdev-amend
b636b4e block/crypto: implement blockdev-amend
989ac21 block/core: add generic infrastructure for x-blockdev-amend qmp command
905e766 iotests: qemu-img tests for luks key management
63c741f iotests: filter few more luks specific create options
7f3246a block/qcow2: extend qemu-img amend interface with crypto options
2e71af1 block/crypto: implement the encryption key management
e23e157 block/crypto: rename two functions
1d744a7 block/amend: refactor qcow2 amend options
9dfeafe block/amend: separate amend and create options for qemu-img
6abf4af block/amend: add 'force' option
70e39d2 qcrypto/luks: implement encryption key management
3e1975e qcrypto/core: add generic infrastructure for crypto options amendment

=== OUTPUT BEGIN ===
1/14 Checking commit 3e1975eea93f (qcrypto/core: add generic infrastructure for crypto options amendment)
2/14 Checking commit 70e39d22b302 (qcrypto/luks: implement encryption key management)
3/14 Checking commit 6abf4afc9174 (block/amend: add 'force' option)
4/14 Checking commit 9dfeafe0f584 (block/amend: separate amend and create options for qemu-img)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#31: FILE: block/qcow2.c:5498:
+#define QCOW_COMMON_OPTIONS                                         \
+    {                                                               \
+        .name = BLOCK_OPT_SIZE,                                     \
+        .type = QEMU_OPT_SIZE,                                      \
+        .help = "Virtual disk size"                                 \
+    },                                                              \
+    {                                                               \
+        .name = BLOCK_OPT_COMPAT_LEVEL,                             \
+        .type = QEMU_OPT_STRING,                                    \
+        .help = "Compatibility level (v2 [0.10] or v3 [1.1])"       \
+    },                                                              \
+    {                                                               \
+        .name = BLOCK_OPT_BACKING_FILE,                             \
+        .type = QEMU_OPT_STRING,                                    \
+        .help = "File name of a base image"                         \
+    },                                                              \
+    {                                                               \
+        .name = BLOCK_OPT_BACKING_FMT,                              \
+        .type = QEMU_OPT_STRING,                                    \
+        .help = "Image format of the base image"                    \
+    },                                                              \
+    {                                                               \
+        .name = BLOCK_OPT_DATA_FILE,                                \
+        .type = QEMU_OPT_STRING,                                    \
+        .help = "File name of an external data file"                \
+    },                                                              \
+    {                                                               \
+        .name = BLOCK_OPT_DATA_FILE_RAW,                            \
+        .type = QEMU_OPT_BOOL,                                      \
+        .help = "The external data file must stay valid "           \
+                "as a raw image"                                    \
+    },                                                              \
+    {                                                               \
+        .name = BLOCK_OPT_ENCRYPT,                                  \
+        .type = QEMU_OPT_BOOL,                                      \
+        .help = "Encrypt the image with format 'aes'. (Deprecated " \
+                "in favor of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)",    \
+    },                                                              \
+    {                                                               \
+        .name = BLOCK_OPT_ENCRYPT_FORMAT,                           \
+        .type = QEMU_OPT_STRING,                                    \
+        .help = "Encrypt the image, format choices: 'aes', 'luks'", \
+    },                                                              \
+    BLOCK_CRYPTO_OPT_DEF_KEY_SECRET("encrypt.",                     \
+        "ID of secret providing qcow AES key or LUKS passphrase"),  \
+    BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG("encrypt."),               \
+    BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE("encrypt."),              \
+    BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG("encrypt."),                \
+    BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG("encrypt."),           \
+    BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG("encrypt."),                 \
+    BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME("encrypt."),                \
+    {                                                               \
+        .name = BLOCK_OPT_CLUSTER_SIZE,                             \
+        .type = QEMU_OPT_SIZE,                                      \
+        .help = "qcow2 cluster size",                               \
+        .def_value_str = stringify(DEFAULT_CLUSTER_SIZE)            \
+    },                                                              \
+    {                                                               \
+        .name = BLOCK_OPT_PREALLOC,                                 \
+        .type = QEMU_OPT_STRING,                                    \
+        .help = "Preallocation mode (allowed values: off, "         \
+                "metadata, falloc, full)"                           \
+    },                                                              \
+    {                                                               \
+        .name = BLOCK_OPT_LAZY_REFCOUNTS,                           \
+        .type = QEMU_OPT_BOOL,                                      \
+        .help = "Postpone refcount updates",                        \
+        .def_value_str = "off"                                      \
+    },                                                              \
+    {                                                               \
+        .name = BLOCK_OPT_REFCOUNT_BITS,                            \
+        .type = QEMU_OPT_NUMBER,                                    \
+        .help = "Width of a reference count entry in bits",         \
+        .def_value_str = "16"                                       \
+    }                                                               \
+

total: 1 errors, 0 warnings, 231 lines checked

Patch 4/14 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/14 Checking commit 1d744a7b0346 (block/amend: refactor qcow2 amend options)
6/14 Checking commit e23e15761eaf (block/crypto: rename two functions)
7/14 Checking commit 2e71af15aca9 (block/crypto: implement the encryption key management)
8/14 Checking commit 7f3246a67d36 (block/qcow2: extend qemu-img amend interface with crypto options)
9/14 Checking commit 63c741ff4dd1 (iotests: filter few more luks specific create options)
10/14 Checking commit 905e766ae0a7 (iotests: qemu-img tests for luks key management)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#15: 
new file mode 100755

total: 0 errors, 1 warnings, 432 lines checked

Patch 10/14 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
11/14 Checking commit 989ac217ef73 (block/core: add generic infrastructure for x-blockdev-amend qmp command)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#31: 
new file mode 100644

total: 0 errors, 1 warnings, 216 lines checked

Patch 11/14 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
12/14 Checking commit b636b4e630bd (block/crypto: implement blockdev-amend)
13/14 Checking commit 73b4eb12a431 (block/qcow2: implement blockdev-amend)
14/14 Checking commit 5d8eee36ada5 (iotests: add tests for blockdev-amend)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#15: 
new file mode 100755

total: 0 errors, 1 warnings, 589 lines checked

Patch 14/14 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200503184324.12506-1-mlevitsk@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [PATCH v3 00/14] LUKS: encryption slot management using amend interface
Posted by Daniel P. Berrangé 4 years ago
On Sun, May 03, 2020 at 09:43:10PM +0300, Maxim Levitsky wrote:
> Hi!
> Here is the updated series of my patches, incorporating all the feedback I received.
> 
> This implements the API interface that we agreed upon except that I merged the
> LUKSKeyslotActive/LUKSKeyslotInactive union into a struct because otherwise
> I need nested unions which are not supported currently by QAPI parser.
> This didn't change the API and thus once support for nested unions is there,
> it can always be implemented in backward compatible way.
> 
> I hope that this series will finally be considered for merging, since I am somewhat running
> out of time to finish this task.
> 
> Patches are strictly divided by topic to 3 groups, and each group depends on former groups.
> 
> * Patches 1,2 implement qcrypto generic amend interface, including definition
>   of structs used in crypto.json and implement this in luks crypto driver
>   Nothing is exposed to the user at this stage
> 
> * Patches 3-9 use the code from patches 1,2 to implement qemu-img amend based encryption slot management
>   for luks and for qcow2, and add a bunch of iotests to cover that.
> 
> * Patches 10-13 add x-blockdev-amend (I'll drop the -x prefix if you like), and wire it
>   to luks and qcow2 driver to implement qmp based encryption slot management also using
>   the code from patches 1,2, and also add a bunch of iotests to cover this.
> 
> Tested with -raw,-qcow2 and -luks iotests and 'make check'
> 
> V3: rebased, addressed most of the review feedback.
> For now I kept the slot bitmap code since I am not sure that replacing it will be better.

I'm still of the opinion that the bitmaps code should be replaced.
With this current design we are iterating over the slot of keys slots
4 times, doing different checks/logic in each iteration. This is really
not nice for understanding how the code works. IMHO we should be iterating
at most twice - once to validate the requested configuration, and once
to apply the requested configuration.   Even if there si duplication of
logic in between the delete/add key codepaths, I think it will be better
as the logic for each operation will be in one place.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


Re: [PATCH v3 00/14] LUKS: encryption slot management using amend interface
Posted by Maxim Levitsky 4 years ago
On Mon, 2020-05-04 at 11:19 +0100, Daniel P. Berrangé wrote:
> On Sun, May 03, 2020 at 09:43:10PM +0300, Maxim Levitsky wrote:
> > Hi!
> > Here is the updated series of my patches, incorporating all the feedback I received.
> > 
> > This implements the API interface that we agreed upon except that I merged the
> > LUKSKeyslotActive/LUKSKeyslotInactive union into a struct because otherwise
> > I need nested unions which are not supported currently by QAPI parser.
> > This didn't change the API and thus once support for nested unions is there,
> > it can always be implemented in backward compatible way.
> > 
> > I hope that this series will finally be considered for merging, since I am somewhat running
> > out of time to finish this task.
> > 
> > Patches are strictly divided by topic to 3 groups, and each group depends on former groups.
> > 
> > * Patches 1,2 implement qcrypto generic amend interface, including definition
> >   of structs used in crypto.json and implement this in luks crypto driver
> >   Nothing is exposed to the user at this stage
> > 
> > * Patches 3-9 use the code from patches 1,2 to implement qemu-img amend based encryption slot management
> >   for luks and for qcow2, and add a bunch of iotests to cover that.
> > 
> > * Patches 10-13 add x-blockdev-amend (I'll drop the -x prefix if you like), and wire it
> >   to luks and qcow2 driver to implement qmp based encryption slot management also using
> >   the code from patches 1,2, and also add a bunch of iotests to cover this.
> > 
> > Tested with -raw,-qcow2 and -luks iotests and 'make check'
> > 
> > V3: rebased, addressed most of the review feedback.
> > For now I kept the slot bitmap code since I am not sure that replacing it will be better.
> 
> I'm still of the opinion that the bitmaps code should be replaced.
> With this current design we are iterating over the slot of keys slots
> 4 times, doing different checks/logic in each iteration. This is really
> not nice for understanding how the code works. IMHO we should be iterating
> at most twice - once to validate the requested configuration, and once
> to apply the requested configuration.   Even if there si duplication of
> logic in between the delete/add key codepaths, I think it will be better
> as the logic for each operation will be in one place.
> 
> 
> Regards,
> Daniel

All right then, I also agree kind of that these iterations aren't
that nice. I just wanted to do this in the same way the spec defines it,
which is currently kind of independent of how key add/remove operation.

I'll send updated code soon.

Best regards,
	Maxim Levitsky