From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34840) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aSqIy-0006hx-Dn for qemu-devel@nongnu.org; Mon, 08 Feb 2016 13:12:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aSqIt-0000Fu-Bx for qemu-devel@nongnu.org; Mon, 08 Feb 2016 13:12:56 -0500 References: <1453311539-1193-1-git-send-email-berrange@redhat.com> <1453311539-1193-13-git-send-email-berrange@redhat.com> From: Eric Blake Message-ID: <56B8DA95.1050504@redhat.com> Date: Mon, 8 Feb 2016 11:12:37 -0700 MIME-Version: 1.0 In-Reply-To: <1453311539-1193-13-git-send-email-berrange@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="tUMFRIbh6nGpMr7jpiVKsCr1sOfhenWQl" Subject: Re: [Qemu-devel] [PATCH v2 12/17] qcow2: convert QCow2 to use QCryptoBlock for encryption List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Kevin Wolf , Fam Zheng , qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --tUMFRIbh6nGpMr7jpiVKsCr1sOfhenWQl Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > With this change it is now required to use the QCryptoSecret > object for providing passwords, instead of the current block > password APIs / interactive prompting. >=20 > $QEMU \ > -object secret,id=3Dsec0,filename=3D/home/berrange/encrypted.pw \ > -drive file=3D/home/berrange/encrypted.qcow2,key-secret=3Dsec0 >=20 > The new LUKS format is set as the new default format when > creating encrypted images. ie >=20 > # qemu-img create --object secret,data=3D123456,id=3Dsec0 \ > -f qcow2 -o encryption,key-secret=3Dsec0 \ > 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? >=20 > 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/ >=20 > 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 >=20 > # qemu-img create --object secret,data=3D123456,id=3Dsec0 \ > -f qcow2 -o encryption,key-secret=3Dsec0,encryption-format=3Dqco= waes \ > test.qcow2 10G >=20 > Signed-off-by: Daniel P. Berrange > --- > 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(-) >=20 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; > } > =20 > + /* encryption */ > + if (s->crypto_header.length) { > + ret =3D 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_c= lusters); > } > =20 > 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" > =20 > /* > 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. > =20 > static int qcow2_probe(const uint8_t *buf, int buf_size, const char *f= ilename) > { > @@ -74,6 +77,75 @@ static int qcow2_probe(const uint8_t *buf, int buf_s= ize, const char *filename) > } > =20 > =20 > +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 =3D opaque; > + BDRVQcow2State *s =3D 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 =3D 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 =3D opaque; > + BDRVQcow2State *s =3D bs->opaque; > + int64_t ret; > + > + s->crypto_header.length =3D headerlen + (headerlen % s->cluster_si= ze); =2E..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 =3D 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 =3D headerlen, full stop. > + if (ret < 0) { > + s->crypto_header.length =3D 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 =3D 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 =3D opaque; > + BDRVQcow2State *s =3D 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 =3D 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; > +} > + > + > /*=20 > * 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_s= ize, 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 =3D bs->opaque; > @@ -159,6 +232,39 @@ static int qcow2_read_extensions(BlockDriverState = *bs, uint64_t start_offset, > } > break; > =20 > + case QCOW2_EXT_MAGIC_CRYPTO_HEADER: { > + unsigned int cflags =3D 0; > + if (s->crypt_method_header !=3D QCOW_CRYPT_LUKS) { > + error_setg(errp, "CRYPTO header extension only " > + "expected with LUKS encryption method"); > + return -EINVAL; > + } > + if (ext.len !=3D sizeof(Qcow2CryptoHeaderExtension)) { > + error_setg(errp, "CRYPTO header extension size %u, " > + "but expected size %zu", ext.len, > + sizeof(Qcow2CryptoHeaderExtension)); > + return -EINVAL; > + } > + > + ret =3D bdrv_pread(bs->file->bs, offset, &s->crypto_header= , ext.len); > + if (ret < 0) { > + error_setg_errno(errp, -ret, > + "Unable to read CRYPTO header extensi= on"); > + 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 |=3D QCRYPTO_BLOCK_OPEN_NO_IO; > + } > + s->crypto =3D 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 =3D { > .type =3D QEMU_OPT_NUMBER, > .help =3D "Clean unused cache entries after this time (in = seconds)", > }, > + { > + .name =3D QCOW2_OPT_KEY_SECRET, > + .type =3D QEMU_OPT_STRING, > + .help =3D "ID of the secret that provides the encryption k= ey", > + }, > { /* end of list */ } > }, > }; > @@ -589,6 +700,111 @@ static void read_cache_sizes(BlockDriverState *bs= , QemuOpts *opts, > } > } > =20 > + > +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 *opt= s, > + Error **errp) > +{ > + OptsVisitor *ov; > + QCryptoBlockCreateOptions *ret; > + Error *local_err =3D NULL, *end_err =3D NULL; > + Visitor *v; > + > + ret =3D g_new0(QCryptoBlockCreateOptions, 1); > + ret->format =3D format; > + > + ov =3D opts_visitor_new(opts); > + v =3D 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(BlockDrive= rState *bs, > r->discard_passthrough[QCOW2_DISCARD_OTHER] =3D > qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false); > =20 > + 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 ima= ge. > @@ -788,6 +1027,9 @@ static void qcow2_update_options_commit(BlockDrive= rState *bs, > s->cache_clean_interval =3D r->cache_clean_interval; > cache_clean_timer_init(bs, bdrv_get_aio_context(bs)); > } > + > + qapi_free_QCryptoBlockOpenOptions(s->crypto_opts); > + s->crypto_opts =3D r->crypto_opts; > } > =20 > static void qcow2_update_options_abort(BlockDriverState *bs, > @@ -799,6 +1041,7 @@ static void qcow2_update_options_abort(BlockDriver= State *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, QDi= ct *options, int flags, > =20 > /* 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 =3D -EINVAL; > goto fail; > } > =20 > + /* 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(BlockDriver= State *bs, > return qcow2_update_header(bs); > } > =20 > + > +static int qcow2_change_encryption(BlockDriverState *bs, QemuOpts *opt= s, > + Error **errp) > +{ > + BDRVQcow2State *s =3D bs->opaque; > + const char *cryptostr; > + QCryptoBlockCreateOptions *cryptoopts =3D NULL; > + QCryptoBlock *crypto =3D NULL; > + size_t i; > + int ret =3D -EINVAL; > + > + /* Default to LUKS if crypto-format is not set */ > + cryptostr =3D qemu_opt_get_del(opts, QCOW2_OPT_CRYPTO_FORMAT); > + if (cryptostr) { > + for (i =3D 0; i < Q_CRYPTO_BLOCK_FORMAT__MAX; i++) { > + if (g_str_equal(QCryptoBlockFormat_lookup[i], cryptostr)) = { > + cryptoopts =3D qcow2_crypto_create_opts_init(i, opts, = errp); > + if (!cryptoopts) { > + ret =3D -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 =3D NULL; > =20 > - /* Reopen the image without BDRV_O_NO_FLUSH to flush it before ret= urning */ > + /* Reopen the image without BDRV_O_NO_FLUSH to flush it before ret= urning 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 th= e top > + * level BlockDriverState, only lower layers, where BDRV_O_NO_IO d= oes > + * not have effect. > + */ > options =3D qdict_new(); > qdict_put(options, "driver", qstring_from_str("qcow2")); > ret =3D 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 =3D qemu_opt_get(opts, BLOCK_OPT_BACKING_FM= T); > } else if (!strcmp(desc->name, BLOCK_OPT_ENCRYPT)) { > encrypt =3D qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT, > - !!s->cipher); > + !!s->crypto); > =20 > - if (encrypt !=3D !!s->cipher) { > + if (encrypt !=3D !!s->crypto) { > error_report("Changing the encryption flag is not supp= orted"); 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; > =20 > +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 { > =20 > CoMutex lock; > =20 > - QCryptoCipher *cipher; /* current cipher, NULL if no key yet */ > + Qcow2CryptoHeaderExtension crypto_header; /* QCow2 header extensio= n */ > + QCryptoBlockOpenOptions *crypto_opts; /* Disk encryption runtime o= ptions */ > + 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); > } > =20 > - > /* 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. > =20 > 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 point= er 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 > =20 > @@ -166,6 +168,78 @@ the header extension data. Each entry look like th= is: > terminated if it has full length) > =20 > =20 > +=3D=3D Full disk encryption header pointer =3D=3D > + > +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 bounda= ry. > + Byte 8 - 16: Length of the encryption header. Must be a multipl= e > + 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= =2E > +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 refe= r > +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 defa= ult value > # is 0 and it disables this feature (since 2.5= ) > +# @key-secret: #optional the ID of a QCryptoSecret object p= roviding > +# 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' } } > =20 > ## > # @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=3D= 1234 "$TEST_IMG" 64M > echo "=3D=3D Check encryption option =3D=3D" > echo > test_qemu_img create -f $IMGFMT -o encryption=3Doff "$TEST_IMG" 64M > -test_qemu_img create -f $IMGFMT -o encryption=3Don "$TEST_IMG" 64M > +test_qemu_img create -f $IMGFMT --object secret,id=3Dsec0,data=3D12345= 6 -o encryption=3Don,key-secret=3Dsec0 "$TEST_IMG" 64M > =20 > echo "=3D=3D Check lazy_refcounts option (only with v3) =3D=3D" > 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=3Dqcow2 size=3D= 67108864 encryption=3Doff cluster_si > qemu-img create -f qcow2 -o encryption=3Doff TEST_DIR/t.qcow2 64M > Formatting 'TEST_DIR/t.qcow2', fmt=3Dqcow2 size=3D67108864 encryption=3D= off cluster_size=3D65536 lazy_refcounts=3Doff refcount_bits=3D16 > =20 > -qemu-img create -f qcow2 -o encryption=3Don TEST_DIR/t.qcow2 64M > +qemu-img create -f qcow2 --object secret,id=3Dsec0,data=3D123456 -o en= cryption=3Don,key-secret=3Dsec0 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=3Dqcow2 size=3D67108864 encryption=3D= on cluster_size=3D65536 lazy_refcounts=3Doff refcount_bits=3D16 > +Formatting 'TEST_DIR/t.qcow2', fmt=3Dqcow2 size=3D67108864 encryption=3D= on cluster_size=3D65536 lazy_refcounts=3Doff refcount_bits=3D16 key-secre= t=3Dsec0 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. > =20 > =3D=3D Check lazy_refcounts option (only with v3) =3D=3D > =20 > 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, fa= lloc, full) > lazy_refcounts Postpone refcount updates > refcount_bits Width of a reference count entry in bits > +encryption-format Encryption format, 'luks' (default), 'qcow' (depreca= ted) > +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 =3D=3D=3D Encrypted image =3D=3D=3D > echo > =20 > -_make_test_img -o encryption=3Don $size > +_make_test_img --object secret,id=3Dsec0,data=3D123456 -o encryption=3D= on,key-secret=3Dsec0 $size > run_qemu -S < { "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 < "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 > =20 > -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/scra= tch/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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --tUMFRIbh6nGpMr7jpiVKsCr1sOfhenWQl Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWuNqVAAoJEKeha0olJ0Nqbj8IAKjffgg4Bz/Wzu34n+sCwUfP dIbSgGTsXgEsIJ7/pOwlfy9hoUdTw5N+Zsiv5c5ietCibHDRtI2LgHu/aDMJ6ckV WwjyG2EZ8JkMN8a+oN1MSE2EH9sgxRx5qsj/jV9ScJVTmWm8fGr0ZBKbwsfrFk+3 z/Cm3NhxTo/6Bq41XChNCzEAVjN90pwxudkUkRGwQJdT//mBtuZX55Z7ty/y34R2 ULTg1FNcjjlXTzTJ4iqCCu+GFG2dXgHIi2/kPLfLe3tQyGUW9m2NCs5abViZHiOc yP/Fal6Cf4ImzfuczKoA/5pNMjqsmKIKAtQySczPj990oigGWYhHVgReWpslHJc= =WfP8 -----END PGP SIGNATURE----- --tUMFRIbh6nGpMr7jpiVKsCr1sOfhenWQl--