On 06/25/2016 01:46 AM, Ján Tomko wrote:
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@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"...
OK
+ 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'"
OK
+ 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.
OK
Also, should we be validating these by converting them to an enum?
Good question! Unfortunately we have no way of knowing what any driver supports so what are we validating them against? I thought of creating a connection API that could/would fetch all the cipher/ivgen algorithm information from the hypervisor that's going to be using them. Then I investigated qemu to see what type of API was available only to find nothing "obvious". So for now, I just left these as a string. I suppose we could "mock up" a qemu response, but it's a lot more effort saved for another day. If bad values are provided, qemu-img will fall over and play dead during the volume create.
+ goto cleanup; + } +
+ /* Optional */
Hence no error following the calls.
Gone.
+ 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.
Gone
+ 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"
OK, done.
+ 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.
Done
+} + + int virStorageEncryptionFormat(virBufferPtr buf, virStorageEncryptionPtr enc)
ACK with the strings escaped.
Thanks for the look/time... John