Keep list_providers order; add cfg tests

ionut-arm · ionut-arm · commit 2fee72fc6487 · 2020-07-07T19:52:22.000+01:00
This commit changes the functionality of the service builder to maintain
the provider list found in the configuration file when responding to
`list_providers` operations.
A new E2E test section is also added for verifying configuration files.

Signed-off-by: Ionut Mihalcea &lt;ionut.mihalcea@arm.com&gt;
diff --git a/ci.sh b/ci.sh
@@ -12,11 +12,12 @@ cleanup () {
     if [ -n "$PARSEC_PID" ]; then kill $PARSEC_PID || true ; fi
     # Stop tpm_server if running
     if [ -n "$TPM_SRV_PID" ]; then kill $TPM_SRV_PID || true; fi
-    # Remove the slot_number line added by find_slot_number.sh
-    sed -i '/^slot_number =.*/d' $CONFIG_PATH
+    # Remove the slot_number line added earlier
+    find e2e_tests -name "*toml" -not -name "Cargo.toml" -exec sed -i 's/^slot_number =.*/# slot_number/' {} \;
     # Remove fake mapping and temp files
-    if [ -d "mappings" ]; then rm -rf -- "mappings"; fi
-    if [ -f "NVChip" ]; then rm "NVChip" ; fi
+    rm -rf "mappings"
+    rm -f "NVChip" 
+    rm -f "e2e_tests/provider_cfg/tmp_config.toml"
 
     if [ -z "$NO_CARGO_CLEAN" ]; then cargo clean; fi
 }
@@ -50,6 +51,7 @@ error_msg () {
 NO_CARGO_CLEAN=
 NO_STRESS_TEST=
 PROVIDER_NAME=
+CONFIG_PATH=$(pwd)/e2e_tests/provider_cfg/tmp_config.toml
 while [ "$#" -gt 0 ]; do
     case "$1" in
         --no-cargo-clean )
@@ -63,7 +65,7 @@ while [ "$#" -gt 0 ]; do
                 error_msg "Only one provider name must be given"
             fi
             PROVIDER_NAME=$1
-            CONFIG_PATH="e2e_tests/provider_cfg/$1/config.toml"
+            cp $(pwd)/e2e_tests/provider_cfg/$1/config.toml $CONFIG_PATH
             if [ "$PROVIDER_NAME" = "all" ]; then
                 FEATURES="--features=all-providers"
             else
@@ -94,8 +96,13 @@ if [ "$PROVIDER_NAME" = "tpm" ] || [ "$PROVIDER_NAME" = "all" ]; then
 fi
 
 if [ "$PROVIDER_NAME" = "pkcs11" ] || [ "$PROVIDER_NAME" = "all" ]; then
-    # Find and append the slot number at the end of the configuration file.
-    e2e_tests/provider_cfg/pkcs11/find_slot_number.sh $CONFIG_PATH
+    pushd e2e_tests
+    # This command suppose that the slot created by the container will be the first one that appears
+    # when printing all the available slots.
+    SLOT_NUMBER=`softhsm2-util --show-slots | head -n2 | tail -n1 | cut -d " " -f 2`
+    # Find all TOML files in the directory (except Cargo.toml) and replace the commented slot number with the valid one
+    find . -name "*toml" -not -name "Cargo.toml" -exec sed -i "s/^# slot_number.*$/slot_number = $SLOT_NUMBER/" {} \;
+    popd
 fi
 
 echo "Build test"
@@ -128,6 +135,7 @@ pgrep -f target/debug/parsec >/dev/null
 if [ "$PROVIDER_NAME" = "all" ]; then
     echo "Execute all-providers tests"
     RUST_BACKTRACE=1 cargo test --manifest-path ./e2e_tests/Cargo.toml all_providers
+    RUST_BACKTRACE=1 cargo test --manifest-path ./e2e_tests/Cargo.toml config
 else
     # Per provider tests
     echo "Execute normal tests"
diff --git a/e2e_tests/.gitignore b/e2e_tests/.gitignore
@@ -1,2 +1,3 @@
 target
-Cargo.lock
+Cargo.lock
+provider_cfg/tmp_config.toml
diff --git a/e2e_tests/Cargo.toml b/e2e_tests/Cargo.toml
@@ -18,7 +18,7 @@ picky-asn1-der = "0.2.2"
 picky-asn1 = "0.2.1"
 serde = { version = "1.0", features = ["derive"] }
 sha2 = "0.8.1"
-parsec-client = { git = "https://github.com/parallaxsecond/parsec-client-rust", features = ["testing"] }
+parsec-client = { version = "0.6.0", features = ["testing"] }
 log = "0.4.8"
 rand = "0.7.3"
 
diff --git a/e2e_tests/provider_cfg/all/config.toml b/e2e_tests/provider_cfg/all/config.toml
@@ -26,6 +26,5 @@ provider_type = "Pkcs11"
 key_info_manager = "on-disk-manager"
 library_path = "/usr/local/lib/softhsm/libsofthsm2.so"
 user_pin = "123456"
-# The slot_number mandatory field is going to be added by the find_slot_number.sh script
-# to the last line of this file in the form:
-# slot_number = 123456
+# The slot_number mandatory field is going to replace the following line with a valid number
+# slot_number
diff --git a/e2e_tests/provider_cfg/pkcs11/config.toml b/e2e_tests/provider_cfg/pkcs11/config.toml
@@ -18,6 +18,5 @@ provider_type = "Pkcs11"
 key_info_manager = "on-disk-manager"
 library_path = "/usr/local/lib/softhsm/libsofthsm2.so"
 user_pin = "123456"
-# The slot_number mandatory field is going to be added by the find_slot_number.sh script
-# to the last line of this file in the form:
-# slot_number = 123456
+# The slot_number mandatory field is going to replace the following line with a valid number
+# slot_number
diff --git a/e2e_tests/provider_cfg/pkcs11/find_slot_number.sh b/e2e_tests/provider_cfg/pkcs11/find_slot_number.sh
diff --git a/e2e_tests/src/lib.rs b/e2e_tests/src/lib.rs
@@ -1,5 +1,6 @@
 // Copyright 2020 Contributors to the Parsec project.
 // SPDX-License-Identifier: Apache-2.0
+#![allow(clippy::multiple_crate_versions)]
 pub mod raw_request;
 pub mod stress;
 
diff --git a/e2e_tests/tests/config/mod.rs b/e2e_tests/tests/config/mod.rs
@@ -0,0 +1,76 @@
+// Copyright 2020 Contributors to the Parsec project.
+// SPDX-License-Identifier: Apache-2.0
+use e2e_tests::TestClient;
+use log::{error, info};
+use std::env;
+use std::fs;
+use std::path::PathBuf;
+use std::process::Command;
+use std::thread;
+use std::time::Duration;
+use uuid::Uuid;
+
+const CONFIG_TOMLS_FOLDER: &str = "tests/config/tomls";
+const SERVICE_CONFIG_PATH: &str = "provider_cfg/tmp_config.toml";
+
+fn set_config(filename: &str) {
+    info!("Changing service configuration file to {}", filename);
+    let config_path = PathBuf::from(SERVICE_CONFIG_PATH);
+    let mut new_config = env::current_dir() // this is the root of the crate for tests
+        .unwrap();
+    new_config.push(CONFIG_TOMLS_FOLDER);
+    new_config.push(filename);
+    if !new_config.exists() {
+        error!("Configuration file {} does not exist", filename);
+        panic!();
+    }
+
+    let _ = fs::copy(new_config, config_path).unwrap();
+}
+
+fn reload_service() {
+    info!("Reloading Parsec service");
+
+    let _ = Command::new("pkill")
+        .arg("-SIGHUP")
+        .arg("parsec")
+        .output()
+        .expect("Reloading service failed");
+
+    // wait for the service to restart
+    thread::sleep(Duration::from_secs(2));
+}
+
+#[test]
+fn list_providers() {
+    set_config("list_providers_1.toml");
+    reload_service();
+
+    let mut client = TestClient::new();
+    let providers = client.list_providers().unwrap();
+    let uuids: Vec<Uuid> = providers.iter().map(|p| p.uuid).collect();
+    assert_eq!(
+        uuids,
+        vec![
+            Uuid::parse_str("1c1139dc-ad7c-47dc-ad6b-db6fdb466552").unwrap(), // Mbed crypto provider
+            Uuid::parse_str("1e4954a4-ff21-46d3-ab0c-661eeb667e1d").unwrap(), // Tpm provider
+            Uuid::parse_str("30e39502-eba6-4d60-a4af-c518b7f5e38f").unwrap(), // Pkcs11 provider
+            Uuid::parse_str("47049873-2a43-4845-9d72-831eab668784").unwrap(), // Core provider
+        ]
+    );
+
+    set_config("list_providers_2.toml");
+    reload_service();
+
+    let providers = client.list_providers().unwrap();
+    let uuids: Vec<Uuid> = providers.iter().map(|p| p.uuid).collect();
+    assert_eq!(
+        uuids,
+        vec![
+            Uuid::parse_str("30e39502-eba6-4d60-a4af-c518b7f5e38f").unwrap(), // Pkcs11 provider
+            Uuid::parse_str("1c1139dc-ad7c-47dc-ad6b-db6fdb466552").unwrap(), // Mbed crypto provider
+            Uuid::parse_str("1e4954a4-ff21-46d3-ab0c-661eeb667e1d").unwrap(), // Tpm provider
+            Uuid::parse_str("47049873-2a43-4845-9d72-831eab668784").unwrap(), // Core provider
+        ]
+    );
+}
diff --git a/e2e_tests/tests/config/tomls/list_providers_1.toml b/e2e_tests/tests/config/tomls/list_providers_1.toml
@@ -0,0 +1,30 @@
+[core_settings]
+# The CI already timestamps the logs
+log_timestamp = false
+log_error_details = true
+
+[listener]
+listener_type = "DomainSocket"
+timeout = 200 # in milliseconds
+
+[[key_manager]]
+name = "on-disk-manager"
+manager_type = "OnDisk"
+
+[[provider]]
+provider_type = "MbedCrypto"
+key_info_manager = "on-disk-manager"
+
+[[provider]]
+provider_type = "Tpm"
+key_info_manager = "on-disk-manager"
+tcti = "mssim"
+owner_hierarchy_auth = "tpm_pass"
+
+[[provider]]
+provider_type = "Pkcs11"
+key_info_manager = "on-disk-manager"
+library_path = "/usr/local/lib/softhsm/libsofthsm2.so"
+user_pin = "123456"
+# The slot_number mandatory field is going to replace the following line with a valid number
+# slot_number
diff --git a/e2e_tests/tests/config/tomls/list_providers_2.toml b/e2e_tests/tests/config/tomls/list_providers_2.toml
@@ -0,0 +1,30 @@
+[core_settings]
+# The CI already timestamps the logs
+log_timestamp = false
+log_error_details = true
+
+[listener]
+listener_type = "DomainSocket"
+timeout = 200 # in milliseconds
+
+[[key_manager]]
+name = "on-disk-manager"
+manager_type = "OnDisk"
+
+[[provider]]
+provider_type = "Pkcs11"
+key_info_manager = "on-disk-manager"
+library_path = "/usr/local/lib/softhsm/libsofthsm2.so"
+user_pin = "123456"
+# The slot_number mandatory field is going to replace the following line with a valid number
+# slot_number
+
+[[provider]]
+provider_type = "MbedCrypto"
+key_info_manager = "on-disk-manager"
+
+[[provider]]
+provider_type = "Tpm"
+key_info_manager = "on-disk-manager"
+tcti = "mssim"
+owner_hierarchy_auth = "tpm_pass"
diff --git a/e2e_tests/tests/mod.rs b/e2e_tests/tests/mod.rs
@@ -31,4 +31,5 @@
 #![allow(clippy::multiple_crate_versions)]
 
 mod all_providers;
+mod config;
 mod per_provider;
diff --git a/src/providers/core_provider/mod.rs b/src/providers/core_provider/mod.rs
@@ -71,41 +71,19 @@ pub struct CoreProviderBuilder {
 }
 
 impl CoreProviderBuilder {
-    pub fn new() -> std::io::Result<Self> {
-        let crate_version: Version = Version::from_str(version!()).or_else(|e| {
-            format_error!("Error parsing the crate version", e);
-            Err(Error::new(
-                ErrorKind::InvalidData,
-                "crate version number has invalid format",
-            ))
-        })?;
-
-        let provider_info = vec![ProviderInfo {
-            // Assigned UUID for this provider: 47049873-2a43-4845-9d72-831eab668784
-            uuid: Uuid::parse_str("47049873-2a43-4845-9d72-831eab668784").or_else(|_| Err(Error::new(
-                ErrorKind::InvalidData,
-                "provider UUID is invalid",
-            )))?,
-            description: String::from("Software provider that implements only administrative (i.e. no cryptographic) operations"),
-            vendor: String::new(),
-            version_maj: crate_version.major,
-            version_min: crate_version.minor,
-            version_rev: crate_version.patch,
-            id: ProviderID::Core,
-        }];
-
+    pub fn new() -> Self {
         let mut provider_opcodes = HashMap::new();
         let _ = provider_opcodes.insert(
             ProviderID::Core,
             SUPPORTED_OPCODES.iter().copied().collect(),
         );
 
-        Ok(CoreProviderBuilder {
+        CoreProviderBuilder {
             version_maj: None,
             version_min: None,
-            provider_info,
+            provider_info: Vec::new(),
             provider_opcodes,
-        })
+        }
     }
 
     pub fn with_wire_protocol_version(mut self, version_min: u8, version_maj: u8) -> Self {
@@ -126,7 +104,28 @@ impl CoreProviderBuilder {
         self
     }
 
-    pub fn build(self) -> std::io::Result<CoreProvider> {
+    pub fn build(mut self) -> std::io::Result<CoreProvider> {
+        let crate_version: Version = Version::from_str(version!()).or_else(|e| {
+            format_error!("Error parsing the crate version", e);
+            Err(Error::new(
+                ErrorKind::InvalidData,
+                "crate version number has invalid format",
+            ))
+        })?;
+        self.provider_info.push(ProviderInfo {
+            // Assigned UUID for this provider: 47049873-2a43-4845-9d72-831eab668784
+            uuid: Uuid::parse_str("47049873-2a43-4845-9d72-831eab668784").or_else(|_| Err(Error::new(
+                ErrorKind::InvalidData,
+                "provider UUID is invalid",
+            )))?,
+            description: String::from("Software provider that implements only administrative (i.e. no cryptographic) operations"),
+            vendor: String::new(),
+            version_maj: crate_version.major,
+            version_min: crate_version.minor,
+            version_rev: crate_version.patch,
+            id: ProviderID::Core,
+        });
+
         let core_provider = CoreProvider {
             wire_protocol_version_maj: self
                 .version_maj
diff --git a/src/utils/service_builder.rs b/src/utils/service_builder.rs
@@ -150,14 +150,14 @@ impl ServiceBuilder {
 }
 
 fn build_backend_handlers(
-    mut providers: HashMap<ProviderID, Provider>,
+    mut providers: Vec<(ProviderID, Provider)>,
 ) -> Result<HashMap<ProviderID, BackEndHandler>> {
     let mut map = HashMap::new();
 
-    let mut core_provider_builder = CoreProviderBuilder::new()?
+    let mut core_provider_builder = CoreProviderBuilder::new()
         .with_wire_protocol_version(WIRE_PROTOCOL_VERSION_MINOR, WIRE_PROTOCOL_VERSION_MAJOR);
 
-    for (provider_id, provider) in providers.drain() {
+    for (provider_id, provider) in providers.drain(..) {
         let (info, opcodes) = provider.describe().or_else(|_| {
             Err(Error::new(
                 ErrorKind::InvalidData,
@@ -192,11 +192,11 @@ fn build_backend_handlers(
 fn build_providers(
     configs: &[ProviderConfig],
     key_info_managers: HashMap<String, KeyInfoManager>,
-) -> HashMap<ProviderID, Provider> {
-    let mut map = HashMap::new();
+) -> Vec<(ProviderID, Provider)> {
+    let mut list = Vec::new();
     for config in configs {
         let provider_id = config.provider_id();
-        if map.contains_key(&provider_id) {
+        if list.iter().any(|(id, _)| *id == provider_id) {
             warn!("Parsec currently only supports one instance of each provider type. Ignoring {} and continuing...", provider_id);
             continue;
         }
@@ -222,10 +222,10 @@ fn build_providers(
                 continue;
             }
         };
-        let _ = map.insert(provider_id, provider);
+        let _ = list.push((provider_id, provider));
     }
 
-    map
+    list
 }
 
 // This cfg_attr is used to allow the fact that key_info_manager is not used when there is no
