On 06/21/2016 09:53 AM, Peter Krempa wrote:
> On Mon, Jun 13, 2016 at 20:27:56 -0400, John Ferlan wrote:
>> 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 pool target path
>> 1a. the file name will either be $volname_$UUID.tmp or $volname_$USAGE.tmp
>> depending on how the encryption xml specified fetching the secret
>> 1b. create/open 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
>> 1d. change file protections to 0400
>>
>> 2. create a secret object
>> 2a. use a similarly crafted alias to the file name
>> 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 | 285 ++++++++++++++++++++++++++++++++++++++---
>> src/storage/storage_backend.h | 3 +-
>> src/util/virqemu.c | 23 ++++
>> src/util/virqemu.h | 6 +
>> tests/storagevolxml2argvtest.c | 3 +-
>> 6 files changed, 300 insertions(+), 21 deletions(-)
>>
[...]
>> +
>> 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;
>> + }
>
> This looks like it's becoming a source of technical debt. Heaps of
> comments aren't helping.
>
It's on the short list of things to deal with.
>>
>> return ret;
>> }
>> @@ -941,21 +972,31 @@ struct _virStorageBackendQemuImgInfo {
>> const char *inputPath;
>> const char *inputFormatStr;
>> int inputFormat;
>> +
>> + char *secretAlias;
>> + const char *secretPath;
>> };
>>
[...]
>>
>> +/* Create a hopefully unique enough name to be used for both the
>> + * secretPath and secretAlias generation
>
> This won't fly. There's only one way to guarantee that there won't be a
> collision. You need to create a file in a private path.
>
>> + */
>> +static char *
>> +virStorageBackendCreateQemuImgLuksName(const char *volname,
>> + virStorageEncryptionPtr enc)
>> +{
>> + char *ret;
>> +
>> + if (enc->secrets[0]->secdef.type == VIR_SECRET_LOOKUP_TYPE_UUID)
>> + ignore_value(virAsprintf(&ret, "%s_%s", volname,
>> + enc->secrets[0]->secdef.u.uuid));
>> + else
>> + ignore_value(virAsprintf(&ret, "%s_%s", volname,
>> + enc->secrets[0]->secdef.u.usage));
>
> usage is user provided text. Also your example in previous patch uses
> slashes in it. I don't think it will end up well in this case.
>
Ugh... right. I'm almost tempted to avoid a file type of secret and
make it be an AES type secret.
This goes away anyway.
>> + return ret;
>> +}
>> +
[...]
>> +static char *
>> +virStorageBackendCreateQemuImgSecretPath(virConnectPtr conn,
>> + virStoragePoolObjPtr pool,
>> + virStorageVolDefPtr vol)
>> +{
>> + virStorageEncryptionPtr enc = vol->target.encryption;
>> + char *str = NULL;
>> + 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;
>> + }
>> +
>> + /* Create a temporary secret file in the pool using */
>> + if (!(str = virStorageBackendCreateQemuImgLuksName(vol->name, enc)))
>> + return NULL;
>> +
>> + /* Since we don't have a file, just go to cleanup using NULL secretPath */
>> + if (!(secretPath = virFileBuildPath(pool->def->target.path, str, ".tmp")))
>
> Apart from creating colliding paths and making possibly volumes appear
> after a refresh this isn't a good idea also due to the fact that
> creating the file with the secret may not be possible on NETFS pools due
> to root squashing.
>
Using an update I have in my local repository, this does work with the
"correct" XML (gotta have the <permissions> set properly).
>> + goto cleanup;
>> +
>> + if ((fd = open(secretPath, O_WRONLY|O_TRUNC|O_CREAT, 0600)) < 0) {
>> + virReportSystemError(errno, "%s",
>> + _("failed to open luks secret file for write"));
>> + goto error;
>> + }
>> +
>> + if (virSecretGetSecretString(conn, &enc->secrets[0]->secdef,
>> + VIR_SECRET_USAGE_TYPE_KEY,
>> + &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 (chown(secretPath, geteuid(), getegid()) < 0) {
>
> You also need to take into account the uid and gid the qemu-img process
> will be using rather than the effective values.
>
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("failed to chown luks secret file"));
>> + goto error;
>> + }
>> +
>> + if (chmod(secretPath, 0400) < 0) {
>
> You've already created this with mode 600. Is 400 really necessary? If
> the process manages to destroy the secret, do you care?
>
Not necessarily - the whole generate a file and set protections is a
reaction to a comment Dan made in an earlier review:
http://www.redhat.com/archives/libvir-list/2016-June/msg00021.html
Would mkostemp() be a "reasonable" option here since we're just going to
delete the file anyway? I'd have to add something to storage_driver to
return a "path" that included "driver->stateDir", then use mkostemp in
order to create a unique name...
The alias name thus just becomes the vol->name+"_luks0".
Once I get some feedback regarding patch 5, 8, & 11 I can create a
shorter v2 with the current set of changes.
Thanks for taking the plunge...
John
>
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("failed to chown luks secret file"));
>> + goto error;
>> + }
>> +
>> + cleanup:
>> + VIR_FREE(str);
>> + VIR_DISPOSE_N(secret, secretlen);
>> + VIR_FORCE_CLOSE(fd);
>> +
>> + return secretPath;
>> +
>> + error:
>> + unlink(secretPath);
>> + VIR_FREE(secretPath);
>> + goto cleanup;
>> +}
>> +
>> +
>> int
>> virStorageBackendCreateQemuImg(virConnectPtr conn,
>> virStoragePoolObjPtr pool,
>
> [...]
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list