Re: [Qemu-block] [PATCH v4 13/26] crypto: implement the LUKS block encryption format

[Eric Blake]

On 02/29/2016 05:00 AM, Daniel P. Berrange wrote:
> Provide a block encryption implementation that follows the
> LUKS/dm-crypt specification.
> 
> This supports all combinations of hash, cipher algorithm,
> cipher mode and iv generator that are implemented by the
> current crypto layer.
> 
> The notable missing feature is support for the 'xts'
> cipher mode, which is commonly used for disk encryption
> instead of 'cbc'. This is because it is not provided by
> either nettle or libgcrypt. A suitable implementation
> will be identified & integrated later.

Stale paragraph, you implemented it earlier in the series.

> 
> There is support for opening existing volumes formatted
> by dm-crypt, and for formatting new volumes. In the latter
> case it will only use key slot 0.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---


> +static int
> +qcrypto_block_luks_open(QCryptoBlock *block,
> +                        QCryptoBlockOpenOptions *options,
> +                        QCryptoBlockReadFunc readfunc,
> +                        void *opaque,
> +                        unsigned int flags,
> +                        Error **errp)
> +{

> +    /* Read the entire LUKS header, minus the key material from
> +     * the underling device */

s/underling/underlying/ (although the typo does read rather humorously -
I now have a mental image of a LUKS overlord :)


> +++ b/qapi/crypto.json
> @@ -117,12 +117,13 @@

>  ##
>  # QCryptoBlockOptionsBase:
> @@ -143,7 +144,8 @@
>  # The options that apply to QCow/QCow2 AES-CBC encryption format
>  #
>  # @key-secret: #optional the ID of a QCryptoSecret object providing the
> -#              decryption key
> +#              decryption key. Mandatory except when probing image for
> +#              metadata only.

Aha - I think this hunk may belong earlier in the series...

>  #
>  # Since: 2.6
>  ##
> @@ -151,6 +153,45 @@
>    'data': { '*key-secret': 'str' }}
>  
>  ##
> +# QCryptoBlockOptionsLUKS:
> +#
> +# The options that apply to LUKS encryption format
> +#
> +# @key-secret: #optional the ID of a QCryptoSecret object providing the
> +#              decryption key

...Although you may want to duplicate it here.

Looks like my review on the earlier version helped, and you addressed
most of my comments.  What I pointed out above is minor enough that I'm
okay if you fix it on the pull request without needing another round of
review, so:

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature