On Mon, Jun 21, 2021 at 1:27 PM Daniel P. Berrangé <berra...@redhat.com> wrote: > > On Mon, Jun 21, 2021 at 01:23:46PM +0200, Ilya Dryomov wrote: > > On Mon, Jun 21, 2021 at 1:04 PM Daniel P. Berrangé <berra...@redhat.com> > > wrote: > > > > > > On Mon, Jun 21, 2021 at 12:59:37PM +0200, Ilya Dryomov wrote: > > > > On Mon, Jun 21, 2021 at 10:32 AM Daniel P. Berrangé > > > > <berra...@redhat.com> wrote: > > > > > > > > > > On Sat, Jun 19, 2021 at 09:44:32PM +0200, Ilya Dryomov wrote: > > > > > > On Thu, Jun 17, 2021 at 6:05 PM Or Ozeri <o...@il.ibm.com> wrote: > > > > > > > > > > > > > > Starting from ceph Pacific, RBD has built-in support for > > > > > > > image-level encryption. > > > > > > > Currently supported formats are LUKS version 1 and 2. > > > > > > > > > > > > > > There are 2 new relevant librbd APIs for controlling encryption, > > > > > > > both expect an > > > > > > > open image context: > > > > > > > > > > > > > > rbd_encryption_format: formats an image (i.e. writes the LUKS > > > > > > > header) > > > > > > > rbd_encryption_load: loads encryptor/decryptor to the image IO > > > > > > > stack > > > > > > > > > > > > > > This commit extends the qemu rbd driver API to support the above. > > > > > > > > > > > > > > Signed-off-by: Or Ozeri <o...@il.ibm.com> > > > > > > > --- > > > > > > > block/raw-format.c | 7 + > > > > > > > block/rbd.c | 371 > > > > > > > ++++++++++++++++++++++++++++++++++++++++++- > > > > > > > qapi/block-core.json | 110 ++++++++++++- > > > > > > > 3 files changed, 482 insertions(+), 6 deletions(-) > > > > > > > > > > > > > > > > > diff --git a/block/rbd.c b/block/rbd.c > > > > > > > index f098a89c7b..183b17cd84 100644 > > > > > > > --- a/block/rbd.c > > > > > > > +++ b/block/rbd.c > > > > > > > @@ -73,6 +73,18 @@ > > > > > > > #define LIBRBD_USE_IOVEC 0 > > > > > > > #endif > > > > > > > > > > > > > > +#define RBD_ENCRYPTION_LUKS_HEADER_VERIFICATION_LEN 8 > > > > > > > + > > > > > > > +static const char rbd_luks_header_verification[ > > > > > > > + RBD_ENCRYPTION_LUKS_HEADER_VERIFICATION_LEN] = { > > > > > > > + 'L', 'U', 'K', 'S', 0xBA, 0xBE, 0, 1 > > > > > > > +}; > > > > > > > + > > > > > > > +static const char rbd_luks2_header_verification[ > > > > > > > + RBD_ENCRYPTION_LUKS_HEADER_VERIFICATION_LEN] = { > > > > > > > + 'L', 'U', 'K', 'S', 0xBA, 0xBE, 0, 2 > > > > > > > +}; > > > > > > > + > > > > > > > typedef enum { > > > > > > > RBD_AIO_READ, > > > > > > > RBD_AIO_WRITE, > > > > > > > @@ -341,6 +353,206 @@ static void qemu_rbd_memset(RADOSCB *rcb, > > > > > > > int64_t offs) > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > +#ifdef LIBRBD_SUPPORTS_ENCRYPTION > > > > > > > +static int qemu_rbd_convert_luks_options( > > > > > > > + RbdEncryptionOptionsLUKSBase *luks_opts, > > > > > > > + char **passphrase, > > > > > > > + Error **errp) > > > > > > > +{ > > > > > > > + int r = 0; > > > > > > > + > > > > > > > + if (!luks_opts->has_key_secret) { > > > > > > > + r = -EINVAL; > > > > > > > + error_setg_errno(errp, -r, "missing encrypt.key-secret"); > > > > > > > + return r; > > > > > > > + } > > > > > > > > > > > > Why is key-secret optional? > > > > > > > > > > It doesn't look like it is handled correctly here, but we need to > > > > > be able to run 'qemu-img info <volume>' and get information back > > > > > on the size of the image, and whether or not it is encrypted, > > > > > without having to supply a passphrase upfront. So it is right that > > > > > key-secret be optional, but also we shouldn't return an fatal > > > > > error like this. > > > > > > > > Hi Daniel, > > > > > > > > The key-secret lives inside RbdEncryptionOptions (or > > > > RbdEncryptionCreateOptions) which are already optional: > > > > > > > > '*encrypt': 'RbdEncryptionOptions' > > > > > > > > '*encrypt' : 'RbdEncryptionCreateOptions' > > > > > > > > The image is opened as usual and then, if "encrypt" is specified, > > > > the encryption profile is loaded (or created and laid down). It does > > > > not make sense to attempt to load or create the encryption profile > > > > without the passphrase -- it would always fail. > > > > > > Ah, that sounds like it is probably ok then. > > > > > > > > > > > Only if BDRV_O_NO_IO is NOT set, should this error be reported > > > > > > > > > > > > > > > > > > > > > > > > > > > static int64_t qemu_rbd_getlength(BlockDriverState *bs) > > > > > > > { > > > > > > > BDRVRBDState *s = bs->opaque; > > > > > > > @@ -1243,6 +1589,22 @@ static QemuOptsList qemu_rbd_create_opts = > > > > > > > { > > > > > > > .type = QEMU_OPT_STRING, > > > > > > > .help = "ID of secret providing the password", > > > > > > > }, > > > > > > > + { > > > > > > > + .name = "encrypt.format", > > > > > > > + .type = QEMU_OPT_STRING, > > > > > > > + .help = "Encrypt the image, format choices: 'luks', > > > > > > > 'luks2'", > > > > > > > > > > > > I think it should be "luks1" and "luks2" to match rbd/librbd.h and > > > > > > "rbd encryption format" command. > > > > > > > > > > No, it should stay "luks" not "luks1", to match the existing QEMU > > > > > terminology for its LUKS v1 encryption support. > > > > > > > > If you insist on following the QEMU nomenclature here it's fine with > > > > me but I want to point out that encryption-formatted clones won't be > > > > interoperable with QEMU LUKS driver or dm-crypt so making the names > > > > match QEMU instead of librbd and rbd CLI seems a bit misleading. > > > > > > In what way is it not interoperable ? > > > > > > If we don't specify any 'encrypt' option, does the guest see the > > > raw LUKS header + payload, or is the header completely hidden > > > and only ever accessible RBD ? > > > > I think it would see the LUKS header but wouldn't be able to open the > > LUKS container or do anything else that requires the passphrase. > > Hmm, that probably means that QEMU's existing general "luks" driver > could be layered on top of an encrypted RBD volume too, at least for > a v1 format. QEMU doesn't support the v2 format at this time.
Yes, as long as the image is not an encryption-formatted clone. This was one of the design goals. > > I wonder what the tradeoffs are in choosing between RBD's builtin > LUKS vs QEMU's LUKS driver. Thin-provisioned COW clones: getting rid of a major obstacle that an RBD clone must be encrypted with same key as its parent. The only way to avoid that today is to basically replace a thin clone with a thick copy. > > RBD's builtin LUKS will do decrypt on the client host I presume ? > Is it a pure userspace impl in terms of crypto ? Yes, on the client. Pure userspace (openssl) at this point. Thanks, Ilya