Encrypt SSH proxy password at rest#446
Merged
Merged
Conversation
The SSH proxy password lived as plaintext inside the machine endpoint JSON column. Encrypt it at rest via the shared AtRestSecretProtector, at the two write seams (register + update) and decrypt at the two read seams (the deploy variable contributor + the health-check connection builder). The read seams are synchronous (IEndpointVariableContributor.ContributeVariables is sync), and decrypt is pure CPU work, so add a synchronous Decrypt to IVariableEncryptionService (the already-sync core of DecryptAsync, which now delegates to it) and a synchronous Unprotect to IAtRestSecretProtector. This keeps the contributor interface sync — no async ripple across every transport. Non-breaking: - ProxyPassword is write-only from the API (no GET/response DTO returns it), so encrypting at rest changes nothing a client observes. - Read-both: a legacy plaintext proxy password in an existing machine's endpoint passes through Unprotect verbatim — no migration. - The protector is an optional ctor param on all four seams (register, update, contributor, health check); DI injects the real one in production, and any test that constructs them without it degrades to the pre-feature plaintext path. Encrypt is idempotent so a re-save (or the unchanged value carried through an update merge) never double-wraps. Tests: - Unit: contributor decrypts an envelope before contributing the SSH connection variable; legacy plaintext passes through; no-protector leaves the value untouched. - Integration (real Postgres + real DI + real AES-256-GCM): a proxy password encrypted via the real protector and stored in a machine's endpoint jsonb is decrypted by the DI-resolved contributor (proves the protector is wired into the read seam); legacy plaintext reads back verbatim. - Full unit suite (6048) green.
…w blockers) Adversarial review found the write-encrypt seams and the health-check decrypt seam had zero test coverage — a regression dropping Protect()/Unprotect() would silently leak plaintext (or break proxy auth) with a green suite. Production code was verified correct; these close the coverage gaps: - Register: RegisterSshAsync with the real protector persists an encrypted envelope (not plaintext) for the proxy password. - Update (MergeSsh): a new plaintext password is encrypted; an unrelated edit does NOT re-wrap an already-encrypted value (idempotency — a double-wrap would break the single read-side Unprotect); a legacy plaintext is opportunistically migrated to encrypted on an unrelated edit. - Health check: the connection builder decrypts the at-rest proxy password before building the SSH connection. All drive the real AtRestSecretProtector (the default-ctor services have no protector, so these are the only tests exercising the encrypt/decrypt branches). Full unit suite (6053) 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
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
AtRestSecretProtector— at the two write seams (MachineRegistrationServiceregister +MachineService.MergeSshupdate) and decrypt at the two read seams (SshEndpointVariableContributordeploy path +SshHealthCheckStrategyconnection builder).IEndpointVariableContributor.ContributeVariables), and decrypt is pure CPU work, so add a synchronousDecrypttoIVariableEncryptionService(the already-sync core ofDecryptAsync, which now delegates to it) and a synchronousUnprotecttoIAtRestSecretProtector. This keeps the contributor interface sync — no async ripple across every transport's contributor.Second P2 at-rest finisher (after
AtRestSecretProtectorextraction #445). Remaining: Tentacle private key (agent-side, separate mechanism).Non-breaking
Machine.EndpointJSON returned by machine GET/list — previously as plaintext, now as the encrypted envelope (a net improvement). No consumer round-trips it (SquidWeb has zero ProxyPassword references; updates send changed fields only).Unprotectverbatim — no migration.Test plan
SshProxyPasswordAtRestTests): contributor decrypts an envelope before contributing the SSH connection variable; legacy plaintext passes through; no-protector leaves the value untouched.IntegrationSshProxyPasswordAtRest): a proxy password encrypted via the real protector and stored in a machine's endpoint jsonb is decrypted by the DI-resolved contributor (proves the protector is wired into the read seam, raw column carries theSQUID_ENCRYPTED_V2:envelope not cleartext); legacy plaintext reads back verbatim.