ClientWebSocket supports client certificates in UWP#21908
Conversation
Diego-Perez-Botero
added
* NO MERGE *
Diego-Perez-Botero
requested review from
CIPop,
DavidGoll,
caesar-chen and
davidsh
|
Marking as NO MERGE while we wait for the UWP test runner to be updated with the Windows Insider SDK flight 16225 metadata (#21786). CC: @joperezr |
|
@dotnet-bot test NETFX x86 release build |
CIPop
left a comment
CIPop
left a comment
There was a problem hiding this comment.
We could make the UWP requirement more discoverable by throwing exceptions with clear messages.
| websocketControl.SupportedProtocols.Add(subProtocol); | ||
| } | ||
|
|
||
| if (MessageWebSocketClientCertificateSupported) |
There was a problem hiding this comment.
I think this should be discoverable by the app-developer as soon as possible - maybe when setting the ClientWebSocketOptions?
There was a problem hiding this comment.
Is your suggestion to throw an exception if MessageWebSocketClientCertificateSupported is false? If so, that sounds like an AppCompat issue (ClientWebSocket.ConnectAsync would stop working outside of Insider Preview builds).
There was a problem hiding this comment.
We should not throw exceptions for this kind of platform difference. See prior HttpClient UWP PRs. In general, we need to avoid throwing PNSE because it breaks developers.
There was a problem hiding this comment.
I was recommending throwing an exception when trying to get or set ClientCertificates on ClientWebSocketOptions.
I think it's better to be able to discover this immediately and root-cause it instead of trying to figure out why, after setting all properties, the app doesn't use client certificate authentication.
There was a problem hiding this comment.
A more recent example of light-up logic using no-op as fallback is #21905
UPDATE: That PR is now going with the "throw an exception" approach.
There was a problem hiding this comment.
In the new iteration, I'm throwing a NotSupportedException if an app has specified some client certs and the necessary WinRT API surface isn't available. The exception is being thrown as part of WinRTWebSocket.ConnectAsync for three reasons:
- Throwing while setting the ClientWebSocketOptions would require a large amount of changes in the PAL, since we should only throw if a client cert is actually added to the ClientWebSocketOptions.ClientCertificates collection. At a minimum, an OnInsert handler would need to be subscribed with the X509CertificateCollection and the necessary "is supported" information would need to be piped through the ClientWebSocketOptions and WebSocketHandle classes.
- WebSocketHandle.ConnectAsyncCore (see WebSocketHandle.WinRT.cs) calls into WinRTWebSocket.ConnectAsync (this method) and catches all exceptions. Those exceptions are then used as the inner exception for a new WebSocketException. Thus, app developers would not need to add a new "catch" statement just to handle this scenario (they should already be catching WebSocketException).
- A descriptive exception message (which I'm adding) still allows a developer to figure out what went wrong.
| foreach (X509Certificate dotNetClientCert in options.ClientCertificates) | ||
| { | ||
| RTCertificate winRtClientCert = ConvertDotNetClientCertToWinRtClientCert(dotNetClientCert); | ||
| if (winRtClientCert != null) |
There was a problem hiding this comment.
We should throw for this case as well (maybe when setting the ClientWebSocketOptions) with a good text indicating that the developer should install the certificate in the My store for this to work.
There was a problem hiding this comment.
.NET Framework doesn't have an equivalent exception type/message. Are we OK with adding a UWP-specific exception here? I'm all for it, but just want to confirm that there's a precedent for that type of divergence in behavior.
There was a problem hiding this comment.
:Are we OK with adding a UWP-specific exception here?
No. We do not add new UWP-specific exceptions like that since it will break devs depending on NETStandard behaviors.
There was a problem hiding this comment.
At most, we should fallback gracefully but log the problem, etc.
There was a problem hiding this comment.
We throw a lot of exceptions and sometimes differently for different implementations. In this case I would expect that the call fails as we cannot honor the given configuration. Something on the lines of "the client certificate will not be used as we cannot access the private key". We don't need to invent a new exception - we can easily reuse exceptions thrown by ConnectAsync such as WebSocketException, ArgumentException, InvalidOperationException, etc.
There was a problem hiding this comment.
Throwing an exception here seems less controversial to me. Unlike the other comment, this exception would be related to the app's certificate usage (fixable by the app developer) instead of the Windows build number (nothing the app developer can do about it).
There was a problem hiding this comment.
In the new iteration, I'm throwing a NotSupportedException if an app has specified some client certs and first cert cannot be converted into a WinRT Certificate. The exception is being thrown as part of WinRTWebSocket.ConnectAsync following the same rationale explained in the other comment thread.
|
@dotnet-bot help |
|
Welcome to the dotnet/corefx Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/corefx:master. Click to expand
The following optional jobs are available in PRs against dotnet/corefx:master. Click to expand
Have a nice day! |
|
Welcome to the dotnet/corefx Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/corefx:master. Click to expand
The following optional jobs are available in PRs against dotnet/corefx:master. Click to expand
Have a nice day! |
|
@dotnet-bot Test Outerloop Windows x86 Release Build |
| { | ||
| if (!MessageWebSocketClientCertificateSupported) | ||
| { | ||
| throw new NotSupportedException(string.Format( |
There was a problem hiding this comment.
We usually throw PlatformNotSupportedException for things like this. See recent HttpClient PR #21905.
The NotSupportedException is ok for the other case about certain client certificates not working. But for this exception, it should be PlatformNotSupportedException because the user can solve this by upgrading the platform to a more recent Windows 10 OS.
| SR.net_WebSockets_UWPClientCertSupportRequiresWindows10InsiderPreviewBuild16215OrGreater)); | ||
| } | ||
|
|
||
| RTCertificate winRtClientCert = ConvertDotNetClientCertToWinRtClientCert(options.ClientCertificates[0]); |
There was a problem hiding this comment.
One thing to mention is that I see you are only taking the first certificate from the array of certs. Did you ever wonder why there is an array of certs and not a parameter taking a single cert?
Historically, System.Net always took a collection of client certificates because some of the client certs could have different issuers. And the old networking stack would pass the trusted issuers list from the server to the client-side network stack during the network i/o. So, System.Net would use that information and help filter down the collection to find client certs that matched the issuers. And then it would take the first one from that.
By default, now, the Windows Server OS doesn't send a trusted issuer list for client cert negotiation. And since we're using WinRT APIs, we don't have visibility into that trusted issuers list anymore.
So, in general, we ended up always just using the first certificate.
I do a little filtering on the list of certs to make sure they are client certs, etc. and then take the first one from that.
There was a problem hiding this comment.
Thanks for the context on why this is taking a collection of certs. It wasn't clear at all from the MSDN documentation. I'll enhance this to do similar filtering, although I'll need to figure out how to handle the case where none of the certs satisfy those additional constraints.
There was a problem hiding this comment.
Thanks for the context on why this is taking a collection of certs. It wasn't clear at all from the MSDN documentation.
The ClientWebSocketOptions object and ClientWebSocket itself came originally from .NET Framework. And it was based on HttpWebRequest originally for the HTTP part and then a separate set of code to handle the WebSocket protocol. HttpWebRequest HTTP stack takes a collection of client certs. So, that is why the API uses a collection .... since it matches the convention of HttpWebRequest.
| "ClientCertificate"); | ||
| } | ||
|
|
||
| // There are currently only two ways to convert a .NET X509Certificate object into a WinRT Certificate without |
There was a problem hiding this comment.
Why is X509Certificate being used instead of the X509Certificate2 object? This comments applies generally to this file. In reality, they are probably always X509Certificate2 objects.
There was a problem hiding this comment.
ClientWebSocketOptions.ClientCertificates is of type X509CertificateCollection and all of its methods deal with X509Certificate objects. If it were an X509Certificate2Collection, we'd be dealing with X509Certificate2 objects.
There was a problem hiding this comment.
Ok. In reality X509Certificate2 is a derived class of X509Certificate. So, any X509Certificate2 is already an X509Certificate object.
As long as you don't need to call X509Certificate2 methods directly, it doesn't matter. If you did need the benefit of calling the extra X509Certificate2 methods, you could test the up-casting to X509Certificate2.
There was a problem hiding this comment.
So far, the X509Certificate contract has been sufficient :)
There was a problem hiding this comment.
Actually, the new cert filtering logic forced me to switch over to using X509Certificate2 objects.
|
Adding @bartonjs for review. |
|
@dotnet-bot Test Outerloop Windows x86 Release Build |
| <value>The requested security protocol is not supported.</value> | ||
| </data> | ||
| <data name="net_WebSockets_UWPClientCertSupportRequiresWindows10InsiderPreviewBuild16215OrGreater" xml:space="preserve"> | ||
| <value>Support for client certificates in UWP requires Windows 10 Insider Preview Build 16215 or greater.</value> |
There was a problem hiding this comment.
Seems weird to bake in the pre-release name and number here.
There was a problem hiding this comment.
We do this for now since this is the official pre-release name and version. When the Windows 10 Fall Creators Update ships, we'll update this again with the official RTM Windows version name. This is similar to what we do with "Windows 10 Version 1607" which means build 14393.
There was a problem hiding this comment.
Agreed. Although literally true, we should make this more generic. Something like, "... unsupported in Windows 10 version 1703 and earlier". As of yesterday, the insider preview build is version 16232, so there's quickly diminishing relevance of baking in the RS3 version.
In reply to: 126160835 [](ancestors = 126160835)
There was a problem hiding this comment.
Then again, we can keep this as-is for now and update it later when RS3 RTMs. That may or may not be within the 2.1.0 milestone. We should open an issue to track it.
In reply to: 126186269 [](ancestors = 126186269,126160835)
There was a problem hiding this comment.
"unsupported in Windows 10 version 1703 and earlier"
I like that although I would suggest making it clearer by adding more text.
"This feature is unsupported in Windows 10 version 1703 and earlier versions. Please upgrade Windows 10 to a later release."
| "ClientCertificate"); | ||
| } | ||
|
|
||
| private static X509Certificate2 GetEligibleClientCertificate(X509Certificate2Collection candidateCerts) |
There was a problem hiding this comment.
This looks like a copy/paste of the one from WInHttpCertificateHelper with an if 0 => null changed to an assert. Can they share?
They both seem to lack the if Count == 1 => use it optimization.
There was a problem hiding this comment.
We already have an issue #14542 to merge multiple copies of this code.
There was a problem hiding this comment.
Added a TODO comment.
| } | ||
| } | ||
|
|
||
| if (eligibleCerts.Count > 0) |
There was a problem hiding this comment.
Depending on your caring levels this function is anywhere from "meh" to "eep!" because Find returns a collection of clones instead of a filtered collection.
So let's say that someone adds in their entire CU\My store, and that it happens to be 45 certificates. 5 are public-key only (one of which has no EKU extension). All 45 have DigitalSignature. And 5 have either the client EKU or no EKU extension.
- 45 certs come in, which were the user's responsibility (and unless the user's me, they probably won't ever dispose them)
- Find (DigitalSignature) returns 45 new certificates.
- Find (ClientAuthOid) returns 5 new certificates.
- HasPrivateKey filtering reduces that to 4 eligible
- Why does web socket not have a server trusted issuers list filter? Oh well.
- One cert, guaranteed different than an input cert, is returned.
So this function created 50 new finalizable objects, returned one of them, and let 49 of them go to the finalizer queue.
Best practices for using Find:
- Don't call it if you don't need to. (if count == 1 => just return it)
- Make sure that you're reducing the candidate space as fast as possible.
- In this case, client EKU is probably more rare than the DigitalSignature KU.
- Though your input set it small, so maybe it doesn't matter.
- Once you've called Find you own the returned objects. Dispose them as able.
Using my made up numbers from above, if you filtered out the !HasPrivateKey certs first you'd be down to 40 certs into the first call to Find. If that was the client EKU test you'd now be down to 4 new certs, and 4 more new certs verifying the KU, one gets returned. 7 finalizations instead of 49. (not to mention fewer allocations)
There was a problem hiding this comment.
Thanks for the detailed explanation and the great suggestions, Jeremy. I've updated this method in the next iteration taking all of the above into account.
| return certificates[0]; | ||
| } | ||
|
|
||
| throw new NotSupportedException(string.Format( |
There was a problem hiding this comment.
Is NotSupportedException appropriate here? I thought that was more of "there's nothing you can do to fix this without rebooting into a different OS/OS-version or changing from one .NET implementation to another". Not "please add your cert to the CU\My store"
There was a problem hiding this comment.
I think we change this to PNSE (PlatformNotSupportedException) similar to others we have.
DavidGoll
left a comment
DavidGoll
left a comment
There was a problem hiding this comment.
Aside from switching the order of the Find calls, signing off.
|
@dotnet-bot Test Outerloop Windows x86 Release Build |
|
I've confirmed that UAP mode tests pass with this PR's changes on a recent Windows Insider flight, now that the test runner's metadata has been updated (#21985). I'm validating these changes on UAP mode in a Windows 10 Creator's Update machine to be 100% certain that it's now safe to merge this PR :) UPDATE: All tests are passing in UAP mode on Windows 10 Creator's Update. It looks like #21985 did the trick. |
|
@dotnet-bot test OSX x64 Debug Build |
Diego-Perez-Botero
removed
the
* NO MERGE *
|
Outerloop Windows x86 Release Build and Outerloop Linux x64 Release Build test failures are due to known ManagedHandler_HttpClientHandler_SslProtocols_Test issues (#21923 #21924 #21925 #21927 #21917). OSX x64 Debug Build build failures are unrelated to this PR. |
* Implement client certificates for HttpClient on UAP Added code for HttpClientHandler.ClientCertificates on UAP. Moved code duplicated in several places into a common helper class, CertificateHelper. Then I discovered a bug introduced into GetEligibleCertificates (from PR #21908) that doesn't properly filter by EKU or KeyUsage if there is only one cert (this was failing the WinHttpHandler Unit tests). So, I removed that optimization. Used the new Azure test endpoint for client certificates. I did not use the loopback server because there is a problem using it with HttpClient in UAP. There is a crash in the SslStream.AuthenticateAsServer that I was not able to diagnose yet. It will require a lot of time debugging and I wanted to get this HttpClientHandler feature done now. Tracking that investigation with #22021. Fixes #21628 Contributes to #14542 * Address PR feedback * Moved method to Windows-only Common file * Fixed some line spacing Other potential refactoring/optimizations will be looked into in #22045
) With these changes, the UWP implementation of ClientWebSocket.ConnectAsync takes the ClientCertificates member of the app-provided ClientWebSocketOptions into account. The first client cert that can be successfully converted into a WinRT client cert (if any) is configured on the underlying WinRT MessageWebSocket instance, leading to the cert being used for mutual TLS authentication when establishing connections with WSS endpoints. This fix leverages WinRT APIs that are only present since Windows 10 Insider Preview Build 16215, so API presence checks are in place. Fixes dotnet/corefx#21393 Commit migrated from dotnet/corefx@0c47830
) * Implement client certificates for HttpClient on UAP Added code for HttpClientHandler.ClientCertificates on UAP. Moved code duplicated in several places into a common helper class, CertificateHelper. Then I discovered a bug introduced into GetEligibleCertificates (from PR dotnet/corefx#21908) that doesn't properly filter by EKU or KeyUsage if there is only one cert (this was failing the WinHttpHandler Unit tests). So, I removed that optimization. Used the new Azure test endpoint for client certificates. I did not use the loopback server because there is a problem using it with HttpClient in UAP. There is a crash in the SslStream.AuthenticateAsServer that I was not able to diagnose yet. It will require a lot of time debugging and I wanted to get this HttpClientHandler feature done now. Tracking that investigation with dotnet/corefx#22021. Fixes dotnet/corefx#21628 Contributes to dotnet/corefx#14542 * Address PR feedback * Moved method to Windows-only Common file * Fixed some line spacing Other potential refactoring/optimizations will be looked into in dotnet/corefx#22045 Commit migrated from dotnet/corefx@472b8d8
8 participants
With these changes, the UWP implementation of ClientWebSocket.ConnectAsync takes the ClientCertificates member of the app-provided ClientWebSocketOptions into account. The first client cert that can be successfully converted into a WinRT client cert (if any) is configured on the underlying WinRT MessageWebSocket instance, leading to the cert being used for mutual TLS authentication when establishing connections with WSS endpoints.
This fix leverages WinRT APIs that are only present since Windows 10 Insider Preview Build 16215, so API presence checks are in place.
Fixes #21393
CC: @mconnew