fix: enforce owner-only permissions on credential files#6242
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>
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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 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 |
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.
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>
8a05735 to
035a90d
Compare
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
0o700) and settings file (0o600), using atomic write behavior for safer updates.0o700) after creation, with best-effort permission correction.