test(ldap): align DashboardLdapLiveTests fixtures with the shared directory (NEXT-06)
The suite's fixtures had drifted from the shared GLAuth config, so a green run proved nothing about the service-account bind: the only success-path test used admin/admin123, but the directory's admin carries the standard dev password, and the "not an admin" test used a readonly user that does not exist there at all -- it passed via the user-not-found branch rather than the group-missing branch it names. Realign to real users from scadaproj/infra/glauth/config.toml: admin/password (othergroups include GwAdmin, gid 5610) for the success path, and gw-viewer/password (GwReader only, gid 5611) for the bind-succeeds-but-no-role path. Both are published dev credentials documented in glauth.md, not secrets. The gw-viewer test drops its old no-leak assertion on the credential literal: the real password is the word "password", which legitimately occurs in the generic denial text, so the check would fail for the wrong reason. The no-leak property is still covered with a distinctive literal by the wrong-password test. In its place the test now asserts the property this fixture is uniquely able to prove -- an authorization failure must be reported with the same message as an authentication failure, so it cannot be used to enumerate valid accounts. appsettings ships Server=localhost, so document the MxGateway__Ldap__Server override the suite needs to reach the shared GLAuth alongside the existing MXGATEWAY_RUN_LIVE_LDAP_TESTS and ServiceAccountPassword variables. Verified live: Failed: 0, Passed: 5 against 10.100.0.35:3893.
This commit is contained in:
Joseph Doherty
@@ -1,6 +1,6 @@
|
||||
# Candidate Findings for the Next Review Cycle (surfaced during 2026-07-12 remediation)
|
||||
|
||||
These were discovered while remediating the 2026-07-12 backlog but were **out of scope** for it — each is either pre-existing, by-design residual, or a new observation. They are recorded here (not fixed) so the next review cycle can triage them. None blocks the 2026-07-12 cycle, which is complete.
|
||||
These were discovered while remediating the 2026-07-12 backlog but were **out of scope** for it — each is either pre-existing, by-design residual, or a new observation. They are recorded here so the next review cycle can triage them. None blocks the 2026-07-12 cycle, which is complete. Rows struck through have since been fixed ahead of that cycle; the original finding text is kept so the triage record stays readable.
|
||||
|
||||
| ID (proposed) | Area | Severity (est.) | Summary |
|
||||
|---|---|---|---|
|
||||
@@ -9,7 +9,7 @@ These were discovered while remediating the 2026-07-12 backlog but were **out of
|
||||
| NEXT-03 | Gateway alarms | Low | `GatewayAlarmMonitor.ApplyReconcile` feed-repair broadcasts (the new acked-delta from GWC-26 **and** the pre-existing Raise/Clear repair) are **at-least-once, not exactly-once**: a periodic reconcile can synthesize a transition whose matching live transition is still buffered in the alarm lease, so both broadcast as indistinguishable duplicates on the alarm feed (StreamAlarms + dashboard hub). Pre-existing (the Raise/Clear repair always had it); GWC-26 documented the at-least-once contract rather than closing the race. Closing it needs reconcile/live serialization or a monotonic dedup marker. |
|
||||
| NEXT-04 | Worker frame writer | Low | WRK-22/WRK-25 cancellation path: a frame `Claimed` by a concurrent lock-holder just before its caller's cancellation races in is never awaited by that caller; if the write then faults, `TrySetException` lands on a `Task` nobody observes (unobserved-task-exception). By-design residual, non-crash (no `UnobservedTaskException` handler registered), pre-existing to single-frame WRK-22 and amplified per-batch by WRK-25. Hygiene fix: attach a fault-observing continuation to abandoned/tombstoned frame completions. |
|
||||
| NEXT-05 | Worker frame writer | Info | A batch whose remaining frames are tombstoned by cancellation leaves dead `PendingFrame` entries in `_eventFrames`/`_controlFrames` until a future `DequeueNext` pops and skips them. Same pre-existing behavior as single-frame WRK-22, amplified per-batch; in practice heartbeats purge them promptly, so not a real leak. |
|
||||
| NEXT-06 | Testing / live LDAP | Medium | `DashboardLdapLiveTests` fixtures have drifted from the shared GLAuth directory, leaving the suite with **no positive-proof coverage of the service-account bind**. Its only success-path test, `AuthenticateAsync_AdminInGwAdminGroup_Succeeds`, binds `admin`/`admin123`, but the directory's `admin` user carries the standard dev password (`scadaproj/infra/glauth/config.toml`), so that assertion cannot pass. `AuthenticateAsync_ReadOnlyUserMissingGwAdminGroup_Fails` binds fixture user `readonly`, which **does not exist** in the GLAuth config at all — it passes for the wrong reason (user-not-found rather than the group-missing branch it names; the `readonly` name is in fact barred by the README's user/group case-collision rule). The three remaining tests are negative assertions that pass whether or not the service account can bind. Net effect: a green `DashboardLdapLiveTests` run proves nothing about the bind credential — surfaced during SEC-36, where the suite was considered as a substitute for the deferred dashboard-login check and rejected. Fix: realign the fixtures to real directory users (e.g. `multi-role`/`gw-viewer`) or add the missing users to the GLAuth config, and add one test that fails when the service-account credential is wrong. |
|
||||
| ~~NEXT-06~~ | Testing / live LDAP | Medium | **Resolved 2026-08-07** — fixtures realigned to the shared directory (`admin`/`password` for the GwAdmin success path, `gw-viewer`/`password` for the bind-succeeds-but-no-role path); verified `Failed: 0, Passed: 5` live against the shared GLAuth at `10.100.0.35:3893`, so the success-path assertion (GwAdmin group claim + Admin role claim) now fails if the service-account credential is wrong. Original finding: `DashboardLdapLiveTests` fixtures have drifted from the shared GLAuth directory, leaving the suite with **no positive-proof coverage of the service-account bind**. Its only success-path test, `AuthenticateAsync_AdminInGwAdminGroup_Succeeds`, binds `admin`/`admin123`, but the directory's `admin` user carries the standard dev password (`scadaproj/infra/glauth/config.toml`), so that assertion cannot pass. `AuthenticateAsync_ReadOnlyUserMissingGwAdminGroup_Fails` binds fixture user `readonly`, which **does not exist** in the GLAuth config at all — it passes for the wrong reason (user-not-found rather than the group-missing branch it names; the `readonly` name is in fact barred by the README's user/group case-collision rule). The three remaining tests are negative assertions that pass whether or not the service account can bind. Net effect: a green `DashboardLdapLiveTests` run proves nothing about the bind credential — surfaced during SEC-36, where the suite was considered as a substitute for the deferred dashboard-login check and rejected. Fix: realign the fixtures to real directory users (e.g. `multi-role`/`gw-viewer`) or add the missing users to the GLAuth config, and add one test that fails when the service-account credential is wrong. |
|
||||
| NEXT-07 | Deployment / windev | High | The `10.100.0.48` (windev) gateway deployment is **stale and crash-looping**, and has been since at least 2026-08-06 (~10k Hosting-failed events/day). The deployed Server binary dates to 2026-06-25 and predates the auth-DB migration of 2026-07-15: it opens a schema-version-3 `gateway-auth.db` that it supports only at version 2 and aborts at startup, so the `MxAccessGw` service never reaches a listening state. Not a code defect in the current tree — a deploy-drift/operations gap — but it means the repo's only deployed host has been dark for over a day and any host-level verification (including SEC-36's dashboard-login check) is blocked until it is repaired. Fix: deploy a current Server build to windev, or restore/downgrade the auth DB to schema 2 if the old binary must stand. Worth asking separately why a service in a permanent restart loop raised no alert. Discovered during SEC-36. |
|
||||
|
||||
## Operator actions still pending (from this cycle's runbooks)
|
||||
|
||||
+24
-10
@@ -215,13 +215,21 @@ service described in `glauth.md`.
|
||||
|
||||
The suite builds the authenticator with `GatewayOptions.Dashboard.GroupToRole`
|
||||
set to `{ GwAdmin: Admin }`. `GwAdmin` is the gateway-specific
|
||||
dashboard-admin role and is **not** part of the five baseline GLAuth role
|
||||
dashboard-admin role and is **not** part of the baseline GLAuth role
|
||||
groups — it must be provisioned before the LDAP live tests pass.
|
||||
`AuthenticateAsync_AdminInGwAdminGroup_Succeeds` fails (rather than skips)
|
||||
when GLAuth has only the baseline groups, so this is a hard prerequisite
|
||||
beyond "LDAP is up." See the "Adding a gw-specific group" section of
|
||||
`glauth.md` for the provisioning step that adds `GwAdmin` and grants it to
|
||||
`admin`.
|
||||
beyond "LDAP is up." The shared directory
|
||||
(`scadaproj/infra/glauth/config.toml`) already provisions `GwAdmin` (gid 5610)
|
||||
and `GwReader` (gid 5611); see the "Adding a gw-specific group" section of
|
||||
`glauth.md` for the per-box equivalent.
|
||||
|
||||
The fixtures name real users from that shared config, so a run only proves the
|
||||
service-account bind when it targets the shared directory. `appsettings.json`
|
||||
ships `Server=localhost` for the local-forward case, so point the suite at the
|
||||
shared GLAuth with `MxGateway__Ldap__Server=10.100.0.35`; the suite's
|
||||
`AddEnvironmentVariables()` layer applies the override to the same
|
||||
`MxGateway:Ldap` section production binds.
|
||||
|
||||
`DashboardAuthenticator` delegates the LDAP bind and group search to the shared
|
||||
`ZB.MOM.WW.Auth.Ldap` provider (`LdapAuthService`) and only maps the resulting
|
||||
@@ -229,12 +237,17 @@ groups to dashboard roles via `DashboardGroupRoleMapper`; the bind/search
|
||||
mechanics that decide each outcome live in that shared provider, not in
|
||||
`DashboardAuthenticator`.
|
||||
|
||||
The suite covers both the success path and the failure outcomes: `admin` whose
|
||||
LDAP groups resolve to the `Admin` role succeeds and emits the role claim;
|
||||
`readonly` is denied because no group in their `memberOf` appears in
|
||||
`GroupToRole`; `admin` with a wrong password fails authentication without leaking
|
||||
the password into `FailureMessage`; an unknown username fails authentication; and
|
||||
an unreachable LDAP server is absorbed into a failed result rather than throwing.
|
||||
The suite covers both the success path and the failure outcomes: `admin`, whose
|
||||
`othergroups` include `GwAdmin`, succeeds and emits the role claim — this is the
|
||||
one test that proves the service-account bind, because every other outcome below
|
||||
fails identically whether or not the bind credential is right; `gw-viewer` is
|
||||
denied because its only group (`GwReader`) is absent from `GroupToRole`, and its
|
||||
denial message must match the unknown-user denial so an authorization failure
|
||||
cannot be used to enumerate valid accounts; `admin` with a wrong password fails
|
||||
authentication without leaking the password into `FailureMessage`; an unknown
|
||||
username fails authentication; and an unreachable LDAP server is absorbed into a
|
||||
failed result rather than throwing. Both live users bind with the shared dev
|
||||
password documented in `glauth.md`.
|
||||
|
||||
`appsettings.json` now ships the LDAP bind password as the unexpanded
|
||||
`${secret:ldap/mxgateway/bind}` token (resolved at gateway startup by the
|
||||
@@ -249,6 +262,7 @@ Run the LDAP live tests explicitly:
|
||||
|
||||
```bash
|
||||
$env:MXGATEWAY_RUN_LIVE_LDAP_TESTS = "1"
|
||||
$env:MxGateway__Ldap__Server = "10.100.0.35"
|
||||
$env:MxGateway__Ldap__ServiceAccountPassword = "<service-account-password>"
|
||||
dotnet test src/ZB.MOM.WW.MxGateway.IntegrationTests/ZB.MOM.WW.MxGateway.IntegrationTests.csproj --filter FullyQualifiedName~DashboardLdapLiveTests
|
||||
```
|
||||
|
||||
@@ -14,7 +14,19 @@ namespace ZB.MOM.WW.MxGateway.IntegrationTests;
|
||||
[Trait("Category", "LiveLdap")]
|
||||
public sealed class DashboardLdapLiveTests
|
||||
{
|
||||
/// <summary>Verifies that an admin user in the GwAdmin group authenticates successfully.</summary>
|
||||
/// <summary>
|
||||
/// The shared dev/test directory issues every human tester the same well-known password, so
|
||||
/// the fixtures name it once rather than repeating a literal that drifts per test. This is a
|
||||
/// published dev credential (see <c>glauth.md</c> and <c>scadaproj/infra/glauth/config.toml</c>),
|
||||
/// not a secret — unlike the service-account bind password, which is never in source and must
|
||||
/// arrive via <c>MxGateway__Ldap__ServiceAccountPassword</c>.
|
||||
/// </summary>
|
||||
private const string SharedDirectoryPassword = "password";
|
||||
|
||||
/// <summary>
|
||||
/// Verifies that <c>admin</c> — a shared-directory user whose <c>othergroups</c> include
|
||||
/// GwAdmin (gid 5610) — authenticates successfully and is granted the Admin dashboard role.
|
||||
/// </summary>
|
||||
/// <returns>A task that represents the asynchronous operation.</returns>
|
||||
[LiveLdapFact]
|
||||
public async Task AuthenticateAsync_AdminInGwAdminGroup_Succeeds()
|
||||
@@ -23,7 +35,7 @@ public sealed class DashboardLdapLiveTests
|
||||
|
||||
DashboardAuthenticationResult result = await authenticator.AuthenticateAsync(
|
||||
"admin",
|
||||
"admin123",
|
||||
SharedDirectoryPassword,
|
||||
CancellationToken.None);
|
||||
|
||||
Assert.True(result.Succeeded);
|
||||
@@ -38,21 +50,43 @@ public sealed class DashboardLdapLiveTests
|
||||
&& claim.Value == DashboardRoles.Admin);
|
||||
}
|
||||
|
||||
/// <summary>Verifies that a readonly user without GwAdmin group fails to authenticate.</summary>
|
||||
/// <summary>
|
||||
/// Verifies that <c>gw-viewer</c> — a shared-directory user whose only group is GwReader
|
||||
/// (gid 5611), which this suite's GroupToRole map deliberately leaves unmapped — is denied
|
||||
/// even though its bind succeeds, and that the denial is indistinguishable from the
|
||||
/// unknown-user denial.
|
||||
/// </summary>
|
||||
/// <returns>A task that represents the asynchronous operation.</returns>
|
||||
[LiveLdapFact]
|
||||
public async Task AuthenticateAsync_ReadOnlyUserMissingGwAdminGroup_Fails()
|
||||
public async Task AuthenticateAsync_ViewerMissingGwAdminGroup_FailsIndistinguishably()
|
||||
{
|
||||
DashboardAuthenticator authenticator = CreateAuthenticator();
|
||||
|
||||
DashboardAuthenticationResult result = await authenticator.AuthenticateAsync(
|
||||
"readonly",
|
||||
"readonly123",
|
||||
"gw-viewer",
|
||||
SharedDirectoryPassword,
|
||||
CancellationToken.None);
|
||||
|
||||
Assert.False(result.Succeeded);
|
||||
Assert.Null(result.Principal);
|
||||
Assert.DoesNotContain("readonly123", result.FailureMessage, StringComparison.Ordinal);
|
||||
|
||||
// This test used to assert the failure message did not echo the credential literal.
|
||||
// That check cannot survive the move to the shared directory: the real password is the
|
||||
// word "password", which legitimately occurs in the generic denial text ("The username
|
||||
// or password is invalid, ..."), so the assertion would fail for the wrong reason. The
|
||||
// no-leak property is still covered — with a distinctive literal — by
|
||||
// AuthenticateAsync_AdminWithWrongPassword_FailsWithoutLeakingPassword below. What is
|
||||
// asserted here instead is the property this fixture is actually uniquely able to prove:
|
||||
// an authorization failure (valid credentials, no mapped role) must be reported with the
|
||||
// same message as an authentication failure, so the response cannot be used to enumerate
|
||||
// valid accounts.
|
||||
DashboardAuthenticationResult unknownUserResult = await authenticator.AuthenticateAsync(
|
||||
"no-such-user-9f3c1",
|
||||
"irrelevant-password",
|
||||
CancellationToken.None);
|
||||
|
||||
Assert.False(string.IsNullOrWhiteSpace(result.FailureMessage));
|
||||
Assert.Equal(unknownUserResult.FailureMessage, result.FailureMessage);
|
||||
}
|
||||
|
||||
/// <summary>Verifies that authentication with wrong password fails without leaking the password.</summary>
|
||||
@@ -98,9 +132,11 @@ public sealed class DashboardLdapLiveTests
|
||||
[LiveLdapFact]
|
||||
public async Task AuthenticateAsync_ServerUnreachable_FailsWithoutThrowing()
|
||||
{
|
||||
// Exercises the connect-failure path: a closed loopback port produces a
|
||||
// connection error that the shared LdapAuthService must absorb into a Fail
|
||||
// result rather than propagating an exception to the dashboard.
|
||||
// Exercises the connect-failure path: overriding only the port keeps whatever host
|
||||
// the run targets (localhost by default, the shared GLAuth under the
|
||||
// MxGateway__Ldap__Server override) while pointing at a port nothing listens on, so
|
||||
// the connection error the shared LdapAuthService must absorb into a Fail result —
|
||||
// rather than propagate as an exception to the dashboard — is reproduced either way.
|
||||
DashboardAuthenticator authenticator = CreateAuthenticator(LibraryOptions() with
|
||||
{
|
||||
// 1 is a reserved port number that no LDAP server listens on.
|
||||
@@ -109,7 +145,7 @@ public sealed class DashboardLdapLiveTests
|
||||
|
||||
DashboardAuthenticationResult result = await authenticator.AuthenticateAsync(
|
||||
"admin",
|
||||
"admin123",
|
||||
SharedDirectoryPassword,
|
||||
CancellationToken.None);
|
||||
|
||||
Assert.False(result.Succeeded);
|
||||
@@ -147,8 +183,10 @@ public sealed class DashboardLdapLiveTests
|
||||
/// <see cref="LibraryLdapOptions.ConnectionTimeoutMs"/>, which governs the
|
||||
/// unreachable-server test's timing) at whatever value the operator configured, and
|
||||
/// cannot silently drop a field added to the shared type. The gateway's
|
||||
/// <c>appsettings.json</c> seeds the dev directory connection (localhost:3893,
|
||||
/// plaintext, AllowInsecure).
|
||||
/// <c>appsettings.json</c> seeds the dev directory connection (port 3893, plaintext,
|
||||
/// AllowInsecure) but ships <c>Server=localhost</c>, so a run against the shared GLAuth
|
||||
/// needs the <c>MxGateway__Ldap__Server=10.100.0.35</c> environment override that the
|
||||
/// <c>AddEnvironmentVariables()</c> layer below applies.
|
||||
/// </summary>
|
||||
private static LibraryLdapOptions LibraryOptions()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user