| Message ID | 1453311539-1193-13-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 01/20 17:38, 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. FWIW, with today's QEMU, it's possible to stack format drivers on top of each other. In other words, even without this patch, we can make LUKS driver encrypt/decrypt the qcow2 payload, while keeping them completely orthogonal. It's someting like: -------------------- | LUKS | -------------------- | v -------------------- | qcow2 | -------------------- | v -------------------- | file | -------------------- The command line looks like this: -drive driver=luks,file.driver=qcow2,file.file.driver=file,\ file.file.filename=$qcow2_image_whose_payload_is_in_luks_format unfortunately I don't know how to create nested images with qemu-img. I tested the nested qcow2 by attaching the outter image to a VM and running "qemu-img create -f qcow2 /dev/vda" in guest shell. Kevin? Fam
On Thu, Jan 21, 2016 at 05:54:23PM +0800, Fam Zheng wrote: > On Wed, 01/20 17:38, 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. > > FWIW, with today's QEMU, it's possible to stack format drivers on top of each > other. In other words, even without this patch, we can make LUKS driver > encrypt/decrypt the qcow2 payload, while keeping them completely orthogonal. Yep, that is certainly possible, and it is what is intended for using LUKS with RBD, iSCSI, & other network drivers. I think there is value in having LUKS integrated directly into qcow2 though. It means that given a qcow2 file you can 100% reliably distinguish between a file created with the intention of QEMU managing the LUKS encryption, from a file where the guest OS happens to have set up LUKS encryption in its virtual disk. If you don't have this, then given a random qcow2 file, you have to probe to see if LUKS is present or not. Given the security issues we've had in the past with raw images being turned into qcow2 images by a malicious guest writing a qcow2 header, I feel that having explicitly integration LUKS support in QCow is worthwhile as a concept. Regards, Daniel
On Thu, 01/21 10:50, Daniel P. Berrange wrote: > On Thu, Jan 21, 2016 at 05:54:23PM +0800, Fam Zheng wrote: > > On Wed, 01/20 17:38, 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. > > > > FWIW, with today's QEMU, it's possible to stack format drivers on top of each > > other. In other words, even without this patch, we can make LUKS driver > > encrypt/decrypt the qcow2 payload, while keeping them completely orthogonal. > > Yep, that is certainly possible, and it is what is intended for using > LUKS with RBD, iSCSI, & other network drivers. > > I think there is value in having LUKS integrated directly into qcow2 > though. It means that given a qcow2 file you can 100% reliably > distinguish between a file created with the intention of QEMU managing > the LUKS encryption, from a file where the guest OS happens to have > set up LUKS encryption in its virtual disk. If you don't have this, > then given a random qcow2 file, you have to probe to see if LUKS is > present or not. Given the security issues we've had in the past with > raw images being turned into qcow2 images by a malicious guest writing > a qcow2 header, I feel that having explicitly integration LUKS support > in QCow is worthwhile as a concept. Yes, I'm not objecting to explicit (read: mandatory) encryption in qcow2 in any way, and extending the format spec for LUKS is a good thing. I mentioned stacked drivers because I wonder how good we can do in reusing block/crypto.c code to achieve this (to save 500+ new code in qcow2). For example I have a rough idea along this: * In qcow2 spec, define type "2" of crypt_method for LUKS encryption, and state that if this method is used, the payload must be LUKS format as defined in cryptsetup's docs/on-disk-format.pdf, and driver will take care of encrypting/decrypting data in a way that is transparent to guest. * In qcow2_open, if header's crypt_method is LUKS, set a special flag in the BlockDriverState to indicate that block layer should create an overlay crypto driver (which will be luks in this case), on top of this BDS. To do this we need some modification to bdrv_open. * When qcow2_open is done, block layer will then instantiate a LUKS driver, which backed by the qcow2 BDS (as the BDS.file). This LUKS BDS will then be attached to guest device. * From the guest PoV, R/W reqs are served as if it's an ordinary qcow2; from LUKS driver's PoV, it works properly formatted luks data, which is backed by whatever it doesn't care, be it an iscsi target, rbd block, local file, or a qcow2 image that has metadata; from the qcow2 driver's PoV, it only made sure that a LUKS driver covers itself, at instantiation time. Everything else operates the same way as a non-encrypted qcow2. * Of course, qcow2_create would need some similar changes to the block layer as to bdrv_open, but that shouldn't be too hard. Makes sense? Fam
On Thu, Jan 21, 2016 at 09:56:11PM +0800, Fam Zheng wrote: > On Thu, 01/21 10:50, Daniel P. Berrange wrote: > > On Thu, Jan 21, 2016 at 05:54:23PM +0800, Fam Zheng wrote: > > > On Wed, 01/20 17:38, 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. > > > > > > FWIW, with today's QEMU, it's possible to stack format drivers on top of each > > > other. In other words, even without this patch, we can make LUKS driver > > > encrypt/decrypt the qcow2 payload, while keeping them completely orthogonal. > > > > Yep, that is certainly possible, and it is what is intended for using > > LUKS with RBD, iSCSI, & other network drivers. > > > > I think there is value in having LUKS integrated directly into qcow2 > > though. It means that given a qcow2 file you can 100% reliably > > distinguish between a file created with the intention of QEMU managing > > the LUKS encryption, from a file where the guest OS happens to have > > set up LUKS encryption in its virtual disk. If you don't have this, > > then given a random qcow2 file, you have to probe to see if LUKS is > > present or not. Given the security issues we've had in the past with > > raw images being turned into qcow2 images by a malicious guest writing > > a qcow2 header, I feel that having explicitly integration LUKS support > > in QCow is worthwhile as a concept. > > Yes, I'm not objecting to explicit (read: mandatory) encryption in qcow2 in any > way, and extending the format spec for LUKS is a good thing. > > I mentioned stacked drivers because I wonder how good we can do in reusing > block/crypto.c code to achieve this (to save 500+ new code in qcow2). For > example I have a rough idea along this: > > * In qcow2 spec, define type "2" of crypt_method for LUKS encryption, and state > that if this method is used, the payload must be LUKS format as defined in > cryptsetup's docs/on-disk-format.pdf, and driver will take care of > encrypting/decrypting data in a way that is transparent to guest. > > * In qcow2_open, if header's crypt_method is LUKS, set a special flag in the > BlockDriverState to indicate that block layer should create an overlay crypto > driver (which will be luks in this case), on top of this BDS. To do this we > need some modification to bdrv_open. > > * When qcow2_open is done, block layer will then instantiate a LUKS driver, > which backed by the qcow2 BDS (as the BDS.file). This LUKS BDS will then be > attached to guest device. > > * From the guest PoV, R/W reqs are served as if it's an ordinary qcow2; from > LUKS driver's PoV, it works properly formatted luks data, which is backed by > whatever it doesn't care, be it an iscsi target, rbd block, local file, or > a qcow2 image that has metadata; from the qcow2 driver's PoV, it only made > sure that a LUKS driver covers itself, at instantiation time. Everything > else operates the same way as a non-encrypted qcow2. > > * Of course, qcow2_create would need some similar changes to the block layer as > to bdrv_open, but that shouldn't be too hard. > > Makes sense? Hmm, interesting idea. So essentially we'd be having a format driver be able to inform the generic block code to insert an encryption layer above it. The downside is that it leaks details about use of encryption out into the generic block code. The upside is that it reduces code in the qcow2 impl. I do wonder if that would lead to more or less code overall, or whether the generic block layer additions would cancel out the removals from qcow2. BTW, we can probably reduce the amount of code in qcow2 regardless, as about 1/2 of the addition is related to handling of the QemuOpts <-> QCryptoBlockOpenOptions conversion, and I could share that with the block/crypto.c driver to a greater extent than I do now. Regards, Daniel
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 <berrange@redhat.com> > --- > 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.
On Mon, Feb 08, 2016 at 11:12:37AM -0700, Eric Blake wrote: > 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. > > (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? '-o encryption' is actually the pre-existing flag used to turn on the existing qcow2 built-in AES encryption. I repurposed it as a generic flag to indicate desire for encryption, and have the new 'encryption-format' flag to say whether you want the legacy AES or modern LUKS format. IOW people using '-o encryption' today will automatically start getting LUKS format after this change is applied. > > 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? No, once a fle has encryption it can't be removed. Likewise you have to choose it at time of creation - you can't add it afterwards. > > 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. Hah, top marks for reverse engineering my magic constant. I had completely forgotten that I used the ASCII values of 'LUKS' for this constant :-) Since this is not luks specific anymore I'll change this value to 0x0537be77. Bonus points to anyone who can explain the significance of that number (hint it is not an ascii representation of anything - its a special number in its own right :-) > > +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?... It is intended to record the allocated space (ie rounded to the cluster boundary), but on reflection, I think I'll change it to exactly the size 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. Agreed, I will make it so. > > > + 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. Yes, this will be ok when the length refers to the luks header only > > { > > 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). Yes its fortunate everyone agrees that big-endian is best on disk :-) > > + > > + 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. Yeah, the crypt_method value saves us, because all older versions are careful to validate that. > > + > > +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. The lines get rather long if I dont :-) > > @@ -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. I don't see us ever allowing in-place changes to encryption. You'd have to re-write the entire file contents with obvious disaster if you hit an error 1/2 way through. Far better to use qemu-img convert instead. > > 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). Note despite the name the qcow2_update_options* methods are actually used during initial image creation, so this code is about updating in-place options. > > + * 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? Not any more, no. > > @@ -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. Yes, its a misleading method name earlier used for 2 different things :-) > > @@ -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. Ok. > 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? Sure, can do. > > +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? I guess it might be a nice idea to have it zeroed in case it becomes important later. > > +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. Yeah, there's no restriction on payload-offset at all. Essentially the payload-offset is completely redundant when used in the context of qcow2. We could easily have said it should be 0, since qcow2 determines the actual payload offset. I felt it was worth leving it to record the value LUKS would ordinarily have used, even though we don't need it. > > +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. NB passwords have no direct relationship to the data encryption key. The passwords are just used to encrypt the master key. So you can change the passwords at any time, without loosing ability to decrypt old data. IOW, we can completely ignore the issue of snapshots. All snapshot data is using the same master key. > 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). Yep, whether any backing file uses encryption is independant of the decision wrt encryption for this file. > 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. This feels like a policy decision that belongs in the mgmt app - indeed that situation already exists today. ie you could have created a qcow2 file that points to a /dev/mapper/BLAH that is in fact backed by a LUKS volume. > > +++ 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. Yes > > 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. Forbidding creation of legacy encryption is dealt with in a later patch. > > == 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? Yeah, good idea. > > +++ 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. You can actually hotplug the master key too, but for simplicity I expect that libvirt will just generate a AES master key for all QEMU versions >= 2.6. We need it for encrypting RBD, iSCSI, Curl passwords too. > > +++ 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. Opps :-) Regards, Daniel
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index f5bc4f2..0512256 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -341,45 +341,6 @@ static int count_contiguous_clusters_by_type(int nb_clusters, return i; } -int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num, - uint8_t *buf, int nb_sectors, bool enc, - Error **errp) -{ - union { - uint64_t ll[2]; - uint8_t b[16]; - } ivec; - int i; - int ret; - - for(i = 0; i < nb_sectors; i++) { - ivec.ll[0] = cpu_to_le64(sector_num); - ivec.ll[1] = 0; - if (qcrypto_cipher_setiv(s->cipher, - ivec.b, G_N_ELEMENTS(ivec.b), - errp) < 0) { - return -1; - } - if (enc) { - ret = qcrypto_cipher_encrypt(s->cipher, - buf, buf, - 512, - errp); - } else { - ret = qcrypto_cipher_decrypt(s->cipher, - buf, buf, - 512, - errp); - } - if (ret < 0) { - return -1; - } - sector_num++; - buf += 512; - } - return 0; -} - static int coroutine_fn copy_sectors(BlockDriverState *bs, uint64_t start_sect, uint64_t cluster_offset, @@ -421,10 +382,9 @@ static int coroutine_fn copy_sectors(BlockDriverState *bs, if (bs->encrypted) { Error *err = NULL; - assert(s->cipher); - if (qcow2_encrypt_sectors(s, start_sect + n_start, - iov.iov_base, n, - true, &err) < 0) { + assert(s->crypto); + if (qcrypto_block_encrypt(s->crypto, start_sect + n_start, + iov.iov_base, n, &err) < 0) { ret = -EIO; error_free(err); goto out; diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index af493f8..d34d59b 100644 --- a/block/qcow2-refcount.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; + } + } + 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 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) { + error_setg(errp, "Request for data outside of extension header"); + return -1; + } + + 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); + + ret = qcow2_alloc_clusters(bs, s->crypto_header.length); + if (ret < 0) { + s->crypto_header.length = 0; + 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"); + 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) { 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); + + 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 */ { @@ -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, + Error **errp) +{ + OptsVisitor *ov; + QCryptoBlockOpenOptions *ret; + Error *local_err = NULL; + + ret = g_new0(QCryptoBlockOpenOptions, 1); + ret->format = format; + + ov = opts_visitor_new(opts); + + switch (format) { + case Q_CRYPTO_BLOCK_FORMAT_QCOW: + ret->u.qcow = g_new0(QCryptoBlockOptionsQCow, 1); + visit_type_QCryptoBlockOptionsQCow(opts_get_visitor(ov), + &ret->u.qcow, + "qcow", &local_err); + break; + + case Q_CRYPTO_BLOCK_FORMAT_LUKS: + ret->u.luks = g_new0(QCryptoBlockOptionsLUKS, 1); + visit_type_QCryptoBlockOptionsLUKS(opts_get_visitor(ov), + &ret->u.luks, + "luks", &local_err); + 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 * +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); + if (local_err) { + goto cleanup; + } + + switch (format) { + case Q_CRYPTO_BLOCK_FORMAT_QCOW: + ret->u.qcow = g_new0(QCryptoBlockOptionsQCow, 1); + visit_type_QCryptoBlockOptionsQCow(v, &ret->u.qcow, + "qcow", &local_err); + break; + + case Q_CRYPTO_BLOCK_FORMAT_LUKS: + ret->u.luks = g_new0(QCryptoBlockCreateOptionsLUKS, 1); + visit_type_QCryptoBlockCreateOptionsLUKS(v, &ret->u.luks, + "luks", &local_err); + break; + + default: + error_setg(&local_err, "Unsupported block format %d", format); + break; + } + + visit_end_struct(v, &end_err); + if (end_err) { + if (!local_err) { + error_propagate(&local_err, end_err); + } else { + error_free(end_err); + } + } + + cleanup: + if (local_err) { + error_propagate(errp, local_err); + qapi_free_QCryptoBlockCreateOptions(ret); + ret = NULL; + return NULL; + } + + opts_visitor_cleanup(ov); + return ret; +} + + typedef struct Qcow2ReopenState { Qcow2Cache *l2_table_cache; Qcow2Cache *refcount_block_cache; @@ -596,6 +812,7 @@ typedef struct Qcow2ReopenState { int overlap_check; bool discard_passthrough[QCOW2_DISCARD_MAX]; uint64_t cache_clean_interval; + QCryptoBlockOpenOptions *crypto_opts; /* Disk encryption runtime options */ } Qcow2ReopenState; static int qcow2_update_options_prepare(BlockDriverState *bs, @@ -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; + + case QCOW_CRYPT_AES: + r->crypto_opts = qcow2_crypto_open_opts_init( + Q_CRYPTO_BLOCK_FORMAT_QCOW, opts, errp); + break; + + case QCOW_CRYPT_LUKS: + r->crypto_opts = qcow2_crypto_open_opts_init( + Q_CRYPTO_BLOCK_FORMAT_LUKS, opts, errp); + break; + + default: + g_assert_not_reached(); + } + if (s->crypt_method_header && !r->crypto_opts) { + ret = -EINVAL; + goto fail; + } + ret = 0; fail: qemu_opts_del(opts); @@ -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); } static int qcow2_update_options(BlockDriverState *bs, QDict *options, @@ -932,7 +1175,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) { void *feature_table = NULL; qcow2_read_extensions(bs, header.header_length, ext_end, - &feature_table, NULL); + &feature_table, flags, NULL); report_unsupported_feature(bs, errp, feature_table, s->incompatible_features & ~QCOW2_INCOMPAT_MASK); @@ -964,20 +1207,10 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, s->refcount_max = UINT64_C(1) << (s->refcount_bits - 1); s->refcount_max += s->refcount_max - 1; - if (header.crypt_method > QCOW_CRYPT_AES) { - error_setg(errp, "Unsupported encryption method: %" PRIu32, - header.crypt_method); - ret = -EINVAL; - goto fail; - } - if (!qcrypto_cipher_supports(QCRYPTO_CIPHER_ALG_AES_128)) { - error_setg(errp, "AES cipher not available"); - ret = -EINVAL; - goto fail; - } s->crypt_method_header = header.crypt_method; if (s->crypt_method_header) { bs->encrypted = 1; + bs->valid_key = 1; } s->l2_bits = s->cluster_bits - 3; /* L2 is always one cluster */ @@ -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 + * 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) { + if (s->crypt_method_header == QCOW_CRYPT_AES) { + unsigned int cflags = 0; + if (flags & BDRV_O_NO_IO) { + cflags |= QCRYPTO_BLOCK_OPEN_NO_IO; + } + s->crypto = qcrypto_block_open(s->crypto_opts, NULL, NULL, + cflags, errp); + if (!s->crypto) { + error_setg(errp, "Could not setup encryption layer"); + ret = -EINVAL; + goto fail; + } + } else if (!(flags & BDRV_O_NO_IO)) { + error_setg(errp, "Missing CRYPTO header for crypt method %d", + s->crypt_method_header); + ret = -EINVAL; + goto fail; + } + } + /* read the backing file name */ if (header.backing_file_offset != 0) { len = header.backing_file_size; @@ -1199,41 +1457,6 @@ static void qcow2_refresh_limits(BlockDriverState *bs, Error **errp) bs->bl.write_zeroes_alignment = s->cluster_sectors; } -static int qcow2_set_key(BlockDriverState *bs, const char *key) -{ - BDRVQcow2State *s = bs->opaque; - uint8_t keybuf[16]; - int len, i; - Error *err = NULL; - - memset(keybuf, 0, 16); - len = strlen(key); - if (len > 16) - len = 16; - /* XXX: we could compress the chars to 7 bits to increase - entropy */ - for(i = 0;i < len;i++) { - keybuf[i] = key[i]; - } - assert(bs->encrypted); - - qcrypto_cipher_free(s->cipher); - s->cipher = qcrypto_cipher_new( - QCRYPTO_CIPHER_ALG_AES_128, - QCRYPTO_CIPHER_MODE_CBC, - keybuf, G_N_ELEMENTS(keybuf), - &err); - - if (!s->cipher) { - /* XXX would be nice if errors in this method could - * be properly propagate to the caller. Would need - * the bdrv_set_key() API signature to be fixed. */ - error_free(err); - return -1; - } - return 0; -} - static int qcow2_reopen_prepare(BDRVReopenState *state, BlockReopenQueue *queue, Error **errp) { @@ -1345,7 +1568,7 @@ static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs, } if (cluster_offset != 0 && ret != QCOW2_CLUSTER_COMPRESSED && - !s->cipher) { + !s->crypto) { index_in_cluster = sector_num & (s->cluster_sectors - 1); cluster_offset |= (index_in_cluster << BDRV_SECTOR_BITS); status |= BDRV_BLOCK_OFFSET_VALID | cluster_offset; @@ -1395,7 +1618,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num, /* prepare next request */ cur_nr_sectors = remaining_sectors; - if (s->cipher) { + if (s->crypto) { cur_nr_sectors = MIN(cur_nr_sectors, QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors); } @@ -1467,7 +1690,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num, } if (bs->encrypted) { - assert(s->cipher); + assert(s->crypto); /* * For encrypted images, read everything into a temporary @@ -1501,11 +1724,10 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num, goto fail; } if (bs->encrypted) { - assert(s->cipher); + assert(s->crypto); Error *err = NULL; - if (qcow2_encrypt_sectors(s, sector_num, cluster_data, - cur_nr_sectors, false, - &err) < 0) { + if (qcrypto_block_decrypt(s->crypto, sector_num, cluster_data, + cur_nr_sectors, &err) < 0) { error_free(err); ret = -EIO; goto fail; @@ -1588,7 +1810,7 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, if (bs->encrypted) { Error *err = NULL; - assert(s->cipher); + assert(s->crypto); if (!cluster_data) { cluster_data = qemu_try_blockalign(bs->file->bs, QCOW_MAX_CRYPT_CLUSTERS @@ -1603,8 +1825,8 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs, QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size); qemu_iovec_to_buf(&hd_qiov, 0, cluster_data, hd_qiov.size); - if (qcow2_encrypt_sectors(s, sector_num, cluster_data, - cur_nr_sectors, true, &err) < 0) { + if (qcrypto_block_encrypt(s->crypto, sector_num, cluster_data, + cur_nr_sectors, &err) < 0) { error_free(err); ret = -EIO; goto fail; @@ -1715,8 +1937,8 @@ static void qcow2_close(BlockDriverState *bs) qcow2_cache_destroy(bs, s->l2_table_cache); qcow2_cache_destroy(bs, s->refcount_block_cache); - qcrypto_cipher_free(s->cipher); - s->cipher = NULL; + qcrypto_block_free(s->crypto); + s->crypto = NULL; g_free(s->unknown_header_fields); cleanup_unknown_header_ext(bs); @@ -1734,7 +1956,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp) { BDRVQcow2State *s = bs->opaque; int flags = s->flags; - QCryptoCipher *cipher = NULL; + QCryptoBlock *crypto = NULL; QDict *options; Error *local_err = NULL; int ret; @@ -1744,8 +1966,8 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp) * that means we don't have to worry about reopening them here. */ - cipher = s->cipher; - s->cipher = NULL; + crypto = s->crypto; + s->crypto = NULL; qcow2_close(bs); @@ -1769,7 +1991,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp) return; } - s->cipher = cipher; + s->crypto = crypto; } static size_t header_ext_add(char *buf, uint32_t magic, const void *s, @@ -1892,6 +2114,22 @@ int qcow2_update_header(BlockDriverState *bs) buflen -= ret; } + /* Full disk encryption header pointer extension */ + if (s->crypto_header.offset != 0) { + cpu_to_be64(s->crypto_header.offset); + cpu_to_be64(s->crypto_header.length); + ret = header_ext_add(buf, QCOW2_EXT_MAGIC_CRYPTO_HEADER, + &s->crypto_header, sizeof(s->crypto_header), + buflen); + be64_to_cpu(s->crypto_header.offset); + be64_to_cpu(s->crypto_header.length); + if (ret < 0) { + goto fail; + } + buf += ret; + buflen -= ret; + } + /* Feature table */ Qcow2Feature features[] = { { @@ -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); + ret = -EINVAL; + goto out; + } + } else { + cryptoopts = qcow2_crypto_create_opts_init( + Q_CRYPTO_BLOCK_FORMAT_LUKS, opts, errp); + if (!cryptoopts) { + ret = -EINVAL; + goto out; + } + } + switch (cryptoopts->format) { + case Q_CRYPTO_BLOCK_FORMAT_QCOW: + s->crypt_method_header = QCOW_CRYPT_AES; + break; + case Q_CRYPTO_BLOCK_FORMAT_LUKS: + s->crypt_method_header = QCOW_CRYPT_LUKS; + break; + default: + error_setg(errp, "Unsupported crypto format %s", cryptostr); + ret = -EINVAL; + goto out; + } + + crypto = qcrypto_block_create(cryptoopts, + qcow2_crypto_hdr_init_func, + qcow2_crypto_hdr_write_func, + bs, errp); + if (!crypto) { + ret = -EINVAL; + goto out; + } + + ret = qcow2_update_header(bs); + if (ret < 0) { + error_setg_errno(errp, -ret, "Could not write encryption header"); + goto out; + } + + out: + qapi_free_QCryptoBlockCreateOptions(cryptoopts); + return ret; +} + + static int preallocate(BlockDriverState *bs) { uint64_t nb_sectors; @@ -2177,11 +2486,8 @@ static int qcow2_create2(const char *filename, int64_t total_size, .header_length = cpu_to_be32(sizeof(*header)), }; - if (flags & BLOCK_FLAG_ENCRYPT) { - header->crypt_method = cpu_to_be32(QCOW_CRYPT_AES); - } else { - header->crypt_method = cpu_to_be32(QCOW_CRYPT_NONE); - } + /* We'll update this to correct value later */ + header->crypt_method = cpu_to_be32(QCOW_CRYPT_NONE); if (flags & BLOCK_FLAG_LAZY_REFCOUNTS) { header->compatible_features |= @@ -2252,6 +2558,14 @@ static int qcow2_create2(const char *filename, int64_t total_size, } } + /* Want encryption? There you go. */ + if (flags & BLOCK_FLAG_ENCRYPT) { + ret = qcow2_change_encryption(bs, opts, errp); + if (ret < 0) { + goto out; + } + } + /* And if we're supposed to preallocate metadata, do that now */ if (prealloc != PREALLOC_MODE_OFF) { BDRVQcow2State *s = bs->opaque; @@ -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 + * 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 */ &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"); return -ENOTSUP; } @@ -3284,6 +3604,41 @@ static QemuOptsList qcow2_create_opts = { .help = "Width of a reference count entry in bits", .def_value_str = "16" }, + { + .name = QCOW2_OPT_CRYPTO_FORMAT, + .type = QEMU_OPT_STRING, + .help = "Encryption format, 'luks' (default), 'qcow' (deprecated)", + }, + { + .name = QCOW2_OPT_KEY_SECRET, + .type = QEMU_OPT_STRING, + .help = "ID of the secret that provides the encryption key", + }, + { + .name = QCOW2_OPT_CIPHER_ALG, + .type = QEMU_OPT_STRING, + .help = "Name of encryption cipher algorithm", + }, + { + .name = QCOW2_OPT_CIPHER_MODE, + .type = QEMU_OPT_STRING, + .help = "Name of encryption cipher mode", + }, + { + .name = QCOW2_OPT_IVGEN_ALG, + .type = QEMU_OPT_STRING, + .help = "Name of IV generator algorithm", + }, + { + .name = QCOW2_OPT_IVGEN_HASH_ALG, + .type = QEMU_OPT_STRING, + .help = "Name of IV generator hash algorithm", + }, + { + .name = QCOW2_OPT_HASH_ALG, + .type = QEMU_OPT_STRING, + .help = "Name of encryption hash algorithm", + }, { /* end of list */ } } }; @@ -3301,7 +3656,6 @@ BlockDriver bdrv_qcow2 = { .bdrv_create = qcow2_create, .bdrv_has_zero_init = bdrv_has_zero_init_1, .bdrv_co_get_block_status = qcow2_co_get_block_status, - .bdrv_set_key = qcow2_set_key, .bdrv_co_readv = qcow2_co_readv, .bdrv_co_writev = qcow2_co_writev, diff --git a/block/qcow2.h b/block/qcow2.h index ae04285..e1f0b5b 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -25,7 +25,7 @@ #ifndef BLOCK_QCOW2_H #define BLOCK_QCOW2_H -#include "crypto/cipher.h" +#include "crypto/block.h" #include "qemu/coroutine.h" //#define DEBUG_ALLOC @@ -36,6 +36,7 @@ #define QCOW_CRYPT_NONE 0 #define QCOW_CRYPT_AES 1 +#define QCOW_CRYPT_LUKS 2 #define QCOW_MAX_CRYPT_CLUSTERS 32 #define QCOW_MAX_SNAPSHOTS 65536 @@ -98,6 +99,15 @@ #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size" #define QCOW2_OPT_CACHE_CLEAN_INTERVAL "cache-clean-interval" +#define QCOW2_OPT_CRYPTO_FORMAT "encryption-format" +#define QCOW2_OPT_KEY_SECRET "key-secret" +#define QCOW2_OPT_CIPHER_ALG "cipher-alg" +#define QCOW2_OPT_CIPHER_MODE "cipher-mode" +#define QCOW2_OPT_IVGEN_ALG "ivgen-alg" +#define QCOW2_OPT_IVGEN_HASH_ALG "ivgen-hash-alg" +#define QCOW2_OPT_HASH_ALG "hash-alg" + + typedef struct QCowHeader { uint32_t magic; uint32_t version; @@ -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; 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, 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 @@ -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 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 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. + +While the header extension allocates the length as a multiple of the +cluster size, the encryption header may not consume the full permitted +allocation. + +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. + +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 + + +-----------------------------+ + | QCow2 header | + +-----------------------------+ + | QCow2 header extension X | + +-----------------------------+ + | QCow2 header extension FDE | + +-----------------------------+ + | QCow2 header extension ... | + +-----------------------------+ + | QCow2 header extension Z | + +-----------------------------+ + | ....other QCow2 tables.... | + . . + . . + +-----------------------------+ + | +-------------------------+ | + | | LUKS partition header | | + | +-------------------------+ | + | | LUKS key material 1 | | + | +-------------------------+ | + | | LUKS key material 2 | | + | +-------------------------+ | + | | LUKS key material ... | | + | +-------------------------+ | + | | LUKS key material 8 | | + | +-------------------------+ | + +-----------------------------+ + | QCow2 cluster payload | + . . + . . + . . + | | + +-----------------------------+ + == Host cluster management == qcow2 manages the allocation of host clusters by maintaining a reference count diff --git a/qapi/block-core.json b/qapi/block-core.json index a194658..b704dd7 100644 --- a/qapi/block-core.json +++ 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) # # 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 +++ 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 == 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) Testing: create -f qcow2 -o ? TEST_DIR/t.qcow2 128M @@ -66,6 +73,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) Testing: create -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 128M @@ -79,6 +93,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) Testing: create -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 128M @@ -92,6 +113,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) Testing: create -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 128M @@ -105,6 +133,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) Testing: create -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 128M @@ -118,6 +153,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) Testing: create -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 128M @@ -131,6 +173,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) Testing: create -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 128M @@ -144,6 +193,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) Testing: create -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 128M @@ -172,6 +228,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 Testing: create -o help Supported options: @@ -234,6 +297,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) Testing: convert -O qcow2 -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base @@ -247,6 +317,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) Testing: convert -O qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base @@ -260,6 +337,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) Testing: convert -O qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base @@ -273,6 +357,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) Testing: convert -O qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base @@ -286,6 +377,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) Testing: convert -O qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base @@ -299,6 +397,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) Testing: convert -O qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base @@ -312,6 +417,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) Testing: convert -O qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base @@ -325,6 +437,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) Testing: convert -O qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.base @@ -353,6 +472,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 Testing: convert -o help Supported options: @@ -412,6 +538,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) Testing: amend -f qcow2 -o ? TEST_DIR/t.qcow2 @@ -425,6 +558,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) Testing: amend -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2 @@ -438,6 +578,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) Testing: amend -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2 @@ -451,6 +598,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) Testing: amend -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2 @@ -464,6 +618,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) Testing: amend -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2 @@ -477,6 +638,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) Testing: amend -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2 @@ -490,6 +658,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) Testing: amend -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2 @@ -503,6 +678,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) Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2 @@ -533,6 +715,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 Testing: convert -o help Supported options: diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087 index af44299..3386668 100755 --- a/tests/qemu-iotests/087 +++ 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" } } } @@ -204,6 +214,15 @@ EOF run_qemu <<EOF { "execute": "qmp_capabilities" } +{ "execute": "object-add", + "arguments": { + "qom-type": "secret", + "id": "sec0", + "props": { + "data": "123456" + } + } +} { "execute": "blockdev-add", "arguments": { "options": { @@ -212,7 +231,8 @@ run_qemu <<EOF "file": { "driver": "file", "filename": "$TEST_IMG" - } + }, + "key-secret": "sec0" } } } @@ -223,7 +243,7 @@ echo echo === Missing driver === 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": "blockdev-add", diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out index 7d62cd5..5fc4823 100644 --- a/tests/qemu-iotests/087.out +++ b/tests/qemu-iotests/087.out @@ -41,13 +41,11 @@ QMP_VERSION 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.IMGFMT', fmt=IMGFMT size=134217728 encryption=on +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on key-secret=sec0 Testing: -S QMP_VERSION {"return": {}} +{"return": {}} 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. @@ -58,6 +56,7 @@ You can use 'qemu-img convert' to convert your image to an unencrypted one. Testing: QMP_VERSION {"return": {}} +{"return": {}} 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. @@ -71,10 +70,7 @@ 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. -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.IMGFMT', fmt=IMGFMT size=134217728 encryption=on +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on key-secret=sec0 Testing: -S QMP_VERSION {"return": {}} diff --git a/tests/qemu-iotests/134 b/tests/qemu-iotests/134 index 1c3820b..e0d634e 100755 --- a/tests/qemu-iotests/134 +++ b/tests/qemu-iotests/134 @@ -44,23 +44,31 @@ _supported_os Linux size=128M -IMGOPTS="encryption=on" _make_test_img $size + +SECRET="secret,id=sec0,data=astrochicken" +SECRETALT="secret,id=sec0,data=platypus" + +_make_test_img --object $SECRET -o "encryption=on,key-secret=sec0" $size + +IMGSPEC="driver=$IMGFMT,file=$TEST_IMG,key-secret=sec0" + +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT echo echo "== reading whole image ==" -echo "astrochicken" | $QEMU_IO -c "read 0 $size" "$TEST_IMG" | _filter_qemu_io | _filter_testdir +$QEMU_IO --object $SECRET --image-opts -c "read 0 $size" $IMGSPEC | _filter_qemu_io | _filter_testdir echo echo "== rewriting whole image ==" -echo "astrochicken" | $QEMU_IO -c "write -P 0xa 0 $size" "$TEST_IMG" | _filter_qemu_io | _filter_testdir +$QEMU_IO --object $SECRET --image-opts -c "write -P 0xa 0 $size" $IMGSPEC | _filter_qemu_io | _filter_testdir echo echo "== verify pattern ==" -echo "astrochicken" | $QEMU_IO -c "read -P 0xa 0 $size" "$TEST_IMG" | _filter_qemu_io | _filter_testdir +$QEMU_IO --object $SECRET --image-opts -c "read -P 0xa 0 $size" $IMGSPEC | _filter_qemu_io | _filter_testdir echo echo "== verify pattern failure with wrong password ==" -echo "platypus" | $QEMU_IO -c "read -P 0xa 0 $size" "$TEST_IMG" | _filter_qemu_io | _filter_testdir +$QEMU_IO --object $SECRETALT --image-opts -c "read -P 0xa 0 $size" $IMGSPEC | _filter_qemu_io | _filter_testdir # success, all done diff --git a/tests/qemu-iotests/134.out b/tests/qemu-iotests/134.out index a16acb8..e9bf302 100644 --- a/tests/qemu-iotests/134.out +++ b/tests/qemu-iotests/134.out @@ -2,17 +2,12 @@ QA output created by 134 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.IMGFMT', fmt=IMGFMT size=134217728 encryption=on +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on key-secret=sec0 == reading whole image == 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. -Disk image 'TEST_DIR/t.qcow2' is encrypted. -password: read 134217728/134217728 bytes at offset 0 128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) @@ -20,8 +15,6 @@ read 134217728/134217728 bytes at offset 0 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. -Disk image 'TEST_DIR/t.qcow2' is encrypted. -password: wrote 134217728/134217728 bytes at offset 0 128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) @@ -29,18 +22,10 @@ wrote 134217728/134217728 bytes at offset 0 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. -Disk image 'TEST_DIR/t.qcow2' is encrypted. -password: read 134217728/134217728 bytes at offset 0 128 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == verify pattern failure with wrong password == -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. -Disk image 'TEST_DIR/t.qcow2' is encrypted. -password: -Pattern verification failed at offset 0, 134217728 bytes -read 134217728/134217728 bytes at offset 0 -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' *** done diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common index ff84f4b..f65d04c 100644 --- a/tests/qemu-iotests/common +++ b/tests/qemu-iotests/common @@ -398,6 +398,7 @@ BEGIN { for (t='$start'; t<='$end'; t++) printf "%03d\n",t }' \ done # Set qemu-io cache mode with $CACHEMODE we have +QEMU_IO_OPTIONS_NO_FMT="$QEMU_IO_OPTIONS --cache $CACHEMODE" QEMU_IO_OPTIONS="$QEMU_IO_OPTIONS -f $IMGFMT --cache $CACHEMODE" # Set default options for qemu-img create -o if they were not specified
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. With the LUKS format it is neccessary to store the LUKS 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 (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 Results in creation of an image using the LUKS format. For compatibility the old qcow2 AES format can still be used via the 'encryption-format' parameter which accepts the values 'luks' or 'qcowaes'. # 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 <berrange@redhat.com> --- 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 +- 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(-)