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

Quote removal off-by-one (on ansible core 2.11.2) #34

Open
grembo opened this issue Jul 31, 2021 · 8 comments · May be fixed by #36
Open

Quote removal off-by-one (on ansible core 2.11.2) #34

grembo opened this issue Jul 31, 2021 · 8 comments · May be fixed by #36
Assignees

Comments

@grembo
Copy link
Contributor

grembo commented Jul 31, 2021

Changing this line

cmd = sudoless[len(quotes):-len(quotes+'?')]

to

    cmd = sudoless[len(quotes):-len(quotes)] 
    if cmd.endswith("'"):
        cmd = cmd[:-1]

fixes the issue (even if it's just a workaround).

Example playbook that fails without the patch:

---
- hosts: all
  gather_facts: False
  
  tasks:
   - name: install python
     raw: test -e /usr/local/bin/python || ( pkg install -y python )
@mcgaw
Copy link

mcgaw commented Aug 9, 2021

- hosts: [email protected]
  name: Bootstrap Python
  gather_facts: no
  tasks:
    # last '_' is removed by bug in sshjail
    - name: Bootstrap Python
      raw: py_boot_

Have also had to work around this. py_boot is copied during jail creation. (excuse the hack!)

austinhyde added a commit that referenced this issue Aug 15, 2021
Fixes #34. There are some cases where quote stripping (needed when
ansible would otherwise try to run sudo in a jail) would remove too
little or too many quotes. This attempts to rectify this using
shlex-based parsing (mostly).
@austinhyde austinhyde linked a pull request Aug 15, 2021 that will close this issue
@austinhyde
Copy link
Owner

@grembo @mcgaw I've put up PR #36 which should fix this. If you get some time in the next week or so, give that a try and let me know if it works for your cases. If you don't get to it, no worries; I tested a few cases, and I'm confident it works correctly for the cases we've encountered to date.

@mcgaw
Copy link

mcgaw commented Aug 30, 2021

@austinhyde I tested your PR this morning by removing my hack, documented above, and it seems to work now. Thanks for spending time on this!

@grembo
Copy link
Contributor Author

grembo commented Aug 30, 2021

@mcgaw @austinhyde I commented on #36, so in general it works, but having a minimum ansible version check would be useful - opened #37 to address this.

@mcgaw
Copy link

mcgaw commented Aug 30, 2021

@grembo I'm using ansible 2.10.6, which seems to be less than the minimum ansible version you mentioned? Sorry if I've picked you up wrong.

@grembo
Copy link
Contributor Author

grembo commented Aug 30, 2021

@mcgaw It seems like this was due to a bug introduced in ansible in ansible/ansible@935528e#diff-38cec806ea1a1ee7c3a286c7865334ecfba7ccc49e21d4e8fb8ec1b17938fda6, which affected 2.11.0, 2.11.1, and 2.11.2. It was corrected it ansible/ansible@a2239d8, which made it to 2.11.3.

So maybe minimum version isn't exactly the right way of addressing this (could be useful to have such a check anyway).

@durin42
Copy link

durin42 commented Dec 20, 2021

I mentioned it on the PR too, but I'm seeing the same quoting bug on 2.12.1.

@grembo
Copy link
Contributor Author

grembo commented Dec 21, 2021

I mentioned it on the PR too, but I'm seeing the same quoting bug on 2.12.1.

Yes, like mentioned on the PR, this is a bug in sshjail that is fixed in #36 (which probably should land/be released). The problems with specific ansible versions were unrelated and just noticed while testing the change.

wombelix pushed a commit to wombelix/fork_austinhyde_ansible-sshjail that referenced this issue Mar 26, 2022
Fixes austinhyde#34. There are some cases where quote stripping (needed when
ansible would otherwise try to run sudo in a jail) would remove too
little or too many quotes. This attempts to rectify this using
shlex-based parsing (mostly).
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 a pull request may close this issue.

4 participants