108 lines
15 KiB
Markdown
108 lines
15 KiB
Markdown
# AuthN, Session & Token Lifecycle
|
|
|
|
Audit date: 2026-06-11
|
|
Scope: login flow, password hashing, session token lifecycle, bootstrap, token transport.
|
|
|
|
Methodology: read `crates/manager-core/src/{auth,auth_api,auth_middleware,auth_bootstrap,admin_session_repo,admin_user_repo,admin_users_api}.rs`, `crates/picloud/src/main.rs` reset-password CLI, `crates/manager-core/migrations/0004_admin_auth.sql`, `dashboard/src/lib/{auth,api}.ts`. Cross-checked against `AUDIT.md` to avoid duplicating remediated findings. `F-S-006`, `F-S-007`, `F-S-011`, `F-S-009` from the prior audit overlap this scope and are noted briefly but not re-reported in full; everything below is either new or a sharper restatement.
|
|
|
|
## High
|
|
|
|
### Dashboard self-password-change does not invalidate other live sessions or API keys
|
|
- **Where**: `crates/manager-core/src/admin_users_api.rs:232-241`
|
|
- **What**: The `PATCH /admin/admins/{id}` password-change branch updates `password_hash` and immediately comments: *"Best practice: rotating your own password should still keep your session alive, so we don't wipe sessions here."* — but it never wipes **other** sessions or any `api_keys` rows either. By contrast, `cmd_reset_password` in `crates/picloud/src/main.rs:189-200` *does* call `sessions.delete_for_user`, and the deactivation branch (`admin_users_api.rs:296-310`) wipes sessions + API keys. Only the in-product password change is silent.
|
|
- **Impact**: An admin who suspects compromise and rotates their password through the dashboard (the natural reaction) leaves every other live session and every minted API key valid until they hit individual TTL expiry (`admin_sessions.expires_at` defaults to 24h sliding, API keys until manually revoked). A stolen `picloud_session` token or `pic_…` bearer continues to authenticate. This is the classic "credential rotation that doesn't rotate credentials" pitfall — and it directly contradicts blueprint §11.4's stated invariant that a password change invalidates all other sessions for that user (memory `admin_auth_design`).
|
|
- **Recommendation**: After `update_password_hash`, fetch the caller's current `token_hash` from the request, call `sessions.delete_for_user(id)`, **then** re-`create` the caller's session (or accept the sign-out and 401 the response). Also call `keys.expire_all_for_user(id)` and document the consequence in the UI ("Changing your password will revoke all of your API keys"). The current behavior is silently more dangerous than either alternative.
|
|
|
|
### Bearer-token Bootstrap can be re-triggered by deleting all admins
|
|
- **Where**: `crates/manager-core/src/auth_bootstrap.rs:89` (count gate) + `crates/manager-core/src/admin_users_api.rs:340-355` (`delete_admin`)
|
|
- **What**: `bootstrap_first_admin_with` short-circuits when `count_active() > 0`. The guard counter is whatever `count_active()` reports — checking that repo: it counts every row (including `is_active=false`), so an empty `admin_users` table re-enables bootstrap on next process start. That alone is fine, **but** `delete_admin` performs a hard DELETE (`users.delete(id)`, no soft-delete). The "last-active-admin" guard only blocks **deactivation/deletion of the only `is_active=true` row**. If an operator deactivates all admins (impossible — last-active guard) OR an attacker with `InstanceManageUsers` can wipe rows after escalation, the bootstrap env vars become live again on next restart. Combined with the fact that `PICLOUD_ADMIN_USERNAME` / `PICLOUD_ADMIN_PASSWORD_HASH` are still set in nearly every production compose file ("for the initial install"), this is a re-takeover path.
|
|
- **Impact**: An attacker who can either (a) gain `InstanceManageUsers` briefly and delete all admin rows, or (b) drop `admin_users` rows via SQL (separate compromise) — followed by a restart (cron, deploy, OOM) — re-seeds the env-supplied admin. The env-var leak risk (leaked from CI, `.env` file, `docker inspect`, `/proc/<pid>/environ`) becomes a perpetual liability rather than a one-shot.
|
|
- **Recommendation**: Either (1) refuse bootstrap if **any** row has ever existed (track with a `bootstrap_done` row in `instance_state` or a sentinel boolean in `admin_users`), or (2) document and enforce that the bootstrap env vars are unset after first start (start-up warning when set; harden compose templates). Option (1) is the safer default; the env var is by design a "fresh install" hatch.
|
|
|
|
## Medium
|
|
|
|
### Admin sessions have no absolute (hard-cap) expiry
|
|
- **Where**: `crates/manager-core/src/admin_session_repo.rs:23-30`, `crates/manager-core/migrations/0004_admin_auth.sql:24-30`, `auth_middleware.rs:245-249`
|
|
- **What**: `admin_sessions` has `expires_at` (sliding) and `last_used_at`, no `absolute_expires_at`. Every authenticated request bumps `expires_at = NOW() + ttl`. A stolen token that's exercised at least once per `PICLOUD_SESSION_TTL_HOURS` (default 24h) never expires. Contrast with `app_user_sessions` (v1.1.8) which has both `expires_at` and `absolute_expires_at` — the admin surface is the **more** sensitive of the two and has weaker session hygiene.
|
|
- **Impact**: Long-lived token theft (e.g., a stale localStorage entry on a shared workstation, a leaked support session) is materially harder to detect; nothing forces a re-auth.
|
|
- **Recommendation**: Add `absolute_expires_at TIMESTAMPTZ NOT NULL` (default `NOW() + 30d` or `NOW() + 2*ttl`) populated at `create`, never touched by `touch`. Filter `WHERE absolute_expires_at > NOW()` in `lookup`. Mirrors the shape of `app_user_sessions`.
|
|
|
|
### Admin bearer token is stored in `localStorage` — XSS = account takeover with no recovery window
|
|
- **Where**: `dashboard/src/lib/auth.ts:22-43`, `dashboard/src/lib/api.ts:493-495`
|
|
- **What**: `setSession()` writes the raw bearer token to `localStorage['picloud.admin.token']`. Any XSS on the dashboard origin trivially exfiltrates it. The HttpOnly cookie is also set, so the **cookie path** is XSS-safe, but the `localStorage` echo defeats that defense — the dashboard's own auth call path reads from localStorage and injects `Authorization: Bearer …`, which means any XSS-injected `fetch` works identically. Adding `Strict-Transport-Security` and a CSP nonce wouldn't help if a script-tag injection lands.
|
|
- **Impact**: An attacker with one stored-XSS primitive in any admin-rendered surface (Markdown in script descriptions, app names, route paths in admin views, audit log fields, etc.) elevates to durable session theft. Combined with the High above, the stolen token survives a password change.
|
|
- **Recommendation**: Drop the localStorage echo and rely on the HttpOnly cookie alone for browser sessions. The cookie already round-trips on every request (Path=/). Keep `Authorization: Bearer …` injection only for non-browser CLI clients that explicitly call the login API and store the token themselves. If localStorage must stay for SPA UX reasons, at minimum: rotate the token on first request after detection of a CSP violation event, and ship a strict CSP (`default-src 'self'; script-src 'self'`) so injection requires a CSP bypass.
|
|
|
|
### Password change does not enforce current-password re-verification
|
|
- **Where**: `crates/manager-core/src/admin_users_api.rs:204-241`
|
|
- **What**: `PATCH /admins/{id}` accepts a new password without any "current password" or step-up auth. Anyone with a live session token can change the account's password to one of their choosing. Combined with the long sliding-TTL session (no absolute cap), a single hijacked session escalates to permanent ownership.
|
|
- **Impact**: Lost / borrowed laptop, brief session theft, XSS — all become permanent takeover because the attacker can reset the password and lock the legit user out (subject to the High finding: their existing session even survives the password change).
|
|
- **Recommendation**: Require the request body to include `current_password` when the patch carries a `password` field for the caller's own id. Verify with `verify_password` on `spawn_blocking`. For admin-on-admin password change, require recent step-up (e.g., the caller re-supplied their password in the last 5 minutes).
|
|
|
|
### `PICLOUD_COOKIE_SECURE` defaults to ON but is silently disablable; no warning emitted
|
|
- **Where**: `crates/manager-core/src/auth_api.rs:221-227`
|
|
- **What**: `build_cookie` honors `PICLOUD_COOKIE_SECURE=0|false|no|off`. There is no startup log surfacing the resolved value, no refusal when combined with a public `PICLOUD_PUBLIC_BASE_URL`. An operator running prod over HTTP-only mode (reverse proxy misconfig, internal-only deploy, dev-on-prod leak — see F-S-009) silently issues plain-HTTP cookies.
|
|
- **Impact**: Network-adjacent attackers (LAN, MITM, hijacked Wi-Fi) capture admin session tokens off the wire.
|
|
- **Recommendation**: Refuse `PICLOUD_COOKIE_SECURE=off` when `PICLOUD_PUBLIC_BASE_URL` starts with `https://` or contains a non-localhost host. At minimum, log an `error!` on every login that resolves a non-Secure cookie.
|
|
|
|
### CLI `reset-password` reads stdin with terminal echo (no `rpassword`)
|
|
- **Where**: `crates/picloud/src/main.rs:203-216`
|
|
- **What**: `prompt_password_from_stdin()` prints "will be read from stdin, no echo" — but it just calls `std::io::stdin().lock().read_line(&mut line)`. On a real TTY, that echoes characters. The "no echo" claim is false. Also: the raw password sits in `line` (a `String` allocated on the heap) with no zeroization on drop.
|
|
- **Impact**: Shoulder-surfing during recovery; password ends up in shell history if the operator piped via `read -p` wrapper; raw password lives in process heap after use (forensic concern).
|
|
- **Recommendation**: Use `rpassword::read_password` (or `termion`) for terminal echo suppression. Wrap the password in `secrecy::SecretString` so it zeroizes on drop. Update the prompt text to reflect reality if the simple-stdin path is kept for piped CI use.
|
|
|
|
### Login surface has no rate limit, lockout, or captcha (recap of F-S-007)
|
|
- **Where**: `crates/manager-core/src/auth_api.rs:74-110`
|
|
- **What**: AUDIT.md F-S-007 already records this — `POST /auth/login` Argon2-verifies every attempt, no per-IP throttle, no per-username lockout. Re-flagged here only because it's *the* missing control for a self-host admin surface. Defense is "stand up Caddy rate-limit in front" which is documented nowhere user-visible.
|
|
- **Recommendation**: As in F-S-007: per-IP token bucket and per-username sliding-window lockout. At minimum, document the requirement explicitly in `serverless_cloud_blueprint.md` deployment notes.
|
|
|
|
## Low
|
|
|
|
### `validate_password` only enforces a length minimum (8); no character class, no breached-password check, no maximum
|
|
- **Where**: `crates/manager-core/src/admin_users_api.rs:35,382-389`
|
|
- **What**: 8 chars is below current NIST guidance for unrestricted-rate verifiers (NIST SP 800-63B suggests ≥8 with breached-password check OR rate-limit). Argon2 absorbs arbitrarily long passwords, so no hard upper bound = a 64MB password POST can complete the Argon2 (memory-bounded). Combined with no login rate limit, a long-password DoS is a small lever.
|
|
- **Recommendation**: Raise minimum to 12, add a maximum (e.g., 1024 chars) to bound input cost, and optionally wire `zxcvbn` for strength feedback in the dashboard.
|
|
|
|
### Logout reveals nothing about whether the token was valid
|
|
- **Where**: `crates/manager-core/src/auth_api.rs:169-187`
|
|
- **What**: Logout always returns 204 regardless of whether the supplied token matched a row. This is correct (idempotent) — the only observation is that an attacker can use `/auth/logout` as a free oracle to test whether a candidate token *exists* by, say, timing the DELETE response. Postgres DELETE timing is data-dependent but the variance is small. Listing as Info-grade because the practical impact is negligible (32-byte random tokens are unguessable).
|
|
- **Recommendation**: None required; noted for completeness.
|
|
|
|
### `me` re-reads the `admin_users` row on every call; no caching of fresh username
|
|
- **Where**: `crates/manager-core/src/auth_api.rs:189-210`
|
|
- **What**: Not a security issue. Cross-reference only because the prior audit's F-P-009 PrincipalCache might leak stale role data through `/me` — verified: `/me` deliberately bypasses the cache and re-fetches, so role changes show up promptly. Behavior is correct.
|
|
|
|
### `extract_token_for_logout` duplicates middleware extraction logic
|
|
- **Where**: `crates/manager-core/src/auth_api.rs:234-261` vs `crates/manager-core/src/auth_middleware.rs:362-386`
|
|
- **What**: Two copies of the same Bearer / cookie extraction. Cosmetic risk of drift; one might add a CSRF check or token-format validation and forget the other.
|
|
- **Recommendation**: Extract to a shared function in `auth.rs`.
|
|
|
|
## Info
|
|
|
|
### Argon2 parameters are OWASP defaults and salt is per-hash from `OsRng`
|
|
- **Where**: `crates/manager-core/src/auth.rs:41-45`
|
|
- Argon2id, `Argon2::default()` (v0x13, m=19456 KiB, t=2, p=1, 32-byte hash), salt from `argon2::password_hash::rand_core::OsRng`. `argon2 = "0.5"`, `rand = "0.8"`. Verify path is the library's `verify_password` (constant-time on hash bytes). PHC encoding pins parameters per-hash so a future raise of defaults doesn't break existing rows. No findings here; recorded for the audit trail.
|
|
|
|
### Session token entropy and storage shape are sound
|
|
- **Where**: `crates/manager-core/src/auth.rs:80-95`, `migrations/0004_admin_auth.sql:24-30`
|
|
- 32 random bytes from `rand::rngs::OsRng` (256 bits, far above the 128-bit minimum) → base64-url-no-pad → SHA-256 hex stored as PK on `admin_sessions.token_hash`. Lookup is index-equality so timing on the DB side is bounded by the index path, not by token contents (the SHA-256 step happens before the DB call and is constant-time over the input). Raw token never persists; logout/touch/delete all keyed on hash. Token-equals-bearer is by design (memory `admin_auth_design`). No findings.
|
|
|
|
### Login is timing-flat against username enumeration
|
|
- **Where**: `crates/manager-core/src/auth.rs:35-36`, `auth_api.rs:79-110`
|
|
- The `TIMING_FLAT_DUMMY_HASH` constant is verified against on missing-user lookups, so the wall-clock cost of "bad username" matches "bad password". `spawn_blocking` is used for the verify (F-P-002 remediation). No timing oracle on enumeration via login.
|
|
|
|
### Bootstrap idempotency race is safe by accident
|
|
- **Where**: `crates/manager-core/src/auth_bootstrap.rs:89-128`
|
|
- `count_active() > 0` check is not atomic with `repo.create`, but `admin_users.username` is `UNIQUE`, so concurrent boots can't double-seed. The second loser surfaces a `Repo` error rather than silently skipping — operator-visible. Acceptable.
|
|
|
|
### MFA / 2FA not present
|
|
- Out of MVP per blueprint and consistent with prior decisions (single-user solo-dev target). Worth tracking as an explicit "deferred until v1.2 multi-admin scenarios" item.
|
|
|
|
## Out of scope (cross-references)
|
|
- **F-S-006** (API-key Argon2 amplifier), **F-S-007** (no login rate-limit), **F-S-009** (DEV_MODE deterministic key), **F-S-011** (no `revoked_at` on `admin_sessions`) — all covered in `AUDIT.md`. Remediation state per memory `audit_2026_06_07_remediation`: F-S-006 and F-S-007 mitigated (cap + spawn_blocking); F-S-009 and F-S-011 outstanding.
|
|
- API-key authorization scopes — covered by agent 2 (authorization).
|
|
- Encryption-at-rest and key rotation (`PICLOUD_SECRET_KEY`) — covered by agent 3 (crypto).
|
|
- CSRF on cookie-auth endpoints — partially in this report (SameSite=Lax decision); cross-origin scenarios covered by agent 6 (network/CSRF).
|
|
- Login DoS via Argon2 memory — covered by agent 7 (DoS).
|
|
- Authorization headers in tower-http TraceLayer logs — covered by agent 10 (information disclosure).
|