-
Notifications
You must be signed in to change notification settings - Fork 690
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
Use non-deprecated code and remove unused imports #389
Conversation
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.
Looks good to me so far.
I'm done with this, finally cleaned up most of the deprecated code |
if (useHTTPS) { | ||
try { | ||
SSLConnectionSocketFactory sslConnectionFactory = new SSLConnectionSocketFactory( | ||
new SSLContextBuilder().loadTrustMaterial(null, new TrustSelfSignedStrategy()).build(), |
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.
Now I'm assuming this is a pure refactoring here, but this is not the greatest idea. There should probably be an admin-controlled config setting to disable hostname verification and whatnot.
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.
This is pure refactoring, we should enhance this from a security perspective outside of this PR.
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.
Changes look good to me. I haven't performed any manual testing.
I'll merge this tomorrow if no issues. |
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.
Great job!
Removing all the deprecated code particularly in the WinRM bits