-
Notifications
You must be signed in to change notification settings - Fork 682
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
Host resource: use bash over netcat in Linux #2607
Conversation
ea0ddc5
to
c58810d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a great improvement, but I think we need to leave the netcat support in as a fallback. Not all Linux versions have a bash installed that was compiled with the /dev/tcp
and /dev/udp
support. For example, I believe the default Debian compile instructions for bash still include --disable-net-redirections
Fair enough, will work on that. |
517095f
to
7835599
Compare
Netcat's presence is widely regarded as a security issue, and thus not always available. This solution first tries to use bash builtins and timeout (from coreutils), so is less likely to require installing additional packages. Signed-off-by: João Vale <[email protected]>
Signed-off-by: João Vale <[email protected]>
Signed-off-by: João Vale <[email protected]>
7835599
to
5dd2e6f
Compare
As suggest, brought back netcat. It is the preferred option, if available on the box. Also added support for UDP in Darwin since the code was pretty much already there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jvale ! This is a really nice addition to the host resource. Just some small suggestions.
lib/resources/host.rb
Outdated
def missing_requirements(protocol) | ||
missing = [] | ||
|
||
if %w{tcp udp}.include?(protocol) and !@has_nc and !@has_ncat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change the and
's to &&
? We try to avoid and/or due to the lower precedence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had no idea about the precedence difference, guess I learned something. :) Done.
lib/resources/host.rb
Outdated
|
||
def ping(hostname, port, protocol) | ||
if %w{tcp udp}.include?(protocol) | ||
if @has_nc or @has_ncat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, if you could change to ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...and also done.
Signed-off-by: João Vale <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jvale !
@adamleff - You mind giving another pass on this one? |
Netcat's presence is widely considered a security issue, and thus not always available. This solution only uses timeout (from coreutils) and bash builtins, so less likely to require installing additional packages.
It also adds UDP support for free and moves a bit further in the direction of solving the concerns raised in #1439.