cdh: support to encrypt block device #617

ChengyuZhu6 · 2024-07-17T06:21:57Z

Support to encrypt block device in cdh.

Fixed: #540 -- part II

Xynnn007

Nice PR! We could image that storage of CoCo could be handled in near future

confidential-data-hub/storage/src/volume_type/rbd/mod.rs

Xynnn007 · 2024-07-17T09:03:37Z

confidential-data-hub/storage/src/volume_type/rbd/mod.rs

+pub(crate) struct Rbd;
+
+async fn random_encrypt_key() -> anyhow::Result<String> {
+    let storage_key_path = "/tmp/random_encrypt_rbd.key";


Do we support multiple multiple encrypted rbd mounts? If so, do we need different keys?

confidential-data-hub/storage/src/volume_type/rbd/mod.rs

zvonkok · 2024-07-17T09:38:51Z

Can we make sure we have proper documentation about this feature? Issues, PRs and discussions are easily consumed by developers, end users need a concise way of consuming such cool features without digging through github.

ChengyuZhu6 · 2024-07-17T11:56:26Z

Can we make sure we have proper documentation about this feature? Issues, PRs and discussions are easily consumed by developers, end users need a concise way of consuming such cool features without digging through github.

Of course we should have a good document about it!

fitzthum

Looks good. A few comments.

Might be nice to come up with some kind of test.

confidential-data-hub/storage/src/volume_type/rbd/mod.rs

fitzthum · 2024-07-17T22:02:50Z

confidential-data-hub/storage/src/volume_type/rbd/mod.rs

+    Ok(base64::engine::general_purpose::STANDARD.encode(&buffer))
+}
+
+async fn get_plaintext_secret(secret: &str) -> anyhow::Result<String> {


I think I was expecting the encryption key to be a resource uri. Maybe having it be a sealed secret is better because that contains info about the KBS/HSM, integrity protection, etc., but so far we haven't used a sealed secret this way in any other parts of the CDH. Are we planning to adopt sealed secret format as a general wrapper around a resource uri?

cc: @Xynnn007

I think they are for different scenarios. Given that we now support both KMS as a KBS backend (use KBS URI to index a key) and KMS as a CDH plugin (use Sealed Secret to index a key), we might need to support both. We can add a condition judgement here, if with kbs:// prefix, use

new_getter("kbs", ProviderSettings::default()).await?.get_secret(&secret, &Annotations::default())

if with sealed. prefix, use

secret::unseal_secret(secret.as_bytes()).await?

else, raise an error saying that illegal keyid format.

Thus we could support both inside CDH code. The consequence is both deploy model / solution could be used and the user does need to worry about underlying logic.

Ok, I think supporting both makes sense. Each scheme has some pros/cons. I wonder if we should do something similar for other features.

fitzthum · 2024-07-17T22:04:46Z

confidential-data-hub/storage/src/volume_type/rbd/mod.rs

+        let rbd_parameter: RbdParameters = serde_json::from_str(&parameters)?;
+
+        if rbd_parameter.encrypt_type == RbdEncryptType::LUKS {
+            let storage_key_path = create_storage_key_file(&rbd_parameter).await?;


should we delete this file at some point?

It would be removed in the binary luks-encrypt-storage

fitzthum · 2024-07-17T22:20:38Z

confidential-data-hub/storage/src/volume_type/mod.rs

@@ -61,6 +62,12 @@ impl Storage {
                    .await?;
                Ok(self.mount_point.clone())
            }
+            Volume::Rbd => {


Rbd = rados block device? I don't see anything in this PR that is specific to RBD. Maybe this should just be called Volume::Luks or Volume::Encrypted in case we support gocryptfs in the future.

It's also interesting to note that a number of storage implementations might share the same encryption scheme but have different ways to mount the volume. I wonder if at some point we should refactor the storage design to have two phases (setup device and decrypt volume) so that some of the code could be shared. In this case we aren't doing anything to setup the device so we can probably worry about it later.

Ah, I mean RBD = raw block device. To avoid any ambiguity, we may consider coming up with a better name.

Maybe we should add some document for this. I think the RBD plugin here would mean "given a raw block device, mount it to the target path and decrypt it with given/ephemeral key".

The initial code does not care about the device setup. This also raises some open question. If CDH cares about the setup of the device, how to ensure the availability? I am not sure how kata now handles volumes if volumes are broken during runtime. Maybe we can learn something from kata.

Maybe spell it out as RawBlockDevice or just call it something else to avoid confusion.

Provisioning the device can involve operations by the kata agent (i.e. mounting something from the host) and/or operations by the CDH (i.e. pulling data from cloud storage). In the first case (this PR) I think we basically have to assume that the kata agent (or more generally the caller of the CDH) has set things up correctly.

When we start implementing more sophisticated storage backends maybe it will make sense to make things a little bit more modular.

Xynnn007

Nice, some nits.

confidential-data-hub/storage/Cargo.toml

confidential-data-hub/storage/src/volume_type/blockdevice/luks/mod.rs

confidential-data-hub/storage/src/volume_type/blockdevice/mod.rs

Xynnn007

Nice. Only last questions

Xynnn007 · 2024-07-19T06:12:44Z

confidential-data-hub/docs/SECURE_STORAGE.md

+    A[Block Device] -- mount --> B[Block Device]
+    subgraph TEE Guest
+        B -- encrypt by cryptsetup --> C[LUKS Block Device]
+        C -- mount --> D[Target Path]


Could we mark which devices are encrypted, and which are plaintext?

Xynnn007 · 2024-07-19T06:32:53Z

confidential-data-hub/docs/use-cases/secure-mount-with-block-device.md

+
+2. Install `luks-encrypt-storage`
+
+Install [luks-encrypt-storage](../../storage/scripts/luks-encrypt-storage) into `/usr/local/bin`


As a separate service, CDH could require install extra things. I am thinking about whether we can integrate the installations of luks-encrypt-storage, cachefs and ossfs binaries/scripts in CDH's Makefile. In CCv0, we explicit install them in kata scripts. This would bring extra maintaining cost for kata. cc @fitzthum @fidencio

I agree that we can move the tasks of installing binaries or scripts to CDH, as they are only used in the CDH. CDH is responsible for installing them, and we can simplify the code in kata.

fitzthum

LGTM

I'm not sure exactly the best way to handle dependencies like the script. Maybe it would be ideal if we converted that to rust at some point. For now you can probably find some way to get it into the rootfs.

fitzthum · 2024-07-19T23:04:05Z

confidential-data-hub/storage/src/volume_type/blockdevice/mod.rs

+pub(crate) struct BlockDevice;
+
+#[async_trait]
+pub trait Encryptor {


should this be encryptor or decryptor?

For trusted storage, this should be encryptor. The guest will need to follow three steps: encrypt the block device from the host as a LUKS-encrypted block device, open the encrypted device with the key, and mount it. However, for secure storage, the guest will only need to perform the latter two steps. Overall, I think encryptor or decryptor are not suitable names for it. Therefore, I have renamed it to Interpreter.

ChengyuZhu6 · 2024-07-20T04:23:12Z

@zvonkok Does the documentation make sense to you? Is there anything else that needs to be added or clarified?

ChengyuZhu6 · 2024-07-20T05:43:04Z

I'm not sure exactly the best way to handle dependencies like the script. Maybe it would be ideal if we converted that to rust at some point. For now you can probably find some way to get it into the rootfs.

Sounds like a good idea for the future. I’ll keep that in mind and we can revisit it later.

Add secure mount interface for block device. Fixed: confidential-containers#540 -- part II Signed-off-by: ChengyuZhu6 <[email protected]>

Mount block device encrypted by LUKS. Signed-off-by: ChengyuZhu6 <[email protected]>

add document for secure mount with block devices. Signed-off-by: ChengyuZhu6 <[email protected]>

Xynnn007 reviewed Jul 17, 2024

View reviewed changes

ChengyuZhu6 mentioned this pull request Jul 17, 2024

Trusted image storage kata-containers/kata-containers#9999

Merged

ChengyuZhu6 force-pushed the encrypt-device branch 8 times, most recently from 4199ece to dffc94c Compare July 17, 2024 13:58

fitzthum reviewed Jul 17, 2024

View reviewed changes

ChengyuZhu6 force-pushed the encrypt-device branch 17 times, most recently from a0b9eac to f35dc71 Compare July 18, 2024 04:41

ChengyuZhu6 force-pushed the encrypt-device branch 6 times, most recently from 6978b62 to d25571a Compare July 18, 2024 06:13

Xynnn007 reviewed Jul 18, 2024

View reviewed changes

confidential-data-hub/storage/Cargo.toml Outdated Show resolved Hide resolved

confidential-data-hub/storage/src/volume_type/blockdevice/luks/mod.rs Outdated Show resolved Hide resolved

confidential-data-hub/storage/src/volume_type/blockdevice/mod.rs Outdated Show resolved Hide resolved

ChengyuZhu6 force-pushed the encrypt-device branch 3 times, most recently from f5eea20 to d96113f Compare July 19, 2024 05:40

ChengyuZhu6 marked this pull request as ready for review July 19, 2024 05:40

ChengyuZhu6 requested a review from sameo as a code owner July 19, 2024 05:40

Xynnn007 reviewed Jul 19, 2024

View reviewed changes

ChengyuZhu6 force-pushed the encrypt-device branch 2 times, most recently from 7509271 to 7f9554d Compare July 19, 2024 07:00

fitzthum approved these changes Jul 19, 2024

View reviewed changes

ChengyuZhu6 force-pushed the encrypt-device branch 2 times, most recently from 3b0a06f to 5178802 Compare July 20, 2024 09:16

ChengyuZhu6 added 3 commits July 20, 2024 17:34

cdh: Add secure mount interface for block device

eeda808

Add secure mount interface for block device. Fixed: confidential-containers#540 -- part II Signed-off-by: ChengyuZhu6 <[email protected]>

cdh/storage: mount block device with luks encryption

525d738

Mount block device encrypted by LUKS. Signed-off-by: ChengyuZhu6 <[email protected]>

cdh/docs: add document for secure mount with block devices

de91150

add document for secure mount with block devices. Signed-off-by: ChengyuZhu6 <[email protected]>

ChengyuZhu6 force-pushed the encrypt-device branch from 5178802 to de91150 Compare July 20, 2024 09:44

Xynnn007 approved these changes Jul 22, 2024

View reviewed changes

Xynnn007 merged commit a43a325 into confidential-containers:main Jul 22, 2024
5 checks passed

ChengyuZhu6 deleted the encrypt-device branch July 22, 2024 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cdh: support to encrypt block device #617

cdh: support to encrypt block device #617

ChengyuZhu6 commented Jul 17, 2024

Xynnn007 left a comment

Xynnn007 Jul 17, 2024

zvonkok commented Jul 17, 2024

ChengyuZhu6 commented Jul 17, 2024

fitzthum left a comment

fitzthum Jul 17, 2024

Xynnn007 Jul 18, 2024 •

edited

Loading

fitzthum Jul 18, 2024

fitzthum Jul 17, 2024

ChengyuZhu6 Jul 18, 2024

fitzthum Jul 17, 2024

ChengyuZhu6 Jul 18, 2024

Xynnn007 Jul 18, 2024

fitzthum Jul 18, 2024

Xynnn007 left a comment

Xynnn007 left a comment •

edited

Loading

Xynnn007 Jul 19, 2024

ChengyuZhu6 Jul 19, 2024

Xynnn007 Jul 19, 2024

ChengyuZhu6 Jul 19, 2024

fitzthum left a comment •

edited

Loading

fitzthum Jul 19, 2024

ChengyuZhu6 Jul 20, 2024

ChengyuZhu6 commented Jul 20, 2024

ChengyuZhu6 commented Jul 20, 2024


		2. Install `luks-encrypt-storage`

		Install [luks-encrypt-storage](../../storage/scripts/luks-encrypt-storage) into `/usr/local/bin`

cdh: support to encrypt block device #617

cdh: support to encrypt block device #617

Conversation

ChengyuZhu6 commented Jul 17, 2024

Xynnn007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zvonkok commented Jul 17, 2024

ChengyuZhu6 commented Jul 17, 2024

fitzthum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xynnn007 Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xynnn007 left a comment

Choose a reason for hiding this comment

Xynnn007 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzthum left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChengyuZhu6 commented Jul 20, 2024

ChengyuZhu6 commented Jul 20, 2024

Xynnn007 Jul 18, 2024 •

edited

Loading

Xynnn007 left a comment •

edited

Loading

fitzthum left a comment •

edited

Loading