Commit Graph

7 Commits

Author SHA1 Message Date
Joseph Doherty c5e894d9b3 feat(secrets): 0.6.2 — the content-root rejection message teaches the container case
Suggested by the OtOpcUa session, the only real containerized deployment in this family
and the one that hit the false positive. A store on a volume mounted inside the content
root is durable across container replacement but is rejected anyway, because the rule
cannot tell a mount from an ordinary directory. The message now says so, gives the fix,
and forecloses the wrong one by stating that no override exists.

A false positive's entire cost is the confusion in the moment, so an error that names its
own fix costs nothing on the true-positive path and turns the false positive into a
two-minute edit. Message only, no behaviour change.

Their case for keeping the rule strict is recorded in the CHANGELOG because it is better
than the trade-off I had written: the workaround is an improvement rather than a
concession, an override flag has the lifecycle of a knob set once and never revisited,
and mount detection would make the verdict depend on runtime platform state so the same
path would pass on one host and fail on another.
2026-08-11 09:04:02 -04:00
Joseph Doherty 6084465e23 test(secrets): the positive control was a tautology propping up the negative one
Raised by the mxaccessgw session, which went back to its own tests after I reported a
vacuous assertion in mine and found the same class one step further from the fuse.

The rejecting test asserts no store file is left at a rejected path. That claim is only
meaningful if something proves the write path is live at all: "no file was created" is
indistinguishable from "nothing in this container ever creates files". The positive
control was supposed to establish that and did not -- it asserted Assert.NotNull on a
resolved SecretsSqliteConnectionFactory, which is close to a tautology once no exception
escaped, and it never ran the migration at all. So if the pre-host container ever stopped
touching the store, the rejecting test would have gone quietly vacuous while the positive
control kept passing. Neither would have failed.

The positive control now runs SqliteSecretsStoreMigrator.MigrateAsync and asserts the
database IS created. The pair is mutually checking: the migration writes the file when
allowed and does not when rejected, and neither assertion stands on its own.

Verified the same way as the last one, by simulating the failure rather than reasoning
about it: replacing the migration with a no-op makes the positive control fail with "the
accepted path must actually get a store file". Reverted after measuring.

No library change; 0.6.1 as published is unaffected. Suite 502 passing.
2026-08-11 08:56:46 -04:00
Joseph Doherty d6fe3ae62a test(secrets): the pre-host regression test was asserting the weaker claim, and vacuously
Two defects in the 0.6.1 regression test, the first raised by the mxaccessgw session
from its own adoption, the second exposed by acting on it.

Assertion order. The test asserted the throw before the artifact. That hides the
stronger finding: a regression that stops the throw but still creates the store fails
with "no exception was thrown" and never mentions the database sitting at the rejected
path -- which IS the defect. Now captures with Record.ExceptionAsync, asserts no file
created first, and asserts the exception type last. Notable because the defect under
test was itself "a boot outcome was observed instead of the artifact being probed", so
closing it with an outcome-first assertion would have repeated the mistake.

Vacuous assertions. Simulating the regression -- replacing the 4-argument call with the
3-argument one, which still compiles, exactly as the call-site comments warn -- showed
the test failing on the exception assertion rather than the file assertion. No file had
been created in EITHER case: the test resolved SecretsSqliteConnectionFactory, whose
constructor never touches the filesystem. The File.Exists assertions could not fail. The
test now resolves SqliteSecretsStoreMigrator and runs MigrateAsync, which is what a
pre-host expander actually does and what actually creates the database.

Verified by simulating the regression again: the test now fails with "a rejected path
must not be left holding a store file" rather than "no exception was thrown". Reverted
after measuring.

No library change; 0.6.1 as published is unaffected. Suite 502 passing.
2026-08-11 08:52:53 -04:00
Joseph Doherty cbfe296279 fix(secrets): close the pre-host hole in the store-path guard (0.6.1)
Found by the HistorianGateway adoption of 0.6.0, which probed the rejected paths
instead of observing a clean boot — a clean boot being equally consistent with the
validator not being wired at all.

An application that expands ${secret:} references before building its host composes
this subsystem in a throwaway ServiceCollection with no IHostEnvironment in it, then
migrates the store from that container. SecretsOptionsValidator resolves the content
root from IHostEnvironment and skips the under-content-root rule when it is absent, so
pre-host an absolute in-content-root path passed the only rule still applying, the
migrator CREATED the database, and the boot failed a second later once the real host
registered IHostEnvironment and the rule finally fired.

The observable result was a rejected boot that still left an empty store, with -wal and
-shm siblings, at the rejected path — the same artifact that made the MxGateway outage
read as "the database is there, it's just empty", which is precisely what this guard
exists to prevent. A relative path was unaffected because the absolute-path rule fires
with or without a content root; that asymmetry is what exposed it.

The mistake was conflating "no IHostEnvironment registered" with "no content root
exists". New AddZbSecrets overload takes the content root explicitly; the 3-argument
overload is unchanged and still falls back to IHostEnvironment, so this is additive.
SecretsOptionsValidator gains a string? constructor alongside the IHostEnvironment? one.

Reproduced first in an isolated harness before changing anything: a bare host rejects
both bad paths leaving no artifacts, and a bare ServiceCollection with no
IHostEnvironment migrates an in-content-root path to disk. Both are now regression tests,
the second paired with a positive control asserting the old behaviour still holds when no
content root is supplied — so the test is about the explicit content root rather than
about paths in general.

Also documents an upgrade note that applies to 0.6.0: a store seeded under the old
relative default is orphaned rather than migrated, and nothing signals it.
2026-08-11 08:37:53 -04:00
Joseph Doherty 944595ad32 feat(secrets): fail-closed store-path validation; default moves outside the content root
Secrets:SqlitePath is now validated at startup by SecretsOptionsValidator using two
rules in the new public SecretsStorePathRules. Both come from production incidents and
neither implies the other.

Must be absolute: a relative value resolves against the process working directory,
which for a Windows service is C:\Windows\System32, so the store lands outside the
installation; in a dev loop it lands in the source tree (a gateway was observed writing
its secrets database into src/, tripping that repo's own tree-hygiene test).

Must not be inside the content root: an absolute path can still point into the
directory every deployment procedure in this family renames on upgrade. On 2026-08-09 a
gateway's credential store sat inside its Server\ directory; the deploy renamed Server
to Server.bak.*, the database went with it, and the process created a fresh empty one
at the same path. Every consumer lost its credential, silently, for two days. The
existing "must be absolute" convention did not help — the path was absolute.

The rules reject rather than relocate. Silently moving a credential store presents later
as an authentication failure pointing nowhere near the configuration that caused it; a
boot error is seen immediately by whoever caused it. Validation is deliberately not
exempted in Development: a guard that never runs in a dev loop or in CI rots, and the
configuration behind the outage looked entirely reasonable.

BREAKING for configuration, not API. SecretsOptions.SqlitePath now defaults to an
absolute per-user path under LocalApplicationData; it was "secrets.db", which the new
rules reject. An application configuring a relative path will fail to boot — remove the
key or set an absolute one outside the content root. All five deployed instances of this
family were checked first and already set an absolute path outside their content root,
so the migration edge (a host relying on the OLD default now finding nothing and
creating an empty store — this bug, re-introduced by its own fix) does not apply to any
of them.

Per-user rather than machine-wide is deliberate: CommonApplicationData is /usr/share on
Linux and macOS, which a normal user cannot write, so a machine-wide library default
would fail every consuming application's boot on a developer machine. An application may
compute its own default; the configured value always wins.

Validator registration is idempotent. AddZbSecrets is legitimately called more than once
in real compositions, and TryAddEnumerable cannot de-duplicate a factory-built
descriptor — it de-duplicates on implementation type, which a factory does not carry, so
it throws on the second call rather than skipping. The existing suite caught this.

Tests cover both rules including the segment-boundary case (/srv/app-data must not read
as inside /srv/app — a false positive on a fail-closed startup rule is an application
that will not boot), that the shipped default satisfies the rules it ships with, that
the default is not machine-wide, idempotent registration, and — most importantly — that
a bad path actually throws when IOptions is resolved, since a correctly implemented
validator that is never invoked is the same "guard that does not fire" failure this
change exists to prevent.

Family bumps to 0.6.0. Adds a dependency on ZB.MOM.WW.Configuration 0.1.0 for the shared
OptionsValidatorBase/ValidationBuilder vocabulary, which every consumer already
references.
2026-08-11 08:18:37 -04:00
Joseph Doherty 9bd3c76d96 fix(secrets): ${secret:} expander skips comment keys (_-prefixed leaf) — 0.1.2
Live HistorianGateway boot caught that the expander scanned a '_secretsComment' appsettings
value documenting the ${secret:...} syntax, tried to resolve the literal example token, and
crashed boot. The family uses _*Comment keys pervasively, so skip any config key whose leaf
segment starts with '_' (the _comment convention). Functional keys never lead with '_'.
2026-07-16 04:16:25 -04:00
Joseph Doherty 0bb553212a feat(secrets): ${secret:} config expander (fail-closed) 2026-07-15 16:58:34 -04:00