From 639c190633c4d3f582c40a77dfad542b77bcf2c4 Mon Sep 17 00:00:00 2001 From: Sebastien Pouliot Date: Wed, 22 May 2019 10:27:26 -0400 Subject: [PATCH 1/3] [foundation] Add custom trust/certificate validation to NSUrlSessionHandler. Fix #4170 Basic application (size) for doing an `HttpClient.GetAsync`, release/llvm, 64bits only - NSUrlSessionHandler (master): 6.4 MB - NSUrlSessionHandler (PR#5936): 7.7 MB - NSUrlSessionHandler (this PR): 6.4 MB The size increase occurs because of the reference to .net `X509*` types. This brings a lot of additional code, including managed cryptographic code, inside the application - even when the feature is **not** used. The solution is to expose an API that only use native (OS) types, which are mostly already part of the application. This has a very low impact on existing applications. It's still possible to hook back to .NET validation if needed (it should not in most cases) but, in this case, the extra price will only be _paid_ if used (and can be lower if the code is needed by something else from the application). In comparison using other `HttpClient` handler produce app sizes of - HttpClientHandler (managed): 10.4 MB - CFNetworkHandler: 6.8 MB Based on/supersede https://github.com/xamarin/xamarin-macios/pull/5733 Fix https://github.com/xamarin/xamarin-macios/issues/4170 --- src/Foundation/NSUrlSessionHandler.cs | 35 +++++- .../System.Net.Http/MessageHandlers.cs | 107 ++++++++++++++++++ 2 files changed, 140 insertions(+), 2 deletions(-) diff --git a/src/Foundation/NSUrlSessionHandler.cs b/src/Foundation/NSUrlSessionHandler.cs index 3f01d420fa21..23b06c95da74 100644 --- a/src/Foundation/NSUrlSessionHandler.cs +++ b/src/Foundation/NSUrlSessionHandler.cs @@ -61,6 +61,8 @@ namespace System.Net.Http { namespace Foundation { #endif + public delegate bool NSUrlSessionHandlerTrustOverrideCallback (NSUrlSessionHandler sender, SecTrust trust); + // useful extensions for the class in order to set it in a header static class NSHttpCookieExtensions { @@ -291,6 +293,18 @@ public ICredentials Credentials { } } + NSUrlSessionHandlerTrustOverrideCallback trustOverride; + + public NSUrlSessionHandlerTrustOverrideCallback TrustOverride { + get { + return trustOverride; + } + set { + EnsureModifiability (); + trustOverride = value; + } + } + bool sentRequest; internal void EnsureModifiability () @@ -655,7 +669,7 @@ public override void DidCompleteWithError (NSUrlSession session, NSUrlSessionTas inflight.CancellationTokenSource.Cancel (); inflight.Errored = true; - var exc = createExceptionForNSError (error); + var exc = inflight.Exception ?? createExceptionForNSError (error); inflight.CompletionSource.TrySetException (exc); inflight.Stream.TrySetException (exc); } else { @@ -705,6 +719,22 @@ public override void DidReceiveChallenge (NSUrlSession session, NSUrlSessionTask if (inflight == null) return; + // ToCToU for the callback + var trustCallback = sessionHandler.TrustOverride; + if (trustCallback != null && challenge.ProtectionSpace.AuthenticationMethod == NSUrlProtectionSpace.AuthenticationMethodServerTrust) { + if (trustCallback (sessionHandler, challenge.ProtectionSpace.ServerSecTrust)) { + var credential = new NSUrlCredential (challenge.ProtectionSpace.ServerSecTrust); + completionHandler (NSUrlSessionAuthChallengeDisposition.UseCredential, credential); + } else { + // user callback rejected the certificate, we want to set the exception, else the user will + // see as if the request was cancelled. + lock (inflight.Lock) { + inflight.Exception = new HttpRequestException ("An error occurred while sending the request.", new WebException ("Error: TrustFailure")); + } + completionHandler (NSUrlSessionAuthChallengeDisposition.CancelAuthenticationChallenge, null); + } + return; + } // case for the basic auth failing up front. As per apple documentation: // The URL Loading System is designed to handle various aspects of the HTTP protocol for you. As a result, you should not modify the following headers using // the addValue(_:forHTTPHeaderField:) or setValue(_:forHTTPHeaderField:) methods: @@ -717,7 +747,7 @@ public override void DidReceiveChallenge (NSUrlSession session, NSUrlSessionTask // but we are hiding such a situation from our users, we can nevertheless know if the header was added and deal with it. The idea is as follows, // check if we are in the first attempt, if we are (PreviousFailureCount == 0), we check the headers of the request and if we do have the Auth // header, it means that we do not have the correct credentials, in any other case just do what it is expected. - + if (challenge.PreviousFailureCount == 0) { var authHeader = inflight.Request?.Headers?.Authorization; if (!(string.IsNullOrEmpty (authHeader?.Scheme) && string.IsNullOrEmpty (authHeader?.Parameter))) { @@ -783,6 +813,7 @@ class InflightData : IDisposable public HttpRequestMessage Request { get; set; } public HttpResponseMessage Response { get; set; } + public Exception Exception { get; set; } public bool ResponseSent { get; set; } public bool Errored { get; set; } public bool Disposed { get; set; } diff --git a/tests/monotouch-test/System.Net.Http/MessageHandlers.cs b/tests/monotouch-test/System.Net.Http/MessageHandlers.cs index 9f5929a4c591..b61418e78a8e 100644 --- a/tests/monotouch-test/System.Net.Http/MessageHandlers.cs +++ b/tests/monotouch-test/System.Net.Http/MessageHandlers.cs @@ -159,5 +159,112 @@ public void RedirectionWithAuthorizationHeaders (Type handlerType) Assert.IsNull (ex, $"Exception {ex} for {json}"); } } + +#if !__WATCHOS__ + [TestCase (typeof (HttpClientHandler))] +#endif + [TestCase (typeof (NSUrlSessionHandler))] + public void RejectSslCertificatesServicePointManager (Type handlerType) + { + TestRuntime.AssertSystemVersion (PlatformName.MacOSX, 10, 9, throwIfOtherPlatform: false); + TestRuntime.AssertSystemVersion (PlatformName.iOS, 7, 0, throwIfOtherPlatform: false); + + bool servicePointManagerCbWasExcuted = false; + bool done = false; + Exception ex = null; + + var handler = GetHandler (handlerType); + if (handler is NSUrlSessionHandler ns) { + ns.TrustOverride += (a,b) => { + servicePointManagerCbWasExcuted = true; + // return false, since we want to test that the exception is raised + return false; + }; + } else { + ServicePointManager.ServerCertificateValidationCallback = (sender, certificate, chain, errors) => { + servicePointManagerCbWasExcuted = true; + // return false, since we want to test that the exception is raised + return false; + }; + } + + TestRuntime.RunAsync (DateTime.Now.AddSeconds (30), async () => + { + try { + HttpClient client = new HttpClient (handler); + client.BaseAddress = new Uri ("https://httpbin.org"); + var byteArray = new UTF8Encoding ().GetBytes ("username:password"); + client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue ("Basic", Convert.ToBase64String(byteArray)); + var result = await client.GetAsync ("https://httpbin.org/redirect/3"); + } catch (Exception e) { + ex = e; + } finally { + done = true; + ServicePointManager.ServerCertificateValidationCallback = null; + } + }, () => done); + + if (!done) { // timeouts happen in the bost due to dns issues, connection issues etc.. we do not want to fail + Assert.Inconclusive ("Request timedout."); + } else { + // assert the exception type + Assert.IsInstanceOfType (typeof (HttpRequestException), ex); + Assert.IsNotNull (ex.InnerException); + Assert.IsInstanceOfType (typeof (WebException), ex.InnerException); + } + } + +#if !__WATCHOS__ + [TestCase (typeof (HttpClientHandler))] +#endif + [TestCase (typeof (NSUrlSessionHandler))] + public void AcceptSslCertificatesServicePointManager (Type handlerType) + { + TestRuntime.AssertSystemVersion (PlatformName.MacOSX, 10, 9, throwIfOtherPlatform: false); + TestRuntime.AssertSystemVersion (PlatformName.iOS, 7, 0, throwIfOtherPlatform: false); + + bool servicePointManagerCbWasExcuted = false; + bool done = false; + Exception ex = null; + + var handler = GetHandler (handlerType); + if (handler is NSUrlSessionHandler ns) { + ns.TrustOverride += (a,b) => { + servicePointManagerCbWasExcuted = true; + return true; + }; + } else { + ServicePointManager.ServerCertificateValidationCallback = (sender, certificate, chain, errors) => { + servicePointManagerCbWasExcuted = true; + return true; + }; + } + + TestRuntime.RunAsync (DateTime.Now.AddSeconds (30), async () => + { + try { + HttpClient client = new HttpClient (handler); + client.BaseAddress = new Uri ("https://httpbin.org"); + var byteArray = new UTF8Encoding ().GetBytes ("username:password"); + client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue ("Basic", Convert.ToBase64String(byteArray)); + var result = await client.GetAsync ("https://httpbin.org/redirect/3"); + } catch (Exception e) { + ex = e; + } finally { + done = true; + ServicePointManager.ServerCertificateValidationCallback = null; + } + }, () => done); + + if (!done) { // timeouts happen in the bost due to dns issues, connection issues etc.. we do not want to fail + Assert.Inconclusive ("Request timedout."); + } else { + // assert that we did not get an exception + if (ex != null && ex.InnerException != null) { + // we could get here.. if we have a diff issue, in that case, lets get the exception message and assert is not the trust issue + Assert.AreNotEqual (ex.InnerException.Message, "Error: TrustFailure"); + } + } + } } } From 0049a560c661284dbc8b537bb2754d49efeb6dc4 Mon Sep 17 00:00:00 2001 From: Sebastien Pouliot Date: Wed, 22 May 2019 11:08:13 -0500 Subject: [PATCH 2/3] fix indentation --- .../monotouch-test/System.Net.Http/MessageHandlers.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/monotouch-test/System.Net.Http/MessageHandlers.cs b/tests/monotouch-test/System.Net.Http/MessageHandlers.cs index b61418e78a8e..8eb2cf64c8ac 100644 --- a/tests/monotouch-test/System.Net.Http/MessageHandlers.cs +++ b/tests/monotouch-test/System.Net.Http/MessageHandlers.cs @@ -181,11 +181,11 @@ public void RejectSslCertificatesServicePointManager (Type handlerType) return false; }; } else { - ServicePointManager.ServerCertificateValidationCallback = (sender, certificate, chain, errors) => { - servicePointManagerCbWasExcuted = true; - // return false, since we want to test that the exception is raised - return false; - }; + ServicePointManager.ServerCertificateValidationCallback = (sender, certificate, chain, errors) => { + servicePointManagerCbWasExcuted = true; + // return false, since we want to test that the exception is raised + return false; + }; } TestRuntime.RunAsync (DateTime.Now.AddSeconds (30), async () => From 2074f76843982f386fc813a01dc0bb2fa98bd272 Mon Sep 17 00:00:00 2001 From: Sebastien Pouliot Date: Wed, 22 May 2019 19:34:55 -0400 Subject: [PATCH 3/3] [tests][intro] There's no binding in System.Net.Http namespace --- tests/introspection/ApiFrameworkTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/introspection/ApiFrameworkTest.cs b/tests/introspection/ApiFrameworkTest.cs index 902c8920eb7d..8b03f241a96c 100644 --- a/tests/introspection/ApiFrameworkTest.cs +++ b/tests/introspection/ApiFrameworkTest.cs @@ -78,10 +78,10 @@ public bool Skip (string @namespace) // not a framework, largely p/invokes to /usr/lib/libSystem.dylib case "Darwin": return true; +#endif // not directly bindings case "System.Net.Http": return true; -#endif default: return false; }