You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Add cryptographic protocol parameter to web cmdlets. #2662
Greg Zimmerman (@thezim) Is the idea that outside of the provided crypto-proto(s) it will return an error? or to "best effort" the provided protocol?
One concern will be testing it. We need a separate port for each crypto-proto. WebListener will need to be expanded to include those ports and protocols. I'm not sure what all Kestrel can handle, but I believe at-least the ones you listed are available.
Mark Kraus (@markekraus) yes, anything outside of System.Net.SecurityProtocolType would error, ServicePointManager only accepts those. I've already modified WebListener to accept a new protocol argument, I didn't see the need to add an additional port as specifying the protocol to Kestrel was easy. New controller and view was added as well for testing feedback.
Greg Zimmerman (@thezim) The reason we would want separate ports is for ensuring a protocol. If you have some other way of ensuring a the parameter when set to TLS12 is erroring when SSL3 is used, then that's fine.
Since ServicePointManager is persistent, we should implement on HttpClientHandler.SslProtocols instead. If users want to set their own persistent protocols, they can research those APIs, but if we have some terminating exception in the middle of call where we set ServicePointManager.SecurityProtocol, the user will be left with subsequent calls exhibiting unexpected behavior.
Ilya (@iSazonov) the problem is that ServicePointManager affects more than just the web cmdlets and setting it can result in unintended consequences with other functions/cmdlets/modules even for things that are not doing HTTP requests. That kind of permanence may be OK in a scripting scenario, but will be terrifying in a console scenario. In Windows PowerShell this is already a pain.
One thing I would like to address for 6.1.0, is session-persistent settings for the web cmdlets only. That way it can be treated more like "browser" settings and less like "platform" settings. It would include these security protocols and the certificate validation callback (maybe even some predefined callbacks to use).
Ilya (@iSazonov) I think that can be tracked elsewhere. For this particular issue, so long as we implement on per-call through HttpClientHandler.SslProtocols it should be fine.
I'm just opposed to tickling other parts of .NET when that behavior is not apparent to the user. Adding session-persistent settings to the web cmdlets is probably too tall an order for 6.0.0. It's tied up with moving to a session persistent HttpClientHandler instead of a per-call instance. We would also want to wrap that implementation to handle the inevitable changes in CoreFX. And then we'd need to expose cmdlets for setting them as well as per-cmdlet overrides (different settings for irm vs iwr).
Point being, if that ever does get implemented, per-call overrides for these settings would still be acceptable. so we should be able to move forward with this one absent a decision by the committee. But I welcome their feedback anyway.
Greg Zimmerman (@thezim) I know you were working on this, but I'm assigning this to myself and submitting a PR as I want to try and get this added in before the release candidates block all new features.
Mark Kraus (@markekraus) understood. My apologies, I could never get consistent test results on my macOS due to the known issues. Sorry for not handing this over sooner.
Greg Zimmerman (@thezim) All good! thanks for helping me hammer this all out and get a clearer understanding! Didn't you have something in your test code to determine what TLS version was used from the request? I could not find anything in the ASP.NET Core API to get me the TLS version. I thought I saw something for it in your repo before but when i went looking this morning I didn't see the branch from before.
Mark Kraus (@markekraus) My WebListener took a command line protocol parameter. It only listened on one protocol. Assumption was if the test code was able to successfully connect that protocol was being used. View would return that value as well. I also could not find a way to get the explicit information from the request side (controller).
Ilya (@iSazonov) Yes, that is used for setting the option in the Kestrel server at launch. but, when a request comes in there is no way to see which protocol the request used in the controller. for example, in the program.cs it could be set as
And then it would support all three versions, but in the controler I would have no way to see what protocol was used. I ended up having to add 2 more listening ports (one for TLS 1.0 and another for TLS 1.1 because the HTTPS port was already TLS 1.2). I was trying to avoid adding more listener ports,
Activity
iSazonov commentedon Oct 7, 2017
/cc Mark Kraus (@markekraus) Have you interest to grab this?
markekraus commentedon Oct 7, 2017
Ilya (@iSazonov) can you add the Area-Cmdlets-Utility Label to it?
I'll add it to my list of things to look into and leave it up for grabs if someone else has the bandwidth before I can get to it.
thezim commentedon Oct 8, 2017
Mark Kraus (@markekraus) seems straight forward. I'll look in to it.
markekraus commentedon Oct 8, 2017
Greg Zimmerman (@thezim) Is the idea that outside of the provided crypto-proto(s) it will return an error? or to "best effort" the provided protocol?
One concern will be testing it. We need a separate port for each crypto-proto. WebListener will need to be expanded to include those ports and protocols. I'm not sure what all Kestrel can handle, but I believe at-least the ones you listed are available.
thezim commentedon Oct 8, 2017
Mark Kraus (@markekraus) yes, anything outside of
System.Net.SecurityProtocolTypewould error,ServicePointManageronly accepts those. I've already modified WebListener to accept a new protocol argument, I didn't see the need to add an additional port as specifying the protocol to Kestrel was easy. New controller and view was added as well for testing feedback.iSazonov commentedon Oct 8, 2017
We should follow CoreFX SecurityProtocolType - the parameter should be the type.
It seems we can use one port for tests.
markekraus commentedon Oct 8, 2017
Greg Zimmerman (@thezim) The reason we would want separate ports is for ensuring a protocol. If you have some other way of ensuring a the parameter when set to TLS12 is erroring when SSL3 is used, then that's fine.
Since
ServicePointManageris persistent, we should implement onHttpClientHandler.SslProtocolsinstead. If users want to set their own persistent protocols, they can research those APIs, but if we have some terminating exception in the middle of call where we setServicePointManager.SecurityProtocol, the user will be left with subsequent calls exhibiting unexpected behavior.iSazonov commentedon Oct 8, 2017
What scenarios do we want address?
Current browsers behavior is enable/disable protocol, no fallback to low version.
thezim commentedon Oct 8, 2017
Ilya (@iSazonov) if you're at the point of explicitly specifying a protocol then we should let it fail with no fallback.
markekraus commentedon Oct 8, 2017
Ilya (@iSazonov) the problem is that
ServicePointManageraffects more than just the web cmdlets and setting it can result in unintended consequences with other functions/cmdlets/modules even for things that are not doing HTTP requests. That kind of permanence may be OK in a scripting scenario, but will be terrifying in a console scenario. In Windows PowerShell this is already a pain.One thing I would like to address for 6.1.0, is session-persistent settings for the web cmdlets only. That way it can be treated more like "browser" settings and less like "platform" settings. It would include these security protocols and the certificate validation callback (maybe even some predefined callbacks to use).
iSazonov commentedon Oct 8, 2017
Mark Kraus (@markekraus) Thanks for clarify! I believe we should get a conclusion from @powershell-committee.
/cc Steve Lee (@SteveL-MSFT)
markekraus commentedon Oct 8, 2017
Ilya (@iSazonov) I think that can be tracked elsewhere. For this particular issue, so long as we implement on per-call through
HttpClientHandler.SslProtocolsit should be fine.I'm just opposed to tickling other parts of .NET when that behavior is not apparent to the user. Adding session-persistent settings to the web cmdlets is probably too tall an order for 6.0.0. It's tied up with moving to a session persistent
HttpClientHandlerinstead of a per-call instance. We would also want to wrap that implementation to handle the inevitable changes in CoreFX. And then we'd need to expose cmdlets for setting them as well as per-cmdlet overrides (different settings for irm vs iwr).Point being, if that ever does get implemented, per-call overrides for these settings would still be acceptable. so we should be able to move forward with this one absent a decision by the committee. But I welcome their feedback anyway.
19 remaining items
markekraus commentedon Nov 3, 2017
Greg Zimmerman (@thezim) I know you were working on this, but I'm assigning this to myself and submitting a PR as I want to try and get this added in before the release candidates block all new features.
thezim commentedon Nov 3, 2017
Mark Kraus (@markekraus) understood. My apologies, I could never get consistent test results on my macOS due to the known issues. Sorry for not handing this over sooner.
markekraus commentedon Nov 3, 2017
Greg Zimmerman (@thezim) All good! thanks for helping me hammer this all out and get a clearer understanding! Didn't you have something in your test code to determine what TLS version was used from the request? I could not find anything in the ASP.NET Core API to get me the TLS version. I thought I saw something for it in your repo before but when i went looking this morning I didn't see the branch from before.
iSazonov commentedon Nov 3, 2017
https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.server.kestrel.https.httpsconnectionadapteroptions.sslprotocols?view=aspnetcore-2.0
/cc Mark Kraus (@markekraus)
thezim commentedon Nov 3, 2017
Mark Kraus (@markekraus) My WebListener took a command line protocol parameter. It only listened on one protocol. Assumption was if the test code was able to successfully connect that protocol was being used. View would return that value as well. I also could not find a way to get the explicit information from the request side (controller).
markekraus commentedon Nov 3, 2017
Ilya (@iSazonov) Yes, that is used for setting the option in the Kestrel server at launch. but, when a request comes in there is no way to see which protocol the request used in the controller. for example, in the program.cs it could be set as
And then it would support all three versions, but in the controler I would have no way to see what protocol was used. I ended up having to add 2 more listening ports (one for TLS 1.0 and another for TLS 1.1 because the HTTPS port was already TLS 1.2). I was trying to avoid adding more listener ports,
iSazonov commentedon Nov 3, 2017
I guess that's the only way today.
Add SslProtocol Support to WebCmdlets (#5329)
Add SslProtocol Support to WebCmdlets (PowerShell#5329)