fix(configuration-database): resolve ConfigurationDatabase-013,014 — fail-fast on missing key ring, single converter local; ConfigurationDatabase-012 left open (cross-module design decision)

code-reviews/ConfigurationDatabase/findings.md

@@ -8,7 +8,7 @@
 | Last reviewed | 2026-05-17 |
 | Reviewer | claude-agent |
 | Commit reviewed | `39d737e` |
-| Open findings | 3 |
+| Open findings | 1 |
 
 ## Summary
 
@@ -636,7 +636,42 @@ key value redacted.
 
 **Resolution**
 
-_Unresolved._
+_Open — requires a cross-module design decision that cannot be made within this
+module's editable scope._
+
+Root cause confirmed against source. `ApiKey.KeyValue`
+(`src/ScadaLink.Commons/Entities/InboundApi/ApiKey.cs`) is mapped as a plaintext
+`nvarchar(500)` column with a unique index (`InboundApiConfiguration.cs:17-22`) and is
+the bearer credential for Inbound API requests. The CD-004 `EncryptedStringConverter`
+is correctly inapplicable here: it is non-deterministic, and the key is resolved
+by value.
+
+The recommended fix — store a deterministic salted/peppered hash instead of the
+plaintext — cannot be completed within this module alone, for three concrete reasons:
+
+1. **Commons entity change (out of scope).** Hashing changes the semantics of
+   `ApiKey.KeyValue`: after creation it can only ever hold the hash, never the live
+   key. The proper shape is a dedicated `KeyHash` field on the `ApiKey` entity (in
+   `ScadaLink.Commons`), which this task may not edit.
+2. **Cross-module consumer change (out of scope).** `ScadaLink.InboundAPI`'s
+   `ApiKeyValidator.FindKeyConstantTime` reads `key.KeyValue` and does a constant-time
+   byte comparison against the *raw presented secret*. If the column becomes a hash,
+   the validator must hash the presented candidate before comparing — an InboundAPI
+   change. The CLI (`SecurityCommands.cs`) and Management Service also create/read
+   `ApiKey` and would need the "plaintext shown once at creation" model.
+3. **Irreversible data migration / operational decision.** A hash is one-way, so a
+   migration cannot convert existing plaintext keys to hashes without the originals —
+   every existing API key must be re-issued. Whether and how to do that is an
+   operational/design call, not a mechanical fix.
+
+**Decision needed (for a follow-up spanning Commons + InboundAPI + ManagementService
++ CLI):** (a) add an `ApiKey.KeyHash` column and a per-key salt (or a system pepper);
+(b) hash on create and authenticate by hashing the presented key; (c) decide the
+hash algorithm (e.g. PBKDF2/Argon2 vs. a fast salted SHA-256 — note the constant-time
+lookup in `ApiKeyValidator` already mitigates timing, so a slow KDF is the safer
+choice if the key space is small); (d) plan the one-time re-issue of existing keys;
+(e) record the scheme in `docs/requirements/Component-ConfigurationDatabase.md` and the
+Inbound API design doc. Until that decision is made the finding stays **Open**.
 
 ### ConfigurationDatabase-013 — Secret-column encryption silently falls back to an ephemeral (throwaway) key
 
@@ -644,7 +679,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Error handling & resilience |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.ConfigurationDatabase/ScadaLinkDbContext.cs:107-124` |
 
 **Description**
@@ -685,7 +720,33 @@ through the EF-activator registration at all (e.g. register only the factory, or
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-17 (commit pending). Root cause confirmed against source:
+`ApplySecretColumnEncryption` resolved the provider as
+`_dataProtectionProvider ?? new EphemeralDataProtectionProvider()`, so a context built
+via the single-argument constructor would silently encrypt secret columns with a
+throwaway key and persist permanently undecryptable ciphertext with no error.
+
+Applied the recommendation — a combination of options (a) and (b). The silent
+ephemeral substitution was removed: a no-provider context now builds its model with an
+internal `SchemaOnlyDataProtector` that satisfies schema generation but throws a clear
+`InvalidOperationException` if it is ever asked to `Protect`/`Unprotect`. On top of
+that, `SaveChanges`/`SaveChangesAsync` were overridden with a `GuardSecretWritesHaveAKeyRing`
+check that fails fast — before any database round-trip — with a clear
+`InvalidOperationException` (naming the offending `Entity.Property`) whenever a
+no-provider context has a pending non-null write to one of the three secret-bearing
+columns. A custom `SecretAwareModelCacheKeyFactory` (registered via
+`OnConfiguring`/`ReplaceService`) folds the presence of a real provider into EF's model
+cache key, so a write-capable and a schema-only context can no longer share one cached
+model. Design-time `dotnet ef` tooling (which only emits schema) and the test fixture
+both keep working — `SqliteTestDbContext` now passes an explicit
+`EphemeralDataProtectionProvider`, making the test intent visible at the call site as
+recommendation (a) asks. Regression tests added in `EphemeralEncryptionFallbackTests.cs`:
+`SingleArgConstructor_WritingSecretColumn_FailsFast_DoesNotPersistThrowawayCiphertext`,
+`SingleArgConstructor_WritingNonSecretColumn_Succeeds`, and
+`ProviderConstructor_WritingSecretColumn_StillSucceeds`. The DI-hardening sub-point
+(register only the factory) was not adopted — `AddConfigurationDatabase` already
+overrides the EF-activator registration with a provider-bearing factory, and the
+fail-fast guard now closes the residual gap for any other resolution path.
 
 ### ConfigurationDatabase-014 — Redundant, inconsistent cast on one `HasConversion` call
 
@@ -693,7 +754,7 @@ _Unresolved._
 |--|--|
 | Severity | Low |
 | Category | Code organization & conventions |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.ConfigurationDatabase/ScadaLinkDbContext.cs:121-123` |
 
 **Description**
@@ -716,4 +777,18 @@ that type once and use it for all three.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-17 (commit pending). Partially re-triaged: the finding is correct
+that the inline fully-qualified cast was inconsistent and noisy, but its premise that
+the cast is *purely redundant* is wrong. `DatabaseConnectionDefinition.ConnectionString`
+is a non-nullable `string`; `EncryptedStringConverter` is `ValueConverter<string?,
+string?>`, so calling `HasConversion(converter)` with `converter` typed as
+`EncryptedStringConverter` on that property raises a real `CS8620` nullability
+error (verified by build). The cast to the non-generic `ValueConverter` base was
+suppressing that — load-bearing, not noise. Applied the recommendation's second
+option: the converter is now held once in a single `ValueConverter`-typed local
+(with an explanatory comment), and all three `HasConversion(converter)` calls read
+identically with no inline cast or fully-qualified name. Behaviour is unchanged, so
+no behavioural regression test is meaningful (cf. CD-005); a forward guard was added
+in `SchemaConfigurationTests.cs` —
+`SecretColumns_AllHaveEncryptedStringConverterApplied` (theory over all three secret
+columns) — asserting each column keeps an `EncryptedStringConverter`.