fix(transport): make TLS trust hook honest and tested

Addresses the final whole-branch review of the #102 TLS trust hook.

- Android: rethrow X509TrustManagerExtensions' opaque IllegalArgumentException
  with the actual requirement and fix, so a hand-written X509TrustManager fails
  diagnosably instead of looping forever under autoReconnect. No silent fallback
  to the unwrapped caller manager.
- Docs: correct the overclaim in README.md, transport-tcp/Module.md and the
  applyMqttTls KDoc. The wrapping keeps caller trust anchors subject to Android's
  network-security-config, pinning and CT policy; it is NOT RFC 6125 subject-name
  matching, which is ktor's and only when SNI is set, so absent for IP-literal
  brokers and absent entirely on JVM/native.
- Add a defaulted platformTrust seam to applyMqttTls and a commonTest that pins
  caller-hook-before-platform-trust ordering on every target.
- TcpTransport.configureTls is now internal so commonTest can assertSame that
  TcpTransportFactory.create() forwards the lambda. Not in the public ABI.
- Drop the unresolvable [serverName] Dokka link and warn against setting
  serverName = null.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: jamesarich

README.md

@@ -349,7 +349,22 @@ client.connect(MqttEndpoint.parse("mqtts://broker.internal:8883"))
 
 The hook is applied after the SNI server name is resolved and before platform trust is configured.
 On Android that ordering matters: your trust manager is *wrapped* by the hostname-aware trust
-manager rather than replacing it, so certificate hostname verification still happens.
+manager rather than replacing it, so your trust anchors remain subject to the platform's
+domain-specific `network_security_config.xml` rules, certificate pinning, and Certificate
+Transparency policy instead of replacing that policy wholesale.
+
+Be clear about what the wrapping does *not* buy you. Android's hostname-aware
+`checkServerTrusted(chain, authType, hostname)` uses the hostname to look up that policy — it does
+not perform RFC 6125 subject-name matching. That check comes from ktor and only runs when the SNI
+server name is set, so it is absent for IP-literal brokers such as `mqtts://192.168.1.50:8883`. On
+JVM and native targets there is no platform trust wrapping at all, so ktor's SNI-gated
+subject-name check is the only peer-identity verification beyond chain validation. If your trust
+manager accepts any chain, nothing else will stop a mismatched certificate.
+
+On Android the trust manager you install must be one the platform can wrap: obtain it from a
+`TrustManagerFactory` initialised with a `KeyStore` containing your CA, rather than hand-writing an
+`X509TrustManager`. A hand-written one that implements only the two-arg `checkServerTrusted` cannot
+be wrapped and the handshake fails with an `IllegalArgumentException` explaining this.
 
 This scopes the extra trust to the MQTT connection alone. It replaces the app-wide workaround of
 adding `<certificates src="user"/>` to `network_security_config.xml`, which would affect every

transport-tcp/Module.md

@@ -24,8 +24,14 @@ val client = MqttClient("sensor") {
 ```
 
 The lambda is applied after the SNI server name is set and before platform trust configuration, so
-on Android a trust manager installed here is still wrapped in the hostname-aware delegate rather
-than replacing it. The added trust applies only to this MQTT connection — unlike Android's
+on Android a trust manager installed here is wrapped in the hostname-aware delegate rather than
+replacing it: the trust anchors you add stay subject to the platform's domain-specific
+`network_security_config.xml` rules, certificate pinning, and Certificate Transparency policy. That
+is all the wrapping buys — RFC 6125 subject-name matching comes from ktor and only when the SNI
+server name is set, so it does not happen for IP-literal brokers, and the platform policy checks do
+not happen at all on JVM or native targets. On Android the trust manager must be one the platform
+can wrap (obtained from a `TrustManagerFactory`, not hand-implemented) or the handshake fails.
+The added trust applies only to this MQTT connection — unlike Android's
 app-wide `network_security_config.xml` trust anchors. `trustManager` itself is available on the
 JVM and Android actuals of `TLSConfigBuilder`; on Apple, Linux, and Windows the lambda still runs,
 but `TLSConfigBuilder` exposes a different set of properties there.

transport-tcp/src/androidMain/kotlin/org/meshtastic/mqtt/transport/tcp/PlatformTls.android.kt

@@ -37,6 +37,19 @@ import javax.net.ssl.X509TrustManager
  * the target host, so an IP-only broker (a common private-broker setup) hits the same
  * failure. The [host] — an IP literal or DNS name — is a valid argument for the 3-arg
  * overload even though an IP must never be sent as the TLS SNI server name.
+ *
+ * **Constraint on caller-supplied trust managers.** Whatever [X509TrustManager] is on the
+ * builder must be one Android can wrap for hostname-aware checking. In practice that means it
+ * must come from a [TrustManagerFactory] (which yields the platform's `TrustManagerImpl`), or
+ * it must itself declare a `checkServerTrusted(X509Certificate[], String, String)` method.
+ * A hand-written `X509TrustManager` that implements only the two-arg overloads cannot be
+ * wrapped, and this function throws [IllegalArgumentException] rather than silently dropping
+ * Android's policy checks. To trust a private CA, load it into a [KeyStore] and initialise a
+ * [TrustManagerFactory] with that store.
+ *
+ * Note that the hostname passed to the 3-arg overload drives network-security-config lookup,
+ * certificate pinning, and Certificate Transparency policy — it does **not** perform RFC 6125
+ * subject-name matching. That comes from ktor and only when the SNI server name is set.
  */
 internal actual fun TLSConfigBuilder.configurePlatformTrust(host: String) {
     if (host.isBlank()) return
@@ -48,7 +61,20 @@ internal actual fun TLSConfigBuilder.configurePlatformTrust(host: String) {
             tmf.trustManagers.filterIsInstance<X509TrustManager>().first()
         }
 
-    trustManager = HostnameAwareTrustManager(baseTm, host)
+    trustManager =
+        try {
+            HostnameAwareTrustManager(baseTm, host)
+        } catch (e: IllegalArgumentException) {
+            throw IllegalArgumentException(
+                "Android cannot wrap the configured X509TrustManager (${baseTm::class.java.name}) " +
+                    "for hostname-aware certificate checking. The trust manager must either be " +
+                    "obtained from TrustManagerFactory (which yields the platform TrustManagerImpl) " +
+                    "or declare checkServerTrusted(X509Certificate[], String, String). To trust a " +
+                    "private CA, load it into a KeyStore and initialise a TrustManagerFactory with " +
+                    "that KeyStore instead of hand-implementing X509TrustManager.",
+                e,
+            )
+        }
 }
 
 /**

transport-tcp/src/commonMain/kotlin/org/meshtastic/mqtt/transport/tcp/PlatformTls.kt

@@ -42,25 +42,39 @@ internal expect fun TLSConfigBuilder.configurePlatformTrust(host: String)
  *
  * This is the single call site for TLS setup, so the ordering below cannot drift:
  *
- * 1. [serverName] — the SNI value, `null` for IP literals (RFC 6066 §3 forbids them).
+ * 1. `serverName` — the SNI value, `null` for IP literals (RFC 6066 §3 forbids them).
  * 2. [configureTls] — the caller's hook, so it can read or override the SNI value and
  *    install its own trust manager (e.g. a private CA).
  * 3. [configurePlatformTrust] — reads whatever trust manager is now on the builder and,
  *    on Android, wraps it in a hostname-aware delegate.
  *
  * Step 2 must precede step 3. Reversing them would let a caller-supplied trust manager
- * *replace* Android's `HostnameAwareTrustManager`, silently dropping the platform's 3-arg
- * `checkServerTrusted` hostname verification. Running the caller first composes instead:
- * a private-CA trust manager is still subject to the hostname check.
+ * *replace* Android's `HostnameAwareTrustManager`, so the caller's trust anchors would no
+ * longer be evaluated against the platform's domain-specific `network_security_config.xml`
+ * rules, certificate pinning, or Certificate Transparency policy. Running the caller first
+ * composes instead: a private-CA trust manager stays subject to those platform checks.
+ *
+ * Note what this ordering does *not* provide. RFC 6125 subject-name matching (verifying the
+ * certificate actually names the host being contacted) comes from ktor, which performs it only
+ * when `serverName` is non-`null` — so it is absent for IP-literal brokers. Android's 3-arg
+ * `checkServerTrusted(chain, authType, hostname)` uses the hostname for config lookup, pinning,
+ * and CT policy, not for subject-name matching. On JVM and native targets
+ * [configurePlatformTrust] is a no-op, so no platform policy check happens there at all.
+ *
+ * The hook may read or replace `serverName`, but setting it to `null` disables ktor's
+ * subject-name verification entirely — the only name matching this transport has on JVM and
+ * native — so do not do that unless you verify the peer identity yourself.
  *
  * @param host the broker host (DNS name or IP literal), used for both SNI and trust evaluation.
  * @param configureTls optional caller customisation; `null` preserves the default behaviour.
+ * @param platformTrust test seam for [configurePlatformTrust]; production callers use the default.
  */
 internal fun TLSConfigBuilder.applyMqttTls(
     host: String,
     configureTls: (TLSConfigBuilder.() -> Unit)? = null,
+    platformTrust: TLSConfigBuilder.(String) -> Unit = { configurePlatformTrust(it) },
 ) {
     serverName = sniServerName(host)
     configureTls?.invoke(this)
-    configurePlatformTrust(host)
+    platformTrust(host)
 }

transport-tcp/src/commonMain/kotlin/org/meshtastic/mqtt/transport/tcp/TcpTransport.kt

@@ -56,7 +56,7 @@ import org.meshtastic.mqtt.packet.VariableByteInt
  *   non-TLS endpoints.
  */
 public class TcpTransport(
-    private val configureTls: (TLSConfigBuilder.() -> Unit)? = null,
+    internal val configureTls: (TLSConfigBuilder.() -> Unit)? = null,
 ) : MqttTransport {
     private var socket: Socket? = null
     private var selectorManager: SelectorManager? = null
@@ -279,8 +279,10 @@ internal fun isIpLiteral(host: String): Boolean {
  *
  * @param configureTls optional hook applied to ktor's [TLSConfigBuilder] for every transport
  *   this factory creates. It runs after the SNI server name is set and before platform trust
- *   is configured, so on Android a trust manager installed here is still wrapped in the
- *   hostname-aware delegate rather than replacing it.
+ *   is configured, so on Android a trust manager installed here is wrapped in the
+ *   hostname-aware delegate rather than replacing it — the caller's trust anchors stay subject
+ *   to the platform's network-security-config, pinning, and Certificate Transparency policy.
+ *   That wrapping is Android-only and is not subject-name matching; see [applyMqttTls].
  */
 public class TcpTransportFactory(
     private val configureTls: (TLSConfigBuilder.() -> Unit)? = null,

transport-tcp/src/commonTest/kotlin/org/meshtastic/mqtt/transport/tcp/TcpTransportFactoryTlsTest.kt

@@ -16,11 +16,14 @@
  */
 package org.meshtastic.mqtt.transport.tcp
 
+import io.ktor.network.tls.TLSConfigBuilder
 import org.meshtastic.mqtt.MqttEndpoint
 import org.meshtastic.mqtt.plus
 import kotlin.test.Test
 import kotlin.test.assertFalse
 import kotlin.test.assertIs
+import kotlin.test.assertNull
+import kotlin.test.assertSame
 import kotlin.test.assertTrue
 
 /**
@@ -46,6 +49,23 @@ class TcpTransportFactoryTlsTest {
         assertFalse(transport.isConnected)
     }
 
+    @Test
+    fun factoryForwardsTlsHookToCreatedTransport() {
+        // Guards against create() silently dropping configureTls: the transport must hold the
+        // very lambda instance the factory was constructed with.
+        val hook: TLSConfigBuilder.() -> Unit = { serverName = "override.example.com" }
+        val transport = TcpTransportFactory(hook).create(MqttEndpoint.Tcp("broker.example.com", 8883, tls = true))
+        assertIs<TcpTransport>(transport)
+        assertSame(hook, transport.configureTls)
+    }
+
+    @Test
+    fun noArgFactoryLeavesTlsHookNull() {
+        val transport = TcpTransportFactory().create(MqttEndpoint.Tcp("broker.example.com", 1883, tls = false))
+        assertIs<TcpTransport>(transport)
+        assertNull(transport.configureTls)
+    }
+
     @Test
     fun noArgFactoryStillWorks() {
         val factory = TcpTransportFactory()

transport-tcp/src/commonTest/kotlin/org/meshtastic/mqtt/transport/tcp/TcpTransportTlsTest.kt

@@ -101,6 +101,33 @@ class TcpTransportTlsTest {
         assertEquals("broker.example.com", observed)
     }
 
+    @Test
+    fun applyMqttTlsRunsCallerHookBeforePlatformTrust() {
+        // Security-relevant ordering: the caller hook must run BEFORE platform trust
+        // configuration. Reversed, a caller-supplied trust manager would replace Android's
+        // hostname-aware wrapper instead of being wrapped by it, dropping the platform's
+        // network-security-config, pinning, and CT policy checks. The platformTrust seam
+        // makes that order observable on every target.
+        val order = mutableListOf<String>()
+        val builder = TLSConfigBuilder()
+        builder.applyMqttTls(
+            host = "broker.example.com",
+            configureTls = { order += "callerHook" },
+            platformTrust = { order += "platformTrust" },
+        )
+        assertEquals(listOf("callerHook", "platformTrust"), order)
+    }
+
+    @Test
+    fun applyMqttTlsPassesTrustHostToPlatformTrustEvenWhenSniSuppressed() {
+        // IP literals suppress SNI but must still reach the platform trust hook with the raw host.
+        var trustHost: String? = null
+        val builder = TLSConfigBuilder()
+        builder.applyMqttTls(host = "192.168.1.50", platformTrust = { trustHost = it })
+        assertNull(builder.serverName)
+        assertEquals("192.168.1.50", trustHost)
+    }
+
     @Test
     fun applyMqttTlsWithoutHookStillSetsSni() {
         val builder = TLSConfigBuilder()