Skip to content
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

feat: check if password file exists before running pass #372

Merged

Conversation

ktetzlaff
Copy link
Contributor

@ktetzlaff ktetzlaff commented Apr 16, 2024

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 <target>.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 <target>.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

@codecov-commenter
Copy link

codecov-commenter commented Apr 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.66%. Comparing base (0489e36) to head (b50ca3b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
+ Coverage   91.37%   91.66%   +0.28%     
==========================================
  Files           1        1              
  Lines         174      180       +6     
  Branches       23       24       +1     
==========================================
+ Hits          159      165       +6     
  Misses         10       10              
  Partials        5        5              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ktetzlaff ktetzlaff force-pushed the detect-dir-instead-of-or-no-password-file branch 3 times, most recently from 4022ff2 to 628b79a Compare April 16, 2024 10:37
@ktetzlaff ktetzlaff changed the title fix: check if password file exists before calling pass fix: check if password file exists before running pass Apr 16, 2024
@ktetzlaff ktetzlaff force-pushed the detect-dir-instead-of-or-no-password-file branch 6 times, most recently from 07b8ddd to 3a0c6ee Compare April 16, 2024 14:29
Copy link
Owner

@languitar languitar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the great contribution. I have added a few annotations for mostly style issues to match the current implementation.

Additionally, as the changelog for the release is automatically built from the commit messages, can you split this PR up into two commits. One for the better error message in case an extractor name is off and a second commit for the actual feature of this PR?

passgithelper.py Outdated Show resolved Hide resolved
passgithelper.py Outdated Show resolved Hide resolved
passgithelper.py Outdated Show resolved Hide resolved
test_passgithelper.py Outdated Show resolved Hide resolved
@ktetzlaff ktetzlaff force-pushed the detect-dir-instead-of-or-no-password-file branch from 60b2a3e to 6e22e44 Compare April 24, 2024 14:04
@ktetzlaff
Copy link
Contributor Author

... split this PR up into two commits. One for the better error message in case an extractor name is off

The username extractor changes are now in in the new PR #375.

@ktetzlaff ktetzlaff force-pushed the detect-dir-instead-of-or-no-password-file branch 4 times, most recently from 91f899b to 2c0f0ae Compare April 24, 2024 15:50
@ktetzlaff ktetzlaff requested a review from languitar April 24, 2024 15:54
@ktetzlaff
Copy link
Contributor Author

@languitar I think I resolved all open issues. Please check.

@ktetzlaff ktetzlaff force-pushed the detect-dir-instead-of-or-no-password-file branch 3 times, most recently from 21e1375 to a6db561 Compare April 24, 2024 16:08
@ktetzlaff ktetzlaff force-pushed the detect-dir-instead-of-or-no-password-file branch from a6db561 to 37620ef Compare April 24, 2024 16:16
@ktetzlaff ktetzlaff changed the title fix: check if password file exists before running pass feat: check if password file exists before running pass Apr 24, 2024
@ktetzlaff ktetzlaff force-pushed the detect-dir-instead-of-or-no-password-file branch from 37620ef to 3d264b8 Compare April 24, 2024 16:24
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 `<target>.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 `<target>.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 languitar#371
@ktetzlaff ktetzlaff force-pushed the detect-dir-instead-of-or-no-password-file branch from 3d264b8 to b50ca3b Compare April 25, 2024 19:47
Copy link
Owner

@languitar languitar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the nice contribution and for updating the PR after the review.

@languitar languitar merged commit 01252c8 into languitar:main Apr 25, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Authentication failure when target matches a directory instead of a file
3 participants