From b50ca3bc2ee6b7b5fc9d1029eb51fd2fae0eb1c0 Mon Sep 17 00:00:00 2001 From: ktetzlaff Date: Tue, 16 Apr 2024 09:54:57 +0200 Subject: [PATCH] feat: check if password file exists before running `pass` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, `pass-git-helper` doesn't check if a password file exists before running `pass`. This leads to authentication failures when the configured password store entry (aka `target`) points to a directory instead of a file. Example: $ tree -F .password-store .password-store/ └── git/ ├── github.com.gpg └── gitlab.com/ ├── user1.gpg └── user2.gpg If target is `git/gitlab.com`, `pass-git-helper` will get the following output from `pass git/gitlab/com`: git/gitlab.com ├── user1 └── user2 and will then use the first line (`git/gitlab/com`) as password. This commit introduces the following changes: 1. To fix the problem described above, `pass-git-helper` first determines the `pass` password store directory as: - the value of `password_store_dir` if defined in `git_pass_mapping.ini` (and non-empty) - the value of the environment variable `PASSWORD_STORE_DIR` if defined (and non-empty) - the default: `~/.password-store` (the latter two correspond to the implementation of `pass`). `pass-git-helper` will then check if an actual `.gpg` is present in the selected directory (to be clear: only one of the three alternative directories gets checked). The new checks implemented in `pass-git-helper` detect two different error cases: 1. the scenario described above where `target` itself is a directory 2. a (rather obscure) scenario where `.gpg` is a directory In both cases, `pass-git-helper` will now exit with an error after presenting the user with a (hopefully) useful error message. 2. The value of `password_store_dir` in the ini file may now contain a leading `~` (*tilde*) which will get replaced by the users *HOME* (or, on Windows, *USERPROFILE*) directory. The change is documented in README.md. 3. New tests for 1. and 2. Fixes #371 --- README.md | 4 +- passgithelper.py | 72 +++++++- .../example.com.gpg/.keep | 0 .../example.com/fakepwd.gpg | 0 test_passgithelper.py | 156 ++++++++++++++++++ 5 files changed, 222 insertions(+), 10 deletions(-) create mode 100644 test_data/dummy-password-store/example.com.gpg/.keep create mode 100644 test_data/dummy-password-store/example.com/fakepwd.gpg diff --git a/README.md b/README.md index 5395a42..c9817ca 100644 --- a/README.md +++ b/README.md @@ -171,13 +171,13 @@ See also the official [documentation](https://git-scm.com/docs/git-config#_inclu ### Switching Password Stores per Mapping To select a different password store for certain entries, the `password_store_dir` configuration key can be set. -If set, `pass` is directed to a different data directory by defining the `PASSWORD_STORE_DIR` environment variable when calling `pass`. +If set to a non-empty value, `pass` is directed to a different data directory by defining the `PASSWORD_STORE_DIR` environment variable when calling `pass`. If the `password_store_dir` value starts with a tilde (`~`) it will be replaced with the users *HOME* directory (i.e. with the value of the `HOME` or, on Windows, `USERPROFILE` environment variable, see [os.path.expanduser()](https://docs.python.org/3/library/os.path.html#os.path.expanduser) for details). The following config demonstrates this practices ```init [github.com/mycompany] -password_store_dir=/home/me/.work-passwords +password_store_dir=~/.work-passwords ``` ## Password Store Layout and Data Extraction diff --git a/passgithelper.py b/passgithelper.py index 7eb96b7..b13d4e0 100755 --- a/passgithelper.py +++ b/passgithelper.py @@ -17,7 +17,7 @@ import re import subprocess import sys -from typing import Dict, IO, Mapping, Optional, Pattern, Sequence, Text +from typing import Dict, IO, Mapping, Optional, Pattern, Sequence, Text, Tuple import xdg.BaseDirectory @@ -357,13 +357,68 @@ def define_pass_target( return pass_target -def compute_pass_environment(section: configparser.SectionProxy) -> Mapping[str, str]: +def compute_pass_environment( + section: configparser.SectionProxy, +) -> Tuple[Mapping[str, str], Path]: + """Returns the environment variables needed to start the ``pass`` subprocess. + + The main task of this function is to determine the password store directory + to be used by ``pass``. It does this by: + + 1. using the value of ``password_store_dir`` in ``section`` (if defined and + non-empty), + 2. using the value of the ``PASSWORD_STORE_DIR`` environment variable (if + defined and non-empty), + 3. falling back to the default: ``~/password-store``. + + In the next step, a leading ``~`` (tilde) in the resulting path gets + replaced by the users ``$HOME`` (on Windows: ``%USERPROFILE%``) directory. + See ``os.path.expanduser()`` for more details. + + Finally, the result is used to add or update ``PASSWORD_STORE_DIR`` to/in a + copy of the current process environment. + + Args: + section: + Ini file section which applies to the current password target. + + Returns: + A tuple (env, dir) where ``env`` is a dictionary comprising a copy of + the current process environment wth updated ``PASSWORD_STORE_DIR`` value + and ``dir`` is the value of ``PASSWORD_STORE_DIR`` as a ``Path`` + instance (for the callers convenience). + + """ environment = os.environ.copy() - password_store_dir = section.get("password_store_dir") - if password_store_dir: - LOGGER.debug('Setting PASSWORD_STORE_DIR to "%s"', password_store_dir) - environment["PASSWORD_STORE_DIR"] = password_store_dir - return environment + password_store_dir = Path( + section.get("password_store_dir") + or environment.get("PASSWORD_STORE_DIR") + or "~/.password-store" + ).expanduser() + LOGGER.debug('Setting PASSWORD_STORE_DIR to "%s"', password_store_dir) + environment["PASSWORD_STORE_DIR"] = str(password_store_dir) + return environment, password_store_dir + + +def check_password_file(password_store_dir: Path, pass_target: str) -> None: + """Check that the password file exists and that it is a regular file. + + Args: + password_store_dir: + Directory which contains the password to be checked. + pass_target: + Path to the actual password file within ``password_store_dir`` + (without ``.gpg`` extension). + + Raises: + - FileNotFoundError if the password file does not exist, + - TypeError if if the password file is not a real file. + """ + pass_target_file = Path(password_store_dir / (pass_target + ".gpg")) + if not pass_target_file.exists(): + raise FileNotFoundError(f"'{pass_target_file}' does not exist") + if not pass_target_file.is_file(): + raise TypeError(f"'{pass_target_file}' is not a file") def get_password( @@ -399,7 +454,8 @@ def get_password( ) username_extractor.configure(section) - environment = compute_pass_environment(section) + environment, password_store_dir = compute_pass_environment(section) + check_password_file(password_store_dir, pass_target) LOGGER.debug('Requesting entry "%s" from pass', pass_target) # silence the subprocess injection warnings as it is the user's diff --git a/test_data/dummy-password-store/example.com.gpg/.keep b/test_data/dummy-password-store/example.com.gpg/.keep new file mode 100644 index 0000000..e69de29 diff --git a/test_data/dummy-password-store/example.com/fakepwd.gpg b/test_data/dummy-password-store/example.com/fakepwd.gpg new file mode 100644 index 0000000..e69de29 diff --git a/test_passgithelper.py b/test_passgithelper.py index 94b7a2e..a875eac 100644 --- a/test_passgithelper.py +++ b/test_passgithelper.py @@ -1,6 +1,7 @@ import configparser from dataclasses import dataclass import io +from pathlib import Path from subprocess import CalledProcessError from typing import Any, Iterable, Optional, Sequence, Text from unittest.mock import ANY @@ -28,6 +29,9 @@ def helper_config(mocker: MockerFixture, request: Any) -> Iterable[Any]: request.param.request ) + if request.param.entry_data != b"check-password-file": + mocker.patch("passgithelper.check_password_file") + subprocess_mock = mocker.patch("subprocess.check_output") if request.param.entry_data: subprocess_mock.return_value = request.param.entry_data @@ -55,6 +59,85 @@ def test_handle_skip_exits(monkeypatch: Any) -> None: passgithelper.handle_skip() +class TestPasswordStoreDirSelection: + """Test password store directory selection. + + The password store directory used by ``pass`` can either be set via the + ``PASSWORD_STORE_DIR`` environment variable or via the + ``password_store_dir`` option in the ini file. In case both are present, the + ini file option overrides the environment variable. + + Empty values are ignored (as if they have not been set). + + The different variants are tested here. + + """ + + def test_uses_default_value(self, monkeypatch: Any) -> None: + ini = configparser.ConfigParser() + + expected = str(Path("~/.password-store").expanduser()) + # no password store configured (neither per env. variable nor ini file + # option) -> default value (`~/.password-store`) + monkeypatch.delenv("PASSWORD_STORE_DIR", raising=False) + ini["example.com"] = {} + env, pwd_store_dir = passgithelper.compute_pass_environment(ini["example.com"]) + assert str(pwd_store_dir) == env.get("PASSWORD_STORE_DIR") + assert pwd_store_dir == Path(expected) + # also: check `~` expansion + assert pwd_store_dir.is_absolute() + + def test_uses_env_var_value(self, monkeypatch: Any) -> None: + ini = configparser.ConfigParser() + + expected = "/tmp/password-store-from-env" + # `PASSWORD_STORE_DIR` in environment, empty ini file section -> value of + # env. variable overrides default + monkeypatch.setenv("PASSWORD_STORE_DIR", expected) + ini["example.com"] = {} + env, pwd_store_dir = passgithelper.compute_pass_environment(ini["example.com"]) + assert str(pwd_store_dir) == env.get("PASSWORD_STORE_DIR") + assert pwd_store_dir == Path(expected) + + def test_ini_file_option_overrides_environment(self, monkeypatch: Any) -> None: + ini = configparser.ConfigParser() + + expected = "/tmp/password-store-from-ini" + # `PASSWORD_STORE_DIR` in environemnt, `password_store_dir` in ini file + # section -> value from ini file overrides env. variable + monkeypatch.setenv("PASSWORD_STORE_DIR", "/tmp/password-store-from-env") + ini["example.com"] = {"password_store_dir": expected} + env, pwd_store_dir = passgithelper.compute_pass_environment(ini["example.com"]) + assert str(pwd_store_dir) == env.get("PASSWORD_STORE_DIR") + assert pwd_store_dir == Path(expected) + + def test_ignores_empty_ini_file_option(self, monkeypatch: Any) -> None: + ini = configparser.ConfigParser() + + expected = "/tmp/password-store-from-env" + # `PASSWORD_STORE_DIR` in environment, empty `password_store_dir` in ini + # file section -> empty ini value ignored, fall back to env. variable + monkeypatch.setenv("PASSWORD_STORE_DIR", expected) + ini["example.com"] = {"password_store_dir": ""} + env, pwd_store_dir = passgithelper.compute_pass_environment(ini["example.com"]) + assert str(pwd_store_dir) == env.get("PASSWORD_STORE_DIR") + assert pwd_store_dir == Path(expected) + + def test_ignores_empty_env_var_value(self, monkeypatch: Any) -> None: + ini = configparser.ConfigParser() + + expected = str(Path("~/.password-store").expanduser()) + # empty `PASSWORD_STORE_DIR` in environment, empty ini file section -> empty + # env. var value ignored, fall back to default + monkeypatch.setenv("PASSWORD_STORE_DIR", "") + ini["example.com"] = {} + env, pwd_store_dir = passgithelper.compute_pass_environment(ini["example.com"]) + assert str(pwd_store_dir) == env.get("PASSWORD_STORE_DIR") + assert pwd_store_dir == Path(expected) + # also: check `~` expansion + assert pwd_store_dir.is_absolute() + + class TestSkippingDataExtractor: class ExtractorImplementation(passgithelper.SkippingDataExtractor): def configure(self, config: configparser.SectionProxy) -> None: @@ -529,3 +612,76 @@ def test_supports_switching_password_store_dirs( helper_config.mock_calls[-1].kwargs["env"]["PASSWORD_STORE_DIR"] == "/some/dir" ) + + @pytest.mark.parametrize( + "helper_config", + [ + HelperConfig( + None, + "\nhost=example.com", + b"ignored", + ), + ], + indirect=True, + ) + def test_uses_password_store_dir_relative_to_home( + self, mocker: MockerFixture, helper_config: Any + ) -> None: + config = configparser.ConfigParser() + config["example.com"] = { + "password_store_dir": "~/some/dir", + "target": "dev/mytest", + } + mocker.patch("passgithelper.parse_mapping").return_value = config + + passgithelper.main(["get"]) + password_store_dir = Path( + helper_config.mock_calls[-1].kwargs["env"]["PASSWORD_STORE_DIR"] + ) + assert password_store_dir.is_absolute() + assert password_store_dir == Path("~/some/dir").expanduser() + + @pytest.mark.parametrize( + "helper_config", + [ + HelperConfig( + None, + "", + b"check-password-file", + ), + ], + indirect=True, + ) + @pytest.mark.usefixtures("helper_config") + def test_verifies_check_password_file_function( + self, monkeypatch: Any, mocker: MockerFixture, capsys: Any + ) -> None: + monkeypatch.setenv( + "PASSWORD_STORE_DIR", str(Path.cwd() / "test_data/dummy-password-store") + ) + config = configparser.ConfigParser() + config["example.com"] = {"target": ""} + mocker.patch("passgithelper.parse_mapping").return_value = config + mocker.patch("passgithelper.parse_request").return_value = { + "host": "example.com" + } + + config["example.com"]["target"] = "example.com" + with pytest.raises(SystemExit): + passgithelper.main(["get"]) + out, err = capsys.readouterr() + assert err.endswith("example.com.gpg' is not a file\n") + assert not out + + config["example.com"]["target"] = "example.com/doesnotexist" + with pytest.raises(SystemExit): + passgithelper.main(["get"]) + _, err = capsys.readouterr() + assert err.endswith("example.com/doesnotexist.gpg' does not exist\n") + assert not out + + config["example.com"]["target"] = "example.com/fakepwd" + passgithelper.main(["get"]) + out, err = capsys.readouterr() + assert out == "password=check-password-file\n" + assert not err