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

[Maxim Levitsky <mlevitsk@redhat.com>]

On Thu, 2020-05-07 at 12:02 +0100, Daniel P. Berrangé wrote:
> On Tue, May 05, 2020 at 11:08:07PM +0300, 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 | 395 +++++++++++++++++++++++++++++++++++++++++++-
> >  qapi/crypto.json    |  61 ++++++-
> >  2 files changed, 452 insertions(+), 4 deletions(-)
> > 
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index 4861db810c..c108518df1 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > +/*
> > + * Erases an keyslot given its index
> > + * Returns:
> > + *    0 if the keyslot was erased successfully
> > + *   -1 if a error occurred while erasing the keyslot
> > + *
> > + */
> > +static int
> > +qcrypto_block_luks_erase_key(QCryptoBlock *block,
> > +                             unsigned int slot_idx,
> > +                             QCryptoBlockWriteFunc writefunc,
> > +                             void *opaque,
> > +                             Error **errp)
> > +{
> > +    QCryptoBlockLUKS *luks = block->opaque;
> > +    QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx];
> > +    g_autofree uint8_t *garbagesplitkey = NULL;
> > +    size_t splitkeylen = luks->header.master_key_len * slot->stripes;
> > +    size_t i;
> > +    Error *local_err = NULL;
> > +    int ret;
> > +
> > +    assert(slot_idx < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> > +    assert(splitkeylen > 0);
> > +    garbagesplitkey = g_new0(uint8_t, splitkeylen);
> > +
> > +    /* Reset the key slot header */
> > +    memset(slot->salt, 0, QCRYPTO_BLOCK_LUKS_SALT_LEN);
> > +    slot->iterations = 0;
> > +    slot->active = QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> > +
> > +    ret = qcrypto_block_luks_store_header(block,  writefunc,
> > +                                          opaque, &local_err);
> > +
> > +    if (ret) {
> 
> ret < 0
Fixed.
> 
> > +        error_propagate(errp, local_err);
> > +    }
> > +    /*
> > +     * Now try to erase the key material, even if the header
> > +     * update failed
> > +     */
> > +    for (i = 0; i < QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS; i++) {
> > +        if (qcrypto_random_bytes(garbagesplitkey,
> > +                                 splitkeylen, &local_err) < 0) {
> > +            /*
> > +             * If we failed to get the random data, still write
> > +             * at least zeros to the key slot at least once
> > +             */
> > +            error_propagate(errp, local_err);
> > +
> > +            if (i > 0) {
> > +                return -1;
> > +            }
> > +        }
> > +        if (writefunc(block,
> > +                      slot->key_offset_sector * QCRYPTO_BLOCK_LUKS_SECTOR_SIZE,
> > +                      garbagesplitkey,
> > +                      splitkeylen,
> > +                      opaque,
> > +                      &local_err) != splitkeylen) {
> > +            error_propagate(errp, local_err);
> > +            return -1;
> > +        }
> > +    }
> > +    return 0;
> 
> We need to "return ret" here, in case the earlier store_header() failed

Fixed, thanks!
> 
> > +}
> >  
> 
> 
> 
> > +static int
> > +qcrypto_block_luks_amend_add_keyslot(QCryptoBlock *block,
> > +                                     QCryptoBlockReadFunc readfunc,
> > +                                     QCryptoBlockWriteFunc writefunc,
> > +                                     void *opaque,
> > +                                     QCryptoBlockAmendOptionsLUKS *opts_luks,
> > +                                     bool force,
> > +                                     Error **errp)
> > +{
> > +    QCryptoBlockLUKS *luks = block->opaque;
> > +    uint64_t iter_time = opts_luks->has_iter_time ?
> > +                         opts_luks->iter_time :
> > +                         QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS;
> > +    int keyslot;
> > +    g_autofree char *old_password = NULL;
> > +    g_autofree char *new_password = NULL;
> > +    g_autofree uint8_t *master_key = NULL;
> > +
> > +    char *secret = opts_luks->has_secret ? opts_luks->secret : luks->secret;
> > +
> > +    if (!opts_luks->has_new_secret) {
> > +        error_setg(errp, "'new-secret' is required to activate a keyslot");
> > +        return -1;
> > +    }
> > +    if (opts_luks->has_old_secret) {
> > +        error_setg(errp,
> > +                   "'old-secret' must not be given when activating keyslots");
> > +        return -1;
> > +    }
> > +
> > +    if (opts_luks->has_keyslot) {
> > +        keyslot = opts_luks->keyslot;
> > +        if (keyslot < 0 || keyslot >= QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS) {
> > +            error_setg(errp,
> > +                       "Invalid keyslot %u specified, must be between 0 and %u",
> > +                       keyslot, QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS - 1);
> > +            return -1;
> > +        }
> > +    } else {
> > +        keyslot = qcrypto_block_luks_find_free_keyslot(luks);
> > +        if (keyslot == -1) {
> > +            error_setg(errp,
> > +                       "Can't add a keyslot - all keyslots are in use");
> > +            return -1;
> > +        }
> > +    }
> > +
> > +    if (!force && qcrypto_block_luks_slot_active(luks, keyslot)) {
> > +        error_setg(errp,
> > +                   "Refusing to overwrite active keyslot %i - "
> > +                   "please erase it first",
> > +                   keyslot);
> > +        return -EINVAL;
> 
> s/-EINVAL/-1/
> 
> > +    }
> > +
> > +    /* Locate the password that will be used to retrieve the master key */
> > +    old_password = qcrypto_secret_lookup_as_utf8(secret, errp);
> > +    if (!old_password) {
> > +        return -1;
> > +    }
> > +
> > +    /* Retrieve the master key */
> > +    master_key = g_new0(uint8_t, luks->header.master_key_len);
> > +
> > +    if (qcrypto_block_luks_find_key(block, old_password, master_key,
> > +                                    readfunc, opaque, errp) < 0) {
> > +        error_append_hint(errp, "Failed to retrieve the master key");
> > +        return -1;
> > +    }
> > +
> > +    /* Locate the new password*/
> > +    new_password = qcrypto_secret_lookup_as_utf8(opts_luks->new_secret, errp);
> > +    if (!new_password) {
> > +        return -EINVAL;
> 
> s/-EINVAL/-1/
> 
> > +    }
> > +
> > +    /* Now set the new keyslots */
> > +    if (qcrypto_block_luks_store_key(block, keyslot, new_password, master_key,
> > +                                     iter_time, writefunc, opaque, errp)) {
> > +        error_append_hint(errp, "Failed to write to keyslot %i", keyslot);
> > +        return -EINVAL;
> 
> s/-EINVAL/-1/
Fixed, sorry about this.


> 
> > +    }
> > +    return 0;
> > +}
> > +
> > +static int
> > +qcrypto_block_luks_amend_erase_keyslots(QCryptoBlock *block,
> > +                                        QCryptoBlockReadFunc readfunc,
> > +                                        QCryptoBlockWriteFunc writefunc,
> > +                                        void *opaque,
> > +                                        QCryptoBlockAmendOptionsLUKS *opts_luks,
> > +                                        bool force,
> > +                                        Error **errp)
> > +{
> > +    QCryptoBlockLUKS *luks = block->opaque;
> > +    g_autofree uint8_t *tmpkey = NULL;
> > +    g_autofree char *old_password = NULL;
> > +
> > +    if (opts_luks->has_new_secret) {
> > +        error_setg(errp,
> > +                   "'new-secret' must not be given when erasing keyslots");
> > +        return -1;
> > +    }
> > +    if (opts_luks->has_iter_time) {
> > +        error_setg(errp,
> > +                   "'iter-time' must not be given when erasing keyslots");
> > +        return -1;
> > +    }
> > +    if (opts_luks->has_secret) {
> > +        error_setg(errp,
> > +                   "'secret' must not be given when erasing keyslots");
> > +        return -1;
> > +    }
> > +
> > +    /* Load the old password if given */
> > +    if (opts_luks->has_old_secret) {
> > +        old_password = qcrypto_secret_lookup_as_utf8(opts_luks->old_secret,
> > +                                                     errp);
> > +        if (!old_password) {
> > +            return -1;
> > +        }
> > +
> > +        /*
> > +         * Allocate a temporary key buffer that we will need when
> > +         * checking if slot matches the given old password
> > +         */
> > +        tmpkey = g_new0(uint8_t, luks->header.master_key_len);
> > +}
> 
> Indent missing
Fixed.
> 
> 
> 
> With the minor points fixed
> 
>    Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> 
> Regards,
> Daniel


Best regards,
	Maxim Levitsky