From 46cb6965ace58695aa76196956ea024b414c8bdf Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 28 May 2026 08:04:10 -0400 Subject: [PATCH] =?UTF-8?q?fix(security):=20close=20Theme=207=20=E2=80=94?= =?UTF-8?q?=208=20secrets=20/=20redaction=20/=20append-only=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security-sensitive batch, handled main-thread for careful judgment on secret-leak and pepper-bypass paths. Secret leak / pepper bypass: - CD-016 (pepper bypass): InboundApiRepository's GetApiKeyByValueAsync no longer hashes the candidate with the unpeppered ApiKeyHasher.Default — ctor takes a lazy Func accessor (lazy so test composition roots without a pepper still bring up the repository), and the DI registration wires sp.GetService() so the production peppered hasher matches the stored KeyHash. Regression test asserts positive (peppered roundtrip) AND negative (Default hasher misses the same key — proving the lookup uses the injected hasher). - MgmtSvc-020 (SMTP credential leak): UpdateSmtpConfig/ListSmtpConfigs now project through SmtpConfigPublicShape so the response payload and audit-row afterState never carry the Credentials field — only a HasCredentials bool. The SMTP password / OAuth2 client secret no longer leaves the Admin-only UpdateSmtpConfig boundary the caller already supplied it to. Redaction: - AuditLog-008 (test-fixture under-redact): new SafeDefaultAuditPayloadFilter (stateless singleton) does HTTP header redaction for the always-sensitive defaults (Authorization, X-Api-Key, Cookie, Set-Cookie). FallbackAuditWriter, CentralAuditWriter, and AuditLogIngestActor (both ingest paths) default to it instead of null — composition roots that bypass AddAuditLog can no longer write unredacted auth headers to the audit store. - NotifService-025 (over-mask): CredentialRedactor.Scrub now only masks the last colon-separated component (password / clientSecret) AND only if it's >= 12 chars (typical password heuristic). Short user names like "root" no longer become global redaction tokens that eat unrelated diagnostic text. The full packed string is always masked regardless of length. 3 new negative tests pin the no-over-mask contract. Audit-row correctness / fail-loud: - InboundAPI-025: Program.cs UseWhen predicate now excludes /api/audit, /api/management, /api/centralui, /api/script-analysis AND requires POST — the AuditWriteMiddleware no longer emits spurious ApiInbound rows for audit-log query/export endpoints (write-on-read recursion broken). - ESG-021: ApplyAuth now logs Warning (not silent) on empty AuthConfiguration for apikey/basic, unknown AuthType, and malformed Basic config. AuthConfiguration value NEVER logged. AuthType=none remains silent (documented unauthenticated sentinel). - Security-021: AddSecurity now logs a startup Warning when RequireHttpsCookie=false — an HTTP-only deployment that previously transmitted the cookie-embedded JWT silently in cleartext is now audible in the log. Defensive: - CD-021: SwitchOutPartitionAsync's monthBoundary format string now yyyy-MM-dd HH:mm:ss.fffffff (datetime2(7) precision) so a future sub-second / non-midnight boundary doesn't silently round to the wrong partition. Plus reconciled stale per-module Open-findings counters that had drifted from earlier sessions (AuditLog, CD, ESG, IAPI, MgmtSvc, NotifService, Security). Build clean; all affected test projects green (Host 208, ConfigDB 242, ESG 69, IAPI 151, MgmtSvc 100, NotifService 55, Security 85, AuditLog 247/248 — 1 pre-existing date-sensitive integration test flake on PartitionPurgeTests, unrelated). README regenerated: 46 open (was 54). --- code-reviews/AuditLog/findings.md | 17 +++- .../ConfigurationDatabase/findings.md | 30 ++++++- .../ExternalSystemGateway/findings.md | 15 +++- code-reviews/InboundAPI/findings.md | 16 +++- code-reviews/ManagementService/findings.md | 15 +++- code-reviews/NotificationService/findings.md | 18 ++++- code-reviews/README.md | 32 +++----- code-reviews/Security/findings.md | 16 +++- .../Central/AuditLogIngestActor.cs | 14 +++- .../Central/CentralAuditWriter.cs | 17 ++-- .../Payload/SafeDefaultAuditPayloadFilter.cs | 79 +++++++++++++++++++ .../Site/FallbackAuditWriter.cs | 18 +++-- .../Repositories/AuditLogRepository.cs | 8 +- .../Repositories/InboundApiRepository.cs | 32 +++++++- .../ServiceCollectionExtensions.cs | 11 ++- .../ExternalSystemClient.cs | 45 ++++++++++- src/ScadaLink.Host/Program.cs | 17 +++- .../ManagementActor.cs | 36 ++++++++- .../CredentialRedactor.cs | 47 ++++++++--- .../ServiceCollectionExtensions.cs | 15 +++- .../InboundApiRepositoryTests.cs | 33 +++++++- .../CredentialRedactorTests.cs | 46 ++++++++++- 22 files changed, 500 insertions(+), 77 deletions(-) create mode 100644 src/ScadaLink.AuditLog/Payload/SafeDefaultAuditPayloadFilter.cs diff --git a/code-reviews/AuditLog/findings.md b/code-reviews/AuditLog/findings.md index de0c30ac..e7ab7f8c 100644 --- a/code-reviews/AuditLog/findings.md +++ b/code-reviews/AuditLog/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 2 | +| Open findings | 1 | ## Summary @@ -411,9 +411,22 @@ resolution instead. |--|--| | Severity | Low | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.AuditLog/Site/FallbackAuditWriter.cs:51-77`, `src/ScadaLink.AuditLog/Central/CentralAuditWriter.cs:77-104`, `src/ScadaLink.AuditLog/Central/AuditLogIngestActor.cs:125,155` | +**Resolution (2026-05-28):** New `SafeDefaultAuditPayloadFilter` in +`src/ScadaLink.AuditLog/Payload/` — a stateless singleton that performs HTTP +header redaction for the hard-coded sensitive defaults (Authorization, +X-Api-Key, Cookie, Set-Cookie). The three writer-chain sites +(`FallbackAuditWriter`, `CentralAuditWriter`, `AuditLogIngestActor` — +both the audit-only and cached-telemetry paths) now default to +`SafeDefaultAuditPayloadFilter.Instance` instead of null when no filter is +injected, so a test fixture (or any composition root that bypasses +`AddAuditLog`) never persists those headers verbatim. The real +`DefaultAuditPayloadFilter` (truncation + body / SQL-param redaction + +per-target overrides) is wired by `AddAuditLog` and takes precedence in +production. + **Description** `FallbackAuditWriter`, `CentralAuditWriter`, and `AuditLogIngestActor` all accept an diff --git a/code-reviews/ConfigurationDatabase/findings.md b/code-reviews/ConfigurationDatabase/findings.md index d0e6626f..25de57f0 100644 --- a/code-reviews/ConfigurationDatabase/findings.md +++ b/code-reviews/ConfigurationDatabase/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 3 | +| Open findings | 1 | ## Summary @@ -946,9 +946,22 @@ throws and exactly one row lands. |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ConfigurationDatabase/Repositories/InboundApiRepository.cs:35-39` | +**Resolution (2026-05-28):** Took option (a) — `InboundApiRepository` ctor now +accepts `Func? hasherAccessor = null` (deferred resolution to +sidestep startup-time pepper-validation in test composition roots that don't +configure one). `GetApiKeyByValueAsync` calls `_hasherAccessor()` so the +hash matches the production peppered digest. `AddConfigurationDatabase` +registers the repository with a factory wiring +`() => sp.GetService() ?? ApiKeyHasher.Default` — the +peppered hasher is used when available, falling back to Default only when +none is registered (legacy / test composition). Regression test +`CD016_GetApiKeyByValue_UsesInjectedPepperedHasher_NotDefault` asserts +positively (peppered roundtrip) and negatively (Default hasher misses the +key entirely, proving the lookup uses the injected hasher). + **Description** `GetApiKeyByValueAsync` resolves an API key by its presented plaintext value by hashing @@ -1191,9 +1204,20 @@ converter on the column) so the defence at the read site is no longer required. |--|--| | Severity | Low | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs:192-338` | +**Resolution (2026-05-28):** Took the targeted (1) part of the recommendation — +the `monthBoundary` format string is now `"yyyy-MM-dd HH:mm:ss.fffffff"` +matching `datetime2(7)` precision, so a future sub-second or non-midnight +boundary won't silently round to the wrong partition. The staging table name +is still interpolated (T-SQL DDL identifiers can't be parameterised) but +remains internally constructed as `$"AuditLog_Staging_{Guid.NewGuid():N}"` +so SQL injection is structurally impossible. The larger +`sp_executesql`-with-typed-parameter migration was scoped to a future +follow-up since the current shape is fully controlled and the precision- +mismatch hazard was the only practical concern. + **Description** `SwitchOutPartitionAsync` builds two large SQL batches via interpolated strings diff --git a/code-reviews/ExternalSystemGateway/findings.md b/code-reviews/ExternalSystemGateway/findings.md index 88d008ba..9c105f06 100644 --- a/code-reviews/ExternalSystemGateway/findings.md +++ b/code-reviews/ExternalSystemGateway/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 4 | +| Open findings | 1 | ## Summary @@ -1180,9 +1180,20 @@ that happens to encode a number (already correctly returns `string`). |--|--| | Severity | Low | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs:385-415` | +**Resolution (2026-05-28):** `ApplyAuth` is now an instance method that uses +the existing `_logger`. Three previously-silent fail-open paths now emit a +`LogWarning` so an operator debugging a recurring 401 sees the cause inside +ScadaLink: (1) empty `AuthConfiguration` for `AuthType=apikey`/`basic`, +(2) unknown `AuthType` (anything except `apikey`/`basic`/`none`), +(3) malformed Basic config (no `:` separator). The `AuthConfiguration` +value is NEVER included in the log message. `AuthType="none"` remains +silent — it's the documented sentinel for intentionally-unauthenticated +systems. Behaviour is otherwise unchanged: the request still goes out +(never block on auth-config issues), the failure mode is just now visible. + **Description** `ApplyAuth` has three fail-open paths that all result in an HTTP request being sent diff --git a/code-reviews/InboundAPI/findings.md b/code-reviews/InboundAPI/findings.md index 6bcd456a..5a88a805 100644 --- a/code-reviews/InboundAPI/findings.md +++ b/code-reviews/InboundAPI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 2 | +| Open findings | 1 | ## Summary @@ -1233,9 +1233,21 @@ immediate change required; this is a watch-list item. |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Host/Program.cs:183-185`; consumers: `src/ScadaLink.ManagementService/AuditEndpoints.cs:93-94`; emitter: `src/ScadaLink.InboundAPI/Middleware/AuditWriteMiddleware.cs:175-252` | +**Resolution (2026-05-28):** Took the defensive path-exclusion in +`Program.cs` (Option 1 from the recommendation). The `UseWhen` predicate +now excludes the four known `/api/*` sub-trees that belong to other modules +(`/api/audit`, `/api/management`, `/api/centralui`, `/api/script-analysis`) +AND additionally requires `POST` — the inbound API method route is the +only POST under `/api/`, so future GET-y additions under any of those +sub-trees still survive the gate. The endpoint-filter refactor option +(move the audit emission into an `IEndpointFilter` co-located with +`MapInboundAPI`) was rejected for batch scope — touches more test fixtures +and the path-predicate is fragile only against future POST additions on +the excluded prefixes, which would be noisy in code review. + **Description** `Program.cs` wires the audit middleware as diff --git a/code-reviews/ManagementService/findings.md b/code-reviews/ManagementService/findings.md index 7ec93b14..fa01b01d 100644 --- a/code-reviews/ManagementService/findings.md +++ b/code-reviews/ManagementService/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 3 (1 Deferred — see ManagementService-012) | +| Open findings | 1 (1 Deferred — see ManagementService-012) | ## Summary @@ -927,9 +927,20 @@ mixed-set intersected. |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ManagementService/ManagementActor.cs:1136`–`:1153` | +**Resolution (2026-05-28):** Added `SmtpConfigPublicShape` projection that +returns every non-secret field plus a `HasCredentials` bool — never the +`Credentials` field itself. Both `HandleListSmtpConfigs` (broader read +access via OperationalAuditRoles) and `HandleUpdateSmtpConfig` (Admin-only +write) now project through it. The audit-row `afterState` and the response +payload both carry the credential-free shape, so the SMTP password / OAuth2 +client secret never leaves the `UpdateSmtpConfig` boundary that the caller +already supplied them to. ManagementService 100/100 tests still pass. +Follow-up to tag `SmtpConfiguration.Credentials` with `[JsonIgnore]` in +Commons remains useful belt-and-braces but is out of scope here. + **Description** `HandleUpdateSmtpConfig` reads the existing `SmtpConfiguration` entity, applies the diff --git a/code-reviews/NotificationService/findings.md b/code-reviews/NotificationService/findings.md index fa9f0bd8..1bdfadd8 100644 --- a/code-reviews/NotificationService/findings.md +++ b/code-reviews/NotificationService/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 5 | +| Open findings | 1 | ## Summary @@ -813,9 +813,23 @@ After NS-019 is decided: |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.NotificationService/CredentialRedactor.cs:34-48` | +**Resolution (2026-05-28):** Tightened the policy per the recommendation — +only the LAST colon-separated component (password in Basic / clientSecret +in OAuth2) is considered a secret, AND only when it's plausibly secret-shaped +(>= `MinSecretLength` = 12 chars). The full packed string is always masked +regardless of length (its exact appearance can only come from the +credential itself). A short user name like `root` (4 chars) or a sender +alias like `smtp` (4 chars) no longer becomes a global redaction token +that eats unrelated diagnostic text. 3 new negative tests added +(`Scrub_ShortUserName_IsNotMaskedOutsidePackedString`, +`Scrub_TenantId_IsNotMaskedOutsidePackedString`, +`Scrub_FullPackedCredential_IsAlwaysMaskedRegardlessOfLength`); 1 +existing test bumped its inline password length from 10→16 chars to stay +above the new threshold. + **Description** ```csharp diff --git a/code-reviews/README.md b/code-reviews/README.md index a98e4721..020aab51 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -41,31 +41,31 @@ module file and counted in **Total**. |----------|---------------| | Critical | 0 | | High | 0 | -| Medium | 19 | -| Low | 35 | -| **Total** | **54** | +| Medium | 16 | +| Low | 30 | +| **Total** | **46** | ## Module Status | Module | Last reviewed | Commit | Open (C/H/M/L) | Open | Total | |--------|---------------|--------|----------------|------|-------| -| [AuditLog](AuditLog/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/1 | 2 | 11 | +| [AuditLog](AuditLog/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/0 | 1 | 11 | | [CLI](CLI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 23 | | [CentralUI](CentralUI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/3 | 3 | 33 | | [ClusterInfrastructure](ClusterInfrastructure/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/3 | 3 | 14 | | [Commons](Commons/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/4 | 4 | 23 | | [Communication](Communication/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/1 | 1 | 22 | -| [ConfigurationDatabase](ConfigurationDatabase/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/2 | 3 | 24 | +| [ConfigurationDatabase](ConfigurationDatabase/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/1 | 1 | 24 | | [DataConnectionLayer](DataConnectionLayer/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 22 | | [DeploymentManager](DeploymentManager/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/3 | 3 | 24 | -| [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/1 | 2 | 23 | +| [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/0 | 1 | 23 | | [HealthMonitoring](HealthMonitoring/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 23 | | [Host](Host/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/3 | 4 | 22 | -| [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/1 | 2 | 25 | -| [ManagementService](ManagementService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/0 | 2 | 23 | +| [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/1 | 1 | 25 | +| [ManagementService](ManagementService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/0 | 1 | 23 | | [NotificationOutbox](NotificationOutbox/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/1 | 1 | 10 | -| [NotificationService](NotificationService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/1 | 2 | 25 | -| [Security](Security/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/1 | 1 | 21 | +| [NotificationService](NotificationService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/0 | 1 | 25 | +| [Security](Security/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 21 | | [SiteCallAudit](SiteCallAudit/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/1 | 3 | 6 | | [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 23 | | [SiteRuntime](SiteRuntime/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/0 | 2 | 26 | @@ -88,16 +88,13 @@ _None open._ _None open._ -### Medium (19) +### Medium (16) | ID | Module | Title | |----|--------|-------| | AuditLog-001 | [AuditLog](AuditLog/findings.md) | Combined-telemetry transport is plumbed end-to-end but never invoked in production | -| ConfigurationDatabase-016 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | `InboundApiRepository.GetApiKeyByValueAsync` hashes the candidate with the unpeppered `ApiKeyHasher.Default` | | ExternalSystemGateway-020 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | `JsonElementToParameterValue` silently downcasts non-Int64 JSON numbers to `double`, losing precision for `decimal` SQL parameters on retry | | Host-016 | [Host](Host/findings.md) | Site `CentralContactPoints` second entry targets the site's own remoting port | -| InboundAPI-025 | [InboundAPI](InboundAPI/findings.md) | `AuditWriteMiddleware` runs against the entire `/api/*` branch — emits spurious `ApiInbound` audit rows for `/api/audit/query` and `/api/audit/export` | -| ManagementService-020 | [ManagementService](ManagementService/findings.md) | UpdateSmtpConfig returns and audits the SMTP Credentials field verbatim | | ManagementService-021 | [ManagementService](ManagementService/findings.md) | Transport bundle handlers have zero test coverage | | NotificationService-024 | [NotificationService](NotificationService/findings.md) | No test affirms the central-only invariant; the orphaned-path tests give a false coverage signal | | SiteCallAudit-001 | [SiteCallAudit](SiteCallAudit/findings.md) | SupervisorStrategy override is dead code; XML claims Resume that is not enforced | @@ -112,11 +109,10 @@ _None open._ | TemplateEngine-020 | [TemplateEngine](TemplateEngine/findings.md) | `Create*` audit entries are written with `EntityId = "0"` before `SaveChangesAsync` populates the real key | | Transport-010 | [Transport](Transport/findings.md) | Critical Overwrite + cross-cutting paths uncovered by tests | -### Low (35) +### Low (30) | ID | Module | Title | |----|--------|-------| -| AuditLog-008 | [AuditLog](AuditLog/findings.md) | Test composition roots that omit `IAuditPayloadFilter` silently pass UNREDACTED payloads through the writer chain | | CLI-020 | [CLI](CLI/findings.md) | `bundle export` success-envelope parse is unguarded | | CLI-022 | [CLI](CLI/findings.md) | `CommandTreeTests` excludes the two new command groups | | CentralUI-029 | [CentralUI](CentralUI/findings.md) | `ConfigurationAuditLog` uses `JS.InvokeAsync("eval", ...)` instead of a dedicated JS module | @@ -130,12 +126,10 @@ _None open._ | Commons-020 | [Commons](Commons/findings.md) | Transport types and new Audit-message types have no unit tests in `ScadaLink.Commons.Tests` | | Commons-023 | [Commons](Commons/findings.md) | Trailing-optional `SourceNode` on positional records mixes additive evolution patterns | | Communication-020 | [Communication](Communication/findings.md) | `SiteAddressCacheLoaded` carries mutable `Dictionary`/`List` types | -| ConfigurationDatabase-021 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | `SwitchOutPartitionAsync` interpolates `monthBoundary` / staging table name into raw SQL | | ConfigurationDatabase-024 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | Missing test coverage for SPLIT-RANGE failure-continuation and production-shape rowversion delete | | DeploymentManager-021 | [DeploymentManager](DeploymentManager/findings.md) | `ResolveSiteIdentifierAsync` silently substitutes the DB id when the site row is missing | | DeploymentManager-022 | [DeploymentManager](DeploymentManager/findings.md) | `Pending` and `InProgress` are written back-to-back with no intervening work | | DeploymentManager-024 | [DeploymentManager](DeploymentManager/findings.md) | Test probe actors hold mutable static state across tests | -| ExternalSystemGateway-021 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | `ApplyAuth` silently sends an unauthenticated request on unknown `AuthType`, empty `AuthConfiguration`, or malformed Basic config | | HealthMonitoring-021 | [HealthMonitoring](HealthMonitoring/findings.md) | `CentralSiteId = "central"` reserved constant silently collides with a real site named "central" | | HealthMonitoring-022 | [HealthMonitoring](HealthMonitoring/findings.md) | `CentralHealthReportLoopTests` uses real-time `PeriodicTimer` + `Task.Delay`; flake-prone on slow CI | | Host-018 | [Host](Host/findings.md) | Shipped per-role configs omit `NodeOptions.NodeName`, leaving `SourceNode` null | @@ -143,8 +137,6 @@ _None open._ | Host-021 | [Host](Host/findings.md) | Microsoft `Logging:LogLevel` section in `appsettings.json` is dead config under Serilog | | InboundAPI-023 | [InboundAPI](InboundAPI/findings.md) | `EndpointExtensions.HandleInboundApiRequest` composition wiring has no test coverage | | NotificationOutbox-008 | [NotificationOutbox](NotificationOutbox/findings.md) | `FallbackMaxRetries` / `FallbackRetryDelay` path is unreachable in production AND untested | -| NotificationService-025 | [NotificationService](NotificationService/findings.md) | `CredentialRedactor` over-masks: any 4-character credential component is masked anywhere it appears, including unrelated log text | -| Security-021 | [Security](Security/findings.md) | `RequireHttpsCookie=false` dev opt-out has no warning path — an HTTP production deployment silently transmits the JWT bearer credential in cleartext | | SiteCallAudit-006 | [SiteCallAudit](SiteCallAudit/findings.md) | Stuck-only paging test does not exercise the multi-page boundary with an interleaved non-stuck row at the cursor | | SiteEventLogging-018 | [SiteEventLogging](SiteEventLogging/findings.md) | `FailedWriteCount` is exposed but never consumed by Health Monitoring | | SiteEventLogging-023 | [SiteEventLogging](SiteEventLogging/findings.md) | Concurrent-stress test uses a non-volatile `stop` flag | diff --git a/code-reviews/Security/findings.md b/code-reviews/Security/findings.md index 1428ff56..56b80fda 100644 --- a/code-reviews/Security/findings.md +++ b/code-reviews/Security/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 1 (Security-021); 1 deferred (Security-008) | +| Open findings | 0 (1 deferred — Security-008) | ## Summary @@ -971,9 +971,21 @@ every required `SecurityOptions` field is enforced at startup, not at first use. |--|--| | Severity | Low | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Security/SecurityOptions.cs:100-108`; `src/ScadaLink.Security/ServiceCollectionExtensions.cs:54-59` | +**Resolution (2026-05-28):** Added `ILoggerFactory` to the cookie-options +`Configure` callback in `AddSecurity` so an explicit `RequireHttpsCookie=false` +opt-out now emits a startup `LogWarning` ("auth cookie SecurePolicy is +SameAsRequest. The cookie-embedded JWT will be transmitted in cleartext +over plain HTTP. This setting is intended for local dev only — set +SecurityOptions:RequireHttpsCookie=true in production."). Default is still +`true`, so production deployments unchanged. The "fail startup when +RequireHttpsCookie=false AND ASPNETCORE_ENVIRONMENT=Production" hard-stop +option was not implemented (the dev Docker cluster intentionally runs with +the flag false, and the env-var name varies across deploy mechanisms); +the warning is the right ergonomic floor. + **Description** The Security-002 fix added `RequireHttpsCookie` (default `true`) so the auth cookie's diff --git a/src/ScadaLink.AuditLog/Central/AuditLogIngestActor.cs b/src/ScadaLink.AuditLog/Central/AuditLogIngestActor.cs index dc909bf1..ab26174c 100644 --- a/src/ScadaLink.AuditLog/Central/AuditLogIngestActor.cs +++ b/src/ScadaLink.AuditLog/Central/AuditLogIngestActor.cs @@ -164,8 +164,12 @@ public class AuditLogIngestActor : ReceiveActor // is a silent no-op. // Filter BEFORE the IngestedAtUtc stamp so the redacted // copy carries the central-side ingest timestamp. Filter - // is contract-bound to never throw; null = pass-through. - var filtered = filter?.Apply(evt) ?? evt; + // is contract-bound to never throw. AuditLog-008: a null + // filter (test composition root, no IAuditPayloadFilter + // registered) now falls back to the SafeDefault rather than + // pass-through, so HTTP header redaction always runs. + var safeFilter = filter ?? Payload.SafeDefaultAuditPayloadFilter.Instance; + var filtered = safeFilter.Apply(evt); var ingested = filtered with { IngestedAtUtc = nowUtc }; await repository.InsertIfNotExistsAsync(ingested).ConfigureAwait(false); accepted.Add(evt.EventId); @@ -239,8 +243,10 @@ public class AuditLogIngestActor : ReceiveActor // Filter the audit half BEFORE the dual-write — only the // AuditLog row's payload columns are filterable; SiteCalls // carries operational state only (status, retry count) and - // is left untouched. - var filteredAudit = filter?.Apply(entry.Audit) ?? entry.Audit; + // is left untouched. AuditLog-008: null filter falls back + // to SafeDefault so header redaction always runs. + var safeFilter = filter ?? Payload.SafeDefaultAuditPayloadFilter.Instance; + var filteredAudit = safeFilter.Apply(entry.Audit); var auditStamped = filteredAudit with { IngestedAtUtc = ingestedAt }; var siteCallStamped = entry.SiteCall with { IngestedAtUtc = ingestedAt }; diff --git a/src/ScadaLink.AuditLog/Central/CentralAuditWriter.cs b/src/ScadaLink.AuditLog/Central/CentralAuditWriter.cs index 7dec9b38..0fde32c0 100644 --- a/src/ScadaLink.AuditLog/Central/CentralAuditWriter.cs +++ b/src/ScadaLink.AuditLog/Central/CentralAuditWriter.cs @@ -41,7 +41,7 @@ public sealed class CentralAuditWriter : ICentralAuditWriter { private readonly IServiceProvider _services; private readonly ILogger _logger; - private readonly IAuditPayloadFilter? _filter; + private readonly IAuditPayloadFilter _filter; private readonly ICentralAuditWriteFailureCounter _failureCounter; private readonly INodeIdentityProvider? _nodeIdentity; @@ -80,7 +80,12 @@ public sealed class CentralAuditWriter : ICentralAuditWriter { _services = services ?? throw new ArgumentNullException(nameof(services)); _logger = logger ?? throw new ArgumentNullException(nameof(logger)); - _filter = filter; + // AuditLog-008: never default to null — over-redact instead. + // SafeDefaultAuditPayloadFilter applies HTTP header redaction with + // hard-coded sensitive defaults so a composition root that omits the + // real filter still scrubs Authorization / X-Api-Key / Cookie / + // Set-Cookie before persistence. + _filter = filter ?? Payload.SafeDefaultAuditPayloadFilter.Instance; _failureCounter = failureCounter ?? new NoOpCentralAuditWriteFailureCounter(); _nodeIdentity = nodeIdentity; } @@ -99,9 +104,11 @@ public sealed class CentralAuditWriter : ICentralAuditWriter try { // Filter BEFORE stamping IngestedAtUtc + handing to the repo. The - // filter contract is "never throws"; the null-coalesce keeps the - // M4 test composition roots (no filter passed) working unchanged. - var filtered = _filter?.Apply(evt) ?? evt; + // filter contract is "never throws". AuditLog-008: _filter is now + // non-null (SafeDefaultAuditPayloadFilter fallback) so header + // redaction always runs even in composition roots that omit the + // real filter. + var filtered = _filter.Apply(evt); // SourceNode-stamping (Task 12): caller-provided value wins // (supports any future direct-write callsite that already has its diff --git a/src/ScadaLink.AuditLog/Payload/SafeDefaultAuditPayloadFilter.cs b/src/ScadaLink.AuditLog/Payload/SafeDefaultAuditPayloadFilter.cs new file mode 100644 index 00000000..d685166e --- /dev/null +++ b/src/ScadaLink.AuditLog/Payload/SafeDefaultAuditPayloadFilter.cs @@ -0,0 +1,79 @@ +using System.Text.RegularExpressions; +using ScadaLink.Commons.Entities.Audit; + +namespace ScadaLink.AuditLog.Payload; + +/// +/// AuditLog-008: minimal always-safe fallback filter used by the writer chain +/// when no is injected (test composition +/// roots, future composition roots that bypass AddAuditLog). Performs +/// HTTP header redaction for the always-sensitive defaults +/// (Authorization, X-Api-Key, Cookie, Set-Cookie) so a fixture that wires a +/// real never persists those headers +/// in cleartext. Does NOT perform body-regex redaction, SQL-parameter +/// redaction, or truncation — those stages need +/// with live options. The contract is: +/// over-redact safely, never throw, never miss a header that's on the +/// default sensitive list. +/// +public sealed class SafeDefaultAuditPayloadFilter : IAuditPayloadFilter +{ + /// Singleton instance — the filter is stateless and side-effect-free. + public static SafeDefaultAuditPayloadFilter Instance { get; } = new SafeDefaultAuditPayloadFilter(); + + private static readonly string[] DefaultHeaderRedactList = + { + "Authorization", + "X-Api-Key", + "Cookie", + "Set-Cookie", + }; + + private static readonly Regex HeaderRegex = new( + @"(?[A-Za-z][A-Za-z0-9\-_]*)\s*:\s*(?[^\r\n]*)", + RegexOptions.Compiled | RegexOptions.IgnoreCase); + + private SafeDefaultAuditPayloadFilter() { } + + /// + public AuditEvent Apply(AuditEvent rawEvent) + { + ArgumentNullException.ThrowIfNull(rawEvent); + try + { + return rawEvent with + { + RequestSummary = RedactHeaders(rawEvent.RequestSummary), + ResponseSummary = RedactHeaders(rawEvent.ResponseSummary), + }; + } + catch + { + // Over-redact: drop both summaries entirely so a malformed parse + // path never leaks the original. The contract is "never throw." + return rawEvent with + { + RequestSummary = "[redacted by SafeDefaultAuditPayloadFilter]", + ResponseSummary = "[redacted by SafeDefaultAuditPayloadFilter]", + }; + } + } + + private static string? RedactHeaders(string? summary) + { + if (string.IsNullOrEmpty(summary)) return summary; + + return HeaderRegex.Replace(summary, m => + { + var name = m.Groups["name"].Value; + foreach (var sensitive in DefaultHeaderRedactList) + { + if (string.Equals(name, sensitive, StringComparison.OrdinalIgnoreCase)) + { + return $"{name}: [REDACTED]"; + } + } + return m.Value; + }); + } +} diff --git a/src/ScadaLink.AuditLog/Site/FallbackAuditWriter.cs b/src/ScadaLink.AuditLog/Site/FallbackAuditWriter.cs index 321d6374..fc53d557 100644 --- a/src/ScadaLink.AuditLog/Site/FallbackAuditWriter.cs +++ b/src/ScadaLink.AuditLog/Site/FallbackAuditWriter.cs @@ -31,7 +31,7 @@ public sealed class FallbackAuditWriter : IAuditWriter private readonly RingBufferFallback _ring; private readonly IAuditWriteFailureCounter _failureCounter; private readonly ILogger _logger; - private readonly IAuditPayloadFilter? _filter; + private readonly IAuditPayloadFilter _filter; private readonly SemaphoreSlim _drainGate = new(1, 1); /// @@ -60,7 +60,14 @@ public sealed class FallbackAuditWriter : IAuditWriter _ring = ring ?? throw new ArgumentNullException(nameof(ring)); _failureCounter = failureCounter ?? throw new ArgumentNullException(nameof(failureCounter)); _logger = logger ?? throw new ArgumentNullException(nameof(logger)); - _filter = filter; // null = no-op pass-through; see WriteAsync. + // AuditLog-008: never default to a null filter — over-redact instead. + // SafeDefaultAuditPayloadFilter.Instance performs HTTP header + // redaction with the hard-coded sensitive defaults (Authorization, + // X-Api-Key, Cookie, Set-Cookie) so a test composition root that + // doesn't bind the real options never persists those headers + // verbatim. The real DefaultAuditPayloadFilter (truncation + body / + // SQL-param redaction) is wired by AddAuditLog and takes precedence. + _filter = filter ?? Payload.SafeDefaultAuditPayloadFilter.Instance; } /// @@ -72,9 +79,10 @@ public sealed class FallbackAuditWriter : IAuditWriter // and (on failure) to the ring buffer — so a primary outage that // drains later still hands the SqliteAuditWriter a row that has // already been truncated and redacted. The filter contract is - // "MUST NOT throw"; the null-coalesce keeps test composition roots - // that don't wire a filter working unchanged. - var filtered = _filter?.Apply(evt) ?? evt; + // "MUST NOT throw". AuditLog-008: _filter is now non-null (defaults + // to SafeDefaultAuditPayloadFilter so header redaction is always + // applied even in composition roots that don't wire the real filter). + var filtered = _filter.Apply(evt); try { diff --git a/src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs b/src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs index 5793a324..676213d0 100644 --- a/src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs +++ b/src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs @@ -198,8 +198,12 @@ VALUES // ISO 8601 in UTC — SQL Server's datetime2 literal parser accepts this // unambiguously and the value is round-trip-safe across SET DATEFORMAT - // settings. - var monthBoundaryStr = monthBoundary.ToUniversalTime().ToString("yyyy-MM-dd HH:mm:ss"); + // settings. CD-021: use datetime2(7) precision (.fffffff) so a future + // non-midnight or sub-second boundary doesn't silently round to the + // wrong partition (today the migration only seeds at T00:00:00 exactly, + // but the format string is on the boundary value's own contract — match + // it to the column precision rather than to the current seed pattern). + var monthBoundaryStr = monthBoundary.ToUniversalTime().ToString("yyyy-MM-dd HH:mm:ss.fffffff"); // Two-statement batch: the first SELECT samples the per-partition row // count BEFORE the dance so we can report it back to the purge actor; diff --git a/src/ScadaLink.ConfigurationDatabase/Repositories/InboundApiRepository.cs b/src/ScadaLink.ConfigurationDatabase/Repositories/InboundApiRepository.cs index e4bafee5..37f7f00f 100644 --- a/src/ScadaLink.ConfigurationDatabase/Repositories/InboundApiRepository.cs +++ b/src/ScadaLink.ConfigurationDatabase/Repositories/InboundApiRepository.cs @@ -10,16 +10,38 @@ namespace ScadaLink.ConfigurationDatabase.Repositories; public class InboundApiRepository : IInboundApiRepository { private readonly ScadaLinkDbContext _context; + // CD-016: lazily resolved so the InboundAPI ApiKeyHasher factory (which throws + // when no pepper is configured) is only invoked if GetApiKeyByValueAsync is + // actually called — Central/Host startup composition roots that never call + // this method (the production ApiKeyValidator deliberately doesn't) get to + // bring InboundApiRepository up without forcing every test to wire a + // throw-away pepper into InboundApiOptions. + private readonly Func _hasherAccessor; private readonly ILogger _logger; /// /// Initializes a new instance of the InboundApiRepository class. /// /// The database context for accessing inbound API data. + /// + /// CD-016: factory that returns the API-key hasher used to digest a candidate + /// plaintext for the peppered lookup. + /// Resolution is deferred to first call so a composition root that doesn't + /// register (or whose factory would throw because + /// no pepper is configured) can still bring up the repository for callers that + /// don't touch the value-lookup path. Defaults to a factory returning + /// ; production wires + /// sp => sp.GetRequiredService<IApiKeyHasher>() via DI so the + /// lookup uses the same peppered digest as the production write path. + /// /// Optional logger instance for warnings and diagnostics. - public InboundApiRepository(ScadaLinkDbContext context, ILogger? logger = null) + public InboundApiRepository( + ScadaLinkDbContext context, + Func? hasherAccessor = null, + ILogger? logger = null) { _context = context ?? throw new ArgumentNullException(nameof(context)); + _hasherAccessor = hasherAccessor ?? (() => ApiKeyHasher.Default); _logger = logger ?? NullLogger.Instance; } @@ -34,7 +56,13 @@ public class InboundApiRepository : IInboundApiRepository /// public async Task GetApiKeyByValueAsync(string keyValue, CancellationToken cancellationToken = default) { - var keyHash = ApiKeyHasher.Default.Hash(keyValue); + // CD-016: hash the candidate with the DI-provided peppered hasher so this + // lookup matches keys whose stored KeyHash was produced by the production + // ApiKeyHasher(pepper). The pre-fix call to ApiKeyHasher.Default would + // silently return null for every real key on any peppered deployment. + // Resolution is deferred until this method is actually called so the + // pepper-validating factory doesn't fire during startup composition. + var keyHash = _hasherAccessor().Hash(keyValue); return await _context.Set().FirstOrDefaultAsync(k => k.KeyHash == keyHash, cancellationToken); } diff --git a/src/ScadaLink.ConfigurationDatabase/ServiceCollectionExtensions.cs b/src/ScadaLink.ConfigurationDatabase/ServiceCollectionExtensions.cs index 5f811f24..afa39a69 100644 --- a/src/ScadaLink.ConfigurationDatabase/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.ConfigurationDatabase/ServiceCollectionExtensions.cs @@ -1,6 +1,7 @@ using Microsoft.AspNetCore.DataProtection; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using ScadaLink.Commons.Interfaces; using ScadaLink.Commons.Interfaces.Repositories; using ScadaLink.Commons.Interfaces.Services; @@ -53,7 +54,15 @@ public static class ServiceCollectionExtensions services.AddScoped(); services.AddScoped(); services.AddScoped(); - services.AddScoped(); + // CD-016: factory registration wires a lazy accessor for IApiKeyHasher so + // the production peppered hasher is used (via DI) when GetApiKeyByValueAsync + // is actually called, but composition roots that never call it (and may + // not register IApiKeyHasher at all) still bring up the repository. + services.AddScoped(sp => new InboundApiRepository( + sp.GetRequiredService(), + hasherAccessor: () => sp.GetService() + ?? Commons.Types.InboundApi.ApiKeyHasher.Default, + logger: sp.GetService>())); services.AddScoped(); services.AddScoped(); services.AddScoped(); diff --git a/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs b/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs index a1d2579c..6336d7a7 100644 --- a/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs +++ b/src/ScadaLink.ExternalSystemGateway/ExternalSystemClient.cs @@ -464,12 +464,29 @@ public class ExternalSystemClient : IExternalSystemClient return url; } - private static void ApplyAuth(HttpRequestMessage request, ExternalSystemDefinition system) + private void ApplyAuth(HttpRequestMessage request, ExternalSystemDefinition system) { - if (string.IsNullOrEmpty(system.AuthConfiguration)) - return; + // ESG-021: distinguish "intentionally unauthenticated" (AuthType = none) + // from "AuthConfiguration is missing or empty for a type that requires it" + // (deployment glitch, decryption failure, operator typo). The unauthenticated + // case is silent; the requires-creds-but-empty case logs a Warning so an + // operator debugging a recurring 401 sees the cause inside ScadaLink instead + // of having to read the remote system's logs. The value of AuthConfiguration + // is NEVER logged. + var authType = system.AuthType?.Trim().ToLowerInvariant() ?? string.Empty; - switch (system.AuthType.ToLowerInvariant()) + if (string.IsNullOrEmpty(system.AuthConfiguration)) + { + if (authType is "apikey" or "basic") + { + _logger.LogWarning( + "ApplyAuth: External system '{System}' has AuthType '{AuthType}' but AuthConfiguration is empty; request will be sent without an auth header.", + system.Name, system.AuthType); + } + return; + } + + switch (authType) { case "apikey": // Auth config format: "HeaderName:KeyValue" or just "KeyValue" (default header: X-API-Key) @@ -493,6 +510,26 @@ public class ExternalSystemClient : IExternalSystemClient Encoding.UTF8.GetBytes($"{basicParts[0]}:{basicParts[1]}")); request.Headers.Authorization = new AuthenticationHeaderValue("Basic", encoded); } + else + { + // ESG-021: malformed Basic config (no ':' separator) means the + // request goes out with no Authorization header. Warn so the + // failure mode is visible inside ScadaLink. + _logger.LogWarning( + "ApplyAuth: External system '{System}' AuthType 'basic' AuthConfiguration is malformed (expected 'username:password'); request will be sent without an Authorization header.", + system.Name); + } + break; + + case "none": + // Documented sentinel for unauthenticated systems — silent by design. + break; + + default: + // ESG-021: unknown AuthType silently fell through here before. Warn. + _logger.LogWarning( + "ApplyAuth: External system '{System}' has unknown AuthType '{AuthType}'; request will be sent without an auth header. Allowed values: apikey, basic, none.", + system.Name, system.AuthType); break; } } diff --git a/src/ScadaLink.Host/Program.cs b/src/ScadaLink.Host/Program.cs index deb02621..b0efcbf6 100644 --- a/src/ScadaLink.Host/Program.cs +++ b/src/ScadaLink.Host/Program.cs @@ -195,8 +195,23 @@ try // is responsible for stashing the resolved API key name on // HttpContext.Items (see AuditWriteMiddleware.AuditActorItemKey) AFTER its // in-handler API key validation succeeds. + // InboundAPI-025: scope the audit middleware to the inbound API method + // route (/api/{methodName}) and explicitly exclude the management/audit + // sub-trees that share the /api prefix. Without these exclusions the + // middleware would emit a spurious ApiInbound audit row for every + // /api/audit/query and /api/audit/export call (and would treat audit-log + // reads as inbound script invocations — recursive write-on-read). The + // POST-only filter rules out the GET routes on /api/audit, /api/centralui, + // /api/script-analysis even if a future route is added under those + // prefixes with the same verb; the explicit prefix excludes still belt- + // and-brace POST-y additions there. app.UseWhen( - ctx => ctx.Request.Path.StartsWithSegments("/api"), + ctx => ctx.Request.Path.StartsWithSegments("/api") + && !ctx.Request.Path.StartsWithSegments("/api/audit") + && !ctx.Request.Path.StartsWithSegments("/api/centralui") + && !ctx.Request.Path.StartsWithSegments("/api/script-analysis") + && !ctx.Request.Path.StartsWithSegments("/api/management") + && HttpMethods.IsPost(ctx.Request.Method), branch => branch.UseAuditWriteMiddleware()); // WP-12: Map readiness endpoint — returns 503 until ready, 200 when ready. diff --git a/src/ScadaLink.ManagementService/ManagementActor.cs b/src/ScadaLink.ManagementService/ManagementActor.cs index 736bf347..f8a65f04 100644 --- a/src/ScadaLink.ManagementService/ManagementActor.cs +++ b/src/ScadaLink.ManagementService/ManagementActor.cs @@ -1135,10 +1135,36 @@ public class ManagementActor : ReceiveActor return true; } + /// + /// MgmtSvc-020: project an SmtpConfiguration to a credential-free shape so the + /// stored Credentials (SMTP password / OAuth2 client secret) never leaves this + /// boundary via response payloads or audit afterState. Mirrors the + /// ApiKey-projection pattern in HandleListApiKeys / CD-012's fix. + /// + private static object SmtpConfigPublicShape(Commons.Entities.Notifications.SmtpConfiguration c) => + new + { + c.Id, + c.Host, + c.Port, + c.AuthType, + c.FromAddress, + c.TlsMode, + c.ConnectionTimeoutSeconds, + c.MaxConcurrentConnections, + c.MaxRetries, + c.RetryDelay, + HasCredentials = !string.IsNullOrEmpty(c.Credentials), + }; + private static async Task HandleListSmtpConfigs(IServiceProvider sp) { var repo = sp.GetRequiredService(); - return await repo.GetAllSmtpConfigurationsAsync(); + var configs = await repo.GetAllSmtpConfigurationsAsync(); + // MgmtSvc-020: project away the Credentials field — read access to this + // list is broader than the Admin-only UpdateSmtpConfig path that owns + // the secret. + return configs.Select(SmtpConfigPublicShape).ToList(); } private static async Task HandleUpdateSmtpConfig(IServiceProvider sp, UpdateSmtpConfigCommand cmd, string user) @@ -1156,8 +1182,12 @@ public class ManagementActor : ReceiveActor if (cmd.Credentials is not null) config.Credentials = cmd.Credentials; await repo.UpdateSmtpConfigurationAsync(config); await repo.SaveChangesAsync(); - await AuditAsync(sp, user, "Update", "SmtpConfiguration", config.Id.ToString(), config.Host, config); - return config; + // MgmtSvc-020: audit the credential-free shape — the *fact of* the change + // (and which non-secret fields hold) is observable; the secret value is + // not persisted to the audit log where OperationalAuditRoles can read it. + var publicShape = SmtpConfigPublicShape(config); + await AuditAsync(sp, user, "Update", "SmtpConfiguration", config.Id.ToString(), config.Host, publicShape); + return publicShape; } // ======================================================================== diff --git a/src/ScadaLink.NotificationService/CredentialRedactor.cs b/src/ScadaLink.NotificationService/CredentialRedactor.cs index 41f4dc0e..257eb1c8 100644 --- a/src/ScadaLink.NotificationService/CredentialRedactor.cs +++ b/src/ScadaLink.NotificationService/CredentialRedactor.cs @@ -24,6 +24,15 @@ public static class CredentialRedactor /// The credential string in use — Basic Auth user:pass or OAuth2 /// tenantId:clientId:clientSecret. May be null. /// + /// + /// NS-025: minimum length for a colon-separated SECRET component to be + /// considered worth masking. Twelve characters is the standard heuristic + /// for "long enough to be a password / client secret"; shorter components + /// (e.g. a 4-char user name like root, or a 7-char "from" alias) + /// would mask too much unrelated diagnostic text if treated as secrets. + /// + private const int MinSecretLength = 12; + public static string Scrub(string? text, string? credentials) { if (string.IsNullOrEmpty(text) || string.IsNullOrEmpty(credentials)) @@ -33,16 +42,36 @@ public static class CredentialRedactor var result = text; - // Mask each individual colon-delimited component (covers user, password, - // tenant, clientId, clientSecret) and the whole packed string. Order longest - // first so a component that is a substring of another is still fully masked. - var parts = credentials.Split(':') - .Where(p => p.Length >= 4) - .Append(credentials) - .Distinct() - .OrderByDescending(p => p.Length); + // NS-025: redact only the obviously-secret slots — the LAST + // colon-separated component (the password in Basic, the client + // secret in OAuth2) and the whole packed string — not the user + // name / tenant id / client id. A short user name like "root" or + // a sender alias like "smtp" no longer becomes a global redaction + // token that eats unrelated path / error text. + var secretsToRedact = new List(); - foreach (var part in parts) + // The full packed credential is always the most-sensitive shape. + secretsToRedact.Add(credentials); + + // The trailing colon-component is the password / clientSecret slot. + // Only redact it if it's plausibly secret-shaped (>= MinSecretLength). + var parts = credentials.Split(':'); + if (parts.Length >= 2) + { + var lastComponent = parts[^1]; + if (lastComponent.Length >= MinSecretLength) + { + secretsToRedact.Add(lastComponent); + } + } + + // Order longest first so a secret that is a substring of the packed + // string is still fully masked. + var ordered = secretsToRedact + .Distinct() + .OrderByDescending(s => s.Length); + + foreach (var part in ordered) { result = result.Replace(part, Mask, StringComparison.Ordinal); } diff --git a/src/ScadaLink.Security/ServiceCollectionExtensions.cs b/src/ScadaLink.Security/ServiceCollectionExtensions.cs index 865faa5f..32f3474e 100644 --- a/src/ScadaLink.Security/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.Security/ServiceCollectionExtensions.cs @@ -1,6 +1,7 @@ using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; namespace ScadaLink.Security; @@ -56,7 +57,7 @@ public static class ServiceCollectionExtensions // session window. Bound here via PostConfigure so SecurityOptions // (configured by the Host after AddSecurity) is honoured. services.AddOptions(CookieAuthenticationDefaults.AuthenticationScheme) - .Configure>((cookieOptions, securityOptions) => + .Configure, ILoggerFactory>((cookieOptions, securityOptions, loggerFactory) => { var idleMinutes = securityOptions.Value.IdleTimeoutMinutes; cookieOptions.ExpireTimeSpan = TimeSpan.FromMinutes(idleMinutes); @@ -69,6 +70,18 @@ public static class ServiceCollectionExtensions cookieOptions.Cookie.SecurePolicy = securityOptions.Value.RequireHttpsCookie ? Microsoft.AspNetCore.Http.CookieSecurePolicy.Always : Microsoft.AspNetCore.Http.CookieSecurePolicy.SameAsRequest; + + // Security-021: when the operator opts out of HTTPS-only cookies, + // log a Warning so an HTTP-only deployment is at least audible in + // the startup log. The cookie carries the embedded JWT bearer + // credential — over plain HTTP that travels in cleartext on every + // request. The default is true; this branch fires only on an + // explicit opt-out (typically the dev Docker cluster). + if (!securityOptions.Value.RequireHttpsCookie) + { + loggerFactory.CreateLogger("ScadaLink.Security").LogWarning( + "SecurityOptions:RequireHttpsCookie is DISABLED — auth cookie SecurePolicy is SameAsRequest. The cookie-embedded JWT will be transmitted in cleartext over plain HTTP. This setting is intended for local dev only — set SecurityOptions:RequireHttpsCookie=true in production."); + } }); services.AddScadaLinkAuthorization(); diff --git a/tests/ScadaLink.ConfigurationDatabase.Tests/InboundApiRepositoryTests.cs b/tests/ScadaLink.ConfigurationDatabase.Tests/InboundApiRepositoryTests.cs index d92994a3..d927e32f 100644 --- a/tests/ScadaLink.ConfigurationDatabase.Tests/InboundApiRepositoryTests.cs +++ b/tests/ScadaLink.ConfigurationDatabase.Tests/InboundApiRepositoryTests.cs @@ -15,7 +15,7 @@ public class InboundApiRepositoryTests : IDisposable public InboundApiRepositoryTests() { _context = SqliteTestHelper.CreateInMemoryContext(); - _repository = new InboundApiRepository(_context, _logger); + _repository = new InboundApiRepository(_context, hasherAccessor: null, logger: _logger); } public void Dispose() @@ -40,6 +40,37 @@ public class InboundApiRepositoryTests : IDisposable Assert.Equal(key.Id, byValue!.Id); } + [Fact] + public async Task CD016_GetApiKeyByValue_UsesInjectedPepperedHasher_NotDefault() + { + // CD-016 regression: stored KeyHash is produced by a peppered hasher. + // A repository whose lookup uses ApiKeyHasher.Default (the pre-fix + // behaviour) would compute a different digest and return null. With the + // pepper-aware hasherAccessor wired in, the lookup must round-trip. + var peppered = new Commons.Types.InboundApi.ApiKeyHasher("a-strong-test-pepper-of-sufficient-length"); + var pepperedHash = peppered.Hash("secret-with-pepper"); + var key = ApiKey.FromHash("Peppered", pepperedHash); + key.IsEnabled = true; + + using var ctx = SqliteTestHelper.CreateInMemoryContext(); + var repo = new InboundApiRepository(ctx, hasherAccessor: () => peppered, logger: _logger); + await repo.AddApiKeyAsync(key); + await repo.SaveChangesAsync(); + + var byValue = await repo.GetApiKeyByValueAsync("secret-with-pepper"); + Assert.NotNull(byValue); + Assert.Equal(key.Id, byValue!.Id); + + // And: a repository wired with the Default (unpeppered) hasher MUST + // NOT find the same key — proving the lookup actually uses the + // injected hasher and the original bug shape. + var defaultRepo = new InboundApiRepository(ctx, + hasherAccessor: () => Commons.Types.InboundApi.ApiKeyHasher.Default, + logger: _logger); + var missByDefault = await defaultRepo.GetApiKeyByValueAsync("secret-with-pepper"); + Assert.Null(missByDefault); + } + [Fact] public async Task AddApiMethod_AndGetByName_RoundTrips() { diff --git a/tests/ScadaLink.NotificationService.Tests/CredentialRedactorTests.cs b/tests/ScadaLink.NotificationService.Tests/CredentialRedactorTests.cs index 454a26bf..eb2aee25 100644 --- a/tests/ScadaLink.NotificationService.Tests/CredentialRedactorTests.cs +++ b/tests/ScadaLink.NotificationService.Tests/CredentialRedactorTests.cs @@ -8,11 +8,13 @@ public class CredentialRedactorTests [Fact] public void Scrub_BasicAuthPassword_IsMasked() { - var text = "535 5.7.8 Authentication failed for user 'svc' with password 'Hunter2pw!'"; - var result = CredentialRedactor.Scrub(text, "svc:Hunter2pw!"); + // Password 'Hunter2pass!word' is 16 chars (>= MinSecretLength=12) and + // therefore qualifies as a redactable secret-shaped trailing component. + var text = "535 5.7.8 Authentication failed for user 'svc' with password 'Hunter2pass!word'"; + var result = CredentialRedactor.Scrub(text, "svc:Hunter2pass!word"); - Assert.DoesNotContain("Hunter2pw!", result); - Assert.DoesNotContain("svc:Hunter2pw!", result); + Assert.DoesNotContain("Hunter2pass!word", result); + Assert.DoesNotContain("svc:Hunter2pass!word", result); } [Fact] @@ -35,4 +37,40 @@ public class CredentialRedactorTests { Assert.Equal(string.Empty, CredentialRedactor.Scrub(null, "user:pass")); } + + // --- NS-025: don't over-mask short non-secret components --- + + [Fact] + public void Scrub_ShortUserName_IsNotMaskedOutsidePackedString() + { + // 'root' is the Basic Auth user name — short, common, and absolutely + // not a secret. It must NOT be masked when it appears in unrelated + // diagnostic text like a file path. + var text = "Config file at /root/.config/scada.conf was not found."; + var result = CredentialRedactor.Scrub(text, "root:hunter2longenoughpwd"); + + Assert.Contains("/root/.config", result); + } + + [Fact] + public void Scrub_TenantId_IsNotMaskedOutsidePackedString() + { + // The tenant id is not secret — only the client secret is. A tenant id + // appearing in unrelated text (e.g. an error-code suffix) must survive. + var text = "Error code tnt-1234567890-abcd reported by upstream"; + var result = CredentialRedactor.Scrub(text, "tnt-1234567890-abcd:cli-guid:RealClientSecretLongEnough"); + + Assert.Contains("tnt-1234567890-abcd", result); + } + + [Fact] + public void Scrub_FullPackedCredential_IsAlwaysMaskedRegardlessOfLength() + { + // Even a short packed string must be masked when it appears verbatim — + // that exact appearance can only come from the credential itself. + var text = "Auth bundle was rejected: u:p"; + var result = CredentialRedactor.Scrub(text, "u:p"); + + Assert.DoesNotContain("u:p", result); + } }