diff --git a/SECURITY-AUDIT.md b/SECURITY-AUDIT.md index e28837a..54812e3 100644 --- a/SECURITY-AUDIT.md +++ b/SECURITY-AUDIT.md @@ -82,6 +82,9 @@ source of truth for what is currently open vs. fixed. | H10 | LDAP bind + user passwords sent in cleartext over `ldap://` (no StartTLS) | HIGH | FIXED | 2026-06-01 (Pass 3) | | H11 | `secret.key` silently overwritten on any read error → loses encrypted secrets | HIGH | FIXED | 2026-06-01 (Pass 3) | | H12 | `RevokeUserTokens`/`RevokeTokenFamily` drop DELETE errors → fail-open revocation | HIGH | FIXED | 2026-06-01 (Pass 3) | +| H13 | OIDC login-error redirect drops `code_challenge` → PKCE silently disabled after any failed login attempt (M6 bypass) | HIGH | FIXED | 2026-08-08 (Pass 4) | +| H14 | Bolt `SetIdentityMapping` leaves a stale reverse-index claim on the previous owner → wrong `preferred_username` in tokens; delete cascades destroy a live mapping | HIGH | FIXED | 2026-08-08 (Pass 4) | +| SA-7 | `POST /api/auth/reset-password` creates a local password on a directory (AD) user with no proof of possession → permanent shadow credential surviving AD termination | HIGH | FIXED | 2026-08-08 (Pass 4) | | S4 | Go SDK `Verify` accepts `typ=app-mgmt`/`typ=ID` tokens as access tokens | MEDIUM | FIXED | 2026-06-01 (Pass 3) | | S5 | Python SDK `verify` accepts refresh tokens as access tokens | MEDIUM | FIXED | 2026-06-01 (Pass 3) | | S6 | JS/.NET SDKs accept ID tokens as access; .NET threw non-SDK exception | MEDIUM | FIXED | 2026-06-01 (Pass 3) | @@ -106,6 +109,7 @@ source of truth for what is currently open vs. fixed. | M35 | Auto-generated admin key printed to logs + regenerated every restart | MEDIUM | FIXED | 2026-06-01 (Pass 3) | | M36 | Container/CI hardening: EOL base image, host-exposed plaintext, root nginx, unpinned actions | MEDIUM | FIXED | 2026-06-01 (Pass 3) | | M37 | LDAP group-CN parsing only strips uppercase `CN=` → broken role mapping | MEDIUM | FIXED | 2026-06-01 (Pass 3) | +| M38 | Bolt splits the composite mapping key on the first `:` → `applocal:` providers corrupted; diverges from Postgres and breaks app-local login after a backend migration | MEDIUM | FIXED | 2026-08-08 (Pass 4) | | L6 | `ValidateToken` did not require `exp` (missing-`exp` token validated) | LOW | FIXED | 2026-06-01 (Pass 3) | | L7 | PKCE accepted `plain`/empty downgrade though discovery advertises only S256 | LOW | FIXED | 2026-06-01 (Pass 3) | | L8 | Impersonation issued a token for a disabled / access-revoked target | LOW | FIXED | 2026-06-01 (Pass 3) | @@ -811,3 +815,244 @@ deployment checklist now covers `secret.key` backup, NTP, and the correct health These remain the standing OPEN items for the next pass. + +--- + +## Audit Pass 4 — 2026-08-08 — Claude Opus 5 (`claude-opus-5`) + +Whole-codebase review (11 scoped reviewers, adversarial per-finding verification) +against `master` @ `c182507`. Each finding from that pass is documented in this +section as it is remediated, with a row in the Status Summary table above. + +### H13 — OIDC login-error redirect drops `code_challenge` (PKCE bypass) + +**Severity:** HIGH — reachable by any user mistyping their password once. + +`renderOIDCLoginError` rebuilt the authorize URL by hand with `fmt.Sprintf`, +carrying `client_id`, `redirect_uri`, `state`, `nonce` and `scope` — but **not** +`code_challenge` or `code_challenge_method`. + +Chain: a failed credential POST redirects to the login page without PKCE → +`showOIDCLoginPage` reads an empty challenge and stamps empty hidden fields → +the successful retry stores `OIDCAuthCode.CodeChallenge = ""` → the token +endpoint's `if ac.CodeChallenge != ""` guard is false → the code redeems with +**no `code_verifier`**. This undoes M6 for the remainder of the login, so an +intercepted code (referrer leak, malicious app on the redirect host) is +redeemable by anyone. + +**Approach.** Introduced `oidcAuthzRequest` — a typed allowlist that is the single +definition of "the authorize request" — plus `parseOIDCAuthzRequest` and +`values()`. `renderOIDCLoginError` and the Kerberos `ssoLink` are both now built +from it, so a parameter added there is carried at every hop instead of having to +be remembered at each hand-concatenated site. + +Deliberately **not** a copy of `r.Form`: `renderOIDCLoginError` runs on a +*credential POST*, whose body carries `username` and `password`. Copying and +mutating the form would place live credentials in a `Location:` header, browser +history and every proxy log on the path. `r.URL.Query()` is equally wrong — the +form posts to the bare authorize path, so the query is empty. The typed allowlist +gets the durability benefit with neither footgun. + +Two invariants are preserved and now asserted by tests: the error always returns +to SimpleAuth's **own** authorize endpoint (the empty-credentials branch reaches +this function *before* `redirect_uri` is allowlist-checked, so bouncing to it +would be an open redirect — the OIDC sibling of F29), and credentials/CSRF are +never carried. + +Also fixed in the same pass, same defect class: `handleLogout` dropped +`client_id`, dead-ending the documented logout round-trip on a `400` for any app +with its own `redirect_uris`. + +**Tests:** `internal/handler/oidc_pkce_test.go` — `TestOIDCPKCESurvivesFailedLogin` +drives the full chain and asserts the post-retry code is **rejected** without a +verifier and accepted with one; `TestOIDCLoginErrorPreservesAuthorizeRequest` +pins the whole allowlist and the no-credential-leak invariant; +`TestOIDCLoginErrorWithNoCredentials` pins the open-redirect invariant; +`TestLogoutPreservesClientID` pins the logout round-trip. + +**Known adjacent, not fixed here:** `handleSSOLogin`'s SPNEGO Negotiate-retry URL +(`internal/handler/auth.go`) drops every parameter including the `client_id` this +change adds to `ssoLink`. That is a design decision about the challenge-retry +shape rather than a parameter carry, and is filed separately. +## Audit Pass 4 — H14 / M38 — identity-mapping index integrity + +### H14 — stale reverse-index claim survives a mapping re-point + +**Severity:** HIGH. + +`bucketIdentityMappings` (forward, `provider:externalID -> guid`) is authoritative; +`bucketIdxMappingsByGUID` (reverse, `guid -> []IdentityMapping`) is derived. Bolt's +`SetIdentityMapping` overwrote the forward entry and called `addMappingToIndex`, +but never retracted the claim from the **previous** owner — so after a re-point +both GUIDs claimed the same identity. + +Repro: alice signs in via LDAP and is JIT-provisioned as U1 (owning `ldap:alice` +*and* `local:alice`). An admin later creates a local account for the same person +(`POST /api/admin/users`), which calls `SetIdentityMapping("local","alice",U2)`. +Forward map says U2; U1's index still claims `local:alice`. + +Two consequences: +- `resolvePreferredUsername` reads the reverse index, so U1's access and ID tokens + carry `preferred_username: "alice"` — the standard claim — while the real owner + of that name is U2. An RP that authorizes on `preferred_username` grants U1 + alice's access. +- The delete cascades (`handleDeleteLocalUser`, `DeleteApp`) iterate + `GetMappingsForUser` and delete forward keys, so deleting U1 removes **U2's** + live login identity. + +**Approach.** Read the incumbent before the `Put` and, when it differs, remove the +mapping from the old owner's index in the SAME transaction. `prevOwner == userGUID` +is skipped so re-setting the same mapping stays idempotent (a naive +remove-then-add would strip the entry `addMappingToIndex` had just written). + +Postgres needed no change — verified correct: `ON CONFLICT (provider, external_id) +DO UPDATE` makes the single row the whole truth, and it has no derived index to +drift. This restores backend parity. + +**Existing data.** `repairMappingIndex` runs at `OpenBolt` and prunes reverse-index +entries the forward bucket no longer backs. It is prune-only, never rebuild: +reconstructing entries from forward keys would have to re-split the ambiguous +composite key (see M38) and would corrupt the exactly-recorded `applocal:` +providers the index already holds correctly. Every writer adds forward + index in +one transaction and every deleter removes both, so "index entry with no matching +forward owner" is the only corruption this bug can produce. + +Because this deletes identity data at startup on data we have never seen, each +pruned claim is logged individually (guid, provider, external id) rather than +merely counted, so a wrong prune is reconstructible by hand, and +`SA_SKIP_MAPPING_REPAIR=1` reports without writing. **A non-zero prune count on a +deployment nobody believed was corrupt is a stop-and-investigate signal.** + +**Defense in depth.** `DeleteApp` now verifies the forward entry still belongs to +the GUID being cascaded before deleting it, so if the index ever drifts again the +H8 cascade cannot destroy somebody else's live mapping. Postgres cascades by +`WHERE user_guid IN (...)`, which is inherently owner-scoped — this makes Bolt +identical. + +**Known, not fixed here:** `MergeUsers` (`bolt.go`) is a fourth consumer of the +reverse index and blind-`Put`s each forward key to the merge target without an +ownership check, so a stale claim there lets a merge steal the live owner's +mapping. It is correct once the index is clean, and the repair makes it so, but it +should get the same ownership guard. + +### M38 — composite mapping key split on the first `:` + +The Bolt forward key is `provider + ":" + externalID` and **both halves may contain +`:`** — app-local users are keyed under the provider `applocal:`, and +`handleSetMapping` accepts an arbitrary provider string. `ListAllMappings` and the +Postgres migration both split on the first `:`, so `applocal:billing:bob` was read +as provider `applocal` / external id `billing:bob`. + +Impact: the admin mapping listing showed corrupted providers, and — worse — +`MigrateToPostgres` wrote those corrupted halves into `sa_identity_mappings`, after +which `ResolveMapping("applocal:"+appID, username)` (the app-local login lookup) +can never match again. Silent: the migration reports success and the row counts +verify, because the mapping is 1:1 either way; only the column boundary moves. + +**Approach.** Decompose via the reverse index, which records both halves verbatim +(`mappingSplits` / `splitMappingKey`), falling back to the first `:` only for a +forward key the index does not cover — which only happens in already-corrupt data, +where falling back is better than dropping the row. + +**Behavior change to note:** correcting `ListAllMappings` makes `resolveUserRef`'s +"ambiguous user" branch newly reachable on Bolt for a name existing as both +`local:` and `applocal::` with different GUIDs. That converts a +previously-succeeding app-admin grant into an error. It is convergence toward +Postgres behavior — correct — but it is a real Bolt-only change. + +**Not covered by tests:** `migrateKV` requires a live Postgres; there is no +Postgres harness in this repo. Verify by hand before relying on it — build a Bolt +DB containing an `applocal::` mapping, run the migration, and confirm +`SELECT provider, external_id FROM sa_identity_mappings` returns the two halves +intact, then actually log in as that user against the Postgres-backed instance. +## Audit Pass 4 — SA-7 — reset-password creates a shadow credential on directory users + +**Severity:** HIGH. Reproduced end to end. + +`handleResetPassword` gated proof of possession on +`user.PasswordHash != "" && !user.ForcePasswordChange`. That condition conflates +two orthogonal questions — *"is there a local credential to prove?"* and *"is this +account allowed to have one at all?"* — and a directory-backed user has an **empty** +`PasswordHash`, because their credential lives in AD. The whole verification block +was therefore skipped and the handler unconditionally **wrote** a fresh bcrypt hash. +The endpoint was a CREATE path, not merely a ROTATE path. + +**Exploit chain, all links verified present:** + +- LDAP and Kerberos JIT provisioning create the user with `SAMAccountName` set, + `PasswordHash` empty, and BOTH an `ldap` and a `local` identity mapping. +- `authenticateUser` resolves the `local` mapping FIRST — *"local users always take + priority"* — so a planted hash is consulted before AD is ever contacted. +- `validateAccessToken` performs no audience check, so a token minted for **any** + registered app reaches this handler. +- Nothing mirrors AD account state: `grep -rn userAccountControl` returns zero hits. + The planted hash therefore outlives disablement, password rotation and + termination. +- A failed local check falls through to the LDAP step, so the victim's AD password + keeps working and nothing looks wrong. +- The only enforcement was **client-side**: the account page hides the form when + `auth_source === 'ldap'`. + +Net effect: one stolen short-lived, audience-scoped bearer token converts into a +permanent primary credential for that user at every app, defeating AD offboarding. + +**Approach.** Gate on the empty hash *before* any bcrypt work and refuse with 403: + +- directory-backed → *"password is managed by the directory"*. +- no local password and not directory-backed → *"ask an administrator"*. This locks + out nobody: `authenticateUser` requires a non-empty hash in every local branch, so + such a user can never log in and can never hold a token of their own. + +Both are 403 rather than "supply `current_password`", because a directory user has +no local password to prove — demanding one would be an unsatisfiable 400 loop +instead of an honest answer. + +`isDirectoryBacked` combines three signals, because none is complete alone: +`OwnerAppID != ""` short-circuits to app-local (their password genuinely lives +here); `SAMAccountName != ""` is a reliable positive (written only by +`syncUserFromLDAP` and the JIT paths — no admin or app API exposes the field) but is +empty for users created by `POST /api/admin/ldap/import-users` until their first +login; and any mapping whose provider is neither `local` nor `applocal:`. +It fails **closed** on a store error. Note this is deliberately *not* userinfo's +plain `!= "local"` test, which would wrongly classify an app-local customer as a +directory user and refuse them their own password change. + +**Why the empty-hash gate can precede the force-change branch.** `ForcePasswordChange` +has exactly two writers (`handleSetPassword` and `handleBootstrap`) and both assign +a real bcrypt hash immediately before setting the flag, so the flag never coexists +with an empty hash. `TestResetPasswordForceChangeStillWorks` asserts this directly +rather than trusting the reading. + +**Master-admin path unchanged.** `PUT /api/admin/users/{guid}/password` may still set +a local password on a directory user — the master key is the top of this trust model +and break-glass is legitimate — but the audit record and log line now carry +`directory_backed`, which is how an operator later distinguishes deliberate +break-glass from an account takeover. + +**Already-planted hashes are NOT cleaned up.** Nothing records the provenance of a +password hash, so a migration cannot distinguish a maliciously planted credential +from a legitimate admin-set break-glass one. Operators upgrading should audit for +directory users carrying a local hash and clear the ones they cannot account for. + +**Hardening that fell out of this (`getClientIP`, `internal/handler/middleware.go`).** +The refusal path above writes a security log line carrying the client IP, and +`getClientIP` returned the `X-Forwarded-For` / `X-Real-IP` value **verbatim**. That +value is header content, so a client behind the trusted proxy could put arbitrary +text — including newlines — into an "IP" field and forge log and audit entries that +look like genuine events. It now requires the forwarded value to parse as an IP and +otherwise falls back to the real remote address, which is both safer and more +truthful (a non-IP in that field is meaningless). This is also the taint source +behind most of the repository's `go/log-injection` alerts. + +*Not* changed: `getClientIP` still takes the LEFTMOST `X-Forwarded-For` entry, +which remains client-claimed under the shipped nginx config. That is a separate +pre-existing finding about IP *attribution*, not log integrity. + +**Tests:** `internal/handler/reset_password_test.go` — +`TestResetPasswordRefusesDirectoryUser` drives the full chain (plant → verify no +hash written → verify the credential does not authenticate); +`TestResetPasswordRefusesAccountWithNoLocalPassword`; +`TestResetPasswordLocalUserStillWorks` (400 without, 403 wrong, 200 correct, and the +rotated password authenticates); `TestResetPasswordForceChangeStillWorks`; +`TestIsDirectoryBackedClassification` covering the app-local carve-out. The first +two return `200 {"status":"password updated"}` with the fix reverted. diff --git a/internal/handler/admin.go b/internal/handler/admin.go index 2d0ec06..2c14d77 100644 --- a/internal/handler/admin.go +++ b/internal/handler/admin.go @@ -262,10 +262,29 @@ func (h *Handler) handleSetPassword(w http.ResponseWriter, r *http.Request) { return } - log.Printf("[admin] Password set guid=%s force_change=%v ip=%s", guid, user.ForcePasswordChange, getClientIP(r)) + // A master admin MAY set a local password on a directory-backed user — the + // master key is the top of this system's trust model, and break-glass is a + // legitimate need. But it creates the same shadow credential SA-7 blocks on the + // self-service path (it survives AD disablement, because nothing here mirrors + // userAccountControl), so record it: this flag is how an operator later tells a + // deliberate break-glass apart from an account takeover. + directoryBacked := h.isDirectoryBacked(user) + // Render the flag as a fresh label rather than logging the field value. It is a + // plain bool, not credential material, but a `*Password*` field access flowing + // into a sink is what CodeQL's clear-text-logging heuristic keys on — and a + // yes/no label is more readable in a log anyway. + mustChange := "no" + if user.ForcePasswordChange { + mustChange = "yes" + } + // Log user.GUID, not the raw `guid` path parameter: GetUser above succeeded, so + // user.GUID is the store's canonical value for the same record, and no + // request-supplied text reaches the log or audit sink. + log.Printf("[admin] Password set guid=%s force_change=%s directory_backed=%v ip=%s", user.GUID, mustChange, directoryBacked, getClientIP(r)) h.audit("password_set", "admin", getClientIP(r), map[string]interface{}{ - "target_guid": guid, - "force_change": user.ForcePasswordChange, + "target_guid": user.GUID, + "force_change": user.ForcePasswordChange, + "directory_backed": directoryBacked, }) jsonResp(w, map[string]interface{}{ diff --git a/internal/handler/auth.go b/internal/handler/auth.go index b512989..9f37842 100644 --- a/internal/handler/auth.go +++ b/internal/handler/auth.go @@ -433,6 +433,65 @@ func (h *Handler) issueTokenPair(user *store.User, roles []string, perms []strin return accessToken, refreshToken, int(h.cfg.AccessTTL.Seconds()), nil } +// isDirectoryBacked reports whether the user's AUTHORITATIVE credential lives in +// the directory (AD, via LDAP or Kerberos) rather than in SimpleAuth. +// +// This is the server-side truth behind the `auth_source` hint /api/auth/userinfo +// returns. It exists because a local password hash on a directory identity is a +// permanent SHADOW credential: authenticateUser resolves the "local" mapping +// FIRST ("local users always take priority") and nothing in SimpleAuth mirrors AD +// account state — there is no userAccountControl read anywhere — so such a hash +// keeps working after the AD account is disabled or the employee is terminated, +// while the victim's AD password still works (a failed local check falls through +// to the LDAP step) so nothing looks wrong (SA-7). +// +// No single signal is complete, so three are combined: +// +// - OwnerAppID != "" → an app-LOCAL user (M5). Their password genuinely lives +// here and they must keep self-service. Checked first because it is exact. +// - SAMAccountName != "" → written ONLY by syncUserFromLDAP and the LDAP/Kerberos +// JIT paths; no admin or app API exposes the field, so it is a reliable +// positive. Not sufficient alone: POST /api/admin/ldap/import-users creates the +// "ldap" mapping but leaves SAMAccountName empty until the first login. +// - any identity mapping whose provider is neither "local" nor "applocal:" +// — "ldap", "kerberos", or any federated provider an admin attached. Note this +// is deliberately NOT userinfo's plain `!= "local"` test, which would wrongly +// call an app-local customer a directory user. +// +// A user who is BOTH (an admin-created local account later linked to AD) counts as +// directory-backed here. That only bites on the CREATE path — callers consult this +// when the local hash is EMPTY; an existing local hash stays rotatable with proof +// of possession. +// +// NOTE: internal/migrate/bundle.go:classifyUser answers a DIFFERENT question — it +// must produce a portable KEY for a user, not a yes/no verdict — and classifies on +// hash-presence and SAMAccountName rather than this predicate. Do not unify the +// two: internal/migrate importing from internal/handler would invert the +// dependency direction. +func (h *Handler) isDirectoryBacked(user *store.User) bool { + if user.OwnerAppID != "" { + return false + } + if user.SAMAccountName != "" { + return true + } + mappings, err := h.store.GetMappingsForUser(user.GUID) + if err != nil { + // Fail CLOSED. A store error means we cannot PROVE the account is local. The + // cost of a false positive is one refused password change (an admin can still + // set it); the cost of a false negative is a permanent shadow credential on + // an AD identity. + log.Printf("[password] mappings lookup failed guid=%s err=%v — treating as directory-backed", user.GUID, err) + return true + } + for _, m := range mappings { + if m.Provider != "local" && !strings.HasPrefix(m.Provider, "applocal:") { + return true + } + } + return false +} + // resolvePreferredUsername finds the username for a user from identity mappings. // Priority: local mapping > ldap mapping > email > display name. func (h *Handler) resolvePreferredUsername(user *store.User) string { @@ -813,8 +872,54 @@ func (h *Handler) handleResetPassword(w http.ResponseWriter, r *http.Request) { return } - // Verify current password if user has one set (skip if force_password_change is set) - if user.PasswordHash != "" && !user.ForcePasswordChange { + // This endpoint ROTATES an existing SimpleAuth-local password. It must never + // CREATE one. The old guard (`PasswordHash != "" && !ForcePasswordChange`) + // skipped the entire proof-of-possession block whenever the hash was empty — + // which is exactly the state of every directory user, whose credential lives + // in AD — so a bearer token minted for ANY audience could POST + // {"new_password":...} with no current_password and plant a permanent local + // bcrypt hash on an AD-backed record (SA-7). See isDirectoryBacked for why + // that hash then outlives the AD account. + // + // Both refusals are 403, never "supply current_password": a directory user has + // no local password to prove, so demanding one would be an unsatisfiable 400 + // loop rather than an honest "wrong place — change it in AD". + if user.PasswordHash == "" { + ip := getClientIP(r) + if h.isDirectoryBacked(user) { + // Log the GUID, not the resolved username: the GUID is server-generated + // and unspoofable, whereas a directory-supplied username in a security + // log is both injectable and less useful for correlation. + log.Printf("[password] REFUSED directory-backed guid=%s ip=%s (attempt to plant a local password)", + user.GUID, ip) + h.audit("password_change_denied", user.GUID, ip, map[string]interface{}{ + "reason": "directory_backed", + }) + jsonError(w, "password is managed by the directory — change it in Active Directory", http.StatusForbidden) + return + } + // Not directory-backed, but no local credential exists either: an + // admin-created account with no password. There is no first-time-set flow + // to protect here — authenticateUser requires a non-empty hash in every + // local branch, so this user can never log in and can never hold a token of + // their own. Setting the initial password is an administrative act + // (PUT /api/admin/users/{guid}/password). + log.Printf("[password] REFUSED no local password guid=%s ip=%s", user.GUID, ip) + h.audit("password_change_denied", user.GUID, ip, map[string]interface{}{ + "reason": "no_local_password", + }) + jsonError(w, "no local password is set for this account — ask an administrator to set one", http.StatusForbidden) + return + } + + // Proof of possession. ForcePasswordChange is the admin temp-password flow: the + // admin already wrote a REAL hash via PUT /api/admin/users/{guid}/password + // (admin.go writes user.PasswordHash immediately before setting the flag), the + // login response carried force_password_change:true, and the user now changes + // it without re-typing the temp password. That flow always runs with a + // NON-EMPTY hash, which is precisely why the empty-hash gate above can sit + // ahead of it. + if !user.ForcePasswordChange { if req.CurrentPassword == "" { jsonError(w, "current_password required", http.StatusBadRequest) return diff --git a/internal/handler/clientip_test.go b/internal/handler/clientip_test.go new file mode 100644 index 0000000..c8e01df --- /dev/null +++ b/internal/handler/clientip_test.go @@ -0,0 +1,81 @@ +package handler + +import ( + "net" + "net/http/httptest" + "strings" + "testing" +) + +// withTrustedProxy makes 192.0.2.0/24 a trusted proxy range for one test. +// trustedCIDRs is package state set at handler init; without this the forwarded +// headers are never consulted and every assertion below would pass vacuously. +func withTrustedProxy(t *testing.T) { + t.Helper() + _, cidr, err := net.ParseCIDR("192.0.2.0/24") + if err != nil { + t.Fatalf("parse cidr: %v", err) + } + prev := trustedCIDRs + trustedCIDRs = []*net.IPNet{cidr} + t.Cleanup(func() { trustedCIDRs = prev }) +} + +// TestGetClientIPRejectsNonIPForwardedValues pins the log-forgery fix. +// +// getClientIP's result is written into log lines and audit records across the +// whole codebase. It used to return the X-Forwarded-For / X-Real-IP value +// verbatim, so a client behind the trusted proxy could put arbitrary text — +// including newlines — into an "IP" field and forge entries that look like +// genuine security events. A value that is not an IP is meaningless in that +// field anyway, so it must fall back to the real remote address. +func TestGetClientIPRejectsNonIPForwardedValues(t *testing.T) { + withTrustedProxy(t) + cases := []struct { + name string + header string + value string + want string + }{ + {"forged log line via XFF", "X-Forwarded-For", + "1.2.3.4\n[admin] Password set guid=victim force_change=false ip=1.2.3.4", "192.0.2.1"}, + {"forged log line via X-Real-IP", "X-Real-IP", + "9.9.9.9\r\n[auth] Local auth success user=\"root\"", "192.0.2.1"}, + {"plain junk", "X-Forwarded-For", "not-an-ip", "192.0.2.1"}, + {"empty after trim", "X-Forwarded-For", " ", "192.0.2.1"}, + // A real IP must still be honoured — the fix must not break proxy support. + {"legitimate IPv4", "X-Forwarded-For", "203.0.113.7", "203.0.113.7"}, + {"legitimate IPv4 with padding", "X-Forwarded-For", " 203.0.113.7 , 10.0.0.1", "203.0.113.7"}, + {"legitimate IPv6", "X-Real-IP", "2001:db8::1", "2001:db8::1"}, + // Canonicalised by net.IP.String(), so the same client correlates across + // entries — and, crucially, the returned string is built by the stdlib + // rather than sliced out of the header. + {"IPv6 non-canonical spelling", "X-Real-IP", "2001:0db8:0000::0001", "2001:db8::1"}, + } + for _, tc := range cases { + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "192.0.2.1:1234" // httptest's default, inside the trusted range + req.Header.Set(tc.header, tc.value) + + got := getClientIP(req) + if got != tc.want { + t.Errorf("%s: getClientIP = %q, want %q", tc.name, got, tc.want) + } + if strings.ContainsAny(got, "\r\n") { + t.Errorf("%s: client IP carried a newline into a log field: %q", tc.name, got) + } + } +} + +// TestGetClientIPIgnoresUntrustedProxy guards the pre-existing trust gate: the +// forwarded headers are only consulted for a connection from a trusted proxy. +func TestGetClientIPIgnoresUntrustedProxy(t *testing.T) { + withTrustedProxy(t) + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = "203.0.113.99:5555" // not a trusted proxy + req.Header.Set("X-Forwarded-For", "198.51.100.1") + + if got := getClientIP(req); got != "203.0.113.99" { + t.Fatalf("forwarded header honoured from an untrusted peer: got %q", got) + } +} diff --git a/internal/handler/hosted_login.go b/internal/handler/hosted_login.go index ac94e51..a8628e0 100644 --- a/internal/handler/hosted_login.go +++ b/internal/handler/hosted_login.go @@ -327,8 +327,16 @@ func (h *Handler) handleLogout(w http.ResponseWriter, r *http.Request) { // is signed out of every other app that shares this SimpleAuth. h.deleteCurrentSession(w, r) - // Redirect to login with manual=1 to prevent auto-SSO on this page load only + // Redirect to login with manual=1 to prevent auto-SSO on this page load only. + // Carry client_id so the login page that follows resolves the SAME app the + // user just logged out of: GET /login rejects an unknown client_id with 400 + // and validates redirect_uri against that app's OWN allowlist, so dropping it + // dead-ends the documented logout round-trip on a 400 for any app with its own + // redirect_uris. Same defect class as H13 / the client_id round-trip fixes. u := h.url("/login") + "?manual=1" + if clientID := r.URL.Query().Get("client_id"); clientID != "" { + u += "&client_id=" + url.QueryEscape(clientID) + } if redirectURI != "" { u += "&redirect_uri=" + url.QueryEscape(redirectURI) } diff --git a/internal/handler/middleware.go b/internal/handler/middleware.go index 81bb801..954ae61 100644 --- a/internal/handler/middleware.go +++ b/internal/handler/middleware.go @@ -136,7 +136,8 @@ func (rl *rateLimiter) cleanup() { } // trustedCIDRs is set during handler initialization from config.TrustedProxyCIDRs. -// If empty, forwarded headers are trusted from any source (backwards compatible). +// If empty, forwarded headers are trusted from NO source (isTrustedProxy returns +// false), which is what prevents X-Forwarded-For spoofing by default. var trustedCIDRs []*net.IPNet func getClientIP(r *http.Request) string { @@ -144,12 +145,25 @@ func getClientIP(r *http.Request) string { // Only trust forwarded headers if the direct connection is from a trusted proxy if isTrustedProxy(remoteIP, trustedCIDRs) { + // The forwarded value must actually PARSE AS AN IP. It is header content, so + // without this an attacker behind the trusted proxy can put arbitrary text — + // including newlines — into every log line and audit record that carries the + // client IP, forging entries that look like genuine events. An IP field + // holding a non-IP is meaningless anyway, so falling back to the real remote + // address is both safer and more truthful. + // + // Return net.IP.String(), not the header substring: the canonical form is a + // fresh string built by the stdlib from a parsed address, so no header text + // reaches a log or audit sink at all. It also normalises IPv6 spellings, so + // the same client correlates across entries. if xff := r.Header.Get("X-Forwarded-For"); xff != "" { parts := strings.SplitN(xff, ",", 2) - return strings.TrimSpace(parts[0]) + if ip := net.ParseIP(strings.TrimSpace(parts[0])); ip != nil { + return ip.String() + } } - if xri := r.Header.Get("X-Real-IP"); xri != "" { - return xri + if ip := net.ParseIP(strings.TrimSpace(r.Header.Get("X-Real-IP"))); ip != nil { + return ip.String() } } diff --git a/internal/handler/oidc.go b/internal/handler/oidc.go index 8b32cb4..f7b2b67 100644 --- a/internal/handler/oidc.go +++ b/internal/handler/oidc.go @@ -322,29 +322,112 @@ func (h *Handler) issueOIDCCodeRedirect(w http.ResponseWriter, r *http.Request, if state != "" { redirectTarget += "&state=" + url.QueryEscape(state) } + // Re-validate at the SINK. redirectURI is allowlist-checked by every caller + // before we get here, and again below when the code is redeemed — but this is + // the moment an auth code leaves the server, so prove the destination one more + // time rather than trusting that every present and future caller did. + if app, err := h.resolveApp(appID); err != nil || !h.appAllowsRedirect(app, redirectURI) { + log.Printf("[oidc] refusing code delivery to a non-allowlisted redirect_uri app=%q", appID) + http.Error(w, "redirect_uri not allowed", http.StatusBadRequest) + return + } http.Redirect(w, r, redirectTarget, http.StatusFound) } +// oidcAuthzRequest is the set of authorize-request parameters that MUST survive +// every hop of the interactive login flow: +// +// GET /auth?… → hidden form fields → (failed attempt) error redirect → retry POST +// → Kerberos SSO link → /login/sso +// +// Drop one at any hop and the retry silently proceeds with WEAKER parameters than +// the client asked for. That is not hypothetical: renderOIDCLoginError used to +// rebuild the URL by hand with fmt.Sprintf and omitted code_challenge, so one +// mistyped password disabled PKCE for the rest of the login — the retry stored an +// empty challenge and the token endpoint's `if ac.CodeChallenge != ""` guard then +// required no code_verifier at all, undoing M6 (H13). +// +// Keep this type as the single definition of the authorize request: adding a +// parameter here makes every hop carry it. And note what it deliberately is NOT — +// a copy of r.Form. The authorize POST body carries `username` and `password`; +// blanket-copying it into a redirect would put live credentials in a Location +// header, the browser's history, and every proxy log on the path. +type oidcAuthzRequest struct { + ClientID string + RedirectURI string + State string + Nonce string + Scope string + CodeChallenge string + CodeChallengeMethod string + Prompt string +} + +// parseOIDCAuthzRequest reads the authorize parameters from either hop: FormValue +// covers the query string on the GET and the parsed body on the credential POST. +func parseOIDCAuthzRequest(r *http.Request) oidcAuthzRequest { + return oidcAuthzRequest{ + ClientID: r.FormValue("client_id"), + RedirectURI: r.FormValue("redirect_uri"), + State: r.FormValue("state"), + Nonce: r.FormValue("nonce"), + Scope: r.FormValue("scope"), + CodeChallenge: r.FormValue("code_challenge"), + CodeChallengeMethod: r.FormValue("code_challenge_method"), + Prompt: r.FormValue("prompt"), + } +} + +// values renders the request back onto a query string. Empty parameters are +// omitted rather than emitted blank, so the retry URL keeps the shape of the +// original authorize request. +func (a oidcAuthzRequest) values() url.Values { + q := url.Values{} + set := func(k, v string) { + if v != "" { + q.Set(k, v) + } + } + set("client_id", a.ClientID) + set("redirect_uri", a.RedirectURI) + set("state", a.State) + set("nonce", a.Nonce) + set("scope", a.Scope) + // Both halves of PKCE travel together or not at all: a challenge that arrives + // without its method is rejected by the authorize POST (F55/L7), so carrying + // one without the other converts a silent downgrade into a hard 400. + if a.CodeChallenge != "" { + q.Set("code_challenge", a.CodeChallenge) + q.Set("code_challenge_method", a.CodeChallengeMethod) + } + set("prompt", a.Prompt) + return q +} + func (h *Handler) showOIDCLoginPage(w http.ResponseWriter, r *http.Request) { - app, err := h.resolveApp(r.URL.Query().Get("client_id")) + authz := parseOIDCAuthzRequest(r) + + app, err := h.resolveApp(authz.ClientID) if err != nil { http.Error(w, "unknown client", http.StatusBadRequest) return } - redirectURI := r.URL.Query().Get("redirect_uri") + redirectURI := authz.RedirectURI if redirectURI != "" && !h.appAllowsRedirect(app, redirectURI) { http.Error(w, "redirect_uri not allowed", http.StatusBadRequest) return } - state := r.URL.Query().Get("state") - nonce := r.URL.Query().Get("nonce") - scope := r.URL.Query().Get("scope") - codeChallenge := r.URL.Query().Get("code_challenge") - codeChallengeMethod := r.URL.Query().Get("code_challenge_method") + state := authz.State + nonce := authz.Nonce + scope := authz.Scope + codeChallenge := authz.CodeChallenge + codeChallengeMethod := authz.CodeChallengeMethod + prompt := authz.Prompt + // `error` is the error CHANNEL, not part of the authorize request — it is set + // by renderOIDCLoginError and never round-tripped from the client. errorMsg := r.URL.Query().Get("error") - prompt := r.URL.Query().Get("prompt") // Session SSO: if the browser has a valid session cookie AND the client // didn't ask for prompt=login, skip the login page and issue an auth code @@ -369,24 +452,20 @@ func (h *Handler) showOIDCLoginPage(w http.ResponseWriter, r *http.Request) { ssoEnabled := h.getKeytabPath() != "" ssoLink := "" if ssoEnabled { - ssoLink = h.url("/login/sso") + "?oidc=1" - // Carry client_id so handleSSOLogin (auth.go) resolves the INITIATING app - // rather than falling back to the default app — otherwise the SPNEGO path - // mints a wrong-audience token or dead-ends on redirect validation. - ssoLink += "&client_id=" + url.QueryEscape(app.AppID) - if redirectURI != "" { - ssoLink += "&redirect_uri=" + url.QueryEscape(redirectURI) - } - if state != "" { - ssoLink += "&state=" + url.QueryEscape(state) - } - if nonce != "" { - ssoLink += "&nonce=" + url.QueryEscape(nonce) - } - if codeChallenge != "" { - ssoLink += "&code_challenge=" + url.QueryEscape(codeChallenge) - ssoLink += "&code_challenge_method=" + url.QueryEscape(codeChallengeMethod) - } + // Built from the same allowlist as the error redirect, so a parameter + // added to oidcAuthzRequest is carried here automatically instead of + // needing to be remembered at a second hand-concatenated site. + q := authz.values() + q.Set("oidc", "1") + // Carry the RESOLVED client_id so handleSSOLogin (auth.go) resolves the + // INITIATING app rather than falling back to the default app — otherwise + // the SPNEGO path mints a wrong-audience token or dead-ends on redirect + // validation. This deliberately OVERWRITES whatever the client sent, + // exactly as the hidden client_id field below does. + q.Set("client_id", app.AppID) + // prompt is a login-page concept; the SPNEGO endpoint has no use for it. + q.Del("prompt") + ssoLink = h.url("/login/sso") + "?" + q.Encode() } // Only auto-redirect when SSO is enabled, there is no error, and we have not @@ -1077,18 +1156,29 @@ func (h *Handler) handleOIDCLogout(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, `Logged Out

You have been logged out.

`) } -// renderOIDCLoginError redirects back to the OIDC login page with an error. +// renderOIDCLoginError bounces the browser back to SimpleAuth's OWN authorize +// page with an error banner, carrying the WHOLE authorize request with it +// (oidcAuthzRequest) — most importantly the PKCE challenge, which this function +// used to drop (H13, an M6 bypass reachable by mistyping a password once). +// +// Two invariants: +// - The target is always SimpleAuth's own authorize endpoint, never the +// client's redirect_uri. The empty-credentials branch reaches here BEFORE +// redirect_uri has been checked against the app's allowlist, so bouncing to +// it would be an open redirect (the OIDC sibling of F29). showOIDCLoginPage +// re-validates redirect_uri on the way back in. +// - username / password / _csrf are NOT carried. Credentials must never enter +// a URL, and showOIDCLoginPage mints a fresh CSRF token + cookie per render +// (F30). This is why the fix is a typed allowlist rather than a copy of +// r.Form — the request this runs on is a CREDENTIAL POST. func (h *Handler) renderOIDCLoginError(w http.ResponseWriter, r *http.Request, msg string) { + q := parseOIDCAuthzRequest(r).values() + // This endpoint only ever issues codes (response_types_supported: ["code"]). + q.Set("response_type", "code") + q.Set("error", msg) + realm := h.cfg.JWTIssuer - u := fmt.Sprintf("%s/realms/%s/protocol/openid-connect/auth?client_id=%s&redirect_uri=%s&state=%s&nonce=%s&scope=%s&response_type=code&error=%s", - h.cfg.BasePath, realm, - url.QueryEscape(r.FormValue("client_id")), - url.QueryEscape(r.FormValue("redirect_uri")), - url.QueryEscape(r.FormValue("state")), - url.QueryEscape(r.FormValue("nonce")), - url.QueryEscape(r.FormValue("scope")), - url.QueryEscape(msg), - ) + u := h.url("/realms/"+realm+"/protocol/openid-connect/auth") + "?" + q.Encode() http.Redirect(w, r, u, http.StatusFound) } diff --git a/internal/handler/oidc_pkce_test.go b/internal/handler/oidc_pkce_test.go new file mode 100644 index 0000000..4b42ebf --- /dev/null +++ b/internal/handler/oidc_pkce_test.go @@ -0,0 +1,264 @@ +package handler + +import ( + "crypto/sha256" + "encoding/base64" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" +) + +// s256 returns the PKCE S256 challenge for a verifier (RFC 7636 §4.2). +func s256(verifier string) string { + sum := sha256.Sum256([]byte(verifier)) + return base64.RawURLEncoding.EncodeToString(sum[:]) +} + +// mkPKCEApp registers an app with its own redirect allowlist plus a local user, +// and returns the callback URL. +func mkPKCEApp(t *testing.T, h *Handler, appID string) string { + t.Helper() + cb := "https://" + appID + ".example/cb" + w := doJSON(h, "POST", "/api/admin/apps", map[string]interface{}{ + "app_id": appID, "audience": appID, "allow_local_users": true, + "redirect_uris": []string{cb}, + }, adminHeaders()) + if w.Code != http.StatusCreated { + t.Fatalf("create app %s: %d %s", appID, w.Code, w.Body.String()) + } + var app map[string]interface{} + parseJSON(t, w, &app) + doJSON(h, "POST", "/api/app/users", map[string]interface{}{ + "username": "buyer", "password": "buypass1", + }, basicAuth(appID, app["app_secret"].(string))) + return cb +} + +// postAuthz submits the authorize credential form and returns the recorder. +func postAuthz(t *testing.T, h *Handler, authzPath string, form url.Values) *httptest.ResponseRecorder { + t.Helper() + form.Set("_csrf", "tok123") + req := httptest.NewRequest("POST", authzPath, strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.AddCookie(&http.Cookie{Name: "__csrf", Value: "tok123"}) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + return rec +} + +// TestOIDCPKCESurvivesFailedLogin is the H13 regression: mistyping a password +// once must not disable PKCE for the rest of the login. +// +// Before the fix, renderOIDCLoginError rebuilt the authorize URL by hand and +// omitted code_challenge/code_challenge_method. The retry page therefore stamped +// empty hidden fields, the successful retry stored OIDCAuthCode.CodeChallenge="", +// and the token endpoint's `if ac.CodeChallenge != ""` guard became false — so +// the code redeemed with NO code_verifier at all, undoing M6. +func TestOIDCPKCESurvivesFailedLogin(t *testing.T) { + h, _ := testSetup(t) + const realm = "test-issuer" + const authzPath = "/realms/" + realm + "/protocol/openid-connect/auth" + const tokenPath = "/realms/" + realm + "/protocol/openid-connect/token" + cb := mkPKCEApp(t, h, "shop4") + + const verifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" + challenge := s256(verifier) + + // 1. A failed attempt, carrying PKCE. + bad := url.Values{} + bad.Set("client_id", "shop4") + bad.Set("redirect_uri", cb) + bad.Set("scope", "openid") + bad.Set("state", "st8") + bad.Set("nonce", "nc9") + bad.Set("code_challenge", challenge) + bad.Set("code_challenge_method", "S256") + bad.Set("username", "buyer") + bad.Set("password", "WRONG-PASSWORD") + rec := postAuthz(t, h, authzPath, bad) + if rec.Code != http.StatusFound { + t.Fatalf("failed login should redirect, got %d %s", rec.Code, rec.Body.String()) + } + loc := rec.Header().Get("Location") + + // The error redirect must carry PKCE back to the login page. + lu, err := url.Parse(loc) + if err != nil { + t.Fatalf("parse error redirect: %v", err) + } + lq := lu.Query() + if lq.Get("code_challenge") != challenge { + t.Fatalf("error redirect dropped code_challenge (H13): %q", loc) + } + if lq.Get("code_challenge_method") != "S256" { + t.Fatalf("error redirect dropped code_challenge_method: %q", loc) + } + // ...and must NOT carry credentials into a URL. + for _, leak := range []string{"username", "password", "_csrf"} { + if lq.Get(leak) != "" { + t.Fatalf("error redirect leaked %q into the Location header: %q", leak, loc) + } + } + + // 2. Follow the redirect: the retry page must re-stamp the challenge. + greq := httptest.NewRequest("GET", loc, nil) + grec := httptest.NewRecorder() + h.ServeHTTP(grec, greq) + if grec.Code != http.StatusOK { + t.Fatalf("retry page: %d %s", grec.Code, grec.Body.String()) + } + if !strings.Contains(grec.Body.String(), `name="code_challenge" value="`+challenge+`"`) { + t.Fatal("retry page must re-stamp code_challenge into the form (H13)") + } + + // 3. The successful retry — using the fields the retry page actually rendered. + good := url.Values{} + for k, v := range lq { + if k == "error" || k == "response_type" { + continue + } + good.Set(k, v[0]) + } + good.Set("username", "buyer") + good.Set("password", "buypass1") + prec := postAuthz(t, h, authzPath, good) + if prec.Code != http.StatusFound { + t.Fatalf("retry login: %d %s", prec.Code, prec.Body.String()) + } + cbURL, _ := url.Parse(prec.Header().Get("Location")) + code := cbURL.Query().Get("code") + if code == "" { + t.Fatalf("expected an auth code, got %q", prec.Header().Get("Location")) + } + + // 4. THE POINT: that code must still require a verifier. + noVerifier := url.Values{"grant_type": {"authorization_code"}, "code": {code}, "redirect_uri": {cb}} + nreq := httptest.NewRequest("POST", tokenPath, strings.NewReader(noVerifier.Encode())) + nreq.Header.Set("Content-Type", "application/x-www-form-urlencoded") + nrec := httptest.NewRecorder() + h.ServeHTTP(nrec, nreq) + if nrec.Code == http.StatusOK { + t.Fatal("code redeemed with NO code_verifier after a failed login — PKCE was dropped (H13)") + } + + // And the correct verifier must still work. The code above was consumed on + // the failed attempt, so drive a fresh round trip for the positive case. + good2 := url.Values{} + for k, v := range good { + good2.Set(k, v[0]) + } + prec2 := postAuthz(t, h, authzPath, good2) + cbURL2, _ := url.Parse(prec2.Header().Get("Location")) + code2 := cbURL2.Query().Get("code") + if code2 == "" { + t.Fatalf("second round trip produced no code: %q", prec2.Header().Get("Location")) + } + withVerifier := url.Values{ + "grant_type": {"authorization_code"}, "code": {code2}, + "redirect_uri": {cb}, "code_verifier": {verifier}, + } + vreq := httptest.NewRequest("POST", tokenPath, strings.NewReader(withVerifier.Encode())) + vreq.Header.Set("Content-Type", "application/x-www-form-urlencoded") + vrec := httptest.NewRecorder() + h.ServeHTTP(vrec, vreq) + if vrec.Code != http.StatusOK { + t.Fatalf("correct code_verifier must succeed, got %d %s", vrec.Code, vrec.Body.String()) + } +} + +// TestOIDCLoginErrorPreservesAuthorizeRequest pins the whole allowlist, not just +// PKCE — every parameter the client sent must survive the error hop, so a future +// parameter added to oidcAuthzRequest cannot be silently dropped at this hop. +func TestOIDCLoginErrorPreservesAuthorizeRequest(t *testing.T) { + h, _ := testSetup(t) + const authzPath = "/realms/test-issuer/protocol/openid-connect/auth" + cb := mkPKCEApp(t, h, "shop5") + + form := url.Values{} + form.Set("client_id", "shop5") + form.Set("redirect_uri", cb) + form.Set("state", "the-state") + form.Set("nonce", "the-nonce") + form.Set("scope", "openid email") + form.Set("code_challenge", s256("v")) + form.Set("code_challenge_method", "S256") + form.Set("username", "buyer") + form.Set("password", "nope") + + rec := postAuthz(t, h, authzPath, form) + lu, err := url.Parse(rec.Header().Get("Location")) + if err != nil { + t.Fatalf("parse: %v", err) + } + q := lu.Query() + for _, k := range []string{"client_id", "redirect_uri", "state", "nonce", "scope", "code_challenge", "code_challenge_method"} { + if q.Get(k) != form.Get(k) { + t.Errorf("error redirect dropped or altered %q: want %q, got %q", k, form.Get(k), q.Get(k)) + } + } + if q.Get("error") == "" { + t.Error("error redirect must carry the error message") + } + // The target is SimpleAuth's own authorize endpoint, never the client's + // redirect_uri — the empty-credentials branch reaches here before redirect_uri + // is allowlist-checked, so bouncing to it would be an open redirect. + if !strings.Contains(lu.Path, "/protocol/openid-connect/auth") { + t.Errorf("error must return to SimpleAuth's authorize page, got %q", lu.Path) + } +} + +// TestOIDCLoginErrorWithNoCredentials covers the branch that reaches +// renderOIDCLoginError BEFORE redirect_uri has been validated: it must still +// land on SimpleAuth's own page and must not reflect an unvalidated destination. +func TestOIDCLoginErrorWithNoCredentials(t *testing.T) { + h, _ := testSetup(t) + const authzPath = "/realms/test-issuer/protocol/openid-connect/auth" + mkPKCEApp(t, h, "shop6") + + form := url.Values{} + form.Set("client_id", "shop6") + form.Set("redirect_uri", "https://evil.example/steal") + // username and password deliberately absent + rec := postAuthz(t, h, authzPath, form) + if rec.Code != http.StatusFound { + t.Fatalf("expected redirect, got %d", rec.Code) + } + loc := rec.Header().Get("Location") + if strings.HasPrefix(loc, "https://evil.example") { + t.Fatalf("open redirect: bounced to an unvalidated redirect_uri: %q", loc) + } + if !strings.Contains(loc, "/protocol/openid-connect/auth") { + t.Fatalf("expected SimpleAuth's own authorize page, got %q", loc) + } +} + +// TestLogoutPreservesClientID pins the handleLogout half of the same defect +// class: GET /login 400s on an unknown client_id and validates redirect_uri +// against THAT app's allowlist, so dropping client_id dead-ends the documented +// logout round trip for any app with its own redirect_uris. +func TestLogoutPreservesClientID(t *testing.T) { + h, _ := testSetup(t) + cb := mkPKCEApp(t, h, "shop7") + + req := httptest.NewRequest("GET", "/logout?client_id=shop7&redirect_uri="+url.QueryEscape(cb), nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusFound { + t.Fatalf("logout should redirect, got %d", rec.Code) + } + loc := rec.Header().Get("Location") + if !strings.Contains(loc, "client_id=shop7") { + t.Fatalf("logout must carry client_id onto the login page, got %q", loc) + } + + // And the resulting login page must actually render (not 400) — the whole + // point: the app's own redirect_uri is only allowlisted for the app itself. + greq := httptest.NewRequest("GET", loc, nil) + grec := httptest.NewRecorder() + h.ServeHTTP(grec, greq) + if grec.Code != http.StatusOK { + t.Fatalf("post-logout login page must render, got %d %s", grec.Code, grec.Body.String()) + } +} diff --git a/internal/handler/reset_password_test.go b/internal/handler/reset_password_test.go new file mode 100644 index 0000000..183063f --- /dev/null +++ b/internal/handler/reset_password_test.go @@ -0,0 +1,252 @@ +package handler + +import ( + "net/http" + "testing" + + "simpleauth/internal/store" +) + +// seedDirectoryUser creates a user in the exact end state the LDAP/Kerberos JIT +// paths leave behind: empty PasswordHash, SAMAccountName set, and BOTH an "ldap" +// and a "local" identity mapping (auth.go creates both so future lookups by +// either name resolve). +func seedDirectoryUser(t *testing.T, s store.Store, username string) *store.User { + t.Helper() + u := &store.User{DisplayName: username, SAMAccountName: username} + if err := s.CreateUser(u); err != nil { + t.Fatalf("create directory user: %v", err) + } + if err := s.SetIdentityMapping("ldap", username, u.GUID); err != nil { + t.Fatalf("set ldap mapping: %v", err) + } + if err := s.SetIdentityMapping("local", username, u.GUID); err != nil { + t.Fatalf("set local mapping: %v", err) + } + return u +} + +// impersonationToken mints an access token for an arbitrary user via master-admin +// POST /api/auth/impersonate. There is no LDAP server in unit tests, so this is +// how a test obtains a bearer token for a directory-backed user — and it is also +// a faithful model of the threat: the attacker holds SOME token for the victim. +func impersonationToken(t *testing.T, h *Handler, guid string) map[string]string { + t.Helper() + w := doJSON(h, "POST", "/api/auth/impersonate", map[string]interface{}{ + "target_guid": guid, + }, adminHeaders()) + if w.Code != http.StatusOK { + t.Fatalf("impersonate: %d %s", w.Code, w.Body.String()) + } + var tok map[string]interface{} + parseJSON(t, w, &tok) + access, _ := tok["access_token"].(string) + if access == "" { + t.Fatalf("impersonate returned no access_token: %v", tok) + } + return map[string]string{"Authorization": "Bearer " + access} +} + +// TestResetPasswordRefusesDirectoryUser is the SA-7 regression, driven as the +// full exploit chain: plant, then try to use. +// +// Before the fix, `PasswordHash != "" && !ForcePasswordChange` skipped the entire +// proof-of-possession block for any user with an empty hash — the state of every +// directory user — so a bearer token alone wrote a permanent local bcrypt hash +// onto an AD-backed record. authenticateUser resolves the "local" mapping FIRST, +// so that hash then outlives AD disablement and account termination. +func TestResetPasswordRefusesDirectoryUser(t *testing.T) { + h, s := testSetup(t) + victim := seedDirectoryUser(t, s, "alice") + bearer := impersonationToken(t, h, victim.GUID) + + // The plant: no current_password, because there is no local password to prove. + w := doJSON(h, "POST", "/api/auth/reset-password", map[string]interface{}{ + "new_password": "Attacker1!", + }, bearer) + if w.Code != http.StatusForbidden { + t.Fatalf("planting a local password on a directory user must be 403, got %d %s", w.Code, w.Body.String()) + } + + // Nothing was written. + after, err := s.GetUser(victim.GUID) + if err != nil { + t.Fatalf("reload: %v", err) + } + if after.PasswordHash != "" { + t.Fatal("a local password hash was written to a directory-backed user (SA-7)") + } + + // And the credential does not work — the part that actually matters. + lw := doJSON(h, "POST", "/api/auth/login", map[string]interface{}{ + "username": "alice", "password": "Attacker1!", + }, nil) + if lw.Code == http.StatusOK { + t.Fatal("planted credential authenticated — the shadow credential survived (SA-7)") + } +} + +// TestResetPasswordRefusesAccountWithNoLocalPassword covers the other empty-hash +// case: an admin-created account that never had a password. Such a user can never +// log in (authenticateUser requires a non-empty hash), so there is no legitimate +// first-time-set flow through this endpoint to protect. +func TestResetPasswordRefusesAccountWithNoLocalPassword(t *testing.T) { + h, s := testSetup(t) + u := &store.User{DisplayName: "No Password"} + if err := s.CreateUser(u); err != nil { + t.Fatalf("create: %v", err) + } + if err := s.SetIdentityMapping("local", "nopass", u.GUID); err != nil { + t.Fatalf("map: %v", err) + } + bearer := impersonationToken(t, h, u.GUID) + + w := doJSON(h, "POST", "/api/auth/reset-password", map[string]interface{}{ + "new_password": "Whatever1!", + }, bearer) + if w.Code != http.StatusForbidden { + t.Fatalf("want 403 for an account with no local password, got %d %s", w.Code, w.Body.String()) + } +} + +// TestResetPasswordLocalUserStillWorks is the most important not-broken test in +// this change: a genuine local user must still rotate their own password with +// proof of possession, and must still be refused without it. +func TestResetPasswordLocalUserStillWorks(t *testing.T) { + h, s := testSetup(t) + w := doJSON(h, "POST", "/api/admin/users", map[string]interface{}{ + "display_name": "Local Larry", "password": "oldpass1", + }, adminHeaders()) + if w.Code != http.StatusCreated && w.Code != http.StatusOK { + t.Fatalf("create: %d %s", w.Code, w.Body.String()) + } + var u map[string]interface{} + parseJSON(t, w, &u) + guid := u["guid"].(string) + if err := s.SetIdentityMapping("local", "larry", guid); err != nil { + t.Fatalf("map: %v", err) + } + + lw := doJSON(h, "POST", "/api/auth/login", map[string]interface{}{ + "username": "larry", "password": "oldpass1", + }, nil) + if lw.Code != http.StatusOK { + t.Fatalf("login: %d %s", lw.Code, lw.Body.String()) + } + var tok map[string]interface{} + parseJSON(t, lw, &tok) + bearer := map[string]string{"Authorization": "Bearer " + tok["access_token"].(string)} + + // Without current_password -> 400, unchanged behaviour. + if w := doJSON(h, "POST", "/api/auth/reset-password", map[string]interface{}{ + "new_password": "newpass1", + }, bearer); w.Code != http.StatusBadRequest { + t.Fatalf("local user without current_password must be 400, got %d %s", w.Code, w.Body.String()) + } + // Wrong current_password -> 403. + if w := doJSON(h, "POST", "/api/auth/reset-password", map[string]interface{}{ + "current_password": "wrong", "new_password": "newpass1", + }, bearer); w.Code != http.StatusForbidden { + t.Fatalf("wrong current_password must be 403, got %d", w.Code) + } + // Correct current_password -> 200, and the new password works. + if w := doJSON(h, "POST", "/api/auth/reset-password", map[string]interface{}{ + "current_password": "oldpass1", "new_password": "newpass1", + }, bearer); w.Code != http.StatusOK { + t.Fatalf("valid rotation must succeed, got %d %s", w.Code, w.Body.String()) + } + if w := doJSON(h, "POST", "/api/auth/login", map[string]interface{}{ + "username": "larry", "password": "newpass1", + }, nil); w.Code != http.StatusOK { + t.Fatalf("rotated password must authenticate, got %d", w.Code) + } +} + +// TestResetPasswordForceChangeStillWorks pins the admin temp-password flow. The +// whole safety of putting the empty-hash gate AHEAD of the force-change branch +// rests on ForcePasswordChange never coexisting with an empty hash — admin.go +// writes a real hash immediately before setting the flag. This asserts it. +func TestResetPasswordForceChangeStillWorks(t *testing.T) { + h, s := testSetup(t) + w := doJSON(h, "POST", "/api/admin/users", map[string]interface{}{ + "display_name": "Temp Tina", "password": "temppass1", + }, adminHeaders()) + var u map[string]interface{} + parseJSON(t, w, &u) + guid := u["guid"].(string) + if err := s.SetIdentityMapping("local", "tina", guid); err != nil { + t.Fatalf("map: %v", err) + } + + // Admin sets a temp password with force_change. + if w := doJSON(h, "PUT", "/api/admin/users/"+guid+"/password", map[string]interface{}{ + "password": "temppass2", "force_change": true, + }, adminHeaders()); w.Code != http.StatusOK { + t.Fatalf("admin set password: %d %s", w.Code, w.Body.String()) + } + su, err := s.GetUser(guid) + if err != nil { + t.Fatalf("reload: %v", err) + } + if su.PasswordHash == "" { + t.Fatal("invariant broken: ForcePasswordChange set with an EMPTY hash — the empty-hash gate would break this flow") + } + + lw := doJSON(h, "POST", "/api/auth/login", map[string]interface{}{ + "username": "tina", "password": "temppass2", + }, nil) + if lw.Code != http.StatusOK { + t.Fatalf("login with temp password: %d %s", lw.Code, lw.Body.String()) + } + var tok map[string]interface{} + parseJSON(t, lw, &tok) + bearer := map[string]string{"Authorization": "Bearer " + tok["access_token"].(string)} + + // The point: change WITHOUT re-typing the temp password must still work. + if w := doJSON(h, "POST", "/api/auth/reset-password", map[string]interface{}{ + "new_password": "chosen99", + }, bearer); w.Code != http.StatusOK { + t.Fatalf("force-change flow must not require current_password, got %d %s", w.Code, w.Body.String()) + } +} + +// TestIsDirectoryBackedClassification pins the predicate itself, including the +// app-local carve-out that a naive `provider != "local"` test would get wrong. +func TestIsDirectoryBackedClassification(t *testing.T) { + h, s := testSetup(t) + + mk := func(name string, u *store.User, provider string) *store.User { + t.Helper() + if err := s.CreateUser(u); err != nil { + t.Fatalf("create %s: %v", name, err) + } + if provider != "" { + if err := s.SetIdentityMapping(provider, name, u.GUID); err != nil { + t.Fatalf("map %s: %v", name, err) + } + } + return u + } + + cases := []struct { + name string + user *store.User + prov string + want bool + }{ + {"plain local user", &store.User{DisplayName: "L"}, "local", false}, + {"app-local user", &store.User{DisplayName: "A", OwnerAppID: "billing"}, "applocal:billing", false}, + {"ldap-mapped, no SAMAccountName (import path)", &store.User{DisplayName: "I"}, "ldap", true}, + {"SAMAccountName set", &store.User{DisplayName: "S", SAMAccountName: "sam"}, "local", true}, + {"kerberos-mapped", &store.User{DisplayName: "K"}, "kerberos", true}, + // An app-local user whose OwnerAppID wins over a SAMAccountName that should + // never be there — the short-circuit must be exact, not best-effort. + {"app-local beats stray SAMAccountName", &store.User{DisplayName: "X", OwnerAppID: "shop", SAMAccountName: "stray"}, "applocal:shop", false}, + } + for _, tc := range cases { + u := mk(tc.name, tc.user, tc.prov) + if got := h.isDirectoryBacked(u); got != tc.want { + t.Errorf("%s: isDirectoryBacked = %v, want %v", tc.name, got, tc.want) + } + } +} diff --git a/internal/store/bolt.go b/internal/store/bolt.go index a798e40..df6da74 100644 --- a/internal/store/bolt.go +++ b/internal/store/bolt.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "io" + "log" "os" "path/filepath" "sort" @@ -76,6 +77,9 @@ func OpenBolt(dataDir string) (*BoltStore, error) { return nil, err } s.migrateRolesAndPermissions() + // Prune reverse-index entries a pre-H14 SetIdentityMapping left stranded on a + // previous owner. Cheap (one pass over the index) and idempotent. + s.repairMappingIndex() return s, nil } @@ -172,7 +176,17 @@ func (s *BoltStore) DeleteApp(appID string) error { var mappings []IdentityMapping if json.Unmarshal(data, &mappings) == nil { for _, m := range mappings { - if err := tx.Bucket(bucketIdentityMappings).Delete(mappingKey(m.Provider, m.ExternalID)); err != nil { + mk := mappingKey(m.Provider, m.ExternalID) + // Only delete a forward entry this user still actually owns. + // The reverse index is derived state; if it ever drifts again, + // the H8 cascade must not delete a live mapping belonging to + // somebody else (H14). Postgres cascades by + // `WHERE user_guid IN (...)`, which is inherently owner-scoped — + // this makes Bolt identical. + if owner := tx.Bucket(bucketIdentityMappings).Get(mk); owner == nil || string(owner) != guid { + continue + } + if err := tx.Bucket(bucketIdentityMappings).Delete(mk); err != nil { return err } } @@ -512,14 +526,86 @@ func mappingKey(provider, externalID string) []byte { return []byte(provider + ":" + externalID) } +// mappingSplits builds forwardKey -> {Provider, ExternalID} from the reverse +// index, which records both halves verbatim. +// +// The Bolt forward key is the composite "provider:externalID" and BOTH halves may +// contain ':' — app-local users are keyed under the provider "applocal:", +// and handleSetMapping accepts an arbitrary provider string — so splitting on the +// first ':' is guesswork. Postgres keeps the halves in separate columns and never +// has to guess; this is how Bolt matches it (M38). +func mappingSplits(tx *bolt.Tx) map[string]IdentityMapping { + out := map[string]IdentityMapping{} + idx := tx.Bucket(bucketIdxMappingsByGUID) + if idx == nil { + return out + } + _ = idx.ForEach(func(_, v []byte) error { + var mappings []IdentityMapping + if json.Unmarshal(v, &mappings) != nil { + return nil + } + for _, m := range mappings { + out[string(mappingKey(m.Provider, m.ExternalID))] = m + } + return nil + }) + return out +} + +// splitMappingKey decomposes a forward key, preferring the reverse index and +// falling back to the first ':' only for a key the index does not cover (which +// only happens in already-corrupt data — fall back rather than drop the row). +func splitMappingKey(key string, known map[string]IdentityMapping) (IdentityMapping, bool) { + if m, ok := known[key]; ok { + return m, true + } + i := strings.Index(key, ":") + if i < 0 { + return IdentityMapping{}, false + } + return IdentityMapping{Provider: key[:i], ExternalID: key[i+1:]}, true +} + +// SetIdentityMapping points provider:externalID at userGUID, re-pointing the +// mapping if it currently resolves to somebody else. +// +// The forward bucket is authoritative and bucketIdxMappingsByGUID is only a +// derived reverse index, so a re-point MUST retract the entry from the previous +// owner in the SAME transaction. Previously only addMappingToIndex ran, so after +// an admin created a local account under a username an LDAP JIT user already +// owned, BOTH GUIDs claimed it. The loser kept a phantom {local,} that +// resolvePreferredUsername stamped into every access and ID token as the standard +// `preferred_username` claim — handing an RP that authorizes on that claim the +// wrong user — and that the delete cascades (handleDeleteLocalUser, DeleteApp) +// followed to delete the REAL owner's live mapping (H14). +// +// Postgres has no reverse index — ON CONFLICT (provider, external_id) DO UPDATE +// already makes the single row the whole truth — so this restores backend parity. func (s *BoltStore) SetIdentityMapping(provider, externalID, userGUID string) error { return s.update(func(tx *bolt.Tx) error { key := mappingKey(provider, externalID) + m := IdentityMapping{Provider: provider, ExternalID: externalID} + + // Read the incumbent BEFORE the Put. bbolt hands back a slice into the + // mmap'd page, which Put may invalidate, so copy it out with string(). + var prevOwner string + if v := tx.Bucket(bucketIdentityMappings).Get(key); v != nil { + prevOwner = string(v) + } if err := tx.Bucket(bucketIdentityMappings).Put(key, []byte(userGUID)); err != nil { return err } - // Update reverse index - return s.addMappingToIndex(tx, userGUID, IdentityMapping{Provider: provider, ExternalID: externalID}) + // Retract from the loser first, then index the winner. The + // prevOwner == userGUID case is skipped outright: a naive remove-then-add + // on the same GUID would strip the entry addMappingToIndex just wrote, and + // re-setting the same mapping must stay idempotent. + if prevOwner != "" && prevOwner != userGUID { + if err := s.removeMappingFromIndex(tx, prevOwner, m); err != nil { + return err + } + } + return s.addMappingToIndex(tx, userGUID, m) }) } @@ -583,6 +669,99 @@ func (s *BoltStore) addMappingToIndex(tx *bolt.Tx, userGUID string, m IdentityMa return tx.Bucket(bucketIdxMappingsByGUID).Put([]byte(userGUID), newData) } +// repairMappingIndex prunes reverse-index entries the forward bucket no longer +// backs. Fixing the writer does not clean data a deployment already corrupted, +// and a single orphan entry is enough on its own to stamp another user's +// `preferred_username` into live tokens and to make the delete cascades destroy +// the real owner's mapping — so repair on open, alongside +// migrateRolesAndPermissions (H14). +// +// PRUNE ONLY, never rebuild. Reconstructing index entries from forward keys would +// have to re-split the ambiguous composite key and would corrupt the exactly +// recorded "applocal:" providers the index already holds correctly. Every +// writer adds forward + index in one transaction (SetIdentityMapping, MergeUsers) +// and every deleter removes both, so "index entry with no matching forward owner" +// is the only corruption this bug can produce. +// +// Each pruned claim is logged individually, not just counted: this deletes +// identity data at startup on data we have never seen, so the log must be +// sufficient to reconstruct by hand what was removed. Set +// SA_SKIP_MAPPING_REPAIR=1 to report without writing — an operator who sees an +// unexpected prune count can use it to inspect before committing. +func (s *BoltStore) repairMappingIndex() { + type change struct { + guid string + kept []IdentityMapping + dropped []IdentityMapping + } + var changes []change + + // Collect under a read tx; bbolt forbids mutating a bucket mid-ForEach. + _ = s.view(func(tx *bolt.Tx) error { + fwd := tx.Bucket(bucketIdentityMappings) + return tx.Bucket(bucketIdxMappingsByGUID).ForEach(func(k, v []byte) error { + var mappings []IdentityMapping + if json.Unmarshal(v, &mappings) != nil { + return nil + } + guid := string(k) // k is only valid inside the callback + kept := make([]IdentityMapping, 0, len(mappings)) + var dropped []IdentityMapping + for _, m := range mappings { + if owner := fwd.Get(mappingKey(m.Provider, m.ExternalID)); owner != nil && string(owner) == guid { + kept = append(kept, m) + } else { + dropped = append(dropped, m) + } + } + if len(dropped) > 0 { + changes = append(changes, change{guid: guid, kept: kept, dropped: dropped}) + } + return nil + }) + }) + if len(changes) == 0 { + return + } + + // Log the full plan BEFORE writing, so the record survives even if the write + // fails or the result turns out to be wrong. + for _, c := range changes { + for _, m := range c.dropped { + log.Printf("[store] mapping-index repair: user=%s no longer owns %s:%s — pruning stale claim (H14)", + c.guid, m.Provider, m.ExternalID) + } + } + if os.Getenv("SA_SKIP_MAPPING_REPAIR") == "1" { + log.Printf("[store] mapping-index repair: SA_SKIP_MAPPING_REPAIR=1 — reported %d user(s), no changes written", len(changes)) + return + } + + if err := s.update(func(tx *bolt.Tx) error { + b := tx.Bucket(bucketIdxMappingsByGUID) + for _, c := range changes { + if len(c.kept) == 0 { + if err := b.Delete([]byte(c.guid)); err != nil { + return err + } + continue + } + data, err := json.Marshal(c.kept) + if err != nil { + return err + } + if err := b.Put([]byte(c.guid), data); err != nil { + return err + } + } + return nil + }); err != nil { + log.Printf("[store] mapping-index repair FAILED: %v", err) + return + } + log.Printf("[store] mapping-index repair: pruned stale claims for %d user(s) (H14)", len(changes)) +} + func (s *BoltStore) removeMappingFromIndex(tx *bolt.Tx, userGUID string, m IdentityMapping) error { var mappings []IdentityMapping data := tx.Bucket(bucketIdxMappingsByGUID).Get([]byte(userGUID)) @@ -610,16 +789,18 @@ func (s *BoltStore) removeMappingFromIndex(tx *bolt.Tx, userGUID string, m Ident func (s *BoltStore) ListAllMappings() ([]IdentityMappingEntry, error) { var result []IdentityMappingEntry err := s.view(func(tx *bolt.Tx) error { - b := tx.Bucket(bucketIdentityMappings) - return b.ForEach(func(k, v []byte) error { - key := string(k) - idx := strings.Index(key, ":") - if idx < 0 { + // Decompose via the reverse index rather than guessing at the first ':', + // so an "applocal:" provider round-trips intact and matches what + // Postgres reports from its two columns (M38). + known := mappingSplits(tx) + return tx.Bucket(bucketIdentityMappings).ForEach(func(k, v []byte) error { + m, ok := splitMappingKey(string(k), known) + if !ok { return nil } result = append(result, IdentityMappingEntry{ - Provider: key[:idx], - ExternalID: key[idx+1:], + Provider: m.Provider, + ExternalID: m.ExternalID, UserGUID: string(v), }) return nil diff --git a/internal/store/mapping_index_test.go b/internal/store/mapping_index_test.go new file mode 100644 index 0000000..e2f57b0 --- /dev/null +++ b/internal/store/mapping_index_test.go @@ -0,0 +1,298 @@ +package store + +import ( + "encoding/json" + "os" + "testing" + + bolt "go.etcd.io/bbolt" +) + +// openTestBolt returns a concrete *BoltStore (openTestStore returns the Store +// interface, and these tests reach into bucket internals). +func openTestBolt(t *testing.T, dir string) *BoltStore { + t.Helper() + s, err := OpenBolt(dir) + if err != nil { + t.Fatalf("OpenBolt: %v", err) + } + t.Cleanup(func() { s.Close() }) + return s +} + +// indexClaims returns the mappings the reverse index attributes to a GUID. +func indexClaims(t *testing.T, s *BoltStore, guid string) []IdentityMapping { + t.Helper() + var out []IdentityMapping + err := s.view(func(tx *bolt.Tx) error { + data := tx.Bucket(bucketIdxMappingsByGUID).Get([]byte(guid)) + if data == nil { + return nil + } + return json.Unmarshal(data, &out) + }) + if err != nil { + t.Fatalf("read index: %v", err) + } + return out +} + +// TestSetIdentityMappingRetractsFromPreviousOwner is the H14 regression. +// +// Re-pointing a mapping used to leave the loser's reverse-index entry in place, +// so BOTH GUIDs claimed the username. resolvePreferredUsername reads the reverse +// index, so the loser kept stamping the winner's username into every token as the +// standard `preferred_username` claim. +func TestSetIdentityMappingRetractsFromPreviousOwner(t *testing.T) { + s := openTestBolt(t, t.TempDir()) + + u1 := &User{DisplayName: "Alice (LDAP JIT)"} + if err := s.CreateUser(u1); err != nil { + t.Fatalf("create u1: %v", err) + } + u2 := &User{DisplayName: "Alice (admin-created)"} + if err := s.CreateUser(u2); err != nil { + t.Fatalf("create u2: %v", err) + } + + // u1 is JIT-provisioned: owns both ldap:alice and local:alice. + if err := s.SetIdentityMapping("ldap", "alice", u1.GUID); err != nil { + t.Fatalf("set ldap: %v", err) + } + if err := s.SetIdentityMapping("local", "alice", u1.GUID); err != nil { + t.Fatalf("set local: %v", err) + } + // An admin later creates a local account for the same name -> re-point. + if err := s.SetIdentityMapping("local", "alice", u2.GUID); err != nil { + t.Fatalf("re-point local: %v", err) + } + + // Forward map is authoritative and must name u2. + if guid, err := s.ResolveMapping("local", "alice"); err != nil || guid != u2.GUID { + t.Fatalf("forward map: want %s, got %q (err %v)", u2.GUID, guid, err) + } + + // u1 must NO LONGER claim local:alice — this is the bug. + for _, m := range indexClaims(t, s, u1.GUID) { + if m.Provider == "local" && m.ExternalID == "alice" { + t.Fatal("previous owner still claims local:alice in the reverse index (H14)") + } + } + // u1 keeps its own untouched mapping. + var keptLDAP bool + for _, m := range indexClaims(t, s, u1.GUID) { + if m.Provider == "ldap" && m.ExternalID == "alice" { + keptLDAP = true + } + } + if !keptLDAP { + t.Fatal("re-pointing local:alice must not disturb u1's ldap:alice mapping") + } + // u2 claims it exactly once. + var count int + for _, m := range indexClaims(t, s, u2.GUID) { + if m.Provider == "local" && m.ExternalID == "alice" { + count++ + } + } + if count != 1 { + t.Fatalf("new owner should claim local:alice exactly once, got %d", count) + } +} + +// TestSetIdentityMappingIdempotent guards the fix against over-correction: a +// naive remove-then-add on the SAME guid would strip the entry addMappingToIndex +// just wrote. (Mirrors TestSetIdentityMapping_Duplicate at the index level.) +func TestSetIdentityMappingIdempotent(t *testing.T) { + s := openTestBolt(t, t.TempDir()) + u := &User{DisplayName: "Bob"} + if err := s.CreateUser(u); err != nil { + t.Fatalf("create: %v", err) + } + for i := 0; i < 3; i++ { + if err := s.SetIdentityMapping("local", "bob", u.GUID); err != nil { + t.Fatalf("set #%d: %v", i, err) + } + } + claims := indexClaims(t, s, u.GUID) + if len(claims) != 1 { + t.Fatalf("re-setting the same mapping must leave exactly 1 index entry, got %d: %+v", len(claims), claims) + } + if guid, err := s.ResolveMapping("local", "bob"); err != nil || guid != u.GUID { + t.Fatalf("forward map broken: %q err=%v", guid, err) + } +} + +// TestListAllMappingsPreservesCompositeProvider is the M38 regression: an +// app-local provider is itself "applocal:", so splitting the composite +// forward key on the FIRST ':' reported provider="applocal" and smuggled the app +// id into the external id — diverging from Postgres, which stores two columns. +func TestListAllMappingsPreservesCompositeProvider(t *testing.T) { + s := openTestBolt(t, t.TempDir()) + u := &User{DisplayName: "Tenant User", OwnerAppID: "billing"} + if err := s.CreateUser(u); err != nil { + t.Fatalf("create: %v", err) + } + if err := s.SetIdentityMapping("applocal:billing", "bob", u.GUID); err != nil { + t.Fatalf("set: %v", err) + } + + entries, err := s.ListAllMappings() + if err != nil { + t.Fatalf("ListAllMappings: %v", err) + } + var found bool + for _, e := range entries { + if e.UserGUID != u.GUID { + continue + } + found = true + if e.Provider != "applocal:billing" || e.ExternalID != "bob" { + t.Fatalf("composite provider corrupted: provider=%q external_id=%q (want applocal:billing / bob)", e.Provider, e.ExternalID) + } + } + if !found { + t.Fatal("mapping missing from ListAllMappings") + } +} + +// TestRepairMappingIndexPrunesStaleClaims covers the existing-data repair: a +// deployment corrupted by the pre-fix writer must be cleaned on open, and a +// healthy index must be left completely alone. +func TestRepairMappingIndexPrunesStaleClaims(t *testing.T) { + dir := t.TempDir() + s := openTestBolt(t, dir) + + u1 := &User{DisplayName: "Loser"} + u2 := &User{DisplayName: "Winner"} + if err := s.CreateUser(u1); err != nil { + t.Fatalf("create u1: %v", err) + } + if err := s.CreateUser(u2); err != nil { + t.Fatalf("create u2: %v", err) + } + if err := s.SetIdentityMapping("local", "carol", u2.GUID); err != nil { + t.Fatalf("set: %v", err) + } + // u1 legitimately owns something else, which must survive the repair. + if err := s.SetIdentityMapping("ldap", "carol", u1.GUID); err != nil { + t.Fatalf("set ldap: %v", err) + } + + // Inject exactly the corruption the old writer produced: u1 claims local:carol + // in the reverse index while the forward map names u2. + if err := s.update(func(tx *bolt.Tx) error { + return s.addMappingToIndex(tx, u1.GUID, IdentityMapping{Provider: "local", ExternalID: "carol"}) + }); err != nil { + t.Fatalf("inject: %v", err) + } + if len(indexClaims(t, s, u1.GUID)) != 2 { + t.Fatal("setup: expected the injected stale claim to be present") + } + s.Close() + + // Reopen — repairMappingIndex runs in OpenBolt. + s2 := openTestBolt(t, dir) + for _, m := range indexClaims(t, s2, u1.GUID) { + if m.Provider == "local" && m.ExternalID == "carol" { + t.Fatal("repair did not prune the stale claim (H14)") + } + } + // The legitimate claim survived. + var keptOwn bool + for _, m := range indexClaims(t, s2, u1.GUID) { + if m.Provider == "ldap" && m.ExternalID == "carol" { + keptOwn = true + } + } + if !keptOwn { + t.Fatal("repair pruned a mapping the user legitimately owns") + } + // The real owner is untouched. + if len(indexClaims(t, s2, u2.GUID)) != 1 { + t.Fatalf("repair disturbed the real owner: %+v", indexClaims(t, s2, u2.GUID)) + } +} + +// TestRepairMappingIndexRespectsSkipEnv pins the escape hatch: an operator who +// sees an unexpected prune count must be able to inspect before committing to it. +func TestRepairMappingIndexRespectsSkipEnv(t *testing.T) { + dir := t.TempDir() + s := openTestBolt(t, dir) + u1 := &User{DisplayName: "Loser"} + u2 := &User{DisplayName: "Winner"} + if err := s.CreateUser(u1); err != nil { + t.Fatalf("create u1: %v", err) + } + if err := s.CreateUser(u2); err != nil { + t.Fatalf("create u2: %v", err) + } + if err := s.SetIdentityMapping("local", "dave", u2.GUID); err != nil { + t.Fatalf("set: %v", err) + } + if err := s.update(func(tx *bolt.Tx) error { + return s.addMappingToIndex(tx, u1.GUID, IdentityMapping{Provider: "local", ExternalID: "dave"}) + }); err != nil { + t.Fatalf("inject: %v", err) + } + s.Close() + + t.Setenv("SA_SKIP_MAPPING_REPAIR", "1") + if os.Getenv("SA_SKIP_MAPPING_REPAIR") != "1" { + t.Fatal("env not set") + } + s2 := openTestBolt(t, dir) + var stillThere bool + for _, m := range indexClaims(t, s2, u1.GUID) { + if m.Provider == "local" && m.ExternalID == "dave" { + stillThere = true + } + } + if !stillThere { + t.Fatal("SA_SKIP_MAPPING_REPAIR=1 must report without writing") + } +} + +// TestDeleteAppOnlyDeletesOwnedMappings covers the defense-in-depth ownership +// check: if the index ever drifts again, the H8 app-delete cascade must not +// delete a live mapping belonging to somebody else. +func TestDeleteAppOnlyDeletesOwnedMappings(t *testing.T) { + s := openTestBolt(t, t.TempDir()) + + if err := s.CreateApp(&App{AppID: "tenant", Audience: "tenant"}); err != nil { + t.Fatalf("create app: %v", err) + } + appUser := &User{DisplayName: "Ghost", OwnerAppID: "tenant"} + if err := s.CreateUser(appUser); err != nil { + t.Fatalf("create app user: %v", err) + } + if err := s.SetIdentityMapping("applocal:tenant", "ghost", appUser.GUID); err != nil { + t.Fatalf("set applocal: %v", err) + } + + // An unrelated user owns a mapping the app user's index falsely claims. + victim := &User{DisplayName: "Victim"} + if err := s.CreateUser(victim); err != nil { + t.Fatalf("create victim: %v", err) + } + if err := s.SetIdentityMapping("local", "victim", victim.GUID); err != nil { + t.Fatalf("set victim: %v", err) + } + if err := s.update(func(tx *bolt.Tx) error { + return s.addMappingToIndex(tx, appUser.GUID, IdentityMapping{Provider: "local", ExternalID: "victim"}) + }); err != nil { + t.Fatalf("inject: %v", err) + } + + if err := s.DeleteApp("tenant"); err != nil { + t.Fatalf("DeleteApp: %v", err) + } + // The app's own mapping is gone (H8 still holds). + if _, err := s.ResolveMapping("applocal:tenant", "ghost"); err == nil { + t.Fatal("app delete must cascade the app-local user's own mapping (H8)") + } + // The victim's live mapping survived. + if guid, err := s.ResolveMapping("local", "victim"); err != nil || guid != victim.GUID { + t.Fatalf("app delete destroyed another user's live mapping (H14): %q err=%v", guid, err) + } +} diff --git a/internal/store/migrate.go b/internal/store/migrate.go index 8c3994b..a9172b2 100644 --- a/internal/store/migrate.go +++ b/internal/store/migrate.go @@ -120,6 +120,17 @@ func MigrateToPostgres(source *BoltStore, target *PostgresStore, statusCh chan<- }) m.send() + // The Bolt forward key is the ambiguous composite "provider:externalID"; the + // reverse index holds the exact halves. Without this, "applocal:" lands + // in Postgres as provider="applocal" / external_id=":", and + // ResolveMapping("applocal:"+appID, username) — the app-local login lookup — + // can never match again after the migration (M38). + var mappingSplitIdx map[string]IdentityMapping + _ = source.db.View(func(tx *bolt.Tx) error { + mappingSplitIdx = mappingSplits(tx) + return nil + }) + // Step 3: Copy data for _, b := range buckets { m.status.Progress[b.name] = "migrating" @@ -131,7 +142,7 @@ func MigrateToPostgres(source *BoltStore, target *PostgresStore, statusCh chan<- return nil } return bucket.ForEach(func(k, v []byte) error { - if err := migrateKV(target, b.name, k, v); err != nil { + if err := migrateKV(target, b.name, k, v, mappingSplitIdx); err != nil { return fmt.Errorf("%s key=%s: %w", b.name, string(k), err) } m.status.MigratedItems++ @@ -184,18 +195,21 @@ func MigrateToPostgres(source *BoltStore, target *PostgresStore, statusCh chan<- } // migrateKV inserts a single BoltDB key-value pair into the correct Postgres table. -func migrateKV(target *PostgresStore, table string, k, v []byte) error { +func migrateKV(target *PostgresStore, table string, k, v []byte, mappingSplit map[string]IdentityMapping) error { key := string(k) switch table { case "users": _, err := target.db.Exec(`INSERT INTO sa_users (guid, data) VALUES ($1, $2) ON CONFLICT (guid) DO UPDATE SET data = $2`, key, v) return err case "identity_mappings": - idx := strings.Index(key, ":") - if idx < 0 { + // Decompose via the reverse index, not the first ':' — see mappingSplits + // (M38). Getting this wrong silently breaks app-local login after the + // migration, with no error at migration time. + m, ok := splitMappingKey(key, mappingSplit) + if !ok { return nil } - _, err := target.db.Exec(`INSERT INTO sa_identity_mappings (provider, external_id, user_guid) VALUES ($1, $2, $3) ON CONFLICT (provider, external_id) DO UPDATE SET user_guid = $3`, key[:idx], key[idx+1:], string(v)) + _, err := target.db.Exec(`INSERT INTO sa_identity_mappings (provider, external_id, user_guid) VALUES ($1, $2, $3) ON CONFLICT (provider, external_id) DO UPDATE SET user_guid = $3`, m.Provider, m.ExternalID, string(v)) return err case "idx_mappings_by_guid": return nil // reverse index — Postgres uses SQL index