[Secrets] Akka replication actor only starts on first ISecretStore use — read-only nodes never receive rotations #33

New Issue

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

dohertj2 commented

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

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


Severity	Medium
Priority	P2
Category	Correctness & logic bugs
Status	Open
Location	`ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/DependencyInjection/AkkaSecretsServiceCollectionExtensions.cs:79`

Description

The actor is created only when SecretReplicationActorProvider.ActorRef is read:

SecretReplicationActorProvider wraps system.ActorOf(...) in a Lazy<IActorRef> (lines 128–132).
The only code that reads .ActorRef is the ISecretReplicator factory (line 91).
That factory is resolved only by the ReplicatingSecretStore factory (lines 99–102), which is the ISecretStore registration.

No hosted service resolves any of these. The only hosted service registered is SecretsMigrationHostedService, which takes ISecretsStoreMigrator only. So until application code resolves ISecretStore (or ISecretResolver, which depends on it), there is no actor. Without the actor, this node:

has no topic subscription and receives none of its peers' live pushes;
sends no manifest announces;
does not answer pull requests.

The node is invisible to replication, and nothing logs that.

Failure scenario

On a 2-node pair, node B's host uses the pre-host ${secret:} expander, which reads its own store instance. At runtime B never touches ISecretStore: nobody opens /admin/secrets on B, and there is no runtime resolve. An operator rotates db/central on node A.

A's live push and every announce reach nothing on B.
B's SQLite keeps the old value indefinitely.
On B's next restart, or a failover onto B, the pre-host expander resolves the stale credential, and login fails.

Verification: a throwaway probe project (not committed) builds the real graph and starts the hosted services. It printed actor exists = False before resolving ISecretStore and True after. HostedProcessResolutionTests hides the gap because it resolves the store explicitly. Its own comment says OtOpcUa's startup hook happens to resolve the store, so the known consumer is likely covered by accident. The library contract does not guarantee this.

Recommendation

Register a small IHostedService in AddZbSecretsAkkaReplication that reads SecretReplicationActorProvider.ActorRef in StartAsync. Do this after the ActorSystem exists; resolving the provider does not re-enter the scadaproj#1 cycle, because the invalidator is still deferred. Add a test that starts the hosted services and asserts that /user/<ActorName> exists without anything resolving ISecretStore. Failing that, document that hosts must resolve ISecretStore at startup.

Resolution

Unresolved.

From the scadaproj#15 shared-library review (`code-reviews/Secrets/findings.md`, **Secrets-010**), verified at 91fb2b4. Update the finding's Resolution row when fixed. | | | |--|--| | Severity | Medium | | Priority | P2 | | Category | Correctness & logic bugs | | Status | Open | | Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/DependencyInjection/AkkaSecretsServiceCollectionExtensions.cs:79` | **Description** The actor is created only when `SecretReplicationActorProvider.ActorRef` is read: - `SecretReplicationActorProvider` wraps `system.ActorOf(...)` in a `Lazy<IActorRef>` (lines 128–132). - The only code that reads `.ActorRef` is the `ISecretReplicator` factory (line 91). - That factory is resolved only by the `ReplicatingSecretStore` factory (lines 99–102), which is the `ISecretStore` registration. No hosted service resolves any of these. The only hosted service registered is `SecretsMigrationHostedService`, which takes `ISecretsStoreMigrator` only. So until application code resolves `ISecretStore` (or `ISecretResolver`, which depends on it), there is no actor. Without the actor, this node: - has no topic subscription and receives none of its peers' live pushes; - sends no manifest announces; - does not answer pull requests. The node is invisible to replication, and nothing logs that. **Failure scenario** On a 2-node pair, node B's host uses the pre-host `${secret:}` expander, which reads its own store instance. At runtime B never touches `ISecretStore`: nobody opens `/admin/secrets` on B, and there is no runtime resolve. An operator rotates `db/central` on node A. - A's live push and every announce reach nothing on B. - B's SQLite keeps the old value indefinitely. - On B's next restart, or a failover onto B, the pre-host expander resolves the stale credential, and login fails. Verification: a throwaway probe project (not committed) builds the real graph and starts the hosted services. It printed `actor exists = False` before resolving `ISecretStore` and `True` after. `HostedProcessResolutionTests` hides the gap because it resolves the store explicitly. Its own comment says OtOpcUa's startup hook happens to resolve the store, so the known consumer is likely covered by accident. The library contract does not guarantee this. **Recommendation** Register a small `IHostedService` in `AddZbSecretsAkkaReplication` that reads `SecretReplicationActorProvider.ActorRef` in `StartAsync`. Do this after the ActorSystem exists; resolving the provider does not re-enter the scadaproj#1 cycle, because the invalidator is still deferred. Add a test that starts the hosted services and asserts that `/user/<ActorName>` exists without anything resolving `ISecretStore`. Failing that, document that hosts must resolve `ISecretStore` at startup. **Resolution** _Unresolved._

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

dohertj2 commented

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

Fixed on main 9fd1a7e (Secrets-010 → Resolved): a new hosted service creates the Akka replication actor once the host has started (in StartAsync without a host lifetime). The ActorSystem must be resolvable at host start even on nodes that never touch a secret — a failure is logged, not fatal. OtOpcUa's own SecretReplicationStarter becomes redundant once it adopts 0.6.6. 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 9fd1a7e (Secrets-010 → Resolved): a new hosted service creates the Akka replication actor once the host has started (in StartAsync without a host lifetime). The ActorSystem must be resolvable at host start even on nodes that never touch a secret — a failure is logged, not fatal. OtOpcUa's own SecretReplicationStarter becomes redundant once it adopts 0.6.6. 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:04 -05:00

dohertj2 referenced this issue

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

[Secrets] CLI appsettings SqlitePath is relative (fails validation); plain IConfigurationRoot reload gap; OtOpcUa starter now redundant #49

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dohertj2/scadaproj#33