Pin the relay's identity, not just its TLS (ios#19, Android half)
🔴 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 <noreply@anthropic.com>
This commit is contained in:
Paul Hitt
co-authored by
Claude Opus 5
Claude Opus 5
parent
4a587a7d4d
commit
ab8ff658b2
@@ -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))
|
||||
@@ -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(
|
||||
|
||||
@@ -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<RelayIdentityPin.Verdict.Changed>(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<RelayIdentityChangedException>(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)),
|
||||
)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user