[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 12/17] qcow2: convert QCow2 to use QCryptoBlo
|
From: |
Eric Blake |
|
Subject: |
Re: [Qemu-devel] [PATCH v2 12/17] qcow2: convert QCow2 to use QCryptoBlock for encryption |
|
Date: |
Mon, 8 Feb 2016 11:12:37 -0700 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 |
On 01/20/2016 10:38 AM, Daniel P. Berrange wrote:
> This converts the qcow2 driver to make use of the QCryptoBlock
> APIs for encrypting image content. As well as continued support
> for the legacy QCow2 encryption format, the appealing benefit
> is that it enables support for the LUKS format inside qcow2.
I know Fam had some interesting ideas on changing qcow2 to be able to
auto-load a LUKS format driver rather than duplicating code, which may
completely change this patch, but I'll go ahead and review this patch as-is.
>
> With the LUKS format it is neccessary to store the LUKS
s/neccessary/necessary/
> partition header and key material in the QCow2 file. This
> data can be many MB in size, so cannot go into the QCow2
> header region directly. Thus the spec is defines a FDE
s/is //
> (Full Disk Encryption) header extension that specifies
> the offset of a set of clusters to hold the FDE headers,
> as well as the length of that region. The LUKS header is
> thus stored in these extra allocated clusters before the
> main image payload.
>
> With this change it is now required to use the QCryptoSecret
> object for providing passwords, instead of the current block
> password APIs / interactive prompting.
>
> $QEMU \
> -object secret,id=sec0,filename=/home/berrange/encrypted.pw \
> -drive file=/home/berrange/encrypted.qcow2,key-secret=sec0
>
> The new LUKS format is set as the new default format when
> creating encrypted images. ie
>
> # qemu-img create --object secret,data=123456,id=sec0 \
> -f qcow2 -o encryption,key-secret=sec0 \
> test.qcow2 10G
Did we add '-o encryption' earlier in the series, or is this line a bit
stale in reference to your --image-opts series?
>
> Results in creation of an image using the LUKS format.
since this is a continuation of the earlier thought, s/Results/results/
s/in/in the/
>
> For compatibility the old qcow2 AES format can still be used
> via the 'encryption-format' parameter which accepts the
> values 'luks' or 'qcowaes'.
s/qcowaes/qcow/ to match your earlier changes
>
> # qemu-img create --object secret,data=123456,id=sec0 \
> -f qcow2 -o encryption,key-secret=sec0,encryption-format=qcowaes \
> test.qcow2 10G
>
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
> block/qcow2-cluster.c | 46 +----
> block/qcow2-refcount.c | 10 +
> block/qcow2.c | 502
> ++++++++++++++++++++++++++++++++++++++-------
> block/qcow2.h | 21 +-
> crypto/block-luks.c | 1 -
> docs/specs/qcow2.txt | 74 +++++++
> qapi/block-core.json | 6 +-
As mentioned earlier in the series, a well-chosen order file
(format-patch -Ofile, or git config diff.orderFile) that hoists these
two changes first might make the overall patch easier to review.
> tests/qemu-iotests/049 | 2 +-
> tests/qemu-iotests/049.out | 7 +-
> tests/qemu-iotests/082.out | 189 +++++++++++++++++
> tests/qemu-iotests/087 | 28 ++-
> tests/qemu-iotests/087.out | 12 +-
> tests/qemu-iotests/134 | 18 +-
> tests/qemu-iotests/134.out | 21 +-
> tests/qemu-iotests/common | 1 +
> 15 files changed, 775 insertions(+), 163 deletions(-)
>
Starting my review at [1], then I'll return here [2].
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f5bc4f2..0512256 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-refcount.c
> @@ -1847,6 +1847,16 @@ static int calculate_refcounts(BlockDriverState *bs,
> BdrvCheckResult *res,
> return ret;
> }
>
> + /* encryption */
> + if (s->crypto_header.length) {
> + ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
> + s->crypto_header.offset,
> + s->crypto_header.length);
> + if (ret < 0) {
> + return ret;
> + }
> + }
> +
Do we ever allow turning off encryption, where we'd need to decrement
the refcounts as the FDE header is removed?
> return check_refblocks(bs, res, fix, rebuild, refcount_table,
> nb_clusters);
> }
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2fae692..5249ca2 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -34,6 +34,8 @@
> #include "qapi-event.h"
> #include "trace.h"
> #include "qemu/option_int.h"
> +#include "qapi/opts-visitor.h"
> +#include "qapi-visit.h"
>
> /*
> Differences with QCOW:
> @@ -60,6 +62,7 @@ typedef struct {
> #define QCOW2_EXT_MAGIC_END 0
> #define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
> #define QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> +#define QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x4c554b53
Aha: ASCII "LUKS", even though we expect the header to be useful for
more than just LUKS encryption schemes. Would ASCII "CRYP" be any
nicer? Or a random number, or the next set of hex digits from pi, or...?
I guess we don't have any real rules or patterns describing how magic
numbers are supposed to be chosen, so it works for me.
>
> static int qcow2_probe(const uint8_t *buf, int buf_size, const char
> *filename)
> {
> @@ -74,6 +77,75 @@ static int qcow2_probe(const uint8_t *buf, int buf_size,
> const char *filename)
> }
>
>
> +static ssize_t qcow2_crypto_hdr_read_func(QCryptoBlock *block, size_t offset,
> + uint8_t *buf, size_t buflen,
> + Error **errp, void *opaque)
> +{
> + BlockDriverState *bs = opaque;
> + BDRVQcow2State *s = bs->opaque;
> + ssize_t ret;
> +
> + if ((offset + buflen) > s->crypto_header.length) {
The inner () are redundant, but I don't mind them.
> + error_setg(errp, "Request for data outside of extension header");
> + return -1;
Nice that you are trying to prevent reads beyond the block of data
reserved by the FDE header; but is s->crypto_header.length the
rounded-up cluster boundary, or just the length of the used portion of
the LUKS header?...
> + }
> +
> + ret = bdrv_pread(bs->file->bs,
> + s->crypto_header.offset + offset, buf, buflen);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Could not read encryption header");
> + return -1;
> + }
> + return ret;
> +}
> +
> +
> +static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t
> headerlen,
> + Error **errp, void *opaque)
> +{
> + BlockDriverState *bs = opaque;
> + BDRVQcow2State *s = bs->opaque;
> + int64_t ret;
> +
> + s->crypto_header.length = headerlen + (headerlen % s->cluster_size);
...Huh? Are you trying to round up to a cluster boundary? This doesn't
do what you want. And even if you had correctly rounded up to cluster
boundary size, that means that qcow2_crypto_hdr_read_func() can now read
the tail beyond the size recorded in the crypto header - which could be
dangerous if it means that tail can overlap the same sector number used
for the first guest payload sector which uses a sector number of
payload_offset from the LUKS header for its IV.
> +
> + ret = qcow2_alloc_clusters(bs, s->crypto_header.length);
Does qcow2_alloc_clusters(bs, headerlen) properly round up the
allocation to a cluster boundary? If so, I think you want
s->crypto_header.length = headerlen, full stop.
> + if (ret < 0) {
> + s->crypto_header.length = 0;
And it may be easier to not modify s->crypto_header.length except on
successful allocation, rather than having to undo it on failure.
> + error_setg_errno(errp, -ret,
> + "Cannot allocate cluster for LUKS header size %zu",
> + headerlen);
> + return -1;
> + }
> +
> + s->crypto_header.offset = ret;
> + return ret;
> +}
> +
> +
> +static ssize_t qcow2_crypto_hdr_write_func(QCryptoBlock *block, size_t
> offset,
> + const uint8_t *buf, size_t buflen,
> + Error **errp, void *opaque)
> +{
> + BlockDriverState *bs = opaque;
> + BDRVQcow2State *s = bs->opaque;
> + ssize_t ret;
> +
> + if ((offset + buflen) > s->crypto_header.length) {
> + error_setg(errp, "Request for data outside of extension header");
Again, I think you want to be very careful and not allow writes beyond
the initial header length reservation, even if that leaves the tail of
the cluster-aligned FDE area unwritable.
> + return -1;
> + }
> +
> + ret = bdrv_pwrite(bs->file->bs,
> + s->crypto_header.offset + offset, buf, buflen);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Could not read encryption header");
> + return -1;
> + }
> + return ret;
> +}
> +
> +
> /*
> * read qcow2 extension and fill bs
> * start reading from start_offset
> @@ -83,6 +155,7 @@ static int qcow2_probe(const uint8_t *buf, int buf_size,
> const char *filename)
> */
> static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> uint64_t end_offset, void **p_feature_table,
> + int flags,
> Error **errp)
Worth packing these last two parameters on a single line?
> {
> BDRVQcow2State *s = bs->opaque;
> @@ -159,6 +232,39 @@ static int qcow2_read_extensions(BlockDriverState *bs,
> uint64_t start_offset,
> }
> break;
>
> + case QCOW2_EXT_MAGIC_CRYPTO_HEADER: {
> + unsigned int cflags = 0;
> + if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
> + error_setg(errp, "CRYPTO header extension only "
> + "expected with LUKS encryption method");
> + return -EINVAL;
> + }
> + if (ext.len != sizeof(Qcow2CryptoHeaderExtension)) {
> + error_setg(errp, "CRYPTO header extension size %u, "
> + "but expected size %zu", ext.len,
> + sizeof(Qcow2CryptoHeaderExtension));
> + return -EINVAL;
> + }
> +
> + ret = bdrv_pread(bs->file->bs, offset, &s->crypto_header,
> ext.len);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret,
> + "Unable to read CRYPTO header extension");
> + return ret;
> + }
> + be64_to_cpu(s->crypto_header.offset);
> + be64_to_cpu(s->crypto_header.length);
Seeing this made me look back at patch 7. It's convenient that the LUKS
header also uses big-endian storage (otherwise, you'd have to tweak the
portion of the qcow2 spec that requires big-endian everywhere to special
case LUKS header content - although that is not the content being
byte-swapped here).
> +
> + if (flags & BDRV_O_NO_IO) {
> + cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
> + }
> + s->crypto = qcrypto_block_open(s->crypto_opts,
> + qcow2_crypto_hdr_read_func,
> + bs, cflags, errp);
> + if (!s->crypto) {
> + return -EINVAL;
> + }
> + } break;
> default:
> /* unknown magic - save it in case we need to rewrite the header
> */
Hmm. Based solely on the presence or absence of unknown headers, earlier
versions of qemu-img that don't recognize the new FDE extension header
would happily open an image without decrypting it, which may have
disastrous results if it writes to guest data. But I think we are safe
in that the only time the FDE extension header is present is if the
crypt_method is 2, but older versions of qemu should reject crypt_method
2 as unknown. Phew - we don't have to burn an incompatible feature bit
to protect newer images from being corrupted by an older qemu.
> {
> @@ -472,6 +578,11 @@ static QemuOptsList qcow2_runtime_opts = {
> .type = QEMU_OPT_NUMBER,
> .help = "Clean unused cache entries after this time (in
> seconds)",
> },
> + {
> + .name = QCOW2_OPT_KEY_SECRET,
> + .type = QEMU_OPT_STRING,
> + .help = "ID of the secret that provides the encryption key",
> + },
> { /* end of list */ }
> },
> };
> @@ -589,6 +700,111 @@ static void read_cache_sizes(BlockDriverState *bs,
> QemuOpts *opts,
> }
> }
>
> +
> +static QCryptoBlockOpenOptions *
> +qcow2_crypto_open_opts_init(QCryptoBlockFormat format, QemuOpts *opts,
qemu style doesn't usually split return type from function name. I won't
request a reformat, but others might be more picky.
> +static QCryptoBlockCreateOptions *
> +qcow2_crypto_create_opts_init(QCryptoBlockFormat format, QemuOpts *opts,
> + Error **errp)
> +{
> + OptsVisitor *ov;
> + QCryptoBlockCreateOptions *ret;
> + Error *local_err = NULL, *end_err = NULL;
> + Visitor *v;
> +
> + ret = g_new0(QCryptoBlockCreateOptions, 1);
> + ret->format = format;
> +
> + ov = opts_visitor_new(opts);
> + v = opts_get_visitor(ov);
> + visit_start_struct(v, NULL, NULL, NULL, 0, &local_err);
Depending on whether Markus' qapi-next branch lands first, this (and
similar) lines will have to change to drop an unused NULL.
> +
> + visit_end_struct(v, &end_err);
and my pending patches beyond what is in qapi-next affect this line. Oh
well, we'll get it figured out.
> @@ -754,6 +971,28 @@ static int qcow2_update_options_prepare(BlockDriverState
> *bs,
> r->discard_passthrough[QCOW2_DISCARD_OTHER] =
> qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
>
> + switch (s->crypt_method_header) {
> + case QCOW_CRYPT_NONE:
> + break;
Are we allowing in-place updates to change the encryption method? It
may be safer to require that in-place updates can't change encryption,
for now (that is, you can only set encryption at creation of a new file,
and then copy data from one file to another to change how it is
encrypted). And even if we DO want to allow a user to convert an image
from encrypted to plain, or from plain to encrypted, I would favor it as
a separate patch, so that we make sure we properly handle the
creation/removal of the FDE extension, as well as en/decrypting every
byte of payload independently from simpler read/write of an encrypted image.
> @@ -788,6 +1027,9 @@ static void qcow2_update_options_commit(BlockDriverState
> *bs,
> s->cache_clean_interval = r->cache_clean_interval;
> cache_clean_timer_init(bs, bdrv_get_aio_context(bs));
> }
> +
> + qapi_free_QCryptoBlockOpenOptions(s->crypto_opts);
> + s->crypto_opts = r->crypto_opts;
> }
>
> static void qcow2_update_options_abort(BlockDriverState *bs,
> @@ -799,6 +1041,7 @@ static void qcow2_update_options_abort(BlockDriverState
> *bs,
> if (r->refcount_block_cache) {
> qcow2_cache_destroy(bs, r->refcount_block_cache);
> }
> + qapi_free_QCryptoBlockOpenOptions(r->crypto_opts);
Again, I don't know that we should allow updating the use of crypto
during an update options task, or if we do, it should be in a separate
patch (let's get reading/writing to an encrypted image correct first,
before we worry about in-place conversion).
> @@ -1104,12 +1337,37 @@ static int qcow2_open(BlockDriverState *bs, QDict
> *options, int flags,
>
> /* read qcow2 extensions */
> if (qcow2_read_extensions(bs, header.header_length, ext_end, NULL,
> - &local_err)) {
> + flags, &local_err)) {
> error_propagate(errp, local_err);
> ret = -EINVAL;
> goto fail;
> }
>
> + /* qcow2_read_extension may have setup the crypto context
s/setup/set up/
> + * if the crypt method needs a header region, some methods
> + * don't need header extensions, so must check here
> + */
> + if (s->crypt_method_header && !s->crypto) {
> @@ -1984,6 +2222,77 @@ static int qcow2_change_backing_file(BlockDriverState
> *bs,
> return qcow2_update_header(bs);
> }
>
> +
> +static int qcow2_change_encryption(BlockDriverState *bs, QemuOpts *opts,
> + Error **errp)
> +{
> + BDRVQcow2State *s = bs->opaque;
> + const char *cryptostr;
> + QCryptoBlockCreateOptions *cryptoopts = NULL;
> + QCryptoBlock *crypto = NULL;
> + size_t i;
> + int ret = -EINVAL;
> +
> + /* Default to LUKS if crypto-format is not set */
> + cryptostr = qemu_opt_get_del(opts, QCOW2_OPT_CRYPTO_FORMAT);
> + if (cryptostr) {
> + for (i = 0; i < Q_CRYPTO_BLOCK_FORMAT__MAX; i++) {
> + if (g_str_equal(QCryptoBlockFormat_lookup[i], cryptostr)) {
> + cryptoopts = qcow2_crypto_create_opts_init(i, opts, errp);
> + if (!cryptoopts) {
> + ret = -EINVAL;
> + goto out;
> + }
> + break;
> + }
> + }
> + if (!cryptoopts) {
> + error_setg(errp, "Unknown crypto format %s", cryptostr);
Could use qapi_enum_parse() here.
> @@ -2267,11 +2581,17 @@ static int qcow2_create2(const char *filename,
> int64_t total_size,
> bdrv_unref(bs);
> bs = NULL;
>
> - /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning
> */
> + /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning
s/$/./
> + * Using BDRV_O_NO_IO, since encryption is now setup we don't want to
> + * have to setup decryption context. We're not doing any I/O on the top
> + * level BlockDriverState, only lower layers, where BDRV_O_NO_IO does
> + * not have effect.
> + */
> options = qdict_new();
> qdict_put(options, "driver", qstring_from_str("qcow2"));
> ret = bdrv_open(&bs, filename, NULL, options,
> - BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
> + BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING |
> + BDRV_O_NO_IO, /* Don't want to activate encryption */
Do we really need the second comment, after the first?
> &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> @@ -3046,9 +3366,9 @@ static int qcow2_amend_options(BlockDriverState *bs,
> QemuOpts *opts,
> backing_format = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
> } else if (!strcmp(desc->name, BLOCK_OPT_ENCRYPT)) {
> encrypt = qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT,
> - !!s->cipher);
> + !!s->crypto);
>
> - if (encrypt != !!s->cipher) {
> + if (encrypt != !!s->crypto) {
> error_report("Changing the encryption flag is not
> supported");
Oh, so we don't support changing the encryptiong yet (good). But it
means I was confused about what was happening above with regards to code
on changing encryption metadata.
> @@ -163,6 +173,11 @@ typedef struct QCowSnapshot {
> struct Qcow2Cache;
> typedef struct Qcow2Cache Qcow2Cache;
>
> +typedef struct Qcow2CryptoHeaderExtension {
> + uint64_t offset;
> + uint64_t length;
> +} QEMU_PACKED Qcow2CryptoHeaderExtension;
> +
> typedef struct Qcow2UnknownHeaderExtension {
> uint32_t magic;
> uint32_t len;
> @@ -256,7 +271,9 @@ typedef struct BDRVQcow2State {
>
> CoMutex lock;
>
> - QCryptoCipher *cipher; /* current cipher, NULL if no key yet */
> + Qcow2CryptoHeaderExtension crypto_header; /* QCow2 header extension */
> + QCryptoBlockOpenOptions *crypto_opts; /* Disk encryption runtime options
> */
> + QCryptoBlock *crypto; /* Disk encryption format driver */
> uint32_t crypt_method_header;
Seems reasonable.
> uint64_t snapshots_offset;
> int snapshots_size;
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 47630ee..2f4b983 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -990,7 +990,6 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> cpu_to_be32s(&luks->header.key_slots[i].stripes);
> }
>
> -
> /* Write out the partition header and key slot headers */
> writefunc(block, 0,
> (const uint8_t *)&luks->header,
Spurious hunk, probably from rebasing.
Continuing on at [3].
> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index f236d8c..4d141a6 100644
> --- a/docs/specs/qcow2.txt
> +++ b/docs/specs/qcow2.txt
[1] Starting my review here.
> @@ -45,6 +45,7 @@ The first cluster of a qcow2 image contains the file header:
> 32 - 35: crypt_method
> 0 for no encryption
> 1 for AES encryption
> + 2 for LUKS encryption
May be worth an additional comment:
The choice of crypt_method affects whether the full disk encryption
header extension must be used; see details below.
>
> 36 - 39: l1_size
> Number of entries in the active L1 table
> @@ -123,6 +124,7 @@ be stored. Each extension has a structure like the
> following:
> 0x00000000 - End of the header extension area
> 0xE2792ACA - Backing file format name
> 0x6803f857 - Feature name table
> + 0x4c554b53 - Full disk encryption header pointer
We aren't very consistent on case within the hex constants. Not your
fault, but might be nice to fix.
> other - Unknown header extension, can be safely
> ignored
>
> @@ -166,6 +168,78 @@ the header extension data. Each entry look like this:
> terminated if it has full length)
>
>
> +== Full disk encryption header pointer ==
> +
> +The full disk encryption header must be present if, and only if, the
> +'crypt_method' header requires metadata. Currently this is only true
> +of the 'LUKS' crypt method. The header extension must be absent for
> +other methods.
> +
> +This header provides the offset at which the crypt method can store
> +its additional data, as well as the length of such data.
> +
> + Byte 0 - 7: Offset into the image file at which the encryption
> + header starts. Must be aligned to a cluster boundary.
> + Byte 8 - 16: Length of the encryption header. Must be a multiple
> + of the cluster size.
I would add "in bytes" to both descriptions.
Should we explicitly document that the length occupied by the extension
header is NOT counted towards the guest-visible length; therefore, the
guest-visible size is the payload of the encrypted disk?
> +
> +While the header extension allocates the length as a multiple of the
> +cluster size, the encryption header may not consume the full permitted
> +allocation.
Should we require that any unused trailing space be all 0?
> +
> +For the LUKS crypt method, the encryption header works as follows.
> +
> +The first 592 bytes of the header clusters will contain the LUKS
> +partition header. This is then followed by the key material data areas.
> +The size of the key material data areas is determined by the number of
> +stripes in the key slot and key size. Refer to the LUKS format
> +specification ('docs/on-disk-format.pdf' in the cryptsetup source
> +package) for details of the LUKS partition header format.
> +
> +In the LUKS partition header, the "payload-offset" field does not refer
> +to the offset of the QCow2 payload. Instead it simply refers to the
> +total required length of the LUKS header plus key material regions.
I'm guessing that you are allowing the payload-offset to NOT be a
multiple of the cluster size. It may be worth documenting that since
LUKS requires encryption to happen on 512-byte sector boundaries, where
the sector number feeds the IV used to encrypt that sector, that
guest-visible sector 0 is treated as LUKS sector payload-offset, even if
payload-offset is not qcow2-cluster-aligned.
> +
> +In the LUKS key slots header, the "key-material-offset" is relative
> +to the start of the LUKS header clusters in the qcow2 container,
> +not the start of the qcow2 file.
> +
> +Logically the layout looks like
> +
How does encryption interact with internal snapshots? Is the encryption
header over all snapshots at once, or are we taking snapshots of the
current key slot usage? That is, if we later add a command to allow key
slot manipulation (add a new user password on a vacant key slot, or
revoke a key slot), what happens if the user takes a snapshot, revokes a
key slot, then takes a second snapshot, then wants to revert to the
first snapshot? Will the revoked password still be usable to unlock the
first snapshot contents, or did revoking it destroy that user's ability
to access the snapshot?
I'm leaning towards the latter - there is only a single LUKS header for
the entire qcow2 file; LUKS key management is global regardless of what
ever other content is stored, and the user passwords that unlock the
LUKS master key control whether a user can access all or none of the
rest of the qcow2 data.
I suspect that backing files are NOT encrypted by the LUKS header of the
active file. That is, if we go to read a sector, and it is not present
in the current qcow2 image, then reading it from the backing file does
NOT need to decrypt data (unless the backing file itself independently
used a LUKS header).
Conversely, if we have a backing file that is encrypted, do we want to
place any requirements that the wrapper file can/must use encryption? I
don't see any technical reasons that would require it, but from a data
safety standpoint, it seems awkward that a user that provides the LUKS
password to read the backing file can then write the same data
unencrypted into their wrapper file on copy-on-write operations.
> +++ b/qapi/block-core.json
> @@ -1789,6 +1789,8 @@
> # @cache-clean-interval: #optional clean unused entries in the L2 and
> refcount
> # caches. The interval is in seconds. The default
> value
> # is 0 and it disables this feature (since 2.5)
> +# @key-secret: #optional the ID of a QCryptoSecret object
> providing
> +# the decryption key (since 2.6)
As in other patches in this series, may be worth mentioning that this is
mandatory for doing I/O on an encrypted disk, and optional when the disk
is not encrypted or when only metadata is being probed.
> #
> # Since: 1.7
> ##
> @@ -1802,8 +1804,8 @@
> '*cache-size': 'int',
> '*l2-cache-size': 'int',
> '*refcount-cache-size': 'int',
> - '*cache-clean-interval': 'int' } }
> -
> + '*cache-clean-interval': 'int',
> + '*key-secret': 'str' } }
>
> ##
> # @BlockdevOptionsArchipelago
> diff --git a/tests/qemu-iotests/049 b/tests/qemu-iotests/049
> index 93aa0ea..765b950 100755
> --- a/tests/qemu-iotests/049
Okay, back to [2], then this will be spot [3].
> +++ b/tests/qemu-iotests/049
> @@ -107,7 +107,7 @@ test_qemu_img create -f $IMGFMT -o preallocation=1234
> "$TEST_IMG" 64M
> echo "== Check encryption option =="
> echo
> test_qemu_img create -f $IMGFMT -o encryption=off "$TEST_IMG" 64M
> -test_qemu_img create -f $IMGFMT -o encryption=on "$TEST_IMG" 64M
> +test_qemu_img create -f $IMGFMT --object secret,id=sec0,data=123456 -o
> encryption=on,key-secret=sec0 "$TEST_IMG" 64M
>
> echo "== Check lazy_refcounts option (only with v3) =="
> echo
> diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
> index a2b6703..c9f0bc5 100644
> --- a/tests/qemu-iotests/049.out
> +++ b/tests/qemu-iotests/049.out
> @@ -186,14 +186,11 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864
> encryption=off cluster_si
> qemu-img create -f qcow2 -o encryption=off TEST_DIR/t.qcow2 64M
> Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=off
> cluster_size=65536 lazy_refcounts=off refcount_bits=16
>
> -qemu-img create -f qcow2 -o encryption=on TEST_DIR/t.qcow2 64M
> +qemu-img create -f qcow2 --object secret,id=sec0,data=123456 -o
> encryption=on,key-secret=sec0 TEST_DIR/t.qcow2 64M
> qemu-img: Encrypted images are deprecated
> Support for them will be removed in a future release.
> You can use 'qemu-img convert' to convert your image to an unencrypted one.
> -qemu-img: Encrypted images are deprecated
> -Support for them will be removed in a future release.
> -You can use 'qemu-img convert' to convert your image to an unencrypted one.
> -Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=on
> cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=on
> cluster_size=65536 lazy_refcounts=off refcount_bits=16 key-secret=sec0
So now that we support LUKS encryption by default, we no longer need the
deprecation warning. Do we still forbid the creation of new images with
non-LUKS encryption? That is, even though the new code will let us read
old images, I want to make sure we test the error message (or
deprecation warning) when trying to use the new options to request the
old format during image creation.
>
> == Check lazy_refcounts option (only with v3) ==
>
> diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
> index a952330..b0572d4 100644
> --- a/tests/qemu-iotests/082.out
> +++ b/tests/qemu-iotests/082.out
> @@ -53,6 +53,13 @@ cluster_size qcow2 cluster size
> preallocation Preallocation mode (allowed values: off, metadata, falloc,
> full)
> lazy_refcounts Postpone refcount updates
> refcount_bits Width of a reference count entry in bits
> +encryption-format Encryption format, 'luks' (default), 'qcow' (deprecated)
> +key-secret ID of the secret that provides the encryption key
> +cipher-alg Name of encryption cipher algorithm
> +cipher-mode Name of encryption cipher mode
> +ivgen-alg Name of IV generator algorithm
> +ivgen-hash-alg Name of IV generator hash algorithm
> +hash-alg Name of encryption hash algorithm
> nocow Turn off copy-on-write (valid only on btrfs)
Should this help text mention defaults?
> +++ b/tests/qemu-iotests/087
> @@ -184,9 +184,18 @@ echo
> echo === Encrypted image ===
> echo
>
> -_make_test_img -o encryption=on $size
> +_make_test_img --object secret,id=sec0,data=123456 -o
> encryption=on,key-secret=sec0 $size
> run_qemu -S <<EOF
> { "execute": "qmp_capabilities" }
> +{ "execute": "object-add",
> + "arguments": {
> + "qom-type": "secret",
> + "id": "sec0",
> + "props": {
> + "data": "123456"
> + }
> + }
> +}
> { "execute": "blockdev-add",
> "arguments": {
> "options": {
> @@ -195,7 +204,8 @@ run_qemu -S <<EOF
> "file": {
> "driver": "file",
> "filename": "$TEST_IMG"
> - }
> + },
> + "key-secret": "sec0"
> }
Nice - proof that we can hot-plug a secret. Which means libvirt will
have to be taught to _always_ prepopulate a master key for new enough
qemu, even if there is no encrypted disk on the command line, to cater
for hotplug down the road.
> +++ b/tests/qemu-iotests/134
> @@ -44,23 +44,31 @@ _supported_os Linux
>
> -128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +can't open device
> /home/berrange/src/virt/qemu/tests/qemu-iotests/scratch/t.qcow2: Invalid
> password, cannot unlock any keyslot
> +no file open, try 'help open'
Umm, you'll want to fix this test to run on more than just your machine.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v2 12/17] qcow2: convert QCow2 to use QCryptoBlock for encryption,
Eric Blake <=