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

jenkins_plugin "params" argument is insecure #30874

Closed
abadger opened this issue Sep 25, 2017 · 7 comments · Fixed by #32708
Closed

jenkins_plugin "params" argument is insecure #30874

abadger opened this issue Sep 25, 2017 · 7 comments · Fixed by #32708
Labels
affects_2.4 This issue/PR affects Ansible v2.4 bug This issue/PR relates to a bug. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community.

Comments

@abadger
Copy link
Contributor

abadger commented Sep 25, 2017

ISSUE TYPE
  • Bug Report
COMPONENT NAME

lib/ansible/modules/web_infrastructure/jenkins_plugin.py

ANSIBLE VERSION
devel 2.4 2.3
CONFIGURATION

N/A

OS / ENVIRONMENT

N/A

SUMMARY

It was noticed that using the jenkins_plugin with username and password would log the password on the remote host. After some digging I discovered that in addition to the normal url_username and url_password arguments for fetch_url() the jenkins_plugin module also has a params argument where arbitrary ansible module arguments can be given. This parameter should go away as it circumvents all the normal argument checking, validation, and normalization.

For url_password, this is bad as using param to send in the url_password instead of the specific url_password argument allows the url_password to be logged. This can be a security problem.

I also found the source of the user's use of param instead of url_username and url_password: the module documentation has an example of using param that has that in it.

STEPS TO FIX
  • I am going to immediately create and merge a PR to remove the param example and replace it with using the url_username and url_password arguments directly.
  • The params argument should also be removed (as it bypasses the no_log setting on url_password, allowing the password to be logged by mistake). Additional parameters that the module uses should be explicitly stated in the argument_spec instead. (I took a look at the module and it does not appear that params is passed directly to the jenkins server, instead, specific keys are always plucked out of the params. So there should be no problem removing params.)
@ansibot
Copy link
Contributor

ansibot commented Sep 25, 2017

cc @jtyr
click here for bot help

@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 bug_report module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. labels Sep 25, 2017
abadger added a commit to abadger/ansible that referenced this issue Sep 25, 2017
params could be logged so never use it for passwords.

Also add code to raise an error if passwords are used in that field.

References ansible#30874
abadger added a commit that referenced this issue Sep 25, 2017
params could be logged so never use it for passwords.

Also add code to raise an error if passwords are used in that field.

References #30874
abadger added a commit that referenced this issue Sep 26, 2017
params could be logged so never use it for passwords.

Also add code to raise an error if passwords are used in that field.

References #30874

(cherry picked from commit 863fcb5)
abadger added a commit that referenced this issue Sep 26, 2017
params could be logged so never use it for passwords.

Also add code to raise an error if passwords are used in that field.

References #30874

(cherry picked from commit 863fcb5)
@abadger
Copy link
Contributor Author

abadger commented Sep 26, 2017

Changed the documentation and also raised an error if params has url_password. I think that will take care of the security aspect of the bug. We can remove the param field in devel to finish off this ticket.

@jtyr
Copy link
Contributor

jtyr commented Sep 26, 2017

@abadger I see the security risk when password is exposed in logs but I don't like to remove this usability feature completely. Wouldn't be possible to define which fields should be obfuscated from the logs on the AnsibleModule level? Something like this:

module = AnsibleModule(
    argument_spec=dict(
        ...
        params=dict(type='dict', nolog=True),
        ...
    )
)

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Sep 26, 2017
@abadger
Copy link
Contributor Author

abadger commented Sep 26, 2017 via email

@jtyr
Copy link
Contributor

jtyr commented Sep 26, 2017

I have added it on the agenda for Thursday ;o)

@jtyr
Copy link
Contributor

jtyr commented Sep 26, 2017

Just thinking about it again and we should not do nolog on the whole params. We want to obfuscate only the particular key of the params which feels like something not really standard thing to do on the AnsibleModule level.

By "We can remove the param field in devel to finish off this ticket" you meant to remove the whole param option from the module? Restricting the params not to hold password options is OK. But removing it all together is not good.

@abadger
Copy link
Contributor Author

abadger commented Sep 27, 2017 via email

prasadkatti pushed a commit to prasadkatti/ansible that referenced this issue Oct 1, 2017
params could be logged so never use it for passwords.

Also add code to raise an error if passwords are used in that field.

References ansible#30874
BondAnthony pushed a commit to BondAnthony/ansible that referenced this issue Oct 5, 2017
params could be logged so never use it for passwords.

Also add code to raise an error if passwords are used in that field.

References ansible#30874
abadger added a commit to abadger/ansible that referenced this issue Nov 8, 2017
It was decided that these options which override Ansible module options
from a generic, unchecked dict are an antipattern for Ansible Modules
and must be removed:

https://meetbot.fedoraproject.org/ansible-meeting/2017-09-28/ansible_dev_meeting.2017-09-28-15.00.log.html

Fixes ansible#30874
s-hertel pushed a commit to s-hertel/ansible that referenced this issue Nov 8, 2017
…le#32708)

* Remove the params option from jenkns_plugin and yum_repository

It was decided that these options which override Ansible module options
from a generic, unchecked dict are an antipattern for Ansible Modules
and must be removed:

https://meetbot.fedoraproject.org/ansible-meeting/2017-09-28/ansible_dev_meeting.2017-09-28-15.00.log.html

Fixes ansible#30874
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bug_report labels Mar 7, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.4 This issue/PR affects Ansible v2.4 bug This issue/PR relates to a bug. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants