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

Silence verify_host_key warning from net-ssh #430

Merged
merged 3 commits into from
Apr 16, 2019

Conversation

clintoncwolfe
Copy link
Contributor

@clintoncwolfe clintoncwolfe commented Mar 20, 2019

The SSH transport always wants to disable host key verification. In net-ssh 5+, the acceptable values for verify_host_key changed ;we currently set it to false, which now results in a deprecation warning from net-ssh

verify_host_key: false is deprecated, use :never

We already choose what option name to use based on the net-ssh version; this PR now chooses the value dynamically, as well.

As I found when passing values from a config file, there is a good chance the values will come in as Strings, so the PR does some questionable remapping of things to try to catch the common cases.

Signed-off-by: Clinton Wolfe clintoncwolfe@gmail.com

@clintoncwolfe clintoncwolfe added Type: Bug Feature not working as expected Component/Logging labels Mar 20, 2019
@clintoncwolfe clintoncwolfe force-pushed the cw/silence-ssh-host-verify-deprecation branch 4 times, most recently from 86a8d15 to 20e7017 Compare March 20, 2019 17:16
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
@clintoncwolfe clintoncwolfe force-pushed the cw/silence-ssh-host-verify-deprecation branch from 20e7017 to 6049ef4 Compare March 26, 2019 19:13
Copy link
Contributor

@marcparadise marcparadise left a comment

Choose a reason for hiding this comment

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

Overall looks good, couple of comments inline.

'false' => :never,
true => :always,
false => :never,
# May be correct value, but strings from JSON config
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing the nil case here, since it's possible for someone to specify the option w/ nil value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

# Likewise, version <5 accepted false; 5+ requires :never or will
# issue a deprecation warning. This method allows a lot of common
# things through.
def verify_host_key_value(given)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some, but stubbing the const used to detect the net-ssh version (which affects the mapping, here) proved elusive. So, the tests check whatever net-ssh version is currently being used, which is not ideal.

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
@clintoncwolfe clintoncwolfe merged commit df91b1e into master Apr 16, 2019
@clintoncwolfe clintoncwolfe deleted the cw/silence-ssh-host-verify-deprecation branch April 16, 2019 18:23
marcparadise pushed a commit to marcparadise/train that referenced this pull request Apr 19, 2019
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants