scadaproj/code-reviews/Auth/findings.md

# Code Review — Auth

| Field | Value |
|-------|-------|
| Library | `ZB.MOM.WW.Auth/` |
| Packages | `ZB.MOM.WW.Auth.Abstractions`, `ZB.MOM.WW.Auth.Ldap`, `ZB.MOM.WW.Auth.ApiKeys`, `ZB.MOM.WW.Auth.AspNetCore` |
| Component spec | `components/auth/spec/SPEC.md` |
| Shared contract | `components/auth/shared-contract/ZB.MOM.WW.Auth.md` |
| Status | Reviewed |
| Last reviewed | 2026-11-09 (re-review; baseline 2026-06-01 at `5f75cd4`) |
| Reviewer | Claude (2026-11-09 re-review, scadaproj#15; baseline: Claude automated) |
| Commit reviewed | `91fb2b4` |
| Open findings | 0 |

## Summary

**2026-11-09 re-review (`91fb2b4`, scadaproj#15).** Re-reviewed at `91fb2b4` (on-disk version `0.4.3`; the feed carries `0.4.1`), covering everything added
since the `5f75cd4` baseline: LDAP `FallbackServers` failover with a sticky preferred endpoint (0.2.0), the AD
continuation-referral skip (0.2.1), the new `ZB.MOM.WW.Auth.ApiKeys.SqlServer` package (stores, migrator that
copes with concurrent boots, SQLite importer, `AddZbApiKeyAuthSqlServer`) (0.3.0), opt-in LDAP user bind
(`BindMode=User` / `UserBindTemplate` / `AcceptDomainQualifiedNames`) (0.4.0), and the canonical username plus
empty-password refusal (0.4.1). Auth-001..006 remain correctly resolved, and the new code holds up on its main
security points:
- **One connection per attempt.** Every endpoint attempt gets a fresh `ILdapConnection`, disposed by `using` on
  every path, including failover.
- **Failover is system-side only.** A `BadCredentials`/49 answer is final and is never replayed against another
  DC.
- **Empty passwords are refused before any network call in both modes**, which closes the unauthenticated-bind
  hole.
- **Filter values are still RFC 4515-escaped.**
- **The SQL Server store is parameterised throughout.** The schema name passes a strict ASCII allow-list
  re-checked at the public DDL entry point.
- **Concurrent first-boot migration converges.** The retry on 1205/2714/1913/2627 with jitter works: the live
  suite passes 92/92 against a throwaway SQL Server 2022, and the DBA script plus a row-only login migrate cleanly.
- **The SQL Server DI extension has container-building tests.**

The main new issue is in user-bind mode. The account that binds and the entry whose groups and username are
returned are found independently: the first from `UserBindTemplate`, the second from a `UserNameAttribute`
search. Nothing checks that they are the same object, so a UPN template, which the validator's own error text
offers first, can sign one user in with another user's groups (Auth-007). The remaining findings are Low:
- **Silent truncation in the SQL Server store.** The store sets explicit `SqlParameter` sizes and uses
  case-insensitive key ids, so it does not match SQLite's semantics (Auth-008).
- **Username from a multi-valued attribute.** The canonical username takes the first value of a possibly
  multi-valued attribute (Auth-009).
- **Store faults escape the verifier.** `ApiKeyVerifier` still lets store exceptions out, which matters more now
  that the store can be a network database (Auth-010).

Offline suite: 331 passed / 76 skipped (live-gated). With `AUTH_SQLSERVER_CONNSTR` set, the SqlServer suite
runs 92/92.

**2026-06-01 baseline (`5f75cd4`).** The security-critical core of this library is sound: API-key secrets are 32-byte CSPRNG values,
hashing is HMAC-SHA256 keyed by an external pepper, verification uses
`CryptographicOperations.FixedTimeEquals`, the SQLite stores use parameterized SQL throughout,
LDAP filter values are RFC 4515-escaped before being interpolated into the search filter, group-DN
parsing is RFC 4514 escape-aware, and the LDAP service is genuinely fail-closed (never throws,
requires exactly one match plus a verified password plus >=1 group for success). Test coverage is
strong and contract-focused (~172 tests; every failure mode, the parser, the hasher, the escaper,
the stores, the migrator and the DI wiring are exercised). The findings below are about
robustness-around-the-edges and documentation, not the cryptographic core. The two themes are
(1) two narrow paths that can turn a recoverable condition into a thrown exception on the
authentication hot path, contradicting the library's own "only exception path is cancellation" /
"fail fast at startup" promises, and (2) public-facing documentation (README) that misstates the
hashing algorithm and the AspNetCore surface. The library is **not yet adopted** by the three apps
(tracked in `components/auth/GAPS.md`).

## Checklist coverage

2026-11-09 re-review at `91fb2b4`:

| # | Category | Examined | Notes |
|---|----------|----------|-------|
| 1 | Correctness & logic bugs | ☑ | Failover walk, sticky index, referral skip, `LdapUserName` normaliser and the expiry check are correct. See Auth-008 (silent truncation / case-insensitive key ids diverge from SQLite) and Auth-009 (canonical username = first value of a multi-valued attribute) |
| 2 | Public API surface & compatibility | ☑ | `IApiKeyStoreMigrator` seam is clean and the old `SqliteAuthStoreMigrator` ctor is kept. Two changes are 0.x source/binary breaks with no other effect: the new `IApiKeyAdminStore` members and the optional parameter on `ApiKeyRecord`. No issues found |
| 3 | Concurrency & thread safety | ☑ | The sticky `_preferredIndex` uses `Volatile`, so its race is benign. The SQL Server migrator's retry with jitter converged under the live 8-way test. Stores open one connection per operation. No issues found |
| 4 | Error handling & resilience | ☑ | `LdapAuthService` stays fail-closed on every stage and `MarkUsedAsync` is best-effort (Auth-002 holds). See Auth-010: store lookup faults (e.g. `SqlException` on a shared-DB outage) still escape `ApiKeyVerifier`, despite its "only cancellation" contract |
| 5 | Security & secret handling | ☑ | Empty passwords are refused in both modes, BadCredentials is never failed over, filter escaping is intact, SQL is parameterised and the schema identifier is allow-listed. See Auth-007 (user-bind mode reads a different entry from the one that bound). DN templates are not escaped, but README documents this |
| 6 | Performance & resource management | ☑ | `ILdapConnection` is disposed via `using` inside the per-attempt try (every path, incl. failover). SQL connections, commands and readers use `await using`, and `OpenConnectionAsync` disposes on a failed open. No issues found |
| 7 | Spec & shared-contract adherence | ☑ | SQL Server rows mirror SQLite column-for-column, apart from Auth-008. The `IApiKeyVerifier` "only cancellation" contract is violated (Auth-010) |
| 8 | Packaging, dependencies & project layout | ☑ | New SqlServer package is cleanly split and depends only on Abstractions-level `Microsoft.Extensions.*`. The Pkcs/SQLitePCLRaw transitive pins are present. No issues found |
| 9 | Testing coverage | ☑ | Strong: failover, user-bind, referral, normaliser, validator, live SQL Server suites, plus DI tests that build containers for `AddZbApiKeyAuthSqlServer`, `AddZbApiKeyAuth` and `AddZbLdapAuth`. Gaps: no test ties the bound identity to the returned entry (Auth-007), no over-length or case-variant key-id test (Auth-008), no multi-valued username attribute test (Auth-009), no store-throws verifier test (Auth-010) |
| 10 | Documentation & XML docs | ☑ | Thorough. `UserBindTemplate` docs warn about UPN mismatch, but the validator message offers the UPN form first (Auth-007). `ApiKeyVerifier` remark overstates its exception contract (Auth-010) |

<details><summary>2026-06-01 baseline checklist (<code>5f75cd4</code>)</summary>

| # | Category | Examined | Notes |
|---|----------|----------|-------|
| 1 | Correctness & logic bugs | ☑ | Core logic correct; see Auth-003 (JSON parse can throw on hot path), Auth-005 (KeyPrefix value inconsistency) |
| 2 | Public API surface & compatibility | ☑ | Surface is minimal, sealed records, correct nullable annotations, internal seams hidden via `InternalsVisibleTo`. No issues found |
| 3 | Concurrency & thread safety | ☑ | Options/records immutable; services stateless singletons; fresh SQLite connection per op with WAL + busy_timeout. No issues found |
| 4 | Error handling & resilience | ☑ | LDAP path fully fail-closed; but see Auth-001 (no `ValidateOnStart`), Auth-002 (`MarkUsedAsync` failure fails a valid auth), Auth-003 |
| 5 | Security & secret handling | ☑ | Peppered HMAC-SHA256 + FixedTimeEquals, CSPRNG secrets, parameterized SQL, LDAP escaping, hash-free list projection, secrets never logged. See Auth-006 (TLS cert-validation hook) |
| 6 | Performance & resource management | ☑ | Connections disposed via `await using`; `LdapConnection` disposed; no hot-path allocs of note. No issues found |
| 7 | Spec & shared-contract adherence | ☑ | Matches SPEC §1–§5 closely (HMAC-SHA256 pepper, constant-time compare, discriminated failures, cookie hardening). See Auth-001 (insecure-refusal deferred to first use) |
| 8 | Packaging, dependencies & project layout | ☑ | Clean 4-package split, central versions, minimal deps, Abstractions has no runtime deps. No issues found |
| 9 | Testing coverage | ☑ | Broad and contract-level; gaps: no test for corrupt-`scopes` deserialization (Auth-003) or `MarkUsedAsync`-throws (Auth-002); `ValidateOnStart` behavior untested (Auth-001) |
| 10 | Documentation & XML docs | ☑ | XML docs thorough and accurate; README is stale — see Auth-004 |

</details>

## Findings

### Auth-001 — LDAP options validator is registered but never runs at startup

| | |
|--|--|
| Severity | Medium |
| Category | Error handling & resilience |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.AspNetCore/ServiceCollectionExtensions.cs:40` |

**Description**

`AddZbLdapAuth` binds `LdapOptions` with `services.Configure<LdapOptions>(...)` and registers the
validator with a plain `services.AddSingleton<IValidateOptions<LdapOptions>, LdapOptionsValidator>()`.
It never calls `OptionsBuilder.ValidateOnStart()`. With `Microsoft.Extensions.Options`, an
`IValidateOptions<T>` runs only when the options instance is first materialized (`IOptions<T>.Value`),
not at host startup. Here the only consumer that materializes the options is the `ILdapAuthService`
singleton factory (line 48), whose lambda runs on first resolution of `ILdapAuthService` — i.e. on
the first login attempt, not at boot.

The result contradicts the explicit promise in both the method comment ("Fail fast at startup on a
misconfigured directory rather than on first login") and the validator's own XML doc ("Validates
`LdapOptions` at startup so a misconfiguration fails fast at boot"). It also weakens SPEC §2 step 1,
which says an insecure transport without `AllowInsecure` must be *refused* — that refusal is silently
deferred until a user tries to log in. A process can therefore boot green with `Transport=None` and
no `AllowInsecure`, and only surface the misconfiguration as a failed login later. Because this is a
shared library consumed by three hosts, the missing guard affects all of them.

**Recommendation**

Register options via the builder and add `ValidateOnStart`:

```csharp
services.AddOptions<LdapOptions>()
    .Bind(config.GetSection(sectionPath))
    .ValidateOnStart();
services.AddSingleton<IValidateOptions<LdapOptions>, LdapOptionsValidator>();
```

(`ValidateOnStart` relies on the host's start-time options validation, which the ASP.NET Core hosts
already run.) Add a test asserting that building + starting the host fails for an insecure config.

**Resolution**

Resolved in `544a6dd`, 2026-06-01 — `AddZbLdapAuth` now binds via `AddOptions<LdapOptions>().Bind(...).ValidateOnStart()` so the validator runs at host start, not on first login (test: `ServiceCollectionExtensionsTests.AddZbLdapAuth_StartingHost_FailsForInsecureConfig`, plus a `...SucceedsForSecureConfig` companion).

### Auth-002 — A failed `MarkUsedAsync` write turns a valid API key into a thrown exception

| | |
|--|--|
| Severity | Medium |
| Category | Error handling & resilience |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/ApiKeyVerifier.cs:66` |

**Description**

After a successful constant-time secret match, `VerifyAsync` does
`await store.MarkUsedAsync(record.KeyId, _timeProvider.GetUtcNow(), ct)` before returning the
success result. `MarkUsedAsync` is a best-effort "last used" bookkeeping write. If that write fails
(SQLite still `SQLITE_BUSY` after the 5 s busy-timeout, disk full, DB locked by a long migration,
etc.), the exception propagates out of `VerifyAsync` and the otherwise-valid credential fails
authentication. The verifier's own class remark states "The only exception path is cancellation",
so callers (gRPC interceptors / middleware) will not be wrapping this in a try/catch and a transient
storage hiccup becomes an auth outage for a legitimate caller.

**Recommendation**

Treat the usage write as best-effort: either swap the order (return success, then fire-and-record),
or wrap the `MarkUsedAsync` call so a failure is logged/swallowed rather than failing the
verification — the authentication decision has already been made by line 60. If `MarkUsedAsync` is
allowed to throw, update the XML remark to say so. Add a test where the store's `MarkUsedAsync`
throws and assert the result is still `Succeeded == true`.

**Resolution**

Resolved in `544a6dd`, 2026-06-01 — `VerifyAsync` now wraps the best-effort `MarkUsedAsync` write in try/catch, re-throwing only `OperationCanceledException` and swallowing all other failures so a valid key still authenticates (tests: `ApiKeyVerifierTests.VerifyAsync_ValidKey_MarkUsedThrows_StillSucceeds` and `..._MarkUsedThrowsOperationCanceled_Propagates`).

### Auth-003 — Corrupt `scopes`/`constraints` column throws `JsonException` through the verifier

| | |
|--|--|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Sqlite/ScopeSerializer.cs:31`, `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Sqlite/SqliteApiKeyStore.cs:71` |

**Description**

`SqliteApiKeyStore.ReadRecord` calls `ScopeSerializer.Deserialize(reader.GetString(4))`, which does
`JsonSerializer.Deserialize<string[]>(value)`. If the `scopes` column ever holds non-array or
malformed JSON (operator tampering, a partial write, a future schema/format change, or a bug in
another writer), `Deserialize` throws `JsonException`. That exception propagates out of
`FindByKeyIdAsync`, and therefore out of `ApiKeyVerifier.VerifyAsync`, as an unhandled exception —
again contradicting the "only exception path is cancellation" contract and converting a single
poisoned row into a crash on the auth path rather than a structured fail-closed result. The same
applies to the admin `ListAsync` projection.

**Recommendation**

Decide the policy explicitly. Either (a) catch `JsonException` in `ScopeSerializer.Deserialize` and
treat a malformed column as an empty/invalid scope set so verification fails closed structurally, or
(b) catch it at the store boundary and surface it as a controlled error. Add a test that inserts a
row with a malformed `scopes` value and asserts the verifier returns a structured failure (or a
documented exception) rather than an unhandled `JsonException`.

**Resolution**

Resolved in `544a6dd`, 2026-06-01 — `ScopeSerializer.Deserialize` now catches `JsonException` and fails closed to an empty scope set (policy (a)), so a corrupt `scopes`/`constraints` column degrades to a zero-scope identity instead of throwing through the store and verifier (tests: `SqliteApiKeyStoreTests.ScopeSerializer_DeserializeMalformed_ReturnsEmptySet_DoesNotThrow` (Theory) and `..._FindByKeyId_CorruptScopesColumn_ReturnsRecordWithEmptyScopes_DoesNotThrow`).

### Auth-004 — README misstates the hashing algorithm and the AspNetCore public surface

| | |
|--|--|
| Severity | Low |
| Category | Documentation & XML docs |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/README.md:13`, `ZB.MOM.WW.Auth/README.md:14` |

**Description**

The README's package table contains two inaccuracies that matter for a security library:

1. It describes `ZB.MOM.WW.Auth.ApiKeys` as a store "with pepper-based **PBKDF2** hashing". The code
   uses **HMAC-SHA256** keyed by the pepper (`ApiKeySecretHasher.Hash` → `new HMACSHA256(pepperBytes)`),
   not PBKDF2. This is the correct algorithm per SPEC §4 ("HMAC-SHA256 with an external pepper"), so
   the *code* is right and the *README* is wrong — but a reader auditing the crypto will be misled
   about iteration count / KDF properties that do not exist here.
2. It says `ZB.MOM.WW.Auth.AspNetCore` provides "ASP.NET Core DI helpers (`AddZbAuth`) ... and
   `LdapOptionsValidator` registration. **Wires together Ldap + ApiKeys + cookie middleware**." The
   actual public surface is a single extension method `AddZbLdapAuth` (LDAP only). There is no
   `AddZbAuth`, no API-key re-export from this package, and no cookie-middleware wiring —
   `ZbCookieDefaults.Apply` is a helper the consumer must call itself, and API-key DI lives in the
   separate `ZB.MOM.WW.Auth.ApiKeys` package (`AddZbApiKeyAuth`). The shared contract
   (`ZB.MOM.WW.Auth.md`) likewise lists a combined DI shape that the code splits differently.

**Recommendation**

Fix the README table: change "PBKDF2" to "HMAC-SHA256", and correct the AspNetCore row to describe
the real surface (`AddZbLdapAuth`, `ZbCookieDefaults.Apply`, `ZbClaimTypes`) and note that API-key
wiring is `AddZbApiKeyAuth` in the ApiKeys package. Optionally reconcile the shared-contract doc's
DI-method names with the shipped names.

**Resolution**

Resolved in `544a6dd`, 2026-06-01 — README package table corrected: "PBKDF2" → "pepper-keyed HMAC-SHA256" and the AspNetCore row rewritten to the real surface (`AddZbLdapAuth`, `ZbCookieDefaults.Apply`, `ZbClaimTypes`, LDAP-only, no API-key/cookie-middleware wiring), with the stale `ApiKeys` dependency dropped from that row to match the `.csproj` (test: doc-only).

### Auth-005 — `CreateKeyAsync` persists `KeyPrefix` as `prefix_keyId`, inconsistent with the read path

| | |
|--|--|
| Severity | Low |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Admin/ApiKeyAdminCommands.cs:104` |

**Description**

When creating a key, `KeyPrefix` is set to `$"{_options.TokenPrefix}_{keyId}"` (e.g. `mxgw_key-1`),
embedding the key id into the column. Elsewhere `KeyPrefix` is treated as the bare token prefix:
the verifier test (`ApiKeyVerifierTests.BuildRecord`) and the store test
(`SqliteApiKeyStoreTests.SampleRecord`) seed it as `"mxgw"` / `"mxgw_ab12"`, and the `ApiKeyRecord`
contract gives no indication it should include the key id. `KeyPrefix` is not consulted during
verification (the token is re-parsed from the header), so there is no security impact — but the
persisted/displayed value is inconsistent and self-referential (the key id is already its own
column), which will confuse admin tooling that surfaces `KeyPrefix`.

**Recommendation**

Persist `KeyPrefix = _options.TokenPrefix` (the bare prefix) and let the key id remain the `KeyId`
column, or document precisely what `KeyPrefix` is meant to contain on the `ApiKeyRecord` /
`ApiKeyListItem` records and align all call sites to it.

**Resolution**

Resolved in `544a6dd`, 2026-06-01 — `CreateKeyAsync` now persists `KeyPrefix = _options.TokenPrefix` (the bare prefix) instead of `prefix_keyId`, aligning the write path with the read/test paths (test: `ApiKeyAdminCommandsTests.CreateKey_PersistsBareTokenPrefix_NotPrefixUnderscoreKeyId`).

### Auth-006 — LDAP connection ignores `allowInsecure` and offers no TLS certificate-validation hook

| | |
|--|--|
| Severity | Low |
| Category | Security & secret handling |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs:16` |

**Description**

`NovellLdapConnection.Connect` receives `bool allowInsecure` but never uses it; transport-security
correctness rests entirely on `LdapOptionsValidator` having run beforehand (and that registration is
itself lazy — see Auth-001). The method also sets `SecureSocketLayer`/`StartTls` but never configures
a server-certificate validation delegate or any pinning/CA hook. The underlying
`Novell.Directory.Ldap.NETStandard` 3.6.0 validates against the OS trust store by default (it does
*not* blind-accept), so this is not an unauthenticated-TLS hole today — hence Low — but the library
gives a consumer no seam to tighten (pin a CA, validate the SAN against `Server`, or deliberately
accept a self-signed dev cert), and the unused parameter reads as a guard that does nothing.

**Recommendation**

Either remove the unused `allowInsecure` parameter from the `ILdapConnection.Connect` seam or use it
to gate an explicit "accept invalid cert" path for dev only. Add an optional
`RemoteCertificateValidationCallback` (or CA-pinning option) so production consumers can harden LDAPS
beyond the OS default, and document the default validation behavior.

**Resolution**

Resolved in `544a6dd`, 2026-06-01 — added an optional `LdapOptions.ServerCertificateValidationCallback` (BCL `RemoteCertificateValidationCallback`, no new dependency) threaded through the `ILdapConnection.Connect` seam; `NovellLdapConnection` now builds `LdapConnectionOptions.ConfigureRemoteCertificateValidationCallback(...)` (a supplied callback wins; otherwise the previously-dead `allowInsecure` gates a dev-only accept-any path; otherwise OS-trust-store default), and the default validation behaviour is documented in XML/README (tests: `LdapAuthServiceTests.Connect_ReceivesConfiguredCertValidationCallback`, `..._Connect_ReceivesAllowInsecureFlag_FromOptions`, `..._Connect_NoCertCallbackConfigured_PassesNull`).

<!-- 2026-11-09 re-review (scadaproj#15) findings below. -->

### Auth-007 — User-bind mode never checks that the entry whose groups it returns is the account that bound

| | |
|--|--|
| Severity | Medium |
| Priority | P2 |
| Category | Security & secret handling |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs:329` |

**Description**

`CompleteAsUser` authenticates by binding as `UserBindTemplate.Replace("{0}", username)` (line 329–332). It then
runs a separate search, `(<UserNameAttribute>=<username>)` (line 345 → `SearchUser`, line 361), and returns the
groups and username of whatever single entry that search finds (line 349 → `SucceedFromEntry`). Two different
directory lookups resolve the bind name and the search filter. Nothing checks that the entry found is the account
the directory just authenticated. There is no comparison of DNs, no RFC 4532 "Who am I?", and no search keyed on
the bind identity. The XML doc and README describe the UPN mismatch as guidance only ("a mismatch can … bind a
different account than the one whose groups are read"). The validator's failure message
(`LdapOptionsValidator.cs:77`) offers `"{0}@example.com"` as its first example template. A DN-form template is also
spliced in unescaped (documented), which is another way for the bind identity and the search key to diverge.

**Failure scenario**

`BindMode=User` with `UserBindTemplate="{0}@corp.com"` and `UserNameAttribute=sAMAccountName` on AD, a combination
the validator message suggests. Mallory's account has explicit UPN `jsmith@corp.com` and `sAMAccountName=jsmith2`,
which is common after a rename or when two people share a name. A separate admin account has `sAMAccountName=jsmith`.
Mallory signs in as `jsmith` with her own password:
1. AD resolves the UPN bind to Mallory's account and accepts it.
2. The search `(sAMAccountName=jsmith)` returns the admin's entry.
3. `LdapAuthResult.Success("jsmith", "Admin Smith", ["UG_Admins"])` is returned.

That is privilege escalation with valid credentials of a different user. I reproduced this through the
`FakeLdapConnection` seam: the bind went to `jsmith@corp.test` and the result carried the other entry's groups.

**Recommendation**

After the bind, identify the entry by the bound identity, not the typed name:
- **UPN templates:** search on `userPrincipalName=<bindName>`.
- **Any template:** use the RFC 4532 Who-am-I extended operation (AD returns `u:DOMAIN\sam`) and check it against
  the found entry's `sAMAccountName`.
- **DN templates:** require the found entry's DN to equal the bind DN, and DN-escape `{0}` (RFC 4514).

Fail with `GroupLookupFailed` on a mismatch. At minimum, reorder the validator example to lead with
`EXAMPLE\{0}`. Add a test where the bind name and the searched entry differ.

**Resolution**

Resolved 2026-11-09 (scadaproj#40, Auth `0.4.4`, unpublished). Re-verified first: through the fake seam, the pre-fix
service returned the searched entry's groups for a UPN bind that resolved to another account, and live against a
throwaway GLAuth (`tests/ZB.MOM.WW.Auth.Ldap.Tests/Integration/glauth-userbind/`), `mallory` bound as herself and signed
in with `boss`'s `Admins` group because `(mail=mallory)` found `boss`. `CompleteAsUser` now establishes the bound
identity and refuses any entry that is not it, with `GroupLookupFailed`:
- RFC 4532 Who-am-I (new `ILdapConnection.WhoAmI`, backed by Novell's `LdapWhoAmIOperation`). A `dn:` answer must
  equal the entry's DN (`LdapDn.Equals`, `distinguishedNameMatch`). AD's `u:DOMAIN\name` answer must equal the
  entry's `sAMAccountName`, which is requested automatically.
- With no Who-am-I answer (GLAuth answers protocol error 2), only a DN template is provable, and its bind DN must
  equal the entry's DN. A UPN or down-level template fails closed.
- `{0}` is RFC 4514-escaped in DN templates. The validator message now leads with `EXAMPLE\{0}`.

Tests: 13 new cases in `LdapAuthServiceUserBindTests`, 11 of which fail against the pre-fix service, plus
`GLAuthUserBindIdentityTests` (live, `ZB_LDAP_USERBIND_IT=1`): the mismatch case fails pre-fix and passes after, and
the positive control passes both ways. Consumer note (README §0.4.4): a non-AD directory in `User` mode needs a DN
template that yields the entry's DN exactly as the search reports it.

### Auth-008 — SQL Server store silently truncates over-length values and matches key ids case-insensitively, diverging from SQLite

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyAdminStore.cs:45` (also `:46`, `:48`; `SqlServerApiKeyStore.cs:56,77`; `SqlServerApiKeyAuditStore.cs:30-32`; `SqliteApiKeyStoreImporter.cs:258-261,292-294`; columns in `SqlServerAuthSchema.cs:79-96`) |

**Description**

Every string parameter is added with an explicit size: `NVarChar, 64` for `key_id`, `event_type` and
`remote_address`, `16` for `key_prefix`, and `256` for `display_name`. `SqlParameter` truncates an input longer
than `Size` without raising an error, so over-length values are cut down rather than rejected. The columns also use
the database's default collation, which is case-insensitive on a stock SQL Server. The SQLite store this one claims
to port "one-for-one" has `TEXT` columns, no length limits and BINARY (case-sensitive) comparison. `CreateKeyAsync`
validates only that `keyId` is non-blank and has no `_`.

I verified this live against a SQL Server 2022 container:
- A 70-char `keyId` with a 20-char prefix and a 300-char display name was created without error, and the row was
  stored as 64/16/256 characters.
- `FindByKeyIdAsync` with the full 70-char id, and with a different 70-char id sharing the first 64 characters,
  both found the row.
- `FindByKeyIdAsync("LINE1")` found the row stored as `line1`.
- Creating `Line1` beside `line1` failed with a PK violation.
- An 80-char audit `event_type`/`remote_address` was stored as 64 characters.

**Failure scenario**

1. **Over-length key id.** An admin mints a key with a long, descriptive key id (more than 64 characters) on a host
   using `AddZbApiKeyAuthSqlServer`. Creation succeeds and the returned token embeds the full id. Verification
   succeeds only because the lookup parameter is truncated the same way. The authenticated `ApiKeyIdentity.KeyId`
   is the 64-character stored value, not the id the admin assigned. Any host ACL or audit correlation keyed on the
   assigned id never matches.
2. **Case-variant ids in the importer.** Two SQLite keys `Line1` and `line1` (legal on SQLite) migrate through
   `SqliteApiKeyStoreImporter`. The second is counted as "skipped" because the `IF NOT EXISTS` check matches
   case-insensitively. After cut-over its token resolves to the other key's row, fails `SecretMismatch`, and that
   client is locked out with no error at import time.

**Recommendation**

- Pass `-1`/no explicit size, or validate lengths up front and throw `ArgumentException`, in both `CreateKeyAsync`
  and the importer.
- Declare `key_id` with a binary or case-sensitive collation (e.g. `COLLATE Latin1_General_100_BIN2`) to match
  SQLite. Alternatively, document and enforce a case-folded, `[A-Za-z0-9-]{1,64}` key-id alphabet in
  `ApiKeyAdminCommands`.
- Make the importer report key-id collisions instead of counting them as plain skips.

**Resolution**

Resolved 2026-11-09 (scadaproj#46, Auth `0.4.4`, unpublished). Re-verified live against a throwaway SQL Server 2022
container (`RTM-CU22`): `FindByKeyIdAsync("LINE1")` found the row stored as `line1`, and the importer counted
`Line1` beside `line1` as a plain skip. The offline tests reproduced the truncation: an over-length create was sent
to the server. Fixed without a schema change, so the live shared store needs no migration. A new internal helper,
`SqlServerColumnLimits`, carries the rules:
- **Lengths.** `SqlServerApiKeyAdminStore.CreateAsync` throws `ArgumentException` before connecting when `KeyId` is
  over 64, `KeyPrefix` over 16 or `DisplayName` over 256 characters. Every key-id lookup and mutation (`Find*`,
  `MarkUsed`, `Revoke`, `Rotate`, `SetScopes`, `SetEnabled`, `Delete`) returns not-found or `false` for an id over
  64 characters without querying. Before, the truncated parameter could match a different key that shares the
  first 64 characters.
- **Case.** Every `WHERE key_id = @key_id` also requires `CAST(key_id AS VARBINARY(128)) = CAST(@key_id AS VARBINARY(128))`.
  The collation-aware `=` still drives the primary-key seek, and the binary compare makes the match exact, like
  SQLite's `BINARY`. The column collation is deliberately not changed: that would be a primary-key migration on a
  live shared store. Consequence: on a case-insensitive database, `Line1` and `line1` still cannot both exist.
  `CreateAsync` already fails loudly with a PK violation in that case, and the importer now reports it.
- **Importer.** The new `init` properties `ApiKeyImportResult.KeyIdCollisions` (case-only clashes with a key
  already in SQL Server) and `KeysRejected` (over-length rows) list what was not imported and why. Before, both kinds
  were folded into `KeysSkipped`, or imported truncated. `KeysSkipped` now counts only exact duplicates. The
  insert-if-absent statement returns a three-way outcome from inside one statement, so it stays race-safe.
- **Audit text.** `key_id`, `event_type` and `remote_address` are still truncated to 64 characters. This is now
  deliberate and documented on `SqlServerApiKeyAuditStore.AppendAsync`: an audit append runs after the action it
  records, so refusing it would lose the record of a change that has already happened.

Tests:
- `SqlServerColumnLimitTests` (offline, refused loopback port). Over-length create is rejected for each of the three
  fields, and all 8 key-id operations answer "not found" for an over-length id. These fail pre-fix with
  `SqlException`. A positive control at exactly the limits still reaches the server.
- `Live/KeyIdFidelityLiveTests` (`AUTH_SQLSERVER_CONNSTR`). Case- and trailing-space-exact lookups, an importer
  case-collision report, and an importer over-length rejection. All 3 fail pre-fix.
- The full SqlServer suite passes live: 101/101.

### Auth-009 — Canonical username is the first value of a possibly multi-valued attribute, not the value the user matched

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs:437` |

**Description**

0.4.1's `ExtractUserName` returns `values[0]` of `UserNameAttribute` whenever that value is non-blank (lines
437–441). The default `UserNameAttribute` is `cn` (`LdapContracts.cs:30`), which is multi-valued in standard LDAP
schemas: OpenLDAP and 389-DS entries routinely carry both `cn: John Smith` and `cn: jsmith`. The search matched on
one of those values, but the code returns whichever value the server lists first, which need not be the one the
user typed. This is the same person and the same entry, so there is no privilege effect. However, the identity
string handed to the host changes with the 0.4.1 upgrade and depends on server attribute ordering.

**Failure scenario**

A host on the default `UserNameAttribute=cn` runs against an OpenLDAP directory where users have two `cn` values.
Before 0.4.1 `jsmith` signed in as `jsmith`. After the upgrade the result's `Username` is `John Smith`. Per-user
records, audit trails and any username-keyed role overrides in the host split into two identities, and a username
containing a space can break downstream consumers that treat it as an account name. I reproduced this through the
fake connection: typed `jsmith` returned `Username == "John Smith"`.

**Recommendation**

Prefer the attribute value that equals the typed (normalised) name under `OrdinalIgnoreCase`, which still
canonicalises case. Fall back to `values[0]` only when no value matches. Add a multi-valued-attribute test.

**Resolution**

Resolved 2026-11-09 (scadaproj#46, Auth `0.4.4`, unpublished). Re-verified through the fake connection: with
`cn: ["John Smith", "jsmith"]`, typed `jsmith` returned `Username == "John Smith"`. `ExtractUserName` now returns the
attribute value that equals the typed (normalised) name under `OrdinalIgnoreCase`. That keeps 0.4.1's case
canonicalisation, so typed `jsmith` against `JSmith` still returns `JSmith`. Only when no value matches does it fall
back to the first non-blank value, and then to the typed name, as 0.4.1 did. Both bind modes go through
`SucceedFromEntry`, so both are covered.

Tests (`LdapAuthServiceTests`):
- `Username_FromAMultiValuedAttribute_IsTheValueTheUserMatched` fails pre-fix.
- `..._StillTakesTheDirectorysCase` fails pre-fix.
- `Username_FallsBackToTheFirstValue_WhenNoValueMatchesTheTypedName` is a guard that passes both ways.

### Auth-010 — `ApiKeyVerifier` lets store exceptions escape despite its "only exception path is cancellation" contract

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Error handling & resilience |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/ApiKeyVerifier.cs:47` (contract stated at `:13`) |

**Description**

The type remark (line 13) promises "The only exception path is cancellation". Auth-002 fixed `MarkUsedAsync` to
honour that promise, but the lookup `await store.FindByKeyIdAsync(...)` at line 47 is unguarded. Its failures
propagate unchanged. With the new `SqlServerApiKeyStore` the store is a network database shared by a cluster, so
lookup failures are an expected operating condition, not a rare local-disk fault. A misconfigured
`SqlServerApiKeyOptions` also escapes: it is validated lazily in the `AuthSqlServerConnectionFactory` constructor.
With `RunMigrationsOnStartup=false` that throws `InvalidOperationException` out of the first verifier resolution or
call.

**Failure scenario**

The shared SQL Server is briefly unreachable during a failover, or the central pair loses its route to it. Every
inbound API request's `VerifyAsync` throws `Microsoft.Data.SqlClient.SqlException`; I verified this against a closed
port. Consumers wrote their middleware and interceptors against the documented contract and map
`Succeeded == false` to 401, so they do not catch this. The request surfaces as an unhandled 500, or a gRPC
`Unknown`, instead of a clean auth failure or 503, and the failure goes unaudited.

**Recommendation**

Wrap the lookup and map non-cancellation exceptions to a structured failure, for example a new
`ApiKeyFailure.StoreUnavailable`. Otherwise, correct the remark to say store exceptions propagate. Add a test
where the store throws. Optionally add `SqlServerApiKeyOptions` startup validation
(`AddOptions().Validate(...).ValidateOnStart()`), mirroring Auth-001.

**Resolution**

Resolved 2026-11-09 (scadaproj#46, Auth `0.4.4`, unpublished). Re-verified offline: an `ApiKeyVerifier` over a real
`SqlServerApiKeyStore` that dials a refused loopback port threw `Microsoft.Data.SqlClient.SqlException` out of
`VerifyAsync`. The fix takes the review's first option. `ApiKeyFailure` gains **`StoreUnavailable`**, appended last so
the existing numeric values are unchanged. `VerifyAsync` now wraps the `FindByKeyIdAsync` lookup: an
`OperationCanceledException` is rethrown, and any other exception returns `Fail(StoreUnavailable)`. The class remark
("the only exception path is cancellation") is now true, and it names the new failure.

Consumers already map `Succeeded == false` to their auth-failure path and audit it, so a store outage becomes an
audited auth failure instead of an unaudited 500 or gRPC `Unknown`. A host that wants 503 can switch on the new value.
MxAccessGateway's `CachingApiKeyVerifier` caches only successes, so a transient `StoreUnavailable` is never cached.

The optional `SqlServerApiKeyOptions` `ValidateOnStart` was **not** added. Option validation still happens in the
`AuthSqlServerConnectionFactory` constructor, so a misconfigured section still throws when the store is first
resolved, which is outside `VerifyAsync`. Validating at start could stop an existing host that registers the package
without configuring the section from booting. Left as a follow-up candidate.

Tests:
- `ApiKeyVerifierTests.VerifyAsync_StoreLookupThrows_ReturnsStoreUnavailable_InsteadOfThrowing` is a theory over a
  `DataException`, an `InvalidOperationException` and an `IOException`. All three cases fail pre-fix.
- `VerifyAsync_StoreLookupCancelled_Propagates` is a guard.
- `SqlServerUnreachableVerifierTests` (offline, refused loopback port) fails pre-fix with the `SqlException`.