-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Relax further SSL verification checks for WSMan on non-Windows hosts with verification available #13786
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
Conversation
PaulHigin
left a comment
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. Just one comment.
src/System.Management.Automation/engine/remoting/fanin/WSManTransportManager.cs
Show resolved
Hide resolved
|
Not sure if there's anything I should do about the CodeFactor errors, they seem to be pre-existing issues. |
PaulHigin
left a comment
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 for fixing this!
|
No worries, thanks for considering the changes. With this change, the only remaining issue on my plate is now CredSSP authentication but I doubt that will happen anytime soon :) |
|
@PoshChan please remind me in 24 hours |
|
Should we have a doc issue for the change? |
src/System.Management.Automation/engine/remoting/fanin/WSManTransportManager.cs
Outdated
Show resolved
Hide resolved
…ansportManager.cs Co-authored-by: Ilya <darpa@yandex.ru>
|
Thanks for the review @iSazonov. As for the docs, this is mostly for a 3rd party library that isn’t really affiliated with PowerShell so I’m not sure how much it should be advertised. Ultimately up to the PowerShell team to decide. |
|
Restart failed CIs. |
|
@rjmholt, this is the reminder you requested 24 hours ago |
|
Thanks everyone for the reviews and help through all this, it's very much appreciated. Looking forward to the 7.2.0 release. |
TravisEz13
left a comment
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.
LGTM
|
🎉 Handy links: |
PR Summary
Only require
-SessionOption (New-PSSessionOption -SkipCACheck -SkipCNCheck)on non-Windows hosts when the client OMI library does not support cert verification.PR Context
The version of
psrpclientandmithat are shipped with PowerShell on non-Windows hosts do not support certificate verification. Because of this limitation if you try to connect over a HTTPS WSMan listener with just-UseSSLPowerShell will fail withThis is a perfectly adequate method to ensure end users are aware no cert verification is happening. In the meantime I have a fork of both
psrpclientandmithat are designed to fix some of the bugs and limitations that face WSMan users on non-Windows clients and one of those fixes is to add certificate verification for WSMan. The current fork is set to always do certificate verification regardless of the session options for 2 reasonspsrpclienttomipsrpclientthat is shipped by PowerShell does nothing when it's told to set those optionsmithat cert verification is off or on, I decided to use env vars to control the behaviour and have verification on by defaultSessionOption (New-PSSessionOption -SkipCACheck -SkipCNCheck)in PowerShell due to the hardcoded checkWhat I'm proposing is to relax that check to only fail if the underlying
psrpclientlibrary does not support passing those options along. That way people using the libs shipped by PowerShell continue to work as usual but people who use my forked copy ofpsrpclientandmiwill be able to omit the options and only add them if they truly want to disable cert verification. This check in PowerShell is simple, it will call WSManGetSessionOptionAsDword with theWSMAN_OPTION_SKIP_CA_CHECKoption. The current build ofpsrpclientshipped by PowerShell will returnMI_RESULT_NOT_SUPPORTEDfor options that aren't implemented (this being one of them) whereas my fork will returnMI_RESULT_OKto state the option was available. By checking that value we can see if the underlying libs support verification and control whether PowerShell displays the error message or not.The underlying changes required to
psrpclientandmiare currently in this PR jborean93/omi#15. I still need to do some more testing but so far they seem to work as desired.More background can be found here #13577.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.