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