Re: [libvirt] [PATCH v2 11/15] storage: Add support to create a luks volume

[John Ferlan]

On 06/24/2016 10:48 AM, Peter Krempa wrote:
> On Thu, Jun 23, 2016 at 13:29:07 -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 XX 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
>>1d. change file protections to 0400
>>
>>  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 
>> ---
>>  src/libvirt_private.syms   |   1 +
>>  src/storage/storage_backend.c  | 260 
>> ++---
>>  src/storage/storage_backend.h  |   3 +-
>>  src/util/virqemu.c |  23 
>>  src/util/virqemu.h |   6 +
>>  tests/storagevolxml2argvtest.c |   3 +-
>>  6 files changed, 275 insertions(+), 21 deletions(-)
>>
> 
> [...]
> 
>> @@ -1240,6 +1366,84 @@ 
>> 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 
>> */
>> +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, >secrets[0]->seclookupdef,
>> + VIR_SECRET_USAGE_TYPE_PASSPHRASE,
>> + , ) < 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,
> 
> [1]
> 
>> +  vol->target.perms->gid) < 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +   _("failed to chown luks secret file"));
>> +goto error;
>> +}
>> +}
>> +
>> +if (chmod(secretPath, 0400) < 0) {
> 
> I still don't understand this step. Owner [1] of the file is able to
> add the write permission back anyways.
> 
>> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +   _("failed to chown luks secret file"));
>> +goto error;
>> +}
>> +
>> + cleanup:
>> +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,
> 
> ACK with either a very good explanation or the chmod gone.
> 

chmod is gone.

Again, the 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


John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 11/15] storage: Add support to create a luks volume

[Peter Krempa]

On Thu, Jun 23, 2016 at 13:29:07 -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 XX 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
>1d. change file protections to 0400
> 
>  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 
> ---
>  src/libvirt_private.syms   |   1 +
>  src/storage/storage_backend.c  | 260 
> ++---
>  src/storage/storage_backend.h  |   3 +-
>  src/util/virqemu.c |  23 
>  src/util/virqemu.h |   6 +
>  tests/storagevolxml2argvtest.c |   3 +-
>  6 files changed, 275 insertions(+), 21 deletions(-)
> 

[...]

> @@ -1240,6 +1366,84 @@ 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 
> */
> +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, >secrets[0]->seclookupdef,
> + VIR_SECRET_USAGE_TYPE_PASSPHRASE,
> + , ) < 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,

[1]

> +  vol->target.perms->gid) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("failed to chown luks secret file"));
> +goto error;
> +}
> +}
> +
> +if (chmod(secretPath, 0400) < 0) {

I still don't understand this step. Owner [1] of the file is able to
add the write permission back anyways.

> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("failed to chown luks secret file"));
> +goto error;
> +}
> +
> + cleanup:
> +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,

ACK with either a very good explanation or the chmod gone.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 11/15] storage: Add support to create a luks volume

[John Ferlan]

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 XX 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
   1d. change file protections to 0400

 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 
---
 src/libvirt_private.syms   |   1 +
 src/storage/storage_backend.c  | 260 ++---
 src/storage/storage_backend.h  |   3 +-
 src/util/virqemu.c |  23 
 src/util/virqemu.h |   6 +
 tests/storagevolxml2argvtest.c |   3 +-
 6 files changed, 275 insertions(+), 21 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b118d1e..9160b22 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2163,6 +2163,7 @@ virProcessWait;
 
 
 # util/virqemu.h
+virQEMUBuildLuksOpts;
 virQEMUBuildObjectCommandlineFromJSON;
 
 
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 89d5962..8918f3e 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, ) < 0)
+goto cleanup;
+
+if (exitstatus == 0)
+ret = true;
+
+ cleanup:
+virCommandFree(cmd);
+return ret;
+}
+
+
 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;
 }
@@ -941,21 +972,31 @@ struct _virStorageBackendQemuImgInfo {
 const char *inputPath;
 const char *inputFormatStr;
 int inputFormat;
+
+char *secretAlias;
+const char *secretPath;
 };
 
+
 static int
-virStorageBackendCreateQemuImgOpts(char **opts,
+virStorageBackendCreateQemuImgOpts(virStorageEncryptionPtr enc,
+   char **opts,
struct _virStorageBackendQemuImgInfo info)
 {
 virBuffer buf = VIR_BUFFER_INITIALIZER;
 
-if (info.backingPath)
-virBufferAsprintf(, "backing_fmt=%s,",
-  
virStorageFileFormatTypeToString(info.backingFormat));
-if (info.encryption)
-virBufferAddLit(, "encryption=on,");
-if (info.preallocate)
-virBufferAddLit(, "preallocation=metadata,");
+if (info.format ==