CDH: fix aa_kbc_params and config reading

CDH should firstly read config specified with commandline, and then env.

aa_kbc_param should be firstly read directly from CDH's config file. If
there is no config file, try to read from env and then kernel cmdline.
If still no values found, use a default one with offline_fs_kbc.

Close #593

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

attestation-agent/attestation-agent/src/config/aa_kbc_params.rs

@@ -1,38 +1,20 @@
 use log::debug;
-use serde::Deserialize;
 use std::env;
-use std::path::Path;
-use std::sync::OnceLock;
 use thiserror::Error;
 
-const PEER_POD_CONFIG_PATH: &str = "/run/peerpod/daemon.json";
-static KATA_AGENT_CONFIG_PATH: OnceLock<String> = OnceLock::new();
-
 #[derive(Error, Debug)]
 pub enum ParamError {
     #[error("illegal aa_kbc_params format: {0}")]
     IllegalFormat(String),
-    #[error("unable to read `aa_kbc_params` entry from kata-agent config file")]
-    AgentConfigParsing(#[from] toml::de::Error),
     #[error("io error")]
     Io(#[from] std::io::Error),
     #[error("no `agent.aa_kbc_params` provided in kernel commandline")]
     MissingInCmdline,
 }
 
 pub struct AaKbcParams {
-    kbc: String,
-    uri: String,
-}
-
-impl AaKbcParams {
-    pub fn kbc(&self) -> &str {
-        &self.kbc
-    }
-
-    pub fn uri(&self) -> &str {
-        &self.uri
-    }
+    pub kbc: String,
+    pub uri: String,
 }
 
 impl TryFrom<String> for AaKbcParams {
@@ -61,11 +43,6 @@ pub fn get_value() -> Result<String, ParamError> {
         return Ok(params);
     }
 
-    // second check whether we are in a peer pod
-    if Path::new(PEER_POD_CONFIG_PATH).exists() {
-        return from_config_file();
-    }
-
     // finally use the kernel cmdline
     from_cmdline()
 }
@@ -75,28 +52,6 @@ pub fn get_params() -> Result<AaKbcParams, ParamError> {
     value.try_into()
 }
 
-// We only care about the aa_kbc_params value at the moment
-#[derive(Debug, Deserialize)]
-struct AgentConfig {
-    aa_kbc_params: String,
-}
-
-fn from_config_file() -> Result<String, ParamError> {
-    debug!("get aa_kbc_params from file");
-
-    // check env for KATA_AGENT_CONFIG_PATH, fall back to default path
-    let path: &String = KATA_AGENT_CONFIG_PATH.get_or_init(|| {
-        env::var("KATA_AGENT_CONFIG_PATH").unwrap_or_else(|_| "/etc/agent-config.toml".into())
-    });
-
-    debug!("reading agent config from {}", path);
-    let agent_config_str = std::fs::read_to_string(path)?;
-
-    let agent_config: AgentConfig = toml::from_str(&agent_config_str)?;
-
-    Ok(agent_config.aa_kbc_params)
-}
-
 fn from_cmdline() -> Result<String, ParamError> {
     debug!("get aa_kbc_params from kernel cmdline");
     let cmdline = std::fs::read_to_string("/proc/cmdline")?;

attestation-agent/attestation-agent/src/config/coco_as.rs

@@ -17,7 +17,7 @@ impl Default for CoCoASConfig {
     fn default() -> Self {
         let aa_kbc_params = aa_kbc_params::get_params().expect("failed to get aa_kbc_params");
         Self {
-            url: aa_kbc_params.uri().into(),
+            url: aa_kbc_params.uri.clone(),
         }
     }
 }

attestation-agent/attestation-agent/src/config/kbs.rs

@@ -20,7 +20,7 @@ impl Default for KbsConfig {
     fn default() -> Self {
         let aa_kbc_params = aa_kbc_params::get_params().expect("failed to get aa_kbc_params");
         Self {
-            url: aa_kbc_params.uri().into(),
+            url: aa_kbc_params.uri.clone(),
             cert: None,
         }
     }

confidential-data-hub/hub/src/bin/config/mod.rs

@@ -3,13 +3,12 @@
 // SPDX-License-Identifier: Apache-2.0
 //
 
-use std::env;
+use std::{env, fs, path::Path};
 
 use anyhow::*;
 use config::{Config, File};
-use log::{debug, warn};
+use log::{debug, info};
 use serde::Deserialize;
-use tokio::fs;
 
 cfg_if::cfg_if! {
     if #[cfg(feature = "ttrpc")] {
@@ -19,7 +18,7 @@ cfg_if::cfg_if! {
     }
 }
 
-#[derive(Deserialize, Debug)]
+#[derive(Deserialize, Debug, PartialEq)]
 pub struct KbsConfig {
     pub name: String,
 
@@ -30,21 +29,32 @@ pub struct KbsConfig {
 
 impl Default for KbsConfig {
     fn default() -> Self {
-        Self {
-            name: "offline_fs_kbc".into(),
-            url: "null".into(),
-            kbs_cert: None,
+        debug!("Try to get kbc and url from env and kernel commandline.");
+        match attestation_agent::config::aa_kbc_params::get_params() {
+            std::result::Result::Ok(aa_kbc_params) => KbsConfig {
+                name: aa_kbc_params.kbc,
+                url: aa_kbc_params.uri,
+                kbs_cert: None,
+            },
+            Err(_) => {
+                debug!("Failed to get aa_kbc_params from env or kernel cmdline. Use offline_fs_kbc by default.");
+                KbsConfig {
+                    name: "offline_fs_kbc".into(),
+                    url: "".into(),
+                    kbs_cert: None,
+                }
+            }
         }
     }
 }
 
-#[derive(Deserialize, Debug)]
+#[derive(Deserialize, Debug, PartialEq)]
 pub struct Credential {
     pub resource_uri: String,
     pub path: String,
 }
 
-#[derive(Deserialize, Debug)]
+#[derive(Deserialize, Debug, PartialEq)]
 pub struct CdhConfig {
     pub kbc: KbsConfig,
 
@@ -65,13 +75,31 @@ impl Default for CdhConfig {
 }
 
 impl CdhConfig {
-    pub async fn init(config_path: &str) -> Result<Self> {
-        let mut config = Self::from_file(config_path).unwrap_or_else(|e| {
-            warn!("read config file {config_path} failed {e:?}, use a default config where `aa_kbc_params` = offline_fs_kbc::null.");
-            Self::default()
+    pub fn new(config_path: Option<String>) -> Result<Self> {
+        let config_path = config_path.or_else(|| {
+            if let std::result::Result::Ok(env_path) = env::var("CDH_CONFIG_PATH") {
+                debug!("Read CDH's config path from env: {env_path}");
+                return Some(env_path);
+            }
+            None
         });
 
-        config.update_from_kernel_cmdline().await?;
+        let mut config = match config_path {
+            Some(path) => {
+                info!("Use configuration file {path}");
+                if !Path::new(&path).exists() {
+                    bail!("Config file {path} not found.")
+                }
+
+                Self::from_file(&path)?
+            }
+            None => {
+                info!("No config path specified, use a default config.");
+                Self::default()
+            }
+        };
+
+        config.extend_credentials_from_kernel_cmdline()?;
         Ok(config)
     }
 
@@ -102,10 +130,8 @@ impl CdhConfig {
     /// path.
     ///
     /// TODO: delete this way after initdata mechanism could cover CDH's launch config.
-    async fn update_from_kernel_cmdline(&mut self) -> Result<()> {
-        let cmdline = fs::read_to_string("/proc/cmdline")
-            .await
-            .context("read kernel cmdline failed")?;
+    fn extend_credentials_from_kernel_cmdline(&mut self) -> Result<()> {
+        let cmdline = fs::read_to_string("/proc/cmdline").context("read kernel cmdline failed")?;
         let kbs_resources = cmdline
             .split_ascii_whitespace()
             .find(|para| para.starts_with("cdh.kbs_resources="))
@@ -127,15 +153,13 @@ impl CdhConfig {
 
 impl CdhConfig {
     pub fn set_configuration_envs(&self) {
-        if attestation_agent::config::aa_kbc_params::get_value().is_err() {
-            debug!("No aa_kbc_params provided in kernel cmdline, env and peerpod config.");
-            // KBS configurations
+        if env::var("AA_KBC_PARAMS").is_err() {
             env::set_var(
                 "AA_KBC_PARAMS",
                 format!("{}::{}", self.kbc.name, self.kbc.url),
             );
         }
-
+        // KBS configurations
         if let Some(kbs_cert) = &self.kbc.kbs_cert {
             env::set_var("KBS_CERT", kbs_cert);
         }
@@ -144,11 +168,12 @@ impl CdhConfig {
 
 #[cfg(test)]
 mod tests {
-    use std::io::Write;
+    use std::{env, io::Write};
 
+    use anyhow::anyhow;
     use rstest::rstest;
 
-    use crate::CdhConfig;
+    use crate::{config::DEFAULT_CDH_SOCKET_ADDR, CdhConfig, KbsConfig};
 
     #[rstest]
     #[case(
@@ -200,4 +225,35 @@ path = "/run/confidential-containers/cdh/kms-credential/aliyun/config.toml"
         let res = CdhConfig::from_file(file.path().to_str().unwrap());
         assert_eq!(res.is_ok(), successful, "{res:?}");
     }
+
+    #[test]
+    fn test_config_path() {
+        // --config takes precedence,
+        // then env.CDH_CONFIG_PATH
+
+        let config = CdhConfig::new(None).expect("Must be successful");
+        let expected = CdhConfig {
+            kbc: KbsConfig {
+                name: "offline_fs_kbc".into(),
+                url: "".into(),
+                kbs_cert: None,
+            },
+            credentials: Vec::new(),
+            socket: DEFAULT_CDH_SOCKET_ADDR.into(),
+        };
+        assert_eq!(config, expected);
+
+        let config = CdhConfig::new(Some("/thing".into())).unwrap_err();
+        let expected = anyhow!("Config file /thing not found.");
+        assert_eq!(format!("{config}"), format!("{expected}"));
+
+        env::set_var("CDH_CONFIG_PATH", "/byenv");
+        let config = CdhConfig::new(None).unwrap_err();
+        let expected = anyhow!("Config file /byenv not found.");
+        assert_eq!(format!("{config}"), format!("{expected}"));
+
+        let config = CdhConfig::new(Some("/thing".into())).unwrap_err();
+        let expected = anyhow!("Config file /thing not found.");
+        assert_eq!(format!("{config}"), format!("{expected}"));
+    }
 }

confidential-data-hub/hub/src/bin/grpc-cdh.rs

@@ -17,8 +17,6 @@ mod message;
 
 use config::*;
 
-const DEFAULT_CONFIG_PATH: &str = "/etc/confidential-data-hub.conf";
-
 const VERSION: &str = include_str!(concat!(env!("OUT_DIR"), "/version"));
 
 #[derive(Debug, Parser)]
@@ -31,23 +29,12 @@ struct Cli {
     config: Option<String>,
 }
 
-fn get_config_path(cli: Cli) -> String {
-    cli.config.unwrap_or_else(|| {
-        if let Ok(env_path) = env::var("CDH_CONFIG_PATH") {
-            return env_path;
-        }
-        DEFAULT_CONFIG_PATH.into()
-    })
-}
-
 #[tokio::main]
 async fn main() -> Result<()> {
     env_logger::init_from_env(env_logger::Env::new().default_filter_or("info"));
     let cli = Cli::parse();
-    let config_path = get_config_path(cli);
-    info!("Use configuration file {}", config_path);
 
-    let config = CdhConfig::init(&config_path).await?;
+    let config = CdhConfig::new(cli.config)?;
     config.set_configuration_envs();
 
     let cdh_socket = config.socket.parse::<SocketAddr>()?;

confidential-data-hub/hub/src/bin/ttrpc-cdh.rs

@@ -28,8 +28,6 @@ mod ttrpc_server;
 
 use config::*;
 
-const DEFAULT_CONFIG_PATH: &str = "/etc/confidential-data-hub.conf";
-
 const UNIX_SOCKET_PREFIX: &str = "unix://";
 
 const VERSION: &str = include_str!(concat!(env!("OUT_DIR"), "/version"));
@@ -52,23 +50,12 @@ macro_rules! ttrpc_service {
     }};
 }
 
-fn get_config_path(cli: Cli) -> String {
-    cli.config.unwrap_or_else(|| {
-        if let Ok(env_path) = env::var("CDH_CONFIG_PATH") {
-            return env_path;
-        }
-        DEFAULT_CONFIG_PATH.into()
-    })
-}
-
 #[tokio::main]
 async fn main() -> Result<()> {
     env_logger::init_from_env(env_logger::Env::new().default_filter_or("info"));
     let cli = Cli::parse();
-    let config_path = get_config_path(cli);
-    info!("Use configuration file {}", config_path);
 
-    let config = CdhConfig::init(&config_path).await?;
+    let config = CdhConfig::new(cli.config)?;
     config.set_configuration_envs();
 
     let unix_socket_path = config
@@ -136,32 +123,3 @@ async fn create_socket_parent_directory(unix_socket_file: &str) -> Result<()> {
     fs::create_dir_all(parent_directory).await?;
     Ok(())
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn test_get_config_path() {
-        // --config takes precedence,
-        // then env.CDH_CONFIG_PATH,
-        // then DEFAULT_CONFIG_PATH.
-
-        let cli = Cli { config: None };
-        assert_eq!(get_config_path(cli), DEFAULT_CONFIG_PATH);
-
-        let cli = Cli {
-            config: Some("/thing".into()),
-        };
-        assert_eq!(get_config_path(cli), "/thing");
-
-        env::set_var("CDH_CONFIG_PATH", "/byenv");
-        let cli = Cli { config: None };
-        assert_eq!(get_config_path(cli), "/byenv");
-
-        let cli = Cli {
-            config: Some("/thing".into()),
-        };
-        assert_eq!(get_config_path(cli), "/thing");
-    }
-}