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

Fix for server_password management #60

Merged
merged 1 commit into from
Jan 22, 2020

Conversation

smortex
Copy link
Member

@smortex smortex commented Jan 15, 2020

Pull Request (PR) description

The fix provided by #53 is incomplete: using shell redirections and control operators need to switch to the 'shell' exec provider, which in turn allows command injections. Moreover, as explained in #52, murmurd exists with an exit code of 1 on success… but also on failure.

Attempt to improve the situation by switching to the shell provider, shell escaping the password thanks to stdlib's shell_escape function, and grep the stderr output of murmurd for a success message.

This Pull Request (PR) fixes the following issues

n/a

@smortex smortex requested a review from sbadia January 15, 2020 21:34
@smortex smortex force-pushed the server_password-fix branch 2 times, most recently from 65f1f33 to bdfa2dc Compare January 16, 2020 04:59
Copy link
Member

@Dan33l Dan33l left a comment

Choose a reason for hiding this comment

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

Hi @smortex and thank you for the PR.

Some inline comments added. Can you have a look ?

.fixtures.yml Show resolved Hide resolved
spec/classes/init_spec.rb Show resolved Hide resolved
@smortex smortex force-pushed the server_password-fix branch 3 times, most recently from c9e6011 to 7d0adb6 Compare January 20, 2020 20:27
@ghoneycutt ghoneycutt self-requested a review January 21, 2020 03:50
The fix provided by voxpupuli#53 is incomplete: using shell redirections and
control operators need to switch to the 'shell' exec provider, which in
turn allows command injections.  Moreover, as explained in voxpupuli#52, murmurd
exists with an exit code of 1 on success…  but also on failure.

Attempt to improve the situation by switching to the shell provider,
shell escaping the password thanks to stdlib's shell_escape function,
and grep the stderr output of murmurd for a success message.
@smortex
Copy link
Member Author

smortex commented Jan 22, 2020

Looks like I added the stdlib requirements to the wrong section in metadata.json (requirements instead of dependencies)… I just pushed a fix!

Copy link
Member

@Dan33l Dan33l left a comment

Choose a reason for hiding this comment

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

LGTM

@ghoneycutt ghoneycutt merged commit d8f52dd into voxpupuli:master Jan 22, 2020
@smortex smortex deleted the server_password-fix branch January 22, 2020 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants