Re: [PATCH 17/19] storage: Add support to create a luks volume

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]