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

[Ilya Dryomov]

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.

>
>
> > > > > +##
> > > > > +# @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