diff --git a/IMPLEMENTATION.md b/IMPLEMENTATION.md index f12226e..6c434ca 100644 --- a/IMPLEMENTATION.md +++ b/IMPLEMENTATION.md @@ -89,6 +89,11 @@ be built as written, or cannot be built once, until something else lands. | 7 | The creator | `docs/waves/wave-7.md` | `B7` `A1` `CR-015` `A2` `A6` `CR-014` `CR-007` `B6` `S1`(creator) `F6` `D1` `D2` `D3` `D4` `D5` `D8` `D9` `D10` | | 8 | Kitting and material | `docs/waves/wave-8.md` | `CR-009` `CR-010` `CR-011` `CR-012` `CR-013` `D6` `D10` | | 9 | Verification and cleanup | `docs/waves/wave-9.md` | `CR-008` `CR-017` `S6` `S7` `C1` `C2` `C4` `D7` | +| 10 | Domain authentication over LDAPS | `docs/waves/wave-10.md` | `D13` | + +Wave 10 was added on August 21, 2026 and is not part of the original nine-wave sequence. It +is new scope (`docs/waves/decisions-2026-08-21.md`), not a reinterpretation of anything +above, and it depends only on wave 9 being merged rather than on any particular item in it. **Waves 1 through 4 produce almost no field-visible change.** That is deliberate and it is roughly the first third of the effort. It is called out here because the Micron team is diff --git a/docs/waves/decisions-2026-08-21.md b/docs/waves/decisions-2026-08-21.md new file mode 100644 index 0000000..7f02d25 --- /dev/null +++ b/docs/waves/decisions-2026-08-21.md @@ -0,0 +1,129 @@ +# Decisions — August 21, 2026 + +One item, and it is the largest single change to the auth model since the login portal +shipped. Like the August 18 and August 20 sets it is a **new item** with its own `D` id, +not a reinterpretation of an existing one. `D1`–`D12` are taken; this is `D13`. + +Raised by Cody Schaefer on Aug 21 2026 while asking how the suite handles HTTPS. Nothing in +`IMPLEMENTATION.md`, in `CR`/`F`/`S`/`A`/`B`/`C`, or in `docs/waves/backlog.md` covers +authentication against the domain — so this is new scope, and it gets a new ID rather than +being folded into the login-portal work that produced `server/auth.py`. + +--- + +## D13 — Authentication moves to the domain over LDAPS + +- **Amends:** the authentication model shipped in `DEPLOY-login-portal.md` (bcrypt hashes in + `users.password_hash`, verified in-process). That document describes what is being + replaced, not what is wrong — it was correct for a suite with no directory behind it. +- **Surface:** `server/` (`auth.py`, `app.py`, `models.py`, `manage_users.py`, `notify.py`, + a new `ldap_auth.py`, a new migration), `html/` (`login.html`, `login.js`, `admin.js`, + `users.js`), `requirements.txt`, `docker-compose.yml`, `Dockerfile`, deployment docs. +- **Wave:** 10 (`docs/waves/wave-10.md`). Depends on wave 9 merged, which it is. + +### The decision + +The suite stops storing passwords. A sign-in becomes a **simple bind to +`ldaps://prime.local:636`** as `@prime.local` using the password the person typed. +A successful bind is the authentication. `users.password_hash` is dropped from the schema. + +Four parts, all four required for the item to be done: + +1. **LDAPS bind replaces local password verification.** `password_hash` is removed from the + model and from the database by migration. No password is stored, hashed or otherwise. +2. **Accounts are provisioned just-in-time.** A successful bind for a username with no + `users` row creates one, at the default role, with `full_name`/`email` read from the + directory. +3. **A required group gates login.** An AD group is configured; a bind that succeeds but + whose account is not in that group is refused. Membership is evaluated including nested + groups. +4. **Existing accounts keep their roles, and roles stay local.** An existing `admin` stays + an admin on first directory login. Granting admin to an existing account continues to + work from the Admin console. The directory supplies *identity*; this app supplies + *authorization*. + +### Why LDAPS and not the certificate already in play + +Recorded because the question was asked directly and the answer is not obvious. + +The site's serving certificate is a **Let's Encrypt** cert (`CN=wp.controls.dev`, issued by +`Let's Encrypt YE2`, expiring 2026-11-08) held by an **OpenResty** instance at +`192.168.3.56` that is not part of this repo. It is a public domain-validated certificate. +It attests that whoever presented it controls DNS for `wp.controls.dev`; it carries no user +identity and no relationship to `prime.local`. There is no configuration that turns it into +a domain credential, so cert-based auth was never available "for free". + +Client-certificate auth (mTLS) was considered and rejected for this wave: TLS terminates two +hops upstream at OpenResty, so the API never sees the handshake, and doing it in-app would +mean bypassing the proxy and losing the CSP/HSTS headers and static serving with it. + +### What was verified before writing this (Aug 21 2026) + +| Fact | Value | +|---|---| +| LDAPS reachable | `192.168.3.37:636` open, TLS 1.3, `TLS_AES_256_GCM_SHA384` | +| DC cert issuer | `CN=PRIME CONTROLS ISSUING CA 1, DC=prime, DC=local` | +| Root of that chain | `CN=PRIME CONTROLS ROOT CA` (self-signed, expires 2051-09-09) | +| Issuing CA expiry | 2036-09-09 | +| DC cert SAN | `DR-DC10Core.prime.local`, `prime.local`, `PRIME` | +| DCs published in `_ldap._tcp.prime.local` | six — `nla-dc10`, `lew-dc20`, `dr-dc30-core`, `lew-dc40`, `SABINEDC`, `dr-dc10core` | +| Chain validates against root+issuing bundle | yes — `Verify return code: 0 (ok)` | + +Two consequences of that table, both binding on the build: + +- **Connect to `prime.local`, not to a DC name or an IP.** Every DC's certificate carries + `prime.local` in its SAN, so the domain name both passes hostname validation and + round-robins across all six DCs. Verified: `prime.local` gives `0 (ok)`; the raw IP + `192.168.3.37` gives `62 (hostname mismatch)`, because there is no IP SAN. +- **The trust anchor is a CA certificate, not a certificate issued to this app.** The API is + the TLS *client*; clients present nothing. It needs `PRIME CONTROLS ROOT CA` plus + `PRIME CONTROLS ISSUING CA 1` as a PEM bundle, which is public information. No CSR, no + enrollment, no private key, nothing to request from IT. + +### Non-negotiables + +These are the ways this change goes wrong, and each has a done-when check in wave 10. + +- **An empty password must be rejected before `bind()` is called.** In LDAP a simple bind + with an empty password is an *anonymous* bind and it **succeeds**. Without an explicit + guard, a blank password authenticates as any username submitted. This is the single + highest-severity failure mode in the item and it gets its own test. +- **`validate=ssl.CERT_REQUIRED` with an explicit CA file.** Not `CERT_NONE`, and not the + system trust store. `CERT_NONE` still encrypts, so it fails silently — what it loses is + the ability to distinguish the real DC from an attacker who terminates the TLS session, + harvests the domain password and relays the bind onward. Since domain credentials now + cross that channel, a compromise escalates from "this app" to Windows, mail and file + shares. The system store is refused separately because it currently trusts five other + self-signed CAs (`prime-DR-CAPRIME-CA`, `prime-DR-CA_PRIME-CA`, `prime-DR-DC20-CA`, + `PRIME CONTROLS ISSUING CA 2`, and a stray `L55401TDKLY3.prime.local` machine cert in + Trusted Root). +- **The app's lockout must trip below the domain's.** `LOGIN_MAX_ATTEMPTS` currently writes + to the local `users` row. Once failures are binds, they count against the **AD** lockout + policy, so an unauthenticated caller hammering `/api/auth/login` can lock real domain + accounts out of Windows. The local throttle must stop calling the DC before the domain + threshold is reached. +- **Never leak which usernames exist.** `login()` today equalises response timing on purpose + so a caller cannot enumerate accounts. Directory error 49 sub-codes (`52e` bad password, + `532` password expired, `533` disabled, `775` locked) are useful in the log and must not + reach the response body. + +### Open, to confirm in the PR rather than decide alone + +- **Break-glass.** Removing `password_hash` means an unreachable DC locks out *everyone*, + admins included. See `T10.2`. +- **Username ↔ `sAMAccountName` mapping.** Existing accounts were created by + `manage_users.py` with hand-typed usernames. Criterion 4 holds only where those match the + directory's `sAMAccountName`. The production account list must be checked against AD + before this deploys; a mismatch means an existing admin gets a *second*, JIT-provisioned + account at the default role instead of keeping their admin. `auth.find_user` already + matches on username **or** email case-insensitively, which covers some of the gap but not + all of it. + +### Explicitly out of scope + +- mTLS / client-certificate authentication (see above). +- Kerberos / SPNEGO single sign-on. It is the better long-term answer for domain-joined + desktops and needs a keytab, an SPN and browser trust configuration; it is not this item. +- Group-to-role mapping (e.g. an AD group that confers `project_admin`). Criterion 4 keeps + authorization local on purpose. Worth its own item later; logged in `backlog.md`. +- Replacing the Let's Encrypt certificate or changing anything on the OpenResty host. diff --git a/docs/waves/wave-10.md b/docs/waves/wave-10.md new file mode 100644 index 0000000..f344a3e --- /dev/null +++ b/docs/waves/wave-10.md @@ -0,0 +1,311 @@ +# Wave 10 — Domain authentication over LDAPS + +**Items:** `D13` +**Depends on:** wave 9 merged (it is — `a8e28bf`) + +One item, eight tasks. The item is stated in `docs/waves/decisions-2026-08-21.md`; read it +before starting, particularly the **Non-negotiables** section, which is where this change +goes wrong if it goes wrong. + +**This wave is field-visible in one direction only.** Nobody gains a screen. People sign in +with their Windows password instead of an app password, the "Forgot password?" link +disappears, and admins stop issuing passwords. Everything else is invisible — which means +the done-when checks are the only evidence the wave worked. + +**Build order is task order** for once. `T10.1` is a standalone module with no callers and +should merge first; nothing else can be tested until it exists. + +--- + +### T10.1 — The LDAPS client + +- **Items:** `D13` (1) +- **Depends on:** nothing +- **Blocks:** T10.2, T10.4, T10.5, T10.7 +- **Surface:** `server/` +- **Files:** new `server/ldap_auth.py`, `server/requirements.txt`, `docker-compose.yml`, + `Dockerfile`, `server/.env.example` + +**Problem:** There is no directory client in the repo. `ldap3` is not a dependency, the API +container has no CA bundle, and the `api` service sits on the `internal` network, which has +no default gateway and therefore no route to `192.168.3.x` at all. + +**Do:** Add `ldap3` (pinned, per the convention at the top of `requirements.txt`). Write +`server/ldap_auth.py` exposing two functions and nothing else: + +- `verify(username, password) -> LdapResult | None` — simple bind as + `f"{username}@{domain}"` against `ldaps://{host}:636`. +- `member_of(conn, group) -> bool` — nested-group aware, via + `LDAP_MATCHING_RULE_IN_CHAIN` (`1.2.840.113556.1.4.1941`). `memberOf` alone is direct + membership only and will wrongly refuse anyone in a nested group. + +Configuration by environment: `LDAP_HOST` (default `prime.local`), `LDAP_DOMAIN` (default +`prime.local`), `LDAP_CA_FILE`, `LDAP_REQUIRED_GROUP`, `LDAP_TIMEOUT_SECONDS`. Attach the +`outbound` network to `api` in `docker-compose.yml` — the same reason `assets_db.py` needed +it, and the comment there already explains the gateway-less `internal` network. Mount or +`COPY` the PEM bundle and point `LDAP_CA_FILE` at it. + +Connect to **`prime.local`**, never a DC hostname and never an IP. See the decision doc for +why: SAN coverage plus round-robin across six DCs in one move. Wrap the bind in a retry +across resolved addresses, because round-robin will hand out a rebooting DC's address. + +**Done when:** + +- [ ] `ldap3` is pinned to an exact version in `requirements.txt` +- [ ] an empty or whitespace-only password returns failure **without calling `bind()`** +- [ ] an empty username returns failure without calling `bind()` +- [ ] `Tls` is constructed with `validate=ssl.CERT_REQUIRED` and an explicit `ca_certs_file` +- [ ] no code path sets `CERT_NONE`, and none falls back to the system trust store +- [ ] `member_of` returns true for an account in a **nested** child of the required group +- [ ] a bind against `192.168.3.37` (raw IP) fails hostname validation rather than silently passing +- [ ] `docker compose exec api openssl s_client -connect prime.local:636 -CAfile $LDAP_CA_FILE` reports `Verify return code: 0 (ok)` +- [ ] the module imports cleanly with no LDAP env set (unconfigured is a first-class state, as with `MICRON_DB_URL`) + +--- + +### T10.2 — The login path binds instead of hashing + +- **Items:** `D13` (1) +- **Depends on:** T10.1 +- **Blocks:** T10.3, T10.4 +- **Surface:** `server/` +- **Files:** `server/app.py` (`login`), `server/auth.py` + +**Problem:** `login()` at `server/app.py:681` calls `auth.verify_password` against +`user.password_hash`. The lockout counter it maintains is about to start counting *domain* +bind failures, which changes what that counter is for. + +**Do:** Replace the credential check with `ldap_auth.verify`. Keep the surrounding shape — +the deliberate timing equalisation, the generic `401`, the `403` for a disabled local +account, `last_login_at`, the session cookie. Rework the throttle so the local counter trips +**before** the domain policy is reached and short-circuits without calling the DC: the +failure mode to avoid is this endpoint being usable to lock domain accounts out of Windows. +Log directory error-49 sub-codes for diagnosis; return the same generic message regardless. + +Decide the break-glass question here (see **Open** in the decision doc) and record the answer +in this file before implementing, because it changes this task's shape. + +**Done when:** + +- [ ] a correct domain password signs in and sets the session cookie +- [ ] a wrong password is refused with the generic message +- [ ] a blank password is refused (guards `T10.1` from the caller's side too) +- [ ] a user not in the required group is refused even though the bind succeeded +- [ ] `is_active = false` locally still refuses, independent of the directory +- [ ] the local throttle trips below the domain lockout threshold and stops calling the DC +- [ ] no response body distinguishes "no such user" from "wrong password" +- [ ] error-49 sub-codes appear in the log and nowhere in any response + +--- + +### T10.3 — Drop `password_hash` + +- **Items:** `D13` (1) +- **Depends on:** T10.2 +- **Blocks:** T10.6, T10.8 +- **Surface:** `server/` +- **Files:** `server/models.py`, new migration, `server/app.py`, `server/auth.py`, + `server/manage_users.py` + +**Problem:** With binds doing the work, every password code path is dead weight and a +liability. It is also the criterion that makes this item irreversible, so it lands on its own +commit. + +**Do:** Remove `password_hash` from `models.User` and drop the column in a new Alembic +revision with `down_revision = 'a1b8c6d4e2f9'` (the current head — confirm with +`alembic heads` rather than trusting this line). Remove from `auth.py`: `hash_password`, +`verify_password`, `password_problem`, `MIN_PASSWORD_LEN`, `_COMMON_PASSWORDS`, +`create_reset_token`, `decode_reset_token`, `RESET_MINUTES`. Remove from `app.py`: +`/api/auth/forgot-password`, `/api/auth/reset-password`, `/api/auth/reset-available`, +`/api/auth/password`, `/api/auth/users/{user_id}/password`, and the `_reset_last` throttle +with `reset_body`. Remove `reset-password` from `manage_users.py` and the password prompt +from `create` / `create-admin`. + +`token_version` **stays.** It is still the session-revocation mechanism — role changes and +deactivation should bump it even though password changes no longer exist. + +**Do not** remove the account-management endpoints themselves. `/api/auth/users/{id}/role` +is criterion 4 and must keep working. + +**Done when:** + +- [ ] `grep -rn "password_hash\|hash_password\|verify_password\|password_problem" server/` returns nothing outside the migration +- [ ] `alembic upgrade head` then `downgrade -1` round-trips on SQLite and on Postgres +- [ ] the migration's `downgrade()` recreates the column nullable, not `NOT NULL` — there are no hashes to put back +- [ ] `token_version` still invalidates sessions, exercised by a role change +- [ ] `manage_users.py list`, `disable`, `enable` still work; `reset-password` is gone +- [ ] `python -m server.manage_users create-admin ` creates an admin with no password prompt + +--- + +### T10.4 — Just-in-time provisioning, without trampling existing accounts + +- **Items:** `D13` (2, 4) +- **Depends on:** T10.2 +- **Blocks:** T10.7 +- **Surface:** `server/` +- **Files:** `server/app.py` (`login`), `server/auth.py` + +**Problem:** Criteria 2 and 4 pull in opposite directions. Provisioning on first login must +create accounts that do not exist, and must not touch the role of accounts that do — an +existing `admin` signing in for the first time after this wave must still be an admin +afterwards. + +**Do:** On a successful bind that passes the group check, look the account up with +`auth.find_user` (already case-insensitive across username **and** email). If it exists, +update only `last_login_at` and — if empty locally — `full_name` and `email` from the +directory. **Never write `role`.** If it does not exist, create it at +`ROLE_PROJECT_USER` with `full_name`/`email` from the directory, honouring the existing +`auto_add_projects` machinery so a JIT account lands in the right projects. Note the +flush-order warning in the `models.py` docstring: `create_user` in `app.py` already handles +this correctly for account + `ProjectMember` rows in one flush — follow it. + +Write an `AuditLog` row for each JIT creation. An account appearing without an administrator +creating it is exactly the kind of event that record exists for. + +**Done when:** + +- [ ] an unknown username with a valid bind and group membership gets a `users` row at `project_user` +- [ ] `full_name` and `email` are populated from the directory on creation +- [ ] an existing `admin` signing in is still `admin` afterwards — asserted, not assumed +- [ ] an existing account with a locally-set `full_name` does not have it overwritten +- [ ] a JIT account with `auto_add_projects` peers gets its `ProjectMember` rows +- [ ] each JIT creation writes an `AuditLog` row +- [ ] a failed bind creates **no** row +- [ ] a bind that succeeds but fails the group check creates **no** row + +--- + +### T10.5 — The required group is configurable + +- **Items:** `D13` (3) +- **Depends on:** T10.1 +- **Blocks:** T10.8 +- **Surface:** `server/` + `html/` +- **Files:** `server/notify.py` (`DEFAULTS`), `server/app.py` (`SettingsIn`, `/api/settings`), + `html/admin.js` + +**Problem:** The group has to be settable without a redeploy, but it gates login — a typo +in a text box locks the whole company out, including whoever typed it. + +**Do:** Add `ldap_required_group` to `notify.DEFAULTS` and `SettingsIn`, and a field in the +Admin console's settings card next to the notification block. `LDAP_REQUIRED_GROUP` from the +environment is the initial value and the fallback when the setting is empty. + +**Validate on save, not on login.** Before persisting, resolve the group in the directory and +confirm the saving admin is themselves a member. Refuse the save with a specific message if +either check fails. That single guard is what stops the lockout scenario, so it is not +optional. Do not add it to `notify.PUBLIC_KEYS` — an unauthenticated caller must not be able +to read the group name off the login page. + +**Done when:** + +- [ ] the field appears in the Admin console and persists across a restart +- [ ] saving a group that does not exist is refused, with a message naming the group +- [ ] saving a group the current admin is not in is refused +- [ ] an empty setting falls back to `LDAP_REQUIRED_GROUP`, and empty-with-no-env means no group gate (documented, logged at startup) +- [ ] the group name is absent from `/api/settings` for a non-admin and from any pre-login response +- [ ] the field is reachable by keyboard, labelled, and announces its save state through the existing `aria-live` region (`C1`) + +--- + +### T10.6 — Strip the password UI + +- **Items:** `D13` (1) +- **Depends on:** T10.3 +- **Blocks:** nothing +- **Surface:** `html/` +- **Files:** `html/login.html`, `html/login.js`, `html/admin.js`, `html/users.js`, + `html/auth-guard.js` + +**Problem:** `login.html` has three views — sign-in, forgot-password, set-new-password — and +two of them now point at endpoints that no longer exist. `auth-guard.js` has a +change-password dialog, and the user-admin UI has a "reset password" action per row. + +**Do:** Remove the `#view-forgot` and `#view-reset` sections, the `#forgot-link`, and the +reset-token handling in `login.js`. Remove the change-password dialog from `auth-guard.js` +(`backlog.md:180` refers to it) and the per-row password reset from the users UI. Keep the +sign-in form, and relabel the password field's hint to say it is the Windows/domain password +— people need to know which password to type. + +Keep the role-granting controls exactly as they are. That is criterion 4. + +**Done when:** + +- [ ] `grep -rn "forgot\|reset-password\|new-password" html/` returns nothing but unrelated matches +- [ ] the sign-in form still submits, and a failure still announces through `role="alert"` (`login.html` already does this correctly — do not regress it) +- [ ] the password field says which password to enter +- [ ] no dead `` or handler remains for a removed view +- [ ] granting admin to an existing user still works from the console +- [ ] exercised at 390px and at 1440px, screenshots in the PR +- [ ] no raw hex added to any stylesheet (the token rule) + +--- + +### T10.7 — Make the suite testable without a domain controller + +- **Items:** `D13` (1, 2, 3) +- **Depends on:** T10.1, T10.4 +- **Blocks:** T10.8 +- **Surface:** `tests/` + `server/` +- **Files:** `server/ldap_auth.py`, `tests/browser_check.py`, `tests/launcher_check.py`, + `tests/console_dialogs_check.py`, `tests/url_state_check.py`, `server/smoketest.py`, + `server/seed_demo.py`, new `tests/ldap_auth_check.py` + +**Problem:** This is the task most likely to be underestimated. Four existing checks build +users with `password_hash=auth.hash_password(PW)` and sign in over HTTP; +`console_dialogs_check.py` drives the password-reset prompt specifically. `smoketest.py` and +`seed_demo.py` both sign in. None of them can reach a DC, and CI has no domain. + +**Do:** Make the bind injectable — a module-level seam in `ldap_auth.py` that a test can +substitute (a fake directory: usernames, passwords, groups, full names), selected by an env +var that is refused when a real `DATABASE_URL` is configured, mirroring how +`auth._load_secret` refuses an ephemeral key in production. Port the four checks onto it. +Delete the password-reset half of `console_dialogs_check.py` — the flow it covers no longer +exists — and say so in the PR rather than leaving a skipped test. + +Add `tests/ldap_auth_check.py` covering the non-negotiables from the decision doc: empty +password, empty username, `CERT_REQUIRED` asserted by inspecting the constructed `Tls`, +nested group membership, group-check refusal creating no user, and existing-admin role +preservation. + +**Done when:** + +- [ ] the full `tests/` suite passes with no DC reachable +- [ ] the stub backend cannot be selected when a non-SQLite `DATABASE_URL` is set — asserted by a test +- [ ] `tests/ldap_auth_check.py` covers all six cases above +- [ ] the empty-password case fails loudly if the guard is removed (verified by removing it) +- [ ] `smoketest.py` and `seed_demo.py` document which credentials they now need +- [ ] the removed password-reset checks are called out in the PR, not silently dropped + +--- + +### T10.8 — Documentation and the deploy runbook + +- **Items:** `D13` +- **Depends on:** T10.3, T10.5, T10.7 +- **Blocks:** nothing +- **Surface:** docs +- **Files:** `DEPLOYMENT.md`, `server/README.md`, `DEPLOY-login-portal.md`, `CLAUDE.md`, + `IMPLEMENTATION.md` + +**Problem:** `DEPLOY-login-portal.md` documents creating the first admin with a password and +is the page an admin will reach for. `DEPLOYMENT.md` describes `AUTH_SECRET_KEY` and SMTP but +knows nothing about a directory. `CLAUDE.md`'s verification section tells anyone touching the +frontend to run the smoke test, which changes here. + +**Do:** Document the LDAP variables, how to produce the CA bundle from the two thumbprints +in the decision doc, and the `prime.local`-not-an-IP rule with the reason. Add the +`openssl s_client -CAfile` check as the first-line diagnostic. Rewrite the +`DEPLOY-login-portal.md` bootstrap step: the first admin is now an existing directory account +promoted with `manage_users.py`, not an account created with a password. State plainly what +happens when the DC is unreachable, whatever `T10.2` decides. + +**Done when:** + +- [ ] every new env var is documented in `server/.env.example` and `DEPLOYMENT.md` +- [ ] the CA bundle procedure is reproducible by an admin who has not read this thread +- [ ] `DEPLOY-login-portal.md` no longer instructs anyone to set a password +- [ ] the DC-unreachable behaviour is stated explicitly +- [ ] `IMPLEMENTATION.md` section 4 lists wave 10 +- [ ] no doc still claims passwords are stored as bcrypt hashes