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

[John Ferlan]

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 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"...
> 

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