diff --git a/config2py/s_configparser.py b/config2py/s_configparser.py index 00e14c0..21a53ae 100644 --- a/config2py/s_configparser.py +++ b/config2py/s_configparser.py @@ -9,6 +9,8 @@ from dol import Store +from config2py.util import secure_open + # from py2store.signatures import Sig _test_config_str = """[Simple Values] @@ -287,7 +289,7 @@ def persist(self): """ if not self._within_context_manager: if self.target_kind == "filepath": - with open(self.source, "w") as fp: + with secure_open(self.source, "w") as fp: return self.write(fp, self.space_around_delimiters) else: if self.target_kind == "stream": diff --git a/config2py/sync_store.py b/config2py/sync_store.py index 8876d06..c09aa0b 100644 --- a/config2py/sync_store.py +++ b/config2py/sync_store.py @@ -31,6 +31,7 @@ from collections.abc import MutableMapping from pathlib import Path import json +import os from functools import reduce __all__ = [ @@ -44,6 +45,25 @@ # Note: Independent module. No imports from config2py, dol etc. # TODO: Do we want to use more stuff from config2py, dol, etc.? + +def _secure_open(path, mode="w"): + """Open ``path`` for writing with owner-only (``0o600``) permissions. + + Local copy of ``config2py.util.secure_open`` -- this module deliberately has no + intra-package imports (see note above). Avoids writing files that may hold secrets + with the process's default (often world-readable) umask; see i2mint/config2py#15. + + Re-tightens via ``os.fchmod`` on the open fd (not just relying on ``os.open``'s + ``mode`` argument, which POSIX only consults when a *new* file is created -- a + pre-existing, already-loose file would otherwise keep its old permissions). + """ + flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC + fd = os.open(path, flags, 0o600) + if hasattr(os, "fchmod"): # POSIX only -- Windows ACLs aren't unix mode bits + os.fchmod(fd, 0o600) + return os.fdopen(fd, mode) + + # Type aliases KeyPath = Union[str, Tuple[str, ...], None] Loader = Callable[[], dict] @@ -350,10 +370,10 @@ def _load_from_file(self) -> dict: # Create file with initial content initial_data = self.create_file_content() - self.filepath.parent.mkdir(parents=True, exist_ok=True) + self.filepath.parent.mkdir(parents=True, exist_ok=True, mode=0o700) content = self._file_dumper(initial_data, **self.dump_kwargs) write_mode = "w" if "b" not in self.mode else "wb" - with open(self.filepath, write_mode) as f: + with _secure_open(self.filepath, write_mode) as f: f.write(content) data = initial_data else: @@ -376,7 +396,7 @@ def _load_from_file(self) -> dict: # Write back to file content = self._file_dumper(full_data, **self.dump_kwargs) write_mode = "w" if "b" not in self.mode else "wb" - with open(self.filepath, write_mode) as f: + with _secure_open(self.filepath, write_mode) as f: f.write(content) return initial_content @@ -394,7 +414,7 @@ def _dump_to_file(self, section_data: dict) -> None: content = self._file_dumper(full_data, **self.dump_kwargs) write_mode = "w" if "b" not in self.mode else "wb" - with open(self.filepath, write_mode) as f: + with _secure_open(self.filepath, write_mode) as f: f.write(content) def __repr__(self): diff --git a/config2py/tests/test_app_data.py b/config2py/tests/test_app_data.py index 61c3c6f..67d9b89 100644 --- a/config2py/tests/test_app_data.py +++ b/config2py/tests/test_app_data.py @@ -107,6 +107,11 @@ def test_creates_file_when_missing(self, tmp_path, mock_seeds_for_ensure): assert result == target assert target.exists() assert target.read_bytes() == b"hello world\nline two\n" + if os.name == "posix": + # i2mint/config2py#15: seeded files (may hold secrets, e.g. AppData.get_config) + # must not be created world-readable. + assert oct(target.stat().st_mode & 0o777) == "0o600" + assert oct(target.parent.stat().st_mode & 0o777) == "0o700" def test_preserves_existing_file(self, tmp_path, mock_seeds_for_ensure): target = tmp_path / "hello.txt" @@ -234,6 +239,8 @@ def test_get_artifact_dir_creates_subdir(self, tmp_path): midi_dir = app.get_artifact_dir("midi") assert midi_dir.is_dir() assert _same_path(midi_dir, tmp_path / "testapp" / "artifacts" / "midi") + if os.name == "posix": # i2mint/config2py#15 + assert oct(midi_dir.stat().st_mode & 0o777) == "0o700" def test_get_artifact_dir_multiple_kinds(self, tmp_path): with _redirect_app_root("data", tmp_path): diff --git a/config2py/tests/test_secure_io.py b/config2py/tests/test_secure_io.py new file mode 100644 index 0000000..b8281dd --- /dev/null +++ b/config2py/tests/test_secure_io.py @@ -0,0 +1,71 @@ +"""Tests for the file/dir permission hardening added for i2mint/config2py#15. + +Covers the pieces the fix touches that ``test_app_data.py`` doesn't already exercise +through ``AppData``/``ensure_seeded``: ``secure_makedirs``'s re-tightening behaviour, +the ``create_directories`` alternate (``max_dirs_to_make``) code path, and the two +concrete file-writing call sites (``ConfigStore.persist`` / ``s_configparser.py`` and +``FileStore`` / ``sync_store.py``) that were updated to use ``secure_open``. + +All permission assertions are POSIX-only (``os.name == "posix"``): file mode bits are +not meaningful on Windows. +""" + +import json +import os +import tempfile + +import pytest + +from config2py.util import create_directories, secure_makedirs +from config2py.s_configparser import ConfigStore +from config2py.sync_store import FileStore + +posix_only = pytest.mark.skipif(os.name != "posix", reason="file mode bits are POSIX-only") + + +@posix_only +def test_secure_makedirs_retightens_existing_dir(tmp_path): + """A pre-existing, loosely-permissioned dir must be re-tightened to 0o700.""" + target = tmp_path / "already_here" + target.mkdir(mode=0o755) + os.chmod(target, 0o755) # mkdir's mode is subject to umask; force it to stick + assert oct(target.stat().st_mode & 0o777) == "0o755" + + secure_makedirs(target) + + assert oct(target.stat().st_mode & 0o777) == "0o700" + + +@posix_only +def test_create_directories_max_dirs_to_make_branch_is_owner_only(tmp_path): + """The ``max_dirs_to_make``-bounded branch of create_directories must also be 0o700.""" + target = tmp_path / "a" / "b" / "c" + assert create_directories(str(target), max_dirs_to_make=5) is True + for p in (target, target.parent, target.parent.parent): + assert oct(p.stat().st_mode & 0o777) == "0o700" + + +@posix_only +def test_config_store_persist_writes_owner_only_file(tmp_path): + """ConfigStore.persist() (s_configparser.py) must not leave a world-readable file.""" + ini_path = tmp_path / "config_store_test.ini" + store = ConfigStore(str(ini_path)) + store["a_section"] = {"key": "value"} # triggers persist() + + assert ini_path.is_file() + assert oct(ini_path.stat().st_mode & 0o777) == "0o600" + + +@posix_only +def test_file_store_write_is_owner_only(tmp_path): + """FileStore (sync_store.py) must not leave a world-readable file after a write.""" + path = tmp_path / "store.json" + path.write_text('{"key": "value"}') + os.chmod(path, 0o644) # start world-readable, like a plain `open(..., "w")` would + + store = FileStore(str(path)) + store["new_key"] = "new_value" # triggers a write via _secure_open + + with open(path) as f: + assert json.load(f)["new_key"] == "new_value" + assert oct(path.stat().st_mode & 0o777) == "0o600" diff --git a/config2py/tests/test_tools.py b/config2py/tests/test_tools.py index 01e6746..e9d562a 100644 --- a/config2py/tests/test_tools.py +++ b/config2py/tests/test_tools.py @@ -34,7 +34,12 @@ def test_simple_config_getter(mock_config_store_factory): # assert config_getter(key) == "from store" # Test getting config with ask_user_if_key_not_found=True - with patch("builtins.input", return_value="from user"): + # (patch both prompt functions ask_user_for_input can dispatch to -- which one is + # used depends on mask_input, default False today, see i2mint/config2py#13) + with ( + patch("builtins.input", return_value="from user"), + patch("getpass.getpass", return_value="from user"), + ): config_getter = simple_config_getter(ask_user_if_key_not_found=True) assert config_getter("new_key") == "from user" diff --git a/config2py/tests/utils_for_testing.py b/config2py/tests/utils_for_testing.py index 5dd3fa2..dff9fd5 100644 --- a/config2py/tests/utils_for_testing.py +++ b/config2py/tests/utils_for_testing.py @@ -4,4 +4,12 @@ def user_input_patch(monkeypatch, user_input_string: str): + """Patch both prompt functions ``ask_user_for_input`` can dispatch to. + + Which one is actually called depends on ``mask_input`` (currently defaults to + ``DFLT_MASKING_INPUT = False``, i.e. ``input`` -- but see the still-open + i2mint/config2py#13, which proposes flipping that default), so tests that don't + care about masking specifically should patch both rather than assume one. + """ monkeypatch.setattr("builtins.input", lambda _: user_input_string) + monkeypatch.setattr("getpass.getpass", lambda _: user_input_string) diff --git a/config2py/util.py b/config2py/util.py index af470d2..4cd2371 100644 --- a/config2py/util.py +++ b/config2py/util.py @@ -28,6 +28,49 @@ no_default = mk_sentinel("no_default") +def secure_open(path, mode="w"): + """Open ``path`` for writing with owner-only (``0o600``) permissions. + + Two cases, both handled: + + - *New* file: the restrictive mode is applied atomically at creation via + ``os.open``, so there is no window where the file briefly exists with the + process's default umask (commonly world-readable, ``0o644``). + - *Pre-existing* file with looser permissions: ``os.open``'s ``mode`` argument + is a POSIX no-op in this case (only consulted when a new file is actually + created), so an explicit ``os.fchmod`` re-tightens it -- on the open file + descriptor, not the path, so it's not subject to a TOCTOU swap either. + + Intended for files that may hold secrets (see i2mint/config2py#15). + + >>> import tempfile, os + >>> path = tempfile.mktemp() + >>> with secure_open(path, "w") as f: + ... _ = f.write("secret") + >>> # Unix mode bits aren't meaningful on Windows -- os.stat there reports 0o666 + >>> # regardless of what secure_open does, so only assert the mode on POSIX. + >>> oct(os.stat(path).st_mode & 0o777) if os.name == "posix" else "0o600" + '0o600' + >>> os.remove(path) + """ + flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC + fd = os.open(path, flags, 0o600) + if hasattr(os, "fchmod"): # POSIX only -- Windows ACLs aren't unix mode bits + os.fchmod(fd, 0o600) # re-tighten if the file already existed (see docstring) + return os.fdopen(fd, mode) + + +def secure_makedirs(dirpath, *, exist_ok=True): + """``os.makedirs(dirpath, mode=0o700)``, re-tightening the mode if it already exists. + + ``os.makedirs(..., mode=0o700, exist_ok=True)`` alone won't re-tighten an existing + directory's mode, so this follows up with an explicit ``os.chmod``. Intended for + directories that may hold config/secret files (see i2mint/config2py#15). + """ + os.makedirs(dirpath, mode=0o700, exist_ok=exist_ok) + os.chmod(dirpath, 0o700) + + def always_true(x: Any) -> bool: """Function that just returns True.""" return True @@ -54,7 +97,13 @@ def is_not_empty(x: Any) -> bool: # preserving confidentiality while retaining full read/write functionality. class EnvironmentVariables(ChainMap): """ - Class to wrap environment variables without revealing sensitive information. + Class to wrap environment variables, hiding values from ``repr``/``print`` only. + + ``__repr__`` is overridden to avoid printing secrets to a REPL or log, but values + are still reachable through normal ``Mapping`` operations -- ``dict(envvar)``, + ``envvar.items()``/``.values()``, ``pickle.dumps(envvar)``, or a structured logger + that walks the mapping. Treat this as UI-level redaction, not access control (see + i2mint/config2py#16). """ def __init__(self): @@ -245,7 +294,7 @@ def create_directories(dirpath, max_dirs_to_make=None): return True if max_dirs_to_make is None: - os.makedirs(dirpath, exist_ok=True) + secure_makedirs(dirpath) return True # Calculate the number of directories to create @@ -261,7 +310,7 @@ def create_directories(dirpath, max_dirs_to_make=None): # Create directories from the top level down for dir_to_make in reversed(dirs_to_make): - os.mkdir(dir_to_make) + os.mkdir(dir_to_make, mode=0o700) return True @@ -467,7 +516,7 @@ def _default_folder_setup(directory_path: str) -> None: This is the default setup callback for directories managed by config2py. """ if not os.path.isdir(directory_path): - os.makedirs(directory_path, exist_ok=True) + secure_makedirs(directory_path) # Add a hidden file to annotate the directory as one managed by config2py. # This helps distinguish it from directories created by other programs # (this can be useful to avoid conflicts). @@ -641,8 +690,9 @@ def ensure_seeded( from importlib.resources import files ref = files(f"{package_name}.{seed_data_dir}.{seed_subpackage}") / filename - target.parent.mkdir(parents=True, exist_ok=True) - target.write_bytes(ref.read_bytes()) + secure_makedirs(target.parent) + with secure_open(target, "wb") as fp: + fp.write(ref.read_bytes()) return target @@ -730,7 +780,7 @@ def get_config(self, name: str) -> Path: def get_artifact_dir(self, kind: str) -> Path: """Return (and create) an artifact sub-directory for *kind*.""" d = self.app_folder(folder_kind="data") / "artifacts" / kind - d.mkdir(parents=True, exist_ok=True) + secure_makedirs(d) return d