scadaproj/code-reviews/Secrets/findings.md

Code Review — Secrets

Field	Value
Library	ZB.MOM.WW.Secrets/
Packages	ZB.MOM.WW.Secrets.Abstractions, ZB.MOM.WW.Secrets, ZB.MOM.WW.Secrets.Replicator.SqlServer, ZB.MOM.WW.Secrets.Replicator.AkkaDotNet, ZB.MOM.WW.Secrets.Replicator.Grpc, ZB.MOM.WW.Secrets.Ui, ZB.MOM.WW.Secrets.Cli (tool)
Component spec	components/secrets/
Shared contract	components/secrets/ (see also ZB.MOM.WW.Secrets/docs/)
Status	Reviewed
Last reviewed	2026-11-09
Reviewer	Claude (scadaproj#15)
Commit reviewed	91fb2b4
Open findings	17

Summary

First review of the library (scadaproj#15). The cryptographic core is sound:

• AES-256-GCM envelope with a fresh random DEK and fresh 96-bit nonces for every seal and every wrap.
• The secret name is bound as AAD on the body, and the KEK id on the wrapped DEK.
• The DEK is zeroed after use, and KEK rewrap is compare-and-swap.
• SQL identifiers are allow-listed and all SQL is parameterized.
• No plaintext or token reaches a log or an exception message.
• The gRPC hub interceptor compares the token in constant time, fails closed, and gates every handler shape.
• Bundles carry ciphertext only.

The defects are in composition, configuration and the operator surfaces, not in the cipher:

• SQL-Server DI wiring depends on call order (Secrets-001, Secrets-002). AddZbSecretsSqlServerStore / AddZbSecretsSqlServerReplication depend on TryAdd ordering, so calling AddZbSecrets first silently cancels them. They also cannot pass contentRootPath, which reopens the pre-host store-path hole that 0.6.1 closed. In hub mode no registration order gets both right.
• Config reload undoes ${secret:} expansion (Secrets-003). Expansion is written into the providers' in-memory data, so the next appsettings.json reload reverts it to the placeholder text.
• Akka replication starts lazily (Secrets-010). The replication actor exists only once something resolves ISecretStore, so a node that only uses the pre-host expander never replicates.
• UI mis-reports a committed write (Secrets-013). If the audit write fails after the store write succeeded, the UI reports and audits the change as a failure, and it skips cache eviction.
• CLI takes plaintext on the command line (Secrets-017). secret set/rotate accept the value as an argument, and the host builder re-parses it as configuration.
• The rest are P3: robustness, latent and documentation issues.

Known open items are not re-reported: the SQLite concurrent-init fix (#25), the Microsoft.Data.Sqlite pool bump (#26) and the fate of Replicator.Grpc (#8).

Build and test (macOS, 91fb2b4):

• Secrets.Tests: 224 passed, 1 skipped (DPAPI).
• Replicator.SqlServer.Tests: 49 passed, 16 skipped (live SQL).
• The Akka, Grpc and Ui suites are green.

Every DI extension has a test that builds the container. The gaps are in which orderings and paths those tests exercise:

• No test mixes AddZbSecrets with a SQL-Server extension, in either order (Secrets-001/002).
• The Akka hosted-resolution test resolves the store explicitly, which hides Secrets-010.

The following were confirmed with throwaway probes in the session scratchpad: Secrets-001, Secrets-002 (with a positive control), Secrets-003, Secrets-004, Secrets-009 (token half), Secrets-010, Secrets-011 and Secrets-017.

Checklist coverage

#	Category	Examined	Notes
1	Correctness & logic bugs	☑	Secrets-003 (expansion reverted on reload), Secrets-006 (stale re-cache after Invalidate), Secrets-010 (Akka actor created lazily), Secrets-013 (committed write reported as failed), Secrets-014 (Add overwrites), Secrets-018 (partial bundle import)
2	Public API surface & compatibility	☑	Secrets-001/002: the SQL-Server extensions depend on registration order and have no contentRootPath parameter. Secrets-016: the public Razor components do no authorization themselves
3	Concurrency & thread safety	☑	Secrets-006 (Invalidate racing an in-flight resolve), Secrets-007 (KEK id and key read non-atomically), Secrets-011 (HandleAnnounceTick publishes from a continuation and loses its sender). The actor otherwise captures Self/Sender before every await, and the interceptor counters use Interlocked
4	Error handling & resilience	☑	Secrets-004 (ArgumentException escapes Decrypt), Secrets-008 (push sweep not isolated per row), Secrets-019 (cancellation surfaces as RpcException)
5	Security & secret handling	☑	Crypto core sound (see Summary). Secrets-005 (lexical path-rule bypasses), Secrets-006 (expired plaintext kept), Secrets-007 (KEK never zeroed), Secrets-015 (reveal fails open on audit), Secrets-016, Secrets-017 (plaintext on argv). The hub endpoint allows http, so the bearer token can travel in cleartext; this appears to be a deliberate design choice and is not raised
6	Performance & resource management	☑	Secrets-007 (KEK re-read twice per operation), Secrets-012 (Akka replies unchunked, so they exceed the frame size). Connections, channels and CTSs are disposed correctly
7	Spec & shared-contract adherence	☑	Secrets-004 (ISecretCipher.Decrypt contract), Secrets-011 (the actor's "slower store" claim)
8	Packaging, dependencies & project layout	☑	No issues found
9	Testing coverage	☑	Container tests exist for every extension, but they miss mixed call order (Secrets-001/002), the lazy actor (Secrets-010), audit-throws in the UI (Secrets-013) and the AddSecretsAuthorization policy matrix (Secrets-016)
10	Documentation & XML docs	☑	Secrets-009 (the README sample fails validation). The docs do not say that an Akka node must resolve ISecretStore (Secrets-010)

Findings

Secrets-001 — Calling AddZbSecrets before a SQL-Server extension silently cancels the SQL-Server store / hub replicator

Severity	Medium
Priority	P2
Category	Public API surface & compatibility
Status	Resolved
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

Resolved 2026-11-09 in the commit tagged (scadaproj#30) (Secrets 0.6.6, unpublished). AddZbSecretsSqlServerStore, AddZbSecretsSqlServerReplication and — the same defect class, not in the original finding — AddZbSecretsAkkaReplication now call AddZbSecrets first and then RemoveAll + AddSingleton their ISecretStore / ISecretsStoreMigrator / ISecretReplicator, instead of TryAdd-ing them beforehand. A later application AddZbSecrets only TryAdds, so it cannot displace them; either order yields one ISecretStore registration. Regression tests build and resolve the real container in both orders: Replicator.SqlServer.Tests/DependencyInjection/RegistrationOrderTests (shared-store + hub) and Replicator.AkkaDotNet.Tests/DependencyInjection/RegistrationOrderTests (against a joined single-node cluster). Without the fix the core-first cases resolve SqliteSecretStore / NoOpSecretReplicator, and the extension-first hub/Akka cases see two ISecretStore registrations.

Secrets-002 — The SQL-Server extensions cannot pass the content root, so the pre-host store-path hole reopens in hub mode

Severity	Medium
Priority	P2
Category	Security & secret handling
Status	Resolved
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/DependencyInjection/SqlServerSecretsServiceCollectionExtensions.cs:94 (with ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DependencyInjection/SecretsServiceCollectionExtensions.cs:76)

Description

Both SQL-Server extensions call the 3-arg AddZbSecrets(config, section), which uses contentRootPath: null. The validator registration is first-wins (SecretsServiceCollectionExtensions.cs:76), so:

• If the SQL-Server extension runs first, a later 4-arg AddZbSecrets(..., contentRoot) cannot supply the content root.
• If the 4-arg call runs first, Secrets-001 disables the replicator.

In a pre-host container (no IHostEnvironment), the under-content-root rule is therefore skipped in hub-replication mode, where the local SQLite store is migrated. This is the exact 0.6.0 hole that the 4-arg overload exists to close. The family guidance ("use the 4-arg overload at every pre-host call site") cannot be followed with these extensions, and there is no registration order that gets both behaviours right.

Failure scenario

The probe used hub mode with Secrets:SqlitePath = <contentRoot>/data/s.db. It called AddZbSecretsSqlServerReplication(cfg) and then AddZbSecrets(cfg, "Secrets", contentRoot), then resolved SqliteSecretsStoreMigrator, and no validation error was raised. The pre-host expander migrates an empty store inside the content root. The next deploy renames the directory, and the store is lost (the 2026-08-09 incident shape). Positive control: the same configuration with only the 4-arg AddZbSecrets throws OptionsValidationException.

Recommendation

Add a string? contentRootPath parameter to both SQL-Server extensions and forward it. Also make the validator registration "last explicit content root wins" rather than first-wins, for example by storing the root in an options object that each call may set.

Resolution

Resolved 2026-11-09 in the commit tagged (scadaproj#31) (Secrets 0.6.6, unpublished). Both recommendations done: (1) AddZbSecretsSqlServerStore, AddZbSecretsSqlServerReplication and AddZbSecretsAkkaReplication gained a trailing optional string? contentRootPath = null, forwarded to the 4-arg AddZbSecrets; (2) the content root moved out of the first-registering call's closure into one shared SecretsContentRoot instance holder in the service collection, which every AddZbSecrets call may set — the last explicit root wins and a null never erases an earlier one; the validator factory still falls back to IHostEnvironment only when no explicit root was given. Regression tests: Replicator.SqlServer.Tests/DependencyInjection/ContentRootTests (hub mode refuses an in-root store via the extension argument, extension-then-core and core-then-extension, artifact asserted before the throw, with an outside-root positive control; shared-store mode forwards too), SecretsStorePathRulesTests.The_last_explicit_content_root_wins_whichever_call_registered_the_validator, and AddZbSecretsAkkaReplicationTests.The_content_root_argument_reaches_the_store_path_guard. Without the holder change the extension-then-core and 3-arg-first cases create the store and do not throw.

Secrets-003 — ${secret:} expansion is undone by configuration reload

Severity	Medium
Priority	P2
Category	Correctness & logic bugs
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs:115

Description

ExpandConfigurationAsync writes the expanded value through config[key] = …. That only sets the value in the providers' in-memory Data. A file provider's Load() replaces Data on reload, so every expanded key reverts to its literal ${secret:…} placeholder. Reload happens on IConfigurationRoot.Reload(), or on any appsettings*.json change when reloadOnChange is on, which is the default for WebApplication.CreateBuilder / Host.CreateDefaultBuilder. The XML docs and the README do not mention this.

Failure scenario

The probe used a JSON file {"Cs":"pw=${secret:x}"}, expanded it to pw=hunter2, and then called cfg.Reload(). cfg["Cs"] became pw=${secret:x}. In production, an operator edits a log level in appsettings.json on a running service. From then on, IConfiguration readers and IOptionsMonitor-rebound options see the placeholder text as the credential. For example, a connection string read per use contains Password=${secret:sql/…}, and logins fail until the process restarts.

Recommendation

Expand through a dedicated configuration provider or source that wraps the others and re-expands on the change token. Alternatively, add an in-memory overlay provider last and re-run expansion on GetReloadToken(). At minimum, document that reload must be disabled for expanded keys.

Resolution

Unresolved.

Secrets-004 — Corrupt or wrong-length crypto fields escape Decrypt as ArgumentException: not audited, and breaks the cipher contract

Severity	Low
Priority	P3
Category	Spec & shared-contract adherence
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Crypto/AesGcmEnvelopeCipher.cs:141 (resolver: ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DefaultSecretResolver.cs:114)

Description

ISecretCipher.Decrypt documents SecretDecryptionException as its only failure. Decrypt catches only CryptographicException (:141, :146). AesGcm.Decrypt throws ArgumentException for a wrong-length nonce or tag, or for a ciphertext/plaintext length mismatch, for example a WrappedDek that is not 32 bytes. Rewrap already handles this (:191, "ArgumentException = a malformed … row"), but Decrypt does not. DefaultSecretResolver catches only SecretDecryptionException, so the decryption-failed audit event is skipped.

Failure scenario

A row is truncated by a buggy or hostile replication peer, or by a hand-edited DB. The probe used a row with Nonce = byte[5] or WrappedDek = byte[31], and Decrypt threw System.ArgumentException. ISecretResolver.GetAsync then throws a raw ArgumentException with no audit record, and callers that classify SecretDecryptionException misreport the cause.

Recommendation

Mirror Rewrap: catch ArgumentException in Decrypt and wrap it as SecretDecryptionException. Add a corrupt-length test.

Resolution

Unresolved.

Secrets-005 — Store-path rules are lexical: drive-relative paths pass "rooted", and \\?\ / UNC / 8.3 / symlink spellings bypass "under content root"

Severity	Low
Priority	P3
Category	Security & secret handling
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretsStorePathRules.cs:57 (rule 2: :90–:115)

Description

• Rule 1. It uses Path.IsPathRooted, which on Windows returns true for drive-relative C:secrets.db and for root-relative \secrets.db. Neither is fully qualified. C:secrets.db resolves against the process's current directory on C:, which for a service is C:\Windows\System32. That is exactly the incident this rule documents.
• Rule 2. It compares Path.GetFullPath strings. GetFullPath does not normalise \\?\ paths, does not expand 8.3 short names, and does not resolve symlinks or junctions. \\?\C:\app\secrets.db, \\localhost\C$\app\secrets.db, C:\PROGRA~1\App\secrets.db, or a path through a symlink into the content root (or a content root reached through a "current" symlink) all pass the check while sitting inside the content root.

Failure scenario

A Windows service with content root C:\Program Files\App is configured with SqlitePath = C:\PROGRA~1\App\data\secrets.db. Validation passes, the store lives inside the install directory, and the next deploy that renames that directory takes the store with it. Similarly, SqlitePath = C:secrets.db passes rule 1 and the store lands in System32.

Recommendation

• Rule 1: use Path.IsPathFullyQualified.
• Rule 2: reject \\?\, \\.\ and UNC spellings outright, or canonicalise both sides before comparing (GetFinalPathNameByHandle / FileSystemInfo.ResolveLinkTarget(returnFinalTarget: true) on the existing ancestor directory).

Resolution

Unresolved.

Secrets-006 — Resolver cache can re-cache a stale value after Invalidate, and expired plaintext is never evicted

Severity	Low
Priority	P3
Category	Concurrency & thread safety
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DefaultSecretResolver.cs:120 (also :92, :132)

Description

There are two problems:

1. Stale write-back. GetAsync reads the row (:99), decrypts it, and then unconditionally writes _cache[key] (:120). If a rotation or delete plus Invalidate (:132) lands between the read and the write, the old plaintext is re-inserted with a fresh full TTL. The Revision stored in the entry is never compared.
2. No eviction. Nothing ever removes expired entries. An entry is only overwritten by a later resolve of the same name. Plaintext for any secret resolved once, such as at startup expansion, stays in the dictionary for the life of the process. The class remarks say it "lives only briefly".

Failure scenario

1. A request thread misses the cache and loads revision 3. The operator rotates a compromised credential to revision 4, and the UI calls Invalidate. The request thread then stores revision 3, which is served for another 30 s, or for ResolveCacheTtl if longer.
2. A heap dump taken hours after startup contains every startup-resolved secret in _cache.

Recommendation

1. Use AddOrUpdate with a guard that refuses to replace a newer Revision, or keep a per-key invalidation generation counter that is checked before insert.
2. Remove the entry on an expired hit (TryRemove with the observed value), and/or sweep periodically.

Resolution

Unresolved.

Secrets-007 — KEK is re-resolved on every access: KekId and key are read non-atomically, the work is repeated per op, and key copies are never zeroed

Severity	Low
Priority	P3
Category	Security & secret handling
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Crypto/AesGcmEnvelopeCipher.cs:52 (also :53, :113, :119; ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/MasterKey/MasterKeyProviderBase.cs:35, :48, :60)

Description

MasterKeyProviderBase re-reads the source on every GetMasterKey() and, when no explicit id is configured, also on every KekId get. The source is the env var, the key file, or a DPAPI unprotect. This causes three problems:

• Non-atomic pair. Encrypt reads KekId (:52) and the key (:53) as two separate resolutions. A key file swapped between the two reads produces a row stamped with the old derived KEK id but wrapped under the new key. That row then fails Decrypt (id mismatch) and also fails Rewrap with the old key (tag mismatch). It is unrecoverable without hand-building a provider with the new key and the old id.
• Repeated work. Each encrypt or decrypt does two file reads, or two DPAPI unprotects.
• No zeroing. None of the returned 32-byte KEK arrays, or the intermediate base64/blob buffers, are ever zeroed. A copy of the KEK is left on the GC heap for every operation. This sits awkwardly with the class's careful DEK zeroing.

Failure scenario

An operator replaces the key file on a running node while a set or rotate is in flight, which the provider docs explicitly support ("a rotated file takes effect immediately"). One row ends up permanently undecryptable.

Recommendation

Have the provider return a single snapshot (key + id) per call. Use it once per cipher operation and zero it in finally. Optionally cache the snapshot keyed on file mtime.

Resolution

Unresolved.

Secrets-008 — Hub push sweep is not isolated per row, so one rejected row stops every later push

Severity	Low
Priority	P3
Category	Error handling & resilience
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/SqlServerSecretSyncService.cs:109

Description

The pull side isolates failures per row (SecretReplicationReconciler.ApplyAsync), precisely to stop a poison row from blocking a batch forever. The push loop (:109–:114) calls _hub.ApplyReplicatedAsync without isolation. The first failure aborts the sweep, and because the push set is recomputed in the same name order every interval, every name sorting after it is never pushed. The SQLite store accepts rows the hub rejects:

• SecretName has no length limit, but the hub column is NVARCHAR(450) (SqlServerSecretsSchema.cs:73).
• An explicit MasterKey:KekId has no length limit, but the hub column is NVARCHAR(256) (:82).

Failure scenario

In hub mode a node stores a secret with a 500-character name. The live publish fails and the failure is swallowed. Every sweep then fails with "String or binary data would be truncated" on that row, and every secret written on that node whose name sorts after it never reaches the hub or the other nodes.

Recommendation

Wrap each push in a try/catch with a per-row log, as the pull side does. Optionally enforce a maximum SecretName length (≤ 450) in the abstraction.

Resolution

Unresolved.

Secrets-009 — README wiring example uses a relative SqlitePath that the validator rejects; empty or unterminated tokens pass through as literals

Severity	Low
Priority	P3
Category	Documentation & XML docs
Status	Open
Location	ZB.MOM.WW.Secrets/README.md:83 (token: ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs:18)

Description

• The README's canonical appsettings.json sets "SqlitePath": "secrets.db", which SecretsOptionsValidator rejects at startup (relative path). A consumer copying the sample gets a boot failure.
• Separately, the expander says it is fail-closed rather than "leaving the placeholder in place". However, TokenPattern requires [^}]+, so ${secret:} and an unterminated ${secret:name are not matched. ExpandConfigurationAsync then leaves them verbatim. The probe confirmed that "pw=${secret:}" is returned unchanged.

Failure scenario

A typo such as Password=${secret:}, or a missing closing brace, boots successfully and uses the literal placeholder text as the credential, instead of failing at startup.

Recommendation

Change the README sample to an absolute path, or drop the key so the default applies. In the expander, throw when a value contains ${secret: but the regex does not consume every occurrence.

Resolution

Unresolved.

Secrets-010 — Akka replication actor is created only when ISecretStore is first resolved; a node that never resolves it never replicates

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.

Secrets-011 — HandleAnnounceTick publishes from a thread-pool continuation: announces carry no sender under an async store, and faults are silently dropped

Severity	Low
Priority	P3
Category	Concurrency & thread safety
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/SecretReplicationActor.cs:196

Description

HandleAnnounceTick (lines 194–211) calls _store.GetManifestAsync(...).ContinueWith(..., ExecuteSynchronously, TaskScheduler.Default) and calls _mediator.Tell(new Publish(...)) inside the continuation (line 202), with no explicit sender. When the task has not already completed, the continuation runs on a pool thread with no actor context. The implicit sender is then NoSender, and receiving peers see Sender == deadLetters.

HandleManifestAnnounce then captures peer = Sender (line 145). It sends its push rows and its SecretPullRequest to dead letters, so that half of the exchange is lost.

This is the same class of bug that the class's own comment (lines 216–225) says was fixed in ReconcileWithPeerAsync. The tick path was missed.

Separately, the continuation only checks IsCompletedSuccessfully. A faulted manifest read is never logged and its exception is never observed, so a node whose store cannot produce a manifest stops announcing and logs nothing.

Failure scenario

Verification: a throwaway probe project (not committed) subscribed a probe to the topic.

• With a synchronous store, the announce sender was akka://s/user/sync-node.
• With a store whose GetManifestAsync awaits a 5 ms delay, the sender was akka://s/deadLetters.

With two genuinely async stores, anti-entropy never converges in either direction; only the best-effort live push works. Today the production store is SqliteSecretStore, whose Microsoft.Data.Sqlite async calls complete synchronously, and the existing async-store test still passes through the peer's own announce. That is why this is latent. SecretReplicationActor.Props is public and accepts any ISecretStore.

Recommendation

Capture IActorRef self = Self in HandleAnnounceTick. Then either PipeTo(self) a private "manifest ready" message and publish it from the mailbox, or call _mediator.Tell(new Publish(...), self). Add an OnlyOnFaulted warning, as HandleRows already does. Extend the async-store test so that both nodes are async.

Resolution

Unresolved.

Secrets-012 — Akka anti-entropy replies are unchunked; a large store exceeds Akka.Remote's frame size and never converges

Severity	Low
Priority	P3
Category	Performance & resource management
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/SecretReplicationActor.cs:242

Description

ReconcileWithPeerAsync sends every row the peer is behind on as one SecretRowsMessage (line 242). HandlePullRequest answers an entire pull as one SecretRowsMessage (lines 184–186), and the pull itself is one SecretPullRequest listing every name (line 251).

SecretReplicationReconciler.ReconcileAsync deliberately chunks at 500 names, and its comment says this is because a cold node's pull set is the peer's entire inventory and an unbounded message "breaks on real backends". The Akka transport bypasses that batching.

The rows are STJ JSON with six base64 byte arrays, a kek_id, timestamps and names, so a row is a few hundred bytes. At the default Akka.Remote maximum-frame-size of 128000 bytes, that caps out at a few hundred rows.

Failure scenario

A node joins cold, or rejoins after its store was rebuilt, against a peer that holds a few hundred secrets. Every announce, the peer builds one oversized SecretRowsMessage. Akka.Remote discards it as an oversized payload. The same message is rebuilt on the next interval, so the cold node never receives any row. It recovers only for rows that are written again later.

Recommendation

Chunk the push and the pull-reply into messages of at most N rows (for example SecretReplicationReconciler.FetchBatchSize, or a smaller row-count constant). Optionally chunk SecretPullRequest too. Document the frame-size bound in clustered-secrets.md.

Resolution

Unresolved.

Secrets-013 — UI add/rotate/delete: if the audit write throws after the store write succeeded, the change is reported and audited as a failure and the resolver cache is not evicted

Severity	Medium
Priority	P2
Category	Correctness & logic bugs
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/SecretEditor.razor:138 (also ConfirmDeleteModal.razor:91)

Description

In SecretEditor.SubmitAsync, Store.UpsertAsync (line 138) and the success Audit.WriteAsync (lines 140–151) sit inside the same try. Its catch (Exception) (lines 155–163) treats any throw as a failed mutation:

• it sets _error = "Unable to add/rotate the secret.";
• it writes a Failure audit event;
• it leaves saved == false, so the ISecretCacheInvalidator.Invalidate and OnSaved block (lines 170–178) is skipped.

ConfirmDeleteModal.ConfirmAsync has the identical shape: DeleteAsync at line 91, audit at lines 93–104, catch at lines 108–114, and eviction skipped at lines 121–126. No UI test makes the audit writer throw; CapturingAuditWriter never fails.

Failure scenario

The audit sink (for example a SQL-backed IAuditWriter) is briefly unavailable. An operator rotates db/central to revoke a leaked credential.

• The new ciphertext is committed and, on a replicated host, already published.
• The UI says the rotation failed, and the audit trail records secret.rotate / Failure.
• The local resolver keeps serving the old plaintext for the rest of its cache TTL.

The operator may retry, creating a second revision, or believe the secret is unchanged. A delete behaves the same way: the secret is tombstoned, yet it is reported as not deleted and keeps resolving from cache.

Recommendation

Move the success audit out of the mutation try. Once the store write returns, set saved = true and invalidate the cache. Then write the success audit in its own best-effort try/catch, and log when it fails. Add bUnit tests with a throwing IAuditWriter for both components.

Resolution

Unresolved.

Secrets-014 — UI "Add secret" silently overwrites an existing (or tombstoned) secret, clobbers its metadata, and audits it as secret.add

Severity	Low
Priority	P3
Category	Correctness & logic bugs
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/SecretEditor.razor:115

Description

In add mode, name = new SecretName(_name) (line 115) is passed straight to Cipher.Encrypt and Store.UpsertAsync (lines 131–138). The store's upsert is ON CONFLICT(name) DO UPDATE, which overwrites the value and also clears any tombstone. Nothing checks whether the name already exists.

The rotate path was specifically changed to preserve the existing description and content type (lines 94–104), because the upsert overwrites them. Add-on-existing-name bypasses that protection: the description becomes null and the content type becomes the form default Text. The event is then audited as secret.add.

Failure scenario

An operator types an existing name, such as db/central, into "Add secret", perhaps by copy-paste from another environment. The live credential is replaced without any prompt, and its description and content type are reset. A ConnectionString secret becomes Text, and consumers that branch on content type may now mis-handle it. The audit says "add" rather than "rotate", which misleads anyone reviewing it.

Recommendation

In add mode, call Store.GetAsync(name) first. If a live row exists, refuse with "already exists — use Rotate". If a tombstone exists, require an explicit confirmation to restore.

Resolution

Unresolved.

Secrets-015 — Reveal shows the plaintext even when its audit write fails, and swallows the failure with no log

Severity	Low
Priority	P3
Category	Security & secret handling
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/RevealButton.razor:108

Description

RevealAsync decrypts and sets _revealed = true (lines 65 and 88) before writing the secret.reveal audit (lines 91–106). The audit is wrapped in a bare catch { } (lines 108–111) that neither logs nor hides the value.

Reveal is the most sensitive operation in this UI: the policy doc calls it "strictly more privileged" and audited. If the audit fails, the plaintext is still displayed and nothing records that it happened.

There is a second gap: if the component is disposed between the store read and the audit, _cts.Token (line 106) throws ObjectDisposedException, which the same bare catch swallows. That reveal was decrypted but never audited.

Failure scenario

The audit sink is down, or someone with Reveal rights deliberately makes it fail. A user with secrets-reveal reveals production credentials one after another. Every value is displayed and there is no audit row and no log line, so the "audited reveal" guarantee is silently fail-open.

Recommendation

Choose the policy explicitly. Either:

• fail closed: write the audit first, and show the value only if the write succeeds; or
• keep it fail-open, but log a Warning or Error naming the actor and the target (never the value) when the audit write fails.

Add a test for a throwing audit writer.

Resolution

Unresolved.

Secrets-016 — The public UI components enforce no authorization of their own; gating relies entirely on the host rendering them inside AuthorizeView

Severity	Low
Priority	P3
Category	Public API surface & compatibility
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/RevealButton.razor:50 (also SecretEditor.razor:108, ConfirmDeleteModal.razor:80)

Description

RevealButton, SecretEditor and ConfirmDeleteModal are public Razor components in ZB.MOM.WW.Secrets.Ui.Components. Their handlers (RevealAsync, SubmitAsync, ConfirmAsync) decrypt, write or tombstone without evaluating SecretsAuthorization.RevealPolicy or ManagePolicy. The gating exists only in SecretsList.razor, which wraps them in AuthorizeView (lines 64 and 67), and on SecretsPage ([Authorize(ManagePolicy)]).

Separately, the role matrix in AddSecretsAuthorization (SecretsAuthorization.cs:66-79) has no test. The bUnit tests use auth.SetPolicies(...), which bypasses the real policy definitions. So nothing pins the security-critical property that secrets-manager does not satisfy RevealPolicy.

Failure scenario

A host composes <RevealButton Name="..."/> into its own page, for example an equipment-config page under a lower-privilege policy, and does not wrap it in AuthorizeView Policy=RevealPolicy. Any user who can view that page can then reveal plaintext.

A future edit that adds ManageRole to the reveal policy would also pass every existing test.

Recommendation

Inject IAuthorizationService into each component and re-check the policy at the top of each mutating or revealing handler, failing closed. Add an AuthorizationOptions test that builds the two policies and evaluates each role against each policy.

Resolution

Unresolved.

Secrets-017 — secret set/rotate take the plaintext on argv; the host builder re-parses argv as configuration and can crash with the value in the error

Severity	Medium
Priority	P2
Category	Security & secret handling
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Program.cs:21 (values consumed at :57, :65)

Description

The headless verbs take the secret value as a positional argument: set <name> <value> and rotate <name> <value> (lines 57 and 65). There is no stdin or masked-prompt alternative.

The rewrap-all path deliberately refuses literal key material, "which would be echoed/logged" (lines 91–92). Secret values get no such protection: they land in shell history and are visible in the process list (ps, or Windows process command lines in EDR telemetry) to any local user for the duration of the run.

Host.CreateApplicationBuilder(args) (line 21) also feeds the same argv into the command-line configuration provider:

• A value containing = becomes a configuration key/value pair.
• A value starting with - and containing = throws an unhandled FormatException. That happens before the verb's catch is set up, and the exception message contains the whole argument.

Failure scenario

secret set db/pw "-p=Hunter2Secret", run from a throwaway probe (not committed), prints:

Unhandled exception. System.FormatException: The short switch '-p=Hunter2Secret' is not defined in the switch mappings.
   at Microsoft.Extensions.Configuration.CommandLine.CommandLineConfigurationProvider.Load()

The secret goes to stderr, which CI logs, deploy-script transcripts and the WinRM output capture all retain. The CLI exits with an unhandled crash rather than its usage code.

Even without the crash, every secret set leaves the plaintext in ~/.zsh_history, PSReadLine history, or the equivalent.

Recommendation

• Accept the value from stdin (- or --value-stdin) or a masked prompt when stdin is a TTY, and document that as the preferred form.
• Build the host with Host.CreateApplicationBuilder(new HostApplicationBuilderSettings { Args = [] }), or strip the verb arguments, so argv is never parsed as configuration.
• Wrap the host build so a configuration exception never echoes argv.

Resolution

Unresolved.

Secrets-018 — Bundle import aborts mid-run on one bad entry, leaving earlier rows applied but reporting only "Import failed"

Severity	Low
Priority	P3
Category	Error handling & resilience
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/BundleService.cs:160

Description

ImportAsync applies rows one at a time (lines 147–211), outside any transaction. If entry #i fails, the method throws InvalidOperationException and loses its running tally:

• a malformed entry, such as bad base64 or an unknown content type, throws at line 160;
• a SecretDecryptionException (a subtype of InvalidOperationException) from cipher.Rewrap (line 169), for example a tampered wrap, also throws.

Entries 0..i-1 have already been written with ApplyReplicatedAsync/UpsertAsync. BundleImportFlow catches the exception and prints only Import failed: … (BundleImportFlow.cs:129-133). It does not render a report, so the operator cannot tell that some rows were imported, or which.

Failure scenario

An operator imports a 40-row bundle whose entry #25 was hand-edited and corrupted. Rows 0–24 are written, which may include conflict winners that overwrote local values, and the console says "Import failed". The operator assumes nothing changed, fixes the bundle and re-imports with the per-conflict prompt. They are now asked about 25 "conflicts" that the first run created, with no record of which local values were overwritten.

Recommendation

Validate the whole bundle first: rehydrate every entry with ToRow, and check that every foreign-KEK row can be rewrapped, before any write. Alternatively, return a partial report: catch per entry, record the failure, and continue or stop, but always render what was written.

Resolution

Unresolved.

Secrets-019 — Hub-client cancellation surfaces as RpcException(Cancelled), so shutdown is recorded as a failed sweep and the failover reader rotates through every endpoint with a cancelled token

Severity	Low
Priority	P3
Category	Error handling & resilience
Status	Open
Location	ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretSyncService.cs:176 (also FailoverSecretsHubReader.cs:114, DependencyInjection/SecretsGrpcHubClientExtensions.cs:89)

Description

The hub channels are created with GrpcChannel.ForAddress(endpoint) and default options (SecretsGrpcHubClientExtensions.cs:89-90 and :115-117). That leaves GrpcChannelOptions.ThrowOperationCanceledOnCancellation at its default of false, so a call cancelled through its CancellationToken throws RpcException with StatusCode.Cancelled, not OperationCanceledException. This contradicts two pieces of code:

• GrpcSecretSyncService.SweepAndRecordAsync (line 176) only treats OperationCanceledException as a shutdown. A cancelled in-flight sweep falls through to the catch-all (lines 180–189), which increments ConsecutiveFailures, sets LastError, and logs the "Secret hub sync failed" warning.
• FailoverSecretsHubReader's remarks (lines 28–33) state that OperationCanceledException "is the host stopping — dialing another endpoint with a cancelled token would only delay shutdown" and propagates. In practice the catch (RpcException) (line 114) catches the cancellation, logs "did not answer (Cancelled); failing over" and tries every remaining endpoint with the same cancelled token.

Failure scenario

A site host is stopped while a sweep is in flight, which is likely on a cold start against a slow hub with CallDeadline = 30s. The log gets a spurious "failing over to …" warning per fallback endpoint plus a "sync failed" warning. Anything reading ISecretsHubSyncStatus in its last moments, such as a readiness probe or a shutdown health snapshot, reports a failure that did not happen.

Recommendation

Either:

• create the channels with new GrpcChannelOptions { ThrowOperationCanceledOnCancellation = true }; or
• in both places, treat RpcException { StatusCode: Cancelled } as cancellation when ct.IsCancellationRequested, rethrowing in the failover reader and returning quietly in the sweep.

Add a test that cancels mid-call against the in-process harness.

Resolution

Unresolved.