docs(secrets): record the P3 batch and #49 in findings, CHANGELOG 0.6.6 and the review index (scadaproj#45)
Secrets-004–009, 011, 012, 014–016, 018 and 019 marked Resolved with commit, fix and regression evidence; Secrets open findings 13 -> 0; code-reviews/README.md regenerated (--check passes). CHANGELOG 0.6.6 records the fixes, behaviour changes and the #49 follow-ups. Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
This commit is contained in:
Joseph Doherty
@@ -7,7 +7,8 @@ one of them.
|
||||
|
||||
## 0.6.6 — unreleased (NOT published to the feed)
|
||||
|
||||
Fixes six scadaproj#15 code-review findings (`code-reviews/Secrets/findings.md`).
|
||||
Fixes the scadaproj#15 code-review findings (`code-reviews/Secrets/findings.md`): six P2s (scadaproj#30–#35),
|
||||
the P3 batch (scadaproj#45: Secrets-004–009, 011, 012, 014–016, 018, 019) and the scadaproj#49 follow-ups.
|
||||
|
||||
### Fixed
|
||||
|
||||
@@ -51,6 +52,58 @@ Fixes six scadaproj#15 code-review findings (`code-reviews/Secrets/findings.md`)
|
||||
failure is logged as a warning (`ILogger<SecretEditor>` / `ILogger<ConfirmDeleteModal>`, when the host
|
||||
registers logging).
|
||||
|
||||
- **A malformed row fails as `SecretDecryptionException`** (scadaproj#45, Secrets-004). A wrong-length
|
||||
nonce, tag or wrapped DEK made `AesGcmEnvelopeCipher.Decrypt` throw a raw `ArgumentException`, which
|
||||
skipped the resolver's `decryption-failed` audit.
|
||||
- **One master-key reading per cipher operation, zeroed afterwards** (scadaproj#45, Secrets-007). The KEK
|
||||
id and the key used to be two separate reads of the source, so a key file replaced mid-`set` could stamp
|
||||
a row with the old key's id and wrap it under the new key — permanently undecryptable. New
|
||||
`MasterKeySnapshot` + default-implemented `IMasterKeyProvider.GetSnapshot()` (additive; existing
|
||||
implementations keep working, their key is copied, never zeroed in place); `MasterKeyProviderBase`
|
||||
resolves once and derives the id from that key. **Stored data is unchanged** — a golden row written by
|
||||
the 0.6.5 cipher is pinned by a test. `MasterKeyProviderBase.ResolveKeyBytes` must return a fresh
|
||||
buffer (it is zeroed after use); every shipped provider already did.
|
||||
- **Store-path rules compare locations, not spellings** (scadaproj#45, Secrets-005). Rooted now means
|
||||
fully qualified (Windows `C:secrets.db` / `\secrets.db` are rejected), and the under-content-root rule
|
||||
also resolves symlinks/junctions, `\\?\` / `\\.\` prefixes, loopback admin shares and 8.3 names.
|
||||
**Behaviour change:** a store that reaches the content root through a link — including macOS's
|
||||
`/var` → `/private/var` — now fails startup validation where it used to pass.
|
||||
- **Resolver cache** (scadaproj#45, Secrets-006): a resolve in flight across an `Invalidate` no longer
|
||||
re-caches the old plaintext for a full TTL, expired entries are evicted (on lookup and by a periodic
|
||||
sweep), and a non-positive TTL caches nothing. `DefaultSecretResolver` is now `IDisposable`.
|
||||
- **Hub push sweep is isolated per row** (scadaproj#45, Secrets-008): one row the hub refuses (a
|
||||
>450-char name) no longer stops every later name from being pushed; it is logged (EventId 4) and retried.
|
||||
- **Malformed `${secret:}` references fail closed** (scadaproj#45, Secrets-009): `${secret:}`, a blank
|
||||
name, an unterminated `${secret:name` or a nested marker throws `FormatException` naming the key
|
||||
(never the value) instead of passing the literal through as the credential. **Behaviour change** for a
|
||||
host whose configuration holds such a typo: it now fails at startup. The README sample no longer uses a
|
||||
relative `SqlitePath`.
|
||||
- **Akka announce carries a sender; anti-entropy is chunked** (scadaproj#45, Secrets-011/012). The
|
||||
periodic announce is published from the actor's mailbox (it lost its sender under a genuinely async
|
||||
store, so peers replied to deadLetters); a failed manifest read is logged. Pushed rows, pull replies and
|
||||
pull requests are split into ≤64 KB messages, so a cold node catches up past Akka.Remote's 128000-byte
|
||||
frame. The announce itself is not split (see `docs/operations/clustered-secrets.md`).
|
||||
- **UI** (scadaproj#45, Secrets-014/015/016): "Add secret" refuses a name that already exists (use
|
||||
Rotate) and restores a deleted one only on a second submit; Reveal **fails closed** — no value is shown
|
||||
unless its `secret.reveal` audit was written, and the failure is logged; `RevealButton`, `SecretEditor`
|
||||
and `ConfirmDeleteModal` re-check their own policy (`RevealPolicy` / `ManagePolicy`) and refuse without
|
||||
it, instead of relying on the host's `AuthorizeView`.
|
||||
- **Bundle import validates the whole bundle first** (scadaproj#45, Secrets-018): a malformed or tampered
|
||||
entry anywhere aborts with nothing written; a store fault mid-apply names the rows already written.
|
||||
- **Hub-call cancellation is cancellation** (scadaproj#45, Secrets-019): the gRPC hub channels set
|
||||
`ThrowOperationCanceledOnCancellation`, so a shutdown mid-sweep is no longer recorded in
|
||||
`ISecretsHubSyncStatus` as a failure, and the failover reader no longer dials every fallback with a
|
||||
cancelled token.
|
||||
- **The CLI's shipped `appsettings.json` no longer sets a relative `SqlitePath`** (scadaproj#49), which
|
||||
failed validation; the per-user default applies unless the working directory's `appsettings.json` or
|
||||
`Secrets__SqlitePath` says otherwise.
|
||||
|
||||
### Documentation
|
||||
|
||||
- scadaproj#49: the plain-`IConfigurationRoot` reload gap is documented, not fixed (README "Reload"); no
|
||||
family host is exposed. `clustered-secrets.md` notes that OtOpcUa's own `SecretReplicationStarter` is
|
||||
redundant once it adopts 0.6.6, and documents the Akka message-size bounds.
|
||||
|
||||
### Changed
|
||||
|
||||
- **CLI `set`/`rotate` take the value on stdin or a masked prompt** (scadaproj#35, Secrets-017).
|
||||
|
||||
@@ -4,7 +4,7 @@ Shared encrypted secret store + `${secret:}` resolution for the **ZB.MOM.WW SCAD
|
||||
|
||||
**Backlog / open work:** lives in Gitea issues on `dohertj2/scadaproj` (`tea issues list --repo dohertj2/scadaproj`) — currently #7 (ignitionoee router pins), #8 (Replicator.Grpc / `0.6.3` fate), #18 (migrator retry jitter). The history below is a record, not a status tracker; file new follow-ups as issues.
|
||||
|
||||
**`0.6.6` — built, NOT published (2026-11-09, scadaproj#30–#35):** six scadaproj#15 review fixes — see `CHANGELOG.md` and `../code-reviews/Secrets/findings.md` (Secrets-001/002/003/010/013/017).
|
||||
**`0.6.6` — built, NOT published (2026-11-09, scadaproj#30–#35, #45, #49):** every scadaproj#15 review finding resolved (Secrets-001–019; 0 open) plus the #49 follow-ups (CLI default `SqlitePath`, plain-root reload gap documented) — see `CHANGELOG.md` and `../code-reviews/Secrets/findings.md`. Behaviour changes worth knowing before adopting: store paths through symlinks into the content root now fail validation, malformed `${secret:}` tokens fail startup, UI Reveal fails closed on audit failure.
|
||||
|
||||
**`0.6.5` — built, NOT published (2026-11-09, scadaproj#26/#27):** `Microsoft.Data.Sqlite` 10.0.7 -> 10.0.13 (efcore#39009 pool fault) with `SQLitePCLRaw.lib.e_sqlite3` pin 2.1.12 -> 2.1.13; Windows test fixes. See `CHANGELOG.md`.
|
||||
|
||||
|
||||
+6
-20
@@ -40,7 +40,7 @@ code-reviews/
|
||||
|
||||
## Summary
|
||||
|
||||
10 of 10 libraries reviewed. 13 pending findings across all libraries.
|
||||
10 of 10 libraries reviewed. 0 pending findings across all libraries.
|
||||
|
||||
Coverage: Secrets, LocalDb, GalaxyRepository and Overview first reviewed, and Auth, Health,
|
||||
Theme, Audit, Configuration and Telemetry re-reviewed, on 2026-11-09 (#15). Each finding also
|
||||
@@ -51,8 +51,8 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low).
|
||||
| Critical | 0 |
|
||||
| High | 0 |
|
||||
| Medium | 0 |
|
||||
| Low | 13 |
|
||||
| **Total** | **13** |
|
||||
| Low | 0 |
|
||||
| **Total** | **0** |
|
||||
|
||||
## Library Status
|
||||
|
||||
@@ -65,7 +65,7 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low).
|
||||
| [Health](Health/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 7 |
|
||||
| [LocalDb](LocalDb/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 7 |
|
||||
| [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 5 |
|
||||
| [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/13 | 13 | 19 |
|
||||
| [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 19 |
|
||||
| [Telemetry](Telemetry/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 11 |
|
||||
| [Theme](Theme/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 8 |
|
||||
|
||||
@@ -88,20 +88,6 @@ _None open._
|
||||
|
||||
_None open._
|
||||
|
||||
### Low (13)
|
||||
### Low (0)
|
||||
|
||||
| ID | Library | Title |
|
||||
|----|---------|-------|
|
||||
| Secrets-004 | [Secrets](Secrets/findings.md) | Corrupt or wrong-length crypto fields escape `Decrypt` as `ArgumentException`: not audited, and breaks the cipher contract |
|
||||
| Secrets-005 | [Secrets](Secrets/findings.md) | Store-path rules are lexical: drive-relative paths pass "rooted", and `\\?\` / UNC / 8.3 / symlink spellings bypass "under content root" |
|
||||
| Secrets-006 | [Secrets](Secrets/findings.md) | Resolver cache can re-cache a stale value after `Invalidate`, and expired plaintext is never evicted |
|
||||
| Secrets-007 | [Secrets](Secrets/findings.md) | 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 |
|
||||
| Secrets-008 | [Secrets](Secrets/findings.md) | Hub push sweep is not isolated per row, so one rejected row stops every later push |
|
||||
| Secrets-009 | [Secrets](Secrets/findings.md) | README wiring example uses a relative `SqlitePath` that the validator rejects; empty or unterminated tokens pass through as literals |
|
||||
| Secrets-011 | [Secrets](Secrets/findings.md) | `HandleAnnounceTick` publishes from a thread-pool continuation: announces carry no sender under an async store, and faults are silently dropped |
|
||||
| Secrets-012 | [Secrets](Secrets/findings.md) | Akka anti-entropy replies are unchunked; a large store exceeds Akka.Remote's frame size and never converges |
|
||||
| Secrets-014 | [Secrets](Secrets/findings.md) | UI "Add secret" silently overwrites an existing (or tombstoned) secret, clobbers its metadata, and audits it as `secret.add` |
|
||||
| Secrets-015 | [Secrets](Secrets/findings.md) | Reveal shows the plaintext even when its audit write fails, and swallows the failure with no log |
|
||||
| Secrets-016 | [Secrets](Secrets/findings.md) | The public UI components enforce no authorization of their own; gating relies entirely on the host rendering them inside `AuthorizeView` |
|
||||
| Secrets-018 | [Secrets](Secrets/findings.md) | Bundle import aborts mid-run on one bad entry, leaving earlier rows applied but reporting only "Import failed" |
|
||||
| Secrets-019 | [Secrets](Secrets/findings.md) | 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 |
|
||||
_None open._
|
||||
|
||||
@@ -10,7 +10,7 @@
|
||||
| Last reviewed | 2026-11-09 |
|
||||
| Reviewer | Claude (scadaproj#15) |
|
||||
| Commit reviewed | `91fb2b4` |
|
||||
| Open findings | 13 |
|
||||
| Open findings | 0 |
|
||||
|
||||
## Summary
|
||||
|
||||
@@ -154,7 +154,7 @@ Resolved 2026-11-09 in the commit tagged `(scadaproj#32)` (Secrets `0.6.6`, unpu
|
||||
| Severity | Low |
|
||||
| Priority | P3 |
|
||||
| Category | Spec & shared-contract adherence |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
| 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**
|
||||
@@ -171,7 +171,7 @@ Mirror `Rewrap`: catch `ArgumentException` in `Decrypt` and wrap it as `SecretDe
|
||||
|
||||
**Resolution**
|
||||
|
||||
_Unresolved._
|
||||
Resolved 2026-11-09 in `34b1b20` (`(scadaproj#45)`, Secrets `0.6.6`, unpublished). `AesGcmEnvelopeCipher.Decrypt` now also catches the `ArgumentException` that `AesGcm` throws for a wrong-length nonce, tag or wrapped DEK and wraps it as `SecretDecryptionException` ("the row is malformed"), mirroring `Rewrap`; the resolver's `decryption-failed` audit therefore fires. Regression tests `Crypto/MalformedRowTests` (each of `Nonce`/`Tag`/`WrappedDek`/`WrapNonce`/`WrapTag` truncated, with an untouched-row positive control, plus `The_resolver_audits_a_malformed_row_as_decryption_failed`) all fail without the fix (raw `ArgumentException`).
|
||||
|
||||
### Secrets-005 — Store-path rules are lexical: drive-relative paths pass "rooted", and `\\?\` / UNC / 8.3 / symlink spellings bypass "under content root"
|
||||
|
||||
@@ -180,7 +180,7 @@ _Unresolved._
|
||||
| Severity | Low |
|
||||
| Priority | P3 |
|
||||
| Category | Security & secret handling |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretsStorePathRules.cs:57` (rule 2: `:90`–`:115`) |
|
||||
|
||||
**Description**
|
||||
@@ -201,7 +201,7 @@ A Windows service with content root `C:\Program Files\App` is configured with `S
|
||||
|
||||
**Resolution**
|
||||
|
||||
_Unresolved._
|
||||
Resolved 2026-11-09 in `57ef43e` (`(scadaproj#45)`, Secrets `0.6.6`, unpublished). Rule 1 uses `Path.IsPathFullyQualified`, so Windows `C:secrets.db` and `\secrets.db` are rejected. Rule 2 refuses a store if EITHER its spelled location (full path; on Windows `\\?\` / `\\.\` / `\\?\UNC\` prefixes stripped and a loopback admin share — `localhost`, `127.0.0.1`, `::1`, this machine's name — mapped to its drive) OR its resolved location (symlinks/junctions on the existing part of the path followed to their final target, recursively; 8.3 names expanded with `GetLongPathNameW` on Windows) is under the likewise-resolved content root. Keeping the spelled check means a store spelled *through* the content root stays refused even when a link inside it currently leads elsewhere (the next deploy's tree has no such link). Regression tests `Configuration/SecretsStorePathSpellingTests`: the two symlink cases (store through an alias of the root; root reached through a `current` link) fail without the fix on macOS; the Windows-only cases (drive/root-relative, device prefixes, loopback shares, 8.3) are `SkippableFact`s, run and passed on windev (Windows; Secrets.Tests 280 passed / 2 skipped there, symlink and 8.3 cases not skipped). The fix also caught the CLI test rig keeping its store under the CLI's working directory (= content root), which the lexical rule missed on macOS only through the `/var` → `/private/var` link (`e50bce7`). Residual, not addressed: a UNC path through a non-loopback or non-admin share that happens to point at the content root is compared as written — resolving it needs server share configuration. Note mxaccessgw carries its own copy of these rules (`GatewayConfigPathRules`) with the same lexical gaps.
|
||||
|
||||
### Secrets-006 — Resolver cache can re-cache a stale value after `Invalidate`, and expired plaintext is never evicted
|
||||
|
||||
@@ -210,7 +210,7 @@ _Unresolved._
|
||||
| Severity | Low |
|
||||
| Priority | P3 |
|
||||
| Category | Concurrency & thread safety |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DefaultSecretResolver.cs:120` (also `:92`, `:132`) |
|
||||
|
||||
**Description**
|
||||
@@ -233,7 +233,7 @@ There are two problems:
|
||||
|
||||
**Resolution**
|
||||
|
||||
_Unresolved._
|
||||
Resolved 2026-11-09 in `532a88c` (`(scadaproj#45)`, Secrets `0.6.6`, unpublished). `DefaultSecretResolver` keeps a per-key invalidation generation; `Invalidate` bumps it before evicting, and a resolve that read the store under an older generation withdraws its insert. Expired entries are removed on lookup (only the exact observed entry) and by a periodic `TimeProvider` sweep every TTL (at least 1 s), started on the first insert; a non-positive TTL now caches nothing. The resolver is `IDisposable` (stops the sweep timer; the DI container disposes it). Regression tests `DefaultSecretResolverCacheTests` (in-flight resolve gated across a rotate+`Invalidate`; expired entry dropped on lookup; sweep with a manual timer; zero TTL) all fail with the fix's four changes reverted. The `Revision` guard was not added: the generation check covers the `Invalidate` path the write side uses.
|
||||
|
||||
### 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
|
||||
|
||||
@@ -242,7 +242,7 @@ _Unresolved._
|
||||
| Severity | Low |
|
||||
| Priority | P3 |
|
||||
| Category | Security & secret handling |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
| 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**
|
||||
@@ -262,7 +262,7 @@ Have the provider return a single snapshot (key + id) per call. Use it once per
|
||||
|
||||
**Resolution**
|
||||
|
||||
_Unresolved._
|
||||
Resolved 2026-11-09 in `34b1b20` (`(scadaproj#45)`, Secrets `0.6.6`, unpublished). New `MasterKeySnapshot` (Abstractions; key + `KekId` from one reading, key zeroed on `Dispose`) and a default-implemented `IMasterKeyProvider.GetSnapshot()` (reads `KekId`, then copies `GetMasterKey()` — so a provider handing out a cached array is never zeroed). `MasterKeyProviderBase` overrides it to resolve the source once and derive the id from that same key; `KekId` zeroes the key it hashed, a wrong-length key is zeroed before the throw, and the file provider zeroes its raw base64 buffer. `Encrypt`/`Decrypt`/`Rewrap` take exactly one snapshot per KEK and dispose it. Additive API; `ResolveKeyBytes` is now documented to return a fresh caller-owned buffer (every in-repo provider already did). The optional mtime-keyed cache was not added — one read per operation instead of two. On-disk format unchanged: `MasterKeySnapshotTests.A_row_written_by_the_0_6_5_cipher_still_decrypts` decrypts a golden row produced by the pre-change cipher with a `FileMasterKeyProvider`. Regression tests (`Encrypt_stamps_the_id_of_the_key_that_actually_wrapped_the_row` with a provider whose key changes after its first read, `Decrypt_and_rewrap_resolve_each_kek_once`, `The_cipher_zeroes_every_key_snapshot_it_used`) fail with the old cipher.
|
||||
|
||||
### Secrets-008 — Hub push sweep is not isolated per row, so one rejected row stops every later push
|
||||
|
||||
@@ -271,7 +271,7 @@ _Unresolved._
|
||||
| Severity | Low |
|
||||
| Priority | P3 |
|
||||
| Category | Error handling & resilience |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/SqlServerSecretSyncService.cs:109` |
|
||||
|
||||
**Description**
|
||||
@@ -290,7 +290,7 @@ Wrap each push in a try/catch with a per-row log, as the pull side does. Optiona
|
||||
|
||||
**Resolution**
|
||||
|
||||
_Unresolved._
|
||||
Resolved 2026-11-09 in `ea487d0` (`(scadaproj#45)`, Secrets `0.6.6`, unpublished). `SqlServerSecretSyncService.SweepAsync` wraps each push in try/catch (cancellation still propagates), logs `EventId 4` with the name only, and continues; the row is retried every sweep. Regression test `SqlServerSecretSyncServiceTests.Sweep_isolates_a_row_the_hub_rejects_so_later_names_still_push` (hub refuses `m/poison`; `z/after` must still reach it) fails without the fix. The optional `SecretName` length cap (≤ 450) was not added — it would be a validation change on the shared abstraction; a >450-char name now costs one warning per sweep instead of blocking every later name.
|
||||
|
||||
### Secrets-009 — README wiring example uses a relative `SqlitePath` that the validator rejects; empty or unterminated tokens pass through as literals
|
||||
|
||||
@@ -299,7 +299,7 @@ _Unresolved._
|
||||
| Severity | Low |
|
||||
| Priority | P3 |
|
||||
| Category | Documentation & XML docs |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
| Location | `ZB.MOM.WW.Secrets/README.md:83` (token: `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs:18`) |
|
||||
|
||||
**Description**
|
||||
@@ -318,7 +318,7 @@ Change the README sample to an absolute path, or drop the key so the default app
|
||||
|
||||
**Resolution**
|
||||
|
||||
_Unresolved._
|
||||
Resolved 2026-11-09 in `d8081ef` (`(scadaproj#45)`, Secrets `0.6.6`, unpublished). `SecretReferenceExpander.ExpandAsync` throws `FormatException` when the count of `${secret:` markers differs from the number of complete references or a reference name is blank (`${secret:}`, `${secret: }`, unterminated `${secret:name`, nested `${secret:a${secret:b}`); `ExpandConfigurationAsync` rethrows naming the key (never the value) and, since everything is resolved before anything is written, leaves the configuration untouched. The README sample drops `"SqlitePath": "secrets.db"` for the per-user default with a commented absolute example. Regression tests: the malformed-token theory + `ExpandConfigurationAsync_names_the_key_holding_a_malformed_reference_and_writes_nothing` fail without the fix; `ShippedCliSettingsTests.The_readme_wiring_sample_passes_store_path_validation` validates the README block. (The CLI's own shipped `appsettings.json` had the same relative path — fixed under scadaproj#49, `8235ee6`.)
|
||||
|
||||
|
||||
### Secrets-010 — Akka replication actor is created only when `ISecretStore` is first resolved; a node that never resolves it never replicates
|
||||
@@ -370,7 +370,7 @@ Resolved 2026-11-09 in the commit tagged `(scadaproj#33)` (Secrets `0.6.6`, unpu
|
||||
| Severity | Low |
|
||||
| Priority | P3 |
|
||||
| Category | Concurrency & thread safety |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/SecretReplicationActor.cs:196` |
|
||||
|
||||
**Description**
|
||||
@@ -397,7 +397,7 @@ Capture `IActorRef self = Self` in `HandleAnnounceTick`. Then either `PipeTo(sel
|
||||
|
||||
**Resolution**
|
||||
|
||||
_Unresolved._
|
||||
Resolved 2026-11-09 in `049cc50` (`(scadaproj#45)`, Secrets `0.6.6`, unpublished). `HandleAnnounceTick` captures `Self` and `PipeTo`s the manifest read back as a private `ManifestReady`/`ManifestFailed` message; the announce is published from the mailbox with `Self` as sender, and a failed read logs a warning. Regression tests `AnnounceAndChunkingTests.An_announce_from_a_genuinely_async_store_carries_the_actor_as_sender` (TestKit probe subscribed to the topic; sender must be the actor, not deadLetters) and `A_failed_manifest_read_is_logged_not_dropped` fail without the fix.
|
||||
|
||||
|
||||
### Secrets-012 — Akka anti-entropy replies are unchunked; a large store exceeds Akka.Remote's frame size and never converges
|
||||
@@ -407,7 +407,7 @@ _Unresolved._
|
||||
| Severity | Low |
|
||||
| Priority | P3 |
|
||||
| Category | Performance & resource management |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/SecretReplicationActor.cs:242` |
|
||||
|
||||
**Description**
|
||||
@@ -428,7 +428,7 @@ Chunk the push and the pull-reply into messages of at most N rows (for example `
|
||||
|
||||
**Resolution**
|
||||
|
||||
_Unresolved._
|
||||
Resolved 2026-11-09 in `049cc50` (`(scadaproj#45)`, Secrets `0.6.6`, unpublished). New internal `SecretReplicationChunks` splits the exchange's push rows, the pull reply and the `SecretPullRequest` into messages of at most 64 000 bytes measured with the wire encoding (half the default 128000-byte frame). A single row over budget is still sent alone. The manifest announce is deliberately not split — a receiver computes its push set against the whole remote manifest — so it caps out around a thousand secrets at the default frame; both bounds are documented in `docs/operations/clustered-secrets.md`. Regression tests `AnnounceAndChunkingTests` (cold-peer push of 300 rows, a 300-row pull reply, a 600-name pull request: each must arrive in several messages each ≤ the budget per `SecretReplicationSerializer.ToBinary`) fail without the fix.
|
||||
|
||||
|
||||
### 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
|
||||
@@ -475,7 +475,7 @@ Resolved 2026-11-09 in the commit tagged `(scadaproj#34)` (Secrets `0.6.6`, unpu
|
||||
| Severity | Low |
|
||||
| Priority | P3 |
|
||||
| Category | Correctness & logic bugs |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/SecretEditor.razor:115` |
|
||||
|
||||
**Description**
|
||||
@@ -494,7 +494,7 @@ In add mode, call `Store.GetAsync(name)` first. If a live row exists, refuse wit
|
||||
|
||||
**Resolution**
|
||||
|
||||
_Unresolved._
|
||||
Resolved 2026-11-09 in `ff7dcf4` (`(scadaproj#45)`, Secrets `0.6.6`, unpublished). `SecretEditor` in add mode reads the name first: a live row is refused ("already exists. Use Rotate…") with no write and no audit; a tombstoned row is restored only when the operator submits the same name again after the warning (audited `secret.add`). Check-then-write is not atomic (documented in the component). Regression tests `SecretEditorAddExistingTests` (live refusal incl. a second click; tombstone needs a second submit) fail without the fix; a new name still adds in one submit.
|
||||
|
||||
|
||||
### Secrets-015 — Reveal shows the plaintext even when its audit write fails, and swallows the failure with no log
|
||||
@@ -504,7 +504,7 @@ _Unresolved._
|
||||
| Severity | Low |
|
||||
| Priority | P3 |
|
||||
| Category | Security & secret handling |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/RevealButton.razor:108` |
|
||||
|
||||
**Description**
|
||||
@@ -529,7 +529,7 @@ Add a test for a throwing audit writer.
|
||||
|
||||
**Resolution**
|
||||
|
||||
_Unresolved._
|
||||
Resolved 2026-11-09 in `93692fb` (`(scadaproj#45)`, Secrets `0.6.6`, unpublished). Policy chosen: **fail closed** — `RevealButton` writes the `secret.reveal` Success audit before showing anything and shows "Reveal is unavailable: the audit record could not be written" if it throws; the failure is logged as a warning with actor and name (never the value). Unlike add/rotate/delete (Secrets-013) nothing has happened yet that a failed audit would misreport, so withholding costs only a retry; flip to fail-open-with-log if operators prefer availability. The cancellation token is captured up front, so a mid-reveal dispose can no longer hide in a swallowed `ObjectDisposedException`. Regression test `RevealAuditFailureTests.A_reveal_whose_audit_fails_shows_nothing_and_logs_it` fails without the fix.
|
||||
|
||||
|
||||
### Secrets-016 — The public UI components enforce no authorization of their own; gating relies entirely on the host rendering them inside `AuthorizeView`
|
||||
@@ -539,7 +539,7 @@ _Unresolved._
|
||||
| Severity | Low |
|
||||
| Priority | P3 |
|
||||
| Category | Public API surface & compatibility |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/RevealButton.razor:50` (also `SecretEditor.razor:108`, `ConfirmDeleteModal.razor:80`) |
|
||||
|
||||
**Description**
|
||||
@@ -560,7 +560,7 @@ Inject `IAuthorizationService` into each component and re-check the policy at th
|
||||
|
||||
**Resolution**
|
||||
|
||||
_Unresolved._
|
||||
Resolved 2026-11-09 in `c9e5176` (`(scadaproj#45)`, Secrets `0.6.6`, unpublished). New internal `SecretsComponentAuthorization`: `RevealButton` (RevealPolicy), `SecretEditor` and `ConfirmDeleteModal` (ManagePolicy) evaluate their policy through `IAuthorizationService` at the top of the handler, using the cascaded `AuthenticationState` (or `AuthenticationStateProvider`), and fail closed when either is missing or the policy fails. Regression tests `ComponentAuthorizationTests`: a ManagePolicy-only user cannot reveal through a bare `RevealButton`, users without ManagePolicy cannot add or delete (all fail without the fix), and the REAL `AddSecretsAuthorization` policies are evaluated per role (`secrets-manager` → manage only; `secrets-reveal`/`Administrator` → both; other/none → neither; unauthenticated → neither) — mutating the reveal policy to include `secrets-manager` fails it.
|
||||
|
||||
|
||||
### 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
|
||||
@@ -614,7 +614,7 @@ Resolved 2026-11-09 in the commit tagged `(scadaproj#35)` (Secrets `0.6.6`, unpu
|
||||
| Severity | Low |
|
||||
| Priority | P3 |
|
||||
| Category | Error handling & resilience |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/BundleService.cs:160` |
|
||||
|
||||
**Description**
|
||||
@@ -635,7 +635,7 @@ Validate the whole bundle first: rehydrate every entry with `ToRow`, and check t
|
||||
|
||||
**Resolution**
|
||||
|
||||
_Unresolved._
|
||||
Resolved 2026-11-09 in `8f7fa30` (`(scadaproj#45)`, Secrets `0.6.6`, unpublished). `BundleService.ImportAsync` is two-phase: it rehydrates every entry and re-wraps every foreign-KEK row first, so a malformed or tampered entry anywhere fails with "Nothing was imported" and the store untouched; then it applies. A store fault in the apply phase (no multi-row transaction) reports the row it stopped at and every row already written, which `BundleImportFlow` prints. Regression tests in `BundleServiceTests` (tampered foreign-KEK entry after a good one; malformed entry after a good one; store fault mid-import names `svc/a` as already written) fail without the fix.
|
||||
|
||||
|
||||
### 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
|
||||
@@ -645,7 +645,7 @@ _Unresolved._
|
||||
| Severity | Low |
|
||||
| Priority | P3 |
|
||||
| Category | Error handling & resilience |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretSyncService.cs:176` (also `FailoverSecretsHubReader.cs:114`, `DependencyInjection/SecretsGrpcHubClientExtensions.cs:89`) |
|
||||
|
||||
**Description**
|
||||
@@ -668,4 +668,4 @@ Add a test that cancels mid-call against the in-process harness.
|
||||
|
||||
**Resolution**
|
||||
|
||||
_Unresolved._
|
||||
Resolved 2026-11-09 in `e5099aa` (`(scadaproj#45)`, Secrets `0.6.6`, unpublished). Both recommendations: the primary and fallback hub channels are created with `ThrowOperationCanceledOnCancellation = true`; and defensively `FailoverSecretsHubReader` rethrows `RpcException(Cancelled)` as `OperationCanceledException` when the caller's token is cancelled (no rotation), and `GrpcSecretSyncService` treats it as shutdown rather than a failure in `ISecretsHubSyncStatus`. A server-side `Cancelled` with the token live still fails over. Regression tests `HubCancellationTests` (the registered channel against a TCP listener that never answers must throw `OperationCanceledException`; failover does not dial the fallback; the sweep records no failure) fail without the fix.
|
||||
|
||||
Reference in New Issue
Block a user