mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-12 09:13:23 +00:00
fix(concord): close ten review findings in the CORD-05 invite path
A high-effort review of the invite work found ten correctness bugs, nine confirmed and one plausible. All are fixed here; the on-device pass on a tablet proved the three that are observable through the UI. The load-bearing one: the kind-13303 Invite List is replaceable, and both clients merged a patch onto a base that silently degraded to EMPTY whenever the read failed — an unanswered relay or a bunker signer declining one decrypt was enough. Republishing that destroys every `signer_sk` it could not read, and those secrets cannot be regenerated, so every outstanding link is orphaned at a dead epoch. `decode` is now `decodeOrNull` and `decrypt` returns null, so "I could not read it" is distinguishable from "it is empty", and the write aborts rather than overwriting. The rest: - `join` is now ban-gated on both clients. A Refounding re-mints every outstanding link onto the new root, and an ex-member keeps the URL and its token forever, so the rotation meant to expel them handed them the new keys instead. Fails closed on an unreadable plane. - Android's Refounding re-read the entry from `liveCommunities` straight after adopting the new root, but that flow decrypts asynchronously, so every link was re-minted onto the epoch just left. `adoptConcordRoot` now returns the entry it wrote. - amy's refound folded the fresh bans locally and then never used them, re-draining from relays instead; a relay slow to echo them back would produce a new epoch whose roster never banned anyone. - Link refresh rebuilt the bundle from scratch, stripping expiry, channel grants, icon and label; it now moves the link's own current bundle and changes only the epoch's key material. - Refresh also re-posted over revocation tombstones, silently un-revoking a retired link. - The 13303 coordinate is (13303, me, "") — one list per account — but was read and written on per-community relays, forking it into divergent versions that newest-wins then collapsed. Now account-outbox only. - Minting returned the URL even when recording the link failed, handing out a link that could never be refreshed. It now fails closed. - amy's refound had no equivalent of Amethyst's recipient cap, leaving the attacker-writable half of the union unbounded. - amy's store dropped the banked epoch's `controlRoot` on the round-trip, losing the staff write key that rebuilds the anti-rollback floor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Vitor Pamplona
co-authored by
Claude Opus 5
Claude Opus 5
parent
66d2777262
commit
eb8c812690
@@ -230,7 +230,7 @@ object ConcordCommands {
|
||||
controlRoot = e.controlRoot ?: priorSameEpoch?.controlRoot ?: "",
|
||||
generalChannelId = prior?.generalChannelId ?: "",
|
||||
relays = e.relays,
|
||||
heldRoots = e.heldRoots.map { StoredHeldRoot(it.epoch, it.key, it.controlPk ?: "") },
|
||||
heldRoots = e.heldRoots.map { StoredHeldRoot(it.epoch, it.key, it.controlPk ?: "", it.controlRoot ?: "") },
|
||||
// Survives every merge: losing the anchor makes the NEXT exclusion
|
||||
// unrecoverable, so a list entry without one must not clear ours.
|
||||
inviteRef = e.inviteRef ?: prior?.inviteRef ?: "",
|
||||
@@ -266,16 +266,13 @@ object ConcordCommands {
|
||||
// (CORD-05 §1); omitted for a legacy community, which has none to carry.
|
||||
val invite = ConcordActions.inviteFor(sc.communityId, sc.owner, sc.ownerSalt, sc.root, sc.rootEpoch, sc.name, sc.relays, sc.controlPk.ifBlank { null })
|
||||
val minted = ConcordActions.mintInviteLink(base, invite, TimeUtils.now(), sc.relays)
|
||||
val ack = ctx.publish(minted.bundleEvent, relaysFor(ctx, sc))
|
||||
RawEventSupport.publishGuard(ack, minted.bundleEvent.id)?.let { return it }
|
||||
|
||||
// Record the link in the CORD-05 Invite List (kind 13303) so any of this creator's
|
||||
// clients — Amethyst, Armada — can later refresh THIS coordinate instead of orphaning
|
||||
// the link at a dead epoch. That list is the liveness half of stranded recovery (A2).
|
||||
publishInviteList(
|
||||
ctx,
|
||||
extraRelays = relaysFor(ctx, sc),
|
||||
patch =
|
||||
// Record the link BEFORE publishing the bundle (CORD-05, kind 13303): a link whose
|
||||
// `signer_sk` was never stored can never be refreshed, so the next Refounding orphans
|
||||
// it and every holder is stranded. Better to mint nothing than to hand out a link that
|
||||
// is already doomed.
|
||||
val recorded =
|
||||
publishInviteList(
|
||||
ctx,
|
||||
ConcordInviteListDocument(
|
||||
entries =
|
||||
listOf(
|
||||
@@ -288,7 +285,16 @@ object ConcordCommands {
|
||||
),
|
||||
),
|
||||
),
|
||||
)
|
||||
)
|
||||
if (!recorded) {
|
||||
return Output.error(
|
||||
"invite_unrecordable",
|
||||
"could not record the link signer in your invite list (kind 13303), so this link could never be refreshed after a Refounding — not minting it",
|
||||
)
|
||||
}
|
||||
|
||||
val ack = ctx.publish(minted.bundleEvent, relaysFor(ctx, sc))
|
||||
RawEventSupport.publishGuard(ack, minted.bundleEvent.id)?.let { return it }
|
||||
|
||||
Output.emit(
|
||||
mapOf(
|
||||
@@ -318,6 +324,34 @@ object ConcordCommands {
|
||||
wraps.firstNotNullOfOrNull { ConcordActions.openBundle(it, parsed.fragment.token) }
|
||||
?: return Output.error("not_found", "no valid bundle for this link").let { 1 }
|
||||
|
||||
// Refuse a link that readmits us after we were removed. A Refounding re-mints every
|
||||
// outstanding link onto the new root (CORD-05), and an ex-member keeps the URL and its
|
||||
// unlock token forever — so without this check the rotation that was supposed to expel
|
||||
// them hands them the new keys instead. `recover` has always been ban-gated; `join` is
|
||||
// the other door into the same room.
|
||||
//
|
||||
// Fails CLOSED on an unreadable plane: no verdict, no join. The banlist is only knowable
|
||||
// after the bundle yields the root, which is why the check lives here rather than before.
|
||||
val joinKeys =
|
||||
ConcordActions.controlPlaneKeys(
|
||||
communityRoot = bundle.communityRoot.hexToByteArray(),
|
||||
communityId = bundle.communityId.hexToByteArray(),
|
||||
rootEpoch = bundle.rootEpoch,
|
||||
controlPk = bundle.controlPk,
|
||||
)
|
||||
val joinRelays = normalize(bundle.relays).ifEmpty { relays }
|
||||
val joinEditions =
|
||||
ConcordActions.controlEditions(
|
||||
ctx.drain(joinRelays.associateWith { listOf(ConcordActions.planeFilter(joinKeys.address)) }, pendingOnAuthRequired = true).map { it.second },
|
||||
joinKeys,
|
||||
)
|
||||
if (joinEditions.isEmpty()) {
|
||||
return Output.error("control_plane_unreadable", "could not fold this community's Control Plane, so whether it has banned you is unknown — refusing to join")
|
||||
}
|
||||
if (AuthorityResolver.resolve(joinEditions, bundle.owner).isBanned(ctx.signer.pubKey)) {
|
||||
return Output.error("banned", "this community has banned this account; the link works but the roster does not admit you (CORD-04)")
|
||||
}
|
||||
|
||||
ConcordStore(dataDir.concordFile).upsert(
|
||||
StoredCommunity(
|
||||
name = bundle.name,
|
||||
@@ -401,7 +435,7 @@ object ConcordCommands {
|
||||
rootEpoch = sc.rootEpoch,
|
||||
controlPk = sc.controlPk.ifBlank { null },
|
||||
controlRoot = sc.controlRoot.ifBlank { null },
|
||||
heldRoots = sc.heldRoots.map { HeldRoot(it.epoch, it.root, it.controlPk.ifBlank { null }) },
|
||||
heldRoots = sc.heldRoots.map { HeldRoot(it.epoch, it.root, it.controlPk.ifBlank { null }, it.controlRoot.ifBlank { null }) },
|
||||
relays = sc.relays,
|
||||
name = sc.name,
|
||||
inviteRef = sc.inviteRef.ifBlank { null },
|
||||
@@ -416,7 +450,7 @@ object ConcordCommands {
|
||||
rootEpoch = entry.rootEpoch,
|
||||
controlPk = entry.controlPk ?: "",
|
||||
controlRoot = entry.controlRoot ?: "",
|
||||
heldRoots = entry.heldRoots.map { StoredHeldRoot(it.epoch, it.key, it.controlPk ?: "") },
|
||||
heldRoots = entry.heldRoots.map { StoredHeldRoot(it.epoch, it.key, it.controlPk ?: "", it.controlRoot ?: "") },
|
||||
relays = entry.relays,
|
||||
name = entry.name.ifBlank { sc.name },
|
||||
inviteRef = entry.inviteRef ?: sc.inviteRef,
|
||||
@@ -585,36 +619,39 @@ object ConcordCommands {
|
||||
* of every link they minted, so a rotation can refresh those links instead of orphaning them.
|
||||
* Empty when none was ever published.
|
||||
*/
|
||||
suspend fun readInviteList(
|
||||
ctx: Context,
|
||||
extraRelays: Set<NormalizedRelayUrl> = emptySet(),
|
||||
): ConcordInviteListDocument {
|
||||
val relays = ctx.outboxRelays() + extraRelays
|
||||
if (relays.isEmpty()) return ConcordInviteListDocument.EMPTY
|
||||
suspend fun readInviteList(ctx: Context): ConcordInviteListDocument? {
|
||||
val relays = ctx.outboxRelays()
|
||||
if (relays.isEmpty()) return null
|
||||
val filter = Filter(kinds = listOf(ConcordInviteListEvent.KIND), authors = listOf(ctx.signer.pubKey))
|
||||
val newest =
|
||||
ctx
|
||||
.drain(relays.associateWith { listOf(filter) })
|
||||
.map { it.second }
|
||||
.maxByOrNull { it.createdAt }
|
||||
return (newest as? ConcordInviteListEvent)?.decrypt(ctx.signer) ?: ConcordInviteListDocument.EMPTY
|
||||
?: return ConcordInviteListDocument.EMPTY // nothing published yet — safe to start one
|
||||
return (newest as? ConcordInviteListEvent)?.decrypt(ctx.signer)
|
||||
}
|
||||
|
||||
/**
|
||||
* Merges [patch] into the published list and republishes it. Read-merge-write rather than
|
||||
* overwrite: the list is replaceable and per-creator, so two devices minting concurrently would
|
||||
* otherwise delete each other's links (and their `signer_sk`, which is unrecoverable).
|
||||
* Merges [patch] into the published list and republishes it, returning whether it landed.
|
||||
*
|
||||
* Read-merge-write, and **aborts rather than overwriting** when the read fails: kind 13303 is
|
||||
* replaceable, so writing a patch-only document over a list we could not read deletes every
|
||||
* other link's `signer_sk`. Those secrets cannot be regenerated, and losing one orphans its
|
||||
* link at the next rotation, stranding everyone holding that URL.
|
||||
*
|
||||
* Account-scoped, like the coordinate itself — (13303, me, "") is one list for every community,
|
||||
* so reading or writing it on a single community's relays would fork it.
|
||||
*/
|
||||
suspend fun publishInviteList(
|
||||
ctx: Context,
|
||||
patch: ConcordInviteListDocument,
|
||||
extraRelays: Set<NormalizedRelayUrl> = emptySet(),
|
||||
) {
|
||||
val relays = ctx.outboxRelays() + extraRelays
|
||||
if (relays.isEmpty()) return
|
||||
val merged = ConcordInviteList.merge(readInviteList(ctx, extraRelays), patch)
|
||||
val event = ConcordInviteListEvent.create(ctx.signer, merged, TimeUtils.now())
|
||||
ctx.publish(event, relays)
|
||||
): Boolean {
|
||||
val relays = ctx.outboxRelays()
|
||||
if (relays.isEmpty()) return false
|
||||
val base = readInviteList(ctx) ?: return false
|
||||
val event = ConcordInviteListEvent.create(ctx.signer, ConcordInviteList.merge(base, patch), TimeUtils.now())
|
||||
return ctx.publish(event, relays).values.any { it.accepted }
|
||||
}
|
||||
|
||||
fun notFound(handle: String): Int {
|
||||
|
||||
@@ -33,7 +33,9 @@ import com.vitorpamplona.quartz.concord.cord02Community.ConcordCommunityState
|
||||
import com.vitorpamplona.quartz.concord.cord04Roles.ConcordPermissions
|
||||
import com.vitorpamplona.quartz.concord.cord04Roles.ControlEdition
|
||||
import com.vitorpamplona.quartz.concord.cord04Roles.RoleEntity
|
||||
import com.vitorpamplona.quartz.concord.cord05Invites.InviteBundleStatus
|
||||
import com.vitorpamplona.quartz.concord.crypto.ControlPlaneKeys
|
||||
import com.vitorpamplona.quartz.nip01Core.core.Event
|
||||
import com.vitorpamplona.quartz.nip01Core.core.hexToByteArray
|
||||
import com.vitorpamplona.quartz.nip01Core.core.toHexKey
|
||||
import com.vitorpamplona.quartz.utils.RandomInstance
|
||||
@@ -272,26 +274,37 @@ object ConcordModCommands {
|
||||
// 1. Ban the removed on the CURRENT plane, so the compacted snapshot — and therefore the
|
||||
// new epoch — carries the ban. Each edition chains onto the updated banlist head.
|
||||
var chain = editions
|
||||
val banWraps = mutableListOf<Event>()
|
||||
for (target in removed) {
|
||||
val banWrap = ConcordModeration.ban(ctx.signer, cp, sc.communityId.hexToByteArray(), target, chain, TimeUtils.now(), owner = sc.owner)
|
||||
ctx.publish(banWrap, relays)
|
||||
val ack = ctx.publish(banWrap, relays)
|
||||
if (ack.values.none { it.accepted }) {
|
||||
return Output.error("ban_not_published", "the pre-rotation ban for $target was not accepted by any relay; refusing to refound with a banlist that would not survive")
|
||||
}
|
||||
banWraps += banWrap
|
||||
chain = chain + (ConcordActions.controlEditions(listOf(banWrap), cp))
|
||||
}
|
||||
|
||||
// 2. Everyone we are keeping. See the note above on why this reaches past the roster.
|
||||
val recipients =
|
||||
val candidates =
|
||||
(rosterOf(authority) + guestbookMembersOf(ctx, sc) + channelAuthorsOf(ctx, sc, state) + me)
|
||||
.mapTo(HashSet()) { it.lowercase() }
|
||||
.apply {
|
||||
removeAll(removed)
|
||||
removeAll(authority.bannedMembers().map { it.lowercase() }.toSet())
|
||||
}.toList()
|
||||
}
|
||||
val recipients = boundRecipients(candidates, authority)
|
||||
|
||||
// 3. Build: new root + fresh control_root, compacted plane, per-recipient blobs (staff
|
||||
// get the 136-byte form carrying the secret, everyone else the 104-byte pubkey one).
|
||||
val newRoot = RandomInstance.bytes(32)
|
||||
val newControlRoot = RandomInstance.bytes(32)
|
||||
val controlWraps = ctx.drain(relays.associateWith { listOf(ConcordActions.planeFilter(cp.address)) }, pendingOnAuthRequired = true).map { it.second }
|
||||
// Compact from what we KNOW the plane holds: the wraps we drained plus the bans we just
|
||||
// published. Re-draining alone would race the relay's indexing, and a relay that has not
|
||||
// yet echoed the ban back (or that ACKed and stored nothing) would produce a new epoch
|
||||
// whose roster never banned the member we are removing.
|
||||
val drained = ctx.drain(relays.associateWith { listOf(ConcordActions.planeFilter(cp.address)) }, pendingOnAuthRequired = true).map { it.second }
|
||||
val controlWraps = (drained + banWraps).distinctBy { it.id }
|
||||
val build =
|
||||
ConcordActions.buildRefounding(
|
||||
rotatorSigner = ctx.signer,
|
||||
@@ -335,33 +348,30 @@ object ConcordModCommands {
|
||||
// Safe for every link because recovery is ban-gated at the epoch being left, and step 1
|
||||
// banned everyone being removed — so a removed member's own `recover` is refused even
|
||||
// though their link now resolves.
|
||||
val refreshedInvite =
|
||||
ConcordActions.inviteFor(
|
||||
stored.communityId,
|
||||
stored.owner,
|
||||
stored.ownerSalt,
|
||||
stored.root,
|
||||
stored.rootEpoch,
|
||||
stored.name,
|
||||
stored.relays,
|
||||
stored.controlPk.ifBlank { null },
|
||||
)
|
||||
val now = TimeUtils.now()
|
||||
var refreshed = 0
|
||||
for (link in ConcordCommands.readInviteList(ctx, relays).entries) {
|
||||
val list = ConcordCommands.readInviteList(ctx)
|
||||
val tombstoned = list?.tombstones?.mapTo(HashSet()) { it.token } ?: emptySet<String>()
|
||||
for (link in list?.entries.orEmpty()) {
|
||||
if (link.communityId != stored.communityId) continue
|
||||
// An elapsed link can no longer be joined, so re-posting it would only resurrect a
|
||||
// dead URL at a live epoch (CORD-05).
|
||||
if (link.isExpired(now)) continue
|
||||
// An elapsed or retired link can no longer be joined, so re-posting it would only
|
||||
// resurrect a dead URL at a live epoch (CORD-05).
|
||||
if (link.isExpired(now) || link.token in tombstoned) continue
|
||||
runCatching {
|
||||
val event =
|
||||
ConcordActions.remintBundleAt(
|
||||
linkSignerPrivKey = link.signerSk.hexToByteArray(),
|
||||
token = link.token.hexToByteArray(),
|
||||
invite = refreshedInvite,
|
||||
createdAt = now,
|
||||
val token = link.token.hexToByteArray()
|
||||
// Refresh from the link's CURRENT bundle so its own fields — expiry, channel
|
||||
// grants, icon, label — survive the rotation, and so a coordinate whose newest
|
||||
// event is a revocation tombstone is left revoked instead of being re-opened.
|
||||
val wraps = ctx.drain(relays.associateWith { listOf(ConcordActions.bundleFilter(link.signerPubKeyHex())) }).map { it.second }
|
||||
val live = ConcordActions.classifyInvite(wraps, token) as? InviteBundleStatus.Live ?: return@runCatching
|
||||
val moved =
|
||||
live.invite.copy(
|
||||
communityRoot = stored.root,
|
||||
rootEpoch = stored.rootEpoch,
|
||||
controlPk = stored.controlPk.ifBlank { null },
|
||||
relays = stored.relays,
|
||||
)
|
||||
ctx.publish(event, relays)
|
||||
ctx.publish(ConcordActions.remintBundleAt(link.signerSk.hexToByteArray(), token, moved, now), relays)
|
||||
refreshed++
|
||||
}
|
||||
}
|
||||
@@ -382,6 +392,40 @@ object ConcordModCommands {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* How many recipients one Refounding will re-key, mirroring Amethyst's own cap.
|
||||
*
|
||||
* Two thirds of the recipient union — Guestbook joins and observed channel authors — are
|
||||
* attacker-writable: any key can announce a join or post once. Without a bound, padding those
|
||||
* sets inflates the cost of the only hard removal Concord has until rotating becomes
|
||||
* impractical, so the attack raises the price of its own remedy (B4 in the soft-ban audit).
|
||||
*/
|
||||
private const val MAX_REFOUNDING_RECIPIENTS = 5_000
|
||||
|
||||
/**
|
||||
* Caps [candidates], keeping the members whose standing is owner-rooted and therefore cannot be
|
||||
* padded from outside. Anything dropped is reported rather than silently truncated — a dropped
|
||||
* member is stranded on the dead epoch and their only way back is `concord recover`.
|
||||
*/
|
||||
private fun boundRecipients(
|
||||
candidates: Set<String>,
|
||||
authority: com.vitorpamplona.quartz.concord.cord04Roles.AuthorityResolver,
|
||||
): List<String> {
|
||||
if (candidates.size <= MAX_REFOUNDING_RECIPIENTS) return candidates.toList()
|
||||
val vouched = (authority.roleHolders() + authority.staffMembers()).mapTo(HashSet()) { it.lowercase() }
|
||||
val kept = LinkedHashSet<String>()
|
||||
candidates.filterTo(kept) { it in vouched }
|
||||
for (candidate in candidates) {
|
||||
if (kept.size >= MAX_REFOUNDING_RECIPIENTS) break
|
||||
kept.add(candidate)
|
||||
}
|
||||
val dropped = candidates.size - kept.size
|
||||
if (dropped > 0) {
|
||||
System.err.println("[concord] refounding recipient set trimmed to ${kept.size} of ${candidates.size}: $dropped member(s) will be stranded on the prior epoch")
|
||||
}
|
||||
return kept.toList()
|
||||
}
|
||||
|
||||
/** Owner + everyone holding a role — owner-rooted, so it cannot be padded from outside. */
|
||||
private fun rosterOf(authority: com.vitorpamplona.quartz.concord.cord04Roles.AuthorityResolver): Set<String> = (authority.roleHolders() + authority.staffMembers()).mapTo(HashSet()) { it.lowercase() }
|
||||
|
||||
|
||||
@@ -61,6 +61,12 @@ data class StoredHeldRoot(
|
||||
val root: String = "",
|
||||
/** That epoch's Control Plane address; blank for a legacy, pre-split epoch (CORD-02 §5). */
|
||||
val controlPk: String = "",
|
||||
/**
|
||||
* That epoch's staff write key, banked only if we held it. A relay that gates the prior epoch's
|
||||
* Control Plane on NIP-42 AUTH as the stream key will not serve those wraps without it — and
|
||||
* those wraps are what rebuild the anti-rollback floor.
|
||||
*/
|
||||
val controlRoot: String = "",
|
||||
)
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user