fix(clients): decrypt client secret on restore (apply-to-new-node bug)#92
Merged
Merged
Conversation
Assigning an (adopted or created) client to a new node failed the rollout with 'apply client failed: bad_request: secret must be exactly 32 hex characters'. The clients Service stores secrets encrypted at rest (secret_ciphertext / PVS2:) and keeps plaintext in the in-memory mirror only transiently after Save/SaveState/adopt. No read path decrypted: Service.Restore (run at startup / mirror reload) loaded the ciphertext straight into the mirror, and decryptSecret was never called anywhere. After a panel restart every client's in-memory secret was the PVS2: ciphertext, so the next client-apply job shipped that ciphertext to telemt, which rejected it. Adopt merely exposed it: adopted clients matched telemt's existing config on the original nodes (no real apply), so the first true apply was the new-node assignment. Secondary corruption: editing such a client (updateClient -> SaveState) re-encrypted the already-ciphertext secret into PVS2:PVS2:… in the DB. Fix: - Restore now decrypts each secret into the mirror via decryptSecretFully, which peels every layer so double-wrapped rows self-heal on next load. - encryptSecret is idempotent: it never re-encrypts an already vault-prefixed value, closing the double-wrap mechanism on all save paths. Tests: reproduce the restore-without-decrypt failure, the double-wrap heal, and encrypt idempotency. Full clients + server suites pass. Operator remediation: deploy + restart the panel — Restore heals the mirror (incl. any double-wrapped row); the next save rewrites the DB row single-encrypted.
|
There was a problem hiding this comment.
Pull request overview
Fixes a bug where client secrets stored encrypted (PVS2:) were loaded into the in-memory mirror as ciphertext on panel restart, causing client-apply jobs to ship ciphertext to telemt and fail validation. Also addresses secondary double-encryption corruption from saves that ran with a stale ciphertext mirror.
Changes:
Service.Restorenow decrypts each client secret into the mirror via a newdecryptSecretFullyhelper that peels multiple encryption layers to heal double-wrapped rows.encryptSecretis made idempotent — returns vault-prefixed inputs unchanged rather than re-encrypting them.- Adds tests for restore decryption, double-wrap self-healing, and encrypt idempotency.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/controlplane/clients/service.go | Decrypt secrets in Restore, add decryptSecretFully, make encryptSecret idempotent. |
| internal/controlplane/clients/encryption_test.go | New tests covering restore decrypt, double-wrap heal, and idempotent encrypt. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Bug
Assigning an existing (adopted or created) client to a newly added node fails the rollout with:
apply client failed: bad_request: secret must be exactly 32 hex characters(the error comes from telemt — the agent shipped a non-32-hex secret).Root cause
The clients Service encrypts secrets at rest (
secret_ciphertext/PVS2:) and keeps plaintext in the in-memory mirror only transiently after Save/SaveState/adopt. No read path decrypts —Service.Restore(startup / mirror reload) loads the ciphertext straight into the mirror anddecryptSecretwas never called anywhere. After a panel restart, every client's in-memory secret is thePVS2:ciphertext, so the next client-apply job ships ciphertext to telemt → rejected.Adopt only exposed it: adopted clients matched telemt's existing config on the original nodes (no real apply happened), so the first true apply was the new-node assignment.
Secondary corruption: editing such a client (
updateClient → SaveState) re-encrypted the already-ciphertext secret intoPVS2:PVS2:…in the DB.Fix
Restoredecrypts each secret into the mirror viadecryptSecretFully, which peels every layer so double-wrapped rows self-heal on next load.encryptSecretis now idempotent — never re-encrypts an already vault-prefixed value, closing the double-wrap mechanism on all save paths.Tests
Reproduce the restore-without-decrypt failure, the double-wrap heal, and encrypt idempotency. Full
clients+serversuites pass;go vetclean.Operator remediation
Deploy + restart the panel —
Restoreheals the mirror (incl. any double-wrapped row); the next save rewrites the DB row single-encrypted. No manual secret reset required.