Encrypt DeploymentAccount.Credentials at rest#437
Merged
Conversation
The Credentials column is the most sensitive secret store in the system — every account type's token / password / cloud secret / SSH private key — and was persisted as plaintext JSON. Encrypt it at rest through DeploymentAccountDataProvider, the single seam every account read and write funnels through (the deploy pipeline, the OpenClaw paths, and the account service all load via it), mirroring how VariableDataProvider protects the Variable table. - Encrypt-on-write (Add/Update) via the existing IVariableEncryptionService V2 envelope, guarded by IsValidEncryptedValue so a re-save never double-wraps. - Decrypt-on-read on every load. Reads use the repository's AsNoTracking query so mutating the in-memory entity can never be flushed back as plaintext; GetAccountByIdAsync is switched from the tracking FindAsync to an AsNoTracking query for the same reason. - Read-both / zero-migration: a value lacking the SQUID_ENCRYPTED_V2 prefix is returned verbatim, so pre-existing plaintext rows keep working and upgrade lazily on next save. No schema change, no backfill. - MapToDto now builds the response summary from the plaintext credentials the service already holds, since the entity carries ciphertext after a write. Reuses the already-configured Security:VariableEncryption:MasterKey — no new key source and no hardcoded default. With an unset MasterKey the envelope is cosmetic; real protection requires an operator-set key (SQUID_MASTER_KEY_ENFORCEMENT=strict).
Pre-merge review caught a blocker: repository.Query<T>() TRACKS (the AsNoTracking variant is the separate QueryNoTracking), so decrypting the loaded entity in place marked it Modified — and the deploy pipeline's shared-scope SaveChanges would flush the plaintext back, silently un-encrypting the account on the first deployment. - Switch all four reads (GetAccountById/ByIds/BySpaceId/Paging) to repository.QueryNoTracking<DeploymentAccount> so the decrypt mutates a DETACHED entity that is never flushed (matching DeploymentCheckpointService's idiom). The Update flow still persists correctly: UpdateAccountAsync re-encrypts before _dbContext.Update re-attaches the detached entity as Modified. Also eliminates the spurious DocumentModified audit event the false-Modified state would have emitted. - Regression test (IntegrationAccountCredentialsAtRest): read+decrypt an account then an unrelated SaveChanges in the SAME scope → assert the raw column still carries the SQUID_ENCRYPTED_V2 envelope. Fails on the tracking code, passes after the fix. - E2E: route K8sTestDataSeeder account creation through the encrypting AddAccountAsync (was raw InsertAsync → plaintext), so every K8s pipeline E2E now exercises the encrypted-at-rest credential path end to end (+ a 3-arg E2E fixture Run overload to resolve the provider).
4 tasks
ppXD
added a commit
that referenced
this pull request
Jun 13, 2026
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
DeploymentAccount.Credentialscolumn at rest — the system's most sensitive secret store (every account type's token / password / cloud secret / SSH private key), previously persisted as plaintext JSON.DeploymentAccountDataProvider(deploy pipeline, OpenClaw paths, and account service all load via it), reusing the existingIVariableEncryptionServiceV2 envelope — mirroring howVariableDataProviderprotects theVariabletable.SQUID_ENCRYPTED_V2:prefix — a value lacking it is returned verbatim, so pre-existing plaintext rows keep working and upgrade lazily on next save. No schema change, no backfill. Encrypt-on-write is idempotent (IsValidEncryptedValueguard) so a re-save never double-wraps.GetAccountByIdAsyncis switched from trackingFindAsyncto an AsNoTracking query for the same reason.MapToDtobuilds the response summary from the plaintext credentials the service already holds (the entity carries ciphertext after a write).Operational prerequisite: reuses the already-configured
Security:VariableEncryption:MasterKey— no new key source, no hardcoded default. With an unset MasterKey the envelope is cosmetic (key derived from empty input); real protection requires an operator-set key +SQUID_MASTER_KEY_ENFORCEMENT=strict. Key-lifecycle hardening (forcing a non-empty key in strict mode, platform-rooted derivation) is tracked separately under the SOTA-audit P5.Test plan
DeploymentAccountCredentialsEncryptionTests, 14): every account type's serialized credentials round-trips through the V2 envelope with the secret intact + never verbatim; legacy plaintext reads back verbatim (read-both); already-encrypted detection (no double-wrap); tampered ciphertext rejected by the GCM tag.IntegrationAccountCredentialsAtRest, 3): a written account's RAW DB column carries theSQUID_ENCRYPTED_V2:prefix and never the cleartext secret; reads back decrypted through the provider; a hand-inserted plaintext row reads back verbatim (read-both); update re-encrypts.DeploymentAccountServiceTests,EndpointContextBuilderTests,PrepareTargetsPhaseTests, OpenClaw) — the MapToDto refactor + provider change are non-breaking.GetAccountByIdAsyncdecrypt seam.