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

Recent bugfix prevents me from using Jinja2 if/else/endif syntax in a task #8233

Closed
drybjed opened this issue Jul 22, 2014 · 54 comments
Closed

Recent bugfix prevents me from using Jinja2 if/else/endif syntax in a task #8233

drybjed opened this issue Jul 22, 2014 · 54 comments
Labels

Comments

@drybjed
Copy link
Contributor

@drybjed drybjed commented Jul 22, 2014

Issue Type:

Bug Report

Ansible Version:

v1.6.7+, devel

Environment:

Debian, Ubuntu

Summary:

Recent security fix prevents me from using Jinja2 {% if %}{% else %}{% endif %} syntax in a task. This prevents me from creating more flexible roles that can be easily customized using inventory lists. Basically 80% of my playbook is based around this... :-(

Steps To Reproduce:

---
# An example play which lets you customize the directory names using variables from inventory
- hosts: all
  vars:
    dir_list:
      - name: 'directory'
        prefix: 'my_'
      - name: 'other_directory'
  tasks:
    - name: Create example directories
      file: state=directory
            {% if item.prefix is defined and item.prefix %}
            dest=/tmp/{{ item.prefix + item.name }}
            {% else %}
            dest=/tmp/{{ item.name }}
            {% endif %}
      with_items: dir_list
Expected Results:

Two directories are created, /tmp/my_directory/ and /tmp/other_directory/.

Actual Results:
PLAY [all] ******************************************************************** 

GATHERING FACTS *************************************************************** 
ok: [winterfell]

TASK: [Create example directories] ******************************************** 
fatal: [winterfell] => a duplicate parameter was found in the argument string (dest)

FATAL: all hosts have already failed -- aborting

PLAY RECAP ******************************************************************** 
           to retry, use: --limit @/home/drybjed/test_dest.retry

winterfell                 : ok=1    changed=0    unreachable=1    failed=0   
@srgvg
Copy link
Member

@srgvg srgvg commented Jul 22, 2014

I also happen to use Jinja conditionals in tasks, this seems a big regression to me too.

@nickjj
Copy link

@nickjj nickjj commented Jul 22, 2014

I use this as well. It would be a shame if it went away.

It's really handy for certain use cases, for example:
https://github.com/nickjj/ansible-ferm/blob/v0.1.2/tasks/main.yml#L33-L37

sthaha added a commit to redhat-openstack/khaleesi that referenced this issue Jul 22, 2014
Ansible from version 1.6.7 fails if there is a duplicate
initialization of a variable.
Refer: ansible/ansible#8233

Change-Id: Ia25c59c6230a3169286798c875979a836ea0a327
@bcoca
Copy link
Member

@bcoca bcoca commented Jul 22, 2014

I would just rewrite the example:

 tasks:
    - name: Create example directories
      file: state=directory
            dest=/tmp/{{ item.prefix|default('') + item.name }}
@Ernest0x
Copy link
Contributor

@Ernest0x Ernest0x commented Jul 22, 2014

I have done some quick analysis on it.

The idea of the recent security fix is this: count parameters before and after rendering the template and if the counts are different, fail. This is done to prevent from a var being crafted so that it injects a param when it is rendered by jinja. The failure message comes from an earlier check for duplicate params while counting the parameters.

To fix this situation, the check for duplicate params should be done only on the rendered version. Also, not a count of, but a specific list of the exact parameters found for each version should be calculated. Then, check that the list of parameters found on the rendered version is a subset of the list of parameters found in the original version, or else fail. Theoretically, this is even more secure than just counting, because the counts could be the same but the actual parameters different.

I could provide a fix for this on my own, but since this is security-related, I think my suggestion should be first reviewed by more people.

renard referenced this issue Jul 22, 2014
* Strip lookup calls out of inventory variables and clean unsafe data
  returned from lookup plugins (CVE-2014-4966)
* Make sure vars don't insert extra parameters into module args and prevent
  duplicate params from superseding previous params (CVE-2014-4967)
@mpdehaan
Copy link
Contributor

@mpdehaan mpdehaan commented Jul 22, 2014

Ansible has never endorsed Jinja2 inside of playbooks much more than to assign variables.

if you remember, the first support was $foo and we didn't have this at all.

Indeed, we cannot use untrusted variables to ADD parameters, not just provide duplicates.

We'll look at this a bit deeper, but there is a reason Ansible has a "when" operator, it's to make cleaner playbooks, and it should be used.

In this case the value of the variable should be defined like this:

install_path: "{% if x %}foo{% else %}bar{% endif %}"

In which case your playbook doesn't have the hideous inline YAML and you can just do

path={{ install_path }}

and things like that.

That being said, my finding of your syntax overcomplicated and non-ansible like (there's a reason the docs don't show it this way) doesn't mean we won't evaluate this a bit closer.

@abadger
Copy link
Member

@abadger abadger commented Jul 22, 2014

There's a second problem that @Ernest0x's solution doesn't 100% fix. Say you're using the command module in a playbook:

  vars:
    real_world_command: "virt-install -n {{ inventory_hostname }} -r {{ mem_size }}
                 --disk bus=virtio,path={{ volgroup }}/{{ inventory_hostname }}
                 --vcpus={{ num_cpus }}  -l {{ ks_repo }} -x
                 ''ksdevice=eth0 ks={{ ks_url }} console=tty0 console=ttyS0
                  hostname={{ inventory_hostname }} nameserver={{ dns }}
                  ip={{ eth0_ip }}::{{ gw }}:{{ nm }}:{{ inventory_hostname }}:eth0:none''
                 --network=bridge=br0,model=virtio
                 --autostart --noautoconsole"
    even_worse_contrived_cmd: echo chdir=test is going to cause problems >> ansible.test
  tasks:
    - shell: "{{ real_world_command }}"
    - shell: "{{ even_worse_contrived_command }}"

The above real_world_command will continue to fail even if the check is done post render as console= is present twice in the command. It will probably be okay if the runner is taught to only check for parameters that the module actually uses as "console" isn't a parameter to the command/shell module.

The contrived command is more problematic as it contains a substring that would be recognized as a valid parameter (chdir is a valid parameter to the command and shell module). A brief look at the code last night makes me think that this will be much harder to fix. In the 0.6.6 code, the "chdir=test" would be treated as a parameter to command. In the 0.6.7 code it will raise an error. What we'd really need is for ansible to recognize that the "chdir=test" is part of the "free_form" parameter to command/shell and not a new parameter. That change has to be added in both the checking code and the module code that parses the parameters.

@drybjed
Copy link
Contributor Author

@drybjed drybjed commented Jul 22, 2014

@mpdehaan I'm using {% if %} syntax in tasks to basically move logic inside public playbook and allow configuration of various parameters using inventory variables, for example this inline YAML syntax allows me to define simple list of users, like:

users_list:
  - name: 'user1'
  - name: 'user2'
    groups: [ 'admins', 'sshusers' ]

in inventory/host_vars/hostname.yml and have my users role take that list and create accounts automatically. Almost all values are optional and have sensible defaults to make definition of this configuration easier.

Because of that my playbook can be completely public and used in different environments, and inventory can stay private and contain actual configuration.

I'm afraid that without {% if %} functionality this whole idea of writing playbook like that becomes very limited, or even impossible to do in a way that I would want.

@Ernest0x
Copy link
Contributor

@Ernest0x Ernest0x commented Jul 22, 2014

It will probably be okay if the runner is taught to only check for parameters that the module actually uses as "console" isn't a parameter to the command/shell module.

Yes, this is safer.

In the 0.6.6 code, the "chdir=test" would be treated as a parameter to command. In the 0.6.7 code it will raise an error.

What error does it raise?

@abadger
Copy link
Member

@abadger abadger commented Jul 22, 2014

@Ernest0x: fatal: [127.0.0.1] => A variable inserted a new parameter into the module args. Be sure to quote variables if they contain equal signs (for example: "{{var}}").

Which is correct to some extent. The code in 0.6.6 is going to treat these as equivalent:

vars:
  command: "echo 'chdir=test is problematic'"
tasks:
  - shell: "{{ command }}"
  - shell: "echo 'is problematic'"
    args:
      - chdir: test

which is probably not what was intended. So the error in 0.6.7 is better but still leaves a cornercase unresolved.

@Ernest0x
Copy link
Contributor

@Ernest0x Ernest0x commented Jul 22, 2014

@abadger: I see. I wonder if a filter named 'ansible_safe' or something like this would make sense for these cases. For example:

{{ expression | ansible_safe }} would exclude that jinja expression from the 'inserted a new param' check.

@sthaha
Copy link

@sthaha sthaha commented Jul 22, 2014

Could you confirm that this fix essentially disables something like
set_fact: "{{ lookup('template', 'add_more_vars.j2') }}"

where add_more_vars.j2 contains:
{{foo}}='{{bar}}'

@renard
Copy link

@renard renard commented Jul 22, 2014

As I said before:

Until that commit I used to have:

tmpfs_tar_options: "--preserve-permissions --numeric-owner --auto-compress --xattrs --xattrs-include='*' --selinux --acls"
- name: create archive
  shell: >-
    tar {{ tmpfs_tar_options }}
    {% for item in tmpfs_tar_exclude %} --exclude '{{ item }}'{% endfor %}
    {% for item in tmpfs_tar_exclude_extra|default([]) %} --exclude '{{ item }}'{% endfor %}
    --exclude '{{tmpfs_target[1:]}}'
    --directory /
    --create --file /{{tmpfs_target}}/{{ansible_hostname}}_rootfs{{ tmpfs_tar_ext }} .

The error message is not very clear:

A variable inserted a new parameter into the module args. Be sure to quote variables if they contain equal signs (for example: "{{var}}").

Maybe I do not declare correctly my variable. Anyway how should I declare a variable with an equal sign in a string?

I guess many people are OK with changes as long as it does not involve rewriting all code.

On the other hand I was wondering what is the difference between the two syntaxes:

- name: Create groups
  group:
    name: '{{ item.name }}'
    gid: '{{ item.gid }}'
    state: '{{ item.state|default("present")}}'
    system: '{{ item.state|default("no")}}'
  with_items: credentials_groups
  when: credentials_groups is defined

and

- name: Create groups
  group:
    name='{{ item.name }}'
    gid='{{ item.gid }}'
    state='{{ item.state|default("present")}}'
    system='{{ item.state|default("no")}}'
  with_items: credentials_groups
  when: credentials_groups is defined
@abadger
Copy link
Member

@abadger abadger commented Jul 22, 2014

For your specific issue, can you change your variable to this?

tmpfs_tar_options: "--preserve-permissions --numeric-owner --auto-compress --xattrs --xattrs-include '*' --selinux --acls"

That's only a workaround but for a lot of command line tools changing the equals into a space should work.

drybjed added a commit to drybjed/ginas that referenced this issue Jul 22, 2014
This change is due to the security fix in Ansible v1.6.7 and problems
with old playbook format used in ginas. More information:
ansible/ansible#8233

This is a temporary change to allow currently working code in a pull
request to be merged cleanly into main repository.
@sontek
Copy link

@sontek sontek commented Jul 22, 2014

I'm also seeing this problem with the following task:

- name: installing services without requirements.txt
  command:
    /opt/webapp/{{ sm_app_role }}/bin/pip install
      -e /opt/src/{{ sm_app_role }}
      -i {{ pypackage_repo }}
      --find-links=/opt/pip/wheels --find-links=/opt/pip/cache

I get this error:

fatal: [192.168.21.254] => a duplicate parameter was found in the argument string (--find-links)

@sontek
Copy link

@sontek sontek commented Jul 22, 2014

Note that mine is not using conditionals, just basic variable expansion.

@renard
Copy link

@renard renard commented Jul 22, 2014

@abadger yes that workaround for that particular use case

@SegFaultAX
Copy link
Contributor

@SegFaultAX SegFaultAX commented Jul 22, 2014

This problem is actually much more basic (and insidious). We have something like the following in a vars file:

accounts:
  - username: SegFaultAX
    full_name: Michael-Keith Bernard
    ssh_pubkeys:
      - "ssh-rsa 12356== myawesomepubkey@somemachine.com"

Note that the ssh pubkey is simply a base64 encoded string and therefore may contain an =. When any module (in this case authorized_key) tries to use the pubkey string, it fails with the following error (where it previously worked on 1.4 - 1.6.6):

fatal: [herp_derp_host] => A variable inserted a new parameter into the module args. Be sure to quote variables if they contain equal signs (for example: "{{var}}").
@renard
Copy link

@renard renard commented Jul 22, 2014

@SegFaultAX do you have the same error with single quotes?

@maxim
Copy link

@maxim maxim commented Jul 22, 2014

I'm having "kind of" the same issue on 1.6.7 when trying to pipe some variable into a local shell script:

- name: build env directory
  local_action: >
    shell echo "{{ env_dict | to_nice_yaml }}" | utils/buildenv

Since this env_dict contains some references to other variables that aren't always parsed with j2, I get this error:

Error:

A variable inserted a new parameter into the module args. Be sure to quote variables if they contain equal signs (for example: "{{var}}").

This works without error on 1.6.6.

Update: looks like there are no j2 snippets in the env_dict, so just plain yaml causes this error.

@jamesmoriarty
Copy link

@jamesmoriarty jamesmoriarty commented Jul 23, 2014

I'm seeing this on ~1.6.8.

bundle config build.pg --with-pg=/usr/pgsql-9.3
@jsmirnov
Copy link

@jsmirnov jsmirnov commented Jul 23, 2014

I'am getting problems with strings.

- name: Sudoers | update sudoers file and validate
  lineinfile: "dest=/etc/sudoers
      insertafter=EOF
      line='{{ item.name }} ALL=(ALL) NOPASSWD: ALL'
      regexp='{{ item.name }} ALL=\\(ALL\\) NOPASSWD: ALL'
      state=present
      validate='visudo -cf %s'"
  with_items: ssh_users

Ansible doesn't like ALL duplication.

@ghost
Copy link

@ghost ghost commented Jul 23, 2014

Ansible 1.6.8:

- name: Unarchive something
  unarchive:
    src={{ var_1 }}/{{ var_2 }}
    dest={{ var_3 }}
msg: duplicate parameter: src
@matthewbsimon
Copy link

@matthewbsimon matthewbsimon commented Jul 23, 2014

Spul, I posted more details about this last one in #8254

@renard
Copy link

@renard renard commented Jul 23, 2014

@jsmirnov @spul have try to use : instead if = as parameter separator?

line: '{{ item.name }} ALL=(ALL) NOPASSWD: ALL'

I don't really understand the real difference between both style but sometimes it seems to be ok with colon instead of equal sign.

@jsmirnov
Copy link

@jsmirnov jsmirnov commented Jul 23, 2014

@renard
Unfortunately no, I'v played with different ways of calling command itself, but still:
a duplicate parameter was found in the argument string (ALL)

@mpdehaan
Copy link
Contributor

@mpdehaan mpdehaan commented Jul 24, 2014

All, since this is a bit of a miscellaneous ticket now, just wanted to update you all and call your attention to some new integration tests.

This contains a list of MOST things above, and once clear, we'll know they are all resolved. As such, if there are parsing scenarios you think are uncovered, adding them to the test here can help solve them.

Some things like Jinja2 param duplications do need to be handled carefully and will be a little different, but I still like @Ernest0x's plan above. In any event, this is a codification of the above issues and proof they remain operational.

Again, stay tuned, as we're working on this one. Thanks!

https://github.com/ansible/ansible/blob/devel/test/integration/roles/test_good_parsing/tasks/main.yml

@mpdehaan
Copy link
Contributor

@mpdehaan mpdehaan commented Jul 24, 2014

All, you can see fixes applied to the devel branch now. All of the integration tests in "test_good_parsing" and "test_bad_parsing" pass now.

There are some duplicate args things like what are mentioned above that are legit error conditions that we still need to filter, though this resolves nearly all false positives.

Please take a look if you would like to try the development branch, and see how this works for you. Note if I've replied and said this is a condition that needs to be filtered out, that remains true in most cases.

@jsmirnov
Copy link

@jsmirnov jsmirnov commented Jul 24, 2014

@mpdehaan
Just tried on devel branch. Unfortunately same result with:

- name: Sudoers | update sudoers file and validate
  lineinfile: "dest=/etc/sudoers
      insertafter=EOF
      line='{{ item.name }} ALL=(ALL) NOPASSWD: ALL'
      regexp='{{ item.name }} ALL=\\(ALL\\) NOPASSWD: ALL'
      state=present
      validate='visudo -cf %s'"
  with_items: ssh_users

a duplicate parameter was found in the argument string (ALL)

Also I had an issue with lookup:
could not locate file in lookup: manager.pub
While file actually exists in /roles/role/files/

---
ssh_users:
  - name: manager
    key: "{{ lookup('file', 'manager.pub') }}"
    shell: /bin/bash
@mpeters
Copy link
Contributor

@mpeters mpeters commented Jul 24, 2014

I can trigger this same error using what I would think is supported and encouraged syntax given this conversation.

  tasks:
    - shell: "id -u sg"
      register: sg_user
      ignore_errors: true

    - set_fact: run_bootstrap="{% if (sg_user is defined) and (sg_user.rc == 0) %}false{% else %}true{% endif %}"

Gives "A variable inserted a new parameter into the module args. Be sure to quote variables if they contain equal signs (for example: "{{var}}")."

I tried quoting the whole line after "set_fact:" with the same results.

@nocive
Copy link

@nocive nocive commented Jul 25, 2014

Same problem started happening to me with a very straightforward task:

- name:  create ssl certificate
  shell: "echo '{{ certificate.crt.content }}' > {{ certificate_path }}/{{ certificate.crt.name }}"
  notify: restart nginx
  when: vhost_ssl_enabled
  tags: [configuration,ssl,nginx]

Throws error "A variable inserted a new parameter into the module args. Be sure to quote variables if they contain equal signs (for example: "{{var}}")."

Had to downgrade to 1.6.6 until this gets fixed.

@renard
Copy link

@renard renard commented Jul 25, 2014

@mpdehaan Ok fair enough or the dynamic script generation. Thanks

@jimi-c
Copy link
Member

@jimi-c jimi-c commented Jul 25, 2014

@mpeters I can confirm your example works now in 1.6.9/devel:

# ansible-playbook -vv 8233_mpeters.yml 
PLAY [localhost] ************************************************************** 
TASK: [shell id -u sg] ******************************************************** 
<127.0.0.1> REMOTE_MODULE command id -u sg #USE_SHELL
failed: [127.0.0.1] => {"changed": true, "cmd": "id -u sg", "delta": "0:00:00.007091", "end": "2014-07-25 10:09:45.608184", "rc": 1, "start": "2014-07-25 10:09:45.601093"}
stderr: id: sg: no such user
...ignoring
TASK: [set_fact run_bootstrap="true"] ***************************************** 
ok: [127.0.0.1] => {"ansible_facts": {"run_bootstrap": "true"}}
PLAY RECAP ******************************************************************** 
127.0.0.1                  : ok=2    changed=1    unreachable=0    failed=0   
@jimi-c
Copy link
Member

@jimi-c jimi-c commented Jul 25, 2014

@nocive can you also test with 1.6.9/devel to see if your issue is resolved?

@jimi-c
Copy link
Member

@jimi-c jimi-c commented Jul 25, 2014

@jsmirnov I can confirm that your example also works on 1.6.9/devel now, using the following example playbook:

- hosts: localhost
  connection: local
  gather_facts: no
  tasks:
  - copy: "dest=/tmp/8233_jsmirnov.txt content='start ALL=(ALL) NOPASSWD: ALL\n'"
  - name: Sudoers | update sudoers file and validate
    lineinfile: "dest=/tmp/8233_jsmirnov.txt insertafter=EOF line='{{ item.name }} ALL=(ALL) NOPASSWD: ALL' regexp='{{ item.name }} ALL=\\(ALL\\) NOPASSWD: ALL' state=present validate='visudo -cf %s'"
    with_items:
    - { name: foo } 
    - { name: bar }
    - { name: baz }

And after the run, the /tmp file contains:

$ cat /tmp/8233_jsmirnov.txt 
start ALL=(ALL) NOPASSWD: ALL 
foo ALL=(ALL) NOPASSWD: ALL
bar ALL=(ALL) NOPASSWD: ALL
baz ALL=(ALL) NOPASSWD: ALL
@mpeters
Copy link
Contributor

@mpeters mpeters commented Jul 25, 2014

@jimi-c Confirmed, ansible 1.7 (devel 57f89b8) does now work. Thanks!

@nocive
Copy link

@nocive nocive commented Jul 25, 2014

@jimi-c yep, just give me a few minutes to set it up.

@nocive
Copy link

@nocive nocive commented Jul 25, 2014

@jimi-c I can confirm it works with 1.6.9

@jimi-c
Copy link
Member

@jimi-c jimi-c commented Jul 25, 2014

Excellent. Are there any other issues above that we've missed or not addressed? Definitely a little difficult to parse through all the comments to be sure.

@jsmirnov
Copy link

@jsmirnov jsmirnov commented Jul 25, 2014

@jimi-c
Tested on 1.6.9 - working fine. So probably I will switch from 1.6.6 to 1.6.9(hope ppa/pip will update soon). Thanks a lot for fixing.
Test on devel - the case with ALL=(ALL) NOPASSWD: ALL is not reproducible.

But on devel I am still experiencing problem which I mentioned at: #8233 (comment)

I am still getting issue with:

could not locate file in lookup: manager.pub
While file actually exists in /roles/role/files/manager.pub

part of /roles/role/vars/main.yml

---
ssh_users:
  - name: manager
    key: "{{ lookup('file', 'manager.pub') }}"
    shell: /bin/bash
@sontek
Copy link

@sontek sontek commented Jul 25, 2014

I'll run my use cases when I get into the office, this is looking much better though

@sontek
Copy link

@sontek sontek commented Jul 25, 2014

+1 our scripts work with the devel branch checked out.

@jimi-c
Copy link
Member

@jimi-c jimi-c commented Jul 25, 2014

@jsmirnov thanks, I'll look into the lookup issue.

@jimi-c
Copy link
Member

@jimi-c jimi-c commented Jul 25, 2014

@jsmirnov I've tried to reproduce this, however I haven't had any luck. Could you please open a new issue for this, with a full playbook that reproduces the issue?

As for this issue, we're going to go ahead and close it out. If there are any further outstanding issues we've missed, please open new issues for them. Thanks!

@jimi-c jimi-c closed this Jul 25, 2014
romeOz added a commit to romeOz/rock-template that referenced this issue Jul 27, 2014
romeOz added a commit to romeOz/rock-cache that referenced this issue Jul 27, 2014
Zharktas added a commit to vrk-kpa/opendata that referenced this issue Jul 29, 2014
…#8233, playbooks work on versions < 1.6.6 and > 1.6.9
@aikomastboom
Copy link

@aikomastboom aikomastboom commented Sep 18, 2014

ansible 1.7.1

fatal: [hostname] => a duplicate parameter was found in the argument string (--exclude)

- name: force restart networking
  raw: "ifdown --exclude=lo -a ; ifup --exclude=lo -a"
rndmh3ro referenced this issue in dev-sec/ansible-os-hardening May 31, 2015
@ansibot ansibot added bug and removed bug_report labels Mar 6, 2018
@ansible ansible locked and limited conversation to collaborators Apr 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.