6:13 a.m.
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(a)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