From ab8ff658b23078e8aae557dabbc320d5cfcea426 Mon Sep 17 00:00:00 2001 From: Paul Hitt Date: Fri, 4 Sep 2026 13:46:57 -0400 Subject: [PATCH] Pin the relay's identity, not just its TLS (ios#19, Android half) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🔴 This was not a missing feature on Android. It was a wrong conclusion. `Trust.verifyHeadSignature` already pinned `issuerPublicKey` the first time a signed log head arrived. When the key later changed, the head signature simply stopped verifying — and the only conclusion available at that point was `KeyLogForkException`. So a relay key rotation somebody performed on purpose was reported to the user as the relay showing two different histories, with `forkDetected` set. Those need different responses. A fork means nothing the relay says can be trusted and the client should stop. A rotation means somebody ran a command, or somebody else is answering on that address, and only a person can tell which. Reporting the second as the first is alarming and unactionable — and an alarm people cannot act on is one they learn to dismiss, which costs the real fork detection its credibility as well. `RelayIdentityPin` now decides what the published key MEANS before it is used, with the same four verdicts as iOS, and a change raises `RelayIdentityChangedException` — deliberately NOT a `KeyLogForkException`, and without setting `forkDetected`, because the log may be perfectly consistent. ⚠️ A relay that publishes no key is a weaker deployment, not an attack, and an unparseable key is broken rather than hostile. Both read as "not published" rather than as a change: refusing to talk to an older self-hosted build would be a worse failure than the one this prevents. ⚠️ The warning names both possibilities and asserts neither, because nothing in the client can tell a rotation from an impostor. Two things found while writing the tests. `RelayClient.host` already existed, so the one I added was a duplicate declaration — removed rather than renamed. And the compiler folded `changed !is KeyLogForkException` to a constant when the value was typed concretely, which is the compiler proving the separation statically and also an assertion testing nothing; it is now held as the sealed base, which is the check a caller's `when` actually performs. 9 unit tests. App builds, every JVM suite green. Co-Authored-By: Claude Opus 5 --- PARITY.md | 16 +- .../io/flamenet/client/RelayIdentityPin.kt | 112 ++++++++++++ .../main/kotlin/io/flamenet/client/Trust.kt | 40 ++++- .../flamenet/client/RelayIdentityPinTest.kt | 167 ++++++++++++++++++ 4 files changed, 326 insertions(+), 9 deletions(-) create mode 100644 client/src/main/kotlin/io/flamenet/client/RelayIdentityPin.kt create mode 100644 client/src/test/kotlin/io/flamenet/client/RelayIdentityPinTest.kt diff --git a/PARITY.md b/PARITY.md index ff3b21d..e6de01c 100644 --- a/PARITY.md +++ b/PARITY.md @@ -161,7 +161,7 @@ being the one that leaks. | Back off on repeated app-lock attempts | ✅ | ✅ | ios#41 — 3 free attempts, then 30s doubling to a 5-minute ceiling | | Handle a wrong clock | ✅ | ✅ | ios#45 — skew learned free from the `Date` header; expiry takes the LATER of device and relay | | Zero key material after use | ✅ | 🟡 | ios#39 — done, but the JVM guarantee is **weaker than iOS**; see below | -| Pin the relay's identity | ✅ | ⚪ | ios#19 | +| Pin the relay's identity | ✅ | ✅ | ios#19 — Android already pinned the key and drew the **wrong conclusion** from a change; see below | ⚠️ **The back-off is not a defence of the data.** It slows a person holding an unlocked phone. The vault and the ratchet keys are protected by the Android @@ -185,6 +185,20 @@ the UI thread, so the two fields are `@Volatile` and updated together under a lock. A torn read of "skew from now, timestamp from an hour ago" would age out an observation just made, or trust one that had not been. +🔴 **The relay pin was not a missing feature on Android — it was a wrong +conclusion.** `Trust.verifyHeadSignature` already pinned `issuerPublicKey` the +first time a signed log head arrived. When the key later changed, the signature +stopped verifying and the only conclusion available at that point was +`KeyLogForkException` — so a rotation somebody performed on purpose was reported +to the user as *the relay showing two different histories*. + +Those need different responses. A fork means nothing the relay says can be +trusted. A rotation means somebody ran a command, or somebody else is answering +on that address, and only a person can tell which. `RelayIdentityChangedException` +is deliberately **not** a `KeyLogForkException`, and `forkDetected` is not set — +an alarm people cannot act on is one they learn to dismiss, which would cost the +real fork detection its credibility too. + 🔴 **Zeroing is where Android cannot match iOS, and the row says 🟡 for that reason rather than being rounded up to ✅.** iOS has `memset_s`, specified not to be optimised away, and iOS does not page application memory to disk. Neither diff --git a/client/src/main/kotlin/io/flamenet/client/RelayIdentityPin.kt b/client/src/main/kotlin/io/flamenet/client/RelayIdentityPin.kt new file mode 100644 index 0000000..38229af --- /dev/null +++ b/client/src/main/kotlin/io/flamenet/client/RelayIdentityPin.kt @@ -0,0 +1,112 @@ +package io.flamenet.client + +/** + * Which relay this is, beyond which certificate it presented (ios#19). + * + * 🔴 **TLS answers "is this host who DNS says it is". It does not answer "is + * this the same relay I have been talking to".** A mis-issued certificate, a + * hijacked DNS record, or a hostile party who obtains the domain all produce a + * server with a perfectly valid certificate for the right name — and every one + * of them is a different relay wearing the same address. + * + * The relay publishes an issuer public key in its descriptor. Pinning that on + * first contact turns "the certificate is valid" into "this is the same relay", + * which is a materially stronger statement and the one that matters. + * + * 🔴 **Android already pinned this key and drew the wrong conclusion from it.** + * `Trust.verifyHeadSignature` pins `issuerPublicKey` the first time a signed log + * head arrives, and if the key later changes the signature simply fails to + * verify — so the client raises `KeyLogForkException`, sets `forkDetected`, and + * tells somebody the relay's key log does not add up. + * + * A rotated relay key is not a forked log. They need different responses: a + * fork means the relay is showing two different histories and nothing it says + * can be trusted; a rotation means somebody ran a command, or somebody else is + * answering on that address. Reporting the second as the first is both alarming + * and unactionable, and an alarm people cannot act on is one they learn to + * dismiss — which costs the real fork detection its credibility too. + * + * ⚠️ **A relay that publishes no key is a weaker deployment, not an attack.** + * Somebody self-hosting an older build has nothing to pin, and refusing to talk + * to them would be a worse failure than the one this prevents. That case is + * distinguished from a key that CHANGED, which is not weakness. + * + * ⚠️ A change is surfaced, never silently accepted and never silently refused. + * The same shape as a contact's identity changing: stop, say so, and let a + * person decide — because a legitimate rotation exists and an app that cannot + * survive one is an app that gets uninstalled. + */ +object RelayIdentityPin { + + /** What a descriptor's key means against what was pinned before. */ + sealed interface Verdict { + /** Nothing pinned and a key published: pin it. */ + data class FirstSight(val offered: ByteArray) : Verdict { + override fun equals(other: Any?) = + other is FirstSight && offered.contentEquals(other.offered) + override fun hashCode() = offered.contentHashCode() + } + + /** The same relay as before. */ + data object Unchanged : Verdict + + /** This deployment publishes no key. Nothing to check. */ + data object NotPublished : Verdict + + /** + * 🔴 A different key than the one pinned. Either a deliberate rotation + * or a different server answering on this address, and nothing here can + * tell those apart — only a person can. + */ + data class Changed(val pinned: ByteArray, val offered: ByteArray) : Verdict { + override fun equals(other: Any?) = other is Changed && + pinned.contentEquals(other.pinned) && offered.contentEquals(other.offered) + override fun hashCode() = 31 * pinned.contentHashCode() + offered.contentHashCode() + } + } + + /** + * Compare what the relay just published against what is pinned. + * + * ⚠️ An empty or unparseable key counts as "not published" rather than as a + * change. A relay that answers with rubbish is broken, and treating broken + * as hostile would raise an alarm nobody can act on. + */ + fun check(offeredBase64: String?, pinned: ByteArray?): Verdict { + val offered = offeredBase64 + ?.takeIf { it.isNotEmpty() } + ?.let { runCatching { Wire.unb64(it) }.getOrNull() } + ?.takeIf { it.isNotEmpty() } + ?: return Verdict.NotPublished + if (pinned == null || pinned.isEmpty()) return Verdict.FirstSight(offered) + return if (offered.contentEquals(pinned)) Verdict.Unchanged + else Verdict.Changed(pinned = pinned, offered = offered) + } + + /** + * What to tell somebody when it changed. + * + * 🔴 Names the two possibilities and does not pretend to know which. An + * alarm that asserts an attack when a rotation is equally likely trains + * people to dismiss alarms. + */ + fun warning(host: String): String = + "The server at $host is presenting a different identity than the one " + + "this app recorded the first time it connected. That happens when the " + + "server's keys are rotated on purpose — and it also happens when " + + "something else is answering in its place. Nothing here can tell those " + + "apart. If you were not expecting a change, stop and check with " + + "whoever runs it before signing in again." +} + +/** + * The relay is not the one this app pinned. + * + * 🔴 **Deliberately NOT a [KeyLogForkException].** A fork means the relay is + * showing two different histories and nothing it says can be trusted. This means + * the server's identity key is not the one recorded — a rotation somebody + * performed, or somebody else's server. The remedies are different, so the + * exceptions are different. + */ +class RelayIdentityChangedException(val host: String) : + RelayException(RelayIdentityPin.warning(host)) diff --git a/client/src/main/kotlin/io/flamenet/client/Trust.kt b/client/src/main/kotlin/io/flamenet/client/Trust.kt index 3d48183..ba4d977 100644 --- a/client/src/main/kotlin/io/flamenet/client/Trust.kt +++ b/client/src/main/kotlin/io/flamenet/client/Trust.kt @@ -612,21 +612,45 @@ class Trust( private fun verifyHeadSignature(head: Head, signatureB64: String) { if (signatureB64.isEmpty()) return val signature = runCatching { Wire.unb64(signatureB64) }.getOrNull() ?: return - var issuer = vault.keyLogIssuerPin() - if (issuer == null) { - val published = runCatching { - Wire.unb64(relay.relayDescriptor().optString("issuerPublicKey")) - }.getOrNull() - if (published == null || published.isEmpty()) return - vault.saveKeyLogIssuerPin(published) - issuer = published + + // ios#19 — decide what the relay's published identity MEANS before + // using it, rather than inferring it from a signature that failed. + // + // 🔴 This is the bug the pin previously hid. A rotated relay key made + // the signature stop verifying, and the only conclusion available at + // that point was "fork" — so a command somebody ran on purpose was + // reported as the relay showing two different histories. Different + // situations, different remedies, and an alarm nobody can act on is one + // people learn to dismiss, which costs real fork detection too. + val offered = runCatching { + relay.relayDescriptor().optString("issuerPublicKey") + }.getOrNull() + val issuer = when (val verdict = RelayIdentityPin.check(offered, vault.keyLogIssuerPin())) { + is RelayIdentityPin.Verdict.NotPublished -> + // Nothing published and nothing pinned: a weaker deployment, + // not an attack. Nothing to check the signature against. + vault.keyLogIssuerPin() ?: return + is RelayIdentityPin.Verdict.FirstSight -> { + vault.saveKeyLogIssuerPin(verdict.offered) + verdict.offered + } + is RelayIdentityPin.Verdict.Unchanged -> vault.keyLogIssuerPin() ?: return + is RelayIdentityPin.Verdict.Changed -> + // ⚠️ NOT a fork, and `forkDetected` is deliberately not set. + // The log may be perfectly consistent; it is the server's + // identity that is not the one recorded. + throw RelayIdentityChangedException(relayHostForPin()) } + if (!KeyLog.verifyHead(head.size, head.root, signature, issuer)) { forkDetected = true throw KeyLogForkException("log head signature does not verify under the pinned key") } } + /** The host to name in an identity-change warning. */ + private fun relayHostForPin(): String = relay.host + private fun consistent(from: Int, to: Int, oldRoot: String, newRoot: String): Boolean { val c = relay.logConsistency(from.toLong(), to.toLong()) return KeyLog.verifyConsistency( diff --git a/client/src/test/kotlin/io/flamenet/client/RelayIdentityPinTest.kt b/client/src/test/kotlin/io/flamenet/client/RelayIdentityPinTest.kt new file mode 100644 index 0000000..173c583 --- /dev/null +++ b/client/src/test/kotlin/io/flamenet/client/RelayIdentityPinTest.kt @@ -0,0 +1,167 @@ +package io.flamenet.client + +import java.util.Base64 +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertIs +import kotlin.test.assertTrue + +/** + * Pinning the relay's identity (ios#19, Android half). + * + * 🔴 **The finding this closes is not a missing feature — it is a wrong + * conclusion.** Android already pinned `issuerPublicKey`, inside + * `Trust.verifyHeadSignature`. When the key later changed, the head signature + * stopped verifying and the only conclusion available at that point was + * `KeyLogForkException` — so a rotation somebody performed on purpose was + * reported to the user as the relay showing two different histories. + * + * Those need different responses. A fork means nothing the relay says can be + * trusted. A rotation means somebody ran a command, or somebody else is + * answering on that address, and a person has to decide which. Reporting the + * second as the first is alarming and unactionable, and an alarm people cannot + * act on is one they learn to dismiss — which costs the real fork detection its + * credibility too. + */ +class RelayIdentityPinTest { + + private fun key(byte: Int) = ByteArray(32) { byte.toByte() } + private fun b64(bytes: ByteArray): String = Base64.getEncoder().encodeToString(bytes) + + // ---- The four verdicts ---------------------------------------------- + + @Test + fun `a published key with nothing pinned is first sight`() { + val offered = key(1) + val verdict = RelayIdentityPin.check(b64(offered), pinned = null) + assertEquals(RelayIdentityPin.Verdict.FirstSight(offered), verdict) + } + + @Test + fun `the same key is unchanged`() { + assertEquals( + RelayIdentityPin.Verdict.Unchanged, + RelayIdentityPin.check(b64(key(1)), pinned = key(1)), + ) + } + + /** + * 🔴 The case the whole file exists for, and the one that used to be + * reported as a forked log. + */ + @Test + fun `a different key is a change, carrying both halves`() { + val verdict = RelayIdentityPin.check(b64(key(2)), pinned = key(1)) + val changed = assertIs(verdict) + assertTrue(changed.pinned.contentEquals(key(1))) + assertTrue(changed.offered.contentEquals(key(2))) + } + + /** + * ⚠️ **A relay that publishes no key is a weaker deployment, not an + * attack.** Somebody self-hosting an older build has nothing to pin, and + * refusing to talk to them would be a worse failure than the one this + * prevents. + */ + @Test + fun `a deployment that publishes nothing is not an attack`() { + assertEquals( + RelayIdentityPin.Verdict.NotPublished, + RelayIdentityPin.check(null, pinned = key(1)), + ) + assertEquals( + RelayIdentityPin.Verdict.NotPublished, + RelayIdentityPin.check("", pinned = key(1)), + ) + } + + /** + * ⚠️ **Broken is not hostile.** A relay answering with rubbish counts as + * "not published" rather than as a change — treating a malformed field as + * an attack would raise an alarm nobody can act on, on a deployment that is + * simply misconfigured. + */ + @Test + fun `an unparseable key reads as not published, not as a change`() { + assertEquals( + RelayIdentityPin.Verdict.NotPublished, + RelayIdentityPin.check("!!! not base64 !!!", pinned = key(1)), + ) + // Valid base64 of nothing is still nothing. + assertEquals( + RelayIdentityPin.Verdict.NotPublished, + RelayIdentityPin.check(b64(ByteArray(0)), pinned = key(1)), + ) + } + + /** An empty pin is no pin: first sight, not a change against zero bytes. */ + @Test + fun `an empty pin is treated as no pin`() { + val offered = key(3) + assertEquals( + RelayIdentityPin.Verdict.FirstSight(offered), + RelayIdentityPin.check(b64(offered), pinned = ByteArray(0)), + ) + } + + // ---- What somebody is told ------------------------------------------ + + /** + * 🔴 The warning names BOTH possibilities and asserts neither. An alarm that + * claims an attack when a rotation is equally likely trains people to + * dismiss alarms — and the next one may be real. + */ + @Test + fun `the warning does not pretend to know which it is`() { + val text = RelayIdentityPin.warning("relay.example.com") + assertTrue(text.contains("relay.example.com"), text) + assertTrue(text.contains("rotated on purpose"), text) + assertTrue(text.contains("something else is answering"), text) + assertTrue(text.contains("Nothing here can tell those apart"), text) + } + + /** + * 🔴 **The exception is not a fork exception, and that separation is the + * point of this change.** A caller catching `KeyLogForkException` to decide + * "the log is untrustworthy" must not be handed a rotation. + */ + @Test + fun `an identity change is not reported as a key-log fork`() { + // ⚠️ Held as the sealed base, deliberately. Typed as the concrete class + // the compiler folds `!is KeyLogForkException` to a constant and warns — + // which is the compiler proving the point statically, and also means the + // assertion would test nothing. Through the base type it is the runtime + // check a caller's `when` actually performs. + val thrown: RelayException = RelayIdentityChangedException("relay.example.com") + assertTrue( + thrown !is KeyLogForkException, + "a rotated relay key is being reported as a forked log", + ) + val identity = assertIs(thrown) + assertEquals("relay.example.com", identity.host) + assertTrue(identity.message!!.contains("rotated on purpose")) + } + + // ---- Shape ----------------------------------------------------------- + + /** + * ⚠️ `ByteArray` has reference equality by default, so the verdicts carry + * hand-written `equals`. Without them every comparison in this suite would + * be false and the tests would have been written around it. + */ + @Test + fun `verdicts compare by content, not by reference`() { + assertEquals( + RelayIdentityPin.Verdict.FirstSight(key(1)), + RelayIdentityPin.Verdict.FirstSight(key(1)), + ) + assertEquals( + RelayIdentityPin.Verdict.Changed(key(1), key(2)), + RelayIdentityPin.Verdict.Changed(key(1), key(2)), + ) + assertTrue( + RelayIdentityPin.Verdict.Changed(key(1), key(2)) != + RelayIdentityPin.Verdict.Changed(key(1), key(3)), + ) + } +}