| Message ID | 20200510134037.18487-8-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LUKS: encryption slot management using amend interface | expand |
On 10.05.20 15:40, Maxim Levitsky wrote: > This implements the encryption key management using the generic code in > qcrypto layer and exposes it to the user via qemu-img > > This code adds another 'write_func' because the initialization > write_func works directly on the underlying file, and amend > works on instance of luks device. > > This commit also adds a 'hack/workaround' I and Kevin Wolf (thanks) > made to make the driver both support write sharing (to avoid breaking the users), > and be safe against concurrent metadata update (the keyslots) > > Eventually the write sharing for luks driver will be deprecated > and removed together with this hack. > > The hack is that we ask (as a format driver) for BLK_PERM_CONSISTENT_READ > and then when we want to update the keys, we unshare that permission. > So if someone else has the image open, even readonly, encryption > key update will fail gracefully. > > Also thanks to Daniel Berrange for the idea of > unsharing read, rather that write permission which allows > to avoid cases when the other user had opened the image read-only. > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > --- > block/crypto.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++-- > block/crypto.h | 34 +++++++++++++ > 2 files changed, 158 insertions(+), 3 deletions(-) > > diff --git a/block/crypto.c b/block/crypto.c > index 2e16b62bdc..b14cb0ff06 100644 > --- a/block/crypto.c > +++ b/block/crypto.c [...] > +static void > +block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c, > + const BdrvChildRole *role, > + BlockReopenQueue *reopen_queue, > + uint64_t perm, uint64_t shared, > + uint64_t *nperm, uint64_t *nshared) > +{ > + > + BlockCrypto *crypto = bs->opaque; > + > + bdrv_filter_default_perms(bs, c, role, reopen_queue, > + perm, shared, nperm, nshared); > + /* > + * Ask for consistent read permission so that if > + * someone else tries to open this image with this permission > + * neither will be able to edit encryption keys, since > + * we will unshare that permission while trying to > + * update the encryption keys > + */ > + if (!(bs->open_flags & BDRV_O_NO_IO)) { > + *nperm |= BLK_PERM_CONSISTENT_READ; > + } I’m not sure this is important, because this really means we won’t do I/O. Its only relevant use in this case is for qemu-img info. Do we really care if someone edits the key slots while qemu-img info is processing? OTOH, I don’t think it’ll harm much. Well, apart from the fact that BDRV_O_NO_IO won’t do much for LUKS images. *shrug* Reviewed-by: Max Reitz <mreitz@redhat.com>
On Thu, May 14, 2020 at 04:09:59PM +0200, Max Reitz wrote: > On 10.05.20 15:40, Maxim Levitsky wrote: > > This implements the encryption key management using the generic code in > > qcrypto layer and exposes it to the user via qemu-img > > > > This code adds another 'write_func' because the initialization > > write_func works directly on the underlying file, and amend > > works on instance of luks device. > > > > This commit also adds a 'hack/workaround' I and Kevin Wolf (thanks) > > made to make the driver both support write sharing (to avoid breaking the users), > > and be safe against concurrent metadata update (the keyslots) > > > > Eventually the write sharing for luks driver will be deprecated > > and removed together with this hack. > > > > The hack is that we ask (as a format driver) for BLK_PERM_CONSISTENT_READ > > and then when we want to update the keys, we unshare that permission. > > So if someone else has the image open, even readonly, encryption > > key update will fail gracefully. > > > > Also thanks to Daniel Berrange for the idea of > > unsharing read, rather that write permission which allows > > to avoid cases when the other user had opened the image read-only. > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > block/crypto.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++-- > > block/crypto.h | 34 +++++++++++++ > > 2 files changed, 158 insertions(+), 3 deletions(-) > > > > diff --git a/block/crypto.c b/block/crypto.c > > index 2e16b62bdc..b14cb0ff06 100644 > > --- a/block/crypto.c > > +++ b/block/crypto.c > > [...] > > > +static void > > +block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c, > > + const BdrvChildRole *role, > > + BlockReopenQueue *reopen_queue, > > + uint64_t perm, uint64_t shared, > > + uint64_t *nperm, uint64_t *nshared) > > +{ > > + > > + BlockCrypto *crypto = bs->opaque; > > + > > + bdrv_filter_default_perms(bs, c, role, reopen_queue, > > + perm, shared, nperm, nshared); > > + /* > > + * Ask for consistent read permission so that if > > + * someone else tries to open this image with this permission > > + * neither will be able to edit encryption keys, since > > + * we will unshare that permission while trying to > > + * update the encryption keys > > + */ > > + if (!(bs->open_flags & BDRV_O_NO_IO)) { > > + *nperm |= BLK_PERM_CONSISTENT_READ; > > + } > > I’m not sure this is important, because this really means we won’t do > I/O. Its only relevant use in this case is for qemu-img info. Do we > really care if someone edits the key slots while qemu-img info is > processing? FWIW, OpenStack runs qemu-img info in a periodic background job, so it can be concurrent with anything else they are running. Having said that due to previous QEMU bugs, they unconditonally pass the arg to qemu-img to explicitly disable locking Regards, Daniel
On 14.05.20 16:14, Daniel P. Berrangé wrote: > On Thu, May 14, 2020 at 04:09:59PM +0200, Max Reitz wrote: >> On 10.05.20 15:40, Maxim Levitsky wrote: >>> This implements the encryption key management using the generic code in >>> qcrypto layer and exposes it to the user via qemu-img >>> >>> This code adds another 'write_func' because the initialization >>> write_func works directly on the underlying file, and amend >>> works on instance of luks device. >>> >>> This commit also adds a 'hack/workaround' I and Kevin Wolf (thanks) >>> made to make the driver both support write sharing (to avoid breaking the users), >>> and be safe against concurrent metadata update (the keyslots) >>> >>> Eventually the write sharing for luks driver will be deprecated >>> and removed together with this hack. >>> >>> The hack is that we ask (as a format driver) for BLK_PERM_CONSISTENT_READ >>> and then when we want to update the keys, we unshare that permission. >>> So if someone else has the image open, even readonly, encryption >>> key update will fail gracefully. >>> >>> Also thanks to Daniel Berrange for the idea of >>> unsharing read, rather that write permission which allows >>> to avoid cases when the other user had opened the image read-only. >>> >>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> >>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> >>> --- >>> block/crypto.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++-- >>> block/crypto.h | 34 +++++++++++++ >>> 2 files changed, 158 insertions(+), 3 deletions(-) >>> >>> diff --git a/block/crypto.c b/block/crypto.c >>> index 2e16b62bdc..b14cb0ff06 100644 >>> --- a/block/crypto.c >>> +++ b/block/crypto.c >> >> [...] >> >>> +static void >>> +block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c, >>> + const BdrvChildRole *role, >>> + BlockReopenQueue *reopen_queue, >>> + uint64_t perm, uint64_t shared, >>> + uint64_t *nperm, uint64_t *nshared) >>> +{ >>> + >>> + BlockCrypto *crypto = bs->opaque; >>> + >>> + bdrv_filter_default_perms(bs, c, role, reopen_queue, >>> + perm, shared, nperm, nshared); >>> + /* >>> + * Ask for consistent read permission so that if >>> + * someone else tries to open this image with this permission >>> + * neither will be able to edit encryption keys, since >>> + * we will unshare that permission while trying to >>> + * update the encryption keys >>> + */ >>> + if (!(bs->open_flags & BDRV_O_NO_IO)) { >>> + *nperm |= BLK_PERM_CONSISTENT_READ; >>> + } >> >> I’m not sure this is important, because this really means we won’t do >> I/O. Its only relevant use in this case is for qemu-img info. Do we >> really care if someone edits the key slots while qemu-img info is >> processing? > > FWIW, OpenStack runs qemu-img info in a periodic background job, so > it can be concurrent with anything else they are running. That might actually be a problem then, because this may cause sporadic failure when trying to change (amend) keyslots; while qemu-img info holds the CONSISTENT_READ permission, the amend process can’t unshare it. That might lead to hard-to-track-down bugs. > Having said > that due to previous QEMU bugs, they unconditonally pass the arg to > qemu-img to explicitly disable locking Well, then it doesn’t matter in this case. But still something to consider, probably. Max
On Thu, 2020-05-14 at 16:32 +0200, Max Reitz wrote: > On 14.05.20 16:14, Daniel P. Berrangé wrote: > > On Thu, May 14, 2020 at 04:09:59PM +0200, Max Reitz wrote: > > > On 10.05.20 15:40, Maxim Levitsky wrote: > > > > This implements the encryption key management using the generic code in > > > > qcrypto layer and exposes it to the user via qemu-img > > > > > > > > This code adds another 'write_func' because the initialization > > > > write_func works directly on the underlying file, and amend > > > > works on instance of luks device. > > > > > > > > This commit also adds a 'hack/workaround' I and Kevin Wolf (thanks) > > > > made to make the driver both support write sharing (to avoid breaking the users), > > > > and be safe against concurrent metadata update (the keyslots) > > > > > > > > Eventually the write sharing for luks driver will be deprecated > > > > and removed together with this hack. > > > > > > > > The hack is that we ask (as a format driver) for BLK_PERM_CONSISTENT_READ > > > > and then when we want to update the keys, we unshare that permission. > > > > So if someone else has the image open, even readonly, encryption > > > > key update will fail gracefully. > > > > > > > > Also thanks to Daniel Berrange for the idea of > > > > unsharing read, rather that write permission which allows > > > > to avoid cases when the other user had opened the image read-only. > > > > > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > > > --- > > > > block/crypto.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++-- > > > > block/crypto.h | 34 +++++++++++++ > > > > 2 files changed, 158 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/block/crypto.c b/block/crypto.c > > > > index 2e16b62bdc..b14cb0ff06 100644 > > > > --- a/block/crypto.c > > > > +++ b/block/crypto.c > > > > > > [...] > > > > > > > +static void > > > > +block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c, > > > > + const BdrvChildRole *role, > > > > + BlockReopenQueue *reopen_queue, > > > > + uint64_t perm, uint64_t shared, > > > > + uint64_t *nperm, uint64_t *nshared) > > > > +{ > > > > + > > > > + BlockCrypto *crypto = bs->opaque; > > > > + > > > > + bdrv_filter_default_perms(bs, c, role, reopen_queue, > > > > + perm, shared, nperm, nshared); > > > > + /* > > > > + * Ask for consistent read permission so that if > > > > + * someone else tries to open this image with this permission > > > > + * neither will be able to edit encryption keys, since > > > > + * we will unshare that permission while trying to > > > > + * update the encryption keys > > > > + */ > > > > + if (!(bs->open_flags & BDRV_O_NO_IO)) { > > > > + *nperm |= BLK_PERM_CONSISTENT_READ; > > > > + } > > > > > > I’m not sure this is important, because this really means we won’t do > > > I/O. Its only relevant use in this case is for qemu-img info. Do we > > > really care if someone edits the key slots while qemu-img info is > > > processing? > > > > FWIW, OpenStack runs qemu-img info in a periodic background job, so > > it can be concurrent with anything else they are running. > > That might actually be a problem then, because this may cause sporadic > failure when trying to change (amend) keyslots; while qemu-img info > holds the CONSISTENT_READ permission, the amend process can’t unshare > it. That might lead to hard-to-track-down bugs. > > > Having said > > that due to previous QEMU bugs, they unconditonally pass the arg to > > qemu-img to explicitly disable locking > > Well, then it doesn’t matter in this case. But still something to > consider, probably. > > Max > So I understand correctly that I should leave the patch as is? Thanks for the review! Best regards, Maxim Levitsky
diff --git a/block/crypto.c b/block/crypto.c index 2e16b62bdc..b14cb0ff06 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -37,6 +37,7 @@ typedef struct BlockCrypto BlockCrypto; struct BlockCrypto { QCryptoBlock *block; + bool updating_keys; }; @@ -71,6 +72,24 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block, return ret; } +static ssize_t block_crypto_write_func(QCryptoBlock *block, + size_t offset, + const uint8_t *buf, + size_t buflen, + void *opaque, + Error **errp) +{ + BlockDriverState *bs = opaque; + ssize_t ret; + + ret = bdrv_pwrite(bs->file, offset, buf, buflen); + if (ret < 0) { + error_setg_errno(errp, -ret, "Could not write encryption header"); + return ret; + } + return ret; +} + struct BlockCryptoCreateData { BlockBackend *blk; @@ -166,6 +185,19 @@ static QemuOptsList block_crypto_create_opts_luks = { }; +static QemuOptsList block_crypto_amend_opts_luks = { + .name = "crypto", + .head = QTAILQ_HEAD_INITIALIZER(block_crypto_create_opts_luks.head), + .desc = { + BLOCK_CRYPTO_OPT_DEF_LUKS_STATE(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_KEYSLOT(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_OLD_SECRET(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_NEW_SECRET(""), + BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME(""), + { /* end of list */ } + }, +}; + QCryptoBlockOpenOptions * block_crypto_open_opts_init(QDict *opts, Error **errp) { @@ -758,6 +790,95 @@ block_crypto_get_specific_info_luks(BlockDriverState *bs, Error **errp) return spec_info; } +static int +block_crypto_amend_options_luks(BlockDriverState *bs, + QemuOpts *opts, + BlockDriverAmendStatusCB *status_cb, + void *cb_opaque, + bool force, + Error **errp) +{ + BlockCrypto *crypto = bs->opaque; + QDict *cryptoopts = NULL; + QCryptoBlockAmendOptions *amend_options = NULL; + int ret; + + assert(crypto); + assert(crypto->block); + crypto->updating_keys = true; + + ret = bdrv_child_refresh_perms(bs, bs->file, errp); + if (ret < 0) { + goto cleanup; + } + + cryptoopts = qemu_opts_to_qdict(opts, NULL); + qdict_put_str(cryptoopts, "format", "luks"); + amend_options = block_crypto_amend_opts_init(cryptoopts, errp); + if (!amend_options) { + ret = -EINVAL; + goto cleanup; + } + + ret = qcrypto_block_amend_options(crypto->block, + block_crypto_read_func, + block_crypto_write_func, + bs, + amend_options, + force, + errp); +cleanup: + crypto->updating_keys = false; + bdrv_child_refresh_perms(bs, bs->file, errp); + qapi_free_QCryptoBlockAmendOptions(amend_options); + qobject_unref(cryptoopts); + return ret; +} + + +static void +block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c, + const BdrvChildRole *role, + BlockReopenQueue *reopen_queue, + uint64_t perm, uint64_t shared, + uint64_t *nperm, uint64_t *nshared) +{ + + BlockCrypto *crypto = bs->opaque; + + bdrv_filter_default_perms(bs, c, role, reopen_queue, + perm, shared, nperm, nshared); + /* + * Ask for consistent read permission so that if + * someone else tries to open this image with this permission + * neither will be able to edit encryption keys, since + * we will unshare that permission while trying to + * update the encryption keys + */ + if (!(bs->open_flags & BDRV_O_NO_IO)) { + *nperm |= BLK_PERM_CONSISTENT_READ; + } + /* + * This driver doesn't modify LUKS metadata except + * when updating the encryption slots. + * Thus unlike a proper format driver we don't ask for + * shared write/read permission. However we need it + * when we are updating the keys, to ensure that only we + * have access to the device. + * + * Encryption update will set the crypto->updating_keys + * during that period and refresh permissions + * + */ + if (crypto->updating_keys) { + /* need exclusive write access for header update */ + *nperm |= BLK_PERM_WRITE; + /* unshare read and write permission */ + *nshared &= ~(BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE); + } +} + + static const char *const block_crypto_strong_runtime_opts[] = { BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET, @@ -770,13 +891,12 @@ static BlockDriver bdrv_crypto_luks = { .bdrv_probe = block_crypto_probe_luks, .bdrv_open = block_crypto_open_luks, .bdrv_close = block_crypto_close, - /* This driver doesn't modify LUKS metadata except when creating image. - * Allow share-rw=on as a special case. */ - .bdrv_child_perm = bdrv_filter_default_perms, + .bdrv_child_perm = block_crypto_child_perms, .bdrv_co_create = block_crypto_co_create_luks, .bdrv_co_create_opts = block_crypto_co_create_opts_luks, .bdrv_co_truncate = block_crypto_co_truncate, .create_opts = &block_crypto_create_opts_luks, + .amend_opts = &block_crypto_amend_opts_luks, .bdrv_reopen_prepare = block_crypto_reopen_prepare, .bdrv_refresh_limits = block_crypto_refresh_limits, @@ -786,6 +906,7 @@ static BlockDriver bdrv_crypto_luks = { .bdrv_measure = block_crypto_measure, .bdrv_get_info = block_crypto_get_info_luks, .bdrv_get_specific_info = block_crypto_get_specific_info_luks, + .bdrv_amend_options = block_crypto_amend_options_luks, .strong_runtime_opts = block_crypto_strong_runtime_opts, }; diff --git a/block/crypto.h b/block/crypto.h index 06e044c9be..c72c3dec61 100644 --- a/block/crypto.h +++ b/block/crypto.h @@ -41,6 +41,11 @@ #define BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG "ivgen-hash-alg" #define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg" #define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time" +#define BLOCK_CRYPTO_OPT_LUKS_KEYSLOT "keyslot" +#define BLOCK_CRYPTO_OPT_LUKS_STATE "state" +#define BLOCK_CRYPTO_OPT_LUKS_OLD_SECRET "old-secret" +#define BLOCK_CRYPTO_OPT_LUKS_NEW_SECRET "new-secret" + #define BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(prefix) \ BLOCK_CRYPTO_OPT_DEF_KEY_SECRET(prefix, \ @@ -88,6 +93,35 @@ .help = "Time to spend in PBKDF in milliseconds", \ } +#define BLOCK_CRYPTO_OPT_DEF_LUKS_STATE(prefix) \ + { \ + .name = prefix BLOCK_CRYPTO_OPT_LUKS_STATE, \ + .type = QEMU_OPT_STRING, \ + .help = "Select new state of affected keyslots (active/inactive)",\ + } + +#define BLOCK_CRYPTO_OPT_DEF_LUKS_KEYSLOT(prefix) \ + { \ + .name = prefix BLOCK_CRYPTO_OPT_LUKS_KEYSLOT, \ + .type = QEMU_OPT_NUMBER, \ + .help = "Select a single keyslot to modify explicitly",\ + } + +#define BLOCK_CRYPTO_OPT_DEF_LUKS_OLD_SECRET(prefix) \ + { \ + .name = prefix BLOCK_CRYPTO_OPT_LUKS_OLD_SECRET, \ + .type = QEMU_OPT_STRING, \ + .help = "Select all keyslots that match this password", \ + } + +#define BLOCK_CRYPTO_OPT_DEF_LUKS_NEW_SECRET(prefix) \ + { \ + .name = prefix BLOCK_CRYPTO_OPT_LUKS_NEW_SECRET, \ + .type = QEMU_OPT_STRING, \ + .help = "New secret to set in the matching keyslots. " \ + "Empty string to erase", \ + } + QCryptoBlockCreateOptions * block_crypto_create_opts_init(QDict *opts, Error **errp);