passwordstore: Prevent using path as password

[grembo]

SUMMARY

Given a password stored in path/to/secret, requesting the password
path/to will literally return path/to. This can lead to using
weak passwords by accident/mess up logic in code, based on the
state of the password store.

This is worked around by applying the same logic pass uses:
If a password was returned, check if there is a .gpg file it could
have come from. If not, treat it as missing.

Fixes #4185

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

passwordstore
plugins/lookup/passwordstore

ADDITIONAL INFORMATION

See:

• passwordstore: Distinguish between subdirs and passwords, don't use path names as passwords #4185

[felixfontein]

Thanks for your contribution! Could you please add a changelog fragment?

I would also suggest to split this up into two PRs, one for the path check, and the other for locking. Combining unrelated things into one PR is something that should really be avoided.

Also regarding locking: I think this should only be done on creation, not in every case. The correct solution for the gpg issues is auto-expand-secmem and not locking. This should probably be configurable (so you can force locking if you don't want to use auto-expand-secmem).

[grembo]

Hi @felixfontein, thanks for reviewing.

Thanks for your contribution! Could you please add a changelog fragment?

I'll look into it.

I would also suggest to split this up into two PRs, one for the path check, and the other for locking. Combining unrelated things into one PR is something that should really be avoided.

To me, they are related from a user perspective, as both issues result in passwordstore returning unexpected/inconsistent passwords. If you insist, I can split it into two PRs once the discussion on the PR finished.

Also regarding locking: I think this should only be done on creation, not in every case. The correct solution for the gpg issues is auto-expand-secmem and not locking. This should probably be configurable (so you can force locking if you don't want to use auto-expand-secmem).

Locking needs to happen in all cases (reads and writes), as

• the logic inside passwordstore.py (regardless of gpg issues) does reads to determine if create/write operations are required, and therefore is racy. So it does a read to figure out if a secret exists and - if it doesn't - it starts creation. This can happen in parallel, which ends up in undefined/unpredictable behavior. Preventing this reliably (within one play as well when running multiple plays in parallel which are backed by the same password store) is complicated and error-prone, when done on the playbook level. So the logic in there requires locking. Also, the pass utility itself doesn't do any internal locking to prevent problems (so calls to pass aren't atomic).
• it works around the gpg issues without requiring users to modify their gpg-agent.conf (which is something each user of the playbook has to do).

I don't think the overhead added by the lock is problematic, looking at the overall cost of passwordstore operations.

IMHO the benefit of having predictable behavior, not having to worry about synchronization in playbooks, and not requiring to adjust gpg-agent.conf (plus reloading the agent) and/or any other special configuration outweighs this little extra overhead. It also makes maintaining the code in passwordstore.py easier and more robust in case pass changes.

[felixfontein]

I would also suggest to split this up into two PRs, one for the path check, and the other for locking. Combining unrelated things into one PR is something that should really be avoided.

To me, they are related from a user perspective, as both issues result in passwordstore returning unexpected/inconsistent passwords. If you insist, I can split it into two PRs once the discussion on the PR finished.

It would be good to split them up. The one is a proper bugfix (and shoul be backported to stable-3 and stable-4), the other more a feature than a bugfix (and thus only should be backported to stable-4). Keeping them in the same PR prevents this from being backported to stable-3.

Also regarding locking: I think this should only be done on creation, not in every case. The correct solution for the gpg issues is auto-expand-secmem and not locking. This should probably be configurable (so you can force locking if you don't want to use auto-expand-secmem).

Locking needs to happen in all cases (reads and writes), as

* the logic inside _passwordstore.py_ (regardless of gpg issues) does reads to determine if create/write operations are required, and therefore is racy.

But that's only true if create=true or missing=create! Otherwise no write is done at all, so locking would be an unnecessary performance hit.

So it does a read to figure out if a secret exists and - if it doesn't - it starts creation. This can happen in parallel, which ends up in undefined/unpredictable behavior. Preventing this reliably (within one play as well when running multiple plays in parallel which are backed by the same password store) is complicated and error-prone, when done on the playbook level. So the logic in there requires locking. Also, the pass utility itself doesn't do any internal locking to prevent problems (so calls to pass aren't atomic).

When create=true or missing=create locking makes sense (and should be done by default). But only then, or when the user explicitly requests it.

* it works around the gpg issues without requiring users to modify their _gpg-agent.conf_ (which is something _each_ user of the playbook has to do).

While this is annoying, it is the correct fix. Putting ugly hacks everywhere just because gpg has stupid defaults is bad practice.

I don't think the overhead added by the lock is problematic, looking at the overall cost of passwordstore operations.

I disagree here. Especially because passwordstore operations are expensive, locking makes the whole situation worse because with locking, all operations must be done sequentially and cannot be done in parallel.

IMHO the benefit of having predictable behavior, not having to worry about synchronization in playbooks, and not requiring to adjust gpg-agent.conf (plus reloading the agent) and/or any other special configuration outweighs this little extra overhead. It also makes maintaining the code in passwordstore.py easier and more robust in case pass changes.

I disagree with this, and I don't think this decision should be forced on every user of this plugin.

[grembo]

It would be good to split them up. The one is a proper bugfix (and should be backported to stable-3 and stable-4), the other more a feature than a bugfix (and thus only should be backported to stable-4). Keeping them in the same PR prevents this from being backported to stable-3.

They are both bugfixes to me (unless you consider preventing undefined behavior a feature).

We don't have to agree on that part and I can split them up of course. I wouldn't describe it as a "minor_change" though, as it fixes broken behavior (or if it's intentional: broken design) that causes problems.

Also regarding locking: I think this should only be done on creation, not in every case. The correct solution for the gpg issues is auto-expand-secmem and not locking. This should probably be configurable (so you can force locking if you don't want to use auto-expand-secmem).

• the logic inside passwordstore.py (regardless of gpg issues) does reads to determine if create/write operations are required, and therefore is racy.

But that's only true if create=true or missing=create! Otherwise no write is done at all, so locking would be an unnecessary performance hit.

Are you certain that there is no scenario where a write is partially done and a read could suffer from that?

When create=true or missing=create locking makes sense (and should be done by default). But only then, or when the user explicitly requests it.

* it works around the gpg issues without requiring users to modify their _gpg-agent.conf_ (which is something _each_ user of the playbook has to do).

While this is annoying, it is the correct fix. Putting ugly hacks everywhere just because gpg has stupid defaults is bad practice.

I think that what's even worse than gpg having "stupid defaults" is to know about them and let users run into the same problem over and over again.

Do you have an idea how the plugin could check if gpg-agent is configured correctly? We certainly don't want users to learn about that it should be configured differently by spending hours figuring out why a playbook that worked yesterday suddenly failed.

If we can detect it reliably, we can make the plugin fail with a helpful error message if gpg-agent is misconfigured. If not, we should make locking the default and add a configuration option to disable it for those who understand the implications.

IMHO the benefit of having predictable behavior, not having to worry about synchronization in playbooks, and not requiring to adjust gpg-agent.conf (plus reloading the agent) and/or any other special configuration outweighs this little extra overhead. It also makes maintaining the code in passwordstore.py easier and more robust in case pass changes.

I disagree with this, and I don't think this decision should be forced on every user of this plugin.

I prefer a playbook to work reliably (even if it's slightly slower), as that's consuming the machine's time, while breakage costs my time and causes fallout.

That said, if we can find a fix that satisfies both positions, all the better.

[grembo]

@felixfontein I reduced this PR to the path fix, I'll create a follow-up PR with locking once ready.

[grembo]

@felixfontein Follow-up for configurable locking is #4194, currently stacked on top of this one.

[grembo]

@felixfontein This change made the logic more obvious (e.g., getting rid of the superfluous e.returncode != 0 while handling CalledProcessError).

[grembo]

@felixfontein This should be ready to land now.

Reg. the localization issue: Turns out this will be as easy setting self.env['LANGUAGE']='C', as this will only affect the gettext library translations and not other parts of the locale system that would have an impact on the character set used, see https://www.gnu.org/software/gettext/manual/html_node/The-LANGUAGE-variable.html

[felixfontein]

Looks good to me. (I'm not a passwordstore user and haven't tested it.)

[felixfontein]

Since nobody else commented here, I assume nobody else has an opinion, so I'll just merge so that the depending PRs can continue.

[patchback]

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/da49c0968d6056e7591a324ffab210ce85f9dde4/pr-4192

Backported as #4217

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[patchback]

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/da49c0968d6056e7591a324ffab210ce85f9dde4/pr-4192

Backported as #4218

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[felixfontein]

@grembo thanks a lot for your contribution!

[grembo]

@grembo thanks a lot for your contribution!

@felixfontein And you for taking the time to review.