Encrypt Certificate.Password + CertificateData at rest#439
Merged
Conversation
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.
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
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
Certificate.Password(the PFX password) ANDCertificate.CertificateData(the PFX/PEM blob), previously cleartext (so the PFX password protection was moot) — inCertificateDataProvider, the single seam every certificate read/write funnels through (deploy-pipeline client-cert auth viaEndpointContextBuilder, the cert-variable expander, the certificate service). Reuses theIVariableEncryptionServiceV2 envelope. Same proven pattern asDeploymentAccount.Credentials(#437) andExternalFeed.Password(#438). Makes the long-staleCertificate.cs"encrypted at rest" comment finally true.SQUID_ENCRYPTED_V2:prefix (unprefixed plaintext returned verbatim → pre-existing rows still load, upgrade lazily). Idempotent encrypt-on-write. No service-layer change —CertificateDtoexposes onlyPasswordHasValue/HasPrivateKey.IRepository.Detach) rather thanQueryNoTracking. Detaching gives the same flush-safety as Encrypt DeploymentAccount.Credentials at rest #437/Encrypt ExternalFeed.Password at rest #438 (the in-place decrypt is never flushed back as plaintext by a later shared-scopeSaveChanges) and frees the identity map, so the existing single-scope create-then-update/delete CRUD tests don't hit a duplicate-tracking conflict. (QueryNoTrackingis production-safe for account/feed since each mediator request is a fresh scope; the certificate CRUD tests run multiple ops in one scope, which detach handles cleanly.)Security:VariableEncryption:MasterKey— no new key, no hardcoded default (P5 key-lifecycle hardening tracked separately).Test plan
IntegrationCertificateSecretsAtRest, 4): both columns carrySQUID_ENCRYPTED_V2:at rest and not the cleartext secret; decrypt-on-read; read-both on a legacy plaintext row; same-scope read+decrypt+SaveChangeskeeps both columns encrypted (flush regression); a metadata-only update preserves + re-encrypts the secrets.IntegrationCertificateCrudintegration tests + 99 Certificate unit tests stay green (the detach approach keeps the single-scope CRUD flows working). At-rest integration across Account + Feed + Certificate (12) green — the additiveIRepository.Detachdoesn't affect theQueryNoTrackingslices.IVariableEncryptionServicetests; this persistence-layer change is covered at the integration tier (Rule 9).