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@xxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list