From c69a1c441ba2493056d96b96461f421ab6ae29bd Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 11 Aug 2026 08:41:54 -0400 Subject: [PATCH 1/3] feat(diagnostics): report MXAccess session health on the active probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a `mxaccess-sessions` health check reporting how many MXAccess sessions are healthy. Each session is one worker process holding one MXAccess COM instance — a live connection into a Galaxy — so this answers "how many Galaxy connections are healthy" in the vocabulary the code actually uses. Zero sessions is Healthy, deliberately, and the rest of the design follows from that. The gateway opens a session when a client asks and holds none otherwise, so an idle gateway is working normally. A count threshold ("unhealthy below N") would sit red forever on a host nothing dials yet, and a permanently red probe is one operators stop reading — which leaves them worse off than no probe. The check therefore grades on whether the sessions that exist are usable: nothing faulted is Healthy, some faulted beside a ready or starting one is Degraded, and every session faulted is Unhealthy. Counts ride along as entry data for the family Overview dashboard. Tagged `active` rather than `ready` for the same reason. Readiness decides whether the process should be sent traffic, and a gateway with no sessions is ready to serve — unlike the auth store, which every call depends on. Failing readiness here would pull a working gateway out of rotation over a condition its own clients create. Reads ISessionRegistry, which already exposes Snapshot(); ISessionManager stays the command surface and grows no enumerator. --- docs/Diagnostics.md | 24 +++ .../Diagnostics/SessionHealthCheck.cs | 114 +++++++++++++++ .../GatewayApplication.cs | 8 +- .../Diagnostics/SessionHealthCheckTests.cs | 138 ++++++++++++++++++ 4 files changed, 283 insertions(+), 1 deletion(-) create mode 100644 src/ZB.MOM.WW.MxGateway.Server/Diagnostics/SessionHealthCheck.cs create mode 100644 src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/SessionHealthCheckTests.cs diff --git a/docs/Diagnostics.md b/docs/Diagnostics.md index 7ea29d4..70c24d0 100644 --- a/docs/Diagnostics.md +++ b/docs/Diagnostics.md @@ -217,6 +217,30 @@ The order matters: putting the logging scope first ensures that authentication f - `DashboardRedactor.Redact` delegates to `RedactClientIdentity` for any value containing the `mxgw_` marker, then falls back to a marker-keyword check for fields like `password` or `token`. This keeps dashboard renders aligned with log redaction. - `ZB.MOM.WW.MxGateway.Tests/Diagnostics/GatewayLogRedactorTests.cs` covers each redaction branch, including the assertion that `WriteSecured` values stay redacted even when `valueLoggingEnabled` is true. +## Health Checks + +The shared `ZB.MOM.WW.Health` package maps three endpoints — `/healthz` (live), `/health/ready`, and +`/health/active` — and each registered check opts into a tier by tag. The gateway registers two: + +| Check | Endpoint tier | Fails when | +|---|---|---| +| `auth-store` | `ready` | The SQLite auth store cannot be opened. Every gRPC call authenticates against it, so its reachability genuinely gates whether the process should receive traffic. | +| `mxaccess-sessions` | `active` | Sessions exist and their workers have faulted. Reports `total` / `ready` / `faulted` / `starting` / `closing` as entry `data`. | + +**Zero sessions is Healthy, and the tier choice follows from that.** The gateway opens an MXAccess +session when a client asks for one and holds none otherwise, so an idle gateway is working normally, +not broken. A count threshold ("unhealthy below N") would sit red forever on a host nothing dials +yet, and a permanently red probe is one operators stop reading — which leaves them worse off than no +probe at all. `mxaccess-sessions` is therefore graded on whether the sessions that exist are usable: + +- nothing faulted → **Healthy** (including no sessions at all) +- some faulted, some still ready or starting → **Degraded** +- every session faulted → **Unhealthy** + +For the same reason it is tagged `active` rather than `ready`. Readiness decides whether the process +should be sent traffic, and a gateway with no sessions is ready to serve; failing readiness there +would pull a working gateway out of rotation over a condition its clients create. + ## Related Documentation - [Identifying A Deployed Build](./runbooks/IdentifyingADeployedBuild.md) — mapping a running binary back to a commit, and why the `InformationalVersion` stamp cannot be trusted on Windows builds from 2026-07-09 to 2026-08-10 diff --git a/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/SessionHealthCheck.cs b/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/SessionHealthCheck.cs new file mode 100644 index 0000000..84d040a --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Diagnostics/SessionHealthCheck.cs @@ -0,0 +1,114 @@ +using Microsoft.Extensions.Diagnostics.HealthChecks; +using ZB.MOM.WW.MxGateway.Contracts.Proto; +using ZB.MOM.WW.MxGateway.Server.Sessions; + +namespace ZB.MOM.WW.MxGateway.Server.Diagnostics; + +/// +/// Reports how many MXAccess sessions are healthy. Each session is one worker process holding one +/// MXAccess COM instance — a live connection into a Galaxy — so this is the "how many Galaxy +/// connections are healthy" probe, expressed in the vocabulary the code actually uses. +/// +/// +/// +/// Zero sessions is healthy, deliberately. The gateway is a server: it opens a session when +/// a client asks and holds none otherwise, so idle-with-no-clients is the normal steady state, not +/// a fault. A count-based rule ("unhealthy below N") would sit red forever on a host nothing dials +/// yet, and a probe that is permanently red is one people learn to ignore — which costs more than +/// having no probe. The status here is therefore false only when a session exists and its worker +/// has actually failed. +/// +/// +/// This is tagged active rather than ready for the same reason. Readiness gates +/// whether the process should receive traffic, and a gateway with no sessions is legitimately ready +/// to serve — unlike the auth store, which every call depends on (see +/// ). Failing readiness on session state would take a working +/// gateway out of rotation for a condition its own clients cause. +/// +/// +public sealed class SessionHealthCheck : IHealthCheck +{ + private readonly ISessionRegistry _sessionRegistry; + + /// Initializes a new instance of the class. + /// Registry holding the live sessions. + public SessionHealthCheck(ISessionRegistry sessionRegistry) => + _sessionRegistry = sessionRegistry ?? throw new ArgumentNullException(nameof(sessionRegistry)); + + /// Buckets the live sessions by state and grades the result. + /// The health check context. + /// Token to cancel the asynchronous operation. + /// + /// Healthy when nothing is faulted (including when no sessions are open), Degraded when some + /// sessions are faulted but others are still usable, and Unhealthy when every session is + /// faulted. + /// + public Task CheckHealthAsync( + HealthCheckContext context, + CancellationToken cancellationToken = default) + { + int ready = 0; + int faulted = 0; + int starting = 0; + int closing = 0; + + foreach (GatewaySession session in _sessionRegistry.Snapshot()) + { + switch (session.State) + { + case SessionState.Ready: + ready++; + break; + case SessionState.Faulted: + faulted++; + break; + case SessionState.Closing: + case SessionState.Closed: + // Counted but excluded from the verdict: a session on its way out is an + // expected lifecycle stage, not a failure, and Snapshot() still returns + // Closed sessions until they are removed from the registry. + closing++; + break; + default: + // Creating / StartingWorker / WaitingForPipe / Handshaking / + // InitializingWorker — mid-startup, not yet usable but not wrong. + // Unspecified lands here too; it is the proto zero value and should not occur. + starting++; + break; + } + } + + int total = ready + faulted + starting + closing; + int usable = ready + starting; + + Dictionary data = new(StringComparer.Ordinal) + { + ["total"] = total, + ["ready"] = ready, + ["faulted"] = faulted, + ["starting"] = starting, + ["closing"] = closing, + }; + + HealthCheckResult result = (faulted, usable) switch + { + (0, _) => HealthCheckResult.Healthy(Describe(total, ready, faulted), data), + (_, 0) => HealthCheckResult.Unhealthy(Describe(total, ready, faulted), data: data), + _ => HealthCheckResult.Degraded(Describe(total, ready, faulted), data: data), + }; + + return Task.FromResult(result); + } + + private static string Describe(int total, int ready, int faulted) + { + if (total == 0) + { + return "No MXAccess sessions are open."; + } + + return faulted == 0 + ? $"{ready} of {total} MXAccess sessions ready." + : $"{ready} of {total} MXAccess sessions ready, {faulted} faulted."; + } +} diff --git a/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs b/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs index 0a38696..49144ee 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs @@ -106,7 +106,13 @@ public static class GatewayApplication .AddTypeActivatedCheck( "auth-store", failureStatus: null, - tags: new[] { ZbHealthTags.Ready }); + tags: new[] { ZbHealthTags.Ready }) + // Active, not Ready: a gateway holding no sessions is legitimately ready to serve. + // See SessionHealthCheck for why zero sessions is healthy. + .AddTypeActivatedCheck( + "mxaccess-sessions", + failureStatus: null, + tags: new[] { ZbHealthTags.Active }); builder.Services.AddSingleton(); builder.AddZbTelemetry(o => { diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/SessionHealthCheckTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/SessionHealthCheckTests.cs new file mode 100644 index 0000000..edff0d0 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Tests/Diagnostics/SessionHealthCheckTests.cs @@ -0,0 +1,138 @@ +using Microsoft.Extensions.Diagnostics.HealthChecks; +using ZB.MOM.WW.MxGateway.Contracts.Proto; +using ZB.MOM.WW.MxGateway.Server.Diagnostics; +using ZB.MOM.WW.MxGateway.Server.Sessions; + +namespace ZB.MOM.WW.MxGateway.Tests.Diagnostics; + +public sealed class SessionHealthCheckTests +{ + /// + /// An idle gateway is healthy. This is the load-bearing case: a gateway holding no sessions is + /// the normal steady state on a host nothing dials yet, and a probe that reports red there is + /// one operators learn to ignore. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task Healthy_WhenNoSessionsAreOpen() + { + var check = new SessionHealthCheck(new SessionRegistry()); + + HealthCheckResult result = await check.CheckHealthAsync(new HealthCheckContext()); + + Assert.Equal(HealthStatus.Healthy, result.Status); + Assert.Equal(0, result.Data["total"]); + Assert.Equal("No MXAccess sessions are open.", result.Description); + } + + /// Every session ready reports healthy, with the counts carried as entry data. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Healthy_WhenAllSessionsReady() + { + var check = new SessionHealthCheck(RegistryWith(SessionState.Ready, SessionState.Ready)); + + HealthCheckResult result = await check.CheckHealthAsync(new HealthCheckContext()); + + Assert.Equal(HealthStatus.Healthy, result.Status); + Assert.Equal(2, result.Data["total"]); + Assert.Equal(2, result.Data["ready"]); + Assert.Equal(0, result.Data["faulted"]); + } + + /// + /// A faulted session alongside a usable one is degraded, not unhealthy — the gateway is still + /// serving the sessions that work. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task Degraded_WhenSomeFaultedAndSomeReady() + { + var check = new SessionHealthCheck(RegistryWith(SessionState.Ready, SessionState.Faulted)); + + HealthCheckResult result = await check.CheckHealthAsync(new HealthCheckContext()); + + Assert.Equal(HealthStatus.Degraded, result.Status); + Assert.Equal(1, result.Data["ready"]); + Assert.Equal(1, result.Data["faulted"]); + Assert.Contains("1 faulted", result.Description, StringComparison.Ordinal); + } + + /// + /// A session still starting counts as usable for grading, so a fault beside it is degraded + /// rather than unhealthy — the startup has not failed yet. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task Degraded_WhenFaultedBesideAStartingSession() + { + var check = new SessionHealthCheck( + RegistryWith(SessionState.Faulted, SessionState.StartingWorker)); + + HealthCheckResult result = await check.CheckHealthAsync(new HealthCheckContext()); + + Assert.Equal(HealthStatus.Degraded, result.Status); + Assert.Equal(1, result.Data["starting"]); + } + + /// Every session faulted is the genuinely bad condition, and the only unhealthy one. + /// A task that represents the asynchronous operation. + [Fact] + public async Task Unhealthy_WhenEverySessionIsFaulted() + { + var check = new SessionHealthCheck(RegistryWith(SessionState.Faulted, SessionState.Faulted)); + + HealthCheckResult result = await check.CheckHealthAsync(new HealthCheckContext()); + + Assert.Equal(HealthStatus.Unhealthy, result.Status); + Assert.Equal(2, result.Data["faulted"]); + Assert.Equal(0, result.Data["ready"]); + } + + /// + /// Closed sessions linger in the registry until they are removed. They are counted separately + /// and excluded from the verdict, so a gateway whose sessions all closed cleanly is healthy — + /// not unhealthy for having zero ready ones. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task Healthy_WhenOnlyClosedSessionsRemain() + { + var check = new SessionHealthCheck(RegistryWith(SessionState.Closed, SessionState.Closed)); + + HealthCheckResult result = await check.CheckHealthAsync(new HealthCheckContext()); + + Assert.Equal(HealthStatus.Healthy, result.Status); + Assert.Equal(2, result.Data["closing"]); + Assert.Equal(0, result.Data["ready"]); + } + + private static SessionRegistry RegistryWith(params SessionState[] states) + { + var registry = new SessionRegistry(); + for (int i = 0; i < states.Length; i++) + { + GatewaySession session = CreateSession($"session-{i}"); + session.TransitionTo(states[i]); + Assert.True(registry.TryAdd(session)); + } + + return registry; + } + + private static GatewaySession CreateSession(string sessionId) + { + return new GatewaySession( + sessionId, + "mxaccess", + $"mxaccess-gateway-1-{sessionId}", + "nonce", + clientIdentity: null, + clientSessionName: "test-session", + clientCorrelationId: "client-correlation", + TimeSpan.FromSeconds(30), + TimeSpan.FromSeconds(5), + TimeSpan.FromSeconds(5), + DateTimeOffset.UnixEpoch); + } +} From 882c7ca3cdeeed257a07740343e559671c6f9c1f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 11 Aug 2026 08:42:16 -0400 Subject: [PATCH 2/3] fix(config): reject credential and cache paths inside the application directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rooted is not the same as safe, and the gap between the two cost a production host every one of its API keys on 2026-08-09. MxGateway:Authentication:SqlitePath was set to an absolute path inside the directory the upgrade procedure renames to Server.bak.*. That passes the existing rooted check cleanly. The deploy renamed the directory away, the store went with it, and the gateway created a fresh empty one at the same path — no error, no log line. No gRPC consumer could authenticate for two days. The deploy itself was correct: the binaries were the point of the rename and the store was collateral. GatewayConfigPathRules gains AddIfUnderContentRoot, applied to the auth store and the Galaxy snapshot. Both are written by the running process and both are lost the same way. The rule compares resolved full paths and requires a directory-separator boundary, so a sibling directory whose name merely starts with the content root's ("/srv/app-data" against "/srv/app") is not treated as inside it — on a fail-closed startup rule, that false positive would be a gateway that refuses to boot on a legitimate path. Case sensitivity follows the running OS rather than assuming case-insensitivity everywhere, which would reject /srv/App as under /srv/app on Linux where they are different directories. The rule is not exempted in Development. An environment-conditional guard is never exercised where the mistake is made, and what failed in production was a config that looked fine. Secrets:SqlitePath is the same defect one layer down: it shipped as a bare relative "mxgateway-secrets.db", which is how a stray database landed in src/…Server/ and tripped the repository's tree-hygiene test. It is bound by the shared ZB.MOM.WW.Secrets package, so appsettings.json now ships no value and the default is computed from CommonApplicationData in code — the same mechanism SEC-33 already used for the Galaxy snapshot, ten lines away, for the same reason. Setting a default for an unset key is deliberately not the same act as relocating a value someone configured, which these rules still refuse to do. Note the migration edge this creates: a host relying on the old repo default now looks somewhere new, finds nothing, and creates an empty store — this bug re-introduced by its own fix. Deployed hosts are safe because they set the path explicitly, in appsettings copied forward or in the service environment. The latter is the more robust of the two, since it cannot be lost by a missed preserve step. --- docs/GatewayConfiguration.md | 6 +- .../GalaxyRepositoryOptionsValidator.cs | 32 +++++ .../Configuration/GatewayConfigPathRules.cs | 111 +++++++++++++++--- .../Configuration/GatewayOptionsValidator.cs | 38 +++++- .../GatewayApplication.cs | 56 +++++++++ .../appsettings.json | 1 - .../GalaxyRepositoryOptionsValidatorTests.cs | 43 +++++++ .../GatewayOptionsValidatorTests.cs | 63 ++++++++++ 8 files changed, 325 insertions(+), 25 deletions(-) diff --git a/docs/GatewayConfiguration.md b/docs/GatewayConfiguration.md index 7529d09..1dc80a9 100644 --- a/docs/GatewayConfiguration.md +++ b/docs/GatewayConfiguration.md @@ -91,7 +91,7 @@ Environment variables use the normal .NET double-underscore form. For example, | Option | Default | Description | |--------|---------|-------------| | `MxGateway:Authentication:Mode` | `ApiKey` | Selects public gRPC authentication. Supported values are `ApiKey` and `Disabled`. `Disabled` bypasses API-key verification and is for local development only. | -| `MxGateway:Authentication:SqlitePath` | derived from `CommonApplicationData` (`C:\ProgramData\MxGateway\gateway-auth.db` on Windows, `/usr/share/MxGateway/gateway-auth.db` or the container equivalent elsewhere) | SQLite database path for API-key records and audit rows when API-key authentication is enabled. The code default is built from `Environment.GetFolderPath(SpecialFolder.CommonApplicationData)` so the credential store never lands in the launch working directory on a non-Windows host. `appsettings.json` no longer ships an explicit value (SEC-33): the removed Windows literal was byte-identical to the Windows code default, and a Windows-absolute literal is **not** rooted on a Unix host, so it would have resolved against the CWD there. Deployed hosts still override the path through the NSSM environment (`MxGateway__Authentication__SqlitePath`). | +| `MxGateway:Authentication:SqlitePath` | derived from `CommonApplicationData` (`C:\ProgramData\MxGateway\gateway-auth.db` on Windows, `/usr/share/MxGateway/gateway-auth.db` or the container equivalent elsewhere) | SQLite database path for API-key records and audit rows when API-key authentication is enabled. The code default is built from `Environment.GetFolderPath(SpecialFolder.CommonApplicationData)` so the credential store never lands in the launch working directory on a non-Windows host. `appsettings.json` no longer ships an explicit value (SEC-33): the removed Windows literal was byte-identical to the Windows code default, and a Windows-absolute literal is **not** rooted on a Unix host, so it would have resolved against the CWD there. Deployed hosts still override the path through the NSSM environment (`MxGateway__Authentication__SqlitePath`). The validator additionally rejects a path **inside the application content root**, even an absolute one: the upgrade procedure renames that directory to `Server.bak.*`, which takes the credential store with it and silently starts an empty one. That is not hypothetical — it happened on a production host on 2026-08-09 and no gRPC client could authenticate for two days. | | `MxGateway:Authentication:PepperSecretName` | `MxGateway:ApiKeyPepper` | Configuration key used to read the HMAC pepper for API-key secret hashing. The dashboard effective configuration redacts this value. | | `MxGateway:Authentication:RunMigrationsOnStartup` | `true` | Runs SQLite auth schema migrations at gateway startup when API-key authentication is enabled. | @@ -291,7 +291,7 @@ section (a sibling of `MxGateway`, not nested under it): | Option | Default | Description | |--------|---------|-------------| -| `Secrets:SqlitePath` | `mxgateway-secrets.db` | Path to the encrypted secrets store, resolved relative to the app content root when not rooted. | +| `Secrets:SqlitePath` | `/MxGateway/mxgateway-secrets.db` | Path to the encrypted secrets store. The default is supplied in code when the key is unset (`C:\ProgramData\MxGateway\...` on Windows), not from `appsettings.json` — a store inside the application directory is renamed away by the upgrade procedure, taking the secrets with it. On non-Windows hosts the default location is usually not writable by a normal user, so a local run must set `Secrets__SqlitePath` explicitly. | | `Secrets:MasterKey:Source` | `Environment` | Key-encryption-key (KEK) provider. `Environment` reads a base64-encoded 32-byte key from an env var; `Dpapi` uses a machine-bound key file instead (see below). | | `Secrets:MasterKey:EnvVarName` | `ZB_SECRETS_MASTER_KEY` | Env var name the `Environment` provider reads the KEK from. | @@ -402,7 +402,7 @@ model requires otherwise. | `MxGateway:Galaxy:CommandTimeoutSeconds` | `60` | Per-command SQL timeout for all Galaxy browse RPCs. | | `MxGateway:Galaxy:DashboardRefreshIntervalSeconds` | `30` | Interval between background refreshes of the dashboard Galaxy summary cache. SQL is hit at most once per interval regardless of dashboard render rate. | | `MxGateway:Galaxy:PersistSnapshot` | `true` | Persists the latest successful Galaxy browse dataset to disk. When `true`, the cache reloads that snapshot at startup so clients can still browse last-known data while the Galaxy database is unreachable. The restored data is served with `Stale` status until a live query confirms it. | -| `MxGateway:Galaxy:SnapshotCachePath` | derived from `CommonApplicationData` (`C:\ProgramData\MxGateway\galaxy-snapshot.json` on Windows, `/usr/share/MxGateway/galaxy-snapshot.json` or the container equivalent elsewhere) | File path for the persisted Galaxy browse snapshot. Ignored when `PersistSnapshot` is `false`. The snapshot is written atomically (temp file plus rename). `appsettings.json` no longer ships an explicit value (SEC-33): the option is bound by the shared `ZB.MOM.WW.GalaxyRepository` package, so the gateway supplies the `CommonApplicationData`-derived default when the bound value is blank and registers `GalaxyRepositoryOptionsValidator` to enforce that — when `PersistSnapshot` is `true` — the path is non-blank, valid, and **rooted on the host running the gateway** (`Path.IsPathRooted`, current OS). A bare filename or a foreign-platform literal fails startup instead of resolving against the launch working directory (SEC-01, SEC-33). | +| `MxGateway:Galaxy:SnapshotCachePath` | derived from `CommonApplicationData` (`C:\ProgramData\MxGateway\galaxy-snapshot.json` on Windows, `/usr/share/MxGateway/galaxy-snapshot.json` or the container equivalent elsewhere) | File path for the persisted Galaxy browse snapshot. Ignored when `PersistSnapshot` is `false`. The snapshot is written atomically (temp file plus rename). `appsettings.json` no longer ships an explicit value (SEC-33): the option is bound by the shared `ZB.MOM.WW.GalaxyRepository` package, so the gateway supplies the `CommonApplicationData`-derived default when the bound value is blank and registers `GalaxyRepositoryOptionsValidator` to enforce that — when `PersistSnapshot` is `true` — the path is non-blank, valid, and **rooted on the host running the gateway** (`Path.IsPathRooted`, current OS). A bare filename or a foreign-platform literal fails startup instead of resolving against the launch working directory (SEC-01, SEC-33). The same validator also rejects a path **inside the application content root**, because the upgrade procedure renames that directory away and the cached snapshot would be discarded on every deploy. | See [Galaxy Repository Browse](./GalaxyRepository.md) for the RPC surface and behavior. diff --git a/src/ZB.MOM.WW.MxGateway.Server/Configuration/GalaxyRepositoryOptionsValidator.cs b/src/ZB.MOM.WW.MxGateway.Server/Configuration/GalaxyRepositoryOptionsValidator.cs index 15a035e..feb632a 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Configuration/GalaxyRepositoryOptionsValidator.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Configuration/GalaxyRepositoryOptionsValidator.cs @@ -13,6 +13,33 @@ namespace ZB.MOM.WW.MxGateway.Server.Configuration; /// public sealed class GalaxyRepositoryOptionsValidator : OptionsValidatorBase { + // See GatewayOptionsValidator for why this is nullable and what null means. + private readonly string? _contentRootPath; + + /// + /// Initializes a new instance of the class for + /// the dependency-injection path, taking the content root from the host environment. + /// + /// The host environment. + public GalaxyRepositoryOptionsValidator(IHostEnvironment environment) + { + ArgumentNullException.ThrowIfNull(environment); + _contentRootPath = environment.ContentRootPath; + } + + /// + /// Initializes a new instance of the class for + /// unit tests and non-DI callers. + /// + /// + /// Content root to test the snapshot path against; leaves the + /// content-root rule inactive. + /// + internal GalaxyRepositoryOptionsValidator(string? contentRootPath = null) + { + _contentRootPath = contentRootPath; + } + /// protected override void Validate(ValidationBuilder builder, GalaxyRepositoryOptions options) { @@ -37,5 +64,10 @@ public sealed class GalaxyRepositoryOptionsValidator : OptionsValidatorBase + /// Fails validation when resolves to a location inside + /// — the directory the application runs from. + /// + /// + /// + /// Rooted is not the same as safe, and this is the rule that closes the gap. + /// stops a store drifting with the working directory, but an + /// absolute path inside the app directory passes it cleanly — and that is what failed + /// in production on 2026-08-09. The upgrade procedure renames the app directory to + /// Server.bak.* and unpacks a new one; a store living there is renamed away with it, the + /// process then creates a fresh empty one at the same path, and nothing reports an error. All + /// API keys were lost and no gRPC client could authenticate for two days. The deploy itself was + /// executed correctly — the binaries were the point of the rename, and the store was collateral. + /// + /// + /// The same shape catches the dev-side symptom: a store under the content root lands in the + /// source tree, which is how mxgateway-secrets.db once tripped the repository's + /// tree-hygiene test. + /// + /// + /// Comparison is case-insensitive only on Windows. On a case-insensitive macOS volume this can + /// miss a violation that differs only in case, which is a missed warning in dev; assuming + /// case-insensitivity on Linux would instead reject a legitimate path, and a false startup + /// abort is the worse failure. + /// + /// + /// The configured path value. + /// The application content root to test against. + /// The failure message to record when the value is under the content root. + /// The validation builder accumulating failures. + public static void AddIfUnderContentRoot( + string? value, + string? contentRoot, + string message, + ValidationBuilder builder) + { + if (string.IsNullOrWhiteSpace(value) || string.IsNullOrWhiteSpace(contentRoot)) + { + return; + } + + // A malformed path is AddIfInvalidPath's message to report; staying silent here keeps one + // bad value from producing two failures that say different things about the same mistake. + if (!TryGetFullPath(value, out string fullValue) || !TryGetFullPath(contentRoot, out string fullRoot)) + { + return; + } + + fullRoot = fullRoot.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + + StringComparison comparison = OperatingSystem.IsWindows() + ? StringComparison.OrdinalIgnoreCase + : StringComparison.Ordinal; + + // The separator is load-bearing: a bare prefix test would also match a sibling directory + // whose name merely starts with the root's ("/srv/app" against "/srv/app-data"). + if (string.Equals(fullValue, fullRoot, comparison) + || fullValue.StartsWith(fullRoot + Path.DirectorySeparatorChar, comparison)) + { + builder.Add(message); + } + } + + private static bool TryGetFullPath(string value, out string fullPath) + { + try + { + fullPath = Path.GetFullPath(value); + return true; + } + catch (ArgumentException) + { + fullPath = string.Empty; + return false; + } + catch (NotSupportedException) + { + fullPath = string.Empty; + return false; + } + catch (PathTooLongException) + { + fullPath = string.Empty; + return false; + } + catch (IOException) + { + fullPath = string.Empty; + return false; + } + } } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptionsValidator.cs b/src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptionsValidator.cs index 2d18295..3732209 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptionsValidator.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptionsValidator.cs @@ -15,15 +15,22 @@ public sealed class GatewayOptionsValidator : OptionsValidatorBase /// Initializes a new instance of the class for the - /// dependency-injection path, deriving the production posture from the host environment. + /// dependency-injection path, deriving the production posture and content root from the host + /// environment. /// /// The host environment. public GatewayOptionsValidator(IHostEnvironment environment) { ArgumentNullException.ThrowIfNull(environment); _isProduction = environment.IsProduction(); + _contentRootPath = environment.ContentRootPath; } /// @@ -32,15 +39,20 @@ public sealed class GatewayOptionsValidator : OptionsValidatorBase to exercise them. /// /// Whether to treat the host as running in Production. - internal GatewayOptionsValidator(bool isProduction = false) + /// + /// Content root to test store paths against; leaves the content-root + /// rule inactive, which is what a caller with no real host wants. + /// + internal GatewayOptionsValidator(bool isProduction = false, string? contentRootPath = null) { _isProduction = isProduction; + _contentRootPath = contentRootPath; } /// protected override void Validate(ValidationBuilder builder, GatewayOptions options) { - ValidateAuthentication(options.Authentication, builder); + ValidateAuthentication(options.Authentication, _contentRootPath, builder); ValidateLdap(options.Ldap, builder, _isProduction); ValidateWorker(options.Worker, builder); ValidateSessions(options.Sessions, builder); @@ -101,7 +113,10 @@ public sealed class GatewayOptionsValidator : OptionsValidatorBase GatewayConfigPathRules.AddIfInvalidPath(value, message, builder); + + // Rooted is not the same as safe: an absolute path inside the app directory passes + // AddIfNotRooted and is still renamed away by the upgrade procedure. See + // GatewayConfigPathRules.AddIfUnderContentRoot. + private static void AddIfUnderContentRoot( + string? value, + string? contentRoot, + string message, + ValidationBuilder builder) + => GatewayConfigPathRules.AddIfUnderContentRoot(value, contentRoot, message, builder); } diff --git a/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs b/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs index 49144ee..c9d2cdd 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs @@ -70,6 +70,8 @@ public static class GatewayApplication }); StaticWebAssetsLoader.UseStaticWebAssets(builder.Environment, builder.Configuration); + ApplyDefaultSecretsStorePath(builder.Configuration); + // Resolve ${secret:...} references in configuration BEFORE any config consumer (TLS, Kestrel, // GatewayOptions/Ldap/Galaxy validators) reads a value, using a standalone secrets provider // (envelope-decrypted via the master key). A token referencing a missing secret fails fast @@ -186,6 +188,60 @@ public static class GatewayApplication }); } + /// + /// Supplies the default location of the encrypted secrets store when nothing configured one. + /// + /// + /// + /// The store used to default to a bare relative mxgateway-secrets.db, which resolves + /// against the working directory and therefore normally lands inside the application directory. + /// That is the shape that lost every API key on a production host: the upgrade procedure renames + /// the application directory away, the store goes with it, and a fresh empty one appears in its + /// place with no error. In development the same default writes a database into the source tree. + /// + /// + /// This sets a default for an unset key; it never relocates a value someone configured. + /// That distinction matters — deliberately + /// rejects bad configured paths rather than quietly moving them, because silently relocating a + /// credential store is worse than a boot error. Choosing where to put a value nobody specified + /// is a different act from overriding one they did. + /// + /// + /// The location mirrors AuthenticationOptions.SqlitePath so both gateway stores sit + /// together, and the mechanism is the one SEC-33 already used for + /// MxGateway:Galaxy:SnapshotCachePath below — same problem, same fix, same file. It also + /// matches what docs/GatewayConfiguration.md already tells operators to + /// pass to the secret CLI — an absolute default also removes the CLI/gateway divergence + /// that a working-directory-relative path can cause. On non-Windows hosts + /// is typically not writable by a + /// normal user, so a local run there must set Secrets__SqlitePath explicitly, exactly as + /// it already must for the auth store. + /// + /// + /// This deliberately differs from the ZB.MOM.WW.Secrets library default, which is + /// -derived so the family's + /// cross-platform apps still boot locally without an override. The gateway keeps + /// CommonApplicationData because it runs as a machine-wide Windows service and its other + /// two stores — the auth database and the Galaxy snapshot — already live there; splitting them + /// would be the greater inconsistency. The value set here always wins, so the library default is + /// unreachable in this app. Do not "fix" the difference by deleting this method: that would + /// silently move the store, which is the failure this whole rule exists to prevent. + /// + /// + /// The configuration to supply the default into. + private static void ApplyDefaultSecretsStorePath(IConfiguration configuration) + { + if (!string.IsNullOrWhiteSpace(configuration["Secrets:SqlitePath"])) + { + return; + } + + configuration["Secrets:SqlitePath"] = Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), + "MxGateway", + "mxgateway-secrets.db"); + } + private static void ConfigureSelfSignedTls(WebApplicationBuilder builder) { if (!Security.Tls.KestrelTlsInspector.RequiresGeneratedCertificate(builder.Configuration)) diff --git a/src/ZB.MOM.WW.MxGateway.Server/appsettings.json b/src/ZB.MOM.WW.MxGateway.Server/appsettings.json index d650c53..e99f5e0 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/appsettings.json +++ b/src/ZB.MOM.WW.MxGateway.Server/appsettings.json @@ -12,7 +12,6 @@ }, "AllowedHosts": "*", "Secrets": { - "SqlitePath": "mxgateway-secrets.db", "MasterKey": { "Source": "Environment", "EnvVarName": "ZB_SECRETS_MASTER_KEY" }, "RunMigrationsOnStartup": true, "ResolveCacheTtl": "00:00:30" diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Configuration/GalaxyRepositoryOptionsValidatorTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Configuration/GalaxyRepositoryOptionsValidatorTests.cs index 204d3bc..d1d4619 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Configuration/GalaxyRepositoryOptionsValidatorTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Configuration/GalaxyRepositoryOptionsValidatorTests.cs @@ -63,6 +63,49 @@ public sealed class GalaxyRepositoryOptionsValidatorTests Assert.True(result.Succeeded); } + /// + /// Verifies an absolute snapshot path inside the application directory fails. Rooted is not the + /// same as safe: the upgrade procedure renames that directory, so a snapshot cached there is + /// discarded on every deploy and the gateway starts cold each time. + /// + [Fact] + public void Validate_Fails_WhenSnapshotPathIsUnderContentRoot() + { + string contentRoot = Path.Combine(Path.GetTempPath(), $"mxgw-root-{Guid.NewGuid():N}"); + GalaxyRepositoryOptions options = new() + { + PersistSnapshot = true, + SnapshotCachePath = Path.Combine(contentRoot, "galaxy-snapshot.json"), + }; + + ValidateOptionsResult result = + new GalaxyRepositoryOptionsValidator(contentRoot).Validate(null, options); + + Assert.True(result.Failed); + Assert.Contains( + result.Failures!, + f => f.Contains("MxGateway:Galaxy:SnapshotCachePath") + && f.Contains("must not be inside the application directory")); + } + + /// Verifies a snapshot path outside the application directory still passes. + [Fact] + public void Validate_Succeeds_WhenSnapshotPathIsOutsideContentRoot() + { + string contentRoot = Path.Combine(Path.GetTempPath(), $"mxgw-root-{Guid.NewGuid():N}"); + GalaxyRepositoryOptions options = new() + { + PersistSnapshot = true, + SnapshotCachePath = + Path.Combine(Path.GetTempPath(), $"mxgw-data-{Guid.NewGuid():N}", "galaxy-snapshot.json"), + }; + + ValidateOptionsResult result = + new GalaxyRepositoryOptionsValidator(contentRoot).Validate(null, options); + + Assert.True(result.Succeeded); + } + /// /// Verifies the gateway supplies a rooted per-OS default when the shipped config leaves /// SnapshotCachePath blank, so the removed appsettings literal is not needed and validation diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Configuration/GatewayOptionsValidatorTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Configuration/GatewayOptionsValidatorTests.cs index c84aa3b..f5f2120 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Configuration/GatewayOptionsValidatorTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Configuration/GatewayOptionsValidatorTests.cs @@ -598,6 +598,69 @@ public sealed class GatewayOptionsValidatorTests f => f.Contains("MxGateway:Authentication:SqlitePath") && f.Contains("rooted")); } + /// + /// Verifies an absolute auth DB path inside the application directory fails. This is + /// the gap the rooted check does not close: the path that lost every API key on a production + /// host on 2026-08-09 was absolute and passed rooting cleanly — it simply lived in the directory + /// the upgrade procedure renames away. + /// + [Fact] + public void Validate_Fails_WhenSqlitePathIsUnderContentRoot() + { + string contentRoot = Path.Combine(Path.GetTempPath(), $"mxgw-root-{Guid.NewGuid():N}"); + GatewayOptions options = CloneWithAuthentication( + ValidOptions(), + new AuthenticationOptions { SqlitePath = Path.Combine(contentRoot, "gateway-auth.db") }); + + ValidateOptionsResult result = + new GatewayOptionsValidator(contentRootPath: contentRoot).Validate(null, options); + + Assert.True(result.Failed); + Assert.Contains( + result.Failures!, + f => f.Contains("MxGateway:Authentication:SqlitePath") + && f.Contains("must not be inside the application directory")); + } + + /// + /// Verifies the content-root rule is not a bare string prefix test: a sibling directory whose + /// name merely begins with the content root's must still pass. + /// + [Fact] + public void Validate_Succeeds_WhenSqlitePathIsSiblingOfContentRoot() + { + string contentRoot = Path.Combine(Path.GetTempPath(), $"mxgw-root-{Guid.NewGuid():N}"); + GatewayOptions options = CloneWithAuthentication( + ValidOptions(), + new AuthenticationOptions { SqlitePath = contentRoot + "-data" + Path.DirectorySeparatorChar + "gateway-auth.db" }); + + ValidateOptionsResult result = + new GatewayOptionsValidator(contentRootPath: contentRoot).Validate(null, options); + + Assert.True(result.Succeeded); + } + + /// + /// Verifies a store path outside the application directory passes — the rule must reject only + /// the genuinely unsafe location, not every absolute path. + /// + [Fact] + public void Validate_Succeeds_WhenSqlitePathIsOutsideContentRoot() + { + string contentRoot = Path.Combine(Path.GetTempPath(), $"mxgw-root-{Guid.NewGuid():N}"); + GatewayOptions options = CloneWithAuthentication( + ValidOptions(), + new AuthenticationOptions + { + SqlitePath = Path.Combine(Path.GetTempPath(), $"mxgw-data-{Guid.NewGuid():N}", "gateway-auth.db"), + }); + + ValidateOptionsResult result = + new GatewayOptionsValidator(contentRootPath: contentRoot).Validate(null, options); + + Assert.True(result.Succeeded); + } + /// /// Verifies rooting is host-meaningful (SEC-33): a Windows drive-qualified literal fails on a /// Unix host (where it is not rooted) rather than being blessed and written as a junk-named From 6ba52a68f03fa4a98f230e1da8bd047838eea901 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 11 Aug 2026 08:42:28 -0400 Subject: [PATCH 3/3] build(secrets): re-pin ZB.MOM.WW.Secrets to 0.6.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0.6.0 moves the store-path rules into the shared library — both the rooted check and the content-root check — with a SecretsOptionsValidator wired into AddZbSecrets and validated on start, so all four consuming apps get the guard from one implementation rather than four copies. mxgw therefore adds no local validator over the Secrets section. This is a four-minor jump, not a re-pin: mxgw was on 0.2.3 and skips 0.3.0, 0.4.0, 0.4.1, 0.5.0 and 0.5.1 in one step. Verified rather than assumed — diffing the 0.2.3 and 0.6.0 assemblies shows the added surface is the new path rules and their plumbing (AddIfNotRooted, AddIfUnderContentRoot, ComputeDefaultSqlitePath, DefaultSqlitePath, IValidateOptions, IsPathRooted, GetFullPath) and nothing touching store or delete behaviour. Secrets.Ui is byte-identical across the range: both assemblies are 39424 bytes and differ only in the version stamp, because this package family shares one version across every package even when a release touches only one of them. So the browser gate run against the /admin/secrets delete modal on 0.2.3 still covers what ships here. The library default is LocalApplicationData-derived so the family's cross-platform apps still boot locally. The gateway keeps its own CommonApplicationData value, which always wins — see the note on ApplyDefaultSecretsStorePath for why the difference is deliberate and why deleting that method as a redundancy would silently move the store. --- .../ZB.MOM.WW.MxGateway.Server.csproj | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ZB.MOM.WW.MxGateway.Server/ZB.MOM.WW.MxGateway.Server.csproj b/src/ZB.MOM.WW.MxGateway.Server/ZB.MOM.WW.MxGateway.Server.csproj index 8d299d8..e9258b2 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/ZB.MOM.WW.MxGateway.Server.csproj +++ b/src/ZB.MOM.WW.MxGateway.Server/ZB.MOM.WW.MxGateway.Server.csproj @@ -21,9 +21,9 @@ - - - + + +