CDH: add get_secret support for Aliyun KMS #423

1570005763 · 2023-12-29T02:45:27Z

Apply get_secret() function for Getter trait.

And two methods of accessing Aliyun KMS is offered. One is through ClientKey, the same as encrypt/decrypt function, the other is through EcsRamRole, which can only be used on Aliyun ECS. More information is updated to the doc file.

Again it is hard to test code on CI. I can only test them on my machine, this is a screenshot of the test results.

1570005763 · 2023-12-29T05:09:41Z

The basic build check failure is irrelevant to this PR. I'll open another PR to fix it.

1570005763 · 2023-12-29T07:34:17Z

The basic build check failure is fixed in #424.

Xynnn007

Thanks @1570005763 for this patch. Here are some comments

Xynnn007 · 2024-01-02T07:20:16Z

confidential-data-hub/kms/src/plugins/aliyun/client.rs

@@ -18,12 +18,12 @@ use serde_json::Value;
 use sha2::{Digest, Sha256};
 use tokio::fs;

-use crate::plugins::aliyun::client::dkms_api::{DecryptRequest, EncryptRequest};
+// use crate::plugins::aliyun::client::dkms_api;


Is the note // here for debug?

Xynnn007 · 2024-01-02T07:23:08Z

confidential-data-hub/kms/src/plugins/aliyun/client.rs

+            "client_key_id": "KAAP.2bc431c0-416f-4ccb-8638-e71245fd60c1",
+            "kms_instance_id": "kst-bjj65794c36xm0v9aeujs",
+        });
+        // init encrypter at user side


encrypter -> getter

Xynnn007 · 2024-01-02T07:24:44Z

confidential-data-hub/kms/src/plugins/aliyun/client.rs

+    #[ignore]
+    #[tokio::test]
+    async fn get_secret_with_client_key() {
+        let secret_name = "test_secret";


Could you use rstest like https://github.com/confidential-containers/guest-components/blob/main/confidential-data-hub/kms/src/plugins/aliyun/client.rs#L320-L326 for test cases?

Sure, rtest looks better.

Xynnn007 · 2024-01-02T08:36:15Z

confidential-data-hub/kms/Cargo.toml

@@ -21,17 +21,19 @@ log.workspace = true
 openssl = { workspace = true, optional = true }
 p12 = { version = "0.6.3", optional = true }
 prost = { workspace = true, optional = true }
+rand.workspace = true
+reqwest = { version = "0.11", optional = true }


as we already imported request in the workspace's Cargo.toml, here can be
reqwest = { workspace = true, optional = true }

Xynnn007 · 2024-01-02T08:36:51Z

confidential-data-hub/kms/src/plugins/aliyun/config.rs

+// use base64::{engine::general_purpose::STANDARD, Engine};
+// use log::debug;
+// use openssl::{
+//     pkey::{PKey, Private},
+//     sign::Signer,
+// };
+// use p12::{CertBag, ContentInfo, MacData, SafeBagKind, PFX};
+// use serde::Deserialize;
+// use yasna::ASN1Result;


Are these left by debug?

Xynnn007 · 2024-01-02T08:56:42Z

confidential-data-hub/kms/src/plugins/aliyun/credential.rs

+                    return Err(anyhow!(
+                        "Request session_credential failed with status: {}",
+                        response.status()
+                    ));


Suggested change

return Err(anyhow!(

"Request session_credential failed with status: {}",

response.status()

));

bail!(

"Request session_credential failed with status: {}",

response.status()

);

Xynnn007 · 2024-01-02T08:58:31Z

confidential-data-hub/kms/src/plugins/aliyun/credential.rs

+                let session_ak = body_json["AccessKeyId"].as_str().unwrap().to_string();
+                let session_sk = body_json["AccessKeySecret"].as_str().unwrap().to_string();
+                let token = body_json["SecurityToken"].as_str().unwrap().to_string();


Can you please avoid using unwrap() but an error handling like ok_or to prevent the process from panic?

Also, if directly use ["SecurityToken"] to index the field in a JSON, it might also panic if no field named SecurityToken exists. Please try to use .get("..") instead.

Helpful suggestion! Has been modified.

Xynnn007 · 2024-01-02T09:00:53Z

confidential-data-hub/kms/src/plugins/aliyun/credential.rs

+        }
+    }
+
+    pub(crate) fn urlencode_openapi(&self, s: &String) -> String {


Suggested change

pub(crate) fn urlencode_openapi(&self, s: &String) -> String {

pub(crate) fn urlencode_openapi(&self, s: &str) -> String {

When use a reference to a String, &str is preferred.

BTW, is this function actually used?

Yeah, it's used in both build_params() and do_request() functions.

Xynnn007 · 2024-01-08T08:52:48Z

confidential-data-hub/kms/src/plugins/aliyun/credential.rs

            client_key_id: ck.key_id.clone(),
            private_key,
        };

        Ok(credential)
    }

-    pub(crate) fn generate_bear_auth(&self, str_to_sign: &str) -> Result<String> {
+    pub(crate) fn sign(&self, str_to_sign: &str) -> Result<String> {


This function actually generates bear auth string. Do we need to change the name?

I was originally hoping to unify the function names for obtaining signatures in clientKey access and ecsRamRole access. It seems that the sign() might be confusing. What do you think about changing it to get_signature()?

1570005763 · 2024-01-18T06:46:26Z

I have reorganized the code structure to make it easier to understand. Please take another look. @Xynnn007

Xynnn007

Some more comments

Xynnn007 · 2024-01-16T09:06:45Z

confidential-data-hub/kms/src/plugins/aliyun/client.rs

+                })?;
+
+            let ecs_ram_role_name =
+                if let Some(ecs_ram_role_name) = ecs_ram_role_json["ecs_ram_role_name"].as_str() {


Please check the existance of ecs_ram_role_json["ecs_ram_role_name"] before using that

Xynnn007 · 2024-01-16T09:10:34Z

confidential-data-hub/kms/src/plugins/aliyun/client.rs

+                    .join("&");
+                let urlencoded_canonicalized_params: String =
+                    credential.urlencode_openapi(&canonicalized_params);
+                let string_to_sign = format!("POST&%2F&{}", urlencoded_canonicalized_params,);


Suggested change

let string_to_sign = format!("POST&%2F&{}", urlencoded_canonicalized_params,);

let string_to_sign = format!("POST&%2F&{}", urlencoded_canonicalized_params);

.gitignore

Xynnn007 · 2024-01-18T06:56:20Z

confidential-data-hub/kms/Cargo.toml

 thiserror.workspace = true
 tokio = { workspace = true, features = ["fs"] }
 toml.workspace = true
 tonic = { workspace = true, optional = true }
+url.workspace = true


We might need to make url as optional to aliyun

Xynnn007 · 2024-01-18T06:56:37Z

confidential-data-hub/kms/Cargo.toml

@@ -21,17 +21,19 @@ log.workspace = true
 openssl = { workspace = true, optional = true }
 p12 = { version = "0.6.3", optional = true }
 prost = { workspace = true, optional = true }
+rand.workspace = true


We might need to make this optional to aliyun

Xynnn007 · 2024-01-18T07:00:24Z

confidential-data-hub/kms/src/plugins/aliyun/client/client_key_client/config.rs

+
+// implement ConfigClientKey related function
+impl ConfigClientKey {
+    pub(crate) fn new(kms_instance_id: &str, endpoint: &str) -> Result<Self> {


If we do not return any Error, usually we do not use Result<Self> but directly Self

Result<_> removed.

Xynnn007 · 2024-01-18T07:01:56Z

confidential-data-hub/kms/src/plugins/aliyun/client/client_key_client/config.rs

@@ -0,0 +1,29 @@
+// Copyright (c) 2023 Alibaba Cloud


2023 -> 2024

Xynnn007 · 2024-01-18T07:13:31Z

confidential-data-hub/kms/src/plugins/aliyun/client/ecs_ram_role_client/config.rs

+
+// implement ConfigEcsRamRole related function
+impl ConfigEcsRamRole {
+    pub(crate) fn new(region_id: &str, endpoint: &str, vpc: &str) -> Result<Self> {


The same issue upon Result<_> usage

Result<_> removed.

Xynnn007 · 2024-01-18T07:16:10Z

confidential-data-hub/kms/src/plugins/aliyun/client/ecs_ram_role_client/mod.rs

+    /// Export the [`ProviderSettings`] of the current client. This function is to be used
+    /// in the encryptor side. The [`ProviderSettings`] will be used to initial a client
+    /// in the decryptor side.
+    pub fn export_provider_settings(&self) -> Result<ProviderSettings> {


The same issue upon Result<_> usage.

Result<_> removed.

Xynnn007 · 2024-01-18T07:22:09Z

confidential-data-hub/kms/src/plugins/aliyun/client/mod.rs

+        let inner_client = if provider_settings.contains_key("client_type")
+            && provider_settings["client_type"] == "client_key"
+        {
+            Client::ClientKey {
+                client_key_client: ClientKeyClient::from_provider_settings(provider_settings)
+                    .await
+                    .map_err(|e| {
+                        Error::AliyunKmsError(format!(
+                            "build ClientKeyClient with `from_provider_settings()` failed: {e}"
+                        ))
+                    })?,
+            }
+        } else if provider_settings.contains_key("client_type")
+            && provider_settings["client_type"] == "ecs_ram_role"
+        {
+            Client::EcsRamRole {
+                ecs_ram_role_client: EcsRamRoleClient::from_provider_settings(provider_settings)
+                    .await
+                    .map_err(|e| {
+                        Error::AliyunKmsError(format!(
+                            "build EcsRamRoleClient with `from_provider_settings()` failed: {e}"
+                        ))
+                    })?,
+            }
+        } else {
+            return Err(Error::AliyunKmsError(
+                "client type invalid or not exist.".to_string(),
+            ));
+        };


Could you do some refactoring like the following to make the logic clearer

Suggested change

let inner_client = if provider_settings.contains_key("client_type")

&& provider_settings["client_type"] == "client_key"

{

Client::ClientKey {

client_key_client: ClientKeyClient::from_provider_settings(provider_settings)

.await

.map_err(|e| {

Error::AliyunKmsError(format!(

"build ClientKeyClient with `from_provider_settings()` failed: {e}"

))

})?,

}

} else if provider_settings.contains_key("client_type")

&& provider_settings["client_type"] == "ecs_ram_role"

{

Client::EcsRamRole {

ecs_ram_role_client: EcsRamRoleClient::from_provider_settings(provider_settings)

.await

.map_err(|e| {

Error::AliyunKmsError(format!(

"build EcsRamRoleClient with `from_provider_settings()` failed: {e}"

))

})?,

}

} else {

return Err(Error::AliyunKmsError(

"client type invalid or not exist.".to_string(),

));

};

let Some(client_type) = provider_settings.get("client_type") else {

return Err(Error::AliyunKmsError(

"client type invalid or not exist.".to_string(),

));

};

let inner_client = match client_type {

"client_key" => ...

"ecs_ram_role" => ...

}

Great! It does look clearer.

Xynnn007

LGTM.

Xynnn007 · 2024-01-19T03:49:42Z

confidential-data-hub/kms/src/plugins/aliyun/client/ecs_ram_role_client/mod.rs

+    /// [`ALIYUN_IN_GUEST_DEFAULT_KEY_PATH`] which is the by default path where the credential
+    /// to access kms is saved.
+    pub async fn from_provider_settings(_provider_settings: &ProviderSettings) -> Result<Self> {
+        let ecs_ram_role_path = format!("{ALIYUN_IN_GUEST_DEFAULT_KEY_PATH}/ecsRamRole.json");


We can use concatcp! to declare this as a const.

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

1570005763 requested a review from sameo as a code owner December 29, 2023 02:45

1570005763 force-pushed the feat-kms-aliyun-sm branch 4 times, most recently from 2a84202 to 90a64d4 Compare December 29, 2023 04:11

1570005763 force-pushed the feat-kms-aliyun-sm branch from 90a64d4 to bf96b6e Compare January 2, 2024 02:38

Xynnn007 reviewed Jan 2, 2024

View reviewed changes

1570005763 force-pushed the feat-kms-aliyun-sm branch from bf96b6e to 83b0b9a Compare January 3, 2024 08:27

Xynnn007 requested changes Jan 8, 2024

View reviewed changes

1570005763 force-pushed the feat-kms-aliyun-sm branch from 9ce601d to 921ed51 Compare January 18, 2024 06:42

Xynnn007 reviewed Jan 18, 2024

View reviewed changes

1570005763 force-pushed the feat-kms-aliyun-sm branch 2 times, most recently from eb4df0d to 2237a86 Compare January 19, 2024 03:04

Xynnn007 approved these changes Jan 19, 2024

View reviewed changes

1570005763 added 3 commits January 19, 2024 12:04

cdh: add 'get_secret' support for aliyun kms

9309eaa

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

cdh: 'get_secret' support EcsRamRole access for aliyun kms

39e0dfd

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

gitignore: add ignore directory

99b1fbf

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

1570005763 force-pushed the feat-kms-aliyun-sm branch from 2237a86 to 99b1fbf Compare January 19, 2024 04:05

Xynnn007 merged commit 178a689 into confidential-containers:main Jan 19, 2024
12 checks passed

	pub(crate) fn urlencode_openapi(&self, s: &String) -> String {
	pub(crate) fn urlencode_openapi(&self, s: &str) -> String {

	let string_to_sign = format!("POST&%2F&{}", urlencoded_canonicalized_params,);
	let string_to_sign = format!("POST&%2F&{}", urlencoded_canonicalized_params);

CDH: add get_secret support for Aliyun KMS #423

CDH: add get_secret support for Aliyun KMS #423

Conversation

1570005763 commented Dec 29, 2023 • edited Loading

1570005763 commented Dec 29, 2023 • edited Loading

1570005763 commented Dec 29, 2023

Xynnn007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1570005763 Jan 3, 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

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

1570005763 commented Jan 18, 2024

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

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

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

Choose a reason for hiding this comment

1570005763 commented Dec 29, 2023 •

edited

Loading

1570005763 commented Dec 29, 2023 •

edited

Loading

1570005763 Jan 3, 2024 •

edited

Loading

Xynnn007 left a comment •

edited

Loading