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

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

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

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

> +}
>  



> +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/

> +    }
> +    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



With the minor points fixed

   Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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