diff --git a/docs/per-user-linux-accounts.md b/docs/per-user-linux-accounts.md index 7e92c825..15ed736a 100644 --- a/docs/per-user-linux-accounts.md +++ b/docs/per-user-linux-accounts.md @@ -164,18 +164,60 @@ requests over a socket is a bigger promise than this feature needs to make. ## Naming -`officer_`, where `` is `toShellUsername(username, email)` — the existing -sanitiser, which already lowercases, strips `@…`, replaces illegal characters and truncates to 32. The -combined name is truncated to 32 again. +**The username the owner chose, verbatim.** `whoami` in a member's terminal says who they are, their +prompt is their name, and a commit from their edge checkout is attributed to something recognisable. -The prefix earns its ugliness three times: it cannot collide with a system account, it makes every -account this feature created greppable in `/etc/passwd`, and it means a member cannot pick a username -that shadows something real. +This carried an `officer_` prefix for about an hour. The prefix bought three things — no collision with a +system account, a greppable record of what the feature created, and a member unable to pick a name that +shadows something real — and cost the only thing anyone would notice. Measured before removing it: +`useradd` on this host accepts everything `validateUsername` already permits, including dots, hyphens, +underscores and uppercase. + +**What replaced the prefix's safety is the adoption rule, and it had to.** `ensureOsUser` reuses an +existing Linux account, which is what makes it re-runnable. That was safe by construction while only we +created `officer_*` names. With the name being whatever was typed, adoption became the dangerous path: a +platform account named `root` would have found root in passwd, and every `runAs` for that member would +have been a root shell. So an existing account is adopted **only when its passwd home is already exactly +the home we are about to confine** — that is what makes it ours — and any uid below 1000 is refused +outright as belt and braces. + +Verified: + +``` +username "root" -> refused: 'root' is a system account on this machine. +username "daemon" -> refused: 'daemon' is a system account on this machine. +the owner's own account -> refused: 'pastilhas' is already a user on this machine, with its + home at /home/pastilhas. Refusing to take it over. +``` The resolved name is **stored** on the user row (`users.os_user`) rather than re-derived. `useradd` can adjust or refuse a name, and re-deriving would mean the platform's idea of who a member is could drift from what is actually in `/etc/passwd`. +## The ancestor trap + +A member's home sits under `DATA_PATH`, which on a normal install sits under the **owner's** home — and +`/home/` is `750` on Debian and Ubuntu. Every mode bit on the account tree can be correct, the +directory can exist, and the member still cannot reach it, because they have no `x` on an ancestor four +levels up. + +What that surfaced as, on the first real install: + +``` +ssh-keygen failed: Could not stat …/data/jg@pertento.ai/home/.ssh: Permission denied +``` + +Which points at exactly the wrong thing. `.ssh` was there and correctly owned; the account could not +traverse `/home/pastilhas`. + +`firstUntraversableAncestor` now walks the chain **as the member** before anything tries to use the home, +and the error names the directory and the fix (`chmod o+x `). `x` without `r` is the ask throughout: +traversal, not listing — nobody gains the ability to enumerate the owner's home. + +The development machine happened to be `751` already, which is exactly why the probe passed there and +failed on a fresh install. Worth remembering as a shape of mistake: the probe used `/tmp`, so it never +crossed the ancestor that mattered. + ## Out of scope, and honest about it - **This is not a sandbox.** A member with a shell is on the machine. They cannot read the owner's files diff --git a/src/servers/os-user.test.ts b/src/servers/os-user.test.ts index 4048ddb1..4a6f7329 100644 --- a/src/servers/os-user.test.ts +++ b/src/servers/os-user.test.ts @@ -2,7 +2,7 @@ import { describe, expect, test } from 'bun:test'; import { chmod, mkdtemp, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { findReadableSecrets, osUserNameFor, runAsArgv, OS_USER_PREFIX } from './os-user'; +import { findReadableSecrets, osUserNameFor, runAsArgv } from './os-user'; // The tests that matter here are the two that prove the MECHANISM rather than the plumbing: that Bun // ignores `uid`, and that `setpriv` does not. Everything else in os-user.ts touches the passwd database @@ -119,28 +119,29 @@ describe('runAsArgv', () => { }); describe('osUserNameFor', () => { - test('prefixes so it cannot collide with a system account', () => { - expect(osUserNameFor({ username: 'ana', email: 'ana@example.com' })).toBe(`${OS_USER_PREFIX}ana`); + // The chosen username, verbatim — so `whoami` in a member's terminal says who they are. Measured on this + // host: useradd accepts dots, hyphens, underscores and uppercase, i.e. everything validateUsername lets + // through. + test('uses the chosen username as-is', () => { + expect(osUserNameFor({ username: 'ana', email: 'ana@example.com' })).toBe('ana'); + expect(osUserNameFor({ username: 'ana.silva', email: 'a@b.com' })).toBe('ana.silva'); + expect(osUserNameFor({ username: 'Ana-Silva_2', email: 'a@b.com' })).toBe('Ana-Silva_2'); }); test('falls back to the email local part when there is no username', () => { - expect(osUserNameFor({ username: null, email: 'Ana.Silva@example.com' })).toBe(`${OS_USER_PREFIX}ana.silva`); + expect(osUserNameFor({ username: null, email: 'Ana.Silva@example.com' })).toBe('ana.silva'); + expect(osUserNameFor({ username: ' ', email: 'Ana.Silva@example.com' })).toBe('ana.silva'); }); - test('sanitises what useradd would refuse', () => { - expect(osUserNameFor({ username: 'Ana Silva!', email: 'a@b.com' })).toBe(`${OS_USER_PREFIX}ana_silva_`); + test('stays within the 32-character limit useradd enforces', () => { + expect(osUserNameFor({ username: 'a'.repeat(40), email: 'a@b.com' })).toHaveLength(32); }); - // The prefix can push an already-32-char sanitised name over the limit, and useradd rejects the whole - // name rather than truncating it. - test('stays within the 32-character limit', () => { - const name = osUserNameFor({ username: 'a'.repeat(40), email: 'a@b.com' }); - expect(name.length).toBe(32); - expect(name.startsWith(OS_USER_PREFIX)).toBe(true); - }); - - test('a username that tries to shadow root is still prefixed', () => { - expect(osUserNameFor({ username: 'root', email: 'r@b.com' })).toBe(`${OS_USER_PREFIX}root`); + // No longer defended by a prefix, so it must be defended by adoption rules instead: `ensureOsUser` + // refuses a name whose existing passwd home is not the one we are about to confine, and refuses any uid + // below 1000 outright. This test records that the NAME itself is no longer the protection. + test('does not neutralise a dangerous name — that is ensureOsUser-s job now', () => { + expect(osUserNameFor({ username: 'root', email: 'r@b.com' })).toBe('root'); }); }); diff --git a/src/servers/os-user.ts b/src/servers/os-user.ts index 7dca6788..fac767a9 100644 --- a/src/servers/os-user.ts +++ b/src/servers/os-user.ts @@ -21,21 +21,35 @@ import { DATA_PATH, HOME_SEED_DIRS, USER_DIRS, toShellUsername } from './data-pa /** Off unless explicitly enabled: this needs root, and a light install has no sudoers entry. */ export const OS_USERS_ENABLED = process.env.OFFICER_OS_USERS === 'true' || process.env.OFFICER_OS_USERS === '1'; -/** Prefixed so it cannot collide with a system account, and so `/etc/passwd` shows what we created. */ -export const OS_USER_PREFIX = 'officer_'; - const MAX_USERNAME = 32; /** - * The Linux account name for a platform account. + * The Linux account name for a platform account: **the username the owner chose**, verbatim. * - * Built on `toShellUsername`, which already lowercases, strips anything from `@` on, replaces illegal - * characters and truncates. Truncated again after the prefix, because the prefix can push a 32-char - * result over the limit and `useradd` would refuse the whole thing. + * This carried an `officer_` prefix until 2026-08-11. The prefix bought two things — no collision with a + * system account, and a greppable record of what we created — and cost the thing the owner actually wants, + * which is that `whoami` in a member's terminal says who they are. Measured before removing it: `useradd` + * on this host accepts everything `validateUsername` permits, including dots, hyphens, underscores and + * uppercase. + * + * What replaced the prefix's safety is a stricter ADOPTION rule in `ensureOsUser`: an existing Linux account + * is only reused when its passwd home is already the home we expect. Without that, a platform account named + * `root` would have adopted root. See there. + * + * `toShellUsername` still handles the fallback when there is no username, since the email local part can + * contain things a Linux name cannot. */ export function osUserNameFor(params: { username: string | null; email: string }): string { - const base = toShellUsername(params.username ?? '', params.email); - return `${OS_USER_PREFIX}${base}`.slice(0, MAX_USERNAME); + const chosen = params.username?.trim(); + if (chosen) return chosen.slice(0, MAX_USERNAME); + return toShellUsername('', params.email).slice(0, MAX_USERNAME); +} + +/** The account's home as passwd records it, or null if it has none / does not exist. */ +async function passwdHome(osUser: string): Promise { + const result = await run(['getent', 'passwd', osUser]); + if (!result.ok) return null; + return result.out.split(':')[5] ?? null; } export type RunAsOptions = { @@ -120,6 +134,34 @@ export async function lookupOsUser(osUser: string): Promise<{ uid: number; gid: /** The member's home: `DATA_PATH//home`, where `provisionUserDirs` already put it. */ export const osUserHome = (email: string): string => join(DATA_PATH, email, 'home'); +/** + * The first directory on the way to `path` that `osUser` cannot traverse, or null if the whole chain is fine. + * + * Exists because of a real and thoroughly confusing failure. A member's home is under `DATA_PATH`, which on + * a normal install is under the OWNER'S home — and `/home/` is 750 on Debian and Ubuntu. So every + * mode bit we set on the account tree was correct, the directory existed, and the member still could not + * reach it, because they had no `x` on an ancestor four levels up. What surfaced was + * `ssh-keygen: Could not stat …/.ssh: Permission denied`, which points at exactly the wrong place. + * + * `x` without `r` is the ask: traversal, not listing. Nobody gains the ability to enumerate the owner's home. + */ +export async function firstUntraversableAncestor(osUser: string, path: string): Promise { + const parts = path.split('/').filter(Boolean); + const chain = parts.map((_, i) => `/${parts.slice(0, i + 1).join('/')}`); + + // One spawn rather than one per level: this runs on every account creation, and the chain is ~6 deep. + const proc = runAs(osUser, [ + 'sh', + '-c', + 'for p in "$@"; do [ -x "$p" ] || { printf %s "$p"; exit 0; }; done', + 'sh', + ...chain, + ]); + const out = (await new Response(proc.stdout).text()).trim(); + await proc.exited; + return out || null; +} + export type EnsureOsUserResult = | { ok: true; osUser: string; uid: number; gid: number; created: boolean } | { ok: false; error: string }; @@ -142,6 +184,34 @@ export async function ensureOsUser(params: { email: string; username: string | n let created = false; let ids = await lookupOsUser(osUser); + // ── Adoption, and why it is this strict ── + // + // An account that already exists is REUSED, which is what makes this function re-runnable. While names + // carried an `officer_` prefix that was safe by construction: nothing else creates those. Now that the + // name is whatever the owner typed, adoption is the dangerous path — a platform account named `root` + // would find root in passwd, and every `runAs` for that member would then be a root shell. + // + // The test is the account's own home. If passwd already points it at exactly the directory we are about + // to confine, it is ours (or a previous run's). Anything else is somebody else's account that happens to + // share a name, and the answer is to refuse rather than to touch it. + // + // The uid floor is belt and braces: a system account below 1000 could in principle be created with a + // matching home, and none of them should ever be handed to a member. + if (ids) { + const existingHome = await passwdHome(osUser); + if (ids.uid < 1000) { + return { ok: false, error: `'${osUser}' is a system account on this machine. Choose another username.` }; + } + if (existingHome !== home) { + return { + ok: false, + error: + `'${osUser}' is already a user on this machine, with its home at ${existingHome ?? 'an unknown path'}. ` + + `Refusing to take it over — choose another username.`, + }; + } + } + if (!ids) { // `-M` because provisionUserDirs already made the directory, and letting useradd create it would // copy /etc/skel in as root-owned. `-s` explicitly: /etc/default/useradd here says /bin/sh, and a @@ -166,6 +236,20 @@ export async function ensureOsUser(params: { email: string; username: string | n const confined = await confineUserTree({ email: params.email, uid: ids.uid, gid: ids.gid }); if (!confined.ok) return { ok: false, error: confined.error }; + // Checked before anything tries to USE the home, so the error names the actual problem. Every mode bit on + // the account tree can be right while the member still cannot get there, because `DATA_PATH` normally + // lives under the owner's home and `/home/` is 750 on Debian and Ubuntu. + const blocked = await firstUntraversableAncestor(osUser, home); + if (blocked) { + return { + ok: false, + error: + `${osUser} cannot traverse into ${blocked}, so it cannot reach its own home. ` + + `Fix with: chmod o+x ${blocked} ` + + `(that grants traversal only — the directory stays unlistable.)`, + }; + } + // Seeded AS the member, because after the chown above their home is 700 and theirs — the platform cannot // write into it, which is exactly the point. Best-effort: an empty file browser is a cosmetic problem, and // failing the whole account creation over Downloads/ would be absurd.