[PATCH RESEND v3 02/10] crypto: Support generic LUKS encryption

Hyman Huang posted 10 patches 11 months, 1 week ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Hyman Huang <yong.huang@smartx.com>
There is a newer version of this series
[PATCH RESEND v3 02/10] crypto: Support generic LUKS encryption
Posted by Hyman Huang 11 months, 1 week ago
By enhancing the LUKS driver, it is possible to enable
the detachable LUKS header and, as a result, achieve
general encryption for any disk format that QEMU has
supported.

Take the qcow2 as an example, the usage of the generic
LUKS encryption as follows:

1. add a protocol blockdev node of data disk
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-1-storage", "driver":"file",
> "filename":"/path/to/test_disk.qcow2"}}'

2. add a protocol blockdev node of LUKS header as above.
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-2-storage", "driver":"file",
> "filename": "/path/to/cipher.gluks" }}'

3. add the secret for decrypting the cipher stored in LUKS
   header above
$ virsh qemu-monitor-command vm '{"execute":"object-add",
> "arguments":{"qom-type":"secret", "id":
> "libvirt-2-storage-secret0", "data":"abc123"}}'

4. add the qcow2-drived blockdev format node
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-1-format", "driver":"qcow2",
> "file":"libvirt-1-storage"}}'

5. add the luks-drived blockdev to link the qcow2 disk with
   LUKS header by specifying the field "header"
$ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> "arguments":{"node-name":"libvirt-2-format", "driver":"luks",
> "file":"libvirt-1-format", "header":"libvirt-2-storage",
> "key-secret":"libvirt-2-format-secret0"}}'

6. add the virtio-blk device finally
$ virsh qemu-monitor-command vm '{"execute":"device_add",
> "arguments": {"num-queues":"1", "driver":"virtio-blk-pci",
> "drive": "libvirt-2-format", "id":"virtio-disk2"}}'

The generic LUKS encryption method of starting a virtual
machine (VM) is somewhat similar to hot-plug in that both
maintaining the same json command while the starting VM
changes the "blockdev-add/device_add" parameters to
"blockdev/device".

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
Message-Id: <910801f303da1601051479d3b7e5c2c6b4e01eb7.1701879996.git.yong.huang@smartx.com>
---
 block/crypto.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/block/crypto.c b/block/crypto.c
index f82b13d32b..6063879bac 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -64,12 +64,14 @@ static int block_crypto_read_func(QCryptoBlock *block,
                                   Error **errp)
 {
     BlockDriverState *bs = opaque;
+    BlockCrypto *crypto = bs->opaque;
     ssize_t ret;
 
     GLOBAL_STATE_CODE();
     GRAPH_RDLOCK_GUARD_MAINLOOP();
 
-    ret = bdrv_pread(bs->file, offset, buflen, buf, 0);
+    ret = bdrv_pread(crypto->header ? crypto->header : bs->file,
+                     offset, buflen, buf, 0);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not read encryption header");
         return ret;
@@ -269,6 +271,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
     QCryptoBlockOpenOptions *open_opts = NULL;
     unsigned int cflags = 0;
     QDict *cryptoopts = NULL;
+    const char *hdr_bdref = qdict_get_try_str(options, "header");
 
     GLOBAL_STATE_CODE();
 
@@ -277,6 +280,15 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
         return ret;
     }
 
+    if (hdr_bdref) {
+        crypto->header = bdrv_open_child(NULL, options, "header", bs,
+                                         &child_of_bds, BDRV_CHILD_METADATA,
+                                         false, errp);
+        if (!crypto->header) {
+            return -EINVAL;
+        }
+    }
+
     GRAPH_RDLOCK_GUARD_MAINLOOP();
 
     bs->supported_write_flags = BDRV_REQ_FUA &
-- 
2.39.1
Re: [PATCH RESEND v3 02/10] crypto: Support generic LUKS encryption
Posted by Daniel P. Berrangé 10 months, 3 weeks ago
On Mon, Dec 25, 2023 at 01:45:04PM +0800, Hyman Huang wrote:
> By enhancing the LUKS driver, it is possible to enable
> the detachable LUKS header and, as a result, achieve
> general encryption for any disk format that QEMU has
> supported.
> 
> Take the qcow2 as an example, the usage of the generic
> LUKS encryption as follows:
> 
> 1. add a protocol blockdev node of data disk
> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > "arguments":{"node-name":"libvirt-1-storage", "driver":"file",
> > "filename":"/path/to/test_disk.qcow2"}}'
> 
> 2. add a protocol blockdev node of LUKS header as above.
> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > "arguments":{"node-name":"libvirt-2-storage", "driver":"file",
> > "filename": "/path/to/cipher.gluks" }}'
> 
> 3. add the secret for decrypting the cipher stored in LUKS
>    header above
> $ virsh qemu-monitor-command vm '{"execute":"object-add",
> > "arguments":{"qom-type":"secret", "id":
> > "libvirt-2-storage-secret0", "data":"abc123"}}'
> 
> 4. add the qcow2-drived blockdev format node
> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > "arguments":{"node-name":"libvirt-1-format", "driver":"qcow2",
> > "file":"libvirt-1-storage"}}'
> 
> 5. add the luks-drived blockdev to link the qcow2 disk with
>    LUKS header by specifying the field "header"
> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > "arguments":{"node-name":"libvirt-2-format", "driver":"luks",
> > "file":"libvirt-1-format", "header":"libvirt-2-storage",
> > "key-secret":"libvirt-2-format-secret0"}}'
> 
> 6. add the virtio-blk device finally
> $ virsh qemu-monitor-command vm '{"execute":"device_add",
> > "arguments": {"num-queues":"1", "driver":"virtio-blk-pci",
> > "drive": "libvirt-2-format", "id":"virtio-disk2"}}'
> 
> The generic LUKS encryption method of starting a virtual
> machine (VM) is somewhat similar to hot-plug in that both
> maintaining the same json command while the starting VM
> changes the "blockdev-add/device_add" parameters to
> "blockdev/device".
> 
> Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> Message-Id: <910801f303da1601051479d3b7e5c2c6b4e01eb7.1701879996.git.yong.huang@smartx.com>
> ---
>  block/crypto.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index f82b13d32b..6063879bac 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -64,12 +64,14 @@ static int block_crypto_read_func(QCryptoBlock *block,
>                                    Error **errp)
>  {
>      BlockDriverState *bs = opaque;
> +    BlockCrypto *crypto = bs->opaque;
>      ssize_t ret;
>  
>      GLOBAL_STATE_CODE();
>      GRAPH_RDLOCK_GUARD_MAINLOOP();
>  
> -    ret = bdrv_pread(bs->file, offset, buflen, buf, 0);
> +    ret = bdrv_pread(crypto->header ? crypto->header : bs->file,
> +                     offset, buflen, buf, 0);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Could not read encryption header");
>          return ret;
> @@ -269,6 +271,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
>      QCryptoBlockOpenOptions *open_opts = NULL;
>      unsigned int cflags = 0;
>      QDict *cryptoopts = NULL;
> +    const char *hdr_bdref = qdict_get_try_str(options, "header");

This is an invalid check to make, because it is assuming the user is
referencing a separate blockdev node name and doesn't work for an
inline definition. eg

  qemu-img info  'json:{"driver":"luks","file":{"filename":"test-payload.img"},"header":{"filename":"test-header.img"}}'


>  
>      GLOBAL_STATE_CODE();
>  
> @@ -277,6 +280,15 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
>          return ret;
>      }
>  
> +    if (hdr_bdref) {

Get rid of this 'if' clause and unconditionally call the next line:

> +        crypto->header = bdrv_open_child(NULL, options, "header", bs,
> +                                         &child_of_bds, BDRV_CHILD_METADATA,
> +                                         false, errp);

but pass 'true' instead of 'false' here to allow the child to be absent,
and thus let it return NULL.

> +        if (!crypto->header) {

You'll need to then check  '*errp != NULL' instead

You'll also need "ERRP_GUARD" at the start of the method

> +            return -EINVAL;
> +        }
> +    }
> +
>      GRAPH_RDLOCK_GUARD_MAINLOOP();
>  
>      bs->supported_write_flags = BDRV_REQ_FUA &

This patch should be combined with the previous patch that adds the new
QAPI schema element as splitting them doesn't add value.


Testing this patch with the changes I suggest above, however, still does
not work:

  $ dd if=/dev/zero of=test-header.img bs=1M count=32
  $ dd if=/dev/zero of=test-payload.img bs=1M count=1000
  $ cryptsetup luksFormat  --header test-header.img test-payload.img  --force-password --type luks1
  $ qemu-img info  'json:{"driver":"luks","file":{"filename":"test-payload.img"},"header":{"filename":"test-header.img"}}'
  qemu-img: Could not open 'json:{"driver":"luks","file":{"filename":"test-payload.img"},"header":{"filename":"test-header.img"}}': LUKS payload is overlapping with the header


You need to pass some info into qcrypto_block_open to tell it that the
header is detached. Add a new enum entry to QCryptoBlockOpenFlags
perhaps.  Then skip the LUKS payload overlap check.

With 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 RESEND v3 02/10] crypto: Support generic LUKS encryption
Posted by Yong Huang 10 months, 3 weeks ago
On Thu, Jan 4, 2024 at 10:40 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, Dec 25, 2023 at 01:45:04PM +0800, Hyman Huang wrote:
> > By enhancing the LUKS driver, it is possible to enable
> > the detachable LUKS header and, as a result, achieve
> > general encryption for any disk format that QEMU has
> > supported.
> >
> > Take the qcow2 as an example, the usage of the generic
> > LUKS encryption as follows:
> >
> > 1. add a protocol blockdev node of data disk
> > $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > > "arguments":{"node-name":"libvirt-1-storage", "driver":"file",
> > > "filename":"/path/to/test_disk.qcow2"}}'
> >
> > 2. add a protocol blockdev node of LUKS header as above.
> > $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > > "arguments":{"node-name":"libvirt-2-storage", "driver":"file",
> > > "filename": "/path/to/cipher.gluks" }}'
> >
> > 3. add the secret for decrypting the cipher stored in LUKS
> >    header above
> > $ virsh qemu-monitor-command vm '{"execute":"object-add",
> > > "arguments":{"qom-type":"secret", "id":
> > > "libvirt-2-storage-secret0", "data":"abc123"}}'
> >
> > 4. add the qcow2-drived blockdev format node
> > $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > > "arguments":{"node-name":"libvirt-1-format", "driver":"qcow2",
> > > "file":"libvirt-1-storage"}}'
> >
> > 5. add the luks-drived blockdev to link the qcow2 disk with
> >    LUKS header by specifying the field "header"
> > $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > > "arguments":{"node-name":"libvirt-2-format", "driver":"luks",
> > > "file":"libvirt-1-format", "header":"libvirt-2-storage",
> > > "key-secret":"libvirt-2-format-secret0"}}'
> >
> > 6. add the virtio-blk device finally
> > $ virsh qemu-monitor-command vm '{"execute":"device_add",
> > > "arguments": {"num-queues":"1", "driver":"virtio-blk-pci",
> > > "drive": "libvirt-2-format", "id":"virtio-disk2"}}'
> >
> > The generic LUKS encryption method of starting a virtual
> > machine (VM) is somewhat similar to hot-plug in that both
> > maintaining the same json command while the starting VM
> > changes the "blockdev-add/device_add" parameters to
> > "blockdev/device".
> >
> > Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> > Message-Id: <
> 910801f303da1601051479d3b7e5c2c6b4e01eb7.1701879996.git.yong.huang@smartx.com
> >
> > ---
> >  block/crypto.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/crypto.c b/block/crypto.c
> > index f82b13d32b..6063879bac 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -64,12 +64,14 @@ static int block_crypto_read_func(QCryptoBlock
> *block,
> >                                    Error **errp)
> >  {
> >      BlockDriverState *bs = opaque;
> > +    BlockCrypto *crypto = bs->opaque;
> >      ssize_t ret;
> >
> >      GLOBAL_STATE_CODE();
> >      GRAPH_RDLOCK_GUARD_MAINLOOP();
> >
> > -    ret = bdrv_pread(bs->file, offset, buflen, buf, 0);
> > +    ret = bdrv_pread(crypto->header ? crypto->header : bs->file,
> > +                     offset, buflen, buf, 0);
> >      if (ret < 0) {
> >          error_setg_errno(errp, -ret, "Could not read encryption
> header");
> >          return ret;
> > @@ -269,6 +271,7 @@ static int
> block_crypto_open_generic(QCryptoBlockFormat format,
> >      QCryptoBlockOpenOptions *open_opts = NULL;
> >      unsigned int cflags = 0;
> >      QDict *cryptoopts = NULL;
> > +    const char *hdr_bdref = qdict_get_try_str(options, "header");
>
> This is an invalid check to make, because it is assuming the user is
> referencing a separate blockdev node name and doesn't work for an
> inline definition. eg
>
>   qemu-img info
> 'json:{"driver":"luks","file":{"filename":"test-payload.img"},"header":{"filename":"test-header.img"}}'
>
>
> >
> >      GLOBAL_STATE_CODE();
> >
> > @@ -277,6 +280,15 @@ static int
> block_crypto_open_generic(QCryptoBlockFormat format,
> >          return ret;
> >      }
> >
> > +    if (hdr_bdref) {
>
> Get rid of this 'if' clause and unconditionally call the next line:
>
> > +        crypto->header = bdrv_open_child(NULL, options, "header", bs,
> > +                                         &child_of_bds,
> BDRV_CHILD_METADATA,
> > +                                         false, errp);
>
> but pass 'true' instead of 'false' here to allow the child to be absent,
> and thus let it return NULL.
>
> > +        if (!crypto->header) {
>
> You'll need to then check  '*errp != NULL' instead
>
> You'll also need "ERRP_GUARD" at the start of the method
>
> > +            return -EINVAL;
> > +        }
> > +    }
> > +
> >      GRAPH_RDLOCK_GUARD_MAINLOOP();
> >
> >      bs->supported_write_flags = BDRV_REQ_FUA &
>
> This patch should be combined with the previous patch that adds the new
> QAPI schema element as splitting them doesn't add value.
>
>
> Testing this patch with the changes I suggest above, however, still does
> not work:
>
>   $ dd if=/dev/zero of=test-header.img bs=1M count=32
>   $ dd if=/dev/zero of=test-payload.img bs=1M count=1000
>   $ cryptsetup luksFormat  --header test-header.img test-payload.img
> --force-password --type luks1
>   $ qemu-img info
> 'json:{"driver":"luks","file":{"filename":"test-payload.img"},"header":{"filename":"test-header.img"}}'
>   qemu-img: Could not open
> 'json:{"driver":"luks","file":{"filename":"test-payload.img"},"header":{"filename":"test-header.img"}}':
> LUKS payload is overlapping with the header


>
> You need to pass some info into qcrypto_block_open to tell it that the
> header is detached. Add a new enum entry to QCryptoBlockOpenFlags
> perhaps.  Then skip the LUKS payload overlap check.
>
> Ok, I'll try this way and do a test using cryptsetup tools in the next
version.
Thanks for testing this series.


> With 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 :|
>
>

-- 
Best regards