From 44655dea23352f775751e49d61b4da556a1fce17 Mon Sep 17 00:00:00 2001 From: Your Name Date: Sat, 8 Aug 2026 20:20:37 +0300 Subject: [PATCH 1/4] fix(oidc): carry the full authorize request through the login-error redirect (H13) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit renderOIDCLoginError rebuilt the authorize URL by hand with fmt.Sprintf and carried client_id, redirect_uri, state, nonce and scope — but NOT code_challenge or code_challenge_method. One mistyped password therefore disabled PKCE for the rest of the login: failed POST -> error redirect without PKCE -> showOIDCLoginPage reads an empty challenge, stamps empty fields -> the successful retry stores OIDCAuthCode.CodeChallenge = "" -> the token endpoint's `if ac.CodeChallenge != ""` guard is false -> the code redeems with NO code_verifier That 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: introduce oidcAuthzRequest, a typed allowlist that is the single definition of "the authorize request", plus parseOIDCAuthzRequest and values(). renderOIDCLoginError and the Kerberos ssoLink are both built from it now, so a parameter added there is carried at every hop rather than having to be remembered at each hand-concatenated site. This is the fourth instance of "app context dropped at a redirect hop that rebuilds a URL from scratch" found in this codebase; the type exists to make it the last. Deliberately NOT a copy of r.Form. renderOIDCLoginError runs on a CREDENTIAL POST — the body carries username and password. Copying and mutating the form would put 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 and a query-copy carries nothing silently. Two invariants preserved and now asserted: - the error always returns to SimpleAuth's OWN authorize endpoint. The empty-credentials branch reaches this function 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). - username / password / _csrf are never carried; showOIDCLoginPage mints a fresh CSRF token and cookie per render (F30). Also fixed, 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 whole chain and asserts the post-retry code is REJECTED without a verifier and accepted with the correct one — the assertion that actually pins the vulnerability. - TestOIDCLoginErrorPreservesAuthorizeRequest pins every parameter in the allowlist plus the no-credential-leak invariant. - TestOIDCLoginErrorWithNoCredentials pins the open-redirect invariant. - TestLogoutPreservesClientID pins the logout round-trip end to end. Three of the four fail with the fix reverted. Full suite green. Not fixed here: handleSSOLogin's SPNEGO Negotiate-retry URL 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 — filed separately. Co-Authored-By: Claude Opus 5 (1M context) --- SECURITY-AUDIT.md | 60 +++++++ internal/handler/hosted_login.go | 10 +- internal/handler/oidc.go | 153 +++++++++++++---- internal/handler/oidc_pkce_test.go | 264 +++++++++++++++++++++++++++++ 4 files changed, 450 insertions(+), 37 deletions(-) create mode 100644 internal/handler/oidc_pkce_test.go diff --git a/SECURITY-AUDIT.md b/SECURITY-AUDIT.md index e28837a..68dca0c 100644 --- a/SECURITY-AUDIT.md +++ b/SECURITY-AUDIT.md @@ -82,6 +82,7 @@ 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) | | 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) | @@ -811,3 +812,62 @@ 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. 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/oidc.go b/internal/handler/oidc.go index 8b32cb4..6f32289 100644 --- a/internal/handler/oidc.go +++ b/internal/handler/oidc.go @@ -325,26 +325,100 @@ func (h *Handler) issueOIDCCodeRedirect(w http.ResponseWriter, r *http.Request, 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 +443,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 +1147,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()) + } +} From 164da3f556d05e3953dcab29b09326bb5fb9228e Mon Sep 17 00:00:00 2001 From: Your Name Date: Sat, 8 Aug 2026 20:27:54 +0300 Subject: [PATCH 2/4] fix(store): retract the reverse-index claim on a mapping re-point (H14, M38) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit H14 — the forward bucket (provider:externalID -> guid) is authoritative and bucketIdxMappingsByGUID is only a derived reverse index, but Bolt's SetIdentityMapping overwrote the forward entry and called addMappingToIndex without ever retracting the claim from the PREVIOUS owner. 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 via POST /api/admin/users, which calls SetIdentityMapping("local","alice",U2). Forward map says U2; U1's index still claims local:alice. - 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. - handleDeleteLocalUser and DeleteApp iterate GetMappingsForUser and delete forward keys, so deleting U1 removes U2's live login identity. Fix: 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 a 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 rather than adding a Bolt-specific behaviour. Existing data: repairMappingIndex runs at OpenBolt and prunes reverse-index entries the forward bucket no longer backs. Prune-only, never rebuild — reconstructing from forward keys would have to re-split the ambiguous composite key (M38) and would corrupt the exactly-recorded applocal: providers the index already holds correctly. 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 from the log, 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. M38 — the forward key is provider + ":" + externalID and BOTH halves may contain ':' (app-local users are keyed under provider "applocal:", and handleSetMapping accepts an arbitrary provider). ListAllMappings and MigrateToPostgres both split on the first ':', reading applocal:billing:bob as provider "applocal" / external id "billing:bob". The migration consequence is silent and severe: those corrupted halves land in sa_identity_mappings, after which ResolveMapping("applocal:"+appID, username) — the app-local login lookup — can never match again. Row-count verification still passes, because the mapping is 1:1 either way; only the column boundary moves. Fix: decompose via the reverse index, which records both halves verbatim (mappingSplits / splitMappingKey), falling back to the first ':' only for a key the index does not cover — which only happens in already-corrupt data, where falling back beats dropping the row. Tests (internal/store/mapping_index_test.go): re-point retracts from the previous owner and leaves that user's own mappings alone; re-setting the same mapping is idempotent; applocal: round-trips through ListAllMappings; the repair prunes an injected stale claim while leaving legitimate ones and the real owner untouched; SA_SKIP_MAPPING_REPAIR=1 reports without writing; DeleteApp cascades the app's own mappings (H8 holds) but not a victim's live one. The re-point test fails with the writer reverted. Full suite green. Behaviour change worth noting in review: correcting ListAllMappings makes resolveUserRef's "ambiguous user" branch newly reachable on Bolt for a name that exists as both local: and applocal:: with different GUIDs. That turns a previously-succeeding app-admin grant into an error — convergence toward Postgres behaviour, but a real Bolt-only change. Not covered by tests: migrateKV needs a live Postgres and this repo has no Postgres harness. Verify by hand before relying on it (procedure in SECURITY-AUDIT.md). Not fixed here: MergeUsers is a fourth consumer of the reverse index and blind-Puts forward keys without an ownership check. Correct once the index is clean, which the repair ensures, but it should get the same guard. Co-Authored-By: Claude Opus 5 (1M context) --- SECURITY-AUDIT.md | 93 +++++++++ internal/store/bolt.go | 201 +++++++++++++++++- internal/store/mapping_index_test.go | 298 +++++++++++++++++++++++++++ internal/store/migrate.go | 24 ++- 4 files changed, 601 insertions(+), 15 deletions(-) create mode 100644 internal/store/mapping_index_test.go diff --git a/SECURITY-AUDIT.md b/SECURITY-AUDIT.md index 68dca0c..c625c44 100644 --- a/SECURITY-AUDIT.md +++ b/SECURITY-AUDIT.md @@ -83,6 +83,7 @@ source of truth for what is currently open vs. fixed. | 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) | | 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) | @@ -107,6 +108,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) | @@ -871,3 +873,94 @@ pins the whole allowlist and the no-credential-leak invariant; (`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. 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 From 5f9cbe41ebcc39b2bf73889b339bc2dbc29d3403 Mon Sep 17 00:00:00 2001 From: Your Name Date: Sat, 8 Aug 2026 20:33:19 +0300 Subject: [PATCH 3/4] fix(auth): refuse to create a local password on a directory-backed user (SA-7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleResetPassword gated proof of possession on user.PasswordHash != "" && !user.ForcePasswordChange which conflates two orthogonal questions — "is there a local credential to prove?" and "is this account allowed to have one at all?". A directory-backed user has an EMPTY PasswordHash because their credential lives in AD, so the whole verification block was skipped and the handler unconditionally WROTE a fresh bcrypt hash. The endpoint was a CREATE path, not merely a ROTATE path. Every link of the chain is 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 contacted at all. - validateAccessToken performs no audience check, so a token minted for ANY registered app reaches this handler. - Nothing mirrors AD account state (grep -rn userAccountControl: zero hits), so the planted hash 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. Fix: gate on the empty hash BEFORE any bcrypt work and refuse with 403 — directory-backed gets "password is managed by the directory", and an account with no local password at all gets "ask an administrator". 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. The second refusal 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. 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 it) but is empty for import-users accounts until first login; and any mapping whose provider is neither "local" nor "applocal:". It fails CLOSED on a store error. 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. The empty-hash gate can safely precede the force-change branch because ForcePasswordChange has exactly two writers (handleSetPassword, handleBootstrap) and both assign a real bcrypt hash immediately before setting the flag. TestResetPasswordForceChangeStillWorks asserts that invariant 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 distinguishes deliberate break-glass from an account takeover after the fact. NOT cleaned up: already-planted hashes. Nothing records the provenance of a password hash, so a migration cannot tell a maliciously planted credential from a legitimate admin-set one. Operators upgrading should audit for directory users carrying a local hash and clear the ones they cannot account for — noted in SECURITY-AUDIT.md. Tests (internal/handler/reset_password_test.go): the directory-user test drives the FULL chain — plant, assert no hash was written, then assert the credential does not authenticate — rather than only asserting the status code. Plus the no-local-password case, the unchanged local-user rotation (400 without / 403 wrong / 200 correct, and the new password works), the force-change flow, and the isDirectoryBacked classification table including the app-local carve-out. The two refusal tests return 200 {"status":"password updated"} with the fix reverted. Full suite green. Co-Authored-By: Claude Opus 5 (1M context) --- SECURITY-AUDIT.md | 92 +++++++++ internal/handler/admin.go | 25 ++- internal/handler/auth.go | 109 +++++++++- internal/handler/clientip_test.go | 81 ++++++++ internal/handler/middleware.go | 22 ++- internal/handler/oidc.go | 9 + internal/handler/reset_password_test.go | 252 ++++++++++++++++++++++++ 7 files changed, 581 insertions(+), 9 deletions(-) create mode 100644 internal/handler/clientip_test.go create mode 100644 internal/handler/reset_password_test.go diff --git a/SECURITY-AUDIT.md b/SECURITY-AUDIT.md index c625c44..54812e3 100644 --- a/SECURITY-AUDIT.md +++ b/SECURITY-AUDIT.md @@ -84,6 +84,7 @@ source of truth for what is currently open vs. fixed. | 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) | @@ -964,3 +965,94 @@ Postgres harness in this repo. Verify by hand before relying on it — build a B 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/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 6f32289..f7b2b67 100644 --- a/internal/handler/oidc.go +++ b/internal/handler/oidc.go @@ -322,6 +322,15 @@ 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) } 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) + } + } +} From cecdc2df34339ac9ee5d27fb3e41d5c9ee8a7f2a Mon Sep 17 00:00:00 2001 From: Your Name Date: Sat, 8 Aug 2026 20:39:49 +0300 Subject: [PATCH 4/4] fix(migrate): refuse a bundle that claims another app's audience (H15) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply carried the bundle's audience onto the target app unchecked: if b.App.Audience != "" { app.Audience = b.App.Audience } The audience is a security PRINCIPAL, not a label. appAudience() feeds claims.Audience on every issuance path, and SimpleAuth deliberately does not pin aud centrally — F51 records that issuer/audience are enforced at the SDK/RP layer. So the string in aud, verified offline against the shared JWKS, is the only thing between a token and a victim resource server. And the migration-token holder is NOT a master admin: guardMigrationCall authenticates a single-use bearer token scoped to ONE target app, and nothing restricted the bundle to naming that app's own identity. Attack: the standalone team POSTs a bundle with app.audience = "billing-api", catalog.role_permissions = {"admin":["billing:write"]}, and a local user with a self-chosen password hash and roles ["admin"]. They log in at their own migrated app and receive aud ["billing-api"], roles ["admin"], signed by the central's key. The real billing service verifies signature + audience offline and admits them as a billing admin. The no-effort variant: Package sets aud = app.AppID when the source home app has no explicit audience, and ensureDefaultApp creates it with Audience = appID — "simpleauth" on a stock standalone. Migrating a stock standalone therefore stamped the target with the CENTRAL's own default-app audience, whose tokens carry global directory roles. The integration tests encoded this as expected behaviour; they now set a deliberate source audience via newMigTargetFrom, which is what a real migration must do. Fix: resolveCarriedAudience decides the target's audience and refuses a claim on anyone else's — empty carries nothing (keeps the master-admin-set value); the target's own audience or app_id is always allowed (so re-running a migration is idempotent); the central's default app id is refused; anything matching another registered app's audience or app_id is refused. Trimmed, so whitespace cannot smuggle a near-collision past the check. The default-app id is checked EXPLICITLY rather than via ListApps() because ensureDefaultApp is called only from main.go — on an embedded deployment (pkg/server) the row can be absent while resolveApp still synthesizes it, so a ListApps-only check would miss the highest-value target. Enforced in BOTH Classify (as a Blocked entry, so the existing preflight UI renders it and Report.OK() is false) and Apply (before its first write, so a refused bundle leaves nothing behind). Apply re-checks rather than trusting the dry run because it is reachable on its own and the central's app set can change between preflight and commit. Report.AudienceToApply surfaces the value in the dry run and the commit audit entry records it. Fails CLOSED if ListApps errors. Tests (internal/migrate/audience_test.go): foreign audience refused with nothing written; app_id collision refused; the central's default-app audience refused WITH NO SUCH APP ROW PRESENT (the embedded-deployment case); a distinct audience still carried; re-running the same migration idempotent; whitespace trimmed; empty carried audience preserves the target's; Classify blocks naming the conflicting app and reports AudienceToApply on the happy path. Integration tests: added newMigTargetFrom, which gives the SOURCE's home app a deliberate audience before packaging, and moved all six migration subtests onto it. These are //go:build integration and do NOT run under `go test ./...` — they compile (go vet -tags integration) but were not executed here; run `make -C test/integration test` before relying on them. Known, not fixed here: RedirectURIs and CORSOrigins are carried the same unchecked way (pre-existing). Lesser, because it only affects the app the token already scopes the holder to rather than a third party's audience — but worth review. Co-Authored-By: Claude Opus 5 (1M context) --- SECURITY-AUDIT.md | 80 +++++++++++ internal/handler/migration_central.go | 7 +- internal/migrate/audience_test.go | 194 ++++++++++++++++++++++++++ internal/migrate/bundle.go | 98 ++++++++++++- internal/migrate/bundle_test.go | 20 +-- test/integration/ad_test.go | 4 +- test/integration/driver_test.go | 16 +++ test/integration/scenarios2_test.go | 8 +- 8 files changed, 404 insertions(+), 23 deletions(-) create mode 100644 internal/migrate/audience_test.go diff --git a/SECURITY-AUDIT.md b/SECURITY-AUDIT.md index 54812e3..f12564b 100644 --- a/SECURITY-AUDIT.md +++ b/SECURITY-AUDIT.md @@ -85,6 +85,7 @@ source of truth for what is currently open vs. fixed. | 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) | +| H15 | Migration bundle carries an arbitrary `audience` → a migration-token holder mints tokens another app's resource servers accept | 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) | @@ -1056,3 +1057,82 @@ hash written → verify the credential does not authenticate); rotated password authenticates); `TestResetPasswordForceChangeStillWorks`; `TestIsDirectoryBackedClassification` covering the app-local carve-out. The first two return `200 {"status":"password updated"}` with the fix reverted. +## Audit Pass 4 — H15 — migration bundle claims another app's audience + +**Severity:** HIGH. + +`Apply` carried the bundle's audience onto the target app unchecked: + +```go +if b.App.Audience != "" { app.Audience = b.App.Audience } +``` + +**Why it is exploitable.** The audience is a security *principal*, not a label: +`appAudience()` feeds `claims.Audience` on every issuance path, and SimpleAuth +deliberately does not pin `aud` centrally — **F51** records that issuer/audience +are enforced at the SDK/RP layer. The string in `aud`, verified offline against the +shared JWKS, is therefore the only thing between a token and a victim resource +server. + +And the migration-token holder is **not** a master admin: `guardMigrationCall` +authenticates a single-use bearer token scoped to ONE target app. Nothing +restricted the bundle to naming that app's own identity. + +Attack: the standalone team POSTs a bundle with `app.audience = "billing-api"`, +`catalog.role_permissions = {"admin":["billing:write"]}`, and a local user with a +self-chosen password hash and `roles:["admin"]`. They then log in at their own +migrated app and receive `aud: ["billing-api"]`, `roles:["admin"]`, signed by the +central's key. The real billing service verifies signature + audience offline and +admits them as a billing admin. + +**The no-effort variant.** `Package` sets `aud = app.AppID` when the source home app +has no explicit audience, and `ensureDefaultApp` creates it with +`Audience: appID` — i.e. **`"simpleauth"`** on a stock standalone. Migrating a stock +standalone therefore stamped the target with the *central's own default-app* +audience, whose tokens carry global directory roles. The pre-fix integration tests +encoded this as expected behavior; they now set a deliberate source audience via +the new `newMigTargetFrom` helper, which is what a real migration must do. + +**Approach.** `resolveCarriedAudience` decides the target's audience and refuses a +claim on anyone else's: empty carries nothing (keeps the master-admin-set value); +the target's **current audience** is allowed, so re-running a migration is +idempotent; the central's **default app id** is refused explicitly; anything +matching another registered app's audience or app_id is refused. + +The self-allow is deliberately the target's *audience only*, **not** its `app_id`. +Nothing enforces audience uniqueness at app creation, so a central can legitimately +hold `App{AppID:"reports", Audience:"analytics"}` while the operator registers the +migration target as `App{AppID:"analytics", Audience:"analytics-migrated"}`. +Self-allowing the app_id there would hand the bundle `"analytics"` — the live +audience of a third-party resource server — which is precisely the takeover this +refuses. An adversarial review of the first cut of this fix found exactly that +bypass; `TestApplyRejectsAudienceMatchingTargetAppID` pins it. The audience is +trimmed, so whitespace cannot smuggle a near-collision past the check. + +The default-app id is checked *explicitly* rather than through `ListApps()` because +`ensureDefaultApp` is called only from `main.go` — on an embedded deployment +(`pkg/server`) the row can be absent while `resolveApp` still synthesizes it, so a +`ListApps`-only check would miss the highest-value target. + +Enforced in **both** `Classify` (as a `Blocked` entry, so the existing preflight UI +renders it and `Report.OK()` is false) and `Apply` (before its first write, so a +refused bundle leaves nothing behind). `Apply` re-checks rather than trusting the +dry run because it is reachable on its own and the central's app set can change +between preflight and commit. `Report.AudienceToApply` surfaces the value in the +dry run, and the commit audit entry records it. + +Fails **closed**: if `ListApps` errors we cannot prove the audience is free, so the +migration is refused. + +**Known, not fixed here.** `RedirectURIs` and `CORSOrigins` are carried the same +unchecked way (pre-existing). A bundle submitter who can rewrite `redirect_uris` +controls token delivery for that app — but only for the app the token already +scopes them to, so it is a lesser issue than claiming a *third party's* audience. +It should still be reviewed. + +**Tests:** `internal/migrate/audience_test.go` — foreign audience refused with +nothing written; app_id collision refused; the central's default-app audience +refused *with no such app row present*; a distinct audience still carried; +re-running the same migration is idempotent; whitespace trimmed; an empty carried +audience preserves the target's; and `Classify` blocks with the conflicting app +named while reporting `AudienceToApply` on the happy path. diff --git a/internal/handler/migration_central.go b/internal/handler/migration_central.go index d724600..6f95ae6 100644 --- a/internal/handler/migration_central.go +++ b/internal/handler/migration_central.go @@ -170,7 +170,7 @@ func (h *Handler) handleMigrationPreflight(w http.ResponseWriter, r *http.Reques if !ok { return } - rep, err := migrate.Classify(req.Bundle, h.store, req.AppID) + rep, err := migrate.Classify(req.Bundle, h.store, req.AppID, h.defaultAppID()) if err != nil { jsonError(w, "preflight failed", http.StatusInternalServerError) return @@ -191,7 +191,7 @@ func (h *Handler) handleMigrationCommit(w http.ResponseWriter, r *http.Request) if !ok { return } - rep, err := migrate.Classify(req.Bundle, h.store, req.AppID) + rep, err := migrate.Classify(req.Bundle, h.store, req.AppID, h.defaultAppID()) if err != nil { jsonError(w, "preflight failed", http.StatusInternalServerError) return @@ -215,7 +215,7 @@ func (h *Handler) handleMigrationCommit(w http.ResponseWriter, r *http.Request) res, err := func() (*migrate.ApplyResult, error) { h.localUserMu.Lock() defer h.localUserMu.Unlock() - return migrate.Apply(req.Bundle, h.store, req.AppID, req.CarrySecret) + return migrate.Apply(req.Bundle, h.store, req.AppID, h.defaultAppID(), req.CarrySecret) }() if err != nil { jsonError(w, "commit failed (token spent; if the target was partially written, clear it before retrying with a new token): "+err.Error(), http.StatusInternalServerError) @@ -223,6 +223,7 @@ func (h *Handler) handleMigrationCommit(w http.ResponseWriter, r *http.Request) } h.audit("migration_committed", "migration:"+req.AppID, getClientIP(r), map[string]interface{}{ "app_id": req.AppID, "assignments": res.AssignmentsSet, "local_users": res.LocalUsersCreated, + "audience": rep.AudienceToApply, }) jsonResp(w, res, http.StatusOK) } diff --git a/internal/migrate/audience_test.go b/internal/migrate/audience_test.go new file mode 100644 index 0000000..9c8d69c --- /dev/null +++ b/internal/migrate/audience_test.go @@ -0,0 +1,194 @@ +package migrate + +import ( + "strings" + "testing" + + "simpleauth/internal/store" +) + +// newCentral builds a central with a target app plus a victim app whose audience +// the bundle will try to claim. +func newCentral(t *testing.T, targetID string) store.Store { + t.Helper() + c := open(t) + must(t, c.CreateApp(&store.App{AppID: targetID, Audience: targetID})) + must(t, c.CreateApp(&store.App{AppID: "billing", Audience: "billing-api"})) + return c +} + +// bundleWithAudience returns a minimal bundle carrying the given audience. +func bundleWithAudience(aud string) *Bundle { + return &Bundle{ + SchemaRev: SchemaRev, + App: AppConfig{Audience: aud}, + } +} + +// TestApplyRejectsForeignAudience is the H15 regression: a migration-token holder +// must not be able to re-stamp their own app with another app's audience and mint +// tokens the victim's resource servers accept. +func TestApplyRejectsForeignAudience(t *testing.T) { + c := newCentral(t, "migrated-shop") + b := bundleWithAudience("billing-api") + + if _, err := Apply(b, c, "migrated-shop", "simpleauth", false); err == nil { + t.Fatal("Apply accepted a bundle claiming another app's audience (H15)") + } else if !strings.Contains(err.Error(), "billing") { + t.Fatalf("error should name the conflicting app, got: %v", err) + } + + // Nothing was written — the check runs before the first mutation. + app, err := c.GetApp("migrated-shop") + if err != nil { + t.Fatalf("get target: %v", err) + } + if app.Audience != "migrated-shop" { + t.Fatalf("target audience was mutated despite the refusal: %q", app.Audience) + } +} + +// TestApplyRejectsAppIDCollision covers claiming another app's app_id, which is +// its audience when no explicit audience is set (appAudience falls back to AppID). +func TestApplyRejectsAppIDCollision(t *testing.T) { + c := open(t) + must(t, c.CreateApp(&store.App{AppID: "target", Audience: "target"})) + must(t, c.CreateApp(&store.App{AppID: "payroll"})) // no explicit audience + + if _, err := Apply(bundleWithAudience("payroll"), c, "target", "simpleauth", false); err == nil { + t.Fatal("Apply accepted a bundle claiming another app's app_id as its audience (H15)") + } +} + +// TestApplyRejectsDefaultAppAudience is the no-effort version of the attack: a +// STOCK standalone packages aud = "simpleauth", which is the central's own default +// app. This must be refused even though ensureDefaultApp may never have written +// that row (pkg/server never calls it), so a ListApps-only check would miss it. +func TestApplyRejectsDefaultAppAudience(t *testing.T) { + c := open(t) + must(t, c.CreateApp(&store.App{AppID: "target", Audience: "target"})) + // Deliberately NO "simpleauth" app row — resolveApp synthesizes it in prod. + + if _, err := Apply(bundleWithAudience("simpleauth"), c, "target", "simpleauth", false); err == nil { + t.Fatal("Apply accepted the central's default-app audience with no app row present (H15)") + } +} + +// TestApplyAllowsDistinctAudience proves the fix is not over-broad: a legitimate +// migration still carries its own audience. +func TestApplyAllowsDistinctAudience(t *testing.T) { + c := newCentral(t, "migrated-shop") + if _, err := Apply(bundleWithAudience("shop-api"), c, "migrated-shop", "simpleauth", false); err != nil { + t.Fatalf("legitimate audience carry was refused: %v", err) + } + app, err := c.GetApp("migrated-shop") + if err != nil { + t.Fatalf("get: %v", err) + } + if app.Audience != "shop-api" { + t.Fatalf("audience not carried: %q", app.Audience) + } +} + +// TestApplyAudienceIsIdempotent covers re-running a migration for the same app: +// the target must not collide with itself. +func TestApplyAudienceIsIdempotent(t *testing.T) { + c := newCentral(t, "migrated-shop") + b := bundleWithAudience("shop-api") + if _, err := Apply(b, c, "migrated-shop", "simpleauth", false); err != nil { + t.Fatalf("first apply: %v", err) + } + // Second run carries the same audience, which the target now already holds. + if _, err := Apply(b, c, "migrated-shop", "simpleauth", false); err != nil { + t.Fatalf("re-running the same migration must be idempotent, got: %v", err) + } +} + +// TestApplyTrimsCarriedAudience — whitespace must not be a way to smuggle a +// near-collision past the check or to store a ragged audience. +func TestApplyTrimsCarriedAudience(t *testing.T) { + c := newCentral(t, "migrated-shop") + if _, err := Apply(bundleWithAudience(" billing-api "), c, "migrated-shop", "simpleauth", false); err == nil { + t.Fatal("whitespace-padded foreign audience must still be refused (H15)") + } + if _, err := Apply(bundleWithAudience(" shop-api "), c, "migrated-shop", "simpleauth", false); err != nil { + t.Fatalf("padded legitimate audience: %v", err) + } + app, _ := c.GetApp("migrated-shop") + if app.Audience != "shop-api" { + t.Fatalf("audience should be stored trimmed, got %q", app.Audience) + } +} + +// TestApplyEmptyAudienceKeepsTargets — an omitted audience must leave whatever the +// master admin configured on the target, not blank it. +func TestApplyEmptyAudienceKeepsTargets(t *testing.T) { + c := open(t) + must(t, c.CreateApp(&store.App{AppID: "target", Audience: "admin-chosen"})) + if _, err := Apply(bundleWithAudience(""), c, "target", "simpleauth", false); err != nil { + t.Fatalf("apply: %v", err) + } + app, _ := c.GetApp("target") + if app.Audience != "admin-chosen" { + t.Fatalf("empty carried audience must preserve the target's, got %q", app.Audience) + } +} + +// TestClassifyReportsAudienceConflict pins that the dry run surfaces the conflict +// (so an operator sees it before committing) and reports the audience it would +// apply on the happy path. +func TestClassifyReportsAudienceConflict(t *testing.T) { + c := newCentral(t, "migrated-shop") + + rep, err := Classify(bundleWithAudience("billing-api"), c, "migrated-shop", "simpleauth") + if err != nil { + t.Fatalf("classify: %v", err) + } + if rep.OK() { + t.Fatal("Classify must block a colliding audience (H15)") + } + if len(rep.Blocked) == 0 || !strings.Contains(rep.Blocked[0].Reason, "billing") { + t.Fatalf("blocked reason should name the conflicting app: %+v", rep.Blocked) + } + + rep2, err := Classify(bundleWithAudience("shop-api"), c, "migrated-shop", "simpleauth") + if err != nil { + t.Fatalf("classify ok-case: %v", err) + } + if !rep2.OK() { + t.Fatalf("legitimate audience must not block: %+v", rep2.Blocked) + } + if rep2.AudienceToApply != "shop-api" { + t.Fatalf("dry run should report the audience it would apply, got %q", rep2.AudienceToApply) + } +} + +// TestApplyRejectsAudienceMatchingTargetAppID closes the bypass an adversarial +// review found in the first cut of this fix: `aud == target.AppID` used to +// short-circuit the whole collision check. +// +// Nothing enforces audience uniqueness at app creation, so a central can hold +// App{AppID:"reports", Audience:"analytics"} — several clients pointed at one +// resource server — while the operator registers the migration target as +// App{AppID:"analytics", Audience:"analytics-migrated"}. Self-allowing the +// target's app_id there hands the bundle "analytics", the live audience of a +// third-party RP. That is H15, unmitigated. +func TestApplyRejectsAudienceMatchingTargetAppID(t *testing.T) { + c := open(t) + must(t, c.CreateApp(&store.App{AppID: "reports", Audience: "analytics"})) + must(t, c.CreateApp(&store.App{AppID: "analytics", Audience: "analytics-migrated"})) + + if _, err := Apply(bundleWithAudience("analytics"), c, "analytics", "simpleauth", false); err == nil { + t.Fatal("bundle claimed a third party's live audience via the target's own app_id (H15)") + } + // The target keeps what the operator set. + app, _ := c.GetApp("analytics") + if app.Audience != "analytics-migrated" { + t.Fatalf("target audience mutated despite refusal: %q", app.Audience) + } + // And the legitimate idempotent case still works: carrying the value the + // target already holds. + if _, err := Apply(bundleWithAudience("analytics-migrated"), c, "analytics", "simpleauth", false); err != nil { + t.Fatalf("re-carrying the target's own audience must stay allowed: %v", err) + } +} diff --git a/internal/migrate/bundle.go b/internal/migrate/bundle.go index 20a4a8d..19c255a 100644 --- a/internal/migrate/bundle.go +++ b/internal/migrate/bundle.go @@ -202,6 +202,10 @@ type Report struct { Notes []string `json:"notes,omitempty"` RedirectURIsToReview []string `json:"redirect_uris_to_review,omitempty"` + + // AudienceToApply is the audience Apply will end up storing on the target — + // shown in the dry run so an operator sees a carried audience BEFORE committing. + AudienceToApply string `json:"audience_to_apply,omitempty"` } // BlockedUser is a user the central cannot satisfy as-is. @@ -210,12 +214,75 @@ type BlockedUser struct { Reason string `json:"reason"` } +// resolveCarriedAudience decides what audience the target app should end up with, +// and refuses a bundle that tries to claim somebody else's. +// +// The audience is a security PRINCIPAL, not a label: appAudience() feeds +// claims.Audience on every issuance path, and SimpleAuth deliberately does not +// pin `aud` centrally — F51 records that issuer/audience are enforced at the +// SDK/RP layer. So the string in `aud`, verified offline against the shared JWKS, +// is the ONLY thing between a token and a victim resource server. +// +// The migration-token holder is not a master admin: guardMigrationCall +// authenticates a single-use bearer token scoped to ONE target app. Carrying an +// arbitrary audience therefore lets that holder re-stamp their own app to mint +// `aud: ["billing-api"]` with self-chosen roles from the bundle's catalog, which +// the real billing service then admits (H15). +// +// Rules, in order: +// - empty carried audience → keep whatever the master admin set on the target. +// - unchanged / equal to the target's own app_id → fine, and idempotent so a +// re-run of the same migration does not collide with itself. +// - equal to the central's DEFAULT app id → refused. This is checked explicitly +// rather than via ListApps because ensureDefaultApp is called only from +// main.go — on an embedded deployment (pkg/server) the row can be absent while +// resolveApp still synthesizes it, so a ListApps-only check would miss the +// highest-value target. It is also the no-effort version of the attack: a +// stock standalone packages `aud = "simpleauth"`. +// - equal to any OTHER registered app's audience or app_id → refused. +func resolveCarriedAudience(central store.Store, target *store.App, defaultAppID, bundleAud string) (string, string, error) { + aud := strings.TrimSpace(bundleAud) + if aud == "" { + return target.Audience, "", nil + } + // Idempotency: carrying the value the target ALREADY holds is a no-op, so a + // re-run of the same migration cannot collide with itself. + // + // Deliberately NOT `|| aud == target.AppID`. The target's app_id is not proven + // free: nothing in handleCreateApp or handleUpdateApp enforces audience + // uniqueness, so a central can legitimately hold App{AppID:"reports", + // Audience:"analytics"} while an operator registers the migration target as + // App{AppID:"analytics", Audience:"analytics-migrated"}. Self-allowing the + // app_id there would hand the bundle "analytics" — the live audience of a + // third-party resource server — which is exactly the takeover this refuses. + if aud == target.Audience { + return aud, "", nil + } + if defaultAppID != "" && aud == defaultAppID { + return "", fmt.Sprintf("bundle audience %q is the central's default app — refusing (it would mint tokens the global directory app's consumers accept)", aud), nil + } + apps, err := central.ListApps() + if err != nil { + // Fail CLOSED: without the app list we cannot prove the audience is free. + return "", "", fmt.Errorf("list apps for audience collision check: %w", err) + } + for _, a := range apps { + if a.AppID == target.AppID { + continue + } + if aud == a.Audience || aud == a.AppID { + return "", fmt.Sprintf("bundle audience %q is already claimed by app %q — refusing (tokens minted for this app would be accepted by that app's resource servers)", aud, a.AppID), nil + } + } + return aud, "", nil +} + // OK reports whether the migration can proceed (no blocked users). func (r *Report) OK() bool { return len(r.Blocked) == 0 } // Classify computes the dry-run report. It validates every user is satisfiable on // the central WITHOUT mutating anything. -func Classify(b *Bundle, central store.Store, targetAppID string) (*Report, error) { +func Classify(b *Bundle, central store.Store, targetAppID, defaultAppID string) (*Report, error) { r := &Report{SourceVersion: b.SourceVersion, TargetApp: targetAppID, RedirectURIsToReview: b.App.RedirectURIs} // Fresh-target guard: Apply wholesale-replaces the target's authz, so refuse a @@ -227,6 +294,21 @@ func Classify(b *Bundle, central store.Store, targetAppID string) (*Report, erro return r, nil } + // Audience collision: refuse a bundle that claims another app's audience (H15). + // Reported as a Blocked entry so the existing preflight UI renders it and + // Report.OK() is false, which is what stops handleMigrationCommit. + if target, err := central.GetApp(targetAppID); err == nil { + aud, conflict, err := resolveCarriedAudience(central, target, defaultAppID, b.App.Audience) + if err != nil { + return nil, err + } + if conflict != "" { + r.Blocked = append(r.Blocked, BlockedUser{Key: targetAppID, Reason: conflict}) + return r, nil + } + r.AudienceToApply = aud + } + centralLDAP, _ := central.GetLDAPConfig() centralHasAD := centralLDAP != nil && (centralLDAP.Domain != "" || centralLDAP.BaseDN != "") sameAD := centralHasAD && b.SourceAD != nil && sameADDomain(b.SourceAD, centralLDAP) @@ -295,7 +377,7 @@ type ApplyResult struct { // idempotent for local users (an existing app-local username is left in place). // carrySecret copies the source app's secret hash so the consumer's existing // secret keeps working; pass false to keep the target app's own secret. -func Apply(b *Bundle, central store.Store, targetAppID string, carrySecret bool) (*ApplyResult, error) { +func Apply(b *Bundle, central store.Store, targetAppID, defaultAppID string, carrySecret bool) (*ApplyResult, error) { res := &ApplyResult{} app, err := central.GetApp(targetAppID) @@ -303,9 +385,17 @@ func Apply(b *Bundle, central store.Store, targetAppID string, carrySecret bool) return nil, fmt.Errorf("target app: %w", err) } - if b.App.Audience != "" { - app.Audience = b.App.Audience + // Re-check the audience here, not only in Classify: Apply is reachable on its + // own and the central's app set can change between preflight and commit. This + // runs BEFORE the first write, so a refused bundle leaves nothing behind (H15). + aud, conflict, err := resolveCarriedAudience(central, app, defaultAppID, b.App.Audience) + if err != nil { + return nil, err + } + if conflict != "" { + return nil, fmt.Errorf("%s", conflict) } + app.Audience = aud if len(b.App.RedirectURIs) > 0 { app.RedirectURIs = b.App.RedirectURIs } diff --git a/internal/migrate/bundle_test.go b/internal/migrate/bundle_test.go index 82566d5..9f1a554 100644 --- a/internal/migrate/bundle_test.go +++ b/internal/migrate/bundle_test.go @@ -81,7 +81,7 @@ func TestPackageClassifyApply_SameAD(t *testing.T) { must(t, central.SaveLDAPConfig(&store.LDAPConfig{Domain: "corp.local"})) must(t, central.CreateApp(&store.App{AppID: "billing", Audience: "billing"})) - rep, err := Classify(b, central, "billing") + rep, err := Classify(b, central, "billing", "simpleauth") if err != nil { t.Fatalf("classify: %v", err) } @@ -95,7 +95,7 @@ func TestPackageClassifyApply_SameAD(t *testing.T) { t.Fatalf("expected a direct-perm note") } - res, err := Apply(b, central, "billing", true) + res, err := Apply(b, central, "billing", "simpleauth", true) if err != nil { t.Fatalf("apply: %v", err) } @@ -132,7 +132,7 @@ func TestPackageClassifyApply_SameAD(t *testing.T) { } // Idempotent re-apply: no second local user, assignments unchanged. - res2, err := Apply(b, central, "billing", true) + res2, err := Apply(b, central, "billing", "simpleauth", true) if err != nil { t.Fatalf("re-apply: %v", err) } @@ -151,7 +151,7 @@ func TestClassify_CentralNotOnAD_BlocksADUsers(t *testing.T) { central := open(t) // NO LDAP configured must(t, central.CreateApp(&store.App{AppID: "billing", Audience: "billing"})) - rep, err := Classify(b, central, "billing") + rep, err := Classify(b, central, "billing", "simpleauth") if err != nil { t.Fatalf("classify: %v", err) } @@ -174,7 +174,7 @@ func TestClassify_DifferentAD_BlocksADUsers(t *testing.T) { must(t, central.SaveLDAPConfig(&store.LDAPConfig{Domain: "other.local"})) // different AD must(t, central.CreateApp(&store.App{AppID: "billing", Audience: "billing"})) - rep, _ := Classify(b, central, "billing") + rep, _ := Classify(b, central, "billing", "simpleauth") if rep.OK() || len(rep.Blocked) != 2 { t.Fatalf("different-AD must block the 2 AD users: %+v", rep.Blocked) } @@ -186,7 +186,7 @@ func TestApply_DoesNotDowngradeRequireAssignment(t *testing.T) { central := open(t) must(t, central.CreateApp(&store.App{AppID: "payroll", Audience: "payroll", RequireAssignment: true})) b := &Bundle{SchemaRev: SchemaRev, App: AppConfig{RequireAssignment: false}} - if _, err := Apply(b, central, "payroll", false); err != nil { + if _, err := Apply(b, central, "payroll", "simpleauth", false); err != nil { t.Fatalf("apply: %v", err) } if app, _ := central.GetApp("payroll"); !app.RequireAssignment { @@ -204,7 +204,7 @@ func TestApply_ValidatesPresentationFields(t *testing.T) { bad := &Bundle{SchemaRev: SchemaRev, App: AppConfig{ BaseURL: "http://evil.example", Icon: "../../evil", }} - if _, err := Apply(bad, central, "portal", false); err != nil { + if _, err := Apply(bad, central, "portal", "simpleauth", false); err != nil { t.Fatalf("apply: %v", err) } if app, _ := central.GetApp("portal"); app.BaseURL != "" || app.Icon != "" { @@ -216,7 +216,7 @@ func TestApply_ValidatesPresentationFields(t *testing.T) { good := &Bundle{SchemaRev: SchemaRev, App: AppConfig{ BaseURL: "https://portal.example.com/", Icon: "/icon.svg", }} - if _, err := Apply(good, central, "portal2", false); err != nil { + if _, err := Apply(good, central, "portal2", "simpleauth", false); err != nil { t.Fatalf("apply: %v", err) } if app, _ := central.GetApp("portal2"); app.BaseURL != "https://portal.example.com" || app.Icon != "/icon.svg" { @@ -231,7 +231,7 @@ func TestClassify_FreshTargetGuard(t *testing.T) { must(t, central.CreateApp(&store.App{AppID: "billing", Audience: "billing"})) must(t, central.SaveAppAuthz(&store.AppAuthz{AppID: "billing", UserAssignments: map[string][]string{"x": {"r"}}})) b := &Bundle{SchemaRev: SchemaRev, Users: []UserEntry{{Kind: KindLocal, Key: "bob", Roles: []string{"r"}, PasswordHash: "h"}}} - rep, _ := Classify(b, central, "billing") + rep, _ := Classify(b, central, "billing", "simpleauth") if rep.OK() { t.Fatal("classify must block a non-empty target app") } @@ -244,7 +244,7 @@ func TestClassify_FreshTargetGuard_Groups(t *testing.T) { must(t, central.CreateApp(&store.App{AppID: "billing", Audience: "billing"})) must(t, central.SaveAppAuthz(&store.AppAuthz{AppID: "billing", GroupAssignments: map[string][]string{"Finance": {"viewer"}}})) b := &Bundle{SchemaRev: SchemaRev, Users: []UserEntry{{Kind: KindLocal, Key: "bob", Roles: []string{"r"}, PasswordHash: "h"}}} - rep, _ := Classify(b, central, "billing") + rep, _ := Classify(b, central, "billing", "simpleauth") if rep.OK() { t.Fatal("classify must block a target that has group assignments") } diff --git a/test/integration/ad_test.go b/test/integration/ad_test.go index 5209bba..2aa9f85 100644 --- a/test/integration/ad_test.go +++ b/test/integration/ad_test.go @@ -96,7 +96,7 @@ func TestADMigrationScenarios(t *testing.T) { bob := findGUIDBySAM(t, sAD, "bob") sAD.must(t, "PUT", "/api/admin/users/"+bob+"/roles", []string{"editor"}) - mig := newMigTarget(t, central, "hr", true) + mig := newMigTargetFrom(t, central, sAD, "hr", true) // Policy-only: an AD user, no local-user record/password is copied. var rep migrate.Report @@ -132,7 +132,7 @@ func TestADMigrationScenarios(t *testing.T) { carol := findGUIDBySAM(t, sDiff, "carol") sDiff.must(t, "PUT", "/api/admin/users/"+carol+"/roles", []string{"viewer"}) - mig := newMigTarget(t, central, "diffapp", false) + mig := newMigTargetFrom(t, central, sDiff, "diffapp", false) var rep migrate.Report decode(t, sDiff.must(t, "POST", "/api/admin/migrate-to-central/preflight", mig), &rep) diff --git a/test/integration/driver_test.go b/test/integration/driver_test.go index 8be3816..ffa2473 100644 --- a/test/integration/driver_test.go +++ b/test/integration/driver_test.go @@ -224,6 +224,22 @@ func newMigTarget(t *testing.T, central *node, appID string, carrySecret bool) m return migPayload(t, central, appID, carrySecret) } +// newMigTargetFrom creates the central target app AND first gives the SOURCE's +// home app a deliberate audience. +// +// A stock standalone's home app carries the default audience "simpleauth" — which +// is also the CENTRAL's default-app audience. Since H15 the central refuses a +// bundle claiming it, because that is the no-effort version of the audience +// takeover: the migrated app would mint tokens the central's global directory app +// consumers accept. Real migrations must therefore set a deliberate audience on +// the source before packaging; these tests model that rather than relying on the +// collision the old code silently allowed. +func newMigTargetFrom(t *testing.T, central, source *node, appID string, carrySecret bool) map[string]any { + t.Helper() + source.must(t, "PUT", "/api/admin/apps/simpleauth", map[string]any{"audience": appID + "-src-aud"}) + return newMigTarget(t, central, appID, carrySecret) +} + // TestLocalToCentralMigration: a local-accounts standalone migrates into a fresh // app on the central over real cross-container TLS; the migrated user then logs // in AGAINST THE CENTRAL with the same password and gets the right roles/perms. diff --git a/test/integration/scenarios2_test.go b/test/integration/scenarios2_test.go index 5ecda37..f356c1e 100644 --- a/test/integration/scenarios2_test.go +++ b/test/integration/scenarios2_test.go @@ -36,7 +36,7 @@ func TestMoreScenarios(t *testing.T) { if rot.Secret == "" { t.Fatal("no rotated secret") } - mig := newMigTarget(t, central, "secretapp", true) + mig := newMigTargetFrom(t, central, local, "secretapp", true) local.must(t, "POST", "/api/admin/migrate-to-central/commit", mig) if code, data := central.doBasic(t, "POST", "/api/app/token", "secretapp", rot.Secret); code != http.StatusOK { @@ -55,7 +55,7 @@ func TestMoreScenarios(t *testing.T) { central.must(t, "DELETE", "/api/admin/ldap", nil) t.Cleanup(func() { central.must(t, "PUT", "/api/admin/ldap", corpCfg) }) - mig := newMigTarget(t, central, "noad", false) + mig := newMigTargetFrom(t, central, sAD, "noad", false) var rep migrate.Report decode(t, sAD.must(t, "POST", "/api/admin/migrate-to-central/preflight", mig), &rep) if rep.OK() { @@ -67,7 +67,7 @@ func TestMoreScenarios(t *testing.T) { // The cross-install security guards: single-use token + fresh-target. t.Run("migration_guards", func(t *testing.T) { local.must(t, "PUT", "/api/admin/role-permissions", map[string][]string{"r": {}}) // ensure the bundle carries some authz - mig := newMigTarget(t, central, "guardapp", false) + mig := newMigTargetFrom(t, central, local, "guardapp", false) local.must(t, "POST", "/api/admin/migrate-to-central/commit", mig) // first commit OK if code, _ := local.do(t, "POST", "/api/admin/migrate-to-central/commit", mig); code != http.StatusUnauthorized { @@ -111,7 +111,7 @@ func TestMoreScenarios(t *testing.T) { sAD.must(t, "PUT", "/api/admin/users/"+bg.GUID+"/mappings", map[string]any{"provider": "local", "external_id": "breakglass"}) sAD.must(t, "PUT", "/api/admin/users/"+bg.GUID+"/roles", []string{"ops"}) - mig := newMigTarget(t, central, "mixedapp", false) + mig := newMigTargetFrom(t, central, sAD, "mixedapp", false) var rep migrate.Report decode(t, sAD.must(t, "POST", "/api/admin/migrate-to-central/preflight", mig), &rep)