Repository: golemfactory/yagna (language: python)

Issue:
Keystore CLI tests for Golem certificates
Adding/removing/listing of Golem certificates could be tested (when final CLI output will be decided) by adding some Golem certs (when these will become available) into https://github.com/golemfactory/yagna/blob/8b8524d30aa09c8f2ea0e0937bdc2e06d91757f1/utils/manifest-utils/test-utils/resources/test/certificates.tar and writing some scenarios here https://github.com/golemfactory/yagna/blob/8b8524d30aa09c8f2ea0e0937bdc2e06d91757f1/agent/provider/tests/manifest_negotiator.rs


Discussion / hints:

Fix patch:
diff --git a/Cargo.lock b/Cargo.lock
index 239399ee4c..b1c0e51edb 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2616,7 +2616,7 @@ dependencies = [
 [[package]]
 name = "golem-certificate"
 version = "0.1.0"
-source = "git+https://github.com/golemfactory/golem-certificate?rev=71cb9a5c76a3e741b26ace018da9fe482ccdc268#71cb9a5c76a3e741b26ace018da9fe482ccdc268"
+source = "git+https://github.com/golemfactory/golem-certificate?rev=a8eb02b80c8fd77daaf92ba27600564d4ff55fef#a8eb02b80c8fd77daaf92ba27600564d4ff55fef"
 dependencies = [
  "anyhow",
  "chrono",
diff --git a/agent/provider/Cargo.toml b/agent/provider/Cargo.toml
index abad8f070d..bffb4e891e 100644
--- a/agent/provider/Cargo.toml
+++ b/agent/provider/Cargo.toml
@@ -25,7 +25,7 @@ ya-utils-cli = "0.1"
 ya-utils-path = "0.1"
 ya-utils-process = { version = "0.2", features = ['lock'] }
 ya-std-utils = "0.1"
-golem-certificate = { git = "https://github.com/golemfactory/golem-certificate", rev = "71cb9a5c76a3e741b26ace018da9fe482ccdc268" }
+golem-certificate = { git = "https://github.com/golemfactory/golem-certificate", rev = "a8eb02b80c8fd77daaf92ba27600564d4ff55fef" }
 
 actix = { version = "0.13", default-features = false }
 actix-rt = "2.7"
diff --git a/agent/provider/src/cli/keystore.rs b/agent/provider/src/cli/keystore.rs
index 20775cb31d..ab3f279b97 100644
--- a/agent/provider/src/cli/keystore.rs
+++ b/agent/provider/src/cli/keystore.rs
@@ -231,14 +231,8 @@ impl CertTableBuilder {
 
         let mut values = Vec::new();
         for (id_prefix, cert) in ids.into_iter().zip(self.entries.into_iter()) {
-            values.push(match cert {
-                Cert::X509(cert) => {
-                    serde_json::json! { [ id_prefix, cert.not_after, cert.subject, cert.permissions ] }
-                }
-                Cert::Golem { cert, .. } => {
-                    serde_json::json! { [ id_prefix, "", "", cert.permissions ] }
-                }
-            });
+            values
+                .push(serde_json::json! { [ id_prefix, cert.not_after(), cert.subject(), cert.permissions() ] });
         }
 
         let columns = vec![
diff --git a/utils/manifest-utils/Cargo.toml b/utils/manifest-utils/Cargo.toml
index a53018ce14..bf1e916db4 100644
--- a/utils/manifest-utils/Cargo.toml
+++ b/utils/manifest-utils/Cargo.toml
@@ -23,7 +23,7 @@ schema = ["schemars"]
 ya-agreement-utils = "0.4.1"
 ya-utils-path = "0.1"
 ya-client-model = "0.5"
-golem-certificate = { git = "https://github.com/golemfactory/golem-certificate", rev = "71cb9a5c76a3e741b26ace018da9fe482ccdc268" }
+golem-certificate = { git = "https://github.com/golemfactory/golem-certificate", rev = "a8eb02b80c8fd77daaf92ba27600564d4ff55fef" }
 
 regex = "1.5"
 serde = "1.0"
@@ -57,3 +57,4 @@ shlex = "1.1"
 tar = "0.4"
 test-case = "2.2"
 ya-manifest-test-utils = "0.1"
+openssl = { version = "0.10", features = ["vendored"] }
diff --git a/utils/manifest-utils/src/keystore.rs b/utils/manifest-utils/src/keystore.rs
index 1bb50d63ed..4823b23eee 100644
--- a/utils/manifest-utils/src/keystore.rs
+++ b/utils/manifest-utils/src/keystore.rs
@@ -6,15 +6,20 @@ use self::{
     x509_keystore::{X509AddParams, X509CertData, X509KeystoreBuilder, X509KeystoreManager},
 };
 use crate::policy::CertPermissions;
+use chrono::SecondsFormat;
 use golem_certificate::validator::validated_data::{ValidatedCertificate, ValidatedNodeDescriptor};
 use itertools::Itertools;
+use serde_json::Value;
 use std::{
-    collections::HashSet,
+    collections::{BTreeSet, HashSet},
     ffi::OsStr,
     fs,
     path::{Path, PathBuf},
 };
 
+// Large enum variant caused by flatten maps of possible additional fields in 'ValidatedCertificate'.
+#[allow(clippy::large_enum_variant)]
+#[derive(Eq, PartialEq)]
 pub enum Cert {
     X509(X509CertData),
     Golem {
@@ -24,6 +29,7 @@ pub enum Cert {
 }
 
 impl Cert {
+    /// Certificate id (long).
     pub fn id(&self) -> String {
         match self {
             Cert::Golem { id, cert: _ } => id.into(),
@@ -31,23 +37,44 @@ impl Cert {
         }
     }
 
+    /// Not_after date in RFC3339 format.
     pub fn not_after(&self) -> String {
+        let not_after = match self {
+            Cert::X509(cert) => cert.not_after,
+            Cert::Golem { id: _, cert } => cert.validity_period.not_after,
+        };
+        not_after.to_rfc3339_opts(SecondsFormat::Secs, true)
+    }
+
+    /// Permissions displayed Json value.
+    pub fn permissions(&self) -> Value {
         match self {
-            Cert::X509(cert) => cert.not_after.clone(),
-            Cert::Golem { id: _, cert: _ } => "".into(),
+            Cert::X509(cert) => serde_json::json!(cert.permissions),
+            Cert::Golem { cert, .. } => serde_json::json!(cert.permissions),
         }
     }
 
-    pub fn subject(&self) -> String {
+    /// Subject displayed Json value.
+    /// Json for X.509 certificate, 'display_name' for Golem certificate.
+    pub fn subject(&self) -> Value {
         match self {
-            Cert::X509(cert) => {
-                serde_json::to_string(&cert.subject).expect("Can serialize X509 fields")
-            }
-            Cert::Golem { id: _, cert: _ } => "".into(),
+            Cert::X509(cert) => serde_json::json!(cert.subject),
+            Cert::Golem { cert, .. } => serde_json::json!(cert.subject.display_name),
         }
     }
 }
 
+impl PartialOrd for Cert {
+    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+        self.id().partial_cmp(&other.id())
+    }
+}
+
+impl Ord for Cert {
+    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
+        self.id().cmp(&other.id())
+    }
+}
 trait CommonAddParams {
     fn certs(&self) -> &Vec<PathBuf>;
 }
@@ -176,11 +203,18 @@ impl CompositeKeystore {
         vec![&mut self.golem_keystore, &mut self.x509_keystore]
     }
 
-    pub fn list_ids(&self) -> HashSet<String> {
-        self.list()
+    fn list_sorted(&self) -> BTreeSet<Cert> {
+        self.keystores()
+            .iter()
+            .flat_map(|keystore| keystore.list())
+            .collect::<BTreeSet<Cert>>()
+    }
+
+    pub fn list_ids(&self) -> Vec<String> {
+        self.list_sorted()
             .into_iter()
             .map(|cert| cert.id())
-            .collect::<HashSet<String>>()
+            .collect::<Vec<String>>()
     }
 
     pub fn verify_x509_signature(
@@ -245,10 +279,7 @@ impl Keystore for CompositeKeystore {
     }
 
     fn list(&self) -> Vec<Cert> {
-        self.keystores()
-            .iter()
-            .flat_map(|keystore| keystore.list())
-            .collect()
+        self.list_sorted().into_iter().collect()
     }
 }
 
diff --git a/utils/manifest-utils/src/keystore/x509_keystore.rs b/utils/manifest-utils/src/keystore/x509_keystore.rs
index f0c761da18..3ad255d3f9 100644
--- a/utils/manifest-utils/src/keystore/x509_keystore.rs
+++ b/utils/manifest-utils/src/keystore/x509_keystore.rs
@@ -4,7 +4,9 @@ use super::{
 };
 use crate::{policy::CertPermissions, util::format_permissions};
 use anyhow::{anyhow, bail};
+use chrono::{DateTime, Duration, NaiveDateTime, Utc};
 use openssl::{
+    asn1::{Asn1Time, Asn1TimeRef},
     hash::MessageDigest,
     nid::Nid,
     pkey::{PKey, Public},
@@ -29,9 +31,10 @@ pub(super) trait X509AddParams {
     fn whole_chain(&self) -> bool;
 }
 
+#[derive(Eq, PartialEq)]
 pub struct X509CertData {
     pub id: String,
-    pub not_after: String,
+    pub not_after: DateTime<Utc>,
     pub subject: BTreeMap<String, String>,
     pub permissions: String,
 }
@@ -39,7 +42,7 @@ pub struct X509CertData {
 impl X509CertData {
     pub fn create(cert: &X509Ref, permissions: &Vec<CertPermissions>) -> anyhow::Result<Self> {
         let id = cert_to_id(cert)?;
-        let not_after = cert.not_after().to_string();
+        let not_after = asn1_time_to_date_time(cert.not_after())?;
         let mut subject = BTreeMap::new();
         add_cert_subject_entries(&mut subject, cert, Nid::COMMONNAME, "CN");
         add_cert_subject_entries(&mut subject, cert, Nid::PKCS9_EMAILADDRESS, "E");
@@ -58,6 +61,16 @@ impl X509CertData {
     }
 }
 
+fn asn1_time_to_date_time(time: &Asn1TimeRef) -> anyhow::Result<DateTime<Utc>> {
+    // Openssl lib allows to access time only through ASN1_TIME_print.
+    // Diff starting from epoch is a workaround to get `not_after` value.
+    let time_diff = Asn1Time::from_unix(0)?.diff(time)?;
+    let not_after = NaiveDateTime::from_timestamp_millis(0).unwrap()
+        + Duration::days(time_diff.days as i64)
+        + Duration::seconds(time_diff.secs as i64);
+    Ok(DateTime::<Utc>::from_utc(not_after, Utc))
+}
+
 pub(super) struct AddX509Response {
     pub loaded: Vec<X509>,
     pub skipped: Vec<X509>,
@@ -669,3 +682,23 @@ impl std::fmt::Debug for X509Keystore {
         write!(f, "Keystore")
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use openssl::asn1::Asn1Time;
+    use test_case::test_case;
+
+    use super::asn1_time_to_date_time;
+
+    // No test for malformed date because 'Asn1Time' arrvies from parsed certificate.
+    #[test_case("20230329115959Z", "2023-03-29T11:59:59Z" ; "After epoch")]
+    #[test_case("19000101000000Z", "1900-01-01T00:00:00Z" ; "Before epoch")]
+    pub fn read_not_after_test(asn1_time: &str, expected_time: &str) {
+        let asn1_time = Asn1Time::from_str(asn1_time).unwrap();
+        let date_time = asn1_time_to_date_time(&asn1_time).unwrap();
+        assert_eq!(
+            expected_time,
+            date_time.to_rfc3339_opts(chrono::SecondsFormat::Secs, true)
+        );
+    }
+}


Test patch:
diff --git a/agent/provider/tests/keystore_cli.rs b/agent/provider/tests/keystore_cli.rs
index 0eee7e1acb..f24f563ee4 100644
--- a/agent/provider/tests/keystore_cli.rs
+++ b/agent/provider/tests/keystore_cli.rs
@@ -145,6 +145,20 @@ fn test_keystore_list_cmd_creates_cert_dir_in_dir_set_by_arg() {
     vec![("fe4f04e2", "none"), ("55e451bd", "outbound-manifest"), ("20e9eb45", "outbound-manifest")];
     "Add multiple certificates and check permissions"
 )]
+#[test_case(
+    vec!["root-certificate.signed.json", "partner-certificate.signed.json"],
+    vec![],
+    false,
+    vec![("80c84b27", "all"), ("cb16a2ed", "{\"outbound\":\"unrestricted\"}")];
+    "Add multiple Golem certificates and check permissions"
+)]
+#[test_case(
+    vec!["foo_req.cert.pem", "partner-certificate.signed.json"],
+    vec![],
+    false,
+    vec![("25b9430c", "none"), ("cb16a2ed", "{\"outbound\":\"unrestricted\"}")];
+    "Add Golem and X509 certificate and then check permissions"
+)]
 #[serial]
 fn test_keystore_add_certificate_permissions(
     certificates: Vec<&str>,
@@ -152,110 +166,168 @@ fn test_keystore_add_certificate_permissions(
     whole_chain: bool,
     expected: Vec<(&str, &str)>,
 ) {
+    let result = add_and_list(certificates, permissions, whole_chain);
+    for (cert_id, perm) in expected {
+        assert_eq!(read_permissions(&result, dbg!(cert_id)), perm);
+    }
+}
+
+#[serial]
+#[test]
+fn test_keystore_set_should_modify_existing_permissions() {
     let (resource_cert_dir, cert_dir) = CERT_TEST_RESOURCES.init_cert_dirs();
 
-    let mut command = Command::cargo_bin("ya-provider").unwrap();
-    command
-        .args(["--cert-dir", cert_dir.as_path().to_str().unwrap()])
-        .arg("keystore")
-        .arg("add");
+    add(
+        vec!["foo_ca-chain.cert.pem"],
+        vec!["all"],
+        true,
+        &resource_cert_dir,
+        &cert_dir,
+    );
 
-    if !certificates.is_empty() {
-        for certificate in certificates {
-            command.arg(resource_cert_dir.join(certificate));
-        }
-    }
+    // This call doesn't specify any permissions, so these should be removed.
+    add(
+        vec!["foo_ca-chain.cert.pem"],
+        vec![],
+        true,
+        &resource_cert_dir,
+        &cert_dir,
+    );
 
-    if !permissions.is_empty() {
-        command.arg("--permissions");
-        permissions.iter().for_each(|permission| {
-            command.arg(permission);
-        })
-    }
+    let result = list_certificates_command(&cert_dir).unwrap();
 
-    if whole_chain {
-        command.arg("--whole-chain");
-    }
+    assert_eq!(read_permissions(&result, "55e451bd"), "none");
+    assert_eq!(read_permissions(&result, "fe4f04e2"), "none");
+}
 
-    command.arg("--json").assert().success();
+#[serial]
+#[test]
+fn test_keystore_remove_certificate_check_permissions() {
+    let (resource_cert_dir, cert_dir) = CERT_TEST_RESOURCES.init_cert_dirs();
+
+    add(
+        vec!["foo_ca-chain.cert.pem", "foo_req.cert.pem"],
+        vec!["outbound-manifest"],
+        true,
+        &resource_cert_dir,
+        &cert_dir,
+    );
+
+    // Removing a certificate from the chain should not remove permissions of remaining ones.
+    remove(&cert_dir, vec!["fe4f04e2"]);
 
     let result = list_certificates_command(&cert_dir).unwrap();
-    println!("Result: {result:#?}");
-    for (cert_id, perm) in expected {
-        assert_eq!(check_permissions(&result, dbg!(cert_id)), perm);
-    }
+
+    assert_eq!(read_permissions(&result, "55e451bd"), "outbound-manifest");
+    assert_eq!(read_permissions(&result, "25b9430c"), "outbound-manifest");
 }
 
 #[serial]
 #[test]
-fn test_keystore_add_certificate_second_time() {
+fn test_add_and_remove_certificates() {
     let (resource_cert_dir, cert_dir) = CERT_TEST_RESOURCES.init_cert_dirs();
+    add(
+        vec!["foo_req.cert.pem", "partner-certificate.signed.json"],
+        vec![],
+        false,
+        &resource_cert_dir,
+        &cert_dir,
+    );
 
-    Command::cargo_bin("ya-provider")
-        .unwrap()
-        .args(["--cert-dir", cert_dir.as_path().to_str().unwrap()])
-        .arg("keystore")
-        .arg("add")
-        .arg(resource_cert_dir.join("foo_ca-chain.cert.pem"))
-        .arg("--permissions")
-        .arg("all")
-        .arg("--whole-chain")
-        .arg("--json")
-        .assert()
-        .success();
+    let result = list_certificates_command(&cert_dir).unwrap();
+    assert!(result.contains_key("cb16a2ed"));
+    assert!(result.contains_key("25b9430c"));
+    assert_eq!(result.len(), 2);
 
-    // This call doesn't specify any permissions, so the will be removed.
-    Command::cargo_bin("ya-provider")
-        .unwrap()
-        .args(["--cert-dir", cert_dir.as_path().to_str().unwrap()])
-        .arg("keystore")
-        .arg("add")
-        .arg(resource_cert_dir.join("foo_ca-chain.cert.pem"))
-        .arg("--whole-chain")
-        .arg("--json")
-        .assert()
-        .success();
+    remove(&cert_dir, vec!["cb16a2ed", "25b9430c"]);
 
     let result = list_certificates_command(&cert_dir).unwrap();
+    assert!(result.is_empty());
+}
 
-    assert_eq!(check_permissions(&result, "55e451bd"), "none");
-    assert_eq!(check_permissions(&result, "fe4f04e2"), "none");
+#[serial]
+#[test]
+fn verify_not_after_date_format() {
+    let result = add_and_list(
+        vec!["foo_req.cert.pem", "partner-certificate.signed.json"],
+        vec![],
+        false,
+    );
+    assert_eq!(read_not_after(&result, "cb16a2ed"), "2025-01-01T00:00:00Z");
+    assert_eq!(read_not_after(&result, "25b9430c"), "2122-07-17T12:05:22Z")
 }
 
 #[serial]
 #[test]
-fn test_keystore_remove_certificate_check_permissions() {
+fn verify_subject_format() {
+    let result = add_and_list(
+        vec!["foo_req.cert.pem", "partner-certificate.signed.json"],
+        vec![],
+        false,
+    );
+    assert_eq!(read_subject(&result, "cb16a2ed"), "Example partner cert");
+    assert_eq!(read_subject(&result, "25b9430c"), "{\"C\":\"CZ\",\"CN\":\"Foo Req\",\"E\":\"office@req.foo.com\",\"O\":\"Foo Req Co\",\"OU\":\"Foo Req HQ\",\"ST\":\"Bohemia\"}")
+}
+
+fn add_and_list(
+    certificates: Vec<&str>,
+    permissions: Vec<&str>,
+    whole_chain: bool,
+) -> HashMap<String, Value> {
     let (resource_cert_dir, cert_dir) = CERT_TEST_RESOURCES.init_cert_dirs();
+    add(
+        certificates,
+        permissions,
+        whole_chain,
+        &resource_cert_dir,
+        &cert_dir,
+    );
+    list_certificates_command(&cert_dir).unwrap()
+}
 
-    Command::cargo_bin("ya-provider")
-        .unwrap()
-        .args(["--cert-dir", cert_dir.as_path().to_str().unwrap()])
+fn add(
+    certificates: Vec<&str>,
+    permissions: Vec<&str>,
+    whole_chain: bool,
+    resource_cert_dir: &Path,
+    cert_dir: &Path,
+) {
+    let mut command = Command::cargo_bin("ya-provider").unwrap();
+    command
+        .args(["--cert-dir", cert_dir.to_str().unwrap()])
         .arg("keystore")
-        .arg("add")
-        .arg(resource_cert_dir.join("foo_ca-chain.cert.pem"))
-        .arg(resource_cert_dir.join("foo_req.cert.pem"))
-        .arg("--permissions")
-        .arg("outbound-manifest")
-        .arg("--whole-chain")
-        .arg("--json")
-        .assert()
-        .success();
+        .arg("add");
 
-    // This call doesn't specify any permissions, so the will be removed.
-    Command::cargo_bin("ya-provider")
-        .unwrap()
-        .args(["--cert-dir", cert_dir.as_path().to_str().unwrap()])
-        .arg("keystore")
-        .arg("remove")
-        .arg("fe4f04e2")
-        .arg("--json")
-        .assert()
-        .success();
+    if !certificates.is_empty() {
+        for certificate in certificates {
+            command.arg(resource_cert_dir.join(certificate));
+        }
+    }
 
-    let result = list_certificates_command(&cert_dir).unwrap();
+    if !permissions.is_empty() {
+        command.arg("--permissions");
+        permissions.iter().for_each(|permission| {
+            command.arg(permission);
+        })
+    }
+
+    if whole_chain {
+        command.arg("--whole-chain");
+    }
 
-    assert_eq!(check_permissions(&result, "55e451bd"), "outbound-manifest");
-    assert_eq!(check_permissions(&result, "25b9430c"), "outbound-manifest");
+    command.arg("--json").assert().success();
+}
+
+fn remove(cert_dir: &Path, certificate_ids: Vec<&str>) {
+    let mut command = Command::cargo_bin("ya-provider").unwrap();
+    command
+        .args(["--cert-dir", cert_dir.to_str().unwrap()])
+        .arg("keystore")
+        .arg("remove");
+    for certificate_id in certificate_ids {
+        command.arg(certificate_id);
+    }
+    command.arg("--json").assert().success();
 }
 
 fn list_certificates_command(
@@ -289,11 +361,24 @@ fn list_certificates_command(
         .collect())
 }
 
-fn check_permissions(certs: &HashMap<String, Value>, id: &str) -> String {
-    certs[id]
-        .get("Permissions")
-        .unwrap()
-        .as_str()
-        .unwrap()
-        .to_string()
+fn read_not_after(certs: &HashMap<String, Value>, id: &str) -> String {
+    read_field(certs, id, "Not After")
+}
+
+fn read_permissions(certs: &HashMap<String, Value>, id: &str) -> String {
+    read_field(certs, id, "Permissions")
+}
+
+fn read_subject(certs: &HashMap<String, Value>, id: &str) -> String {
+    read_field(certs, id, "Subject")
+}
+
+fn read_field(certs: &HashMap<String, Value>, id: &str, field: &str) -> String {
+    let permissions = certs[id].get(field).unwrap();
+    if permissions.is_string() {
+        // Calling `to_string` would result in quoted string.
+        permissions.as_str().unwrap().into()
+    } else {
+        permissions.to_string()
+    }
 }
diff --git a/utils/manifest-utils/test-utils/resources/test/certificates.tar b/utils/manifest-utils/test-utils/resources/test/certificates.tar
index 9e8a5cb229..20b2df3325 100644
Binary files a/utils/manifest-utils/test-utils/resources/test/certificates.tar and b/utils/manifest-utils/test-utils/resources/test/certificates.tar differ
diff --git a/utils/manifest-utils/tests/certificate_store.rs b/utils/manifest-utils/tests/certificate_store.rs
index dc60e34bc2..d4d33ed9fa 100644
--- a/utils/manifest-utils/tests/certificate_store.rs
+++ b/utils/manifest-utils/tests/certificate_store.rs
@@ -78,11 +78,7 @@ fn certificate_store_test(
     remove_certificates(&test_cert_dir, ids_to_remove);
     // When
     let keystore = CompositeKeystore::load(&test_cert_dir).expect("Can load keystore");
-    let loaded_ids = keystore
-        .list()
-        .into_iter()
-        .map(|c| c.id())
-        .collect::<HashSet<String>>();
+    let loaded_ids = keystore.list_ids().into_iter().collect::<HashSet<String>>();
     // Then
     let expected_ids = expected_ids
         .iter()