no-log-password rule should be removed

[felixfontein]

The no-log-password rule (added in #1558) makes no sense, since modules that accept *password* fields must mark these with no_log (or they will fail ansible-test's sanity tests), which explicitly avoids logging their contents. Also, currently Ansible has some built-in protection which will mark a *password* field no_log=true if not specified by the module author (see https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/basic.py#L1383) and emits a warning so users will see that the module is buggy (admittedly it will not match things like userpassword due to the way the regex is selected, that's covered by the sanity tests though).

This rule forces users to either add no_log: true to all tasks with *password* options, which is very damaging since it prohibits any kind of logging by Ansible, or to globally ignoring this rule (which I did in my projects). Basically I would recommend everyone to globally ignore this rule.

(no_log should be used for modules which return secrets, but not for modules which accept secrets and properly use no_log!)

CC @noonedeadpunk

[noonedeadpunk]

I think it's true unless you do loop over such tasks? As then you will got your secrets exposed? Example:

- name: Add a user for rabbitmq
  hosts: rabbitmq_all[0]
  gather_facts: no
  tasks:
    - name: Configure Rabbitmq user
      rabbitmq_user:
        user: "{{ item.user }}"
        password: "{{ item.password }}"
        vhost: "{{ item.vhost }}"
        configure_priv: ".*"
        read_priv: ".*"
        write_priv: ".*"
        state: "present"
      with_items:
        - user: testguest
          password: secrete
          vhost: /testvhost

Wil result in:

root@aio1:# ansible-playbook ../test.yml 
PLAY [Add a user for rabbitmq] *************************************************************************************************************************************************

TASK [Configure Rabbitmq user] *************************************************************************************************************************************************
failed: [aio1_rabbit_mq_container-902c93ea] (item={'user': 'testguest', 'password': 'secrete', 'vhost': '/testvhost'}) => {"ansible_facts": {"discovered_interpreter_python": "/usr/bin/python3"}, "ansible_loop_var": "item", "changed": false, "item": {"password": "secrete", "user": "testguest", "vhost": "/testvhost"}, "msg": "rabbitmqctl exited with non-zero code: Virtual host '/testvhost' does not exist\n", "rc": 65, "stdout": "", "stdout_lines": []}

PLAY RECAP *********************************************************************************************************************************************************************
aio1_rabbit_mq_container-902c93ea : ok=0    changed=0    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0  

But yes, I agree that we need to adjust test to check for loops only.

It was eventually just created at times, when ansible modules didn't have such protection, so explicit no_log should been set everywhere

Suggested PR #1590 with changes to the test which make it fail only when task has loop in it.

[felixfontein]

Yes, the problems are loops, and not modules / actions. But password is a very specific kind of secret that can be leaked by a loop, and most people use appropriate measures if secrets actually show up in loops. I don't think there's a good way to have a linter rule for this case.

[konstruktoid]

Task test:

- hosts: all
  tasks:
    - name: Succeed when no_log is not used but no loop present
      become: 'yes'
      user:
        name: bidule
        password: "wow"

    - name: Hashed passwd
      become: 'yes'
      user:
        name: bidule
        password: $6$mysecretsalt$EWP4eKGjgNi9Uz/XPJv/0SMs19eAtCuFvS8YZm4eLiW3hV1c4EXeZrB2e/qTCN4lEZXxrnkR7qebrMdUADfYw1

    - name: Fail when no_log is set to False
      become: 'yes'
      user:
        name: bidule
        password: "{{ item }}"
      with_items:
        - wow
        - $6$mysecretsalt$EWP4eKGjgNi9Uz/XPJv/0SMs19eAtCuFvS8YZm4eLiW3hV1c4EXeZrB2e/qTCN4lEZXxrnkR7qebrMdUADfYw1

~$ ansible-playbook -vv -c local -i '127.0.0.1,' passwd.yml 
ansible-playbook [core 2.11.1] 
  config file = None
  configured module search path = ['/home/vagrant/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/vagrant/.local/lib/python3.8/site-packages/ansible
  ansible collection location = /home/vagrant/.ansible/collections:/usr/share/ansible/collections
  executable location = /home/vagrant/.local/bin/ansible-playbook
  python version = 3.8.5 (default, Jan 27 2021, 15:41:15) [GCC 9.3.0]
  jinja version = 2.10.1
  libyaml = True
No config file found; using defaults
Skipping callback 'default', as we already have a stdout callback.
Skipping callback 'minimal', as we already have a stdout callback.
Skipping callback 'oneline', as we already have a stdout callback.

PLAYBOOK: passwd.yml *********************************************************
1 plays in passwd.yml

PLAY [all] *************************************************************************

TASK [Gathering Facts] ***********************************************************
task path: /home/vagrant/passwd.yml:1
ok: [127.0.0.1]
META: ran handlers

TASK [Succeed when no_log is not used but no loop present] ***************************************************************************
task path: /home/vagrant/passwd.yml:3
[WARNING]: The input password appears not to have been hashed. The 'password' argument must be encrypted for this module to work properly.
changed: [127.0.0.1] => {"append": false, "changed": true, "comment": "", "group": 1002, "home": "/home/bidule", "move_home": false, "name": "bidule", "password": "NOT_LOGGING_PASSWORD", "shell": "/bin/sh", "state": "present", "uid": 1002}

TASK [Hashed passwd] ************************************************************
task path: /home/vagrant/passwd.yml:9
changed: [127.0.0.1] => {"append": false, "changed": true, "comment": "", "group": 1002, "home": "/home/bidule", "move_home": false, "name": "bidule", "password": "NOT_LOGGING_PASSWORD", "shell": "/bin/sh", "state": "present", "uid": 1002}

TASK [Fail when no_log is set to False] **********************************************
task path: /home/vagrant/passwd.yml:15
changed: [127.0.0.1] => (item=wow) => {"ansible_loop_var": "item", "append": false, "changed": true, "comment": "", "group": 1002, "home": "/home/bidule", "item": "wow", "move_home": false, "name": "bidule", "password": "NOT_LOGGING_PASSWORD", "shell": "/bin/sh", "state": "present", "uid": 1002, "warnings": ["The input password appears not to have been hashed. The 'password' argument must be encrypted for this module to work properly."]}
changed: [127.0.0.1] => (item=$6$mysecretsalt$EWP4eKGjgNi9Uz/XPJv/0SMs19eAtCuFvS8YZm4eLiW3hV1c4EXeZrB2e/qTCN4lEZXxrnkR7qebrMdUADfYw1) => {"ansible_loop_var": "item", "append": false, "changed": true, "comment": "", "group": 1002, "home": "/home/bidule", "item": "$6$mysecretsalt$EWP4eKGjgNi9Uz/XPJv/0SMs19eAtCuFvS8YZm4eLiW3hV1c4EXeZrB2e/qTCN4lEZXxrnkR7qebrMdUADfYw1", "move_home": false, "name": "bidule", "password": "NOT_LOGGING_PASSWORD", "shell": "/bin/sh", "state": "present", "uid": 1002}
META: ran handlers
META: ran handlers

PLAY RECAP ********************************************************************
127.0.0.1                  : ok=4    changed=3    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   

[felixfontein]

The "security goodies" we are talking about have been first added in Ansible 2.3.0 (see ansible/ansible#21211), i.e. over 4 years ago. So even Debian Buster's outdated Ansible has this included.

Also, printing the password item when looping feels like an upstream bug.

This is not a bug. Ansible has no way of knowing which random data you are giving it is sensitive and which not. (Also this is not what the rule is currently about. The rule - so far - is about regular module arguments. #1590 improves the rule to actually look at loops, but also that fix only covers a tiny amount of the actual set of problems, and instead causes a huge amount of unnecessary noise.)

I believe that "no_log by default" was initially enabled in version 2.9.20 (https://github.com/ansible/ansible/blob/stable-2.9/changelogs/CHANGELOG-v2.9.rst#v2920), so any version old than that would "need" this linting rule.

2.9.20 fixed a bunch of modules where no_log=True was not specified. Most modules however did that before, and the amount of modules fixed in that release where this rule would have actually helped is tiny (due to ansible/ansible#21211).

[konstruktoid]

Thanks @felixfontein, my investigative abilites failed when I tried to find the correct Ansible version regarding this.

[noonedeadpunk]

but also that fix only covers a tiny amount of the actual set of problems, and instead causes a huge amount of unnecessary noise

That is super opinionated statement IMO and I never got the idea it's based on. What set of problems it does not cover? What is meant under unnecessary noise? What do you put under necessary "noise" then? But meh.

As I said in PR - feel free to drop rule. It won't cost me much to revive maintaining it locally as we did for years.

[konstruktoid]

I see no reason to drop the rule, when (if wanted) it's easily ignored by adding it to the skip_list.

[felixfontein]

but also that fix only covers a tiny amount of the actual set of problems, and instead causes a huge amount of unnecessary noise

That is super opinionated statement IMO and I never got the idea it's based on. What set of problems it does not cover?

1. Looking only for options that contain password does not cover a lot of options that contain secret data.
2. The updated rule does not care whether the value for such module options actually comes from item, or from some expression based on item (or not even based on item). Only the first case is something that should be flagged. The second and third case usually do not need to be flagged.
3. The rule does not look at the data that is looped over. If the data contains fields like password or secretkey that are not used at all, or even used in module options that do not contain password in its name, these are not flagged.

What is meant under unnecessary noise? What do you put under necessary "noise" then? But meh.

This rule flags completely correct code and scares users into using way too many no_log: true entries in their roles and playbooks. Scaring users into doing things that are wrong (namely: using no_log way too often) is bad, and totally unnecessary.