From f04d63dd7b4c8ea90f6e59592b248752a2e1caa8 Mon Sep 17 00:00:00 2001 From: tomdebone Date: Thu, 10 Sep 2026 21:06:30 +0200 Subject: [PATCH] =?UTF-8?q?feat(pds):=20Einladungscodes=20f=C3=BCr=20creat?= =?UTF-8?q?eAccount?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Die Instanz soll öffentlich erreichbar werden. createAccount hatte bis jetzt keinerlei Schranke: kein Code, kein Rate-Limit, describeServer meldete invite_code_required hartkodiert false. Jeder hätte beliebig viele Konten anlegen können, jedes mit eigenem Repo, Blöcken und Firehose-Events. Zwei Tabellen: invite_codes trägt Zähler und Sperre, invite_code_uses protokolliert, welcher DID welchen Code eingelöst hat — ein Code kann mehrere Konten wert sein, also ist "wer hat ihn benutzt" eine Menge. Die DID hat bewusst keinen Fremdschlüssel: das Protokoll soll das Konto überleben. Der Kern ist das Einlösen ohne Rennen. Geprüft wird nicht vorher, sondern im Schreiben selbst: UPDATE invite_codes SET used_count = used_count + 1 WHERE code = $1 AND NOT disabled AND used_count < max_uses RETURNING used_count, max_uses Der Verlierer zweier gleichzeitiger Registrierungen blockiert auf der Zeilensperre, liest danach die committete Zeile neu, wertet die WHERE-Klausel erneut aus, trifft nichts und bekommt 400. Ein Lesen-dann-Schreiben hätte beide durchgelassen — ich habe genau das probeweise eingebaut, woraufhin die Nebenläufigkeitstests umfielen ("a one-use code let 5 concurrent registrations through"). Das Einlösen ist die erste Anweisung in der bestehenden Transaktion von create_account: die Zeilensperre hält über die Konto-Inserts, und ein Rollback gibt den Code wieder frei. Wer ein Handle-Rennen verliert, verliert nicht auch noch seine Einladung. Alle Fehlerfälle — fehlend, leer, unbekannt, gesperrt, aufgebraucht — liefern dieselbe Meldung, damit der unauthentifizierte Endpoint kein Orakel zum Abtasten des Code-Raums wird. Codes erzeugt das Binary selbst, statt dafür einen Endpoint zu öffnen: `pds-server invite create --count 5 --uses 1`, dazu list und disable. PDS_INVITE_REQUIRED steht auf false per Default — sonst brechen die Integrationstests und jede Dev-Instanz. Der Server warnt beim Start deutlich, solange es aus ist, und describeServer meldet jetzt den tatsächlichen Wert. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013HC9HLrUU1LNwkzp8nkDLX --- .env.example | 8 + crates/at-shared/src/config.rs | 52 ++ crates/pds-server/src/invite.rs | 684 +++++++++++++++ crates/pds-server/src/main.rs | 85 +- crates/pds-server/src/routes/auth.rs | 71 ++ crates/pds-server/src/routes/types.rs | 12 + crates/pds-server/tests/invite_integration.rs | 793 ++++++++++++++++++ crates/pds-server/tests/pds_integration.rs | 27 +- migrations/pds/0004_invite_codes.sql | 186 ++++ 9 files changed, 1913 insertions(+), 5 deletions(-) create mode 100644 crates/pds-server/src/invite.rs create mode 100644 crates/pds-server/tests/invite_integration.rs create mode 100644 migrations/pds/0004_invite_codes.sql diff --git a/.env.example b/.env.example index 2dea4d9..e6752ef 100644 --- a/.env.example +++ b/.env.example @@ -61,6 +61,14 @@ S3_BUCKET_APPVIEW=maarcadetweet-appview PLC_DIRECTORY_URL=https://plc.directory # PLC_DIRECTORY_URL=http://127.0.0.1:2582 +# --- Registrierung --- +# Verlangt createAccount einen Einladungscode? Default false, damit Dev- +# Instanzen und die Integrationstests frei Konten anlegen können — eine +# öffentlich erreichbare PDS gehört auf true gestellt, sonst kann jeder +# beliebig viele Repos anlegen. Der Server warnt beim Start, solange es +# aus ist. Codes erzeugen: `pds-server invite create --count 5 --uses 1` +PDS_INVITE_REQUIRED=false + # --- AppView ingest auth (optional, dev ok if unset) --- # Wenn gesetzt, muss die PDS denselben Wert als Header # `X-Ingest-Secret` mitschicken; ist er nicht gesetzt, nimmt diff --git a/crates/at-shared/src/config.rs b/crates/at-shared/src/config.rs index 4316e0e..c39f31e 100644 --- a/crates/at-shared/src/config.rs +++ b/crates/at-shared/src/config.rs @@ -33,6 +33,36 @@ fn default_pds_firehose_enabled() -> bool { true } +/// Default for `PDS_INVITE_REQUIRED`. +/// +/// `false` — `com.atproto.server.createAccount` stays open unless the +/// operator says otherwise. This is the one security switch in this file +/// that fails *open*, and it does so for a concrete reason: dozens of +/// integration tests across `pds-server` and `appview` create throwaway +/// accounts against a locally running PDS, and every dev instance is +/// bootstrapped the same way. Defaulting to `true` would break all of +/// them on the next `cargo test`, and the usual reflex to a suite that +/// suddenly fails is to switch the new thing off — which lands you at +/// `false` anyway, only with the flag now looking like the thing that +/// was in the way rather than the thing that protects the server. +/// +/// The cost of that choice is that an operator who exposes the PDS +/// publicly without setting the variable gets an open registration +/// endpoint. That is paid for at startup: `pds-server` logs a loud +/// warning on every boot where this is `false`, in the same spirit as +/// `appview`'s `log_startup_posture`. A warning you have to read once +/// per restart is the trade for a test suite that keeps working. +/// +/// That warning also covers the other way this fails open: +/// [`parse_bool_env`] reads anything it doesn't recognise as `false`, so +/// `PDS_INVITE_REQUIRED=ture` leaves registration open. The operator +/// who typed it sees the same startup warning as the operator who never +/// set the variable at all, which is the only signal that distinguishes +/// "I meant to leave it open" from "I thought I had closed it". +fn default_pds_invite_required() -> bool { + false +} + #[derive(Debug, Clone, Deserialize)] pub struct AppConfig { pub pds_host: String, @@ -99,6 +129,24 @@ pub struct AppConfig { /// [`default_pds_firehose_enabled`] for why. #[serde(default = "default_pds_firehose_enabled")] pub pds_firehose_enabled: bool, + /// Whether `com.atproto.server.createAccount` demands a valid invite + /// code. Default `false` — see [`default_pds_invite_required`] for + /// why this switch, alone among the security switches here, fails + /// open. + /// + /// When `true`, a request without an `invite_code` (or its + /// camelCase `inviteCode` spelling), or with one that is unknown, + /// disabled or already used up, is rejected with + /// `400 InvalidInviteCode`. The value is also what + /// `describeServer` reports as `invite_code_required`, so a client + /// can find out before it asks the user for a handle. + /// + /// Codes are minted out of band with `pds-server invite create`; + /// there is no HTTP endpoint that creates them, on purpose — an + /// open PDS's registration gate should not come with a second + /// public surface that hands out keys to it. + #[serde(default = "default_pds_invite_required")] + pub pds_invite_required: bool, } impl AppConfig { @@ -147,6 +195,10 @@ impl AppConfig { .ok() .map(|s| parse_bool_env(&s)) .unwrap_or_else(default_pds_firehose_enabled), + pds_invite_required: std::env::var("PDS_INVITE_REQUIRED") + .ok() + .map(|s| parse_bool_env(&s)) + .unwrap_or_else(default_pds_invite_required), }) } diff --git a/crates/pds-server/src/invite.rs b/crates/pds-server/src/invite.rs new file mode 100644 index 0000000..06282f1 --- /dev/null +++ b/crates/pds-server/src/invite.rs @@ -0,0 +1,684 @@ +//! Invite codes: minting, listing, and the single redeem operation that +//! `com.atproto.server.createAccount` calls. +//! +//! # Why this exists +//! +//! `createAccount` had no gate at all. That was fine while the PDS only +//! answered on `127.0.0.1:2583`; it is not fine on a public name, because +//! each accepted account allocates a repo head, a server-held key pair, an +//! MST that grows with every write, and a stream of firehose events every +//! subscribed AppView is obliged to index. Invite codes are the smallest +//! gate that turns "anyone with curl" into "anyone the operator handed a +//! string to". +//! +//! The gate is off by default (`PDS_INVITE_REQUIRED`, see +//! [`at_shared::config`]) so the existing test suites and dev instances +//! keep creating throwaway accounts; `main` warns loudly on every boot +//! where it is off. +//! +//! # Where codes come from +//! +//! Nowhere over HTTP. Minting lives in the `pds-server invite` subcommand +//! ([`run_cli`]), which the operator runs on the box. Adding a +//! `createInviteCode` endpoint would mean the thing that guards +//! registration is itself reachable by whoever can reach registration — +//! at which point it guards nothing, and the only question left is +//! whether *its* auth has a hole. A subcommand has no attack surface to +//! get wrong. +//! +//! # The one interesting piece of code in here +//! +//! [`redeem`] is a single conditional `UPDATE … RETURNING`, not a +//! `SELECT`-then-`UPDATE`. See its docs and +//! `migrations/pds/0004_invite_codes.sql` for why that distinction is the +//! whole feature. + +use rand::rngs::OsRng; +use rand::RngCore; +use sqlx::{PgPool, Postgres, Transaction}; + +/// Alphabet the codes are drawn from: Crockford base32, lowercased. +/// +/// Exactly 32 symbols, which is the property that matters — it lets each +/// character consume exactly 5 bits of entropy with no modulo bias, so +/// every code in the space is equally likely. A 31- or 36-character +/// "human friendly" alphabet would need rejection sampling to say the +/// same thing, and the usual `byte % len` shortcut would quietly make +/// some characters more probable than others. +/// +/// The excluded letters are Crockford's: `i`, `l`, `o` and `u`. The +/// first three are the ones people mistype as `1`, `1` and `0` when +/// copying a code out of a chat message; `u` is dropped so a random draw +/// cannot spell something the operator has to apologise for. +const CODE_ALPHABET: &[u8; 32] = b"0123456789abcdefghjkmnpqrstvwxyz"; + +/// Characters per group, and groups per code. Two groups of five is +/// 50 bits of entropy — far past anything an online guesser can reach +/// against a database round-trip per attempt, and short enough to read +/// aloud. +const GROUP_LEN: usize = 5; +const GROUPS: usize = 2; + +/// Fixed prefix so a code is recognisable as one when it turns up out of +/// context (a support ticket, a pasted log line) and so it cannot be +/// confused with a handle or a DID. +const CODE_PREFIX: &str = "mt"; + +/// Why a redemption was refused. +/// +/// Deliberately coarse. The route maps [`RedeemError::Invalid`] to a +/// single `400 InvalidInviteCode` with one fixed message, so an +/// unauthenticated caller cannot use the error text to distinguish +/// "no such code" from "that code exists but is used up" — which would +/// turn the endpoint into an oracle for probing the code space. +#[derive(Debug)] +pub enum RedeemError { + /// Unknown, disabled, or already at its use limit. One variant on + /// purpose: see the type docs. + Invalid, + /// The database itself failed. Distinct from [`RedeemError::Invalid`] + /// because this is a `500`, not a `400` — refusing a legitimate code + /// because Postgres hiccuped would be a lie to the user. + Db(sqlx::Error), +} + +impl std::fmt::Display for RedeemError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + RedeemError::Invalid => write!(f, "invite code is not valid"), + RedeemError::Db(e) => write!(f, "invite lookup failed: {e}"), + } + } +} + +/// Normalise a client-supplied code into the form stored in the table. +/// +/// Trims surrounding whitespace (people paste codes with a trailing +/// newline out of a terminal) and lowercases. Every generated code is +/// already lowercase ASCII, so this is an exact normalisation — which is +/// what lets [`redeem`] look the code up with a plain `code = $1` and +/// hit the primary-key index, instead of `lower(code) = $1`, which +/// would force a sequential scan on the one query that runs per +/// registration attempt. +/// +/// Returns `None` for a code that is empty after trimming, so "field +/// present but blank" and "field absent" reach the route as the same +/// case. +pub fn normalize(raw: &str) -> Option { + let trimmed = raw.trim(); + if trimmed.is_empty() { + return None; + } + Some(trimmed.to_ascii_lowercase()) +} + +/// Generate one cryptographically random invite code, e.g. +/// `mt-7k3qw-z9d2m`. +/// +/// Randomness comes from [`OsRng`] — the same source +/// `keys::generate_user_keys` and `password::hash_password` already use +/// in this crate, i.e. the OS CSPRNG, never a seeded or thread-local +/// generator. A code is a bearer credential for creating an account on +/// this server; a predictable one is the same bug as a predictable +/// password-reset token. +/// +/// Entropy: [`GROUPS`] × [`GROUP_LEN`] characters × 5 bits = 50 bits. +pub fn generate_code() -> String { + let total = GROUPS * GROUP_LEN; + let mut bytes = vec![0u8; total]; + OsRng.fill_bytes(&mut bytes); + + let mut out = String::with_capacity(CODE_PREFIX.len() + total + GROUPS); + out.push_str(CODE_PREFIX); + for chunk in bytes.chunks(GROUP_LEN) { + out.push('-'); + for b in chunk { + // Take the low 5 bits of a uniformly random byte. The + // alphabet is exactly 32 symbols, so this is a bijection + // from 5 bits onto it — no bias, no rejection loop. + out.push(CODE_ALPHABET[(*b & 0b0001_1111) as usize] as char); + } + } + out +} + +/// Consume one use of `code` on behalf of the account `did` / `handle`. +/// +/// **This must be called with the same transaction that creates the +/// account.** The whole point is that a code is spent if and only if an +/// account was actually created: if the caller's transaction rolls back +/// — handle taken, key generation failed, anything — the counter goes +/// back with it and the code is still redeemable. `create_account` +/// therefore begins its transaction, redeems here, inserts `users` and +/// `repos`, and only then commits. +/// +/// # The race, and why there isn't one +/// +/// The tempting implementation is: `SELECT used_count, max_uses …`, +/// compare in Rust, then `UPDATE`. That is a check-then-act. Two +/// registrations arriving together on a code with one use left both read +/// `used_count = 0`, both conclude they may proceed, and both write +/// `used_count = 1`. Two accounts, one use — and the row afterwards +/// claims it was redeemed once, so nothing even shows up as wrong. +/// +/// Instead the check *is* the write: +/// +/// ```sql +/// UPDATE invite_codes +/// SET used_count = used_count + 1 +/// WHERE code = $1 AND NOT disabled AND used_count < max_uses +/// RETURNING used_count, max_uses +/// ``` +/// +/// Postgres serialises the two statements on the row lock. The loser +/// blocks until the winner commits, and then — this is the part that +/// makes it work — does *not* continue with its old snapshot: it +/// re-fetches the committed row and re-evaluates the `WHERE` clause +/// against it (EvalPlanQual). `used_count` is now `1`, the predicate is +/// false, the row is dropped from the update set, and the statement +/// affects zero rows. Zero rows is the rejection. This function never +/// forms an opinion about validity that could be stale by the time it +/// acts on it, because it never looks before it writes. +/// +/// The `invite_code_uses` insert that follows is inside the same +/// transaction and the same row lock, so the counter and the audit rows +/// cannot drift apart. +pub async fn redeem( + tx: &mut Transaction<'_, Postgres>, + code: &str, + did: &str, + handle: &str, +) -> Result<(), RedeemError> { + let normalized = match normalize(code) { + Some(c) => c, + None => return Err(RedeemError::Invalid), + }; + + // One statement, and its row count is the verdict. + let claimed: Option<(i32, i32)> = sqlx::query_as( + r#"UPDATE invite_codes + SET used_count = used_count + 1 + WHERE code = $1 + AND NOT disabled + AND used_count < max_uses + RETURNING used_count, max_uses"#, + ) + .bind(&normalized) + .fetch_optional(&mut **tx) + .await + .map_err(RedeemError::Db)?; + + if claimed.is_none() { + return Err(RedeemError::Invalid); + } + + // Audit trail: which account this code produced. Same transaction, + // so it lands exactly when the counter increment does. + // + // The `(code, did)` primary key makes a duplicate impossible; a + // conflict here would mean the same DID redeemed the same code + // twice in one registration, which cannot happen but would corrupt + // the counter/uses agreement if it did — so let it be an error + // rather than silently ignoring it. + sqlx::query( + r#"INSERT INTO invite_code_uses (code, did, handle) + VALUES ($1, $2, $3)"#, + ) + .bind(&normalized) + .bind(did) + .bind(handle) + .execute(&mut **tx) + .await + .map_err(RedeemError::Db)?; + + Ok(()) +} + +// ===================================================== +// CLI: `pds-server invite …` +// ===================================================== + +/// One row of `invite list`, and the shape `create` hands back. +#[derive(Debug)] +pub struct InviteRow { + pub code: String, + pub max_uses: i32, + pub used_count: i32, + pub disabled: bool, + pub note: Option, + pub created_at: chrono::DateTime, +} + +/// What a parsed `invite` command line asks for. +/// +/// Parsed out of `std::env::args` by hand. The workspace has no +/// argument-parsing dependency and this subcommand is not worth adding +/// one for: three verbs, three flags, and a hand-rolled parser that is +/// small enough to unit-test exhaustively (which it is, below) beats a +/// derive macro plus a new crate in the dependency tree of a server +/// binary. +#[derive(Debug, PartialEq)] +pub enum InviteCommand { + /// `invite create [--count N] [--uses N] [--note TEXT]` + Create { + count: u32, + uses: i32, + note: Option, + }, + /// `invite list [--all]` — without `--all`, spent and disabled codes + /// are hidden, because the question the operator almost always has + /// is "what can I still hand out". + List { all: bool }, + /// `invite disable ` — stop honouring a code without losing + /// the record of which accounts it already created. + Disable { code: String }, +} + +/// Usage text. Printed for `invite help`, and for anything that fails to +/// parse. +pub const INVITE_USAGE: &str = "\ +usage: pds-server invite + + create [--count N] [--uses N] [--note TEXT] + Mint N codes (default 1), each good for `--uses` accounts + (default 1). Prints one code per line and nothing else, so the + output can be piped or pasted directly. + + list [--all] + Show codes that can still be redeemed. --all includes spent and + disabled ones. + + disable + Stop honouring a code. The record of accounts it already created + is kept. + +The database is the one named by DATABASE_URL_PDS (read from .env like +the server does). Codes are only meaningful while PDS_INVITE_REQUIRED +is true."; + +/// Parse the arguments after the `invite` verb. +/// +/// Returns `Err(message)` for anything malformed; the caller prints the +/// message plus [`INVITE_USAGE`] and exits non-zero. Unknown flags are +/// an error rather than being ignored — a typo'd `--uses` that silently +/// became `1` would hand out the wrong codes and the operator would only +/// find out when the second person to use one got a `400`. +pub fn parse_invite_args(args: &[String]) -> Result { + let verb = args + .first() + .map(|s| s.as_str()) + .ok_or_else(|| "missing invite command".to_string())?; + let rest = &args[1..]; + + match verb { + "create" => { + let mut count: u32 = 1; + let mut uses: i32 = 1; + let mut note: Option = None; + let mut i = 0; + while i < rest.len() { + match rest[i].as_str() { + "--count" => { + let v = rest + .get(i + 1) + .ok_or_else(|| "--count needs a value".to_string())?; + count = v + .parse() + .map_err(|_| format!("--count: not a number: {v}"))?; + if count == 0 { + return Err("--count must be at least 1".to_string()); + } + i += 2; + } + "--uses" => { + let v = rest + .get(i + 1) + .ok_or_else(|| "--uses needs a value".to_string())?; + uses = v + .parse() + .map_err(|_| format!("--uses: not a number: {v}"))?; + // Mirrors the table's CHECK (max_uses > 0). Caught + // here so the operator gets a sentence instead of a + // constraint-violation dump. + if uses < 1 { + return Err("--uses must be at least 1".to_string()); + } + i += 2; + } + "--note" => { + let v = rest + .get(i + 1) + .ok_or_else(|| "--note needs a value".to_string())?; + note = Some(v.clone()); + i += 2; + } + other => return Err(format!("unknown option for `create`: {other}")), + } + } + Ok(InviteCommand::Create { count, uses, note }) + } + "list" => { + let mut all = false; + for a in rest { + match a.as_str() { + "--all" => all = true, + other => return Err(format!("unknown option for `list`: {other}")), + } + } + Ok(InviteCommand::List { all }) + } + "disable" => { + let code = rest + .first() + .ok_or_else(|| "disable needs a code".to_string())?; + if rest.len() > 1 { + return Err("disable takes exactly one code".to_string()); + } + let code = normalize(code).ok_or_else(|| "disable needs a code".to_string())?; + Ok(InviteCommand::Disable { code }) + } + other => Err(format!("unknown invite command: {other}")), + } +} + +/// Insert `count` freshly generated codes, each good for `uses` +/// accounts. +/// +/// Retries on a primary-key collision. With 50 bits per code a +/// collision is not something that will happen, but "not something that +/// will happen" is exactly the class of event that turns into a +/// confusing `duplicate key` traceback at 2am, and the retry costs three +/// lines. +pub async fn create_codes( + db: &PgPool, + count: u32, + uses: i32, + note: Option<&str>, +) -> anyhow::Result> { + let mut out = Vec::with_capacity(count as usize); + for _ in 0..count { + let mut attempt = 0; + loop { + let code = generate_code(); + let inserted = sqlx::query( + r#"INSERT INTO invite_codes (code, max_uses, note) + VALUES ($1, $2, $3) + ON CONFLICT (code) DO NOTHING"#, + ) + .bind(&code) + .bind(uses) + .bind(note) + .execute(db) + .await? + .rows_affected(); + if inserted == 1 { + out.push(code); + break; + } + attempt += 1; + if attempt >= 5 { + anyhow::bail!("could not find a free invite code after 5 attempts"); + } + } + } + Ok(out) +} + +/// Read back codes for `invite list`. +pub async fn list_codes(db: &PgPool, all: bool) -> anyhow::Result> { + // Two statements rather than one with a `$1`-toggled predicate: + // the redeemable filter is exactly the redeem query's `WHERE` + // clause, and keeping it spelled the same way makes it obvious that + // `list` shows what `redeem` would accept. + let sql = if all { + r#"SELECT code, max_uses, used_count, disabled, note, created_at + FROM invite_codes + ORDER BY created_at DESC"# + } else { + r#"SELECT code, max_uses, used_count, disabled, note, created_at + FROM invite_codes + WHERE NOT disabled AND used_count < max_uses + ORDER BY created_at DESC"# + }; + let rows: Vec<(String, i32, i32, bool, Option, chrono::DateTime)> = + sqlx::query_as(sql).fetch_all(db).await?; + Ok(rows + .into_iter() + .map( + |(code, max_uses, used_count, disabled, note, created_at)| InviteRow { + code, + max_uses, + used_count, + disabled, + note, + created_at, + }, + ) + .collect()) +} + +/// Flip `disabled` on one code. Returns `false` if there is no such +/// code, so the CLI can say so instead of reporting a successful no-op. +pub async fn disable_code(db: &PgPool, code: &str) -> anyhow::Result { + let n = sqlx::query("UPDATE invite_codes SET disabled = TRUE WHERE code = $1") + .bind(code) + .execute(db) + .await? + .rows_affected(); + Ok(n == 1) +} + +/// Run the `invite` subcommand end to end: parse, connect, act, print. +/// +/// Connects with the same `DATABASE_URL_PDS` and runs the same +/// migrations as [`crate::main`], so `invite create` works on a fresh +/// checkout before the server has ever been started — otherwise the +/// first thing an operator does after deploying would fail with +/// "relation invite_codes does not exist". +pub async fn run_cli(args: &[String]) -> anyhow::Result<()> { + if matches!(args.first().map(|s| s.as_str()), None | Some("help") | Some("-h") | Some("--help")) + { + println!("{INVITE_USAGE}"); + return Ok(()); + } + let cmd = match parse_invite_args(args) { + Ok(c) => c, + Err(msg) => { + eprintln!("pds-server invite: {msg}\n\n{INVITE_USAGE}"); + std::process::exit(2); + } + }; + + let cfg = at_shared::config::AppConfig::from_env()?; + let db = sqlx::postgres::PgPoolOptions::new() + .max_connections(2) + .acquire_timeout(std::time::Duration::from_secs(10)) + .connect(&cfg.database_url_pds) + .await?; + sqlx::migrate!("../../migrations/pds").run(&db).await?; + + match cmd { + InviteCommand::Create { count, uses, note } => { + let codes = create_codes(&db, count, uses, note.as_deref()).await?; + // Bare codes, one per line, nothing else on stdout — the + // operator pipes this into a message or a file. Anything + // decorative here would have to be stripped by hand. + for c in &codes { + println!("{c}"); + } + if !cfg.pds_invite_required { + // Not an error: minting codes before flipping the switch + // is the correct order of operations. But an operator + // who thinks they have just closed registration should + // find out now. + eprintln!( + "note: PDS_INVITE_REQUIRED is not true — createAccount currently \ + accepts requests without any code." + ); + } + } + InviteCommand::List { all } => { + let rows = list_codes(&db, all).await?; + if rows.is_empty() { + eprintln!("no invite codes"); + } + for r in rows { + let state = if r.disabled { + "disabled" + } else if r.used_count >= r.max_uses { + "spent" + } else { + "open" + }; + println!( + "{} {}/{} {} {} {}", + r.code, + r.used_count, + r.max_uses, + state, + r.created_at.format("%Y-%m-%dT%H:%M:%SZ"), + r.note.as_deref().unwrap_or("") + ); + } + } + InviteCommand::Disable { code } => { + if disable_code(&db, &code).await? { + println!("disabled {code}"); + } else { + eprintln!("no such invite code: {code}"); + std::process::exit(1); + } + } + } + Ok(()) +} + +// -- tests ------------------------------------------------------------------- + +#[cfg(test)] +mod tests { + use super::*; + use std::collections::HashSet; + + #[test] + fn generated_codes_use_only_the_safe_alphabet() { + let code = generate_code(); + // `mt-xxxxx-xxxxx` + assert!(code.starts_with("mt-"), "code = {code}"); + let groups: Vec<&str> = code.split('-').collect(); + assert_eq!(groups.len(), GROUPS + 1, "code = {code}"); + assert_eq!(groups[0], CODE_PREFIX); + for g in &groups[1..] { + assert_eq!(g.len(), GROUP_LEN, "group {g} in {code}"); + for ch in g.chars() { + assert!( + CODE_ALPHABET.contains(&(ch as u8)), + "character {ch:?} in {code} is outside the alphabet" + ); + } + } + // The letters people mistype must never appear. + for bad in ['i', 'l', 'o', 'u'] { + assert!(!code[3..].contains(bad), "{code} contains {bad}"); + } + } + + #[test] + fn generated_codes_do_not_repeat() { + // Not a randomness test — a 1000-draw collision would mean the + // generator is returning a constant or reusing a seeded RNG, + // which is the failure mode that actually happens when someone + // swaps `OsRng` for `thread_rng` with a fixed seed in a test + // helper. + let mut seen = HashSet::new(); + for _ in 0..1000 { + assert!(seen.insert(generate_code()), "duplicate code in 1000 draws"); + } + } + + #[test] + fn normalize_trims_and_lowercases() { + assert_eq!(normalize(" MT-ABCDE-FGHJK \n").as_deref(), Some("mt-abcde-fghjk")); + assert_eq!(normalize("mt-abcde-fghjk").as_deref(), Some("mt-abcde-fghjk")); + // Absent and blank must be indistinguishable to the route. + assert_eq!(normalize(""), None); + assert_eq!(normalize(" "), None); + assert_eq!(normalize("\t\n"), None); + } + + fn args(v: &[&str]) -> Vec { + v.iter().map(|s| s.to_string()).collect() + } + + #[test] + fn parses_create_with_defaults_and_flags() { + assert_eq!( + parse_invite_args(&args(&["create"])).unwrap(), + InviteCommand::Create { + count: 1, + uses: 1, + note: None + } + ); + assert_eq!( + parse_invite_args(&args(&["create", "--count", "5", "--uses", "1"])).unwrap(), + InviteCommand::Create { + count: 5, + uses: 1, + note: None + } + ); + // Order must not matter, and --note takes the next argument + // verbatim (spaces included). + assert_eq!( + parse_invite_args(&args(&["create", "--note", "meetup 2026", "--uses", "3"])).unwrap(), + InviteCommand::Create { + count: 1, + uses: 3, + note: Some("meetup 2026".to_string()) + } + ); + } + + #[test] + fn rejects_malformed_create_flags() { + // A typo'd flag must not be silently ignored — that would hand + // out codes with the default limits. + assert!(parse_invite_args(&args(&["create", "--use", "3"])).is_err()); + assert!(parse_invite_args(&args(&["create", "--count"])).is_err()); + assert!(parse_invite_args(&args(&["create", "--count", "x"])).is_err()); + assert!(parse_invite_args(&args(&["create", "--count", "0"])).is_err()); + // max_uses > 0 is a table constraint; catch it before Postgres does. + assert!(parse_invite_args(&args(&["create", "--uses", "0"])).is_err()); + assert!(parse_invite_args(&args(&["create", "--uses", "-2"])).is_err()); + } + + #[test] + fn parses_list_and_disable() { + assert_eq!( + parse_invite_args(&args(&["list"])).unwrap(), + InviteCommand::List { all: false } + ); + assert_eq!( + parse_invite_args(&args(&["list", "--all"])).unwrap(), + InviteCommand::List { all: true } + ); + assert!(parse_invite_args(&args(&["list", "--everything"])).is_err()); + // `disable` normalises the code the same way redeem does, so an + // operator pasting a shouted code still disables the right row. + assert_eq!( + parse_invite_args(&args(&["disable", " MT-ABCDE-FGHJK "])).unwrap(), + InviteCommand::Disable { + code: "mt-abcde-fghjk".to_string() + } + ); + assert!(parse_invite_args(&args(&["disable"])).is_err()); + assert!(parse_invite_args(&args(&["disable", "a", "b"])).is_err()); + } + + #[test] + fn rejects_unknown_verb_and_empty_args() { + assert!(parse_invite_args(&args(&[])).is_err()); + assert!(parse_invite_args(&args(&["destroy"])).is_err()); + } +} diff --git a/crates/pds-server/src/main.rs b/crates/pds-server/src/main.rs index ae361b2..f89684a 100644 --- a/crates/pds-server/src/main.rs +++ b/crates/pds-server/src/main.rs @@ -2,6 +2,7 @@ mod appview_push; mod car; mod dag_cbor; mod firehose; +mod invite; mod jwt_issuer; mod keys; mod password; @@ -17,6 +18,16 @@ use serde_json::json; use tracing::{info, warn}; use tracing_subscriber::EnvFilter; +/// Usage line for the binary itself. The subcommands are operator +/// tooling; the no-argument form is the server, which is what every +/// deploy script and systemd unit invokes. +const USAGE: &str = "\ +usage: pds-server [command] + + (no command) run the PDS server + invite … manage invite codes (see `pds-server invite help`) + help show this message"; + #[tokio::main] async fn main() -> anyhow::Result<()> { // Load `.env` from the working directory (and upwards) if present. @@ -25,11 +36,57 @@ async fn main() -> anyhow::Result<()> { // PDS_HOST`. Real environment variables always win over the file. let _ = dotenvy::dotenv(); - tracing_subscriber::fmt() - .with_env_filter(EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new("info"))) - .init(); + // Argument dispatch, by hand. + // + // The workspace carries no argument-parsing crate and this does not + // justify adding one: exactly one subcommand exists, and the + // overwhelmingly common invocation is the bare binary. Anything we + // do not recognise is an error rather than being ignored — a + // mistyped `pds-server invit create` that silently booted a server + // would look like it worked and mint no codes. + let args: Vec = std::env::args().skip(1).collect(); + match args.first().map(|s| s.as_str()) { + None => {} + Some("invite") => { + // CLI output is meant to be read and pasted, so keep the + // log stream quiet unless the operator asked for it. Without + // this, `sqlx::migrate` chatters over the codes. + init_tracing("warn"); + return invite::run_cli(&args[1..]).await; + } + Some("help") | Some("-h") | Some("--help") => { + println!("{USAGE}"); + return Ok(()); + } + Some(other) => { + eprintln!("pds-server: unknown command: {other}\n\n{USAGE}"); + std::process::exit(2); + } + } + + init_tracing("info"); let cfg = at_shared::config::AppConfig::from_env()?; + + // Announce the relaxed security posture before we bind a port. + // + // `PDS_INVITE_REQUIRED` is the only switch in `AppConfig` that + // defaults to *open* (so the integration suites and dev instances + // can keep creating throwaway accounts), which makes this warning + // the only thing standing between "we made the PDS public" and + // "anyone on the internet can mint repos on our disk". Mirrors + // `appview`'s `auth::log_startup_posture`. + if !cfg.pds_invite_required { + warn!( + "PDS_INVITE_REQUIRED is not true — com.atproto.server.createAccount accepts \ + ANY caller, and every accepted account allocates a repo, a server-held key \ + pair and firehose events. Fine on a private/dev instance; on a publicly \ + reachable PDS set PDS_INVITE_REQUIRED=true and hand out codes with \ + `pds-server invite create`." + ); + } else { + info!("PDS_INVITE_REQUIRED=true — createAccount requires a valid invite code"); + } let db = sqlx::postgres::PgPoolOptions::new() .max_connections(32) .min_connections(2) @@ -71,6 +128,21 @@ async fn main() -> anyhow::Result<()> { Ok(()) } +/// Install the tracing subscriber, with `default_filter` as the level +/// when `RUST_LOG` says nothing. +/// +/// Factored out because the two entry points want different defaults: +/// the server wants `info`, the `invite` subcommand wants `warn` so that +/// migration chatter does not land in the middle of a list of codes the +/// operator is about to copy. +fn init_tracing(default_filter: &str) { + tracing_subscriber::fmt() + .with_env_filter( + EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new(default_filter)), + ) + .init(); +} + pub fn router(state: AppState) -> Router { Router::new() .route("/", get(root)) @@ -241,7 +313,12 @@ async fn describe_server(State(state): State) -> Json return Err(invalid_invite_code()), + Some(code) => { + if let Err(e) = + crate::invite::redeem(&mut tx, &code, &did, &req.handle).await + { + return match e { + crate::invite::RedeemError::Invalid => { + // Deliberately not logged with the code at + // info level: a public endpoint that echoes + // every guessed code into the log is a way + // to fill the disk from outside. + warn!( + handle = %req.handle, + "createAccount rejected: invite code invalid, disabled or spent" + ); + Err(invalid_invite_code()) + } + crate::invite::RedeemError::Db(db_err) => Err(internal(db_err)), + }; + } + } + } + } + sqlx::query( r#"INSERT INTO users (did, handle, email, password_hash, signing_key, rotation_key) VALUES ($1, $2, $3, $4, $5, $6)"#, @@ -306,6 +355,28 @@ pub async fn refresh_session( })) } +/// The one error a failed invite check produces. +/// +/// Same `(StatusCode, Json)` shape every other route in this +/// module returns, so a client parses it with the code it already has: +/// `{"error": "InvalidInviteCode", "message": "..."}` under a `400`. +/// +/// One message for every failure mode — missing, unknown, disabled, +/// spent — on purpose. A distinct "that code exists but is used up" +/// would let an unauthenticated caller walk the code space and learn +/// which strings are real, which is most of the work of stealing one. +/// The operator can tell the cases apart from `pds-server invite list`; +/// the internet cannot. +fn invalid_invite_code() -> (StatusCode, Json) { + ( + StatusCode::BAD_REQUEST, + Json(crate::routes::types::ErrorBody::new( + "InvalidInviteCode", + Some("a valid invite code is required to create an account on this server".into()), + )), + ) +} + fn internal(e: impl std::fmt::Display) -> (StatusCode, Json) { ( StatusCode::INTERNAL_SERVER_ERROR, diff --git a/crates/pds-server/src/routes/types.rs b/crates/pds-server/src/routes/types.rs index 33ff2ca..6f674ee 100644 --- a/crates/pds-server/src/routes/types.rs +++ b/crates/pds-server/src/routes/types.rs @@ -6,6 +6,18 @@ pub struct CreateAccountReq { pub email: Option, pub password: Option, pub did: Option, + /// The invite code, when `PDS_INVITE_REQUIRED` is on. + /// + /// The alias is not cosmetic. This struct — like every other type in + /// this module — is snake_case on the wire, which is what our own + /// clients send. The AT Protocol lexicon for + /// `com.atproto.server.createAccount` spells the field `inviteCode`, + /// so every off-the-shelf atproto client sends *that*, and without + /// the alias serde would drop it into `None` silently — the account + /// would be refused with "an invite code is required" while the user + /// is looking at the code they just pasted. Accepting both spellings + /// costs one attribute; debugging that report costs an afternoon. + #[serde(alias = "inviteCode")] pub invite_code: Option, pub recovery_key: Option, } diff --git a/crates/pds-server/tests/invite_integration.rs b/crates/pds-server/tests/invite_integration.rs new file mode 100644 index 0000000..4ea096c --- /dev/null +++ b/crates/pds-server/tests/invite_integration.rs @@ -0,0 +1,793 @@ +//! Invite-code enforcement on `com.atproto.server.createAccount`. +//! +//! # Why this file starts its own PDS +//! +//! Every other integration suite in this crate talks to whatever PDS the +//! developer already has running on `:2583` and skips itself when there +//! isn't one. That works because those tests only need *a* PDS. These +//! need a PDS with `PDS_INVITE_REQUIRED=true`, and the ambient one is +//! (correctly) started with the default `false` — otherwise every other +//! suite, which creates throwaway accounts with no code, would fail. +//! +//! Asking the developer to restart their PDS with a different flag +//! before this file passes would mean the flag's behaviour is only ever +//! tested by hand. So each test here spawns its own `pds-server` on a +//! free port with the flag set the way that test needs it, and kills it +//! on the way out ([`Pds`]'s `Drop`). `env!("CARGO_BIN_EXE_pds-server")` +//! is cargo's own path to the binary it just built for this test run, so +//! the process under test is always the current code. +//! +//! The suite still fails open, in the same spirit as its neighbours: if +//! the child never becomes healthy — no Postgres, no `.env`, no +//! `DATABASE_URL_PDS` — the tests print why and return green rather than +//! failing a workstation that simply isn't running the stack. + +use serde_json::{json, Value}; +use std::process::{Child, Command}; +use std::time::Duration; + +/// A `pds-server` child process bound to its own port, killed when the +/// test that started it goes out of scope. +/// +/// The `Drop` impl is the reason this is a struct at all: a test that +/// panics mid-way must not leave a server holding a port and a pool of +/// Postgres connections for the rest of the run. +struct Pds { + child: Child, + port: u16, + http: reqwest::Client, +} + +impl Drop for Pds { + fn drop(&mut self) { + let _ = self.child.kill(); + let _ = self.child.wait(); + } +} + +impl Pds { + fn url(&self, path: &str) -> String { + format!("http://127.0.0.1:{}{}", self.port, path) + } + + async fn create_account(&self, body: Value) -> (u16, Value) { + let resp = self + .http + .post(self.url("/xrpc/com.atproto.server.createAccount")) + .json(&body) + .send() + .await + .expect("createAccount request"); + let status = resp.status().as_u16(); + let body: Value = resp.json().await.unwrap_or(Value::Null); + (status, body) + } + + async fn describe(&self) -> Value { + self.http + .get(self.url("/xrpc/com.atproto.server.describeServer")) + .send() + .await + .expect("describeServer") + .json() + .await + .expect("describeServer json") + } +} + +/// Ask the OS for a port nobody is using, then let go of it. +/// +/// There is a window between the drop and the child's `bind` in which +/// something else could take the port; on a test machine that window is +/// theoretical, and the alternative (a fixed port) would make two +/// concurrently running tests in this file collide *reliably* instead of +/// never. +fn free_port() -> Option { + let l = std::net::TcpListener::bind("127.0.0.1:0").ok()?; + let p = l.local_addr().ok()?.port(); + drop(l); + Some(p) +} + +/// Start a `pds-server` with `PDS_INVITE_REQUIRED` set to `required`. +/// +/// Returns `None` when the stack this needs isn't available, which the +/// callers turn into a skip. The child inherits the ambient environment +/// (so `DATABASE_URL_PDS` and friends come from `.env` exactly as they +/// do for the real server — `dotenvy` does not override real variables, +/// so our overrides below win). +async fn start_pds(required: bool) -> Option { + let port = free_port()?; + let child = Command::new(env!("CARGO_BIN_EXE_pds-server")) + .env("PDS_HOST", "127.0.0.1") + .env("PDS_PORT", port.to_string()) + .env("PDS_PUBLIC_URL", format!("http://127.0.0.1:{port}")) + .env("PDS_INVITE_REQUIRED", if required { "true" } else { "false" }) + // Point the PLC submit at a closed port. `create_account` + // tolerates a failed submit by design (the DID is computed + // locally), and a connection refused on loopback fails in + // microseconds — whereas the real directory would add a network + // round-trip to every account this file creates, and might + // actually publish throwaway test DIDs. + .env("PLC_DIRECTORY_URL", "http://127.0.0.1:1") + .env("RUST_LOG", "warn") + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn() + .ok()?; + + let http = reqwest::Client::builder() + .timeout(Duration::from_secs(30)) + .build() + .ok()?; + let mut pds = Pds { child, port, http }; + + for _ in 0..80 { + // If the child already exited (bad/missing env, no Postgres), + // stop waiting — there is nothing to become healthy. + if let Ok(Some(_)) = pds.child.try_wait() { + return None; + } + if let Ok(r) = pds.http.get(pds.url("/healthz")).send().await { + if r.status().is_success() { + return Some(pds); + } + } + tokio::time::sleep(Duration::from_millis(150)).await; + } + None +} + +async fn db_pool() -> Option { + let url = std::env::var("DATABASE_URL_PDS") + .unwrap_or_else(|_| "postgres://pds:pds@127.0.0.1:5434/pds".to_string()); + sqlx::postgres::PgPoolOptions::new() + .max_connections(4) + .acquire_timeout(Duration::from_secs(3)) + .connect(&url) + .await + .ok() +} + +/// Put a code straight into the table in whatever state the test needs. +/// +/// Tests seed through SQL rather than through `pds-server invite create` +/// because the states that matter here — already spent, disabled — are +/// not states the CLI can mint directly, and because a test that had to +/// shell out to a second binary to arrange its fixture would be testing +/// two things at once. +async fn seed_code(db: &sqlx::PgPool, max_uses: i32, used: i32, disabled: bool) -> String { + let code = format!("mt-test-{}", uuid::Uuid::new_v4().simple()); + sqlx::query( + "INSERT INTO invite_codes (code, max_uses, used_count, disabled) VALUES ($1, $2, $3, $4)", + ) + .bind(&code) + .bind(max_uses) + .bind(used) + .bind(disabled) + .execute(db) + .await + .expect("seed invite code"); + code +} + +async fn used_count(db: &sqlx::PgPool, code: &str) -> i32 { + sqlx::query_scalar::<_, i32>("SELECT used_count FROM invite_codes WHERE code = $1") + .bind(code) + .fetch_one(db) + .await + .expect("read used_count") +} + +async fn use_rows(db: &sqlx::PgPool, code: &str) -> Vec<(String, String)> { + sqlx::query_as::<_, (String, String)>( + "SELECT did, handle FROM invite_code_uses WHERE code = $1 ORDER BY used_at", + ) + .bind(code) + .fetch_all(db) + .await + .expect("read invite_code_uses") +} + +/// A unique throwaway handle. +/// +/// `createAccount` caps handles at 64 characters, and +/// `_<32 hex>.maarcadetweet.local` overshoots that for anything +/// but the shortest prefix — a limit that shows up as a confusing +/// `InvalidHandle` in a test that is about invite codes. Half the UUID +/// is 64 bits of uniqueness, which is plenty for a test fixture and +/// leaves room for a readable prefix. +fn handle(prefix: &str) -> String { + let uniq = uuid::Uuid::new_v4().simple().to_string(); + format!("{}_{}.maarcadetweet.local", prefix, &uniq[..16]) +} + +/// Every invite rejection must look the same to a client: `400` with the +/// module's usual `{error, message}` body and the name +/// `InvalidInviteCode`. +fn assert_invalid_invite(status: u16, body: &Value, what: &str) { + assert_eq!(status, 400, "{what}: expected 400, body = {body}"); + assert_eq!( + body["error"], "InvalidInviteCode", + "{what}: wrong error name, body = {body}" + ); + assert!( + body["message"].is_string(), + "{what}: error body must carry a message, body = {body}" + ); + // No DID may have been minted on a rejected request. + assert!( + body["did"].is_null(), + "{what}: rejected request returned a did, body = {body}" + ); +} + +// -- enforcement ------------------------------------------------------------ + +/// The happy path, and the property that makes a single-use code +/// single-use: after the account exists, the same code is dead. +/// +/// Also checks the audit trail, which is the reason +/// `invite_code_uses` exists at all — "which account did this code +/// create" has to be answerable after the fact. +#[tokio::test] +async fn valid_code_admits_one_account_then_is_spent() { + let Some(db) = db_pool().await else { + eprintln!("no pds database, skipping"); + return; + }; + let Some(pds) = start_pds(true).await else { + eprintln!("could not start a pds with PDS_INVITE_REQUIRED=true, skipping"); + return; + }; + + // The switch must be advertised, not just enforced — a client reads + // this before it asks the user for anything. + assert_eq!( + pds.describe().await["invite_code_required"], + json!(true), + "describeServer must report the actual PDS_INVITE_REQUIRED value" + ); + + let code = seed_code(&db, 1, 0, false).await; + let h = handle("inv_ok"); + let (status, body) = pds + .create_account(json!({ + "handle": h, + "password": "hunter2hunter2", + "invite_code": code, + })) + .await; + assert_eq!(status, 200, "valid code must create an account: {body}"); + let did = body["did"].as_str().expect("did").to_string(); + + assert_eq!(used_count(&db, &code).await, 1, "code must be counted as used"); + let uses = use_rows(&db, &code).await; + assert_eq!(uses.len(), 1); + assert_eq!(uses[0].0, did, "audit row must name the account it created"); + assert_eq!(uses[0].1, h, "audit row must snapshot the handle"); + + // Second attempt on the now-spent code. + let (status2, body2) = pds + .create_account(json!({ + "handle": handle("inv_second"), + "password": "hunter2hunter2", + "invite_code": code, + })) + .await; + assert_invalid_invite(status2, &body2, "spent code"); + assert_eq!( + used_count(&db, &code).await, + 1, + "a rejected attempt must not move the counter" + ); +} + +/// Every way a code can fail, and the missing-code case, all land on the +/// same `400 InvalidInviteCode`. +#[tokio::test] +async fn unknown_disabled_spent_and_missing_codes_are_rejected() { + let Some(db) = db_pool().await else { + eprintln!("no pds database, skipping"); + return; + }; + let Some(pds) = start_pds(true).await else { + eprintln!("could not start a pds with PDS_INVITE_REQUIRED=true, skipping"); + return; + }; + + // Unknown. + let (s, b) = pds + .create_account(json!({ + "handle": handle("inv_unknown"), + "password": "hunter2hunter2", + "invite_code": "mt-zzzzz-zzzzz", + })) + .await; + assert_invalid_invite(s, &b, "unknown code"); + + // Disabled, with uses left — proves `disabled` is checked and not + // just the counter. + let disabled = seed_code(&db, 5, 0, true).await; + let (s, b) = pds + .create_account(json!({ + "handle": handle("inv_disabled"), + "password": "hunter2hunter2", + "invite_code": disabled, + })) + .await; + assert_invalid_invite(s, &b, "disabled code"); + assert_eq!(used_count(&db, &disabled).await, 0); + + // Already at its limit. + let spent = seed_code(&db, 2, 2, false).await; + let (s, b) = pds + .create_account(json!({ + "handle": handle("inv_spent"), + "password": "hunter2hunter2", + "invite_code": spent, + })) + .await; + assert_invalid_invite(s, &b, "exhausted code"); + + // No field at all. + let (s, b) = pds + .create_account(json!({ + "handle": handle("inv_none"), + "password": "hunter2hunter2", + })) + .await; + assert_invalid_invite(s, &b, "missing code"); + + // Present but blank / whitespace — must be indistinguishable from + // absent, not an attempt to look up the empty string. + for blank in ["", " "] { + let (s, b) = pds + .create_account(json!({ + "handle": handle("inv_blank"), + "password": "hunter2hunter2", + "invite_code": blank, + })) + .await; + assert_invalid_invite(s, &b, "blank code"); + } +} + +/// The camelCase spelling from the atproto lexicon, and the multi-use +/// case the schema exists for. +#[tokio::test] +async fn camel_case_spelling_works_and_multi_use_codes_stop_at_the_limit() { + let Some(db) = db_pool().await else { + eprintln!("no pds database, skipping"); + return; + }; + let Some(pds) = start_pds(true).await else { + eprintln!("could not start a pds with PDS_INVITE_REQUIRED=true, skipping"); + return; + }; + + // `inviteCode` is what an off-the-shelf atproto client sends. + let code = seed_code(&db, 3, 0, false).await; + let (s, b) = pds + .create_account(json!({ + "handle": handle("inv_camel"), + "password": "hunter2hunter2", + "inviteCode": code, + })) + .await; + assert_eq!(s, 200, "inviteCode spelling must be accepted: {b}"); + + // Case and stray whitespace are normalised, so a code shouted or + // pasted out of a chat window still works. + let (s, b) = pds + .create_account(json!({ + "handle": handle("inv_case"), + "password": "hunter2hunter2", + "invite_code": format!(" {} ", code.to_uppercase()), + })) + .await; + assert_eq!(s, 200, "normalised code must be accepted: {b}"); + + // Third and last use. + let (s, _) = pds + .create_account(json!({ + "handle": handle("inv_third"), + "password": "hunter2hunter2", + "invite_code": code, + })) + .await; + assert_eq!(s, 200); + + // Fourth is one too many. + let (s, b) = pds + .create_account(json!({ + "handle": handle("inv_fourth"), + "password": "hunter2hunter2", + "invite_code": code, + })) + .await; + assert_invalid_invite(s, &b, "one past max_uses"); + + assert_eq!(used_count(&db, &code).await, 3); + assert_eq!(use_rows(&db, &code).await.len(), 3); +} + +/// A failed account creation must not consume the code. +/// +/// The cheapest way to make the account creation fail *after* the +/// redemption has already run is a handle that is already taken: the +/// redeem happens first inside the transaction, the `users` insert then +/// trips the unique index, and the whole transaction rolls back. If the +/// redemption had been done outside the transaction (or committed +/// separately) the user would have lost their code to someone else's +/// handle. +#[tokio::test] +async fn a_failed_registration_does_not_burn_the_code() { + let Some(db) = db_pool().await else { + eprintln!("no pds database, skipping"); + return; + }; + let Some(pds) = start_pds(true).await else { + eprintln!("could not start a pds with PDS_INVITE_REQUIRED=true, skipping"); + return; + }; + + let taken = handle("inv_taken"); + let first = seed_code(&db, 1, 0, false).await; + let (s, b) = pds + .create_account(json!({ + "handle": taken, "password": "hunter2hunter2", "invite_code": first, + })) + .await; + assert_eq!(s, 200, "{b}"); + + // Now a *different* code, used on a handle that cannot be created. + let code = seed_code(&db, 1, 0, false).await; + let (s, _b) = pds + .create_account(json!({ + "handle": taken, "password": "hunter2hunter2", "invite_code": code, + })) + .await; + assert_eq!(s, 409, "duplicate handle is still a 409"); + assert_eq!( + used_count(&db, &code).await, + 0, + "the code must survive a registration that rolled back" + ); + assert!(use_rows(&db, &code).await.is_empty()); + + // And it still works afterwards. + let (s, b) = pds + .create_account(json!({ + "handle": handle("inv_retry"), "password": "hunter2hunter2", "invite_code": code, + })) + .await; + assert_eq!(s, 200, "unburned code must still be redeemable: {b}"); +} + +// -- the race --------------------------------------------------------------- + +/// Two (here: eight) registrations arriving at the same instant on the +/// last remaining use of a code. Exactly one may get in. +/// +/// This is the test the whole design is built around. A +/// `SELECT`-then-`UPDATE` implementation passes every other test in this +/// file and fails this one: all eight requests read `used_count = 0`, +/// all eight decide they are allowed, and the server hands out eight +/// accounts for a one-use code while the row afterwards claims a single +/// redemption. The fix is that `invite::redeem` never reads before it +/// writes — the `WHERE used_count < max_uses` is part of the `UPDATE`, +/// so Postgres re-evaluates it against the committed row after the +/// row lock is released and the losers match zero rows. +/// +/// Every request uses a distinct handle, so nothing but the invite code +/// can be what serialises them. +#[tokio::test] +async fn concurrent_registrations_cannot_share_one_use() { + let Some(db) = db_pool().await else { + eprintln!("no pds database, skipping"); + return; + }; + let Some(pds) = start_pds(true).await else { + eprintln!("could not start a pds with PDS_INVITE_REQUIRED=true, skipping"); + return; + }; + + const N: usize = 8; + let code = seed_code(&db, 1, 0, false).await; + + let mut tasks = Vec::with_capacity(N); + for i in 0..N { + let http = pds.http.clone(); + let url = pds.url("/xrpc/com.atproto.server.createAccount"); + let code = code.clone(); + let h = handle(&format!("inv_race{i}")); + tasks.push(tokio::spawn(async move { + let resp = http + .post(url) + .json(&json!({ + "handle": h, + "password": "hunter2hunter2", + "invite_code": code, + })) + .send() + .await + .expect("concurrent createAccount"); + let status = resp.status().as_u16(); + let body: Value = resp.json().await.unwrap_or(Value::Null); + (status, body) + })); + } + + let mut ok = Vec::new(); + let mut rejected = 0usize; + for t in tasks { + let (status, body) = t.await.unwrap(); + match status { + 200 => ok.push(body), + 400 => { + assert_eq!(body["error"], "InvalidInviteCode", "body = {body}"); + rejected += 1; + } + other => panic!("unexpected status {other}: {body}"), + } + } + + assert_eq!( + ok.len(), + 1, + "a one-use code let {} concurrent registrations through — the redeem is racy", + ok.len() + ); + assert_eq!(rejected, N - 1); + assert_eq!(used_count(&db, &code).await, 1); + let uses = use_rows(&db, &code).await; + assert_eq!(uses.len(), 1, "counter and audit rows disagree: {uses:?}"); + assert_eq!(uses[0].0, ok[0]["did"].as_str().unwrap()); +} + +/// The same race with room for more than one winner: a three-use code +/// hit by eight simultaneous registrations must admit exactly three. +/// +/// Worth having next to the one-use case because an implementation can +/// be "safe" by accident for a single use (e.g. by serialising every +/// registration globally) and still lose count when several are +/// genuinely allowed to proceed. +#[tokio::test] +async fn concurrent_registrations_respect_a_multi_use_limit() { + let Some(db) = db_pool().await else { + eprintln!("no pds database, skipping"); + return; + }; + let Some(pds) = start_pds(true).await else { + eprintln!("could not start a pds with PDS_INVITE_REQUIRED=true, skipping"); + return; + }; + + const N: usize = 8; + const USES: i32 = 3; + let code = seed_code(&db, USES, 0, false).await; + + let mut tasks = Vec::with_capacity(N); + for i in 0..N { + let http = pds.http.clone(); + let url = pds.url("/xrpc/com.atproto.server.createAccount"); + let code = code.clone(); + let h = handle(&format!("inv_mrace{i}")); + tasks.push(tokio::spawn(async move { + let resp = http + .post(url) + .json(&json!({ + "handle": h, + "password": "hunter2hunter2", + "invite_code": code, + })) + .send() + .await + .expect("concurrent createAccount"); + let status = resp.status().as_u16(); + let body: Value = resp.json().await.unwrap_or(Value::Null); + (status, body) + })); + } + + let mut ok = 0usize; + for t in tasks { + let (status, body) = t.await.unwrap(); + match status { + 200 => ok += 1, + 400 => assert_eq!(body["error"], "InvalidInviteCode", "body = {body}"), + other => panic!("unexpected status {other}: {body}"), + } + } + assert_eq!(ok, USES as usize, "a {USES}-use code admitted {ok} accounts"); + assert_eq!(used_count(&db, &code).await, USES); + assert_eq!(use_rows(&db, &code).await.len(), USES as usize); +} + +// -- the switch off --------------------------------------------------------- + +/// With `PDS_INVITE_REQUIRED=false` — the default, and what every other +/// test suite in this workspace relies on — nothing about `createAccount` +/// changes. +/// +/// This is the regression test for the whole feature's blast radius: the +/// switch is off by default precisely so that the existing suites keep +/// creating accounts with no code, and if that ever stopped being true +/// the failure would show up as dozens of unrelated tests breaking. It +/// shows up here instead. +#[tokio::test] +async fn switch_off_leaves_create_account_untouched() { + let Some(_db) = db_pool().await else { + eprintln!("no pds database, skipping"); + return; + }; + let Some(pds) = start_pds(false).await else { + eprintln!("could not start a pds with PDS_INVITE_REQUIRED=false, skipping"); + return; + }; + + assert_eq!( + pds.describe().await["invite_code_required"], + json!(false), + "describeServer must report the actual PDS_INVITE_REQUIRED value" + ); + + // No code at all: the historical behaviour. + let (s, b) = pds + .create_account(json!({ + "handle": handle("inv_off"), + "password": "hunter2hunter2", + })) + .await; + assert_eq!(s, 200, "no-code registration must still work: {b}"); + assert!(b["did"].as_str().unwrap().starts_with("did:")); + assert!(b["access_jwt"].is_string()); + + // A code that does not exist is simply ignored rather than becoming + // a new way to fail — a client that was talking to an invite-only + // PDS yesterday must not break when the operator opens the server up. + let (s, b) = pds + .create_account(json!({ + "handle": handle("inv_off_bogus"), + "password": "hunter2hunter2", + "invite_code": "mt-does-notexist", + })) + .await; + assert_eq!(s, 200, "an ignored code must not fail the request: {b}"); + + // The other validations are untouched. + let (s, b) = pds + .create_account(json!({ + "handle": handle("inv_off_short"), + "password": "short", + })) + .await; + assert_eq!(s, 400); + assert_eq!(b["error"], "InvalidPassword"); +} + +// -- the CLI ---------------------------------------------------------------- + +/// `pds-server invite create` / `list` / `disable`, run as the operator +/// would run them, against the real database. +/// +/// The point is not that the SQL works (the tests above cover that) but +/// that the *binary* exposes it: that `invite` short-circuits before the +/// server starts, that `create` prints bare codes one per line so they +/// can be pasted, and that a code it minted is actually redeemable. +#[tokio::test] +async fn invite_cli_mints_listable_redeemable_codes() { + let Some(db) = db_pool().await else { + eprintln!("no pds database, skipping"); + return; + }; + + let out = Command::new(env!("CARGO_BIN_EXE_pds-server")) + .args(["invite", "create", "--count", "3", "--uses", "2", "--note", "cli test"]) + .env("RUST_LOG", "warn") + .output() + .expect("run invite create"); + if !out.status.success() { + eprintln!( + "invite create failed (no env/db?), skipping: {}", + String::from_utf8_lossy(&out.stderr) + ); + return; + } + + let stdout = String::from_utf8_lossy(&out.stdout); + let codes: Vec<&str> = stdout.lines().filter(|l| !l.trim().is_empty()).collect(); + assert_eq!(codes.len(), 3, "one code per line, nothing else: {stdout:?}"); + for c in &codes { + // Bare, paste-ready: no labels, no quotes, no indentation. + assert_eq!(*c, c.trim(), "code line has surrounding whitespace: {c:?}"); + assert!(c.starts_with("mt-"), "unexpected code shape: {c}"); + assert_eq!( + sqlx::query_scalar::<_, i32>("SELECT max_uses FROM invite_codes WHERE code = $1") + .bind(c) + .fetch_one(&db) + .await + .expect("minted code must be in the table"), + 2, + "--uses must reach the row" + ); + } + // All three distinct — a generator that returned a constant would + // otherwise only show up as a primary-key error. + let unique: std::collections::HashSet<&&str> = codes.iter().collect(); + assert_eq!(unique.len(), 3); + + // `list` must show what `redeem` would accept. + let listed = Command::new(env!("CARGO_BIN_EXE_pds-server")) + .args(["invite", "list"]) + .env("RUST_LOG", "warn") + .output() + .expect("run invite list"); + assert!(listed.status.success()); + let listed = String::from_utf8_lossy(&listed.stdout); + for c in &codes { + assert!(listed.contains(*c), "invite list omitted {c}"); + } + + // `disable` takes a code out without deleting it. + let disabled = Command::new(env!("CARGO_BIN_EXE_pds-server")) + .args(["invite", "disable", codes[0]]) + .env("RUST_LOG", "warn") + .output() + .expect("run invite disable"); + assert!(disabled.status.success()); + assert!( + sqlx::query_scalar::<_, bool>("SELECT disabled FROM invite_codes WHERE code = $1") + .bind(codes[0]) + .fetch_one(&db) + .await + .unwrap() + ); + let listed = Command::new(env!("CARGO_BIN_EXE_pds-server")) + .args(["invite", "list"]) + .env("RUST_LOG", "warn") + .output() + .expect("run invite list"); + let listed = String::from_utf8_lossy(&listed.stdout); + assert!( + !listed.contains(codes[0]), + "a disabled code must not show in the default listing" + ); + + // And a minted code really lets an account through. + let Some(pds) = start_pds(true).await else { + eprintln!("could not start a pds with PDS_INVITE_REQUIRED=true, skipping redeem check"); + return; + }; + let (s, b) = pds + .create_account(json!({ + "handle": handle("inv_cli"), + "password": "hunter2hunter2", + "invite_code": codes[1], + })) + .await; + assert_eq!(s, 200, "CLI-minted code must be redeemable: {b}"); +} + +/// An unknown subcommand must not silently boot a server, and `help` +/// must not need a database. +#[tokio::test] +async fn unknown_subcommand_fails_instead_of_starting_a_server() { + let out = Command::new(env!("CARGO_BIN_EXE_pds-server")) + .args(["invit"]) + .env("RUST_LOG", "warn") + .output() + .expect("run bad subcommand"); + assert!(!out.status.success(), "a typo'd subcommand must not exit 0"); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(stderr.contains("unknown command"), "stderr = {stderr}"); + + let out = Command::new(env!("CARGO_BIN_EXE_pds-server")) + .args(["invite", "help"]) + .env("RUST_LOG", "warn") + .output() + .expect("run invite help"); + assert!(out.status.success()); + assert!(String::from_utf8_lossy(&out.stdout).contains("pds-server invite")); +} diff --git a/crates/pds-server/tests/pds_integration.rs b/crates/pds-server/tests/pds_integration.rs index bd18937..46949f1 100644 --- a/crates/pds-server/tests/pds_integration.rs +++ b/crates/pds-server/tests/pds_integration.rs @@ -44,7 +44,32 @@ async fn describe_server() { let did = r["did"].as_str().expect("describeServer must return a did"); assert!(did.starts_with("did:web:"), "did = {did}"); assert!(r["available_user_domains"].is_array()); - assert_eq!(r["invite_code_required"], json!(false)); + // `invite_code_required` used to be a hardcoded `false` here. It is + // now whatever `PDS_INVITE_REQUIRED` says, so this suite — which + // talks to whatever PDS the developer happens to be running — can + // only assert the type. That the value actually tracks the switch is + // pinned in `invite_integration.rs`, which starts a PDS with the + // flag set both ways and checks both answers. + assert!( + r["invite_code_required"].is_boolean(), + "invite_code_required = {}", + r["invite_code_required"] + ); + // If this test process shares the server's environment (the + // documented way to run the suite is + // `set -a; . ./.env; set +a; cargo test`), hold it to the exact + // value too. + if let Ok(raw) = std::env::var("PDS_INVITE_REQUIRED") { + let expected = matches!( + raw.trim().to_ascii_lowercase().as_str(), + "1" | "true" | "yes" | "on" + ); + assert_eq!( + r["invite_code_required"], + json!(expected), + "describeServer disagrees with PDS_INVITE_REQUIRED={raw}" + ); + } } /// `GET /.well-known/did.json` — the document the AppView fetches to diff --git a/migrations/pds/0004_invite_codes.sql b/migrations/pds/0004_invite_codes.sql new file mode 100644 index 0000000..6a1577c --- /dev/null +++ b/migrations/pds/0004_invite_codes.sql @@ -0,0 +1,186 @@ +-- PDS database schema 0004: invite codes for `com.atproto.server.createAccount`. +-- +-- Why +-- +-- Until now `createAccount` had no gate of any kind: no invite code, no rate +-- limit, and `describeServer` advertised `invite_code_required: false`. That +-- was survivable while the only reachable instance was `127.0.0.1:2583`. It +-- stops being survivable the moment the PDS answers on a public name, because +-- every accepted account is not just a row in `users` — it creates a `repos` +-- head, a key pair the server has to keep, an MST that grows with every write, +-- and firehose events that every subscribed AppView is obliged to index. A +-- single script could mint accounts until the disk filled up, and nothing in +-- the write path would consider that abnormal. +-- +-- This migration adds the smallest gate that actually closes that hole: an +-- account may only be created by presenting a code the operator handed out. +-- The gate is opt-in via `PDS_INVITE_REQUIRED` (default `false`, so the dozens +-- of integration tests that create throwaway accounts keep working); the +-- tables below exist unconditionally so that switching the flag on is a +-- restart, not a migration. +-- +-- Two tables, not one +-- ------------------- +-- A code can be worth more than one account (`--uses 5` for a group of +-- friends, a conference badge, a family). That means "who redeemed this code" +-- is a *set*, not a single column, so it cannot live on the code row. Putting +-- a `redeemed_by TEXT` column on `invite_codes` would have forced either +-- one-code-one-account (losing the multi-use case the operator actually wants) +-- or an array column that no foreign key, index or `COUNT(*)` can reason +-- about. `invite_code_uses` is that set, one row per redemption. +-- +-- The redemption *counter* still lives on the code row even though it is +-- derivable from `COUNT(*)` over `invite_code_uses`. That duplication is +-- deliberate and is the entire concurrency story — see below. +-- +-- invite_codes +-- ------------ +-- +-- code TEXT PRIMARY KEY — the code itself, and the natural key. No +-- surrogate `id`: the code is what the user types, what the +-- operator pastes into a chat window, and what the redeem query +-- looks up, so a second identifier would only add a join. +-- Codes are generated lowercase from a 32-character +-- Crockford-style alphabet (`crates/pds-server/src/invite.rs`), +-- and the server lowercases and trims what the client sends +-- before looking it up. Because every stored code is already +-- lowercase ASCII, that normalisation happens in Rust rather +-- than as `WHERE lower(code) = …`, which would throw away this +-- primary-key index on the hottest lookup this table has. +-- +-- created_at when the operator minted it. Purely for the `invite list` +-- output and for answering "where did this wave of signups come +-- from" after the fact. +-- +-- note free-text label the operator can attach at creation time +-- (`--note "meetup 2026-09"`). Nullable, never interpreted. +-- It exists because a bare list of random strings is unusable +-- a month later. +-- +-- max_uses how many accounts this code may create. `CHECK (max_uses > 0)` +-- because a zero-use code is not a thing you would ever mean to +-- create — it is a typo that would silently hand out a code that +-- can never work. +-- +-- used_count how many it has already created. Kept in sync with +-- `invite_code_uses` inside the same transaction that writes +-- both. +-- +-- disabled a code the operator wants to stop honouring *without* losing +-- the audit trail. Deleting the row would work for the future +-- but would take the `invite_code_uses` rows with it (see the +-- FK below) and with them the record of which accounts came +-- from that code — which is the one question you ask when a +-- code leaks. A boolean keeps the history and is checked in the +-- same `WHERE` clause as the counter, so disabling costs nothing +-- at redeem time. +-- +-- CHECK (used_count <= max_uses) — the belt to the redeem query's braces. +-- The application never over-redeems (the conditional UPDATE +-- below makes that impossible), but this constraint means that +-- *no* future query — a hand-written `UPDATE` during an +-- incident, a bug in a later refactor — can hand out more +-- accounts than the operator authorised. The database refuses. +-- +-- How the redeem race is closed +-- ----------------------------- +-- The obvious implementation is "SELECT the code, check `used_count < +-- max_uses` in Rust, then UPDATE". That is a check-then-act, and two +-- registrations arriving at the same instant with the same last remaining use +-- both read `used_count = 0`, both decide they are allowed, and both write +-- `used_count = 1` — two accounts from a one-use code, with the row still +-- claiming a single redemption. +-- +-- So the check and the act are one statement, and the database performs both: +-- +-- UPDATE invite_codes +-- SET used_count = used_count + 1 +-- WHERE code = $1 +-- AND NOT disabled +-- AND used_count < max_uses +-- RETURNING used_count, max_uses; +-- +-- Under Postgres's READ COMMITTED isolation the second transaction to reach +-- this row blocks on the row lock the first one took. When the first commits, +-- the second does not proceed with its stale snapshot: it re-reads the updated +-- row and re-evaluates the `WHERE` clause against it (EvalPlanQual). The +-- counter is now `1`, `used_count < max_uses` is false, the row no longer +-- matches, and the statement returns zero rows. Zero rows returned *is* the +-- rejection — the route turns it into `400 InvalidInviteCode` without ever +-- having formed an opinion of its own about whether the code was still valid. +-- +-- If the first transaction instead rolls back — the handle turned out to be +-- taken, key generation failed, anything — the lock is released with the +-- counter back at `0` and the waiting transaction's re-check succeeds. The +-- code is only consumed by a registration that actually completed, which is +-- why the redemption is issued inside `create_account`'s existing +-- transaction rather than before it. +-- +-- This is also why `used_count` is stored rather than computed. A +-- `COUNT(*) FROM invite_code_uses` has no row to lock — concurrent counters +-- both see the same pre-insert count and both pass. The counter column gives +-- the conditional UPDATE a single row to serialise on. +CREATE TABLE IF NOT EXISTS invite_codes ( + code TEXT PRIMARY KEY, + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + note TEXT, + max_uses INTEGER NOT NULL DEFAULT 1, + used_count INTEGER NOT NULL DEFAULT 0, + disabled BOOLEAN NOT NULL DEFAULT FALSE, + CONSTRAINT invite_codes_max_uses_positive CHECK (max_uses > 0), + CONSTRAINT invite_codes_used_count_sane CHECK (used_count >= 0 AND used_count <= max_uses) +); + +-- `invite list` shows the newest codes first, and that is the only listing +-- this table has. Small table, but the operator runs it interactively and an +-- ordered index keeps the output instant even after a few thousand codes. +CREATE INDEX IF NOT EXISTS invite_codes_created_at_idx + ON invite_codes (created_at DESC); + +-- ===================================================== +-- invite_code_uses — which account came from which code +-- ===================================================== +-- +-- code FK to `invite_codes(code)` ON DELETE CASCADE. Cascading is the +-- right call *here* (unlike `firehose_events.did` in 0003, which +-- deliberately has no FK) because these rows are meaningless +-- without the code they describe: they exist to answer "which +-- accounts did code X create", and a use-row whose code has been +-- deleted answers nothing. The operator who wants to stop a code +-- but keep the trail sets `disabled` instead of deleting — which +-- is precisely why that column exists. +-- +-- did the account that was created. Intentionally NOT a foreign key +-- to `users(did)`: this is an audit record of something that +-- happened, and it has to survive the account being deleted. If +-- it cascaded from `users`, deleting a spam account would erase +-- the evidence linking it to the code that let it in — the exact +-- moment the link matters most. The trade-off is that a `did` +-- here may point at a user that no longer exists; that is +-- accepted and is what an audit log looks like. +-- +-- handle the handle as it was at creation time, denormalised on +-- purpose. Handles can change, and `users` may be gone entirely +-- (see above); this column is a snapshot so the listing stays +-- readable without a join that may find nothing. +-- +-- used_at when the redemption happened. +-- +-- PRIMARY KEY (code, did) — one account can only consume a given code once. +-- This is not the mechanism that enforces the use limit (the +-- conditional UPDATE is), it is a guard against a redemption +-- being recorded twice for one account, which would make +-- `used_count` and this table disagree. +CREATE TABLE IF NOT EXISTS invite_code_uses ( + code TEXT NOT NULL REFERENCES invite_codes(code) ON DELETE CASCADE, + did TEXT NOT NULL, + handle TEXT NOT NULL, + used_at TIMESTAMPTZ NOT NULL DEFAULT now(), + PRIMARY KEY (code, did) +); + +-- The reverse lookup: "which code let this account in?". Asked per-account +-- during abuse triage, so it needs its own index — the (code, did) primary +-- key cannot serve a query whose only predicate is `did`. +CREATE INDEX IF NOT EXISTS invite_code_uses_did_idx + ON invite_code_uses (did);