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

Fail NoLogPassword only when loops are used #1590

Merged
merged 2 commits into from
Jun 4, 2021

Conversation

noonedeadpunk
Copy link
Contributor

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.

Fair note has been made in ansible#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
Copy link
Contributor Author

Um, this failure looks like smth not realted?

@konstruktoid
Copy link
Contributor

Ref #1589

@noonedeadpunk
Copy link
Contributor Author

recheck

Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

noonedeadpunk commented May 31, 2021

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
Copy link
Contributor

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
Copy link
Contributor

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.

@ssbarnea ssbarnea added the bug label Jun 2, 2021
@ssbarnea ssbarnea merged commit e892ce9 into ansible:master Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants