Repository navigation
Preserve legacy encrypted stores - #6711
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6711 +/- ##
==========================================
- Coverage 79.26% 79.24% -0.03%
==========================================
Files 800 800
Lines 80936 80991 +55
==========================================
+ Hits 64154 64178 +24
- Misses 16777 16808 +31
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
The compatibility direction here makes sense: ordinary access should not silently rewrite shared state into a format that existing processes cannot use. Keeping legacy stores in their observed format is a reasonable immediate containment fix, while new stores continue using Argon2id.
The requirement is continued access, not manual migration for its own sake. Automatic migration is fine where supported existing consumers can still read and write the resulting store. Reading old files with the new binary is not sufficient evidence of backward compatibility, and a confirmation prompt does not make a breaking cutover compatible.
Requesting changes for the safety issue and acceptance-criteria gaps detailed inline:
- The existing lock-file unlink lifecycle permits overlapping cross-process critical sections. With format-preserving writes, that can undo a successfully verified protection upgrade; rollback can also overwrite a concurrent update. The lock defect is inherited, but the upgrade depends on fixing it.
- The failed-upgrade test fails at lock creation before reaching migration. It does not establish verification/rollback behavior.
- The tests do not cover the reported old-process/new-CLI sharing scenario. The legacy-decryption helper and two current-version managers are useful, but do not meet that cross-version requirement.
Please also strengthen the operational guidance before shipping:
- Put the pre-v0.50 compatibility boundary and the instruction to stop/upgrade other installations and running servers, proxies, and workloads in the actual confirmation (
cmd/thv/app/secret.go:397–398), not only in--help. - Explicitly document recovery for stores already automatically converted: preserve the current secrets file and keyring password, upgrade every accessing installation to a format-aware version, and restart old processes. Replacing an executable does not update a running process. Installing this fix alone will not restore a pre-v0.50 process's access to an already-converted store. Do not recommend deleting the store or resetting the keyring for this mismatch.
Preserving legacy compatibility intentionally retains the weaker SHA-256 password derivation until protection is upgraded. That is a real security trade-off, not simply a false-positive warning. Keep already-upgraded stores protected; I would not expand this fix into an automatic downgrade or dual-store workaround.
Reviewed commit 381efe4bbfe15803a7b46db595ddb85baef81dc3. Static source/history and test-source review only. No tests or builds were run; the concurrency finding is statically traced, not dynamically reproduced.
|
Re: finding #3 (no genuine cross-version test) — re-ran the manual cross-version repro against the final code (including the lock-fix from finding #1), this time with real compiled binaries from the actual historical commits, not just current-version managers:
This confirms, against the real pre-fix binaries rather than two current-version managers: the bug reproduces exactly as reported, the fix stops it for ordinary access, the explicit This was run manually (three binaries built from |
381efe4 to
11694b0
Compare
github-actions
Bot
added
size/L
|
Thanks for the thorough review — went through each point:
Ready for another look. |
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
Thanks, Jakub. The revised failure/rollback coverage, the warning in the actual confirmation prompt, and the recovery instructions address those review points. The historical-binary results are useful evidence for the format-compatibility fix; I don't think building old revisions on every CI run needs to block this. Keeping the small manual harness available would be a useful follow-up.
Approving the compatibility fix at 11694b0. It stops ordinary access from silently cutting older consumers off, without deliberately downgrading already-upgraded stores. The new inode check also addresses the normal acquire/release race for upgraded WithFileLock callers.
Non-blocking follow-up: the cleanup paths still need attention. CleanupAll and CleanupStaleLocks still unlock before unlinking (pkg/lockfile/cleanup.go:51–55,180–184). A waiter can acquire and pass the inode check between those steps, then have its lock pathname removed while another process locks a replacement inode. Startup stale-lock cleanup includes the secrets-store directory, so this isn't just an unused helper. Older binaries retaining the old release behavior are another limit on mixed-version concurrency guarantees.
This is an inherited locking problem remaining outside the normal-path fix, rather than a new regression that should hold up stopping silent store conversion. Please track cleanup/lock-lifecycle hardening separately, and avoid describing the current change as eliminating every cross-process locking race.
The outstanding lint and CodeQL findings still need to be cleared or appropriately triaged before merge; this approval does not waive those checks.
Static review only. I inspected the updated code and test sources and read your reported historical-binary results; I did not run tests or builds.
Routine upgrades previously rewrote legacy encrypted stores when opened, breaking older ToolHive processes that shared the same secrets file. Preserve the observed file format for ordinary mutations and require an explicit confirmed protection upgrade for Argon2id migration. The upgrade verifies the replacement and restores the original encrypted bytes when verification fails. Fixes #6710
11694b0 to
ef77788
Compare
|
Fixed the lint failures (gci formatting in the test file, a line over the length limit in For the two CodeQL findings: dismissed both, with the same rationale already applied elsewhere in this repo -
For the cleanup-path follow-up: opened #6713 to track |
github-actions
Bot
added
size/L
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
Re-approving at ef77788. I checked the delta from the previously approved revision: only the two formatting fixes changed, with no behavioral changes.
The failure/rollback coverage, confirmation warning, recovery guidance, and reported historical-binary compatibility checks address the substantive feedback for this fix. The remaining inherited cleanup-lock race is tracked separately in #6713 and remains a non-blocking follow-up.
Lint and CodeQL are now passing; the alert dispositions explicitly record the legacy-hashing trade-off and the path-alert rationale. Remaining required CI checks should pass before merge.
Static review only; no local tests or builds run.
Reorder CleanupAll and CleanupStaleLocks to remove the lock file before unlocking to avoid the race described in the review of stacklok#6711. This prevents the path-to-inode reuse window where a waiter could acquire a stale inode after we release. Addresses stacklok#6713 (follow-up to stacklok#6711).
Summary
Routine secret-store operations previously rewrote legacy encrypted stores to the current on-disk format as a side effect of opening them, silently upgrading the encryption/KDF parameters. This breaks compatibility for older ToolHive processes that share the same secrets file, since they can no longer read the upgraded format.
thv secret upgrade-protection) to migrate a store to Argon2idFixes #6710
Type of change
Test plan
task test)task test-e2e)Changes
pkg/secrets/encrypted.gopkg/secrets/factory.gocmd/thv/app/secret.gothv secret upgrade-protectioncommandpkg/secrets/encrypted_test.gotest/e2e/cli_secrets_scoped_test.godocs/arch/04-secrets-management.md,docs/cli/*Does this introduce a user-facing change?
Yes — adds a new
thv secret upgrade-protectionCLI command. Existing stores are no longer silently rewritten to a newer format on ordinary use.