-
Notifications
You must be signed in to change notification settings - Fork 51
fix: restrict file permissions on auth, config, and log files to prevent local credential theft #244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: restrict file permissions on auth, config, and log files to prevent local credential theft #244
Changes from all commits
154e0d3
3aef7ad
b415dc1
cb185e3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| kind: fixed | ||
| body: Restrict file and directory permissions on auth, config, context, and log paths to prevent local credential exposure on multi-user systems | ||
| time: 2026-06-07T13:40:43+02:00 | ||
| custom: | ||
| Author: iemejia | ||
| AuthorLink: https://github.com/iemejia | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -156,10 +156,21 @@ def log_debug_http_request_exception(e): | |
| def _get_log_file_path(): | ||
| """Create a log file path in the user's log directory.""" | ||
| log_dir = user_log_dir("fabric-cli") | ||
| os.makedirs(log_dir, exist_ok=True) | ||
| os.makedirs(log_dir, mode=0o700, exist_ok=True) | ||
| # Enforce permissions on pre-existing directories from older versions | ||
| _chmod_if_posix(log_dir, 0o700) | ||
| return os.path.join(log_dir, "fabcli_debug.log") | ||
|
Comment on lines
156
to
162
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Added |
||
|
|
||
|
|
||
| def _chmod_if_posix(path, mode): | ||
| """Best-effort chmod with warning on failure; no-op on Windows.""" | ||
| if os.name != "nt": | ||
| try: | ||
| os.chmod(path, mode) | ||
| except OSError: | ||
| pass | ||
|
|
||
|
|
||
| def get_logger(): | ||
| """Singleton logger instance with a single file handler.""" | ||
| global _logger_instance, log_file_path | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,16 +2,31 @@ | |
| # Licensed under the MIT License. | ||
|
|
||
| import json | ||
| import logging | ||
| import os | ||
| from os.path import exists, expanduser | ||
| from os.path import expanduser | ||
|
|
||
| from fabric_cli.core import fab_constant | ||
|
|
||
| _logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _chmod_if_posix(path, mode): | ||
| """Best-effort chmod with warning on failure; no-op on Windows.""" | ||
| if os.name != "nt": | ||
| try: | ||
| os.chmod(path, mode) | ||
| except OSError as e: | ||
| _logger.warning( | ||
| "Failed to set permissions %o on %s: %s", mode, path, e | ||
| ) | ||
|
|
||
|
|
||
| def config_location(): | ||
| _location = expanduser("~/.config/fab/") | ||
| if not exists(_location): | ||
| os.makedirs(_location) | ||
| os.makedirs(_location, mode=0o700, exist_ok=True) | ||
| # Enforce permissions on pre-existing directories from older versions | ||
| _chmod_if_posix(_location, 0o700) | ||
| return _location | ||
|
Comment on lines
25
to
30
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Changed to |
||
|
|
||
|
|
||
|
|
@@ -29,8 +44,29 @@ def read_config(file_path) -> dict: | |
|
|
||
|
|
||
| def write_config(data): | ||
| with open(config_file, "w") as file: | ||
| json.dump(data, file, indent=4) | ||
| _write_restricted_file(config_file, json.dumps(data, indent=4)) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Added |
||
|
|
||
|
|
||
| def _write_restricted_file(file_path, content): | ||
| """Write content to a file with owner-only permissions (0o600). | ||
|
|
||
| Handles both new file creation and tightening permissions on | ||
| pre-existing files from older CLI versions. | ||
| """ | ||
| fd = os.open(file_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) | ||
| try: | ||
| with os.fdopen(fd, "w") as file: | ||
| file.write(content) | ||
| except Exception: | ||
| # os.fdopen may fail before wrapping fd; close to avoid leak. | ||
| # If os.fdopen succeeded, fd is already closed by the with block. | ||
| try: | ||
| os.close(fd) | ||
| except OSError: | ||
| pass | ||
| raise | ||
|
Comment on lines
+50
to
+67
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
| # Enforce permissions on pre-existing files from older versions | ||
| _chmod_if_posix(file_path, 0o600) | ||
|
|
||
|
|
||
| def set_config(key, value): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,8 @@ | |
| import os | ||
| import tempfile | ||
|
|
||
| import pytest | ||
|
|
||
| import fabric_cli.core.fab_state_config as cfg | ||
| from fabric_cli.core import fab_constant | ||
|
|
||
|
|
@@ -165,3 +167,85 @@ def test_init_defaults_preserves_user_overrides_success(monkeypatch, tmp_path): | |
| assert result[fab_constant.FAB_CACHE_ENABLED] == "false" | ||
|
|
||
| # endregion | ||
|
|
||
|
|
||
| # region security: file permission tests | ||
|
|
||
| _skip_on_windows = pytest.mark.skipif( | ||
| os.name == "nt", reason="POSIX permission tests not applicable on Windows" | ||
| ) | ||
|
|
||
|
|
||
| @_skip_on_windows | ||
| def test_config_location_creates_directory_with_restricted_permissions( | ||
| monkeypatch, tmp_path | ||
| ): | ||
| """Verify config directory is created with mode 0o700 (owner-only access).""" | ||
| monkeypatch.setattr( | ||
| "fabric_cli.core.fab_state_config.expanduser", | ||
| lambda path: path.replace("~", str(tmp_path)), | ||
| ) | ||
|
|
||
| location = cfg.config_location() | ||
| assert os.path.isdir(location) | ||
|
Comment on lines
+180
to
+190
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Test now patches |
||
|
|
||
| mode = oct(os.stat(location).st_mode & 0o777) | ||
| assert mode == "0o700", f"Config directory has mode {mode}, expected 0o700" | ||
|
|
||
|
|
||
| @_skip_on_windows | ||
| def test_config_location_tightens_permissions_on_existing_directory( | ||
| monkeypatch, tmp_path | ||
| ): | ||
| """Verify config_location() enforces 0o700 on a pre-existing permissive directory.""" | ||
| config_dir = tmp_path / ".config" / "fab" | ||
| config_dir.mkdir(parents=True, mode=0o755) | ||
| monkeypatch.setattr( | ||
| "fabric_cli.core.fab_state_config.expanduser", | ||
| lambda path: path.replace("~", str(tmp_path)), | ||
| ) | ||
|
|
||
| cfg.config_location() | ||
|
|
||
| mode = oct(config_dir.stat().st_mode & 0o777) | ||
| assert mode == "0o700", f"Config directory has mode {mode}, expected 0o700" | ||
|
|
||
|
|
||
| @_skip_on_windows | ||
| def test_write_config_creates_file_with_restricted_permissions(monkeypatch, tmp_path): | ||
| """Verify config files are created with mode 0o600 (owner read/write only).""" | ||
| config_file = os.path.join(str(tmp_path), "config.json") | ||
| monkeypatch.setattr(cfg, "config_file", config_file) | ||
|
|
||
| cfg.write_config({"key": "value"}) | ||
|
|
||
| assert os.path.exists(config_file) | ||
| mode = oct(os.stat(config_file).st_mode & 0o777) | ||
| assert mode == "0o600", f"Config file has mode {mode}, expected 0o600" | ||
|
|
||
| # Verify content is still correct | ||
| data = cfg.read_config(config_file) | ||
| assert data == {"key": "value"} | ||
|
|
||
|
|
||
| @_skip_on_windows | ||
| def test_write_config_tightens_permissions_on_existing_file(monkeypatch, tmp_path): | ||
| """Verify write_config() enforces 0o600 on a pre-existing permissive file.""" | ||
| config_file = os.path.join(str(tmp_path), "config.json") | ||
| monkeypatch.setattr(cfg, "config_file", config_file) | ||
|
|
||
| # Create file with overly permissive mode (simulating old CLI version) | ||
| with open(config_file, "w") as f: | ||
| json.dump({"old": "data"}, f) | ||
| os.chmod(config_file, 0o644) | ||
|
|
||
| cfg.write_config({"new": "data"}) | ||
|
|
||
| mode = oct(os.stat(config_file).st_mode & 0o777) | ||
| assert mode == "0o600", f"Config file has mode {mode} after overwrite, expected 0o600" | ||
|
|
||
| data = cfg.read_config(config_file) | ||
| assert data == {"new": "data"} | ||
|
|
||
|
|
||
| # endregion | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Updated the changelog text to accurately describe the scope: "Restrict file and directory permissions on auth, config, context, and log paths".