diff --git a/archreview/2026-07-12/remediation/90-candidate-findings-next-cycle.md b/archreview/2026-07-12/remediation/90-candidate-findings-next-cycle.md index 4436481..c64add2 100644 --- a/archreview/2026-07-12/remediation/90-candidate-findings-next-cycle.md +++ b/archreview/2026-07-12/remediation/90-candidate-findings-next-cycle.md @@ -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) diff --git a/docs/GatewayTesting.md b/docs/GatewayTesting.md index 9ecd8a7..5388562 100644 --- a/docs/GatewayTesting.md +++ b/docs/GatewayTesting.md @@ -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 = "" dotnet test src/ZB.MOM.WW.MxGateway.IntegrationTests/ZB.MOM.WW.MxGateway.IntegrationTests.csproj --filter FullyQualifiedName~DashboardLdapLiveTests ``` diff --git a/src/ZB.MOM.WW.MxGateway.IntegrationTests/DashboardLdapLiveTests.cs b/src/ZB.MOM.WW.MxGateway.IntegrationTests/DashboardLdapLiveTests.cs index cd5cd69..5ee6788 100644 --- a/src/ZB.MOM.WW.MxGateway.IntegrationTests/DashboardLdapLiveTests.cs +++ b/src/ZB.MOM.WW.MxGateway.IntegrationTests/DashboardLdapLiveTests.cs @@ -14,7 +14,19 @@ namespace ZB.MOM.WW.MxGateway.IntegrationTests; [Trait("Category", "LiveLdap")] public sealed class DashboardLdapLiveTests { - /// Verifies that an admin user in the GwAdmin group authenticates successfully. + /// + /// 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 glauth.md and scadaproj/infra/glauth/config.toml), + /// not a secret — unlike the service-account bind password, which is never in source and must + /// arrive via MxGateway__Ldap__ServiceAccountPassword. + /// + private const string SharedDirectoryPassword = "password"; + + /// + /// Verifies that admin — a shared-directory user whose othergroups include + /// GwAdmin (gid 5610) — authenticates successfully and is granted the Admin dashboard role. + /// /// A task that represents the asynchronous operation. [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); } - /// Verifies that a readonly user without GwAdmin group fails to authenticate. + /// + /// Verifies that gw-viewer — 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. + /// /// A task that represents the asynchronous operation. [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); } /// Verifies that authentication with wrong password fails without leaking the password. @@ -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 /// , 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 - /// appsettings.json seeds the dev directory connection (localhost:3893, - /// plaintext, AllowInsecure). + /// appsettings.json seeds the dev directory connection (port 3893, plaintext, + /// AllowInsecure) but ships Server=localhost, so a run against the shared GLAuth + /// needs the MxGateway__Ldap__Server=10.100.0.35 environment override that the + /// AddEnvironmentVariables() layer below applies. /// private static LibraryLdapOptions LibraryOptions() {