Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd SkipCA and SkipCN check requirement to WinRM/OMI HTTPS connection #8279
Conversation
PaulHigin
added some commits
Nov 14, 2018
PaulHigin
requested review from
adityapatwardhan,
anmenaga,
BrucePay,
daxian-dbw,
mirichmo and
TravisEz13
as
code owners
Nov 15, 2018
PaulHigin
assigned
adityapatwardhan
Nov 15, 2018
PaulHigin
removed request for
daxian-dbw,
BrucePay,
anmenaga,
mirichmo and
adityapatwardhan
Nov 15, 2018
PaulHigin
added
Area-Remoting
Breaking-Change
labels
Nov 15, 2018
PaulHigin
requested a review
from
SteveL-MSFT
Nov 15, 2018
SteveL-MSFT
requested changes
Nov 15, 2018
This comment has been minimized.
This comment has been minimized.
|
@PaulHigin Please resolve merge conflict. Also, enable tests here for non-windows: https://github.com/PowerShell/PowerShell/blob/master/test/powershell/engine/Remoting/SessionOption.Tests.ps1 |
adityapatwardhan
requested changes
Nov 15, 2018
|
Please enable SessionOption tests. |
adityapatwardhan
added
the
Documentation Needed
label
Nov 15, 2018
SteveL-MSFT
approved these changes
Nov 15, 2018
This comment has been minimized.
This comment has been minimized.
|
SessionOption.Tests.ps1 contains tests for WSMan SessionOption object, which we do not expose for non-Windows platforms. But I'll add tests for New-PSSessionOption. |
This comment has been minimized.
This comment has been minimized.
|
PowerShell Committee for the breaking change? |
iSazonov
added
the
CL-Engine
label
Nov 16, 2018
PaulHigin
added some commits
Nov 16, 2018
TravisEz13
added
the
Review - Committee
label
Nov 16, 2018
This comment has been minimized.
This comment has been minimized.
|
Adding the committee to review the breaking change |
PaulHigin
added some commits
Nov 16, 2018
adityapatwardhan
requested changes
Nov 20, 2018
PaulHigin
added some commits
Nov 20, 2018
iSazonov
reviewed
Nov 21, 2018
| throw "No Exception!" | ||
| } | ||
| catch { | ||
| $_.Exception.ErrorCode | Should -Be $expectedErrorCode |
This comment has been minimized.
This comment has been minimized.
iSazonov
Nov 21, 2018
Collaborator
We can use new Pester features:
$exc = { & $scriptBlock } | Should -Throw -ErrorId "..." -PassThru
$exc.Exception.ErrorCode | Should -Be $expectedErrorCode
This comment has been minimized.
This comment has been minimized.
PaulHigin
Nov 26, 2018
Author
Contributor
I like this pattern, however the ErrorId is not being checked. At least in my current version of Pester (4.4.2).
This comment has been minimized.
This comment has been minimized.
TravisEz13
reviewed
Nov 26, 2018
| @@ -1670,4 +1670,7 @@ All WinRM sessions connected to PowerShell session configurations, such as Micro | |||
| <data name="CannotGetHostInteropTypes" xml:space="preserve"> | |||
| <value>Host system does not have the correct version of Hyper-V schema.</value> | |||
| </data> | |||
| <data name="UnixOnlyHttpsWithoutSkipCACheckNotSupported" xml:space="preserve"> | |||
| <value>HTTPS on Unix does not currently support CA or CN checks. Use the PSSessionOption -SkipCACheck and -SkipCNCheck if you are certain of the server you are connecting to.</value> | |||
This comment has been minimized.
This comment has been minimized.
TravisEz13
Nov 26, 2018
Member
| <value>HTTPS on Unix does not currently support CA or CN checks. Use the PSSessionOption -SkipCACheck and -SkipCNCheck if you are certain of the server you are connecting to.</value> | |
| <value>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.</value> |
TravisEz13
requested changes
Nov 26, 2018
|
One comment about the message |
TravisEz13
approved these changes
Nov 27, 2018
This comment has been minimized.
This comment has been minimized.
|
I thought |
adityapatwardhan
approved these changes
Nov 28, 2018
adityapatwardhan
merged commit 0657163
into
PowerShell:master
Nov 28, 2018
6 of 7 checks passed
This comment has been minimized.
This comment has been minimized.
|
@PowerShell/powershell-committee I did not realize this was marked for committee review. If this does not pass review, I will revert. |
This comment has been minimized.
This comment has been minimized.
|
@PowerShell/powershell-committee Also, consider if this should be back-ported to 6.1 and 6.0? |
PaulHigin commentedNov 15, 2018
PR Summary
OMI does not support server certificate validation, so this change is to add a requirement that -SkipCACheck and -SkipCNCheck options are specified before a connection is allowed. This way users will be aware that no server certificate validation is performed.
This only applies to PSCore6 remoting clients on non-Windows platforms, when creating a remote session to a Windows machine via WinRM\OMI.
This is a breaking change because these skip check options are now required. If the skip check options are not included then the connection will be denied.
Examples:
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests