Extract AtRestSecretProtector as the single at-rest secret contract#445
Merged
Conversation
The account / feed / certificate DataProviders each carried a near-identical value transform: encrypt-if-needed (guarded by IsValidEncryptedValue so a re-save never double-wraps) on write, and read-both decrypt (legacy plaintext passes through verbatim) on read. Four copies of the same contract — and the upcoming SSH proxy-password slice would have been a fifth. Extract it into IAtRestSecretProtector (Protect / UnprotectAsync over a KDF scope), wrapping IVariableEncryptionService. The three providers now depend on this narrow contract instead of re-implementing it. The entity-level anti-flush concern (QueryNoTracking, or Detach before the in-place decrypt) stays in each provider — it is entity-specific and orthogonal to the value transform. Pure refactor, behaviour-identical and non-breaking: the provider ctors swap IVariableEncryptionService for IAtRestSecretProtector (both auto-registered via IScopedDependency, DI-resolved everywhere — no call site constructs them by hand). The existing real-Postgres at-rest integration tests for all three providers pass unchanged, proving the round-trip behaviour is preserved. Tests: AtRestSecretProtectorTests pins the idempotent-Protect (null/empty/ already-encrypted passthrough) + read-both Unprotect contract; the account/ feed/certificate at-rest integration tests are the behaviour-preservation proof. Full unit suite (6043) green.
… real-service tests Two fix-optional review items, both folded in: - Document that the Protect (sync) / UnprotectAsync (async) asymmetry is deliberate — it mirrors IVariableEncryptionService (EncryptAsync is synchronous despite its name; DecryptAsync is genuinely async) — so a future reader does not "normalize" the two halves. - Add two real-VariableEncryptionService-backed cases to the protector unit file so the idempotency + V2-envelope contract is pinned at the seam itself (Protect(plaintext) -> envelope, re-Protect -> unchanged, round-trip), not only via the provider/Postgres integration tier.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
IsValidEncryptedValueso a re-save never double-wraps) on write, and read-both decrypt (legacy plaintext passes through verbatim) on read — four copies of the same contract, and the upcoming SSH proxy-password slice would have been a fifth.IAtRestSecretProtector(Protect/UnprotectAsyncover a KDF scope), wrappingIVariableEncryptionService. The three providers now depend on this narrow SSOT instead of re-implementing it.QueryNoTracking, orDetachbefore the in-place decrypt) stays in each provider — it is entity-specific and orthogonal to the value transform.Foundation for the remaining P2 at-rest finishers (SSH proxy password, then Tentacle private key).
Non-breaking
Pure refactor, behaviour-identical: the provider ctors swap
IVariableEncryptionServiceforIAtRestSecretProtector(both auto-registered viaIScopedDependency, DI-resolved everywhere — no call site constructs them by hand). No interface/enum/signature/wire/DB change.Test plan
AtRestSecretProtectorTests: idempotentProtect(null / empty / already-encrypted passthrough, no double-wrap) + read-bothUnprotectAsync(delegates to the encryption service, legacy plaintext passthrough) + KDF scope forwarded.