Re: [Qemu-block] [Qemu-devel] [PATCH 04/13] qapi: Formalize qcow2 encryption probing

2018-05-11 Thread Max Reitz 
On 2018-05-10 09:58, Daniel P. Berrangé wrote:
> On Wed, May 09, 2018 at 06:55:21PM +0200, Max Reitz wrote:
>> Currently, you can give no encryption format for a qcow2 file while
>> still passing a key-secret.  That does not conform to the schema, so
>> this patch changes the schema to allow it.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  qapi/block-core.json | 44 
>>  1 file changed, 40 insertions(+), 4 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 71c9ab8538..092a1aba2d 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -43,6 +43,19 @@
>>  { 'struct': 'ImageInfoSpecificQCow2EncryptionBase',
>>'data': { 'format': 'BlockdevQcow2EncryptionFormat'}}
>>  
>> +##
>> +# @ImageInfoSpecificQCow2EncryptionNoInfo:
>> +#
>> +# Only used for the qcow2 encryption format "from-image" in which the
>> +# actual encryption format is determined from the image header.
>> +# Therefore, this encryption format will never be reported in
>> +# ImageInfoSpecificQCow2Encryption.
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'struct': 'ImageInfoSpecificQCow2EncryptionNoInfo',
>> +  'data': { } }
>> +
>>  ##
>>  # @ImageInfoSpecificQCow2Encryption:
>>  #
>> @@ -52,7 +65,8 @@
>>'base': 'ImageInfoSpecificQCow2EncryptionBase',
>>'discriminator': 'format',
>>'data': { 'aes': 'QCryptoBlockInfoQCow',
>> -'luks': 'QCryptoBlockInfoLUKS' } }
>> +'luks': 'QCryptoBlockInfoLUKS',
>> +'from-image': 'ImageInfoSpecificQCow2EncryptionNoInfo' } }
>>  
>>  ##
>>  # @ImageInfoSpecificQCow2:
>> @@ -2739,10 +2753,30 @@
>>  # @BlockdevQcow2EncryptionFormat:
>>  # @aes: AES-CBC with plain64 initialization venctors
>>  #
>> +# @from-image:  Determine the encryption format from the image
>> +#   header.  This only allows the use of the
>> +#   key-secret option.  (Since: 2.13)
>> +#
>>  # Since: 2.10
>>  ##
>>  { 'enum': 'BlockdevQcow2EncryptionFormat',
>> -  'data': [ 'aes', 'luks' ] }
>> +  'data': [ 'aes', 'luks', 'from-image' ] }
>> +
>> +##
>> +# @BlockdevQcow2EncryptionSecret:
>> +#
>> +# Allows specifying a key-secret without specifying the exact
>> +# encryption format, which is determined automatically from the image
>> +# header.
>> +#
>> +# @key-secret:  The ID of a QCryptoSecret object providing the
>> +#   decryption key.  Mandatory except when probing
>> +#   image for metadata only.
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'struct': 'BlockdevQcow2EncryptionSecret',
>> +  'data': { '*key-secret': 'str' } }
>>  
>>  ##
>>  # @BlockdevQcow2Encryption:
>> @@ -2750,10 +2784,12 @@
>>  # Since: 2.10
>>  ##
>>  { 'union': 'BlockdevQcow2Encryption',
>> -  'base': { 'format': 'BlockdevQcow2EncryptionFormat' },
>> +  'base': { '*format': 'BlockdevQcow2EncryptionFormat' },
>>'discriminator': 'format',
>> +  'default-variant': 'from-image',
>>'data': { 'aes': 'QCryptoBlockOptionsQCow',
>> -'luks': 'QCryptoBlockOptionsLUKS'} }
>> +'luks': 'QCryptoBlockOptionsLUKS',
>> +'from-image': 'BlockdevQcow2EncryptionSecret' } }
> 
> Bike-shedding on name, how about "auto" or "probe" ?

Sure.  I like "probe" a bit better than "auto", although "auto" is what
we usually have...  But I think "probe" is still a bit better.

> 
> IIUC, this schema addition means the QAPI parser now allows
> 
>encrypt.format=from-image,encrypt.key-secret=sec0,...other opts...
> 
> but the code will not accept "from-image" as a valid string.

Ah, right, I forgot that.  Will fix.

Thanks for reviewing!

Max

> eg qcow2_update_options_prepare() will do
> 
> case QCOW_CRYPT_AES:
> if (encryptfmt && !g_str_equal(encryptfmt, "aes")) {
> error_setg(errp,
>"Header reported 'aes' encryption format but "
>"options specify '%s'", encryptfmt);
> ret = -EINVAL;
> goto fail;
> }
> 
>...snip
> 
> case QCOW_CRYPT_LUKS:
> if (encryptfmt && !g_str_equal(encryptfmt, "luks")) {
> error_setg(errp,
>"Header reported 'luks' encryption format but "
>"options specify '%s'", encryptfmt);
> ret = -EINVAL;
> goto fail;
> }
> 
> 
> Regards,
> Daniel
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 04/13] qapi: Formalize qcow2 encryption probing

2018-05-10 Thread Eric Blake 

On 05/10/2018 02:58 AM, Daniel P. Berrangé wrote:

On Wed, May 09, 2018 at 06:55:21PM +0200, Max Reitz wrote:

Currently, you can give no encryption format for a qcow2 file while
still passing a key-secret.  That does not conform to the schema, so
this patch changes the schema to allow it.

Signed-off-by: Max Reitz 
---
  qapi/block-core.json | 44 
  1 file changed, 40 insertions(+), 4 deletions(-)



  { 'union': 'BlockdevQcow2Encryption',
-  'base': { 'format': 'BlockdevQcow2EncryptionFormat' },
+  'base': { '*format': 'BlockdevQcow2EncryptionFormat' },
'discriminator': 'format',
+  'default-variant': 'from-image',
'data': { 'aes': 'QCryptoBlockOptionsQCow',
-'luks': 'QCryptoBlockOptionsLUKS'} }
+'luks': 'QCryptoBlockOptionsLUKS',
+'from-image': 'BlockdevQcow2EncryptionSecret' } }


Bike-shedding on name, how about "auto" or "probe" ?


Either of those sounds nicer to me; 'auto' might be better in the 
context of creation (that way, we can state that creating a NEW image 
with x-blockdev-create maps 'auto' to 'luks'; while connecting to an 
EXISTING image maps 'auto' to either 'aes' or 'luks' as appropriate).




IIUC, this schema addition means the QAPI parser now allows

encrypt.format=from-image,encrypt.key-secret=sec0,...other opts...


Yes.  You could, perhaps, add a special case on the command line parsing 
code to reject an explicit use of format=from-image, but the QMP should 
not reject an explicit discriminator.


Hmm, it plays in with my comment on 1/13 - should the QMP parser 
automatically set has_discriminator to true when it supplies the 
default?  If it does, you lose the ability to see whether the user 
supplied an explicit encrypt.format=from-image (or the equivalent when 
using QMP instead of the command line), if you wanted to enforce that 
the user MUST omit format when relying on the from-image variant.


I don't see a problem in allowing the user to explicitly specify the 
name of the default branch, but I _do_ think the patch is incomplete for 
not handling the new QCOW_CRYPT_FROM_IMAGE case and converting it as 
soon as possible back into one of the other two preferred enum values.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [Qemu-devel] [PATCH 04/13] qapi: Formalize qcow2 encryption probing

2018-05-10 Thread Daniel P . Berrangé 
On Wed, May 09, 2018 at 06:55:21PM +0200, Max Reitz wrote:
> Currently, you can give no encryption format for a qcow2 file while
> still passing a key-secret.  That does not conform to the schema, so
> this patch changes the schema to allow it.
> 
> Signed-off-by: Max Reitz 
> ---
>  qapi/block-core.json | 44 
>  1 file changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 71c9ab8538..092a1aba2d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -43,6 +43,19 @@
>  { 'struct': 'ImageInfoSpecificQCow2EncryptionBase',
>'data': { 'format': 'BlockdevQcow2EncryptionFormat'}}
>  
> +##
> +# @ImageInfoSpecificQCow2EncryptionNoInfo:
> +#
> +# Only used for the qcow2 encryption format "from-image" in which the
> +# actual encryption format is determined from the image header.
> +# Therefore, this encryption format will never be reported in
> +# ImageInfoSpecificQCow2Encryption.
> +#
> +# Since: 2.13
> +##
> +{ 'struct': 'ImageInfoSpecificQCow2EncryptionNoInfo',
> +  'data': { } }
> +
>  ##
>  # @ImageInfoSpecificQCow2Encryption:
>  #
> @@ -52,7 +65,8 @@
>'base': 'ImageInfoSpecificQCow2EncryptionBase',
>'discriminator': 'format',
>'data': { 'aes': 'QCryptoBlockInfoQCow',
> -'luks': 'QCryptoBlockInfoLUKS' } }
> +'luks': 'QCryptoBlockInfoLUKS',
> +'from-image': 'ImageInfoSpecificQCow2EncryptionNoInfo' } }
>  
>  ##
>  # @ImageInfoSpecificQCow2:
> @@ -2739,10 +2753,30 @@
>  # @BlockdevQcow2EncryptionFormat:
>  # @aes: AES-CBC with plain64 initialization venctors
>  #
> +# @from-image:  Determine the encryption format from the image
> +#   header.  This only allows the use of the
> +#   key-secret option.  (Since: 2.13)
> +#
>  # Since: 2.10
>  ##
>  { 'enum': 'BlockdevQcow2EncryptionFormat',
> -  'data': [ 'aes', 'luks' ] }
> +  'data': [ 'aes', 'luks', 'from-image' ] }
> +
> +##
> +# @BlockdevQcow2EncryptionSecret:
> +#
> +# Allows specifying a key-secret without specifying the exact
> +# encryption format, which is determined automatically from the image
> +# header.
> +#
> +# @key-secret:  The ID of a QCryptoSecret object providing the
> +#   decryption key.  Mandatory except when probing
> +#   image for metadata only.
> +#
> +# Since: 2.13
> +##
> +{ 'struct': 'BlockdevQcow2EncryptionSecret',
> +  'data': { '*key-secret': 'str' } }
>  
>  ##
>  # @BlockdevQcow2Encryption:
> @@ -2750,10 +2784,12 @@
>  # Since: 2.10
>  ##
>  { 'union': 'BlockdevQcow2Encryption',
> -  'base': { 'format': 'BlockdevQcow2EncryptionFormat' },
> +  'base': { '*format': 'BlockdevQcow2EncryptionFormat' },
>'discriminator': 'format',
> +  'default-variant': 'from-image',
>'data': { 'aes': 'QCryptoBlockOptionsQCow',
> -'luks': 'QCryptoBlockOptionsLUKS'} }
> +'luks': 'QCryptoBlockOptionsLUKS',
> +'from-image': 'BlockdevQcow2EncryptionSecret' } }

Bike-shedding on name, how about "auto" or "probe" ?

IIUC, this schema addition means the QAPI parser now allows

   encrypt.format=from-image,encrypt.key-secret=sec0,...other opts...

but the code will not accept "from-image" as a valid string.

eg qcow2_update_options_prepare() will do

case QCOW_CRYPT_AES:
if (encryptfmt && !g_str_equal(encryptfmt, "aes")) {
error_setg(errp,
   "Header reported 'aes' encryption format but "
   "options specify '%s'", encryptfmt);
ret = -EINVAL;
goto fail;
}

   ...snip

case QCOW_CRYPT_LUKS:
if (encryptfmt && !g_str_equal(encryptfmt, "luks")) {
error_setg(errp,
   "Header reported 'luks' encryption format but "
   "options specify '%s'", encryptfmt);
ret = -EINVAL;
goto fail;
}


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|