Skip to content

Conversation

@jborean93
Copy link
Collaborator

@jborean93 jborean93 commented Oct 15, 2020

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 psrpclient and mi that 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 -UseSSL PowerShell will fail with

Invoke-Command: HTTPS on Unix does not currently support CA or CN checks. Use the PSSessionOption -SkipCACheck and -SkipCNCheck if you are certain you trust the server you are connecting to and the network in between.

This 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 psrpclient and mi that 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 reasons

  • The session options right now are not based down from PowerShell to psrpclient to mi
    • First reason for this is the hardcoded check but even without that, the build of psrpclient that is shipped by PowerShell does nothing when it's told to set those options
  • Without a way to get PowerShell to tell mi that cert verification is off or on, I decided to use env vars to control the behaviour and have verification on by default
    • If a user wants to disable verification they need to set the proper env vars
    • Unfortunately they still need SessionOption (New-PSSessionOption -SkipCACheck -SkipCNCheck) in PowerShell due to the hardcoded check

What I'm proposing is to relax that check to only fail if the underlying psrpclient library 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 of psrpclient and mi will 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 the WSMAN_OPTION_SKIP_CA_CHECK option. The current build of psrpclient shipped by PowerShell will return MI_RESULT_NOT_SUPPORTED for options that aren't implemented (this being one of them) whereas my fork will return MI_RESULT_OK to 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 psrpclient and mi are 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

@ghost ghost assigned rjmholt Oct 15, 2020
Copy link
Contributor

@PaulHigin PaulHigin left a 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.

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Oct 15, 2020
@jborean93
Copy link
Collaborator Author

Not sure if there's anything I should do about the CodeFactor errors, they seem to be pre-existing issues.

Copy link
Contributor

@PaulHigin PaulHigin left a 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!

@jborean93
Copy link
Collaborator Author

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 :)

@rjmholt
Copy link
Collaborator

rjmholt commented Oct 15, 2020

@PoshChan please remind me in 24 hours

@iSazonov
Copy link
Collaborator

Should we have a doc issue for the change?

…ansportManager.cs

Co-authored-by: Ilya <darpa@yandex.ru>
@jborean93
Copy link
Collaborator Author

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.

@iSazonov
Copy link
Collaborator

Restart failed CIs.

@rjmholt rjmholt merged commit 5fe17ba into PowerShell:master Oct 16, 2020
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Oct 16, 2020
@iSazonov iSazonov added this to the 7.2.0-preview.1 milestone Oct 16, 2020
@jborean93 jborean93 deleted the https-wsman branch October 16, 2020 21:49
@PoshChan
Copy link
Collaborator

@rjmholt, this is the reminder you requested 24 hours ago

@jborean93
Copy link
Collaborator Author

Thanks everyone for the reviews and help through all this, it's very much appreciated. Looking forward to the 7.2.0 release.

Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

LGTM

@TravisEz13 TravisEz13 added the WG-Security security related areas such as JEA label Oct 30, 2020
@ghost
Copy link

ghost commented Nov 17, 2020

🎉v7.2.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log WG-Security security related areas such as JEA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants