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 | 19 |

## 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 | 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._

### 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 | Open |
| 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**

_Unresolved._

### 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:

```text
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._