diff --git a/docs/account.md b/docs/account.md index 014f618462..7c90dc40cb 100644 --- a/docs/account.md +++ b/docs/account.md @@ -267,10 +267,14 @@ is what authenticates the PC. close at once (`auth_revoked`). A `remove` signed before this PC itself paired the key again leaves that pairing be (`unchanged`), so a late or replayed one cannot undo it. A row an `add` or a remote approval made goes, however late the `remove` arrives. -- **Revoke** and **Revoke all** on the PC leave a tombstone too, so no record brings the key back. +- **Revoke** and **Revoke all** on the PC leave a tombstone too, so no record brings the key back, + even after the PC joins an account. An account `remove` for a key the PC revoked first takes over + its tombstone. - A pairing made on this PC (a code started on the PC, or a comparison approved here) clears that key's tombstone, after its row is written. A code started through the relay, or a comparison an - account member approves remotely, is refused for a tombstoned key. + account member approves remotely, is refused for a tombstoned key, except on a PC in no account: + there a code started through the relay may let back a key the PC revoked itself (not one the + account removed), and clears its tombstone. - A stop between two such writes leaves a row and a tombstone for one key. On start the PC tells them apart by time: a row made before the key's removal is revoked, one made after it keeps its device and the tombstone goes. Until then no such row signs in, signs or approves. diff --git a/server/account/account.contract.test.ts b/server/account/account.contract.test.ts index 61c0dc5e70..02e115ae2d 100644 --- a/server/account/account.contract.test.ts +++ b/server/account/account.contract.test.ts @@ -15,6 +15,7 @@ import { exportPublicKey, generateKeyPair, importPublicKey } from "../../shared/ import { bytesToHex } from "@noble/hashes/utils.js"; import { deriveServerId } from "../../shared/relay-server-proof.ts"; import { applyAccountRecords } from "./records.ts"; +import { createServer } from "../index.ts"; type Fixture = ReturnType; type Link = Awaited>; @@ -751,6 +752,110 @@ test("a waiting remove survives a stop right after the service took its page's a expect(f.app.context.services.devices.hasKey(c.spki)).toBe(false); }); +test("a key paired again in one of two state dirs stays paired, without its tombstone, when the other starts", async () => { + const f = fixture(); + const { key, link } = await member(f); + const gone = await accountKey(); + await post(link, "/api/account/records", { records: [await add(key, gone.spki)] }); + await post(link, "/api/account/records", { records: [await remove(key, gone.spki)] }); + const sibling = mkdtempSync(join(tmpdir(), "ovis-account-sibling-")); + // both dirs share the registry and the removal + for (const name of ["devices.json", "account-tombstones.json"]) writeFileSync(join(sibling, name), readFileSync(join(f.stateDir, name)), { mode: 0o600 }); + const store = f.app.context.services.devices; + // a second after the removal, as a person pairing again would be + const later = Date.now() + 1_000; + expect(store.pairRelay(store.startPairing(later).code, "Mac app", gone.spki, later)).not.toBeNull(); + const other = createServer({ port: 0, hostname: "127.0.0.1", stateDir: sibling, siblingStateDir: f.stateDir, token: "t", tailscaleOwner: null, machines: false, sidecar: false }); + try { + // the merge brings the other dir's tombstone back, and the start-up repair drops it: the row is newer + for (const devices of [other.context.services.devices, new DeviceStore(f.stateDir)]) { + expect(devices.hasKey(gone.spki)).toBe(true); + expect(devices.isTombstoned(gone.spki)).toBe(false); + } + } finally { + other.stop(); + rmSync(sibling, { recursive: true, force: true }); + } +}); + +test("a key revoked before the PC joined an account stays out: a member's signed add for it is refused", async () => { + const f = fixture(); + const store = f.app.context.services.devices; + const revoked = await accountKey(); + const before = store.pairRelay(store.startPairing().code, "Old phone", revoked.spki); + if (!before) throw new Error("Expected relay enrollment"); + store.revoke(before.device.id); + const { key, link } = await member(f); + const outcome = await post(link, "/api/account/records", { records: [await add(key, revoked.spki)] }); + expect((outcome.body as unknown as { results: { outcome: string; code?: string }[] }).results[0]).toMatchObject({ outcome: "refused", code: "tombstoned" }); + expect(store.hasKey(revoked.spki)).toBe(false); +}); + +test("an account remove for a key this PC revoked first makes its tombstone the account's", async () => { + const f = fixture(); + const { key, link } = await member(f); + const store = f.app.context.services.devices; + const gone = await accountKey(); + await post(link, "/api/account/records", { records: [await add(key, gone.spki)] }); + store.revoke(accountDeviceId(SERVER_ID, gone.spki)); + const removal = await remove(key, gone.spki); + await post(link, "/api/account/records", { records: [removal] }); + const kept = JSON.parse(readFileSync(join(f.stateDir, "account-tombstones.json"), "utf8")) as { tombstones: { key: string; record_id: string }[] }; + expect(kept.tombstones.find((t) => t.key === gone.spki)?.record_id).toBe(removal.id); +}); + +test("a key revoked on this PC stays out when a sibling dir that missed the revoke added it through the account", async () => { + const f = fixture(); + const { key, link } = await member(f); + const store = f.app.context.services.devices; + const gone = await accountKey(); + await post(link, "/api/account/records", { records: [await add(key, gone.spki)] }); + const sibling = mkdtempSync(join(tmpdir(), "ovis-account-sibling-")); + try { + // the sibling dir still has the account's row; this dir then revokes it on the PC + writeFileSync(join(sibling, "devices.json"), readFileSync(join(f.stateDir, "devices.json")), { mode: 0o600 }); + store.revoke(accountDeviceId(SERVER_ID, gone.spki)); + const registry = JSON.parse(readFileSync(join(sibling, "devices.json"), "utf8")) as { devices: { relay_public_key?: string; created_at: string }[] }; + // made a second after the revoke, as the sibling knew nothing of it + for (const row of registry.devices) if (row.relay_public_key === gone.spki) row.created_at = new Date(Date.now() + 1_000).toISOString(); + writeFileSync(join(sibling, "devices.json"), JSON.stringify(registry), { mode: 0o600 }); + const app = createServer({ port: 0, hostname: "127.0.0.1", stateDir: f.stateDir, siblingStateDir: sibling, token: "t", tailscaleOwner: null, machines: false, sidecar: false }); + try { + expect(app.context.services.devices.hasKey(gone.spki)).toBe(false); + } finally { app.stop(); } + } finally { rmSync(sibling, { recursive: true, force: true }); } +}); + +test("merging two dirs keeps the account's removal of a key over a revoke on the PC, with the later time", () => { + const [a, b] = [mkdtempSync(join(tmpdir(), "ovis-account-a-")), mkdtempSync(join(tmpdir(), "ovis-account-b-"))]; + try { + const key = "a-removed-key"; + writeFileSync(join(a, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: [{ key, account: ACCOUNT, record_id: "r1", removed_at: "2026-11-01T00:00:00.000Z" }] })); + writeFileSync(join(b, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: [{ key, account: "", record_id: "local", removed_at: "2026-11-02T00:00:00.000Z" }] })); + expect(mergeAccountTombstones(a, b)).toBeNull(); + for (const dir of [a, b]) { + const kept = JSON.parse(readFileSync(join(dir, "account-tombstones.json"), "utf8")) as { tombstones: { record_id: string; removed_at: string }[] }; + expect(kept.tombstones.map((t) => [t.record_id, t.removed_at])).toEqual([["r1", "2026-11-02T00:00:00.000Z"]]); + } + } finally { for (const dir of [a, b]) rmSync(dir, { recursive: true, force: true }); } +}); + +test("merging two dirs rewrites a dir whose tombstone for the key is the PC's revoke at the same time as the account's removal", () => { + const [a, b] = [mkdtempSync(join(tmpdir(), "ovis-account-a-")), mkdtempSync(join(tmpdir(), "ovis-account-b-"))]; + try { + const key = "a-removed-key"; + const at = "2026-11-02T00:00:00.000Z"; + writeFileSync(join(a, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: [{ key, account: "", record_id: "local", removed_at: at }] })); + writeFileSync(join(b, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: [{ key, account: ACCOUNT, record_id: "r1", removed_at: at }] })); + expect(mergeAccountTombstones(a, b)).toBeNull(); + for (const dir of [a, b]) { + const kept = JSON.parse(readFileSync(join(dir, "account-tombstones.json"), "utf8")) as { tombstones: { record_id: string }[] }; + expect(kept.tombstones.map((t) => t.record_id)).toEqual(["r1"]); + } + } finally { for (const dir of [a, b]) rmSync(dir, { recursive: true, force: true }); } +}); + + test("a watching member leaves the account on a PC with a remove signed by its own key", async () => { const f = fixture(); const { key, link } = await member(f); diff --git a/server/device-access.contract.test.ts b/server/device-access.contract.test.ts index 9993983924..09ddd5d205 100644 --- a/server/device-access.contract.test.ts +++ b/server/device-access.contract.test.ts @@ -1180,3 +1180,73 @@ it("a script's first pairing on a fresh install is kept at once, so its token st rmSync(stateDir, { recursive: true, force: true }); } }); + +it("a PC in no account lets a revoked key pair again with a code started through the relay, which clears its tombstone", async () => { + const stateDir = mkdtempSync(join(tmpdir(), "ovis-relay-repair-")); + try { + const store = new DeviceStore(stateDir); + const keys = await crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign", "verify"]); + const publicKey = Buffer.from(await crypto.subtle.exportKey("spki", keys.publicKey)).toString("base64url"); + const first = store.pairRelay(store.startPairing().code, "Relay phone", publicKey); + if (!first) throw new Error("Expected relay enrollment"); + store.revoke(first.device.id); + expect(store.isTombstoned(publicKey)).toBe(true); + const again = store.pairRelay(store.startPairing(Date.now(), { remote: true, startedBy: null, memberRequired: false }).code, "Relay phone", publicKey); + expect(again?.device.label).toBe("Relay phone"); + expect(new DeviceStore(stateDir).isTombstoned(publicKey)).toBe(false); + } finally { + rmSync(stateDir, { recursive: true, force: true }); + } +}); + +it("a PC in no account still refuses, with a code started through the relay, a key its account removed", async () => { + const stateDir = mkdtempSync(join(tmpdir(), "ovis-relay-account-removed-")); + try { + const keys = await crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign", "verify"]); + const publicKey = Buffer.from(await crypto.subtle.exportKey("spki", keys.publicKey)).toString("base64url"); + writeFileSync(join(stateDir, "account-tombstones.json"), JSON.stringify({ v: 1, tombstones: [{ key: publicKey, account: "acct_1", record_id: "r1", removed_at: "2026-11-01T00:00:00.000Z" }] }), { mode: 0o600 }); + const store = new DeviceStore(stateDir); + expect(store.pairRelay(store.startPairing(Date.now(), { remote: true, startedBy: null, memberRequired: false }).code, "Phone", publicKey)).toBeNull(); + expect(store.isTombstoned(publicKey)).toBe(true); + } finally { + rmSync(stateDir, { recursive: true, force: true }); + } +}); + +it("a PC in no account whose tombstone file will not parse refuses a code started through the relay", async () => { + const stateDir = mkdtempSync(join(tmpdir(), "ovis-relay-tombstones-broken-")); + try { + writeFileSync(join(stateDir, "account-tombstones.json"), "{broken", { mode: 0o600 }); + const store = new DeviceStore(stateDir); + const keys = await crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign", "verify"]); + const publicKey = Buffer.from(await crypto.subtle.exportKey("spki", keys.publicKey)).toString("base64url"); + expect(store.pairRelay(store.startPairing(Date.now(), { remote: true, startedBy: null, memberRequired: false }).code, "Phone", publicKey)).toBeNull(); + } finally { + rmSync(stateDir, { recursive: true, force: true }); + } +}); + +it("a key re-paired on a PC in no account with a code another device started stays when its tombstone comes back", async () => { + const stateDir = mkdtempSync(join(tmpdir(), "ovis-relay-repair-starter-")); + try { + const store = new DeviceStore(stateDir); + const fresh = async () => { + const keys = await crypto.subtle.generateKey({ name: "ECDSA", namedCurve: "P-256" }, true, ["sign", "verify"]); + return Buffer.from(await crypto.subtle.exportKey("spki", keys.publicKey)).toString("base64url"); + }; + const [starterKey, publicKey] = [await fresh(), await fresh()]; + const starter = store.pairRelay(store.startPairing().code, "Laptop", starterKey); + const first = store.pairRelay(store.startPairing().code, "Phone", publicKey); + if (!starter || !first) throw new Error("Expected relay enrollment"); + store.revoke(first.device.id); + const tombstones = readFileSync(join(stateDir, "account-tombstones.json")); + // a second later, as a person pairing again would be + const later = Date.now() + 1_000; + expect(store.pairRelay(store.startPairing(later, { remote: true, startedBy: starter.device.id, memberRequired: false }).code, "Phone", publicKey, later)).not.toBeNull(); + // the revoke's tombstone back, as a stop before it was cleared or a sibling dir's merge leaves it + writeFileSync(join(stateDir, "account-tombstones.json"), tombstones, { mode: 0o600 }); + expect(new DeviceStore(stateDir).hasKey(publicKey)).toBe(true); + } finally { + rmSync(stateDir, { recursive: true, force: true }); + } +}); diff --git a/server/devices.ts b/server/devices.ts index 1d89eee65c..4b0d03dd36 100644 --- a/server/devices.ts +++ b/server/devices.ts @@ -206,11 +206,16 @@ export function mergeAccountTombstones(active: string, sibling: string): string const merged = new Map(); for (const tombstone of [...read.flatMap((list) => list ?? []), ...pending]) { const kept = merged.get(tombstone.key); - if (!kept || tombstone.removed_at > kept.removed_at) merged.set(tombstone.key, tombstone); + if (!kept) { merged.set(tombstone.key, tombstone); continue; } + // the account's removal outranks a revoke on the PC (only it keeps a relay code out), with the later of the two times + const local = (t: AccountTombstone) => t.record_id === "local"; + const winner = local(kept) && !local(tombstone) ? tombstone : !local(kept) && local(tombstone) ? kept : tombstone.removed_at > kept.removed_at ? tombstone : kept; + const removedAt = tombstone.removed_at > kept.removed_at ? tombstone.removed_at : kept.removed_at; + merged.set(tombstone.key, { ...winner, removed_at: removedAt }); } const tombstones = [...merged.values()]; for (const [index, path] of paths.entries()) { - if ((read[index] ?? []).length !== tombstones.length || tombstones.some((t) => !read[index]?.some((own) => own.key === t.key && own.removed_at === t.removed_at))) { + if ((read[index] ?? []).length !== tombstones.length || tombstones.some((t) => !read[index]?.some((own) => own.key === t.key && own.removed_at === t.removed_at && own.record_id === t.record_id))) { writePrivateState(path, { v: 1, tombstones }); } } @@ -479,14 +484,21 @@ export class DeviceStore { // a code started through the relay is not a pairing made on this PC (docs/account.md, rule 5) const remote = this.pending?.remote ?? false; const startedBy = this.pending?.startedBy ?? null; - // a code started through the relay is not a pairing made on this PC; a member's code goes with the member - if ((remote && (this.accountError !== null || this.isTombstoned(publicKey))) || (this.pending?.memberRequired && (startedBy === null || !this.isAccountApprover(startedBy)))) { + // a code started through the relay is not a pairing made on this PC; a member's code goes with the member. + // On a PC in no account such a code may bring back a key this PC revoked itself, as before account linking; + // never one the account removed + const accountless = this.account() === null; + const refusedKey = accountless ? this.tombstones.some((t) => t.key === publicKey && t.record_id !== "local") : this.isTombstoned(publicKey); + if ((remote && (this.accountError !== null || refusedKey)) || (this.pending?.memberRequired && (startedBy === null || !this.isAccountApprover(startedBy)))) { this.noteRelayFailure("code", now); return null; } - // a device let in by a code started through the relay is its starter's addition, not a pairing made on this PC (removeAccountKey) - const paired = this.enrollRelay(label, publicKey, now, true, startedBy ?? undefined); - if (!remote) this.clearTombstone(publicKey); + // a device let in by a code started through the relay is its starter's addition, not a pairing made on this PC (removeAccountKey); + // one that brings back a key this PC revoked itself (on a PC in no account) undoes that revoke, and is the PC's own + // pairing, so a tombstone that comes back (a sibling dir, a stop before it is cleared) takes it only if newer + const undoesRevoke = remote && accountless && this.isTombstoned(publicKey); + const paired = this.enrollRelay(label, publicKey, now, true, undoesRevoke ? undefined : startedBy ?? undefined); + if (!remote || accountless) this.clearTombstone(publicKey); return paired; } @@ -774,7 +786,10 @@ export class DeviceStore { if (this.devices.some((d) => d.relay_public_key === key && d.added_by === undefined && d.created_at > record.issuedAt)) return false; const recordId = record.id; const fresh = !this.isTombstoned(key); - if (fresh) this.writeTombstones([...this.tombstones, { key, account, record_id: recordId, removed_at: new Date(now).toISOString() }]); + const removal = { key, account, record_id: recordId, removed_at: new Date(now).toISOString() }; + if (fresh) this.writeTombstones([...this.tombstones, removal]); + // a key this PC revoked first becomes the account's removal, so a code started through the relay cannot bring it back + else if (this.tombstones.some((t) => t.key === key && t.record_id === "local")) this.writeTombstones(this.tombstones.map((t) => t.key === key ? removal : t)); const ids = this.devices.filter((d) => d.relay_public_key === key).map((d) => d.id); for (const id of ids) this.revoke(id, record.by); return fresh || ids.length > 0;