[Secrets] AddZbSecrets before the SqlServer/hub extensions silently cancels the shared store / replicator #30

New Issue

2026-11-09T14:39:18-05:00

dohertj2 commented

2026-11-09 14:39:18 -05:00

From the scadaproj#15 shared-library review (code-reviews/Secrets/findings.md, Secrets-001), verified at 91fb2b4. Update the finding's Resolution row when fixed.


Severity	Medium
Priority	P2
Category	Public API surface & compatibility
Status	Open
Location	`ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/DependencyInjection/SqlServerSecretsServiceCollectionExtensions.cs:53` (also `:54`, `:90`)

Description

Both SQL-Server modes put their registrations in with TryAddSingleton, and they only take effect because they run before the extension's own internal AddZbSecrets call (:57, :94). If the application has already called AddZbSecrets, the SQLite ISecretStore, the SQLite ISecretsStoreMigrator and NoOpSecretReplicator are already registered. Every TryAdd then does nothing:

AddZbSecretsSqlServerStore changes nothing at all.
AddZbSecretsSqlServerReplication still decorates the store, but with the no-op replicator.

There is no exception, no log line and no documentation of the order requirement. Calling AddZbSecrets first is the natural order: it is the order the README teaches, and it is the order an application uses to pass contentRootPath (see Secrets-002).

Failure scenario

The application calls services.AddZbSecrets(cfg, "Secrets", contentRoot) and then services.AddZbSecretsSqlServerStore(cfg). The probe resolved ISecretStore = SqliteSecretStore and ISecretsStoreMigrator = SqliteSecretsStoreMigrator. Each node of the "shared-store" central pair therefore writes to its own local SQLite file. A secret set on node A is never visible on node B, and the hub database is never even migrated. In hub mode the probe resolved ISecretReplicator = NoOpSecretReplicator, so live publish is silently disabled and propagation waits for the sync interval.

Recommendation

Make the SQL-Server extensions order-independent: use Replace / RemoveAll + Add for ISecretStore, ISecretsStoreMigrator and ISecretReplicator, rather than TryAdd. Alternatively, throw if a conflicting registration is already present. Add container tests for both orders.

Resolution

Unresolved.

From the scadaproj#15 shared-library review (`code-reviews/Secrets/findings.md`, **Secrets-001**), verified at 91fb2b4. Update the finding's Resolution row when fixed. | | | |--|--| | Severity | Medium | | Priority | P2 | | Category | Public API surface & compatibility | | Status | Open | | Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/DependencyInjection/SqlServerSecretsServiceCollectionExtensions.cs:53` (also `:54`, `:90`) | **Description** Both SQL-Server modes put their registrations in with `TryAddSingleton`, and they only take effect because they run before the extension's own internal `AddZbSecrets` call (`:57`, `:94`). If the application has already called `AddZbSecrets`, the SQLite `ISecretStore`, the SQLite `ISecretsStoreMigrator` and `NoOpSecretReplicator` are already registered. Every `TryAdd` then does nothing: - `AddZbSecretsSqlServerStore` changes nothing at all. - `AddZbSecretsSqlServerReplication` still decorates the store, but with the no-op replicator. There is no exception, no log line and no documentation of the order requirement. Calling `AddZbSecrets` first is the natural order: it is the order the README teaches, and it is the order an application uses to pass `contentRootPath` (see Secrets-002). **Failure scenario** The application calls `services.AddZbSecrets(cfg, "Secrets", contentRoot)` and then `services.AddZbSecretsSqlServerStore(cfg)`. The probe resolved `ISecretStore = SqliteSecretStore` and `ISecretsStoreMigrator = SqliteSecretsStoreMigrator`. Each node of the "shared-store" central pair therefore writes to its own local SQLite file. A secret set on node A is never visible on node B, and the hub database is never even migrated. In hub mode the probe resolved `ISecretReplicator = NoOpSecretReplicator`, so live publish is silently disabled and propagation waits for the sync interval. **Recommendation** Make the SQL-Server extensions order-independent: use `Replace` / `RemoveAll` + `Add` for `ISecretStore`, `ISecretsStoreMigrator` and `ISecretReplicator`, rather than `TryAdd`. Alternatively, throw if a conflicting registration is already present. Add container tests for both orders. **Resolution** _Unresolved._

dohertj2 added the type:review-finding priority:p2 labels 2026-11-09 14:39:18 -05:00

dohertj2 commented

2026-11-09 15:01:02 -05:00

Fixed on main 4f069c1 (Secrets-001 → Resolved): the SqlServer and Akka extensions call AddZbSecrets themselves, then replace the store/migrator/replicator registrations instead of TryAdd-ing first — registration order no longer matters (before: AddZbSecrets first → SQLite store / NoOpSecretReplicator in hub and Akka modes). Applied to AddZbSecretsAkkaReplication too. Evidence: Secrets solution (5 test projects) all pass — Core 246 (1 DPAPI skip), SqlServer 60 (16 live-SQL skips), Akka 44, Grpc 196, Ui 20; each regression test fails with its fix removed. Ships in Secrets 0.6.6 (unpublished).

Fixed on main 4f069c1 (Secrets-001 → Resolved): the SqlServer and Akka extensions call AddZbSecrets themselves, then *replace* the store/migrator/replicator registrations instead of TryAdd-ing first — registration order no longer matters (before: AddZbSecrets first → SQLite store / NoOpSecretReplicator in hub and Akka modes). Applied to AddZbSecretsAkkaReplication too. Evidence: Secrets solution (5 test projects) all pass — Core 246 (1 DPAPI skip), SqlServer 60 (16 live-SQL skips), Akka 44, Grpc 196, Ui 20; each regression test fails with its fix removed. Ships in Secrets **0.6.6 (unpublished)**.

dohertj2 closed this issue

2026-11-09 15:01:02 -05:00

dohertj2 referenced this issue from a commit

2026-11-09 15:01:03 -05:00

fix(secrets): SqlServer/Akka extensions take contentRootPath; last explicit root wins (scadaproj#31)

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dohertj2/scadaproj#30