Re: [Qemu-block] [Qemu-devel] [PATCH 03/10] qcrypto-luks: implement the encryption key management

[Maxim Levitsky]

On Fri, 2019-09-06 at 14:55 +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 30, 2019 at 11:56:01PM +0300, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky <address@hidden>
> > ---
> >  crypto/block-luks.c | 366 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 364 insertions(+), 2 deletions(-)
> > 
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index ba20d55246..21325fbc79 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > @@ -70,6 +70,9 @@ typedef struct QCryptoBlockLUKSKeySlot 
> > QCryptoBlockLUKSKeySlot;
> >  
> >  #define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL
> >  
> > +#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME 2000
> 
> Perhaps use  ITER_TIME_MS to make it clear it is millisecs
Why not... done.

> 
> > +#define QCRYPTO_BLOCK_LUKS_ERASE_ITERATIONS 40
> > +
> >  static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] = 
> > {
> >      'L', 'U', 'K', 'S', 0xBA, 0xBE
> >  };
> > @@ -219,6 +222,9 @@ struct QCryptoBlockLUKS {
> >  
> >      /* Hash algorithm used in pbkdf2 function */
> >      QCryptoHashAlgorithm hash_alg;
> > +
> > +    /* Name of the secret that was used to open the image */
> > +    char *secret;
> > +
> > +/*
> > + * Returns true if a slot i is marked as active
> > + * (contains encrypted copy of the master key)
> > + */
> > +
> > +static bool
> 
> No blank line is wanted between the comment & function.
> Likewise for the rest of this patch series
No problem!


> 
> > +qcrypto_block_luks_slot_active(const QCryptoBlockLUKS *luks,
> > +                               unsigned int slot_idx)
> > +{
> > +    uint32_t val = luks->header.key_slots[slot_idx].active;
> > +    return val ==  QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED;
> > +}
> 
> 
> 
> > +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;
> > +
> > +    assert(slot_idx < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> > +    assert(splitkeylen > 0);
> > +
> > +    garbagesplitkey = g_malloc0(splitkeylen);
> 
> I'd prefer   g_new0(uint8_t, splitkeylen)
Done.

> 
> > +
> > +    /* 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;
> > +
> 
> 
> > @@ -1522,6 +1700,187 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> >  }
> >  
> >  
> > +#define CHECK_NON_AMEND_OPTION(luks, luks_opts, name) \
> > +    if (luks_opts.has_##name && luks_opts.name != luks->name) { \
> > +            error_setg(errp, "Option \"" #name "\" can't be amended"); \
> > +            goto cleanup; \
> > +    }
> > +
> > +static int
> > +qcrypto_block_luks_amend_options(QCryptoBlock *block,
> > +                                 QCryptoBlockReadFunc readfunc,
> > +                                 QCryptoBlockWriteFunc writefunc,
> > +                                 void *opaque,
> > +                                 QCryptoBlockCreateOptions *options,
> > +                                 bool force,
> > +                                 Error **errp)
> > +{
> > +    QCryptoBlockLUKS *luks = block->opaque;
> > +    QCryptoBlockCreateOptionsLUKS luks_opts;
> > +    g_autofree char *old_password = NULL;
> > +    g_autofree char *password = NULL;
> > +    const char *unlock_secret = luks->secret;
> > +    g_autofree uint8_t *masterkey = NULL;
> > +    int slot = -1;
> > +    int ret = -1;
> > +    bool active = true;
> > +    int64_t iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME;
> > +
> > +    memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts));
> > +
> > +    CHECK_NON_AMEND_OPTION(luks, luks_opts, cipher_alg);
> > +    CHECK_NON_AMEND_OPTION(luks, luks_opts, cipher_mode);
> > +    CHECK_NON_AMEND_OPTION(luks, luks_opts, ivgen_alg);
> > +    CHECK_NON_AMEND_OPTION(luks, luks_opts, ivgen_hash_alg);
> > +    CHECK_NON_AMEND_OPTION(luks, luks_opts, hash_alg);
> > +
> > +    /* Read given slot and check it */
> > +    if (luks_opts.has_slot) {
> > +        slot = luks_opts.slot;
> > +        if (slot < 0 || slot >= QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS) {
> > +            error_setg(errp,
> > +                       "Given key slot %i is not supported by LUKS", slot);
> > +             goto cleanup;
> 
> Off by 1 one indent
Sorry about that.
> 
> > +        }
> > +    }
> > +
> > +    if (luks_opts.has_iter_time) {
> > +        iter_time = luks_opts.iter_time;
> > +    }
> > +
> > +    if (luks_opts.has_active && luks_opts.active == false) {
> > +        active = false;
> > +    }
> > +
> > +    if (active) {
> > +
> > +        /* Check that we are not overwriting an active slot */
> > +        if (!force && slot != -1 &&
> > +            qcrypto_block_luks_slot_active(luks, slot)) {
> > +
> > +            error_setg(errp, "Can't update an active key slot %i",
> > +                       slot);
> > +            goto cleanup;
> > +        }
> > +
> > +        /* check that we have the passwords*/
> > +        if (!luks_opts.has_key_secret) {
> > +            error_setg(errp, "Can't add a key slot without a  password");
> > +            goto cleanup;
> > +        }
> > +
> > +        if (luks_opts.has_unlock_secret) {
> > +            unlock_secret = luks_opts.unlock_secret;
> > +        }
> > +
> > +        /* Read the old password */
> > +        old_password = qcrypto_secret_lookup_as_utf8(unlock_secret, errp);
> > +        if (!old_password) {
> > +            goto cleanup;
> > +        }
> > +
> > +        masterkey = g_new0(uint8_t, luks->header.master_key_len);
> > +
> > +        /* Retrieve the master key*/
> > +        if (qcrypto_block_luks_find_key(block, old_password, masterkey,
> > +                                        readfunc, opaque,
> > +                                        errp) < 0) {
> > +            error_append_hint(errp,
> > +                              "unlock secret, doesn't unlock the image");
> 
> No need for the ',' in this msg I think.
Fixed and reworded a bit.

> 
> > +            goto cleanup;
> > +        }
> > +
> > +        /* Read the new password*/
> > +        password = qcrypto_secret_lookup_as_utf8(luks_opts.key_secret, 
> > errp);
> > +        if (!password) {
> > +            goto cleanup;
> > +        }
> > +
> > +        /* Find the new slot to write to */
> > +        if (slot == -1) {
> > +            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");
> > +                goto cleanup;
> > +
> > +            }
> 
> Extra blank line
Agree
> 
> > +        }
> > +
> > +        /* Store the master key to the new slot */
> > +        if (qcrypto_block_luks_store_key(block, slot, password, masterkey,
> > +                                         iter_time, writefunc, opaque,
> > +                                         errp)) {
> > +
> 
> 
> Extra blank line
> 
> > +            error_append_hint(errp, "Failed to store the keyslot %i", 
> > slot);
> > +            goto cleanup;
> > +        }
> > +
> > +    } else {
> > +
> > +        /* Check that we are not erasing last key slot */
> > +        if (qcrypto_block_luks_count_active_slots(luks) <= 1) {
> > +
> 
> Extra blank line(s)

I hope I cleaned all blank lines, sorry about that.

Best regards,
        Maxim Levitsky