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

-- 
2.17.2

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

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

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