CDH | Refactor secure mount module #539

Xynnn007 · 2024-04-11T03:14:44Z

The old secure mount API aligns with kata-agent's Storage object, thus brings more parameters of no use, like source, fstype, etc.

This new definition organizes parameters into options(k-v pairs) and flags (purely strings), also a volume_type field to specify the concrete secure mount type, and last a mount_point to specify the target mount point.

The change makes it much easier and straightforward to write requests when we directly use CDH inside a non-CoCo scenario.

This PR would break PR https://github.com/kata-containers/kata-containers/pull/7965/files. But as it is not merged now, we can make a new PR based on that when doing secure mount feature merge-to-main.

cc @ChengyuZhu6

The old secure mount API aligns with kata-agent's Storage object, thus brings more parameters of no use, like `source`, `fstype`, etc. This new definition organizes parameters into options(k-v pairs) and flags (purely strings), also a `volume_type` field to specify the concrete secure mount type, and last a `mount_point` to specify the target mount point. The change makes it easier to write requests when we directly use CDH inside a non-CoCo scenario. Signed-off-by: Xynnn007 <[email protected]>

This commit refactored the secure mount module according to the new API defnition. Signed-off-by: Xynnn007 <[email protected]>

Signed-off-by: Xynnn007 <[email protected]>

Before this PR, it is possible that CDH will read a file that is still not flushed onto the local filesystem. The similiar problem is the gocryptfs mount is based on oss mount. We should make sure that the oss mount succeeds and then do gocryptfs mount. Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 · 2024-04-11T03:15:46Z

confidential-data-hub/docs/use-cases/secure-mount-with-aliyun-oss.md

 {
-    "driver": "",
-    "driver_options": [
-        "alibaba-cloud-oss={\"akId\":\"XXX\",\"akSecret\":\"XXX\",\"annotations\":\"\",\"bucket\":\"<bucket-id>\",\"encrypted\":\"gocryptfs\",\"encPasswd\":\"<PASSWORD>\",\"kmsKeyId\":\"\",\"otherOpts\":\"-o max_stat_cache_size=0 -o allow_other\",\"path\":\"<bucket-path>\",\"readonly\":\"\",\"targetPath\":\"/mnt/aliyun-oss\",\"url\":\"https://oss-cn-beijing.aliyuncs.com\",\"volumeId\":\"\"}"
-    ],
-    "source": "",
-    "fstype": "",
-    "options": [],
+    "volume_type": "alibaba-cloud-oss",
+    "options": {
+        "akId": "XXX",
+        "akSecret": "XXX",
+        "annotations": "",
+        "bucket": "<bucket-id>",
+        "encrypted": "gocryptfs",
+        "encPasswd": "<PASSWORD>",
+        "kmsKeyId": "",
+        "otherOpts": "-o max_stat_cache_size=0 -o allow_other",
+        "path": "<bucket-path>",
+        "readonly": "",
+        "targetPath": "/mnt/aliyun-oss",
+        "url": "https://oss-cn-beijing.aliyuncs.com",
+        "volumeId": ""
+    },
+    "flags": [],
    "mount_point": "/mnt/target-path"
 }


I hope that this new request format makes much more sense to you. : ) @bpradipt @fitzthum

@Xynnn007 indeed :-)
A related question, can I use the secure mount functionality today to mount S3 or Azure blob volumes inside the container today ? Or code changes will be needed to support different object storages ?

Take aliyun oss for example, we might need

CDH to impl the plugin. If 1 is achieved, a confidential VM support is ok. The following points are for confidential containers.

kata-runtime & kata-agent deliver secure-mount metadata to CDH. This is what https://github.com/kata-containers/kata-containers/pull/7965/files did.

The CSI driver of S3/Azure blob/other kinds of storages should support deliver mount info to kata as aliyun-oss does

Thanks @Xynnn007. This makes sense

fitzthum

Looks pretty good overall.

I guess you're removing the source parameter (at least from the top level). I think that's fine, but I wonder if it could be useful in cases where the encrypted dir is already inside the guest i.e. mounted from the host by the kata agent.

confidential-data-hub/storage/src/volume_type/alibaba_cloud_oss/error.rs

fitzthum · 2024-04-12T17:55:29Z

confidential-data-hub/storage/src/volume_type/alibaba_cloud_oss/error.rs

+
+#[derive(Error, Debug)]
+pub enum Error {
+    #[error("Error when getting plaintext of OSS parameters")]


Might want to add {0} to the error message somewhere. I'm not sure the source error gets printed otherwise.

This seems like a high level issue. I'd suggest that we resolve this in another separate issue? Probably #541 can cover this.

Xynnn007 · 2024-04-13T01:52:31Z

I guess you're removing the source parameter (at least from the top level). I think that's fine, but I wonder if it could be useful in cases where the encrypted dir is already inside the guest i.e. mounted from the host by the kata agent.

Oh. I once thought from the CDH point of view, such a parameter could be included inside options map, as it is used to indicate how to mount. Thus setting an key named source in the options when kata-agent gets a non-empty source Storage object might also work? This hints that kata-agent should do some parameter organization work.

Signed-off-by: Xynnn007 <[email protected]>

fitzthum · 2024-04-15T16:43:35Z

Oh. I once thought from the CDH point of view, such a parameter could be included inside options map, as it is used to indicate how to mount. Thus setting an key named source in the options when kata-agent gets a non-empty source Storage object might also work? This hints that kata-agent should do some parameter organization work.

yeah, should be fine to us an option for it, but that might mean we do a bit more juggling in the kata agent. i guess it might actually be better to have an option for it since the source probably won't be used in most cases

fitzthum

LGTM

ChengyuZhu6

LGTM thanks @Xynnn007

Xynnn007 · 2024-04-16T09:35:16Z

yeah, should be fine to us an option for it, but that might mean we do a bit more juggling in the kata agent. i guess it might actually be better to have an option for it since the source probably won't be used in most cases

Well. I am still thinking about the scenarios outside CoCo, like in confidential VM.

The requests to CDH should be easy and clear. Extra source would make it confusing for users beyond kata-containers.
Even in kata-contianers, as you've mentioned, some storage does not require a host-to-guest storage volume (what source points to) like mounts directly from the internet, so we might not need such a fixed field, even it is optional.
Try to think about this: what's the scenarios where source is set? Obviously when some storage is shared from the host to the guest, we need source. We can almost ensure that directVolume is such case in kata. We can enhence https://github.com/kata-containers/kata-containers/tree/main/src/tools/csi-kata-directvolume to support encrypted mode, where source will be delivered inside options. This would meet the barrier of no-source field and we need source semantics.

So, maybe we can leave this as-is?

fitzthum · 2024-04-16T13:24:28Z

So, maybe we can leave this as-is?

That's fine. We can always change it in the future once we start implementing more storage backends.

Xynnn007 added 4 commits April 11, 2024 10:26

CDH: refactor secure mount module

0ce18a2

This commit refactored the secure mount module according to the new API defnition. Signed-off-by: Xynnn007 <[email protected]>

CDH: update the secure mount document

b46184f

Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 requested a review from sameo as a code owner April 11, 2024 03:14

Xynnn007 commented Apr 11, 2024

View reviewed changes

Xynnn007 mentioned this pull request Apr 11, 2024

CDH | EncryptedDirectMount plugin for SecureMount #540

Closed

fitzthum reviewed Apr 12, 2024

View reviewed changes

CDH/storage: reorganize aliyun plugins

e1a207d

Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 force-pushed the fix-secure-mount branch from 848cc5b to e1a207d Compare April 15, 2024 05:34

fitzthum approved these changes Apr 15, 2024

View reviewed changes

ChengyuZhu6 approved these changes Apr 16, 2024

View reviewed changes

Xynnn007 merged commit 7f60f17 into confidential-containers:main Apr 16, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CDH | Refactor secure mount module #539

CDH | Refactor secure mount module #539

Xynnn007 commented Apr 11, 2024

Xynnn007 Apr 11, 2024

bpradipt Apr 11, 2024

Xynnn007 Apr 11, 2024

bpradipt Apr 11, 2024

fitzthum left a comment

fitzthum Apr 12, 2024

Xynnn007 Apr 15, 2024 •

edited

Loading

Xynnn007 commented Apr 13, 2024 •

edited

Loading

fitzthum commented Apr 15, 2024 •

edited

Loading

fitzthum left a comment

ChengyuZhu6 left a comment

Xynnn007 commented Apr 16, 2024 •

edited

Loading

fitzthum commented Apr 16, 2024

CDH | Refactor secure mount module #539

CDH | Refactor secure mount module #539

Conversation

Xynnn007 commented Apr 11, 2024

Xynnn007 Apr 11, 2024

Choose a reason for hiding this comment

bpradipt Apr 11, 2024

Choose a reason for hiding this comment

Xynnn007 Apr 11, 2024

Choose a reason for hiding this comment

bpradipt Apr 11, 2024

Choose a reason for hiding this comment

fitzthum left a comment

Choose a reason for hiding this comment

fitzthum Apr 12, 2024

Choose a reason for hiding this comment

Xynnn007 Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Xynnn007 commented Apr 13, 2024 • edited Loading

fitzthum commented Apr 15, 2024 • edited Loading

fitzthum left a comment

Choose a reason for hiding this comment

ChengyuZhu6 left a comment

Choose a reason for hiding this comment

Xynnn007 commented Apr 16, 2024 • edited Loading

fitzthum commented Apr 16, 2024

Xynnn007 Apr 15, 2024 •

edited

Loading

Xynnn007 commented Apr 13, 2024 •

edited

Loading

fitzthum commented Apr 15, 2024 •

edited

Loading

Xynnn007 commented Apr 16, 2024 •

edited

Loading