Derive secrets encryption key with Argon2id - #6657
Conversation
The encrypted secrets provider derived its AES-256-GCM key from the user's password with a single unsalted SHA-256. That password is human-chosen and doubles as a recovery credential, so an attacker holding a copy of the secrets file could try billions of guesses per second against it. The file travels in ways the OS keyring does not — backups, disk images, synced home directories — which is exactly the case this protects. Derive the key with Argon2id and a per-file random salt instead. The salt goes in a new file header; the cost parameters stay in code, selected by the header's format version, so nothing attacker-controllable reaches Argon2id's memory allocation. Files written before the header existed are detected by the absent magic prefix, read with the old key, and rewritten on open. Migrating on open rather than on the next write matters: a file that is only ever read would otherwise keep the weaker derivation indefinitely, which is the common case for workload credential injection. Derivation is memoised for the process. A provider is constructed for nearly every operation that touches a secret, including once per API request, and Argon2id is deliberately expensive — without the memo the memory hardness that protects the file at rest would become a denial of service vector against the server. A thv binary predating the header cannot read a migrated file. Fixes CodeQL alert #105 (go/weak-sensitive-data-hashing) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6657 +/- ##
==========================================
+ Coverage 78.92% 78.95% +0.02%
==========================================
Files 782 783 +1
Lines 78065 78135 +70
==========================================
+ Hits 61612 61689 +77
+ Misses 16448 16441 -7
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JAORMX
left a comment
There was a problem hiding this comment.
Reviewed 1449758 with a multi-agent panel, including a GPT-6-Astra reviewer focused on the security context and local single-user threat model. No practical merge-blocking security defect found. The inline comments are non-blocking upgrade/documentation and compatibility concerns.
Security context
The KDF improvement is worthwhile, but the rationale should distinguish two cases. Interactive CLI setup uses a human-chosen password, whereas API setup without a supplied password generates 32 random bytes (256 bits of entropy; pkg/api/v1/secrets.go:143-158, pkg/secrets/factory.go:427-438). Cheap offline guessing is a real concern for the former if someone obtains only the encrypted file. It is not a practical guessing weakness for the generated-password path.
For a local single-user deployment, this is conditional, moderate-risk at-rest hardening, not a new defence against an attacker who already controls the local account, keyring, or process. File-only disclosure through backups or synced directories still makes the improvement useful. The blanket statement that the password is human-chosen should be qualified, as should the unbenchmarked billions-of-guesses-per-second claim.
Implementation
The fixed Argon2id parameters, random per-file salt, authentication before migration, re-read under the file lock, atomic replacement, and bounded password-aware derivation memo look sound. Deferring higher costs and a separate keyring migration is reasonable. The remaining CodeQL comment is on the intentional legacy SHA-256 reader, not the new derivation path.
User impact
The most important follow-up is actionable upgrade/recovery guidance: opening the store converts it, and older local ToolHive processes can then fail secret access or OAuth token persistence. The PR's mixed-version thv-proxyrunner example should instead refer to local thv processes; the normal Kubernetes path uses Kubernetes Secrets rather than this XDG store. No downgrade implementation is requested.
Verification
All checks at the reviewed head passed, with the optional Claude action skipped. No local tests or linters were run. Useful non-blocking coverage additions are a failed migration that preserves the legacy ciphertext and a concurrent cache/constructor test.
Recommendation: keep the KDF improvement, qualify the security rationale, and strengthen the user-facing upgrade/recovery notes before release.
| printSecretsFileHint(filePath, err) | ||
| return nil, err | ||
| } | ||
| manager.migrateLegacyFile() |
There was a problem hiding this comment.
Non-blocking: document the mixed-version restart sequence. This migrates on provider construction, including read-only operations. If an older thv serve or detached local proxy is still running when a newly installed CLI first opens this file, subsequent secret accesses from the old process fail. The API constructs a provider per request (pkg/api/v1/secrets.go:511-525); a remote OAuth proxy can retain its active in-memory session but fail later refresh-token persistence (pkg/runner/runner.go:869-903). Already-injected container environment variables are unaffected.
The no-downgrade tradeoff is documented and does not need a compatibility switch. Could the user-facing upgrade/release notes explicitly tell users to stop older local API servers and secret-using proxies before first access with the new version, then restart them with the new binary? The constructor godoc should also disclose that opening an existing store can rewrite it and that migration is best effort.
There was a problem hiding this comment.
Done in 143d9f8.
The constructor godoc now states that opening an existing store can rewrite it and that the rewrite is best effort.
docs/arch/04-secrets-management.md gains an Upgrading section saying exactly this: migration happens on open including for read-only operations, so stop older local ToolHive processes before the first access with the new version and restart them on the new binary; already-injected container env vars are unaffected; and it does not apply to the Kubernetes path.
You were right about the thv-proxyrunner example being wrong — cmd/thv-proxyrunner/ has no pkg/secrets import at all, so it was never the risk. The PR body now refers to older local thv processes and cites the refresh-token persister at pkg/runner/runner.go:869-903 instead.
| opened — migration is transparent and requires no user action. It is best effort: | ||
| if the rewrite fails (read-only filesystem, full disk) the file stays readable in | ||
| the legacy format. A `thv` binary predating the framed format cannot read a | ||
| migrated file. |
There was a problem hiding this comment.
Non-blocking: add rollback and backup recovery guidance. The incompatibility warning is useful, but users also need to know that an older binary's incorrect-password hint does not establish corruption: resetting the keyring or deleting a valid migrated store is not the first recovery step. Returning to a compatible binary is the straightforward recovery; rollback to an older binary needs a securely retained pre-migration copy and its corresponding password.
Also clarify the security boundary: this upgrades only the live file. Historical legacy backups remain cheaply offline-verifiable, and recovering the unchanged password from one also permits decrypting a newer copy. A migrated-file backup alone does not replace the need to retain the password/keyring for recovery. No automatic backup creation or downgrade implementation is being requested.
There was a problem hiding this comment.
Done in 143d9f8. Two new sections in docs/arch/04-secrets-management.md.
Recovering from a rollback states that the older binary's incorrect-password hint predates this format and does not indicate corruption, that resetting the keyring or deleting the store is the wrong first step and loses secrets, and that the straightforward fix is returning to a binary that understands the framed format — with a real rollback needing a securely retained pre-migration copy plus its password.
What migration does and does not protect covers the boundary you identified, which I had not thought through: only the live file is upgraded, pre-migration backups keep the unsalted derivation and stay cheaply crackable, and since the password is unchanged, recovering it from an old backup also decrypts the migrated file — so retiring old backups matters as much as the upgrade. It also notes the converse, that a migrated-file backup is not a substitute for retaining the password or keyring entry.
No automatic backup creation or downgrade implementation added.
| // | ||
| // The manager takes the password rather than a key because the key derivation | ||
| // salt lives in the secrets file itself and is only known once the file is read. | ||
| func NewEncryptedManager(filePath string, password []byte) (Provider, error) { |
There was a problem hiding this comment.
Non-blocking compatibility note: the exported signature still accepts []byte, but its meaning changes from an AES key to a password. A direct consumer that previously passed sha256(password) still compiles, yet legacy decryption now hashes that key again and fails. The in-repository production factory is updated, so this is not an established in-tree regression, and I found no compatibility commitment or downstream caller that makes it a blocker.
Please call out this semantic change for direct Go consumers if pkg/secrets is a supported integration surface. No speculative legacy adapter is requested; the key and password cannot reliably be distinguished from the argument's type or length.
There was a problem hiding this comment.
Called out in 143d9f8. The NewEncryptedManager godoc now opens with:
passwordis the user's password, not a derived key. Callers that previously passedsha256(password)still compile against this signature but will fail to decrypt, because that value is now hashed again as if it were a password.
On whether pkg/secrets is a supported integration surface: CONTRIBUTING.md:167 scopes the stability commitment to cmd/thv-operator/api/v1beta1/ only, so there is no commitment here and no api-break-allowed label needed. In-tree, pkg/secrets/factory.go is the only production caller and it is updated.
One option I did not take, since you did not ask for it and it widens the diff: renaming to NewEncryptedManagerFromPassword would turn this from a silent runtime decryption failure into a compile error for any external consumer. You are right that key and password are indistinguishable by type or length, which is what makes the silent path possible. Happy to make that change if you would prefer a loud break over a documented one.
Review feedback on the Argon2id migration. The constructor's godoc did not say that opening an existing store can rewrite it, nor that the password argument is no longer a derived key — a caller still passing sha256(password) compiles and then fails to decrypt. The architecture doc gains the operational guidance the change actually needs: stop older local thv processes before the first access with a new binary, since migration happens on open and an older process loses secret access afterwards; an older binary's "password is incorrect" hint does not mean corruption, so resetting the keyring is the wrong first move; and migration upgrades only the live file, so pre-migration backups stay cheaply crackable and the password recovered from one still opens the migrated file. Add the two suggested tests: a failed migration leaves the legacy ciphertext readable and untouched, and concurrent derivations through the memo agree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Merging based on Ozz's approval from earlier |
Summary
Why: The encrypted secrets provider derived its AES-256-GCM key from the password with a single unsalted
sha256.Sum256, so guessing a password against a stolensecrets_encryptedcost one hash per attempt.This matters for the interactive path, where the password is human-chosen and doubles as a recovery credential (
thv secret reset-keyringtells users to remember it). It does not meaningfully matter for the API setup path, which generates 32 random bytes when no password is supplied (pkg/api/v1/secrets.go:152-158) — 256 bits of entropy is out of reach regardless of the KDF.Scope it honestly: for a local single-user install this is at-rest hardening against file-only disclosure — backups, disk images, synced home directories, which travel in ways the OS keyring does not. It is not a defence against an attacker who already has the local account, the keyring, or the process.
Measured on one machine, same hardware for both: SHA-256
36.2 ns, Argon2id at these parameters15.4 ms— a ~427,000× increase in work per guess. The larger effect is memory hardness: each guess now needs 19 MiB, which is what denies an attacker cheap GPU parallelism. (Absolute cracking rates are hardware-specific and not benchmarked here.)What: Derive the key with Argon2id (OWASP minimum: 19 MiB, 2 iterations, 1 lane) over a per-file 16-byte random salt. The salt goes in a new file header; the cost parameters stay in code, selected by the header's version.
What: Pre-existing files are detected by the absent magic prefix, read with the old key, and rewritten on open. Migration is transparent and best effort — if the rewrite fails, the file stays readable in the legacy format.
What: Derivation is memoised for the process lifetime.
Fixes CodeQL alert #105
go/weak-sensitive-data-hashing.Type of change
Test plan
task test)task test-e2e)task lint-fix)task testpasses with-race, exit 0, no failures. New coverage inpkg/secrets:aes.Encryptundersha256.Sum256(password), written raw with no header) is still readable — the guard against destroying existing users' secrets.ErrMalformedSecretsFileand never panic.task lint-fixcould not be run locally — it fails repo-wide (including on cleanmain) because the installed golangci-lint's type-checker cannot read Go 1.27 export data:pkg/security/security.go:7:8: could not import crypto/subtle (... export data version 4 is greater than maximum supported version 2). That file is untouched here. Relying on CI'sLinting / Lint Go Codejob.Changes
pkg/secrets/kdf.gopkg/secrets/encrypted.gopkg/secrets/factory.goNewEncryptedManagerinstead of a pre-derived keydocs/arch/04-secrets-management.mdDoes this introduce a user-facing change?
Yes, in two ways — both invisible in the normal case.
thvbinary predating this change cannot read an upgraded file. Downgrade support was deliberately not implemented. Worth knowing: an older binary hitting an upgraded file prints the existing "the password is incorrect… try again with your original password" hint, which is misleading — we can't change what already-released binaries print, but this PR adds a distinct, accurate message for the equivalent case in the new binary. This also affects other local processes still on an older binary: migration happens when the store is opened, including by read-only commands, so an olderthv serveor a detached proxy that persists OAuth refresh tokens (pkg/runner/runner.go:869-903) loses secret access once a newer binary touches the file. Stop older local ToolHive processes before the first access with the new version, then restart them on the new binary; secrets already injected into running containers are unaffected. This does not apply to the Kubernetes path, which uses Kubernetes Secrets rather than this XDG store.Special notes for reviewers
Cost parameters are in code, not in the file. The obvious design stores them in the header so they can be raised later. I tried that first and backed it out: it puts an attacker-controllable value directly into Argon2id's
make([]block, memory), and it turned out not to deliver the flexibility it promised — a file that already has parameters keeps them forever, so a future defaults bump would be a no-op for exactly the users who already have secrets. Keying parameters off the format version makes an increase an explicit0x02, and removes the entire bounds-checking surface.Why derivation is memoised.
secrets.CreateProvideris called from 15 sites — twice back-to-back in a singlethv run(runner.go:297,301) and once per HTTP request in the API server (api/v1/secrets.go:525). Measured cost is ~16 ms and 19 MiB per derivation, so without the memo a few hundred concurrent requests to the secrets endpoints would pin gigabytes. The memo holds its lock across the derivation on purpose: concurrent callers wait for the first result instead of each allocating 19 MiB. It is matched on the password as well as the path and salt, so a second password derives its own key rather than being handed the first one — and the password is compared, never hashed, since a fast hash of a password is the thing this PR exists to remove.Deliberately out of scope, both follow-ups:
Generated with Claude Code