| Message ID | 20200114193350.10830-3-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | LUKS: encryption slot management using amend interface | expand |
Reviewing just the QAPI schema. Maxim Levitsky <mlevitsk@redhat.com> writes: > Next few patches will expose that functionality > to the user. > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > crypto/block-luks.c | 374 +++++++++++++++++++++++++++++++++++++++++++- > qapi/crypto.json | 50 +++++- > 2 files changed, 421 insertions(+), 3 deletions(-) > > diff --git a/crypto/block-luks.c b/crypto/block-luks.c > index 4861db810c..349e95fed3 100644 > --- a/crypto/block-luks.c > +++ b/crypto/block-luks.c > @@ -32,6 +32,7 @@ > #include "qemu/uuid.h" > > #include "qemu/coroutine.h" > +#include "qemu/bitmap.h" > > /* > * Reference for the LUKS format implemented here is > @@ -70,6 +71,9 @@ typedef struct QCryptoBlockLUKSKeySlot QCryptoBlockLUKSKeySlot; > > #define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL > > +#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS 2000 > +#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 +223,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; > }; > > > @@ -1069,6 +1076,112 @@ qcrypto_block_luks_find_key(QCryptoBlock *block, > return -1; > } > > +/* > + * Returns true if a slot i is marked as active > + * (contains encrypted copy of the master key) > + */ > +static bool > +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; > +} > + > +/* > + * Returns the number of slots that are marked as active > + * (slots that contain encrypted copy of the master key) > + */ > +static unsigned int > +qcrypto_block_luks_count_active_slots(const QCryptoBlockLUKS *luks) > +{ > + size_t i = 0; > + unsigned int ret = 0; > + > + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { > + if (qcrypto_block_luks_slot_active(luks, i)) { > + ret++; > + } > + } > + return ret; > +} > + > +/* > + * Finds first key slot which is not active > + * Returns the key slot index, or -1 if it doesn't exist > + */ > +static int > +qcrypto_block_luks_find_free_keyslot(const QCryptoBlockLUKS *luks) > +{ > + size_t i; > + > + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { > + if (!qcrypto_block_luks_slot_active(luks, i)) { > + return i; > + } > + } > + return -1; > + > +} > + > +/* > + * 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; > + > + 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; > + > + qcrypto_block_luks_store_header(block, writefunc, opaque, errp); > + > + /* > + * 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, errp) < 0) { > + /* > + * If we failed to get the random data, still write > + * at least zeros to the key slot at least once > + */ > + if (i > 0) { > + return -1; > + } > + } > + > + if (writefunc(block, > + slot->key_offset_sector * QCRYPTO_BLOCK_LUKS_SECTOR_SIZE, > + garbagesplitkey, > + splitkeylen, > + opaque, > + errp) != splitkeylen) { > + return -1; > + } > + } > + return 0; > +} > > static int > qcrypto_block_luks_open(QCryptoBlock *block, > @@ -1099,6 +1212,7 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > luks = g_new0(QCryptoBlockLUKS, 1); > block->opaque = luks; > + luks->secret = g_strdup(options->u.luks.key_secret); > > if (qcrypto_block_luks_load_header(block, readfunc, opaque, errp) < 0) { > goto fail; > @@ -1164,6 +1278,7 @@ qcrypto_block_luks_open(QCryptoBlock *block, > fail: > qcrypto_block_free_cipher(block); > qcrypto_ivgen_free(block->ivgen); > + g_free(luks->secret); > g_free(luks); > return -1; > } > @@ -1204,7 +1319,7 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts)); > if (!luks_opts.has_iter_time) { > - luks_opts.iter_time = 2000; > + luks_opts.iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS; > } > if (!luks_opts.has_cipher_alg) { > luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256; > @@ -1244,6 +1359,8 @@ qcrypto_block_luks_create(QCryptoBlock *block, > optprefix ? optprefix : ""); > goto error; > } > + luks->secret = g_strdup(options->u.luks.key_secret); > + > password = qcrypto_secret_lookup_as_utf8(luks_opts.key_secret, errp); > if (!password) { > goto error; > @@ -1471,10 +1588,260 @@ qcrypto_block_luks_create(QCryptoBlock *block, > qcrypto_block_free_cipher(block); > qcrypto_ivgen_free(block->ivgen); > > + g_free(luks->secret); > g_free(luks); > return -1; > } > > +/* > + * Given LUKSKeyslotUpdate command, return @slots_bitmap with all slots > + * that will be updated with new password (or erased) > + * returns number of affected slots > + */ > +static int qcrypto_block_luks_get_slots_bitmap(QCryptoBlock *block, > + QCryptoBlockReadFunc readfunc, > + void *opaque, > + const LUKSKeyslotUpdate *command, > + unsigned long *slots_bitmap, > + Error **errp) > +{ > + const QCryptoBlockLUKS *luks = block->opaque; > + size_t i; > + int ret = 0; > + > + if (command->has_keyslot) { > + /* keyslot set, select only this keyslot */ > + int keyslot = command->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); > + goto error; > + } > + bitmap_set(slots_bitmap, keyslot, 1); > + ret++; > + > + } else if (command->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); > + ret++; > + } > + } > + } 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"); > + goto error; > + } > + bitmap_set(slots_bitmap, slot, 1); > + ret++; > + } > + > + if (command->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(command->old_secret, > + errp); > + if (!old_password) { > + goto error; > + } > + > + rv = qcrypto_block_luks_load_key(block, > + i, > + old_password, > + tmpkey, > + readfunc, > + opaque, > + errp); > + if (rv == -1) > + goto error; > + else if (rv == 0) { > + bitmap_clear(slots_bitmap, i, 1); > + ret--; > + } > + } > + } > + return ret; > +error: > + return -1; > +} > + > +/* > + * Apply a single keyslot update command as described in @command > + * Optionally use @unlock_secret to retrieve the master key > + */ > +static int > +qcrypto_block_luks_apply_keyslot_update(QCryptoBlock *block, > + QCryptoBlockReadFunc readfunc, > + QCryptoBlockWriteFunc writefunc, > + void *opaque, > + LUKSKeyslotUpdate *command, > + const char *unlock_secret, > + uint8_t **master_key, > + bool force, > + Error **errp) > +{ > + QCryptoBlockLUKS *luks = block->opaque; > + g_autofree unsigned long *slots_bitmap = NULL; > + int64_t iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS; > + int slot_count; > + size_t i; > + char *new_password; > + bool erasing; > + > + slots_bitmap = bitmap_new(QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS); > + slot_count = qcrypto_block_luks_get_slots_bitmap(block, readfunc, opaque, > + command, slots_bitmap, > + errp); > + if (slot_count == -1) { > + goto error; > + } > + /* no matching slots, so nothing to do */ > + if (slot_count == 0) { > + error_setg(errp, "Requested operation didn't match any slots"); > + goto error; > + } > + /* > + * slot is erased when the password is set to null, or empty string > + * (for compatibility with command line) > + */ > + erasing = command->new_secret->type == QTYPE_QNULL || > + strlen(command->new_secret->u.s) == 0; > + > + /* safety checks */ > + if (!force) { > + if (erasing) { > + if (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"); > + goto error; > + } > + } else { > + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { > + if (!test_bit(i, slots_bitmap)) { > + continue; > + } > + if (qcrypto_block_luks_slot_active(luks, i)) { > + error_setg(errp, > + "Refusing to overwrite active slot %zu - " > + "please erase it first", i); > + goto error; > + } > + } > + } > + } > + > + /* setup the data needed for storing the new keyslot */ > + if (!erasing) { > + /* Load the master key if it wasn't already loaded */ > + if (!*master_key) { > + g_autofree char *old_password; > + old_password = qcrypto_secret_lookup_as_utf8(unlock_secret, errp); > + if (!old_password) { > + goto error; > + } > + *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"); > + goto error; > + } > + } > + new_password = qcrypto_secret_lookup_as_utf8(command->new_secret->u.s, > + errp); > + if (!new_password) { > + goto error; > + } > + if (command->has_iter_time) { > + iter_time = command->iter_time; > + } > + } > + > + /* new apply the update */ > + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { > + if (!test_bit(i, slots_bitmap)) { > + continue; > + } > + if (erasing) { > + if (qcrypto_block_luks_erase_key(block, i, > + writefunc, > + opaque, > + errp)) { > + error_append_hint(errp, "Failed to erase keyslot %zu", i); > + goto error; > + } > + } else { > + if (qcrypto_block_luks_store_key(block, i, > + new_password, > + *master_key, > + iter_time, > + writefunc, > + opaque, > + errp)) { > + error_append_hint(errp, "Failed to write to keyslot %zu", i); > + goto error; > + } > + } > + } > + return 0; > +error: > + return -EINVAL; > +} > + > +static int > +qcrypto_block_luks_amend_options(QCryptoBlock *block, > + QCryptoBlockReadFunc readfunc, > + QCryptoBlockWriteFunc writefunc, > + void *opaque, > + QCryptoBlockAmendOptions *options, > + bool force, > + Error **errp) > +{ > + QCryptoBlockLUKS *luks = block->opaque; > + QCryptoBlockAmendOptionsLUKS *options_luks = &options->u.luks; > + LUKSKeyslotUpdateList *ptr; > + g_autofree uint8_t *master_key = NULL; > + int ret; > + > + char *unlock_secret = options_luks->has_unlock_secret ? > + options_luks->unlock_secret : > + luks->secret; > + > + for (ptr = options_luks->keys; ptr; ptr = ptr->next) { > + ret = qcrypto_block_luks_apply_keyslot_update(block, readfunc, > + writefunc, opaque, > + ptr->value, > + unlock_secret, > + &master_key, > + force, errp); > + > + if (ret != 0) { > + goto error; > + } > + } > + return 0; > +error: > + return -1; > +} > > static int qcrypto_block_luks_get_info(QCryptoBlock *block, > QCryptoBlockInfo *info, > @@ -1523,7 +1890,9 @@ static int qcrypto_block_luks_get_info(QCryptoBlock *block, > > static void qcrypto_block_luks_cleanup(QCryptoBlock *block) > { > - g_free(block->opaque); > + QCryptoBlockLUKS *luks = block->opaque; > + g_free(luks->secret); > + g_free(luks); > } > > > @@ -1560,6 +1929,7 @@ qcrypto_block_luks_encrypt(QCryptoBlock *block, > const QCryptoBlockDriver qcrypto_block_driver_luks = { > .open = qcrypto_block_luks_open, > .create = qcrypto_block_luks_create, > + .amend = qcrypto_block_luks_amend_options, > .get_info = qcrypto_block_luks_get_info, > .cleanup = qcrypto_block_luks_cleanup, > .decrypt = qcrypto_block_luks_decrypt, > diff --git a/qapi/crypto.json b/qapi/crypto.json > index 9faebd03d4..e83847c71e 100644 > --- a/qapi/crypto.json > +++ b/qapi/crypto.json > @@ -1,6 +1,8 @@ > # -*- Mode: Python -*- > # > > +{ 'include': 'common.json' } > + > ## > # = Cryptography > ## > @@ -310,6 +312,52 @@ > 'discriminator': 'format', > 'data': { 'luks': 'QCryptoBlockInfoLUKS' } } > > +## > +# @LUKSKeyslotUpdate: > +# > +# @keyslot: If specified, will update only keyslot with this index > +# > +# @old-secret: If specified, will only update keyslots that > +# can be opened with password which is contained in > +# QCryptoSecret with @old-secret ID > +# > +# If neither @keyslot nor @old-secret is specified, > +# first empty keyslot is selected for the update > +# > +# @new-secret: The ID of a QCryptoSecret object providing a new decryption > +# key to place in all matching keyslots. > +# null/empty string erases all matching keyslots I hate making the empty string do something completely different than a non-empty string. What about making @new-secret optional, and have absent @new-secret erase? > +# unless > +# last valid keyslot is erased. Leaves me to wonder what happens when I try. If I read your code correctly, it's an error. Suggest "You cannot erase the last valid keyslot". Not documented here: "Refusing to overwrite active slot". > +# > +# @iter-time: number of milliseconds to spend in > +# PBKDF passphrase processing Default? > +# Since: 5.0 > +## > +{ 'struct': 'LUKSKeyslotUpdate', > + 'data': { > + '*keyslot': 'int', > + '*old-secret': 'str', > + 'new-secret' : 'StrOrNull', > + '*iter-time' : 'int' } } > + > + > +## > +# @QCryptoBlockAmendOptionsLUKS: > +# > +# The options that can be changed on existing luks encrypted device > +# @keys: list of keyslot updates to perform > +# (updates are performed in order) > +# @unlock-secret: use this secret to retrieve the current master key > +# if not given will use the same secret as one "as the one"? > +# that was used to open the image > +# > +# Since: 5.0 > +## > +{ 'struct': 'QCryptoBlockAmendOptionsLUKS', > + 'data' : { > + 'keys': ['LUKSKeyslotUpdate'], > + '*unlock-secret' : 'str' } } > + > > > ## > @@ -324,4 +372,4 @@ > 'base': 'QCryptoBlockOptionsBase', > 'discriminator': 'format', > 'data': { > - } } > + 'luks': 'QCryptoBlockAmendOptionsLUKS' } }
On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote: <trimmed> > > +## > > +# @LUKSKeyslotUpdate: > > +# > > +# @keyslot: If specified, will update only keyslot with this index > > +# > > +# @old-secret: If specified, will only update keyslots that > > +# can be opened with password which is contained in > > +# QCryptoSecret with @old-secret ID > > +# > > +# If neither @keyslot nor @old-secret is specified, > > +# first empty keyslot is selected for the update > > +# > > +# @new-secret: The ID of a QCryptoSecret object providing a new decryption > > +# key to place in all matching keyslots. > > +# null/empty string erases all matching keyslots > > I hate making the empty string do something completely different than a > non-empty string. > > What about making @new-secret optional, and have absent @new-secret > erase? I don't remember already why I and Keven Wolf decided to do this this way, but I think that you are right here. I don't mind personally to do this this way. empty string though is my addition, since its not possible to pass null on command line. > > > +# unless > > +# last valid keyslot is erased. > > Leaves me to wonder what happens when I try. If I read your code > correctly, it's an error. Suggest "You cannot erase the last valid > keyslot". > > Not documented here: "Refusing to overwrite active slot". In my current implementation, if all slots are selected for erase, I just refuse the erase operation. In former versions of my patches, I would instead erase all but the last one. IMHO, its unlikely that more that one slot would have the same password, thus anyway correct usage for replacing the password would be first add a new slot, then erase all slots that match the old password. If all slots are active and have the same password, then user still can use 'force' option to overwrite one of them. > > > +# > > +# @iter-time: number of milliseconds to spend in > > +# PBKDF passphrase processing > > Default? 2000, as in QCryptoBlockCreateOptionsLUKS. I forgot to copy this here. > > > +# Since: 5.0 > > +## > > +{ 'struct': 'LUKSKeyslotUpdate', > > + 'data': { > > + '*keyslot': 'int', > > + '*old-secret': 'str', > > + 'new-secret' : 'StrOrNull', > > + '*iter-time' : 'int' } } > > + > > + > > +## > > +# @QCryptoBlockAmendOptionsLUKS: > > +# > > +# The options that can be changed on existing luks encrypted device > > +# @keys: list of keyslot updates to perform > > +# (updates are performed in order) > > +# @unlock-secret: use this secret to retrieve the current master key > > +# if not given will use the same secret as one > > "as the one"? Yea, this is wrong wording, I'll drop those words. Thanks. > > > +# that was used to open the image > > +# > > +# Since: 5.0 > > +## > > +{ 'struct': 'QCryptoBlockAmendOptionsLUKS', > > + 'data' : { > > + 'keys': ['LUKSKeyslotUpdate'], > > + '*unlock-secret' : 'str' } } > > + > > > > > > ## > > @@ -324,4 +372,4 @@ > > 'base': 'QCryptoBlockOptionsBase', > > 'discriminator': 'format', > > 'data': { > > - } } > > + 'luks': 'QCryptoBlockAmendOptionsLUKS' } } Thanks for review, Best regards, Maxim Levitsky
On Tue, Jan 21, 2020 at 03:13:01PM +0200, Maxim Levitsky wrote: > On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote: > > <trimmed> > > > > +## > > > +# @LUKSKeyslotUpdate: > > > +# > > > +# @keyslot: If specified, will update only keyslot with this index > > > +# > > > +# @old-secret: If specified, will only update keyslots that > > > +# can be opened with password which is contained in > > > +# QCryptoSecret with @old-secret ID > > > +# > > > +# If neither @keyslot nor @old-secret is specified, > > > +# first empty keyslot is selected for the update > > > +# > > > +# @new-secret: The ID of a QCryptoSecret object providing a new decryption > > > +# key to place in all matching keyslots. > > > +# null/empty string erases all matching keyslots > > > > I hate making the empty string do something completely different than a > > non-empty string. > > > > What about making @new-secret optional, and have absent @new-secret > > erase? > > I don't remember already why I and Keven Wolf decided to do this this way, but I think that you are right here. > I don't mind personally to do this this way. > empty string though is my addition, since its not possible to pass null on command line. IIUC this a result of using "StrOrNull" for this one field... > > > +# Since: 5.0 > > > +## > > > +{ 'struct': 'LUKSKeyslotUpdate', > > > + 'data': { > > > + '*keyslot': 'int', > > > + '*old-secret': 'str', > > > + 'new-secret' : 'StrOrNull', > > > + '*iter-time' : 'int' } } It looks wierd here to be special casing "new-secret" to "StrOrNull" instead of just marking it as an optional string field "*new-secret": "str" which would be possible to use from the command line, as you simply omit the field. I guess the main danger here is that we're using this as a trigger to erase keyslots. So simply omitting "new-secret" can result in damage to the volume by accident which is not an attractive mode. > > > + > > > +## > > > +# @QCryptoBlockAmendOptionsLUKS: > > > +# > > > +# The options that can be changed on existing luks encrypted device > > > +# @keys: list of keyslot updates to perform > > > +# (updates are performed in order) > > > +# @unlock-secret: use this secret to retrieve the current master key > > > +# if not given will use the same secret as one > > > > "as the one"? > Yea, this is wrong wording, I'll drop those words. Thanks. > > > > > > +# that was used to open the image > > > +# > > > +# Since: 5.0 > > > +## > > > +{ 'struct': 'QCryptoBlockAmendOptionsLUKS', > > > + 'data' : { > > > + 'keys': ['LUKSKeyslotUpdate'], > > > + '*unlock-secret' : 'str' } } > > > + > > > > > > > > > ## > > > @@ -324,4 +372,4 @@ > > > 'base': 'QCryptoBlockOptionsBase', > > > 'discriminator': 'format', > > > 'data': { > > > - } } > > > + 'luks': 'QCryptoBlockAmendOptionsLUKS' } } > > > Thanks for review, > Best regards, > Maxim Levitsky > Regards, Daniel
On Tue, Jan 14, 2020 at 09:33:39PM +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 | 374 +++++++++++++++++++++++++++++++++++++++++++- > qapi/crypto.json | 50 +++++- > 2 files changed, 421 insertions(+), 3 deletions(-) > > diff --git a/crypto/block-luks.c b/crypto/block-luks.c > index 4861db810c..349e95fed3 100644 > --- a/crypto/block-luks.c > +++ b/crypto/block-luks.c > @@ -32,6 +32,7 @@ > #include "qemu/uuid.h" > > #include "qemu/coroutine.h" > +#include "qemu/bitmap.h" > > /* > * Reference for the LUKS format implemented here is > @@ -70,6 +71,9 @@ typedef struct QCryptoBlockLUKSKeySlot QCryptoBlockLUKSKeySlot; > > #define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL > > +#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS 2000 > +#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 +223,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; > }; > > > @@ -1069,6 +1076,112 @@ qcrypto_block_luks_find_key(QCryptoBlock *block, > return -1; > } > > +/* > + * Returns true if a slot i is marked as active > + * (contains encrypted copy of the master key) > + */ > +static bool > +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; > +} > + > +/* > + * Returns the number of slots that are marked as active > + * (slots that contain encrypted copy of the master key) > + */ > +static unsigned int > +qcrypto_block_luks_count_active_slots(const QCryptoBlockLUKS *luks) > +{ > + size_t i = 0; > + unsigned int ret = 0; > + > + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { > + if (qcrypto_block_luks_slot_active(luks, i)) { > + ret++; > + } > + } > + return ret; > +} > + > +/* > + * Finds first key slot which is not active > + * Returns the key slot index, or -1 if it doesn't exist > + */ > +static int > +qcrypto_block_luks_find_free_keyslot(const QCryptoBlockLUKS *luks) > +{ > + size_t i; > + > + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { > + if (!qcrypto_block_luks_slot_active(luks, i)) { > + return i; > + } > + } > + return -1; > + > +} > + > +/* > + * 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; > + > + 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; > + > + qcrypto_block_luks_store_header(block, writefunc, opaque, errp); > + > + /* > + * 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, errp) < 0) { > + /* > + * If we failed to get the random data, still write > + * at least zeros to the key slot at least once > + */ > + if (i > 0) { > + return -1; > + } > + } > + > + if (writefunc(block, > + slot->key_offset_sector * QCRYPTO_BLOCK_LUKS_SECTOR_SIZE, > + garbagesplitkey, > + splitkeylen, > + opaque, > + errp) != splitkeylen) { > + return -1; > + } > + } > + return 0; > +} > > static int > qcrypto_block_luks_open(QCryptoBlock *block, > @@ -1099,6 +1212,7 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > luks = g_new0(QCryptoBlockLUKS, 1); > block->opaque = luks; > + luks->secret = g_strdup(options->u.luks.key_secret); > > if (qcrypto_block_luks_load_header(block, readfunc, opaque, errp) < 0) { > goto fail; > @@ -1164,6 +1278,7 @@ qcrypto_block_luks_open(QCryptoBlock *block, > fail: > qcrypto_block_free_cipher(block); > qcrypto_ivgen_free(block->ivgen); > + g_free(luks->secret); > g_free(luks); > return -1; > } > @@ -1204,7 +1319,7 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts)); > if (!luks_opts.has_iter_time) { > - luks_opts.iter_time = 2000; > + luks_opts.iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS; > } > if (!luks_opts.has_cipher_alg) { > luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256; > @@ -1244,6 +1359,8 @@ qcrypto_block_luks_create(QCryptoBlock *block, > optprefix ? optprefix : ""); > goto error; > } > + luks->secret = g_strdup(options->u.luks.key_secret); > + > password = qcrypto_secret_lookup_as_utf8(luks_opts.key_secret, errp); > if (!password) { > goto error; > @@ -1471,10 +1588,260 @@ qcrypto_block_luks_create(QCryptoBlock *block, > qcrypto_block_free_cipher(block); > qcrypto_ivgen_free(block->ivgen); > > + g_free(luks->secret); > g_free(luks); > return -1; > } > > +/* > + * Given LUKSKeyslotUpdate command, return @slots_bitmap with all slots > + * that will be updated with new password (or erased) > + * returns number of affected slots > + */ > +static int qcrypto_block_luks_get_slots_bitmap(QCryptoBlock *block, > + QCryptoBlockReadFunc readfunc, > + void *opaque, > + const LUKSKeyslotUpdate *command, > + unsigned long *slots_bitmap, > + Error **errp) > +{ > + const QCryptoBlockLUKS *luks = block->opaque; > + size_t i; > + int ret = 0; > + > + if (command->has_keyslot) { > + /* keyslot set, select only this keyslot */ > + int keyslot = command->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); > + goto error; > + } > + bitmap_set(slots_bitmap, keyslot, 1); > + ret++; > + > + } else if (command->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); > + ret++; > + } > + } > + } 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"); > + goto error; > + } > + bitmap_set(slots_bitmap, slot, 1); > + ret++; > + } > + > + if (command->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(command->old_secret, > + errp); > + if (!old_password) { > + goto error; > + } > + > + rv = qcrypto_block_luks_load_key(block, > + i, > + old_password, > + tmpkey, > + readfunc, > + opaque, > + errp); > + if (rv == -1) > + goto error; > + else if (rv == 0) { > + bitmap_clear(slots_bitmap, i, 1); > + ret--; > + } > + } > + } > + return ret; > +error: > + return -1; > +} > + > +/* > + * Apply a single keyslot update command as described in @command > + * Optionally use @unlock_secret to retrieve the master key > + */ > +static int > +qcrypto_block_luks_apply_keyslot_update(QCryptoBlock *block, > + QCryptoBlockReadFunc readfunc, > + QCryptoBlockWriteFunc writefunc, > + void *opaque, > + LUKSKeyslotUpdate *command, > + const char *unlock_secret, > + uint8_t **master_key, > + bool force, > + Error **errp) > +{ > + QCryptoBlockLUKS *luks = block->opaque; > + g_autofree unsigned long *slots_bitmap = NULL; > + int64_t iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS; > + int slot_count; > + size_t i; > + char *new_password; > + bool erasing; > + > + slots_bitmap = bitmap_new(QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS); > + slot_count = qcrypto_block_luks_get_slots_bitmap(block, readfunc, opaque, > + command, slots_bitmap, > + errp); > + if (slot_count == -1) { > + goto error; > + } > + /* no matching slots, so nothing to do */ > + if (slot_count == 0) { > + error_setg(errp, "Requested operation didn't match any slots"); > + goto error; > + } > + /* > + * slot is erased when the password is set to null, or empty string > + * (for compatibility with command line) > + */ > + erasing = command->new_secret->type == QTYPE_QNULL || > + strlen(command->new_secret->u.s) == 0; > + > + /* safety checks */ > + if (!force) { > + if (erasing) { > + if (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"); > + goto error; > + } > + } else { > + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { > + if (!test_bit(i, slots_bitmap)) { > + continue; > + } > + if (qcrypto_block_luks_slot_active(luks, i)) { > + error_setg(errp, > + "Refusing to overwrite active slot %zu - " > + "please erase it first", i); > + goto error; > + } > + } > + } > + } > + > + /* setup the data needed for storing the new keyslot */ > + if (!erasing) { > + /* Load the master key if it wasn't already loaded */ > + if (!*master_key) { > + g_autofree char *old_password; > + old_password = qcrypto_secret_lookup_as_utf8(unlock_secret, errp); > + if (!old_password) { > + goto error; > + } > + *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"); > + goto error; > + } > + } > + new_password = qcrypto_secret_lookup_as_utf8(command->new_secret->u.s, > + errp); > + if (!new_password) { > + goto error; > + } > + if (command->has_iter_time) { > + iter_time = command->iter_time; > + } > + } > + > + /* new apply the update */ > + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { > + if (!test_bit(i, slots_bitmap)) { > + continue; > + } > + if (erasing) { > + if (qcrypto_block_luks_erase_key(block, i, > + writefunc, > + opaque, > + errp)) { > + error_append_hint(errp, "Failed to erase keyslot %zu", i); > + goto error; > + } > + } else { > + if (qcrypto_block_luks_store_key(block, i, > + new_password, > + *master_key, > + iter_time, > + writefunc, > + opaque, > + errp)) { > + error_append_hint(errp, "Failed to write to keyslot %zu", i); > + goto error; > + } > + } > + } > + return 0; > +error: > + return -EINVAL; > +} I feel the this method is confusing from trying to handle both adding and erasing keyslots.... > + > +static int > +qcrypto_block_luks_amend_options(QCryptoBlock *block, > + QCryptoBlockReadFunc readfunc, > + QCryptoBlockWriteFunc writefunc, > + void *opaque, > + QCryptoBlockAmendOptions *options, > + bool force, > + Error **errp) > +{ > + QCryptoBlockLUKS *luks = block->opaque; > + QCryptoBlockAmendOptionsLUKS *options_luks = &options->u.luks; > + LUKSKeyslotUpdateList *ptr; > + g_autofree uint8_t *master_key = NULL; > + int ret; > + > + char *unlock_secret = options_luks->has_unlock_secret ? > + options_luks->unlock_secret : > + luks->secret; > + > + for (ptr = options_luks->keys; ptr; ptr = ptr->next) { > + ret = qcrypto_block_luks_apply_keyslot_update(block, readfunc, > + writefunc, opaque, > + ptr->value, > + unlock_secret, > + &master_key, > + force, errp); .... imho we sould do bool erasing = command->new_secret->type == QTYPE_QNULL; if (erasing) { ret = qcrypto_block_luks_disable_keyslot(block, readfunc, writefunc, opaque, ptr->value, unlock_secret, &master_key, force, errp); } else { ret = qcrypto_block_luks_enable_keyslot(block, readfunc, writefunc, opaque, ptr->value, unlock_secret, &master_key, force, errp); } > + > + if (ret != 0) { > + goto error; > + } > + } > + return 0; > +error: > + return -1; > +} If there's no code to run in the 'error' label, then we should just get rid of it and 'return -1' instead of "goto error" > > static int qcrypto_block_luks_get_info(QCryptoBlock *block, > QCryptoBlockInfo *info, > @@ -1523,7 +1890,9 @@ static int qcrypto_block_luks_get_info(QCryptoBlock *block, > > static void qcrypto_block_luks_cleanup(QCryptoBlock *block) > { > - g_free(block->opaque); > + QCryptoBlockLUKS *luks = block->opaque; > + g_free(luks->secret); Check if "luks" is non-NULL for robustness in early failure scenarios > + g_free(luks); > } > Regards, Daniel
On Tue, Jan 28, 2020 at 05:11:16PM +0000, Daniel P. Berrangé wrote: > On Tue, Jan 21, 2020 at 03:13:01PM +0200, Maxim Levitsky wrote: > > On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote: > > > > <trimmed> > > > > > > +## > > > > +# @LUKSKeyslotUpdate: > > > > +# > > > > +# @keyslot: If specified, will update only keyslot with this index > > > > +# > > > > +# @old-secret: If specified, will only update keyslots that > > > > +# can be opened with password which is contained in > > > > +# QCryptoSecret with @old-secret ID > > > > +# > > > > +# If neither @keyslot nor @old-secret is specified, > > > > +# first empty keyslot is selected for the update > > > > +# > > > > +# @new-secret: The ID of a QCryptoSecret object providing a new decryption > > > > +# key to place in all matching keyslots. > > > > +# null/empty string erases all matching keyslots > > > > > > I hate making the empty string do something completely different than a > > > non-empty string. > > > > > > What about making @new-secret optional, and have absent @new-secret > > > erase? > > > > I don't remember already why I and Keven Wolf decided to do this this way, but I think that you are right here. > > I don't mind personally to do this this way. > > empty string though is my addition, since its not possible to pass null on command line. > > IIUC this a result of using "StrOrNull" for this one field... > > > > > > +# Since: 5.0 > > > > +## > > > > +{ 'struct': 'LUKSKeyslotUpdate', > > > > + 'data': { > > > > + '*keyslot': 'int', > > > > + '*old-secret': 'str', > > > > + 'new-secret' : 'StrOrNull', > > > > + '*iter-time' : 'int' } } > > It looks wierd here to be special casing "new-secret" to "StrOrNull" > instead of just marking it as an optional string field > > "*new-secret": "str" > > which would be possible to use from the command line, as you simply > omit the field. > > I guess the main danger here is that we're using this as a trigger > to erase keyslots. So simply omitting "new-secret" can result > in damage to the volume by accident which is not an attractive > mode. Thinking about this again, I really believe we ought to be moire explicit about disabling the keyslot by having the "active" field. eg { 'struct': 'LUKSKeyslotUpdate', 'data': { 'active': 'bool', '*keyslot': 'int', '*old-secret': 'str', '*new-secret' : 'str', '*iter-time' : 'int' } } "new-secret" is thus only needed when "active" == true. This avoids the problem with being unable to specify a null for StrOrNull on the command line too. Regards, Daniel
On Tue, 2020-01-28 at 17:32 +0000, Daniel P. Berrangé wrote: > On Tue, Jan 28, 2020 at 05:11:16PM +0000, Daniel P. Berrangé wrote: > > On Tue, Jan 21, 2020 at 03:13:01PM +0200, Maxim Levitsky wrote: > > > On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote: > > > > > > <trimmed> > > > > > > > > +## > > > > > +# @LUKSKeyslotUpdate: > > > > > +# > > > > > +# @keyslot: If specified, will update only keyslot with this index > > > > > +# > > > > > +# @old-secret: If specified, will only update keyslots that > > > > > +# can be opened with password which is contained in > > > > > +# QCryptoSecret with @old-secret ID > > > > > +# > > > > > +# If neither @keyslot nor @old-secret is specified, > > > > > +# first empty keyslot is selected for the update > > > > > +# > > > > > +# @new-secret: The ID of a QCryptoSecret object providing a new decryption > > > > > +# key to place in all matching keyslots. > > > > > +# null/empty string erases all matching keyslots > > > > > > > > I hate making the empty string do something completely different than a > > > > non-empty string. > > > > > > > > What about making @new-secret optional, and have absent @new-secret > > > > erase? > > > > > > I don't remember already why I and Keven Wolf decided to do this this way, but I think that you are right here. > > > I don't mind personally to do this this way. > > > empty string though is my addition, since its not possible to pass null on command line. > > > > IIUC this a result of using "StrOrNull" for this one field... > > > > > > > > > +# Since: 5.0 > > > > > +## > > > > > +{ 'struct': 'LUKSKeyslotUpdate', > > > > > + 'data': { > > > > > + '*keyslot': 'int', > > > > > + '*old-secret': 'str', > > > > > + 'new-secret' : 'StrOrNull', > > > > > + '*iter-time' : 'int' } } > > > > It looks wierd here to be special casing "new-secret" to "StrOrNull" > > instead of just marking it as an optional string field > > > > "*new-secret": "str" > > > > which would be possible to use from the command line, as you simply > > omit the field. > > > > I guess the main danger here is that we're using this as a trigger > > to erase keyslots. So simply omitting "new-secret" can result > > in damage to the volume by accident which is not an attractive > > mode. > > Thinking about this again, I really believe we ought to be moire > explicit about disabling the keyslot by having the "active" field. > eg > > { 'struct': 'LUKSKeyslotUpdate', > 'data': { > 'active': 'bool', > '*keyslot': 'int', > '*old-secret': 'str', > '*new-secret' : 'str', > '*iter-time' : 'int' } } > > "new-secret" is thus only needed when "active" == true. > > This avoids the problem with being unable to specify a > null for StrOrNull on the command line too. I fully support this idea. If no objections from anybody else, I'll do it this way. Best regards, Maxim Levitsky > > Regards, > Daniel
Am 28.01.2020 um 18:32 hat Daniel P. Berrangé geschrieben: > On Tue, Jan 28, 2020 at 05:11:16PM +0000, Daniel P. Berrangé wrote: > > On Tue, Jan 21, 2020 at 03:13:01PM +0200, Maxim Levitsky wrote: > > > On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote: > > > > > > <trimmed> > > > > > > > > +## > > > > > +# @LUKSKeyslotUpdate: > > > > > +# > > > > > +# @keyslot: If specified, will update only keyslot with this index > > > > > +# > > > > > +# @old-secret: If specified, will only update keyslots that > > > > > +# can be opened with password which is contained in > > > > > +# QCryptoSecret with @old-secret ID > > > > > +# > > > > > +# If neither @keyslot nor @old-secret is specified, > > > > > +# first empty keyslot is selected for the update > > > > > +# > > > > > +# @new-secret: The ID of a QCryptoSecret object providing a new decryption > > > > > +# key to place in all matching keyslots. > > > > > +# null/empty string erases all matching keyslots > > > > > > > > I hate making the empty string do something completely different than a > > > > non-empty string. > > > > > > > > What about making @new-secret optional, and have absent @new-secret > > > > erase? > > > > > > I don't remember already why I and Keven Wolf decided to do this this way, but I think that you are right here. > > > I don't mind personally to do this this way. > > > empty string though is my addition, since its not possible to pass null on command line. > > > > IIUC this a result of using "StrOrNull" for this one field... > > > > > > > > > +# Since: 5.0 > > > > > +## > > > > > +{ 'struct': 'LUKSKeyslotUpdate', > > > > > + 'data': { > > > > > + '*keyslot': 'int', > > > > > + '*old-secret': 'str', > > > > > + 'new-secret' : 'StrOrNull', > > > > > + '*iter-time' : 'int' } } > > > > It looks wierd here to be special casing "new-secret" to "StrOrNull" > > instead of just marking it as an optional string field > > > > "*new-secret": "str" > > > > which would be possible to use from the command line, as you simply > > omit the field. > > > > I guess the main danger here is that we're using this as a trigger > > to erase keyslots. So simply omitting "new-secret" can result > > in damage to the volume by accident which is not an attractive > > mode. Right. It's been a while since I discussed this with Maxim, but I think this was the motivation for me to suggest an explicit null value. As long as we don't support passing null from the command line, I see the problem with it, though. Empty string (which I think we didn't discuss before) looks like a reasonable enough workaround to me, but if you think this is too much magic, then maybe not. > Thinking about this again, I really believe we ought to be moire > explicit about disabling the keyslot by having the "active" field. > eg > > { 'struct': 'LUKSKeyslotUpdate', > 'data': { > 'active': 'bool', > '*keyslot': 'int', > '*old-secret': 'str', > '*new-secret' : 'str', > '*iter-time' : 'int' } } > > "new-secret" is thus only needed when "active" == true. Hm. At the very least, I would make 'active' optional and default to true, so that for adding or updating you must only specify 'new-secret' and for deleting only 'active'. > This avoids the problem with being unable to specify a null for > StrOrNull on the command line too. If we ever get a way to pass null on the command line, how would we think about a struct like this? Will it still feel right, or will it feel like we feel about simple unions today (they exist, we would like to get rid of them, but we can't because compatibility)? Instead of keeping talking about potential future extensions, would it make more sense to just extend the grammar of the keyval parser now so that you can specify a type, including null? We already wanted to use an alternate for keyslot (int) and old-secret (str) initially, which makes it clear on the schema level that you can only specify one of both. It would have worked fine for QMP, but not on the command line because we can't tell integers from strings there. If we can distinguish them as foo:int=2 and foo:str=2 then that wouldn't be a problem any more either. Kevin
On Thu, Jan 30, 2020 at 01:38:47PM +0100, Kevin Wolf wrote: > Am 28.01.2020 um 18:32 hat Daniel P. Berrangé geschrieben: > > On Tue, Jan 28, 2020 at 05:11:16PM +0000, Daniel P. Berrangé wrote: > > > On Tue, Jan 21, 2020 at 03:13:01PM +0200, Maxim Levitsky wrote: > > > > On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote: > > > > > > > > <trimmed> > > > > > > > > > > +## > > > > > > +# @LUKSKeyslotUpdate: > > > > > > +# > > > > > > +# @keyslot: If specified, will update only keyslot with this index > > > > > > +# > > > > > > +# @old-secret: If specified, will only update keyslots that > > > > > > +# can be opened with password which is contained in > > > > > > +# QCryptoSecret with @old-secret ID > > > > > > +# > > > > > > +# If neither @keyslot nor @old-secret is specified, > > > > > > +# first empty keyslot is selected for the update > > > > > > +# > > > > > > +# @new-secret: The ID of a QCryptoSecret object providing a new decryption > > > > > > +# key to place in all matching keyslots. > > > > > > +# null/empty string erases all matching keyslots > > > > > > > > > > I hate making the empty string do something completely different than a > > > > > non-empty string. > > > > > > > > > > What about making @new-secret optional, and have absent @new-secret > > > > > erase? > > > > > > > > I don't remember already why I and Keven Wolf decided to do this this way, but I think that you are right here. > > > > I don't mind personally to do this this way. > > > > empty string though is my addition, since its not possible to pass null on command line. > > > > > > IIUC this a result of using "StrOrNull" for this one field... > > > > > > > > > > > > +# Since: 5.0 > > > > > > +## > > > > > > +{ 'struct': 'LUKSKeyslotUpdate', > > > > > > + 'data': { > > > > > > + '*keyslot': 'int', > > > > > > + '*old-secret': 'str', > > > > > > + 'new-secret' : 'StrOrNull', > > > > > > + '*iter-time' : 'int' } } > > > > > > It looks wierd here to be special casing "new-secret" to "StrOrNull" > > > instead of just marking it as an optional string field > > > > > > "*new-secret": "str" > > > > > > which would be possible to use from the command line, as you simply > > > omit the field. > > > > > > I guess the main danger here is that we're using this as a trigger > > > to erase keyslots. So simply omitting "new-secret" can result > > > in damage to the volume by accident which is not an attractive > > > mode. > > Right. It's been a while since I discussed this with Maxim, but I think > this was the motivation for me to suggest an explicit null value. > > As long as we don't support passing null from the command line, I see > the problem with it, though. Empty string (which I think we didn't > discuss before) looks like a reasonable enough workaround to me, but if > you think this is too much magic, then maybe not. > > > Thinking about this again, I really believe we ought to be moire > > explicit about disabling the keyslot by having the "active" field. > > eg > > > > { 'struct': 'LUKSKeyslotUpdate', > > 'data': { > > 'active': 'bool', > > '*keyslot': 'int', > > '*old-secret': 'str', > > '*new-secret' : 'str', > > '*iter-time' : 'int' } } > > > > "new-secret" is thus only needed when "active" == true. > > Hm. At the very least, I would make 'active' optional and default to > true, so that for adding or updating you must only specify 'new-secret' > and for deleting only 'active'. Is that asymmetry really worth while ? It merely saves a few characters of typing by omitting "active: true", so I'm not really convinced. > > > This avoids the problem with being unable to specify a null for > > StrOrNull on the command line too. > > If we ever get a way to pass null on the command line, how would we > think about a struct like this? Will it still feel right, or will it > feel like we feel about simple unions today (they exist, we would like > to get rid of them, but we can't because compatibility)? Personally I really don't like the idea of using "new-secret:null" as a way to request deletion of a keyslot. That's too magical for an action that is so dangerous to data IMhO. I think of these operations as activating & deactivating keyslots, hence my suggestion to use an explicit "active: true|false" to associate the core action being performed, instead of inferring the action indirectly from the secret. I think this could lend itself better to future extensions too. eg currently we're just activating or deactivating a keyslot. it is conceivable in future (LUKS2) we might want to modify an existing keyslot in some way. In that scenario, "active" can be updated to be allowed to be optional such that: - active: true -> activate a currently inactive keyslot - active: false -> deactivate a currently active keyslot - active omitted -> modify a currently active keyslot Regards, Daniel
On Tue, 2020-01-28 at 17:21 +0000, Daniel P. Berrangé wrote: > On Tue, Jan 14, 2020 at 09:33:39PM +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 | 374 +++++++++++++++++++++++++++++++++++++++++++- > > qapi/crypto.json | 50 +++++- > > 2 files changed, 421 insertions(+), 3 deletions(-) > > > > diff --git a/crypto/block-luks.c b/crypto/block-luks.c > > index 4861db810c..349e95fed3 100644 > > --- a/crypto/block-luks.c > > +++ b/crypto/block-luks.c > > @@ -32,6 +32,7 @@ > > #include "qemu/uuid.h" > > > > #include "qemu/coroutine.h" > > +#include "qemu/bitmap.h" > > > > /* > > * Reference for the LUKS format implemented here is > > @@ -70,6 +71,9 @@ typedef struct QCryptoBlockLUKSKeySlot QCryptoBlockLUKSKeySlot; > > > > #define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL > > > > +#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS 2000 > > +#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 +223,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; > > }; > > > > > > @@ -1069,6 +1076,112 @@ qcrypto_block_luks_find_key(QCryptoBlock *block, > > return -1; > > } > > > > +/* > > + * Returns true if a slot i is marked as active > > + * (contains encrypted copy of the master key) > > + */ > > +static bool > > +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; > > +} > > + > > +/* > > + * Returns the number of slots that are marked as active > > + * (slots that contain encrypted copy of the master key) > > + */ > > +static unsigned int > > +qcrypto_block_luks_count_active_slots(const QCryptoBlockLUKS *luks) > > +{ > > + size_t i = 0; > > + unsigned int ret = 0; > > + > > + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { > > + if (qcrypto_block_luks_slot_active(luks, i)) { > > + ret++; > > + } > > + } > > + return ret; > > +} > > + > > +/* > > + * Finds first key slot which is not active > > + * Returns the key slot index, or -1 if it doesn't exist > > + */ > > +static int > > +qcrypto_block_luks_find_free_keyslot(const QCryptoBlockLUKS *luks) > > +{ > > + size_t i; > > + > > + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { > > + if (!qcrypto_block_luks_slot_active(luks, i)) { > > + return i; > > + } > > + } > > + return -1; > > + > > +} > > + > > +/* > > + * 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; > > + > > + 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; > > + > > + qcrypto_block_luks_store_header(block, writefunc, opaque, errp); > > + > > + /* > > + * 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, errp) < 0) { > > + /* > > + * If we failed to get the random data, still write > > + * at least zeros to the key slot at least once > > + */ > > + if (i > 0) { > > + return -1; > > + } > > + } > > + > > + if (writefunc(block, > > + slot->key_offset_sector * QCRYPTO_BLOCK_LUKS_SECTOR_SIZE, > > + garbagesplitkey, > > + splitkeylen, > > + opaque, > > + errp) != splitkeylen) { > > + return -1; > > + } > > + } > > + return 0; > > +} > > > > static int > > qcrypto_block_luks_open(QCryptoBlock *block, > > @@ -1099,6 +1212,7 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > > > luks = g_new0(QCryptoBlockLUKS, 1); > > block->opaque = luks; > > + luks->secret = g_strdup(options->u.luks.key_secret); > > > > if (qcrypto_block_luks_load_header(block, readfunc, opaque, errp) < 0) { > > goto fail; > > @@ -1164,6 +1278,7 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > fail: > > qcrypto_block_free_cipher(block); > > qcrypto_ivgen_free(block->ivgen); > > + g_free(luks->secret); > > g_free(luks); > > return -1; > > } > > @@ -1204,7 +1319,7 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > > > memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts)); > > if (!luks_opts.has_iter_time) { > > - luks_opts.iter_time = 2000; > > + luks_opts.iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS; > > } > > if (!luks_opts.has_cipher_alg) { > > luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256; > > @@ -1244,6 +1359,8 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > optprefix ? optprefix : ""); > > goto error; > > } > > + luks->secret = g_strdup(options->u.luks.key_secret); > > + > > password = qcrypto_secret_lookup_as_utf8(luks_opts.key_secret, errp); > > if (!password) { > > goto error; > > @@ -1471,10 +1588,260 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > qcrypto_block_free_cipher(block); > > qcrypto_ivgen_free(block->ivgen); > > > > + g_free(luks->secret); > > g_free(luks); > > return -1; > > } > > > > +/* > > + * Given LUKSKeyslotUpdate command, return @slots_bitmap with all slots > > + * that will be updated with new password (or erased) > > + * returns number of affected slots > > + */ > > +static int qcrypto_block_luks_get_slots_bitmap(QCryptoBlock *block, > > + QCryptoBlockReadFunc readfunc, > > + void *opaque, > > + const LUKSKeyslotUpdate *command, > > + unsigned long *slots_bitmap, > > + Error **errp) > > +{ > > + const QCryptoBlockLUKS *luks = block->opaque; > > + size_t i; > > + int ret = 0; > > + > > + if (command->has_keyslot) { > > + /* keyslot set, select only this keyslot */ > > + int keyslot = command->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); > > + goto error; > > + } > > + bitmap_set(slots_bitmap, keyslot, 1); > > + ret++; > > + > > + } else if (command->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); > > + ret++; > > + } > > + } > > + } 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"); > > + goto error; > > + } > > + bitmap_set(slots_bitmap, slot, 1); > > + ret++; > > + } > > + > > + if (command->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(command->old_secret, > > + errp); > > + if (!old_password) { > > + goto error; > > + } > > + > > + rv = qcrypto_block_luks_load_key(block, > > + i, > > + old_password, > > + tmpkey, > > + readfunc, > > + opaque, > > + errp); > > + if (rv == -1) > > + goto error; > > + else if (rv == 0) { > > + bitmap_clear(slots_bitmap, i, 1); > > + ret--; > > + } > > + } > > + } > > + return ret; > > +error: > > + return -1; > > +} > > + > > +/* > > + * Apply a single keyslot update command as described in @command > > + * Optionally use @unlock_secret to retrieve the master key > > + */ > > +static int > > +qcrypto_block_luks_apply_keyslot_update(QCryptoBlock *block, > > + QCryptoBlockReadFunc readfunc, > > + QCryptoBlockWriteFunc writefunc, > > + void *opaque, > > + LUKSKeyslotUpdate *command, > > + const char *unlock_secret, > > + uint8_t **master_key, > > + bool force, > > + Error **errp) > > +{ > > + QCryptoBlockLUKS *luks = block->opaque; > > + g_autofree unsigned long *slots_bitmap = NULL; > > + int64_t iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS; > > + int slot_count; > > + size_t i; > > + char *new_password; > > + bool erasing; > > + > > + slots_bitmap = bitmap_new(QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS); > > + slot_count = qcrypto_block_luks_get_slots_bitmap(block, readfunc, opaque, > > + command, slots_bitmap, > > + errp); > > + if (slot_count == -1) { > > + goto error; > > + } > > + /* no matching slots, so nothing to do */ > > + if (slot_count == 0) { > > + error_setg(errp, "Requested operation didn't match any slots"); > > + goto error; > > + } > > + /* > > + * slot is erased when the password is set to null, or empty string > > + * (for compatibility with command line) > > + */ > > + erasing = command->new_secret->type == QTYPE_QNULL || > > + strlen(command->new_secret->u.s) == 0; > > + > > + /* safety checks */ > > + if (!force) { > > + if (erasing) { > > + if (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"); > > + goto error; > > + } > > + } else { > > + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { > > + if (!test_bit(i, slots_bitmap)) { > > + continue; > > + } > > + if (qcrypto_block_luks_slot_active(luks, i)) { > > + error_setg(errp, > > + "Refusing to overwrite active slot %zu - " > > + "please erase it first", i); > > + goto error; > > + } > > + } > > + } > > + } > > + > > + /* setup the data needed for storing the new keyslot */ > > + if (!erasing) { > > + /* Load the master key if it wasn't already loaded */ > > + if (!*master_key) { > > + g_autofree char *old_password; > > + old_password = qcrypto_secret_lookup_as_utf8(unlock_secret, errp); > > + if (!old_password) { > > + goto error; > > + } > > + *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"); > > + goto error; > > + } > > + } > > + new_password = qcrypto_secret_lookup_as_utf8(command->new_secret->u.s, > > + errp); > > + if (!new_password) { > > + goto error; > > + } > > + if (command->has_iter_time) { > > + iter_time = command->iter_time; > > + } > > + } > > + > > + /* new apply the update */ > > + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { > > + if (!test_bit(i, slots_bitmap)) { > > + continue; > > + } > > + if (erasing) { > > + if (qcrypto_block_luks_erase_key(block, i, > > + writefunc, > > + opaque, > > + errp)) { > > + error_append_hint(errp, "Failed to erase keyslot %zu", i); > > + goto error; > > + } > > + } else { > > + if (qcrypto_block_luks_store_key(block, i, > > + new_password, > > + *master_key, > > + iter_time, > > + writefunc, > > + opaque, > > + errp)) { > > + error_append_hint(errp, "Failed to write to keyslot %zu", i); > > + goto error; > > + } > > + } > > + } > > + return 0; > > +error: > > + return -EINVAL; > > +} > > I feel the this method is confusing from trying to handle both > adding and erasing keyslots.... > > > > + > > +static int > > +qcrypto_block_luks_amend_options(QCryptoBlock *block, > > + QCryptoBlockReadFunc readfunc, > > + QCryptoBlockWriteFunc writefunc, > > + void *opaque, > > + QCryptoBlockAmendOptions *options, > > + bool force, > > + Error **errp) > > +{ > > + QCryptoBlockLUKS *luks = block->opaque; > > + QCryptoBlockAmendOptionsLUKS *options_luks = &options->u.luks; > > + LUKSKeyslotUpdateList *ptr; > > + g_autofree uint8_t *master_key = NULL; > > + int ret; > > + > > + char *unlock_secret = options_luks->has_unlock_secret ? > > + options_luks->unlock_secret : > > + luks->secret; > > + > > + for (ptr = options_luks->keys; ptr; ptr = ptr->next) { > > + ret = qcrypto_block_luks_apply_keyslot_update(block, readfunc, > > + writefunc, opaque, > > + ptr->value, > > + unlock_secret, > > + &master_key, > > + force, errp); > > .... imho we sould do > > bool erasing = command->new_secret->type == QTYPE_QNULL; > > if (erasing) { > ret = qcrypto_block_luks_disable_keyslot(block, readfunc, > writefunc, opaque, > ptr->value, > unlock_secret, > &master_key, > force, errp); > } else { > ret = qcrypto_block_luks_enable_keyslot(block, readfunc, > writefunc, opaque, > ptr->value, > unlock_secret, > &master_key, > force, errp); > } I implemented something like that now, I'll send this in next version of the patches. > > > + > > + if (ret != 0) { > > + goto error; > > + } > > + } > > + return 0; > > +error: > > + return -1; > > +} > > If there's no code to run in the 'error' label, then we should just > get rid of it and 'return -1' instead of "goto error" Old habit. Fixed now. > > > > > static int qcrypto_block_luks_get_info(QCryptoBlock *block, > > QCryptoBlockInfo *info, > > @@ -1523,7 +1890,9 @@ static int qcrypto_block_luks_get_info(QCryptoBlock *block, > > > > static void qcrypto_block_luks_cleanup(QCryptoBlock *block) > > { > > - g_free(block->opaque); > > + QCryptoBlockLUKS *luks = block->opaque; > > + g_free(luks->secret); > > Check if "luks" is non-NULL for robustness in early failure scenarios 100% agree. Fixed. > > > + g_free(luks); > > } > > > > Regards, > Daniel Thanks for the review, Best regards, Maxim Levitsky
Am 30.01.2020 um 13:53 hat Daniel P. Berrangé geschrieben: > On Thu, Jan 30, 2020 at 01:38:47PM +0100, Kevin Wolf wrote: > > Am 28.01.2020 um 18:32 hat Daniel P. Berrangé geschrieben: > > > On Tue, Jan 28, 2020 at 05:11:16PM +0000, Daniel P. Berrangé wrote: > > > > On Tue, Jan 21, 2020 at 03:13:01PM +0200, Maxim Levitsky wrote: > > > > > On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote: > > > > > > > > > > <trimmed> > > > > > > > > > > > > +## > > > > > > > +# @LUKSKeyslotUpdate: > > > > > > > +# > > > > > > > +# @keyslot: If specified, will update only keyslot with this index > > > > > > > +# > > > > > > > +# @old-secret: If specified, will only update keyslots that > > > > > > > +# can be opened with password which is contained in > > > > > > > +# QCryptoSecret with @old-secret ID > > > > > > > +# > > > > > > > +# If neither @keyslot nor @old-secret is specified, > > > > > > > +# first empty keyslot is selected for the update > > > > > > > +# > > > > > > > +# @new-secret: The ID of a QCryptoSecret object providing a new decryption > > > > > > > +# key to place in all matching keyslots. > > > > > > > +# null/empty string erases all matching keyslots > > > > > > > > > > > > I hate making the empty string do something completely different than a > > > > > > non-empty string. > > > > > > > > > > > > What about making @new-secret optional, and have absent @new-secret > > > > > > erase? > > > > > > > > > > I don't remember already why I and Keven Wolf decided to do this this way, but I think that you are right here. > > > > > I don't mind personally to do this this way. > > > > > empty string though is my addition, since its not possible to pass null on command line. > > > > > > > > IIUC this a result of using "StrOrNull" for this one field... > > > > > > > > > > > > > > > +# Since: 5.0 > > > > > > > +## > > > > > > > +{ 'struct': 'LUKSKeyslotUpdate', > > > > > > > + 'data': { > > > > > > > + '*keyslot': 'int', > > > > > > > + '*old-secret': 'str', > > > > > > > + 'new-secret' : 'StrOrNull', > > > > > > > + '*iter-time' : 'int' } } > > > > > > > > It looks wierd here to be special casing "new-secret" to "StrOrNull" > > > > instead of just marking it as an optional string field > > > > > > > > "*new-secret": "str" > > > > > > > > which would be possible to use from the command line, as you simply > > > > omit the field. > > > > > > > > I guess the main danger here is that we're using this as a trigger > > > > to erase keyslots. So simply omitting "new-secret" can result > > > > in damage to the volume by accident which is not an attractive > > > > mode. > > > > Right. It's been a while since I discussed this with Maxim, but I think > > this was the motivation for me to suggest an explicit null value. > > > > As long as we don't support passing null from the command line, I see > > the problem with it, though. Empty string (which I think we didn't > > discuss before) looks like a reasonable enough workaround to me, but if > > you think this is too much magic, then maybe not. > > > > > Thinking about this again, I really believe we ought to be moire > > > explicit about disabling the keyslot by having the "active" field. > > > eg > > > > > > { 'struct': 'LUKSKeyslotUpdate', > > > 'data': { > > > 'active': 'bool', > > > '*keyslot': 'int', > > > '*old-secret': 'str', > > > '*new-secret' : 'str', > > > '*iter-time' : 'int' } } > > > > > > "new-secret" is thus only needed when "active" == true. > > > > Hm. At the very least, I would make 'active' optional and default to > > true, so that for adding or updating you must only specify 'new-secret' > > and for deleting only 'active'. > > Is that asymmetry really worth while ? It merely saves a few > characters of typing by omitting "active: true", so I'm not > really convinced. > > > > > > This avoids the problem with being unable to specify a null for > > > StrOrNull on the command line too. > > > > If we ever get a way to pass null on the command line, how would we > > think about a struct like this? Will it still feel right, or will it > > feel like we feel about simple unions today (they exist, we would like > > to get rid of them, but we can't because compatibility)? > > Personally I really don't like the idea of using "new-secret:null" > as a way to request deletion of a keyslot. That's too magical > for an action that is so dangerous to data IMhO. > > I think of these operations as activating & deactivating keyslots, > hence my suggestion to use an explicit "active: true|false" to > associate the core action being performed, instead of inferring > the action indirectly from the secret. The general idea of the amend interface is more that you describe a desired state rather than operations to achieve it. > I think this could lend itself better to future extensions too. > eg currently we're just activating or deactivating a keyslot. > it is conceivable in future (LUKS2) we might want to modify an > existing keyslot in some way. In that scenario, "active" can > be updated to be allowed to be optional such that: > > - active: true -> activate a currently inactive keyslot > - active: false -> deactivate a currently active keyslot > - active omitted -> modify a currently active keyslot This distinction feels artificial to me. All three operations just change the content of a keyslot. Whether it contained a key or not in the old state shouldn't make a difference for how to get a new value (which could be a new key or just an empty keyslot) written to it. Making an omitted key mean something different from the other options so that it's not just defaulting to one of them is problematic, too. We have at least one place where it works like this (backing files) and it tends to give us headaches. Kevin
On Thu, Jan 30, 2020 at 03:23:10PM +0100, Kevin Wolf wrote: > Am 30.01.2020 um 13:53 hat Daniel P. Berrangé geschrieben: > > On Thu, Jan 30, 2020 at 01:38:47PM +0100, Kevin Wolf wrote: > > > Am 28.01.2020 um 18:32 hat Daniel P. Berrangé geschrieben: > > > > On Tue, Jan 28, 2020 at 05:11:16PM +0000, Daniel P. Berrangé wrote: > > > > > On Tue, Jan 21, 2020 at 03:13:01PM +0200, Maxim Levitsky wrote: > > > > > > On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote: > > > > > > > > > > > > <trimmed> > > > > > > > > > > > > > > +## > > > > > > > > +# @LUKSKeyslotUpdate: > > > > > > > > +# > > > > > > > > +# @keyslot: If specified, will update only keyslot with this index > > > > > > > > +# > > > > > > > > +# @old-secret: If specified, will only update keyslots that > > > > > > > > +# can be opened with password which is contained in > > > > > > > > +# QCryptoSecret with @old-secret ID > > > > > > > > +# > > > > > > > > +# If neither @keyslot nor @old-secret is specified, > > > > > > > > +# first empty keyslot is selected for the update > > > > > > > > +# > > > > > > > > +# @new-secret: The ID of a QCryptoSecret object providing a new decryption > > > > > > > > +# key to place in all matching keyslots. > > > > > > > > +# null/empty string erases all matching keyslots > > > > > > > > > > > > > > I hate making the empty string do something completely different than a > > > > > > > non-empty string. > > > > > > > > > > > > > > What about making @new-secret optional, and have absent @new-secret > > > > > > > erase? > > > > > > > > > > > > I don't remember already why I and Keven Wolf decided to do this this way, but I think that you are right here. > > > > > > I don't mind personally to do this this way. > > > > > > empty string though is my addition, since its not possible to pass null on command line. > > > > > > > > > > IIUC this a result of using "StrOrNull" for this one field... > > > > > > > > > > > > > > > > > > +# Since: 5.0 > > > > > > > > +## > > > > > > > > +{ 'struct': 'LUKSKeyslotUpdate', > > > > > > > > + 'data': { > > > > > > > > + '*keyslot': 'int', > > > > > > > > + '*old-secret': 'str', > > > > > > > > + 'new-secret' : 'StrOrNull', > > > > > > > > + '*iter-time' : 'int' } } > > > > > > > > > > It looks wierd here to be special casing "new-secret" to "StrOrNull" > > > > > instead of just marking it as an optional string field > > > > > > > > > > "*new-secret": "str" > > > > > > > > > > which would be possible to use from the command line, as you simply > > > > > omit the field. > > > > > > > > > > I guess the main danger here is that we're using this as a trigger > > > > > to erase keyslots. So simply omitting "new-secret" can result > > > > > in damage to the volume by accident which is not an attractive > > > > > mode. > > > > > > Right. It's been a while since I discussed this with Maxim, but I think > > > this was the motivation for me to suggest an explicit null value. > > > > > > As long as we don't support passing null from the command line, I see > > > the problem with it, though. Empty string (which I think we didn't > > > discuss before) looks like a reasonable enough workaround to me, but if > > > you think this is too much magic, then maybe not. > > > > > > > Thinking about this again, I really believe we ought to be moire > > > > explicit about disabling the keyslot by having the "active" field. > > > > eg > > > > > > > > { 'struct': 'LUKSKeyslotUpdate', > > > > 'data': { > > > > 'active': 'bool', > > > > '*keyslot': 'int', > > > > '*old-secret': 'str', > > > > '*new-secret' : 'str', > > > > '*iter-time' : 'int' } } > > > > > > > > "new-secret" is thus only needed when "active" == true. > > > > > > Hm. At the very least, I would make 'active' optional and default to > > > true, so that for adding or updating you must only specify 'new-secret' > > > and for deleting only 'active'. > > > > Is that asymmetry really worth while ? It merely saves a few > > characters of typing by omitting "active: true", so I'm not > > really convinced. > > > > > > > > > This avoids the problem with being unable to specify a null for > > > > StrOrNull on the command line too. > > > > > > If we ever get a way to pass null on the command line, how would we > > > think about a struct like this? Will it still feel right, or will it > > > feel like we feel about simple unions today (they exist, we would like > > > to get rid of them, but we can't because compatibility)? > > > > Personally I really don't like the idea of using "new-secret:null" > > as a way to request deletion of a keyslot. That's too magical > > for an action that is so dangerous to data IMhO. > > > > I think of these operations as activating & deactivating keyslots, > > hence my suggestion to use an explicit "active: true|false" to > > associate the core action being performed, instead of inferring > > the action indirectly from the secret. > > The general idea of the amend interface is more that you describe a > desired state rather than operations to achieve it. > > > I think this could lend itself better to future extensions too. > > eg currently we're just activating or deactivating a keyslot. > > it is conceivable in future (LUKS2) we might want to modify an > > existing keyslot in some way. In that scenario, "active" can > > be updated to be allowed to be optional such that: > > > > - active: true -> activate a currently inactive keyslot > > - active: false -> deactivate a currently active keyslot > > - active omitted -> modify a currently active keyslot > > This distinction feels artificial to me. All three operations just > change the content of a keyslot. Whether it contained a key or not in > the old state shouldn't make a difference for how to get a new value > (which could be a new key or just an empty keyslot) written to it. There is an explicit "active" state associated with keyslots on disk and in the LUKS crypto data structures in QEMU. So this is simply exposing "active" as a field in the amend interface, directly. This also matches up with the "qemu-img info" output for a luks volume which reports the "active" state of each key slot. > Making an omitted key mean something different from the other options so > that it's not just defaulting to one of them is problematic, too. We > have at least one place where it works like this (backing files) and it > tends to give us headaches. Omitting "active" in my example above, just means that we're doing something that is not changing the "active" state in the keyslot on disk. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Thu, Jan 30, 2020 at 01:38:47PM +0100, Kevin Wolf wrote: >> Am 28.01.2020 um 18:32 hat Daniel P. Berrangé geschrieben: >> > On Tue, Jan 28, 2020 at 05:11:16PM +0000, Daniel P. Berrangé wrote: >> > > On Tue, Jan 21, 2020 at 03:13:01PM +0200, Maxim Levitsky wrote: >> > > > On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote: >> > > > >> > > > <trimmed> >> > > > >> > > > > > +## >> > > > > > +# @LUKSKeyslotUpdate: >> > > > > > +# >> > > > > > +# @keyslot: If specified, will update only keyslot with this index >> > > > > > +# >> > > > > > +# @old-secret: If specified, will only update keyslots that >> > > > > > +# can be opened with password which is contained in >> > > > > > +# QCryptoSecret with @old-secret ID >> > > > > > +# >> > > > > > +# If neither @keyslot nor @old-secret is specified, >> > > > > > +# first empty keyslot is selected for the update >> > > > > > +# >> > > > > > +# @new-secret: The ID of a QCryptoSecret object providing a new decryption >> > > > > > +# key to place in all matching keyslots. >> > > > > > +# null/empty string erases all matching keyslots >> > > > > >> > > > > I hate making the empty string do something completely different than a >> > > > > non-empty string. >> > > > > >> > > > > What about making @new-secret optional, and have absent @new-secret >> > > > > erase? >> > > > >> > > > I don't remember already why I and Keven Wolf decided to do this this way, but I think that you are right here. >> > > > I don't mind personally to do this this way. >> > > > empty string though is my addition, since its not possible to pass null on command line. >> > > >> > > IIUC this a result of using "StrOrNull" for this one field... >> > > >> > > >> > > > > > +# Since: 5.0 >> > > > > > +## >> > > > > > +{ 'struct': 'LUKSKeyslotUpdate', >> > > > > > + 'data': { >> > > > > > + '*keyslot': 'int', >> > > > > > + '*old-secret': 'str', >> > > > > > + 'new-secret' : 'StrOrNull', >> > > > > > + '*iter-time' : 'int' } } >> > > >> > > It looks wierd here to be special casing "new-secret" to "StrOrNull" >> > > instead of just marking it as an optional string field >> > > >> > > "*new-secret": "str" >> > > >> > > which would be possible to use from the command line, as you simply >> > > omit the field. >> > > >> > > I guess the main danger here is that we're using this as a trigger >> > > to erase keyslots. So simply omitting "new-secret" can result >> > > in damage to the volume by accident which is not an attractive >> > > mode. >> >> Right. It's been a while since I discussed this with Maxim, but I think >> this was the motivation for me to suggest an explicit null value. A bit of redundancy to guard against catastrophic accidents makes sense. We can discuss its shape. >> As long as we don't support passing null from the command line, I see >> the problem with it, though. Empty string (which I think we didn't >> discuss before) looks like a reasonable enough workaround to me, but if >> you think this is too much magic, then maybe not. >> >> > Thinking about this again, I really believe we ought to be moire >> > explicit about disabling the keyslot by having the "active" field. >> > eg >> > >> > { 'struct': 'LUKSKeyslotUpdate', >> > 'data': { >> > 'active': 'bool', >> > '*keyslot': 'int', >> > '*old-secret': 'str', >> > '*new-secret' : 'str', >> > '*iter-time' : 'int' } } >> > >> > "new-secret" is thus only needed when "active" == true. I figure @iter-time, too. >> Hm. At the very least, I would make 'active' optional and default to >> true, so that for adding or updating you must only specify 'new-secret' >> and for deleting only 'active'. > > Is that asymmetry really worth while ? It merely saves a few > characters of typing by omitting "active: true", so I'm not > really convinced. > >> > This avoids the problem with being unable to specify a null for >> > StrOrNull on the command line too. >> >> If we ever get a way to pass null on the command line, how would we >> think about a struct like this? Will it still feel right, or will it >> feel like we feel about simple unions today (they exist, we would like >> to get rid of them, but we can't because compatibility)? > > Personally I really don't like the idea of using "new-secret:null" > as a way to request deletion of a keyslot. That's too magical > for an action that is so dangerous to data IMhO. I tend to agree. Expressing "zap the secret" as '"new-secret": null' is clever and kind of cute, but "clever" and "cute" have no place next to "irrevocably destroy data". > I think of these operations as activating & deactivating keyslots, > hence my suggestion to use an explicit "active: true|false" to > associate the core action being performed, instead of inferring > the action indirectly from the secret. > > I think this could lend itself better to future extensions too. > eg currently we're just activating or deactivating a keyslot. > it is conceivable in future (LUKS2) we might want to modify an > existing keyslot in some way. In that scenario, "active" can > be updated to be allowed to be optional such that: > > - active: true -> activate a currently inactive keyslot > - active: false -> deactivate a currently active keyslot > - active omitted -> modify a currently active keyslot A boolean provides two actions. By making it optional, we can squeeze out a third, at the price of making the interface unintuitive: how would you know what "@active absent" means without looking it up? Why not have an @action of enum type instead? Values "add" and "delete" now (or "activate" and "deactivate", whatever makes the most sense when writing the docs), leaving us room to later add whatever comes up. This also lets us turn LUKSKeyslotUpdate into a union. Brief detour before I sketch that: update safety. Unless writing a keyslot is atomic, i.e. always either succeeds completely, or fails without changing anything, updating a slot in place is dangerous: you may destroy the old key without getting your new one in place. To safely replace an existing secret, you first write the new secret to a free slot, and only when that succeeded, you delete the old one. This leads to the following safe operations: * "Activate": write a secret to a free keyslot (chosen by the system) * "Deactivate": delete an existing secret from all keyslots containing it (commonly just one) Dangerous and unwanted: * Replace existing secret in place Low-level operations we may or may not want to support: * Write a secret to specific keyslot (dangerous unless it's free) * Zap a specific keyslot (hope it contains the secret you think it does) Now let me sketch LUKSKeyslotUpdate as union. First without support for the low-level operations: { state: 'LUKSKeyslotUpdateAction', 'data': [ 'add', 'delete' ] } { 'struct': 'LUKSKeyslotAdd', 'data': { 'secret': 'str', '*iter-time': 'int' } } { 'struct': 'LUKSKeyslotDelete', 'data': { 'secret': 'str' } { 'union: 'LUKSKeyslotUpdate', 'base': { 'action': 'LUKSKeyslotUpdateAction' } 'discriminator': 'action', 'data': { 'add': 'LUKSKeyslotAdd' }, { 'delete': 'LUKSKeyslotDelete' } } Since @secret occurs in all variants, we could also put it in @base instead. Matter of taste. I think this way is clearer. Lets us easily add a variant that doesn't want @secret later on (although moving it from @base to variants then would be possible). Compare: * Activate free keyslot to hold a secret { "new-secret": "CIA/GRU/MI6" } vs. { "active": true, "new-secret": "CIA/GRU/MI6" } vs. { "action": "add", "secret": "CIA/GRU/MI6" } * Deactivate keyslots holding a secret { "old-secret": "CIA/GRU/MI6", "new-secret": null } vs. { "active": false, "old-secret": "CIA/GRU/MI6" } vs. { "action": "delete", "secret": "CIA/GRU/MI6" } * Replace existing secret in-place (unsafe!) { "old-secret": "OSS/NKVD/MI6", "new-secret": "CIA/GRU/MI6" } vs. Can't do. Now let's add support for the low-level operations. To "write specific slot" to "add", we add optional LUKSKeyslotAdd member @keyslot to direct the write to that keyslot instead of the first free one: { 'struct': 'LUKSKeyslotAdd', 'data': { 'secret': 'str', '*keyslot': 'int', '*iter-time': 'int' } } To add "zap specific slot" to delete, we need to pass a slot number instead of the old secret. We could add member @keyslot, make both optional, and require users to specify exactly one of them: { 'struct': 'LUKSKeyslotDelete', 'data': { '*secret': 'str', # must pass either @secret '*keyslot': 'int' } } # or @keyslot Or we use an alternate: { 'alternate': 'LUKSKeyslotMatch', 'data': { 'secret': 'str', 'keyslot': 'int' } } { 'struct': 'LUKSKeyslotDelete', 'data': { 'match': 'LUKSKeyslotMatch' } } Hmm, that gets us into trouble with dotted keys, because match=1 could be keyslot#1 or the (truly bad) secret "1". Nevermind. Or we add a separate "zap" action: { state: 'LUKSKeyslotUpdateAction', 'data': [ 'add', 'delete', 'zap' ] } { 'struct': 'LUKSKeyslotZap', 'data': { 'keyslot': 'int' } } { 'union: 'LUKSKeyslotUpdate', 'base': { 'action': 'LUKSKeyslotUpdateAction' } 'discriminator': 'action', 'data': { 'add': 'LUKSKeyslotAdd' }, { 'delete': 'LUKSKeyslotDelete' }, { 'zap': 'LUKSKeyslotZap' } } Compare: * Write to keyslot#1 { "new-secret": "CIA/GRU/MI6", "keyslot": 1 } vs. { "active": true, "new-secret": "CIA/GRU/MI6", "keyslot": 1 } vs. { "action": "add", "secret": "CIA/GRU/MI6", "keyslot": 1 } * Zap keyslot#1 { "keyslot": 1, "new-secret": null } vs. { "active": false, "keyslot": 1 } vs. { "action": "delete", "keyslot": 1 } vs. { "action": "zap", "keyslot": 1 }
Kevin Wolf <kwolf@redhat.com> writes: > Am 30.01.2020 um 13:53 hat Daniel P. Berrangé geschrieben: [...] >> Personally I really don't like the idea of using "new-secret:null" >> as a way to request deletion of a keyslot. That's too magical >> for an action that is so dangerous to data IMhO. >> >> I think of these operations as activating & deactivating keyslots, >> hence my suggestion to use an explicit "active: true|false" to >> associate the core action being performed, instead of inferring >> the action indirectly from the secret. > > The general idea of the amend interface is more that you describe a > desired state rather than operations to achieve it. Point taken. >> I think this could lend itself better to future extensions too. >> eg currently we're just activating or deactivating a keyslot. >> it is conceivable in future (LUKS2) we might want to modify an >> existing keyslot in some way. In that scenario, "active" can >> be updated to be allowed to be optional such that: >> >> - active: true -> activate a currently inactive keyslot >> - active: false -> deactivate a currently active keyslot >> - active omitted -> modify a currently active keyslot > > This distinction feels artificial to me. All three operations just > change the content of a keyslot. Whether it contained a key or not in > the old state shouldn't make a difference for how to get a new value > (which could be a new key or just an empty keyslot) written to it. *If* you can get it to fail only safely. Can you? > Making an omitted key mean something different from the other options so > that it's not just defaulting to one of them is problematic, too. We > have at least one place where it works like this (backing files) and it > tends to give us headaches. Seconded.
On Thu, Jan 30, 2020 at 03:47:00PM +0100, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Thu, Jan 30, 2020 at 01:38:47PM +0100, Kevin Wolf wrote: > >> Am 28.01.2020 um 18:32 hat Daniel P. Berrangé geschrieben: > >> > On Tue, Jan 28, 2020 at 05:11:16PM +0000, Daniel P. Berrangé wrote: > >> > > On Tue, Jan 21, 2020 at 03:13:01PM +0200, Maxim Levitsky wrote: > >> > > > On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote: > >> > > > > >> > > > <trimmed> > >> > > > > >> > > > > > +## > >> > > > > > +# @LUKSKeyslotUpdate: > >> > > > > > +# > >> > > > > > +# @keyslot: If specified, will update only keyslot with this index > >> > > > > > +# > >> > > > > > +# @old-secret: If specified, will only update keyslots that > >> > > > > > +# can be opened with password which is contained in > >> > > > > > +# QCryptoSecret with @old-secret ID > >> > > > > > +# > >> > > > > > +# If neither @keyslot nor @old-secret is specified, > >> > > > > > +# first empty keyslot is selected for the update > >> > > > > > +# > >> > > > > > +# @new-secret: The ID of a QCryptoSecret object providing a new decryption > >> > > > > > +# key to place in all matching keyslots. > >> > > > > > +# null/empty string erases all matching keyslots > >> > > > > > >> > > > > I hate making the empty string do something completely different than a > >> > > > > non-empty string. > >> > > > > > >> > > > > What about making @new-secret optional, and have absent @new-secret > >> > > > > erase? > >> > > > > >> > > > I don't remember already why I and Keven Wolf decided to do this this way, but I think that you are right here. > >> > > > I don't mind personally to do this this way. > >> > > > empty string though is my addition, since its not possible to pass null on command line. > >> > > > >> > > IIUC this a result of using "StrOrNull" for this one field... > >> > > > >> > > > >> > > > > > +# Since: 5.0 > >> > > > > > +## > >> > > > > > +{ 'struct': 'LUKSKeyslotUpdate', > >> > > > > > + 'data': { > >> > > > > > + '*keyslot': 'int', > >> > > > > > + '*old-secret': 'str', > >> > > > > > + 'new-secret' : 'StrOrNull', > >> > > > > > + '*iter-time' : 'int' } } > >> > > > >> > > It looks wierd here to be special casing "new-secret" to "StrOrNull" > >> > > instead of just marking it as an optional string field > >> > > > >> > > "*new-secret": "str" > >> > > > >> > > which would be possible to use from the command line, as you simply > >> > > omit the field. > >> > > > >> > > I guess the main danger here is that we're using this as a trigger > >> > > to erase keyslots. So simply omitting "new-secret" can result > >> > > in damage to the volume by accident which is not an attractive > >> > > mode. > >> > >> Right. It's been a while since I discussed this with Maxim, but I think > >> this was the motivation for me to suggest an explicit null value. > > A bit of redundancy to guard against catastrophic accidents makes sense. > We can discuss its shape. > > >> As long as we don't support passing null from the command line, I see > >> the problem with it, though. Empty string (which I think we didn't > >> discuss before) looks like a reasonable enough workaround to me, but if > >> you think this is too much magic, then maybe not. > >> > >> > Thinking about this again, I really believe we ought to be moire > >> > explicit about disabling the keyslot by having the "active" field. > >> > eg > >> > > >> > { 'struct': 'LUKSKeyslotUpdate', > >> > 'data': { > >> > 'active': 'bool', > >> > '*keyslot': 'int', > >> > '*old-secret': 'str', > >> > '*new-secret' : 'str', > >> > '*iter-time' : 'int' } } > >> > > >> > "new-secret" is thus only needed when "active" == true. > > I figure @iter-time, too. > > >> Hm. At the very least, I would make 'active' optional and default to > >> true, so that for adding or updating you must only specify 'new-secret' > >> and for deleting only 'active'. > > > > Is that asymmetry really worth while ? It merely saves a few > > characters of typing by omitting "active: true", so I'm not > > really convinced. > > > >> > This avoids the problem with being unable to specify a null for > >> > StrOrNull on the command line too. > >> > >> If we ever get a way to pass null on the command line, how would we > >> think about a struct like this? Will it still feel right, or will it > >> feel like we feel about simple unions today (they exist, we would like > >> to get rid of them, but we can't because compatibility)? > > > > Personally I really don't like the idea of using "new-secret:null" > > as a way to request deletion of a keyslot. That's too magical > > for an action that is so dangerous to data IMhO. > > I tend to agree. Expressing "zap the secret" as '"new-secret": null' is > clever and kind of cute, but "clever" and "cute" have no place next to > "irrevocably destroy data". > > > I think of these operations as activating & deactivating keyslots, > > hence my suggestion to use an explicit "active: true|false" to > > associate the core action being performed, instead of inferring > > the action indirectly from the secret. > > > > I think this could lend itself better to future extensions too. > > eg currently we're just activating or deactivating a keyslot. > > it is conceivable in future (LUKS2) we might want to modify an > > existing keyslot in some way. In that scenario, "active" can > > be updated to be allowed to be optional such that: > > > > - active: true -> activate a currently inactive keyslot > > - active: false -> deactivate a currently active keyslot > > - active omitted -> modify a currently active keyslot > > A boolean provides two actions. By making it optional, we can squeeze > out a third, at the price of making the interface unintuitive: how would > you know what "@active absent" means without looking it up? > > Why not have an @action of enum type instead? Values "add" and "delete" > now (or "activate" and "deactivate", whatever makes the most sense when > writing the docs), leaving us room to later add whatever comes up. I probably worded my suggestion badly - "active" should not be thought of as expressing an operation type; it should be considered a direct reflection of the "active" metadat field in a LUKS keyslot on disk. So I should have described it as: - active: true|false -> set the keyslot active state to this value - active omitted -> don't change the keyslot active state The three possible states of the "active" field then happen to provide the way to express the desired operations. > > This also lets us turn LUKSKeyslotUpdate into a union. > > Brief detour before I sketch that: update safety. > > Unless writing a keyslot is atomic, i.e. always either succeeds > completely, or fails without changing anything, updating a slot in place > is dangerous: you may destroy the old key without getting your new one > in place. > > To safely replace an existing secret, you first write the new secret to > a free slot, and only when that succeeded, you delete the old one. > > This leads to the following safe operations: > > * "Activate": write a secret to a free keyslot (chosen by the system) > > * "Deactivate": delete an existing secret from all keyslots containing > it (commonly just one) > > Dangerous and unwanted: > > * Replace existing secret in place > > Low-level operations we may or may not want to support: > > * Write a secret to specific keyslot (dangerous unless it's free) > > * Zap a specific keyslot (hope it contains the secret you think it does) > > Now let me sketch LUKSKeyslotUpdate as union. First without support for > the low-level operations: > > { state: 'LUKSKeyslotUpdateAction', > 'data': [ 'add', 'delete' ] } > { 'struct': 'LUKSKeyslotAdd', > 'data': { 'secret': 'str', > '*iter-time': 'int' } } > { 'struct': 'LUKSKeyslotDelete', > 'data': { 'secret': 'str' } > { 'union: 'LUKSKeyslotUpdate', > 'base': { 'action': 'LUKSKeyslotUpdateAction' } > 'discriminator': 'action', > 'data': { 'add': 'LUKSKeyslotAdd' }, > { 'delete': 'LUKSKeyslotDelete' } } > > Since @secret occurs in all variants, we could also put it in @base > instead. Matter of taste. I think this way is clearer. Lets us easily > add a variant that doesn't want @secret later on (although moving it > from @base to variants then would be possible). This kind of approach is what I originally believed we should do, but it is contrary to the design expectations of the "amend" operation. That is not supposed to be expressing operations, rather expressing the desired state of the resulting disk. Regards, Daniel
On Thu, 2020-01-30 at 15:47 +0100, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Thu, Jan 30, 2020 at 01:38:47PM +0100, Kevin Wolf wrote: > > > Am 28.01.2020 um 18:32 hat Daniel P. Berrangé geschrieben: > > > > On Tue, Jan 28, 2020 at 05:11:16PM +0000, Daniel P. Berrangé wrote: > > > > > On Tue, Jan 21, 2020 at 03:13:01PM +0200, Maxim Levitsky wrote: > > > > > > On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote: > > > > > > > > > > > > <trimmed> > > > > > > > > > > > > > > +## > > > > > > > > +# @LUKSKeyslotUpdate: > > > > > > > > +# > > > > > > > > +# @keyslot: If specified, will update only keyslot with this index > > > > > > > > +# > > > > > > > > +# @old-secret: If specified, will only update keyslots that > > > > > > > > +# can be opened with password which is contained in > > > > > > > > +# QCryptoSecret with @old-secret ID > > > > > > > > +# > > > > > > > > +# If neither @keyslot nor @old-secret is specified, > > > > > > > > +# first empty keyslot is selected for the update > > > > > > > > +# > > > > > > > > +# @new-secret: The ID of a QCryptoSecret object providing a new decryption > > > > > > > > +# key to place in all matching keyslots. > > > > > > > > +# null/empty string erases all matching keyslots > > > > > > > > > > > > > > I hate making the empty string do something completely different than a > > > > > > > non-empty string. > > > > > > > > > > > > > > What about making @new-secret optional, and have absent @new-secret > > > > > > > erase? > > > > > > > > > > > > I don't remember already why I and Keven Wolf decided to do this this way, but I think that you are right here. > > > > > > I don't mind personally to do this this way. > > > > > > empty string though is my addition, since its not possible to pass null on command line. > > > > > > > > > > IIUC this a result of using "StrOrNull" for this one field... > > > > > > > > > > > > > > > > > > +# Since: 5.0 > > > > > > > > +## > > > > > > > > +{ 'struct': 'LUKSKeyslotUpdate', > > > > > > > > + 'data': { > > > > > > > > + '*keyslot': 'int', > > > > > > > > + '*old-secret': 'str', > > > > > > > > + 'new-secret' : 'StrOrNull', > > > > > > > > + '*iter-time' : 'int' } } > > > > > > > > > > It looks wierd here to be special casing "new-secret" to "StrOrNull" > > > > > instead of just marking it as an optional string field > > > > > > > > > > "*new-secret": "str" > > > > > > > > > > which would be possible to use from the command line, as you simply > > > > > omit the field. > > > > > > > > > > I guess the main danger here is that we're using this as a trigger > > > > > to erase keyslots. So simply omitting "new-secret" can result > > > > > in damage to the volume by accident which is not an attractive > > > > > mode. > > > > > > Right. It's been a while since I discussed this with Maxim, but I think > > > this was the motivation for me to suggest an explicit null value. > > A bit of redundancy to guard against catastrophic accidents makes sense. > We can discuss its shape. > > > > As long as we don't support passing null from the command line, I see > > > the problem with it, though. Empty string (which I think we didn't > > > discuss before) looks like a reasonable enough workaround to me, but if > > > you think this is too much magic, then maybe not. > > > > > > > Thinking about this again, I really believe we ought to be moire > > > > explicit about disabling the keyslot by having the "active" field. > > > > eg > > > > > > > > { 'struct': 'LUKSKeyslotUpdate', > > > > 'data': { > > > > 'active': 'bool', > > > > '*keyslot': 'int', > > > > '*old-secret': 'str', > > > > '*new-secret' : 'str', > > > > '*iter-time' : 'int' } } > > > > > > > > "new-secret" is thus only needed when "active" == true. > > I figure @iter-time, too. > > > > Hm. At the very least, I would make 'active' optional and default to > > > true, so that for adding or updating you must only specify 'new-secret' > > > and for deleting only 'active'. > > > > Is that asymmetry really worth while ? It merely saves a few > > characters of typing by omitting "active: true", so I'm not > > really convinced. > > > > > > This avoids the problem with being unable to specify a null for > > > > StrOrNull on the command line too. > > > > > > If we ever get a way to pass null on the command line, how would we > > > think about a struct like this? Will it still feel right, or will it > > > feel like we feel about simple unions today (they exist, we would like > > > to get rid of them, but we can't because compatibility)? > > > > Personally I really don't like the idea of using "new-secret:null" > > as a way to request deletion of a keyslot. That's too magical > > for an action that is so dangerous to data IMhO. > > I tend to agree. Expressing "zap the secret" as '"new-secret": null' is > clever and kind of cute, but "clever" and "cute" have no place next to > "irrevocably destroy data". > > > I think of these operations as activating & deactivating keyslots, > > hence my suggestion to use an explicit "active: true|false" to > > associate the core action being performed, instead of inferring > > the action indirectly from the secret. > > > > I think this could lend itself better to future extensions too. > > eg currently we're just activating or deactivating a keyslot. > > it is conceivable in future (LUKS2) we might want to modify an > > existing keyslot in some way. In that scenario, "active" can > > be updated to be allowed to be optional such that: > > > > - active: true -> activate a currently inactive keyslot > > - active: false -> deactivate a currently active keyslot > > - active omitted -> modify a currently active keyslot > > A boolean provides two actions. By making it optional, we can squeeze > out a third, at the price of making the interface unintuitive: how would > you know what "@active absent" means without looking it up? Note that modifying a currently active keyslot is potentially dangerous operation and thus not allowed at all by default unless user passes 'force' parameter. The right safe usage is always to add a new keyslot and then erase the old one(s). > > Why not have an @action of enum type instead? Values "add" and "delete" > now (or "activate" and "deactivate", whatever makes the most sense when > writing the docs), leaving us room to later add whatever comes up. > > This also lets us turn LUKSKeyslotUpdate into a union. > > Brief detour before I sketch that: update safety. > > Unless writing a keyslot is atomic, i.e. always either succeeds > completely, or fails without changing anything, updating a slot in place > is dangerous: you may destroy the old key without getting your new one > in place. Exactly. The keyslot is scattered on the disk. It partially resides in 1st 512 bytes sector as part of 8 keyslot header table, and partially resides in area after that header, where the encrypted master key is stored. Due to anti-forensic algorithm used, that area for each keyslot even takes itself several sectors. Write can fail partially/fully and leave us with broken keyslot. In theory you can restore the old values, but since write failure usually means media error (e.g. bad disk sector), this won't help much. In theory the code can try to write a other keyslot, but that is even uglier. Its best to leave this to user and thus user indeed is supposed to write first to a free keyslot, check that write succeeded and only then erase the old keyslot. > > To safely replace an existing secret, you first write the new secret to > a free slot, and only when that succeeded, you delete the old one. > > This leads to the following safe operations: > > * "Activate": write a secret to a free keyslot (chosen by the system) > > * "Deactivate": delete an existing secret from all keyslots containing > it (commonly just one) This can be dangerous too if last keyslot is erased since that basically guarantees data loss, and therefore also needs 'force' option in my patchset. Exactly > > Dangerous and unwanted: > > * Replace existing secret in place > > Low-level operations we may or may not want to support: > > * Write a secret to specific keyslot (dangerous unless it's free) > > * Zap a specific keyslot (hope it contains the secret you think it does) ^^ and the above is especially bad if erasing last keyslot as explained above. > > Now let me sketch LUKSKeyslotUpdate as union. First without support for > the low-level operations: > > { state: 'LUKSKeyslotUpdateAction', > 'data': [ 'add', 'delete' ] } > { 'struct': 'LUKSKeyslotAdd', > 'data': { 'secret': 'str', > '*iter-time': 'int' } } > { 'struct': 'LUKSKeyslotDelete', > 'data': { 'secret': 'str' } > { 'union: 'LUKSKeyslotUpdate', > 'base': { 'action': 'LUKSKeyslotUpdateAction' } > 'discriminator': 'action', > 'data': { 'add': 'LUKSKeyslotAdd' }, > { 'delete': 'LUKSKeyslotDelete' } } > > Since @secret occurs in all variants, we could also put it in @base > instead. Matter of taste. I think this way is clearer. Lets us easily > add a variant that doesn't want @secret later on (although moving it > from @base to variants then would be possible). > > Compare: > > * Activate free keyslot to hold a secret > > { "new-secret": "CIA/GRU/MI6" } > > vs. > > { "active": true, "new-secret": "CIA/GRU/MI6" } > > vs. > > { "action": "add", "secret": "CIA/GRU/MI6" } > > * Deactivate keyslots holding a secret > > { "old-secret": "CIA/GRU/MI6", "new-secret": null } > > vs. > > { "active": false, "old-secret": "CIA/GRU/MI6" } > > vs. > > { "action": "delete", "secret": "CIA/GRU/MI6" } > > * Replace existing secret in-place (unsafe!) > > { "old-secret": "OSS/NKVD/MI6", "new-secret": "CIA/GRU/MI6" } Note that my code doesn't support this currently, and user can do this by first adding a secret and then erasing old one. I can add this just for fun (but only when 'force' is used). > > vs. > > Can't do. > > Now let's add support for the low-level operations. To "write specific > slot" to "add", we add optional LUKSKeyslotAdd member @keyslot to direct > the write to that keyslot instead of the first free one: > > { 'struct': 'LUKSKeyslotAdd', > 'data': { 'secret': 'str', > '*keyslot': 'int', > '*iter-time': 'int' } } OK. > > To add "zap specific slot" to delete, we need to pass a slot number > instead of the old secret. We could add member @keyslot, make both > optional, and require users to specify exactly one of them: > > { 'struct': 'LUKSKeyslotDelete', > 'data': { '*secret': 'str', # must pass either @secret > '*keyslot': 'int' } } # or @keyslot No problem with that. > > Or we use an alternate: > > { 'alternate': 'LUKSKeyslotMatch', > 'data': { 'secret': 'str', > 'keyslot': 'int' } } > { 'struct': 'LUKSKeyslotDelete', > 'data': { 'match': 'LUKSKeyslotMatch' } } > > Hmm, that gets us into trouble with dotted keys, because match=1 could > be keyslot#1 or the (truly bad) secret "1". Nevermind. > > Or we add a separate "zap" action: > > { state: 'LUKSKeyslotUpdateAction', > 'data': [ 'add', 'delete', 'zap' ] } > { 'struct': 'LUKSKeyslotZap', > 'data': { 'keyslot': 'int' } } > { 'union: 'LUKSKeyslotUpdate', > 'base': { 'action': 'LUKSKeyslotUpdateAction' } > 'discriminator': 'action', > 'data': { 'add': 'LUKSKeyslotAdd' }, > { 'delete': 'LUKSKeyslotDelete' }, > { 'zap': 'LUKSKeyslotZap' } } I am not sure I like the 'zap' action, but if this is agreed upon, I won't argue about this. > > Compare: > > * Write to keyslot#1 > > { "new-secret": "CIA/GRU/MI6", "keyslot": 1 } > > vs. > > { "active": true, "new-secret": "CIA/GRU/MI6", "keyslot": 1 } > > vs. > > { "action": "add", "secret": "CIA/GRU/MI6", "keyslot": 1 } > > * Zap keyslot#1 > > { "keyslot": 1, "new-secret": null } > > vs. > > { "active": false, "keyslot": 1 } > > vs. > > { "action": "delete", "keyslot": 1 } > > vs. > > { "action": "zap", "keyslot": 1 } Best regards, Maxim Levitsky
Daniel P. Berrangé <berrange@redhat.com> writes: > On Thu, Jan 30, 2020 at 03:47:00PM +0100, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > On Thu, Jan 30, 2020 at 01:38:47PM +0100, Kevin Wolf wrote: >> >> Am 28.01.2020 um 18:32 hat Daniel P. Berrangé geschrieben: >> >> > On Tue, Jan 28, 2020 at 05:11:16PM +0000, Daniel P. Berrangé wrote: >> >> > > On Tue, Jan 21, 2020 at 03:13:01PM +0200, Maxim Levitsky wrote: >> >> > > > On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote: >> >> > > > >> >> > > > <trimmed> >> >> > > > >> >> > > > > > +## >> >> > > > > > +# @LUKSKeyslotUpdate: >> >> > > > > > +# >> >> > > > > > +# @keyslot: If specified, will update only keyslot with this index >> >> > > > > > +# >> >> > > > > > +# @old-secret: If specified, will only update keyslots that >> >> > > > > > +# can be opened with password which is contained in >> >> > > > > > +# QCryptoSecret with @old-secret ID >> >> > > > > > +# >> >> > > > > > +# If neither @keyslot nor @old-secret is specified, >> >> > > > > > +# first empty keyslot is selected for the update >> >> > > > > > +# >> >> > > > > > +# @new-secret: The ID of a QCryptoSecret object providing a new decryption >> >> > > > > > +# key to place in all matching keyslots. >> >> > > > > > +# null/empty string erases all matching keyslots >> >> > > > > >> >> > > > > I hate making the empty string do something completely different than a >> >> > > > > non-empty string. >> >> > > > > >> >> > > > > What about making @new-secret optional, and have absent @new-secret >> >> > > > > erase? >> >> > > > >> >> > > > I don't remember already why I and Keven Wolf decided to do this this way, but I think that you are right here. >> >> > > > I don't mind personally to do this this way. >> >> > > > empty string though is my addition, since its not possible to pass null on command line. >> >> > > >> >> > > IIUC this a result of using "StrOrNull" for this one field... >> >> > > >> >> > > >> >> > > > > > +# Since: 5.0 >> >> > > > > > +## >> >> > > > > > +{ 'struct': 'LUKSKeyslotUpdate', >> >> > > > > > + 'data': { >> >> > > > > > + '*keyslot': 'int', >> >> > > > > > + '*old-secret': 'str', >> >> > > > > > + 'new-secret' : 'StrOrNull', >> >> > > > > > + '*iter-time' : 'int' } } >> >> > > >> >> > > It looks wierd here to be special casing "new-secret" to "StrOrNull" >> >> > > instead of just marking it as an optional string field >> >> > > >> >> > > "*new-secret": "str" >> >> > > >> >> > > which would be possible to use from the command line, as you simply >> >> > > omit the field. >> >> > > >> >> > > I guess the main danger here is that we're using this as a trigger >> >> > > to erase keyslots. So simply omitting "new-secret" can result >> >> > > in damage to the volume by accident which is not an attractive >> >> > > mode. >> >> >> >> Right. It's been a while since I discussed this with Maxim, but I think >> >> this was the motivation for me to suggest an explicit null value. >> >> A bit of redundancy to guard against catastrophic accidents makes sense. >> We can discuss its shape. >> >> >> As long as we don't support passing null from the command line, I see >> >> the problem with it, though. Empty string (which I think we didn't >> >> discuss before) looks like a reasonable enough workaround to me, but if >> >> you think this is too much magic, then maybe not. >> >> >> >> > Thinking about this again, I really believe we ought to be moire >> >> > explicit about disabling the keyslot by having the "active" field. >> >> > eg >> >> > >> >> > { 'struct': 'LUKSKeyslotUpdate', >> >> > 'data': { >> >> > 'active': 'bool', >> >> > '*keyslot': 'int', >> >> > '*old-secret': 'str', >> >> > '*new-secret' : 'str', >> >> > '*iter-time' : 'int' } } >> >> > >> >> > "new-secret" is thus only needed when "active" == true. >> >> I figure @iter-time, too. >> >> >> Hm. At the very least, I would make 'active' optional and default to >> >> true, so that for adding or updating you must only specify 'new-secret' >> >> and for deleting only 'active'. >> > >> > Is that asymmetry really worth while ? It merely saves a few >> > characters of typing by omitting "active: true", so I'm not >> > really convinced. >> > >> >> > This avoids the problem with being unable to specify a null for >> >> > StrOrNull on the command line too. >> >> >> >> If we ever get a way to pass null on the command line, how would we >> >> think about a struct like this? Will it still feel right, or will it >> >> feel like we feel about simple unions today (they exist, we would like >> >> to get rid of them, but we can't because compatibility)? >> > >> > Personally I really don't like the idea of using "new-secret:null" >> > as a way to request deletion of a keyslot. That's too magical >> > for an action that is so dangerous to data IMhO. >> >> I tend to agree. Expressing "zap the secret" as '"new-secret": null' is >> clever and kind of cute, but "clever" and "cute" have no place next to >> "irrevocably destroy data". >> >> > I think of these operations as activating & deactivating keyslots, >> > hence my suggestion to use an explicit "active: true|false" to >> > associate the core action being performed, instead of inferring >> > the action indirectly from the secret. >> > >> > I think this could lend itself better to future extensions too. >> > eg currently we're just activating or deactivating a keyslot. >> > it is conceivable in future (LUKS2) we might want to modify an >> > existing keyslot in some way. In that scenario, "active" can >> > be updated to be allowed to be optional such that: >> > >> > - active: true -> activate a currently inactive keyslot >> > - active: false -> deactivate a currently active keyslot >> > - active omitted -> modify a currently active keyslot >> >> A boolean provides two actions. By making it optional, we can squeeze >> out a third, at the price of making the interface unintuitive: how would >> you know what "@active absent" means without looking it up? >> >> Why not have an @action of enum type instead? Values "add" and "delete" >> now (or "activate" and "deactivate", whatever makes the most sense when >> writing the docs), leaving us room to later add whatever comes up. > > I probably worded my suggestion badly - "active" should not be > thought of as expressing an operation type; it should be considered > a direct reflection of the "active" metadat field in a LUKS keyslot > on disk. > > So I should have described it as: > > - active: true|false -> set the keyslot active state to this value > - active omitted -> don't change the keyslot active state > > The three possible states of the "active" field then happen to > provide the way to express the desired operations. > >> >> This also lets us turn LUKSKeyslotUpdate into a union. >> >> Brief detour before I sketch that: update safety. >> >> Unless writing a keyslot is atomic, i.e. always either succeeds >> completely, or fails without changing anything, updating a slot in place >> is dangerous: you may destroy the old key without getting your new one >> in place. >> >> To safely replace an existing secret, you first write the new secret to >> a free slot, and only when that succeeded, you delete the old one. >> >> This leads to the following safe operations: >> >> * "Activate": write a secret to a free keyslot (chosen by the system) >> >> * "Deactivate": delete an existing secret from all keyslots containing >> it (commonly just one) >> >> Dangerous and unwanted: >> >> * Replace existing secret in place >> >> Low-level operations we may or may not want to support: >> >> * Write a secret to specific keyslot (dangerous unless it's free) >> >> * Zap a specific keyslot (hope it contains the secret you think it does) >> >> Now let me sketch LUKSKeyslotUpdate as union. First without support for >> the low-level operations: >> >> { state: 'LUKSKeyslotUpdateAction', >> 'data': [ 'add', 'delete' ] } >> { 'struct': 'LUKSKeyslotAdd', >> 'data': { 'secret': 'str', >> '*iter-time': 'int' } } >> { 'struct': 'LUKSKeyslotDelete', >> 'data': { 'secret': 'str' } >> { 'union: 'LUKSKeyslotUpdate', >> 'base': { 'action': 'LUKSKeyslotUpdateAction' } >> 'discriminator': 'action', >> 'data': { 'add': 'LUKSKeyslotAdd' }, >> { 'delete': 'LUKSKeyslotDelete' } } >> >> Since @secret occurs in all variants, we could also put it in @base >> instead. Matter of taste. I think this way is clearer. Lets us easily >> add a variant that doesn't want @secret later on (although moving it >> from @base to variants then would be possible). > > > This kind of approach is what I originally believed we > should do, but it is contrary to the design expectations > of the "amend" operation. That is not supposed to be > expressing operations, rather expressing the desired > state of the resulting disk. I got that now, so let's talk state. A keyslot can be either inactive or active. Let's start low-level, i.e. we specify the slot by slot#: state new state action inactive inactive nop inactive active put secret, iter-time, mark active active inactive mark inactive (effectively deletes secret) active active in general, error (unsafe update in place) we can make it a nop when secret, iter-time remain unchanged we can allow unsafe update with force: true As struct: { 'struct': 'LUKSKeyslotUpdate', 'data': { 'active': 'bool', # could do enum instead 'keyslot': 'int', '*secret': 'str', # present if @active is true '*iter-time': 'int' } } # absent if @active is false As union: { 'enum': 'LUKSKeyslotState', 'data': [ 'active', 'inactive' ] } { 'struct': 'LUKSKeyslotActive', 'data': { 'secret': 'str', '*iter-time': 'int } } { 'union': 'LUKSKeyslotAmend', 'base': { 'state': 'LUKSKeyslotState' } # must do enum 'discriminator': 'state', 'data': { 'active': 'LUKSKeyslotActive' } } When we don't specify the slot#, then "new state active" selects an inactive slot (chosen by the system, and "new state inactive selects slots by secret (commonly just one slot). New state active: state new state action inactive active put secret, iter-time, mark active active active N/A (system choses inactive slot) New state inactive, for each slot holding the specified secret: state new state action inactive inactive N/A (inactive slot holds no secret) active inactive mark inactive (effectively deletes secret) As struct: { 'struct': 'LUKSKeyslotUpdate', 'data': { 'active': 'bool', # could do enum instead '*keyslot': 'int', '*secret': 'str', # present if @active is true '*iter-time': 'int' } } # absent if @active is false As union: { 'enum': 'LUKSKeyslotState', 'data': [ 'active', 'inactive' ] } { 'struct': 'LUKSKeyslotActive', 'data': { 'secret': 'str', '*iter-time': 'int } } { 'union': 'LUKSKeyslotAmend', 'base': { '*keyslot': 'int', 'state': 'LUKSKeyslotState' } 'discriminator': 'state', 'data': { 'active': 'LUKSKeyslotActive' } } Union looks more complicated because our union notation sucks[*]. I like it anyway, because you don't have to explain when which optional members aren't actually optional. Regardless of struct vs. union, this supports an active -> active transition only with an explicit keyslot. Feels fine to me. If we want to support it without keyslot as well, we need a way to specify both old and new secret. Do we? [*] I hope to fix that one day. It's not even hard.
Daniel, Kevin, any comments or objections to the QAPI schema design
sketch developed below?
For your convenience, here's the result again:
{ 'enum': 'LUKSKeyslotState',
'data': [ 'active', 'inactive' ] }
{ 'struct': 'LUKSKeyslotActive',
'data': { 'secret': 'str',
'*iter-time': 'int } }
{ 'union': 'LUKSKeyslotAmend',
'base': { '*keyslot': 'int',
'state': 'LUKSKeyslotState' }
'discriminator': 'state',
'data': { 'active': 'LUKSKeyslotActive' } }
Markus Armbruster <armbru@redhat.com> writes:
[...]
> A keyslot can be either inactive or active.
>
> Let's start low-level, i.e. we specify the slot by slot#:
>
> state new state action
> inactive inactive nop
> inactive active put secret, iter-time, mark active
> active inactive mark inactive (effectively deletes secret)
> active active in general, error (unsafe update in place)
> we can make it a nop when secret, iter-time
> remain unchanged
> we can allow unsafe update with force: true
>
> As struct:
>
> { 'struct': 'LUKSKeyslotUpdate',
> 'data': { 'active': 'bool', # could do enum instead
> 'keyslot': 'int',
> '*secret': 'str', # present if @active is true
> '*iter-time': 'int' } } # absent if @active is false
>
> As union:
>
> { 'enum': 'LUKSKeyslotState',
> 'data': [ 'active', 'inactive' ] }
> { 'struct': 'LUKSKeyslotActive',
> 'data': { 'secret': 'str',
> '*iter-time': 'int } }
> { 'union': 'LUKSKeyslotAmend',
> 'base': { 'state': 'LUKSKeyslotState' } # must do enum
> 'discriminator': 'state',
> 'data': { 'active': 'LUKSKeyslotActive' } }
>
> When we don't specify the slot#, then "new state active" selects an
> inactive slot (chosen by the system, and "new state inactive selects
> slots by secret (commonly just one slot).
>
> New state active:
>
> state new state action
> inactive active put secret, iter-time, mark active
> active active N/A (system choses inactive slot)
>
> New state inactive, for each slot holding the specified secret:
>
> state new state action
> inactive inactive N/A (inactive slot holds no secret)
> active inactive mark inactive (effectively deletes secret)
>
> As struct:
>
> { 'struct': 'LUKSKeyslotUpdate',
> 'data': { 'active': 'bool', # could do enum instead
> '*keyslot': 'int',
> '*secret': 'str', # present if @active is true
> '*iter-time': 'int' } } # absent if @active is false
>
> As union:
>
> { 'enum': 'LUKSKeyslotState',
> 'data': [ 'active', 'inactive' ] }
> { 'struct': 'LUKSKeyslotActive',
> 'data': { 'secret': 'str',
> '*iter-time': 'int } }
> { 'union': 'LUKSKeyslotAmend',
> 'base': { '*keyslot': 'int',
> 'state': 'LUKSKeyslotState' }
> 'discriminator': 'state',
> 'data': { 'active': 'LUKSKeyslotActive' } }
>
> Union looks more complicated because our union notation sucks[*]. I
> like it anyway, because you don't have to explain when which optional
> members aren't actually optional.
>
> Regardless of struct vs. union, this supports an active -> active
> transition only with an explicit keyslot. Feels fine to me. If we want
> to support it without keyslot as well, we need a way to specify both old
> and new secret. Do we?
>
>
> [*] I hope to fix that one day. It's not even hard.
Am 05.02.2020 um 09:24 hat Markus Armbruster geschrieben: > Daniel, Kevin, any comments or objections to the QAPI schema design > sketch developed below? > > For your convenience, here's the result again: > > { 'enum': 'LUKSKeyslotState', > 'data': [ 'active', 'inactive' ] } > { 'struct': 'LUKSKeyslotActive', > 'data': { 'secret': 'str', > '*iter-time': 'int } } > { 'union': 'LUKSKeyslotAmend', > 'base': { '*keyslot': 'int', > 'state': 'LUKSKeyslotState' } > 'discriminator': 'state', > 'data': { 'active': 'LUKSKeyslotActive' } } I think one of the requirements was that you can specify the keyslot not only by using its number, but also by specifying the old secret. Trivial extension, you just get another optional field that can be specified instead of 'keyslot'. Resulting commands: Adding a key: qemu-img amend -o encrypt.keys.0.state=active,encrypt.keys.0.secret=sec0 test.qcow2 Deleting a key: qemu-img amend -o encrypt.keys.0.state=inactive,encrypt.keys.0.keyslot=2 test.qcow2 Previous version (if this series is applied unchanged): Adding a key: qemu-img amend -o encrypt.keys.0.new-secret=sec0 test.qcow2 Deleting a key: qemu-img amend -o encrypt.keys.0.new-secret=,encrypt.keys.0.keyslot=2 test.qcow2 Adding a key gets more complicated with your proposed interface because state must be set explicitly now whereas before it was derived automatically from the fact that if you give a key, only active makes sense. Deleting stays more or less the same, you just change the state instead of clearing the secret. Kevin > Markus Armbruster <armbru@redhat.com> writes: > > [...] > > A keyslot can be either inactive or active. > > > > Let's start low-level, i.e. we specify the slot by slot#: > > > > state new state action > > inactive inactive nop > > inactive active put secret, iter-time, mark active > > active inactive mark inactive (effectively deletes secret) > > active active in general, error (unsafe update in place) > > we can make it a nop when secret, iter-time > > remain unchanged > > we can allow unsafe update with force: true > > > > As struct: > > > > { 'struct': 'LUKSKeyslotUpdate', > > 'data': { 'active': 'bool', # could do enum instead > > 'keyslot': 'int', > > '*secret': 'str', # present if @active is true > > '*iter-time': 'int' } } # absent if @active is false > > > > As union: > > > > { 'enum': 'LUKSKeyslotState', > > 'data': [ 'active', 'inactive' ] } > > { 'struct': 'LUKSKeyslotActive', > > 'data': { 'secret': 'str', > > '*iter-time': 'int } } > > { 'union': 'LUKSKeyslotAmend', > > 'base': { 'state': 'LUKSKeyslotState' } # must do enum > > 'discriminator': 'state', > > 'data': { 'active': 'LUKSKeyslotActive' } } > > > > When we don't specify the slot#, then "new state active" selects an > > inactive slot (chosen by the system, and "new state inactive selects > > slots by secret (commonly just one slot). > > > > New state active: > > > > state new state action > > inactive active put secret, iter-time, mark active > > active active N/A (system choses inactive slot) > > > > New state inactive, for each slot holding the specified secret: > > > > state new state action > > inactive inactive N/A (inactive slot holds no secret) > > active inactive mark inactive (effectively deletes secret) > > > > As struct: > > > > { 'struct': 'LUKSKeyslotUpdate', > > 'data': { 'active': 'bool', # could do enum instead > > '*keyslot': 'int', > > '*secret': 'str', # present if @active is true > > '*iter-time': 'int' } } # absent if @active is false > > > > As union: > > > > { 'enum': 'LUKSKeyslotState', > > 'data': [ 'active', 'inactive' ] } > > { 'struct': 'LUKSKeyslotActive', > > 'data': { 'secret': 'str', > > '*iter-time': 'int } } > > { 'union': 'LUKSKeyslotAmend', > > 'base': { '*keyslot': 'int', > > 'state': 'LUKSKeyslotState' } > > 'discriminator': 'state', > > 'data': { 'active': 'LUKSKeyslotActive' } } > > > > Union looks more complicated because our union notation sucks[*]. I > > like it anyway, because you don't have to explain when which optional > > members aren't actually optional. > > > > Regardless of struct vs. union, this supports an active -> active > > transition only with an explicit keyslot. Feels fine to me. If we want > > to support it without keyslot as well, we need a way to specify both old > > and new secret. Do we? > > > > > > [*] I hope to fix that one day. It's not even hard.
Kevin Wolf <kwolf@redhat.com> writes: > Am 05.02.2020 um 09:24 hat Markus Armbruster geschrieben: >> Daniel, Kevin, any comments or objections to the QAPI schema design >> sketch developed below? >> >> For your convenience, here's the result again: >> >> { 'enum': 'LUKSKeyslotState', >> 'data': [ 'active', 'inactive' ] } >> { 'struct': 'LUKSKeyslotActive', >> 'data': { 'secret': 'str', >> '*iter-time': 'int } } >> { 'union': 'LUKSKeyslotAmend', >> 'base': { '*keyslot': 'int', >> 'state': 'LUKSKeyslotState' } >> 'discriminator': 'state', >> 'data': { 'active': 'LUKSKeyslotActive' } } > > I think one of the requirements was that you can specify the keyslot not > only by using its number, but also by specifying the old secret. Quoting myself: When we don't specify the slot#, then "new state active" selects an inactive slot (chosen by the system, and "new state inactive selects slots by secret (commonly just one slot). This takes care of selecting (active) slots by old secret with "new state inactive". I intentionally did not provide for selecting (active) slots by old secret with "new state active", because that's unsafe update in place. We want to update secrets, of course. But the safe way to do that is to put the new secret into a free slot, and if that succeeds, deactivate the old secret. If deactivation fails, you're left with both old and new secret, which beats being left with no secret when update in place fails. > Trivial > extension, you just get another optional field that can be specified > instead of 'keyslot'. > > Resulting commands: > > Adding a key: > qemu-img amend -o encrypt.keys.0.state=active,encrypt.keys.0.secret=sec0 test.qcow2 This activates an inactive slot chosen by the sysem. You can activate a specific keyslot N by throwing in encrypt.keys.0.keyslot=N. > Deleting a key: > qemu-img amend -o encrypt.keys.0.state=inactive,encrypt.keys.0.keyslot=2 test.qcow2 This deactivates keyslot#2. You can deactivate slots holding a specific secret S by replacing encrypt.keys.0.keyslot=2 by encrypt.keys.0.secret=S. > Previous version (if this series is applied unchanged): > > Adding a key: > qemu-img amend -o encrypt.keys.0.new-secret=sec0 test.qcow2 > > Deleting a key: > qemu-img amend -o encrypt.keys.0.new-secret=,encrypt.keys.0.keyslot=2 test.qcow2 > > Adding a key gets more complicated with your proposed interface because > state must be set explicitly now whereas before it was derived > automatically from the fact that if you give a key, only active makes > sense. The explicitness could be viewed as an improvement :) If you'd prefer implicit here: Max has patches for making union tags optional with a default. They'd let you default active to true. > Deleting stays more or less the same, you just change the state instead > of clearing the secret. Thanks for your input!
On Wed, Feb 05, 2020 at 10:30:11AM +0100, Kevin Wolf wrote: > Am 05.02.2020 um 09:24 hat Markus Armbruster geschrieben: > > Daniel, Kevin, any comments or objections to the QAPI schema design > > sketch developed below? > > > > For your convenience, here's the result again: > > > > { 'enum': 'LUKSKeyslotState', > > 'data': [ 'active', 'inactive' ] } > > { 'struct': 'LUKSKeyslotActive', > > 'data': { 'secret': 'str', > > '*iter-time': 'int } } > > { 'union': 'LUKSKeyslotAmend', > > 'base': { '*keyslot': 'int', > > 'state': 'LUKSKeyslotState' } > > 'discriminator': 'state', > > 'data': { 'active': 'LUKSKeyslotActive' } } We need 'secret' in the 'inactive' case too > > I think one of the requirements was that you can specify the keyslot not > only by using its number, but also by specifying the old secret. Trivial > extension, you just get another optional field that can be specified > instead of 'keyslot'. > > Resulting commands: > > Adding a key: > qemu-img amend -o encrypt.keys.0.state=active,encrypt.keys.0.secret=sec0 test.qcow2 > > Deleting a key: > qemu-img amend -o encrypt.keys.0.state=inactive,encrypt.keys.0.keyslot=2 test.qcow2 I think this is good as a design. Expanding the examples to cover all scenarios we've discussed - Activating a new keyslot, auto-picking slot qemu-img amend -o encrypt.keys.0.state=active,\ encrypt.keys.0.secret=sec0 \ test.qcow2 Must raise an error if no free slots - Activating a new keyslot, picking a specific slot qemu-img amend -o encrypt.keys.0.state=active,\ encrypt.keys.0.secret=sec0 \ encrypt.keys.0.keyslot=3 \ test.qcow2 Must raise an error if slot is already active - Deactivating a old keyslot, auto-picking slot(s) from existing password qemu-img amend -o encrypt.keys.0.state=inactive,\ encrypt.keys.0.secret=sec0 \ test.qcow2 Must raise an error if this would leave zero keyslots after processing. - Deactivating a old keyslot, picking a specific slot qemu-img amend -o encrypt.keys.0.state=inactive,\ encrypt.keys.0.keyslot=2 \ test.qcow2 Always succeeds even if zero keyslots left. Regards, Daniel
Am 05.02.2020 um 11:03 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 05.02.2020 um 09:24 hat Markus Armbruster geschrieben: > >> Daniel, Kevin, any comments or objections to the QAPI schema design > >> sketch developed below? > >> > >> For your convenience, here's the result again: > >> > >> { 'enum': 'LUKSKeyslotState', > >> 'data': [ 'active', 'inactive' ] } > >> { 'struct': 'LUKSKeyslotActive', > >> 'data': { 'secret': 'str', > >> '*iter-time': 'int } } > >> { 'union': 'LUKSKeyslotAmend', > >> 'base': { '*keyslot': 'int', > >> 'state': 'LUKSKeyslotState' } > >> 'discriminator': 'state', > >> 'data': { 'active': 'LUKSKeyslotActive' } } > > > > I think one of the requirements was that you can specify the keyslot not > > only by using its number, but also by specifying the old secret. > > Quoting myself: > > When we don't specify the slot#, then "new state active" selects an > inactive slot (chosen by the system, and "new state inactive selects > slots by secret (commonly just one slot). > > This takes care of selecting (active) slots by old secret with "new > state inactive". "new secret inactive" can't select a slot by secret because 'secret' doesn't even exist for inactive. > I intentionally did not provide for selecting (active) slots by old > secret with "new state active", because that's unsafe update in place. > > We want to update secrets, of course. But the safe way to do that is to > put the new secret into a free slot, and if that succeeds, deactivate > the old secret. If deactivation fails, you're left with both old and > new secret, which beats being left with no secret when update in place > fails. Right. I wonder if qemu-img wants support for that specifically (possibly with allowing to enter the key interactively) rather than requiring the user to call qemu-img amend twice. > > Trivial > > extension, you just get another optional field that can be specified > > instead of 'keyslot'. > > > > Resulting commands: > > > > Adding a key: > > qemu-img amend -o encrypt.keys.0.state=active,encrypt.keys.0.secret=sec0 test.qcow2 > > This activates an inactive slot chosen by the sysem. > > You can activate a specific keyslot N by throwing in > encrypt.keys.0.keyslot=N. Yes. The usual case is that you just want to add a new key somwhere. > > Deleting a key: > > qemu-img amend -o encrypt.keys.0.state=inactive,encrypt.keys.0.keyslot=2 test.qcow2 > > This deactivates keyslot#2. > > You can deactivate slots holding a specific secret S by replacing > encrypt.keys.0.keyslot=2 by encrypt.keys.0.secret=S. Not with your definition above, but with the appropriate changes, this makes sense. > > Previous version (if this series is applied unchanged): > > > > Adding a key: > > qemu-img amend -o encrypt.keys.0.new-secret=sec0 test.qcow2 > > > > Deleting a key: > > qemu-img amend -o encrypt.keys.0.new-secret=,encrypt.keys.0.keyslot=2 test.qcow2 > > > > Adding a key gets more complicated with your proposed interface because > > state must be set explicitly now whereas before it was derived > > automatically from the fact that if you give a key, only active makes > > sense. > > The explicitness could be viewed as an improvement :) Not really. I mean, I really know to appreciate the advantages of -blockdev where needed, but usually I don't want to type all that stuff for the most common tasks. qemu-img amend is similar. For deleting, I might actually agree that explicitness is an improvement, but for creating it's just unnecessary verbosity. > If you'd prefer implicit here: Max has patches for making union tags > optional with a default. They'd let you default active to true. I guess this would improve the usability in this case. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 05.02.2020 um 11:03 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 05.02.2020 um 09:24 hat Markus Armbruster geschrieben: >> >> Daniel, Kevin, any comments or objections to the QAPI schema design >> >> sketch developed below? >> >> >> >> For your convenience, here's the result again: >> >> >> >> { 'enum': 'LUKSKeyslotState', >> >> 'data': [ 'active', 'inactive' ] } >> >> { 'struct': 'LUKSKeyslotActive', >> >> 'data': { 'secret': 'str', >> >> '*iter-time': 'int } } >> >> { 'union': 'LUKSKeyslotAmend', >> >> 'base': { '*keyslot': 'int', >> >> 'state': 'LUKSKeyslotState' } >> >> 'discriminator': 'state', >> >> 'data': { 'active': 'LUKSKeyslotActive' } } >> > >> > I think one of the requirements was that you can specify the keyslot not >> > only by using its number, but also by specifying the old secret. >> >> Quoting myself: >> >> When we don't specify the slot#, then "new state active" selects an >> inactive slot (chosen by the system, and "new state inactive selects >> slots by secret (commonly just one slot). >> >> This takes care of selecting (active) slots by old secret with "new >> state inactive". > > "new secret inactive" can't select a slot by secret because 'secret' > doesn't even exist for inactive. My mistake. My text leading up to my schema has it, but the schema itself doesn't. Obvious fix: As struct: { 'struct': 'LUKSKeyslotUpdate', 'data': { 'active': 'bool', # could do enum instead '*keyslot': 'int', '*secret': 'str', # present if @active is true # or @keyslot is absent '*iter-time': 'int' } } # absent if @active is false As union: { 'enum': 'LUKSKeyslotState', 'data': [ 'active', 'inactive' ] } { 'struct': 'LUKSKeyslotActive', 'data': { 'secret': 'str', '*iter-time': 'int } } { 'struct': 'LUKSKeyslotInactive', 'data': { '*secret': 'str' } } # either @secret or @keyslot present # might want to name this @old-secret { 'union': 'LUKSKeyslotAmend', 'base': { '*keyslot': 'int', 'state': 'LUKSKeyslotState' } 'discriminator': 'state', 'data': { 'active': 'LUKSKeyslotActive', 'inactive': 'LUKSKeyslotInactive' } } The "deactivate secret" operation needs a bit of force to fit into the amend interface's "describe desired state" mold: the desired state is (state=inactive, secret=S). In other words, the inactive slot keeps its secret, you just can't use it for anything. Sadly, even with a union, we now have optional members that aren't really optional: "either @secret or @keyslot present". To avoid that, we'd have to come up with sane semantics for "neither" and "both". Let me try. The basic idea is to have @keyslot and @secret each select a set of slots, and take the intersection. If @keyslot is present: { @keyslot } absent: all slots If @secret is present: the set of slots holding @secret absent: all slots Neither present: select all slots. Both present: slot @keyslot if it holds @secret, else no slots The ability to specify @keyslot and @secret might actually be appreciated by some users. Belt *and* suspenders. Selecting no slots could be a no-op or an error. As a user, I don't care as long as I can tell what the command actually changed. Selecting all slots is an error because deactivating the last slot is. No different from selecting all slots with a particular secret when no active slots with different secrets exist. I'm not sure this is much of an improvement. >> I intentionally did not provide for selecting (active) slots by old >> secret with "new state active", because that's unsafe update in place. >> >> We want to update secrets, of course. But the safe way to do that is to >> put the new secret into a free slot, and if that succeeds, deactivate >> the old secret. If deactivation fails, you're left with both old and >> new secret, which beats being left with no secret when update in place >> fails. > > Right. I wonder if qemu-img wants support for that specifically > (possibly with allowing to enter the key interactively) rather than > requiring the user to call qemu-img amend twice. Human users may well appreciate such a "replace secret" operation. As so often with high-level operations, the difficulty is its failure modes: * Activation fails: no change (old secret still active) * Deactivate fails: both secrets are active Humans should be able to deal with both failure modes, provided the error reporting is sane. If I'd have to program a machine, however, I'd rather use the primitive operations, because each either succeeds completely or fails completely, which means I don't have to figure out *how* something failed. Note that such a high-level "replace secret" doesn't quite fit into the amend interface's "describe desired state" mold: the old secret is not part of the desired state. >> > Trivial >> > extension, you just get another optional field that can be specified >> > instead of 'keyslot'. >> > >> > Resulting commands: >> > >> > Adding a key: >> > qemu-img amend -o encrypt.keys.0.state=active,encrypt.keys.0.secret=sec0 test.qcow2 >> >> This activates an inactive slot chosen by the sysem. >> >> You can activate a specific keyslot N by throwing in >> encrypt.keys.0.keyslot=N. > > Yes. The usual case is that you just want to add a new key somwhere. Sure. >> > Deleting a key: >> > qemu-img amend -o encrypt.keys.0.state=inactive,encrypt.keys.0.keyslot=2 test.qcow2 >> >> This deactivates keyslot#2. >> >> You can deactivate slots holding a specific secret S by replacing >> encrypt.keys.0.keyslot=2 by encrypt.keys.0.secret=S. > > Not with your definition above, but with the appropriate changes, this > makes sense. Appropriately corrected, I hope. >> > Previous version (if this series is applied unchanged): >> > >> > Adding a key: >> > qemu-img amend -o encrypt.keys.0.new-secret=sec0 test.qcow2 >> > >> > Deleting a key: >> > qemu-img amend -o encrypt.keys.0.new-secret=,encrypt.keys.0.keyslot=2 test.qcow2 >> > >> > Adding a key gets more complicated with your proposed interface because >> > state must be set explicitly now whereas before it was derived >> > automatically from the fact that if you give a key, only active makes >> > sense. >> >> The explicitness could be viewed as an improvement :) > > Not really. I mean, I really know to appreciate the advantages of > -blockdev where needed, but usually I don't want to type all that stuff > for the most common tasks. qemu-img amend is similar. > > For deleting, I might actually agree that explicitness is an > improvement, but for creating it's just unnecessary verbosity. > >> If you'd prefer implicit here: Max has patches for making union tags >> optional with a default. They'd let you default active to true. > > I guess this would improve the usability in this case. > > Kevin
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Feb 05, 2020 at 10:30:11AM +0100, Kevin Wolf wrote: >> Am 05.02.2020 um 09:24 hat Markus Armbruster geschrieben: >> > Daniel, Kevin, any comments or objections to the QAPI schema design >> > sketch developed below? >> > >> > For your convenience, here's the result again: >> > >> > { 'enum': 'LUKSKeyslotState', >> > 'data': [ 'active', 'inactive' ] } >> > { 'struct': 'LUKSKeyslotActive', >> > 'data': { 'secret': 'str', >> > '*iter-time': 'int } } >> > { 'union': 'LUKSKeyslotAmend', >> > 'base': { '*keyslot': 'int', >> > 'state': 'LUKSKeyslotState' } >> > 'discriminator': 'state', >> > 'data': { 'active': 'LUKSKeyslotActive' } } > > We need 'secret' in the 'inactive' case too Yes, my mistake. >> I think one of the requirements was that you can specify the keyslot not >> only by using its number, but also by specifying the old secret. Trivial >> extension, you just get another optional field that can be specified >> instead of 'keyslot'. >> >> Resulting commands: >> >> Adding a key: >> qemu-img amend -o encrypt.keys.0.state=active,encrypt.keys.0.secret=sec0 test.qcow2 >> >> Deleting a key: >> qemu-img amend -o encrypt.keys.0.state=inactive,encrypt.keys.0.keyslot=2 test.qcow2 > > I think this is good as a design. > > Expanding the examples to cover all scenarios we've discussed > > > - Activating a new keyslot, auto-picking slot > > qemu-img amend -o encrypt.keys.0.state=active,\ > encrypt.keys.0.secret=sec0 \ > test.qcow2 > > Must raise an error if no free slots > > > - Activating a new keyslot, picking a specific slot > > qemu-img amend -o encrypt.keys.0.state=active,\ > encrypt.keys.0.secret=sec0 \ > encrypt.keys.0.keyslot=3 \ > test.qcow2 > > Must raise an error if slot is already active From the "describe desired state" point of view: * Always suceeds when slot is inactive * No-op when active and its secret is already the desired secret * Must raise "in place update refused" error otherwise > - Deactivating a old keyslot, auto-picking slot(s) from existing password > > qemu-img amend -o encrypt.keys.0.state=inactive,\ > encrypt.keys.0.secret=sec0 \ > test.qcow2 > > Must raise an error if this would leave zero keyslots > after processing. > > > - Deactivating a old keyslot, picking a specific slot > > qemu-img amend -o encrypt.keys.0.state=inactive,\ > encrypt.keys.0.keyslot=2 \ > test.qcow2 > > Always succeeds even if zero keyslots left. This one's dangerous. Here's a variation: permit operations that may or will lose data only with 'force': true. When @keyslot is absent, using force has no effect. When @keyslot is present, using force permits update in place and deactivating the last slot.
One more question regarding the array in
{ 'struct': 'QCryptoBlockAmendOptionsLUKS',
'data' : {
'keys': ['LUKSKeyslotUpdate'],
'*unlock-secret' : 'str' } }
Why an array? Do we really need multiple keyslot updates in one amend
operation?
On Thu, Feb 06, 2020 at 02:20:11PM +0100, Markus Armbruster wrote: > One more question regarding the array in > > { 'struct': 'QCryptoBlockAmendOptionsLUKS', > 'data' : { > 'keys': ['LUKSKeyslotUpdate'], > '*unlock-secret' : 'str' } } > > Why an array? Do we really need multiple keyslot updates in one amend > operation? I think it it is unlikely we'd use this in libvirt. In the case of wanting to *change* a key, it is safer to do a sequence of "add key" and then "remove key". If you combine them into the same operation, and you get an error back, it is hard to know /where/ it failed ? was the new key added or not ? Regards, Daniel
Markus Armbruster <armbru@redhat.com> writes: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 05.02.2020 um 11:03 hat Markus Armbruster geschrieben: >>> Kevin Wolf <kwolf@redhat.com> writes: [...] >>> > Adding a key gets more complicated with your proposed interface because >>> > state must be set explicitly now whereas before it was derived >>> > automatically from the fact that if you give a key, only active makes >>> > sense. >>> >>> The explicitness could be viewed as an improvement :) >> >> Not really. I mean, I really know to appreciate the advantages of >> -blockdev where needed, but usually I don't want to type all that stuff >> for the most common tasks. qemu-img amend is similar. >> >> For deleting, I might actually agree that explicitness is an >> improvement, but for creating it's just unnecessary verbosity. >> >>> If you'd prefer implicit here: Max has patches for making union tags >>> optional with a default. They'd let you default active to true. >> >> I guess this would improve the usability in this case. Thinking and writing in the "Making QEMU easier for management tools and applications" monster thread have made me realize we're mixing up two aspects that ought to be kept separate: machine-friendly QMP and human-friendly CLI. You argue that $ qemu-img amend -o encrypt.keys.0.new-secret=sec0 test.qcow2 is nicer than $ qemu-img amend -o encrypt.keys.0.state=active,encrypt.keys.0.secret=sec0 test.qcow2 and you do have a point: humans want their CLI terse. Redundancy is unwanted, except perhaps to protect users from dangerous accidents. In this example, state=active is redundant when a secret is given, because anything else would be an error. In QMP, however, we like things simple and explicit, and we eschew magic. This particular magic might just be simple enough to be acceptable in QMP. We'd "merely" have to support explicit defaults in the schema (a clear improvement if you ask me), and optional union tags (tolerable as long as the default comes from the schema, I guess). My point is: QAPI schema design *must* focus on QMP and nothing else. If we try to serve both QMP and human-friendly CLI, we'll likely botch both. I believe a truly human-friendly CLI requires more than just human-friendly concrete syntax for QMP. Same as HMP, really.
On Thu, Feb 06, 2020 at 02:44:45PM +0100, Markus Armbruster wrote: > Markus Armbruster <armbru@redhat.com> writes: > > > Kevin Wolf <kwolf@redhat.com> writes: > > > >> Am 05.02.2020 um 11:03 hat Markus Armbruster geschrieben: > >>> Kevin Wolf <kwolf@redhat.com> writes: > [...] > >>> > Adding a key gets more complicated with your proposed interface because > >>> > state must be set explicitly now whereas before it was derived > >>> > automatically from the fact that if you give a key, only active makes > >>> > sense. > >>> > >>> The explicitness could be viewed as an improvement :) > >> > >> Not really. I mean, I really know to appreciate the advantages of > >> -blockdev where needed, but usually I don't want to type all that stuff > >> for the most common tasks. qemu-img amend is similar. > >> > >> For deleting, I might actually agree that explicitness is an > >> improvement, but for creating it's just unnecessary verbosity. > >> > >>> If you'd prefer implicit here: Max has patches for making union tags > >>> optional with a default. They'd let you default active to true. > >> > >> I guess this would improve the usability in this case. > > Thinking and writing in the "Making QEMU easier for management tools and > applications" monster thread have made me realize we're mixing up two > aspects that ought to be kept separate: machine-friendly QMP and > human-friendly CLI. > > You argue that > > $ qemu-img amend -o encrypt.keys.0.new-secret=sec0 test.qcow2 > > is nicer than > > $ qemu-img amend -o encrypt.keys.0.state=active,encrypt.keys.0.secret=sec0 test.qcow2 > > and you do have a point: humans want their CLI terse. Redundancy is > unwanted, except perhaps to protect users from dangerous accidents. In > this example, state=active is redundant when a secret is given, because > anything else would be an error. > > In QMP, however, we like things simple and explicit, and we eschew > magic. > > This particular magic might just be simple enough to be acceptable in > QMP. We'd "merely" have to support explicit defaults in the schema (a > clear improvement if you ask me), and optional union tags (tolerable as > long as the default comes from the schema, I guess). > > My point is: QAPI schema design *must* focus on QMP and nothing else. > If we try to serve both QMP and human-friendly CLI, we'll likely botch > both. > > I believe a truly human-friendly CLI requires more than just > human-friendly concrete syntax for QMP. Same as HMP, really. A human-friendly approach to this problem would never even have the generic "amend" design IMHO. Friendly would be to have a CLI that is approx the same as "cryptsetup" provides eg $ qemu-img add-key /path/to/disk enter key>.. re-enter key>... or qemu-img add-key --keyfile /some/file.txt /path/to/disk Regards, Daniel
On 06.02.20 14:49, Daniel P. Berrangé wrote: > On Thu, Feb 06, 2020 at 02:44:45PM +0100, Markus Armbruster wrote: >> Markus Armbruster <armbru@redhat.com> writes: >> >>> Kevin Wolf <kwolf@redhat.com> writes: >>> >>>> Am 05.02.2020 um 11:03 hat Markus Armbruster geschrieben: >>>>> Kevin Wolf <kwolf@redhat.com> writes: >> [...] >>>>>> Adding a key gets more complicated with your proposed interface because >>>>>> state must be set explicitly now whereas before it was derived >>>>>> automatically from the fact that if you give a key, only active makes >>>>>> sense. >>>>> >>>>> The explicitness could be viewed as an improvement :) >>>> >>>> Not really. I mean, I really know to appreciate the advantages of >>>> -blockdev where needed, but usually I don't want to type all that stuff >>>> for the most common tasks. qemu-img amend is similar. >>>> >>>> For deleting, I might actually agree that explicitness is an >>>> improvement, but for creating it's just unnecessary verbosity. >>>> >>>>> If you'd prefer implicit here: Max has patches for making union tags >>>>> optional with a default. They'd let you default active to true. >>>> >>>> I guess this would improve the usability in this case. >> >> Thinking and writing in the "Making QEMU easier for management tools and >> applications" monster thread have made me realize we're mixing up two >> aspects that ought to be kept separate: machine-friendly QMP and >> human-friendly CLI. >> >> You argue that >> >> $ qemu-img amend -o encrypt.keys.0.new-secret=sec0 test.qcow2 >> >> is nicer than >> >> $ qemu-img amend -o encrypt.keys.0.state=active,encrypt.keys.0.secret=sec0 test.qcow2 >> >> and you do have a point: humans want their CLI terse. Redundancy is >> unwanted, except perhaps to protect users from dangerous accidents. In >> this example, state=active is redundant when a secret is given, because >> anything else would be an error. >> >> In QMP, however, we like things simple and explicit, and we eschew >> magic. >> >> This particular magic might just be simple enough to be acceptable in >> QMP. We'd "merely" have to support explicit defaults in the schema (a >> clear improvement if you ask me), and optional union tags (tolerable as >> long as the default comes from the schema, I guess). >> >> My point is: QAPI schema design *must* focus on QMP and nothing else. >> If we try to serve both QMP and human-friendly CLI, we'll likely botch >> both. >> >> I believe a truly human-friendly CLI requires more than just >> human-friendly concrete syntax for QMP. Same as HMP, really. > > A human-friendly approach to this problem would never even > have the generic "amend" design IMHO. Friendly would be to > have a CLI that is approx the same as "cryptsetup" provides > eg > > $ qemu-img add-key /path/to/disk > enter key>.. > re-enter key>... > > or > > qemu-img add-key --keyfile /some/file.txt /path/to/disk I have only scanned through the discussion up until this point, but I agree that amend doesn’t need to be human-friendly at all cost. If we really want a human-friendly keyslot modification interface, we can always add a specific qemu-img subcommand that provides high-level succinct operations based on a low-level and more verbose amend interface. (Or just a script that isn’t even built into qemu-img, because I suppose such a operation “translation” would be easier to implement in a scripting language. Maybe qemu-img could be extended to invoke external scripts for specific subcommands? But anyway, those would all be ideas for the future.) Max
Am 06.02.2020 um 14:36 hat Daniel P. Berrangé geschrieben: > On Thu, Feb 06, 2020 at 02:20:11PM +0100, Markus Armbruster wrote: > > One more question regarding the array in > > > > { 'struct': 'QCryptoBlockAmendOptionsLUKS', > > 'data' : { > > 'keys': ['LUKSKeyslotUpdate'], > > '*unlock-secret' : 'str' } } > > > > Why an array? Do we really need multiple keyslot updates in one amend > > operation? > > I think it it is unlikely we'd use this in libvirt. In the case of wanting > to *change* a key, it is safer to do a sequence of "add key" and then > "remove key". If you combine them into the same operation, and you get > an error back, it is hard to know /where/ it failed ? was the new key > added or not ? I think the array came in because of the "describe the new state" approach. The state has eight keyslots, so in order to fully describe the new state, you would have to be able to pass multiple slots at once. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 06.02.2020 um 14:36 hat Daniel P. Berrangé geschrieben: >> On Thu, Feb 06, 2020 at 02:20:11PM +0100, Markus Armbruster wrote: >> > One more question regarding the array in >> > >> > { 'struct': 'QCryptoBlockAmendOptionsLUKS', >> > 'data' : { >> > 'keys': ['LUKSKeyslotUpdate'], >> > '*unlock-secret' : 'str' } } >> > >> > Why an array? Do we really need multiple keyslot updates in one amend >> > operation? >> >> I think it it is unlikely we'd use this in libvirt. In the case of wanting >> to *change* a key, it is safer to do a sequence of "add key" and then >> "remove key". If you combine them into the same operation, and you get >> an error back, it is hard to know /where/ it failed ? was the new key >> added or not ? > > I think the array came in because of the "describe the new state" > approach. The state has eight keyslots, so in order to fully describe > the new state, you would have to be able to pass multiple slots at once. I see. Of course, it can also describe multiple new states for the same slot. Example: [{'state': 'active', 'keyslot': 0, 'secret': 'sec0'}, {'state': 'active', 'keyslot': 0, 'secret': 'sec1'}] where slot 0's old state is 'inactive'. Which one is the new state? If we execute the array elements one by one, this first makes slot 0 active with secret 'sec0', then tries to make it active with secret 'sec1', which fails. Simple enough, but it's not really "describe the new state", it's still "specify a series of state transitions". If we merge the array elements into a description of the new state of all eight slots, where a slot's description can be "same as old state", then this makes slot 0 active with either secret 'sec0' or 'sec1', depending on how we resolve the conflict. We could even make conflicts an error, and then this would fail without changing anything. What do we want? Is this worth the trouble?
On Thu, 2020-02-06 at 16:19 +0100, Markus Armbruster wrote: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 06.02.2020 um 14:36 hat Daniel P. Berrangé geschrieben: > > > On Thu, Feb 06, 2020 at 02:20:11PM +0100, Markus Armbruster wrote: > > > > One more question regarding the array in > > > > > > > > { 'struct': 'QCryptoBlockAmendOptionsLUKS', > > > > 'data' : { > > > > 'keys': ['LUKSKeyslotUpdate'], > > > > '*unlock-secret' : 'str' } } > > > > > > > > Why an array? Do we really need multiple keyslot updates in one amend > > > > operation? > > > > > > I think it it is unlikely we'd use this in libvirt. In the case of wanting > > > to *change* a key, it is safer to do a sequence of "add key" and then > > > "remove key". If you combine them into the same operation, and you get > > > an error back, it is hard to know /where/ it failed ? was the new key > > > added or not ? > > > > I think the array came in because of the "describe the new state" > > approach. The state has eight keyslots, so in order to fully describe > > the new state, you would have to be able to pass multiple slots at once. > > I see. > > Of course, it can also describe multiple new states for the same slot. > > Example: > > [{'state': 'active', 'keyslot': 0, 'secret': 'sec0'}, > {'state': 'active', 'keyslot': 0, 'secret': 'sec1'}] > > where slot 0's old state is 'inactive'. > > Which one is the new state? > > If we execute the array elements one by one, this first makes slot 0 > active with secret 'sec0', then tries to make it active with secret > 'sec1', which fails. Simple enough, but it's not really "describe the > new state", it's still "specify a series of state transitions". > > If we merge the array elements into a description of the new state of > all eight slots, where a slot's description can be "same as old state", > then this makes slot 0 active with either secret 'sec0' or 'sec1', > depending on how we resolve the conflict. We could even make conflicts > an error, and then this would fail without changing anything. > > What do we want? > > Is this worth the trouble? Yes, that is my thoughts on this as well. Best regards, Maxim Levitsky
Review of this patch led to a lengthy QAPI schema design discussion.
Let me try to condense it into a concrete proposal.
This is about the QAPI schema, and therefore about QMP. The
human-friendly interface is out of scope. Not because it's not
important (it clearly is!), only because we need to *focus* to have a
chance at success.
I'm going to include a few design options. I'll mark them "Option:".
The proposed "amend" interface takes a specification of desired state,
and figures out how to get from here to there by itself. LUKS keyslots
are one part of desired state.
We commonly have eight LUKS keyslots. Each keyslot is either active or
inactive. An active keyslot holds a secret.
Goal: a QAPI type for specifying desired state of LUKS keyslots.
Proposal:
{ 'enum': 'LUKSKeyslotState',
'data': [ 'active', 'inactive' ] }
{ 'struct': 'LUKSKeyslotActive',
'data': { 'secret': 'str',
'*iter-time': 'int } }
{ 'struct': 'LUKSKeyslotInactive',
'data': { '*old-secret': 'str' } }
{ 'union': 'LUKSKeyslotAmend',
'base': { '*keyslot': 'int',
'state': 'LUKSKeyslotState' }
'discriminator': 'state',
'data': { 'active': 'LUKSKeyslotActive',
'inactive': 'LUKSKeyslotInactive' } }
LUKSKeyslotAmend specifies desired state for a set of keyslots.
Four cases:
* @state is "active"
Desired state is active holding the secret given by @secret. Optional
@iter-time tweaks key stretching.
The keyslot is chosen either by the user or by the system, as follows:
- @keyslot absent
One inactive keyslot chosen by the system. If none exists, error.
- @keyslot present
The keyslot given by @keyslot.
If it's already active holding @secret, no-op. Rationale: the
current state is the desired state.
If it's already active holding another secret, error. Rationale:
update in place is unsafe.
Option: delete the "already active holding @secret" case. Feels
inelegant to me. Okay if it makes things substantially simpler.
* @state is "inactive"
Desired state is inactive.
Error if the current state has active keyslots, but the desired state
has none.
The user choses the keyslot by number and/or by the secret it holds,
as follows:
- @keyslot absent, @old-secret present
All active keyslots holding @old-secret. If none exists, error.
- @keyslot present, @old-secret absent
The keyslot given by @keyslot.
If it's already inactive, no-op. Rationale: the current state is
the desired state.
- both @keyslot and @old-secret present
The keyslot given by keyslot.
If it's inactive or holds a secret other than @old-secret, error.
Option: error regardless of @old-secret, if that makes things
simpler.
- neither @keyslot not @old-secret present
All keyslots. Note that this will error out due to "desired state
has no active keyslots" unless the current state has none, either.
Option: error out unconditionally.
Note that LUKSKeyslotAmend can specify only one desired state for
commonly just one keyslot. Rationale: this satisfies practical needs.
An array of LUKSKeyslotAmend could specify desired state for all
keyslots. However, multiple array elements could then apply to the same
slot. We'd have to specify how to resolve such conflicts, and we'd have
to code up conflict detection. Not worth it.
Examples:
* Add a secret to some free keyslot:
{ "state": "active", "secret": "CIA/GRU/MI6" }
* Deactivate all keyslots holding a secret:
{ "state": "inactive", "old-secret": "CIA/GRU/MI6" }
* Add a secret to a specific keyslot:
{ "state": "active", "secret": "CIA/GRU/MI6", "keyslot": 0 }
* Deactivate a specific keyslot:
{ "state": "inactive", "keyslot": 0 }
Possibly less dangerous:
{ "state": "inactive", "keyslot": 0, "old-secret": "CIA/GRU/MI6" }
Option: Make use of Max's patches to support optional union tag with
default value to let us default @state to "active". I doubt this makes
much of a difference in QMP. A human-friendly interface should probably
be higher level anyway (Daniel pointed to cryptsetup).
Option: LUKSKeyslotInactive member @old-secret could also be named
@secret. I don't care.
Option: delete @keyslot. It provides low-level slot access.
Complicates the interface. Fine if we need lov-level slot access. Do
we?
I apologize for the time it has taken me to write this.
Comments?
On Sat, 2020-02-15 at 15:51 +0100, Markus Armbruster wrote: > Review of this patch led to a lengthy QAPI schema design discussion. > Let me try to condense it into a concrete proposal. > > This is about the QAPI schema, and therefore about QMP. The > human-friendly interface is out of scope. Not because it's not > important (it clearly is!), only because we need to *focus* to have a > chance at success. 100% agree. > > I'm going to include a few design options. I'll mark them "Option:". > > The proposed "amend" interface takes a specification of desired state, > and figures out how to get from here to there by itself. LUKS keyslots > are one part of desired state. > > We commonly have eight LUKS keyslots. Each keyslot is either active or > inactive. An active keyslot holds a secret. > > Goal: a QAPI type for specifying desired state of LUKS keyslots. > > Proposal: > > { 'enum': 'LUKSKeyslotState', > 'data': [ 'active', 'inactive' ] } > > { 'struct': 'LUKSKeyslotActive', > 'data': { 'secret': 'str', > '*iter-time': 'int } } > > { 'struct': 'LUKSKeyslotInactive', > 'data': { '*old-secret': 'str' } } > > { 'union': 'LUKSKeyslotAmend', > 'base': { '*keyslot': 'int', > 'state': 'LUKSKeyslotState' } > 'discriminator': 'state', > 'data': { 'active': 'LUKSKeyslotActive', > 'inactive': 'LUKSKeyslotInactive' } } > > LUKSKeyslotAmend specifies desired state for a set of keyslots. > > Four cases: > > * @state is "active" > > Desired state is active holding the secret given by @secret. Optional > @iter-time tweaks key stretching. > > The keyslot is chosen either by the user or by the system, as follows: > > - @keyslot absent > > One inactive keyslot chosen by the system. If none exists, error. > > - @keyslot present > > The keyslot given by @keyslot. > > If it's already active holding @secret, no-op. Rationale: the > current state is the desired state. > > If it's already active holding another secret, error. Rationale: > update in place is unsafe. > > Option: delete the "already active holding @secret" case. Feels > inelegant to me. Okay if it makes things substantially simpler. I didn't really understand this, since in state=active we shouldn't delete anything. Looks OK otherwise. > > * @state is "inactive" > > Desired state is inactive. > > Error if the current state has active keyslots, but the desired state > has none. > > The user choses the keyslot by number and/or by the secret it holds, > as follows: > > - @keyslot absent, @old-secret present > > All active keyslots holding @old-secret. If none exists, error. > > - @keyslot present, @old-secret absent > > The keyslot given by @keyslot. > > If it's already inactive, no-op. Rationale: the current state is > the desired state. > > - both @keyslot and @old-secret present > > The keyslot given by keyslot. > > If it's inactive or holds a secret other than @old-secret, error. Yea, that would be very nice to have. > > Option: error regardless of @old-secret, if that makes things > simpler. > > - neither @keyslot not @old-secret present > > All keyslots. Note that this will error out due to "desired state > has no active keyslots" unless the current state has none, either. > > Option: error out unconditionally. Yep, that the best IMHO. > > Note that LUKSKeyslotAmend can specify only one desired state for > commonly just one keyslot. Rationale: this satisfies practical needs. > An array of LUKSKeyslotAmend could specify desired state for all > keyslots. However, multiple array elements could then apply to the same > slot. We'd have to specify how to resolve such conflicts, and we'd have > to code up conflict detection. Not worth it. 110% agree (that is not a typo :-) ) > > Examples: > > * Add a secret to some free keyslot: > > { "state": "active", "secret": "CIA/GRU/MI6" } > > * Deactivate all keyslots holding a secret: > > { "state": "inactive", "old-secret": "CIA/GRU/MI6" } > > * Add a secret to a specific keyslot: > > { "state": "active", "secret": "CIA/GRU/MI6", "keyslot": 0 } > > * Deactivate a specific keyslot: > > { "state": "inactive", "keyslot": 0 } > > Possibly less dangerous: > > { "state": "inactive", "keyslot": 0, "old-secret": "CIA/GRU/MI6" } > > Option: Make use of Max's patches to support optional union tag with > default value to let us default @state to "active". I doubt this makes > much of a difference in QMP. A human-friendly interface should probably > be higher level anyway (Daniel pointed to cryptsetup). Also agree. > > Option: LUKSKeyslotInactive member @old-secret could also be named > @secret. I don't care. I prefer old-secret. > > Option: delete @keyslot. It provides low-level slot access. > Complicates the interface. Fine if we need lov-level slot access. Do > we? I don't have strong opinion on that. I'll probably would like to keep this for tests/debugging/etc. > > I apologize for the time it has taken me to write this. Thank you very much for doing this. > > Comments? Looks good to me. Best regards, Maxim Levitsky
Maxim Levitsky <mlevitsk@redhat.com> writes: > On Sat, 2020-02-15 at 15:51 +0100, Markus Armbruster wrote: >> Review of this patch led to a lengthy QAPI schema design discussion. >> Let me try to condense it into a concrete proposal. >> >> This is about the QAPI schema, and therefore about QMP. The >> human-friendly interface is out of scope. Not because it's not >> important (it clearly is!), only because we need to *focus* to have a >> chance at success. > 100% agree. >> >> I'm going to include a few design options. I'll mark them "Option:". >> >> The proposed "amend" interface takes a specification of desired state, >> and figures out how to get from here to there by itself. LUKS keyslots >> are one part of desired state. >> >> We commonly have eight LUKS keyslots. Each keyslot is either active or >> inactive. An active keyslot holds a secret. >> >> Goal: a QAPI type for specifying desired state of LUKS keyslots. >> >> Proposal: >> >> { 'enum': 'LUKSKeyslotState', >> 'data': [ 'active', 'inactive' ] } >> >> { 'struct': 'LUKSKeyslotActive', >> 'data': { 'secret': 'str', >> '*iter-time': 'int } } >> >> { 'struct': 'LUKSKeyslotInactive', >> 'data': { '*old-secret': 'str' } } >> >> { 'union': 'LUKSKeyslotAmend', >> 'base': { '*keyslot': 'int', >> 'state': 'LUKSKeyslotState' } >> 'discriminator': 'state', >> 'data': { 'active': 'LUKSKeyslotActive', >> 'inactive': 'LUKSKeyslotInactive' } } >> >> LUKSKeyslotAmend specifies desired state for a set of keyslots. >> >> Four cases: >> >> * @state is "active" >> >> Desired state is active holding the secret given by @secret. Optional >> @iter-time tweaks key stretching. >> >> The keyslot is chosen either by the user or by the system, as follows: >> >> - @keyslot absent >> >> One inactive keyslot chosen by the system. If none exists, error. >> >> - @keyslot present >> >> The keyslot given by @keyslot. >> >> If it's already active holding @secret, no-op. Rationale: the >> current state is the desired state. >> >> If it's already active holding another secret, error. Rationale: >> update in place is unsafe. >> >> Option: delete the "already active holding @secret" case. Feels >> inelegant to me. Okay if it makes things substantially simpler. > I didn't really understand this, since in state=active we shouldn't > delete anything. Looks OK otherwise. Let me try to clarify. Option: make the "already active holding @secret" case an error like the "already active holding another secret" case. In longhand: - @keyslot present The keyslot given by @keyslot. If it's already active, error. It feels inelegant to me, because it deviates from "specify desired state" paradigm: the specified desired state is fine, the way to get there from current state is obvious (do nothing), yet it's still an error. >> * @state is "inactive" >> >> Desired state is inactive. >> >> Error if the current state has active keyslots, but the desired state >> has none. >> >> The user choses the keyslot by number and/or by the secret it holds, >> as follows: >> >> - @keyslot absent, @old-secret present >> >> All active keyslots holding @old-secret. If none exists, error. >> >> - @keyslot present, @old-secret absent >> >> The keyslot given by @keyslot. >> >> If it's already inactive, no-op. Rationale: the current state is >> the desired state. >> >> - both @keyslot and @old-secret present >> >> The keyslot given by keyslot. >> >> If it's inactive or holds a secret other than @old-secret, error. > Yea, that would be very nice to have. >> >> Option: error regardless of @old-secret, if that makes things >> simpler. >> >> - neither @keyslot not @old-secret present >> >> All keyslots. Note that this will error out due to "desired state >> has no active keyslots" unless the current state has none, either. >> >> Option: error out unconditionally. > Yep, that the best IMHO. It's a matter of taste. If we interpret "both absent" as "all keyslots", then all cases flow out of the following simple spec: 0. Start with the set of all keyslots 1. If @old-secret is present, interset it with the set of slots holding that secret. 2. If @keyslots is present, intersect it with the set of slots with that slot number. The order of steps 1 and 2 doesn't matter. To error out unconditionally, we have to make "both absent" a special case. A good way to resolve such matters of taste is to try writing doc comments for all proposals. If you find you hate one of them much less, you have a winner :) [...]
On Mon, 2020-02-17 at 07:45 +0100, Markus Armbruster wrote: > Maxim Levitsky <mlevitsk@redhat.com> writes: > > > On Sat, 2020-02-15 at 15:51 +0100, Markus Armbruster wrote: > > > Review of this patch led to a lengthy QAPI schema design discussion. > > > Let me try to condense it into a concrete proposal. > > > > > > This is about the QAPI schema, and therefore about QMP. The > > > human-friendly interface is out of scope. Not because it's not > > > important (it clearly is!), only because we need to *focus* to have a > > > chance at success. > > > > 100% agree. > > > > > > I'm going to include a few design options. I'll mark them "Option:". > > > > > > The proposed "amend" interface takes a specification of desired state, > > > and figures out how to get from here to there by itself. LUKS keyslots > > > are one part of desired state. > > > > > > We commonly have eight LUKS keyslots. Each keyslot is either active or > > > inactive. An active keyslot holds a secret. > > > > > > Goal: a QAPI type for specifying desired state of LUKS keyslots. > > > > > > Proposal: > > > > > > { 'enum': 'LUKSKeyslotState', > > > 'data': [ 'active', 'inactive' ] } > > > > > > { 'struct': 'LUKSKeyslotActive', > > > 'data': { 'secret': 'str', > > > '*iter-time': 'int } } > > > > > > { 'struct': 'LUKSKeyslotInactive', > > > 'data': { '*old-secret': 'str' } } > > > > > > { 'union': 'LUKSKeyslotAmend', > > > 'base': { '*keyslot': 'int', > > > 'state': 'LUKSKeyslotState' } > > > 'discriminator': 'state', > > > 'data': { 'active': 'LUKSKeyslotActive', > > > 'inactive': 'LUKSKeyslotInactive' } } > > > > > > LUKSKeyslotAmend specifies desired state for a set of keyslots. > > > > > > Four cases: > > > > > > * @state is "active" > > > > > > Desired state is active holding the secret given by @secret. Optional > > > @iter-time tweaks key stretching. > > > > > > The keyslot is chosen either by the user or by the system, as follows: > > > > > > - @keyslot absent > > > > > > One inactive keyslot chosen by the system. If none exists, error. > > > > > > - @keyslot present > > > > > > The keyslot given by @keyslot. > > > > > > If it's already active holding @secret, no-op. Rationale: the > > > current state is the desired state. > > > > > > If it's already active holding another secret, error. Rationale: > > > update in place is unsafe. > > > > > > Option: delete the "already active holding @secret" case. Feels > > > inelegant to me. Okay if it makes things substantially simpler. > > > > I didn't really understand this, since in state=active we shouldn't > > delete anything. Looks OK otherwise. > > Let me try to clarify. > > Option: make the "already active holding @secret" case an error like the > "already active holding another secret" case. In longhand: > > - @keyslot present > > The keyslot given by @keyslot. > > If it's already active, error. > > It feels inelegant to me, because it deviates from "specify desired > state" paradigm: the specified desired state is fine, the way to get > there from current state is obvious (do nothing), yet it's still an > error. Yep, although in theory we also specify that iteration count, which might not match (and it will never exactly match since it is benchmark based), thus if user specified it, we might err out, and otherwise indeed ignore this. This is of course very minor issue. > > > > * @state is "inactive" > > > > > > Desired state is inactive. > > > > > > Error if the current state has active keyslots, but the desired state > > > has none. > > > > > > The user choses the keyslot by number and/or by the secret it holds, > > > as follows: > > > > > > - @keyslot absent, @old-secret present > > > > > > All active keyslots holding @old-secret. If none exists, error. > > > > > > - @keyslot present, @old-secret absent > > > > > > The keyslot given by @keyslot. > > > > > > If it's already inactive, no-op. Rationale: the current state is > > > the desired state. > > > > > > - both @keyslot and @old-secret present > > > > > > The keyslot given by keyslot. > > > > > > If it's inactive or holds a secret other than @old-secret, error. > > > > Yea, that would be very nice to have. > > > > > > Option: error regardless of @old-secret, if that makes things > > > simpler. > > > > > > - neither @keyslot not @old-secret present > > > > > > All keyslots. Note that this will error out due to "desired state > > > has no active keyslots" unless the current state has none, either. > > > > > > Option: error out unconditionally. > > > > Yep, that the best IMHO. > > It's a matter of taste. > > If we interpret "both absent" as "all keyslots", then all cases flow out > of the following simple spec: > > 0. Start with the set of all keyslots > > 1. If @old-secret is present, interset it with the set of slots > holding that secret. > > 2. If @keyslots is present, intersect it with the set of slots with > that slot number. > > The order of steps 1 and 2 doesn't matter. > > To error out unconditionally, we have to make "both absent" a special > case. Yes but to be honest it is few lines of code, and gets you a better error message in the process. I don't have a strong opinion on this though. > > A good way to resolve such matters of taste is to try writing doc > comments for all proposals. If you find you hate one of them much less, > you have a winner :) I am already out of this game for some time, I myself won't argue over this interface (in addition to that that current interface is very good IMHO), and I am just waiting for others to accept it. Best regards and thanks for help, Maxim Levitsky > > [...]
Am 15.02.2020 um 15:51 hat Markus Armbruster geschrieben: > Review of this patch led to a lengthy QAPI schema design discussion. > Let me try to condense it into a concrete proposal. > > This is about the QAPI schema, and therefore about QMP. The > human-friendly interface is out of scope. Not because it's not > important (it clearly is!), only because we need to *focus* to have a > chance at success. > > I'm going to include a few design options. I'll mark them "Option:". > > The proposed "amend" interface takes a specification of desired state, > and figures out how to get from here to there by itself. LUKS keyslots > are one part of desired state. > > We commonly have eight LUKS keyslots. Each keyslot is either active or > inactive. An active keyslot holds a secret. > > Goal: a QAPI type for specifying desired state of LUKS keyslots. > > Proposal: > > { 'enum': 'LUKSKeyslotState', > 'data': [ 'active', 'inactive' ] } > > { 'struct': 'LUKSKeyslotActive', > 'data': { 'secret': 'str', > '*iter-time': 'int } } > > { 'struct': 'LUKSKeyslotInactive', > 'data': { '*old-secret': 'str' } } > > { 'union': 'LUKSKeyslotAmend', > 'base': { '*keyslot': 'int', > 'state': 'LUKSKeyslotState' } > 'discriminator': 'state', > 'data': { 'active': 'LUKSKeyslotActive', > 'inactive': 'LUKSKeyslotInactive' } } > > LUKSKeyslotAmend specifies desired state for a set of keyslots. Though not arbitrary sets of keyslots, it's only a single keyslot or multiple keyslots containing the same secret. Might be good enough in practice, though it means that you may have to issue multiple amend commands to get to the final state that you really want (even if doing everything at once would be safe). > Four cases: > > * @state is "active" > > Desired state is active holding the secret given by @secret. Optional > @iter-time tweaks key stretching. > > The keyslot is chosen either by the user or by the system, as follows: > > - @keyslot absent > > One inactive keyslot chosen by the system. If none exists, error. > > - @keyslot present > > The keyslot given by @keyslot. > > If it's already active holding @secret, no-op. Rationale: the > current state is the desired state. > > If it's already active holding another secret, error. Rationale: > update in place is unsafe. > > Option: delete the "already active holding @secret" case. Feels > inelegant to me. Okay if it makes things substantially simpler. > > * @state is "inactive" > > Desired state is inactive. > > Error if the current state has active keyslots, but the desired state > has none. > > The user choses the keyslot by number and/or by the secret it holds, > as follows: > > - @keyslot absent, @old-secret present > > All active keyslots holding @old-secret. If none exists, error. > > - @keyslot present, @old-secret absent > > The keyslot given by @keyslot. > > If it's already inactive, no-op. Rationale: the current state is > the desired state. > > - both @keyslot and @old-secret present > > The keyslot given by keyslot. > > If it's inactive or holds a secret other than @old-secret, error. > > Option: error regardless of @old-secret, if that makes things > simpler. > > - neither @keyslot not @old-secret present > > All keyslots. Note that this will error out due to "desired state > has no active keyslots" unless the current state has none, either. > > Option: error out unconditionally. > > Note that LUKSKeyslotAmend can specify only one desired state for > commonly just one keyslot. Rationale: this satisfies practical needs. > An array of LUKSKeyslotAmend could specify desired state for all > keyslots. However, multiple array elements could then apply to the same > slot. We'd have to specify how to resolve such conflicts, and we'd have > to code up conflict detection. Not worth it. > > Examples: > > * Add a secret to some free keyslot: > > { "state": "active", "secret": "CIA/GRU/MI6" } > > * Deactivate all keyslots holding a secret: > > { "state": "inactive", "old-secret": "CIA/GRU/MI6" } > > * Add a secret to a specific keyslot: > > { "state": "active", "secret": "CIA/GRU/MI6", "keyslot": 0 } > > * Deactivate a specific keyslot: > > { "state": "inactive", "keyslot": 0 } > > Possibly less dangerous: > > { "state": "inactive", "keyslot": 0, "old-secret": "CIA/GRU/MI6" } > > Option: Make use of Max's patches to support optional union tag with > default value to let us default @state to "active". I doubt this makes > much of a difference in QMP. A human-friendly interface should probably > be higher level anyway (Daniel pointed to cryptsetup). > > Option: LUKSKeyslotInactive member @old-secret could also be named > @secret. I don't care. > > Option: delete @keyslot. It provides low-level slot access. > Complicates the interface. Fine if we need lov-level slot access. Do > we? > > I apologize for the time it has taken me to write this. > > Comments? Works for me (without taking any of the options). The unclear part is what the human-friendly interface should look like and where it should live. I'm afraid doing only the QMP part and calling the feature completed like we do so often won't work in this case. Kevin
On Mon, 2020-02-17 at 11:37 +0100, Kevin Wolf wrote: > Am 15.02.2020 um 15:51 hat Markus Armbruster geschrieben: > > Review of this patch led to a lengthy QAPI schema design discussion. > > Let me try to condense it into a concrete proposal. > > > > This is about the QAPI schema, and therefore about QMP. The > > human-friendly interface is out of scope. Not because it's not > > important (it clearly is!), only because we need to *focus* to have a > > chance at success. > > > > I'm going to include a few design options. I'll mark them "Option:". > > > > The proposed "amend" interface takes a specification of desired state, > > and figures out how to get from here to there by itself. LUKS keyslots > > are one part of desired state. > > > > We commonly have eight LUKS keyslots. Each keyslot is either active or > > inactive. An active keyslot holds a secret. > > > > Goal: a QAPI type for specifying desired state of LUKS keyslots. > > > > Proposal: > > > > { 'enum': 'LUKSKeyslotState', > > 'data': [ 'active', 'inactive' ] } > > > > { 'struct': 'LUKSKeyslotActive', > > 'data': { 'secret': 'str', > > '*iter-time': 'int } } > > > > { 'struct': 'LUKSKeyslotInactive', > > 'data': { '*old-secret': 'str' } } > > > > { 'union': 'LUKSKeyslotAmend', > > 'base': { '*keyslot': 'int', > > 'state': 'LUKSKeyslotState' } > > 'discriminator': 'state', > > 'data': { 'active': 'LUKSKeyslotActive', > > 'inactive': 'LUKSKeyslotInactive' } } > > > > LUKSKeyslotAmend specifies desired state for a set of keyslots. > > Though not arbitrary sets of keyslots, it's only a single keyslot or > multiple keyslots containing the same secret. Might be good enough in > practice, though it means that you may have to issue multiple amend > commands to get to the final state that you really want (even if doing > everything at once would be safe). > > > Four cases: > > > > * @state is "active" > > > > Desired state is active holding the secret given by @secret. Optional > > @iter-time tweaks key stretching. > > > > The keyslot is chosen either by the user or by the system, as follows: > > > > - @keyslot absent > > > > One inactive keyslot chosen by the system. If none exists, error. > > > > - @keyslot present > > > > The keyslot given by @keyslot. > > > > If it's already active holding @secret, no-op. Rationale: the > > current state is the desired state. > > > > If it's already active holding another secret, error. Rationale: > > update in place is unsafe. > > > > Option: delete the "already active holding @secret" case. Feels > > inelegant to me. Okay if it makes things substantially simpler. > > > > * @state is "inactive" > > > > Desired state is inactive. > > > > Error if the current state has active keyslots, but the desired state > > has none. > > > > The user choses the keyslot by number and/or by the secret it holds, > > as follows: > > > > - @keyslot absent, @old-secret present > > > > All active keyslots holding @old-secret. If none exists, error. > > > > - @keyslot present, @old-secret absent > > > > The keyslot given by @keyslot. > > > > If it's already inactive, no-op. Rationale: the current state is > > the desired state. > > > > - both @keyslot and @old-secret present > > > > The keyslot given by keyslot. > > > > If it's inactive or holds a secret other than @old-secret, error. > > > > Option: error regardless of @old-secret, if that makes things > > simpler. > > > > - neither @keyslot not @old-secret present > > > > All keyslots. Note that this will error out due to "desired state > > has no active keyslots" unless the current state has none, either. > > > > Option: error out unconditionally. > > > > Note that LUKSKeyslotAmend can specify only one desired state for > > commonly just one keyslot. Rationale: this satisfies practical needs. > > An array of LUKSKeyslotAmend could specify desired state for all > > keyslots. However, multiple array elements could then apply to the same > > slot. We'd have to specify how to resolve such conflicts, and we'd have > > to code up conflict detection. Not worth it. > > > > Examples: > > > > * Add a secret to some free keyslot: > > > > { "state": "active", "secret": "CIA/GRU/MI6" } > > > > * Deactivate all keyslots holding a secret: > > > > { "state": "inactive", "old-secret": "CIA/GRU/MI6" } > > > > * Add a secret to a specific keyslot: > > > > { "state": "active", "secret": "CIA/GRU/MI6", "keyslot": 0 } > > > > * Deactivate a specific keyslot: > > > > { "state": "inactive", "keyslot": 0 } > > > > Possibly less dangerous: > > > > { "state": "inactive", "keyslot": 0, "old-secret": "CIA/GRU/MI6" } > > > > Option: Make use of Max's patches to support optional union tag with > > default value to let us default @state to "active". I doubt this makes > > much of a difference in QMP. A human-friendly interface should probably > > be higher level anyway (Daniel pointed to cryptsetup). > > > > Option: LUKSKeyslotInactive member @old-secret could also be named > > @secret. I don't care. > > > > Option: delete @keyslot. It provides low-level slot access. > > Complicates the interface. Fine if we need lov-level slot access. Do > > we? > > > > I apologize for the time it has taken me to write this. > > > > Comments? > > Works for me (without taking any of the options). > > The unclear part is what the human-friendly interface should look like > and where it should live. I'm afraid doing only the QMP part and calling > the feature completed like we do so often won't work in this case. IMHO, I think that the best way to create human friendly part is to implement luks specific commands for qemu-img and use interface very similar to what cryptsetup does. Best regards, Maxim Levitsky > > Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 15.02.2020 um 15:51 hat Markus Armbruster geschrieben: >> Review of this patch led to a lengthy QAPI schema design discussion. >> Let me try to condense it into a concrete proposal. >> >> This is about the QAPI schema, and therefore about QMP. The >> human-friendly interface is out of scope. Not because it's not >> important (it clearly is!), only because we need to *focus* to have a >> chance at success. >> >> I'm going to include a few design options. I'll mark them "Option:". >> >> The proposed "amend" interface takes a specification of desired state, >> and figures out how to get from here to there by itself. LUKS keyslots >> are one part of desired state. >> >> We commonly have eight LUKS keyslots. Each keyslot is either active or >> inactive. An active keyslot holds a secret. >> >> Goal: a QAPI type for specifying desired state of LUKS keyslots. >> >> Proposal: >> >> { 'enum': 'LUKSKeyslotState', >> 'data': [ 'active', 'inactive' ] } >> >> { 'struct': 'LUKSKeyslotActive', >> 'data': { 'secret': 'str', >> '*iter-time': 'int } } >> >> { 'struct': 'LUKSKeyslotInactive', >> 'data': { '*old-secret': 'str' } } >> >> { 'union': 'LUKSKeyslotAmend', >> 'base': { '*keyslot': 'int', >> 'state': 'LUKSKeyslotState' } >> 'discriminator': 'state', >> 'data': { 'active': 'LUKSKeyslotActive', >> 'inactive': 'LUKSKeyslotInactive' } } >> >> LUKSKeyslotAmend specifies desired state for a set of keyslots. > > Though not arbitrary sets of keyslots, it's only a single keyslot or > multiple keyslots containing the same secret. Might be good enough in > practice, though it means that you may have to issue multiple amend > commands to get to the final state that you really want (even if doing > everything at once would be safe). True. I traded expressiveness for simplicity. Here's the only practical case I can think of where the lack of expressiveness may hurt: replace secrets. With this interface, you need two operations: activate a free slot with the new secret, deactivate the slot(s) with the old secret. There is an intermediate state with both secrets active. A more expressive interface could let you do both in one step. Relevant only if the implementation actually provides atomicity. Can it? >> Four cases: >> >> * @state is "active" >> >> Desired state is active holding the secret given by @secret. Optional >> @iter-time tweaks key stretching. >> >> The keyslot is chosen either by the user or by the system, as follows: >> >> - @keyslot absent >> >> One inactive keyslot chosen by the system. If none exists, error. >> >> - @keyslot present >> >> The keyslot given by @keyslot. >> >> If it's already active holding @secret, no-op. Rationale: the >> current state is the desired state. >> >> If it's already active holding another secret, error. Rationale: >> update in place is unsafe. >> >> Option: delete the "already active holding @secret" case. Feels >> inelegant to me. Okay if it makes things substantially simpler. >> >> * @state is "inactive" >> >> Desired state is inactive. >> >> Error if the current state has active keyslots, but the desired state >> has none. >> >> The user choses the keyslot by number and/or by the secret it holds, >> as follows: >> >> - @keyslot absent, @old-secret present >> >> All active keyslots holding @old-secret. If none exists, error. >> >> - @keyslot present, @old-secret absent >> >> The keyslot given by @keyslot. >> >> If it's already inactive, no-op. Rationale: the current state is >> the desired state. >> >> - both @keyslot and @old-secret present >> >> The keyslot given by keyslot. >> >> If it's inactive or holds a secret other than @old-secret, error. >> >> Option: error regardless of @old-secret, if that makes things >> simpler. >> >> - neither @keyslot not @old-secret present >> >> All keyslots. Note that this will error out due to "desired state >> has no active keyslots" unless the current state has none, either. >> >> Option: error out unconditionally. >> >> Note that LUKSKeyslotAmend can specify only one desired state for >> commonly just one keyslot. Rationale: this satisfies practical needs. >> An array of LUKSKeyslotAmend could specify desired state for all >> keyslots. However, multiple array elements could then apply to the same >> slot. We'd have to specify how to resolve such conflicts, and we'd have >> to code up conflict detection. Not worth it. >> >> Examples: >> >> * Add a secret to some free keyslot: >> >> { "state": "active", "secret": "CIA/GRU/MI6" } >> >> * Deactivate all keyslots holding a secret: >> >> { "state": "inactive", "old-secret": "CIA/GRU/MI6" } >> >> * Add a secret to a specific keyslot: >> >> { "state": "active", "secret": "CIA/GRU/MI6", "keyslot": 0 } >> >> * Deactivate a specific keyslot: >> >> { "state": "inactive", "keyslot": 0 } >> >> Possibly less dangerous: >> >> { "state": "inactive", "keyslot": 0, "old-secret": "CIA/GRU/MI6" } >> >> Option: Make use of Max's patches to support optional union tag with >> default value to let us default @state to "active". I doubt this makes >> much of a difference in QMP. A human-friendly interface should probably >> be higher level anyway (Daniel pointed to cryptsetup). >> >> Option: LUKSKeyslotInactive member @old-secret could also be named >> @secret. I don't care. >> >> Option: delete @keyslot. It provides low-level slot access. >> Complicates the interface. Fine if we need lov-level slot access. Do >> we? >> >> I apologize for the time it has taken me to write this. >> >> Comments? > > Works for me (without taking any of the options). > > The unclear part is what the human-friendly interface should look like > and where it should live. I'm afraid doing only the QMP part and calling > the feature completed like we do so often won't work in this case. No argument. Perhaps Daniel can help with designing a human-friendly high-level interface.
On 2/17/20 6:28 AM, Markus Armbruster wrote: >>> Proposal: >>> >>> { 'enum': 'LUKSKeyslotState', >>> 'data': [ 'active', 'inactive' ] } >>> >>> { 'struct': 'LUKSKeyslotActive', >>> 'data': { 'secret': 'str', >>> '*iter-time': 'int } } >>> >>> { 'struct': 'LUKSKeyslotInactive', >>> 'data': { '*old-secret': 'str' } } >>> >>> { 'union': 'LUKSKeyslotAmend', >>> 'base': { '*keyslot': 'int', >>> 'state': 'LUKSKeyslotState' } >>> 'discriminator': 'state', >>> 'data': { 'active': 'LUKSKeyslotActive', >>> 'inactive': 'LUKSKeyslotInactive' } } >>> >>> LUKSKeyslotAmend specifies desired state for a set of keyslots. >> >> Though not arbitrary sets of keyslots, it's only a single keyslot or >> multiple keyslots containing the same secret. Might be good enough in >> practice, though it means that you may have to issue multiple amend >> commands to get to the final state that you really want (even if doing >> everything at once would be safe). > > True. I traded expressiveness for simplicity. > > Here's the only practical case I can think of where the lack of > expressiveness may hurt: replace secrets. > > With this interface, you need two operations: activate a free slot with > the new secret, deactivate the slot(s) with the old secret. There is an > intermediate state with both secrets active. > > A more expressive interface could let you do both in one step. Relevant > only if the implementation actually provides atomicity. Can it? Or put another way, can atomicity be added via 'transaction' later? If so, reusing one common interface to provide atomicity is nicer than making every interface reimplement atomicity on an ad hoc basis.
On Mon, Feb 17, 2020 at 01:28:51PM +0100, Markus Armbruster wrote: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 15.02.2020 um 15:51 hat Markus Armbruster geschrieben: > >> Review of this patch led to a lengthy QAPI schema design discussion. > >> Let me try to condense it into a concrete proposal. > >> > >> This is about the QAPI schema, and therefore about QMP. The > >> human-friendly interface is out of scope. Not because it's not > >> important (it clearly is!), only because we need to *focus* to have a > >> chance at success. > >> > >> I'm going to include a few design options. I'll mark them "Option:". > >> > >> The proposed "amend" interface takes a specification of desired state, > >> and figures out how to get from here to there by itself. LUKS keyslots > >> are one part of desired state. > >> > >> We commonly have eight LUKS keyslots. Each keyslot is either active or > >> inactive. An active keyslot holds a secret. > >> > >> Goal: a QAPI type for specifying desired state of LUKS keyslots. > >> > >> Proposal: > >> > >> { 'enum': 'LUKSKeyslotState', > >> 'data': [ 'active', 'inactive' ] } > >> > >> { 'struct': 'LUKSKeyslotActive', > >> 'data': { 'secret': 'str', > >> '*iter-time': 'int } } > >> > >> { 'struct': 'LUKSKeyslotInactive', > >> 'data': { '*old-secret': 'str' } } > >> > >> { 'union': 'LUKSKeyslotAmend', > >> 'base': { '*keyslot': 'int', > >> 'state': 'LUKSKeyslotState' } > >> 'discriminator': 'state', > >> 'data': { 'active': 'LUKSKeyslotActive', > >> 'inactive': 'LUKSKeyslotInactive' } } > >> > >> LUKSKeyslotAmend specifies desired state for a set of keyslots. > > > > Though not arbitrary sets of keyslots, it's only a single keyslot or > > multiple keyslots containing the same secret. Might be good enough in > > practice, though it means that you may have to issue multiple amend > > commands to get to the final state that you really want (even if doing > > everything at once would be safe). > > True. I traded expressiveness for simplicity. > > Here's the only practical case I can think of where the lack of > expressiveness may hurt: replace secrets. > > With this interface, you need two operations: activate a free slot with > the new secret, deactivate the slot(s) with the old secret. There is an > intermediate state with both secrets active. > > A more expressive interface could let you do both in one step. Relevant > only if the implementation actually provides atomicity. Can it? This restriction is already present in the the long standing cryptsetup command, so I don't think it is a big deal. Or to put it another way I don't see a compelling justification for why QEMU needs to be special and do it in op. Regards, Daniel
On Sat, Feb 15, 2020 at 03:51:46PM +0100, Markus Armbruster wrote: > Review of this patch led to a lengthy QAPI schema design discussion. > Let me try to condense it into a concrete proposal. > > This is about the QAPI schema, and therefore about QMP. The > human-friendly interface is out of scope. Not because it's not > important (it clearly is!), only because we need to *focus* to have a > chance at success. OK > I'm going to include a few design options. I'll mark them "Option:". > > The proposed "amend" interface takes a specification of desired state, > and figures out how to get from here to there by itself. LUKS keyslots > are one part of desired state. > > We commonly have eight LUKS keyslots. Each keyslot is either active or > inactive. An active keyslot holds a secret. > > Goal: a QAPI type for specifying desired state of LUKS keyslots. > > Proposal: > > { 'enum': 'LUKSKeyslotState', > 'data': [ 'active', 'inactive' ] } > > { 'struct': 'LUKSKeyslotActive', > 'data': { 'secret': 'str', > '*iter-time': 'int } } > > { 'struct': 'LUKSKeyslotInactive', > 'data': { '*old-secret': 'str' } } > > { 'union': 'LUKSKeyslotAmend', > 'base': { '*keyslot': 'int', > 'state': 'LUKSKeyslotState' } > 'discriminator': 'state', > 'data': { 'active': 'LUKSKeyslotActive', > 'inactive': 'LUKSKeyslotInactive' } } > > LUKSKeyslotAmend specifies desired state for a set of keyslots. > > Four cases: > > * @state is "active" > > Desired state is active holding the secret given by @secret. Optional > @iter-time tweaks key stretching. > > The keyslot is chosen either by the user or by the system, as follows: > > - @keyslot absent > > One inactive keyslot chosen by the system. If none exists, error. > > - @keyslot present > > The keyslot given by @keyslot. > > If it's already active holding @secret, no-op. Rationale: the > current state is the desired state. > > If it's already active holding another secret, error. Rationale: > update in place is unsafe. > > Option: delete the "already active holding @secret" case. Feels > inelegant to me. Okay if it makes things substantially simpler. > > * @state is "inactive" > > Desired state is inactive. > > Error if the current state has active keyslots, but the desired state > has none. > > The user choses the keyslot by number and/or by the secret it holds, > as follows: > > - @keyslot absent, @old-secret present > > All active keyslots holding @old-secret. If none exists, error. > > - @keyslot present, @old-secret absent > > The keyslot given by @keyslot. > > If it's already inactive, no-op. Rationale: the current state is > the desired state. > > - both @keyslot and @old-secret present > > The keyslot given by keyslot. > > If it's inactive or holds a secret other than @old-secret, error. > > Option: error regardless of @old-secret, if that makes things > simpler. > > - neither @keyslot not @old-secret present > > All keyslots. Note that this will error out due to "desired state > has no active keyslots" unless the current state has none, either. > > Option: error out unconditionally. > > Note that LUKSKeyslotAmend can specify only one desired state for > commonly just one keyslot. Rationale: this satisfies practical needs. > An array of LUKSKeyslotAmend could specify desired state for all > keyslots. However, multiple array elements could then apply to the same > slot. We'd have to specify how to resolve such conflicts, and we'd have > to code up conflict detection. Not worth it. > > Examples: > > * Add a secret to some free keyslot: > > { "state": "active", "secret": "CIA/GRU/MI6" } > > * Deactivate all keyslots holding a secret: > > { "state": "inactive", "old-secret": "CIA/GRU/MI6" } > > * Add a secret to a specific keyslot: > > { "state": "active", "secret": "CIA/GRU/MI6", "keyslot": 0 } > > * Deactivate a specific keyslot: > > { "state": "inactive", "keyslot": 0 } > > Possibly less dangerous: > > { "state": "inactive", "keyslot": 0, "old-secret": "CIA/GRU/MI6" } > > Option: Make use of Max's patches to support optional union tag with > default value to let us default @state to "active". I doubt this makes > much of a difference in QMP. A human-friendly interface should probably > be higher level anyway (Daniel pointed to cryptsetup). > > Option: LUKSKeyslotInactive member @old-secret could also be named > @secret. I don't care. > > Option: delete @keyslot. It provides low-level slot access. > Complicates the interface. Fine if we need lov-level slot access. Do > we? > > I apologize for the time it has taken me to write this. > > Comments? This is all fine with me. I have no strong opinion on the handful of options listed above, so fine with any choices out of them. Regards, Daniel
On Mon, Feb 17, 2020 at 01:07:23PM +0200, Maxim Levitsky wrote: > On Mon, 2020-02-17 at 11:37 +0100, Kevin Wolf wrote: > > Am 15.02.2020 um 15:51 hat Markus Armbruster geschrieben: > > > Review of this patch led to a lengthy QAPI schema design discussion. > > > Let me try to condense it into a concrete proposal. > > > > > > This is about the QAPI schema, and therefore about QMP. The > > > human-friendly interface is out of scope. Not because it's not > > > important (it clearly is!), only because we need to *focus* to have a > > > chance at success. > > > > > > I'm going to include a few design options. I'll mark them "Option:". > > > > > > The proposed "amend" interface takes a specification of desired state, > > > and figures out how to get from here to there by itself. LUKS keyslots > > > are one part of desired state. > > > > > > We commonly have eight LUKS keyslots. Each keyslot is either active or > > > inactive. An active keyslot holds a secret. > > > > > > Goal: a QAPI type for specifying desired state of LUKS keyslots. > > > > > > Proposal: > > > > > > { 'enum': 'LUKSKeyslotState', > > > 'data': [ 'active', 'inactive' ] } > > > > > > { 'struct': 'LUKSKeyslotActive', > > > 'data': { 'secret': 'str', > > > '*iter-time': 'int } } > > > > > > { 'struct': 'LUKSKeyslotInactive', > > > 'data': { '*old-secret': 'str' } } > > > > > > { 'union': 'LUKSKeyslotAmend', > > > 'base': { '*keyslot': 'int', > > > 'state': 'LUKSKeyslotState' } > > > 'discriminator': 'state', > > > 'data': { 'active': 'LUKSKeyslotActive', > > > 'inactive': 'LUKSKeyslotInactive' } } > > > > > > LUKSKeyslotAmend specifies desired state for a set of keyslots. > > > > Though not arbitrary sets of keyslots, it's only a single keyslot or > > multiple keyslots containing the same secret. Might be good enough in > > practice, though it means that you may have to issue multiple amend > > commands to get to the final state that you really want (even if doing > > everything at once would be safe). > > > > > Four cases: > > > > > > * @state is "active" > > > > > > Desired state is active holding the secret given by @secret. Optional > > > @iter-time tweaks key stretching. > > > > > > The keyslot is chosen either by the user or by the system, as follows: > > > > > > - @keyslot absent > > > > > > One inactive keyslot chosen by the system. If none exists, error. > > > > > > - @keyslot present > > > > > > The keyslot given by @keyslot. > > > > > > If it's already active holding @secret, no-op. Rationale: the > > > current state is the desired state. > > > > > > If it's already active holding another secret, error. Rationale: > > > update in place is unsafe. > > > > > > Option: delete the "already active holding @secret" case. Feels > > > inelegant to me. Okay if it makes things substantially simpler. > > > > > > * @state is "inactive" > > > > > > Desired state is inactive. > > > > > > Error if the current state has active keyslots, but the desired state > > > has none. > > > > > > The user choses the keyslot by number and/or by the secret it holds, > > > as follows: > > > > > > - @keyslot absent, @old-secret present > > > > > > All active keyslots holding @old-secret. If none exists, error. > > > > > > - @keyslot present, @old-secret absent > > > > > > The keyslot given by @keyslot. > > > > > > If it's already inactive, no-op. Rationale: the current state is > > > the desired state. > > > > > > - both @keyslot and @old-secret present > > > > > > The keyslot given by keyslot. > > > > > > If it's inactive or holds a secret other than @old-secret, error. > > > > > > Option: error regardless of @old-secret, if that makes things > > > simpler. > > > > > > - neither @keyslot not @old-secret present > > > > > > All keyslots. Note that this will error out due to "desired state > > > has no active keyslots" unless the current state has none, either. > > > > > > Option: error out unconditionally. > > > > > > Note that LUKSKeyslotAmend can specify only one desired state for > > > commonly just one keyslot. Rationale: this satisfies practical needs. > > > An array of LUKSKeyslotAmend could specify desired state for all > > > keyslots. However, multiple array elements could then apply to the same > > > slot. We'd have to specify how to resolve such conflicts, and we'd have > > > to code up conflict detection. Not worth it. > > > > > > Examples: > > > > > > * Add a secret to some free keyslot: > > > > > > { "state": "active", "secret": "CIA/GRU/MI6" } > > > > > > * Deactivate all keyslots holding a secret: > > > > > > { "state": "inactive", "old-secret": "CIA/GRU/MI6" } > > > > > > * Add a secret to a specific keyslot: > > > > > > { "state": "active", "secret": "CIA/GRU/MI6", "keyslot": 0 } > > > > > > * Deactivate a specific keyslot: > > > > > > { "state": "inactive", "keyslot": 0 } > > > > > > Possibly less dangerous: > > > > > > { "state": "inactive", "keyslot": 0, "old-secret": "CIA/GRU/MI6" } > > > > > > Option: Make use of Max's patches to support optional union tag with > > > default value to let us default @state to "active". I doubt this makes > > > much of a difference in QMP. A human-friendly interface should probably > > > be higher level anyway (Daniel pointed to cryptsetup). > > > > > > Option: LUKSKeyslotInactive member @old-secret could also be named > > > @secret. I don't care. > > > > > > Option: delete @keyslot. It provides low-level slot access. > > > Complicates the interface. Fine if we need lov-level slot access. Do > > > we? > > > > > > I apologize for the time it has taken me to write this. > > > > > > Comments? > > > > Works for me (without taking any of the options). > > > > The unclear part is what the human-friendly interface should look like > > and where it should live. I'm afraid doing only the QMP part and calling > > the feature completed like we do so often won't work in this case. > > IMHO, I think that the best way to create human friendly part is to implement > luks specific commands for qemu-img and use interface very similar > to what cryptsetup does. I think we can have a generic 'qemu-img amend' for machine type, with the complex dotted syntax. And then have two human friendly commands 'qemu-img crypt-add-key' and 'qemu-img crypt-del-key' similarish to cryptsetup. Regards, Daniel
On Mon, 2020-02-24 at 14:46 +0000, Daniel P. Berrangé wrote: > On Mon, Feb 17, 2020 at 01:07:23PM +0200, Maxim Levitsky wrote: > > On Mon, 2020-02-17 at 11:37 +0100, Kevin Wolf wrote: > > > Am 15.02.2020 um 15:51 hat Markus Armbruster geschrieben: > > > > Review of this patch led to a lengthy QAPI schema design discussion. > > > > Let me try to condense it into a concrete proposal. > > > > > > > > This is about the QAPI schema, and therefore about QMP. The > > > > human-friendly interface is out of scope. Not because it's not > > > > important (it clearly is!), only because we need to *focus* to have a > > > > chance at success. > > > > > > > > I'm going to include a few design options. I'll mark them "Option:". > > > > > > > > The proposed "amend" interface takes a specification of desired state, > > > > and figures out how to get from here to there by itself. LUKS keyslots > > > > are one part of desired state. > > > > > > > > We commonly have eight LUKS keyslots. Each keyslot is either active or > > > > inactive. An active keyslot holds a secret. > > > > > > > > Goal: a QAPI type for specifying desired state of LUKS keyslots. > > > > > > > > Proposal: > > > > > > > > { 'enum': 'LUKSKeyslotState', > > > > 'data': [ 'active', 'inactive' ] } > > > > > > > > { 'struct': 'LUKSKeyslotActive', > > > > 'data': { 'secret': 'str', > > > > '*iter-time': 'int } } > > > > > > > > { 'struct': 'LUKSKeyslotInactive', > > > > 'data': { '*old-secret': 'str' } } > > > > > > > > { 'union': 'LUKSKeyslotAmend', > > > > 'base': { '*keyslot': 'int', > > > > 'state': 'LUKSKeyslotState' } > > > > 'discriminator': 'state', > > > > 'data': { 'active': 'LUKSKeyslotActive', > > > > 'inactive': 'LUKSKeyslotInactive' } } > > > > > > > > LUKSKeyslotAmend specifies desired state for a set of keyslots. > > > > > > Though not arbitrary sets of keyslots, it's only a single keyslot or > > > multiple keyslots containing the same secret. Might be good enough in > > > practice, though it means that you may have to issue multiple amend > > > commands to get to the final state that you really want (even if doing > > > everything at once would be safe). > > > > > > > Four cases: > > > > > > > > * @state is "active" > > > > > > > > Desired state is active holding the secret given by @secret. Optional > > > > @iter-time tweaks key stretching. > > > > > > > > The keyslot is chosen either by the user or by the system, as follows: > > > > > > > > - @keyslot absent > > > > > > > > One inactive keyslot chosen by the system. If none exists, error. > > > > > > > > - @keyslot present > > > > > > > > The keyslot given by @keyslot. > > > > > > > > If it's already active holding @secret, no-op. Rationale: the > > > > current state is the desired state. > > > > > > > > If it's already active holding another secret, error. Rationale: > > > > update in place is unsafe. > > > > > > > > Option: delete the "already active holding @secret" case. Feels > > > > inelegant to me. Okay if it makes things substantially simpler. > > > > > > > > * @state is "inactive" > > > > > > > > Desired state is inactive. > > > > > > > > Error if the current state has active keyslots, but the desired state > > > > has none. > > > > > > > > The user choses the keyslot by number and/or by the secret it holds, > > > > as follows: > > > > > > > > - @keyslot absent, @old-secret present > > > > > > > > All active keyslots holding @old-secret. If none exists, error. > > > > > > > > - @keyslot present, @old-secret absent > > > > > > > > The keyslot given by @keyslot. > > > > > > > > If it's already inactive, no-op. Rationale: the current state is > > > > the desired state. > > > > > > > > - both @keyslot and @old-secret present > > > > > > > > The keyslot given by keyslot. > > > > > > > > If it's inactive or holds a secret other than @old-secret, error. > > > > > > > > Option: error regardless of @old-secret, if that makes things > > > > simpler. > > > > > > > > - neither @keyslot not @old-secret present > > > > > > > > All keyslots. Note that this will error out due to "desired state > > > > has no active keyslots" unless the current state has none, either. > > > > > > > > Option: error out unconditionally. > > > > > > > > Note that LUKSKeyslotAmend can specify only one desired state for > > > > commonly just one keyslot. Rationale: this satisfies practical needs. > > > > An array of LUKSKeyslotAmend could specify desired state for all > > > > keyslots. However, multiple array elements could then apply to the same > > > > slot. We'd have to specify how to resolve such conflicts, and we'd have > > > > to code up conflict detection. Not worth it. > > > > > > > > Examples: > > > > > > > > * Add a secret to some free keyslot: > > > > > > > > { "state": "active", "secret": "CIA/GRU/MI6" } > > > > > > > > * Deactivate all keyslots holding a secret: > > > > > > > > { "state": "inactive", "old-secret": "CIA/GRU/MI6" } > > > > > > > > * Add a secret to a specific keyslot: > > > > > > > > { "state": "active", "secret": "CIA/GRU/MI6", "keyslot": 0 } > > > > > > > > * Deactivate a specific keyslot: > > > > > > > > { "state": "inactive", "keyslot": 0 } > > > > > > > > Possibly less dangerous: > > > > > > > > { "state": "inactive", "keyslot": 0, "old-secret": "CIA/GRU/MI6" } > > > > > > > > Option: Make use of Max's patches to support optional union tag with > > > > default value to let us default @state to "active". I doubt this makes > > > > much of a difference in QMP. A human-friendly interface should probably > > > > be higher level anyway (Daniel pointed to cryptsetup). > > > > > > > > Option: LUKSKeyslotInactive member @old-secret could also be named > > > > @secret. I don't care. > > > > > > > > Option: delete @keyslot. It provides low-level slot access. > > > > Complicates the interface. Fine if we need lov-level slot access. Do > > > > we? > > > > > > > > I apologize for the time it has taken me to write this. > > > > > > > > Comments? > > > > > > Works for me (without taking any of the options). > > > > > > The unclear part is what the human-friendly interface should look like > > > and where it should live. I'm afraid doing only the QMP part and calling > > > the feature completed like we do so often won't work in this case. > > > > IMHO, I think that the best way to create human friendly part is to implement > > luks specific commands for qemu-img and use interface very similar > > to what cryptsetup does. > > I think we can have a generic 'qemu-img amend' for machine type, with the > complex dotted syntax. > > And then have two human friendly commands 'qemu-img crypt-add-key' and > 'qemu-img crypt-del-key' similarish to cryptsetup. Yep, this is exactly what I was thinking about this as well! Best regards, Maxim Levitsky > > Regards, > Daniel
On 15.02.20 15:51, Markus Armbruster wrote: > Review of this patch led to a lengthy QAPI schema design discussion. > Let me try to condense it into a concrete proposal. > > This is about the QAPI schema, and therefore about QMP. The > human-friendly interface is out of scope. Not because it's not > important (it clearly is!), only because we need to *focus* to have a > chance at success. > > I'm going to include a few design options. I'll mark them "Option:". > > The proposed "amend" interface takes a specification of desired state, > and figures out how to get from here to there by itself. LUKS keyslots > are one part of desired state. > > We commonly have eight LUKS keyslots. Each keyslot is either active or > inactive. An active keyslot holds a secret. > > Goal: a QAPI type for specifying desired state of LUKS keyslots. > > Proposal: > > { 'enum': 'LUKSKeyslotState', > 'data': [ 'active', 'inactive' ] } > > { 'struct': 'LUKSKeyslotActive', > 'data': { 'secret': 'str', > '*iter-time': 'int } } > > { 'struct': 'LUKSKeyslotInactive', > 'data': { '*old-secret': 'str' } } > > { 'union': 'LUKSKeyslotAmend', > 'base': { '*keyslot': 'int', > 'state': 'LUKSKeyslotState' } > 'discriminator': 'state', > 'data': { 'active': 'LUKSKeyslotActive', > 'inactive': 'LUKSKeyslotInactive' } } Looks OK to me. The only thing is that @old-secret kind of works as an address, just like @keyslot, so it might also make sense to me to put @keyslot/@old-secret into a union in the base structure. (One of the problems that come to mind with that approach is that not specifying either of @old-secret or @keyslot has different meanings for activating/inactivating a keyslot: When activating it, it means “The first unused one”; when deactivating it, it’s just an error because it doesn’t really mean anything.) *shrug* Max
Max Reitz <mreitz@redhat.com> writes: > On 15.02.20 15:51, Markus Armbruster wrote: >> Review of this patch led to a lengthy QAPI schema design discussion. >> Let me try to condense it into a concrete proposal. >> >> This is about the QAPI schema, and therefore about QMP. The >> human-friendly interface is out of scope. Not because it's not >> important (it clearly is!), only because we need to *focus* to have a >> chance at success. >> >> I'm going to include a few design options. I'll mark them "Option:". >> >> The proposed "amend" interface takes a specification of desired state, >> and figures out how to get from here to there by itself. LUKS keyslots >> are one part of desired state. >> >> We commonly have eight LUKS keyslots. Each keyslot is either active or >> inactive. An active keyslot holds a secret. >> >> Goal: a QAPI type for specifying desired state of LUKS keyslots. >> >> Proposal: >> >> { 'enum': 'LUKSKeyslotState', >> 'data': [ 'active', 'inactive' ] } >> >> { 'struct': 'LUKSKeyslotActive', >> 'data': { 'secret': 'str', >> '*iter-time': 'int } } >> >> { 'struct': 'LUKSKeyslotInactive', >> 'data': { '*old-secret': 'str' } } >> >> { 'union': 'LUKSKeyslotAmend', >> 'base': { '*keyslot': 'int', >> 'state': 'LUKSKeyslotState' } >> 'discriminator': 'state', >> 'data': { 'active': 'LUKSKeyslotActive', >> 'inactive': 'LUKSKeyslotInactive' } } > > Looks OK to me. The only thing is that @old-secret kind of works as an > address, just like @keyslot, It does. > so it might also make sense to me to put > @keyslot/@old-secret into a union in the base structure. I'm fine with state-specific extra adressing modes (I better be, I proposed them). I'd also be fine with a single state-independent addressing mode, as long as we can come up with sane semantics. Less flexible when adding states, but we almost certainly won't. Let's see how we could merge my two addressing modes into one. The two are * active keyslot old-secret slot(s) selected absent N/A one inactive slot if exist, else error present N/A the slot given by @keyslot * inactive keyslot old-secret slot(s) selected absent absent all keyslots present absent the slot given by @keyslot absent present all active slots holding @old-secret present present the slot given by @keyslot, error unless it's active holding @old-secret They conflict: > (One of the problems that come to mind with that approach is that not > specifying either of @old-secret or @keyslot has different meanings for > activating/inactivating a keyslot: When activating it, it means “The > first unused one”; when deactivating it, it’s just an error because it > doesn’t really mean anything.) > > *shrug* Note we we don't really care what "inactive, both absent" does. My proposed semantics are just the most regular I could find. We can therefore resolve the conflict by picking "active, both absent": keyslot old-secret slot(s) selected absent absent one inactive slot if exist, else error present absent the slot given by @keyslot absent present all active slots holding @old-secret present present the slot given by @keyslot, error unless it's active holding @old-secret Changes: * inactive, both absent: changed; we select "one inactive slot" instead of "all slots". "All slots" is a no-op when the current state has no active keyslots, else error. "One inactive slot" is a no-op when the current state has one, else error. Thus, we no-op rather than error in some states. * active, keyslot absent or present, old-secret present: new; selects active slot(s) holding @old-secret, no-op when old-secret == secret, else error (no in place update) Can do. It's differently irregular, and has a few more combinations that are basically useless, which I find unappealing. Matter of taste, I guess. Anyone got strong feelings here?
On 25.02.20 17:48, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > >> On 15.02.20 15:51, Markus Armbruster wrote: >>> Review of this patch led to a lengthy QAPI schema design discussion. >>> Let me try to condense it into a concrete proposal. >>> >>> This is about the QAPI schema, and therefore about QMP. The >>> human-friendly interface is out of scope. Not because it's not >>> important (it clearly is!), only because we need to *focus* to have a >>> chance at success. >>> >>> I'm going to include a few design options. I'll mark them "Option:". >>> >>> The proposed "amend" interface takes a specification of desired state, >>> and figures out how to get from here to there by itself. LUKS keyslots >>> are one part of desired state. >>> >>> We commonly have eight LUKS keyslots. Each keyslot is either active or >>> inactive. An active keyslot holds a secret. >>> >>> Goal: a QAPI type for specifying desired state of LUKS keyslots. >>> >>> Proposal: >>> >>> { 'enum': 'LUKSKeyslotState', >>> 'data': [ 'active', 'inactive' ] } >>> >>> { 'struct': 'LUKSKeyslotActive', >>> 'data': { 'secret': 'str', >>> '*iter-time': 'int } } >>> >>> { 'struct': 'LUKSKeyslotInactive', >>> 'data': { '*old-secret': 'str' } } >>> >>> { 'union': 'LUKSKeyslotAmend', >>> 'base': { '*keyslot': 'int', >>> 'state': 'LUKSKeyslotState' } >>> 'discriminator': 'state', >>> 'data': { 'active': 'LUKSKeyslotActive', >>> 'inactive': 'LUKSKeyslotInactive' } } >> >> Looks OK to me. The only thing is that @old-secret kind of works as an >> address, just like @keyslot, > > It does. > >> so it might also make sense to me to put >> @keyslot/@old-secret into a union in the base structure. > > I'm fine with state-specific extra adressing modes (I better be, I > proposed them). > > I'd also be fine with a single state-independent addressing mode, as > long as we can come up with sane semantics. Less flexible when adding > states, but we almost certainly won't. > > Let's see how we could merge my two addressing modes into one. > > The two are > > * active > > keyslot old-secret slot(s) selected > absent N/A one inactive slot if exist, else error > present N/A the slot given by @keyslot Oh, I thought that maybe we could use old-secret here, too, for modifying the iter-time. But if old-secret makes no sense for to-be-active slots, then there’s little point in putting old-secret in the base. (OTOH, specifying old-secret for to-be-active slots does have a sensible meaning; it’s just that we won’t support changing anything about already-active slots, except making them inactive. So that might be an argument for not making it a syntactic error, but just a semantic error.) [...] > Note we we don't really care what "inactive, both absent" does. My > proposed semantics are just the most regular I could find. We can > therefore resolve the conflict by picking "active, both absent": > > keyslot old-secret slot(s) selected > absent absent one inactive slot if exist, else error > present absent the slot given by @keyslot > absent present all active slots holding @old-secret > present present the slot given by @keyslot, error unless > it's active holding @old-secret > > Changes: > > * inactive, both absent: changed; we select "one inactive slot" instead of > "all slots". > > "All slots" is a no-op when the current state has no active keyslots, > else error. > > "One inactive slot" is a no-op when the current state has one, else > error. Thus, we no-op rather than error in some states. > > * active, keyslot absent or present, old-secret present: new; selects > active slot(s) holding @old-secret, no-op when old-secret == secret, > else error (no in place update) > > Can do. It's differently irregular, and has a few more combinations > that are basically useless, which I find unappealing. Matter of taste, > I guess. > > Anyone got strong feelings here? The only strong feeling I have is that I absolutely don’t have a strong feeling about this. :) As such, I think we should just treat my rambling as such and stick to your proposal, since we’ve already gathered support for it. Max
On Tue, Feb 25, 2020 at 05:48:02PM +0100, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > > > On 15.02.20 15:51, Markus Armbruster wrote: > >> Review of this patch led to a lengthy QAPI schema design discussion. > >> Let me try to condense it into a concrete proposal. > >> > >> This is about the QAPI schema, and therefore about QMP. The > >> human-friendly interface is out of scope. Not because it's not > >> important (it clearly is!), only because we need to *focus* to have a > >> chance at success. > >> > >> I'm going to include a few design options. I'll mark them "Option:". > >> > >> The proposed "amend" interface takes a specification of desired state, > >> and figures out how to get from here to there by itself. LUKS keyslots > >> are one part of desired state. > >> > >> We commonly have eight LUKS keyslots. Each keyslot is either active or > >> inactive. An active keyslot holds a secret. > >> > >> Goal: a QAPI type for specifying desired state of LUKS keyslots. > >> > >> Proposal: > >> > >> { 'enum': 'LUKSKeyslotState', > >> 'data': [ 'active', 'inactive' ] } > >> > >> { 'struct': 'LUKSKeyslotActive', > >> 'data': { 'secret': 'str', > >> '*iter-time': 'int } } > >> > >> { 'struct': 'LUKSKeyslotInactive', > >> 'data': { '*old-secret': 'str' } } > >> > >> { 'union': 'LUKSKeyslotAmend', > >> 'base': { '*keyslot': 'int', > >> 'state': 'LUKSKeyslotState' } > >> 'discriminator': 'state', > >> 'data': { 'active': 'LUKSKeyslotActive', > >> 'inactive': 'LUKSKeyslotInactive' } } > > > > Looks OK to me. The only thing is that @old-secret kind of works as an > > address, just like @keyslot, > > It does. > > > so it might also make sense to me to put > > @keyslot/@old-secret into a union in the base structure. > > I'm fine with state-specific extra adressing modes (I better be, I > proposed them). > > I'd also be fine with a single state-independent addressing mode, as > long as we can come up with sane semantics. Less flexible when adding > states, but we almost certainly won't. > > Let's see how we could merge my two addressing modes into one. > > The two are > > * active > > keyslot old-secret slot(s) selected > absent N/A one inactive slot if exist, else error > present N/A the slot given by @keyslot > > * inactive > > keyslot old-secret slot(s) selected > absent absent all keyslots > present absent the slot given by @keyslot > absent present all active slots holding @old-secret > present present the slot given by @keyslot, error unless > it's active holding @old-secret > > They conflict: > > > (One of the problems that come to mind with that approach is that not > > specifying either of @old-secret or @keyslot has different meanings for > > activating/inactivating a keyslot: When activating it, it means “The > > first unused one”; when deactivating it, it’s just an error because it > > doesn’t really mean anything.) > > > > *shrug* > > Note we we don't really care what "inactive, both absent" does. My > proposed semantics are just the most regular I could find. We can > therefore resolve the conflict by picking "active, both absent": > > keyslot old-secret slot(s) selected > absent absent one inactive slot if exist, else error > present absent the slot given by @keyslot > absent present all active slots holding @old-secret > present present the slot given by @keyslot, error unless > it's active holding @old-secret > > Changes: > > * inactive, both absent: changed; we select "one inactive slot" instead of > "all slots". > > "All slots" is a no-op when the current state has no active keyslots, > else error. > > "One inactive slot" is a no-op when the current state has one, else > error. Thus, we no-op rather than error in some states. > > * active, keyslot absent or present, old-secret present: new; selects > active slot(s) holding @old-secret, no-op when old-secret == secret, > else error (no in place update) > > Can do. It's differently irregular, and has a few more combinations > that are basically useless, which I find unappealing. Matter of taste, > I guess. > > Anyone got strong feelings here? I don't feel like the changes give us any real world benefit, and especially think deleting one arbitrary slot is just wierd. IMHO, inactive with both keyslot & old-secret missing should just be an error condition. Regards, Daniel
Max Reitz <mreitz@redhat.com> writes: > On 25.02.20 17:48, Markus Armbruster wrote: >> Max Reitz <mreitz@redhat.com> writes: >> >>> On 15.02.20 15:51, Markus Armbruster wrote: >>>> Review of this patch led to a lengthy QAPI schema design discussion. >>>> Let me try to condense it into a concrete proposal. >>>> >>>> This is about the QAPI schema, and therefore about QMP. The >>>> human-friendly interface is out of scope. Not because it's not >>>> important (it clearly is!), only because we need to *focus* to have a >>>> chance at success. >>>> >>>> I'm going to include a few design options. I'll mark them "Option:". >>>> >>>> The proposed "amend" interface takes a specification of desired state, >>>> and figures out how to get from here to there by itself. LUKS keyslots >>>> are one part of desired state. >>>> >>>> We commonly have eight LUKS keyslots. Each keyslot is either active or >>>> inactive. An active keyslot holds a secret. >>>> >>>> Goal: a QAPI type for specifying desired state of LUKS keyslots. >>>> >>>> Proposal: >>>> >>>> { 'enum': 'LUKSKeyslotState', >>>> 'data': [ 'active', 'inactive' ] } >>>> >>>> { 'struct': 'LUKSKeyslotActive', >>>> 'data': { 'secret': 'str', >>>> '*iter-time': 'int } } >>>> >>>> { 'struct': 'LUKSKeyslotInactive', >>>> 'data': { '*old-secret': 'str' } } >>>> >>>> { 'union': 'LUKSKeyslotAmend', >>>> 'base': { '*keyslot': 'int', >>>> 'state': 'LUKSKeyslotState' } >>>> 'discriminator': 'state', >>>> 'data': { 'active': 'LUKSKeyslotActive', >>>> 'inactive': 'LUKSKeyslotInactive' } } >>> >>> Looks OK to me. The only thing is that @old-secret kind of works as an >>> address, just like @keyslot, >> >> It does. >> >>> so it might also make sense to me to put >>> @keyslot/@old-secret into a union in the base structure. >> >> I'm fine with state-specific extra adressing modes (I better be, I >> proposed them). >> >> I'd also be fine with a single state-independent addressing mode, as >> long as we can come up with sane semantics. Less flexible when adding >> states, but we almost certainly won't. >> >> Let's see how we could merge my two addressing modes into one. >> >> The two are >> >> * active >> >> keyslot old-secret slot(s) selected >> absent N/A one inactive slot if exist, else error >> present N/A the slot given by @keyslot > > Oh, I thought that maybe we could use old-secret here, too, for > modifying the iter-time. Update in place is unsafe. > But if old-secret makes no sense for > to-be-active slots, then there’s little point in putting old-secret in > the base. > > (OTOH, specifying old-secret for to-be-active slots does have a sensible > meaning; it’s just that we won’t support changing anything about > already-active slots, except making them inactive. So that might be an > argument for not making it a syntactic error, but just a semantic error.) Matter of taste. I like to keep simple things syntactic, and thus visible in introspection. > [...] > >> Note we we don't really care what "inactive, both absent" does. My >> proposed semantics are just the most regular I could find. We can >> therefore resolve the conflict by picking "active, both absent": >> >> keyslot old-secret slot(s) selected >> absent absent one inactive slot if exist, else error >> present absent the slot given by @keyslot >> absent present all active slots holding @old-secret >> present present the slot given by @keyslot, error unless >> it's active holding @old-secret >> >> Changes: >> >> * inactive, both absent: changed; we select "one inactive slot" instead of >> "all slots". >> >> "All slots" is a no-op when the current state has no active keyslots, >> else error. >> >> "One inactive slot" is a no-op when the current state has one, else >> error. Thus, we no-op rather than error in some states. >> >> * active, keyslot absent or present, old-secret present: new; selects >> active slot(s) holding @old-secret, no-op when old-secret == secret, >> else error (no in place update) >> >> Can do. It's differently irregular, and has a few more combinations >> that are basically useless, which I find unappealing. Matter of taste, >> I guess. >> >> Anyone got strong feelings here? > > The only strong feeling I have is that I absolutely don’t have a strong > feeling about this. :) > > As such, I think we should just treat my rambling as such and stick to > your proposal, since we’ve already gathered support for it. Thanks!
On Wed, 2020-02-26 at 08:28 +0100, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > > > On 25.02.20 17:48, Markus Armbruster wrote: > > > Max Reitz <mreitz@redhat.com> writes: > > > > > > > On 15.02.20 15:51, Markus Armbruster wrote: > > > > > Review of this patch led to a lengthy QAPI schema design discussion. > > > > > Let me try to condense it into a concrete proposal. > > > > > > > > > > This is about the QAPI schema, and therefore about QMP. The > > > > > human-friendly interface is out of scope. Not because it's not > > > > > important (it clearly is!), only because we need to *focus* to have a > > > > > chance at success. > > > > > > > > > > I'm going to include a few design options. I'll mark them "Option:". > > > > > > > > > > The proposed "amend" interface takes a specification of desired state, > > > > > and figures out how to get from here to there by itself. LUKS keyslots > > > > > are one part of desired state. > > > > > > > > > > We commonly have eight LUKS keyslots. Each keyslot is either active or > > > > > inactive. An active keyslot holds a secret. > > > > > > > > > > Goal: a QAPI type for specifying desired state of LUKS keyslots. > > > > > > > > > > Proposal: > > > > > > > > > > { 'enum': 'LUKSKeyslotState', > > > > > 'data': [ 'active', 'inactive' ] } > > > > > > > > > > { 'struct': 'LUKSKeyslotActive', > > > > > 'data': { 'secret': 'str', > > > > > '*iter-time': 'int } } > > > > > > > > > > { 'struct': 'LUKSKeyslotInactive', > > > > > 'data': { '*old-secret': 'str' } } > > > > > > > > > > { 'union': 'LUKSKeyslotAmend', > > > > > 'base': { '*keyslot': 'int', > > > > > 'state': 'LUKSKeyslotState' } > > > > > 'discriminator': 'state', > > > > > 'data': { 'active': 'LUKSKeyslotActive', > > > > > 'inactive': 'LUKSKeyslotInactive' } } > > > > > > > > Looks OK to me. The only thing is that @old-secret kind of works as an > > > > address, just like @keyslot, > > > > > > It does. > > > > > > > so it might also make sense to me to put > > > > @keyslot/@old-secret into a union in the base structure. > > > > > > I'm fine with state-specific extra adressing modes (I better be, I > > > proposed them). > > > > > > I'd also be fine with a single state-independent addressing mode, as > > > long as we can come up with sane semantics. Less flexible when adding > > > states, but we almost certainly won't. > > > > > > Let's see how we could merge my two addressing modes into one. > > > > > > The two are > > > > > > * active > > > > > > keyslot old-secret slot(s) selected > > > absent N/A one inactive slot if exist, else error > > > present N/A the slot given by @keyslot > > > > Oh, I thought that maybe we could use old-secret here, too, for > > modifying the iter-time. > > Update in place is unsafe. > > > But if old-secret makes no sense for > > to-be-active slots, then there’s little point in putting old-secret in > > the base. > > > > (OTOH, specifying old-secret for to-be-active slots does have a sensible > > meaning; it’s just that we won’t support changing anything about > > already-active slots, except making them inactive. So that might be an > > argument for not making it a syntactic error, but just a semantic error.) > > Matter of taste. I like to keep simple things syntactic, and thus > visible in introspection. > > > [...] > > > > > Note we we don't really care what "inactive, both absent" does. My > > > proposed semantics are just the most regular I could find. We can > > > therefore resolve the conflict by picking "active, both absent": > > > > > > keyslot old-secret slot(s) selected > > > absent absent one inactive slot if exist, else error > > > present absent the slot given by @keyslot > > > absent present all active slots holding @old-secret > > > present present the slot given by @keyslot, error unless > > > it's active holding @old-secret > > > > > > Changes: > > > > > > * inactive, both absent: changed; we select "one inactive slot" instead of > > > "all slots". > > > > > > "All slots" is a no-op when the current state has no active keyslots, > > > else error. > > > > > > "One inactive slot" is a no-op when the current state has one, else > > > error. Thus, we no-op rather than error in some states. > > > > > > * active, keyslot absent or present, old-secret present: new; selects > > > active slot(s) holding @old-secret, no-op when old-secret == secret, > > > else error (no in place update) > > > > > > Can do. It's differently irregular, and has a few more combinations > > > that are basically useless, which I find unappealing. Matter of taste, > > > I guess. > > > > > > Anyone got strong feelings here? > > > > The only strong feeling I have is that I absolutely don’t have a strong > > feeling about this. :) > > > > As such, I think we should just treat my rambling as such and stick to > > your proposal, since we’ve already gathered support for it. > > Thanks! So in summary, do I have the green light to implement the Markus's proposal as is? Best regards, Maxim Levitsky
On Sat, 2020-02-15 at 15:51 +0100, Markus Armbruster wrote: > Review of this patch led to a lengthy QAPI schema design discussion. > Let me try to condense it into a concrete proposal. > > This is about the QAPI schema, and therefore about QMP. The > human-friendly interface is out of scope. Not because it's not > important (it clearly is!), only because we need to *focus* to have a > chance at success. > > I'm going to include a few design options. I'll mark them "Option:". > > The proposed "amend" interface takes a specification of desired state, > and figures out how to get from here to there by itself. LUKS keyslots > are one part of desired state. > > We commonly have eight LUKS keyslots. Each keyslot is either active or > inactive. An active keyslot holds a secret. > > Goal: a QAPI type for specifying desired state of LUKS keyslots. > > Proposal: > > { 'enum': 'LUKSKeyslotState', > 'data': [ 'active', 'inactive' ] } > > { 'struct': 'LUKSKeyslotActive', > 'data': { 'secret': 'str', > '*iter-time': 'int } } > > { 'struct': 'LUKSKeyslotInactive', > 'data': { '*old-secret': 'str' } } > > { 'union': 'LUKSKeyslotAmend', > 'base': { '*keyslot': 'int', > 'state': 'LUKSKeyslotState' } > 'discriminator': 'state', > 'data': { 'active': 'LUKSKeyslotActive', > 'inactive': 'LUKSKeyslotInactive' } } > > LUKSKeyslotAmend specifies desired state for a set of keyslots. > > Four cases: > > * @state is "active" > > Desired state is active holding the secret given by @secret. Optional > @iter-time tweaks key stretching. > > The keyslot is chosen either by the user or by the system, as follows: > > - @keyslot absent > > One inactive keyslot chosen by the system. If none exists, error. > > - @keyslot present > > The keyslot given by @keyslot. > > If it's already active holding @secret, no-op. Rationale: the > current state is the desired state. > > If it's already active holding another secret, error. Rationale: > update in place is unsafe. > > Option: delete the "already active holding @secret" case. Feels > inelegant to me. Okay if it makes things substantially simpler. > > * @state is "inactive" > > Desired state is inactive. > > Error if the current state has active keyslots, but the desired state > has none. > > The user choses the keyslot by number and/or by the secret it holds, > as follows: > > - @keyslot absent, @old-secret present > > All active keyslots holding @old-secret. If none exists, error. > > - @keyslot present, @old-secret absent > > The keyslot given by @keyslot. > > If it's already inactive, no-op. Rationale: the current state is > the desired state. > > - both @keyslot and @old-secret present > > The keyslot given by keyslot. > > If it's inactive or holds a secret other than @old-secret, error. > > Option: error regardless of @old-secret, if that makes things > simpler. > > - neither @keyslot not @old-secret present > > All keyslots. Note that this will error out due to "desired state > has no active keyslots" unless the current state has none, either. > > Option: error out unconditionally. > > Note that LUKSKeyslotAmend can specify only one desired state for > commonly just one keyslot. Rationale: this satisfies practical needs. > An array of LUKSKeyslotAmend could specify desired state for all > keyslots. However, multiple array elements could then apply to the same > slot. We'd have to specify how to resolve such conflicts, and we'd have > to code up conflict detection. Not worth it. > > Examples: > > * Add a secret to some free keyslot: > > { "state": "active", "secret": "CIA/GRU/MI6" } > > * Deactivate all keyslots holding a secret: > > { "state": "inactive", "old-secret": "CIA/GRU/MI6" } > > * Add a secret to a specific keyslot: > > { "state": "active", "secret": "CIA/GRU/MI6", "keyslot": 0 } > > * Deactivate a specific keyslot: > > { "state": "inactive", "keyslot": 0 } > > Possibly less dangerous: > > { "state": "inactive", "keyslot": 0, "old-secret": "CIA/GRU/MI6" } > > Option: Make use of Max's patches to support optional union tag with > default value to let us default @state to "active". I doubt this makes > much of a difference in QMP. A human-friendly interface should probably > be higher level anyway (Daniel pointed to cryptsetup). > > Option: LUKSKeyslotInactive member @old-secret could also be named > @secret. I don't care. > > Option: delete @keyslot. It provides low-level slot access. > Complicates the interface. Fine if we need lov-level slot access. Do > we? > > I apologize for the time it has taken me to write this. > > Comments? I tried today to implement this but I hit a very unpleasant roadblock: Since QCrypto is generic (even though it only implements currently luks for raw/qcow2 usage, and legacy qcow2 aes encryption), I still can't assume that this is always the case. Thus I implemented the Qcrypto amend API in this way: ## # @QCryptoBlockAmendOptions: # # The options that are available for all encryption formats # when amending encryption settings # # Since: 5.0 ## { 'union': 'QCryptoBlockAmendOptions', 'base': 'QCryptoBlockOptionsBase', 'discriminator': 'format', 'data': { 'luks': 'QCryptoBlockAmendOptionsLUKS' } } However the QCryptoBlockAmendOptionsLUKS is a union too to be in line with the API proposal, but that is not supported on QAPI level and after I and Markus talked about we are not sure that it is worth it to implement this support only for this case. So far I see the following solutions 1. Drop the QCryptoBlockAmendOptionsLUKS union for now. This will bring the schema pretty much to be the same as my original proposal, however the API will be the same thus once nested unions are implemented this union can always be introduced again. 2. Drop the QCryptoBlockAmendOptions union. Strictly speaking this union is not needed since it only has one member anyway, however this union is used both by qcow2 QAPI scheme, so that it doesn't hardcode an encryption format for amend just like it doesn't for creation, (this can be hardcoded for now as well for now as long as we don't have more amendable encryption formats). However I also use the QCryptoBlockAmendOptions in C code in QCrypto API thus it will be ugly to use the QCryptoBlockAmendOptionsLUKS instead. 3. Make QCryptoBlockAmendOptionsLUKS a struct and add to it a nested member with new union type (say QCryptoBlockAmendOptionsLUKS1) which will be exactly as QCryptoBlockAmendOptionsLUKS was. This IMHO is even uglier since it changes the API (which we can't later fix) and adds both a dummy struct field and a dummy struct name. I personally vote 1. Best regards, Maxim Levitsky
On Tue, 2020-03-03 at 11:18 +0200, Maxim Levitsky wrote: > On Sat, 2020-02-15 at 15:51 +0100, Markus Armbruster wrote: > > Review of this patch led to a lengthy QAPI schema design discussion. > > Let me try to condense it into a concrete proposal. > > > > This is about the QAPI schema, and therefore about QMP. The > > human-friendly interface is out of scope. Not because it's not > > important (it clearly is!), only because we need to *focus* to have a > > chance at success. > > > > I'm going to include a few design options. I'll mark them "Option:". > > > > The proposed "amend" interface takes a specification of desired state, > > and figures out how to get from here to there by itself. LUKS keyslots > > are one part of desired state. > > > > We commonly have eight LUKS keyslots. Each keyslot is either active or > > inactive. An active keyslot holds a secret. > > > > Goal: a QAPI type for specifying desired state of LUKS keyslots. > > > > Proposal: > > > > { 'enum': 'LUKSKeyslotState', > > 'data': [ 'active', 'inactive' ] } > > > > { 'struct': 'LUKSKeyslotActive', > > 'data': { 'secret': 'str', > > '*iter-time': 'int } } > > > > { 'struct': 'LUKSKeyslotInactive', > > 'data': { '*old-secret': 'str' } } > > > > { 'union': 'LUKSKeyslotAmend', > > 'base': { '*keyslot': 'int', > > 'state': 'LUKSKeyslotState' } > > 'discriminator': 'state', > > 'data': { 'active': 'LUKSKeyslotActive', > > 'inactive': 'LUKSKeyslotInactive' } } > > > > LUKSKeyslotAmend specifies desired state for a set of keyslots. > > > > Four cases: > > > > * @state is "active" > > > > Desired state is active holding the secret given by @secret. Optional > > @iter-time tweaks key stretching. > > > > The keyslot is chosen either by the user or by the system, as follows: > > > > - @keyslot absent > > > > One inactive keyslot chosen by the system. If none exists, error. > > > > - @keyslot present > > > > The keyslot given by @keyslot. > > > > If it's already active holding @secret, no-op. Rationale: the > > current state is the desired state. > > > > If it's already active holding another secret, error. Rationale: > > update in place is unsafe. > > > > Option: delete the "already active holding @secret" case. Feels > > inelegant to me. Okay if it makes things substantially simpler. > > > > * @state is "inactive" > > > > Desired state is inactive. > > > > Error if the current state has active keyslots, but the desired state > > has none. > > > > The user choses the keyslot by number and/or by the secret it holds, > > as follows: > > > > - @keyslot absent, @old-secret present > > > > All active keyslots holding @old-secret. If none exists, error. > > > > - @keyslot present, @old-secret absent > > > > The keyslot given by @keyslot. > > > > If it's already inactive, no-op. Rationale: the current state is > > the desired state. > > > > - both @keyslot and @old-secret present > > > > The keyslot given by keyslot. > > > > If it's inactive or holds a secret other than @old-secret, error. > > > > Option: error regardless of @old-secret, if that makes things > > simpler. > > > > - neither @keyslot not @old-secret present > > > > All keyslots. Note that this will error out due to "desired state > > has no active keyslots" unless the current state has none, either. > > > > Option: error out unconditionally. > > > > Note that LUKSKeyslotAmend can specify only one desired state for > > commonly just one keyslot. Rationale: this satisfies practical needs. > > An array of LUKSKeyslotAmend could specify desired state for all > > keyslots. However, multiple array elements could then apply to the same > > slot. We'd have to specify how to resolve such conflicts, and we'd have > > to code up conflict detection. Not worth it. > > > > Examples: > > > > * Add a secret to some free keyslot: > > > > { "state": "active", "secret": "CIA/GRU/MI6" } > > > > * Deactivate all keyslots holding a secret: > > > > { "state": "inactive", "old-secret": "CIA/GRU/MI6" } > > > > * Add a secret to a specific keyslot: > > > > { "state": "active", "secret": "CIA/GRU/MI6", "keyslot": 0 } > > > > * Deactivate a specific keyslot: > > > > { "state": "inactive", "keyslot": 0 } > > > > Possibly less dangerous: > > > > { "state": "inactive", "keyslot": 0, "old-secret": "CIA/GRU/MI6" } > > > > Option: Make use of Max's patches to support optional union tag with > > default value to let us default @state to "active". I doubt this makes > > much of a difference in QMP. A human-friendly interface should probably > > be higher level anyway (Daniel pointed to cryptsetup). > > > > Option: LUKSKeyslotInactive member @old-secret could also be named > > @secret. I don't care. > > > > Option: delete @keyslot. It provides low-level slot access. > > Complicates the interface. Fine if we need lov-level slot access. Do > > we? > > > > I apologize for the time it has taken me to write this. > > > > Comments? > > I tried today to implement this but I hit a very unpleasant roadblock: > > Since QCrypto is generic (even though it only implements currently luks for raw/qcow2 usage, > and legacy qcow2 aes encryption), I still can't assume that this is always the case. > Thus I implemented the Qcrypto amend API in this way: > > ## > # @QCryptoBlockAmendOptions: > # > # The options that are available for all encryption formats > # when amending encryption settings > # > # Since: 5.0 > ## > { 'union': 'QCryptoBlockAmendOptions', > 'base': 'QCryptoBlockOptionsBase', > 'discriminator': 'format', > 'data': { > 'luks': 'QCryptoBlockAmendOptionsLUKS' } } > > However the QCryptoBlockAmendOptionsLUKS is a union too to be in line with the API proposal, > but that is not supported on QAPI level and after I and Markus talked about we are not sure > that it is worth it to implement this support only for this case. > > So far I see the following solutions > > > 1. Drop the QCryptoBlockAmendOptionsLUKS union for now. > This will bring the schema pretty much to be the same as my original proposal, > however the API will be the same thus once nested unions are implemented this union > can always be introduced again. > > 2. Drop the QCryptoBlockAmendOptions union. Strictly speaking this union is not needed > since it only has one member anyway, however this union is used both by qcow2 QAPI scheme, > so that it doesn't hardcode an encryption format for amend just like it doesn't for creation, > (this can be hardcoded for now as well for now as long as we don't have more amendable encryption formats). > However I also use the QCryptoBlockAmendOptions in C code in QCrypto API thus it will be ugly to use the > QCryptoBlockAmendOptionsLUKS instead. > > > 3. Make QCryptoBlockAmendOptionsLUKS a struct and add to it a nested member with new union type > (say QCryptoBlockAmendOptionsLUKS1) which will be exactly as QCryptoBlockAmendOptionsLUKS was. > > This IMHO is even uglier since it changes the API (which we can't later fix) and adds both a dummy struct > field and a dummy struct name. > > I personally vote 1. Any update? > > Best regards, > Maxim Levitsky > > >
diff --git a/crypto/block-luks.c b/crypto/block-luks.c index 4861db810c..349e95fed3 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -32,6 +32,7 @@ #include "qemu/uuid.h" #include "qemu/coroutine.h" +#include "qemu/bitmap.h" /* * Reference for the LUKS format implemented here is @@ -70,6 +71,9 @@ typedef struct QCryptoBlockLUKSKeySlot QCryptoBlockLUKSKeySlot; #define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL +#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS 2000 +#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 +223,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; }; @@ -1069,6 +1076,112 @@ qcrypto_block_luks_find_key(QCryptoBlock *block, return -1; } +/* + * Returns true if a slot i is marked as active + * (contains encrypted copy of the master key) + */ +static bool +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; +} + +/* + * Returns the number of slots that are marked as active + * (slots that contain encrypted copy of the master key) + */ +static unsigned int +qcrypto_block_luks_count_active_slots(const QCryptoBlockLUKS *luks) +{ + size_t i = 0; + unsigned int ret = 0; + + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { + if (qcrypto_block_luks_slot_active(luks, i)) { + ret++; + } + } + return ret; +} + +/* + * Finds first key slot which is not active + * Returns the key slot index, or -1 if it doesn't exist + */ +static int +qcrypto_block_luks_find_free_keyslot(const QCryptoBlockLUKS *luks) +{ + size_t i; + + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { + if (!qcrypto_block_luks_slot_active(luks, i)) { + return i; + } + } + return -1; + +} + +/* + * 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; + + 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; + + qcrypto_block_luks_store_header(block, writefunc, opaque, errp); + + /* + * 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, errp) < 0) { + /* + * If we failed to get the random data, still write + * at least zeros to the key slot at least once + */ + if (i > 0) { + return -1; + } + } + + if (writefunc(block, + slot->key_offset_sector * QCRYPTO_BLOCK_LUKS_SECTOR_SIZE, + garbagesplitkey, + splitkeylen, + opaque, + errp) != splitkeylen) { + return -1; + } + } + return 0; +} static int qcrypto_block_luks_open(QCryptoBlock *block, @@ -1099,6 +1212,7 @@ qcrypto_block_luks_open(QCryptoBlock *block, luks = g_new0(QCryptoBlockLUKS, 1); block->opaque = luks; + luks->secret = g_strdup(options->u.luks.key_secret); if (qcrypto_block_luks_load_header(block, readfunc, opaque, errp) < 0) { goto fail; @@ -1164,6 +1278,7 @@ qcrypto_block_luks_open(QCryptoBlock *block, fail: qcrypto_block_free_cipher(block); qcrypto_ivgen_free(block->ivgen); + g_free(luks->secret); g_free(luks); return -1; } @@ -1204,7 +1319,7 @@ qcrypto_block_luks_create(QCryptoBlock *block, memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts)); if (!luks_opts.has_iter_time) { - luks_opts.iter_time = 2000; + luks_opts.iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS; } if (!luks_opts.has_cipher_alg) { luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256; @@ -1244,6 +1359,8 @@ qcrypto_block_luks_create(QCryptoBlock *block, optprefix ? optprefix : ""); goto error; } + luks->secret = g_strdup(options->u.luks.key_secret); + password = qcrypto_secret_lookup_as_utf8(luks_opts.key_secret, errp); if (!password) { goto error; @@ -1471,10 +1588,260 @@ qcrypto_block_luks_create(QCryptoBlock *block, qcrypto_block_free_cipher(block); qcrypto_ivgen_free(block->ivgen); + g_free(luks->secret); g_free(luks); return -1; } +/* + * Given LUKSKeyslotUpdate command, return @slots_bitmap with all slots + * that will be updated with new password (or erased) + * returns number of affected slots + */ +static int qcrypto_block_luks_get_slots_bitmap(QCryptoBlock *block, + QCryptoBlockReadFunc readfunc, + void *opaque, + const LUKSKeyslotUpdate *command, + unsigned long *slots_bitmap, + Error **errp) +{ + const QCryptoBlockLUKS *luks = block->opaque; + size_t i; + int ret = 0; + + if (command->has_keyslot) { + /* keyslot set, select only this keyslot */ + int keyslot = command->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); + goto error; + } + bitmap_set(slots_bitmap, keyslot, 1); + ret++; + + } else if (command->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); + ret++; + } + } + } 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"); + goto error; + } + bitmap_set(slots_bitmap, slot, 1); + ret++; + } + + if (command->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(command->old_secret, + errp); + if (!old_password) { + goto error; + } + + rv = qcrypto_block_luks_load_key(block, + i, + old_password, + tmpkey, + readfunc, + opaque, + errp); + if (rv == -1) + goto error; + else if (rv == 0) { + bitmap_clear(slots_bitmap, i, 1); + ret--; + } + } + } + return ret; +error: + return -1; +} + +/* + * Apply a single keyslot update command as described in @command + * Optionally use @unlock_secret to retrieve the master key + */ +static int +qcrypto_block_luks_apply_keyslot_update(QCryptoBlock *block, + QCryptoBlockReadFunc readfunc, + QCryptoBlockWriteFunc writefunc, + void *opaque, + LUKSKeyslotUpdate *command, + const char *unlock_secret, + uint8_t **master_key, + bool force, + Error **errp) +{ + QCryptoBlockLUKS *luks = block->opaque; + g_autofree unsigned long *slots_bitmap = NULL; + int64_t iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME_MS; + int slot_count; + size_t i; + char *new_password; + bool erasing; + + slots_bitmap = bitmap_new(QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS); + slot_count = qcrypto_block_luks_get_slots_bitmap(block, readfunc, opaque, + command, slots_bitmap, + errp); + if (slot_count == -1) { + goto error; + } + /* no matching slots, so nothing to do */ + if (slot_count == 0) { + error_setg(errp, "Requested operation didn't match any slots"); + goto error; + } + /* + * slot is erased when the password is set to null, or empty string + * (for compatibility with command line) + */ + erasing = command->new_secret->type == QTYPE_QNULL || + strlen(command->new_secret->u.s) == 0; + + /* safety checks */ + if (!force) { + if (erasing) { + if (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"); + goto error; + } + } else { + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { + if (!test_bit(i, slots_bitmap)) { + continue; + } + if (qcrypto_block_luks_slot_active(luks, i)) { + error_setg(errp, + "Refusing to overwrite active slot %zu - " + "please erase it first", i); + goto error; + } + } + } + } + + /* setup the data needed for storing the new keyslot */ + if (!erasing) { + /* Load the master key if it wasn't already loaded */ + if (!*master_key) { + g_autofree char *old_password; + old_password = qcrypto_secret_lookup_as_utf8(unlock_secret, errp); + if (!old_password) { + goto error; + } + *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"); + goto error; + } + } + new_password = qcrypto_secret_lookup_as_utf8(command->new_secret->u.s, + errp); + if (!new_password) { + goto error; + } + if (command->has_iter_time) { + iter_time = command->iter_time; + } + } + + /* new apply the update */ + for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { + if (!test_bit(i, slots_bitmap)) { + continue; + } + if (erasing) { + if (qcrypto_block_luks_erase_key(block, i, + writefunc, + opaque, + errp)) { + error_append_hint(errp, "Failed to erase keyslot %zu", i); + goto error; + } + } else { + if (qcrypto_block_luks_store_key(block, i, + new_password, + *master_key, + iter_time, + writefunc, + opaque, + errp)) { + error_append_hint(errp, "Failed to write to keyslot %zu", i); + goto error; + } + } + } + return 0; +error: + return -EINVAL; +} + +static int +qcrypto_block_luks_amend_options(QCryptoBlock *block, + QCryptoBlockReadFunc readfunc, + QCryptoBlockWriteFunc writefunc, + void *opaque, + QCryptoBlockAmendOptions *options, + bool force, + Error **errp) +{ + QCryptoBlockLUKS *luks = block->opaque; + QCryptoBlockAmendOptionsLUKS *options_luks = &options->u.luks; + LUKSKeyslotUpdateList *ptr; + g_autofree uint8_t *master_key = NULL; + int ret; + + char *unlock_secret = options_luks->has_unlock_secret ? + options_luks->unlock_secret : + luks->secret; + + for (ptr = options_luks->keys; ptr; ptr = ptr->next) { + ret = qcrypto_block_luks_apply_keyslot_update(block, readfunc, + writefunc, opaque, + ptr->value, + unlock_secret, + &master_key, + force, errp); + + if (ret != 0) { + goto error; + } + } + return 0; +error: + return -1; +} static int qcrypto_block_luks_get_info(QCryptoBlock *block, QCryptoBlockInfo *info, @@ -1523,7 +1890,9 @@ static int qcrypto_block_luks_get_info(QCryptoBlock *block, static void qcrypto_block_luks_cleanup(QCryptoBlock *block) { - g_free(block->opaque); + QCryptoBlockLUKS *luks = block->opaque; + g_free(luks->secret); + g_free(luks); } @@ -1560,6 +1929,7 @@ qcrypto_block_luks_encrypt(QCryptoBlock *block, const QCryptoBlockDriver qcrypto_block_driver_luks = { .open = qcrypto_block_luks_open, .create = qcrypto_block_luks_create, + .amend = qcrypto_block_luks_amend_options, .get_info = qcrypto_block_luks_get_info, .cleanup = qcrypto_block_luks_cleanup, .decrypt = qcrypto_block_luks_decrypt, diff --git a/qapi/crypto.json b/qapi/crypto.json index 9faebd03d4..e83847c71e 100644 --- a/qapi/crypto.json +++ b/qapi/crypto.json @@ -1,6 +1,8 @@ # -*- Mode: Python -*- # +{ 'include': 'common.json' } + ## # = Cryptography ## @@ -310,6 +312,52 @@ 'discriminator': 'format', 'data': { 'luks': 'QCryptoBlockInfoLUKS' } } +## +# @LUKSKeyslotUpdate: +# +# @keyslot: If specified, will update only keyslot with this index +# +# @old-secret: If specified, will only update keyslots that +# can be opened with password which is contained in +# QCryptoSecret with @old-secret ID +# +# If neither @keyslot nor @old-secret is specified, +# first empty keyslot is selected for the update +# +# @new-secret: The ID of a QCryptoSecret object providing a new decryption +# key to place in all matching keyslots. +# null/empty string erases all matching keyslots unless +# last valid keyslot is erased. +# +# @iter-time: number of milliseconds to spend in +# PBKDF passphrase processing +# Since: 5.0 +## +{ 'struct': 'LUKSKeyslotUpdate', + 'data': { + '*keyslot': 'int', + '*old-secret': 'str', + 'new-secret' : 'StrOrNull', + '*iter-time' : 'int' } } + + +## +# @QCryptoBlockAmendOptionsLUKS: +# +# The options that can be changed on existing luks encrypted device +# @keys: list of keyslot updates to perform +# (updates are performed in order) +# @unlock-secret: use this secret to retrieve the current master key +# if not given will use the same secret as one +# that was used to open the image +# +# Since: 5.0 +## +{ 'struct': 'QCryptoBlockAmendOptionsLUKS', + 'data' : { + 'keys': ['LUKSKeyslotUpdate'], + '*unlock-secret' : 'str' } } + ## @@ -324,4 +372,4 @@ 'base': 'QCryptoBlockOptionsBase', 'discriminator': 'format', 'data': { - } } + 'luks': 'QCryptoBlockAmendOptionsLUKS' } }
Next few patches will expose that functionality to the user. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- crypto/block-luks.c | 374 +++++++++++++++++++++++++++++++++++++++++++- qapi/crypto.json | 50 +++++- 2 files changed, 421 insertions(+), 3 deletions(-)