diff --git a/src/Squid.Core/Services/Deployments/Account/DeploymentAccountDataProvider.cs b/src/Squid.Core/Services/Deployments/Account/DeploymentAccountDataProvider.cs index 88f7755a..a1de6474 100644 --- a/src/Squid.Core/Services/Deployments/Account/DeploymentAccountDataProvider.cs +++ b/src/Squid.Core/Services/Deployments/Account/DeploymentAccountDataProvider.cs @@ -30,7 +30,7 @@ public interface IDeploymentAccountDataProvider : IScopedDependency /// upgrade naturally on the next save. Reads use the repository's AsNoTracking query so /// decrypting the in-memory entity can never be flushed back to the DB as plaintext. /// -public class DeploymentAccountDataProvider(IRepository repository, IUnitOfWork unitOfWork, IVariableEncryptionService encryption) : IDeploymentAccountDataProvider +public class DeploymentAccountDataProvider(IRepository repository, IUnitOfWork unitOfWork, IAtRestSecretProtector protector) : IDeploymentAccountDataProvider { // The V2 envelope derives its key from a random per-payload salt, so the legacy // KDF-scope argument (a V1-only deterministic-salt input) is irrelevant for accounts, @@ -110,22 +110,20 @@ public async Task> GetAccountsBySpaceIdAsync(int spaceId return (count, await DecryptCredentialsAsync(accounts).ConfigureAwait(false)); } - // Encrypt only if not already an envelope — idempotent, so a re-save (or a value that - // somehow arrives pre-encrypted) never double-wraps. + // Protect is idempotent (null/empty/already-encrypted passes through), so a re-save never double-wraps. private void EncryptCredentials(DeploymentAccount account) { - if (account == null || string.IsNullOrEmpty(account.Credentials)) return; - if (encryption.IsValidEncryptedValue(account.Credentials)) return; + if (account == null) return; - account.Credentials = encryption.EncryptAsync(account.Credentials, CredentialsKdfScope); + account.Credentials = protector.Protect(account.Credentials, CredentialsKdfScope); } private async Task DecryptCredentialsAsync(DeploymentAccount account) { if (account == null || string.IsNullOrEmpty(account.Credentials)) return account; - // Read-both: a plaintext (unprefixed) value is returned verbatim by DecryptAsync. - account.Credentials = await encryption.DecryptAsync(account.Credentials, CredentialsKdfScope).ConfigureAwait(false); + // Read-both: a plaintext (unprefixed) value is returned verbatim by Unprotect. + account.Credentials = await protector.UnprotectAsync(account.Credentials, CredentialsKdfScope).ConfigureAwait(false); return account; } diff --git a/src/Squid.Core/Services/Deployments/Certificates/CertificateDataProvider.cs b/src/Squid.Core/Services/Deployments/Certificates/CertificateDataProvider.cs index 632ac6ad..9481a662 100644 --- a/src/Squid.Core/Services/Deployments/Certificates/CertificateDataProvider.cs +++ b/src/Squid.Core/Services/Deployments/Certificates/CertificateDataProvider.cs @@ -38,7 +38,7 @@ public interface ICertificateDataProvider : IScopedDependency /// create-then-update/delete (as the CRUD tests exercise) does not hit a duplicate-tracking /// conflict. /// -public class CertificateDataProvider(IUnitOfWork unitOfWork, IRepository repository, IVariableEncryptionService encryption) : ICertificateDataProvider +public class CertificateDataProvider(IUnitOfWork unitOfWork, IRepository repository, IAtRestSecretProtector protector) : ICertificateDataProvider { // The V2 envelope derives its key from a random per-payload salt, so the legacy KDF-scope // argument (a V1-only deterministic-salt input) is irrelevant for certificates (write V2 only). @@ -102,21 +102,14 @@ public async Task DeleteCertificatesAsync(List DecryptSecretsAsync(Persistence.Entities.Deployments.Certificate certificate) @@ -127,9 +120,9 @@ private string Protect(string value) // map is freed (no duplicate-tracking conflict on a same-scope update/delete). repository.Detach(certificate); - // Read-both: a plaintext (unprefixed) value is returned verbatim by DecryptAsync. - certificate.Password = await encryption.DecryptAsync(certificate.Password, SecretKdfScope).ConfigureAwait(false); - certificate.CertificateData = await encryption.DecryptAsync(certificate.CertificateData, SecretKdfScope).ConfigureAwait(false); + // Read-both: a plaintext (unprefixed) value is returned verbatim by Unprotect. + certificate.Password = await protector.UnprotectAsync(certificate.Password, SecretKdfScope).ConfigureAwait(false); + certificate.CertificateData = await protector.UnprotectAsync(certificate.CertificateData, SecretKdfScope).ConfigureAwait(false); return certificate; } diff --git a/src/Squid.Core/Services/Deployments/ExternalFeeds/ExternalFeedDataProvider.cs b/src/Squid.Core/Services/Deployments/ExternalFeeds/ExternalFeedDataProvider.cs index ea623138..93bcc167 100644 --- a/src/Squid.Core/Services/Deployments/ExternalFeeds/ExternalFeedDataProvider.cs +++ b/src/Squid.Core/Services/Deployments/ExternalFeeds/ExternalFeedDataProvider.cs @@ -33,7 +33,7 @@ public interface IExternalFeedDataProvider : IScopedDependency /// in-memory entity can never be flushed back to the DB as plaintext by a later shared-scope /// SaveChanges. /// -public class ExternalFeedDataProvider(IUnitOfWork unitOfWork, IRepository repository, IVariableEncryptionService encryption) : IExternalFeedDataProvider +public class ExternalFeedDataProvider(IUnitOfWork unitOfWork, IRepository repository, IAtRestSecretProtector protector) : IExternalFeedDataProvider { // The V2 envelope derives its key from a random per-payload salt, so the legacy KDF-scope // argument (a V1-only deterministic-salt input) is irrelevant for feeds, which only write V2. @@ -100,21 +100,20 @@ public async Task DeleteExternalFeedsAsync(List DecryptPasswordAsync(Persistence.Entities.Deployments.ExternalFeed feed) { if (feed == null || string.IsNullOrEmpty(feed.Password)) return feed; - // Read-both: a plaintext (unprefixed) value is returned verbatim by DecryptAsync. - feed.Password = await encryption.DecryptAsync(feed.Password, PasswordKdfScope).ConfigureAwait(false); + // Read-both: a plaintext (unprefixed) value is returned verbatim by Unprotect. + feed.Password = await protector.UnprotectAsync(feed.Password, PasswordKdfScope).ConfigureAwait(false); return feed; } diff --git a/src/Squid.Core/Services/Security/AtRestSecretProtector.cs b/src/Squid.Core/Services/Security/AtRestSecretProtector.cs new file mode 100644 index 00000000..b3d9e2aa --- /dev/null +++ b/src/Squid.Core/Services/Security/AtRestSecretProtector.cs @@ -0,0 +1,57 @@ +namespace Squid.Core.Services.Security; + +/// +/// Single source of truth for the per-field at-rest secret contract that every DataProvider +/// seam (account credentials, feed password, certificate password + data, SSH proxy password) +/// applies before persisting / after loading a sensitive string column or JSON field. +/// +/// Two halves, both non-breaking by construction: +/// +/// Protect — encrypt idempotently: an already-encrypted envelope (or a null/empty +/// value) is returned unchanged, so a re-save never double-wraps. +/// Unprotect — decrypt read-both: a legacy plaintext (unprefixed) value is returned +/// verbatim, so reads work before AND after migration with no DB migration. +/// +/// +/// The sync/async split is intentional and mirrors the wrapped service: Protect is +/// synchronous because is synchronous despite +/// its name (a pre-existing misnomer — it returns string, not a Task), while +/// UnprotectAsync is genuinely async because +/// is. Do NOT "normalize" the two halves — making Protect async adds a needless Task allocation +/// and making UnprotectAsync sync would block on a real async decrypt. +/// +/// This wraps (the AES-256-GCM V2 envelope + KDF) so +/// the providers depend on this narrow contract rather than re-implementing the +/// IsValidEncryptedValue-guarded encrypt + passthrough-decrypt in each one (previously four +/// near-identical copies). The ENTITY-level anti-flush concern (read via QueryNoTracking or +/// Detach before the in-place decrypt) stays in each provider — it is entity-specific and +/// orthogonal to this value transform. +/// +public interface IAtRestSecretProtector : IScopedDependency +{ + /// Encrypt for at-rest storage, idempotently. Null / empty / + /// already-encrypted input is returned unchanged. is the per-concern + /// KDF salt scope (the entity's encryption scope constant). + string Protect(string value, int kdfScope); + + /// Decrypt an at-rest , read-both: a legacy unprefixed plaintext + /// is returned verbatim. must match the scope used to Protect it. + Task UnprotectAsync(string value, int kdfScope); +} + +public sealed class AtRestSecretProtector(IVariableEncryptionService encryption) : IAtRestSecretProtector +{ + public string Protect(string value, int kdfScope) + { + // Guard null/empty AND already-an-envelope so a re-save (or a value a previous save already + // encrypted) is never double-wrapped. EncryptAsync also short-circuits empty, but the explicit + // guard keeps IsValidEncryptedValue off a null and documents the idempotency contract here. + if (string.IsNullOrEmpty(value) || encryption.IsValidEncryptedValue(value)) return value; + + return encryption.EncryptAsync(value, kdfScope); + } + + public Task UnprotectAsync(string value, int kdfScope) + // DecryptAsync is read-both: an unprefixed plaintext value passes through verbatim. + => encryption.DecryptAsync(value, kdfScope); +} diff --git a/tests/Squid.UnitTests/Services/Security/AtRestSecretProtectorTests.cs b/tests/Squid.UnitTests/Services/Security/AtRestSecretProtectorTests.cs new file mode 100644 index 00000000..1a096d1b --- /dev/null +++ b/tests/Squid.UnitTests/Services/Security/AtRestSecretProtectorTests.cs @@ -0,0 +1,120 @@ +using Microsoft.Extensions.Configuration; +using Squid.Core.Services.Security; +using Squid.Core.Settings.Security; + +namespace Squid.UnitTests.Services.Security; + +/// +/// Unit tests for — the SSOT value-transform extracted from the +/// account / feed / certificate DataProviders (and reused by the SSH proxy slice). Pins the two +/// non-breaking guarantees: idempotent Protect (null / empty / already-encrypted passes through, so +/// a re-save never double-wraps) and read-both Unprotect (delegates to the encryption service's +/// passthrough decrypt). The KDF scope is forwarded verbatim. +/// +public sealed class AtRestSecretProtectorTests +{ + private readonly Mock _encryption = new(); + private readonly AtRestSecretProtector _sut; + + public AtRestSecretProtectorTests() + { + _sut = new AtRestSecretProtector(_encryption.Object); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + public void Protect_NullOrEmpty_ReturnsUnchanged_WithoutEncrypting(string value) + { + _sut.Protect(value, kdfScope: 0).ShouldBe(value); + + _encryption.Verify(e => e.EncryptAsync(It.IsAny(), It.IsAny()), Times.Never); + _encryption.Verify(e => e.IsValidEncryptedValue(It.IsAny()), Times.Never, + failMessage: "null/empty must short-circuit before IsValidEncryptedValue is asked."); + } + + [Fact] + public void Protect_AlreadyEncrypted_ReturnsUnchanged_WithoutReencrypting() + { + const string envelope = "SQUID_ENCRYPTED_V2:abc"; + _encryption.Setup(e => e.IsValidEncryptedValue(envelope)).Returns(true); + + _sut.Protect(envelope, kdfScope: 7).ShouldBe(envelope); + + _encryption.Verify(e => e.EncryptAsync(It.IsAny(), It.IsAny()), Times.Never, + failMessage: "an already-encrypted envelope must not be double-wrapped on re-save."); + } + + [Fact] + public void Protect_Plaintext_Encrypts_WithGivenScope() + { + _encryption.Setup(e => e.IsValidEncryptedValue("secret")).Returns(false); + _encryption.Setup(e => e.EncryptAsync("secret", 7)).Returns("SQUID_ENCRYPTED_V2:xyz"); + + _sut.Protect("secret", kdfScope: 7).ShouldBe("SQUID_ENCRYPTED_V2:xyz"); + + _encryption.Verify(e => e.EncryptAsync("secret", 7), Times.Once, + failMessage: "a plaintext value must be encrypted with the supplied KDF scope."); + } + + [Fact] + public async Task UnprotectAsync_DelegatesToDecrypt_WithGivenScope() + { + _encryption.Setup(e => e.DecryptAsync("SQUID_ENCRYPTED_V2:xyz", 7)).ReturnsAsync("secret"); + + (await _sut.UnprotectAsync("SQUID_ENCRYPTED_V2:xyz", 7)).ShouldBe("secret"); + } + + [Fact] + public async Task UnprotectAsync_LegacyPlaintext_PassesThrough_ViaReadBothDecrypt() + { + // Read-both is DecryptAsync's contract — the protector just delegates. Pin that a value the + // encryption service returns verbatim (legacy unprefixed plaintext) flows through unchanged. + _encryption.Setup(e => e.DecryptAsync("legacy-plaintext", 0)).ReturnsAsync("legacy-plaintext"); + + (await _sut.UnprotectAsync("legacy-plaintext", 0)).ShouldBe("legacy-plaintext"); + } + + // ── Real-service round-trip (no mock) — pins the idempotency + envelope contract at the seam ── + + [Fact] + public void Protect_RealService_Plaintext_YieldsV2Envelope_AndIsIdempotent() + { + var sut = new AtRestSecretProtector(RealEncryption()); + + var envelope = sut.Protect("plaintext-secret", kdfScope: 0); + + envelope.ShouldStartWith("SQUID_ENCRYPTED_V2:", + customMessage: "Protect over the real service must emit the V2 envelope, not the plaintext."); + envelope.ShouldNotContain("plaintext-secret"); + sut.Protect(envelope, kdfScope: 0).ShouldBe(envelope, + customMessage: "re-Protecting an already-encrypted envelope must return it unchanged (no double-wrap)."); + } + + [Fact] + public async Task ProtectThenUnprotect_RealService_RoundTrips() + { + var sut = new AtRestSecretProtector(RealEncryption()); + + var envelope = sut.Protect("plaintext-secret", kdfScope: 0); + + (await sut.UnprotectAsync(envelope, kdfScope: 0)).ShouldBe("plaintext-secret"); + } + + // Real VariableEncryptionService with a real 32-byte key — passes Strict enforcement (a real key + // is mode-invariant, so no env control is needed here). + private static IVariableEncryptionService RealEncryption() + { + var key = new byte[32]; + for (var i = 0; i < key.Length; i++) key[i] = (byte)(i + 1); + + var config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Security:VariableEncryption:MasterKey"] = Convert.ToBase64String(key) + }) + .Build(); + + return new VariableEncryptionService(new SecuritySetting(config)); + } +}