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. > > > > > > > +## > > > > > +# @RbdEncryptionCreateOptionsLUKSBase: > > > > > +# > > > > > +# @cipher-alg: The encryption algorithm > > > > > +# > > > > > +# Since: 6.1 > > > > > +## > > > > > +{ 'struct': 'RbdEncryptionCreateOptionsLUKSBase', > > > > > + 'base': 'RbdEncryptionOptionsLUKSBase', > > > > > + 'data': { '*cipher-alg': 'QCryptoCipherAlgorithm'}} > > > > > > > > Why QCryptoCipherAlgorithm instead of just enumerating the two > > > > algorithms that librbd supports? An early failure when parsing > > > > seems better than failing in qemu_rbd_convert_luks_create_options() > > > > and having to clean up the newly created image. > > > > > > We don't want to duplicate algorithm names that already have > > > a defined enum data type. > > > > Did you see my other comment on this? Quoting it just in case: > > > > > ... QCryptoCipherAlgorithm is a set of 12 algorithms of > > > which librbd supports only two. On top of that, e.g. "aes-256" for > > > librbd really means "aes-256" + "xts" + "plain64" -- it bundles > > > QCryptoCipherAlgorithm, QCryptoCipherMode and QCryptoIVGenAlgorithm > > > with the latter two being hard-coded. > > > > This is not a big deal, but I just don't see how confusing everyone > > who introspects the QAPI into thinking that all these algorithms are > > supported (and forgoing an early parsing failure as a side effect) is > > worth avoiding a trivial [ 'aes-128', 'aes-256' ] definition here. > > Even for the existing LUKS code in QEMU there is no guarantee that the > impl supports all the ciphers listed in the enum. You can't rely on the > introspection to that degree. I see, that makes it clearer. Thanks, Ilya