From 7996f811573486b7c8aae22b85c6ce7cb2fd263c Mon Sep 17 00:00:00 2001 From: Your Name Date: Sat, 8 Aug 2026 20:43:55 +0300 Subject: [PATCH 1/2] fix(migrate): classify by directory identity before local password (H16) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit classifyUser tested u.PasswordHash != "" BEFORE the directory signal, so any AD-backed user who also carried a local hash migrated as an app-LOCAL user keyed by that credential. Three things went wrong at once: - the central ended up holding a STANDING PASSWORD for someone whose identity is directory-governed, so the migrated account outlived AD-side disablement, lockout and password policy — exactly what this package's doc comment promises never happens ("no record or password is copied"); - Classify never BLOCKS a local user, so an AD population misclassified this way sailed straight past the "central is on a different AD" guard that exists to stop unauthenticatable users being imported; - Apply flipped allow_local_users on the target for accounts that must never use the app-local login path. Composes with SA-7: that bug was one way an AD user acquired a local hash in the first place. Fixing SA-7 reduces the population going forward but neither eliminates it (master-admin break-glass stays legitimate) nor cleans existing data, so this inversion is independently necessary. Fix: resolve the directory key first; only a user with NO directory identity is local. A KindAD entry no longer carries a password hash — the central re-binds that person from the same AD, so copying the credential would recreate the very shadow account this prevents. A HadLocalPassword BOOLEAN (never the hash) travels instead so the dry run can tell the operator which break-glass logins do not survive the move. directoryKey prefers SAMAccountName but falls back to ldap/kerberos mappings, because handleImportLDAPUsers provisions a user with an ldap mapping and NO SAMAccountName until first login — without the fallback such a user is misclassified as local or dropped entirely. The `local` provider is deliberately not consulted: every LDAP/Kerberos JIT provision writes BOTH an ldap and a local mapping, which is exactly how localUsername happily returned an AD username under the old precedence. Selection is deterministic across backends: GetMappingsForUser returns insertion order on Bolt and UNORDERED rows on Postgres, and a user commonly carries both a UPN and a sAMAccountName form. The bare form wins, ties break lexicographically, so both backends produce an identical bundle. OwnerAppID != "" short-circuits to local: an app-local user is local by construction and must never be resolved against the directory. Note on the two directory predicates: handler.isDirectoryBacked (SA-7) is deny-by-default; migrate.directoryKey is an allow-list of ldap/kerberos. The asymmetry is INTENTIONAL — one returns a verdict, the other must return a key. Documented in both places; do not unify (internal/migrate importing from internal/handler would invert the dependency direction). WIRE BREAK: SchemaRev 1 -> 2. Classification happens on the SOURCE side, so a patched central cannot trust the Kind values in a rev-1 bundle. guardMigrationCall exact-matches the rev, so this surfaces as "upgrade the older deployment first" rather than a silent import of buggy classification. In-flight migrations must upgrade the source first — release notes. Also fixed: dishonest preflight counts. Apply skips a user with no effective roles entirely while Classify counted them as migrating, so preflight promised N and Apply delivered fewer with nothing explaining the gap. Report.NoRoles counts them separately with a note. The AD block checks still run for them — "the central cannot authenticate this person" is worth saying regardless of roles. Tests (internal/migrate/classify_test.go): directory user with a local hash -> AD, no hash carried, HadLocalPassword set; imported LDAP user with no SAMAccountName still directory; genuine local user unchanged and keeps their hash; app-local user never directory even with a stray SAMAccountName; directoryKey order-independent across insertion orders; NoRoles accounting matches Apply. The first two fail with the precedence reverted. Full suite green. Stacked on the H15 audience fix — same functions, same hunks. Co-Authored-By: Claude Opus 5 (1M context) --- SECURITY-AUDIT.md | 113 +++++++++++++ internal/migrate/bundle.go | 219 +++++++++++++++++++++--- internal/migrate/classify_test.go | 268 ++++++++++++++++++++++++++++++ 3 files changed, 579 insertions(+), 21 deletions(-) create mode 100644 internal/migrate/classify_test.go diff --git a/SECURITY-AUDIT.md b/SECURITY-AUDIT.md index f12564b..595c06c 100644 --- a/SECURITY-AUDIT.md +++ b/SECURITY-AUDIT.md @@ -86,6 +86,7 @@ source of truth for what is currently open vs. fixed. | 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) | +| H16 | `classifyUser` tests `PasswordHash` before the directory identity → AD users migrate as app-local shadows carrying a standing password | 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) | @@ -1136,3 +1137,115 @@ 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. + +--- + +## Audit Pass 4 — H16 — migration classifies AD users as local shadows + +**Severity:** HIGH. + +`classifyUser` tested `u.PasswordHash != ""` **before** the directory signal, so +any AD-backed user who also carried a local hash migrated as an app-LOCAL user +keyed by that credential. Three things went wrong at once: + +- The central ended up holding a **standing password** for someone whose identity + is directory-governed, so the migrated account outlived AD-side disablement, + lockout and password policy — exactly what this package's doc comment promises + never happens (*"no record or password is copied"*). +- `Classify` never **blocks** a local user, so an AD population misclassified this + way sailed straight past the *"central is on a different AD"* guard that exists + to stop unauthenticatable users being imported. +- `Apply` flipped `allow_local_users` on the target for accounts that must never + use the app-local login path. + +This composes with **SA-7**: that bug was one way an AD user acquired a local hash +in the first place. Fixing SA-7 reduces the population going forward but neither +eliminates it (master-admin break-glass remains legitimate) nor cleans existing +data, so the precedence inversion is independently necessary. + +**Approach.** Resolve the directory key first; only a user with no directory +identity at all is local. A `KindAD` entry no longer carries a password hash — +the central re-binds that person from the same AD, so copying the credential would +recreate the very shadow account this prevents. A `HadLocalPassword` **boolean** +(never the hash) travels instead, so the dry run can tell the operator which +break-glass logins do not survive the move. + +`isDirectoryProvider` covers both the bare provider names and the per-directory +forms (`ldap:`, `kerberos:`). An adversarial review of the first cut +found the allow-list was an exact match on `"ldap"` — and `build.md:214` documents +multi-directory deployments as exactly `ldap:corp` / `ldap:partner`, with +`handleSetMapping` accepting an arbitrary provider string. So H16 was unfixed for +any deployment that followed the documentation: their directory users classified as +local and had their password hashes exported. +`TestClassifyDirectoryProviderVariants` pins every documented form. + +`localUsername` fails closed for the same reason `directoryKey` does — it calls the +same store method, and swallowing the error made it fall through to `u.Email` and +export a credential keyed by something that is not the user's login. + +`directoryKey` prefers `SAMAccountName` but falls back to `ldap`/`kerberos` +mappings, because `handleImportLDAPUsers` provisions a user with an `ldap` mapping +and **no** `SAMAccountName` until their first login — without the fallback such a +user is misclassified as local or dropped from the bundle entirely. The `local` +provider is deliberately not consulted: every LDAP/Kerberos JIT provision writes +BOTH an `ldap` and a `local` mapping, which is exactly how `localUsername` happily +returned an AD username under the old precedence. + +Selection is **order-independent**: `GetMappingsForUser` returns insertion order on +Bolt and unordered rows on Postgres, and an LDAP user commonly carries both a UPN +and a sAMAccountName form. The bare form wins and ties break lexicographically, so +a given mapping *set* yields the same key on both backends. Note the scope of that +claim: the mapping SET itself is only as accurate as Bolt's reverse index, which is +what H14 repairs — this fix rides on that one landing first. + +`directoryKey` fails **closed**: an error from `GetMappingsForUser` used to be +swallowed, which reads as "no directory identity" and therefore classifies the +user as LOCAL and **exports their password hash** — the exact outcome this +precedence prevents. An adversarial review of the first cut found it; classification +now refuses rather than guessing, so a store failure yields a short bundle (visible +in the preflight counts) instead of a leaked credential. + +`Apply` also only flips `allow_local_users` for an entry it will actually +materialize. A zero-role local entry is skipped, and `Classify` no longer counts +it, so opening the target's local-login gate for a user that is never created +weakened the app's authentication surface for nothing. + +`OwnerAppID != ""` short-circuits to local: an app-local user is local by +construction and must never be resolved against the directory, whatever mappings +an admin hung off the record. + +**Note on the two directory predicates.** `handler.isDirectoryBacked` (SA-7) is +deny-by-default over "not `local` and not `applocal:*`"; `migrate.isDirectoryProvider` +is an allow-list of `ldap` / `kerberos` and their per-directory forms +(`ldap:`, `kerberos:`). The asymmetry is **intentional** — one returns a +verdict, the other must return a KEY — but it means an exotic provider (say `saml`) +is "directory" to the password gate and "not directory" to the migrator. An earlier +draft of this entry claimed that produced no exploit because such a user "has no +hash to export"; that was **not** substantiated and is withdrawn. The migrator's +allow-list now covers every provider form this repository documents +(`build.md` gives `ldap:corp` as the multi-directory example), and a provider +outside it classifies as local — so if a future provider is added, it must be added +to `isDirectoryProvider` in the same change. Do not unify the two predicates: +`internal/migrate` importing from `internal/handler` would invert the dependency +direction. + +**Wire break: `SchemaRev` 1 → 2.** Classification happens on the SOURCE side, so a +patched central cannot trust the `Kind` values in a rev-1 bundle. The exact-match +check in `guardMigrationCall` turns the bump into the right operator instruction +rather than a silent import of buggy classification. **In-flight migrations must +upgrade the older deployment first — this belongs in the release notes.** + +**Also fixed: dishonest preflight counts.** `Apply` skips a user with no effective +roles entirely (no assignment written, no local account created) while `Classify` +counted them as migrating, so preflight promised N and Apply delivered fewer with +nothing explaining the gap. `Report.NoRoles` now counts them separately, with a +note. The AD block checks still run for them — *"the central cannot authenticate +this person"* is worth saying regardless of whether they carry roles today. + +**Tests:** `internal/migrate/classify_test.go` — a directory user with a local hash +classifies as AD with **no** hash carried and `HadLocalPassword` set; an imported +LDAP user with no `SAMAccountName` still classifies as directory; a genuine local +user is unchanged and keeps their hash; an app-local user is never directory even +with a stray `SAMAccountName`; `directoryKey` is order-independent; and the +`NoRoles` accounting matches what `Apply` will do. The first two fail with the +precedence reverted. diff --git a/internal/migrate/bundle.go b/internal/migrate/bundle.go index 19c255a..5fd4302 100644 --- a/internal/migrate/bundle.go +++ b/internal/migrate/bundle.go @@ -28,7 +28,15 @@ import ( ) // SchemaRev is the bundle wire-format revision; bump on incompatible changes. -const SchemaRev = 1 +// +// 2 (H16): user classification moved from "PasswordHash first" to "directory +// identity first", and a KindAD entry no longer carries a password hash. That +// decision is made on the SOURCE side, so a patched central cannot trust the Kind +// values in a rev-1 bundle. The exact-match check in guardMigrationCall turns the +// bump into the right operator instruction — "incompatible migration bundle; +// upgrade the older deployment first" — instead of silently importing a bundle +// built by the buggy classifier. +const SchemaRev = 2 // UserKind is how a user authenticates, which determines how they migrate. type UserKind string @@ -93,6 +101,13 @@ type UserEntry struct { DisplayName string `json:"display_name,omitempty"` Email string `json:"email,omitempty"` PasswordHash string `json:"password_hash,omitempty"` + + // HadLocalPassword marks a KindAD user who ALSO held a local password on the + // source (a break-glass account, or one acquired via a reset path). The hash is + // NOT carried — see classifyUser — because the central re-binds this identity + // from AD. Only the boolean travels, so the dry run can tell the operator which + // logins do not survive the move. + HadLocalPassword bool `json:"had_local_password,omitempty"` } // Package builds a Bundle from a standalone deployment's store, capturing its home @@ -148,6 +163,22 @@ func Package(s store.Store, homeAppID, sourceVersion string) (*Bundle, error) { // classifyUser turns a source user into a portable UserEntry. ok=false skips a // user with no usable identity. +// +// PRECEDENCE (H16): a DIRECTORY identity beats a local credential, always. This +// used to test PasswordHash first, so any AD-backed user who ALSO carried a local +// hash migrated as an app-LOCAL shadow keyed by that credential. Three things went +// wrong at once: +// - the central ended up holding a standing password for someone whose identity +// is directory-governed, so the account outlived AD-side disablement, lockout +// and password policy — exactly what "no record or password is copied" in this +// package's doc comment promises never happens; +// - Classify never BLOCKS a local user, so a whole AD population misclassified +// as local sailed straight past the "central is on a different AD" guard; +// - Apply flipped allow_local_users on the target for accounts that must never +// use the app-local login path. +// +// So: resolve the directory key first. Only a user with NO directory identity at +// all is a local user. func classifyUser(s store.Store, u *store.User, defaultRoles []string) (UserEntry, bool) { roles, _ := s.GetUserRoles(u.GUID) if len(roles) == 0 { @@ -155,9 +186,36 @@ func classifyUser(s store.Store, u *store.User, defaultRoles []string) (UserEntr } direct, _ := s.GetUserPermissions(u.GUID) + // An app-local user (OwnerAppID set) is local BY CONSTRUCTION: created only by + // the app self-service path, authenticated locally, and never auto-provisioned + // from the directory (M12). Package has already dropped the ones owned by + // ANOTHER app, so an owner still set here is the home app's. Never resolve + // these against the directory, whatever mappings an admin may have hung off + // the record. + if u.OwnerAppID == "" { + key, err := directoryKey(s, u) + if err != nil { + // Cannot prove this user is local, so do not export a credential for + // them. Skipping is the safe direction: a missing user is visible in the + // preflight counts, a leaked hash is not. + return UserEntry{}, false + } + if key != "" { + // Directory-governed. The local hash — if any — is deliberately left + // behind: the central re-binds this person from the SAME AD, so copying + // the credential would recreate exactly the shadow account this + // precedence exists to prevent. HadLocalPassword (a bool, never the + // hash) lets the dry run tell the operator it did not travel. + return UserEntry{ + Kind: KindAD, Key: key, Roles: roles, DirectPerms: direct, + HadLocalPassword: u.PasswordHash != "", + }, true + } + } + if u.PasswordHash != "" { - username := localUsername(s, u) - if username == "" { + username, err := localUsername(s, u) + if err != nil || username == "" { return UserEntry{}, false } return UserEntry{ @@ -165,29 +223,115 @@ func classifyUser(s store.Store, u *store.User, defaultRoles []string) (UserEntr DisplayName: u.DisplayName, Email: u.Email, PasswordHash: u.PasswordHash, }, true } + return UserEntry{}, false +} + +// isDirectoryProvider reports whether an identity-mapping provider denotes a +// DIRECTORY identity (AD via LDAP or Kerberos) rather than a credential stored +// here. +// +// Both the bare names and the per-directory forms count. build.md documents +// multi-directory deployments as `ldap:corp` / `ldap:partner`, and +// handleSetMapping accepts an arbitrary provider string with no allow-list, so an +// exact match on "ldap" silently misses every deployment that followed the docs — +// their directory users would classify as local and have their password hashes +// exported, which is the whole defect H16 exists to prevent. +// +// "local" and "applocal:" are deliberately excluded: those ARE credentials +// stored here. +func isDirectoryProvider(provider string) bool { + switch provider { + case "ldap", "kerberos": + return true + } + return strings.HasPrefix(provider, "ldap:") || strings.HasPrefix(provider, "kerberos:") +} + +// directoryKey returns the AD key a directory-governed user travels under, or "" +// when the user has no directory identity at all. +// +// SAMAccountName is preferred when present, but it is NOT universal: it is written +// on the LDAP bind path (and self-healed by syncUserFromLDAP), while the Kerberos +// SPNEGO path writes only a `kerberos` mapping and leaves SAMAccountName empty +// (handleNegotiate), and handleImportLDAPUsers leaves it empty until the user's +// first login. Hence the mapping fallback — without it those users misclassify as +// local and have their password hash exported. +// +// The `local` provider is deliberately NOT consulted. On the LDAP password path a +// JIT provision writes BOTH an `ldap` and a `local` mapping, so `local` is present +// on many AD users too — which is exactly how localUsername happily returned an AD +// username under the old precedence. (The Kerberos path writes neither, so this +// exclusion is about the LDAP case specifically.) +// +// Ordering must not depend on the backend: GetMappingsForUser returns insertion +// order on Bolt and UNORDERED rows on Postgres, and an LDAP user commonly carries +// two mappings (the typed cname/UPN plus the real sAMAccountName). Preferring a +// candidate without "@" is a best-effort tiebreak toward the sAMAccountName form, +// not a guarantee of it — a Kerberos-only user's sole candidate is a +// realm-qualified `user@REALM` cname, and that is simply what they travel under. +// Ties break lexicographically, so a given mapping SET yields the same key on both +// backends. +// +// NOTE: internal/handler has its own directory predicate (isDirectoryBacked, +// deny-by-default over "not local and not applocal"). This one is an allow-list +// because it must produce a KEY, not a verdict. The asymmetry is intentional; do +// not unify them — internal/migrate importing from internal/handler would invert +// the dependency direction. +func directoryKey(s store.Store, u *store.User) (string, error) { if u.SAMAccountName != "" { - return UserEntry{Kind: KindAD, Key: u.SAMAccountName, Roles: roles, DirectPerms: direct}, true + return u.SAMAccountName, nil } - return UserEntry{}, false + mappings, err := s.GetMappingsForUser(u.GUID) + if err != nil { + // Fail CLOSED. Swallowing this would mean "no directory identity", which + // classifies the user as LOCAL and EXPORTS their password hash into the + // bundle — the exact outcome this precedence exists to prevent. Refuse to + // classify instead; Package drops the user and the operator sees a short + // bundle rather than a leaked credential. + return "", fmt.Errorf("mappings for %s: %w", u.GUID, err) + } + var cands []string + for _, m := range mappings { + if isDirectoryProvider(m.Provider) && m.ExternalID != "" { + cands = append(cands, m.ExternalID) + } + } + if len(cands) == 0 { + return "", nil + } + sort.Slice(cands, func(i, j int) bool { + iUPN, jUPN := strings.Contains(cands[i], "@"), strings.Contains(cands[j], "@") + if iUPN != jUPN { + return jUPN // a bare sAMAccountName sorts before a UPN/cname form + } + return cands[i] < cands[j] + }) + return cands[0], nil } // localUsername finds the login username for a local user. -func localUsername(s store.Store, u *store.User) string { - mappings, _ := s.GetMappingsForUser(u.GUID) +func localUsername(s store.Store, u *store.User) (string, error) { + mappings, err := s.GetMappingsForUser(u.GUID) + if err != nil { + // Same reasoning as directoryKey: swallowing this makes the function fall + // through to u.Email and export a credential under a key that is not the + // user's login. Refuse instead. + return "", fmt.Errorf("mappings for %s: %w", u.GUID, err) + } for _, m := range mappings { if m.Provider == "local" { - return m.ExternalID + return m.ExternalID, nil } } for _, m := range mappings { if strings.HasPrefix(m.Provider, "applocal:") { - return m.ExternalID + return m.ExternalID, nil } } if u.SAMAccountName != "" { - return u.SAMAccountName + return u.SAMAccountName, nil } - return u.Email + return u.Email, nil } // Report is the dry-run result the central computes before any write. @@ -195,11 +339,16 @@ type Report struct { SourceVersion string `json:"source_version"` TargetApp string `json:"target_app"` - ADUsersSameDomain int `json:"ad_users_same_domain"` // resolvable from the central's AD - ADUsersKnown int `json:"ad_users_known"` // already present in the central directory - LocalUsers int `json:"local_users"` // materialized as app-local users - Blocked []BlockedUser `json:"blocked,omitempty"` - Notes []string `json:"notes,omitempty"` + ADUsersSameDomain int `json:"ad_users_same_domain"` // resolvable from the central's AD + ADUsersKnown int `json:"ad_users_known"` // already present in the central directory + LocalUsers int `json:"local_users"` // materialized as app-local users + // NoRoles counts users the bundle carries that Apply will NOT migrate: with no + // effective roles there is nothing to grant, so no assignment is written and no + // local account is created. Counted separately so the dry run's numbers are the + // numbers the operator actually gets. + NoRoles int `json:"no_roles"` + Blocked []BlockedUser `json:"blocked,omitempty"` + Notes []string `json:"notes,omitempty"` RedirectURIsToReview []string `json:"redirect_uris_to_review,omitempty"` @@ -323,13 +472,29 @@ func Classify(b *Bundle, central store.Store, targetAppID, defaultAppID string) } directPermUsers := 0 + adWithLocalPassword := 0 for _, u := range b.Users { if len(u.DirectPerms) > 0 { directPermUsers++ } + if u.HadLocalPassword { + adWithLocalPassword++ + } + // Apply grants nothing for a user with no effective roles and does not even + // materialize a local account for them, so the report must not count them as + // migrating — preflight promised N and Apply delivered fewer, with nothing in + // the report explaining the gap. The AD BLOCK checks below still run either + // way: "the central cannot authenticate this person" is worth telling the + // operator regardless of whether they happen to carry roles today. + migrating := len(u.Roles) > 0 + if !migrating { + r.NoRoles++ + } switch u.Kind { case KindLocal: - r.LocalUsers++ + if migrating { + r.LocalUsers++ + } case KindAD: switch { case !centralHasAD: @@ -337,9 +502,11 @@ func Classify(b *Bundle, central store.Store, targetAppID, defaultAppID string) case !sameAD: r.Blocked = append(r.Blocked, BlockedUser{Key: u.Key, Reason: "central is on a different AD; key by UPN/email or connect the same AD"}) default: - r.ADUsersSameDomain++ - if known[u.Key] { - r.ADUsersKnown++ + if migrating { + r.ADUsersSameDomain++ + if known[u.Key] { + r.ADUsersKnown++ + } } } } @@ -348,6 +515,12 @@ func Classify(b *Bundle, central store.Store, targetAppID, defaultAppID string) if directPermUsers > 0 { r.Notes = append(r.Notes, fmt.Sprintf("%d user(s) have direct (non-role) permissions that are NOT carried in this version — re-grant via roles on the target app", directPermUsers)) } + if r.NoRoles > 0 { + r.Notes = append(r.Notes, fmt.Sprintf("%d user(s) carry no effective roles (no explicit role on the source and no default_roles) — they do NOT migrate: no assignment is written and no local account is created", r.NoRoles)) + } + if adWithLocalPassword > 0 { + r.Notes = append(r.Notes, fmt.Sprintf("%d AD user(s) also had a LOCAL password on the source; it is NOT carried — the central re-binds them from AD. Re-create any break-glass login deliberately on the central", adWithLocalPassword)) + } if b.SourceAD != nil && !centralHasAD { r.Notes = append(r.Notes, "source is AD-connected but the central is not — connect the central to the same AD to migrate AD users") } @@ -432,7 +605,11 @@ func Apply(b *Bundle, central store.Store, targetAppID, defaultAppID string, car app.SecretHash = b.App.SecretHash } for _, u := range b.Users { - if u.Kind == KindLocal { + // Only for an entry Apply will actually materialize. A zero-role local + // entry is skipped below, and Classify no longer counts it — opening the + // target's local-login gate for a user that is never created would weaken + // the app's authentication surface for nothing. + if u.Kind == KindLocal && len(u.Roles) > 0 { app.AllowLocalUsers = true break } diff --git a/internal/migrate/classify_test.go b/internal/migrate/classify_test.go new file mode 100644 index 0000000..1d02c1c --- /dev/null +++ b/internal/migrate/classify_test.go @@ -0,0 +1,268 @@ +package migrate + +import ( + "fmt" + "strings" + "testing" + + "simpleauth/internal/store" +) + +// TestClassifyDirectoryBeatsLocalPassword is the H16 regression. +// +// An AD-backed user who ALSO carries a local password hash must migrate as a +// DIRECTORY user (policy only, re-bound from the central's AD), not as an +// app-local shadow keyed by that credential. The old precedence tested +// PasswordHash first, so such a user's password travelled to the central and the +// resulting account outlived AD-side disablement — and, because Classify never +// blocks a local user, an entire AD population misclassified this way sailed past +// the "central is on a different AD" guard. +func TestClassifyDirectoryBeatsLocalPassword(t *testing.T) { + s := open(t) + // Exactly what the reset-password path (SA-7) or a break-glass admin produces: + // a directory user carrying a local hash, with both ldap and local mappings. + must(t, s.CreateUser(&store.User{ + GUID: "g-dual", DisplayName: "Dual", SAMAccountName: "dual", + PasswordHash: "$2a$10$notarealhashbutlongenoughxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", + })) + must(t, s.SetIdentityMapping("ldap", "dual", "g-dual")) + must(t, s.SetIdentityMapping("local", "dual", "g-dual")) + must(t, s.SetUserRoles("g-dual", []string{"admin"})) + + u, err := s.GetUser("g-dual") + if err != nil { + t.Fatalf("get: %v", err) + } + entry, ok := classifyUser(s, u, nil) + if !ok { + t.Fatal("user should be classifiable") + } + if entry.Kind != KindAD { + t.Fatalf("a directory user with a local hash must classify as AD, got %q (H16)", entry.Kind) + } + if entry.Key != "dual" { + t.Fatalf("key should be the sAMAccountName, got %q", entry.Key) + } + if entry.PasswordHash != "" { + t.Fatal("a directory user's local password hash must NOT travel in the bundle (H16)") + } + if !entry.HadLocalPassword { + t.Fatal("HadLocalPassword must flag that a local credential existed and was left behind") + } +} + +// TestClassifyImportedLDAPUserWithoutSAM covers the gap SAMAccountName alone +// leaves: handleImportLDAPUsers creates an "ldap"-mapped user with NO +// SAMAccountName, and it stays empty until first login. Such a user must still +// classify as directory, keyed off the mapping. +func TestClassifyImportedLDAPUserWithoutSAM(t *testing.T) { + s := open(t) + must(t, s.CreateUser(&store.User{GUID: "g-imp", DisplayName: "Imported"})) + must(t, s.SetIdentityMapping("ldap", "imported", "g-imp")) + must(t, s.SetUserRoles("g-imp", []string{"viewer"})) + + u, _ := s.GetUser("g-imp") + entry, ok := classifyUser(s, u, nil) + if !ok { + t.Fatal("imported LDAP user should be classifiable") + } + if entry.Kind != KindAD || entry.Key != "imported" { + t.Fatalf("want AD/imported, got %q/%q", entry.Kind, entry.Key) + } +} + +// TestClassifyGenuineLocalUserUnchanged guards against over-correction: a user +// with only a local mapping and a password is still a local user. +func TestClassifyGenuineLocalUserUnchanged(t *testing.T) { + s := open(t) + must(t, s.CreateUser(&store.User{ + GUID: "g-loc", DisplayName: "Local", Email: "l@x.test", + PasswordHash: "$2a$10$stillnotarealhashxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", + })) + must(t, s.SetIdentityMapping("local", "localuser", "g-loc")) + must(t, s.SetUserRoles("g-loc", []string{"clerk"})) + + u, _ := s.GetUser("g-loc") + entry, ok := classifyUser(s, u, nil) + if !ok { + t.Fatal("local user should be classifiable") + } + if entry.Kind != KindLocal { + t.Fatalf("want local, got %q", entry.Kind) + } + if entry.Key != "localuser" { + t.Fatalf("want key localuser, got %q", entry.Key) + } + if entry.PasswordHash == "" { + t.Fatal("a genuine local user's hash must still travel — that is how they log in on the central") + } +} + +// TestClassifyAppLocalUserIsNeverDirectory pins the OwnerAppID short-circuit: an +// app-local user is local by construction, whatever mappings hang off the record. +func TestClassifyAppLocalUserIsNeverDirectory(t *testing.T) { + s := open(t) + must(t, s.CreateUser(&store.User{ + GUID: "g-app", DisplayName: "AppUser", OwnerAppID: "shop", + SAMAccountName: "stray", // must be ignored + PasswordHash: "$2a$10$appuserhashxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", + })) + must(t, s.SetIdentityMapping("applocal:shop", "shopper", "g-app")) + must(t, s.SetUserRoles("g-app", []string{"buyer"})) + + u, _ := s.GetUser("g-app") + entry, ok := classifyUser(s, u, nil) + if !ok { + t.Fatal("app-local user should be classifiable") + } + if entry.Kind != KindLocal { + t.Fatalf("an app-local user must never classify as directory, got %q", entry.Kind) + } + if entry.Key != "shopper" { + t.Fatalf("want key shopper, got %q", entry.Key) + } +} + +// TestDirectoryKeyIsDeterministic pins backend-independent selection: +// GetMappingsForUser returns insertion order on Bolt and unordered rows on +// Postgres, and a user commonly carries both a UPN and a sAMAccountName form. The +// bare sAMAccountName must win, regardless of insertion order. +func TestDirectoryKeyIsDeterministic(t *testing.T) { + for _, order := range [][]string{ + {"dana@corp.local", "dana"}, + {"dana", "dana@corp.local"}, + } { + s := open(t) + must(t, s.CreateUser(&store.User{GUID: "g-d", DisplayName: "Dana"})) + for _, ext := range order { + must(t, s.SetIdentityMapping("ldap", ext, "g-d")) + } + u, _ := s.GetUser("g-d") + got, err := directoryKey(s, u) + if err != nil { + t.Fatalf("directoryKey: %v", err) + } + if got != "dana" { + t.Fatalf("insertion order %v: directoryKey = %q, want the bare sAMAccountName form %q", order, got, "dana") + } + } +} + +// TestClassifyNoIdentityIsSkipped — a user with neither a directory identity nor +// a password has nothing portable. +func TestClassifyNoIdentityIsSkipped(t *testing.T) { + s := open(t) + must(t, s.CreateUser(&store.User{GUID: "g-non", DisplayName: "Nobody"})) + u, _ := s.GetUser("g-non") + if _, ok := classifyUser(s, u, nil); ok { + t.Fatal("a user with no directory identity and no password must be skipped") + } +} + +// TestClassifyReportsNoRolesHonestly pins the preflight/Apply accounting gap: a +// user with no effective roles is not migrated by Apply, so the dry run must not +// promise them. +func TestClassifyReportsNoRolesHonestly(t *testing.T) { + c := open(t) + must(t, c.CreateApp(&store.App{AppID: "target", Audience: "target"})) + + b := &Bundle{ + SchemaRev: SchemaRev, + Users: []UserEntry{ + {Kind: KindLocal, Key: "with-roles", Roles: []string{"clerk"}, PasswordHash: "x"}, + {Kind: KindLocal, Key: "no-roles", PasswordHash: "x"}, + }, + } + rep, err := Classify(b, c, "target", "simpleauth") + if err != nil { + t.Fatalf("classify: %v", err) + } + if rep.LocalUsers != 1 { + t.Fatalf("only the role-carrying user migrates; LocalUsers = %d, want 1", rep.LocalUsers) + } + if rep.NoRoles != 1 { + t.Fatalf("NoRoles = %d, want 1", rep.NoRoles) + } + var noted bool + for _, n := range rep.Notes { + // Assert the note actually explains the gap, not merely that some note + // starting with "1" exists — the operator has to understand WHY the count + // they were shown is lower than the number of users in the bundle. + if strings.Contains(n, "no effective roles") && strings.Contains(n, "do NOT migrate") { + noted = true + } + } + if !noted { + t.Fatalf("expected a note explaining the non-migrating user, got: %v", rep.Notes) + } +} + +// errStore makes GetMappingsForUser fail so we can pin that classification fails +// CLOSED. Swallowing that error meant "no directory identity", which classified +// the user as LOCAL and exported their password hash — found by adversarial +// review of the first cut of this fix. +type errStore struct{ store.Store } + +func (errStore) GetMappingsForUser(string) ([]store.IdentityMapping, error) { + return nil, errTestMappings +} + +var errTestMappings = fmt.Errorf("simulated store failure") + +func TestClassifyFailsClosedOnMappingError(t *testing.T) { + base := open(t) + // Email matters: localUsername falls back to it, so WITHOUT the fail-closed + // guard the local branch succeeds and exports the hash. Omit it and the user + // is skipped for an unrelated reason and the test cannot fail. + must(t, base.CreateUser(&store.User{ + GUID: "g-err", DisplayName: "Err", Email: "err@corp.test", + PasswordHash: "$2a$10$hashthatmustnotescapexxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", + })) + u, _ := base.GetUser("g-err") + + // No SAMAccountName, so classification must consult mappings — which fail. + entry, ok := classifyUser(errStore{base}, u, nil) + if ok { + t.Fatalf("classification must fail closed when the directory lookup errors, got %+v", entry) + } + if entry.PasswordHash != "" { + t.Fatal("a password hash escaped into the bundle on a store error (H16)") + } +} + +// TestClassifyDirectoryProviderVariants covers the provider forms an adversarial +// review found missing: build.md documents multi-directory deployments as +// `ldap:corp` / `ldap:partner`, and handleSetMapping accepts an arbitrary provider +// string, so an exact match on "ldap" left every such deployment unfixed — their +// directory users classified as local and had their password hash exported. +func TestClassifyDirectoryProviderVariants(t *testing.T) { + directory := []string{"ldap", "kerberos", "ldap:corp", "ldap:partner", "kerberos:CORP.LOCAL"} + for _, prov := range directory { + s := open(t) + must(t, s.CreateUser(&store.User{ + GUID: "g-" + prov, DisplayName: "D", Email: "d@corp.test", + PasswordHash: "$2a$10$mustnotescapexxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", + })) + must(t, s.SetIdentityMapping(prov, "dana", "g-"+prov)) + u, _ := s.GetUser("g-" + prov) + + entry, ok := classifyUser(s, u, nil) + if !ok { + t.Errorf("provider %q: user should classify", prov) + continue + } + if entry.Kind != KindAD { + t.Errorf("provider %q: want KindAD, got %q — a directory user would export their hash", prov, entry.Kind) + } + if entry.PasswordHash != "" { + t.Errorf("provider %q: password hash escaped into the bundle", prov) + } + } + + // And the credential-bearing providers must NOT be treated as directory. + for _, prov := range []string{"local", "applocal:shop"} { + if isDirectoryProvider(prov) { + t.Errorf("provider %q must not count as a directory identity", prov) + } + } +} From a994b11af64885384eed1cb08970718f7a96af44 Mon Sep 17 00:00:00 2001 From: Your Name Date: Sat, 8 Aug 2026 20:50:16 +0300 Subject: [PATCH 2/2] chore(auth): render the Kerberos/LDAP diagnostic pages with html/template (M39) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL reported 10 open go/reflected-xss alerts in internal/handler/auth.go — the last of the class after #58 retired the oidc.go and hosted_login.go ones. Unlike those, which were the sanitizer-not-recognised false positive with every value already html.EscapeString-wrapped, THREE of these sites had no escaping at all: fmt.Fprintf(w, h.bp(negotiateTestKrbFailedHTML), "Invalid SPNEGO token: "+err.Error()) fmt.Fprintf(w, h.bp(negotiateTestKrbFailedHTML), "Kerberos ticket could not be parsed: "+err.Error()) err there is the result of parsing tokenBytes — the base64 payload of the caller's `Authorization: Negotiate` header. A fourth, adjacent site DID escape (html.EscapeString(err.Error())), which is what makes the omission a slip rather than a policy. The success page was worse in breadth: it reflected every AD attribute — displayName, mail, department, company, title, memberOf — raw into an HTML table. Those are attacker-influenced for any principal who can edit their own directory record. Exploitability is bounded and stated honestly: both endpoints register only when AUTH_ENABLE_TEST_ENDPOINTS=true, which defaults OFF and was already gated for exactly this reason under H1 (they perform live LDAP binds and are a password oracle). Whether a crafted SPNEGO token can drive '<' into a gokrb5 ASN.1 error string was NOT established — it depends on that library's error formatting. So: a genuine unescaped reflection of attacker-derived data, on a default-off endpoint, with uncertain end-to-end exploitability. Fixed on the merits rather than argued about. Approach: the same conversion #58 applied to the OIDC login page. Six Fprintf templates become html/template with a typed negotiateTestData struct, parsed once at package init via template.Must. {{BASE_PATH}} — which h.bp() substituted with strings.ReplaceAll BEFORE the Fprintf — becomes a real {{.BasePath}} field, so the base path is escaped for its context too and the bp() hop disappears on these paths. All manual html.EscapeString calls here are removed; the template owns escaping. One site deliberately NOT converted: the SPNEGO retry meta-refresh (). html/template classifies as contentTypeUnsafe — it attribute-escapes but does NOT URL-filter — so a rewrite would add no guarantee. retryURL is built server-side from h.url() plus url.QueryEscape'd values and is never echoed from the request; the EscapeString there is defence in depth. Recorded in a comment so it is not "cleaned up" later. Tests (internal/handler/negotiate_test_pages_test.go): error pages escape a script payload and are SINGLE-encoded (a surviving manual EscapeString would show as &lt;); the success page escapes AD-controlled attributes; every form page renders a real base path with no placeholder left behind; and the wait page still returns 401, which the SPNEGO handshake depends on — the conversion moved WriteHeader into a shared helper, exactly the kind of thing a refactor drops silently. Full suite green. Co-Authored-By: Claude Opus 5 (1M context) --- SECURITY-AUDIT.md | 64 +++++++ internal/handler/auth.go | 161 +++++++++++------- internal/handler/negotiate_test_pages_test.go | 123 +++++++++++++ 3 files changed, 290 insertions(+), 58 deletions(-) create mode 100644 internal/handler/negotiate_test_pages_test.go diff --git a/SECURITY-AUDIT.md b/SECURITY-AUDIT.md index 595c06c..431abad 100644 --- a/SECURITY-AUDIT.md +++ b/SECURITY-AUDIT.md @@ -112,6 +112,7 @@ source of truth for what is currently open vs. fixed. | 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) | +| M39 | Kerberos/LDAP diagnostic pages reflect SPNEGO parse errors and AD attributes into hand-built HTML; three sites had NO escaping | 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) | @@ -1249,3 +1250,66 @@ user is unchanged and keeps their hash; an app-local user is never directory eve with a stray `SAMAccountName`; `directoryKey` is order-independent; and the `NoRoles` accounting matches what `Apply` will do. The first two fail with the precedence reverted. +## Audit Pass 4 — M39 — diagnostic pages rendered with fmt.Fprintf + +CodeQL reported 10 open `go/reflected-xss` alerts in `internal/handler/auth.go`, +the last of the class after PR #58 retired the `oidc.go` / `hosted_login.go` ones. + +Unlike those — which were the sanitizer-not-recognised false positive, every value +already `html.EscapeString`-wrapped — **three of these sites had no escaping at +all**: + +```go +fmt.Fprintf(w, h.bp(negotiateTestKrbFailedHTML), "Invalid SPNEGO token: "+err.Error()) +fmt.Fprintf(w, h.bp(negotiateTestKrbFailedHTML), "Kerberos ticket could not be parsed: "+err.Error()) +``` + +`err` there is the result of parsing `tokenBytes`, which is the base64 payload of +the caller's `Authorization: Negotiate` header. A fourth site *was* escaped +(`html.EscapeString(err.Error())`), which is what makes the omission a slip rather +than a policy. + +The success page was worse in breadth: it reflected **every** AD attribute — +`displayName`, `mail`, `department`, `company`, `title`, `memberOf` — raw into an +HTML table. Those are attacker-influenced for any principal who can edit their own +directory record. + +**Exploitability is bounded, and honestly so:** both endpoints are registered only +when `AUTH_ENABLE_TEST_ENDPOINTS=true`, which defaults to **off** and was already +gated for exactly this reason under H1 (they perform live LDAP binds and are a +password oracle). Whether a crafted SPNEGO token can drive `<` into a gokrb5 ASN.1 +error string was not established — it depends on that library's error formatting. +So: a genuine unescaped reflection of attacker-derived data, on a default-off +endpoint, with uncertain end-to-end exploitability. Fixed on the merits rather +than argued about. + +**Approach.** The same conversion PR #58 applied to the OIDC login page: six +`fmt.Fprintf` templates become `html/template` with a typed `negotiateTestData` +struct, parsed once at package init via `template.Must`. `{{BASE_PATH}}` — which +`h.bp()` used to substitute with `strings.ReplaceAll` *before* the `Fprintf` — +becomes a real `{{.BasePath}}` field, so the base path is now escaped for its +context too and the `bp()` hop disappears. All manual `html.EscapeString` calls on +these paths are removed; the template owns escaping. + +`negotiateTestCSS` also carried `fmt`-escaped `%%` (`border-radius:50%%`, +`width:100%%`). `html/template` is not a format string, so those would have shipped +literally and broken every rule containing them — including on the three pages that +already used `fmt.Fprint` (no formatting) and were therefore silently broken +before this change too. Un-doubled, and the test asserts no `%%` survives into any +rendered page. + +One site is deliberately **not** converted: the SPNEGO retry meta-refresh +(``). `html/template` classifies +`` as `contentTypeUnsafe` — it attribute-escapes but does **not** +URL-filter — so a rewrite would add no guarantee. `retryURL` is built server-side +from `h.url()` plus `url.QueryEscape`'d values and is never echoed from the +request; the `EscapeString` there is defence in depth. The reasoning is recorded +in a comment so it is not "cleaned up" later. + +**Tests:** `internal/handler/negotiate_test_pages_test.go` — error pages escape a +script payload and are *single*-encoded (a surviving manual `EscapeString` would +show as `&lt;`); the success page escapes AD-controlled attributes; every form +page renders a real base path with no placeholder left behind; and the wait page +still returns **401**, which the SPNEGO handshake depends on — the conversion moved +`WriteHeader` into a shared helper, exactly the kind of thing a refactor drops +silently. diff --git a/internal/handler/auth.go b/internal/handler/auth.go index 9f37842..2815dcc 100644 --- a/internal/handler/auth.go +++ b/internal/handler/auth.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "html" + "html/template" "log" "net/http" "net/url" @@ -1088,9 +1089,7 @@ func (h *Handler) handleNegotiateTest(w http.ResponseWriter, r *http.Request) { if keytabPath != "" { w.Header().Set("WWW-Authenticate", "Negotiate") } - w.Header().Set("Content-Type", "text/html; charset=utf-8") - w.WriteHeader(http.StatusUnauthorized) - fmt.Fprint(w, h.bp(negotiateTestWaitHTML)) + h.renderNegotiateTest(w, negotiateTestWaitTmpl, http.StatusUnauthorized, negotiateTestData{}) return } @@ -1105,8 +1104,7 @@ func (h *Handler) handleNegotiateTest(w http.ResponseWriter, r *http.Request) { // Check if this is NTLM instead of Kerberos if isNTLMToken(tokenBytes) { // NTLM fallback: show form with explanation - w.Header().Set("Content-Type", "text/html; charset=utf-8") - fmt.Fprint(w, h.bp(negotiateTestNTLMFallbackHTML)) + h.renderNegotiateTest(w, negotiateTestNTLMFallbackTmpl, http.StatusOK, negotiateTestData{}) return } @@ -1137,8 +1135,7 @@ func (h *Handler) handleNegotiateTest(w http.ResponseWriter, r *http.Request) { return } // Can't parse as SPNEGO or raw AP-REQ — fall back to login form - w.Header().Set("Content-Type", "text/html; charset=utf-8") - fmt.Fprintf(w, h.bp(negotiateTestKrbFailedHTML), "Invalid SPNEGO token: "+err.Error()) + h.renderNegotiateTest(w, negotiateTestKrbFailedTmpl, http.StatusOK, negotiateTestData{Error: "Invalid SPNEGO token: " + err.Error()}) return } @@ -1148,8 +1145,7 @@ func (h *Handler) handleNegotiateTest(w http.ResponseWriter, r *http.Request) { } if len(spnegoToken.NegTokenInit.MechTokenBytes) == 0 { - w.Header().Set("Content-Type", "text/html; charset=utf-8") - fmt.Fprintf(w, h.bp(negotiateTestKrbFailedHTML), "No mechanism token in SPNEGO negotiation.") + h.renderNegotiateTest(w, negotiateTestKrbFailedTmpl, http.StatusOK, negotiateTestData{Error: "No mechanism token in SPNEGO negotiation."}) return } @@ -1158,8 +1154,7 @@ func (h *Handler) handleNegotiateTest(w http.ResponseWriter, r *http.Request) { if isNTLMToken(mechBytes) { log.Printf("[spnego] NTLM token detected inside SPNEGO") - w.Header().Set("Content-Type", "text/html; charset=utf-8") - fmt.Fprint(w, h.bp(negotiateTestNTLMFallbackHTML)) + h.renderNegotiateTest(w, negotiateTestNTLMFallbackTmpl, http.StatusOK, negotiateTestData{}) return } @@ -1170,8 +1165,7 @@ func (h *Handler) handleNegotiateTest(w http.ResponseWriter, r *http.Request) { if err := apReq.Unmarshal(mechBytes); err != nil { log.Printf("[spnego] AP-REQ unmarshal failed: %v, mechToken first bytes: %x", err, mechBytes[:min(32, len(mechBytes))]) // AP-REQ parse failed — fall back to login form - w.Header().Set("Content-Type", "text/html; charset=utf-8") - fmt.Fprintf(w, h.bp(negotiateTestKrbFailedHTML), "Kerberos ticket could not be parsed: "+err.Error()) + h.renderNegotiateTest(w, negotiateTestKrbFailedTmpl, http.StatusOK, negotiateTestData{Error: "Kerberos ticket could not be parsed: " + err.Error()}) return } @@ -1184,8 +1178,7 @@ func (h *Handler) handleNegotiateTest(w http.ResponseWriter, r *http.Request) { func (h *Handler) completeKerberosAuth(w http.ResponseWriter, r *http.Request, apReq *krbmsg.APReq, kt *keytab.Keytab) { username, cname, err := h.verifyAPReq(apReq, kt) if err != nil { - w.Header().Set("Content-Type", "text/html; charset=utf-8") - fmt.Fprintf(w, h.bp(negotiateTestKrbFailedHTML), "Kerberos verification failed: "+html.EscapeString(err.Error())) + h.renderNegotiateTest(w, negotiateTestKrbFailedTmpl, http.StatusOK, negotiateTestData{Error: "Kerberos verification failed: " + err.Error()}) return } @@ -1264,6 +1257,13 @@ func (h *Handler) handleSSOLogin(w http.ResponseWriter, r *http.Request) { w.Header().Set("WWW-Authenticate", "Negotiate") w.WriteHeader(http.StatusUnauthorized) // Redirect to self with sso_attempt=1 so we can detect failure + // retryURL is built server-side from h.url() + url.QueryEscape'd values — + // it is never echoed from the request. Escaped here for defence in depth. + // NOTE: html/template treats as contentTypeUnsafe (it + // attribute-escapes but does NOT URL-filter), so a template rewrite would + // not add a guarantee here; the safety rests on the server-side + // construction above. Left as an explicit EscapeString rather than moved + // into a template, deliberately. fmt.Fprintf(w, `

Authenticating...

`, html.EscapeString(retryURL)) return } @@ -1508,8 +1508,7 @@ func (h *Handler) handleNegotiateTestForm(w http.ResponseWriter, r *http.Request username := r.FormValue("username") password := r.FormValue("password") if username == "" || password == "" { - w.Header().Set("Content-Type", "text/html; charset=utf-8") - fmt.Fprint(w, h.bp(negotiateTestFormErrorHTML)) + h.renderNegotiateTest(w, negotiateTestFormErrorTmpl, http.StatusOK, negotiateTestData{}) return } @@ -1517,8 +1516,7 @@ func (h *Handler) handleNegotiateTestForm(w http.ResponseWriter, r *http.Request ldapCfg, ldapErr := h.getLDAPConfigDecrypted() if ldapErr != nil { log.Printf("[test-negotiate] No LDAP configured") - w.Header().Set("Content-Type", "text/html; charset=utf-8") - fmt.Fprintf(w, h.bp(negotiateTestLoginFailedHTML), "LDAP not configured") + h.renderNegotiateTest(w, negotiateTestLoginFailedTmpl, http.StatusOK, negotiateTestData{Error: "LDAP not configured"}) return } @@ -1532,8 +1530,7 @@ func (h *Handler) handleNegotiateTestForm(w http.ResponseWriter, r *http.Request errMsg = authErr.Error() } log.Printf("[test-negotiate] Auth failed for user=%q: %s", username, errMsg) - w.Header().Set("Content-Type", "text/html; charset=utf-8") - fmt.Fprintf(w, h.bp(negotiateTestLoginFailedHTML), html.EscapeString(errMsg)) + h.renderNegotiateTest(w, negotiateTestLoginFailedTmpl, http.StatusOK, negotiateTestData{Error: errMsg}) return } @@ -1699,21 +1696,20 @@ func (h *Handler) enrichUserInfoFromLDAP(userInfo map[string]string, username st // renderNegotiateSuccess renders the success page with user info. func (h *Handler) renderNegotiateSuccess(w http.ResponseWriter, userInfo map[string]string) { - w.Header().Set("Content-Type", "text/html; charset=utf-8") - fmt.Fprintf(w, h.bp(negotiateTestSuccessHTML), - userInfo["auth_method"], - mapGet(userInfo, "principal", "-"), - mapGet(userInfo, "realm", "-"), - mapGet(userInfo, "username", "-"), - mapGet(userInfo, "provider_name", "none"), - mapGet(userInfo, "provider_id", "-"), - mapGet(userInfo, "display_name", "-"), - mapGet(userInfo, "email", "-"), - mapGet(userInfo, "department", "-"), - mapGet(userInfo, "company", "-"), - mapGet(userInfo, "job_title", "-"), - mapGet(userInfo, "groups", "-"), - ) + h.renderNegotiateTest(w, negotiateTestSuccessTmpl, http.StatusOK, negotiateTestData{ + Method: userInfo["auth_method"], + Principal: mapGet(userInfo, "principal", "-"), + Realm: mapGet(userInfo, "realm", "-"), + Username: mapGet(userInfo, "username", "-"), + ProviderName: mapGet(userInfo, "provider_name", "none"), + ProviderID: mapGet(userInfo, "provider_id", "-"), + DisplayName: mapGet(userInfo, "display_name", "-"), + Email: mapGet(userInfo, "email", "-"), + Department: mapGet(userInfo, "department", "-"), + Company: mapGet(userInfo, "company", "-"), + JobTitle: mapGet(userInfo, "job_title", "-"), + Groups: mapGet(userInfo, "groups", "-"), + }) } func mapGet(m map[string]string, key, fallback string) string { @@ -1733,14 +1729,14 @@ body{font-family:system-ui,-apple-system,sans-serif;background:var(--bg);color:v .gold-bar{height:3px;background:linear-gradient(90deg,var(--gold-light),var(--gold-dark));border-radius:999px;margin-bottom:24px} .error{background:var(--error-bg);color:var(--error-text);padding:12px 16px;border-radius:8px;font-size:0.85rem;margin-bottom:16px} .success{background:var(--green-bg);color:var(--green);padding:16px;border-radius:8px;text-align:center;margin-bottom:24px;font-weight:600;font-size:1.1rem} -.spinner{display:inline-block;width:24px;height:24px;border:3px solid var(--border);border-top-color:var(--burgundy);border-radius:50%%;animation:spin 0.8s linear infinite;margin-bottom:16px} +.spinner{display:inline-block;width:24px;height:24px;border:3px solid var(--border);border-top-color:var(--burgundy);border-radius:50%;animation:spin 0.8s linear infinite;margin-bottom:16px} @keyframes spin{to{transform:rotate(360deg)}} label{display:block;font-size:0.875rem;font-weight:600;margin-bottom:6px} -input[type=text],input[type=password]{width:100%%;padding:10px 14px;background:var(--card);border:1px solid var(--border);border-radius:8px;font-size:0.875rem;font-family:inherit;color:var(--text);margin-bottom:14px} +input[type=text],input[type=password]{width:100%;padding:10px 14px;background:var(--card);border:1px solid var(--border);border-radius:8px;font-size:0.875rem;font-family:inherit;color:var(--text);margin-bottom:14px} input:focus{outline:none;border-color:var(--burgundy);box-shadow:0 0 0 3px rgba(139,21,61,0.15)} -button{width:100%%;padding:10px;background:var(--burgundy);color:#fff;border:none;border-radius:8px;font-size:0.875rem;font-weight:600;cursor:pointer;font-family:inherit} +button{width:100%;padding:10px;background:var(--burgundy);color:#fff;border:none;border-radius:8px;font-size:0.875rem;font-weight:600;cursor:pointer;font-family:inherit} button:hover{background:var(--burgundy-hover)} -table{width:100%%;border-collapse:collapse} +table{width:100%;border-collapse:collapse} th,td{text-align:left;padding:10px 12px;border-bottom:1px solid var(--border)} th{font-size:0.8rem;text-transform:uppercase;color:var(--muted);width:130px} td{font-size:0.9rem} @@ -1750,6 +1746,55 @@ td{font-size:0.9rem} #fallback{display:none} ` +// negotiateTestData drives the Kerberos/LDAP diagnostic pages. +// +// These pages are rendered with html/template rather than fmt.Fprintf because +// every value below originates outside SimpleAuth: Error derives from parsing the +// attacker-supplied `Authorization: Negotiate` header, and the success-page fields +// are AD attributes (displayName, mail, department, memberOf) that a directory +// principal may control. Three of the old call sites passed err.Error() with NO +// escaping at all, and the success page reflected every AD attribute raw (M39). +// +// html/template also escapes per CONTEXT, which matters here: BasePath lands in a +// form action (URL context) while the rest land in HTML text. +type negotiateTestData struct { + BasePath string + Error string + + // Success page. + Method string + Principal string + Realm string + Username string + ProviderName string + ProviderID string + DisplayName string + Email string + Department string + Company string + JobTitle string + Groups string +} + +var ( + negotiateTestWaitTmpl = template.Must(template.New("negoWait").Parse(negotiateTestWaitHTML)) + negotiateTestNTLMFallbackTmpl = template.Must(template.New("negoNTLM").Parse(negotiateTestNTLMFallbackHTML)) + negotiateTestFormErrorTmpl = template.Must(template.New("negoFormErr").Parse(negotiateTestFormErrorHTML)) + negotiateTestKrbFailedTmpl = template.Must(template.New("negoKrbFail").Parse(negotiateTestKrbFailedHTML)) + negotiateTestLoginFailedTmpl = template.Must(template.New("negoLoginFail").Parse(negotiateTestLoginFailedHTML)) + negotiateTestSuccessTmpl = template.Must(template.New("negoSuccess").Parse(negotiateTestSuccessHTML)) +) + +// renderNegotiateTest executes a diagnostic template, always supplying BasePath. +func (h *Handler) renderNegotiateTest(w http.ResponseWriter, t *template.Template, status int, d negotiateTestData) { + d.BasePath = h.cfg.BasePath + w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.WriteHeader(status) + if err := t.Execute(w, d); err != nil { + log.Printf("[negotiate-test] render: %v", err) + } +} + const negotiateTestWaitHTML = ` Authentication Test — SimpleAuth @@ -1764,7 +1809,7 @@ const negotiateTestWaitHTML = `

Sign In

Kerberos not available — enter your AD credentials

-
+ @@ -1785,7 +1830,7 @@ const negotiateTestNTLMFallbackHTML = `

Kerberos unavailable (browser sent NTLM) — use credentials instead

Your browser could not obtain a Kerberos ticket and fell back to NTLM. Check that the SPN matches the URL hostname and you are on the domain.
- + @@ -1802,7 +1847,7 @@ const negotiateTestFormErrorHTML = `

Sign In

Username and password are required.
- + @@ -1819,8 +1864,8 @@ const negotiateTestKrbFailedHTML = `

Sign In

Kerberos authentication failed — use credentials instead

-
%s
- +
{{.Error}}
+ @@ -1836,8 +1881,8 @@ const negotiateTestLoginFailedHTML = `

Sign In

-
Authentication failed: %s
- +
Authentication failed: {{.Error}}
+ @@ -1854,16 +1899,16 @@ const negotiateTestSuccessHTML = `
Authentication Successful

Authenticated User

- - - - - - - - - - - + + + + + + + + + + +
Method%s
Principal%s
Realm%s
Username%s
LDAP Provider%s (%s)
Display Name%s
Email%s
Department%s
Company%s
Job Title%s
Groups%s
Method{{.Method}}
Principal{{.Principal}}
Realm{{.Realm}}
Username{{.Username}}
LDAP Provider{{.ProviderName}} ({{.ProviderID}})
Display Name{{.DisplayName}}
Email{{.Email}}
Department{{.Department}}
Company{{.Company}}
Job Title{{.JobTitle}}
Groups{{.Groups}}
` diff --git a/internal/handler/negotiate_test_pages_test.go b/internal/handler/negotiate_test_pages_test.go new file mode 100644 index 0000000..38cb281 --- /dev/null +++ b/internal/handler/negotiate_test_pages_test.go @@ -0,0 +1,123 @@ +package handler + +import ( + "html/template" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestNegotiateDiagnosticPagesEscapeErrors is the M39 regression. +// +// Three call sites passed err.Error() with NO escaping at all (the SPNEGO +// unmarshal, raw-AP-REQ and ticket-parse failures), and err derives from the +// attacker-supplied `Authorization: Negotiate` header. +func TestNegotiateDiagnosticPagesEscapeErrors(t *testing.T) { + h, _ := testSetup(t) + const payload = `` + + for name, tmpl := range map[string]*template.Template{ + "krbFailed": negotiateTestKrbFailedTmpl, + "loginFailed": negotiateTestLoginFailedTmpl, + } { + rec := httptest.NewRecorder() + h.renderNegotiateTest(rec, tmpl, http.StatusOK, negotiateTestData{Error: payload}) + body := rec.Body.String() + + if strings.Contains(body, "") { + t.Errorf("%s: reflected an unescaped script tag (M39)", name) + } + if !strings.Contains(body, "<script>") { + t.Errorf("%s: payload should appear HTML-escaped", name) + } + // Single-encoded, not double: a surviving manual html.EscapeString would + // produce &lt; and ship visibly mangled text to the operator. + if strings.Contains(body, "&lt;") { + t.Errorf("%s: payload double-escaped — a manual EscapeString survived the conversion", name) + } + } +} + +// TestNegotiateSuccessPageEscapesDirectoryAttributes covers the AD-controlled +// fields. displayName / department / memberOf are attacker-influenced for anyone +// who can edit their own directory record, and were reflected raw. +func TestNegotiateSuccessPageEscapesDirectoryAttributes(t *testing.T) { + h, _ := testSetup(t) + const imgPayload = `` + + rec := httptest.NewRecorder() + h.renderNegotiateSuccess(rec, map[string]string{ + "auth_method": "Kerberos", + "display_name": imgPayload, + "department": imgPayload, + "groups": imgPayload, + "email": `">`, + }) + body := rec.Body.String() + + for _, bad := range []string{"alert(2)"} { + if strings.Contains(body, bad) { + t.Errorf("success page reflected an unescaped AD attribute: %q (M39)", bad) + } + } + if !strings.Contains(body, "<img") { + t.Error("AD attribute should appear escaped") + } +} + +// TestNegotiateDiagnosticPagesRenderBasePath pins the {{BASE_PATH}} → {{.BasePath}} +// migration. A missed occurrence leaves a literal placeholder in the form action, +// silently breaking the diagnostic form's POST target — and a stray {{ would have +// panicked template.Must at package init instead, taking every test with it. +func TestNegotiateDiagnosticPagesRenderBasePath(t *testing.T) { + h, _ := testSetup(t) + h.cfg.BasePath = "/sauth" + + // The five pages that carry a form. + pages := map[string]struct { + tmpl *template.Template + status int + }{ + "wait": {negotiateTestWaitTmpl, http.StatusUnauthorized}, + "ntlm": {negotiateTestNTLMFallbackTmpl, http.StatusOK}, + "formError": {negotiateTestFormErrorTmpl, http.StatusOK}, + "krbFailed": {negotiateTestKrbFailedTmpl, http.StatusOK}, + "loginFailed": {negotiateTestLoginFailedTmpl, http.StatusOK}, + } + for name, p := range pages { + rec := httptest.NewRecorder() + h.renderNegotiateTest(rec, p.tmpl, p.status, negotiateTestData{Error: "x"}) + body := rec.Body.String() + + if strings.Contains(body, "{{BASE_PATH}}") || strings.Contains(body, "{{.BasePath}}") { + t.Errorf("%s: an unrendered base-path placeholder survived", name) + } + // negotiateTestCSS used to carry fmt-escaped "%%" for the Fprintf paths. + // html/template is not a format string, so those would ship literally and + // break every rule containing them (border-radius:50%%, width:100%%). + if strings.Contains(body, "%%") { + t.Errorf("%s: fmt-escaped %%%% survived into the rendered CSS", name) + } + if !strings.Contains(body, `action="/sauth/test-negotiate"`) { + t.Errorf("%s: form action should carry the configured base path", name) + } + } +} + +// TestNegotiateWaitPageKeeps401 guards the status code the SPNEGO handshake +// depends on: the wait page MUST be a 401 or the browser never retries with a +// Negotiate token. The conversion moved WriteHeader into the shared helper, so +// this is exactly the kind of thing a refactor can silently drop. +func TestNegotiateWaitPageKeeps401(t *testing.T) { + h, _ := testSetup(t) + rec := httptest.NewRecorder() + h.renderNegotiateTest(rec, negotiateTestWaitTmpl, http.StatusUnauthorized, negotiateTestData{}) + + if rec.Code != http.StatusUnauthorized { + t.Fatalf("wait page must render 401 (the SPNEGO challenge), got %d", rec.Code) + } + if ct := rec.Header().Get("Content-Type"); !strings.HasPrefix(ct, "text/html") { + t.Fatalf("content-type = %q", ct) + } +}