Fail NoLogPassword only when loops are used

[noonedeadpunk]

Fair note has been made in #1589 regarding modules already marking
password fields with no_log.
However, when loop is used, passwords are still being exposed in case
of task failure or running in debug.

So we change logic of the check and fail it only when task has some
iteration in it.

[noonedeadpunk]

Um, this failure looks like smth not realted?

[konstruktoid]

Ref #1589

[noonedeadpunk]

recheck

[felixfontein]

I don't think this is a good rule either. Having an action parameter named password (or something containing password) and having a loop at the same time is a very weak indication of a needed no_log. This also produces a huge amount of false positives, while not flagging another huge amount of actual secret leaks due to loops.

[felixfontein]

For example, if someone correctly wrote the loop as

- user:
    name: '{{ item }}'
    password: '{{ user_passwords[item] }}'
  loop: '{{ user_passwords.keys() }}'
  vars:
    user_passwords:
      foo: hunter2

the rule would flag this as "requires no_log", even though nothing is leaked here.

[konstruktoid]

How about defining the rule on any known modules with password variables? No need to guess with regex and some issues will be handled by ansible.

[noonedeadpunk]

I don't think this is a good rule either. Having an action parameter named password (or something containing password) and having a loop at the same time is a very weak indication of a needed no_log. This also produces a huge amount of false positives, while not flagging another huge amount of actual secret leaks due to loops.

Well, it's your opinion and I'm not going to argue here. But let's then drop other rules that are super arguable.
Examples:

• no-changed-when - having command and not having changed_when is a very week indication of failure
• risky-shell-pipe - having shell with pipe doesn't really mean that pipefail is required
• no-handler

and etc.

You can suggest PR to remove rule and I will continue maintaining it locally like I did 5 years before that.

How about defining the rule on any known modules with password variables

Um, I think it would be hard, taking into account plenty collections for different kinds of stuff, clouds, net equipment, etc.

[felixfontein]

How about defining the rule on any known modules with password variables? No need to guess with regex and some issues will be handled by ansible.

Password variables are already handled by Ansible. There's no need for ansible-lint to handle them as well, except for buggy modules. (But these should better be fixed.)

The problem this PR is trying to solve is secret leaking in loops. I don't see how knowing password (or secret) options for modules does help here.

[felixfontein]

I don't think this is a good rule either. Having an action parameter named password (or something containing password) and having a loop at the same time is a very weak indication of a needed no_log. This also produces a huge amount of false positives, while not flagging another huge amount of actual secret leaks due to loops.

Well, it's your opinion and I'm not going to argue here. But let's then drop other rules that are super arguable.

While I agree that the rules you listed are also very arguable, they still make more sense than this one IMO. The heuristic used by this rule produces huge lists of false positives and false negatives. IMO should not be enabled by default in a linter.