From 7da303d7bb5886c8c3b02f5f099e1afc8cbcf76e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 17 May 2026 05:42:52 -0400 Subject: [PATCH] =?UTF-8?q?fix(configuration-database):=20resolve=20Config?= =?UTF-8?q?urationDatabase-012=20=E2=80=94=20store=20inbound-API=20keys=20?= =?UTF-8?q?as=20HMAC-SHA256=20hashes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Inbound-API bearer credentials are no longer persisted in plaintext. ApiKey now holds a KeyHash (peppered HMAC-SHA256); the key is shown once at creation and only its hash is stored. Lookup and validation hash the presented candidate. Cross-module: Commons (ApiKey, ApiKeyHasher), ConfigurationDatabase (mapping + HashApiKeyValue migration), InboundAPI (ApiKeyValidator), ManagementService (key creation), CentralUI (ApiKeys.razor). Existing keys must be re-issued. --- .../ConfigurationDatabase/findings.md | 92 +- .../Components/Pages/Admin/ApiKeys.razor | 4 +- .../Entities/InboundApi/ApiKey.cs | 50 +- .../Types/InboundApi/ApiKeyHasher.cs | 92 ++ .../Configurations/InboundApiConfiguration.cs | 9 +- ...20260517073000_HashApiKeyValue.Designer.cs | 1350 +++++++++++++++++ .../20260517073000_HashApiKeyValue.cs | 77 + .../ScadaLinkDbContextModelSnapshot.cs | 8 +- .../Repositories/InboundApiRepository.cs | 15 +- src/ScadaLink.InboundAPI/ApiKeyValidator.cs | 39 +- src/ScadaLink.InboundAPI/InboundApiOptions.cs | 14 + .../ServiceCollectionExtensions.cs | 14 + .../ManagementActor.cs | 35 +- .../Entities/ApiKeyTests.cs | 49 + .../Types/ApiKeyHasherTests.cs | 84 + .../SchemaConfigurationTests.cs | 26 + .../ApiKeyHashValidationTests.cs | 96 ++ .../ApiKeyCreationTests.cs | 121 ++ 18 files changed, 2113 insertions(+), 62 deletions(-) create mode 100644 src/ScadaLink.Commons/Types/InboundApi/ApiKeyHasher.cs create mode 100644 src/ScadaLink.ConfigurationDatabase/Migrations/20260517073000_HashApiKeyValue.Designer.cs create mode 100644 src/ScadaLink.ConfigurationDatabase/Migrations/20260517073000_HashApiKeyValue.cs create mode 100644 tests/ScadaLink.Commons.Tests/Entities/ApiKeyTests.cs create mode 100644 tests/ScadaLink.Commons.Tests/Types/ApiKeyHasherTests.cs create mode 100644 tests/ScadaLink.InboundAPI.Tests/ApiKeyHashValidationTests.cs create mode 100644 tests/ScadaLink.ManagementService.Tests/ApiKeyCreationTests.cs diff --git a/code-reviews/ConfigurationDatabase/findings.md b/code-reviews/ConfigurationDatabase/findings.md index 01e4ac8..d2f94ec 100644 --- a/code-reviews/ConfigurationDatabase/findings.md +++ b/code-reviews/ConfigurationDatabase/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-17 | | Reviewer | claude-agent | | Commit reviewed | `39d737e` | -| Open findings | 1 | +| Open findings | 0 | ## Summary @@ -599,7 +599,7 @@ Regression: `Constructor_NullContext_Throws` tests were added for all four affec |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ConfigurationDatabase/Configurations/InboundApiConfiguration.cs:17-19` | **Description** @@ -636,42 +636,66 @@ key value redacted. **Resolution** -_Open — requires a cross-module design decision that cannot be made within this -module's editable scope._ +Resolved 2026-05-17 (commit ``) — approved cross-module change spanning +Commons + ConfigurationDatabase + InboundAPI + ManagementService. The inbound-API +bearer credential is now persisted as a deterministic HMAC-SHA256 keyed hash, never +as plaintext, so a configuration-database dump no longer yields usable API keys +while the constant-time by-value lookup still works. -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. +Root cause confirmed against source: `ApiKey.KeyValue` was a plaintext +`nvarchar(500)` column with a unique index, holding the live `X-API-Key` credential. -The recommended fix — store a deterministic salted/peppered hash instead of the -plaintext — cannot be completed within this module alone, for three concrete reasons: +Design implemented — **deterministic keyed hash** (the recommendation's first option): -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. +- **Hash scheme.** HMAC-SHA256 over the UTF-8 key bytes, keyed with a server-side + *pepper*; the digest is stored Base64-encoded. No per-row random salt — an API key + is already a high-entropy random token, and a random salt would break the + deterministic by-value lookup the authentication path relies on. The pepper instead + binds every hash to the deployment. Implemented as `IApiKeyHasher` / `ApiKeyHasher` + in `ScadaLink.Commons` (`Types/InboundApi/ApiKeyHasher.cs`); the constructor + rejects a missing or weak (`< 16`-char) pepper with `ArgumentException` — fail-fast. +- **Where the pepper lives.** `InboundApiOptions.ApiKeyPepper`, a component-owned + Options class already bound from the `ScadaLink:InboundApi` configuration section + (Options pattern); it is never hard-coded. `AddInboundAPI` registers `IApiKeyHasher` + via a factory that reads the bound options, so a missing/weak pepper fails the + deployment fast rather than degrading silently. (Operators must supply the pepper + via configuration / a secret store; `appsettings*.json` is intentionally left + without a value.) +- **Commons.** `ApiKey` drops the persisted `KeyValue` property and gains a persisted + `KeyHash` — the plaintext is never a field on the entity. `ApiKey.FromHash(name, + keyHash)` creates an entity from a precomputed hash (the production create-path); + the existing `ApiKey(name, keyValue)` constructor is retained as a convenience that + hashes its input with the unpeppered default hasher. +- **ConfigurationDatabase.** EF mapping maps `KeyHash` (`nvarchar(256)`, required, + unique index `IX_ApiKeys_KeyHash`); migration `20260517073000_HashApiKeyValue` + drops `KeyValue`/`IX_ApiKeys_KeyValue`, adds `KeyHash`, and deletes existing + `ApiKeys` rows (a one-way hash cannot recover plaintext). `GetApiKeyByValueAsync` + now hashes the supplied value and looks up by `KeyHash`. +- **InboundAPI.** `ApiKeyValidator` takes an `IApiKeyHasher`; `FindKeyConstantTime` + hashes the presented candidate and compares hashes via + `CryptographicOperations.FixedTimeEquals` — the comparison stays constant-time. +- **ManagementService.** `HandleCreateApiKey` generates a random 32-byte key, stores + only its peppered hash, and returns the plaintext to the caller exactly once; + `HandleListApiKeys` no longer exposes the hash (returns id/name/enabled only). -**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**. +TDD regression tests (written first, confirmed failing, then passing): +`ApiKeyHasherTests` + `ApiKeyTests` (Commons — determinism, pepper fail-fast, no +plaintext property), `ApiKeyHashValidationTests` (InboundAPI — auth succeeds via +hash, wrong key / wrong-pepper fail), `ApiKeyCreationTests` (ManagementService — only +the hash is persisted, plaintext returned once), and `SchemaConfigurationTests` +additions (ConfigurationDatabase — `KeyHash` mapped/uniquely-indexed, no `KeyValue` +column). `dotnet build` of all five touched modules is clean and their test suites +are green. + +**Operational consequence: every existing API key must be re-issued.** A hash is +one-way, so the `HashApiKeyValue` migration deletes existing key rows; after applying +it, operators must create new keys (CLI / Management API / Central UI), distribute +the one-time plaintext to callers, and re-approve the new keys on their methods. + +Follow-up (outside this task's editable scope): record the hash scheme in +`docs/requirements/Component-ConfigurationDatabase.md` and the Inbound API design +doc, and update the Central UI API-keys page, which previously displayed a masked +`KeyValue`. ### ConfigurationDatabase-013 — Secret-column encryption silently falls back to an ephemeral (throwaway) key diff --git a/src/ScadaLink.CentralUI/Components/Pages/Admin/ApiKeys.razor b/src/ScadaLink.CentralUI/Components/Pages/Admin/ApiKeys.razor index 0a5bed0..cc13ea5 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Admin/ApiKeys.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Admin/ApiKeys.razor @@ -46,7 +46,7 @@ ID Name - Key Value + Key Hash Actions @@ -62,7 +62,7 @@ Disabled } - @MaskKeyValue(key.KeyValue) + @MaskKeyValue(key.KeyHash)