Re: [libvirt PATCH 2/2] storage: add support for qcow2 LUKS encryption

[Han Han <hhan@xxxxxxxxxx>]

On Thu, Sep 17, 2020 at 1:06 AM Daniel P. Berrangé <berrange@xxxxxxxxxx> wrote:

The storage driver was wired up to support creating raw volumes in LUKS
format, but was never adapted to support LUKS-in-qcow2. This is trivial
as it merely requires the encryption properties to be prefixed with
the "encrypt." prefix, and "encrypt.format=luks" when creating the
volume.

Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
---
 src/storage/storage_util.c | 70 +++++++++++++++++++++++++++++---------
 src/util/virqemu.c         | 23 +++++++++----
 src/util/virqemu.h         |  1 +
 3 files changed, 72 insertions(+), 22 deletions(-)

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index cf82ea0a87..e5e4fe428f 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -707,7 +707,7 @@ storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDefPtr encinfo,
                           virStorageFileFormatTypeToString(info->backingFormat));

     if (encinfo)
-        virQEMUBuildQemuImgKeySecretOpts(&buf, encinfo, info->secretAlias);
+        virQEMUBuildQemuImgKeySecretOpts(&buf, info->format, encinfo, info->secretAlias);

     if (info->preallocate) {
         if (info->size_arg > info->allocation)
@@ -761,7 +761,8 @@ storageBackendCreateQemuImgCheckEncryption(int format,
 {
     virStorageEncryptionPtr enc = vol->target.encryption;

-    if (format == VIR_STORAGE_FILE_RAW) {
+    if (format == VIR_STORAGE_FILE_RAW ||
+        format == VIR_STORAGE_FILE_QCOW2) {
         if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                            _("unsupported volume encryption format %d"),
@@ -927,21 +928,34 @@ storageBackendCreateQemuImgSecretObject(virCommandPtr cmd,
 }


-/* Add a --image-opts to the qemu-img resize command line:
+/* Add a --image-opts to the qemu-img resize command line for use
+ * with encryption:
  *    --image-opts driver=luks,file.filename=$volpath,key-secret=$secretAlias
+ * or
+ *    --image-opts driver=qcow2,file.filename=$volpath,encrypt.key-secret=$secretAlias
  *
- *    NB: format=raw is assumed
  */
 static int
 storageBackendResizeQemuImgImageOpts(virCommandPtr cmd,
+                                     int format,
                                      const char *path,
                                      const char *secretAlias)
 {
     g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
     g_autofree char *commandStr = NULL;
+    const char *encprefix;
+    const char *driver;

-    virBufferAsprintf(&buf, "driver=luks,key-secret=%s,file.filename=",
-                      secretAlias);
+    if (format == VIR_STORAGE_FILE_QCOW2) {
+        driver = "qcow2";
+        encprefix = "encrypt.";
+    } else {
+        driver = "luks";
+        encprefix = "";
+    }
+
+    virBufferAsprintf(&buf, "driver=%s,%skey-secret=%s,file.filename=",
+                      driver, encprefix, secretAlias);
     virQEMUBuildBufferEscapeComma(&buf, path);

     commandStr = virBufferContentAndReset(&buf);
@@ -1006,6 +1020,16 @@ virStorageBackendCreateQemuImgSetInfo(virStoragePoolObjPtr pool,
             return -1;
         }
     }

storagevolxml2argvtest gets segment fault at this function:

➜  ~ abs_top_srcdir='/root/libvirt-6.7.0' LC_ALL='C' abs_top_builddir='/root/libvirt-6.7.0/build' abs_srcdir='/root/libvirt-6.7.0/tests' abs_builddir='/root/libvirt-6.7.0/build/tests' VIR_TEST_EXPENSIVE='0' LIBVIRT_AUTOSTART='0' /root/libvirt-6.7.0/build/tests/storagevolxml2argvtest                                                                                                                                                                                            

TEST: storagevolxml2argvtest
      ...................![1]    916320 segmentation fault (core dumped)  abs_top_srcdir='/root/libvirt-6.7.0' LC_ALL='C' abs_top_builddir= abs_srcdir=

Backtrace:

(gdb) bt
#0  0x0000558b997ea149 in virStorageBackendCreateQemuImgSetInfo (info=<synthetic pointer>, convertStep=VIR_STORAGE_VOL_ENCRYPT_CREATE, inputvol=0x558b9b32e810, vol=0x558b9b32f840, pool=0x558b9b323b30)                                  
    at ../src/storage/storage_util.c:1025
#1  0x0000558b997ea149 in virStorageBackendCreateQemuImgCmdFromVol
    (pool=0x558b9b323b30, vol=0x558b9b32f840, inputvol=0x558b9b32e810, flags=0, create_tool=0x558b997f6a00 <create_tool> "qemu-img", secretPath=0x558b997f6537 "/path/to/secretFile", inputSecretPath=0x558b997f654b "/path/to/inputSecretFile", convertStep=VIR_STORAGE_VOL_ENCRYPT_CREATE) at ../src/storage/storage_util.c:1103
#2  0x0000558b997e4b57 in testCompareXMLToArgvFiles
    (parse_flags=<optimized out>, flags=0, cmdline=0x558b9b325030 "/root/libvirt-6.7.0/tests/storagevolxml2argvdata/luks-convert-encrypt2fileraw.argv", inputvolxml=0x558b9b32f7f0 "/root/libvirt-6.7.0/tests/storagevolxml2xmlin/vol-encrypt2.xml", inputpoolxml=0x558b9b331730 "/root/libvirt-6.7.0/tests/storagepoolxml2xmlin/pool-dir.xml", volxml=0x558b9b3309f0 "/root/libvirt-6.7.0/tests/storagevolxml2xmlin/vol-file.xml", poolxml=0x558b9b32a530 "/root/libvirt-6.7.0/tests/storagepoolxml2xmlin/pool-dir.xml", shouldFail=false) at ../tests/storagevolxml2argvtest.c:92
#3  0x0000558b997e4b57 in testCompareXMLToArgvHelper (data="" out>) at ../tests/storagevolxml2argvtest.c:174
#4  0x0000558b997e561a in virTestRun (title=0x558b997f6928 "Storage Vol XML-2-argv luks-convert-encrypt2fileraw", body=0x558b997e4930 <testCompareXMLToArgvHelper>, data="" at ../tests/testutils.c:142                      
#5  0x0000558b997e489a in mymain () at ../tests/storagevolxml2argvtest.c:271
#6  0x0000558b997e6512 in virTestMain (argc=1, argv=0x7ffea16a5aa8, func=0x558b997e40d0 <mymain>) at ../tests/testutils.c:841                                                                                                              
#7  0x00007f2597f5b7b3 in __libc_start_main (main=0x558b997e3fd0 <main>, argc=1, argv=0x7ffea16a5aa8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffea16a5a98) at ../csu/libc-start.c:308          
#8  0x0000558b997e400e in _start () at ../tests/storagevolxml2argvtest.c:282

+    if (inputvol && inputvol->target.format == VIR_STORAGE_FILE_RAW &&
+        inputvol->target.encryption) {
+        if (vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
+            info->type = "luks";
+        } else {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("Only luks encryption is supported for raw files"));
+            return -1;
+        }
+    }

     if (inputvol &&
         storageBackendCreateQemuImgSetInput(inputvol, convertStep, info) < 0)
@@ -1056,6 +1080,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
     virStorageEncryptionPtr inputenc = inputvol ? inputvol->target.encryption : NULL;
     virStorageEncryptionInfoDefPtr encinfo = NULL;
     g_autofree char *inputSecretAlias = NULL;
+    const char *encprefix;
+    const char *inputencprefix;

     virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);

@@ -1134,24 +1160,34 @@ virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool,
             virCommandAddArgFormat(cmd, "%lluK", info.size_arg);
     } else {
         /* source */
-        if (inputenc)
+        if (inputenc) {
+            if (inputvol->target.format == VIR_STORAGE_FILE_QCOW2)
+                inputencprefix = "encrypt.";
+            else
+                inputencprefix = "";
             virCommandAddArgFormat(cmd,
-                                   "driver=luks,file.filename=%s,key-secret=%s",
-                                   info.inputPath, inputSecretAlias);
-        else
+                                   "driver=%s,file.filename=%s,%skey-secret=%s",
+                                   info.inputType, info.inputPath, inputencprefix, inputSecretAlias);
+        } else {
             virCommandAddArgFormat(cmd, "driver=%s,file.filename=%s",
                                    info.inputType ? info.inputType : "raw",
                                    info.inputPath);
+        }

         /* dest */
-        if (enc)
+        if (enc) {
+            if (vol->target.format == VIR_STORAGE_FILE_QCOW2)
+                encprefix = "encrypt.";
+            else
+                encprefix = "";
+
             virCommandAddArgFormat(cmd,
-                                   "driver=%s,file.filename=%s,key-secret=%s",
-                                   info.type, info.path, info.secretAlias);
-        else
+                                   "driver=%s,file.filename=%s,%skey-secret=%s",
+                                   info.type, info.path, encprefix, info.secretAlias);
+        } else {
             virCommandAddArgFormat(cmd, "driver=%s,file.filename=%s",
                                    info.type, info.path);
-
+        }
     }
     VIR_FREE(info.secretAlias);

@@ -2276,7 +2312,9 @@ storageBackendResizeQemuImg(virStoragePoolObjPtr pool,
                                                     secretAlias) < 0)
             goto cleanup;

-        if (storageBackendResizeQemuImgImageOpts(cmd, vol->target.path,
+        if (storageBackendResizeQemuImgImageOpts(cmd,
+                                                 vol->target.format,
+                                                 vol->target.path,
                                                  secretAlias) < 0)
             goto cleanup;
     }
diff --git a/src/util/virqemu.c b/src/util/virqemu.c
index 25d6fd35c5..bbb38eed75 100644
--- a/src/util/virqemu.c
+++ b/src/util/virqemu.c
@@ -28,6 +28,7 @@
 #include "virqemu.h"
 #include "virstring.h"
 #include "viralloc.h"
+#include "virstoragefile.h"

 #define VIR_FROM_THIS VIR_FROM_NONE

@@ -407,36 +408,46 @@ virQEMUBuildBufferEscapeComma(virBufferPtr buf, const char *str)
  */
 void
 virQEMUBuildQemuImgKeySecretOpts(virBufferPtr buf,
+                                 int format,
                                  virStorageEncryptionInfoDefPtr encinfo,
                                  const char *alias)
 {
-    virBufferAsprintf(buf, "key-secret=%s,", alias);
+    const char *encprefix;
+
+    if (format == VIR_STORAGE_FILE_QCOW2) {
+        virBufferAsprintf(buf, "encrypt.format=luks,");

syntax check is failed here:

--- command ---
09:13:21 /usr/bin/make -C /root/libvirt-6.7.0/build/build-aux sc_prohibit_virBufferAsprintf_with_string_literal
--- stdout ---
make: Entering directory '/root/libvirt-6.7.0/build/build-aux'
prohibit_virBufferAsprintf_with_string_literal
/root/libvirt-6.7.0/src/util/virqemu.c:418:        virBufferAsprintf(buf, "encrypt.format=luks,");
 

+        encprefix = "encrypt.";
+    } else {
+        encprefix = "";
+    }
+
+    virBufferAsprintf(buf, "%skey-secret=%s,", encprefix, alias);

     if (!encinfo->cipher_name)
         return;

-    virBufferAddLit(buf, "cipher-alg=");
+    virBufferAsprintf(buf, "%scipher-alg=", encprefix);
     virQEMUBuildBufferEscapeComma(buf, encinfo->cipher_name);
     virBufferAsprintf(buf, "-%u,", encinfo->cipher_size);
     if (encinfo->cipher_mode) {
-        virBufferAddLit(buf, "cipher-mode=");
+        virBufferAsprintf(buf, "%scipher-mode=", encprefix);
         virQEMUBuildBufferEscapeComma(buf, encinfo->cipher_mode);
         virBufferAddLit(buf, ",");
     }
     if (encinfo->cipher_hash) {
-        virBufferAddLit(buf, "hash-alg=");
+        virBufferAsprintf(buf, "%shash-alg=", encprefix);
         virQEMUBuildBufferEscapeComma(buf, encinfo->cipher_hash);
         virBufferAddLit(buf, ",");
     }
     if (!encinfo->ivgen_name)
         return;

-    virBufferAddLit(buf, "ivgen-alg=");
+    virBufferAsprintf(buf, "%sivgen-alg=", encprefix);
     virQEMUBuildBufferEscapeComma(buf, encinfo->ivgen_name);
     virBufferAddLit(buf, ",");

     if (encinfo->ivgen_hash) {
-        virBufferAddLit(buf, "ivgen-hash-alg=");
+        virBufferAsprintf(buf, "%sivgen-hash-alg=", encprefix);
         virQEMUBuildBufferEscapeComma(buf, encinfo->ivgen_hash);
         virBufferAddLit(buf, ",");
     }
diff --git a/src/util/virqemu.h b/src/util/virqemu.h
index b1296cb657..be14c04d51 100644
--- a/src/util/virqemu.h
+++ b/src/util/virqemu.h
@@ -60,6 +60,7 @@ char *virQEMUBuildDriveCommandlineFromJSON(virJSONValuePtr src);

 void virQEMUBuildBufferEscapeComma(virBufferPtr buf, const char *str);
 void virQEMUBuildQemuImgKeySecretOpts(virBufferPtr buf,
+                                      int format,
                                       virStorageEncryptionInfoDefPtr enc,
                                       const char *alias)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
--
2.26.2

--

Reviewed-by: Han Han <hhan@xxxxxxxxxx>