[libvirt] [PATCH v3 06/10] storage: Add support to create a luks volume
John Ferlan
jferlan at redhat.com
Sun Jun 26 11:03:59 UTC 2016
On 06/25/2016 02:18 AM, Ján Tomko wrote:
> On Fri, Jun 24, 2016 at 04:53:35PM -0400, John Ferlan wrote:
>> Partially resolves:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1301021
>>
>> If the volume xml was looking to create a luks volume take the necessary
>> steps in order to make that happen.
>>
>> The processing will be:
>> 1. create a temporary file in the storage driver state dir path
>> 1a. the file name will include the pool name, the volume name, secret,
>> and XXXXXX for usage in the mkostemp call.
>> 1b. mkostemp the file, initially setting mode to 0600 with current
>> effective uid:gid as owner
>> 1c. fetch the secret into a buffer and write that into the file
>>
>> 2. create a secret object
>> 2a. use an alias combinding the volume name and "_luks0"
>> 2b. add the file to the object
>>
>> 3. create/add luks options to the commandline
>> 3a. at the very least a "key-secret" using the secret object alias
>> 3b. if found in the XML the various "cipher" and "ivgen" options
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>> src/libvirt_private.syms | 1 +
>> src/storage/storage_backend.c | 254
>> ++++++++++++++++++++++++++++++++++++++---
>> src/storage/storage_backend.h | 3 +-
>> src/util/virqemu.c | 23 ++++
>> src/util/virqemu.h | 6 +
>> tests/storagevolxml2argvtest.c | 3 +-
>> 6 files changed, 269 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 9ac033d..eea18dd 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2169,6 +2169,7 @@ virProcessWait;
>>
>>
>> # util/virqemu.h
>> +virQEMUBuildLuksOpts;
>> virQEMUBuildObjectCommandlineFromJSON;
>>
>>
>> diff --git a/src/storage/storage_backend.c
>> b/src/storage/storage_backend.c
>> index 97f6ffe..4fb77fc 100644
>> --- a/src/storage/storage_backend.c
>> +++ b/src/storage/storage_backend.c
>> @@ -55,11 +55,14 @@
>> #include "viralloc.h"
>> #include "internal.h"
>> #include "secret_conf.h"
>> +#include "secret_util.h"
>> #include "viruuid.h"
>> #include "virstoragefile.h"
>> #include "storage_backend.h"
>> #include "virlog.h"
>> #include "virfile.h"
>> +#include "virjson.h"
>> +#include "virqemu.h"
>> #include "stat-time.h"
>> #include "virstring.h"
>> #include "virxml.h"
>
>> @@ -880,6 +883,7 @@ virStoragePloopResize(virStorageVolDefPtr vol,
>> enum {
>> QEMU_IMG_BACKING_FORMAT_OPTIONS = 0,
>> QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT,
>> + QEMU_IMG_FORMAT_LUKS,
>> };
>>
>> static bool
>> @@ -907,6 +911,27 @@ virStorageBackendQemuImgSupportsCompat(const char
>> *qemuimg)
>> return ret;
>> }
>>
>> +
>> +static bool
>> +virStorageBackendQemuImgSupportsLuks(const char *qemuimg)
>> +{
>> + bool ret = false;
>> + int exitstatus = -1;
>> + virCommandPtr cmd = virCommandNewArgList(qemuimg, "create", "-o",
>> "?",
>> + "-f", "luks",
>> "/dev/null", NULL);
>> +
>> + if (virCommandRun(cmd, &exitstatus) < 0)
>> + goto cleanup;
>> +
>> + if (exitstatus == 0)
>> + ret = true;
>> +
>> + cleanup:
>> + virCommandFree(cmd);
>> + return ret;
>> +}
>> +
>> +
>
> NACK to this function.
>
> Old qemu-img supports the -f option and correctly errors out on
> unsupported LUKS format.
> There is no reason to probe for it.
>
>> static int
>> virStorageBackendQEMUImgBackingFormat(const char *qemuimg)
>> {
>> @@ -915,12 +940,18 @@ virStorageBackendQEMUImgBackingFormat(const char
>> *qemuimg)
>> * out what else we have */
>> int ret = QEMU_IMG_BACKING_FORMAT_OPTIONS;
>>
>> - /* QEMU 2.0 changed to using a format that only QEMU 1.1 and newer
>> - * understands. Since we still support QEMU 0.12 and newer, we need
>> - * to be able to handle the previous format as can be set via a
>> - * compat=0.10 option. */
>> - if (virStorageBackendQemuImgSupportsCompat(qemuimg))
>> - ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT;
>> + /* QEMU 2.6 added support for luks - let's check for that.
>> + * If available, then we can also assume OPTIONS_COMPAT is
>> present */
>> + if (virStorageBackendQemuImgSupportsLuks(qemuimg)) {
>> + ret = QEMU_IMG_FORMAT_LUKS;
>> + } else {
>> + /* QEMU 2.0 changed to using a format that only QEMU 1.1 and
>> newer
>> + * understands. Since we still support QEMU 0.12 and newer,
>> we need
>> + * to be able to handle the previous format as can be set via a
>> + * compat=0.10 option. */
>> + if (virStorageBackendQemuImgSupportsCompat(qemuimg))
>> + ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT;
>> + }
>>
>> return ret;
>> }
>
> This won't be needed either.
>
That'll make Peter happier... It did seem though that the purpose of
this function was to check for features available by the options that
are found in qemu-img. Sure, qemu-img will fail, but that will be much
later.
>> @@ -1121,6 +1184,7 @@
>> virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool,
>>
>> static int
>> virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd,
>> + virStorageVolDefPtr vol,
>> int imgformat,
>> struct
>> _virStorageBackendQemuImgInfo info)
>
> FWIW the whole point of _virStorageBackendQemuImgInfo was to separate
> command line building from the volume definition, to allow reuse in
> qemuDomainSnapshotCreateInactiveExternal where we don't deal with a
> volume. But I never got around to finishing it.
>
> Please use virStorageEncryptionInfoDefPtr instead of virStorageVolDefPtr
> here.
>
OK - that works. I also added a bit of comments to the structure...
>> {
>> @@ -1130,7 +1194,8 @@
>> virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd,
>> imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT)
>> info.compat = "0.10";
>>
>> - if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0)
>> + if
>> (virStorageBackendCreateQemuImgOpts(&vol->target.encryption->encinfo,
>
> Can vol->target.encryption be NULL here?
>
Good catch.
Back in the caller virStorageBackendCreateQemuImgCmdFromVol, I added a
local 'enc' initialized to NULL. Then only set it when info.format ==
VIR_STORAGE_FILE_LUKS && virStorageBackendCreateQemuImgSecretObject is
successful.
Then in virStorageBackendCreateQemuImgOpts, if info.format ==
VIR_STORAGE_FILE_LUKS, I'll ensure 'enc' is not NULL before trying to
call virQEMUBuildLuksOpts. If it is NULL, then the code will just error
out.
>> + &opts, info) < 0)
>> return -1;
>> if (opts)
>> virCommandAddArgList(cmd, "-o", opts, NULL);
>> @@ -1140,6 +1205,43 @@
>> virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd,
>> }
>>
>>
>> +/* Add a secret object to the command line:
>> + * --object secret,id=$secretAlias,file=$secretPath
>> + *
>> + * NB: format=raw is assumed
>> + */
>> +static int
>> +virStorageBackendCreateQemuImgSecretObject(virCommandPtr cmd,
>> + virStorageVolDefPtr vol,
>> + struct
>> _virStorageBackendQemuImgInfo *info)
>> +{
>> + char *str = NULL;
>> + virJSONValuePtr props = NULL;
>> + char *commandStr = NULL;
>> +
>> + if (virAsprintf(&info->secretAlias, "%s_luks0", vol->name) < 0) {
>> + VIR_FREE(str);
>> + return -1;
>> + }
>> + VIR_FREE(str);
>> +
>> + if (virJSONValueObjectCreate(&props, "s:file", info->secretPath,
>> NULL) < 0)
>> + return -1;
>> +
>> + if (!(commandStr = virQEMUBuildObjectCommandlineFromJSON("secret",
>> +
>> info->secretAlias,
>> + props))) {
>> + virJSONValueFree(props);
>> + return -1;
>> + }
>> + virJSONValueFree(props);
>> +
>> + virCommandAddArgList(cmd, "--object", commandStr, NULL);
>> +
>> + return 0;
>> +}
>> +
>> +
>> /* Create a qemu-img virCommand from the supplied binary path,
>> * volume definitions and imgformat
>> */
>> @@ -1150,7 +1252,8 @@
>> virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
>> virStorageVolDefPtr inputvol,
>> unsigned int flags,
>> const char *create_tool,
>> - int imgformat)
>> + int imgformat,
>> + const char *secretPath)
>> {
>> virCommandPtr cmd = NULL;
>> const char *type;
>> @@ -1162,6 +1265,8 @@
>> virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
>> .compat = vol->target.compat,
>> .features = vol->target.features,
>> .nocow = vol->target.nocow,
>> + .secretPath = secretPath,
>> + .secretAlias = NULL,
>> };
>>
>> virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
>> @@ -1192,6 +1297,18 @@
>> virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
>> _("format features only available with qcow2"));
>> return NULL;
>> }
>> + if (info.format == VIR_STORAGE_FILE_LUKS) {
>> + if (inputvol) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("cannot use inputvol with luks volume"));
>> + return NULL;
>> + }
>> + if (!info.encryption) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("missing encryption description"));
>> + return NULL;
>> + }
>> + }
>>
>> if (inputvol &&
>> virStorageBackendCreateQemuImgSetInput(inputvol, &info) < 0)
>> @@ -1207,7 +1324,6 @@
>> virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
>> conn, vol) < 0)
>> return NULL;
>>
>> -
>
> Spurious whitespace change.
>
OK
>> /* Size in KB */
>> info.size_arg = VIR_DIV_UP(vol->target.capacity, 1024);
>>
>> @@ -1226,11 +1342,21 @@
>> virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
>> if (info.backingPath)
>> virCommandAddArgList(cmd, "-b", info.backingPath, NULL);
>>
>> - if (virStorageBackendCreateQemuImgSetOptions(cmd, imgformat,
>> info) < 0) {
>> + if (info.format == VIR_STORAGE_FILE_LUKS &&
>> + virStorageBackendCreateQemuImgSecretObject(cmd, vol, &info) <
>> 0) {
>> + VIR_FREE(info.secretAlias);
>> virCommandFree(cmd);
>> return NULL;
>> }
>>
>> + if (virStorageBackendCreateQemuImgSetOptions(cmd, vol, imgformat,
>> + info) < 0) {
>> + VIR_FREE(info.secretAlias);
>> + virCommandFree(cmd);
>> + return NULL;
>> + }
>> + VIR_FREE(info.secretAlias);
>> +
>> if (info.inputPath)
>> virCommandAddArg(cmd, info.inputPath);
>> virCommandAddArg(cmd, info.path);
>> @@ -1240,6 +1366,78 @@
>> virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
>> return cmd;
>> }
>>
>> +
>> +static char *
>> +virStorageBackendCreateQemuImgSecretPath(virConnectPtr conn,
>> + virStoragePoolObjPtr pool,
>> + virStorageVolDefPtr vol)
>> +{
>> + virStorageEncryptionPtr enc = vol->target.encryption;
>> + char *secretPath = NULL;
>> + int fd = -1;
>> + uint8_t *secret = NULL;
>> + size_t secretlen = 0;
>> +
>> + if (!enc) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("missing encryption description"));
>> + return NULL;
>> + }
>> +
>> + if (!conn || !conn->secretDriver ||
>> + !conn->secretDriver->secretLookupByUUID ||
>> + !conn->secretDriver->secretLookupByUsage ||
>> + !conn->secretDriver->secretGetValue) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("unable to look up encryption secret"));
>> + return NULL;
>> + }
>> +
>
>> + /* Since we don't have a file, just go to cleanup using NULL
>> secretPath */
>
> What is the purpose of this comment?
>
it's gone
>> + if (!(secretPath = virStoragePoolObjBuildTempFilePath(pool, vol)))
>> + goto cleanup;
>> +
>> + if ((fd = mkostemp(secretPath, O_CLOEXEC)) < 0) {
>> + virReportSystemError(errno, "%s",
>> + _("failed to open luks secret file for
>> write"));
>> + goto error;
>> + }
>> +
>> + if (virSecretGetSecretString(conn, &enc->secrets[0]->seclookupdef,
>> + VIR_SECRET_USAGE_TYPE_PASSPHRASE,
>> + &secret, &secretlen) < 0)
>> + goto error;
>> +
>> + if (safewrite(fd, secret, secretlen) < 0) {
>> + virReportSystemError(errno, "%s",
>> + _("failed to write luks secret file"));
>> + goto error;
>> + }
>> + VIR_FORCE_CLOSE(fd);
>> +
>> + if ((vol->target.perms->uid != (uid_t) -1) &&
>> + (vol->target.perms->gid != (gid_t) -1)) {
>> + if (chown(secretPath, vol->target.perms->uid,
>> + vol->target.perms->gid) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("failed to chown luks secret file"));
>> + goto error;
>> + }
>> + }
>> +
>> + cleanup:
>> + VIR_DISPOSE_N(secret, secretlen);
>
> Using VIR_DISPOSE seems superstitious, considering that:
> we don't use it even once in the secret code and the secrets are in
> memory all the time.
>
Sure, but it's a "consistency" thing related to other uses where once
the stack variable for secret is saved away, we clear out the whatever
we had locally.
>> + VIR_FORCE_CLOSE(fd);
>> +
>> + return secretPath;
>> +
>> + error:
>> + unlink(secretPath);
>> + VIR_FREE(secretPath);
>> + goto cleanup;
>> +}
>> +
>> +
>> int
>> virStorageBackendCreateQemuImg(virConnectPtr conn,
>> virStoragePoolObjPtr pool,
>> @@ -1251,6 +1449,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
>> char *create_tool;
>> int imgformat;
>> virCommandPtr cmd;
>> + char *secretPath = NULL;
>>
>> virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, -1);
>>
>> @@ -1266,8 +1465,21 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
>> if (imgformat < 0)
>> goto cleanup;
>>
>> + if (vol->target.format == VIR_STORAGE_FILE_LUKS) {
>> + if (imgformat < QEMU_IMG_FORMAT_LUKS) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("this version of '%s' does not support
>> luks"),
>> + create_tool);
>> + goto cleanup;
>> + }
>> + if (!(secretPath =
>> + virStorageBackendCreateQemuImgSecretPath(conn, pool,
>> vol)))
>> + goto cleanup;
>> + }
>> +
>> cmd = virStorageBackendCreateQemuImgCmdFromVol(conn, pool, vol,
>> inputvol,
>> - flags,
>> create_tool, imgformat);
>> + flags, create_tool,
>> + imgformat,
>> secretPath);
>> if (!cmd)
>> goto cleanup;
>>
>> @@ -1275,6 +1487,10 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
>>
>> virCommandFree(cmd);
>> cleanup:
>> + if (secretPath) {
>> + unlink(secretPath);
>> + VIR_FREE(secretPath);
>> + }
>> VIR_FREE(create_tool);
>> return ret;
>> }
>> diff --git a/src/storage/storage_backend.h
>> b/src/storage/storage_backend.h
>> index 5bc622c..28e1a65 100644
>> --- a/src/storage/storage_backend.h
>> +++ b/src/storage/storage_backend.h
>> @@ -241,7 +241,8 @@
>> virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
>> virStorageVolDefPtr inputvol,
>> unsigned int flags,
>> const char *create_tool,
>> - int imgformat);
>> + int imgformat,
>> + const char *secretPath);
>>
>> /* ------- virStorageFile backends ------------ */
>> typedef struct _virStorageFileBackend virStorageFileBackend;
>> diff --git a/src/util/virqemu.c b/src/util/virqemu.c
>> index 895168e..4e4a6dd 100644
>> --- a/src/util/virqemu.c
>> +++ b/src/util/virqemu.c
>> @@ -140,3 +140,26 @@ virQEMUBuildObjectCommandlineFromJSON(const char
>> *type,
>> virBufferFreeAndReset(&buf);
>> return ret;
>> }
>> +
>> +
>> +void
>> +virQEMUBuildLuksOpts(virBufferPtr buf,
>> + virStorageEncryptionInfoDefPtr enc,
>> + const char *alias)
>> +{
>> + virBufferAsprintf(buf, "key-secret=%s,", alias);
>> +
>> + /* If there's any cipher, then add that to the command line */
>> + if (enc->cipher_name) {
>> + virBufferAsprintf(buf, "cipher-alg=%s-%u,",
>> + enc->cipher_name, enc->cipher_size);
>> + if (enc->cipher_mode)
>> + virBufferAsprintf(buf, "cipher-mode=%s,", enc->cipher_mode);
>> + if (enc->cipher_hash)
>> + virBufferAsprintf(buf, "hash-alg=%s,", enc->cipher_hash);
>> + if (enc->ivgen_name)
>> + virBufferAsprintf(buf, "ivgen-alg=%s,", enc->ivgen_name);
>> + if (enc->ivgen_hash)
>> + virBufferAsprintf(buf, "ivgen-hash-alg=%s,",
>> enc->ivgen_hash);
>
> These strings should be escaped or otherwise validated.
Escaped
>
> ACK with the possible segfault fixed and
> virStorageBackendQemuImgSupportsLuks removed.
>
Thanks -
John
More information about the libvir-list
mailing list