From 3c75510f0144483c3244f3bfa5d1b4dfd9dc6ca6 Mon Sep 17 00:00:00 2001 From: James Rich <2199651+jamesarich@users.noreply.github.com> Date: Wed, 6 May 2026 12:09:49 -0500 Subject: [PATCH] fix(mqtt): harden TLS enforcement, add user CA trust, and improve error diagnostics (#5365) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../main/res/xml/network_security_config.xml | 1 + .../core/data/manager/MqttManagerImpl.kt | 7 +-- .../network/repository/MQTTRepositoryImpl.kt | 20 ++++++++- .../repository/MQTTRepositoryImplTest.kt | 44 +++++++++++++++++++ .../composeResources/values/strings.xml | 2 +- feature/settings/build.gradle.kts | 1 + .../radio/component/MQTTConfigItemList.kt | 14 +++--- 7 files changed, 76 insertions(+), 13 deletions(-) diff --git a/app/src/main/res/xml/network_security_config.xml b/app/src/main/res/xml/network_security_config.xml index da60fc884..11e036e0a 100644 --- a/app/src/main/res/xml/network_security_config.xml +++ b/app/src/main/res/xml/network_security_config.xml @@ -3,6 +3,7 @@ + diff --git a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MqttManagerImpl.kt b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MqttManagerImpl.kt index 653949665..5d24a9aaf 100644 --- a/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MqttManagerImpl.kt +++ b/core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MqttManagerImpl.kt @@ -35,6 +35,7 @@ import org.meshtastic.core.model.MqttProbeStatus import org.meshtastic.core.network.repository.MQTTRepository import org.meshtastic.core.network.repository.resolveEndpoint import org.meshtastic.core.repository.MqttManager +import org.meshtastic.core.repository.NodeRepository import org.meshtastic.core.repository.PacketHandler import org.meshtastic.core.repository.ServiceRepository import org.meshtastic.mqtt.ConnectionState @@ -44,13 +45,13 @@ import org.meshtastic.mqtt.ProbeResult import org.meshtastic.mqtt.probe import org.meshtastic.proto.MqttClientProxyMessage import org.meshtastic.proto.ToRadio -import kotlin.time.Clock @Single class MqttManagerImpl( private val mqttRepository: MQTTRepository, private val packetHandler: PacketHandler, private val serviceRepository: ServiceRepository, + private val nodeRepository: NodeRepository, @Named("ServiceScope") private val scope: CoroutineScope, ) : MqttManager { private var mqttMessageFlow: Job? = null @@ -131,8 +132,8 @@ class MqttManagerImpl( val endpoint = resolveEndpoint(address, tlsEnabled) val result = MqttClient.probe(endpoint = endpoint) { - // Provide a valid client ID for the probe; brokers reject empty identifiers - clientId = "MeshtasticProbe-${Clock.System.now().toEpochMilliseconds()}" + // Use node ID for consistent client identification across platforms + clientId = "MeshtasticAndroidMqttProbe-${nodeRepository.myId.value ?: "unknown"}" val user = username?.takeUnless { it.isEmpty() } val pass = password?.takeUnless { it.isEmpty() } if (user != null) this.username = user diff --git a/core/network/src/commonMain/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImpl.kt b/core/network/src/commonMain/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImpl.kt index 9838af9a7..ae5f5fb77 100644 --- a/core/network/src/commonMain/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImpl.kt +++ b/core/network/src/commonMain/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImpl.kt @@ -290,4 +290,22 @@ fun resolveEndpoint(rawAddress: String, tlsEnabled: Boolean): MqttEndpoint = if private const val DEFAULT_PUBLIC_SERVER = "mqtt.meshtastic.org" fun effectiveTlsEnabled(address: String, tlsEnabled: Boolean): Boolean = - tlsEnabled || address.equals(DEFAULT_PUBLIC_SERVER, ignoreCase = true) + tlsEnabled || extractHost(address).equals(DEFAULT_PUBLIC_SERVER, ignoreCase = true) + +/** + * Extracts the bare hostname from an address that may include a scheme, port, or path. Examples: + * - `mqtt.meshtastic.org` → `mqtt.meshtastic.org` + * - `mqtt.meshtastic.org:1883` → `mqtt.meshtastic.org` + * - `tcp://mqtt.meshtastic.org:1883` → `mqtt.meshtastic.org` + * - `ssl://mqtt.meshtastic.org` → `mqtt.meshtastic.org` + */ +internal fun extractHost(address: String): String { + val afterScheme = + if (address.contains("://")) { + address.substringAfter("://") + } else { + address + } + // Remove path (if any), then remove port + return afterScheme.substringBefore("/").substringBefore(":") +} diff --git a/core/network/src/commonTest/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImplTest.kt b/core/network/src/commonTest/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImplTest.kt index b028ed396..a42f1ee44 100644 --- a/core/network/src/commonTest/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImplTest.kt +++ b/core/network/src/commonTest/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImplTest.kt @@ -110,6 +110,21 @@ class MQTTRepositoryImplTest { assertEquals(true, effectiveTlsEnabled("MQTT.MESHTASTIC.ORG", tlsEnabled = false)) } + @Test + fun `default server with explicit port still forces TLS`() { + assertEquals(true, effectiveTlsEnabled("mqtt.meshtastic.org:1883", tlsEnabled = false)) + } + + @Test + fun `default server with tcp scheme still forces TLS`() { + assertEquals(true, effectiveTlsEnabled("tcp://mqtt.meshtastic.org:1883", tlsEnabled = false)) + } + + @Test + fun `default server with ssl scheme still forces TLS`() { + assertEquals(true, effectiveTlsEnabled("ssl://mqtt.meshtastic.org", tlsEnabled = false)) + } + @Test fun `custom server respects tlsEnabled false`() { assertEquals(false, effectiveTlsEnabled("mqtt.myserver.pt", tlsEnabled = false)) @@ -122,6 +137,35 @@ class MQTTRepositoryImplTest { // endregion + // region extractHost — address canonicalization tests. + + @Test + fun `extractHost bare hostname`() { + assertEquals("mqtt.meshtastic.org", extractHost("mqtt.meshtastic.org")) + } + + @Test + fun `extractHost with port`() { + assertEquals("mqtt.meshtastic.org", extractHost("mqtt.meshtastic.org:8883")) + } + + @Test + fun `extractHost with tcp scheme and port`() { + assertEquals("mqtt.meshtastic.org", extractHost("tcp://mqtt.meshtastic.org:1883")) + } + + @Test + fun `extractHost with ssl scheme no port`() { + assertEquals("mqtt.meshtastic.org", extractHost("ssl://mqtt.meshtastic.org")) + } + + @Test + fun `extractHost with path`() { + assertEquals("broker.example.com", extractHost("ws://broker.example.com:8080/mqtt")) + } + + // endregion + // region MqttJsonPayload — keep the existing JSON contract tests. @Test diff --git a/core/resources/src/commonMain/composeResources/values/strings.xml b/core/resources/src/commonMain/composeResources/values/strings.xml index 9542f2513..0f3cf4e28 100644 --- a/core/resources/src/commonMain/composeResources/values/strings.xml +++ b/core/resources/src/commonMain/composeResources/values/strings.xml @@ -719,7 +719,7 @@ Reachable (%1$s) Cannot reach broker (TCP) Timed out after %1$d ms - TLS handshake failed + TLS handshake failed: %1$s Connected Connecting… Disconnected diff --git a/feature/settings/build.gradle.kts b/feature/settings/build.gradle.kts index 1004f44a1..3725dd293 100644 --- a/feature/settings/build.gradle.kts +++ b/feature/settings/build.gradle.kts @@ -36,6 +36,7 @@ kotlin { implementation(projects.core.domain) implementation(projects.core.model) implementation(projects.core.navigation) + implementation(projects.core.network) implementation(projects.core.proto) implementation(projects.core.repository) implementation(projects.core.service) diff --git a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/MQTTConfigItemList.kt b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/MQTTConfigItemList.kt index 5f951766b..f02590ae4 100644 --- a/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/MQTTConfigItemList.kt +++ b/feature/settings/src/commonMain/kotlin/org/meshtastic/feature/settings/radio/component/MQTTConfigItemList.kt @@ -49,9 +49,9 @@ import androidx.lifecycle.compose.collectAsStateWithLifecycle import org.jetbrains.compose.resources.stringResource import org.meshtastic.core.model.MqttConnectionState import org.meshtastic.core.model.MqttProbeStatus +import org.meshtastic.core.network.repository.effectiveTlsEnabled import org.meshtastic.core.resources.Res import org.meshtastic.core.resources.address -import org.meshtastic.core.resources.default_mqtt_address import org.meshtastic.core.resources.encryption_enabled import org.meshtastic.core.resources.json_output_enabled import org.meshtastic.core.resources.map_reporting @@ -183,9 +183,8 @@ fun MQTTConfigScreen(viewModel: RadioConfigViewModel, onBack: () -> Unit) { containerColor = CardDefaults.cardColors().containerColor, ) HorizontalDivider() - val defaultAddress = stringResource(Res.string.default_mqtt_address) - val isDefault = formState.value.address.isEmpty() || formState.value.address.contains(defaultAddress) - val enforceTls = isDefault && formState.value.proxy_to_client_enabled + val resolvedAddress = formState.value.address.ifEmpty { "mqtt.meshtastic.org" } + val enforceTls = effectiveTlsEnabled(resolvedAddress, tlsEnabled = false) SwitchPreference( title = stringResource(Res.string.tls_enabled), checked = formState.value.tls_enabled || enforceTls, @@ -318,14 +317,13 @@ private fun MqttAddressAndProbe( }, ) HorizontalDivider() - val defaultAddress = stringResource(Res.string.default_mqtt_address) MqttProbeRow( enabled = enabled && formState.value.address.isNotBlank(), status = probeStatus, onTestClick = { focusManager.clearFocus() - val isDefault = formState.value.address.isEmpty() || formState.value.address.contains(defaultAddress) - val effectiveTls = formState.value.tls_enabled || isDefault + val resolvedAddress = formState.value.address.ifEmpty { "mqtt.meshtastic.org" } + val effectiveTls = effectiveTlsEnabled(resolvedAddress, formState.value.tls_enabled) onProbe(formState.value.address, effectiveTls, formState.value.username, formState.value.password) }, ) @@ -376,7 +374,7 @@ private fun MqttProbeStatus?.toLabel(): Pair? = when (this) { stringResource(Res.string.mqtt_probe_tcp_failure) to MaterialTheme.colorScheme.error is MqttProbeStatus.TlsFailure -> - stringResource(Res.string.mqtt_probe_tls_failure) to MaterialTheme.colorScheme.error + stringResource(Res.string.mqtt_probe_tls_failure, message ?: "unknown") to MaterialTheme.colorScheme.error is MqttProbeStatus.Timeout -> stringResource(Res.string.mqtt_probe_timeout, timeoutMs.toInt()) to MaterialTheme.colorScheme.error