-
Notifications
You must be signed in to change notification settings - Fork 144
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
Enable PowerShell Core #271
Conversation
Attempted a netstandard2.0 build of SolarWinds.InformationService.Contract.csproj. Does not compile.
Updated SwisPowerShell.Tests.csproj to target netcoreapp3.1 instead of netstandard2.0 to address some build warnings.
Updated InformationService.Contract.Tests.csproj from NUnit 2.6.4 to 3.9.0 to match the other test project. Adapted a handful of tests based on breaking changes to NUnit. Also referenced Microsoft.NET.Test.Sdk and NUnit3TestAdapter in the csproj file to get nicer Visual Studio integration.
Retargeted SwisPowerShell.Tests.csproj to net48 instead of net5.0 due to build framework support.
Added missing System assemblies that are now required after changes to support .NET Standard/.NET Core to the Product.wxs for the WiX installer.
|
||
namespace SolarWinds.InformationService.Contract2 | ||
{ | ||
class AllTrustingCertificateValidator : X509CertificateValidator |
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.
See UsernameCredentials.cs for usage of this class. Is there any way we can perform more robust validation here, given that many/most Orion installations will be using a self-signed certificate that won't be trusted by default on a remote machine?
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.
I think the best we could do is an SSH-style approve-thumbprint-on-first-connect flow. But for now I am satisfied as long as the result is not worse than the current version of SwisPowerShell.
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.
There are a couple of changes that need a closer security look before we publish.
@@ -143,7 +141,7 @@ private void Initialize(EndpointAddress address, Binding binding, ServiceCredent | |||
SslStreamSecurityBindingElement element = elements.Find<SslStreamSecurityBindingElement>(); | |||
if (element != null) | |||
{ | |||
element.IdentityVerifier = new SWIdentityVerifier(); | |||
//element.IdentityVerifier = new SWIdentityVerifier(); |
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.
What is the consequence of this being removed? (I was probably the one to remove it, but I don't remember why.)
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.
If uncommented, it produces the following error:
SwisProxyBase.cs(144,25,144,41): error CS1061: 'SslStreamSecurityBindingElement' does not contain a definition for 'IdentityVerifier' and no accessible extension method 'IdentityVerifier' accepting a first argument of type 'SslStreamSecurityBindingElement' could be found (are you missing a using directive or an assembly reference?)
This property seems to have been removed in the reduced surface area supported by .NET Standard and .NET Core. I changed the target framework for SolarWinds.InformationService.Contract.csproj from netstandard2.0 to netcoreapp3.1 and got the same error.
X509ChainPolicy chainPolicy = new X509ChainPolicy(); | ||
chainPolicy.VerificationFlags = X509VerificationFlags.AllowUnknownCertificateAuthority | X509VerificationFlags.IgnoreNotTimeValid; | ||
channelFactory.Credentials.ServiceCertificate.Authentication.CustomCertificateValidator = X509CertificateValidator.CreateChainTrustValidator(true, chainPolicy); | ||
channelFactory.Credentials.ServiceCertificate.Authentication.CustomCertificateValidator = new AllTrustingCertificateValidator(); |
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 replacing a "chain trust validator" that ignores the certificate authority and valid time range (i.e., basically everything) with one that ignores everything. Are we losing any meaningful security here? Given that the old one didn't really check anything, I don't think so. What do you think?
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.
The full set of flags in the X509VerificationFlags enumeration include several settings, and the docs for the VerificationFlags Property on X509ChainPolicy indicate that NoFlag
is the default. But allowing an unknown certificate authority would seem to make a lot of other checks pretty much irrelevant. From what I can tell, this change won't have any meaningful impact.
Src/Contract/WindowsCredential.cs
Outdated
@@ -109,7 +109,8 @@ public override void ApplyTo(ChannelFactory channelFactory) | |||
|
|||
X509ChainPolicy chainPolicy = new X509ChainPolicy(); | |||
chainPolicy.VerificationFlags = X509VerificationFlags.AllowUnknownCertificateAuthority | X509VerificationFlags.IgnoreNotTimeValid; | |||
channelFactory.Credentials.ServiceCertificate.Authentication.CustomCertificateValidator = X509CertificateValidator.CreateChainTrustValidator(true, chainPolicy); | |||
// TODO - this is missing from .net standard, but it may not be possible to do Windows auth safely without it | |||
//channelFactory.Credentials.ServiceCertificate.Authentication.CustomCertificateValidator = X509CertificateValidator.CreateChainTrustValidator(true, chainPolicy); |
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 comment is concerning. We should follow up.
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.
See the changes in b051e84. I'm hoping that will do the trick.
Src/SwisPowerShell/ConnectSwis.cs
Outdated
@@ -178,7 +178,8 @@ private InfoServiceProxy ConnectNetTcp() | |||
if (Streamed.IsPresent) | |||
{ | |||
binding.TransferMode = TransferMode.Streamed; | |||
binding.PortSharingEnabled = true; | |||
// TODO - is this a problem? | |||
//binding.PortSharingEnabled = true; |
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.
It doesn't seem like this matters, since SWIS is always using net.tcp port sharing and this client works fine without binding.PortSharingEnabled = true
. We should just remove the comments here.
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.
Done.
Removed commented code for setting binding.PortSharingEnabled in ConnectSwis.
Adapted certificate validation in WindowsCredential to work around the absence of X509CertificateValidator.CreateChainTrustValidator in the .NET Standard and .NET Core flavors of WCF.
This PR includes several changes to enable the SwisPowerShell commandlet to work with PowerShell 7, previously known as PowerShell Core. The goal is to enable cross-platform usage of this PowerShell commandlet. See the supported platforms for PowerShell 7.
To enable this change, several assemblies were retargeted from net48 to netstandard2.0. This required changes to system dependencies that are reflected in the MSI installer. The most significant changes (courtesy of @tdanner) were related to the Windows Communication Foundation (WCF) protocol that SWIS uses. It has limited support in .NET Core.