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

vault-password: ensure other users can't read vault password #11756

Closed

Conversation

billwanjohi
Copy link
Contributor

@billwanjohi billwanjohi commented Jul 27, 2015

SUMMARY

Take 2 on #11754, this time on live code, and after testing locally.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

lib/ansible/cli/init.py

ANSIBLE VERSION

2.3

ADDITIONAL INFORMATION

@bcoca
Copy link
Member

bcoca commented Jul 27, 2015

I'm not sure we want this, many times a group shares a vault file, permissions should be up to the user as the context can vary.

@billwanjohi
Copy link
Contributor Author

I took inspiration for this from other common utilities like ssh-agent and postgresql that will fail when given overly permissive files. Example from postgresql documentation:

On Unix systems, the permissions on .pgpass must disallow any access to world or group; achieve this by the command chmod 0600 ~/.pgpass. If the permissions are less strict than this, the file will be ignored.

I do follow you that this would be a nuisance to some subset of users (though I doubt many), but this is such an important security consideration that I'd argue for the interests of the majority of users, who will be better served by the more restrictive default.

(Ideally I'd even like to confirm that the file isn't tracked by git or other revision control systems, but that's for another pull request.)

@bcoca
Copy link
Member

bcoca commented Jul 27, 2015

I don't think it is a minority as vault is frequently used within a team setting, I would make it into a config enforce_vault_permissions=0440, for example.

@amenonsen
Copy link
Contributor

That makes sense to me: if enforce_vault_permissions is set to something and the permissions don't match, complain. If it's not set, don't do anything. (But that means this PR also needs a documentation update.)

@bcoca bcoca added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 28, 2015
@jimi-c jimi-c removed the P4 label Dec 7, 2015
@alikins alikins self-assigned this May 26, 2016
@ansibot ansibot added affects_2.3 This issue/PR affects Ansible v2.3 needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Dec 13, 2016
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jan 2, 2017
@ansibot
Copy link
Contributor

ansibot commented Jan 5, 2017

@billwanjohi This PR was tested by travis-ci.org, which is no longer used. Please rebase your branch to trigger running of current tests.

click here for bot help

@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 5, 2017
@ansibot
Copy link
Contributor

ansibot commented Jan 6, 2017

@billwanjohi This PR was tested by travis-ci.org, which is no longer used. Please rebase your branch to trigger running of current tests.

click here for bot help

@ansibot ansibot added the c:cli/ label Jan 12, 2017
@ansibot ansibot added needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. labels Apr 4, 2017
@ansibot
Copy link
Contributor

ansibot commented Apr 4, 2017

@billwanjohi Greetings! Thanks for taking the time to open this pullrequest. In order for the community to handle your pullrequest effectively, we need a bit more information.

Here are the items we could not find in your description:

  • issue type
  • ansible version
  • component name

Please set the description of this pullrequest with this template:
https://raw.githubusercontent.com/ansible/ansible/devel/.github/PULL_REQUEST_TEMPLATE.md

click here for bot help

@ansibot ansibot removed needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. labels Apr 4, 2017
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 12, 2017
@ansibot ansibot added the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label Jun 29, 2017
@ansibot ansibot added the cli/ label Sep 7, 2017
@ansibot ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 2, 2018
@jstoja
Copy link

jstoja commented Sep 13, 2018

I don't think this relates very well to ssh-agent or pgsql .pgpass file that are clear text. Ansible vault like is encrypted, which means that even if you have access to it, you cannot read its content. Limiting the permission too is nice, but if it's not done, I don't think it deserves any warning. cc @bcoca

@kenyon
Copy link
Contributor

kenyon commented Nov 23, 2019

I don't think this relates very well to ssh-agent or pgsql .pgpass file that are clear text. Ansible vault like is encrypted, which means that even if you have access to it, you cannot read its content. Limiting the permission too is nice, but if it's not done, I don't think it deserves any warning. cc @bcoca

This is about the vault password file, not the vault data files themselves. That is, the --vault-password-file referred to in ansible-vault.

@jstoja
Copy link

jstoja commented Nov 26, 2019 via email

@kenyon
Copy link
Contributor

kenyon commented Nov 26, 2019

IMO if a group is sharing a vault, each user should have their own password file (if they want). That way restrictive permissions could be enforced as suggested.

@ansibot ansibot added the needs_triage Needs a first human triage before being processed. label May 17, 2020
@mkrizek mkrizek removed the needs_triage Needs a first human triage before being processed. label May 18, 2020
@ansibot ansibot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Jul 6, 2020
@samdoran samdoran closed this Jul 8, 2020
@samdoran samdoran reopened this Jul 8, 2020
@ansibot ansibot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jul 8, 2020
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 16, 2020
@ansibot ansibot added pre_azp This PR was last tested before migration to Azure Pipelines. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 8, 2020
@bcoca
Copy link
Member

bcoca commented Feb 2, 2022

closing due to inactivity, if the proposed modifications are addressed, feel free to resubmit as a new PR.

@bcoca bcoca closed this Feb 2, 2022
@ansible ansible locked and limited conversation to collaborators Feb 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.3 This issue/PR affects Ansible v2.3 c:cli/ cli/ feature This issue/PR relates to a feature request. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. pre_azp This PR was last tested before migration to Azure Pipelines. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants