From a175eaac7bbaf2ddf2c793aba1bc89aa0f8b03d8 Mon Sep 17 00:00:00 2001 From: Albin Suresh Date: Mon, 18 Mar 2024 16:49:45 +0000 Subject: [PATCH 1/2] fix: Honor c8y.mqtt setting on tedge connect #2787 --- .../src/tedge_config_cli/error.rs | 3 ++ .../src/tedge_config_cli/models/host_port.rs | 33 +++++++++---- .../src/tedge_config_cli/models/port.rs | 12 ++--- crates/core/c8y_api/src/http_proxy.rs | 28 ++++++++++- crates/core/tedge/src/bridge/aws.rs | 17 +++---- crates/core/tedge/src/bridge/azure.rs | 19 ++++---- crates/core/tedge/src/bridge/c8y.rs | 10 ++-- crates/core/tedge/src/bridge/config.rs | 46 +++++-------------- .../core/tedge/src/cli/certificate/upload.rs | 4 +- .../src/cli/connect/c8y_direct_connection.rs | 7 ++- crates/core/tedge/src/cli/connect/command.rs | 12 +++-- 11 files changed, 104 insertions(+), 87 deletions(-) diff --git a/crates/common/tedge_config/src/tedge_config_cli/error.rs b/crates/common/tedge_config/src/tedge_config_cli/error.rs index 543662f2dc6..85d05e00207 100644 --- a/crates/common/tedge_config/src/tedge_config_cli/error.rs +++ b/crates/common/tedge_config/src/tedge_config_cli/error.rs @@ -29,6 +29,9 @@ pub enum TEdgeConfigError { #[error(transparent)] DirNotFound(#[from] tedge_utils::paths::PathsError), + + #[error(transparent)] + FromParseHostPortError(#[from] crate::tedge_config_cli::models::host_port::ParseHostPortError), } impl TEdgeConfigError { diff --git a/crates/common/tedge_config/src/tedge_config_cli/models/host_port.rs b/crates/common/tedge_config/src/tedge_config_cli/models/host_port.rs index 57e98d05c13..a6e9957ce13 100644 --- a/crates/common/tedge_config/src/tedge_config_cli/models/host_port.rs +++ b/crates/common/tedge_config/src/tedge_config_cli/models/host_port.rs @@ -58,14 +58,6 @@ impl HostPort

{ pub fn port(&self) -> Port { self.port } - - /// Returns a string representation of the host. - /// - /// In practice, it just returns the input string used to construct the - /// struct. - pub fn as_str(&self) -> &str { - &self.input - } } impl From> for String { @@ -76,7 +68,7 @@ impl From> for String { impl fmt::Display for HostPort

{ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.input.fmt(f) + write!(f, "{}:{}", self.hostname, self.port) } } @@ -115,6 +107,14 @@ impl TryFrom for HostPort

{ } } +impl TryFrom<&str> for HostPort

{ + type Error = ParseHostPortError; + + fn try_from(value: &str) -> Result { + Self::try_from(value.to_owned()) + } +} + impl From for HostPort

{ fn from(value: ConnectUrl) -> Self { HostPort { @@ -138,3 +138,18 @@ pub enum ParseHostPortError { #[error("Could not parse port")] ParsePort(#[from] ParseIntError), } + +#[cfg(test)] +mod tests { + use super::*; + use crate::HTTPS_PORT; + use crate::MQTT_TLS_PORT; + use test_case::test_case; + + #[test_case(HostPort::::try_from("test.com").unwrap(), "test.com:443")] + #[test_case(HostPort::::try_from("test.com").unwrap(), "test.com:8883")] + #[test_case(HostPort::::try_from("test.com:8443").unwrap(), "test.com:8443")] + fn to_string(input: HostPort

, expected: &str) { + assert_eq!(input.to_string(), expected); + } +} diff --git a/crates/common/tedge_config/src/tedge_config_cli/models/port.rs b/crates/common/tedge_config/src/tedge_config_cli/models/port.rs index 60fbc932804..079b082e88e 100644 --- a/crates/common/tedge_config/src/tedge_config_cli/models/port.rs +++ b/crates/common/tedge_config/src/tedge_config_cli/models/port.rs @@ -1,5 +1,5 @@ use std::convert::TryFrom; -use std::convert::TryInto; +use std::fmt::Display; #[derive(Debug, Copy, Clone, Eq, PartialEq)] pub struct Port(pub u16); @@ -22,11 +22,9 @@ impl TryFrom for Port { } } -impl TryInto for Port { - type Error = std::convert::Infallible; - - fn try_into(self) -> Result { - Ok(format!("{}", self.0)) +impl Display for Port { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.0.fmt(f) } } @@ -53,5 +51,5 @@ fn conversion_from_longer_integer_fails() { #[test] fn conversion_from_port_to_string() { - assert_matches!(TryInto::::try_into(Port(1234)), Ok(port_str) if port_str == "1234"); + assert_eq!(Port(1234).to_string(), "1234"); } diff --git a/crates/core/c8y_api/src/http_proxy.rs b/crates/core/c8y_api/src/http_proxy.rs index 7742b4fcb41..4a3a8dae5a9 100644 --- a/crates/core/c8y_api/src/http_proxy.rs +++ b/crates/core/c8y_api/src/http_proxy.rs @@ -101,12 +101,19 @@ impl C8yEndPoint { // * . eg: t12345.c8y.io // These URLs may be both equivalent and point to the same tenant. // We are going to remove that and only check if the domain is the same. - let tenant_uri = &self.c8y_host; + let (tenant_host, _port) = self + .c8y_host + .split_once(':') + .unwrap_or((&self.c8y_host, "")); let url = Url::parse(url).ok()?; let url_host = url.domain()?; let (_, host) = url_host.split_once('.').unwrap_or((url_host, "")); - let (_, c8y_host) = tenant_uri.split_once('.').unwrap(); + let (_, c8y_host) = tenant_host.split_once('.').unwrap_or((tenant_host, "")); + + // The configured `c8y.http` setting may have a port value specified, + // but the incoming URL is less likely to have any port specified. + // Hence just matching the host prefix. (host == c8y_host).then_some(url) } } @@ -237,11 +244,28 @@ mod tests { #[test_case("http://test.co.te")] #[test_case("http://test.com:123456")] #[test_case("http://test.com::12345")] + #[test_case("http://localhost")] + #[test_case("http://abc.com")] fn url_is_my_tenant_incorrect_urls(url: &str) { let c8y = C8yEndPoint::new("test.test.com", "test_device"); assert!(c8y.maybe_tenant_url(url).is_none()); } + #[test] + fn url_is_my_tenant_with_hostname_without_commas() { + let c8y = C8yEndPoint::new("custom-domain", "test_device"); + let url = "http://custom-domain/path"; + assert_eq!(c8y.maybe_tenant_url(url), Some(url.parse().unwrap())); + } + + #[ignore = "Until #2804 is fixed"] + #[test] + fn url_is_my_tenant_check_not_too_broad() { + let c8y = C8yEndPoint::new("abc.com", "test_device"); + dbg!(c8y.maybe_tenant_url("http://xyz.com")); + assert!(c8y.maybe_tenant_url("http://xyz.com").is_none()); + } + #[test] fn check_non_cached_internal_id_for_a_device() { let mut c8y = C8yEndPoint::new("test_host", "test_device"); diff --git a/crates/core/tedge/src/bridge/aws.rs b/crates/core/tedge/src/bridge/aws.rs index 4bc6cc53f97..4c3df7b7f49 100644 --- a/crates/core/tedge/src/bridge/aws.rs +++ b/crates/core/tedge/src/bridge/aws.rs @@ -1,14 +1,14 @@ use super::BridgeConfig; use crate::bridge::config::BridgeLocation; use camino::Utf8PathBuf; -use tedge_config::ConnectUrl; +use tedge_config::HostPort; +use tedge_config::MQTT_TLS_PORT; const MOSQUITTO_BRIDGE_TOPIC: &str = "te/device/main/service/mosquitto-aws-bridge/status/health"; #[derive(Debug, Eq, PartialEq)] pub struct BridgeConfigAwsParams { - pub connect_url: ConnectUrl, - pub mqtt_tls_port: u16, + pub mqtt_host: HostPort, pub config_file: String, pub remote_clientid: String, pub bridge_root_cert_path: Utf8PathBuf, @@ -19,8 +19,7 @@ pub struct BridgeConfigAwsParams { impl From for BridgeConfig { fn from(params: BridgeConfigAwsParams) -> Self { let BridgeConfigAwsParams { - connect_url, - mqtt_tls_port, + mqtt_host, config_file, bridge_root_cert_path, remote_clientid, @@ -28,7 +27,6 @@ impl From for BridgeConfig { bridge_keyfile, } = params; - let address = format!("{}:{}", connect_url, mqtt_tls_port); let user_name = remote_clientid.to_string(); // telemetry/command topics for use by the user @@ -50,7 +48,7 @@ impl From for BridgeConfig { cloud_name: "aws".into(), config_file, connection: "edge_to_aws".into(), - address, + address: mqtt_host, remote_username: Some(user_name), bridge_root_cert_path, remote_clientid, @@ -86,8 +84,7 @@ fn test_bridge_config_from_aws_params() -> anyhow::Result<()> { use std::convert::TryFrom; let params = BridgeConfigAwsParams { - connect_url: ConnectUrl::try_from("test.test.io")?, - mqtt_tls_port: 8883, + mqtt_host: HostPort::::try_from("test.test.io")?, config_file: "aws-bridge.conf".into(), remote_clientid: "alpha".into(), bridge_root_cert_path: "./test_root.pem".into(), @@ -101,7 +98,7 @@ fn test_bridge_config_from_aws_params() -> anyhow::Result<()> { cloud_name: "aws".into(), config_file: "aws-bridge.conf".to_string(), connection: "edge_to_aws".into(), - address: "test.test.io:8883".into(), + address: HostPort::::try_from("test.test.io")?, remote_username: Some("alpha".into()), bridge_root_cert_path: Utf8PathBuf::from("./test_root.pem"), remote_clientid: "alpha".into(), diff --git a/crates/core/tedge/src/bridge/azure.rs b/crates/core/tedge/src/bridge/azure.rs index a4db45b5bc6..2b3e3ab8d41 100644 --- a/crates/core/tedge/src/bridge/azure.rs +++ b/crates/core/tedge/src/bridge/azure.rs @@ -1,14 +1,14 @@ use super::BridgeConfig; use crate::bridge::config::BridgeLocation; use camino::Utf8PathBuf; -use tedge_config::ConnectUrl; +use tedge_config::HostPort; +use tedge_config::MQTT_TLS_PORT; const MOSQUITTO_BRIDGE_TOPIC: &str = "te/device/main/service/mosquitto-az-bridge/status/health"; #[derive(Debug, Eq, PartialEq)] pub struct BridgeConfigAzureParams { - pub connect_url: ConnectUrl, - pub mqtt_tls_port: u16, + pub mqtt_host: HostPort, pub config_file: String, pub remote_clientid: String, pub bridge_root_cert_path: Utf8PathBuf, @@ -19,8 +19,7 @@ pub struct BridgeConfigAzureParams { impl From for BridgeConfig { fn from(params: BridgeConfigAzureParams) -> Self { let BridgeConfigAzureParams { - connect_url, - mqtt_tls_port, + mqtt_host, config_file, bridge_root_cert_path, remote_clientid, @@ -28,10 +27,11 @@ impl From for BridgeConfig { bridge_keyfile, } = params; - let address = format!("{}:{}", connect_url, mqtt_tls_port); + let address = mqtt_host.clone(); let user_name = format!( "{}/{}/?api-version=2018-06-30", - connect_url, remote_clientid + mqtt_host.host(), + remote_clientid ); let pub_msg_topic = format!("messages/events/# out 1 az/ devices/{}/", remote_clientid); let sub_msg_topic = format!( @@ -84,8 +84,7 @@ fn test_bridge_config_from_azure_params() -> anyhow::Result<()> { use std::convert::TryFrom; let params = BridgeConfigAzureParams { - connect_url: ConnectUrl::try_from("test.test.io")?, - mqtt_tls_port: 8883, + mqtt_host: HostPort::::try_from("test.test.io")?, config_file: "az-bridge.conf".into(), remote_clientid: "alpha".into(), bridge_root_cert_path: "./test_root.pem".into(), @@ -99,7 +98,7 @@ fn test_bridge_config_from_azure_params() -> anyhow::Result<()> { cloud_name: "az".into(), config_file: "az-bridge.conf".to_string(), connection: "edge_to_az".into(), - address: "test.test.io:8883".into(), + address: HostPort::::try_from("test.test.io")?, remote_username: Some("test.test.io/alpha/?api-version=2018-06-30".into()), bridge_root_cert_path: Utf8PathBuf::from("./test_root.pem"), remote_clientid: "alpha".into(), diff --git a/crates/core/tedge/src/bridge/c8y.rs b/crates/core/tedge/src/bridge/c8y.rs index f4928b53dce..e319f67fdc2 100644 --- a/crates/core/tedge/src/bridge/c8y.rs +++ b/crates/core/tedge/src/bridge/c8y.rs @@ -93,11 +93,7 @@ impl From for BridgeConfig { cloud_name: "c8y".into(), config_file, connection: "edge_to_c8y".into(), - address: format!( - "{host}:{port}", - host = mqtt_host.host(), - port = mqtt_host.port().0 - ), + address: mqtt_host, remote_username: None, bridge_root_cert_path, remote_clientid, @@ -159,7 +155,7 @@ mod tests { fn test_bridge_config_from_c8y_params() -> anyhow::Result<()> { use std::convert::TryFrom; let params = BridgeConfigC8yParams { - mqtt_host: HostPort::::try_from("test.test.io".to_string())?, + mqtt_host: HostPort::::try_from("test.test.io")?, config_file: C8Y_CONFIG_FILENAME.into(), remote_clientid: "alpha".into(), bridge_root_cert_path: Utf8PathBuf::from("./test_root.pem"), @@ -176,7 +172,7 @@ mod tests { cloud_name: "c8y".into(), config_file: C8Y_CONFIG_FILENAME.into(), connection: "edge_to_c8y".into(), - address: "test.test.io:8883".into(), + address: HostPort::::try_from("test.test.io")?, remote_username: None, bridge_root_cert_path: Utf8PathBuf::from("./test_root.pem"), remote_clientid: "alpha".into(), diff --git a/crates/core/tedge/src/bridge/config.rs b/crates/core/tedge/src/bridge/config.rs index 7bcf039490f..aca84e23766 100644 --- a/crates/core/tedge/src/bridge/config.rs +++ b/crates/core/tedge/src/bridge/config.rs @@ -1,7 +1,8 @@ use camino::Utf8PathBuf; +use tedge_config::HostPort; use tedge_config::TEdgeConfigLocation; +use tedge_config::MQTT_TLS_PORT; use tedge_utils::paths::DraftFile; -use url::Url; use crate::ConnectError; @@ -13,7 +14,7 @@ pub struct BridgeConfig { // XXX: having file name squished together with 20 fields which go into file content is a bit obscure pub config_file: String, pub connection: String, - pub address: String, + pub address: HostPort, pub remote_username: Option, pub bridge_root_cert_path: Utf8PathBuf, pub remote_clientid: String, @@ -91,10 +92,6 @@ impl BridgeConfig { } pub fn validate(&self) -> Result<(), ConnectError> { - // XXX: This is actually wrong. Our address looks like this: `domain:port` - // `Url::parse` will treat `domain` as `schema` ... - Url::parse(&self.address)?; - if !self.bridge_root_cert_path.exists() { return Err(ConnectError::Certificate); } @@ -141,6 +138,8 @@ impl BridgeConfig { #[cfg(test)] mod test { + use std::str::FromStr; + use super::*; use camino::Utf8Path; use camino::Utf8PathBuf; @@ -154,7 +153,7 @@ mod test { cloud_name: "test".into(), config_file: "test-bridge.conf".into(), connection: "edge_to_test".into(), - address: "test.test.io:8883".into(), + address: HostPort::::try_from("test.test.io:8883")?, remote_username: None, bridge_root_cert_path: bridge_root_cert_path.to_owned(), remote_clientid: "alpha".into(), @@ -220,7 +219,7 @@ bridge_attempt_unsubscribe false cloud_name: "test".into(), config_file: "test-bridge.conf".into(), connection: "edge_to_test".into(), - address: "test.test.io:8883".into(), + address: HostPort::::try_from("test.test.io:8883")?, remote_username: None, bridge_root_cert_path: bridge_root_cert_path.to_owned(), remote_clientid: "alpha".into(), @@ -285,7 +284,7 @@ bridge_attempt_unsubscribe false cloud_name: "az".into(), config_file: "az-bridge.conf".into(), connection: "edge_to_az".into(), - address: "test.test.io:8883".into(), + address: HostPort::::try_from("test.test.io:8883")?, remote_username: Some("test.test.io/alpha/?api-version=2018-06-30".into()), bridge_root_cert_path: bridge_root_cert_path.to_owned(), remote_clientid: "alpha".into(), @@ -355,10 +354,8 @@ bridge_attempt_unsubscribe false let key_file = tempfile::NamedTempFile::new()?; let bridge_keyfile = Utf8Path::from_path(key_file.path()).unwrap().to_owned(); - let correct_url = "http://test.com"; - let config = BridgeConfig { - address: correct_url.into(), + address: HostPort::::try_from("test.test.io:8883".to_string())?, bridge_root_cert_path: bridge_ca_path.to_owned(), bridge_certfile, bridge_keyfile, @@ -370,30 +367,12 @@ bridge_attempt_unsubscribe false Ok(()) } - // XXX: This test is flawed as it is not clear what it tests. - // It can fail due to either `incorrect_url` OR `non_existent_path`. - #[test] - fn test_validate_wrong_url() { - let incorrect_url = "noturl"; - let non_existent_path = Utf8PathBuf::from("/path/that/does/not/exist"); - - let config = BridgeConfig { - address: incorrect_url.into(), - bridge_certfile: non_existent_path.clone(), - bridge_keyfile: non_existent_path, - ..default_bridge_config() - }; - - assert!(config.validate().is_err()); - } - #[test] fn test_validate_wrong_cert_path() { - let correct_url = "http://test.com"; let non_existent_path = Utf8PathBuf::from("/path/that/does/not/exist"); let config = BridgeConfig { - address: correct_url.into(), + address: HostPort::::try_from("test.com").unwrap(), bridge_certfile: non_existent_path.clone(), bridge_keyfile: non_existent_path, ..default_bridge_config() @@ -406,11 +385,10 @@ bridge_attempt_unsubscribe false fn test_validate_wrong_key_path() -> anyhow::Result<()> { let cert_file = tempfile::NamedTempFile::new()?; let bridge_certfile = Utf8Path::from_path(cert_file.path()).unwrap().to_owned(); - let correct_url = "http://test.com"; let non_existent_path = "/path/that/does/not/exist"; let config = BridgeConfig { - address: correct_url.into(), + address: HostPort::::try_from("test.com".to_string())?, bridge_certfile, bridge_keyfile: non_existent_path.into(), ..default_bridge_config() @@ -426,7 +404,7 @@ bridge_attempt_unsubscribe false cloud_name: "az/c8y".into(), config_file: "cfg".to_string(), connection: "edge_to_az/c8y".into(), - address: "".into(), + address: HostPort::::from_str("test.com").unwrap(), remote_username: None, bridge_root_cert_path: "".into(), bridge_certfile: "".into(), diff --git a/crates/core/tedge/src/cli/certificate/upload.rs b/crates/core/tedge/src/cli/certificate/upload.rs index e0ba36623ae..a44e17226a8 100644 --- a/crates/core/tedge/src/cli/certificate/upload.rs +++ b/crates/core/tedge/src/cli/certificate/upload.rs @@ -82,7 +82,7 @@ impl UploadCertCmd { // and therefore we need to get tenant_id. let tenant_id = get_tenant_id_blocking( &client, - build_get_tenant_id_url(self.host.as_str())?, + build_get_tenant_id_url(&self.host.to_string())?, &self.username, &password, )?; @@ -95,7 +95,7 @@ impl UploadCertCmd { tenant_id: &str, password: &str, ) -> Result<(), CertError> { - let post_url = build_upload_certificate_url(self.host.as_str(), tenant_id)?; + let post_url = build_upload_certificate_url(&self.host.to_string(), tenant_id)?; let post_body = UploadCertBody { auto_registration_enabled: true, diff --git a/crates/core/tedge/src/cli/connect/c8y_direct_connection.rs b/crates/core/tedge/src/cli/connect/c8y_direct_connection.rs index 62e828c30bd..2ec97bfd285 100644 --- a/crates/core/tedge/src/cli/connect/c8y_direct_connection.rs +++ b/crates/core/tedge/src/cli/connect/c8y_direct_connection.rs @@ -25,9 +25,12 @@ pub fn create_device_with_direct_connection( const DEVICE_CREATE_ERROR_TOPIC: &str = "s/e"; let address = bridge_config.address.clone(); - let host: Vec<&str> = address.split(':').collect(); - let mut mqtt_options = MqttOptions::new(bridge_config.remote_clientid.clone(), host[0], 8883); + let mut mqtt_options = MqttOptions::new( + bridge_config.remote_clientid.clone(), + address.host().to_string(), + address.port().into(), + ); mqtt_options.set_keep_alive(std::time::Duration::from_secs(5)); let tls_config = create_tls_config( diff --git a/crates/core/tedge/src/cli/connect/command.rs b/crates/core/tedge/src/cli/connect/command.rs index a9145f98b18..a3e8de91061 100644 --- a/crates/core/tedge/src/cli/connect/command.rs +++ b/crates/core/tedge/src/cli/connect/command.rs @@ -187,8 +187,10 @@ pub fn bridge_config( match cloud { Cloud::Azure => { let params = BridgeConfigAzureParams { - connect_url: config.az.url.or_config_not_set()?.clone(), - mqtt_tls_port: MQTT_TLS_PORT, + mqtt_host: HostPort::::try_from( + config.az.url.or_config_not_set()?.as_str(), + ) + .map_err(TEdgeConfigError::from)?, config_file: AZURE_CONFIG_FILENAME.into(), bridge_root_cert_path: config.az.root_cert_path.clone(), remote_clientid: config.device.id.try_read(config)?.clone(), @@ -200,8 +202,10 @@ pub fn bridge_config( } Cloud::Aws => { let params = BridgeConfigAwsParams { - connect_url: config.aws.url.or_config_not_set()?.clone(), - mqtt_tls_port: MQTT_TLS_PORT, + mqtt_host: HostPort::::try_from( + config.aws.url.or_config_not_set()?.as_str(), + ) + .map_err(TEdgeConfigError::from)?, config_file: AWS_CONFIG_FILENAME.into(), bridge_root_cert_path: config.aws.root_cert_path.clone(), remote_clientid: config.device.id.try_read(config)?.clone(), From 2fa7266e133e96b3c24d1f65ccfa021eca9ff835 Mon Sep 17 00:00:00 2001 From: Albin Suresh Date: Wed, 20 Mar 2024 15:31:30 +0000 Subject: [PATCH 2/2] Update system test CA cert generation instruction --- tests/RobotFramework/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/RobotFramework/README.md b/tests/RobotFramework/README.md index a90fe4a2186..098eabb1970 100644 --- a/tests/RobotFramework/README.md +++ b/tests/RobotFramework/README.md @@ -234,8 +234,8 @@ The following dependencies are requires for the instructions: 3. Add the `CA_KEY` and `CA_PUB` environment variables to your `.env` file used by the tests. ```sh - echo CA_KEY=\""$(cat ~/tedge-ca.key | base64)"\" >> .env - echo CA_PUB=\""$(cat ~/tedge-ca.crt | base64)"\" >> .env + echo CA_KEY=\""$(cat ~/tedge-ca.key | base64 | tr -d '\n')"\" >> .env + echo CA_PUB=\""$(cat ~/tedge-ca.crt | base64 | tr -d '\n')"\" >> .env ``` **Note**