librbd: add encryption format support for clones (part 1/2) #40363

orozery · 2021-03-24T12:46:45Z

Summary of changes:

Change FormatRequest and LoadRequest to store/read ancestor cryptors to/from image metadata.
Add passphrase prefix when using thin format, to avoid the user from opening such image using external tools (e.g. dm-crypt).
Add format_thin API to librbd.cc, python interface, and rbd cli.
Add wrappers get_crypto and set_crypto to ImageCtx with relevant lock acquiring.
Change QEMU integration tests to use formatted clones.

Still to come: documentation update

src/librbd/ImageCtx.cc

src/librbd/crypto/EncryptionFormat.h

src/librbd/crypto/luks/EncryptionFormat.cc

src/librbd/api/Image.cc

src/librbd/crypto/FormatRequest.cc

src/librbd/crypto/Types.h

doc/man/8/rbd.rst

orozery · 2021-04-19T12:11:08Z

@trociny for the flattening operation, I will need to read/write the LUKS header.
I see 2 options:

Shutting-down the crypto object/image dispatch layers, then doing the IO, then restoring those layers.
Add an object_dispatch_flag and image_dispatch_flag to skip the crypto layers. This will require touching all functions in io\ImageDispatch.cc to translate the image dispatch flag to an object dispatch flag.

Thoughts?

trociny · 2021-04-20T06:57:06Z

@trociny for the flattening operation, I will need to read/write the LUKS header.

Sorry, I am still not quite familiar with the crypto code. I am not sure I understand why you need to read/write the LUKS header on flatten? I thought the child header, written on the clone operation, was going to be used for all child objects. Why would you need to overwrite? it on flatten?

orozery · 2021-04-20T07:23:56Z

@trociny for the flattening operation, I will need to read/write the LUKS header.

Sorry, I am still not quite familiar with the crypto code. I am not sure I understand why you need to read/write the LUKS header on flatten? I thought the child header, written on the clone operation, was going to be used for all child objects. Why would you need to overwrite? it on flatten?

I will be changing the LUKS magic string in the LUKS header to prevent external applications (e.g. dm-crypt) to try and open the cloned formatted volume.
This will be done when formatting a cloned image.

When flattening an image, I will restore the LUKS magic to re-allow parsing the image by external tools.

trociny · 2021-04-20T09:00:47Z

I will be changing the LUKS magic string in the LUKS header to prevent external applications (e.g. dm-crypt) to try and open the cloned formatted volume.

Sorry for a dumb question, but why do we need to prevent external applications from doing this?

orozery · 2021-04-20T09:18:39Z

Trying to open the image via krbd+dm-crypt should fail.

Ah, it is because krbd has not implemented the encryption yet. I see.

Ok. Then am I right understanding the problem is that when you want to change the "non-compatible" header to "compatible" after flatten is complete, for only this operation you have to write (read?) bypassing the crypto layer. If this is the case, then may be just open a new image context (with crypto disabled) for this update?

trociny · 2021-04-20T10:58:58Z

@orozery sorry, I seemed to edit your comment and writing my response there instead of making a new comment.

Restoring your response (to my dumb question) here:

Well we advertise our implementation as LUKS compatible, which is true for flat images.
For formatted clones it is no longer LUKS compatible, and thus we should note that in the header, otherwise external tools would return success upon mounting it, which could lead to data corruption.

The obvious use-case is a client which access the same images from librbd+encryption and with krbd+dm-crypt (since krbd does not support encryption for now).
Trying to open the image via krbd+dm-crypt should fail.

orozery · 2021-04-20T12:15:58Z

Trying to open the image via krbd+dm-crypt should fail.

Ah, it is because krbd has not implemented the encryption yet. I see.

Ok. Then am I right understanding the problem is that when you want to change the "non-compatible" header to "compatible" after flatten is complete, for only this operation you have to write (read?) bypassing the crypto layer. If this is the case, then may be just open a new image context (with crypto disabled) for this update?

Yep, so I need to read the old header, modify it and write.

So I should do something like to duplicate an image_ctx:

ImageCtx* raw_image_ctx = new ImageCtx(image_ctx.name, image_ctx.id,
					       nullptr, image_ctx.data_ctx, false);

raw_image_ctx->state->open(0, on_finish);

?

trociny · 2021-04-20T14:35:11Z

So I should do something like to duplicate an image_ctx:

Yes. I would try this way.

orozery · 2021-04-20T17:47:01Z

@trociny another question:
Flattening of a formatted clone must load the encryption before starting.
How do you suggest to modify the CLI to support that?

Currently I see these relevant places that we need to address:
rbd flatten
rbd migration prepare (where flatten option is used)
rbd deep cp

It would be nice to cover other places as well, e.g. rbd export, rbd bench, etc, to allow these commands to run with encryption on.
Perhaps encode encryption-spec to image-spec string to cover all RBD cli?

trociny · 2021-04-20T18:06:07Z

@trociny another question:
Flattening of a formatted clone must load the encryption before starting.
How do you suggest to modify the CLI to support that?

Currently I see these relevant places that we need to address:
rbd flatten
rbd migration prepare (where flatten option is used)

I suppose it should rather be rbd migration execute that would need update?

rbd deep cp

It would be nice to cover other places as well, e.g. rbd export, rbd bench, etc, to allow these commands to run with encryption on.
Perhaps encode encryption-spec to image-spec string to cover all RBD cli?

How do you suppose the spec would look?

Actually I thought you would need to add --encryption-format and --encryption-passphrase-file options, similar to what had been done with rbd-nbd? No?

And I think it is not necessary to update all commands in this PR, it could be done in the future requests. But sure you are free to do it in a way you like.

orozery · 2021-04-20T18:14:12Z

@trociny another question:
Flattening of a formatted clone must load the encryption before starting.
How do you suggest to modify the CLI to support that?
Currently I see these relevant places that we need to address:
rbd flatten
rbd migration prepare (where flatten option is used)

I suppose it should rather be rbd migration execute that would need update?

I just noticed that the flatten flag is parsed at Migration<I>::prepare.

rbd deep cp
It would be nice to cover other places as well, e.g. rbd export, rbd bench, etc, to allow these commands to run with encryption on.
Perhaps encode encryption-spec to image-spec string to cover all RBD cli?

How do you suppose the spec would look?

Actually I thought you would need to add --encryption-format and --encryption-passphrase-file options, similar to what had been done with rbd-nbd? No?

And I think it is not necessary to update all commands in this PR, it could be done in the future requests. But sure you are free to do it in a way you like.

Well you got me thinking...
Since we indeed already added encryption-passphrase-file and encryption-format options, we should stick to this to conform.
So I guess that for now I'll add these options just to these 3 that are doing flattening.

trociny · 2021-04-21T06:57:59Z

I just noticed that the flatten flag is parsed at Migration<I>::prepare.

Yes. But it is just stored in the migration header on this stage. The real flattening happens on execute (if the flag was set on prepare).

orozery · 2021-04-27T15:10:42Z

@trociny So I plan to add an optional encryption spec argument to:

rbd deep copy <src-image-spec> <dest-image-spec> --flatten
rbd flatten <image-spec>
rbd migration execute <image-spec> # when specifying flatten on prepare

One option is to change the format of image-spec, from:
[pool-name/[namespace-name/]]image-name
to something like:
[pool-name/[namespace-name/]]image-name[:key1=value1,key2=value2,...]
like QEMU is doing in its RBD driver.

This will be the easiest to implement.

Second option will be to add a new optional argument like --encryption-spec for the last 2, and --src-encryption-spec / --dest-encryption-spec for deep-copy.

Thoughts?

trociny · 2021-04-27T15:24:33Z

@trociny So I plan to add an optional encryption spec argument to:
rbd deep copy <src-image-spec> <dest-image-spec> --flatten
rbd flatten <image-spec>
rbd migration execute <image-spec> # when specifying flatten on prepare
One option is to change the format of image-spec, from:
[pool-name/[namespace-name/]]image-name
to something like:
[pool-name/[namespace-name/]]image-name[:key1=value1,key2=value2,...]
like QEMU is doing in its RBD driver.

I don't like the idea of overloading the image spec. I can see reasons why qemu would need, but in our case, when it can be easily provided via optional arguments... And If you plan to avoid optional arguments this way, you will not, because we allow to specify image spec both via one positional argument and via optional arguments (like --pool and --image).

This will be the easiest to implement.

Second option will be to add a new optional argument like --encryption-spec for the last 2, and --src-encryption-spec / --dest-encryption-spec for deep-copy.

I still don't understand, why don't you want to use --encryption-format and --encryption-passphrase-file?

orozery · 2021-04-27T15:32:43Z

@trociny So I plan to add an optional encryption spec argument to:
rbd deep copy <src-image-spec> <dest-image-spec> --flatten
rbd flatten <image-spec>
rbd migration execute <image-spec> # when specifying flatten on prepare
One option is to change the format of image-spec, from:
[pool-name/[namespace-name/]]image-name
to something like:
[pool-name/[namespace-name/]]image-name[:key1=value1,key2=value2,...]
like QEMU is doing in its RBD driver.
I don't like the idea of overloading the image spec. I can see reasons why qemu would need, but in our case, when it can be easily provided via optional arguments... And If you plan to avoid optional arguments this way, you will not, because we allow to specify image spec both via one positional argument and via optional arguments (like --pool and --image).

This will be the easiest to implement.
Second option will be to add a new optional argument like --encryption-spec for the last 2, and --src-encryption-spec / --dest-encryption-spec for deep-copy.

I still don't understand, why don't you want to use --encryption-format and --encryption-passphrase-file?

Same thing, that's what I meant.

But for deep copy, we'll do:
--source-encryption-format, --dest-encryption-format?

trociny · 2021-04-27T15:48:32Z

But for deep copy, we'll do:
--source-encryption-format, --dest-encryption-format?

We are still talking about flatten case only, right? Then do we need the destination encryption? I though we just needed the source encryption settings in this case. Or are you also thinking about the case when we want to change the destination image encryption?

If you are going to work only on the flatten case right now, then I think just --encryption-format will be fine, and later it may be made alias to --source-encryption-format (we already use this convention for other options).

orozery · 2021-04-27T15:51:24Z

But for deep copy, we'll do:
--source-encryption-format, --dest-encryption-format?

We are still talking about flatten case only, right? Then do we need the destination encryption? I though we just needed the source encryption settings in this case. Or are you also thinking about the case when we want to change the destination image encryption?

If you are going to work only on the flatten case right now, then I think just --encryption-format will be fine, and later it may be made alias to --source-encryption-format (we already use this convention for other options).

Got you.
So we'll start with just the source encryption spec which will be denoted by --encryption-format and --encryption-passphrase-file.
Note that in this case the destination image will be plaintext.

trociny · 2021-04-27T15:52:28Z

And again, making flatten work could be a goal for another PR. In this PR you could just add a check and make the commands (with flatten option) fail if encryption is used. But as you wish.

trociny · 2021-04-27T15:57:30Z

So we'll start with just the source encryption spec which will be denoted by --encryption-format and --encryption-passphrase-file.
Note that in this case the destination image will be plaintext.

Could it be encrypted as the source? Because I suppose this will be the case when --flatten is not specified and this is what the user will likely to expect.

If it requires much additional work (i.e. implementing the destination encryption) then may be just forbid flatten with encryption right now? Because we will not be able to change this behavior (encrypted -> plain) later.

src/librbd/crypto/LoadRequest.cc

idryomov · 2022-08-25T12:05:49Z

src/librbd/crypto/luks/FlattenRequest.h

+   * FLUSH
+   *    |
+   *    v
+   * <finish> (+ RESTORE_CRYPTO)


I think I already addressed the review you posted a few weeks ago. There are a few other nits you posted a few days ago which should be addressed in the changes I pushed today.

Those are indeed addressed but there are still three nits (one in luks/test_mock_FlattenRequest.cc, one in luks/test_mock_LoadRequest.cc and one in luks/LoadRequest.cc) from July:

librbd: add encryption format support for clones (part 1/2) #40363 (comment)

librbd: add encryption format support for clones (part 1/2) #40363 (comment)

librbd: add encryption format support for clones (part 1/2) #40363 (comment)

As for this flattening issue, I'm still experimenting. I believe I have something promising but it is not complete (yet to be fully compiled even -- I keep being distracted). I will update this thread when I have more.

CryptoInterface was being a reference counted for lifecycle management. In this commit, we remove the ref-counting, and instead have it owned by EncryptionFormat, which in turn is owned by ImageCtx. This commit also changes crypto::LoadRequest to actually load new encryption instances to parent images, instead of blind-copying the encryption instances from the child image. Signed-off-by: Or Ozeri <oro@il.ibm.com>

Add further useful debug loggings to librbd crypto code. Additionally, increase libcryptsetup debug level to 30 (as it prints out to stdout). Signed-off-by: Or Ozeri <oro@il.ibm.com>

Currently luks format operation sets the data offset to be aligned with the image object size. When using non-trivial striping, rbd objects can contain a combination of luks header and actual encrypted data, which will pose a problem later on for flattening a cloned image that was formatted. Therefore, we change the data offset to align with stripe_period instead. Furthermore, the written header is zero-padded to stripe period alignment, to ensure that parent plaintext won't be copied-up while writing it. Signed-off-by: Or Ozeri <oro@il.ibm.com>

The LUKS master key is read in LoadRequest<I>::read_volume_key() and saved to the stack. Add a wipe to that key at the end of the function. Signed-off-by: Or Ozeri <oro@il.ibm.com>

crypto loading involves reading image data. This data may get cached and should be invalidated before completing the load. This commit adds a complete image flush before loading, as well as complete cache invalidation after loading completes. Signed-off-by: Or Ozeri <oro@il.ibm.com>

handle_read so far assumed that a successful read returns 0. This is true for the core layer which returns 0 at ObjectRequest.cc. However, this is not the case for ParentCacheObjectDispatch, which returns the (possibly positive) size of data read, instead of 0. To account for that, we change the success check from r==0 to r>=0. Signed-off-by: Or Ozeri <oro@il.ibm.com>

This commit fixes ParentCacheObjectDispatch to treat read_flags, specifically READ_FLAG_DISABLE_READ_FROM_PARENT. Signed-off-by: Or Ozeri <oro@il.ibm.com>

This commit changes ShutDownCryptoRequest to shutdown crypto not only on the given image context, but also for for ancestor (e.g. parent) image contexts. Signed-off-by: Or Ozeri <oro@il.ibm.com>

This commit switches the luks magic from LUKS to RBDL when formatting a cloned image, so that if the user tries to load the image using external LUKS parser (such as dm-crypt) it will fail. Note that formatted clones are not valid LUKS format, and thus we aim to fail LUKS parsers. Signed-off-by: Or Ozeri <oro@il.ibm.com>

This commit adds a flatten operation to EncryptionFormat, which allows defining custom format-specific flattening operation. Specifically, when flattening a LUKS format, we replace back the magic from RBDL to LUKS, to make the image parse-able again by LUKS parsers, such as dm-crypt. Furthermore, we copy the LUKS header from the parent to the child image. Signed-off-by: Or Ozeri <oro@il.ibm.com>

This commits extends the crypto FormatRequest and LoadRequest to support thin formatting of cloned images. This code is shadowed, as it is currently non-reachable from the librbd external API. Signed-off-by: Or Ozeri <oro@il.ibm.com>

This commit enables correct flattening of images where encryption is loaded. In particular, this means: 1. Flattening the encryption header (which is non addressable when encryption is loaded) 2. Running format-specific flattening operation (i.e. reverting the LUKS magic in the encryption header) The allow the above 2 operations, we shut-down the crypto layers, and restore them back when these operations end. The above operations are performed as part of a format-specific handler (LUKSEncryptionFormat::flatten). Signed-off-by: Or Ozeri <oro@il.ibm.com>

Encryption loading (i.e. rbd_encryption_load) gets a single passphrase and tries to applies it to all ancestor images. If it fails, the entire load fails. This commits extends encryption loading to assume ancestor is actually in plaintext format if no known encryption header magic is detected. Signed-off-by: Or Ozeri <oro@il.ibm.com>

This commit extends encryption_format and adds encryption_load2 to librbd API, which enables crypto formatting and loading of cloned images. i.e. the child image is encrypted with a key different from its parent, while keeping the child thinly-provisioned. Signed-off-by: Or Ozeri <oro@il.ibm.com>

This commit adds the encryption format support for cloned images via the RBD cli, making the child image be encrypted with a key different from it parent, while keeping the child thinly-provisioned. Additionally, other APIs are extended to support flattening of such images. Signed-off-by: Or Ozeri <oro@il.ibm.com>

This commit changes the format for encrypted disks to have the child image and the parent image encrypted with different keys. This to allow testing of the new formatted clones feature in librbd/crypto. Signed-off-by: Or Ozeri <oro@il.ibm.com>

idryomov · 2022-08-29T10:57:29Z

Seemingly no related failures but all CentOS and RHEL jobs are broken, see https://tracker.ceph.com/issues/57332. The second and third runs which look much better are with the main CentOS and RHEL facets taken out of the matrix:

https://pulpito.ceph.com/dis-2022-08-27_23:30:08-rbd-wip-dis-testing-distro-default-smithi/
https://pulpito.ceph.com/dis-2022-08-28_08:32:24-rbd-wip-dis-testing-distro-default-smithi/
https://pulpito.ceph.com/dis-2022-08-28_20:29:14-rbd-wip-dis-testing-distro-default-smithi/

idryomov · 2022-08-29T10:57:39Z

jenkins test make check

idryomov · 2022-08-29T10:57:55Z

jenkins test api

idryomov · 2022-10-23T17:22:50Z

jenkins test api

idryomov · 2022-10-25T06:41:06Z

No related failures (about a dozen jobs didn't run due to infrastructure issues but this would be tested again with encrypted flatten, etc changes anyway):

https://pulpito.ceph.com/dis-2022-10-24_00:37:15-rbd-wip-dis-testing-distro-default-smithi/
https://pulpito.ceph.com/dis-2022-10-24_14:04:04-rbd-wip-dis-testing-distro-default-smithi/

idryomov

Merging this as is because, if appended here, encrypted I/O path + encrypted flatten + encrypted resize fixes would grow this PR by over a half, to ~5000 lines of code.

github-actions bot added build/ops documentation pybind rbd tests labels Mar 24, 2021

orozery force-pushed the rbd-clone-encryption branch 4 times, most recently from ee29748 to 0285ee0 Compare March 24, 2021 19:58

dillaman reviewed Mar 25, 2021

View reviewed changes

trociny reviewed Apr 18, 2021

View reviewed changes

doc/man/8/rbd.rst Outdated Show resolved Hide resolved

idryomov mentioned this pull request Aug 18, 2022

librbd/io: allow to skip crypto layers for image reads/writes #47601

Closed

14 tasks

orozery force-pushed the rbd-clone-encryption branch from f60190c to c42bad3 Compare August 21, 2022 11:03

idryomov reviewed Aug 25, 2022

View reviewed changes

orozery added 16 commits August 25, 2022 16:58

librbd/crypto: add further debug loggings

0ab002e

Add further useful debug loggings to librbd crypto code. Additionally, increase libcryptsetup debug level to 30 (as it prints out to stdout). Signed-off-by: Or Ozeri <oro@il.ibm.com>

librbd/crypto: missing key wiping when leaving LoadRequest

c765ce3

The LUKS master key is read in LoadRequest<I>::read_volume_key() and saved to the stack. Add a wipe to that key at the end of the function. Signed-off-by: Or Ozeri <oro@il.ibm.com>

librbd/cache: treat read_flags at ParentCacheObjectDispatch

12cc945

This commit fixes ParentCacheObjectDispatch to treat read_flags, specifically READ_FLAG_DISABLE_READ_FROM_PARENT. Signed-off-by: Or Ozeri <oro@il.ibm.com>

librbd/crypto: apply ShutDownCryptoRequest to ancestors

85aca44

This commit changes ShutDownCryptoRequest to shutdown crypto not only on the given image context, but also for for ancestor (e.g. parent) image contexts. Signed-off-by: Or Ozeri <oro@il.ibm.com>

orozery force-pushed the rbd-clone-encryption branch from c42bad3 to 3b2908b Compare August 25, 2022 15:47

idryomov added the wip-dis-testing label Aug 29, 2022

idryomov approved these changes Oct 25, 2022

View reviewed changes

idryomov changed the title ~~librbd: add encryption format support for clones~~ librbd: add encryption format support for clones (part 1/2) Oct 25, 2022

idryomov merged commit 0f93f74 into ceph:main Oct 25, 2022
11 of 12 checks passed

idryomov mentioned this pull request Oct 25, 2022

librbd: add encryption format support for clones (part 2/2) #48618

Merged

librbd: add encryption format support for clones (part 1/2) #40363

librbd: add encryption format support for clones (part 1/2) #40363

orozery commented Mar 24, 2021

orozery commented Apr 19, 2021

trociny commented Apr 20, 2021

orozery commented Apr 20, 2021

trociny commented Apr 20, 2021

orozery commented Apr 20, 2021 •

edited by trociny

trociny commented Apr 20, 2021

orozery commented Apr 20, 2021 •

edited

trociny commented Apr 20, 2021

orozery commented Apr 20, 2021

trociny commented Apr 20, 2021

orozery commented Apr 20, 2021

trociny commented Apr 21, 2021

orozery commented Apr 27, 2021

trociny commented Apr 27, 2021

orozery commented Apr 27, 2021

trociny commented Apr 27, 2021

orozery commented Apr 27, 2021

trociny commented Apr 27, 2021

trociny commented Apr 27, 2021 •

edited

idryomov Aug 25, 2022

idryomov commented Aug 29, 2022 •

edited

idryomov commented Aug 29, 2022

idryomov commented Aug 29, 2022

idryomov commented Oct 23, 2022

idryomov commented Oct 25, 2022

idryomov left a comment

librbd: add encryption format support for clones (part 1/2) #40363

librbd: add encryption format support for clones (part 1/2) #40363

Conversation

orozery commented Mar 24, 2021

orozery commented Apr 19, 2021

trociny commented Apr 20, 2021

orozery commented Apr 20, 2021

trociny commented Apr 20, 2021

orozery commented Apr 20, 2021 • edited by trociny

trociny commented Apr 20, 2021

orozery commented Apr 20, 2021 • edited

trociny commented Apr 20, 2021

orozery commented Apr 20, 2021

trociny commented Apr 20, 2021

orozery commented Apr 20, 2021

trociny commented Apr 21, 2021

orozery commented Apr 27, 2021

trociny commented Apr 27, 2021

orozery commented Apr 27, 2021

trociny commented Apr 27, 2021

orozery commented Apr 27, 2021

trociny commented Apr 27, 2021

trociny commented Apr 27, 2021 • edited

idryomov Aug 25, 2022

Choose a reason for hiding this comment

idryomov commented Aug 29, 2022 • edited

idryomov commented Aug 29, 2022

idryomov commented Aug 29, 2022

idryomov commented Oct 23, 2022

idryomov commented Oct 25, 2022

idryomov left a comment

Choose a reason for hiding this comment

orozery commented Apr 20, 2021 •

edited by trociny

orozery commented Apr 20, 2021 •

edited

trociny commented Apr 27, 2021 •

edited

idryomov commented Aug 29, 2022 •

edited