Encrypt ExternalFeed.Password at rest#438
Merged
Merged
Conversation
The package-feed credential was persisted in cleartext. Encrypt it at rest in ExternalFeedDataProvider — the single seam every feed read/write funnels through (deploy-pipeline package fetch, Helm/K8s registry auth, the feed service all load via GetFeedByIdAsync / GetExternalFeedsByIdsAsync / paging) — reusing the IVariableEncryptionService V2 envelope, same pattern as DeploymentAccount (#437). - Encrypt-on-write (Add/Update, idempotent IsValidEncryptedValue guard). - Decrypt-on-read via repository.QueryNoTracking (detached entities) so the in-place decrypt is never flushed back as plaintext by a later shared-scope SaveChanges — the exact hazard caught + fixed in #437, applied here from the start. - Read-both: an unprefixed value is returned verbatim, so pre-existing plaintext rows still load and upgrade lazily. No schema change, no migration. No service-layer change: ExternalFeedDto exposes only PasswordHasValue (a non-empty ciphertext still reports true), so the response stays correct with the entity holding ciphertext. Reuses the existing Security:VariableEncryption:MasterKey (no new key, no hardcoded default). Integration tests (real Postgres) cover encrypted-at-rest column, decrypt-on-read, read-both on a legacy plaintext row, update re-encrypt, and the same-scope flush regression.
4 tasks
ppXD
added a commit
that referenced
this pull request
Jun 13, 2026
A certificate's PFX password AND the PFX/PEM blob (private-key material) were both persisted in cleartext — so the PFX password protection was moot. Encrypt both at rest in CertificateDataProvider, the single seam every certificate read/write funnels through (deploy-pipeline client-cert auth via EndpointContextBuilder, the cert-variable expander, the certificate service), reusing the IVariableEncryptionService V2 envelope. Same pattern as DeploymentAccount (#437) / ExternalFeed (#438). Read-both (unprefixed plaintext passthrough) = non-breaking, no migration. No service change (CertificateDto is HasValue-only). Makes the long-stale Certificate.cs 'encrypted at rest' comment finally true. Reads use a load-tracked-then-Detach-then-decrypt approach (new IRepository.Detach) rather than QueryNoTracking. Detaching gives the same flush-safety guarantee as #437/#438 (the in-place decrypt is never flushed back as plaintext by a later shared-scope SaveChanges) AND, unlike a second AsNoTracking instance, frees the identity map — so the existing single-scope create-then-update/delete CRUD tests do not hit a duplicate-tracking conflict. (QueryNoTracking is production-safe for account/feed because each mediator request is a fresh scope; the certificate CRUD tests exercise multiple ops in one scope, which detach handles cleanly.) Tests: integration (real Postgres) covers both columns encrypted-at-rest, decrypt-on-read, read-both on a legacy plaintext row, the same-scope flush regression, and a metadata-only update preserving + re-encrypting the secrets. All 34 existing Certificate integration tests + 99 unit tests stay green.
4 tasks
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
ExternalFeed.Passwordcolumn (package-feed credential, previously cleartext) at rest inExternalFeedDataProvider— the single seam every feed read/write funnels through (deploy-pipeline package fetch, Helm/K8s registry auth, the feed service), reusing theIVariableEncryptionServiceV2 envelope. Same proven pattern asDeploymentAccount.Credentials(#437).repository.QueryNoTracking(detached entities) so the in-place decrypt can never be flushed back as plaintext by a later shared-scopeSaveChanges— the flush-back hazard caught & fixed in Encrypt DeploymentAccount.Credentials at rest #437, applied here from the start.SQUID_ENCRYPTED_V2:prefix (unprefixed values returned verbatim → pre-existing plaintext rows still load, upgrade lazily). Encrypt-on-write is idempotent. No service-layer change —ExternalFeedDtoexposes onlyPasswordHasValue(a non-empty ciphertext still reportstrue), so the response stays correct with the entity holding ciphertext.Security:VariableEncryption:MasterKey— no new key source, no hardcoded default (P5 key-lifecycle hardening tracked separately).Test plan
IntegrationExternalFeedPasswordAtRest, 4): raw column carriesSQUID_ENCRYPTED_V2:and not the cleartext secret; decrypt-on-read; read-both on a hand-inserted plaintext row; update re-encrypts; same-scope read+decrypt then unrelatedSaveChangeskeeps the column encrypted (the Encrypt DeploymentAccount.Credentials at rest #437 flush-back regression, applied as a guard here).IVariableEncryptionServicetests from Encrypt DeploymentAccount.Credentials at rest #437; this persistence-layer change is correctly covered at the integration tier (Rule 9).