From 4a2eb41d58e5aa8c79a2a21f5d3517e45cd84af6 Mon Sep 17 00:00:00 2001 From: Didier Wenzek Date: Mon, 25 Nov 2024 16:31:26 +0100 Subject: [PATCH 1/6] Remove duplicated code Signed-off-by: Didier Wenzek --- crates/common/certificate/src/lib.rs | 59 +++++++++++----------------- 1 file changed, 22 insertions(+), 37 deletions(-) diff --git a/crates/common/certificate/src/lib.rs b/crates/common/certificate/src/lib.rs index 7e82efaf9f0..15a5fbb16f1 100644 --- a/crates/common/certificate/src/lib.rs +++ b/crates/common/certificate/src/lib.rs @@ -124,6 +124,20 @@ impl KeyCertPair { let not_before = today - Duration::days(1); // Ensure the certificate is valid today let params = Self::create_selfsigned_certificate_parameters(config, id, key_kind, not_before)?; + + Ok(KeyCertPair { + certificate: Zeroizing::new(Certificate::from_params(params)?), + }) + } + + pub fn new_certificate_sign_request( + config: &NewCertificateConfig, + id: &str, + key_kind: &KeyKind, + ) -> Result { + // Create Certificate without `not_before` and `not_after` fields + // as rcgen library will not parse it for certificate signing request + let params = Self::create_csr_parameters(config, id, key_kind)?; Ok(KeyCertPair { certificate: Zeroizing::new(Certificate::from_params(params)?), }) @@ -135,48 +149,19 @@ impl KeyCertPair { key_kind: &KeyKind, not_before: OffsetDateTime, ) -> Result { - KeyCertPair::check_identifier(id, config.max_cn_size)?; - let mut distinguished_name = rcgen::DistinguishedName::new(); - distinguished_name.push(rcgen::DnType::CommonName, id); - distinguished_name.push(rcgen::DnType::OrganizationName, &config.organization_name); - distinguished_name.push( - rcgen::DnType::OrganizationalUnitName, - &config.organizational_unit_name, - ); - - let mut params = CertificateParams::default(); - - params.distinguished_name = distinguished_name; + let mut params = Self::create_csr_parameters(config, id, key_kind)?; let not_after = not_before + Duration::days(config.validity_period_days.into()); params.not_before = not_before; params.not_after = not_after; - params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); // IsCa::SelfSignedOnly is rejected by C8Y - - params.alg = &rcgen::PKCS_ECDSA_P256_SHA256; // ECDSA signing using the P-256 curves and SHA-256 hashing as per RFC 5758 - - if let KeyKind::Reuse { keypair_pem } = key_kind { - params.key_pair = Some(KeyPair::from_pem(keypair_pem)?); - } + // IsCa::SelfSignedOnly is rejected by C8Y with "422 Unprocessable Entity" + params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); Ok(params) } - // Create Certificate without `not_before` and `not_after` fields - // as rcgen library will not parse it for certificate signing request - pub fn new_certificate_sign_request( - config: &NewCertificateConfig, - id: &str, - key_kind: &KeyKind, - ) -> Result { - let params = Self::create_certificate_sign_request_parameters(config, id, key_kind)?; - Ok(KeyCertPair { - certificate: Zeroizing::new(Certificate::from_params(params)?), - }) - } - - fn create_certificate_sign_request_parameters( + fn create_csr_parameters( config: &NewCertificateConfig, id: &str, key_kind: &KeyKind, @@ -193,7 +178,8 @@ impl KeyCertPair { let mut params = CertificateParams::default(); params.distinguished_name = distinguished_name; - params.alg = &rcgen::PKCS_ECDSA_P256_SHA256; // ECDSA signing using the P-256 curves and SHA-256 hashing as per RFC 5758 + // ECDSA signing using the P-256 curves and SHA-256 hashing as per RFC 5758 + params.alg = &rcgen::PKCS_ECDSA_P256_SHA256; if let KeyKind::Reuse { keypair_pem } = key_kind { params.key_pair = Some(KeyPair::from_pem(keypair_pem)?); @@ -464,9 +450,8 @@ mod tests { let config = NewCertificateConfig::default(); let id = "some-id"; - let params = - KeyCertPair::create_certificate_sign_request_parameters(&config, id, &KeyKind::New) - .expect("Fail to get a certificate parameters"); + let params = KeyCertPair::create_csr_parameters(&config, id, &KeyKind::New) + .expect("Fail to get a certificate parameters"); let keypair = KeyCertPair { certificate: Zeroizing::new( From 3047ac2fcfaef69133efc5023cb5bd22bb58ae04 Mon Sep 17 00:00:00 2001 From: Didier Wenzek Date: Tue, 26 Nov 2024 18:18:56 +0100 Subject: [PATCH 2/6] Untangle impl for cert/csr creation/renewal Signed-off-by: Didier Wenzek --- crates/common/certificate/src/lib.rs | 2 +- .../core/tedge/src/cli/certificate/create.rs | 183 +++++++++++------- 2 files changed, 111 insertions(+), 74 deletions(-) diff --git a/crates/common/certificate/src/lib.rs b/crates/common/certificate/src/lib.rs index 15a5fbb16f1..6b8efe0780c 100644 --- a/crates/common/certificate/src/lib.rs +++ b/crates/common/certificate/src/lib.rs @@ -8,7 +8,7 @@ use std::path::Path; use std::path::PathBuf; use time::Duration; use time::OffsetDateTime; -use zeroize::Zeroizing; +pub use zeroize::Zeroizing; #[cfg(feature = "reqwest")] mod cloud_root_certificate; #[cfg(feature = "reqwest")] diff --git a/crates/core/tedge/src/cli/certificate/create.rs b/crates/core/tedge/src/cli/certificate/create.rs index edac191679c..a5afe81cc19 100644 --- a/crates/core/tedge/src/cli/certificate/create.rs +++ b/crates/core/tedge/src/cli/certificate/create.rs @@ -10,11 +10,10 @@ use certificate::PemCertificate; use std::fs::File; use std::fs::OpenOptions; use std::io::prelude::*; -use std::io::ErrorKind; use std::path::Path; -use tedge_utils::file::create_file_with_mode_or_overwrite; use tedge_utils::paths::set_permission; use tedge_utils::paths::validate_parent_dir_exists; + /// Create a self-signed device certificate pub struct CreateCertCmd { /// The device identifier @@ -48,99 +47,102 @@ impl Command for CreateCertCmd { impl CreateCertCmd { pub fn create_test_certificate(&self, config: &NewCertificateConfig) -> Result<(), CertError> { - // Reuse private key if it already exists - let key_kind = match std::fs::read_to_string(&self.key_path) { - Ok(keypair_pem) => KeyKind::Reuse { keypair_pem }, - Err(err) if err.kind() == ErrorKind::NotFound => KeyKind::New, - Err(err) => return Err(CertError::IoError(err).cert_context(self.cert_path.clone())), - }; - self.create_test_certificate_for(config, &key_kind) + let cert = KeyCertPair::new_selfsigned_certificate(config, &self.id, &KeyKind::New)?; + + let cert_path = &self.cert_path; + self.persist_new_public_key(cert_path, cert.certificate_pem_string()?) + .map_err(|err| err.cert_context(cert_path.clone()))?; + + let key_path = &self.key_path; + self.persist_new_private_key(key_path, cert.private_key_pem_string()?) + .map_err(|err| err.key_context(key_path.clone()))?; + Ok(()) } pub fn renew_test_certificate(&self, config: &NewCertificateConfig) -> Result<(), CertError> { - let keypair_pem = std::fs::read_to_string(&self.key_path) - .map_err(|e| CertError::IoError(e).key_context(self.key_path.clone()))?; - self.create_test_certificate_for(config, &KeyKind::Reuse { keypair_pem }) + let cert_path = &self.cert_path; + let key_path = &self.key_path; + + let previous_key = reuse_private_key(key_path) + .map_err(|e| CertError::IoError(e).key_context(key_path.clone()))?; + let cert = KeyCertPair::new_selfsigned_certificate(config, &self.id, &previous_key)?; + + self.override_public_key(cert_path, cert.certificate_pem_string()?) + .map_err(|err| err.cert_context(cert_path.clone()))?; + Ok(()) } pub fn create_certificate_signing_request( &self, config: &NewCertificateConfig, ) -> Result<(), CertError> { - // Reuse private key if it already exists - let key_kind = match std::fs::read_to_string(&self.key_path) { - Ok(keypair_pem) => KeyKind::Reuse { keypair_pem }, - Err(err) if err.kind() == ErrorKind::NotFound => KeyKind::New, - Err(err) => return Err(CertError::IoError(err).cert_context(self.cert_path.clone())), - }; - self.create_test_certificate_for(config, &key_kind) + // FIXME the current implementation of CreateCertCmd is ambiguous + // One can prepare to create a self-signed certificate and then try to create a CSR + let csr_path = self + .csr_path + .as_ref() + .expect("creating a CSR instead of a self-signed certificate"); + let key_path = &self.key_path; + + let previous_key = reuse_private_key(key_path).unwrap_or(KeyKind::New); + let cert = KeyCertPair::new_certificate_sign_request(config, &self.id, &previous_key)?; + + if let KeyKind::New = previous_key { + self.persist_new_private_key(key_path, cert.private_key_pem_string()?) + .map_err(|err| err.key_context(key_path.clone()))?; + } + self.override_public_key(csr_path, cert.certificate_signing_request_string()?) + .map_err(|err| err.cert_context(csr_path.clone()))?; + Ok(()) } - fn create_test_certificate_for( + fn persist_new_public_key( &self, - config: &NewCertificateConfig, - key_kind: &KeyKind, + cert_path: &Utf8PathBuf, + pem_string: String, ) -> Result<(), CertError> { - validate_parent_dir_exists(&self.cert_path).map_err(CertError::CertPathError)?; - validate_parent_dir_exists(&self.key_path).map_err(CertError::KeyPathError)?; + let (user, group) = self.certificate_user_group(); - let (user, group) = match self.bridge_location { - BridgeLocation::BuiltIn => ("tedge", "tedge"), - BridgeLocation::Mosquitto => (crate::BROKER_USER, crate::BROKER_GROUP), - }; - - let cert = match &self.csr_path { - Some(csr_path) => { - validate_parent_dir_exists(csr_path).map_err(CertError::CsrPathError)?; - - let cert = KeyCertPair::new_certificate_sign_request(config, &self.id, key_kind)?; - let cert_csr = cert.certificate_signing_request_string()?; - - create_file_with_mode_or_overwrite(csr_path, Some(cert_csr.as_str()), 0o444)?; - - cert - } - None => { - let cert = KeyCertPair::new_selfsigned_certificate(config, &self.id, key_kind)?; - - // Creating files with permission 644 owned by the MQTT broker - let mut cert_file = create_new_file(&self.cert_path, user, group) - .map_err(|err| err.cert_context(self.cert_path.clone()))?; - - let cert_pem = cert.certificate_pem_string()?; - cert_file.write_all(cert_pem.as_bytes())?; - cert_file.sync_all()?; - - // Prevent the certificate to be overwritten - set_permission(&cert_file, 0o444)?; - - cert - } - }; + validate_parent_dir_exists(cert_path).map_err(CertError::CertPathError)?; + persist_public_key(create_new_file(cert_path, user, group)?, pem_string)?; + Ok(()) + } - if let KeyKind::New = key_kind { - // TODO cope with broker user being tedge - let mut key_file = - create_new_file(&self.key_path, crate::BROKER_USER, crate::BROKER_GROUP) - .map_err(|err| err.key_context(self.key_path.clone()))?; + fn persist_new_private_key( + &self, + key_path: &Utf8PathBuf, + key: certificate::Zeroizing, + ) -> Result<(), CertError> { + let (user, group) = self.certificate_user_group(); - // Make sure the key is secret, before write - set_permission(&key_file, 0o600)?; + validate_parent_dir_exists(key_path).map_err(CertError::KeyPathError)?; + persist_private_key(create_new_file(key_path, user, group)?, key)?; + Ok(()) + } - // Zero the private key on drop - let cert_key = cert.private_key_pem_string()?; - key_file.write_all(cert_key.as_bytes())?; - key_file.sync_all()?; + fn override_public_key( + &self, + cert_path: &Utf8PathBuf, + pem_string: String, + ) -> Result<(), CertError> { + validate_parent_dir_exists(cert_path).map_err(CertError::CertPathError)?; + persist_public_key(override_file(cert_path)?, pem_string)?; + Ok(()) + } - // Prevent the key to be overwritten - set_permission(&key_file, 0o400)?; + fn certificate_user_group(&self) -> (&str, &str) { + match self.bridge_location { + BridgeLocation::BuiltIn => ("tedge", "tedge"), + BridgeLocation::Mosquitto => (crate::BROKER_USER, crate::BROKER_GROUP), } - - Ok(()) } } -fn create_new_file(path: impl AsRef, user: &str, group: &str) -> Result { +fn create_new_file( + path: impl AsRef, + user: &str, + group: &str, +) -> Result { let file = OpenOptions::new() .write(true) .create_new(true) @@ -154,6 +156,41 @@ fn create_new_file(path: impl AsRef, user: &str, group: &str) -> Result) -> Result { + OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .open(path.as_ref()) +} + +fn reuse_private_key(key_path: &Utf8PathBuf) -> Result { + std::fs::read_to_string(key_path).map(|keypair_pem| KeyKind::Reuse { keypair_pem }) +} + +fn persist_private_key( + mut key_file: File, + cert_key: certificate::Zeroizing, // Zero the private key on drop +) -> Result<(), std::io::Error> { + // Make sure the key is secret, before write + set_permission(&key_file, 0o600)?; + key_file.write_all(cert_key.as_bytes())?; + key_file.sync_all()?; + + // Prevent the key to be overwritten + set_permission(&key_file, 0o400)?; + Ok(()) +} + +fn persist_public_key(mut key_file: File, cert_pem: String) -> Result<(), std::io::Error> { + key_file.write_all(cert_pem.as_bytes())?; + key_file.sync_all()?; + + // Make the file public + set_permission(&key_file, 0o444)?; + Ok(()) +} + pub fn cn_of_self_signed_certificate(cert_path: &Utf8PathBuf) -> Result { let pem = PemCertificate::from_pem_file(cert_path).map_err(|err| match err { certificate::CertificateError::IoError { error, .. } => { From 7ebf3c2d51a50c6fd4d069fb033fb7be79d4066b Mon Sep 17 00:00:00 2001 From: Didier Wenzek Date: Tue, 26 Nov 2024 18:35:17 +0100 Subject: [PATCH 3/6] Remove misleading field: CreateCertCmd::csr_path This field was used to hack the CreateCertCmd making it creating a CSR instead of a self-signed certificate. Signed-off-by: Didier Wenzek --- crates/core/tedge/src/cli/certificate/cli.rs | 1 - .../core/tedge/src/cli/certificate/create.rs | 19 +++---------------- .../tedge/src/cli/certificate/create_csr.rs | 4 +--- .../core/tedge/src/cli/certificate/renew.rs | 2 -- 4 files changed, 4 insertions(+), 22 deletions(-) diff --git a/crates/core/tedge/src/cli/certificate/cli.rs b/crates/core/tedge/src/cli/certificate/cli.rs index fb85094b6e2..4cdfd5c3398 100644 --- a/crates/core/tedge/src/cli/certificate/cli.rs +++ b/crates/core/tedge/src/cli/certificate/cli.rs @@ -64,7 +64,6 @@ impl BuildCommand for TEdgeCertCli { id, cert_path: config.device.cert_path.clone(), key_path: config.device.key_path.clone(), - csr_path: None, bridge_location, }; cmd.into_boxed() diff --git a/crates/core/tedge/src/cli/certificate/create.rs b/crates/core/tedge/src/cli/certificate/create.rs index a5afe81cc19..681d4181763 100644 --- a/crates/core/tedge/src/cli/certificate/create.rs +++ b/crates/core/tedge/src/cli/certificate/create.rs @@ -14,20 +14,17 @@ use std::path::Path; use tedge_utils::paths::set_permission; use tedge_utils::paths::validate_parent_dir_exists; -/// Create a self-signed device certificate +/// Create self-signed device certificate and signing request pub struct CreateCertCmd { /// The device identifier pub id: String, - /// The path where the device certificate will be stored + /// The path where the device certificate / request will be stored pub cert_path: Utf8PathBuf, /// The path where the device private key will be stored pub key_path: Utf8PathBuf, - /// The path where the device CSR file will be stored - pub csr_path: Option, - /// The component that is configured to host the MQTT bridge logic pub bridge_location: BridgeLocation, } @@ -76,12 +73,7 @@ impl CreateCertCmd { &self, config: &NewCertificateConfig, ) -> Result<(), CertError> { - // FIXME the current implementation of CreateCertCmd is ambiguous - // One can prepare to create a self-signed certificate and then try to create a CSR - let csr_path = self - .csr_path - .as_ref() - .expect("creating a CSR instead of a self-signed certificate"); + let csr_path = &self.cert_path; let key_path = &self.key_path; let previous_key = reuse_private_key(key_path).unwrap_or(KeyKind::New); @@ -226,7 +218,6 @@ mod tests { id: String::from(id), cert_path: cert_path.clone(), key_path: key_path.clone(), - csr_path: None, bridge_location: BridgeLocation::Mosquitto, }; @@ -255,7 +246,6 @@ mod tests { id: "my-device-id".into(), cert_path: cert_path.clone(), key_path: key_path.clone(), - csr_path: None, bridge_location: BridgeLocation::Mosquitto, }; @@ -278,7 +268,6 @@ mod tests { id: "my-device-id".into(), cert_path, key_path, - csr_path: None, bridge_location: BridgeLocation::Mosquitto, }; @@ -298,7 +287,6 @@ mod tests { id: "my-device-id".into(), cert_path, key_path, - csr_path: None, bridge_location: BridgeLocation::Mosquitto, }; @@ -318,7 +306,6 @@ mod tests { id: "my-device-id".into(), cert_path, key_path, - csr_path: None, bridge_location: BridgeLocation::Mosquitto, }; diff --git a/crates/core/tedge/src/cli/certificate/create_csr.rs b/crates/core/tedge/src/cli/certificate/create_csr.rs index 34277995d90..6d3baebfda1 100644 --- a/crates/core/tedge/src/cli/certificate/create_csr.rs +++ b/crates/core/tedge/src/cli/certificate/create_csr.rs @@ -41,9 +41,8 @@ impl CreateCsrCmd { let create_cmd = CreateCertCmd { id, - cert_path: self.cert_path.clone(), + cert_path: self.csr_path.clone(), key_path: self.key_path.clone(), - csr_path: Some(self.csr_path.clone()), bridge_location: self.bridge_location, }; @@ -98,7 +97,6 @@ mod tests { id: String::from(id), cert_path: cert_path.clone(), key_path: key_path.clone(), - csr_path: None, bridge_location: BridgeLocation::Mosquitto, }; diff --git a/crates/core/tedge/src/cli/certificate/renew.rs b/crates/core/tedge/src/cli/certificate/renew.rs index 519101e4584..20fbac43435 100644 --- a/crates/core/tedge/src/cli/certificate/renew.rs +++ b/crates/core/tedge/src/cli/certificate/renew.rs @@ -39,7 +39,6 @@ impl RenewCertCmd { id, cert_path: self.cert_path.clone(), key_path: self.key_path.clone(), - csr_path: None, bridge_location: self.bridge_location, }; @@ -65,7 +64,6 @@ mod tests { id: String::from(id), cert_path: cert_path.clone(), key_path: key_path.clone(), - csr_path: None, bridge_location: BridgeLocation::Mosquitto, }; From 22fab3ce955e45081f5157d73e9ca3c07d15c5f9 Mon Sep 17 00:00:00 2001 From: Didier Wenzek Date: Tue, 26 Nov 2024 19:01:21 +0100 Subject: [PATCH 4/6] Directly provice the cert owner to CreateCertCmd Previously, this user was derived from the bridge type introducing an unrelated domain, even if in practice thin-edge mainly uses certificates to authenticate the bridge. Signed-off-by: Didier Wenzek --- crates/core/tedge/src/cli/certificate/cli.rs | 16 +-- .../core/tedge/src/cli/certificate/create.rs | 115 ++++++++++-------- .../tedge/src/cli/certificate/create_csr.rs | 16 ++- .../core/tedge/src/cli/certificate/renew.rs | 13 +- 4 files changed, 88 insertions(+), 72 deletions(-) diff --git a/crates/core/tedge/src/cli/certificate/cli.rs b/crates/core/tedge/src/cli/certificate/cli.rs index 4cdfd5c3398..f61a01a6bb2 100644 --- a/crates/core/tedge/src/cli/certificate/cli.rs +++ b/crates/core/tedge/src/cli/certificate/cli.rs @@ -1,4 +1,3 @@ -use crate::bridge::BridgeLocation; use camino::Utf8PathBuf; use tedge_config::OptionalConfigError; use tedge_config::ProfileName; @@ -52,10 +51,10 @@ pub enum TEdgeCertCli { impl BuildCommand for TEdgeCertCli { fn build_command(self, context: BuildContext) -> Result, ConfigError> { let config = context.load_config()?; - let bridge_location = if config.mqtt.bridge.built_in { - BridgeLocation::BuiltIn + let (user, group) = if config.mqtt.bridge.built_in { + ("tedge", "tedge") } else { - BridgeLocation::Mosquitto + (crate::BROKER_USER, crate::BROKER_USER) }; let cmd = match self { @@ -64,7 +63,8 @@ impl BuildCommand for TEdgeCertCli { id, cert_path: config.device.cert_path.clone(), key_path: config.device.key_path.clone(), - bridge_location, + user: user.to_owned(), + group: group.to_owned(), }; cmd.into_boxed() } @@ -76,7 +76,8 @@ impl BuildCommand for TEdgeCertCli { key_path: config.device.key_path.clone(), // Use output file instead of csr_path from tedge config if provided csr_path: output_path.unwrap_or_else(|| config.device.csr_path.clone()), - bridge_location, + user: user.to_owned(), + group: group.to_owned(), }; cmd.into_boxed() } @@ -122,7 +123,8 @@ impl BuildCommand for TEdgeCertCli { let cmd = RenewCertCmd { cert_path: config.device.cert_path.clone(), key_path: config.device.key_path.clone(), - bridge_location, + user: user.to_owned(), + group: group.to_owned(), }; cmd.into_boxed() } diff --git a/crates/core/tedge/src/cli/certificate/create.rs b/crates/core/tedge/src/cli/certificate/create.rs index 681d4181763..a5a8cb2d6a2 100644 --- a/crates/core/tedge/src/cli/certificate/create.rs +++ b/crates/core/tedge/src/cli/certificate/create.rs @@ -1,5 +1,4 @@ use super::error::CertError; -use crate::bridge::BridgeLocation; use crate::command::Command; use crate::log::MaybeFancy; use camino::Utf8PathBuf; @@ -25,8 +24,9 @@ pub struct CreateCertCmd { /// The path where the device private key will be stored pub key_path: Utf8PathBuf, - /// The component that is configured to host the MQTT bridge logic - pub bridge_location: BridgeLocation, + /// The owner of the private key + pub user: String, + pub group: String, } impl Command for CreateCertCmd { @@ -47,12 +47,22 @@ impl CreateCertCmd { let cert = KeyCertPair::new_selfsigned_certificate(config, &self.id, &KeyKind::New)?; let cert_path = &self.cert_path; - self.persist_new_public_key(cert_path, cert.certificate_pem_string()?) - .map_err(|err| err.cert_context(cert_path.clone()))?; + persist_new_public_key( + cert_path, + cert.certificate_pem_string()?, + &self.user, + &self.group, + ) + .map_err(|err| err.cert_context(cert_path.clone()))?; let key_path = &self.key_path; - self.persist_new_private_key(key_path, cert.private_key_pem_string()?) - .map_err(|err| err.key_context(key_path.clone()))?; + persist_new_private_key( + key_path, + cert.private_key_pem_string()?, + &self.user, + &self.group, + ) + .map_err(|err| err.key_context(key_path.clone()))?; Ok(()) } @@ -64,7 +74,7 @@ impl CreateCertCmd { .map_err(|e| CertError::IoError(e).key_context(key_path.clone()))?; let cert = KeyCertPair::new_selfsigned_certificate(config, &self.id, &previous_key)?; - self.override_public_key(cert_path, cert.certificate_pem_string()?) + override_public_key(cert_path, cert.certificate_pem_string()?) .map_err(|err| err.cert_context(cert_path.clone()))?; Ok(()) } @@ -80,54 +90,46 @@ impl CreateCertCmd { let cert = KeyCertPair::new_certificate_sign_request(config, &self.id, &previous_key)?; if let KeyKind::New = previous_key { - self.persist_new_private_key(key_path, cert.private_key_pem_string()?) - .map_err(|err| err.key_context(key_path.clone()))?; + persist_new_private_key( + key_path, + cert.private_key_pem_string()?, + &self.user, + &self.group, + ) + .map_err(|err| err.key_context(key_path.clone()))?; } - self.override_public_key(csr_path, cert.certificate_signing_request_string()?) + override_public_key(csr_path, cert.certificate_signing_request_string()?) .map_err(|err| err.cert_context(csr_path.clone()))?; Ok(()) } +} - fn persist_new_public_key( - &self, - cert_path: &Utf8PathBuf, - pem_string: String, - ) -> Result<(), CertError> { - let (user, group) = self.certificate_user_group(); - - validate_parent_dir_exists(cert_path).map_err(CertError::CertPathError)?; - persist_public_key(create_new_file(cert_path, user, group)?, pem_string)?; - Ok(()) - } - - fn persist_new_private_key( - &self, - key_path: &Utf8PathBuf, - key: certificate::Zeroizing, - ) -> Result<(), CertError> { - let (user, group) = self.certificate_user_group(); - - validate_parent_dir_exists(key_path).map_err(CertError::KeyPathError)?; - persist_private_key(create_new_file(key_path, user, group)?, key)?; - Ok(()) - } +fn persist_new_public_key( + cert_path: &Utf8PathBuf, + pem_string: String, + user: &str, + group: &str, +) -> Result<(), CertError> { + validate_parent_dir_exists(cert_path).map_err(CertError::CertPathError)?; + persist_public_key(create_new_file(cert_path, user, group)?, pem_string)?; + Ok(()) +} - fn override_public_key( - &self, - cert_path: &Utf8PathBuf, - pem_string: String, - ) -> Result<(), CertError> { - validate_parent_dir_exists(cert_path).map_err(CertError::CertPathError)?; - persist_public_key(override_file(cert_path)?, pem_string)?; - Ok(()) - } +fn persist_new_private_key( + key_path: &Utf8PathBuf, + key: certificate::Zeroizing, + user: &str, + group: &str, +) -> Result<(), CertError> { + validate_parent_dir_exists(key_path).map_err(CertError::KeyPathError)?; + persist_private_key(create_new_file(key_path, user, group)?, key)?; + Ok(()) +} - fn certificate_user_group(&self) -> (&str, &str) { - match self.bridge_location { - BridgeLocation::BuiltIn => ("tedge", "tedge"), - BridgeLocation::Mosquitto => (crate::BROKER_USER, crate::BROKER_GROUP), - } - } +fn override_public_key(cert_path: &Utf8PathBuf, pem_string: String) -> Result<(), CertError> { + validate_parent_dir_exists(cert_path).map_err(CertError::CertPathError)?; + persist_public_key(override_file(cert_path)?, pem_string)?; + Ok(()) } fn create_new_file( @@ -218,7 +220,8 @@ mod tests { id: String::from(id), cert_path: cert_path.clone(), key_path: key_path.clone(), - bridge_location: BridgeLocation::Mosquitto, + user: "mosquitto".to_string(), + group: "mosquitto".to_string(), }; assert_matches!( @@ -246,7 +249,8 @@ mod tests { id: "my-device-id".into(), cert_path: cert_path.clone(), key_path: key_path.clone(), - bridge_location: BridgeLocation::Mosquitto, + user: "mosquitto".to_string(), + group: "mosquitto".to_string(), }; assert!(cmd @@ -268,7 +272,8 @@ mod tests { id: "my-device-id".into(), cert_path, key_path, - bridge_location: BridgeLocation::Mosquitto, + user: "mosquitto".to_string(), + group: "mosquitto".to_string(), }; let cert_error = cmd @@ -287,7 +292,8 @@ mod tests { id: "my-device-id".into(), cert_path, key_path, - bridge_location: BridgeLocation::Mosquitto, + user: "mosquitto".to_string(), + group: "mosquitto".to_string(), }; let cert_error = cmd @@ -306,7 +312,8 @@ mod tests { id: "my-device-id".into(), cert_path, key_path, - bridge_location: BridgeLocation::Mosquitto, + user: "mosquitto".to_string(), + group: "mosquitto".to_string(), }; let cert_error = cmd diff --git a/crates/core/tedge/src/cli/certificate/create_csr.rs b/crates/core/tedge/src/cli/certificate/create_csr.rs index 6d3baebfda1..e14a38f35c1 100644 --- a/crates/core/tedge/src/cli/certificate/create_csr.rs +++ b/crates/core/tedge/src/cli/certificate/create_csr.rs @@ -1,6 +1,5 @@ use super::create::cn_of_self_signed_certificate; use super::error::CertError; -use crate::bridge::BridgeLocation; use crate::command::Command; use crate::log::MaybeFancy; use crate::CreateCertCmd; @@ -12,7 +11,8 @@ pub struct CreateCsrCmd { pub cert_path: Utf8PathBuf, pub key_path: Utf8PathBuf, pub csr_path: Utf8PathBuf, - pub bridge_location: BridgeLocation, + pub user: String, + pub group: String, } impl Command for CreateCsrCmd { @@ -43,7 +43,8 @@ impl CreateCsrCmd { id, cert_path: self.csr_path.clone(), key_path: self.key_path.clone(), - bridge_location: self.bridge_location, + user: self.user.clone(), + group: self.group.clone(), }; create_cmd.create_certificate_signing_request(config) @@ -73,7 +74,8 @@ mod tests { cert_path: cert_path.clone(), key_path: key_path.clone(), csr_path: csr_path.clone(), - bridge_location: BridgeLocation::Mosquitto, + user: "mosquitto".to_string(), + group: "mosquitto".to_string(), }; assert_matches!( @@ -97,7 +99,8 @@ mod tests { id: String::from(id), cert_path: cert_path.clone(), key_path: key_path.clone(), - bridge_location: BridgeLocation::Mosquitto, + user: "mosquitto".to_string(), + group: "mosquitto".to_string(), }; // create private key and public cert with standard command @@ -116,7 +119,8 @@ mod tests { cert_path: cert_path.clone(), key_path: key_path.clone(), csr_path: csr_path.clone(), - bridge_location: BridgeLocation::Mosquitto, + user: "mosquitto".to_string(), + group: "mosquitto".to_string(), }; // create csr using existing private key and device_id from public cert diff --git a/crates/core/tedge/src/cli/certificate/renew.rs b/crates/core/tedge/src/cli/certificate/renew.rs index 20fbac43435..719b5f834b7 100644 --- a/crates/core/tedge/src/cli/certificate/renew.rs +++ b/crates/core/tedge/src/cli/certificate/renew.rs @@ -1,6 +1,5 @@ use super::create::cn_of_self_signed_certificate; use super::error::CertError; -use crate::bridge::BridgeLocation; use crate::command::Command; use crate::log::MaybeFancy; use crate::CreateCertCmd; @@ -10,7 +9,8 @@ use certificate::NewCertificateConfig; pub struct RenewCertCmd { pub cert_path: Utf8PathBuf, pub key_path: Utf8PathBuf, - pub bridge_location: BridgeLocation, + pub user: String, + pub group: String, } impl Command for RenewCertCmd { @@ -39,7 +39,8 @@ impl RenewCertCmd { id, cert_path: self.cert_path.clone(), key_path: self.key_path.clone(), - bridge_location: self.bridge_location, + user: self.user.clone(), + group: self.group.clone(), }; create_cmd.renew_test_certificate(config) @@ -64,7 +65,8 @@ mod tests { id: String::from(id), cert_path: cert_path.clone(), key_path: key_path.clone(), - bridge_location: BridgeLocation::Mosquitto, + user: "mosquitto".to_string(), + group: "mosquitto".to_string(), }; // First create both cert and key @@ -83,7 +85,8 @@ mod tests { let cmd = RenewCertCmd { cert_path: cert_path.clone(), key_path: key_path.clone(), - bridge_location: BridgeLocation::Mosquitto, + user: "mosquitto".to_string(), + group: "mosquitto".to_string(), }; cmd.renew_test_certificate(&NewCertificateConfig::default()) .unwrap(); From 58f8dbba59ac120608cedc1b7dafa7e95db5bde9 Mon Sep 17 00:00:00 2001 From: Didier Wenzek Date: Tue, 26 Nov 2024 22:09:47 +0100 Subject: [PATCH 5/6] Fully untangle certificate request, creation and renewal Signed-off-by: Didier Wenzek --- crates/core/tedge/src/cli/certificate/cli.rs | 2 - .../core/tedge/src/cli/certificate/create.rs | 69 ++----------------- .../tedge/src/cli/certificate/create_csr.rs | 42 ++++++++--- .../core/tedge/src/cli/certificate/renew.rs | 49 +++++++++---- 4 files changed, 72 insertions(+), 90 deletions(-) diff --git a/crates/core/tedge/src/cli/certificate/cli.rs b/crates/core/tedge/src/cli/certificate/cli.rs index f61a01a6bb2..7c843d97346 100644 --- a/crates/core/tedge/src/cli/certificate/cli.rs +++ b/crates/core/tedge/src/cli/certificate/cli.rs @@ -123,8 +123,6 @@ impl BuildCommand for TEdgeCertCli { let cmd = RenewCertCmd { cert_path: config.device.cert_path.clone(), key_path: config.device.key_path.clone(), - user: user.to_owned(), - group: group.to_owned(), }; cmd.into_boxed() } diff --git a/crates/core/tedge/src/cli/certificate/create.rs b/crates/core/tedge/src/cli/certificate/create.rs index a5a8cb2d6a2..7deaa148122 100644 --- a/crates/core/tedge/src/cli/certificate/create.rs +++ b/crates/core/tedge/src/cli/certificate/create.rs @@ -13,12 +13,12 @@ use std::path::Path; use tedge_utils::paths::set_permission; use tedge_utils::paths::validate_parent_dir_exists; -/// Create self-signed device certificate and signing request +/// Create self-signed device certificate pub struct CreateCertCmd { /// The device identifier pub id: String, - /// The path where the device certificate / request will be stored + /// The path where the device certificate will be stored pub cert_path: Utf8PathBuf, /// The path where the device private key will be stored @@ -65,46 +65,9 @@ impl CreateCertCmd { .map_err(|err| err.key_context(key_path.clone()))?; Ok(()) } - - pub fn renew_test_certificate(&self, config: &NewCertificateConfig) -> Result<(), CertError> { - let cert_path = &self.cert_path; - let key_path = &self.key_path; - - let previous_key = reuse_private_key(key_path) - .map_err(|e| CertError::IoError(e).key_context(key_path.clone()))?; - let cert = KeyCertPair::new_selfsigned_certificate(config, &self.id, &previous_key)?; - - override_public_key(cert_path, cert.certificate_pem_string()?) - .map_err(|err| err.cert_context(cert_path.clone()))?; - Ok(()) - } - - pub fn create_certificate_signing_request( - &self, - config: &NewCertificateConfig, - ) -> Result<(), CertError> { - let csr_path = &self.cert_path; - let key_path = &self.key_path; - - let previous_key = reuse_private_key(key_path).unwrap_or(KeyKind::New); - let cert = KeyCertPair::new_certificate_sign_request(config, &self.id, &previous_key)?; - - if let KeyKind::New = previous_key { - persist_new_private_key( - key_path, - cert.private_key_pem_string()?, - &self.user, - &self.group, - ) - .map_err(|err| err.key_context(key_path.clone()))?; - } - override_public_key(csr_path, cert.certificate_signing_request_string()?) - .map_err(|err| err.cert_context(csr_path.clone()))?; - Ok(()) - } } -fn persist_new_public_key( +pub fn persist_new_public_key( cert_path: &Utf8PathBuf, pem_string: String, user: &str, @@ -115,7 +78,7 @@ fn persist_new_public_key( Ok(()) } -fn persist_new_private_key( +pub fn persist_new_private_key( key_path: &Utf8PathBuf, key: certificate::Zeroizing, user: &str, @@ -126,7 +89,7 @@ fn persist_new_private_key( Ok(()) } -fn override_public_key(cert_path: &Utf8PathBuf, pem_string: String) -> Result<(), CertError> { +pub fn override_public_key(cert_path: &Utf8PathBuf, pem_string: String) -> Result<(), CertError> { validate_parent_dir_exists(cert_path).map_err(CertError::CertPathError)?; persist_public_key(override_file(cert_path)?, pem_string)?; Ok(()) @@ -158,7 +121,7 @@ fn override_file(path: impl AsRef) -> Result { .open(path.as_ref()) } -fn reuse_private_key(key_path: &Utf8PathBuf) -> Result { +pub fn reuse_private_key(key_path: &Utf8PathBuf) -> Result { std::fs::read_to_string(key_path).map(|keypair_pem| KeyKind::Reuse { keypair_pem }) } @@ -302,26 +265,6 @@ mod tests { assert_matches!(cert_error, CertError::KeyPathError { .. }); } - #[test] - fn renew_certificate_without_key() { - let dir = tempdir().unwrap(); - let cert_path = temp_file_path(&dir, "my-device-cert.pem"); - let key_path = Utf8PathBuf::from("/non/existent/key/path"); - - let cmd = CreateCertCmd { - id: "my-device-id".into(), - cert_path, - key_path, - user: "mosquitto".to_string(), - group: "mosquitto".to_string(), - }; - - let cert_error = cmd - .renew_test_certificate(&NewCertificateConfig::default()) - .unwrap_err(); - assert_matches!(cert_error, CertError::KeyNotFound { .. }); - } - fn temp_file_path(dir: &TempDir, filename: &str) -> Utf8PathBuf { dir.path().join(filename).try_into().unwrap() } diff --git a/crates/core/tedge/src/cli/certificate/create_csr.rs b/crates/core/tedge/src/cli/certificate/create_csr.rs index e14a38f35c1..f2f6c52b879 100644 --- a/crates/core/tedge/src/cli/certificate/create_csr.rs +++ b/crates/core/tedge/src/cli/certificate/create_csr.rs @@ -2,15 +2,27 @@ use super::create::cn_of_self_signed_certificate; use super::error::CertError; use crate::command::Command; use crate::log::MaybeFancy; -use crate::CreateCertCmd; +use crate::override_public_key; +use crate::persist_new_private_key; +use crate::reuse_private_key; use camino::Utf8PathBuf; +use certificate::KeyCertPair; +use certificate::KeyKind; use certificate::NewCertificateConfig; +/// Create a certificate signing request (CSR) pub struct CreateCsrCmd { + /// The device identifier (either explicitly given or extracted from a previous certificate) pub id: Option, pub cert_path: Utf8PathBuf, + + /// The path where the device private key will be stored pub key_path: Utf8PathBuf, + + /// The path where the device CSR will be stored pub csr_path: Utf8PathBuf, + + /// The owner of the private key pub user: String, pub group: String, } @@ -38,16 +50,24 @@ impl CreateCsrCmd { Some(id) => id.clone(), None => cn_of_self_signed_certificate(&self.cert_path)?, }; - - let create_cmd = CreateCertCmd { - id, - cert_path: self.csr_path.clone(), - key_path: self.key_path.clone(), - user: self.user.clone(), - group: self.group.clone(), - }; - - create_cmd.create_certificate_signing_request(config) + let csr_path = &self.csr_path; + let key_path = &self.key_path; + + let previous_key = reuse_private_key(key_path).unwrap_or(KeyKind::New); + let cert = KeyCertPair::new_certificate_sign_request(config, &id, &previous_key)?; + + if let KeyKind::New = previous_key { + persist_new_private_key( + key_path, + cert.private_key_pem_string()?, + &self.user, + &self.group, + ) + .map_err(|err| err.key_context(key_path.clone()))?; + } + override_public_key(csr_path, cert.certificate_signing_request_string()?) + .map_err(|err| err.cert_context(csr_path.clone()))?; + Ok(()) } } diff --git a/crates/core/tedge/src/cli/certificate/renew.rs b/crates/core/tedge/src/cli/certificate/renew.rs index 719b5f834b7..6763b79b90c 100644 --- a/crates/core/tedge/src/cli/certificate/renew.rs +++ b/crates/core/tedge/src/cli/certificate/renew.rs @@ -2,15 +2,19 @@ use super::create::cn_of_self_signed_certificate; use super::error::CertError; use crate::command::Command; use crate::log::MaybeFancy; -use crate::CreateCertCmd; +use crate::override_public_key; +use crate::reuse_private_key; use camino::Utf8PathBuf; +use certificate::KeyCertPair; use certificate::NewCertificateConfig; +/// Renew the self-signed device certificate pub struct RenewCertCmd { + /// The path of the certificate to be updated pub cert_path: Utf8PathBuf, + + /// The path of the private key to re-use pub key_path: Utf8PathBuf, - pub user: String, - pub group: String, } impl Command for RenewCertCmd { @@ -28,28 +32,30 @@ impl Command for RenewCertCmd { impl RenewCertCmd { fn renew_test_certificate(&self, config: &NewCertificateConfig) -> Result<(), CertError> { - let id = cn_of_self_signed_certificate(&self.cert_path)?; + let cert_path = &self.cert_path; + let key_path = &self.key_path; + let id = cn_of_self_signed_certificate(cert_path)?; // Remove only certificate std::fs::remove_file(&self.cert_path) .map_err(|e| CertError::IoError(e).cert_context(self.cert_path.clone()))?; // Re-create the certificate from the key, with new validity - let create_cmd = CreateCertCmd { - id, - cert_path: self.cert_path.clone(), - key_path: self.key_path.clone(), - user: self.user.clone(), - group: self.group.clone(), - }; + let previous_key = reuse_private_key(key_path) + .map_err(|e| CertError::IoError(e).key_context(key_path.clone()))?; + let cert = KeyCertPair::new_selfsigned_certificate(config, &id, &previous_key)?; - create_cmd.renew_test_certificate(config) + override_public_key(cert_path, cert.certificate_pem_string()?) + .map_err(|err| err.cert_context(cert_path.clone()))?; + Ok(()) } } #[cfg(test)] mod tests { use super::*; + use crate::CreateCertCmd; + use assert_matches::assert_matches; use std::path::Path; use std::thread::sleep; use std::time::Duration; @@ -85,8 +91,6 @@ mod tests { let cmd = RenewCertCmd { cert_path: cert_path.clone(), key_path: key_path.clone(), - user: "mosquitto".to_string(), - group: "mosquitto".to_string(), }; cmd.renew_test_certificate(&NewCertificateConfig::default()) .unwrap(); @@ -112,6 +116,23 @@ mod tests { ); } + #[test] + fn renew_certificate_without_key() { + let dir = tempdir().unwrap(); + let cert_path = temp_file_path(&dir, "my-device-cert.pem"); + let key_path = Utf8PathBuf::from("/non/existent/key/path"); + + let cmd = RenewCertCmd { + cert_path, + key_path, + }; + + let cert_error = cmd + .renew_test_certificate(&NewCertificateConfig::default()) + .unwrap_err(); + assert_matches!(cert_error, CertError::CertificateNotFound { .. }); + } + fn temp_file_path(dir: &TempDir, filename: &str) -> Utf8PathBuf { dir.path().join(filename).try_into().unwrap() } From 88bcaad71273c876f1a91946aa09497a991ae252 Mon Sep 17 00:00:00 2001 From: Didier Wenzek Date: Wed, 27 Nov 2024 15:34:53 +0100 Subject: [PATCH 6/6] Make CreateCsrCmd fields less confusing Two fields were used to pass a device id to a CSR command, one being only used if the other was not suitable. Now the device id is provided by the caller. Signed-off-by: Didier Wenzek --- crates/core/tedge/src/cli/certificate/cli.rs | 6 +++++- .../tedge/src/cli/certificate/create_csr.rs | 21 ++++++------------- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/crates/core/tedge/src/cli/certificate/cli.rs b/crates/core/tedge/src/cli/certificate/cli.rs index 7c843d97346..44c25056ba0 100644 --- a/crates/core/tedge/src/cli/certificate/cli.rs +++ b/crates/core/tedge/src/cli/certificate/cli.rs @@ -70,9 +70,13 @@ impl BuildCommand for TEdgeCertCli { } TEdgeCertCli::CreateCsr { id, output_path } => { + // Use the current device id if no id is provided + let id = match id { + Some(id) => id, + None => config.device.id.try_read(&config)?.clone(), + }; let cmd = CreateCsrCmd { id, - cert_path: config.device.cert_path.clone(), key_path: config.device.key_path.clone(), // Use output file instead of csr_path from tedge config if provided csr_path: output_path.unwrap_or_else(|| config.device.csr_path.clone()), diff --git a/crates/core/tedge/src/cli/certificate/create_csr.rs b/crates/core/tedge/src/cli/certificate/create_csr.rs index f2f6c52b879..73c9468075e 100644 --- a/crates/core/tedge/src/cli/certificate/create_csr.rs +++ b/crates/core/tedge/src/cli/certificate/create_csr.rs @@ -1,4 +1,3 @@ -use super::create::cn_of_self_signed_certificate; use super::error::CertError; use crate::command::Command; use crate::log::MaybeFancy; @@ -12,9 +11,8 @@ use certificate::NewCertificateConfig; /// Create a certificate signing request (CSR) pub struct CreateCsrCmd { - /// The device identifier (either explicitly given or extracted from a previous certificate) - pub id: Option, - pub cert_path: Utf8PathBuf, + /// The device identifier + pub id: String, /// The path where the device private key will be stored pub key_path: Utf8PathBuf, @@ -45,16 +43,12 @@ impl CreateCsrCmd { &self, config: &NewCertificateConfig, ) -> Result<(), CertError> { - // Use id of public certificate if not provided - let id = match &self.id { - Some(id) => id.clone(), - None => cn_of_self_signed_certificate(&self.cert_path)?, - }; + let id = &self.id; let csr_path = &self.csr_path; let key_path = &self.key_path; let previous_key = reuse_private_key(key_path).unwrap_or(KeyKind::New); - let cert = KeyCertPair::new_certificate_sign_request(config, &id, &previous_key)?; + let cert = KeyCertPair::new_certificate_sign_request(config, id, &previous_key)?; if let KeyKind::New = previous_key { persist_new_private_key( @@ -84,14 +78,12 @@ mod tests { #[test] fn create_signing_request_when_private_key_does_not_exist() { let dir = tempdir().unwrap(); - let cert_path = temp_file_path(&dir, "my-device-cert.pem"); let key_path = temp_file_path(&dir, "my-device-key.pem"); let csr_path = temp_file_path(&dir, "my-device-csr.csr"); let id = "my-device-id"; let cmd = CreateCsrCmd { - id: Some(String::from(id)), - cert_path: cert_path.clone(), + id: id.to_string(), key_path: key_path.clone(), csr_path: csr_path.clone(), user: "mosquitto".to_string(), @@ -135,8 +127,7 @@ mod tests { let first_x509_cert = first_pem.parse_x509().expect("X.509: decoding DER failed"); let cmd = CreateCsrCmd { - id: Some(String::from(id)), - cert_path: cert_path.clone(), + id: id.to_string(), key_path: key_path.clone(), csr_path: csr_path.clone(), user: "mosquitto".to_string(),