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

[Ilya Dryomov]

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.

"qemu-img info" can get the size, etc without loading the encryption
profile (i.e. ->bdrv_get_info and ->bdrv_get_specific_info don't need
it).  But note that once the encryption profile is loaded, the size
changes because librbd subtracts the LUKS header overhead.  So

    $ qemu-img info --image-opts driver=rbd,pool=foo,image=bar

and

    $ qemu-img info --object secret,file=passphrase.txt,id=sec0
--image-opts 
driver=rbd,pool=foo,image=bar,encrypt.format=luks2,encrypt.key-secret=sec0

would give different results.

>
> 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.

>
>
> > > @@ -3609,6 +3622,94 @@
> > >  { 'enum': 'RbdAuthMode',
> > >    'data': [ 'cephx', 'none' ] }
> > >
> > > +##
> > > +# @RbdImageEncryptionFormat:
> > > +#
> > > +# Since: 6.1
> > > +##
> > > +{ 'enum': 'RbdImageEncryptionFormat',
> > > +  'data': [ 'luks', 'luks2' ] }
> >
> > Ditto -- "luks1" and "luks2".
>
> No, the patch is correct as is.
>
> > > +# @RbdEncryptionOptionsLUKSBase:
> > > +#
> > > +# @key-secret: ID of a QCryptoSecret object providing a passphrase
> > > +#              for unlocking the encryption
> > > +#
> > > +# Since: 6.1
> > > +##
> > > +{ 'struct': 'RbdEncryptionOptionsLUKSBase',
> > > +  'data': { '*key-secret': 'str' }}
> >
> > When would we not need a passphrase?  I think it should be required.
>
> When running 'qemu-img info'
>
> > > +##
> > > +# @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.

Thanks,

                Ilya