Fix SocketsNetHttpHandler TLS client cert handling (#27753)

davidsh · web-flow · commit d0cb45113979 · 2018-03-06T07:43:44.000-08:00
SocketNetHttpHandler was not filtering down the client certificates when sending in
"Manual" mode. This caused a client certificate to be sent to a server when it
didn't have the proper EKU OID for "Client Authentication".

Changed this so that both "Manual" and "Automatic" modes will use the CertificateHelper
GetEligibleCertificate() API.

Fixes #23128
diff --git a/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Unix.cs b/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Unix.cs
@@ -17,6 +17,7 @@ public partial class HttpClientHandler : HttpMessageHandler
         private readonly CurlHandler _curlHandler;
         private readonly SocketsHttpHandler _socketsHttpHandler;
         private readonly DiagnosticsHandler _diagnosticsHandler;
+        private ClientCertificateOption _clientCertificateOptions;
 
         public HttpClientHandler() : this(UseSocketsHttpHandler) { }
 
@@ -26,6 +27,7 @@ public HttpClientHandler() : this(UseSocketsHttpHandler) { }
             {
                 _socketsHttpHandler = new SocketsHttpHandler();
                 _diagnosticsHandler = new DiagnosticsHandler(_socketsHttpHandler);
+                ClientCertificateOptions = ClientCertificateOption.Manual;
             }
             else
             {
@@ -91,9 +93,7 @@ public ClientCertificateOption ClientCertificateOptions
                 }
                 else
                 {
-                    return _socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback != null ?
-                        ClientCertificateOption.Automatic :
-                        ClientCertificateOption.Manual;
+                    return _clientCertificateOptions;
                 }
             }
             set
@@ -108,11 +108,13 @@ public ClientCertificateOption ClientCertificateOptions
                     {
                         case ClientCertificateOption.Manual:
                             ThrowForModifiedManagedSslOptionsIfStarted();
-                            _socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback = null;
+                            _clientCertificateOptions = value;
+                            _socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback = (sender, targetHost, localCertificates, remoteCertificate, acceptableIssuers) => CertificateHelper.GetEligibleClientCertificate(ClientCertificates);
                             break;
 
                         case ClientCertificateOption.Automatic:
                             ThrowForModifiedManagedSslOptionsIfStarted();
+                            _clientCertificateOptions = value;
                             _socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback = (sender, targetHost, localCertificates, remoteCertificate, acceptableIssuers) => CertificateHelper.GetEligibleClientCertificate();
                             break;
 
diff --git a/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Windows.cs b/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Windows.cs
@@ -19,6 +19,7 @@ public partial class HttpClientHandler : HttpMessageHandler
         private readonly SocketsHttpHandler _socketsHttpHandler;
         private readonly DiagnosticsHandler _diagnosticsHandler;
         private bool _useProxy;
+        private ClientCertificateOption _clientCertificateOptions;
 
         public HttpClientHandler() : this(UseSocketsHttpHandler) { }
 
@@ -28,6 +29,8 @@ public HttpClientHandler() : this(UseSocketsHttpHandler) { }
             {
                 _socketsHttpHandler = new SocketsHttpHandler();
                 _diagnosticsHandler = new DiagnosticsHandler(_socketsHttpHandler);
+                ClientCertificateOptions = ClientCertificateOption.Manual;
+
             }
             else
             {
@@ -319,9 +322,7 @@ public ClientCertificateOption ClientCertificateOptions
                 }
                 else
                 {
-                    return _socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback != null ?
-                        ClientCertificateOption.Automatic :
-                        ClientCertificateOption.Manual;
+                    return _clientCertificateOptions;
                 }
             }
             set
@@ -336,11 +337,13 @@ public ClientCertificateOption ClientCertificateOptions
                     {
                         case ClientCertificateOption.Manual:
                             ThrowForModifiedManagedSslOptionsIfStarted();
-                            _socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback = null;
+                            _clientCertificateOptions = value;
+                            _socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback = (sender, targetHost, localCertificates, remoteCertificate, acceptableIssuers) => CertificateHelper.GetEligibleClientCertificate(ClientCertificates);
                             break;
 
                         case ClientCertificateOption.Automatic:
                             ThrowForModifiedManagedSslOptionsIfStarted();
+                            _clientCertificateOptions = value;
                             _socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback = (sender, targetHost, localCertificates, remoteCertificate, acceptableIssuers) => CertificateHelper.GetEligibleClientCertificate();
                             break;
 
diff --git a/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ClientCertificates.cs b/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ClientCertificates.cs
@@ -142,12 +142,6 @@ public void Manual_SendClientCertificateWithClientAuthEKUToRemoteServer_OK()
         [Fact]
         public void Manual_SendClientCertificateWithServerAuthEKUToRemoteServer_Forbidden()
         {
-            if (UseSocketsHttpHandler)
-            {
-                // TODO #23128: SocketsHttpHandler is currently sending out client certificates when it shouldn't.
-                return;
-            }
-
             if (!CanTestClientCertificates) // can't use [Conditional*] right now as it's evaluated at the wrong time for SocketsHttpHandler
             {
                 _output.WriteLine($"Skipping {nameof(Manual_SendClientCertificateWithServerAuthEKUToRemoteServer_Forbidden)}()");

Original file line number	Diff line number	Diff line change
`@@ -17,6 +17,7 @@ public partial class HttpClientHandler : HttpMessageHandler`
`17`	`17`	`private readonly CurlHandler _curlHandler;`
`18`	`18`	`private readonly SocketsHttpHandler _socketsHttpHandler;`
`19`	`19`	`private readonly DiagnosticsHandler _diagnosticsHandler;`
	`20`	`+ private ClientCertificateOption _clientCertificateOptions;`
`20`	`21`
`21`	`22`	`public HttpClientHandler() : this(UseSocketsHttpHandler) { }`
`22`	`23`
`@@ -26,6 +27,7 @@ public HttpClientHandler() : this(UseSocketsHttpHandler) { }`
`26`	`27`	`{`
`27`	`28`	`_socketsHttpHandler = new SocketsHttpHandler();`
`28`	`29`	`_diagnosticsHandler = new DiagnosticsHandler(_socketsHttpHandler);`
	`30`	`+ ClientCertificateOptions = ClientCertificateOption.Manual;`
`29`	`31`	`}`
`30`	`32`	`else`
`31`	`33`	`{`
`@@ -91,9 +93,7 @@ public ClientCertificateOption ClientCertificateOptions`
`91`	`93`	`}`
`92`	`94`	`else`
`93`	`95`	`{`
`94`		`- return _socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback != null ?`
`95`		`- ClientCertificateOption.Automatic :`
`96`		`- ClientCertificateOption.Manual;`
	`96`	`+ return _clientCertificateOptions;`
`97`	`97`	`}`
`98`	`98`	`}`
`99`	`99`	`set`
`@@ -108,11 +108,13 @@ public ClientCertificateOption ClientCertificateOptions`
`108`	`108`	`{`
`109`	`109`	`case ClientCertificateOption.Manual:`
`110`	`110`	`ThrowForModifiedManagedSslOptionsIfStarted();`
`111`		`- _socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback = null;`
	`111`	`+ _clientCertificateOptions = value;`
	`112`	`+ _socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback = (sender, targetHost, localCertificates, remoteCertificate, acceptableIssuers) => CertificateHelper.GetEligibleClientCertificate(ClientCertificates);`
`112`	`113`	`break;`
`113`	`114`
`114`	`115`	`case ClientCertificateOption.Automatic:`
`115`	`116`	`ThrowForModifiedManagedSslOptionsIfStarted();`
	`117`	`+ _clientCertificateOptions = value;`
`116`	`118`	`_socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback = (sender, targetHost, localCertificates, remoteCertificate, acceptableIssuers) => CertificateHelper.GetEligibleClientCertificate();`
`117`	`119`	`break;`
`118`	`120`
Original file line number	Diff line number	Diff line change
`@@ -19,6 +19,7 @@ public partial class HttpClientHandler : HttpMessageHandler`
`19`	`19`	`private readonly SocketsHttpHandler _socketsHttpHandler;`
`20`	`20`	`private readonly DiagnosticsHandler _diagnosticsHandler;`
`21`	`21`	`private bool _useProxy;`
	`22`	`+ private ClientCertificateOption _clientCertificateOptions;`
`22`	`23`
`23`	`24`	`public HttpClientHandler() : this(UseSocketsHttpHandler) { }`
`24`	`25`
`@@ -28,6 +29,8 @@ public HttpClientHandler() : this(UseSocketsHttpHandler) { }`
`28`	`29`	`{`
`29`	`30`	`_socketsHttpHandler = new SocketsHttpHandler();`
`30`	`31`	`_diagnosticsHandler = new DiagnosticsHandler(_socketsHttpHandler);`
	`32`	`+ ClientCertificateOptions = ClientCertificateOption.Manual;`
	`33`	`+`
`31`	`34`	`}`
`32`	`35`	`else`
`33`	`36`	`{`
`@@ -319,9 +322,7 @@ public ClientCertificateOption ClientCertificateOptions`
`319`	`322`	`}`
`320`	`323`	`else`
`321`	`324`	`{`
`322`		`- return _socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback != null ?`
`323`		`- ClientCertificateOption.Automatic :`
`324`		`- ClientCertificateOption.Manual;`
	`325`	`+ return _clientCertificateOptions;`
`325`	`326`	`}`
`326`	`327`	`}`
`327`	`328`	`set`
`@@ -336,11 +337,13 @@ public ClientCertificateOption ClientCertificateOptions`
`336`	`337`	`{`
`337`	`338`	`case ClientCertificateOption.Manual:`
`338`	`339`	`ThrowForModifiedManagedSslOptionsIfStarted();`
`339`		`- _socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback = null;`
	`340`	`+ _clientCertificateOptions = value;`
	`341`	`+ _socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback = (sender, targetHost, localCertificates, remoteCertificate, acceptableIssuers) => CertificateHelper.GetEligibleClientCertificate(ClientCertificates);`
`340`	`342`	`break;`
`341`	`343`
`342`	`344`	`case ClientCertificateOption.Automatic:`
`343`	`345`	`ThrowForModifiedManagedSslOptionsIfStarted();`
	`346`	`+ _clientCertificateOptions = value;`
`344`	`347`	`_socketsHttpHandler.SslOptions.LocalCertificateSelectionCallback = (sender, targetHost, localCertificates, remoteCertificate, acceptableIssuers) => CertificateHelper.GetEligibleClientCertificate();`
`345`	`348`	`break;`
`346`	`349`
Original file line number	Diff line number	Diff line change
`@@ -142,12 +142,6 @@ public void Manual_SendClientCertificateWithClientAuthEKUToRemoteServer_OK()`
`142`	`142`	`[Fact]`
`143`	`143`	`public void Manual_SendClientCertificateWithServerAuthEKUToRemoteServer_Forbidden()`
`144`	`144`	`{`
`145`		`- if (UseSocketsHttpHandler)`
`146`		`- {`
`147`		`- // TODO #23128: SocketsHttpHandler is currently sending out client certificates when it shouldn't.`
`148`		`- return;`
`149`		`- }`
`150`		`-`
`151`	`145`	`if (!CanTestClientCertificates) // can't use [Conditional*] right now as it's evaluated at the wrong time for SocketsHttpHandler`
`152`	`146`	`{`
`153`	`147`	`_output.WriteLine($"Skipping {nameof(Manual_SendClientCertificateWithServerAuthEKUToRemoteServer_Forbidden)}()");`