From: Ilya Dryomov <idryomov@gmail.com>
To: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: kwolf@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org,
Mykola Golub <to.my.trociny@gmail.com>, Or Ozeri <oro@il.ibm.com>,
Danny Harnik <dannyh@il.ibm.com>
Subject: Re: [PATCH] block/rbd: Add support for rbd image encryption
Date: Mon, 21 Jun 2021 13:23:46 +0200 [thread overview]
Message-ID: <CAOi1vP9_+JVJ1LoueWeLUN5cT49aHJNV+E-mF3Qe6Q9yGDQwOw@mail.gmail.com> (raw)
In-Reply-To: <YNByJ13sZxM9cv4s@redhat.com>
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
next prev parent reply other threads:[~2021-06-21 11:25 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-17 16:05 [PATCH] block/rbd: Add support for rbd image encryption Or Ozeri
2021-06-19 19:44 ` Ilya Dryomov
2021-06-20 10:35 ` Ilya Dryomov
2021-06-21 8:32 ` Daniel P. Berrangé
2021-06-21 10:59 ` Ilya Dryomov
2021-06-21 11:04 ` Daniel P. Berrangé
2021-06-21 11:23 ` Ilya Dryomov [this message]
2021-06-21 11:27 ` Daniel P. Berrangé
2021-06-21 11:52 ` Ilya Dryomov
2021-06-20 12:46 ` Or Ozeri
-- strict thread matches above, loose matches on Subject: below --
2021-06-27 8:30 Or Ozeri
2021-06-27 8:44 ` Or Ozeri
2021-06-27 11:00 ` Ilya Dryomov
2021-06-27 11:09 ` Or Ozeri
2021-06-27 11:24 ` Ilya Dryomov
2021-06-21 14:48 Or Ozeri
2021-06-26 10:44 ` Ilya Dryomov
2021-06-21 14:21 Or Ozeri
2021-06-21 14:27 ` no-reply
2021-05-05 16:48 Or Ozeri
2021-05-02 7:36 Or Ozeri
2021-05-02 14:00 ` no-reply
2021-05-04 14:46 ` Daniel P. Berrangé
2021-05-05 15:32 ` Or Ozeri
2021-05-05 15:36 ` Daniel P. Berrangé
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOi1vP9_+JVJ1LoueWeLUN5cT49aHJNV+E-mF3Qe6Q9yGDQwOw@mail.gmail.com \
--to=idryomov@gmail.com \
--cc=berrange@redhat.com \
--cc=dannyh@il.ibm.com \
--cc=kwolf@redhat.com \
--cc=oro@il.ibm.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=to.my.trociny@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).