Re: [PATCH v2 02/14] qcrypto/luks: implement encryption key management

["Daniel P. Berrangé" <berrange@redhat.com>]

On Sun, May 03, 2020 at 11:55:35AM +0300, Maxim Levitsky wrote:
> On Tue, 2020-04-28 at 14:16 +0100, Daniel P. Berrangé wrote:
> > On Sun, Mar 08, 2020 at 05:18:51PM +0200, Maxim Levitsky wrote:
> > > Next few patches will expose that functionality
> > > to the user.
> > > 
> > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > ---
> > >  crypto/block-luks.c | 398 +++++++++++++++++++++++++++++++++++++++++++-
> > >  qapi/crypto.json    |  61 ++++++-
> > >  2 files changed, 455 insertions(+), 4 deletions(-)


> > > +/*
> > > + * Given LUKSKeyslotUpdate command, set @slots_bitmap with all slots
> > > + * that will be updated with new password (or erased)
> > > + * returns 0 on success, and -1 on failure
> > > + */
> > > +static int
> > > +qcrypto_block_luks_get_update_bitmap(QCryptoBlock *block,
> > > +                                     QCryptoBlockReadFunc readfunc,
> > > +                                     void *opaque,
> > > +                                     const QCryptoBlockAmendOptionsLUKS *opts,
> > > +                                     unsigned long *slots_bitmap,
> > > +                                     Error **errp)
> > > +{
> > > +    const QCryptoBlockLUKS *luks = block->opaque;
> > > +    size_t i;
> > > +
> > > +    bitmap_zero(slots_bitmap, QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> > > +
> > > +    if (opts->has_keyslot) {
> > > +        /* keyslot set, select only this keyslot */
> > > +        int keyslot = opts->keyslot;
> > > +
> > > +        if (keyslot < 0 || keyslot >= QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS) {
> > > +            error_setg(errp,
> > > +                       "Invalid slot %u specified, must be between 0 and %u",
> > > +                       keyslot, QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS - 1);
> > > +            return -1;
> > > +        }
> > > +        bitmap_set(slots_bitmap, keyslot, 1);
> > > +
> > > +    } else if (opts->has_old_secret) {
> > > +        /* initially select all active keyslots */
> > > +        for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> > > +            if (qcrypto_block_luks_slot_active(luks, i)) {
> > > +                bitmap_set(slots_bitmap, i, 1);
> > > +            }
> > > +        }
> > > +    } else {
> > > +        /* find a free keyslot */
> > > +        int slot = qcrypto_block_luks_find_free_keyslot(luks);
> > > +
> > > +        if (slot == -1) {
> > > +            error_setg(errp,
> > > +                       "Can't add a keyslot - all key slots are in use");
> > > +            return -1;
> > > +        }
> > > +        bitmap_set(slots_bitmap, slot, 1);
> > > +    }
> > > +
> > > +    if (opts->has_old_secret) {
> > > +        /* now deselect all keyslots that don't contain the password */
> > > +        g_autofree uint8_t *tmpkey = g_new0(uint8_t,
> > > +                                            luks->header.master_key_len);
> > > +
> > > +        for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> > > +            g_autofree char *old_password = NULL;
> > > +            int rv;
> > > +
> > > +            if (!test_bit(i, slots_bitmap)) {
> > > +                continue;
> > > +            }
> > > +
> > > +            old_password = qcrypto_secret_lookup_as_utf8(opts->old_secret,
> > > +                                                         errp);
> > > +            if (!old_password) {
> > > +                return -1;
> > > +            }
> > > +
> > > +            rv = qcrypto_block_luks_load_key(block,
> > > +                                             i,
> > > +                                             old_password,
> > > +                                             tmpkey,
> > > +                                             readfunc,
> > > +                                             opaque,
> > > +                                             errp);
> > > +            if (rv == -1) {
> > > +                return -1;
> > > +            } else if (rv == 0) {
> > > +                bitmap_clear(slots_bitmap, i, 1);
> > > +            }
> > > +        }
> > > +    }
> > > +    return 0;
> > > +}
> > 
> > I'm not really liking this function as a concept. Some of the code
> > only applies to the "add key" code path, while some of it only
> > applies to the "erase key" code path.
> > 
> > I'd prefer it if qcrypto_block_luks_erase_keys directly had the
> > required logic, likewise qcrypto_block_luks_set_keys, and thus
> > get rid of the bitmap concept entirely. I thin kit'd make the
> > logic easier to understand.
> 
> It used to be like that in former versions that I did send, I added the concept
> of the bitmap very recently to reflect the way we defined this in the spec.
> I don't mind that much coming back to older version of doing this,
> but beware that it won't be that clear either.

My view is that removing and adding keys are fundamentally different
operations, so although there's some parts that are in common, overall
it is better to keep them clearly separate.

> > > +/*
> > > + * Erase a set of keyslots given in @slots_bitmap
> > > + */
> > > +static int qcrypto_block_luks_erase_keys(QCryptoBlock *block,
> > > +                                         QCryptoBlockReadFunc readfunc,
> > > +                                         QCryptoBlockWriteFunc writefunc,
> > > +                                         void *opaque,
> > > +                                         unsigned long *slots_bitmap,
> > > +                                         bool force,
> > > +                                         Error **errp)
> > > +{
> > > +    QCryptoBlockLUKS *luks = block->opaque;
> > > +    long slot_count = bitmap_count_one(slots_bitmap,
> > > +                                       QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> > > +    size_t i;
> > > +
> > > +    /* safety checks */
> > > +    if (!force && slot_count == qcrypto_block_luks_count_active_slots(luks)) {
> > > +        error_setg(errp,
> > > +                   "Requested operation will erase all active keyslots"
> > > +                   " which will erase all the data in the image"
> > > +                   " irreversibly - refusing operation");
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    /* new apply the update */
> > > +    for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> > > +        if (!test_bit(i, slots_bitmap)) {
> > > +            continue;
> > > +        }
> > > +        if (qcrypto_block_luks_erase_key(block, i, writefunc, opaque, errp)) {
> > > +            error_append_hint(errp, "Failed to erase keyslot %zu", i);
> > > +            return -EINVAL;
> > > +        }
> > > +    }
> > > +    return 0;
> > > +}
> > > +
> > > +/*
> > > + * Set a set of keyslots to @master_key encrypted by @new_secret
> > > + */
> > > +static int qcrypto_block_luks_set_keys(QCryptoBlock *block,
> > > +                                       QCryptoBlockReadFunc readfunc,
> > > +                                       QCryptoBlockWriteFunc writefunc,
> > > +                                       void *opaque,
> > > +                                       unsigned long *slots_bitmap,
> > > +                                       uint8_t *master_key,
> > > +                                       uint64_t iter_time,
> > > +                                       char *new_secret,
> > > +                                       bool force,
> > > +                                       Error **errp)
> > 
> > I'd call this  "add_key" instead of "set_keys".  I'm also unclear why
> > we need to support setting a range of keyslots. AFAIK, adding a key
> > should only ever affect a single keyslot.
> Mostly for consistency. There is a very corner case of inline replacing
> all keys that match one password with another.

I don't see that as a use case we care about. There's no benefit to having
the same password repeated in multiple slots.

> If possible I would like to keep it this way though.

IMHO the the bitmap just needlessly complicates the code for a feature
that is irrelevant to us.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|