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

Invoke vault password scripts via original path specified #18320

Closed
wants to merge 0 commits into from

Conversation

jmehnle
Copy link

@jmehnle jmehnle commented Nov 2, 2016

ISSUE TYPE
  • Bug Report
COMPONENT NAME

--vault-password-file

ANSIBLE VERSION
ansible 1.9.3

(but also occurs in Ansible 2)

DESCRIPTION

When specifying an executable vault password file, Ansible invokes it by its "real" path, not by the path as specified. This matters if what's passed is actually a symlink to an executable, and the executable uses the path by which it is invoked to determine the password to produce.

For example:

$ ls -l
total 16
-rwxr-xr-x  1 julian  staff  290 Nov  2 23:03 get-password
-rw-------  1 julian  staff  355 Nov  2 23:10 secrets.yml

secrets.yml is a dummy variables vault file created by ansible-vault create with the password foo-bar-ansible.

$ cat get-password
#!/bin/sh

basename="$(basename $0)"
basename_prefix="get-password"
default_password="foo-bar"

case "${basename}" in
  "${basename_prefix}"-*)
    password="${default_password}-${basename#${basename_prefix}-}"
    ;;
  *)
    password="${default_password}"
    ;;
esac

echo "${password}"

This script produces the password foo-bar if invoked directly, but produces a different password when invoked through a symlink that begins with get-password-, e.g., foo-bar-quux when invoked as get-password-quux. This script is a synthetic example, but I actually use this mechanism with a more complicated password retrieval script that installs various symlinks for different applications, Ansible being one of them.

Now lets create a symlink with a suitable name:

$ ln -s get-password get-password-ansible
$ ls -l get-password*
-rwxr-xr-x  1 julian  staff  290 Nov  2 23:03 get-password
lrwxr-xr-x  1 julian  staff   12 Nov  2 23:24 get-password-ansible -> get-password
$ ./get-password
foo-bar
$ ./get-password-ansible
foo-bar-ansible
$ ansible all -i localhost, -c local -a uptime -e @secrets.yml --vault-password-file get-password-ansible
ERROR: Decryption failed

Oops. We're invoking get-password-ansible, which as we just saw, produces the correct password, foo-bar-ansible. What gives?

Well, now let's try replacing the symlink with a straight-up copy of the real file:

$ rm -f get-password-ansible
$ cp get-password get-password-ansible
$ ls -l get-password*
-rwxr-xr-x  1 julian  staff  290 Nov  2 23:03 get-password
-rwxr-xr-x  1 julian  staff  290 Nov  2 23:12 get-password-ansible
$ ./get-password
foo-bar
$ ./get-password-ansible
foo-bar-ansible
$ ansible all -i localhost, -c local -a uptime -e @secrets.yml --vault-password-file get-password-ansible
localhost | success | rc=0 >>
23:12  up 29 days,  2:46, 24 users, load averages: 1.22 1.50 1.58

Oh, hey, this time it worked!

And this is consistent with how the real_vault_password_file function is defined in https://github.com/ansible/ansible/blob/devel/lib/ansible/cli/__init__.py#L613. It calls os.path.realpath() on the file name provided and then invokes the script by that name.

It would be more useful if the vault password file's executability was determined by its real path, etc., but then invoked by its original path.

@ansibot ansibot added affects_2.3 This issue/PR affects Ansible v2.3 bugfix_pullrequest labels Dec 13, 2016
@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. labels Dec 15, 2016
@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_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. labels Jan 2, 2017
@ansibot ansibot added the c:cli/ label Jan 12, 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 11, 2017
@calfonso
Copy link
Contributor

@jmehnle Would you mind rebasing and testing, we believe this has been fixed.

@jmehnle
Copy link
Author

jmehnle commented May 31, 2017

@calfonso, how would this have been fixed? It seems the code still suffers from the same confusion of what my patch calls the "full path" with the "real path". Unless you separate the two out, I don't believe the problem can be fixed.

@ansibot
Copy link
Contributor

ansibot commented May 31, 2017

@jmehnle this PR contains the following merge comits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot ansibot added merge_commit This PR contains at least one merge commit. Please resolve! 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 May 31, 2017
@ansibot ansibot removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels May 31, 2017
@mattclay
Copy link
Member

mattclay commented Jun 5, 2017

Rebase needed.

@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jun 5, 2017
@jmehnle
Copy link
Author

jmehnle commented Jun 5, 2017

I just rebased it the other day. What gives??

@ansibot
Copy link
Contributor

ansibot commented Jun 5, 2017

@jmehnle this PR contains the following merge comits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot ansibot added the merge_commit This PR contains at least one merge commit. Please resolve! label Jun 5, 2017
@ansibot ansibot removed merge_commit This PR contains at least one merge commit. Please resolve! 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. labels Jun 5, 2017
@mattclay
Copy link
Member

mattclay commented Jun 6, 2017

@jmehnle The latest rebase was needed because the file changed by this PR had been modified by other PR(s) which were already merged, causing conflicts with your changes.

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 24, 2017
@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. labels Aug 3, 2017
@ansibot ansibot added the cli/ label Sep 7, 2017
@ansibot ansibot added the new_contributor This PR is the first contribution by a new community member. label Oct 18, 2017
@ansibot ansibot removed the new_contributor This PR is the first contribution by a new community member. label Nov 3, 2017
@ansibot ansibot added the new_contributor This PR is the first contribution by a new community member. label Jan 22, 2018
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 2, 2018
@sivel
Copy link
Member

sivel commented Nov 30, 2018

This issue still exists, but the code has change quite a lot.

I think this would be a good change, as I understand the reasons behind it.

The new code is located at:

def get_file_vault_secret(filename=None, vault_id=None, encoding=None, loader=None):
this_path = os.path.realpath(os.path.expanduser(filename))

I think we just need to change realpath to abspath there.

bcoca added a commit to bcoca/ansible that referenced this pull request Nov 30, 2018
  keeps context of the symlink instead of original file path
  fixes ansible#18320
@bcoca
Copy link
Member

bcoca commented Nov 30, 2018

@jmehnle can you confirm that PR above resolves your issue?

@jmehnle jmehnle closed this Jul 2, 2019
bcoca added a commit to bcoca/ansible that referenced this pull request Jul 3, 2019
  keeps context of the symlink instead of original file path
  fixes ansible#18320
@ansible ansible locked and limited conversation to collaborators Aug 5, 2019
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 bug This issue/PR relates to a bug. c:cli/ cli/ 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. new_contributor This PR is the first contribution by a new community member. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. 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

6 participants