[libvirt] [PATCH v3 05/10] encryption: Add <cipher> and <ivgen> to encryption

[Ján Tomko]

On Fri, Jun 24, 2016 at 04:53:34PM -0400, John Ferlan wrote:
>For a luks device, allow the configuration of a specific cipher to be
>used for encrypting the volume.
>
>Signed-off-by: John Ferlan <jferlan at redhat.com>
>---
> docs/formatstorageencryption.html.in               |  83 ++++++++++++-
> docs/schemas/storagecommon.rng                     |  44 ++++++-
> src/conf/domain_conf.c                             |  11 ++
> src/util/virstorageencryption.c                    | 138 +++++++++++++++++++++
> src/util/virstorageencryption.h                    |  14 +++
> .../qemuxml2argv-luks-disk-cipher.xml              |  45 +++++++
> .../qemuxml2xmlout-luks-disk-cipher.xml            |   1 +
> tests/qemuxml2xmltest.c                            |   1 +
> tests/storagevolxml2xmlin/vol-luks-cipher.xml      |  23 ++++
> tests/storagevolxml2xmlout/vol-luks-cipher.xml     |  23 ++++
> tests/storagevolxml2xmltest.c                      |   1 +
> 11 files changed, 378 insertions(+), 6 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.xml
> create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disk-cipher.xml
> create mode 100644 tests/storagevolxml2xmlin/vol-luks-cipher.xml
> create mode 100644 tests/storagevolxml2xmlout/vol-luks-cipher.xml
>

>+static int
>+virStorageEncryptionInfoParseCipher(xmlNodePtr info_node,
>+                                    virStorageEncryptionInfoDefPtr info)
>+{
>+    int ret = -1;
>+    char *size_str = NULL;
>+
>+    if (!(info->cipher_name = virXMLPropString(info_node, "name"))) {
>+        virReportError(VIR_ERR_XML_ERROR, "%s",
>+                       _("missing cipher info name string"));

Either 'missing cipher name' or use "cipher info missing '%s'
attribute"...

>+        goto cleanup;
>+    }
>+
>+    if ((size_str = virXMLPropString(info_node, "size")) &&
>+        virStrToLong_uip(size_str, NULL, 10, &info->cipher_size) < 0) {
>+        virReportError(VIR_ERR_XML_ERROR,
>+                       _("cannot parse cipher info size string '%s'"),

"cannot parse cipher size: '%s'"

>+                       size_str);
>+        goto cleanup;
>+    }
>+
>+    if (!size_str) {
>+        virReportError(VIR_ERR_XML_ERROR, "%s",
>+                       _("cipher info missing 'size' attribute"));

to share the string with this error.

Also, should we be validating these by converting them to an enum?

>+        goto cleanup;
>+    }
>+

>+    /* Optional */

Hence no error following the calls.

>+    info->cipher_mode = virXMLPropString(info_node, "mode");
>+    info->cipher_hash = virXMLPropString(info_node, "hash");
>+
>+    ret = 0;
>+
>+ cleanup:
>+    VIR_FREE(size_str);
>+    return ret;
>+}
>+
>+
>+static int
>+virStorageEncryptionInfoParseIvgen(xmlNodePtr info_node,
>+                                   virStorageEncryptionInfoDefPtr info)
>+{
>+    if (!(info->ivgen_name = virXMLPropString(info_node, "name"))) {
>+        virReportError(VIR_ERR_XML_ERROR, "%s",
>+                       _("missing ivgen info name string"));
>+        return -1;
>+    }
>+

>+    /* Optional */

Also redundant.

>+    info->ivgen_hash = virXMLPropString(info_node, "hash");
>+
>+    return 0;
>+}
>+
>+
> static virStorageEncryptionPtr
> virStorageEncryptionParseXML(xmlXPathContextPtr ctxt)
> {
>@@ -196,6 +286,28 @@ virStorageEncryptionParseXML(xmlXPathContextPtr ctxt)
>         VIR_FREE(nodes);
>     }
>
>+    if (ret->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
>+        xmlNodePtr tmpnode;
>+
>+        if ((tmpnode = virXPathNode("./cipher[1]", ctxt))) {
>+            if (virStorageEncryptionInfoParseCipher(tmpnode, &ret->encinfo) < 0)
>+                goto cleanup;
>+        }
>+
>+        if ((tmpnode = virXPathNode("./ivgen[1]", ctxt))) {

>+            /* If no cipher node, then fail */

Rather than putting it in this comment,

>+            if (!ret->encinfo.cipher_name) {
>+                virReportError(VIR_ERR_XML_ERROR, "%s",
>+                                _("missing storage encryption cipher"));

how about putting it in the error:
"ivgen element found but cipher is missing"

>+                goto cleanup;
>+            }
>+
>+            if (virStorageEncryptionInfoParseIvgen(tmpnode, &ret->encinfo) < 0)
>+                goto cleanup;
>+        }
>+    }
>+
>+
>     return ret;
>
>  cleanup:
>@@ -250,6 +362,28 @@ virStorageEncryptionSecretFormat(virBufferPtr buf,
>     return 0;
> }
>
>+
>+static void
>+virStorageEncryptionInfoDefFormat(virBufferPtr buf,
>+                                  const virStorageEncryptionInfoDef *enc)
>+{
>+    virBufferAsprintf(buf, "<cipher name='%s' size='%u'",
>+                      enc->cipher_name, enc->cipher_size);
>+    if (enc->cipher_mode)
>+        virBufferAsprintf(buf, " mode='%s'", enc->cipher_mode);
>+    if (enc->cipher_hash)
>+        virBufferAsprintf(buf, " hash='%s'", enc->cipher_hash);
>+    virBufferAddLit(buf, "/>\n");
>+
>+    if (enc->ivgen_name) {
>+        virBufferAsprintf(buf, "<ivgen name='%s'", enc->ivgen_name);
>+        if (enc->ivgen_hash)
>+            virBufferAsprintf(buf, " hash='%s'", enc->ivgen_hash);
>+        virBufferAddLit(buf, "/>\n");
>+    }

The strings need to be escaped.

>+}
>+
>+
> int
> virStorageEncryptionFormat(virBufferPtr buf,
>                            virStorageEncryptionPtr enc)

ACK with the strings escaped.

Jan