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

ansible_mitogen: Handle unsafe paths in _remote_chmod #1087

Merged
merged 2 commits into from
Aug 30, 2024

Conversation

jrosser
Copy link

@jrosser jrosser commented Jul 16, 2024

This is missing from b822f20

Seems to be only triggered when using with_items and the destination filenames have an extension.

The rather surprising behaviour of only failing when there is a filename extension is due to https://github.com/ansible/ansible/blob/906c969b551b346ef54a2c0b41e04f632b7b73c2/lib/ansible/plugins/action/copy.py#L294-L300

The temporary file name created by the copy module gets the source path extension appended if the source file has an extension (i.e. contains a dot). Something in Ansible wraps the filename in an unsafe and because the filename is unsafe, so is the extension pulled from it and therefore so is the temporary file name once the extension gets appended.

- hosts: targets
  gather_facts: false
  vars:
    pip_proxy: "test-proxy"
    use_pip_proxy: true
  tasks:
    - name: Create pip.conf if pip_proxy is defined
      template:
        src: pip.conf.j2
        dest: "{{ item }}"
        mode: u=rw,g=r,o=r
      when:
        - pip_proxy is defined
        - pip_proxy | length != 0
        - use_pip_proxy
      with_items:
        - "/tmp/foo.conf"  # <- must have a file extension to fail
        - "/tmp/bar.conf"
Traceback (most recent call last):
  File "/Users/jrosser/cloud/mitogen-richh/mitogen/ansible_mitogen/mixins.py", line 172, in fake_shell
    rc = func()
         ^^^^^^
  File "/Users/jrosser/cloud/mitogen-richh/mitogen/ansible_mitogen/mixins.py", line 281, in <lambda>
    return self.fake_shell(lambda: mitogen.select.Select.all(
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jrosser/cloud/mitogen-richh/mitogen/mitogen/select.py", line 152, in all
    return list(msg.unpickle() for msg in cls(receivers))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jrosser/cloud/mitogen-richh/mitogen/mitogen/select.py", line 152, in <genexpr>
    return list(msg.unpickle() for msg in cls(receivers))
                ^^^^^^^^^^^^^^
  File "/Users/jrosser/cloud/mitogen-richh/mitogen/mitogen/core.py", line 1009, in unpickle
    raise obj
mitogen.core.CallError: mitogen.core.StreamError: cannot unpickle 'ansible.utils.unsafe_proxy'/'AnsibleUnsafeText'

@moreati
Copy link
Member

moreati commented Aug 12, 2024

Thank you, would you be able to rebase this, include a changelog entry, and an automated test case? If not don't worry, I can have a go.

@jrosser
Copy link
Author

jrosser commented Aug 12, 2024

I've rebased this, but I'm missing a GH issue to reference in the changelog, and I also failed at the first step of trying to look at adding tests as the instructions here https://github.com/mitogen-hq/mitogen/blob/master/tests/README.md#steps-to-prepare-development-environment reference some scripts that don't seem to exist any more.

@moreati
Copy link
Member

moreati commented Aug 12, 2024

The PR number is good enough for the changelog. I'll try to look at the devenv instructions.

@moreati
Copy link
Member

moreati commented Aug 29, 2024

Seems to be only triggered when using with_items and the destination filenames have an extension.

Holy smoke this bug is specific, even loop: doesn't trigger it. It's also stealth, the exit status of ansible-playbook is 0 despite the error.

➜  tmp ANSIBLE_STRATEGY=mitogen_linear ANSIBLE_STRATEGY_PLUGINS=v312/lib/python3.12/site-packages/ansible_mitogen/plugins/strategy v312/bin/ansible-playbook issue1087_repro.yml

PLAY [u2004.mynet] *************************************************************

TASK [Using with_items] ********************************************************
changed: [u2004.mynet] => (item=/tmp/foo)
ERROR! [mux  42933] 11:42:31.261211 E mitogen.[ssh.u2004.mynet]: raw pickle was: b'\x80\x02(X!\x00\x00\x00kintha-42934-1f78b8f40-49b20120f7q\x00X\x16\x00\x00\x00ansible_mitogen.targetq\x01NX\r\x00\x00\x00set_file_modeq\x02cansible.utils.unsafe_proxy\nAnsibleUnsafeText\nq\x03XF\x00\x00\x00/root/.ansible/tmp/ansible_mitogen_action_20ca83c336859b8f/.source.txtq\x04\x85q\x05Rq\x06X\x03\x00\x00\x00u+xq\x07\x86q\x08cmitogen.core\nKwargs\nq\t}q\n\x85q\x0bRq\x0ctq\r.'
ERROR! [task 42934] 11:42:31.261907 E ansible_mitogen.mixins: While emulating a shell command
Traceback (most recent call last):
  File "/Users/alex/tmp/v312/lib/python3.12/site-packages/ansible_mitogen/mixins.py", line 172, in fake_shell
    rc = func()
         ^^^^^^
  File "/Users/alex/tmp/v312/lib/python3.12/site-packages/ansible_mitogen/mixins.py", line 281, in <lambda>
    return self.fake_shell(lambda: mitogen.select.Select.all(
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alex/tmp/v312/lib/python3.12/site-packages/mitogen/select.py", line 152, in all
    return list(msg.unpickle() for msg in cls(receivers))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alex/tmp/v312/lib/python3.12/site-packages/mitogen/select.py", line 152, in <genexpr>
    return list(msg.unpickle() for msg in cls(receivers))
                ^^^^^^^^^^^^^^
  File "/Users/alex/tmp/v312/lib/python3.12/site-packages/mitogen/core.py", line 1009, in unpickle
    raise obj
mitogen.core.CallError: mitogen.core.StreamError: cannot unpickle 'ansible.utils.unsafe_proxy'/'AnsibleUnsafeText'
  File "<stdin>", line 3860, in _dispatch_one
  File "<stdin>", line 3843, in _parse_request
  File "<stdin>", line 998, in unpickle
  File "<stdin>", line 789, in find_class
  File "<stdin>", line 899, in _find_global
changed: [u2004.mynet] => (item=/tmp/foo.txt)

TASK [Cleanup 1] ***************************************************************
changed: [u2004.mynet] => (item=/tmp/foo)
changed: [u2004.mynet] => (item=/tmp/foo.txt)
[WARNING]: Platform linux on host u2004.mynet is using the discovered Python interpreter at /usr/bin/python3.8, but future installation of another Python interpreter could change the meaning of that path. See
https://docs.ansible.com/ansible-core/2.17/reference_appendices/interpreter_discovery.html for more information.

TASK [Using loop] **************************************************************
changed: [u2004.mynet] => (item=/tmp/foo)
changed: [u2004.mynet] => (item=/tmp/foo.txt)

TASK [Cleanup 2] ***************************************************************
changed: [u2004.mynet] => (item=/tmp/foo)
changed: [u2004.mynet] => (item=/tmp/foo.txt)

PLAY RECAP *********************************************************************
u2004.mynet                : ok=4    changed=4    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   

➜  tmp echo $?
0

➜  tmp v312/bin/ansible-playbook --version
ansible-playbook [core 2.17.3]
  config file = /Users/alex/.ansible.cfg
  configured module search path = ['/Users/alex/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/alex/tmp/v312/lib/python3.12/site-packages/ansible
  ansible collection location = /Users/alex/.ansible/collections:/usr/share/ansible/collections
  executable location = v312/bin/ansible-playbook
  python version = 3.12.5 (v3.12.5:ff3bc82f7c9, Aug  7 2024, 05:32:06) [Clang 13.0.0 (clang-1300.0.29.30)] (/Users/alex/tmp/v312/bin/python3.12)
  jinja version = 3.1.4
  libyaml = True
➜  tmp v312/bin/pip list
Package      Version
------------ -------
ansible      10.3.0
ansible-core 2.17.3
cffi         1.17.0
cryptography 43.0.0
Jinja2       3.1.4
MarkupSafe   2.1.5
mitogen      0.3.9
packaging    24.1
pip          24.2
pycparser    2.22
PyYAML       6.0.2
resolvelib   1.0.1
- hosts: u2004.mynet
  gather_facts: false
  vars:
    foo: Foo
    bar: Bar
  tasks:
    - name: Using with_items
      template:
        src: foo.bar.j2
        dest: "{{ item }}"
        mode: u=rw,go=r
      with_items:
        - /tmp/foo
        - /tmp/foo.txt

    - name: Cleanup 1
      file:
        path: "{{ item }}"
        state: absent
      with_items:
        - /tmp/foo
        - /tmp/foo.txt

    - name: Using loop
      template:
        src: foo.bar.j2
        dest: "{{ item }}"
        mode: u=rw,go=r
      loop:
        - /tmp/foo
        - /tmp/foo.txt

    - name: Cleanup 2
      file:
        path: "{{ item }}"
        state: absent
      with_items:
        - /tmp/foo
        - /tmp/foo.txt

@moreati
Copy link
Member

moreati commented Aug 29, 2024

@jrosser please could you tick "Allow edits and access to secrets by maintainers" on this PR and #1110? This should allow me to push to the branches in question. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

I think I have the finished tests for this PR in master...moreati:mitogen:unsafe-chmod. I may needfurther tweaks once CI has run with it.

@moreati
Copy link
Member

moreati commented Aug 29, 2024

@jrosser please could you tick "Allow edits and access to secrets by maintainers"

On second thoughts that probably won't work. github.com/bbc is an organisation account not a personal account, so the tickbox isn't applicable.

@jrosser
Copy link
Author

jrosser commented Aug 29, 2024

@moreati I have updated my branch with your changes.

@moreati moreati merged commit 84a4fcd into mitogen-hq:master Aug 30, 2024
46 checks passed
moreati added a commit to moreati/mitogen that referenced this pull request Oct 28, 2024
moreati added a commit to moreati/mitogen that referenced this pull request Oct 28, 2024
moreati added a commit to moreati/mitogen that referenced this pull request Oct 28, 2024
moreati added a commit to moreati/mitogen that referenced this pull request Oct 29, 2024
Some ansible_mitogen connection plugins look more like become plugins (e.g.
mitogen_sudo) & use become plugin options. For now there's special handling in
PlayContextSpec._become_option(). Further design/discussion can go in mitogen-hq#1173.

Refs mitogen-hq#1087.
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 this pull request may close these issues.

2 participants