Re: [Qemu-devel] [PATCH v4 19/26] block: add generic full disk encryption driver

[Eric Blake]

On 02/29/2016 05:00 AM, Daniel P. Berrange wrote:
> Add a block driver that is capable of supporting any full disk
> encryption format. This utilizes the previously added block
> encryption code, and at this time supports the LUKS format.
> 
> The driver code is capable of supporting any format supported
> by the QCryptoBlock module, so it registers one block driver
> for each format.
> 
> At this time, the "luks" driver is registered. New LUKS
> compatible volume can be formatted using qemu-img with
> defaults for all settings.

Sounds a bit redundant between the 1st and 3rd paragraph; but I'm not
sure if I have any suggestions on shortening it for only a single
mention that LUKS is the first (and so far only) supported driver.

> 
> $ qemu-img create --object secret,data=123456,id=sec0 \
>       -f luks -o key-secret=sec0 demo.luks 10G
> 
> Alternatively the cryptographic settings can explicitly
> set
> 
> $ qemu-img create --object secret,data=123456,id=sec0 \
>       -f luks -o key-secret=sec0,cipher-alg=aes-256,\
>                  cipher-mode=cbc,ivgen-alg=plain64,hash-alg=sha256 \
>       demo.luks 10G
> 
> And query its size
> 
> $ qemu-img info demo.img
> image: demo.img
> file format: luks
> virtual size: 10G (10737418240 bytes)
> disk size: 132K
> encrypted: yes
> 
> Note that it was not neccessary to provide the password

s/neccessary/necessary/

> when querying info for the volume. The password is only
> required when performing I/O on the volume
> 
> All volumes created by this new 'luks' driver should be
> capable of being opened by the kernel dm-crypt driver.
> 
> The only algorithms listed in the LUKS spec that are
> not currently supported by this impl are sha512 and
> ripemd160 hashes and cast6 cipher.
> 
> A new I/O test is added which is used to validate the
> interoperability of the QEMU implementation of LUKS,
> with the dm-crypt/cryptsetup implementation. Due to
> the need to run cryptsetup as root, this test requires
> that the user have password-less sudo configured.

and therefore, the test better not be run by default, but by an explicit
target.  Worth spelling out what that target is? [1]

> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
> +
> +static QCryptoBlockOpenOptions *
> +block_crypto_open_opts_init(QCryptoBlockFormat format,
> +                            QemuOpts *opts,
> +                            Error **errp)
> +{
> +    OptsVisitor *ov;
> +    QCryptoBlockOpenOptions *ret;
> +    QCryptoBlockOptionsLUKS *tmp;
> +    Error *local_err = NULL;
> +
> +    ret = g_new0(QCryptoBlockOpenOptions, 1);
> +    ret->format = format;
> +
> +    ov = opts_visitor_new(opts);
> +
> +    switch (format) {
> +    case Q_CRYPTO_BLOCK_FORMAT_LUKS:
> +        visit_type_QCryptoBlockOptionsLUKS(opts_get_visitor(ov), "luks",
> +                                           &tmp, &local_err);
> +        if (!local_err) {
> +            memcpy(&ret->u.luks, tmp, sizeof(*tmp));
> +            g_free(tmp);
> +        }

Commit 4d91e911 has landed; you should be using
visit_type_QCryptoBlockOptionsLUKS_members() here rather than tmp and
memcpy().


> +        break;
> +
> +    default:
> +        error_setg(&local_err, "Unsupported block format %d", format);
> +        break;
> +    }
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        opts_visitor_cleanup(ov);
> +        qapi_free_QCryptoBlockOpenOptions(ret);
> +        return NULL;
> +    }
> +
> +    opts_visitor_cleanup(ov);
> +    return ret;
> +}
> +
> +
> +static QCryptoBlockCreateOptions *
> +block_crypto_create_opts_init(QCryptoBlockFormat format,
> +                              QemuOpts *opts,
> +                              Error **errp)
> +{
> +    OptsVisitor *ov;
> +    QCryptoBlockCreateOptions *ret;
> +    QCryptoBlockCreateOptionsLUKS *tmp;
> +    Error *local_err = NULL;
> +
> +    ret = g_new0(QCryptoBlockCreateOptions, 1);
> +    ret->format = format;
> +
> +    ov = opts_visitor_new(opts);
> +
> +    switch (format) {
> +    case Q_CRYPTO_BLOCK_FORMAT_LUKS:
> +        visit_type_QCryptoBlockCreateOptionsLUKS(opts_get_visitor(ov), 
> "luks",
> +                                                 &tmp, &local_err);
> +        if (!local_err) {
> +            memcpy(&ret->u.luks, tmp, sizeof(*tmp));
> +            g_free(tmp);

And again.


> +static int block_crypto_open_generic(QCryptoBlockFormat format,
> +                                     QemuOptsList *opts_spec,
> +                                     BlockDriverState *bs,
> +                                     QDict *options,
> +                                     int flags,
> +                                     Error **errp)
> +{

> +
> +    bs->encrypted = 1;
> +    bs->valid_key = 1;

These look like they should be bool; but then again, I think you are
getting rid of them later in the series, so it's not worth the churn.

> +++ b/qapi/block-core.json
> @@ -242,11 +242,12 @@
>  # @drv: the name of the block format used to open the backing device. As of
>  #       0.14.0 this can be: 'blkdebug', 'bochs', 'cloop', 'cow', 'dmg',
>  #       'file', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
> -#       'http', 'https', 'nbd', 'parallels', 'qcow',
> +#       'http', 'https', 'luks', 'nbd', 'parallels', 'qcow',
>  #       'qcow2', 'raw', 'tftp', 'vdi', 'vmdk', 'vpc', 'vvfat'
>  #       2.2: 'archipelago' added, 'cow' dropped
>  #       2.3: 'host_floppy' deprecated
>  #       2.5: 'host_floppy' dropped
> +#       2.6: 'luks' added

Nice - you remembered docs.

> +++ b/tests/qemu-iotests/146
> @@ -0,0 +1,521 @@

> +
> +def verify_passwordless_sudo():
> +    """Check whether sudo is configured to allow
> +       password-less access to commands"""
> +
> +    args = ["sudo", "-n", "/bin/true"]
> +
> +    proc = subprocess.Popen(args,
> +                            stdin=subprocess.PIPE,
> +                            stdout=subprocess.PIPE,
> +                            stderr=subprocess.STDOUT)
> +
> +    msg = proc.communicate()[0]
> +
> +    if proc.returncode != 0:
> +        iotests.notrun('requires password-less sudo access: %s' % msg)

[1] Okay, the check is part of qemu-iotests (not run as part of 'make
check', but independently run), and looks like this should do the job of
making it obvious whether the test was skipped due to setup reasons.

Overall looks like a nice test.  The changes above about using the more
efficient _members() visit could be done as a followup if desired, so
I'm okay if you fix the commit message typo and add:
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