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

Add SslProtocol Support to WebCmdlets #5329

Merged
merged 10 commits into from Nov 13, 2017

Conversation

@markekraus
Collaborator

markekraus commented Nov 3, 2017

closes #2662

This feature adds the ability to restrict the SSL/TLS protocol used when making the web request. In 5.1 the user could make use of .NET API's to enforce this on the Web Cmdlets. With the move to HttpClient in PowerShell Core, those APIs have no impact. The user still has requirements to ensure specific protocols are used.

The public enum WebSslProtocol is added as a wrapper to the underlying SslProtocols enum. Neither it nor SecurityProtocolType can be used because Ssl3 and Ssl2 are not supported by HttpClientHandler.SslProtocols. While it may not be intuitive to a PowerShell user to use -bor or "Tls, Tls11" to set multiple options, the general use case for this will be a single protocol.

  • Adds -SslProtocol parameter to Web Cmdlets
  • Adds WebSslProtocol Enum to support limited subset of SslProtocol enum supported by HttpClientHandler
  • Adds TLS 1.1 and TLS 1.0 listening ports to WebListener
  • Adds 100% test coverage for new feature

I spoke with @SteveL-MSFT about getting this approved before RC release.

[Feature] Add SslProtocol Support to WebCmdlets
* Adds `-SslProtocol` parameter to Web Cmdlets
* Adds WebSslProtocol Enum to support limited subset of SslProtocol enum supported by HttpClientHandler
* Adds TLS 1.1 and TLS 1.0 listening ports to WebListener
* Adds 100% test coverage for new feature
Show outdated Hide outdated ...s.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs Outdated
public enum WebSslProtocol
{
/// <summary>
/// No SSL protocol will be set and the system defaults will be used.

This comment has been minimized.

@iSazonov

iSazonov Nov 8, 2017

Collaborator

What is "system default"? In .Net? Os?

@iSazonov

iSazonov Nov 8, 2017

Collaborator

What is "system default"? In .Net? Os?

This comment has been minimized.

@markekraus

markekraus Nov 8, 2017

Collaborator

It is environment specific and subject to implementation changes by CoreFX, thus the comment is not specific on purpose. It is SslProtocol.None... which is terribly named for PowerShell users as it implies no SSL/TLS would be used (that's a C# specific terminology for the 0 field on flag enums).

@markekraus

markekraus Nov 8, 2017

Collaborator

It is environment specific and subject to implementation changes by CoreFX, thus the comment is not specific on purpose. It is SslProtocol.None... which is terribly named for PowerShell users as it implies no SSL/TLS would be used (that's a C# specific terminology for the 0 field on flag enums).

Show outdated Hide outdated ...Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated
/// <summary>
/// No SSL protocol will be set and the system defaults will be used.
/// </summary>
Default = 0,

This comment has been minimized.

@iSazonov

iSazonov Nov 8, 2017

Collaborator

I think we should be exactly Default = SslProtocols.None.

@iSazonov

iSazonov Nov 8, 2017

Collaborator

I think we should be exactly Default = SslProtocols.None.

This comment has been minimized.

@markekraus

markekraus Nov 8, 2017

Collaborator

I'm not sure how well that works with Flags. We have to define 0 for the default here and if CoreFX decides none is no longer 0 we lose our Default option.

@markekraus

markekraus Nov 8, 2017

Collaborator

I'm not sure how well that works with Flags. We have to define 0 for the default here and if CoreFX decides none is no longer 0 we lose our Default option.

This comment has been minimized.

@iSazonov

iSazonov Nov 8, 2017

Collaborator

I see handler.SslProtocols = (SslProtocols)SslProtocol; so I believe Default = SslProtocols.None is more accurate.

@iSazonov

iSazonov Nov 8, 2017

Collaborator

I see handler.SslProtocols = (SslProtocols)SslProtocol; so I believe Default = SslProtocols.None is more accurate.

This comment has been minimized.

@markekraus

markekraus Nov 8, 2017

Collaborator

SslProtocols will always have a 0 too since it is a Flag as well. I'm weary of changing this one. Wehn I was researching I saw multiple asks for the SslProtocols to be cleaned up an None removed. it seems to be universally detested as a field name that is better defined as SystemDefault. Looking at the implementation of SecurityProtocolType they, too, erred on the side of using 0.

https://github.com/dotnet/corefx/blob/ba0e14d957cd2fed3e1dec54d2c5f2a92a746c74/src/System.Net.ServicePoint/src/System/Net/SecurityProtocolType.cs#L12

@markekraus

markekraus Nov 8, 2017

Collaborator

SslProtocols will always have a 0 too since it is a Flag as well. I'm weary of changing this one. Wehn I was researching I saw multiple asks for the SslProtocols to be cleaned up an None removed. it seems to be universally detested as a field name that is better defined as SystemDefault. Looking at the implementation of SecurityProtocolType they, too, erred on the side of using 0.

https://github.com/dotnet/corefx/blob/ba0e14d957cd2fed3e1dec54d2c5f2a92a746c74/src/System.Net.ServicePoint/src/System/Net/SecurityProtocolType.cs#L12

Show outdated Hide outdated test/tools/Modules/WebListener/WebListener.psm1 Outdated

markekraus added some commits Nov 8, 2017

Address PR feedback
* TLSO 2.0 -> TLS 1.2
* remove onbvious comment
/// Gets or sets the TLS/SSL protocol used by the Web Cmdlet
/// </summary>
[Parameter]
public virtual WebSslProtocol SslProtocol { get; set; } = WebSslProtocol.Default;

This comment has been minimized.

@iSazonov

iSazonov Nov 9, 2017

Collaborator

I should ask: maybe use WebSslProtocol for parameter name?

@iSazonov

iSazonov Nov 9, 2017

Collaborator

I should ask: maybe use WebSslProtocol for parameter name?

This comment has been minimized.

@markekraus

markekraus Nov 9, 2017

Collaborator

I think the Web is kind of redundant in the parameter name. I was just continuing with the convention of prefixing Web in front of Enums used for web cmdlet parameter options. Technically, it is setting the SslProtocol, we are just limiting the options that the user can use to those it actually supports.

@markekraus

markekraus Nov 9, 2017

Collaborator

I think the Web is kind of redundant in the parameter name. I was just continuing with the convention of prefixing Web in front of Enums used for web cmdlet parameter options. Technically, it is setting the SslProtocol, we are just limiting the options that the user can use to those it actually supports.

@iSazonov

LGTM with one concern about Default = 0 vs Default = SslProtocols.None.

$result.headers.Host | Should Be $params.Uri.Authority
}
It "Verifies Invoke-WebRequest -SslProtocol -SslProtocol <IntendedProtocol> fails on a <ActualProtocol> only connection" -TestCases @(

This comment has been minimized.

@TravisEz13

TravisEz13 Nov 10, 2017

Member

We should have some tests were intended is multiple protocols. that should be allowed since you used a flags enum.

@TravisEz13

TravisEz13 Nov 10, 2017

Member

We should have some tests were intended is multiple protocols. that should be allowed since you used a flags enum.

This comment has been minimized.

@markekraus

markekraus Nov 10, 2017

Collaborator

Added.

@markekraus

markekraus Nov 10, 2017

Collaborator

Added.

@markekraus

This comment has been minimized.

Show comment
Hide comment
@markekraus

markekraus Nov 10, 2017

Collaborator

@TravisEz13 It looks like macOS does not support multiple protocols being set at once. This is not surprising as the entire SSL/TLS/Certificate implementation in CoreFX is flakey on macOS where some things work sometimes, not at all, or only partially.

I'm going to logic gate the multi-protocol test cases so they don't run on macOS.

$handler = [System.Net.Http.HttpClientHandler]::new() 
$handler.SslProtocols = [System.Security.Authentication.SslProtocols]::Tls -bor [System.Security.Authentication.SslProtocols]::Tls11
$HttpClient = [System.Net.Http.HttpClient]::New($handler) 
$Response = $HttpClient.GetAsync("https://httpbin.org/user-agent").GetAwaiter().GetResult()

result

Exception calling "GetResult" with "0" argument(s): "The requested security protocol is not supported."
At line:1 char:1
+ $Response = $HttpClient.GetAsync("https://httpbin.org/user-agent").Ge ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
+ FullyQualifiedErrorId : NotSupportedException

ErrorRecord                 : Exception calling "GetResult" with "0" argument(s): "The requested security protocol is not supported."
WasThrownFromThrowStatement : False
Message                     : Exception calling "GetResult" with "0" argument(s): "The requested security protocol is not supported."
Data                        : {System.Management.Automation.Interpreter.InterpretedFrameInfo}
InnerException              : System.NotSupportedException: The requested security protocol is not supported.
                                 at System.Net.Http.CurlHandler.SslProvider.SetSslVersion(EasyRequest easy)
                                 at System.Net.Http.CurlHandler.SslProvider.SetSslOptions(EasyRequest easy, ClientCertificateOption clientCertOption)
                                 at System.Net.Http.CurlHandler.EasyRequest.InitializeCurl()
                                 at System.Net.Http.CurlHandler.MultiAgent.ActivateNewRequest(EasyRequest easy)
                              --- End of stack trace from previous location where exception was thrown ---
                                 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
                                 at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
                                 at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
                                 at System.Net.Http.HttpClient.<FinishSendAsyncBuffered>d__58.MoveNext()
                              --- End of stack trace from previous location where exception was thrown ---
                                 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
                                 at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
                                 at CallSite.Target(Closure , CallSite , Object )
TargetSite                  : Void ConvertToMethodInvocationException(System.Exception, System.Type, System.String, Int32, System.Reflection.MemberInfo)
StackTrace                  :    at System.Management.Automation.ExceptionHandlingOps.ConvertToMethodInvocationException(Exception exception, Type typeToThrow, String meth
                              odName, Int32 numArgs, MemberInfo memberInfo)
                                 at CallSite.Target(Closure , CallSite , Object )
                                 at System.Dynamic.UpdateDelegates.UpdateAndExecute1[T0,TRet](CallSite site, T0 arg0)
                                 at System.Management.Automation.Interpreter.DynamicInstruction`2.Run(InterpretedFrame frame)
                                 at System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame frame)
HelpLink                    :
Source                      : System.Management.Automation
HResult                     : -2146233087
Collaborator

markekraus commented Nov 10, 2017

@TravisEz13 It looks like macOS does not support multiple protocols being set at once. This is not surprising as the entire SSL/TLS/Certificate implementation in CoreFX is flakey on macOS where some things work sometimes, not at all, or only partially.

I'm going to logic gate the multi-protocol test cases so they don't run on macOS.

$handler = [System.Net.Http.HttpClientHandler]::new() 
$handler.SslProtocols = [System.Security.Authentication.SslProtocols]::Tls -bor [System.Security.Authentication.SslProtocols]::Tls11
$HttpClient = [System.Net.Http.HttpClient]::New($handler) 
$Response = $HttpClient.GetAsync("https://httpbin.org/user-agent").GetAwaiter().GetResult()

result

Exception calling "GetResult" with "0" argument(s): "The requested security protocol is not supported."
At line:1 char:1
+ $Response = $HttpClient.GetAsync("https://httpbin.org/user-agent").Ge ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
+ FullyQualifiedErrorId : NotSupportedException

ErrorRecord                 : Exception calling "GetResult" with "0" argument(s): "The requested security protocol is not supported."
WasThrownFromThrowStatement : False
Message                     : Exception calling "GetResult" with "0" argument(s): "The requested security protocol is not supported."
Data                        : {System.Management.Automation.Interpreter.InterpretedFrameInfo}
InnerException              : System.NotSupportedException: The requested security protocol is not supported.
                                 at System.Net.Http.CurlHandler.SslProvider.SetSslVersion(EasyRequest easy)
                                 at System.Net.Http.CurlHandler.SslProvider.SetSslOptions(EasyRequest easy, ClientCertificateOption clientCertOption)
                                 at System.Net.Http.CurlHandler.EasyRequest.InitializeCurl()
                                 at System.Net.Http.CurlHandler.MultiAgent.ActivateNewRequest(EasyRequest easy)
                              --- End of stack trace from previous location where exception was thrown ---
                                 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
                                 at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
                                 at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
                                 at System.Net.Http.HttpClient.<FinishSendAsyncBuffered>d__58.MoveNext()
                              --- End of stack trace from previous location where exception was thrown ---
                                 at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
                                 at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
                                 at CallSite.Target(Closure , CallSite , Object )
TargetSite                  : Void ConvertToMethodInvocationException(System.Exception, System.Type, System.String, Int32, System.Reflection.MemberInfo)
StackTrace                  :    at System.Management.Automation.ExceptionHandlingOps.ConvertToMethodInvocationException(Exception exception, Type typeToThrow, String meth
                              odName, Int32 numArgs, MemberInfo memberInfo)
                                 at CallSite.Target(Closure , CallSite , Object )
                                 at System.Dynamic.UpdateDelegates.UpdateAndExecute1[T0,TRet](CallSite site, T0 arg0)
                                 at System.Management.Automation.Interpreter.DynamicInstruction`2.Run(InterpretedFrame frame)
                                 at System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame frame)
HelpLink                    :
Source                      : System.Management.Automation
HResult                     : -2146233087

Waiting on updates from author

@markekraus

This comment has been minimized.

Show comment
Hide comment
@markekraus

markekraus Nov 10, 2017

Collaborator

Hmm. It looks like the 'Tls, Tls12' failure on Linux is a possible CoreFX bug. It appears it doesn't honor the Tls12 property when it is supplied with Tls and instead tries to use Tls only. All the other combinations work, including 'Tls, Tls11, Tls12', 'Tls11, Tls12', and 'Tls12'.

$url = Get-WebListenerUrl -Test Get -Https -SslProtocol Tls12
$handler = [System.Net.Http.HttpClientHandler]::new() 
$handler.ServerCertificateCustomValidationCallback = [System.Net.Http.HttpClientHandler]::DangerousAcceptAnyServerCertificateValidator
$handler.SslProtocols = [System.Security.Authentication.SslProtocols]::Tls -bor [System.Security.Authentication.SslProtocols]::Tls12
$HttpClient = [System.Net.Http.HttpClient]::New($handler) 
$Response = $HttpClient.GetAsync("$url").GetAwaiter().GetResult()
Message        : An error occurred while sending the request.
Data           : {}
InnerException : System.Net.Http.CurlException: SSL connect error
                    at System.Net.Http.CurlHandler.ThrowIfCURLEError(CURLcode error)
                    at System.Net.Http.CurlHandler.MultiAgent.FinishRequest(StrongToWeakReference`1 easyWrapper, CURLcode messageResult)
TargetSite     : Void Throw()
StackTrace     :    at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
                    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
                    at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
                    at System.Net.Http.HttpClient.<FinishSendAsyncBuffered>d__58.MoveNext()
                 --- End of stack trace from previous location where exception was thrown ---
                    at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
                    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
                    at CallSite.Target(Closure , CallSite , Object )
HelpLink       :
Source         : System.Private.CoreLib
HResult        : 35

I'll logic this one out of being run on both linux and macOS. When I get some time I will open an issue with CoreFX.

Collaborator

markekraus commented Nov 10, 2017

Hmm. It looks like the 'Tls, Tls12' failure on Linux is a possible CoreFX bug. It appears it doesn't honor the Tls12 property when it is supplied with Tls and instead tries to use Tls only. All the other combinations work, including 'Tls, Tls11, Tls12', 'Tls11, Tls12', and 'Tls12'.

$url = Get-WebListenerUrl -Test Get -Https -SslProtocol Tls12
$handler = [System.Net.Http.HttpClientHandler]::new() 
$handler.ServerCertificateCustomValidationCallback = [System.Net.Http.HttpClientHandler]::DangerousAcceptAnyServerCertificateValidator
$handler.SslProtocols = [System.Security.Authentication.SslProtocols]::Tls -bor [System.Security.Authentication.SslProtocols]::Tls12
$HttpClient = [System.Net.Http.HttpClient]::New($handler) 
$Response = $HttpClient.GetAsync("$url").GetAwaiter().GetResult()
Message        : An error occurred while sending the request.
Data           : {}
InnerException : System.Net.Http.CurlException: SSL connect error
                    at System.Net.Http.CurlHandler.ThrowIfCURLEError(CURLcode error)
                    at System.Net.Http.CurlHandler.MultiAgent.FinishRequest(StrongToWeakReference`1 easyWrapper, CURLcode messageResult)
TargetSite     : Void Throw()
StackTrace     :    at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
                    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
                    at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
                    at System.Net.Http.HttpClient.<FinishSendAsyncBuffered>d__58.MoveNext()
                 --- End of stack trace from previous location where exception was thrown ---
                    at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
                    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
                    at CallSite.Target(Closure , CallSite , Object )
HelpLink       :
Source         : System.Private.CoreLib
HResult        : 35

I'll logic this one out of being run on both linux and macOS. When I get some time I will open an issue with CoreFX.

[Feature] Run 'Tls, Tls12' test only on Windows
Possible CoreFX bug where `Tls, Tls12` does not honor the Tls12 on a Tls12 endpoint.

@SteveL-MSFT SteveL-MSFT added this to the 6.0.0-RC milestone Nov 11, 2017

@iSazonov

This comment has been minimized.

Show comment
Hide comment
@iSazonov

iSazonov Nov 11, 2017

Collaborator

In previous years, we had a simple logic when we started with a highest protocol and fallback at a lower.
After it has been recognized as a security hole, I guess the setting of several protocols is irrelevant and we should block it.

Collaborator

iSazonov commented Nov 11, 2017

In previous years, we had a simple logic when we started with a highest protocol and fallback at a lower.
After it has been recognized as a security hole, I guess the setting of several protocols is irrelevant and we should block it.

@markekraus

This comment has been minimized.

Show comment
Hide comment
@markekraus

markekraus Nov 11, 2017

Collaborator

@iSazonov Your recommendation is to switch it to a Non-Flags Enum?

My plan was to not document the possibility of multiple protocols and leave as an exercise for advanced users. They don't show up in intellisense and unless someone advanced enough in the l;language took a good look at the Enum, they wouldn't really know.

Multiple protocols work 100% on windows , 99% on linux and 0% on macOS. The linux one is a bug and macOS curl is a known trouble spot in CoreFX that they have been steadily improving.

I'd rather leave it in and let advanced users give it a try than take the option away.

If there is a concern, it could be documented that "setting multiple protocols is possible, though not supported on all platforms".

Collaborator

markekraus commented Nov 11, 2017

@iSazonov Your recommendation is to switch it to a Non-Flags Enum?

My plan was to not document the possibility of multiple protocols and leave as an exercise for advanced users. They don't show up in intellisense and unless someone advanced enough in the l;language took a good look at the Enum, they wouldn't really know.

Multiple protocols work 100% on windows , 99% on linux and 0% on macOS. The linux one is a bug and macOS curl is a known trouble spot in CoreFX that they have been steadily improving.

I'd rather leave it in and let advanced users give it a try than take the option away.

If there is a concern, it could be documented that "setting multiple protocols is possible, though not supported on all platforms".

@iSazonov

This comment has been minimized.

Show comment
Hide comment
@iSazonov

iSazonov Nov 11, 2017

Collaborator

Thinking about security I believe we lost nothing if remove Flags. @PaulHigin Could you please comment?

Collaborator

iSazonov commented Nov 11, 2017

Thinking about security I believe we lost nothing if remove Flags. @PaulHigin Could you please comment?

@markekraus

This comment has been minimized.

Show comment
Hide comment
@markekraus

markekraus Nov 11, 2017

Collaborator

@iSazonov you are right in that there is no security loss. But, lets say a user has a contractual agreement to use TLS 1.1 at minimum, they could supply 'Tls11, Tls12' and ensure the endpoint isn't using TLS 1.0. That works perfect on Windows and Linux. Now we take the Flags away the user must now call first with TLS 1.1 and then try again with TLS 1.2.

So no, security loss, but, we do make it harder for the user.

Collaborator

markekraus commented Nov 11, 2017

@iSazonov you are right in that there is no security loss. But, lets say a user has a contractual agreement to use TLS 1.1 at minimum, they could supply 'Tls11, Tls12' and ensure the endpoint isn't using TLS 1.0. That works perfect on Windows and Linux. Now we take the Flags away the user must now call first with TLS 1.1 and then try again with TLS 1.2.

So no, security loss, but, we do make it harder for the user.

@iSazonov

This comment has been minimized.

Show comment
Hide comment
@iSazonov

iSazonov Nov 11, 2017

Collaborator

Since POODLE attacks TLS fallbacks was disabled. Currently only client moderated fallbacks is allowed by means of TLS_FALLBACK_SCSV fake cipher suite.
I'm sure this is a lower level API than PowerShell. I don't know if CoreFX support this. If no so there's no point in specifying multiple protocols.
If CoreFX could support this, we could indicate Tls12,Tls11, which would mean: try to connect with Tls12, if server reply with TLS_FALLBACK_SCSV then use Tls11.

Collaborator

iSazonov commented Nov 11, 2017

Since POODLE attacks TLS fallbacks was disabled. Currently only client moderated fallbacks is allowed by means of TLS_FALLBACK_SCSV fake cipher suite.
I'm sure this is a lower level API than PowerShell. I don't know if CoreFX support this. If no so there's no point in specifying multiple protocols.
If CoreFX could support this, we could indicate Tls12,Tls11, which would mean: try to connect with Tls12, if server reply with TLS_FALLBACK_SCSV then use Tls11.

@markekraus

This comment has been minimized.

Show comment
Hide comment
@markekraus

markekraus Nov 11, 2017

Collaborator

@iSazonov I'm not exactly sure what is being done, but I know for a fact from the tests in This PR, that if you supply Tls12, Tls11 to a Tls12 endpoint it works, to a Tls11 endpoints it works, to a Tls endpoint it fails. So at the very least it is allow for only Tls12 and Tls11 endpoints.

Collaborator

markekraus commented Nov 11, 2017

@iSazonov I'm not exactly sure what is being done, but I know for a fact from the tests in This PR, that if you supply Tls12, Tls11 to a Tls12 endpoint it works, to a Tls11 endpoints it works, to a Tls endpoint it fails. So at the very least it is allow for only Tls12 and Tls11 endpoints.

@TravisEz13

This comment has been minimized.

Show comment
Hide comment
@TravisEz13

TravisEz13 Nov 11, 2017

Member

This design is similar to most browsers. I don't think we need to remove fallback. The user can of course specify just one protocol. We are giving the user the control.

Member

TravisEz13 commented Nov 11, 2017

This design is similar to most browsers. I don't think we need to remove fallback. The user can of course specify just one protocol. We are giving the user the control.

@iSazonov

This comment has been minimized.

Show comment
Hide comment
@iSazonov

iSazonov Nov 12, 2017

Collaborator

The design - yes, most of browsers has check boxes to select some protocols. But its behavior is secure - all of browsers disable fallback since POODLE discovered. Currently client and server must support TLS_FALLBACK_SCSV to do moderated fallback. If PowerShell and .Net Core allow to select some protocols and don't support TLS_FALLBACK_SCSV - it is security hole, an user using PowerShell can be redirected to a fake (Azure) server.

Collaborator

iSazonov commented Nov 12, 2017

The design - yes, most of browsers has check boxes to select some protocols. But its behavior is secure - all of browsers disable fallback since POODLE discovered. Currently client and server must support TLS_FALLBACK_SCSV to do moderated fallback. If PowerShell and .Net Core allow to select some protocols and don't support TLS_FALLBACK_SCSV - it is security hole, an user using PowerShell can be redirected to a fake (Azure) server.

@markekraus

This comment has been minimized.

Show comment
Hide comment
@markekraus

markekraus Nov 12, 2017

Collaborator

@iSazonov I do not follow your logic.

User has a requirement to NOT use TLS 1.0. The user supplies -SslProtocol 'Tls11, Tls12'. Here are the conditions of the endpoint:

End Point Protocol Result
TLS 1.0 FAILED
TLS 1.1 Success
TLS 1.2 Success

I'm not sure how POODLE is even a problem since SSL 3.0 is not even supported in CoreFX by the HttpClient.

When HttpClientHandler.SslProtocols is not set, supplied None, or supplied Tls, Tls11, Tls12 they act exactly the same except for the logic to determine which Flags were supplied. So if this is insecure, for some reason, it is already insecure and supplying multiple Flags doesn't change that. At the very least, it offers enhanced security because at least the user can disallow a specific protocol.

Collaborator

markekraus commented Nov 12, 2017

@iSazonov I do not follow your logic.

User has a requirement to NOT use TLS 1.0. The user supplies -SslProtocol 'Tls11, Tls12'. Here are the conditions of the endpoint:

End Point Protocol Result
TLS 1.0 FAILED
TLS 1.1 Success
TLS 1.2 Success

I'm not sure how POODLE is even a problem since SSL 3.0 is not even supported in CoreFX by the HttpClient.

When HttpClientHandler.SslProtocols is not set, supplied None, or supplied Tls, Tls11, Tls12 they act exactly the same except for the logic to determine which Flags were supplied. So if this is insecure, for some reason, it is already insecure and supplying multiple Flags doesn't change that. At the very least, it offers enhanced security because at least the user can disallow a specific protocol.

@iSazonov

This comment has been minimized.

Show comment
Hide comment
@iSazonov

iSazonov Nov 12, 2017

Collaborator

@markekraus Currently not only TLS->SSL fallback is security hole, TLS high level -> TLS low level also is security hole and is under the prohibition. If PowerShell is trying to connect with TLS 1.2 and server reject the connection, PowerShell shouldn't fallback to TLS 1.1 in classic way (only by supporting TLS_FALLBACK_SCSV).
So main question is - do underline system (CoreFX) implement the fallback on TLS_FALLBACK_SCSV? If yes I am fine with setting many protocols, if no I believe we shouldn't violate security and the modern standard for the TLS_FALLBACK_SCSV fallback.

Also if today CoreFX don't implement the same behavior for all platforms it is better for script portability to be limited to one protocol.

Collaborator

iSazonov commented Nov 12, 2017

@markekraus Currently not only TLS->SSL fallback is security hole, TLS high level -> TLS low level also is security hole and is under the prohibition. If PowerShell is trying to connect with TLS 1.2 and server reject the connection, PowerShell shouldn't fallback to TLS 1.1 in classic way (only by supporting TLS_FALLBACK_SCSV).
So main question is - do underline system (CoreFX) implement the fallback on TLS_FALLBACK_SCSV? If yes I am fine with setting many protocols, if no I believe we shouldn't violate security and the modern standard for the TLS_FALLBACK_SCSV fallback.

Also if today CoreFX don't implement the same behavior for all platforms it is better for script portability to be limited to one protocol.

@markekraus

This comment has been minimized.

Show comment
Hide comment
@markekraus

markekraus Nov 12, 2017

Collaborator

@iSazonov I doesn't matter. if the user doesn't want TLS 1.0, or TLS 1,1, they specify -SslProtocol 'Tls12' The connection will not work on anything other than TLS 1.2. The test in this PR prove that.

Collaborator

markekraus commented Nov 12, 2017

@iSazonov I doesn't matter. if the user doesn't want TLS 1.0, or TLS 1,1, they specify -SslProtocol 'Tls12' The connection will not work on anything other than TLS 1.2. The test in this PR prove that.

@markekraus

This comment has been minimized.

Show comment
Hide comment
@markekraus

markekraus Nov 12, 2017

Collaborator

Also if today CoreFX don't implement the same behavior for all platforms it is better for script portability to be limited to one protocol.

Then we should rip out all of our security features for macOS since they are all partially or not supported or only work under the right circumstances. or, we should completely rip out these cmdlets from macOS because the entire CoreFX HttpClient implementation on macOS is riddled with issues. When we go RTM I am expecting macOS users to flood with web cmdlets issue. CoreFX is just weak there.

This kind of thing just needs to be documented and to a degree it will be expected by users. Python, PHP, and other cross-platform languages have plenty of their own cross-platform implementation quirks. Something being either not supported on one platform or partially supported on another never stops other languages from implementing enhancements for a majority of platforms.

Again, multiple protocols works 100% on Windows, 99% on Linux (the one instance that doesn't work appears to be a bug in CoreFX) and not at all on macOS. I don't think throwing the feature out just because macOS and CoreFX have issues with libcurl is worth it, when all windows platforms, and almost all Linux platforms will support it. If we start down that path we will never get to implement anything that is an improvement for most platforms just because one platform happens to be fragile. If this only worked on Windows, then maybe I would agree it's not worth implementing.

CoreFX will have better support for these things in macOS in the future. Their issues are littered with HttpClient implementation issues on macOS. They know it's a weak point and we should acknowledge it as one for ourselves as well.

Collaborator

markekraus commented Nov 12, 2017

Also if today CoreFX don't implement the same behavior for all platforms it is better for script portability to be limited to one protocol.

Then we should rip out all of our security features for macOS since they are all partially or not supported or only work under the right circumstances. or, we should completely rip out these cmdlets from macOS because the entire CoreFX HttpClient implementation on macOS is riddled with issues. When we go RTM I am expecting macOS users to flood with web cmdlets issue. CoreFX is just weak there.

This kind of thing just needs to be documented and to a degree it will be expected by users. Python, PHP, and other cross-platform languages have plenty of their own cross-platform implementation quirks. Something being either not supported on one platform or partially supported on another never stops other languages from implementing enhancements for a majority of platforms.

Again, multiple protocols works 100% on Windows, 99% on Linux (the one instance that doesn't work appears to be a bug in CoreFX) and not at all on macOS. I don't think throwing the feature out just because macOS and CoreFX have issues with libcurl is worth it, when all windows platforms, and almost all Linux platforms will support it. If we start down that path we will never get to implement anything that is an improvement for most platforms just because one platform happens to be fragile. If this only worked on Windows, then maybe I would agree it's not worth implementing.

CoreFX will have better support for these things in macOS in the future. Their issues are littered with HttpClient implementation issues on macOS. They know it's a weak point and we should acknowledge it as one for ourselves as well.

@iSazonov

This comment has been minimized.

Show comment
Hide comment
@iSazonov

iSazonov Nov 12, 2017

Collaborator

we should completely rip out these cmdlets from macOS because the entire CoreFX HttpClient implementation on macOS is riddled with issues.

If I were a user of PowerShell on MacOs, I'd feel cheated if I had cmdlets that didn't work.

Collaborator

iSazonov commented Nov 12, 2017

we should completely rip out these cmdlets from macOS because the entire CoreFX HttpClient implementation on macOS is riddled with issues.

If I were a user of PowerShell on MacOs, I'd feel cheated if I had cmdlets that didn't work.

@markekraus

This comment has been minimized.

Show comment
Hide comment
@markekraus

markekraus Nov 12, 2017

Collaborator

If I were a user of PowerShell on MacOs, I'd feel cheated if I had cmdlets that didn't work.

I'd rather have a web cmdlets that at least works for the most part with basic web calls than none at all. Even if certain features aren't all supported on my platform, I'd still want it. The point is that partial support is better than none at all.

Collaborator

markekraus commented Nov 12, 2017

If I were a user of PowerShell on MacOs, I'd feel cheated if I had cmdlets that didn't work.

I'd rather have a web cmdlets that at least works for the most part with basic web calls than none at all. Even if certain features aren't all supported on my platform, I'd still want it. The point is that partial support is better than none at all.

@TravisEz13

This comment has been minimized.

Show comment
Hide comment
@TravisEz13

TravisEz13 Nov 13, 2017

Member

If we need to remove a disable the cmdlets for macOS file another issue. For the fallback issue, I think fallback is a required feature based on past experience. If we need a more restricted fallback, I think we would need support for .NET to support that.

Member

TravisEz13 commented Nov 13, 2017

If we need to remove a disable the cmdlets for macOS file another issue. For the fallback issue, I think fallback is a required feature based on past experience. If we need a more restricted fallback, I think we would need support for .NET to support that.

@TravisEz13 TravisEz13 merged commit ecf0f8c into PowerShell:master Nov 13, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@markekraus

This comment has been minimized.

Show comment
Hide comment
@markekraus

markekraus Nov 13, 2017

Collaborator

I'm leaving a note here for those who view this discussion in the future and for clarity to avoid panic:

The issues I have seen with HttpClient on macOS are not reliably reproducible. I am uncertain how much of it relates to an instability of CoreFX on macOS or just consistent bad luck and phantom external issues.

For the most part, simple HTTP calls work to a high rate of success. However, they will randomly fail even in our tests in this project for macOS where they are not failing for the same tests in Windows and Linux. Some of that maybe Travis CI related. However, since I acquired a macOS machine to test with I have seen similar stability issues on my own machine. Again, I don't know how much of that is just bad luck, my lack of experience with macOS in general, or possible stability issues in CoreFX. I cannot reliably reproduce the issues I find, so it is impossible at my skill level to track down a specific root cause.

As such, I can only say, without solid evidence, that I have general distrust for the stability of HttpClient on CoreFX and thus a similar feeling about the Web Cmdlets which use HttpClient. Without being able to quantify my experience, I cannot recommend the Cmdlets be removed from macOS.

It is clear, however, that many of the features relating to security and encryption are not fully implemented CoreFX on macOS. They work at the basic level depending on how the environment is configured. Sometimes they do not work at all, again, depending on how the environment is configured. However, I do not feel it is prudent to cripple these features for all platforms simply because a single platform cannot full implement them.

Collaborator

markekraus commented Nov 13, 2017

I'm leaving a note here for those who view this discussion in the future and for clarity to avoid panic:

The issues I have seen with HttpClient on macOS are not reliably reproducible. I am uncertain how much of it relates to an instability of CoreFX on macOS or just consistent bad luck and phantom external issues.

For the most part, simple HTTP calls work to a high rate of success. However, they will randomly fail even in our tests in this project for macOS where they are not failing for the same tests in Windows and Linux. Some of that maybe Travis CI related. However, since I acquired a macOS machine to test with I have seen similar stability issues on my own machine. Again, I don't know how much of that is just bad luck, my lack of experience with macOS in general, or possible stability issues in CoreFX. I cannot reliably reproduce the issues I find, so it is impossible at my skill level to track down a specific root cause.

As such, I can only say, without solid evidence, that I have general distrust for the stability of HttpClient on CoreFX and thus a similar feeling about the Web Cmdlets which use HttpClient. Without being able to quantify my experience, I cannot recommend the Cmdlets be removed from macOS.

It is clear, however, that many of the features relating to security and encryption are not fully implemented CoreFX on macOS. They work at the basic level depending on how the environment is configured. Sometimes they do not work at all, again, depending on how the environment is configured. However, I do not feel it is prudent to cripple these features for all platforms simply because a single platform cannot full implement them.

@iSazonov

This comment has been minimized.

Show comment
Hide comment
@iSazonov

iSazonov Nov 14, 2017

Collaborator

@markekraus Clould you please open tracking Issue? It wouldd be usefull for documentating the limitations and tracking CoreFX.

Collaborator

iSazonov commented Nov 14, 2017

@markekraus Clould you please open tracking Issue? It wouldd be usefull for documentating the limitations and tracking CoreFX.

@markekraus

This comment has been minimized.

Show comment
Hide comment
@markekraus

markekraus Nov 14, 2017

Collaborator

@iSazonov #4650 is related I could just update that. The underlying cause is the same.

I'm not sure if we should open a master issue to track all macOS limitations, or just Web Cmdlets. As I recall, there are several other cmdlets that have issues on macOS but I can't recall which ones immediately.

Collaborator

markekraus commented Nov 14, 2017

@iSazonov #4650 is related I could just update that. The underlying cause is the same.

I'm not sure if we should open a master issue to track all macOS limitations, or just Web Cmdlets. As I recall, there are several other cmdlets that have issues on macOS but I can't recall which ones immediately.

@iSazonov

This comment has been minimized.

Show comment
Hide comment
@iSazonov

iSazonov Nov 14, 2017

Collaborator

That would be very good if users were given this description (broken cmdlets) in the documentation (release notes). If you open the issue anyone can complete it at any time.

Collaborator

iSazonov commented Nov 14, 2017

That would be very good if users were given this description (broken cmdlets) in the documentation (release notes). If you open the issue anyone can complete it at any time.

@markekraus markekraus referenced this pull request Nov 14, 2017

Closed

macOS Limitations in 6.0.0 #5443

0 of 2 tasks complete
@markekraus

This comment has been minimized.

Show comment
Hide comment
@markekraus
Collaborator

markekraus commented Nov 14, 2017

@iSazonov #5443

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