Encrypt Tentacle certificate password and subscription id at rest#449
Merged
Conversation
The Tentacle's at-rest encryption (machine-key AES-256-GCM protecting the certificate password, which guards the PFX private key, and the polling subscription id) was implemented and unit-tested in TentacleCertificateManager but never wired: the runtime and all five CLI commands constructed the encryptor-less overload, so production agents wrote both secrets in plaintext. Add ProductionTentacleCertificateManager as the single construction site that wires the machine-key encryptor (degrading to the legacy plaintext manager only if key derivation fails, so it never blocks startup) and route the runtime + register/show-thumbprint/show-config/check-services/new-certificate through it. Routing every site through one factory keeps the write and read paths in agreement on whether the cert password is encrypted. Non-breaking: existing plaintext installs migrate on first encryptor-aware load (TentacleCertificateManager already writes the existing secrets back encrypted), so no data migration is needed.
Adversarial review of the at-rest wiring surfaced two issues:
1. Machine-id drift bricked the agent. Wiring a real encryptor in production
made the PFX password random+encrypted, so a host machine-id change (VM
clone, container rebuild, /etc/machine-id reset) left the password
undecryptable: ResolveCertPassword fell back to the wrong legacy password
and LoadPkcs12FromFile threw an UNCAUGHT CryptographicException -> boot
crash-loop, and even register crashed. The subscription-id path was
asymmetric, returning the raw ciphertext AS the identity (corrupt). Now
both paths treat an undecryptable secret as lost identity: regenerate a
fresh cert / subscription id (operator re-registers), the same graceful
recovery on both. Narrowed to CryptographicException so transient IO still
propagates; legacy plaintext migration is unaffected (it never hits the
decrypt-failure branch).
2. No cross-platform guard stopped a future production site reverting to the
encryptor-less constructor (silent plaintext regression, caught only by the
Linux-skipped E2E). Add a source-scan unit test (Rule 12.5) asserting
'new TentacleCertificateManager(' appears in src only inside the factory.
Also: strengthen D5h to prove the encrypted password actually decrypts and
opens the PFX (show-thumbprint reopen), document the threat boundary on the
factory, and note the defensive degrade catch is unreachable under the current
MachineIdProvider contract.
…trip Re-verification of the prior hardening surfaced two low-severity gaps: - The drift guard used a plain substring match, so a reformatted (multi-space) or fully-qualified revert to the bare constructor would evade it. Switch to a whitespace/qualifier-tolerant regex (still requires 'new', so the ctor declaration is never flagged). Red-green confirmed against a fully-qualified injection. - The D5h reopen assertion only checked for any 40-char thumbprint, but the new regenerate-on-decrypt-failure behaviour means show-thumbprint exits 0 with a NEW thumbprint even on a writer/reader password mismatch — the exact failure the assertion claimed to catch. Assert it equals the thumbprint register persisted, making it a true reopen round-trip.
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
TentacleCertificateManager, but never wired: the runtime (TentacleApp) and all five CLI commands (register,show-thumbprint,show-config,check-services,new-certificate) constructed the encryptor-less overload, so production agents wrote both secrets in plaintext on disk.ProductionTentacleCertificateManageras the single construction site that wires the machine-key encryptor, and route all six production sites through it. Routing every site through one factory keeps the write path (register/new-certificate) and the read paths (show-thumbprint, the runtime load) in agreement on whether the cert password is encrypted — otherwise a writer storing a random encrypted password and a reader built without the encryptor would fall back to the legacy fixed password and fail to open the PFX.Non-breaking
TentacleCertificateManageralready writes the existing password / subscription id back in encrypted form (the migration logic predates this PR). No data migration required.TentacleCertificateManager(certsPath)constructor stays for tests and back-compat.Test plan
ProductionTentacleCertificateManagerTests— the factory produces a manager that encrypts the subscription id (v1:envelope +.encryptedmarker) and the cert password at rest, round-trips across reopen, and never returns null.TentacleCertificateManagerEncryptorTests/TentacleCertificateManagerTestsstill green (21/21 cert-manager tests pass)./etc/squid-tentaclefilesystem):D5h_RegisterWritesEncryptedSecretsAtRest— after a realregister, the on-disksubscription-idandtentacle-cert.pfx.pwdarev1:-encrypted. The existingD1hregister→show-thumbprint round-trip guards write/read path agreement.LocalScriptServiceIdempotencyTestsfailures are unrelated (process/state-file tests that also fail onmainon this host).