fix: enforce owner-only permissions on credential files#6242
fix: enforce owner-only permissions on credential files#6242theCyberTech wants to merge 3 commits into
Conversation
Credentials stored at rest were left world-readable on multi-user hosts:
- TokenManager._get_secure_storage_path() documented its credential dir as
mode 0o700 but created it via mkdir() with default perms (0o755), leaving
the Fernet secret.key and encrypted tokens.enc in a traversable dir.
- Settings.dump() persisted tool_repository_password (plaintext) to
settings.json via open("w"), producing a 0o644 file, and created the
config dir at 0o755 — despite the sibling token_manager already writing
secrets atomically at 0o600.
Fixes:
- TokenManager: chmod the credential dir to 0o700 after mkdir (robust against
umask and pre-existing dirs).
- Settings: write settings.json atomically at 0o600 (mkstemp + chmod +
os.replace) and chmod the dedicated config dir to 0o700. The /tmp and cwd
fallback parents are deliberately not chmod'd; the 0o600 file mode protects
the credential there.
Adds regression tests asserting 0o600 files and 0o700 dirs, and that shared
fallback dirs are not globally tightened.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![corridor-security[bot]](https://avatars.githubusercontent.com/in/1177339?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary: This PR tightens credential-at-rest permissions by enforcing owner-only modes on settings files and credential directories and using atomic JSON writes.
Risk: Low risk. No exploitable security vulnerabilities were identified in the added code; the changes reduce local disclosure risk without introducing a new public attack surface or weakening authentication or authorization controls.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesPOSIX Permission Enforcement for Config and Credential Files
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR hardens credential-at-rest handling by enforcing owner-only permissions for the token credential directory and the persisted settings file, aligning behavior with the existing secure-write approach used elsewhere in the codebase.
Changes:
- Enforce
0o700on the TokenManager credential directory after creation (umask- and pre-existing-dir-safe). - Write
settings.jsonatomically and enforce0o600for the file and0o700for the dedicated config directory (while avoiding chmod on shared fallback dirs). - Add regression tests asserting the restrictive modes and ensuring shared fallback directories aren’t chmod’d.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/crewai-core/src/crewai_core/token_manager.py | Tightens credential directory permissions to match documented 0o700. |
| lib/crewai-core/src/crewai_core/settings.py | Adds secure directory-mode enforcement and atomic 0o600 settings writes. |
| lib/cli/tests/test_token_manager.py | Adds permission regression tests for TokenManager storage directory. |
| lib/cli/tests/test_config.py | Adds permission regression tests for settings file/dir and shared fallback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _write_secure_json(path: Path, data: dict[str, Any]) -> None: | ||
| """Atomically write ``data`` as JSON to ``path`` with owner-only (0o600) mode.""" | ||
| fd, tmp = tempfile.mkstemp(dir=path.parent, prefix=f".{path.name}.") | ||
| try: | ||
| with os.fdopen(fd, "w") as f: | ||
| json.dump(data, f, indent=4) | ||
| os.chmod(tmp, 0o600) | ||
| os.replace(tmp, path) | ||
| except BaseException: | ||
| if os.path.exists(tmp): | ||
| os.unlink(tmp) | ||
| raise |
| class TestSecureStoragePathPermissions(unittest.TestCase): | ||
| """Test that the credential directory is created with restrictive permissions.""" | ||
|
|
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Summary
Two credential-at-rest files were left world-readable on multi-user hosts. Both stem from
mkdir/openusing default (umask-dependent) permissions instead of restricting to the owner. Thetoken_manager.pymodule already writes its secrets atomically at0o600; this brings the rest of the credential surface in line.Findings
TokenManager._get_secure_storage_path()— docstring promises a0o700credential directory, butmkdir(parents=True, exist_ok=True)created it at the default0o755. The dir holds the Fernetsecret.keyand encryptedtokens.enc, so other local users could traverse/list it.Settings.dump()— persiststool_repository_password(plaintext) to~/.config/crewai/settings.jsonviaopen("w"), producing a world-readable0o644file; the config dir was also0o755.Fixes
chmod(0o700)the credential dir aftermkdir(robust against umask and pre-existing dirs).settings.jsonatomically (mkstemp→json.dump→chmod 0o600→os.replace) andchmod(0o700)the dedicated config dir. The/tmpand cwd fallback parents are deliberately not chmod'd — the0o600file mode protects the credential there without tightening a shared directory.Threat model
Local information disclosure. Low impact on a single-user dev laptop; real exposure on shared/multi-user hosts and via the
/tmp/cwd config fallbacks. Clearly a framework defect rather than operator error — the user never introduces the world-readability, and the inconsistency withtoken_manager's existing0o600writes is the tell.Out of scope (follow-up)
The
/tmpand cwd fallbacks inget_writable_config_path()remain. They're far less dangerous now (files are0o600), but silently writing credentials to a predictable/tmppath or cwd is worth revisiting separately.Tests
New regression tests assert
0o600files and0o700dirs, and that shared fallback dirs are not globally tightened. All fail on the pre-fix code and pass after. Full settings/token suite (39 tests) green; ruff + mypy clean.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
0o700) and settings file (0o600), and by writing settings atomically for safer updates.0o700) after creation (best-effort, without blocking token usage).Tests