On Tue, Sep 14, 2021 at 05:43:15 -0500, Or Ozeri wrote:
> Starting from ceph Pacific, RBD has built-in support for image-level encryption.
> qemu 6.1 added support for this encryption using a new "encrypt" property
> to the RBD qdict.
> This commit extends the libvirt XML API to allow the user to choose between
> the existing qemu encryption engine, and the new librbd encryption engine.
>
> Signed-off-by: Or Ozeri <oro@xxxxxxxxxx>
> ---
> docs/formatstorageencryption.html.in | 8 +++-
> docs/schemas/domainbackup.rng | 7 ++++
> docs/schemas/storagecommon.rng | 8 ++++
> src/conf/storage_encryption_conf.c | 30 +++++++++++++-
> src/conf/storage_encryption_conf.h | 11 +++++
> src/qemu/qemu_block.c | 40 +++++++++++++++++++
> src/qemu/qemu_domain.c | 3 +-
> .../backup-pull-encrypted.xml | 6 +--
> .../backup-pull-internal-invalid.xml | 6 +--
> .../backup-push-encrypted.xml | 6 +--
> tests/qemustatusxml2xmldata/upgrade-out.xml | 6 +--
> tests/qemuxml2argvdata/disk-nvme.xml | 2 +-
> tests/qemuxml2argvdata/disk-slices.xml | 4 +-
> .../qemuxml2argvdata/encrypted-disk-usage.xml | 2 +-
> tests/qemuxml2argvdata/encrypted-disk.xml | 2 +-
> .../luks-disks-source-qcow2.xml | 14 +++----
> tests/qemuxml2argvdata/luks-disks-source.xml | 10 ++---
> tests/qemuxml2argvdata/luks-disks.xml | 4 +-
> tests/qemuxml2argvdata/user-aliases.xml | 2 +-
> .../disk-slices.x86_64-latest.xml | 4 +-
> tests/qemuxml2xmloutdata/encrypted-disk.xml | 2 +-
> .../luks-disks-source-qcow2.x86_64-latest.xml | 14 +++----
> .../qemuxml2xmloutdata/luks-disks-source.xml | 10 ++---
> .../storagevolxml2xmlout/vol-luks-cipher.xml | 2 +-
> tests/storagevolxml2xmlout/vol-luks.xml | 2 +-
> .../vol-qcow2-encryption.xml | 2 +-
> tests/storagevolxml2xmlout/vol-qcow2-luks.xml | 2 +-
> 27 files changed, 154 insertions(+), 55 deletions(-)
There's a bit too much going on in this single commit. You'll probably
need to split it into more granular pieces.
>
> diff --git a/docs/formatstorageencryption.html.in b/docs/formatstorageencryption.html.in
> index 7215c307d7..e0eb8697aa 100644
> --- a/docs/formatstorageencryption.html.in
> +++ b/docs/formatstorageencryption.html.in
> @@ -18,11 +18,17 @@
> is <code>encryption</code>, with a mandatory
> attribute <code>format</code>. Currently defined values
> of <code>format</code> are <code>default</code>, <code>qcow</code>,
> - and <code>luks</code>.
> + <code>luks</code>, and <code>luks2</code>.
Adding 'luks2' ...
> Each value of <code>format</code> implies some expectations about the
> content of the <code>encryption</code> tag. Other format values may be
> defined in the future.
> </p>
> + <p>
> + The <code>encryption</code> tag supports an optional <code>engine</code>
> + tag, which allows selecting which component actually handles
> + the encryption. Currently defined values of <code>engine</code> are
> + <code>qemu</code> (default) and <code>librbd</code>.
... should be done separately from adding this.
> + </p>
> <p>
> The <code>encryption</code> tag can currently contain a sequence of
> <code>secret</code> tags, each with mandatory attributes <code>type</code>
[...]
> diff --git a/src/conf/storage_encryption_conf.c b/src/conf/storage_encryption_conf.c
> index 9112b96cc7..64044057bf 100644
> --- a/src/conf/storage_encryption_conf.c
> +++ b/src/conf/storage_encryption_conf.c
> @@ -44,7 +44,12 @@ VIR_ENUM_IMPL(virStorageEncryptionSecret,
>
> VIR_ENUM_IMPL(virStorageEncryptionFormat,
> VIR_STORAGE_ENCRYPTION_FORMAT_LAST,
> - "default", "qcow", "luks",
> + "default", "qcow", "luks", "luks2",
> +);
> +
> +VIR_ENUM_IMPL(virStorageEncryptionEngine,
> + VIR_STORAGE_ENCRYPTION_ENGINE_LAST,
> + "qemu", "librbd",
Same here. These are two separate changes.
> );
>
> static void
[...]
> @@ -239,6 +245,18 @@ virStorageEncryptionParseNode(xmlNodePtr node,
> goto cleanup;
> }
>
> + if (!(engine_str = virXPathString("string(./@engine)", ctxt))) {
> + encdef->engine = VIR_STORAGE_ENCRYPTION_ENGINE_QEMU;
QEMU must not be the default, more on that below. Additionally we
initialize the structs to 0, thus doing this explicitly is pointless.
> + } else {
> + if ((encdef->engine =
> + virStorageEncryptionEngineTypeFromString(engine_str)) < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
This should be "VIR_ERR_XML_ERROR".
> + _("unknown volume encryption engine type %s"),
> + engine_str);
> + goto cleanup;
> + }
> + }
> +
> if ((n = virXPathNodeSet("./secret", ctxt, &nodes)) < 0)
> goto cleanup;
>
> @@ -327,15 +345,23 @@ int
> virStorageEncryptionFormat(virBuffer *buf,
> virStorageEncryption *enc)
> {
> + const char *engine;
> const char *format;
> size_t i;
>
> + if (!(engine = virStorageEncryptionEngineTypeToString(enc->engine))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("unexpected encryption engine"));
> + return -1;
> + }
> +
> if (!(format = virStorageEncryptionFormatTypeToString(enc->format))) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("unexpected encryption format"));
> return -1;
> }
> - virBufferAsprintf(buf, "<encryption format='%s'>\n", format);
> + virBufferAsprintf(buf, "<encryption format='%s' engine='%s'>\n", format,
> + engine);
You'll need to have possibility to keep engine optional.
> virBufferAdjustIndent(buf, 2);
>
> for (i = 0; i < enc->nsecrets; i++) {
> diff --git a/src/conf/storage_encryption_conf.h b/src/conf/storage_encryption_conf.h
> index 34adbd5f7b..bd8787be98 100644
> --- a/src/conf/storage_encryption_conf.h
> +++ b/src/conf/storage_encryption_conf.h
> @@ -51,11 +51,21 @@ struct _virStorageEncryptionInfoDef {
> char *ivgen_hash;
> };
>
> +typedef enum {
> + /* "default" is only valid for volume creation */
> + VIR_STORAGE_ENCRYPTION_ENGINE_QEMU = 0, /* default */
This is a too qemu-centric view. We'll have to keep a default of the
engine being unspecified. We can then use either some logic in the post
parse callback or when starting the VM in the qemu driver to pick QEMU
as the engine but filling it in into everything is IMO not desired.
> + VIR_STORAGE_ENCRYPTION_ENGINE_LIBRBD,
> +
> + VIR_STORAGE_ENCRYPTION_ENGINE_LAST,
> +} virStorageEncryptionEngineType;
> +VIR_ENUM_DECL(virStorageEncryptionEngine);
[...]
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 0bc92f6a23..e064e5a490 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
[...]
> @@ -899,12 +901,47 @@ qemuBlockStorageSourceGetRBDProps(virStorageSource *src,
> return NULL;
> }
>
> + if (src->encryption &&
> + src->encryption->engine == VIR_STORAGE_ENCRYPTION_ENGINE_LIBRBD) {
> + switch ((virStorageEncryptionFormatType) src->encryption->format) {
> + case VIR_STORAGE_ENCRYPTION_FORMAT_LUKS:
> + encformat = "luks";
> + break;
> +
> + case VIR_STORAGE_ENCRYPTION_FORMAT_LUKS2:
> + encformat = "luks2";
> + break;
> +
> + case VIR_STORAGE_ENCRYPTION_FORMAT_DEFAULT:
> + case VIR_STORAGE_ENCRYPTION_FORMAT_QCOW:
> + case VIR_STORAGE_ENCRYPTION_FORMAT_LAST:
> + default:
> + virReportEnumRangeError(virStorageEncryptionFormatType,
> + src->encryption->format);
> + return NULL;
This should also be validated in 'qemu_validate.c' so that we fail
before even attempting to start the VM.
> + }
> +
> + if (!srcPriv || !srcPriv->encinfo || !srcPriv->encinfo->s.aes.alias) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing secret info for rbd encryption driver"));
> + return NULL;
> + }
This shouldn't be possible and needs to be checked elsewhere. The props
formatter isn't a place for such logic.
> +
> + encrypt = virJSONValueNewObject();
> + if (virJSONValueObjectAdd(encrypt,
Use virJSONValueObjectCreate instead of the two lines.
> + "s:format", encformat,
> + "s:key-secret", srcPriv->encinfo->s.aes.alias,
> + NULL) < 0)
> + return NULL;
> + }
> +
> if (virJSONValueObjectCreate(&ret,
> "s:pool", src->volume,
> "s:image", src->path,
> "S:snapshot", src->snapshot,
> "S:conf", src->configFile,
> "A:server", &servers,
> + "A:encrypt", &encrypt,
> "S:user", username,
> "A:auth-client-required", &authmodes,
> "S:key-secret", keysecret,
> diff --git a/tests/qemuxml2argvdata/disk-nvme.xml b/tests/qemuxml2argvdata/disk-nvme.xml
> index 1ccbbfd598..9a5fafce7d 100644
> --- a/tests/qemuxml2argvdata/disk-nvme.xml
> +++ b/tests/qemuxml2argvdata/disk-nvme.xml
> @@ -42,7 +42,7 @@
> <driver name='qemu' type='qcow2' cache='none'/>
> <source type='pci' managed='no' namespace='2'>
> <address domain='0x0001' bus='0x02' slot='0x00' function='0x0'/>
> - <encryption format='luks'>
> + <encryption format='luks' engine='qemu'>
It should be also now possible to use both
<encryption format='luks' engine='qemu'>
and
<encryption format='luks' engine='librbd'>
In the same config given the -blockdev layering. You should add a test
for it.
> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/>
> </encryption>
> </source>
qemuxml2argvtest and qemuxml2xmltest cases using librbd are completely
missing.
Also if librbd encryption was added recently you will need to add
capability probing for it and validate that given qemu actually supports
it.
