From 7996f811573486b7c8aae22b85c6ce7cb2fd263c Mon Sep 17 00:00:00 2001 From: Your Name Date: Sat, 8 Aug 2026 20:43:55 +0300 Subject: [PATCH] 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) + } + } +}