cdh: add secure mount feature in cdh #345

LindaYu17 · 2023-09-07T03:16:08Z

No description provided.

Xynnn007

Thank you @LindaYu17 for the contribution! only some nits..

Xynnn007 · 2023-09-07T03:48:25Z

confidential-data-hub/storage/test/mountinfo.json

+{  "volume-type":"alibaba-cloud-oss",
+    "device":"/var/lib/kubelet/pods/xxxx-xxxx-xxxx-xxxx/volumes/kubernetes.io~csi/pv-oss-direct-assigned/mount",
+    "fstype":"oss",
+    "metadata":
+    {
+        "akId":"sealed.XXX",
+        "akSecret":"sealed.XXX",
+        "annotations":"",
+        "bucket":"oss",
+        "encrypted":"gocryptfs",
+        "encPasswd":"sealed.XXX",
+        "kmsKeyId":"",
+        "otherOpts":"-o max_stat_cache_size=0 -o allow_other",
+        "path":"/",
+        "readonly":"false",
+        "targetPath":"/var/lib/kubelet/pods/xxxx-xxxx-xxxx-xxxx/volumes/kubernetes.io~csi/pv-oss-direct-assigned/mount",
+        "url":"oss-cn-hangzhou.aliyuncs.com",
+        "volumeId":"pv-oss-direct-assigned"
+    },
+    "options":["-o","max_stat_cache_size=0","-o","allow_other"]
+}


I think it is better to include this test json in the concrete implementation of fn secure_mount PR, or it would be confusing to other reviewers

Xynnn007 · 2023-09-07T03:50:21Z

confidential-data-hub/hub/protos/api.proto

@@ -18,10 +18,22 @@ message GetResourceResponse {
    bytes Resource = 1;
 }

+message SecureMountRequest {
+    bytes mountInfo = 1;


so it is implied that the concrete mount type (s.t. driver type to be called) would be included inside the mountInfo? Thus do we have a common struct format of the parameter?

Xynnn007 · 2023-09-07T03:51:02Z

confidential-data-hub/hub/protos/api.proto

+}
+
+message SecureMountResponse {
+    bytes mountPath = 1;


only a little question: as path is a string, is it better to use a string rather than a bytes?

fitzthum · 2023-09-12T20:22:19Z

confidential-data-hub/hub/src/hub.rs

@@ -66,4 +66,8 @@ impl DataHub for Hub {
            .map_err(|e| Error::GetResource(format!("get rersource failed: {e}")))?;
        Ok(res)
    }
+
+    async fn secure_mount(&self, mount_info: Vec<u8>) -> Result<Vec<u8>> {
+        todo!()


Do you want to do this feature in multiple parts or should we wait for this to be implemented before merging?

Do you want to do this feature in multiple parts or should we wait for this to be implemented before merging?

the feature is still under development, and the patches will be submitted within this PR soon, so we can wait for them. Thank you @fitzthum

You might want to make this PR into a draft for now btw

the development is done now, and I will update based on the review comments.

jepio · 2023-09-15T09:51:24Z

Please don't add compiled binaries to the repository. If they are needed by this feature then you have to build them into the kata guest image in the kata-containers repo.

fitzthum · 2023-09-15T16:32:36Z

Please don't add compiled binaries to the repository. If they are needed by this feature then you have to build them into the kata guest image in the kata-containers repo.

Also, depending on how large dependencies are (including for future storage options, we might need to think about whether we can really support a generic image.

fitzthum

I know this isn't done yet, but I was looking through it and made a few comments.

fitzthum · 2023-09-15T19:42:07Z

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

+                    .to_string();
+
+                match volumetype.as_str() {
+                    "alibaba-cloud-oss" => {


Maybe we should have a feature to enable/disable particular storage vendors?

fixed, thanks

fitzthum · 2023-09-15T19:45:18Z

confidential-data-hub/storage/src/volume_type/alibaba_cloud_oss/scripts/mount.sh

Can we just call ossfs and gocryptfs directly from Rust and get rid of this script?

need the oss account to do more test, and then I will update accordingly, thanks

call ossfs and gocryptfs from rust in latest commit, and the script is removed. Thanks

fitzthum · 2023-09-15T19:46:03Z

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

+                .arg("-o")
+                .arg(self.other_opts.clone())
+                .arg("-d")
+                .arg(source)


-d is the mountpoint according to the script. It's a bit confusing to me that it is set to the source here.

mount.sh is removed.

LindaYu17 · 2023-09-20T08:01:54Z

Please don't add compiled binaries to the repository. If they are needed by this feature then you have to build them into the kata guest image in the kata-containers repo.

removed, thanks

Xynnn007 · 2023-09-20T10:43:51Z

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

+                .ok_or(anyhow!("split by \"=\" failed"))
+                .map_err(|e| Error::SecureMountFailed(format!("split by \"=\" failed: {e}")))?;


Suggested change

.ok_or(anyhow!("split by \"=\" failed"))

.map_err(|e| Error::SecureMountFailed(format!("split by \"=\" failed: {e}")))?;

.ok_or(Error::SecureMountFailed(format!("split by \"=\" failed)))?;

fixed ,thanks

Xynnn007 · 2023-09-20T10:45:03Z

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

+                &_ => {
+                    return Err(Error::SecureMountFailed(format!(
+                        "illegal mount info with unsupported volumetype"
+                    )))


We'd better mention what volume type is for better debugging

Suggested change

&_ => {

return Err(Error::SecureMountFailed(format!(

"illegal mount info with unsupported volumetype"

)))

other => {

return Err(Error::SecureMountFailed(format!(

"illegal mount info with unsupported volumetype: {other}"

)))

fixed, thanks

Xynnn007 · 2023-09-20T10:48:25Z

confidential-data-hub/hub/src/hub.rs

+        let res = storage
+            .mount()
+            .await
+            .map_err(|e| Error::SecureMount(format!("mount failed: {e}")))?;


duplicated error information

Suggested change

.map_err(|e| Error::SecureMount(format!("mount failed: {e}")))?;

.map_err(|e| Error::SecureMount(e.to_string())))?;

fixed, thanks

confidential-data-hub/hub/src/api.rs

Xynnn007 · 2023-09-22T06:18:07Z

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

+#[derive(Error, Debug)]
+pub enum Error {
+    #[error("secure mount failed: {0}")]
+    SecureMountFailed(String),


Nice to see that thiserror crate is used!

I suggest that we have some more fine grained error types. In fact SecureMountFailed applies all the errors returned from the crate.

The storage is somehow similiar with kms. I divided the errors by the plugin type here https://github.com/confidential-containers/guest-components/blob/main/confidential-data-hub/kms/src/error.rs and I know that is not perfect. Please think about this and refer to this in your preferred way

do some changes

Xynnn007 · 2023-09-22T06:19:20Z

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

+                        .await;
+                }
+                other => {
+                    println!("skip mount info with unsupported volumetype: {other}");


shall we use log here instead of println!? s.t. log::info! or log::debug!

fixed, thanks

bpradipt

@LindaYu17 is there any description of the feature where I can read about the scope of secure mount, limitations etc. ?

LindaYu17 · 2023-11-06T07:46:01Z

@LindaYu17 is there any description of the feature where I can read about the scope of secure mount, limitations etc. ?
thank you @bpradipt for the review. I add some docs here, hope it will be helpful
8875291

Signed-off-by: Linda Yu <linda.yu@intel.com>

Xynnn007 · 2023-11-16T10:38:58Z

LGTM

Xynnn007 · 2023-11-16T10:40:37Z

@fitzthum @jepio @bpradipt Can this pr be merged now?

fitzthum

I think this is almost ready. I just added a few small comments.

fitzthum · 2023-11-16T16:16:21Z

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

+impl Storage {
+    pub async fn mount(&self) -> Result<String> {
+        for driver_option in &self.driver_options {
+            let (volumetype, metadata) =


nit: volumetype -> volume_type

fixed, thanks.

fitzthum · 2023-11-16T16:17:41Z

confidential-data-hub/hub/protos/api.proto

+}
+
+message SecureMountResponse {
+    string mountPath = 1;


nit: mountPath -> mount_path

fixed, thanks

confidential-data-hub/hub/protos/api.proto

fitzthum · 2023-11-16T16:19:30Z

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

+const OSSFS_PASSWD_FILE: &str = "/tmp/ossfs_passwd";
+const GOCRYPTFS_PASSWD_FILE: &str = "/tmp/gocryptfs_passwd";
+const OSSFS_BIN: &str = "/usr/local/bin/ossfs";
+const GOCRYPTFS_BIN: &str = "/usr/local/bin/gocryptfs";


I assume there will be changes in Kata to install these in the rootfs? Are we going to add them to every rootfs or just certain runtime classes?

add the tools to rootfs image like this:
kata-containers/kata-containers@066e042

fitzthum · 2023-11-16T16:21:16Z

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

+    Ok(res)
+}
+
+async fn get_plain(secret: &str) -> Result<String> {


Can this function name be a little more descriptive, like get_plaintext_secret?

fixed, thanks.

Signed-off-by: Linda Yu <linda.yu@intel.com>

bpradipt · 2023-11-17T04:48:50Z

@fitzthum @jepio @bpradipt Can this pr be merged now?

@Xynnn007 @LindaYu17 I don't have the expertise to review this code. Although I wanted to have a document describing the details which @LindaYu17 kindly added. So I'm good with it. Thanks.

fitzthum

LGTM. Thanks @LindaYu17

I think the test failure can be fixed by a rebase.

arronwy

Thanks @LindaYu17 LGTM!

LindaYu17 requested a review from sameo as a code owner September 7, 2023 03:16

Xynnn007 reviewed Sep 7, 2023

View reviewed changes

fitzthum reviewed Sep 12, 2023

View reviewed changes

LindaYu17 force-pushed the oss branch from a983d67 to 0f1cbae Compare September 15, 2023 06:40

LindaYu17 changed the title ~~cdh/hub: add api for secure mount~~ cdh: add secure mount feature in cdh Sep 15, 2023

LindaYu17 force-pushed the oss branch from 0f1cbae to 14532ba Compare September 15, 2023 08:28

fitzthum reviewed Sep 15, 2023

View reviewed changes

LindaYu17 force-pushed the oss branch from 5513a26 to 796d763 Compare September 20, 2023 08:00

Xynnn007 reviewed Sep 20, 2023

View reviewed changes

LindaYu17 mentioned this pull request Sep 22, 2023

CC | Port Attestation Code to Main kata-containers/kata-containers#8043

Open

LindaYu17 force-pushed the oss branch from 796d763 to 4161fbd Compare September 22, 2023 05:53

Xynnn007 reviewed Sep 22, 2023

View reviewed changes

LindaYu17 force-pushed the oss branch from 4161fbd to 880376f Compare September 27, 2023 03:03

mozillazg mentioned this pull request Oct 9, 2023

oss: support CoCo direct volume feature kubernetes-sigs/alibaba-cloud-csi-driver#882

Merged

LindaYu17 mentioned this pull request Oct 12, 2023

CC | add secure storage support in kata kata-containers/kata-containers#7965

Open

fitzthum mentioned this pull request Oct 13, 2023

CC: Trusted storage on guest kata-containers/kata-containers#8142

Closed

LindaYu17 force-pushed the oss branch from 880376f to 771c920 Compare October 25, 2023 13:33

bpradipt reviewed Nov 3, 2023

View reviewed changes

LindaYu17 added 2 commits November 16, 2023 18:04

cdh/storage: add api for secure mount

b0f910c

Signed-off-by: Linda Yu <linda.yu@intel.com>

cdh/storage: mount encrypted alibaba cloud oss

3e6bb40

Signed-off-by: Linda Yu <linda.yu@intel.com>

LindaYu17 force-pushed the oss branch from 8875291 to 985e1e0 Compare November 16, 2023 10:22

Xynnn007 approved these changes Nov 16, 2023

View reviewed changes

fitzthum reviewed Nov 16, 2023

View reviewed changes

cdh/storage: refine error message

e64697c

Signed-off-by: Linda Yu <linda.yu@intel.com>

LindaYu17 added 2 commits November 17, 2023 12:36

cdh/storage: create tmp dir for oss

e51f244

Signed-off-by: Linda Yu <linda.yu@intel.com>

cdh/storage: add docs for secure storage

3c47f3f

Signed-off-by: Linda Yu <linda.yu@intel.com>

LindaYu17 force-pushed the oss branch from 985e1e0 to 3c47f3f Compare November 17, 2023 04:39

fitzthum approved these changes Nov 20, 2023

View reviewed changes

arronwy approved these changes Nov 21, 2023

View reviewed changes

arronwy merged commit 119feca into confidential-containers:main Nov 21, 2023
4 of 5 checks passed

		.ok_or(anyhow!("split by \"=\" failed"))
		.map_err(\|e\| Error::SecureMountFailed(format!("split by \"=\" failed: {e}")))?;

	.ok_or(anyhow!("split by \"=\" failed"))
	.map_err(\|e\| Error::SecureMountFailed(format!("split by \"=\" failed: {e}")))?;
	.ok_or(Error::SecureMountFailed(format!("split by \"=\" failed)))?;

	.map_err(\|e\| Error::SecureMount(format!("mount failed: {e}")))?;
	.map_err(\|e\| Error::SecureMount(e.to_string())))?;

cdh: add secure mount feature in cdh #345

cdh: add secure mount feature in cdh #345

Conversation

LindaYu17 commented Sep 7, 2023

Xynnn007 left a 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

fitzthum Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jepio commented Sep 15, 2023

fitzthum commented Sep 15, 2023

fitzthum left a 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

Choose a reason for hiding this comment

LindaYu17 commented Sep 20, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpradipt left a comment

Choose a reason for hiding this comment

LindaYu17 commented Nov 6, 2023

Xynnn007 commented Nov 16, 2023

Xynnn007 commented Nov 16, 2023

fitzthum left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpradipt commented Nov 17, 2023

fitzthum left a comment

Choose a reason for hiding this comment

arronwy left a comment

Choose a reason for hiding this comment

fitzthum Sep 13, 2023 •

edited

Loading