Repository navigation
fix(credentials): create the auth profile store owner-only - #5795
Conversation
tinysweeper
Bot
added
the
priority: p3
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
371aa12 to
46e31e2
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe credential profile save path now writes temporary files with owner-only permissions on Unix. Tests cover store permissions, temporary-file repair, truncation, and cross-platform test behavior. ChangesCredential store hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change tightens credential-store file permissions without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation All changes support the linked issue. The helper, temporary-file integration, Unix permission tests, truncation tests, and end-to-end store test are directly related to credential-store permission hardening. Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
`fs::write` creates at `0o666 & ~umask` — 0644 under the usual 022 — and `fs::rename` carries the source mode onto the destination, so the credential store inherited it on every save. Any process running under a different UID could read it, and on the encrypted-JSON path — the normal state on Linux and on any headless install without a keyring — that file is OAuth token ciphertext, leaving `.secret_key` as the only remaining control. Same shape as the fix tinyhumansai#2360 landed for the secret key in `keyring/encrypted_store.rs`: create with the mode already set rather than widening then narrowing, so the file is never briefly world-readable. The explicit `set_permissions` covers a leftover tmp from an interrupted save, which `.mode()` alone would not, since it only applies at creation. Rebased onto main's split layout: `write_owner_only` sits in profiles.rs, the call site moved to profiles_impl_01_part_02.rs, and the tests are a sibling file — the layout gate rejects inline `#[cfg(test)] mod` blocks. The added `a_saved_store_file_is_owner_only` goes through `upsert_profile` rather than calling the helper, because the three helper tests cannot see the wiring: swapping the call site back to `fs::write` leaves all three green. It compiles on every platform so the store API usage is type-checked everywhere; only the mode assertion is unix-gated.
46e31e2 to
1ba1982
Compare
|
Maintainer review (no changes pushed — review only). Assessment: this looks ready.
I confirmed the premise against One adjacent thing for a follow-up, explicitly not for this PR. The same pattern is still live one file over: That is another credential tmp written at
|
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Approved after a maintainer-side verification pass.
Verified on the current head: MERGEABLE against main, zero failing and zero pending required checks, and no unresolved, non-outdated review threads.
This is one of two required approvals; a second maintainer review is still needed before merge.
…and the e2e gate Three behaviours merged in the last week had no test that would fail if the fix were reverted. Each test below drives the changed code path and was revert-checked against the specific hunk it covers. - tinyhumansai#5795 — the auth profile store is written owner-only. Driven through `auth.store_provider_credentials` rather than `AuthProfilesStore`, so the RPC path acquiring a different writer is caught too. The assertion is umask-sensitive by nature, so the test probes the ambient umask first and says so rather than reporting a pass it did not earn. - tinyhumansai#5947 — `validate_only` is a dry run for the stt workload, not only tts. A reserved slug is used because it is the one dry-run answer fully determined without a network call. - tinyhumansai#5936 — the domain e2e coverage gate itself. Its only prior test asserted `--help` output, so all three defects it fixed were unpinned. Fixture-driven cover for part-file discovery, the empty-namespace failure, and measuring a namespace MODULES does not name. Refs tinyhumansai#5795, tinyhumansai#5947, tinyhumansai#5936
…and the e2e gate Three behaviours merged in the last week had no test that would fail if the fix were reverted. Each test below drives the changed code path and was revert-checked against the specific hunk it covers. - tinyhumansai#5795 — the auth profile store is written owner-only. Driven through `auth.store_provider_credentials` rather than `AuthProfilesStore`, so the RPC path acquiring a different writer is caught too. The assertion is umask-sensitive by nature, so the test probes the ambient umask first and says so rather than reporting a pass it did not earn. - tinyhumansai#5947 — `validate_only` is a dry run for the stt workload, not only tts. A reserved slug is used because it is the one dry-run answer fully determined without a network call. - tinyhumansai#5936 — the domain e2e coverage gate itself. Its only prior test asserted `--help` output, so all three defects it fixed were unpinned. Fixture-driven cover for part-file discovery, the empty-namespace failure, and measuring a namespace MODULES does not name. Refs tinyhumansai#5795, tinyhumansai#5947, tinyhumansai#5936
…and the e2e gate Three behaviours merged in the last week had no test that would fail if the fix were reverted. Each test below drives the changed code path and was revert-checked against the specific hunk it covers. - tinyhumansai#5795 — the auth profile store is written owner-only. Driven through `auth.store_provider_credentials` rather than `AuthProfilesStore`, so the RPC path acquiring a different writer is caught too. The assertion is umask-sensitive by nature, so the test probes the ambient umask first and says so rather than reporting a pass it did not earn. - tinyhumansai#5947 — `validate_only` is a dry run for the stt workload, not only tts. A reserved slug is used because it is the one dry-run answer fully determined without a network call. - tinyhumansai#5936 — the domain e2e coverage gate itself. Its only prior test asserted `--help` output, so all three defects it fixed were unpinned. Fixture-driven cover for part-file discovery, the empty-namespace failure, and measuring a namespace MODULES does not name. Refs tinyhumansai#5795, tinyhumansai#5947, tinyhumansai#5936
…al-store-permissions\n\nfix(credentials): create the auth profile store owner-only\n
…and the e2e gate\n\nThree behaviours merged in the last week had no test that would fail if the\nfix were reverted. Each test below drives the changed code path and was\nrevert-checked against the specific hunk it covers.\n\n- tinyhumansai#5795 — the auth profile store is written owner-only. Driven through\n `auth.store_provider_credentials` rather than `AuthProfilesStore`, so the\n RPC path acquiring a different writer is caught too. The assertion is\n umask-sensitive by nature, so the test probes the ambient umask first and\n says so rather than reporting a pass it did not earn.\n- tinyhumansai#5947 — `validate_only` is a dry run for the stt workload, not only tts.\n A reserved slug is used because it is the one dry-run answer fully\n determined without a network call.\n- tinyhumansai#5936 — the domain e2e coverage gate itself. Its only prior test asserted\n `--help` output, so all three defects it fixed were unpinned. Fixture-driven\n cover for part-file discovery, the empty-namespace failure, and measuring a\n namespace MODULES does not name.\n\nRefs tinyhumansai#5795, tinyhumansai#5947, tinyhumansai#5936\n
2 participants
Closes #5724.
The defect
write_persisted_lockedwrote the credential store withfs::write, which creates at0o666 & ~umask—0644under the usual022— and thenfs::renamed it into place.renamecarries the source file's mode onto the destination, soauth-profiles.jsonwas world-readable after every save.The mode was never a decision.
grep -rn "set_permissions\|PermissionsExt\|from_mode" src/openhuman/security/credentials/returns nothing: there was no hardening anywhere in the module, and no runtime repair either.What that exposes, and where it actually matters
Not on every install, and I would rather be precise than reach for a severity label:
enc2:ciphertextRow 2 is the normal state on Linux and on any server install without a working keyring. There, confidentiality rested entirely on
.secret_keybeing0600— defence-in-depth collapsing to a single control on exactly the installs least likely to have a keyring.The fix
write_owner_only()creates the file with0o600already set rather than widening then narrowing — the same shape #2360 landed for the secret key inkeyring/encrypted_store.rs:330, whose comment names the goal: "to avoid a TOCTOU race where the file is briefly world-readable".It also chmods explicitly after opening.
.mode()applies only when the file is created, so an interrupted save that left a0644tmp behind would have it reused as-is by the next save — andrenamewould carry that mode straight through to the store. That is the failure this fix would otherwise still have.Windows keeps
fs::write: there is no mode to set, and the file inherits the ACL of the per-user profile directory.Tests
Three cases in a
#[cfg(all(test, unix))]module:a_new_credential_file_is_owner_only— mode is0600, not0644;a_leftover_world_readable_tmp_is_repaired— seeds a0644tmp, asserts the precondition, then asserts the write narrows it. This is the case.mode()alone does not cover;the_contents_are_written_and_truncated— a shorter payload must not leave stale bytes behind, sinceOpenOptionsreplacesfs::write's implicit truncate.Verification note
cargo check -p openhuman --lib→ exit 0, no diagnostic inprofiles.rs;cargo fmtapplied.I could not run the tests: they are
unix-gated and this is a Windows box, where that module does not even compile in. So they are reviewed-by-eye here and will first execute on CI. If you would rather I verify them on Linux before you spend review time, say so and I will find a way to.Not done here
Existing installs stay
0644until their next save, since this fixes the write path rather than repairing on load. #5635 does repairconfig.toml's mode on load; adding the same for the credential store is a reasonable follow-up but a different change, and I did not want to widen this one without asking.Summary by CodeRabbit
Security
Bug Fixes
Tests