From 3d3f43229f216e094617c88da667220b53990d73 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 17 May 2026 03:18:24 -0400 Subject: [PATCH] =?UTF-8?q?fix(configuration-database):=20resolve=20Config?= =?UTF-8?q?urationDatabase-013,014=20=E2=80=94=20fail-fast=20on=20missing?= =?UTF-8?q?=20key=20ring,=20single=20converter=20local;=20ConfigurationDat?= =?UTF-8?q?abase-012=20left=20open=20(cross-module=20design=20decision)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../ConfigurationDatabase/findings.md | 87 ++++++++++- .../ScadaLinkDbContext.cs | 139 ++++++++++++++++-- .../EphemeralEncryptionFallbackTests.cs | 85 +++++++++++ .../SchemaConfigurationTests.cs | 22 +++ .../SqliteTestHelper.cs | 9 +- 5 files changed, 326 insertions(+), 16 deletions(-) create mode 100644 tests/ScadaLink.ConfigurationDatabase.Tests/EphemeralEncryptionFallbackTests.cs diff --git a/code-reviews/ConfigurationDatabase/findings.md b/code-reviews/ConfigurationDatabase/findings.md index fd43563..01e4ac8 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 | 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`, 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`. diff --git a/src/ScadaLink.ConfigurationDatabase/ScadaLinkDbContext.cs b/src/ScadaLink.ConfigurationDatabase/ScadaLinkDbContext.cs index 6812c66..d908e93 100644 --- a/src/ScadaLink.ConfigurationDatabase/ScadaLinkDbContext.cs +++ b/src/ScadaLink.ConfigurationDatabase/ScadaLinkDbContext.cs @@ -1,6 +1,8 @@ using Microsoft.AspNetCore.DataProtection; using Microsoft.AspNetCore.DataProtection.EntityFrameworkCore; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Storage.ValueConversion; using ScadaLink.Commons.Entities.Audit; using ScadaLink.Commons.Entities.Deployment; using ScadaLink.Commons.Entities.ExternalSystems; @@ -85,6 +87,19 @@ public class ScadaLinkDbContext : DbContext, IDataProtectionKeyContext // Data Protection Keys (for shared ASP.NET Data Protection across nodes) public DbSet DataProtectionKeys => Set(); + protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) + { + base.OnConfiguring(optionsBuilder); + + // The secret-column converter built in OnModelCreating differs depending on whether + // a real Data Protection provider was supplied (encrypting converter) or not + // (schema-only converter). EF Core's default model cache keys only on context type, + // so a provider-bearing and a schema-only context sharing the same options type + // would otherwise share one cached model — and whichever was built first would win. + // Distinguish them so each gets its own model. + optionsBuilder.ReplaceService(); + } + protected override void OnModelCreating(ModelBuilder modelBuilder) { modelBuilder.ApplyConfigurationsFromAssembly(typeof(ScadaLinkDbContext).Assembly); @@ -92,23 +107,106 @@ public class ScadaLinkDbContext : DbContext, IDataProtectionKeyContext ApplySecretColumnEncryption(modelBuilder); } + public override int SaveChanges(bool acceptAllChangesOnSuccess) + { + GuardSecretWritesHaveAKeyRing(); + return base.SaveChanges(acceptAllChangesOnSuccess); + } + + public override Task SaveChangesAsync( + bool acceptAllChangesOnSuccess, CancellationToken cancellationToken = default) + { + GuardSecretWritesHaveAKeyRing(); + return base.SaveChangesAsync(acceptAllChangesOnSuccess, cancellationToken); + } + + /// True when this context was constructed with a real Data Protection provider. + internal bool HasSecretEncryptionProvider => _dataProtectionProvider is not null; + + /// + /// Fails fast — before any database round-trip — if a context built without a real + /// Data Protection key ring (the schema-only single-argument constructor) is about to + /// persist a non-null secret-bearing column. Without this guard the schema-only + /// protector would still throw, but only deep inside EF's update pipeline wrapped in a + /// DbUpdateException/CryptographicException; surfacing a clear + /// here makes the misconfiguration obvious. + /// + private void GuardSecretWritesHaveAKeyRing() + { + if (_dataProtectionProvider is not null) + return; + + foreach (var entry in ChangeTracker.Entries()) + { + if (entry.State is not (EntityState.Added or EntityState.Modified)) + continue; + + string? secretProperty = entry.Entity switch + { + SmtpConfiguration => nameof(SmtpConfiguration.Credentials), + ExternalSystemDefinition => nameof(ExternalSystemDefinition.AuthConfiguration), + DatabaseConnectionDefinition => nameof(DatabaseConnectionDefinition.ConnectionString), + _ => null + }; + if (secretProperty is null) + continue; + + if (entry.Property(secretProperty).CurrentValue is not null) + { + throw new InvalidOperationException( + "This ScadaLinkDbContext was constructed without a Data Protection key " + + "ring (the single-argument, schema-only constructor). It cannot persist " + + $"the secret-bearing column '{entry.Entity.GetType().Name}.{secretProperty}'. " + + "Construct the context with the DI-registered IDataProtectionProvider " + + "(AddConfigurationDatabase wires this up)."); + } + } + } + + /// + /// Model cache key factory that folds into the + /// cache key, so a write-capable (provider-bearing) context and a schema-only context do + /// not share a cached model. + /// + private sealed class SecretAwareModelCacheKeyFactory : IModelCacheKeyFactory + { + public object Create(DbContext context, bool designTime) + => (context.GetType(), + designTime, + (context as ScadaLinkDbContext)?.HasSecretEncryptionProvider ?? false); + + public object Create(DbContext context) => Create(context, false); + } + /// /// Applies encryption-at-rest to columns that hold authentication secrets /// (SMTP credentials, external-system auth config, database connection strings) /// so they are never persisted as plaintext. /// /// - /// When no Data Protection provider is supplied (design-time dotnet ef tooling, - /// which only emits schema and never reads or writes secret data), an ephemeral provider - /// is used. The encrypted-column type is nvarchar either way, so the generated - /// schema is identical regardless of which provider is in effect. The runtime path always - /// receives the DI-registered provider whose keys are persisted to this database. + /// When no Data Protection provider is supplied — design-time dotnet ef tooling, + /// which only emits schema and never reads or writes secret data — a schema-only + /// protector is used. It produces an identical nvarchar schema, but throws a + /// clear if a secret column is ever read or + /// written through it. This deliberately does NOT silently substitute an ephemeral + /// (in-memory, process-lifetime) key: encrypting a runtime write with a throwaway key + /// would persist ciphertext that becomes permanently undecryptable on the next process + /// restart, with no error. A write-capable context must be constructed with the + /// DI-registered provider whose keys are persisted to this database. /// private void ApplySecretColumnEncryption(ModelBuilder modelBuilder) { - IDataProtectionProvider provider = _dataProtectionProvider ?? new EphemeralDataProtectionProvider(); - var converter = new EncryptedStringConverter( - provider.CreateProtector(EncryptedStringConverter.ProtectorPurpose)); + IDataProtector protector = _dataProtectionProvider is { } provider + ? provider.CreateProtector(EncryptedStringConverter.ProtectorPurpose) + : SchemaOnlyDataProtector.Instance; + + // Held as the non-generic ValueConverter base so all three HasConversion calls + // read identically. The converter's CLR type is string?->string?; binding it to + // the non-nullable DatabaseConnectionDefinition.ConnectionString property would + // otherwise raise a CS8620 nullability mismatch — the non-generic reference is + // the supported way to apply one converter uniformly across nullable and + // non-nullable string columns. + ValueConverter converter = new EncryptedStringConverter(protector); modelBuilder.Entity() .Property(s => s.Credentials) @@ -120,6 +218,29 @@ public class ScadaLinkDbContext : DbContext, IDataProtectionKeyContext modelBuilder.Entity() .Property(d => d.ConnectionString) - .HasConversion((Microsoft.EntityFrameworkCore.Storage.ValueConversion.ValueConverter)converter); + .HasConversion(converter); + } + + /// + /// An for contexts built without a real Data Protection + /// provider (design-time / schema-only). It satisfies model building but fails fast + /// with a clear message if a secret column is actually read or written, rather than + /// silently producing throwaway ciphertext that cannot be decrypted after a restart. + /// + private sealed class SchemaOnlyDataProtector : IDataProtector + { + internal static readonly SchemaOnlyDataProtector Instance = new(); + + private const string Message = + "This ScadaLinkDbContext was constructed without a Data Protection key ring " + + "(the single-argument, schema-only constructor). Secret-bearing configuration " + + "columns cannot be read or written through it. Construct the context with the " + + "DI-registered IDataProtectionProvider (AddConfigurationDatabase wires this up)."; + + public IDataProtector CreateProtector(string purpose) => this; + + public byte[] Protect(byte[] plaintext) => throw new InvalidOperationException(Message); + + public byte[] Unprotect(byte[] protectedData) => throw new InvalidOperationException(Message); } } diff --git a/tests/ScadaLink.ConfigurationDatabase.Tests/EphemeralEncryptionFallbackTests.cs b/tests/ScadaLink.ConfigurationDatabase.Tests/EphemeralEncryptionFallbackTests.cs new file mode 100644 index 0000000..519a51b --- /dev/null +++ b/tests/ScadaLink.ConfigurationDatabase.Tests/EphemeralEncryptionFallbackTests.cs @@ -0,0 +1,85 @@ +using Microsoft.AspNetCore.DataProtection; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; +using ScadaLink.Commons.Entities.ExternalSystems; +using ScadaLink.ConfigurationDatabase; + +namespace ScadaLink.ConfigurationDatabase.Tests; + +/// +/// Regression guard for ConfigurationDatabase-013: a +/// constructed without an explicit Data Protection provider (the single-argument +/// constructor) must NOT silently encrypt secret columns with a throwaway ephemeral +/// key — that would persist ciphertext that becomes permanently undecryptable on the +/// next process restart, with no error. Writing a secret column on such a context +/// must fail fast with a clear instead. +/// +public class EphemeralEncryptionFallbackTests +{ + private static DbContextOptions SqliteOptions() => + new DbContextOptionsBuilder() + .UseSqlite("DataSource=:memory:") + .ConfigureWarnings(w => w.Ignore(RelationalEventId.PendingModelChangesWarning)) + .Options; + + [Fact] + public async Task SingleArgConstructor_WritingSecretColumn_FailsFast_DoesNotPersistThrowawayCiphertext() + { + // Single-argument constructor: no Data Protection provider supplied (the + // design-time / schema-only path). Schema creation must still succeed. + using var context = new ScadaLinkDbContext(SqliteOptions()); + context.Database.OpenConnection(); + context.Database.EnsureCreated(); + + // AuthConfiguration is an encrypted secret column. Persisting it without a real + // key ring would produce undecryptable ciphertext; it must throw instead. + var ext = new ExternalSystemDefinition("Erp", "https://erp.example.com", "ApiKey") + { + AuthConfiguration = "{\"apiKey\":\"live-secret\"}" + }; + context.ExternalSystemDefinitions.Add(ext); + + var ex = await Assert.ThrowsAsync( + () => context.SaveChangesAsync()); + Assert.Contains("Data Protection", ex.Message); + } + + [Fact] + public async Task SingleArgConstructor_WritingNonSecretColumn_Succeeds() + { + // The schema-only / no-provider context must remain fully usable for entities + // that have no encrypted secret columns — only secret writes are gated. + using var context = new ScadaLinkDbContext(SqliteOptions()); + context.Database.OpenConnection(); + context.Database.EnsureCreated(); + + var ext = new ExternalSystemDefinition("Erp", "https://erp.example.com", "None"); + context.ExternalSystemDefinitions.Add(ext); + + await context.SaveChangesAsync(); + + Assert.True(ext.Id > 0); + } + + [Fact] + public async Task ProviderConstructor_WritingSecretColumn_StillSucceeds() + { + // Sanity check: the gating must not regress the supported runtime path where a + // real Data Protection provider is supplied. + using var context = new ScadaLinkDbContext(SqliteOptions(), new EphemeralDataProtectionProvider()); + context.Database.OpenConnection(); + context.Database.EnsureCreated(); + + var ext = new ExternalSystemDefinition("Erp", "https://erp.example.com", "ApiKey") + { + AuthConfiguration = "{\"apiKey\":\"live-secret\"}" + }; + context.ExternalSystemDefinitions.Add(ext); + + await context.SaveChangesAsync(); + context.ChangeTracker.Clear(); + + var loaded = await context.ExternalSystemDefinitions.SingleAsync(e => e.Id == ext.Id); + Assert.Equal("{\"apiKey\":\"live-secret\"}", loaded.AuthConfiguration); + } +} diff --git a/tests/ScadaLink.ConfigurationDatabase.Tests/SchemaConfigurationTests.cs b/tests/ScadaLink.ConfigurationDatabase.Tests/SchemaConfigurationTests.cs index 81bdbdb..3fa62ec 100644 --- a/tests/ScadaLink.ConfigurationDatabase.Tests/SchemaConfigurationTests.cs +++ b/tests/ScadaLink.ConfigurationDatabase.Tests/SchemaConfigurationTests.cs @@ -1,4 +1,6 @@ using Microsoft.EntityFrameworkCore; +using ScadaLink.Commons.Entities.ExternalSystems; +using ScadaLink.Commons.Entities.Notifications; using ScadaLink.Commons.Entities.Sites; using ScadaLink.Commons.Entities.Templates; using ScadaLink.ConfigurationDatabase; @@ -48,6 +50,26 @@ public class SchemaConfigurationTests : IDisposable Assert.Equal(siblingMaxLength, entity.FindProperty(nameof(Site.GrpcNodeAAddress))!.GetMaxLength()); Assert.Equal(siblingMaxLength, entity.FindProperty(nameof(Site.GrpcNodeBAddress))!.GetMaxLength()); } + + // ConfigurationDatabase-014: the encrypting value converter must be applied + // uniformly to all three secret-bearing columns, including the non-nullable + // DatabaseConnectionDefinition.ConnectionString. A regression here (e.g. the + // converter dropped from one HasConversion call) would silently store a secret + // in plaintext. + + [Theory] + [InlineData(typeof(SmtpConfiguration), nameof(SmtpConfiguration.Credentials))] + [InlineData(typeof(ExternalSystemDefinition), nameof(ExternalSystemDefinition.AuthConfiguration))] + [InlineData(typeof(DatabaseConnectionDefinition), nameof(DatabaseConnectionDefinition.ConnectionString))] + public void SecretColumns_AllHaveEncryptedStringConverterApplied(Type entityType, string propertyName) + { + var converter = _context.Model + .FindEntityType(entityType)! + .FindProperty(propertyName)! + .GetValueConverter(); + + Assert.IsType(converter); + } } public class SplitQueryBehaviourTests : IDisposable diff --git a/tests/ScadaLink.ConfigurationDatabase.Tests/SqliteTestHelper.cs b/tests/ScadaLink.ConfigurationDatabase.Tests/SqliteTestHelper.cs index 026f8ca..6c60328 100644 --- a/tests/ScadaLink.ConfigurationDatabase.Tests/SqliteTestHelper.cs +++ b/tests/ScadaLink.ConfigurationDatabase.Tests/SqliteTestHelper.cs @@ -1,3 +1,4 @@ +using Microsoft.AspNetCore.DataProtection; using Microsoft.EntityFrameworkCore; using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.EntityFrameworkCore.Storage.ValueConversion; @@ -10,10 +11,16 @@ namespace ScadaLink.ConfigurationDatabase.Tests; /// Test DbContext that adapts SQL Server-specific features for SQLite: /// - Maps DateTimeOffset to sortable ISO 8601 strings (SQLite has no native DateTimeOffset ORDER BY) /// - Replaces SQL Server RowVersion with a nullable byte[] column (SQLite can't auto-generate rowversion) +/// +/// Constructed with an explicit ephemeral Data Protection provider so secret-bearing +/// columns are write-capable in tests. The schema-only no-provider constructor would +/// throw on a secret-column write (ConfigurationDatabase-013); passing a provider here +/// makes the test fixture's intent explicit at the call site. /// public class SqliteTestDbContext : ScadaLinkDbContext { - public SqliteTestDbContext(DbContextOptions options) : base(options) + public SqliteTestDbContext(DbContextOptions options) + : base(options, new EphemeralDataProtectionProvider()) { }