From 1dd042720a90dbdaa7efa415ca0e73d977a290c9 Mon Sep 17 00:00:00 2001 From: David Shulman Date: Wed, 5 Jul 2017 12:22:49 -0700 Subject: [PATCH 1/2] Fix HttpClientHandler ServerCertificateCustomValidationCallback for UWP This PR implements the ServerCertificateCustomValidationCallback for UWP. While implementing this, I discovered that the behavior of the current .NET Core code is incorrect regarding how to propagate exceptions from the user provided callback. I opened up a new issue #21904 and revised the test as well. Fixes #21627 --- .../System/Net/Http/WinHttpRequestCallback.cs | 6 +- .../src/System.Net.Http.csproj | 12 +- .../System/Net/CookieHelper.cs | 0 .../System/Net/HttpClientHandler.cs | 135 ++++++++++++++++-- .../System/Net/HttpHandlerToFilter.cs | 29 +++- .../{netcore50 => uap}/System/Net/cookie.cs | 0 .../System/Net/cookieexception.cs | 0 .../HttpClientHandlerTest.AcceptAllCerts.cs | 6 +- ...ttpClientHandlerTest.ServerCertificates.cs | 66 ++++++--- 9 files changed, 214 insertions(+), 40 deletions(-) rename src/System.Net.Http/src/{netcore50 => uap}/System/Net/CookieHelper.cs (100%) rename src/System.Net.Http/src/{netcore50 => uap}/System/Net/HttpClientHandler.cs (78%) rename src/System.Net.Http/src/{netcore50 => uap}/System/Net/HttpHandlerToFilter.cs (92%) rename src/System.Net.Http/src/{netcore50 => uap}/System/Net/cookie.cs (100%) rename src/System.Net.Http/src/{netcore50 => uap}/System/Net/cookieexception.cs (100%) diff --git a/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs b/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs index 7a80bb75b02c..636229ad3f2b 100644 --- a/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs +++ b/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs @@ -350,7 +350,7 @@ private static void OnRequestError(WinHttpRequestState state, Interop.WinHttp.WI { // TODO: Issue #2165. We need to pass in the cancellation token from the // user's ReadAsync() call into the TrySetCanceled(). - Debug.WriteLine("RequestCallback: QUERY_DATA_AVAILABLE - ERROR_WINHTTP_OPERATION_CANCELLED"); + WinHttpTraceHelper.Trace("RequestCallback: QUERY_DATA_AVAILABLE - ERROR_WINHTTP_OPERATION_CANCELLED"); state.LifecycleAwaitable.SetCanceled(); } else @@ -365,7 +365,7 @@ private static void OnRequestError(WinHttpRequestState state, Interop.WinHttp.WI { // TODO: Issue #2165. We need to pass in the cancellation token from the // user's ReadAsync() call into the TrySetCanceled(). - Debug.WriteLine("RequestCallback: API_READ_DATA - ERROR_WINHTTP_OPERATION_CANCELLED"); + WinHttpTraceHelper.Trace("RequestCallback: API_READ_DATA - ERROR_WINHTTP_OPERATION_CANCELLED"); state.LifecycleAwaitable.SetCanceled(); } else @@ -379,7 +379,7 @@ private static void OnRequestError(WinHttpRequestState state, Interop.WinHttp.WI { // TODO: Issue #2165. We need to pass in the cancellation token from the // user's WriteAsync() call into the TrySetCanceled(). - Debug.WriteLine("RequestCallback: API_WRITE_DATA - ERROR_WINHTTP_OPERATION_CANCELLED"); + WinHttpTraceHelper.Trace("RequestCallback: API_WRITE_DATA - ERROR_WINHTTP_OPERATION_CANCELLED"); state.TcsInternalWriteDataToRequestStream.TrySetCanceled(); } else diff --git a/src/System.Net.Http/src/System.Net.Http.csproj b/src/System.Net.Http/src/System.Net.Http.csproj index 47e8b4e05652..22be9e2eabd6 100644 --- a/src/System.Net.Http/src/System.Net.Http.csproj +++ b/src/System.Net.Http/src/System.Net.Http.csproj @@ -372,7 +372,9 @@ + + Common\System\NotImplemented.cs @@ -389,11 +391,11 @@ Common\System\Threading\Tasks\TaskToApm.cs - - - - - + + + + + diff --git a/src/System.Net.Http/src/netcore50/System/Net/CookieHelper.cs b/src/System.Net.Http/src/uap/System/Net/CookieHelper.cs similarity index 100% rename from src/System.Net.Http/src/netcore50/System/Net/CookieHelper.cs rename to src/System.Net.Http/src/uap/System/Net/CookieHelper.cs diff --git a/src/System.Net.Http/src/netcore50/System/Net/HttpClientHandler.cs b/src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs similarity index 78% rename from src/System.Net.Http/src/netcore50/System/Net/HttpClientHandler.cs rename to src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs index 5a0717e35c6e..fa2e5b6cd4f8 100644 --- a/src/System.Net.Http/src/netcore50/System/Net/HttpClientHandler.cs +++ b/src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs @@ -8,9 +8,12 @@ using System.Net; using System.Net.Http.Headers; using System.Net.Security; +using System.Runtime.ExceptionServices; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; +using System.Runtime.InteropServices.WindowsRuntime; using System.Security.Authentication; +using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; using System.Threading; using System.Threading.Tasks; @@ -24,18 +27,27 @@ using RTCertificate = Windows.Security.Cryptography.Certificates.Certificate; using RTCertificateQuery = Windows.Security.Cryptography.Certificates.CertificateQuery; using RTCertificateStores = Windows.Security.Cryptography.Certificates.CertificateStores; +using RTChainValidationResult = Windows.Security.Cryptography.Certificates.ChainValidationResult; +using RTHttpServerCustomValidationRequestedEventArgs = Windows.Web.Http.Filters.HttpServerCustomValidationRequestedEventArgs; +using RTIBuffer = Windows.Storage.Streams.IBuffer; namespace System.Net.Http { public partial class HttpClientHandler : HttpMessageHandler { + private const string RequestMessageLookupKey = "System.Net.Http.HttpRequestMessage"; + private const string SavedExceptionDispatchInfoLookupKey = "System.Runtime.ExceptionServices.ExceptionDispatchInfo"; private const string ClientAuthenticationOID = "1.3.6.1.5.5.7.3.2"; + private static Oid s_serverAuthOid = new Oid("1.3.6.1.5.5.7.3.1", "1.3.6.1.5.5.7.3.1"); private static readonly Lazy s_RTCookieUsageBehaviorSupported = new Lazy(InitRTCookieUsageBehaviorSupported); private static bool RTCookieUsageBehaviorSupported => s_RTCookieUsageBehaviorSupported.Value; private static readonly Lazy s_RTNoCacheSupported = new Lazy(InitRTNoCacheSupported); private static bool RTNoCacheSupported => s_RTNoCacheSupported.Value; + private static readonly Lazy s_RTServerCustomValidationRequestedSupported = + new Lazy(InitRTServerCustomValidationRequestedSupported); + private static bool RTServerCustomValidationRequestedSupported => s_RTServerCustomValidationRequestedSupported.Value; #region Fields @@ -55,6 +67,7 @@ public partial class HttpClientHandler : HttpMessageHandler private IWebProxy _proxy; private X509Certificate2Collection _clientCertificates; private IDictionary _properties; // Only create dictionary when required. + private Func _serverCertificateCustomValidationCallback; #endregion Fields @@ -291,18 +304,14 @@ public X509CertificateCollection ClientCertificates public Func ServerCertificateCustomValidationCallback { - // TODO: Not yet implemented. Issue #7623. - get{ return null; } + get + { + return _serverCertificateCustomValidationCallback; + } set { CheckDisposedOrStarted(); - if (value != null) - { - /* - throw new PlatformNotSupportedException(String.Format(CultureInfo.InvariantCulture, - SR.net_http_value_not_supported, value, nameof(ServerCertificateCustomValidationCallback))); - */ - } + _serverCertificateCustomValidationCallback = value; } } @@ -349,6 +358,8 @@ public HttpClientHandler() { _rtFilter = new RTHttpBaseProtocolFilter(); _handlerToFilter = new HttpHandlerToFilter(_rtFilter); + _handlerToFilter.RequestMessageLookupKey = RequestMessageLookupKey; + _handlerToFilter.SavedExceptionDispatchInfoLookupKey = SavedExceptionDispatchInfoLookupKey; _diagnosticsPipeline = new DiagnosticsHandler(_handlerToFilter); _clientCertificateOptions = ClientCertificateOption.Manual; @@ -573,7 +584,7 @@ protected internal override async Task SendAsync(HttpReques } catch (Exception ex) { - // Convert back to the expected exception type + // Convert back to the expected exception type. throw new HttpRequestException(SR.net_http_client_execution_error, ex); } @@ -645,10 +656,35 @@ private void SetOperationStarted() { if (!_operationStarted) { - // Since this is the first operation, we set the proxy and server credentials on the - // WinRT filter based on the .NET handler's property settings. + // Since this is the first operation, we set all the necessary WinRT filter properties. SetFilterProxyCredential(); SetFilterServerCredential(); + if (RTServerCustomValidationRequestedSupported && _serverCertificateCustomValidationCallback != null) + { + // The WinRT layer uses a different model for the certificate callback. The callback is + // considered "extra" validation. We need to explicitly ignore errors so that the callback + // will get called. + // + // In addition, the WinRT layer restricts some errors so that they cannot be ignored, such + // as "Revoked". This will result in behavior differences between UWP and other platforms. + // The following errors cannot be ignored right now in the WinRT layer: + // + // ChainValidationResult.BasicConstraintsError + // ChainValidationResult.InvalidCertificateAuthorityPolicy + // ChainValidationResult.InvalidSignature + // ChainValidationResult.OtherErrors + // ChainValidationResult.Revoked + // ChainValidationResult.UnknownCriticalExtension + _rtFilter.IgnorableServerCertificateErrors.Add(RTChainValidationResult.Expired); + _rtFilter.IgnorableServerCertificateErrors.Add(RTChainValidationResult.IncompleteChain); + _rtFilter.IgnorableServerCertificateErrors.Add(RTChainValidationResult.InvalidName); + _rtFilter.IgnorableServerCertificateErrors.Add(RTChainValidationResult.RevocationFailure); + _rtFilter.IgnorableServerCertificateErrors.Add(RTChainValidationResult.RevocationInformationMissing); + _rtFilter.IgnorableServerCertificateErrors.Add(RTChainValidationResult.Untrusted); + _rtFilter.IgnorableServerCertificateErrors.Add(RTChainValidationResult.WrongUsage); + _rtFilter.ServerCustomValidationRequested += RTServerCertificateCallback; + } + _operationStarted = true; } } @@ -684,6 +720,81 @@ private static bool InitRTNoCacheSupported() "NoCache"); } + private static bool InitRTServerCustomValidationRequestedSupported() + { + return RTApiInformation.IsEventPresent( + "Windows.Web.Http.Filters.HttpBaseProtocolFilter", + "ServerCustomValidationRequested"); + } + + private void RTServerCertificateCallback(RTHttpBaseProtocolFilter sender, RTHttpServerCustomValidationRequestedEventArgs args) + { + // Convert WinRT certificate to .NET certificate. + X509Certificate2 serverCert = ConvertPublicKeyCertificate(args.ServerCertificate); + + // Create .NET X509Chain from the WinRT information. We need to rebuild the chain since WinRT only + // gives us an array of intermediate certificates and not a X509Chain object. + var serverChain = new X509Chain(); + SslPolicyErrors sslPolicyErrors = SslPolicyErrors.None; + foreach (RTCertificate cert in args.ServerIntermediateCertificates) + { + serverChain.ChainPolicy.ExtraStore.Add(ConvertPublicKeyCertificate(cert)); + } + serverChain.ChainPolicy.RevocationMode = X509RevocationMode.Online; // WinRT always checks revocation. + serverChain.ChainPolicy.RevocationFlag = X509RevocationFlag.ExcludeRoot; + // Authenticate the remote party: (e.g. when operating in client mode, authenticate the server). + serverChain.ChainPolicy.ApplicationPolicy.Add(s_serverAuthOid); + if (!serverChain.Build(serverCert)) + { + sslPolicyErrors |= SslPolicyErrors.RemoteCertificateChainErrors; + } + + // Determine name-mismatch error from the existing WinRT information since .NET X509Chain.Build does not + // return that in the X509Chain.ChainStatus fields. + foreach (RTChainValidationResult result in args.ServerCertificateErrors) + { + if (result == RTChainValidationResult.InvalidName) + { + sslPolicyErrors |= SslPolicyErrors.RemoteCertificateNameMismatch; + break; + } + } + + // Get the .NET HttpRequestMessage we saved in the property bag of the WinRT HttpRequestMessage. + HttpRequestMessage request = (HttpRequestMessage)args.RequestMessage.Properties[RequestMessageLookupKey]; + + // Call the .NET callback. + bool success = false; + try + { + success = _serverCertificateCustomValidationCallback(request, serverCert, serverChain, sslPolicyErrors); + } + catch (Exception ex) + { + // Save the exception info. We will return it later via the SendAsync response processing. + args.RequestMessage.Properties.Add( + SavedExceptionDispatchInfoLookupKey, + ExceptionDispatchInfo.Capture(ex)); + } + finally + { + serverChain.Dispose(); + serverCert.Dispose(); + } + + if (!success) + { + args.Reject(); + } + } + + private X509Certificate2 ConvertPublicKeyCertificate(RTCertificate cert) + { + // Convert Windows X509v2 cert to .NET X509v2 cert. + RTIBuffer blob = cert.GetCertificateBlob(); + return new X509Certificate2(blob.ToArray()); + } + #endregion Helpers } } diff --git a/src/System.Net.Http/src/netcore50/System/Net/HttpHandlerToFilter.cs b/src/System.Net.Http/src/uap/System/Net/HttpHandlerToFilter.cs similarity index 92% rename from src/System.Net.Http/src/netcore50/System/Net/HttpHandlerToFilter.cs rename to src/System.Net.Http/src/uap/System/Net/HttpHandlerToFilter.cs index 322f0227e10d..dc75083cd60e 100644 --- a/src/System.Net.Http/src/netcore50/System/Net/HttpHandlerToFilter.cs +++ b/src/System.Net.Http/src/uap/System/Net/HttpHandlerToFilter.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.IO; +using System.Runtime.ExceptionServices; using System.Runtime.InteropServices.WindowsRuntime; using System.Threading; using System.Threading.Tasks; @@ -39,6 +40,9 @@ internal HttpHandlerToFilter(RTHttpBaseProtocolFilter filter) _filterMaxVersionSet = 0; } + internal string RequestMessageLookupKey { get; set; } + internal string SavedExceptionDispatchInfoLookupKey { get; set; } + protected internal override async Task SendAsync(HttpRequestMessage request, CancellationToken cancel) { if (request == null) @@ -47,9 +51,27 @@ protected internal override async Task SendAsync(HttpReques } cancel.ThrowIfCancellationRequested(); - RTHttpRequestMessage rtRequest = await ConvertRequestAsync(request).ConfigureAwait(false); - RTHttpResponseMessage rtResponse = await _next.SendRequestAsync(rtRequest).AsTask(cancel).ConfigureAwait(false); + + RTHttpResponseMessage rtResponse; + try + { + rtResponse = await _next.SendRequestAsync(rtRequest).AsTask(cancel).ConfigureAwait(false); + } + catch (TaskCanceledException) + { + throw; + } + catch (Exception) + { + object info; + if (rtRequest.Properties.TryGetValue(SavedExceptionDispatchInfoLookupKey, out info)) + { + ((ExceptionDispatchInfo)info).Throw(); + } + + throw; + } // Update in case of redirects request.RequestUri = rtRequest.RequestUri; @@ -64,6 +86,9 @@ private async Task ConvertRequestAsync(HttpRequestMessage { RTHttpRequestMessage rtRequest = new RTHttpRequestMessage(new RTHttpMethod(request.Method.Method), request.RequestUri); + // Add a reference from the WinRT object back to the .NET object. + rtRequest.Properties.Add(RequestMessageLookupKey, request); + // We can only control the Version on the first request message since the WinRT API // has this property designed as a filter/handler property. In addition the overall design // of HTTP/2.0 is such that once the first request is using it, all the other requests diff --git a/src/System.Net.Http/src/netcore50/System/Net/cookie.cs b/src/System.Net.Http/src/uap/System/Net/cookie.cs similarity index 100% rename from src/System.Net.Http/src/netcore50/System/Net/cookie.cs rename to src/System.Net.Http/src/uap/System/Net/cookie.cs diff --git a/src/System.Net.Http/src/netcore50/System/Net/cookieexception.cs b/src/System.Net.Http/src/uap/System/Net/cookieexception.cs similarity index 100% rename from src/System.Net.Http/src/netcore50/System/Net/cookieexception.cs rename to src/System.Net.Http/src/uap/System/Net/cookieexception.cs diff --git a/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.AcceptAllCerts.cs b/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.AcceptAllCerts.cs index 69ce6fff3708..783e24901b7d 100644 --- a/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.AcceptAllCerts.cs +++ b/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.AcceptAllCerts.cs @@ -15,6 +15,9 @@ namespace System.Net.Http.Functional.Tests public class HttpClientHandler_DangerousAcceptAllCertificatesValidator_Test { + // TODO: https://github.com/dotnet/corefx/issues/7812 + private static bool ClientSupportsDHECipherSuites => (!PlatformDetection.IsWindows || PlatformDetection.IsWindows10Version1607OrGreater); + [Fact] public void SingletonReturnsTrue() { @@ -57,9 +60,8 @@ await TestHelper.WhenAllCompletedOrAnyFailed( new object[] { Configuration.Http.WrongHostNameCertRemoteServer }, }; - [ActiveIssue(7812, TestPlatforms.Windows)] [OuterLoop] // TODO: Issue #11345 - [Theory] + [ConditionalTheory(nameof(ClientSupportsDHECipherSuites))] [MemberData(nameof(InvalidCertificateServers))] public async Task InvalidCertificateServers_CertificateValidationDisabled_Succeeds(string url) { diff --git a/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs b/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs index 2428d74ce7f9..7091271f51f0 100644 --- a/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs +++ b/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs @@ -14,19 +14,43 @@ namespace System.Net.Http.Functional.Tests { using Configuration = System.Net.Test.Common.Configuration; - [SkipOnTargetFramework(TargetFrameworkMonikers.Uap | TargetFrameworkMonikers.NetFramework, "uap: dotnet/corefx #20010, netfx: dotnet/corefx #16805")] + [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, ".NET Framework throws PNSE for ServerCertificateCustomValidationCallback")] public partial class HttpClientHandler_ServerCertificates_Test { - [OuterLoop] // TODO: Issue #11345 + // TODO: https://github.com/dotnet/corefx/issues/7812 + private static bool ClientSupportsDHECipherSuites => (!PlatformDetection.IsWindows || PlatformDetection.IsWindows10Version1607OrGreater); + private static bool BackendSupportsCustomCertificateHandlingAndClientSupportsDHECipherSuites => + (BackendSupportsCustomCertificateHandling && ClientSupportsDHECipherSuites); + [Fact] - public async Task NoCallback_ValidCertificate_CallbackNotCalled() + [SkipOnTargetFramework(~TargetFrameworkMonikers.Uap)] + public void Ctor_ExpectedDefaultPropertyValues_UapPlatform() { - var handler = new HttpClientHandler(); - using (var client = new HttpClient(handler)) + using (var handler = new HttpClientHandler()) + { + Assert.Null(handler.ServerCertificateCustomValidationCallback); + Assert.True(handler.CheckCertificateRevocationList); + } + } + + [Fact] + [SkipOnTargetFramework(TargetFrameworkMonikers.Uap)] + public void Ctor_ExpectedDefaultValues_NotUapPlatform() + { + using (var handler = new HttpClientHandler()) { Assert.Null(handler.ServerCertificateCustomValidationCallback); Assert.False(handler.CheckCertificateRevocationList); + } + } + [OuterLoop] // TODO: Issue #11345 + [Fact] + public async Task NoCallback_ValidCertificate_SuccessAndExpectedPropertyBehavior() + { + var handler = new HttpClientHandler(); + using (var client = new HttpClient(handler)) + { using (HttpResponseMessage response = await client.GetAsync(Configuration.Http.SecureRemoteEchoServer)) { Assert.Equal(HttpStatusCode.OK, response.StatusCode); @@ -37,6 +61,7 @@ public async Task NoCallback_ValidCertificate_CallbackNotCalled() } } + [ActiveIssue(20010, TargetFrameworkMonikers.Uap)] [OuterLoop] // TODO: Issue #11345 [ConditionalFact(nameof(BackendSupportsCustomCertificateHandling))] public async Task UseCallback_HaveNoCredsAndUseAuthenticatedCustomProxyAndPostToSecureServer_ProxyAuthenticationRequiredStatusCode() @@ -131,7 +156,13 @@ public async Task UseCallback_ValidCertificate_ExpectedValuesDuringCallback(Uri Assert.Equal(SslPolicyErrors.None, errors); Assert.True(chain.ChainElements.Count > 0); Assert.NotEmpty(cert.Subject); - Assert.Equal(checkRevocation ? X509RevocationMode.Online : X509RevocationMode.NoCheck, chain.ChainPolicy.RevocationMode); + + // UWP always uses CheckCertificateRevocationList=true regardless of setting the property and + // the getter always returns true. So, for this next Assert, it is better to get the property + // value back from the handler instead of using the parameter value of the test. + Assert.Equal( + handler.CheckCertificateRevocationList ? X509RevocationMode.Online : X509RevocationMode.NoCheck, + chain.ChainPolicy.RevocationMode); return true; }; @@ -162,14 +193,14 @@ public async Task UseCallback_CallbackReturnsFailure_ThrowsException() } } - [ActiveIssue(21904)] + [ActiveIssue(21904, ~TargetFrameworkMonikers.Uap)] [OuterLoop] // TODO: Issue #11345 - [Fact] - public async Task UseCallback_CallbackThrowsException_ExceptionPropagates() + [ConditionalFact(nameof(BackendSupportsCustomCertificateHandling))] + public async Task UseCallback_CallbackThrowsException_ExceptionPropagatesAsInnerException() { if (BackendDoesNotSupportCustomCertificateHandling) // can't use [Conditional*] right now as it's evaluated at the wrong time for the managed handler { - Console.WriteLine($"Skipping {nameof(UseCallback_CallbackThrowsException_ExceptionPropagates)}()"); + Console.WriteLine($"Skipping {nameof(UseCallback_CallbackThrowsException_ExceptionPropagatesAsInnerException)}()"); return; } @@ -178,7 +209,9 @@ public async Task UseCallback_CallbackThrowsException_ExceptionPropagates() { var e = new DivideByZeroException(); handler.ServerCertificateCustomValidationCallback = delegate { throw e; }; - Assert.Same(e, await Assert.ThrowsAsync(() => client.GetAsync(Configuration.Http.SecureRemoteEchoServer))); + + HttpRequestException ex = await Assert.ThrowsAsync(() => client.GetAsync(Configuration.Http.SecureRemoteEchoServer)); + Assert.Same(e, ex.InnerException); } } @@ -189,9 +222,8 @@ public async Task UseCallback_CallbackThrowsException_ExceptionPropagates() new object[] { Configuration.Http.WrongHostNameCertRemoteServer }, }; - [ActiveIssue(7812, TestPlatforms.Windows)] [OuterLoop] // TODO: Issue #11345 - [Theory] + [ConditionalTheory(nameof(ClientSupportsDHECipherSuites))] [MemberData(nameof(CertificateValidationServers))] public async Task NoCallback_BadCertificate_ThrowsException(string url) { @@ -201,8 +233,9 @@ public async Task NoCallback_BadCertificate_ThrowsException(string url) } } + [SkipOnTargetFramework(TargetFrameworkMonikers.Uap, "UAP doesn't allow revocation checking to be turned off")] [OuterLoop] // TODO: Issue #11345 - [Fact] + [ConditionalFact(nameof(ClientSupportsDHECipherSuites))] public async Task NoCallback_RevokedCertificate_NoRevocationChecking_Succeeds() { // On macOS (libcurl+darwinssl) we cannot turn revocation off. @@ -247,9 +280,9 @@ public async Task NoCallback_RevokedCertificate_RevocationChecking_Fails() new object[] { Configuration.Http.WrongHostNameCertRemoteServer , SslPolicyErrors.RemoteCertificateNameMismatch}, }; - [ActiveIssue(7812, TestPlatforms.Windows)] + [ActiveIssue(21945, TargetFrameworkMonikers.Uap)] [OuterLoop] // TODO: Issue #11345 - [Theory] + [ConditionalTheory(nameof(BackendSupportsCustomCertificateHandlingAndClientSupportsDHECipherSuites))] [MemberData(nameof(CertificateValidationServersAndExpectedPolicies))] public async Task UseCallback_BadCertificate_ExpectedPolicyErrors(string url, SslPolicyErrors expectedErrors) { @@ -321,6 +354,7 @@ public async Task SSLBackendNotSupported_Revocation_ThrowsPlatformNotSupportedEx } } + [SkipOnTargetFramework(TargetFrameworkMonikers.Uap, "UAP doesn't expose channel binding information")] [OuterLoop] // TODO: Issue #11345 [PlatformSpecific(TestPlatforms.Windows)] // CopyToAsync(Stream, TransportContext) isn't used on unix [Fact] From e3657eea4cb3a9df2decd205bdec26c3d3e6a073 Mon Sep 17 00:00:00 2001 From: David Shulman Date: Thu, 6 Jul 2017 15:17:41 -0700 Subject: [PATCH 2/2] Address more PR feedback --- .../src/Resources/Strings.resx | 3 + .../src/uap/System/Net/HttpClientHandler.cs | 47 ++++++++++--- ...ttpClientHandlerTest.ServerCertificates.cs | 66 +++++++++++-------- 3 files changed, 77 insertions(+), 39 deletions(-) diff --git a/src/System.Net.Http/src/Resources/Strings.resx b/src/System.Net.Http/src/Resources/Strings.resx index 39e9575ced0d..1497d0210f46 100644 --- a/src/System.Net.Http/src/Resources/Strings.resx +++ b/src/System.Net.Http/src/Resources/Strings.resx @@ -298,4 +298,7 @@ The handler does not support changing revocation settings with this combination of libcurl ({0}) and its SSL backend ("{1}"). + + Using this feature requires Windows 10 Version 1607. + diff --git a/src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs b/src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs index fa2e5b6cd4f8..dd9a1bec257b 100644 --- a/src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs +++ b/src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs @@ -23,6 +23,7 @@ using RTHttpCacheReadBehavior = Windows.Web.Http.Filters.HttpCacheReadBehavior; using RTHttpCacheWriteBehavior = Windows.Web.Http.Filters.HttpCacheWriteBehavior; using RTHttpCookieUsageBehavior = Windows.Web.Http.Filters.HttpCookieUsageBehavior; +using RTHttpRequestMessage = Windows.Web.Http.HttpRequestMessage; using RTPasswordCredential = Windows.Security.Credentials.PasswordCredential; using RTCertificate = Windows.Security.Cryptography.Certificates.Certificate; using RTCertificateQuery = Windows.Security.Cryptography.Certificates.CertificateQuery; @@ -311,6 +312,15 @@ public Func intermediateCerts, + IReadOnlyList certErrors) { // Convert WinRT certificate to .NET certificate. - X509Certificate2 serverCert = ConvertPublicKeyCertificate(args.ServerCertificate); + X509Certificate2 serverCert = ConvertPublicKeyCertificate(cert); // Create .NET X509Chain from the WinRT information. We need to rebuild the chain since WinRT only // gives us an array of intermediate certificates and not a X509Chain object. var serverChain = new X509Chain(); SslPolicyErrors sslPolicyErrors = SslPolicyErrors.None; - foreach (RTCertificate cert in args.ServerIntermediateCertificates) + foreach (RTCertificate intermediateCert in intermediateCerts) { serverChain.ChainPolicy.ExtraStore.Add(ConvertPublicKeyCertificate(cert)); } @@ -751,7 +781,7 @@ private void RTServerCertificateCallback(RTHttpBaseProtocolFilter sender, RTHttp // Determine name-mismatch error from the existing WinRT information since .NET X509Chain.Build does not // return that in the X509Chain.ChainStatus fields. - foreach (RTChainValidationResult result in args.ServerCertificateErrors) + foreach (RTChainValidationResult result in certErrors) { if (result == RTChainValidationResult.InvalidName) { @@ -761,7 +791,7 @@ private void RTServerCertificateCallback(RTHttpBaseProtocolFilter sender, RTHttp } // Get the .NET HttpRequestMessage we saved in the property bag of the WinRT HttpRequestMessage. - HttpRequestMessage request = (HttpRequestMessage)args.RequestMessage.Properties[RequestMessageLookupKey]; + HttpRequestMessage request = (HttpRequestMessage)requestMessage.Properties[RequestMessageLookupKey]; // Call the .NET callback. bool success = false; @@ -772,7 +802,7 @@ private void RTServerCertificateCallback(RTHttpBaseProtocolFilter sender, RTHttp catch (Exception ex) { // Save the exception info. We will return it later via the SendAsync response processing. - args.RequestMessage.Properties.Add( + requestMessage.Properties.Add( SavedExceptionDispatchInfoLookupKey, ExceptionDispatchInfo.Capture(ex)); } @@ -782,10 +812,7 @@ private void RTServerCertificateCallback(RTHttpBaseProtocolFilter sender, RTHttp serverCert.Dispose(); } - if (!success) - { - args.Reject(); - } + return success; } private X509Certificate2 ConvertPublicKeyCertificate(RTCertificate cert) diff --git a/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs b/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs index 7091271f51f0..b102f2a392c0 100644 --- a/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs +++ b/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs @@ -354,7 +354,7 @@ public async Task SSLBackendNotSupported_Revocation_ThrowsPlatformNotSupportedEx } } - [SkipOnTargetFramework(TargetFrameworkMonikers.Uap, "UAP doesn't expose channel binding information")] + //[SkipOnTargetFramework(TargetFrameworkMonikers.Uap, "UAP doesn't expose channel binding information")] [OuterLoop] // TODO: Issue #11345 [PlatformSpecific(TestPlatforms.Windows)] // CopyToAsync(Stream, TransportContext) isn't used on unix [Fact] @@ -369,38 +369,46 @@ public async Task PostAsync_Post_ChannelBinding_ConfiguredCorrectly() // Validate the ChannelBinding object exists. ChannelBinding channelBinding = content.ChannelBinding; - Assert.NotNull(channelBinding); - - // Validate the ChannelBinding's validity. - if (BackendSupportsCustomCertificateHandling) + if (PlatformDetection.IsUap) { - Assert.False(channelBinding.IsInvalid, "Expected valid binding"); - Assert.NotEqual(IntPtr.Zero, channelBinding.DangerousGetHandle()); - - // Validate the ChannelBinding's description. - string channelBindingDescription = channelBinding.ToString(); - Assert.NotNull(channelBindingDescription); - Assert.NotEmpty(channelBindingDescription); - Assert.True((channelBindingDescription.Length + 1) % 3 == 0, $"Unexpected length {channelBindingDescription.Length}"); - for (int i = 0; i < channelBindingDescription.Length; i++) - { - char c = channelBindingDescription[i]; - if (i % 3 == 2) - { - Assert.Equal(' ', c); - } - else - { - Assert.True((c >= '0' && c <= '9') || (c >= 'A' && c <= 'F'), $"Expected hex, got {c}"); - } - } + // UAP currently doesn't expose channel binding information. + Assert.Null(channelBinding); } else { - // Backend doesn't support getting the details to create the CBT. - Assert.True(channelBinding.IsInvalid, "Expected invalid binding"); - Assert.Equal(IntPtr.Zero, channelBinding.DangerousGetHandle()); - Assert.Null(channelBinding.ToString()); + Assert.NotNull(channelBinding); + + // Validate the ChannelBinding's validity. + if (BackendSupportsCustomCertificateHandling) + { + Assert.False(channelBinding.IsInvalid, "Expected valid binding"); + Assert.NotEqual(IntPtr.Zero, channelBinding.DangerousGetHandle()); + + // Validate the ChannelBinding's description. + string channelBindingDescription = channelBinding.ToString(); + Assert.NotNull(channelBindingDescription); + Assert.NotEmpty(channelBindingDescription); + Assert.True((channelBindingDescription.Length + 1) % 3 == 0, $"Unexpected length {channelBindingDescription.Length}"); + for (int i = 0; i < channelBindingDescription.Length; i++) + { + char c = channelBindingDescription[i]; + if (i % 3 == 2) + { + Assert.Equal(' ', c); + } + else + { + Assert.True((c >= '0' && c <= '9') || (c >= 'A' && c <= 'F'), $"Expected hex, got {c}"); + } + } + } + else + { + // Backend doesn't support getting the details to create the CBT. + Assert.True(channelBinding.IsInvalid, "Expected invalid binding"); + Assert.Equal(IntPtr.Zero, channelBinding.DangerousGetHandle()); + Assert.Null(channelBinding.ToString()); + } } } }