Re: [PATCH v2 02/11] qcrypto-luks: extend the create options for upcoming encryption key management
On Fri, 2019-11-08 at 11:48 +0100, Max Reitz wrote: > On 08.11.19 10:28, Maxim Levitsky wrote: > > On Fri, 2019-10-04 at 19:42 +0200, Max Reitz wrote: > > > On 13.09.19 00:30, Maxim Levitsky wrote: > > > > Now you can specify which slot to put the encryption key to > > > > Plus add 'active' option which will let user erase the key secret > > > > instead of adding it. > > > > Check that active=true it when creating. > > > > > > > > Signed-off-by: Maxim Levitsky > > > > --- > > > > block/crypto.c | 2 ++ > > > > block/crypto.h | 16 +++ > > > > block/qcow2.c | 2 ++ > > > > crypto/block-luks.c| 26 +++--- > > > > qapi/crypto.json | 19 ++ > > > > tests/qemu-iotests/082.out | 54 ++ > > > > 6 files changed, 115 insertions(+), 4 deletions(-) > > > > > > (Just doing a cursory RFC-style review) > > > > > > I think we also want to reject unlock-secret if it’s given for creation; > > > > Agree, I'll do this in the next version. > > > > > and I suppose it’d be more important to print which slots are OK than > > > the slot the user has given. (It isn’t like we shouldn’t print that > > > slot index, but it’s more likely the user knows that than what the > > > limits are. I think.) > > > > I don't really understand what you mean here :-( > > > > Since this is qmp interface, > > I can't really print anything from it, other that error messages. > > Exactly, I’m referring to the error message. Right now it’s: > > "Invalid slot %" PRId64 " is specified", luks_opts.slot > > I think it should be something like: > > "Invalid slot %" PRId64 " specified, must be between 0 and %u", > luks_opt.slot, QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS - 1 This is a very good idea! implemented now and will post in the next version. Best regards, Maxim Levitsky
Re: [PATCH v2 02/11] qcrypto-luks: extend the create options for upcoming encryption key management
On 08.11.19 10:28, Maxim Levitsky wrote: > On Fri, 2019-10-04 at 19:42 +0200, Max Reitz wrote: >> On 13.09.19 00:30, Maxim Levitsky wrote: >>> Now you can specify which slot to put the encryption key to >>> Plus add 'active' option which will let user erase the key secret >>> instead of adding it. >>> Check that active=true it when creating. >>> >>> Signed-off-by: Maxim Levitsky >>> --- >>> block/crypto.c | 2 ++ >>> block/crypto.h | 16 +++ >>> block/qcow2.c | 2 ++ >>> crypto/block-luks.c| 26 +++--- >>> qapi/crypto.json | 19 ++ >>> tests/qemu-iotests/082.out | 54 ++ >>> 6 files changed, 115 insertions(+), 4 deletions(-) >> >> (Just doing a cursory RFC-style review) >> >> I think we also want to reject unlock-secret if it’s given for creation; > Agree, I'll do this in the next version. > >> and I suppose it’d be more important to print which slots are OK than >> the slot the user has given. (It isn’t like we shouldn’t print that >> slot index, but it’s more likely the user knows that than what the >> limits are. I think.) > I don't really understand what you mean here :-( > > Since this is qmp interface, > I can't really print anything from it, other that error messages. Exactly, I’m referring to the error message. Right now it’s: "Invalid slot %" PRId64 " is specified", luks_opts.slot I think it should be something like: "Invalid slot %" PRId64 " specified, must be between 0 and %u", luks_opt.slot, QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS - 1 Max signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 02/11] qcrypto-luks: extend the create options for upcoming encryption key management
On Thu, 2019-10-10 at 15:44 +0200, Kevin Wolf wrote: > Am 13.09.2019 um 00:30 hat Maxim Levitsky geschrieben: > > Now you can specify which slot to put the encryption key to > > Plus add 'active' option which will let user erase the key secret > > instead of adding it. > > Check that active=true it when creating. > > > > Signed-off-by: Maxim Levitsky > > diff --git a/qapi/crypto.json b/qapi/crypto.json > > index b2a4cff683..9b83a70634 100644 > > --- a/qapi/crypto.json > > +++ b/qapi/crypto.json > > @@ -190,6 +190,20 @@ > > # Currently defaults to 'sha256' > > # @hash-alg: the master key hash algorithm > > #Currently defaults to 'sha256' > > +# > > +# @active: Should the new secret be added (true) or erased (false) > > +# (amend only, since 4.2) > > +# > > +# @slot: The slot in which to put/erase the secret > > +#if not given, will select first free slot for secret addtion > > +#and erase all matching keyslots for erase. except last one > > +#(optional, since 4.2) > > +# > > +# @unlock-secret: The secret to use to unlock the image > > +#If not given, will use the secret that was used > > +#when opening the image. > > +#(optional, for amend only, since 4.2) > > +# > > # @iter-time: number of milliseconds to spend in > > # PBKDF passphrase processing. Currently defaults > > # to 2000. (since 2.8) > > This approach doesn't look right to me. BlockdevCreateOptions should > describe the state of the image after the operation. You're describing > an update instead (and in a way that doesn't allow you to change > everything that you may want to change, so that you need to call the > operation multiple times). > > I imagined the syntax of a blockdev-amend QMP command similar to > x-blockdev-reopen: Describe the full set of options that you want to > have in effect after the operation; if you don't want to change some > option, you just specify it again with its old value. This approach is a compromise trying to create more or less usable interface. In particular we (I and Daniel) wanted the following to work: 1. ability to add a new password to an empty keyslot and then remove the old password. This is probably the most common operation and it won't require the caller to know anything about the keyslots. 2. Allow the user to not know the passwords of some keyslots. For example if I want to add a new keyslot, I might not know some of the other keyslots. Specifying all the active keyslots, on each amend would force the user to know all the passwords (you can't 'extract' a password by reading a keyslot, since only hash of it is stored there for the security reasons) Thus the amend interface either allows you to add a keyslot (either a specific one, or first free one), and to remove a keyslot (again, either a specific one, or one that matches given password). > > Specifically for luks, this probably means that you have a @slots, which > is a list that contains at least the secret for each slot, or JSON null > for a slot that should be left empty. > > With the same approach, you don't have to make 'size' optional in later > patches, you can just require that the current size is re-specified. And > later, blockdev-amend could actually allow changing the size of images > if you provide a different value. This can be done IMHO. > > Kevin Best regards, Maxim Levitsky
Re: [PATCH v2 02/11] qcrypto-luks: extend the create options for upcoming encryption key management
On Fri, 2019-10-04 at 19:42 +0200, Max Reitz wrote: > On 13.09.19 00:30, Maxim Levitsky wrote: > > Now you can specify which slot to put the encryption key to > > Plus add 'active' option which will let user erase the key secret > > instead of adding it. > > Check that active=true it when creating. > > > > Signed-off-by: Maxim Levitsky > > --- > > block/crypto.c | 2 ++ > > block/crypto.h | 16 +++ > > block/qcow2.c | 2 ++ > > crypto/block-luks.c| 26 +++--- > > qapi/crypto.json | 19 ++ > > tests/qemu-iotests/082.out | 54 ++ > > 6 files changed, 115 insertions(+), 4 deletions(-) > > (Just doing a cursory RFC-style review) > > I think we also want to reject unlock-secret if it’s given for creation; Agree, I'll do this in the next version. > and I suppose it’d be more important to print which slots are OK than > the slot the user has given. (It isn’t like we shouldn’t print that > slot index, but it’s more likely the user knows that than what the > limits are. I think.) I don't really understand what you mean here :-( Since this is qmp interface, I can't really print anything from it, other that error messages. > > Max > Best regards, Maxim Levitsky
Re: [PATCH v2 02/11] qcrypto-luks: extend the create options for upcoming encryption key management
Am 13.09.2019 um 00:30 hat Maxim Levitsky geschrieben: > Now you can specify which slot to put the encryption key to > Plus add 'active' option which will let user erase the key secret > instead of adding it. > Check that active=true it when creating. > > Signed-off-by: Maxim Levitsky > diff --git a/qapi/crypto.json b/qapi/crypto.json > index b2a4cff683..9b83a70634 100644 > --- a/qapi/crypto.json > +++ b/qapi/crypto.json > @@ -190,6 +190,20 @@ > # Currently defaults to 'sha256' > # @hash-alg: the master key hash algorithm > #Currently defaults to 'sha256' > +# > +# @active: Should the new secret be added (true) or erased (false) > +# (amend only, since 4.2) > +# > +# @slot: The slot in which to put/erase the secret > +#if not given, will select first free slot for secret addtion > +#and erase all matching keyslots for erase. except last one > +#(optional, since 4.2) > +# > +# @unlock-secret: The secret to use to unlock the image > +#If not given, will use the secret that was used > +#when opening the image. > +#(optional, for amend only, since 4.2) > +# > # @iter-time: number of milliseconds to spend in > # PBKDF passphrase processing. Currently defaults > # to 2000. (since 2.8) This approach doesn't look right to me. BlockdevCreateOptions should describe the state of the image after the operation. You're describing an update instead (and in a way that doesn't allow you to change everything that you may want to change, so that you need to call the operation multiple times). I imagined the syntax of a blockdev-amend QMP command similar to x-blockdev-reopen: Describe the full set of options that you want to have in effect after the operation; if you don't want to change some option, you just specify it again with its old value. Specifically for luks, this probably means that you have a @slots, which is a list that contains at least the secret for each slot, or JSON null for a slot that should be left empty. With the same approach, you don't have to make 'size' optional in later patches, you can just require that the current size is re-specified. And later, blockdev-amend could actually allow changing the size of images if you provide a different value. Kevin
Re: [PATCH v2 02/11] qcrypto-luks: extend the create options for upcoming encryption key management
On 13.09.19 00:30, Maxim Levitsky wrote: > Now you can specify which slot to put the encryption key to > Plus add 'active' option which will let user erase the key secret > instead of adding it. > Check that active=true it when creating. > > Signed-off-by: Maxim Levitsky > --- > block/crypto.c | 2 ++ > block/crypto.h | 16 +++ > block/qcow2.c | 2 ++ > crypto/block-luks.c| 26 +++--- > qapi/crypto.json | 19 ++ > tests/qemu-iotests/082.out | 54 ++ > 6 files changed, 115 insertions(+), 4 deletions(-) (Just doing a cursory RFC-style review) I think we also want to reject unlock-secret if it’s given for creation; and I suppose it’d be more important to print which slots are OK than the slot the user has given. (It isn’t like we shouldn’t print that slot index, but it’s more likely the user knows that than what the limits are. I think.) Max signature.asc Description: OpenPGP digital signature
