| Message ID | 20190912223028.18496-1-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | RFC crypto/luks: encryption key managment using amend interface | expand |
On 9/12/19 6:30 PM, Maxim Levitsky wrote: > This patch series is continuation of my work to add encryption > key managment to luks/qcow2 with luks. > > This is second version of this patch set. > The changes are mostly addressing the review feedback, > plus I tested (and fixed sadly) the somewhat ugly code > that allows to still write share a raw luks device, > while preveting the key managment from happening in this case, > as it is unsafe. > I added a new iotest dedicated to that as well. > > Best regards, > Maxim Levitsky > What branch is this based on? It doesn't seem to apply to origin/master. --js
On Fri, 2019-09-20 at 17:14 -0400, John Snow wrote: > > On 9/12/19 6:30 PM, Maxim Levitsky wrote: > > This patch series is continuation of my work to add encryption > > key managment to luks/qcow2 with luks. > > > > This is second version of this patch set. > > The changes are mostly addressing the review feedback, > > plus I tested (and fixed sadly) the somewhat ugly code > > that allows to still write share a raw luks device, > > while preveting the key managment from happening in this case, > > as it is unsafe. > > I added a new iotest dedicated to that as well. > > > > Best regards, > > Maxim Levitsky > > > > What branch is this based on? > It doesn't seem to apply to origin/master. > > --js It is based on refactoring patch series I send before, which is also under review: "[PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment" Best regards, Maxim Levitsky
On Fri, 2019-09-20 at 17:14 -0400, John Snow wrote: > > On 9/12/19 6:30 PM, Maxim Levitsky wrote: > > This patch series is continuation of my work to add encryption > > key managment to luks/qcow2 with luks. > > > > This is second version of this patch set. > > The changes are mostly addressing the review feedback, > > plus I tested (and fixed sadly) the somewhat ugly code > > that allows to still write share a raw luks device, > > while preveting the key managment from happening in this case, > > as it is unsafe. > > I added a new iotest dedicated to that as well. > > > > Best regards, > > Maxim Levitsky > > > > What branch is this based on? > It doesn't seem to apply to origin/master. > > --js Hi! Most of the refactoring patches are now on the master branch, (one patch was dropped due to me being blind :-(), so should I resend this patch series with the missing patch or wait for some review? At that stage I would like to hear about agreement/disagreement on the new APIs, and stuff like that. Best regards, Maxim Levitsky
On 13.09.19 00:30, Maxim Levitsky wrote: > This patch series is continuation of my work to add encryption > key managment to luks/qcow2 with luks. > > This is second version of this patch set. > The changes are mostly addressing the review feedback, > plus I tested (and fixed sadly) the somewhat ugly code > that allows to still write share a raw luks device, > while preveting the key managment from happening in this case, > as it is unsafe. > I added a new iotest dedicated to that as well. > > Best regards, > Maxim Levitsky At least for an RFC looks good from my perspective. I didn’t look at the crypto things very closely (assuming Dan would do so), and I didn’t check the iotests in detail. (But it definitely doesn’t look like they lack in breadth. Maybe I’d like to see a test that you cannot have other useful nodes attached to the LUKS or qcow2 node while the amendment process is ongoing (because CONSISTENT_READ is unshared). But that’s the only thing I can think of.) Max
Maxim Levitsky <mlevitsk@redhat.com> writes: > On Fri, 2019-09-20 at 17:14 -0400, John Snow wrote: >> >> On 9/12/19 6:30 PM, Maxim Levitsky wrote: >> > This patch series is continuation of my work to add encryption >> > key managment to luks/qcow2 with luks. >> > >> > This is second version of this patch set. >> > The changes are mostly addressing the review feedback, >> > plus I tested (and fixed sadly) the somewhat ugly code >> > that allows to still write share a raw luks device, >> > while preveting the key managment from happening in this case, >> > as it is unsafe. >> > I added a new iotest dedicated to that as well. >> > >> > Best regards, >> > Maxim Levitsky >> > >> >> What branch is this based on? >> It doesn't seem to apply to origin/master. >> >> --js > It is based on refactoring patch series I send before, > which is also under review: > "[PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment" Recommend to note such dependencies in the cover letter as Based-on: <message-id>
On Mon, 2019-10-07 at 10:05 +0200, Markus Armbruster wrote: > Maxim Levitsky <mlevitsk@redhat.com> writes: > > > On Fri, 2019-09-20 at 17:14 -0400, John Snow wrote: > > > > > > On 9/12/19 6:30 PM, Maxim Levitsky wrote: > > > > This patch series is continuation of my work to add encryption > > > > key managment to luks/qcow2 with luks. > > > > > > > > This is second version of this patch set. > > > > The changes are mostly addressing the review feedback, > > > > plus I tested (and fixed sadly) the somewhat ugly code > > > > that allows to still write share a raw luks device, > > > > while preveting the key managment from happening in this case, > > > > as it is unsafe. > > > > I added a new iotest dedicated to that as well. > > > > > > > > Best regards, > > > > Maxim Levitsky > > > > > > > > > > What branch is this based on? > > > It doesn't seem to apply to origin/master. > > > > > > --js > > > > It is based on refactoring patch series I send before, > > which is also under review: > > "[PATCH v2 00/13] RFC crypto/luks: preparation for encryption key managment" > > Recommend to note such dependencies in the cover letter as > > Based-on: <message-id> I'll take a note! Note that now all these patches are merged thus, this patch series should more or less apply on top of master branch. I'll probably resend a V3 after I finish going over the review of this series. Best regards, Maxim Levitsky
On Fri, 2019-10-04 at 21:10 +0200, Max Reitz wrote: > On 13.09.19 00:30, Maxim Levitsky wrote: > > This patch series is continuation of my work to add encryption > > key managment to luks/qcow2 with luks. > > > > This is second version of this patch set. > > The changes are mostly addressing the review feedback, > > plus I tested (and fixed sadly) the somewhat ugly code > > that allows to still write share a raw luks device, > > while preveting the key managment from happening in this case, > > as it is unsafe. > > I added a new iotest dedicated to that as well. > > > > Best regards, > > Maxim Levitsky > > At least for an RFC looks good from my perspective. I didn’t look at > the crypto things very closely (assuming Dan would do so), and I didn’t > check the iotests in detail. (But it definitely doesn’t look like they > lack in breadth. Maybe I’d like to see a test that you cannot have > other useful nodes attached to the LUKS or qcow2 node while the > amendment process is ongoing (because CONSISTENT_READ is unshared). But > that’s the only thing I can think of.) Could you elaborate on this? Inside the same process several users can access that luks node at the same time while one of them changes encryption keys, since this doesn't affect IO of the data. Two users in same process I was *I think* told that can't do the amend in the same time since qmp is protected with a lock. However since I use a block job (to be consistent with blockdev-create) I wonder if several qmp amend commands couldn't race one with another. These jobs is running on the block device AIO context (I changed this recently after a review), but stil I am not sure there can't be a race. And when there is access to the same image from multiple processes, I do have a test that checks that as long as more that one process has the image open, noone can change the encryption keys (this is only relevant for raw luks format, since for qcow2 this is forbidden anyway). I probably missed something though. Best regards, Maxim Levitsky > > Max >
On 08.11.19 16:07, Maxim Levitsky wrote: > On Fri, 2019-10-04 at 21:10 +0200, Max Reitz wrote: >> On 13.09.19 00:30, Maxim Levitsky wrote: >>> This patch series is continuation of my work to add encryption >>> key managment to luks/qcow2 with luks. >>> >>> This is second version of this patch set. >>> The changes are mostly addressing the review feedback, >>> plus I tested (and fixed sadly) the somewhat ugly code >>> that allows to still write share a raw luks device, >>> while preveting the key managment from happening in this case, >>> as it is unsafe. >>> I added a new iotest dedicated to that as well. >>> >>> Best regards, >>> Maxim Levitsky >> >> At least for an RFC looks good from my perspective. I didn’t look at >> the crypto things very closely (assuming Dan would do so), and I didn’t >> check the iotests in detail. (But it definitely doesn’t look like they >> lack in breadth. Maybe I’d like to see a test that you cannot have >> other useful nodes attached to the LUKS or qcow2 node while the >> amendment process is ongoing (because CONSISTENT_READ is unshared). But >> that’s the only thing I can think of.) > Could you elaborate on this? > > Inside the same process several users can access that luks node at the same > time while one of them changes encryption keys, since this doesn't affect IO of the data. > > Two users in same process I was *I think* told that can't do the amend in the same time > since qmp is protected with a lock. However since I use a block job (to be consistent with blockdev-create) > I wonder if several qmp amend commands couldn't race one with another. These jobs is running > on the block device AIO context (I changed this recently after a review), but stil I am not sure > there can't be a race. > > And when there is access to the same image from multiple processes, I do have a test that > checks that as long as more that one process has the image open, noone can change the encryption keys > (this is only relevant for raw luks format, since for qcow2 this is forbidden anyway). Yes, sorry, I don’t remember/know where I got the qcow2 part from. (I probably just forgot during after reviewing that only LUKS’s permissions are changed by this series.) But for LUKS, those changed permissions do apply. If you can’t do something between two different qemu instances, you can’t do it in a single one: The file locks are equivalent to the internal permission mask. So if you can’t change the encryption keys while another process has the image open, you can’t change the encryption keys while another node uses the file node in the same process. For example, you can’t attach two LUKS nodes to a single file node and then change the keys on one of the nodes. Max
On Tue, 2019-11-12 at 12:58 +0100, Max Reitz wrote: > On 08.11.19 16:07, Maxim Levitsky wrote: > > On Fri, 2019-10-04 at 21:10 +0200, Max Reitz wrote: > > > On 13.09.19 00:30, Maxim Levitsky wrote: > > > > This patch series is continuation of my work to add encryption > > > > key managment to luks/qcow2 with luks. > > > > > > > > This is second version of this patch set. > > > > The changes are mostly addressing the review feedback, > > > > plus I tested (and fixed sadly) the somewhat ugly code > > > > that allows to still write share a raw luks device, > > > > while preveting the key managment from happening in this case, > > > > as it is unsafe. > > > > I added a new iotest dedicated to that as well. > > > > > > > > Best regards, > > > > Maxim Levitsky > > > > > > At least for an RFC looks good from my perspective. I didn’t look at > > > the crypto things very closely (assuming Dan would do so), and I didn’t > > > check the iotests in detail. (But it definitely doesn’t look like they > > > lack in breadth. Maybe I’d like to see a test that you cannot have > > > other useful nodes attached to the LUKS or qcow2 node while the > > > amendment process is ongoing (because CONSISTENT_READ is unshared). But > > > that’s the only thing I can think of.) > > > > Could you elaborate on this? > > > > Inside the same process several users can access that luks node at the same > > time while one of them changes encryption keys, since this doesn't affect IO of the data. > > > > Two users in same process I was *I think* told that can't do the amend in the same time > > since qmp is protected with a lock. However since I use a block job (to be consistent with blockdev-create) > > I wonder if several qmp amend commands couldn't race one with another. These jobs is running > > on the block device AIO context (I changed this recently after a review), but stil I am not sure > > there can't be a race. > > > > And when there is access to the same image from multiple processes, I do have a test that > > checks that as long as more that one process has the image open, noone can change the encryption keys > > (this is only relevant for raw luks format, since for qcow2 this is forbidden anyway). > > Yes, sorry, I don’t remember/know where I got the qcow2 part from. (I > probably just forgot during after reviewing that only LUKS’s permissions > are changed by this series.) > > But for LUKS, those changed permissions do apply. If you can’t do > something between two different qemu instances, you can’t do it in a > single one: The file locks are equivalent to the internal permission mask. > > So if you can’t change the encryption keys while another process has the > image open, you can’t change the encryption keys while another node uses > the file node in the same process. For example, you can’t attach two > LUKS nodes to a single file node and then change the keys on one of the > nodes. > > Max > Ah, I understand now. I'll add a test for that! Best regards, Maxim Levitsky
This patch series is continuation of my work to add encryption key managment to luks/qcow2 with luks. This is second version of this patch set. The changes are mostly addressing the review feedback, plus I tested (and fixed sadly) the somewhat ugly code that allows to still write share a raw luks device, while preveting the key managment from happening in this case, as it is unsafe. I added a new iotest dedicated to that as well. Best regards, Maxim Levitsky Maxim Levitsky (11): qcrypto: add suport for amend options qcrypto-luks: extend the create options for upcoming encryption key management qcrypto-luks: implement the encryption key management block: amend: add 'force' option block/crypto: implement the encryption key management qcow2: implement crypto amend options block: add x-blockdev-amend qmp command block/crypto: implement blockdev-amend block/qcow2: implement blockdev-amend iotests: filter few more luks specific create options iotests : add tests for encryption key management block.c | 4 +- block/Makefile.objs | 2 +- block/amend.c | 116 ++++++++++ block/crypto.c | 167 +++++++++++++- block/crypto.h | 16 ++ block/qcow2.c | 151 ++++++++++-- crypto/block-luks.c | 382 ++++++++++++++++++++++++++++++- crypto/block.c | 31 +++ crypto/blockpriv.h | 8 + include/block/block.h | 1 + include/block/block_int.h | 22 +- include/crypto/block.h | 22 ++ qapi/block-core.json | 39 +++- qapi/crypto.json | 19 ++ qapi/job.json | 4 +- qemu-img-cmds.hx | 4 +- qemu-img.c | 8 +- qemu-img.texi | 6 +- tests/qemu-iotests/082.out | 54 +++++ tests/qemu-iotests/087.out | 6 +- tests/qemu-iotests/134.out | 2 +- tests/qemu-iotests/158.out | 4 +- tests/qemu-iotests/188.out | 2 +- tests/qemu-iotests/189.out | 4 +- tests/qemu-iotests/198.out | 4 +- tests/qemu-iotests/300 | 202 ++++++++++++++++ tests/qemu-iotests/300.out | 98 ++++++++ tests/qemu-iotests/301 | 90 ++++++++ tests/qemu-iotests/301.out | 30 +++ tests/qemu-iotests/302 | 252 ++++++++++++++++++++ tests/qemu-iotests/302.out | 18 ++ tests/qemu-iotests/303 | 228 ++++++++++++++++++ tests/qemu-iotests/303.out | 28 +++ tests/qemu-iotests/common.filter | 6 +- tests/qemu-iotests/group | 9 + 35 files changed, 1986 insertions(+), 53 deletions(-) create mode 100644 block/amend.c create mode 100755 tests/qemu-iotests/300 create mode 100644 tests/qemu-iotests/300.out create mode 100755 tests/qemu-iotests/301 create mode 100644 tests/qemu-iotests/301.out create mode 100644 tests/qemu-iotests/302 create mode 100644 tests/qemu-iotests/302.out create mode 100644 tests/qemu-iotests/303 create mode 100644 tests/qemu-iotests/303.out