Re: [PATCH] block/rbd: Add support for rbd image encryption

[Ilya Dryomov]

On Mon, Jun 21, 2021 at 1:27 PM Daniel P. Berrangé <berrange@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é <berrange@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é 
> > > > <berrange@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 <oro@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 <oro@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