kayg/calternal
Watch
1
0
Fork
You've already forked calternal
 0
Code Issues 191 Pull requests 61 Projects Releases Packages Wiki Activity Actions 20

auth: DAV app passwords (ADR 0064 piece 5, D3) #782

Closed
kayg wants to merge 0 commits from feat/dav-app-passwords into dev
pull from: feat/dav-app-passwords
merge into: kayg:dev
Conversation 9 Commits - Files changed -
kayg commented 2026-10-31 05:27:32 +00:00
Owner
Copy link

Final round implementation plan (resume from 40cfc587)

Keep the existing assertion timestamp, v9 migration and fair 250 ms cold queue. Add a minimum 8 ms lifetime to each of the eight global cold permits (including cancelled requests): eight-read burst, at most 1,000 cold DB reads/s sustained across all sources. Warm entries and TTL rechecks remain separate. Finite queued bursts admit valid cold passwords; sustained overload can exceed the bounded wait and return 429. Add a deterministic test with all eight permits held by junk behind a blocked DB worker, plus concurrent HTTP flood coverage.

Limiter lookup stays O(1); recording/eviction uses an ordered LRU index in O(log n), no capacity scan. Expiry sweeps stay at most once per five seconds. Memory remains bounded at 10,000 buckets per limiter. Make failure deduplication atomic. Preserve the existing monotonic checked_at and touch-only-if-present behavior.

Extend Authenticator's return type with an explicit HTTP rejection; all extractors propagate it. DAV middleware and trait implementation share response construction, preserving 401/404/429/503 and retry headers. Strengthen passkey-add tests with a real ceremony, and explicitly test setup/recovery/invite sessions and all Bidi_Control characters.

Failure Effect Detection Recovery Test
Concurrent junk at cold DB worker bounded queue/load global permits + minimum lifetime FIFO admission or 429 after 250 ms blocked-worker and concurrent HTTP tests
Old stolen session adds a key permanent credential escalation assertion freshness at add-finish/remove/mint sign in with a passkey real add ceremony rejected; setup/recovery/invite rejected
DAV trait rejection callers previously collapsed to 401 explicit rejection response preserve Retry-After trait status/header tests

Gates on dev-01 via Nix and slot: auth/server suites, contract tests, clippy (also attachments/backdrops affected by the trait), fmt. This finishes round 2; no additional review round or merge.

Plan (written first): DAV app passwords (ADR 0064 piece 5, plan step D3)

What this adds

  • auth.db v8: installations.scopes (comma list; non-empty only for kind dav). Triggers keep that pairing true. Old dav rows get an empty scope and so reach nothing.
  • App password = Installation of kind dav + scopes. Secret ca1_ + 52 base32 chars (32 random bytes). Only the BLAKE3 hash is stored (same choice as session tokens: the secret is 256 bits of randomness, so a slow password hash buys nothing and would make each DAV request expensive).
  • POST /api/v1/auth/app-passwords {name, scopes?}: signed-in only, returns the secret once. A plain route, not an action, because action results are kept in the idempotency table and the secret must never be stored.
  • Actions account.app-passwords (list, never secrets) and account.app-password-revoke. account.installations also shows them (with scopes).
  • Auth::dav_gate(scope) -> DavGate: HTTP Basic check for DAV routes only (/dav/**, /.well-known/{carddav,caldav}). Works as an axum middleware (adds DavIdentity) and as an Authenticator. D4 mounts it; nothing is mounted here.
  • Session auth (Auth as Authenticator) refuses DAV paths; the session lookup skips kind dav; the app-password lookup wants kind dav; prefixes differ (ci1_ vs ca1_).

Sizes and big-O

  • Per request: one BLAKE3 of 56 bytes, one HashMap lookup, one revoked-set lookup, one 32-byte constant-time compare. O(1). First use per process: one indexed SQLite read (token_hash is UNIQUE). Last-seen write at most hourly, in the background.
  • Mint: one transaction, O(1) (a count over the person's live app passwords, capped at 50). Revoke: O(1) update + set insert + cache purge.
  • Memory: cache entry about 100 bytes per app password seen; failure limiter capped at 10,000 sources (existing Limiter).
  • Basic header read capped at 1 KiB; exactly one Authorization header.

Failure table

Failure Effect Detection Recovery Test
Wrong password (near miss, junk, long, wrong case) none hash lookup + constant-time compare 401 + Basic challenge, empty body a_wrong_password_is_refused_with_a_challenge, near_misses_of_every_length_are_refused
Compare leaks timing secret guessed byte by byte blake3::Hash equality (constant time) on a fixed 32-byte hash; the map is keyed by the hash, never by the secret n/a hash_compare_catches_a_change_anywhere (unit); timing itself is not asserted (flaky)
Revoked password used (also on a warm cache) access after revoke revoked set checked after every load 401 on the next request revoking_stops_the_next_request_and_a_replay_fails
Replay after revoke, after restart access after revoke revoked_at in auth.db 401 a_revoked_password_stays_revoked_after_a_restart
Wrong scope (calendar password on contacts route and back) scope bleed scope list checked per route 401 the_wrong_scope_is_refused
App password on a non-DAV route (cookie, old cookie, bearer, Basic; create more passwords; log out) privilege widening session lookup skips kind dav; prefix check; Auth refuses DAV paths 401 app_passwords_do_not_reach_anything_outside_dav
Session on DAV (as Basic password, cookie, bearer) wrong door gate reads Basic only; lookup wants kind dav 401 sessions_do_not_reach_dav
User A's password on user B's URL cross-user read URL user id vs token user 404, empty (same as a person who does not exist) a_password_for_someone_else_s_url_is_not_found
Brute force / flood load, guessing failure counter per source (default 20/min, dav_failures_per_minute) 429 + Retry-After: 60, even with a good password from that source; other sources unaffected; a missing header is free (every client starts that way) guessing_is_throttled_per_source_and_does_not_spill_over
DB leak secrets usable only hashes stored; secret never in an action result or journal n/a the_database_never_holds_a_secret
Table fills up disk / list size cap 50 per person 429 there_is_a_limit_on_app_passwords_per_person
Revoking someone else's / a browser session through the app-password action wrong target user_id and kind in the UPDATE 404, nothing touched revoke_only_touches_your_own_app_passwords
Migration breaks auth down startup migrate in one immediate transaction old binary sees user_version 8 and opens as before (column is additive) version_7_gets_scopes_and_scope_rules_are_enforced
Bad scope combination written by buggy code dav with no scope, or browser with scope DB triggers insert/update aborts same test

Decisions (no questions asked)

  • Hash: BLAKE3, not Argon2 (see above; also what ADR 0064 says: reuse token.rs/session.rs).
  • Throttling is per source address only. A wrong password cannot be tied to a person (the user name field is ignored by design), so there is no per-user lockout; a per-user lockout would also let an attacker lock a person out of their own phone.
  • Valid password but wrong URL is not counted as a failure.
  • Revoke is the existing mechanism (immediate, in-memory revoked set + revoked_at).
  • Calendar exists as a scope so scope isolation is testable now, but creating one over HTTP is refused until CalDAV exists. Tests mint it through Auth::mint_app_password.
  • Last-seen updates at most hourly, and app passwords never expire or slide.
  • contracts/actions.json and apps/web generated types updated for the two new actions. No UI here.

Gates (run on calternal-dev-01-compute02, Nix dev shell, via the slot helper)

  • cargo test -j4 -p calternal-auth --no-fail-fast: all green, including 14 new tests in tests/app_passwords.rs and 3 new unit tests.
  • cargo test -j4 -p calternal contract (contracts/actions.json in sync), cargo test -j4 -p calternal-server --no-fail-fast: green.
  • cargo clippy -j4 -p calternal-auth --all-targets -- -D warnings and cargo fmt --all -- --check: clean.
  • Three existing tests that fake an old schema by setting user_version now also drop the v8 column/triggers first; their version asserts went 7 to 8.

Open risks

  • Nothing mounts the gate yet (D4 does). Tests use stand-in routes behind DavGate::middleware.
  • Behind a reverse proxy, throttling is per client address only if trusted_proxies is set; otherwise all clients share the proxy's bucket (same as login).
  • The revoked set is per process (existing design): a revoke done by a separate CLI process would not reach a running server until restart. Revoke through the server (the action) is immediate.
  • No UI yet for creating/listing app passwords (D11).

Co-Authored-By: Claude Sonnet 5.5 noreply@anthropic.com

🤖 Generated with Claude Code

https://claude.ai/code/session_01Xi6YxsuZ7m2ESGC7aS91AN

## Final round implementation plan (resume from 40cfc587) Keep the existing assertion timestamp, v9 migration and fair 250 ms cold queue. Add a minimum 8 ms lifetime to each of the eight global cold permits (including cancelled requests): eight-read burst, at most 1,000 cold DB reads/s sustained across all sources. Warm entries and TTL rechecks remain separate. Finite queued bursts admit valid cold passwords; sustained overload can exceed the bounded wait and return 429. Add a deterministic test with all eight permits held by junk behind a blocked DB worker, plus concurrent HTTP flood coverage. Limiter lookup stays O(1); recording/eviction uses an ordered LRU index in O(log n), no capacity scan. Expiry sweeps stay at most once per five seconds. Memory remains bounded at 10,000 buckets per limiter. Make failure deduplication atomic. Preserve the existing monotonic checked_at and touch-only-if-present behavior. Extend Authenticator's return type with an explicit HTTP rejection; all extractors propagate it. DAV middleware and trait implementation share response construction, preserving 401/404/429/503 and retry headers. Strengthen passkey-add tests with a real ceremony, and explicitly test setup/recovery/invite sessions and all Bidi_Control characters. | Failure | Effect | Detection | Recovery | Test | |---|---|---|---|---| | Concurrent junk at cold DB worker | bounded queue/load | global permits + minimum lifetime | FIFO admission or 429 after 250 ms | blocked-worker and concurrent HTTP tests | | Old stolen session adds a key | permanent credential escalation | assertion freshness at add-finish/remove/mint | sign in with a passkey | real add ceremony rejected; setup/recovery/invite rejected | | DAV trait rejection | callers previously collapsed to 401 | explicit rejection response | preserve Retry-After | trait status/header tests | Gates on dev-01 via Nix and slot: auth/server suites, contract tests, clippy (also attachments/backdrops affected by the trait), fmt. This finishes round 2; no additional review round or merge. # Plan (written first): DAV app passwords (ADR 0064 piece 5, plan step D3) ## What this adds - `auth.db` v8: `installations.scopes` (comma list; non-empty only for kind `dav`). Triggers keep that pairing true. Old `dav` rows get an empty scope and so reach nothing. - App password = Installation of kind `dav` + scopes. Secret `ca1_` + 52 base32 chars (32 random bytes). Only the BLAKE3 hash is stored (same choice as session tokens: the secret is 256 bits of randomness, so a slow password hash buys nothing and would make each DAV request expensive). - `POST /api/v1/auth/app-passwords {name, scopes?}`: signed-in only, returns the secret once. A plain route, **not an action**, because action results are kept in the idempotency table and the secret must never be stored. - Actions `account.app-passwords` (list, never secrets) and `account.app-password-revoke`. `account.installations` also shows them (with `scopes`). - `Auth::dav_gate(scope)` -> `DavGate`: HTTP Basic check for DAV routes only (`/dav/**`, `/.well-known/{carddav,caldav}`). Works as an axum middleware (adds `DavIdentity`) and as an `Authenticator`. D4 mounts it; nothing is mounted here. - Session auth (`Auth as Authenticator`) refuses DAV paths; the session lookup skips kind `dav`; the app-password lookup wants kind `dav`; prefixes differ (`ci1_` vs `ca1_`). ## Sizes and big-O - Per request: one BLAKE3 of 56 bytes, one `HashMap` lookup, one revoked-set lookup, one 32-byte constant-time compare. O(1). First use per process: one indexed SQLite read (`token_hash` is UNIQUE). Last-seen write at most hourly, in the background. - Mint: one transaction, O(1) (a count over the person's live app passwords, capped at 50). Revoke: O(1) update + set insert + cache purge. - Memory: cache entry about 100 bytes per app password seen; failure limiter capped at 10,000 sources (existing `Limiter`). - Basic header read capped at 1 KiB; exactly one `Authorization` header. ## Failure table | Failure | Effect | Detection | Recovery | Test | |---|---|---|---|---| | Wrong password (near miss, junk, long, wrong case) | none | hash lookup + constant-time compare | 401 + Basic challenge, empty body | `a_wrong_password_is_refused_with_a_challenge`, `near_misses_of_every_length_are_refused` | | Compare leaks timing | secret guessed byte by byte | `blake3::Hash` equality (constant time) on a fixed 32-byte hash; the map is keyed by the hash, never by the secret | n/a | `hash_compare_catches_a_change_anywhere` (unit); timing itself is not asserted (flaky) | | Revoked password used (also on a warm cache) | access after revoke | revoked set checked after every load | 401 on the next request | `revoking_stops_the_next_request_and_a_replay_fails` | | Replay after revoke, after restart | access after revoke | `revoked_at` in `auth.db` | 401 | `a_revoked_password_stays_revoked_after_a_restart` | | Wrong scope (calendar password on contacts route and back) | scope bleed | scope list checked per route | 401 | `the_wrong_scope_is_refused` | | App password on a non-DAV route (cookie, old cookie, bearer, Basic; create more passwords; log out) | privilege widening | session lookup skips kind `dav`; prefix check; `Auth` refuses DAV paths | 401 | `app_passwords_do_not_reach_anything_outside_dav` | | Session on DAV (as Basic password, cookie, bearer) | wrong door | gate reads Basic only; lookup wants kind `dav` | 401 | `sessions_do_not_reach_dav` | | User A's password on user B's URL | cross-user read | URL user id vs token user | 404, empty (same as a person who does not exist) | `a_password_for_someone_else_s_url_is_not_found` | | Brute force / flood | load, guessing | failure counter per source (default 20/min, `dav_failures_per_minute`) | 429 + `Retry-After: 60`, even with a good password from that source; other sources unaffected; a missing header is free (every client starts that way) | `guessing_is_throttled_per_source_and_does_not_spill_over` | | DB leak | secrets usable | only hashes stored; secret never in an action result or journal | n/a | `the_database_never_holds_a_secret` | | Table fills up | disk / list size | cap 50 per person | 429 | `there_is_a_limit_on_app_passwords_per_person` | | Revoking someone else's / a browser session through the app-password action | wrong target | user_id and kind in the UPDATE | 404, nothing touched | `revoke_only_touches_your_own_app_passwords` | | Migration breaks | auth down | startup migrate in one immediate transaction | old binary sees user_version 8 and opens as before (column is additive) | `version_7_gets_scopes_and_scope_rules_are_enforced` | | Bad scope combination written by buggy code | dav with no scope, or browser with scope | DB triggers | insert/update aborts | same test | ## Decisions (no questions asked) - Hash: BLAKE3, not Argon2 (see above; also what ADR 0064 says: reuse `token.rs`/`session.rs`). - Throttling is per source address only. A wrong password cannot be tied to a person (the user name field is ignored by design), so there is no per-user lockout; a per-user lockout would also let an attacker lock a person out of their own phone. - Valid password but wrong URL is not counted as a failure. - Revoke is the existing mechanism (immediate, in-memory revoked set + `revoked_at`). - `Calendar` exists as a scope so scope isolation is testable now, but creating one over HTTP is refused until CalDAV exists. Tests mint it through `Auth::mint_app_password`. - Last-seen updates at most hourly, and app passwords never expire or slide. - `contracts/actions.json` and `apps/web` generated types updated for the two new actions. No UI here. ## Gates (run on calternal-dev-01-compute02, Nix dev shell, via the slot helper) - `cargo test -j4 -p calternal-auth --no-fail-fast`: all green, including 14 new tests in `tests/app_passwords.rs` and 3 new unit tests. - `cargo test -j4 -p calternal contract` (contracts/actions.json in sync), `cargo test -j4 -p calternal-server --no-fail-fast`: green. - `cargo clippy -j4 -p calternal-auth --all-targets -- -D warnings` and `cargo fmt --all -- --check`: clean. - Three existing tests that fake an old schema by setting `user_version` now also drop the v8 column/triggers first; their version asserts went 7 to 8. ## Open risks - Nothing mounts the gate yet (D4 does). Tests use stand-in routes behind `DavGate::middleware`. - Behind a reverse proxy, throttling is per client address only if `trusted_proxies` is set; otherwise all clients share the proxy's bucket (same as login). - The revoked set is per process (existing design): a revoke done by a separate CLI process would not reach a running server until restart. Revoke through the server (the action) is immediate. - No UI yet for creating/listing app passwords (D11). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Xi6YxsuZ7m2ESGC7aS91AN
web: regenerate action types
Some checks failed
CI / server (pull_request) Has been cancelled
Details
CI / rust (pull_request) Has been cancelled
Details
CI / web (pull_request) Has been cancelled
Details
70ccd57cb9
kayg commented 2026-10-31 05:30:38 +00:00
Author
Owner
Copy link

Independent security review, round 1

Reviewed 70ccd57c (diff vs origin/dev, ADR 0064, plan in the PR body). Read-only; nothing built or run. Line numbers refer to the PR head.

Counts: P0 0, P1 2, P2 3, P3 8.

Checked and fine

  • Secret: 32 bytes from getrandom, base32, 52 chars, ca1_ prefix. 256 bits; BLAKE3 is fine for a uniformly random 256-bit secret (nothing to brute force; same as sessions). The secret is only in AppPassword and the mint response (no-store via the router layer). It is not in the action pipeline or idempotency table (plain route), not in the DB (hash only), and there is no TraceLayer or header logging. The debug!(?why) log carries only the enum.
  • Compare: the cache is keyed by the hash, so there is no secret-dependent compare on the lookup path; same_hash is constant time and redundant but harmless.
  • Scope in both directions: session lookup skips dav (session.rs:229), the ci1_/ca1_ prefixes differ, Auth::authenticate returns None on DAV paths (http.rs, is_dav_path), the gate reads Basic only, and DavGate::check denies any non-DAV path. Bearer or cookie carrying ca1_ fails plausible_session_token.
  • Revocation race: verify inserts into the cache, then checks the revoked set. forget_installation marks the set first, then purges. Every interleaving I walked through ends in 401; touch re-inserting a purged entry is also caught by the revoked check. A restart is covered by revoked_at in the DB.
  • Migration v8: additive. Old dav rows get scope '' and reach nothing; the old session lookup no longer honours them. The triggers only fire on UPDATE OF kind, scopes, so revoke is unaffected. An old binary on a v8 DB works (it only accepts ci1_).
  • Basic parsing: exactly one header, length cap before decode, strict base64, UTF-8, colon required, scheme case-insensitive. No panics (t[4..] is safe after the ASCII prefix check).
  • No user-existence oracle for unauthenticated callers: every URL gives 401 until a valid password is presented; 404 only after a valid password.

P1

P1-1. The URL-user check fails open when the path does not parse as a user id. dav.rs:135, dav.rs:57-59.
url_user returns None for /dav, /dav/, /dav/<non-uuid>/.., /dav//<uuid>/.., and percent-encoded ids (/dav/%31...). is_some_and(...) is then false and the request is Allowed with a valid DavIdentity for user A. The check compares the raw, still-encoded path. If D4's router or handlers decode the segment (or take the user from the URL, which ADR 0064 says they do), /dav/%<encoded uuid of B>/contacts/ passes the gate as A and a handler reads B's book. It is latent today because nothing is mounted, but this is the one place the cross-user rule is enforced.
Fix: for any path under /dav/ (not the bare /dav or the two well-known paths), require segment 1 to be a canonical user id with no %, \, .. or empty segments, and return NotFound otherwise. Document and test that handlers must use DavIdentity.user, never re-parse the URL. Add tests: /dav/%<uuid>/x, /dav//<uuid>/x, /dav/./<uuid>, uppercase uuid, /dav/<uuid>%2f...

P1-2. The per-source throttle also blocks valid credentials, so one bad device or one attacker locks out everyone sharing the source. dav.rs:123, limit.rs has_room; the test guessing_is_throttled_per_source... asserts this behaviour.
has_room is checked before the password is verified, so past 20 failures a source gets 429 even with a correct password. Scenarios:
(a) A household or office NAT: a phone with a revoked or stale password retries on every sync (Apple retries aggressively). 20 failures per minute is easy, and every other device on that IP, including the same user's working ones, gets 429 and keeps getting it. The outage is permanent while the stale device stays configured.
(b) trusted_proxies unset (the default) behind a reverse proxy, or no ConnectInfo: source_key is the proxy address or "unknown". One anonymous attacker sending 20 bad Basic headers per minute locks every user out of DAV indefinitely. The plan accepts this as "same as login", but login lockout does not take down an already-configured background sync.
Fix: the secret is 256 bits, so the throttle only protects the database and CPU, not guessing. Verify first: a secret found in the cache or DB (valid, unrevoked, right scope) always passes, and the limiter only gates the uncached/unknown lookup path (check has_room only when the DB has to be consulted, and count only unknown secrets). Also log a startup warning when the origin looks proxied and trusted_proxies is empty. Change the test to assert a valid password still works while the source is throttled.

P2

P2-1. Any session, including agent and CLI bearer tokens, can mint a permanent credential; no step-up. http.rs:408-412.
app_password_create accepts whatever authenticate accepts: browser, native, cli or agent installations. A stolen cookie, or an AI agent token, can mint a non-expiring ca1_ secret that outlives revocation of the session that created it (no link between them) and survives "sign out" of that session. Fix: require kind Browser/Native, preferably a fresh passkey assertion (as for adding a passkey), reject Cli/Agent, and emit a visible event or notification (announce is in the background only).

P2-2. The app-password cache never expires and never re-reads the DB; DB errors are swallowed. app_password.rs:175-200.
Once cached, a password is valid until process restart or an in-process revoke. A revoke from another process (calternal user ... CLI, which the code explicitly supports opening auth.db next to a live server, or a second instance during a rolling deploy) never reaches it. Sessions at least have expires_at; app passwords have no bound at all. The PR lists this as an open risk, but with non-expiring credentials it should be bounded. Also .ok().flatten().ok_or(Unknown) turns a DB error (busy, I/O) into a 401 that counts as a failure and feeds the throttle, so a DB hiccup throttles legitimate devices.
Fix: add a TTL to cache entries (about 60 s) and re-check revoked_at on expiry (one indexed read); map DB errors to 503 and do not count them.

P2-3. DAV credentials get ambient-credential CSRF exposure once D6 writes exist. dav.rs:170 (WWW-Authenticate: Basic).
Sending a Basic challenge on the app's own origin lets browsers prompt and then cache the app password for the origin, attached automatically to cross-site requests (<form> POST, fetch with no-cors) to /dav/**. Mostly mitigated by a DAV write needing a vCard body and a method that browsers rarely send cross-site, but PUT/DELETE through simple methods are not all safe. Fix: in the gate, refuse requests that carry a browser Origin or Sec-Fetch-Site: cross-site (DAV clients send neither), and consider a challenge only on DAV paths (already the case). Add a test.

P3

  1. app_password.rs:63 AppPassword derives Debug with the secret in the struct; a stray {:?} or dbg! logs it. Use a redacting manual Debug for secret.
  2. dav.rs:51 The whole credential must be UTF-8, including the ignored user name. Clients that send a Latin-1 user name with a valid ASCII password get 401 and a counted failure. Decode lossy for the user part, or split on the first : in bytes and require only the password to be UTF-8.
  3. dav.rs:32 Path matching is exact and case-sensitive, which is right, but nothing guards the D4 mount: no NormalizePath/trailing-slash layer, no percent-decoding before the gate, and /.well-known/carddav/ or //dav/x are classed non-DAV (so session auth applies to them, which is only a problem if they route). Add an end-to-end test through the real router for //dav/x, /%64av/x, /dav/../api/v1/auth/me, /DAV/x, /.well-known/carddav/, plus HEAD and OPTIONS on every DAV route.
  4. app_password.rs:105 The cap of 50 can be filled by anyone holding a session (see P2-1), which blocks the owner from adding a device; mint is not rate limited. Add a small per-user mint rate limit.
  5. IPv6 sources are bucketed per full address, so a /64 rotates around the throttle; bucket by /64. The limiter's retain is O(n) under a mutex on every failure (10,000 cap); an attacker spreading failures across many sources adds contention, and each unknown plausible secret costs one SQLite read.
  6. dav.rs serves Basic with no scheme check. AuthConfig requires an https origin (localhost excepted), so it is fine by default, but a plain-http listener behind a proxy would accept app passwords; consider refusing when X-Forwarded-Proto: http from a trusted proxy.
  7. db.rs triggers only test non-empty scopes, so a dav row with scopes='bogus' is accepted (it decodes to no scope and grants nothing; harmless, but a GLOB check would make the DB rule match the code).
  8. session.rs:347 clean_name strips control characters but not bidi overrides or zero-width characters, so a device name can spoof another in the list. Names also show no creator or creation source.
# Independent security review, round 1 Reviewed 70ccd57c (diff vs `origin/dev`, ADR 0064, plan in the PR body). Read-only; nothing built or run. Line numbers refer to the PR head. **Counts: P0 0, P1 2, P2 3, P3 8.** ## Checked and fine - Secret: 32 bytes from `getrandom`, base32, 52 chars, `ca1_` prefix. 256 bits; BLAKE3 is fine for a uniformly random 256-bit secret (nothing to brute force; same as sessions). The secret is only in `AppPassword` and the mint response (`no-store` via the router layer). It is not in the action pipeline or idempotency table (plain route), not in the DB (hash only), and there is no TraceLayer or header logging. The `debug!(?why)` log carries only the enum. - Compare: the cache is keyed by the hash, so there is no secret-dependent compare on the lookup path; `same_hash` is constant time and redundant but harmless. - Scope in both directions: session lookup skips `dav` (`session.rs:229`), the `ci1_`/`ca1_` prefixes differ, `Auth::authenticate` returns None on DAV paths (`http.rs`, `is_dav_path`), the gate reads Basic only, and `DavGate::check` denies any non-DAV path. Bearer or cookie carrying `ca1_` fails `plausible_session_token`. - Revocation race: `verify` inserts into the cache, then checks the revoked set. `forget_installation` marks the set first, then purges. Every interleaving I walked through ends in 401; `touch` re-inserting a purged entry is also caught by the revoked check. A restart is covered by `revoked_at` in the DB. - Migration v8: additive. Old `dav` rows get scope `''` and reach nothing; the old session lookup no longer honours them. The triggers only fire on `UPDATE OF kind, scopes`, so revoke is unaffected. An old binary on a v8 DB works (it only accepts `ci1_`). - Basic parsing: exactly one header, length cap before decode, strict base64, UTF-8, colon required, scheme case-insensitive. No panics (`t[4..]` is safe after the ASCII prefix check). - No user-existence oracle for unauthenticated callers: every URL gives 401 until a valid password is presented; 404 only after a valid password. ## P1 **P1-1. The URL-user check fails open when the path does not parse as a user id.** `dav.rs:135`, `dav.rs:57-59`. `url_user` returns None for `/dav`, `/dav/`, `/dav/<non-uuid>/..`, `/dav//<uuid>/..`, and percent-encoded ids (`/dav/%31...`). `is_some_and(...)` is then false and the request is `Allowed` with a valid `DavIdentity` for user A. The check compares the raw, still-encoded path. If D4's router or handlers decode the segment (or take the user from the URL, which ADR 0064 says they do), `/dav/%<encoded uuid of B>/contacts/` passes the gate as A and a handler reads B's book. It is latent today because nothing is mounted, but this is the one place the cross-user rule is enforced. Fix: for any path under `/dav/` (not the bare `/dav` or the two well-known paths), require segment 1 to be a canonical user id with no `%`, `\`, `..` or empty segments, and return NotFound otherwise. Document and test that handlers must use `DavIdentity.user`, never re-parse the URL. Add tests: `/dav/%<uuid>/x`, `/dav//<uuid>/x`, `/dav/./<uuid>`, uppercase uuid, `/dav/<uuid>%2f..`. **P1-2. The per-source throttle also blocks valid credentials, so one bad device or one attacker locks out everyone sharing the source.** `dav.rs:123`, `limit.rs has_room`; the test `guessing_is_throttled_per_source...` asserts this behaviour. `has_room` is checked before the password is verified, so past 20 failures a source gets 429 even with a correct password. Scenarios: (a) A household or office NAT: a phone with a revoked or stale password retries on every sync (Apple retries aggressively). 20 failures per minute is easy, and every other device on that IP, including the same user's working ones, gets 429 and keeps getting it. The outage is permanent while the stale device stays configured. (b) `trusted_proxies` unset (the default) behind a reverse proxy, or no `ConnectInfo`: `source_key` is the proxy address or `"unknown"`. One anonymous attacker sending 20 bad Basic headers per minute locks every user out of DAV indefinitely. The plan accepts this as "same as login", but login lockout does not take down an already-configured background sync. Fix: the secret is 256 bits, so the throttle only protects the database and CPU, not guessing. Verify first: a secret found in the cache or DB (valid, unrevoked, right scope) always passes, and the limiter only gates the uncached/unknown lookup path (check `has_room` only when the DB has to be consulted, and count only unknown secrets). Also log a startup warning when the origin looks proxied and `trusted_proxies` is empty. Change the test to assert a valid password still works while the source is throttled. ## P2 **P2-1. Any session, including agent and CLI bearer tokens, can mint a permanent credential; no step-up.** `http.rs:408-412`. `app_password_create` accepts whatever `authenticate` accepts: browser, native, cli or agent installations. A stolen cookie, or an AI agent token, can mint a non-expiring `ca1_` secret that outlives revocation of the session that created it (no link between them) and survives "sign out" of that session. Fix: require kind Browser/Native, preferably a fresh passkey assertion (as for adding a passkey), reject Cli/Agent, and emit a visible event or notification (`announce` is in the background only). **P2-2. The app-password cache never expires and never re-reads the DB; DB errors are swallowed.** `app_password.rs:175-200`. Once cached, a password is valid until process restart or an in-process revoke. A revoke from another process (`calternal user ...` CLI, which the code explicitly supports opening `auth.db` next to a live server, or a second instance during a rolling deploy) never reaches it. Sessions at least have `expires_at`; app passwords have no bound at all. The PR lists this as an open risk, but with non-expiring credentials it should be bounded. Also `.ok().flatten().ok_or(Unknown)` turns a DB error (busy, I/O) into a 401 that counts as a failure and feeds the throttle, so a DB hiccup throttles legitimate devices. Fix: add a TTL to cache entries (about 60 s) and re-check `revoked_at` on expiry (one indexed read); map DB errors to 503 and do not count them. **P2-3. DAV credentials get ambient-credential CSRF exposure once D6 writes exist.** `dav.rs:170` (`WWW-Authenticate: Basic`). Sending a Basic challenge on the app's own origin lets browsers prompt and then cache the app password for the origin, attached automatically to cross-site requests (`<form>` POST, `fetch` with `no-cors`) to `/dav/**`. Mostly mitigated by a DAV write needing a vCard body and a method that browsers rarely send cross-site, but PUT/DELETE through simple methods are not all safe. Fix: in the gate, refuse requests that carry a browser `Origin` or `Sec-Fetch-Site: cross-site` (DAV clients send neither), and consider a challenge only on DAV paths (already the case). Add a test. ## P3 1. `app_password.rs:63` `AppPassword` derives `Debug` with the secret in the struct; a stray `{:?}` or `dbg!` logs it. Use a redacting manual `Debug` for `secret`. 2. `dav.rs:51` The whole credential must be UTF-8, including the ignored user name. Clients that send a Latin-1 user name with a valid ASCII password get 401 and a counted failure. Decode lossy for the user part, or split on the first `:` in bytes and require only the password to be UTF-8. 3. `dav.rs:32` Path matching is exact and case-sensitive, which is right, but nothing guards the D4 mount: no `NormalizePath`/trailing-slash layer, no percent-decoding before the gate, and `/.well-known/carddav/` or `//dav/x` are classed non-DAV (so session auth applies to them, which is only a problem if they route). Add an end-to-end test through the real router for `//dav/x`, `/%64av/x`, `/dav/../api/v1/auth/me`, `/DAV/x`, `/.well-known/carddav/`, plus HEAD and OPTIONS on every DAV route. 4. `app_password.rs:105` The cap of 50 can be filled by anyone holding a session (see P2-1), which blocks the owner from adding a device; mint is not rate limited. Add a small per-user mint rate limit. 5. IPv6 sources are bucketed per full address, so a /64 rotates around the throttle; bucket by /64. The limiter's `retain` is O(n) under a mutex on every failure (10,000 cap); an attacker spreading failures across many sources adds contention, and each unknown plausible secret costs one SQLite read. 6. `dav.rs` serves Basic with no scheme check. `AuthConfig` requires an https origin (localhost excepted), so it is fine by default, but a plain-http listener behind a proxy would accept app passwords; consider refusing when `X-Forwarded-Proto: http` from a trusted proxy. 7. `db.rs` triggers only test non-empty scopes, so a `dav` row with `scopes='bogus'` is accepted (it decodes to no scope and grants nothing; harmless, but a `GLOB` check would make the DB rule match the code). 8. `session.rs:347 clean_name` strips control characters but not bidi overrides or zero-width characters, so a device name can spoof another in the list. Names also show no creator or creation source.
kayg commented 2026-10-31 05:39:59 +00:00
Author
Owner
Copy link

Review round 1: findings and status (head e358f3fa)

Finding Status What changed / test
P1-1 URL-user check fails open Fixed dav_target is strict and never decodes. Under /dav/ the first segment must be exactly the owner's canonical (lowercase, hyphenated) id; %, \, ;, empty, ., .. segments and non-ASCII are refused (404 after sign-in). is_dav_path now also catches //dav, /%64av, /DAV, trailing-slash well-known. Handlers must use DavIdentity.user, never the URL. Tests: odd_paths_never_reach_a_handler_as_someone_else (catch-all and real-style routes), unit dav_targets_fail_closed
P1-2 Throttle blocks valid credentials Fixed The secret is checked first. A valid or cached password is never throttled. Only unknown/revoked secrets count, and a source is charged once per credential per minute; each credential also has its own bucket (so a stale device throttles only itself). A source over its limit still gets DB lookups through a small semaphore, so a good password passes a flood; unknown secrets get 429. Tests: a_stale_device_behind_a_shared_address_locks_nobody_out, an_anonymous_flood_does_not_block_a_valid_user, rewritten guessing_is_throttled_per_source.... Startup warning for unset trusted_proxies: not done (follow-up)
P2-1 Mint without step-up Fixed (design below) Session must be kind browser/native (cli/agent refused, 403) and created within app_password_fresh (10 min). A session is only created by passkey sign-in, so its age is the age of the last passkey assertion; older sessions sign in again. A separate "re-verify with passkey" endpoint is a UI follow-up. Test: minting_needs_a_fresh_interactive_session. Visible notification of a new device: not done (follow-up)
P2-2 Cache outlives revoke elsewhere; DB errors Fixed Chose a TTL re-check (30 s, app_password_recheck): one indexed read per active password per interval, cheaper than a generation counter that needs a read on every request. DB errors are 503 with Retry-After: 5 and are not counted. Tests: a_revoke_from_another_process_is_seen_after_the_recheck, the_default_recheck_keeps_the_cache_warm, a_database_error_is_503_and_is_not_counted
P2-3 CSRF for D6 writes Follow-up (D6) Required design: the DAV write gate must reject requests carrying Origin from another site or Sec-Fetch-Site: cross-site, and must not rely on a Basic challenge being cached by browsers; decide the UA/header rule in D6 when writes exist. Nothing is mounted for writes yet
P3-1 Secret in Debug Fixed Manual Debug hides it. Test the_secret_stays_out_of_debug_output
P3-2 Latin-1 user name Fixed Only the password must be UTF-8
P3-3 Odd paths / router test Fixed See P1-1 (//dav/x, /%64av/x, /DAV/x, /dav/../api/v1/auth/me, well-known trailing slash). HEAD/OPTIONS per route: D4
P3-4 Mint rate limit Fixed dav_mints_per_minute (10) per person. Test minting_is_rate_limited_per_person
P3-5 IPv6 /64 bucketing Follow-up Not changed
P3-6 Plaintext-http check Follow-up Not changed
P3-7 Trigger accepts any scope text Fixed v8 triggers now allow only '', contacts, calendar, calendar,contacts (v8 is unmerged, edited in place). Test in db.rs
P3-8 Bidi/zero-width in device names Fixed clean_name strips them. Unit test

Gates (compute02, Nix shell, ~/bin/slot, -j4): cargo fmt --all --check; cargo test -p calternal-auth --no-fail-fast (all green, 23 tests in app_passwords); cargo test -p calternal contract; cargo test -p calternal-server --no-fail-fast; cargo clippy -p calternal-auth -p calternal-server --all-targets -- -D warnings.

# Review round 1: findings and status (head e358f3fa) | Finding | Status | What changed / test | |---|---|---| | P1-1 URL-user check fails open | Fixed | `dav_target` is strict and never decodes. Under `/dav/` the first segment must be exactly the owner's canonical (lowercase, hyphenated) id; `%`, `\`, `;`, empty, `.`, `..` segments and non-ASCII are refused (404 after sign-in). `is_dav_path` now also catches `//dav`, `/%64av`, `/DAV`, trailing-slash well-known. Handlers must use `DavIdentity.user`, never the URL. Tests: `odd_paths_never_reach_a_handler_as_someone_else` (catch-all and real-style routes), unit `dav_targets_fail_closed` | | P1-2 Throttle blocks valid credentials | Fixed | The secret is checked first. A valid or cached password is never throttled. Only unknown/revoked secrets count, and a source is charged once per credential per minute; each credential also has its own bucket (so a stale device throttles only itself). A source over its limit still gets DB lookups through a small semaphore, so a good password passes a flood; unknown secrets get 429. Tests: `a_stale_device_behind_a_shared_address_locks_nobody_out`, `an_anonymous_flood_does_not_block_a_valid_user`, rewritten `guessing_is_throttled_per_source...`. Startup warning for unset `trusted_proxies`: not done (follow-up) | | P2-1 Mint without step-up | Fixed (design below) | Session must be kind browser/native (cli/agent refused, 403) and created within `app_password_fresh` (10 min). A session is only created by passkey sign-in, so its age is the age of the last passkey assertion; older sessions sign in again. A separate "re-verify with passkey" endpoint is a UI follow-up. Test: `minting_needs_a_fresh_interactive_session`. Visible notification of a new device: not done (follow-up) | | P2-2 Cache outlives revoke elsewhere; DB errors | Fixed | Chose a TTL re-check (30 s, `app_password_recheck`): one indexed read per active password per interval, cheaper than a generation counter that needs a read on every request. DB errors are 503 with `Retry-After: 5` and are not counted. Tests: `a_revoke_from_another_process_is_seen_after_the_recheck`, `the_default_recheck_keeps_the_cache_warm`, `a_database_error_is_503_and_is_not_counted` | | P2-3 CSRF for D6 writes | Follow-up (D6) | Required design: the DAV write gate must reject requests carrying `Origin` from another site or `Sec-Fetch-Site: cross-site`, and must not rely on a Basic challenge being cached by browsers; decide the UA/header rule in D6 when writes exist. Nothing is mounted for writes yet | | P3-1 Secret in `Debug` | Fixed | Manual `Debug` hides it. Test `the_secret_stays_out_of_debug_output` | | P3-2 Latin-1 user name | Fixed | Only the password must be UTF-8 | | P3-3 Odd paths / router test | Fixed | See P1-1 (`//dav/x`, `/%64av/x`, `/DAV/x`, `/dav/../api/v1/auth/me`, well-known trailing slash). HEAD/OPTIONS per route: D4 | | P3-4 Mint rate limit | Fixed | `dav_mints_per_minute` (10) per person. Test `minting_is_rate_limited_per_person` | | P3-5 IPv6 /64 bucketing | Follow-up | Not changed | | P3-6 Plaintext-http check | Follow-up | Not changed | | P3-7 Trigger accepts any scope text | Fixed | v8 triggers now allow only `''`, `contacts`, `calendar`, `calendar,contacts` (v8 is unmerged, edited in place). Test in `db.rs` | | P3-8 Bidi/zero-width in device names | Fixed | `clean_name` strips them. Unit test | Gates (compute02, Nix shell, `~/bin/slot`, `-j4`): `cargo fmt --all --check`; `cargo test -p calternal-auth --no-fail-fast` (all green, 23 tests in `app_passwords`); `cargo test -p calternal contract`; `cargo test -p calternal-server --no-fail-fast`; `cargo clippy -p calternal-auth -p calternal-server --all-targets -- -D warnings`.
kayg commented 2026-10-31 05:43:13 +00:00
Author
Owner
Copy link

Independent security review, round 2

Reviewed head e358f3fa (diff 70ccd57c..HEAD and the whole PR vs origin/dev). Read-only; nothing built or run. Line numbers are for the PR head.

Verdict: ready to merge as a pre-D4 building block, with the P2s below filed as D4 blockers. No P0, no P1. All round-1 P1s are fixed and their tests prove them. The new findings are about the fixes' edges (the step-up is weaker than described, the cold-lookup path can still be starved).

Round-1 items

Item Result Notes
P1-1 URL-user fails open Verified dav_target (dav.rs:110) never decodes; refuses %, \, ;, ?, #, non-ASCII, empty/./.. segments, non-canonical ids. check (dav.rs:217) only lets Root or User(u)==owner through; everything else is 404. I walked /dav/%<uuid>, //dav, uppercase, hyphenless, <uuid>%2f.., trailing-slash well-known: all refused. is_dav_path is wide on purpose (decode + lowercase + trim slashes), so session auth refuses every spelling. The tests (odd_paths_never_reach_a_handler_as_someone_else against a catch-all and real-style routes, plus unit dav_targets_fail_closed) assert the right things, including that user B's id never appears in the body.
P1-2 Throttle blocks valid users Verified, one hole (N-1) Secret is checked first; cached/valid is never throttled (app_password.rs:223-273). Per-credential bucket plus "charge the source once per credential per minute" stops a stale device starving its NAT neighbours (test shows a 30-iteration retry loop and a fresh device still passing). Memory: credential and source limiters are each capped at 10,000 entries with LRU eviction (limit.rs:43), a junk flood from one source stops creating credential buckets after 20 because over returns before fail(). Bounded. See N-1 for the concurrency hole.
P2-1 Mint step-up Partly fixed (N-2) Kind check is solid (only passkey login and setup create sessions, both Browser; cli/agent get 403). created_at is never changed by touch (it only moves last_seen_at/expires_at) and there is no rotation, so session age really is time since sign-in. But see N-2.
P2-2 Cache TTL / DB errors Verified 30 s checked_at recheck refreshes scopes and revocation from the DB; DB error gives 503 with Retry-After: 5 and is not charged (app_password.rs:245-251). Revoke from another process test uses a real second connection. Not covered: error on a warm-expired entry (it returns 503, which is fine fail-closed).
P2-3 CSRF Follow-up (not re-raised)
P3-1 Debug Verified Manual Debug hides secret; no other type holding the secret derives Debug; debug!(?why) is enum only.
P3-2 Latin-1 user Verified Split on first : in bytes, only the password must be UTF-8; unit test covers it.
P3-3 odd paths Verified See P1-1.
P3-4 Mint rate limit Verified dav_mint_limit per user (http.rs:455), test passes. Counts failed JSON/scope attempts too; harmless.
P3-5, P3-6 Follow-up
P3-7 Trigger values Verified Insert and update triggers allow only '', contacts, calendar, calendar,contacts; encode_scopes sorts and dedups so it always writes one of those. Test covers bogus values and stripping.
P3-8 Bidi Verified, gap (N-6)

New findings

P2

N-1. A flood still starves a valid user whose password is not cached. app_password.rs:228-244, dav_overflow in lib.rs.
try_acquire on the 8-permit semaphore fails immediately when the permits are busy, and the permit holders are the attacker's own lookups queued behind the single auth-db worker (db.rs, one thread, queue of 64). With more than 8 concurrent junk requests from the shared source (the default when trusted_proxies is empty: everyone is "unknown"), all 8 permits are held nearly all the time, so a cold valid password from that source gets 429. Cold means: first request after a restart or deploy, a new device, or after any entry is evicted. Warm clients are fine (cached entries skip the whole branch), so the failure shows up exactly after restarts, when every device is cold at once. Apple backs off on 429, so it can last a while. The test an_anonymous_flood_does_not_block_a_valid_user is sequential; it never holds a permit, so it does not prove the semaphore.
Separately: lookups for sources that are not yet over their limit are not capped globally. N sources times 20 distinct junk secrets per minute all go to the one DB worker, which also serves logins, session misses, action revokes and the 30 s rechecks of every warm DAV password. A botnet or an IPv6 /64 can fill that queue (64 slots, send().await blocks) and add latency to all of auth.
Fix: make the cold-lookup permit apply to every uncached lookup, and use acquire with a short timeout (e.g. 250 ms) for sources that are not over, try_acquire only for over sources, so a valid password queues behind junk rather than losing to it. Add a global per-second cap on cold lookups. Add a concurrent test: hold all 8 permits with slow junk lookups, then assert a valid cold password gets 200 within a bound.

N-2. The step-up is bypassable through passkey-add, and its description overstates what created_at proves. http.rs:410-441, account.rs (account.passkey-add-*), setup.rs:387.
Session created_at is the time of a passkey sign-in, a setup/recovery/invite link redemption (setup.rs:387 also makes a Browser session from a bearer link plus a new passkey, no assertion), or nothing else. That is fine as far as it goes. But account.passkey-add-start/finish needs only an ordinary session of any age. An attacker holding a stolen old cookie adds their own passkey, signs in with it (fresh session, 0 s old), and mints. The 10-minute window then buys nothing against cookie theft; it only stops a cookie that is both stolen and used within the window by someone who does not know about passkey-add. The same goes for passkey-remove of the victim's key.
Fix: apply the same freshness rule (fresh_interactive_session) to passkey-add-finish and passkey-remove, or better add the real step-up (a new assertion whose ceremony id is consumed by the mint request). Until then, say in the PR and ADR that the step-up defends against a cli/agent token and a very old stolen session only. Also emit a visible event on mint and on passkey-add (already a documented follow-up for new devices, but it is now what makes this bypass detectable).

P3

N-3. Malformed passwords are charged to the source every time, not once per credential. app_password.rs:208-212. Only well-formed unknown secrets are deduplicated by fail(). A device with a typo'd, truncated or padded password (or the person's login password typed into Apple Contacts) sends junk that is not ca1_ + 52, and every retry costs the source a full count, so 20 retries a minute push the NAT's source over. Effect is mild now (cold valid lookups then need a permit, see N-1), but it brings back a small version of round-1 P1-2. Fix: dedupe those by the hash of the presented string in the credential bucket too, or charge them once per source per 10 s.

N-4. Limiter::attempt is O(n) under a global mutex on every call, and DAV now calls it on the hottest unauthenticated path. limit.rs:42 (retain over up to 10,000 entries). dav_failure_limit.attempt runs for every junk header with no DB cost, has_room/count take the same lock. At tens of thousands of requests per second this burns a core and serializes the async threads on a std mutex. Fix: sweep expired entries at most once per few seconds (or when len is near capacity), not on each call.

N-5. touch reinserts the cloned entry with its old checked_at. app_password.rs:293-295, 333-341. It can overwrite a fresher entry or resurrect one that a recheck just removed because a revoke happened in another process, extending exposure by at most one more 30 s window. Also checked_at uses wall-clock now_ms, so a clock step backwards makes now - checked_at < recheck stay true for as long as the step. Fix: in touch update only touched_at if the key is still present; use a monotonic Instant for the recheck.

N-6. Name sanitizing misses some invisible/bidi characters. session.rs:347. Not covered: U+061C (Arabic letter mark, a bidi control), U+00AD, U+034F, U+180E, U+3164, variation selectors, tag characters U+E0000-E007F. Use char::is_control plus a Default_Ignorable_Code_Point check, and reuse it for passkey labels (passkeys::clean_label does not strip bidi at all).

N-7. DB migration edited in place. db.rs SCHEMA_V8. Any dev database already at user_version = 8 from the first commit keeps the old, looser triggers and never gets the tightened ones. Only matters for machines that ran the earlier branch head; note it, or bump to a v9 that drops and recreates the triggers.

N-8. D4 mounting notes (not bugs today). (a) check reads parts.uri.path(); if D4 mounts the gate on a nested router the path is stripped, is_dav_path is false and everything returns 401 (fail closed, but confusing); use OriginalUri. (b) The Authenticator impl for DavGate turns 429/503 into None, so callers using it get 401 instead; use the middleware for DAV routes. (c) The gate treats /dav and /dav/ as Root, so handlers on those paths must never list or touch data of anyone but DavIdentity.user.

Things I checked and found fine

  • Revoke race: verify re-checks the revoked set after every cache load or insert; forget_installation marks before purging; an expired entry that finds the row gone is removed.
  • Valid-but-wrong-scope is not counted; revoked-in-process hits are charged once per credential.
  • No new user-existence oracle: 404 only after a valid password.
  • Mint response and secret handling unchanged and still outside the action pipeline; guard (JSON content type, Origin match) applies to the mint route.
  • Memory: DAV cache only ever holds live passwords (at most 50 per person); both limiters are capped.

Counts: P0 0, P1 0, P2 2, P3 5. Verdict: ready for merge as scaffolding (nothing is mounted); fix N-1 and N-2 before D4 mounts the gate.

# Independent security review, round 2 Reviewed head e358f3fa (diff 70ccd57c..HEAD and the whole PR vs `origin/dev`). Read-only; nothing built or run. Line numbers are for the PR head. **Verdict: ready to merge as a pre-D4 building block, with the P2s below filed as D4 blockers.** No P0, no P1. All round-1 P1s are fixed and their tests prove them. The new findings are about the fixes' edges (the step-up is weaker than described, the cold-lookup path can still be starved). ## Round-1 items | Item | Result | Notes | |---|---|---| | P1-1 URL-user fails open | **Verified** | `dav_target` (dav.rs:110) never decodes; refuses `%`, `\`, `;`, `?`, `#`, non-ASCII, empty/`.`/`..` segments, non-canonical ids. `check` (dav.rs:217) only lets `Root` or `User(u)==owner` through; everything else is 404. I walked `/dav/%<uuid>`, `//dav`, uppercase, hyphenless, `<uuid>%2f..`, trailing-slash well-known: all refused. `is_dav_path` is wide on purpose (decode + lowercase + trim slashes), so session auth refuses every spelling. The tests (`odd_paths_never_reach_a_handler_as_someone_else` against a catch-all and real-style routes, plus unit `dav_targets_fail_closed`) assert the right things, including that user B's id never appears in the body. | | P1-2 Throttle blocks valid users | **Verified, one hole (N-1)** | Secret is checked first; cached/valid is never throttled (`app_password.rs:223-273`). Per-credential bucket plus "charge the source once per credential per minute" stops a stale device starving its NAT neighbours (test shows a 30-iteration retry loop and a fresh device still passing). Memory: credential and source limiters are each capped at 10,000 entries with LRU eviction (limit.rs:43), a junk flood from one source stops creating credential buckets after 20 because `over` returns before `fail()`. Bounded. See N-1 for the concurrency hole. | | P2-1 Mint step-up | **Partly fixed (N-2)** | Kind check is solid (only passkey login and setup create sessions, both `Browser`; cli/agent get 403). `created_at` is never changed by `touch` (it only moves `last_seen_at`/`expires_at`) and there is no rotation, so session age really is time since sign-in. But see N-2. | | P2-2 Cache TTL / DB errors | **Verified** | 30 s `checked_at` recheck refreshes scopes and revocation from the DB; DB error gives 503 with `Retry-After: 5` and is not charged (app_password.rs:245-251). Revoke from another process test uses a real second connection. Not covered: error on a warm-expired entry (it returns 503, which is fine fail-closed). | | P2-3 CSRF | Follow-up (not re-raised) | | | P3-1 Debug | **Verified** | Manual `Debug` hides `secret`; no other type holding the secret derives Debug; `debug!(?why)` is enum only. | | P3-2 Latin-1 user | **Verified** | Split on first `:` in bytes, only the password must be UTF-8; unit test covers it. | | P3-3 odd paths | **Verified** | See P1-1. | | P3-4 Mint rate limit | **Verified** | `dav_mint_limit` per user (http.rs:455), test passes. Counts failed JSON/scope attempts too; harmless. | | P3-5, P3-6 | Follow-up | | | P3-7 Trigger values | **Verified** | Insert and update triggers allow only `''`, `contacts`, `calendar`, `calendar,contacts`; `encode_scopes` sorts and dedups so it always writes one of those. Test covers bogus values and stripping. | | P3-8 Bidi | **Verified, gap (N-6)** | | ## New findings ### P2 **N-1. A flood still starves a valid user whose password is not cached.** `app_password.rs:228-244`, `dav_overflow` in lib.rs. `try_acquire` on the 8-permit semaphore fails immediately when the permits are busy, and the permit holders are the attacker's own lookups queued behind the single `auth-db` worker (db.rs, one thread, queue of 64). With more than 8 concurrent junk requests from the shared source (the default when `trusted_proxies` is empty: everyone is `"unknown"`), all 8 permits are held nearly all the time, so a cold valid password from that source gets 429. Cold means: first request after a restart or deploy, a new device, or after any entry is evicted. Warm clients are fine (cached entries skip the whole branch), so the failure shows up exactly after restarts, when every device is cold at once. Apple backs off on 429, so it can last a while. The test `an_anonymous_flood_does_not_block_a_valid_user` is sequential; it never holds a permit, so it does not prove the semaphore. Separately: lookups for sources that are not yet over their limit are not capped globally. N sources times 20 distinct junk secrets per minute all go to the one DB worker, which also serves logins, session misses, action revokes and the 30 s rechecks of every warm DAV password. A botnet or an IPv6 /64 can fill that queue (64 slots, `send().await` blocks) and add latency to all of auth. Fix: make the cold-lookup permit apply to every uncached lookup, and use `acquire` with a short timeout (e.g. 250 ms) for sources that are not over, `try_acquire` only for over sources, so a valid password queues behind junk rather than losing to it. Add a global per-second cap on cold lookups. Add a concurrent test: hold all 8 permits with slow junk lookups, then assert a valid cold password gets 200 within a bound. **N-2. The step-up is bypassable through passkey-add, and its description overstates what `created_at` proves.** `http.rs:410-441`, `account.rs` (`account.passkey-add-*`), `setup.rs:387`. Session `created_at` is the time of a passkey sign-in, a setup/recovery/invite link redemption (`setup.rs:387` also makes a `Browser` session from a bearer link plus a new passkey, no assertion), or nothing else. That is fine as far as it goes. But `account.passkey-add-start/finish` needs only an ordinary session of any age. An attacker holding a stolen old cookie adds their own passkey, signs in with it (fresh session, 0 s old), and mints. The 10-minute window then buys nothing against cookie theft; it only stops a cookie that is both stolen and used within the window by someone who does not know about passkey-add. The same goes for `passkey-remove` of the victim's key. Fix: apply the same freshness rule (`fresh_interactive_session`) to `passkey-add-finish` and `passkey-remove`, or better add the real step-up (a new assertion whose ceremony id is consumed by the mint request). Until then, say in the PR and ADR that the step-up defends against a cli/agent token and a very old stolen session only. Also emit a visible event on mint and on passkey-add (already a documented follow-up for new devices, but it is now what makes this bypass detectable). ### P3 **N-3. Malformed passwords are charged to the source every time, not once per credential.** `app_password.rs:208-212`. Only well-formed unknown secrets are deduplicated by `fail()`. A device with a typo'd, truncated or padded password (or the person's login password typed into Apple Contacts) sends junk that is not `ca1_` + 52, and every retry costs the source a full count, so 20 retries a minute push the NAT's source `over`. Effect is mild now (cold valid lookups then need a permit, see N-1), but it brings back a small version of round-1 P1-2. Fix: dedupe those by the hash of the presented string in the credential bucket too, or charge them once per source per 10 s. **N-4. `Limiter::attempt` is O(n) under a global mutex on every call, and DAV now calls it on the hottest unauthenticated path.** `limit.rs:42` (`retain` over up to 10,000 entries). `dav_failure_limit.attempt` runs for every junk header with no DB cost, `has_room`/`count` take the same lock. At tens of thousands of requests per second this burns a core and serializes the async threads on a std mutex. Fix: sweep expired entries at most once per few seconds (or when len is near capacity), not on each call. **N-5. `touch` reinserts the cloned entry with its old `checked_at`.** `app_password.rs:293-295, 333-341`. It can overwrite a fresher entry or resurrect one that a recheck just removed because a revoke happened in another process, extending exposure by at most one more 30 s window. Also `checked_at` uses wall-clock `now_ms`, so a clock step backwards makes `now - checked_at < recheck` stay true for as long as the step. Fix: in `touch` update only `touched_at` if the key is still present; use a monotonic `Instant` for the recheck. **N-6. Name sanitizing misses some invisible/bidi characters.** `session.rs:347`. Not covered: U+061C (Arabic letter mark, a bidi control), U+00AD, U+034F, U+180E, U+3164, variation selectors, tag characters U+E0000-E007F. Use `char::is_control` plus a `Default_Ignorable_Code_Point` check, and reuse it for passkey labels (`passkeys::clean_label` does not strip bidi at all). **N-7. DB migration edited in place.** `db.rs` SCHEMA_V8. Any dev database already at `user_version = 8` from the first commit keeps the old, looser triggers and never gets the tightened ones. Only matters for machines that ran the earlier branch head; note it, or bump to a v9 that drops and recreates the triggers. **N-8. D4 mounting notes (not bugs today).** (a) `check` reads `parts.uri.path()`; if D4 mounts the gate on a `nest`ed router the path is stripped, `is_dav_path` is false and everything returns 401 (fail closed, but confusing); use `OriginalUri`. (b) The `Authenticator` impl for `DavGate` turns 429/503 into `None`, so callers using it get 401 instead; use the middleware for DAV routes. (c) The gate treats `/dav` and `/dav/` as `Root`, so handlers on those paths must never list or touch data of anyone but `DavIdentity.user`. ## Things I checked and found fine - Revoke race: `verify` re-checks the revoked set after every cache load or insert; `forget_installation` marks before purging; an expired entry that finds the row gone is removed. - Valid-but-wrong-scope is not counted; revoked-in-process hits are charged once per credential. - No new user-existence oracle: 404 only after a valid password. - Mint response and secret handling unchanged and still outside the action pipeline; `guard` (JSON content type, Origin match) applies to the mint route. - Memory: DAV cache only ever holds live passwords (at most 50 per person); both limiters are capped. **Counts: P0 0, P1 0, P2 2, P3 5. Verdict: ready for merge as scaffolding (nothing is mounted); fix N-1 and N-2 before D4 mounts the gate.**
kayg commented 2026-10-31 16:13:41 +00:00
Author
Owner
Copy link

Review round 2: resumed fixes and final status

Head 551b62c4da, pushed to feat/dav-app-passwords. Resumed from 40cfc587; kept the preceding agent's assertion timestamp, v9 migration, monotonic recheck and safe touch changes. This completes the requested fixes without another review round. Never merged.

Finding Status Implementation / evidence
N-1 concurrent cold-password starvation / anonymous DB load Fixed with bounded overload policy Every cold lookup joins the existing FIFO 250 ms permit wait, regardless of source. Eight global permits are each held at least 8 ms, including on cancellation: burst 8, sustained at most 1,000 cold reads/s across all sources. Warm reads and TTL rechecks bypass the cold queue. cold_lookups_queue_globally_behind_slow_junk blocks the DB worker, fills all eight permits with junk, polls a cold valid lookup while they are held, then proves it succeeds within 250 ms after release. Independent-source junk also exercises global pacing. a_cold_valid_password_gets_through_a_concurrent_flood runs 200 junk HTTP requests plus a valid uncached password. Both pass.
N-2 passkey-add step-up bypass Fixed; strengthened tests Retained assertion_at, set only by passkey login. Mint, add-finish and remove require a recent browser/native assertion. an_old_session_cannot_add_or_remove_passkeys now supplies a real registration ceremony and credential. Setup, recovery and invite sessions explicitly fail minting; recovery/invite also fail add-finish. Existing fresh add/remove tests pass. ADR 0064 states the protection and the remaining fresh-cookie risk.
N-3 malformed password repeatedly charges source Fixed Retained hash deduplication and made its first-attempt decision atomic. the_same_malformed_password_is_charged_once and concurrent_retries_have_only_one_first_attempt pass.
N-4 limiter scans 10,000 entries under mutex per call Fixed Retained five-second expiry sweeps. Replaced capacity scan with an ordered LRU index: reads O(1), recording/eviction O(log n), bounded at 10,000 entries. Existing capacity and source-churn tests pass.
N-5 touch overwrites checked_at / resurrects removed row Fixed in preceding commits; verified Monotonic Instant for checked_at; touch changes only touched_at on an existing entry. New touch_preserves_rechecks_and_never_resurrects_a_removed_entry passes.
N-6 invisible/bidi names Fixed in preceding commits; verified Shared sanitizer used for installation and passkey labels. Added explicit coverage for U+061C and every Bidi_Control character, plus passkey-label coverage.
N-7 existing v8 databases keep loose triggers Fixed in preceding commits; verified v9 recreates the triggers and adds assertion_at. Migration gates pass.
N-8 Authenticator loses 429/503 Fixed Authenticator returns Result<Option<Identity>, Response>; server, attachment and backdrop extractors propagate rejections. DAV middleware and trait share response construction. authenticator_keeps_dav_rejection_status_and_headers proves 429/503, Retry-After and no-store. OriginalUri handling from the preceding commits is retained.

Gates

All passed on calternal-dev-01-compute02, Nix dev shell, inside one ~/bin/slot bash ~/dav782-run-gates.sh job (~/dav782-all-gates.log, ending DAV782_GATES_PASS):

  • cargo test -j4 -p calternal-auth -p calternal-server --no-fail-fast — green, including all 29 app-password integration tests and 40 auth unit tests. Existing perf-only tests remain ignored as intended.
  • cargo test -j4 -p calternal contract — green (contract_matches_the_server).
  • cargo clippy -j4 -p calternal-auth -p calternal-server -p calternal-attachments -p calternal-backdrops --all-targets -- -D warnings — clean.
  • cargo fmt --all -- --check — clean; rechecked after comment/doc clarifications, also git diff --check.

The Nix flake files on the VM were excluded from the commit.

Decisions and remaining limits

Chose the existing short FIFO queue plus a small global cold-read rate bound, preserving the token format and hash-only storage. A finite concurrent burst now queues correctly; sustained overload or a sufficiently slow DB can still exceed 250 ms and return 429. The bound is deliberate and documented, rather than promising cold-client availability under arbitrary load.

Freshness protects against CLI/agent tokens and old stolen sessions; a stolen session within the ten-minute assertion window can still mint/add keys. Setup/recovery/invite sessions require a real passkey sign-in. Passkey-add emits its existing account event and mint emits the Installation event; a prominent device notification and separate step-up prompt remain UI work.

D4 still has to mount the gate and keep handlers scoped to DavIdentity.user, including root paths. D6 browser-origin/CSRF policy, IPv6 /64 bucketing, trusted-proxy warning and plaintext-proxy checks remain the already documented follow-ups. No DAV routes were mounted here.

# Review round 2: resumed fixes and final status Head **551b62c4da4cc1cbb4f7e101bdd05ef6e3c9c4b8**, pushed to `feat/dav-app-passwords`. Resumed from 40cfc587; kept the preceding agent's assertion timestamp, v9 migration, monotonic recheck and safe touch changes. This completes the requested fixes without another review round. Never merged. | Finding | Status | Implementation / evidence | |---|---|---| | N-1 concurrent cold-password starvation / anonymous DB load | Fixed with bounded overload policy | Every cold lookup joins the existing FIFO 250 ms permit wait, regardless of source. Eight global permits are each held at least 8 ms, including on cancellation: burst 8, sustained at most 1,000 cold reads/s across all sources. Warm reads and TTL rechecks bypass the cold queue. `cold_lookups_queue_globally_behind_slow_junk` blocks the DB worker, fills all eight permits with junk, polls a cold valid lookup while they are held, then proves it succeeds within 250 ms after release. Independent-source junk also exercises global pacing. `a_cold_valid_password_gets_through_a_concurrent_flood` runs 200 junk HTTP requests plus a valid uncached password. Both pass. | | N-2 passkey-add step-up bypass | Fixed; strengthened tests | Retained `assertion_at`, set only by passkey login. Mint, add-finish and remove require a recent browser/native assertion. `an_old_session_cannot_add_or_remove_passkeys` now supplies a real registration ceremony and credential. Setup, recovery and invite sessions explicitly fail minting; recovery/invite also fail add-finish. Existing fresh add/remove tests pass. ADR 0064 states the protection and the remaining fresh-cookie risk. | | N-3 malformed password repeatedly charges source | Fixed | Retained hash deduplication and made its first-attempt decision atomic. `the_same_malformed_password_is_charged_once` and `concurrent_retries_have_only_one_first_attempt` pass. | | N-4 limiter scans 10,000 entries under mutex per call | Fixed | Retained five-second expiry sweeps. Replaced capacity scan with an ordered LRU index: reads O(1), recording/eviction O(log n), bounded at 10,000 entries. Existing capacity and source-churn tests pass. | | N-5 touch overwrites checked_at / resurrects removed row | Fixed in preceding commits; verified | Monotonic Instant for checked_at; touch changes only touched_at on an existing entry. New `touch_preserves_rechecks_and_never_resurrects_a_removed_entry` passes. | | N-6 invisible/bidi names | Fixed in preceding commits; verified | Shared sanitizer used for installation and passkey labels. Added explicit coverage for U+061C and every Bidi_Control character, plus passkey-label coverage. | | N-7 existing v8 databases keep loose triggers | Fixed in preceding commits; verified | v9 recreates the triggers and adds assertion_at. Migration gates pass. | | N-8 Authenticator loses 429/503 | Fixed | Authenticator returns `Result<Option<Identity>, Response>`; server, attachment and backdrop extractors propagate rejections. DAV middleware and trait share response construction. `authenticator_keeps_dav_rejection_status_and_headers` proves 429/503, Retry-After and no-store. OriginalUri handling from the preceding commits is retained. | ## Gates All passed on **calternal-dev-01-compute02**, Nix dev shell, inside one `~/bin/slot bash ~/dav782-run-gates.sh` job (`~/dav782-all-gates.log`, ending `DAV782_GATES_PASS`): - `cargo test -j4 -p calternal-auth -p calternal-server --no-fail-fast` — green, including all 29 app-password integration tests and 40 auth unit tests. Existing perf-only tests remain ignored as intended. - `cargo test -j4 -p calternal contract` — green (`contract_matches_the_server`). - `cargo clippy -j4 -p calternal-auth -p calternal-server -p calternal-attachments -p calternal-backdrops --all-targets -- -D warnings` — clean. - `cargo fmt --all -- --check` — clean; rechecked after comment/doc clarifications, also `git diff --check`. The Nix flake files on the VM were excluded from the commit. ## Decisions and remaining limits Chose the existing short FIFO queue plus a small global cold-read rate bound, preserving the token format and hash-only storage. A finite concurrent burst now queues correctly; sustained overload or a sufficiently slow DB can still exceed 250 ms and return 429. The bound is deliberate and documented, rather than promising cold-client availability under arbitrary load. Freshness protects against CLI/agent tokens and old stolen sessions; a stolen session within the ten-minute assertion window can still mint/add keys. Setup/recovery/invite sessions require a real passkey sign-in. Passkey-add emits its existing account event and mint emits the Installation event; a prominent device notification and separate step-up prompt remain UI work. D4 still has to mount the gate and keep handlers scoped to DavIdentity.user, including root paths. D6 browser-origin/CSRF policy, IPv6 /64 bucketing, trusted-proxy warning and plaintext-proxy checks remain the already documented follow-ups. No DAV routes were mounted here.
kayg commented 2026-10-31 16:18:18 +00:00
Author
Owner
Copy link

Independent security review, round 3 (final)

Reviewed 551b62c4da. Read the PR body and all five preceding review/status comments, including #70764; reviewed e358f3fa..40cfc587, 40cfc587..551b62c4, and the whole PR against origin/dev (2f304154). Defensive review of our own code. No code changed or pushed.

Verdict: small fix. New findings: P0 0, P1 0, P2 1, P3 0. Fix R3-1 below before mounting this gate. This is the final review round; no fourth round requested. The rule requiring “stop patching, simplify the design” for new P0/P1 findings is not triggered.

Round-2 items

Item Result Evidence at reviewed head
N-1 cold-lookup starvation / global DB load Not fixed completely; bounded-overload policy verified Every cold lookup queues FIFO for 250 ms; eight permits held for at least 8 ms bound cold DB submissions to an eight-request burst and at most 1,000/s sustained. ColdPermit::drop retains the permit through the minimum lifetime even on cancellation. Warm entries and TTL rechecks bypass this queue. Finite-burst tests exercise actual occupied permits. Sustained overload can still deny a valid uncached device indefinitely, and pending-waiter memory lacks a hard cap (R3-1).
N-2 passkey-add bypass / false freshness Verified passkey.rs:195 sets assertion time only after successful passkey login; setup.rs:387 passes false for setup/recovery/invite redemption. v9 leaves existing sessions with NULL assertion time. session.rs:353 checks browser/native kind and assertion freshness; mint, add-finish and remove all call it. The old-session add test now presents a real registration response. Setup/recovery/invite rejection tests cover the intended distinction. A stolen fresh cookie retains the explicitly documented ten-minute authority.
N-3 malformed-password repeated charging Verified app_password.rs:229-234 hashes malformed passwords and uses the same deduplication; fail uses atomic first_attempt. Concurrent retries cannot all charge the source.
N-4 limiter scan under mutex Verified limit.rs:63 sweeps at most every five seconds. HashMap reads are O(1); the ordered LRU index makes ordinary recording/eviction O(log n). Both indexes stay at 10,000 entries per limiter.
N-5 touch / clock recheck Verified DavLive.checked_at is monotonic; touch only updates an existing entry's touched time. It cannot overwrite checked_at or resurrect a removed entry.
N-6 invisible/bidi names Verified Shared hidden_char sanitizer covers the reported characters and all Bidi_Control characters, and is used for installation names and passkey labels.
N-7 edited v8 migration Verified v9 drops/recreates the scope triggers and adds nullable assertion_at, including for databases already at v8. Existing unknown scopes grant no extra scope.
N-8 mounting / Authenticator statuses Verified DAV checks OriginalUri when present. Both middleware and Authenticator use the same rejection response. Server, attachment and backdrop extractors propagate Err(Response), preserving 401/404/429/503, challenge, Retry-After and no-store. Root handlers must still use DavIdentity.user when D4 mounts them.

New finding

R3-1 — P2: the new FIFO wait bounds time, but not the number of pending cold requests.

Location: crates/auth/src/app_password.rs:262, with the eight-permit setup at crates/auth/src/lib.rs:133.

timeout(COLD_WAIT, semaphore.acquire_owned()) permits any number of request futures to join the semaphore queue. Eight permits bound active cold work, not waiters. Each waiter retains its request state and timer until admission, cancellation or timeout. No outer request-concurrency/admission cap is present in the server wiring. Fresh plausible junk credentials can therefore create arbitrarily many pending cold requests in a burst, including after their source is over its failure budget: source exhaustion changes the response after the lookup, not admission.

The limiter maps are bounded; queue memory instead scales with concurrent arrivals (roughly arrivals during the preceding 250 ms under overload). This is an allocation/availability amplification introduced by replacing immediate refusal with waiting. It is a static finding, not a measured OOM claim; no DAV routes are mounted yet, so it is P2.

Fix: add a small, separate pending-cold admission budget acquired with try_acquire_owned before joining the FIFO wait. Hold that admission permit through the cold operation; return the existing 429/no-store/Retry-After response immediately when full. Keep the existing eight worker permits and cancellation-safe pacing; warm reads must bypass admission. Test with a blocked DB worker and more requests than the pending cap: excess requests must fail immediately, retained admissions must never exceed the cap, and cancellation/timeout must return admission permits. A timeout alone does not establish a memory bound.

Explicit availability and memory limits

Yes, a sustained flood can keep a valid uncached device at 429 indefinitely. FIFO fairness only helps requests whose position becomes serviceable within 250 ms; every timed-out retry loses its place. Roughly a few hundred queued requests at the maximum service rate already consume that wait budget. The 32-junk blocked-worker and 200-request HTTP tests prove finite-burst behavior, not sustained-load availability. #70764 and the ADR acknowledge this residual policy; I am not reporting it again as a new finding.

The eight cold worker permits and 10,000-entry limiter indexes have hard bounds. Pending cold waiters do not. The DAV cache also has no global cardinality cap: it stores successfully looked-up credentials, with TTL rechecking on use, rather than being an anonymous negative cache. The 50-live-password cap is per user, not a global process-memory limit.

Verification and scope

Read the existing dev-01 gate script and ~/dav782-all-gates.log; the VM worktree is at 551b62c4. The log confirms the blocked-worker, concurrent-flood, real passkey-add, setup/recovery/invite and Authenticator tests passed, with 40 auth unit tests and 29 app-password integration tests, and ends DAV782_GATES_PASS. Recorded commands:

  • cargo test -j4 -p calternal-auth -p calternal-server --no-fail-fast
  • cargo test -j4 -p calternal contract
  • cargo clippy -j4 -p calternal-auth -p calternal-server -p calternal-attachments -p calternal-backdrops --all-targets -- -D warnings
  • cargo fmt --all -- --check

I did not rerun builds/tests. My local git diff --check origin/dev...551b62c4 passed. No new secret disclosure, cross-user authorization bypass or scope widening found. Existing D6 CSRF, IPv6 /64, plaintext HTTP, trusted-proxy warning and new-device-notification follow-ups were not re-raised.

Required small fix: R3-1 only. New P0/P1: none.

# Independent security review, round 3 (final) Reviewed **551b62c4da4cc1cbb4f7e101bdd05ef6e3c9c4b8**. Read the PR body and all five preceding review/status comments, including #70764; reviewed `e358f3fa..40cfc587`, `40cfc587..551b62c4`, and the whole PR against `origin/dev` (2f304154). Defensive review of our own code. No code changed or pushed. **Verdict: small fix. New findings: P0 0, P1 0, P2 1, P3 0.** Fix R3-1 below before mounting this gate. This is the final review round; no fourth round requested. The rule requiring “stop patching, simplify the design” for new P0/P1 findings is not triggered. ## Round-2 items | Item | Result | Evidence at reviewed head | |---|---|---| | N-1 cold-lookup starvation / global DB load | **Not fixed completely; bounded-overload policy verified** | Every cold lookup queues FIFO for 250 ms; eight permits held for at least 8 ms bound cold DB submissions to an eight-request burst and at most 1,000/s sustained. `ColdPermit::drop` retains the permit through the minimum lifetime even on cancellation. Warm entries and TTL rechecks bypass this queue. Finite-burst tests exercise actual occupied permits. Sustained overload can still deny a valid uncached device indefinitely, and pending-waiter memory lacks a hard cap (R3-1). | | N-2 passkey-add bypass / false freshness | **Verified** | `passkey.rs:195` sets assertion time only after successful passkey login; `setup.rs:387` passes false for setup/recovery/invite redemption. v9 leaves existing sessions with NULL assertion time. `session.rs:353` checks browser/native kind and assertion freshness; mint, add-finish and remove all call it. The old-session add test now presents a real registration response. Setup/recovery/invite rejection tests cover the intended distinction. A stolen fresh cookie retains the explicitly documented ten-minute authority. | | N-3 malformed-password repeated charging | **Verified** | `app_password.rs:229-234` hashes malformed passwords and uses the same deduplication; `fail` uses atomic `first_attempt`. Concurrent retries cannot all charge the source. | | N-4 limiter scan under mutex | **Verified** | `limit.rs:63` sweeps at most every five seconds. HashMap reads are O(1); the ordered LRU index makes ordinary recording/eviction O(log n). Both indexes stay at 10,000 entries per limiter. | | N-5 touch / clock recheck | **Verified** | `DavLive.checked_at` is monotonic; `touch` only updates an existing entry's touched time. It cannot overwrite checked_at or resurrect a removed entry. | | N-6 invisible/bidi names | **Verified** | Shared `hidden_char` sanitizer covers the reported characters and all Bidi_Control characters, and is used for installation names and passkey labels. | | N-7 edited v8 migration | **Verified** | v9 drops/recreates the scope triggers and adds nullable assertion_at, including for databases already at v8. Existing unknown scopes grant no extra scope. | | N-8 mounting / Authenticator statuses | **Verified** | DAV checks OriginalUri when present. Both middleware and Authenticator use the same rejection response. Server, attachment and backdrop extractors propagate Err(Response), preserving 401/404/429/503, challenge, Retry-After and no-store. Root handlers must still use DavIdentity.user when D4 mounts them. | ## New finding **R3-1 — P2: the new FIFO wait bounds time, but not the number of pending cold requests.** Location: [crates/auth/src/app_password.rs:262](https://git.kayg.org/kayg/calternal/src/commit/551b62c4da4cc1cbb4f7e101bdd05ef6e3c9c4b8/crates/auth/src/app_password.rs#L262), with the eight-permit setup at `crates/auth/src/lib.rs:133`. `timeout(COLD_WAIT, semaphore.acquire_owned())` permits any number of request futures to join the semaphore queue. Eight permits bound active cold work, not waiters. Each waiter retains its request state and timer until admission, cancellation or timeout. No outer request-concurrency/admission cap is present in the server wiring. Fresh plausible junk credentials can therefore create arbitrarily many pending cold requests in a burst, including after their source is over its failure budget: source exhaustion changes the response after the lookup, not admission. The limiter maps are bounded; queue memory instead scales with concurrent arrivals (roughly arrivals during the preceding 250 ms under overload). This is an allocation/availability amplification introduced by replacing immediate refusal with waiting. It is a static finding, not a measured OOM claim; no DAV routes are mounted yet, so it is P2. **Fix:** add a small, separate pending-cold admission budget acquired with `try_acquire_owned` before joining the FIFO wait. Hold that admission permit through the cold operation; return the existing 429/no-store/Retry-After response immediately when full. Keep the existing eight worker permits and cancellation-safe pacing; warm reads must bypass admission. Test with a blocked DB worker and more requests than the pending cap: excess requests must fail immediately, retained admissions must never exceed the cap, and cancellation/timeout must return admission permits. A timeout alone does not establish a memory bound. ## Explicit availability and memory limits Yes, a sustained flood can keep a valid uncached device at 429 indefinitely. FIFO fairness only helps requests whose position becomes serviceable within 250 ms; every timed-out retry loses its place. Roughly a few hundred queued requests at the maximum service rate already consume that wait budget. The 32-junk blocked-worker and 200-request HTTP tests prove finite-burst behavior, not sustained-load availability. #70764 and the ADR acknowledge this residual policy; I am not reporting it again as a new finding. The eight cold worker permits and 10,000-entry limiter indexes have hard bounds. Pending cold waiters do not. The DAV cache also has no global cardinality cap: it stores successfully looked-up credentials, with TTL rechecking on use, rather than being an anonymous negative cache. The 50-live-password cap is per user, not a global process-memory limit. ## Verification and scope Read the existing dev-01 gate script and `~/dav782-all-gates.log`; the VM worktree is at 551b62c4. The log confirms the blocked-worker, concurrent-flood, real passkey-add, setup/recovery/invite and Authenticator tests passed, with 40 auth unit tests and 29 app-password integration tests, and ends `DAV782_GATES_PASS`. Recorded commands: - `cargo test -j4 -p calternal-auth -p calternal-server --no-fail-fast` - `cargo test -j4 -p calternal contract` - `cargo clippy -j4 -p calternal-auth -p calternal-server -p calternal-attachments -p calternal-backdrops --all-targets -- -D warnings` - `cargo fmt --all -- --check` I did not rerun builds/tests. My local `git diff --check origin/dev...551b62c4` passed. No new secret disclosure, cross-user authorization bypass or scope widening found. Existing D6 CSRF, IPv6 /64, plaintext HTTP, trusted-proxy warning and new-device-notification follow-ups were not re-raised. **Required small fix: R3-1 only. New P0/P1: none.**
kayg commented 2026-10-31 16:32:06 +00:00
Author
Owner
Copy link

R3-1 fixed: cap pending cold DAV lookups

Pushed a59f782e43 to feat/dav-app-passwords. Only three auth files changed; never merged.

AuthConfig::dav_max_pending_cold defaults to 256, counting both active and waiting cold lookups. A separate semaphore is acquired with try_acquire_owned before allocating a FIFO waiter or timeout. Full admission returns the existing 429 + Retry-After: 60 + Cache-Control: no-store immediately. Admission is held through the database read and returned on completion, cancellation or timeout. Cached passwords, including TTL rechecks, bypass admission. The existing eight workers, 8 ms pacing and 250 ms fair wait are unchanged.

Added cold_admission_is_bounded_and_cached_passwords_bypass_it: with a custom cap of 16 and the DB worker blocked, fill all 16 admissions, then concurrently flood 64 excess requests. Each excess gate check must finish on its first poll with 429 and the retry/no-store headers; retained admissions remain at the cap. A cached valid password succeeds while full. Aborting active/queued requests and timing out a queued request both restore admission capacity. The regression failed before wiring admission into verification and passed after the fix.

All gates passed on calternal-dev-01-compute02, through the Nix dev shell and ~/bin/slot, log ~/dav782-r3-gates.log, ending DAV782_R3_GATES_PASS:

  • cargo test -j4 -p calternal-auth -p calternal-server --no-fail-fast
  • cargo test -j4 -p calternal contract
  • cargo clippy -j4 -p calternal-auth --all-targets -- -D warnings (the only touched crate)
  • cargo fmt --all -- --check

Also ran the new regression separately, checked git diff --check, and verified the three tested VM files match the pushed files by SHA-256. Existing perf-only tests remain ignored. VM Nix flake files are excluded from the commit.

Decision: keep the admission mechanism separate and non-waiting, with a default cap large enough for the existing finite-burst tests. Sustained overload can still refuse an uncached valid device; cached valid passwords bypass the cap. No additional review round or unrelated changes.

## R3-1 fixed: cap pending cold DAV lookups Pushed **a59f782e43f85566d70981f3e89b4b016e3ef6d8** to `feat/dav-app-passwords`. Only three auth files changed; never merged. `AuthConfig::dav_max_pending_cold` defaults to **256**, counting both active and waiting cold lookups. A separate semaphore is acquired with `try_acquire_owned` before allocating a FIFO waiter or timeout. Full admission returns the existing **429 + Retry-After: 60 + Cache-Control: no-store** immediately. Admission is held through the database read and returned on completion, cancellation or timeout. Cached passwords, including TTL rechecks, bypass admission. The existing eight workers, 8 ms pacing and 250 ms fair wait are unchanged. Added `cold_admission_is_bounded_and_cached_passwords_bypass_it`: with a custom cap of 16 and the DB worker blocked, fill all 16 admissions, then concurrently flood 64 excess requests. Each excess gate check must finish on its first poll with 429 and the retry/no-store headers; retained admissions remain at the cap. A cached valid password succeeds while full. Aborting active/queued requests and timing out a queued request both restore admission capacity. The regression failed before wiring admission into verification and passed after the fix. All gates passed on **calternal-dev-01-compute02**, through the Nix dev shell and `~/bin/slot`, log `~/dav782-r3-gates.log`, ending `DAV782_R3_GATES_PASS`: - `cargo test -j4 -p calternal-auth -p calternal-server --no-fail-fast` - `cargo test -j4 -p calternal contract` - `cargo clippy -j4 -p calternal-auth --all-targets -- -D warnings` (the only touched crate) - `cargo fmt --all -- --check` Also ran the new regression separately, checked `git diff --check`, and verified the three tested VM files match the pushed files by SHA-256. Existing perf-only tests remain ignored. VM Nix flake files are excluded from the commit. Decision: keep the admission mechanism separate and non-waiting, with a default cap large enough for the existing finite-burst tests. Sustained overload can still refuse an uncached valid device; cached valid passwords bypass the cap. No additional review round or unrelated changes.
kayg commented 2026-10-31 20:19:35 +00:00
Author
Owner
Copy link

BM4 passkey regression implementation plan

Setup currently verifies registration but creates a session without assertion_at. Record that proof after registration. Add an additive v10 registration_session flag (one integer per installation, default false for existing rows) so app-password minting keeps its existing sign-in-only rule. Passkey management accepts recent registration or sign-in proof, still restricted to browser/native sessions. Check add-start and add-finish; return passkey_reverification_required for stale/missing proof. The UI offers a passkey check and retries the requested operation once; cancellation leaves a visible message. Re-check uses login without signing out first and verifies that the resulting user is unchanged.

All freshness checks remain one indexed installation lookup, O(1), with no new cache.

Failure → effect → detection → recovery → test:

  • Setup registration missing freshness → second key refused → setup regression → record proof in redemption transaction → Rust + Chromium ceremony.
  • Missing/stale proof → add/remove refused → explicit 403 code → passkey check then one retry → Rust + browser prompt/cancel/retry.
  • Registration session mints app password → policy changes → registration_session flag → require sign-in assertion → existing setup/recovery/invite mint tests.
  • Older database → new flag missing → v10 migration → default false, no old timestamps invented → migration suite.

Gates: Rust auth/server and core error catalogue tests, contract check, format check; targeted passkeys ceremony in Chromium and passkeys UI in Chromium/WebKit on dev-01. WebKit has no virtual passkey authenticator, so the real ceremony stays Chromium-only. Rebuild BM4 by merging the updated feature branch, push and report. No full train run and no merge into dev.

BM4 passkey regression implementation plan Setup currently verifies registration but creates a session without assertion_at. Record that proof after registration. Add an additive v10 registration_session flag (one integer per installation, default false for existing rows) so app-password minting keeps its existing sign-in-only rule. Passkey management accepts recent registration or sign-in proof, still restricted to browser/native sessions. Check add-start and add-finish; return passkey_reverification_required for stale/missing proof. The UI offers a passkey check and retries the requested operation once; cancellation leaves a visible message. Re-check uses login without signing out first and verifies that the resulting user is unchanged. All freshness checks remain one indexed installation lookup, O(1), with no new cache. Failure → effect → detection → recovery → test: - Setup registration missing freshness → second key refused → setup regression → record proof in redemption transaction → Rust + Chromium ceremony. - Missing/stale proof → add/remove refused → explicit 403 code → passkey check then one retry → Rust + browser prompt/cancel/retry. - Registration session mints app password → policy changes → registration_session flag → require sign-in assertion → existing setup/recovery/invite mint tests. - Older database → new flag missing → v10 migration → default false, no old timestamps invented → migration suite. Gates: Rust auth/server and core error catalogue tests, contract check, format check; targeted passkeys ceremony in Chromium and passkeys UI in Chromium/WebKit on dev-01. WebKit has no virtual passkey authenticator, so the real ceremony stays Chromium-only. Rebuild BM4 by merging the updated feature branch, push and report. No full train run and no merge into dev.
kayg commented 2026-10-31 20:34:06 +00:00
Author
Owner
Copy link

BM4 passkey regression fixed and targeted gates passed

Feature #782: 850e604774 (feat/dav-app-passwords). Rebuilt train #810: 29175e3782 (train/2026-11-01-bm4), including the updated feature branch. Both pushed. No merge into dev and no full train run started.

Cause and behavior

Confirmed by reading setup redemption and reproducing e2e/passkeys.spec.ts on the original BM4 build: after Add a passkey, the list stayed at one row. Setup verified a registration but created its session with no assertion_at. The new Rust setup-add regression also failed before the fix.

A completed registration now records assertion_at in the redemption transaction. Add-start, add-finish and remove require recent browser/native passkey proof. Stale/missing proof returns a distinct 403 passkey_reverification_required with “Verify with your passkey to continue.” Settings offers a passkey check, then retries the operation. Cancellation keeps the session and offers another try. Re-check requires the current account's passkey; another account's key cannot replace the session.

App-password minting keeps its sign-in-only rule. Additive auth.db v10 records registration_session so registration proof alone cannot mint. Existing v9 proof timestamps are preserved and old sessions are not assigned new proof. Recovery/invite redemption also performs registration in this implementation, so its completed ceremony counts for passkey management; sessions with no ceremony do not. Rust tests cover setup, recovery, invite, missing/stale proof, sign-in, mint restrictions, current-account re-check, and migration.

Verification

On calternal-dev-01-compute02, Nix dev shell, through ~/bin/slot bash ~/bm4-passkey-train-gates.sh, worktree ~/Developer/Worktrees/bm4-passkey-web. Log: ~/bm4-passkey-train.log, ending BM4_PASSKEY_TRAIN_EXIT=0. VM worktree is clean at the train head above. Rust sources were unchanged by the last merge, which only switched the e2e timestamp helper to Nix's sqlite3.

All passed:

cargo fmt --all -- --check
cargo test -j4 -p calternal-core -p calternal-auth -p calternal-server --no-fail-fast
cargo test -j4 -p calternal contract
cargo clippy -j4 -p calternal-core -p calternal-auth -p calternal-server --all-targets -- -D warnings
cargo build -j4 -p calternal
bun install --frozen-lockfile
# from apps/web:
bun run check
CALTERNAL_BIN=/home/claude/Developer/Worktrees/bm4-gates/target/debug/calternal bunx playwright test e2e/passkeys.spec.ts e2e/passkeys-ui.spec.ts --project=passkeys --project=chromium-1440 --project=webkit-1440 --workers=1

Browser result: 5 passed, zero retries. Chromium exercised real setup/add/rename/remove, stale removal with cancellation and retry, and stale add with verification followed by successful registration. Chromium/WebKit UI tests cover the stale-session prompt and a failed re-check with another attempt. WebKit has no virtual WebAuthn authenticator, so the real ceremony remains in the Chromium passkeys project. Svelte check: 0 errors, 14 warnings in 8 existing files; clippy clean.

One earlier feature-suite run failed counter_concurrent_mutations_drift_and_restart (16577 vs 16440). The final rebuilt-train suite passed it. No quota code changed. Existing ignored performance tests remain ignored. The full train rerun, including the previously reported WebKit swipe case, is left to the orchestrator.

## BM4 passkey regression fixed and targeted gates passed Feature #782: **850e6047740974a7c0b31ce874da1bf14c8f5807** (`feat/dav-app-passwords`). Rebuilt train #810: **29175e37829bf54038d9c8660252044904abdfce** (`train/2026-11-01-bm4`), including the updated feature branch. Both pushed. No merge into dev and no full train run started. ### Cause and behavior Confirmed by reading setup redemption and reproducing `e2e/passkeys.spec.ts` on the original BM4 build: after Add a passkey, the list stayed at one row. Setup verified a registration but created its session with no assertion_at. The new Rust setup-add regression also failed before the fix. A completed registration now records assertion_at in the redemption transaction. Add-start, add-finish and remove require recent browser/native passkey proof. Stale/missing proof returns a distinct 403 `passkey_reverification_required` with “Verify with your passkey to continue.” Settings offers a passkey check, then retries the operation. Cancellation keeps the session and offers another try. Re-check requires the current account's passkey; another account's key cannot replace the session. App-password minting keeps its sign-in-only rule. Additive auth.db v10 records registration_session so registration proof alone cannot mint. Existing v9 proof timestamps are preserved and old sessions are not assigned new proof. Recovery/invite redemption also performs registration in this implementation, so its completed ceremony counts for passkey management; sessions with no ceremony do not. Rust tests cover setup, recovery, invite, missing/stale proof, sign-in, mint restrictions, current-account re-check, and migration. ### Verification On **calternal-dev-01-compute02**, Nix dev shell, through `~/bin/slot bash ~/bm4-passkey-train-gates.sh`, worktree `~/Developer/Worktrees/bm4-passkey-web`. Log: **~/bm4-passkey-train.log**, ending **BM4_PASSKEY_TRAIN_EXIT=0**. VM worktree is clean at the train head above. Rust sources were unchanged by the last merge, which only switched the e2e timestamp helper to Nix's sqlite3. All passed: ```sh cargo fmt --all -- --check cargo test -j4 -p calternal-core -p calternal-auth -p calternal-server --no-fail-fast cargo test -j4 -p calternal contract cargo clippy -j4 -p calternal-core -p calternal-auth -p calternal-server --all-targets -- -D warnings cargo build -j4 -p calternal bun install --frozen-lockfile # from apps/web: bun run check CALTERNAL_BIN=/home/claude/Developer/Worktrees/bm4-gates/target/debug/calternal bunx playwright test e2e/passkeys.spec.ts e2e/passkeys-ui.spec.ts --project=passkeys --project=chromium-1440 --project=webkit-1440 --workers=1 ``` Browser result: **5 passed**, zero retries. Chromium exercised real setup/add/rename/remove, stale removal with cancellation and retry, and stale add with verification followed by successful registration. Chromium/WebKit UI tests cover the stale-session prompt and a failed re-check with another attempt. WebKit has no virtual WebAuthn authenticator, so the real ceremony remains in the Chromium `passkeys` project. Svelte check: 0 errors, 14 warnings in 8 existing files; clippy clean. One earlier feature-suite run failed `counter_concurrent_mutations_drift_and_restart` (16577 vs 16440). The final rebuilt-train suite passed it. No quota code changed. Existing ignored performance tests remain ignored. The full train rerun, including the previously reported WebKit swipe case, is left to the orchestrator.
kayg commented 2026-11-04 13:04:40 +00:00
Author
Owner
Copy link

Closing: head 850e6047 is already in dev (reached via 29175e37); nothing left to merge. Branch kept.

Closing: head 850e6047 is already in dev (reached via 29175e37); nothing left to merge. Branch kept.
kayg closed this pull request 2026-11-04 13:04:40 +00:00
Some checks failed
CI / server (pull_request) Has been cancelled
Details
CI / rust (pull_request) Has been cancelled
Details
CI / web (pull_request) Has been cancelled
Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
backend
Rust server

  
design
Mockups, UI and TASTE

  
docs
ADRs, CONTEXT, TASTE

  
idea
Parked idea, never worked until promoted

  
infra
Deploy, CI, VMs

  
perf
Performance and budgets

  
reliability
Data integrity, corruption traps

  
web
Web client

No labels
backend
design
docs
idea
infra
perf
reliability
web
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
kayg/calternal!782
No description provided.