Re: [libvirt] [PATCH 19/19] qemu: Add luks support for domain disk

[John Ferlan]

On 06/21/2016 09:03 AM, Peter Krempa wrote:
> On Mon, Jun 13, 2016 at 20:27:58 -0400, John Ferlan wrote:
>> Generate the luks command line using the AES secret key to encrypt the
>> luks secret. A luks secret object will be in addition to a an AES secret.
>>
>> Add tests for sample output
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_command.c| 12 ++--
>>  src/qemu/qemu_domain.c | 19 ++--
>>  .../qemuxml2argv-luks-disk-cipher.args | 36 
>> ++
>>  .../qemuxml2argvdata/qemuxml2argv-luks-disks.args  | 36 
>> ++
>>  tests/qemuxml2argvtest.c   | 11 ++-
>>  5 files changed, 109 insertions(+), 5 deletions(-)
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.args
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args
> 
> Note that this won't work with disk hotplug until you add code that will
> add the secret objects too.

Oh right - I had forgotten about the secret object... I had the same
issue with the TLS code, so I know what has to be done...

> 
> Also I've noticed that RBD hotplug is now broken too due to the same
> reason. The code that selects whether to use plaintext password or pass
> it via the secret object does not check whether it's called on the
> hotplug path and thus uses the secret object.

Oh damn...  I think that's a disconnect in qemuDomainSecretSetup (called
from qemuDomainSecretDiskPrepare from hotplug) and the assumption of an
AES secinfo for RBD when there's a secret object available.

> 
> The secret object is not added using the monitor though. You'll need to
> fix that first and if you opt for the correct approach this will then
> work automagically.
> 
> Looks okay but I can't ACK this with hotplug not working.
> 
> Also please note that on hot-unplug you need to remove the secrets as
> you'll possibly be adding a secret with the same name (Since alias will
> be reused)

yep...

> 
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 490260f..7062c17 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1103,6 +1103,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>>  int actualType = virStorageSourceGetActualType(disk->src);
>>  qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>>  qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
>> +qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo;
>>  bool emitDeviceSyntax = qemuDiskBusNeedsDeviceArg(disk->bus);
>>  
>>  if (idx < 0) {
>> @@ -1237,10 +1238,13 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>>  qemuBufferEscapeComma(, source);
>>  virBufferAddLit(, ",");
>>  
>> -if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
>> +if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES)
>>  virBufferAsprintf(, "password-secret=%s,",
>>secinfo->s.aes.alias);
>> -}
>> +
>> +if (encinfo)
>> +virQEMUBuildLuksOpts(, disk->src->encryption,
>> + encinfo->s.aes.alias);
> 
> This wrapper is not really useful here. It only adds "key-secret=" all
> the other options are necessary only if creating the volume.
> 

I was thinking of the future if/when new things are added.  I'm not
clear yet what would have to be done for block-copy and snapshots.

>>  
>>  if (disk->src->format > 0 &&
>>  disk->src->type != VIR_STORAGE_TYPE_DIR)
>> @@ -1920,6 +1924,7 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>>  virDomainDiskDefPtr disk = def->disks[i];
>>  qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>>  qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
>> +qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo;
>>  
>>  /* PowerPC pseries based VMs do not support floppy device */
>>  if ((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) &&
>> @@ -1949,6 +1954,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>>  if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0)
>>  return -1;
>>  
>> +if (qemuBuildDiskSecinfoCommandLine(cmd, encinfo) < 0)
>> +return -1;
>> +
>>  virCommandAddArg(cmd, "-drive");
>>  
>>  optstr = qemuBuildDriveStr(disk,
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index c288fa0..fb3e91f 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
> 
> [...]
> 
>> @@ -1026,7 +1028,6 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
>>   src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) {
>>  
>>  virSecretUsageType secretUsageType;
>> -qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>>  
>>  if (VIR_ALLOC(secinfo) < 0)
>>  

Re: [libvirt] [PATCH 19/19] qemu: Add luks support for domain disk

[Peter Krempa]

On Tue, Jun 21, 2016 at 15:03:51 +0200, Peter Krempa wrote:
> On Mon, Jun 13, 2016 at 20:27:58 -0400, John Ferlan wrote:
> > Generate the luks command line using the AES secret key to encrypt the
> > luks secret. A luks secret object will be in addition to a an AES secret.
> > 
> > Add tests for sample output
> > 
> > Signed-off-by: John Ferlan 
> > ---

[...]

> > @@ -1237,10 +1238,13 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
> >  qemuBufferEscapeComma(, source);
> >  virBufferAddLit(, ",");
> >  
> > -if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
> > +if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES)
> >  virBufferAsprintf(, "password-secret=%s,",
> >secinfo->s.aes.alias);
> > -}
> > +
> > +if (encinfo)
> > +virQEMUBuildLuksOpts(, disk->src->encryption,
> > + encinfo->s.aes.alias);
> 
> This wrapper is not really useful here. It only adds "key-secret=" all
> the other options are necessary only if creating the volume.

Okay, in the end this might be a reasonable idea if we'll want to add
support for block-copy-ing into a luks volume.

On the other hand, you'll need to disallow snapshots if the disk is
LUKS until we add support for full backing chain tracking since you'll
lose the definitions for the key once you take a snapshot. A second
start of that VM will not be possible then.

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

Re: [libvirt] [PATCH 19/19] qemu: Add luks support for domain disk

[Peter Krempa]

On Mon, Jun 13, 2016 at 20:27:58 -0400, John Ferlan wrote:
> Generate the luks command line using the AES secret key to encrypt the
> luks secret. A luks secret object will be in addition to a an AES secret.
> 
> Add tests for sample output
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_command.c| 12 ++--
>  src/qemu/qemu_domain.c | 19 ++--
>  .../qemuxml2argv-luks-disk-cipher.args | 36 
> ++
>  .../qemuxml2argvdata/qemuxml2argv-luks-disks.args  | 36 
> ++
>  tests/qemuxml2argvtest.c   | 11 ++-
>  5 files changed, 109 insertions(+), 5 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disk-cipher.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks.args

Note that this won't work with disk hotplug until you add code that will
add the secret objects too.

Also I've noticed that RBD hotplug is now broken too due to the same
reason. The code that selects whether to use plaintext password or pass
it via the secret object does not check whether it's called on the
hotplug path and thus uses the secret object.

The secret object is not added using the monitor though. You'll need to
fix that first and if you opt for the correct approach this will then
work automagically.

Looks okay but I can't ACK this with hotplug not working.

Also please note that on hot-unplug you need to remove the secrets as
you'll possibly be adding a secret with the same name (Since alias will
be reused)

> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 490260f..7062c17 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1103,6 +1103,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>  int actualType = virStorageSourceGetActualType(disk->src);
>  qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>  qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
> +qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo;
>  bool emitDeviceSyntax = qemuDiskBusNeedsDeviceArg(disk->bus);
>  
>  if (idx < 0) {
> @@ -1237,10 +1238,13 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>  qemuBufferEscapeComma(, source);
>  virBufferAddLit(, ",");
>  
> -if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
> +if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES)
>  virBufferAsprintf(, "password-secret=%s,",
>secinfo->s.aes.alias);
> -}
> +
> +if (encinfo)
> +virQEMUBuildLuksOpts(, disk->src->encryption,
> + encinfo->s.aes.alias);

This wrapper is not really useful here. It only adds "key-secret=" all
the other options are necessary only if creating the volume.

>  
>  if (disk->src->format > 0 &&
>  disk->src->type != VIR_STORAGE_TYPE_DIR)
> @@ -1920,6 +1924,7 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>  virDomainDiskDefPtr disk = def->disks[i];
>  qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>  qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
> +qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo;
>  
>  /* PowerPC pseries based VMs do not support floppy device */
>  if ((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) &&
> @@ -1949,6 +1954,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>  if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0)
>  return -1;
>  
> +if (qemuBuildDiskSecinfoCommandLine(cmd, encinfo) < 0)
> +return -1;
> +
>  virCommandAddArg(cmd, "-drive");
>  
>  optstr = qemuBuildDriveStr(disk,
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index c288fa0..fb3e91f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c

[...]

> @@ -1026,7 +1028,6 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
>   src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) {
>  
>  virSecretUsageType secretUsageType;
> -qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>  
>  if (VIR_ALLOC(secinfo) < 0)
>  return -1;
> @@ -1044,6 +1045,20 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
>  diskPriv->secinfo = secinfo;
>  }
>  
> +if (conn && !virStorageSourceIsEmpty(src) &&

This part is the same with the block above. It may be worth extracting
it ...

> +src->encryption && src->format == VIR_STORAGE_FILE_LUKS) {
> +
> +if (VIR_ALLOC(secinfo) < 0)
> +return -1;
> +
> +if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias,
> +  VIR_SECRET_USAGE_TYPE_KEY, NULL,
> +  >encryption->secrets[0]->secdef) < 0)
> +goto