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

quote path name before executing setfacl / getfacl #36874

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
6 participants
@sebcmp

sebcmp commented Feb 28, 2018

SUMMARY

This PR quotes the path name before executing setfacl / getfacl.
It also adjust the test to use a path name with spaces

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

file ACL module

lib/ansible/modules/files/acl.py

ANSIBLE VERSION

2.6

ADDITIONAL INFORMATION

acl.py performs os.path.exists() before executing setfacl / getfacl. However, if path contains spaces, and is escaped in the playbook, os.path.exists() will fail due to the added quotes. If the path is not quoted setfacl / getfacl will fail.

@ansibot

This comment has been minimized.

Contributor

ansibot commented Feb 28, 2018

@sebcmp

This comment has been minimized.

sebcmp commented Mar 5, 2018

!component =lib/ansible/modules/files/acl.py

@bcoca

bcoca approved these changes Mar 5, 2018

@bcoca bcoca requested review from abadger and dagwieers Mar 5, 2018

@@ -33,4 +33,4 @@
test_user: ansible_user
test_group: ansible_group
test_file: /tmp/ansible_file

This comment has been minimized.

@abadger

abadger Mar 5, 2018

Member

Should we change test_file to have spaces as well?

This comment has been minimized.

@sebcmp

sebcmp Jun 13, 2018

No, because shlex.quote() behaves differently in each cases, it only escapes when needed.
Both cases should be tested.

>>> shlex.quote('/tmp/file')
'/tmp/file'
>>> shlex.quote('/tmp/file space')
"'/tmp/file space'"
@@ -201,7 +202,7 @@ def build_command(module, mode, path, follow, default, recursive, entry=''):
if default:
cmd.insert(1, '-d')
cmd.append(path)
cmd.append(shlex_quote(path))

This comment has been minimized.

@abadger

abadger Mar 5, 2018

Member

I think instead of quoting here we should instead pass the list of arguments into run_command on line 227. The code would then look something like this:

 def run_acl(module, cmd, check_rc=True):
 
     try:
         (rc, out, err) = module.run_command('cmd, check_rc=check_rc)
     except Exception as e:
         module.fail_json(msg=to_native(e))

This comment has been minimized.

@abadger

abadger Mar 5, 2018

Member

That should have been:

         (rc, out, err) = module.run_command(cmd, check_rc=check_rc)

This comment has been minimized.

@sebcmp

sebcmp Jun 13, 2018

In the python spirit, quoting seems to be the most explicit thing to do?

" ".join(cmd)

doesn't explain why escaping is needed. Quoting is also more secure.

This comment has been minimized.

@abadger

abadger Jun 13, 2018

Member

Passing a list of command srgs to run,_commsnd means that the Shell is never involved. Instead each element of the list is a single argument. That should be more secure than attempting to quote.

@ansibot ansibot added the stale_ci label Mar 13, 2018

@abadger

This comment has been minimized.

Member

abadger commented Mar 20, 2018

needs_revision

@ansibot ansibot added needs_revision and removed core_review labels Mar 20, 2018

@abadger

This comment has been minimized.

Member

abadger commented Mar 20, 2018

@sebcmp are you interested in making the suggested changes?

@ansibot ansibot removed the stale_ci label Mar 22, 2018

@mattclay

This comment has been minimized.

Member

mattclay commented Mar 23, 2018

CI failure in integration tests: https://app.shippable.com/github/ansible/ansible/runs/58869/40/tests

test/integration/targets/acl/tasks/acl.yml:36 / [testhost] testhost: acl : Grant ansible user read access to a file etype=user, path={{ test_file }}, state=present, permissions=r, entity={{ test_user }}

failure: setfacl: Option -m: Invalid argument near character 1

{
"changed": false, 
"cmd": "/usr/bin/setfacl --test '-m \"user:ansible_user:r\"' '/root/ansible_testing/ansible file'", 
"msg": "setfacl: Option -m: Invalid argument near character 1", 
"rc": 2, 
"stderr": "setfacl: Option -m: Invalid argument near character 1\n", 
"stderr_lines": [
"setfacl: Option -m: Invalid argument near character 1"
], 
"stdout": "", 
"stdout_lines": []
}

@ansibot ansibot added the stale_ci label Mar 31, 2018

@ansibot ansibot added the affects_2.6 label May 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment