Fix HttpClientHandler ServerCertificateCustomValidationCallback for UWP #21905

davidsh · 2017-07-05T22:17:40Z

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

stephentoub · 2017-07-05T22:23:23Z

src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs

+
+            // Create .NET X509Chain from the WinRT information. We need to rebuild
+            // the chain since WinRT only gives us an array of intermediate certificates.
+            var serverChain = new X509Chain();


Can/should we dispose of the serverCert, serverChain, etc. when done with them?

Probably. I was afraid to do it because I didn't remember the behavior contract in terms of .NET Framework. I.e., what happens if a user-provided callback wants to save those values for later inspection, i.e. cert-pinning scenarios?

I did notice that we dispose of them in WinHttpHandler.

finally { if (chain != null) { chain.Dispose(); } serverCertificate.Dispose(); }

But that might not be correct. We might need to stop disposing of them in WinHttpHandler and *Nix. Need to research what the behavior is on .NET Framework.

I checked .NET Framework. It does indeed dispose of the chain and server certificate after the callback is done. So, I will revise this code to do the same.

For reference, here is a summary of the .NET Framework code:
https://github.com/Microsoft/referencesource/blob/master/System/net/System/Net/_SecureChannel.cs#L1281

if (remoteCertValidationCallback != null) { success = remoteCertValidationCallback(m_HostName, remoteCertificateEx, chain, sslPolicyErrors); } // ... finally { // At least on Win2k server the chain is found to have dependencies on the original cert context. // So it should be closed first. if (chain != null) { chain.Reset(); } if (remoteCertificateEx != null) { remoteCertificateEx.Reset(); } }

@bartonjs What does the .Reset() do in X509Chain and X509Certificate2 How is compared to doing .Dispose()?

Dispose just calls Reset. Any X509Certificate2 objects extracted from chain.ChainElements are still valid, it just releases the native resources.

Fixed. I added .Dispose() calls.

stephentoub · 2017-07-05T22:25:02Z

src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs

+
+        private X509Certificate2 ConvertCertificate(RTCertificate cert)
+        {
+            // Conver Windows X509v2 cert to .NET X509v2 cert.


Nit: Conver => Convert

stephentoub · 2017-07-05T22:25:34Z

src/System.Net.Http/src/uap/System/Net/HttpHandlerToFilter.cs

+            {
+                rtResponse = await _next.SendRequestAsync(rtRequest).AsTask(cancel).ConfigureAwait(false);
+            }
+            catch (TaskCanceledException)


Just TaskCanceledException, or any OperationCanceledException?

I copied this pattern from here:
https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L100

If it's wrong there, we can fix both in later PR.

stephentoub · 2017-07-05T22:26:56Z

src/System.Net.Http/src/uap/System/Net/HttpHandlerToFilter.cs

+                Exception savedEx = (Exception)rtRequest.Properties[SavedExceptionLookupKey];
+                if (savedEx != null)
+                {
+                    throw savedEx;


Does something higher up the call stack wrap this? It'd be nice to maintain the original call stack if possible, so if we do need to throw it here, using ExceptionDispatchInfo.Throw(savedEx) instead, or capturing the ExceptionDispatchInfo at the original catch site and then throwing that here.

This is wrapped in an HttpRequestException here:
https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/netcore50/System/Net/HttpClientHandler.cs#L577

I'll look into seeing if the ExceptionDispatchInfo is useful here.

@stephentoub So, I could change the throw savedEx here to ExceptionDispatchInfo.Throw(savedEx).

But, since that exception is saved earlier in WinRTCertificateCallback, how can I save the exception w/ the right call stacks there and then later throw it here? I don't understand ExceptionDispatchInfo object.

Looking at the MSDN, it seems I should save an ExceptionDispatchInfo object obtained by doing ExceptionDispatchInfo.Capture(exception) instead of saving an Exception like I'm doing now. Then I can throw it later with savedExceptionDispatchInfo.Throw() ?

Fixed. Use ExceptionDispatchInfo

stephentoub · 2017-07-05T22:28:23Z

cc: @bartonjs to help review the RTServerCertificateCallback method

CIPop · 2017-07-05T22:43:22Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs

@@ -177,6 +201,7 @@ public static IEnumerable<object[]> UseCallback_ValidCertificate_ExpectedValuesD
            }
        }

+        [ActiveIssue(7812, TestPlatforms.Windows)]


The last comment in this bug is saying this is an OS problem. Can we just run the test for OS versions that have the fix?

Since a new OS which fixes the problem can be either client or server side, yes, I can re-enable the test on newer Windows client builds.

Fixed. I changed the ActiveIssue to a ConditionalFact/ConditionalTheory.

CIPop · 2017-07-05T22:46:34Z

src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs

+            {
+                serverChain.ChainPolicy.ExtraStore.Add(ConvertCertificate(cert));
+            }
+            serverChain.Build(serverCert);


Shouldn't building the chain be identical with the code in SslStream and WinHttpHandler (include OID checks, etc)?

CIPop · 2017-07-05T22:47:42Z

src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs

+            {
+                sslPolicyErrors |= SslPolicyErrors.RemoteCertificateChainErrors;
+            }
+            foreach (RTChainValidationResult result in args.ServerCertificateErrors)


Wouldn't this be enough to identify the chain errors? If no, is there any reason for not removing / augmenting this check with code we already have for other components (see above comment).

I'd like @bartonjs to review the "chain building" code here. I didn't use the code in SslStream and WinHttpHandler because that code also uses X509Chain.Build to build the chain. The only reason it then uses the low-level SSPI calls is to find out if the name mismatch occurs.

The current .NET X509Chain.Build doesn't take any DNS name to compare against. And that is why the low-level SSPI calls are necessary.

On the other hand, this information is already available in the WinRT ChainValidationResult. So, that means, I don't need to call the SSPI stuff since I already have the potential name-mismatch determination.

One thing that's missing is the OID check.

The chain building part here should match SslStream. Just like in SslStream the hostname error would come from a second source (in this case, the RTChainValidationResult value seems legit).

The chain building part here should match SslStream.

@bartonjs What does this mean exactly? Does it mean that using Chain.Build() as I am doing here sufficient? Or we need to call the low-level SSPI stuff as is doing in SslStream and WinHttpHandler. And by the way, that code also calls Chain.Build(). The only time it uses SSPI stuff is to figure out the "name mismatch" determination. And we already know that because the WinRT APIs return that information already (a feature missing in .NET X509Chain.Build because it doesn't take a DNS server name parameter).

So, what should I do regarding my PR and how I am "converting" from WinRT certificate information to .NET X509Chain?

ok. Thx. I'll update the chain.Build() calls to match the WinHttpHandler code to include OID check and RevocationMode flags.

Will you be able to unify the code instead of creating a copy?

This is an existing TODO, #2165

// TODO: Issue #2165. Merge with similar code used in System.Net.Security move to Common/src//System/Net. public static void BuildChain(

I will update the current issue to reflect that we have more places to consolidate.

Correction: It seems that issue is now closed. :(

I will open up a new issue and mark this code with it.

Actually, there is a new issue, #14542 for it. I will amend that issue with this information.

CIPop · 2017-07-05T22:50:54Z

src/System.Net.Http/src/uap/System/Net/HttpHandlerToFilter.cs

+            {
+                throw;
+            }
+            catch (Exception)


What if there are two separate kinds of Exception types (one saved and a completely different one here)?

First, I changed the "savedException" of type Exception to be a "savedExceptionDispatchInfo" of type ExceptionDispatchInfo as recommended by @stephentoub.

If there is a saved ExceptionDispatchInfo, we will use it because at the point it happened, the user callback (.NET callback) threw an unhandled exception. We need to report that. It also stops all further processing of the HTTP request, so there are no other exceptions we need to worry about.

When such an unhandled exception is thrown by the user provided .NET callback, our WinRTCertificateCallback wrapper traps that then calls Reject() on the WinRT callback layer. That causes the WinRT layer to throw an exception. That is really the exception we are trapping here in that case. And so, we need to see if we saved the ExceptionDispatchInfo object because that is really the root cause of why the WinRT layer threw an exception. And so, we need to propagate that.

CIPop · 2017-07-05T22:56:26Z

src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs

+                    // 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 


I couldn't find any documentation on this here or here.

From your first link:

Default OS validation of the server certificate is performed before raising this event. If the certificate fails this validation, the connection is terminated and your event handler is not called.

In order to skip parts of the OS validation (not recommended for production scenarios), use the IgnorableServerCertificateErrors property to specify the errors that you want to ignore. Then as long as the certificate does not have any other errors, the OS validation will be considered successful and your event handler will be called.

Or are you asking specifically about the ChainValidationResult values that the app is allowed to ignore?

I agree the documentation is not complete in all WinRT areas.

For example, trying to do this:

var filter = new Windows.Web.Http.Filters.HttpBaseProtocolFilter(); filter.IgnorableServerCertificateErrors.Add(ChainValidationResult.Revoked);

results in this exception:

System.ArgumentException: The parameter is incorrect.
'item': Provided value is not an ignorable ChainValidationResult value.

Diego-Perez-Botero · 2017-07-05T23:21:04Z

src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs

+                    //     ChainValidationResult.InvalidCertificateAuthorityPolicy
+                    //     ChainValidationResult.InvalidSignature
+                    //     ChainValidationResult.OtherErrors
+                    //     ChainValidationResult.Revoked


ChainValidationResult.Revoked can be ignored, but not ChainValidationResult.IncompleteChain.

Update: This was based on the Wininet flags that are currently available in the OS (see WINHTTP_OPTION_SECURITY_FLAGS in MSDN), but it turns out that WinRT has its own list of ignorable conditions.

Wouldn't it be more future-proof to just add ALL ChainValidationResult values to the IgnorableServerCertificateErrors, and just comment that some of them will not effectively be ignored?

@Diego-Perez-Botero See my comment above. Passing in ChainValidation.Revoked to the filter.IgnorableServerCertificateErrors.Add returns an error.

Wouldn't it be more future-proof to just add ALL ChainValidationResult values to the IgnorableServerCertificateErrors, and just comment that some of them will not effectively be ignored?

Yes, but passing in ones that can't be ignored throws exceptions from the Add() method. And I didn't want to add exception throwing noise all the time even if we catch it. It adds noise to debugging sessions.

Got it. I didn't know that exceptions were thrown for "unignorable" values :)

Diego-Perez-Botero · 2017-07-06T00:05:31Z

src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs

+            }
+        }
+
+        private X509Certificate2 ConvertCertificate(RTCertificate cert)


Consider renaming this to "ConvertPublicKeyCertificate" or something to that effect. The conversion being carried out here will drop any private keys from the WinRT certificate, which is fine for our needs (server's identity certificates don't contain private keys), but we don't want future developers to assume that this method's conversion approach can be used for all purposes.

Diego-Perez-Botero · 2017-07-06T02:18:45Z

src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs

+                    _rtFilter.IgnorableServerCertificateErrors.Add(RTChainValidationResult.RevocationInformationMissing);
+                    _rtFilter.IgnorableServerCertificateErrors.Add(RTChainValidationResult.Untrusted);
+                    _rtFilter.IgnorableServerCertificateErrors.Add(RTChainValidationResult.WrongUsage);
+                    _rtFilter.ServerCustomValidationRequested += RTServerCertificateCallback;


Doesn't this need to be done in "light up" fashion (checking that the custom validation callback API surface is available)? This property was added in 14393, so I'm not sure if "light up" logic is in order.

Good point. I added light-up for this. The fallback is to ignore any callback.

The fallback is to ignore any callback.

I think the fallback has to be to throw. Not all callbacks are ignoring errors, some of them are imposing restrictions. You can't let a connection transmit data if a custom callback didn't get to have its say.

You can't let a connection transmit data if a custom callback didn't get to have its say.

This is a good point. I think this demonstrates one example where we have to throw an exception at runtime if the light-up detection fails and we are trying to use that feature.

We can either throw an exception during the property setter of the callback. That would be a PNSE (PlatformNotSupportedException). Otherwise, if we validate everything during .SendAsync, we'll need to throw an HttpRequestException with perhaps a PNSE inside that.

I'm leaning toward throwing at the property setter since it is can be calculated right away that the feature won't work since we're running on a Windows 10 OS version before that callback was implemented in WinRT.

cc: @Diego-Perez-Botero @DavidGoll @CIPop

We can either throw an exception during the property setter or the callback

For reference, we have a similar issue with CurlHandler, and we throw the PNSE during the request/callback. While in some situations we could detect this based purely on inspecting the libcurl version/build being used, in general we don't know whether a callback is supported until we try to register it with libcurl and it gives us an answer about whether it could hook it up.

davidsh · 2017-07-06T20:00:25Z

I pushed a new commit which should address all feedback. Please PTAL. After approvals, I still need to rebase (to fix merge conflicts) and run final tests.

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

davidsh · 2017-07-06T21:08:17Z

@dotnet-bot Test Outerloop Windows x86 Release Build
@dotnet-bot Test Outerloop Linux x64 Release Build

davidsh · 2017-07-06T21:08:56Z

@dotnet-bot Test Outerloop UWP CoreCLR x64 Debug Build

bartonjs · 2017-07-06T21:24:38Z

src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs

+
+            if (!success)
+            {
+                args.Reject();


I don't like that the API requires call to Reject instead of one to Accept. Not yours, I know.

But to make sure that no early returns get added to this function to make it dangerous I think it would be useful to make the work be done in a helper method, then this is a simple

if (!HandleCustomCallback(args)) { args.Reject(); }

(It'd possibly be better if each piece that needed data from args was fed the data without args leaving this function, so it's clear that no one else alters it)

I don't like that the API requires call to Reject instead of one to Accept. Not yours, I know.

Actually, my team implemented that. :) We had long discussions about Accept vs. Reject. The architects concluded that since we were adding this callback and it was "extra" validation, the default behavior for an empty callback function was to accept. This was because one still has to manually turn on the flags to ignore errors. So, by default, adding a WinRT callback adds checks and doesn't allow errors to occur.

That aside, I will see about refactoring this code to make it more maintainable.

bartonjs

SslPolicyErrors building in custom callbacks looks good to me. But requesting a custom callback and not being able to service it needs to throw (fail closed) and I'd be happier with the refactoring to make early returns not break the security model (also fail closed behavior).

bartonjs · 2017-07-06T21:27:49Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.AcceptAllCerts.cs

        [OuterLoop] // TODO: Issue #11345
-        [Theory]
+        [ConditionalTheory(nameof(ClientSupportsDHECipherSuites))]


Is there a negative behavior that we should be validating? Like a kind of exception?

Please refer to #7812. This is to prevent CI failures. We don't want these tests running on Windows OS versions that don't have the fixes to the DHE handshake bugs. It is not a feature we want to test, per se.

bartonjs · 2017-07-06T21:29:36Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs

        [OuterLoop] // TODO: Issue #11345
-        [Theory]
+        [ConditionalTheory(nameof(ClientSupportsDHECipherSuites))]
        [MemberData(nameof(CertificateValidationServers))]
        public async Task NoCallback_BadCertificate_ThrowsException(string url)


Disabling this test seems weird. If no cipher suite was negotiated, wouldn't an exception be thrown? So wouldn't this test just work?

Please refer to #7812. This is to prevent CI failures. We don't want these tests running on Windows OS versions that don't have the fixes to the DHE handshake bugs. It is not a feature we want to test, per se.

bartonjs · 2017-07-06T21:29:59Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs

@@ -321,6 +354,7 @@ public static IEnumerable<object[]> UseCallback_ValidCertificate_ExpectedValuesD
            }
        }

+        [SkipOnTargetFramework(TargetFrameworkMonikers.Uap, "UAP doesn't expose channel binding information")]


So there should be a test that shows that it throws?

This is more about a missing feature in UAP. Channel binding information is used but the WinRT API doesn't expose it up-stack. Hence, we can't expose it in the .NET layer either. So, we simply don't run the test on UAP. If we were to write a test to "pin" this behavior, the test would simply be validating a null byte array for channel binding information is returned. Is that what you are suggesting I add?

Yep. I mean, the function does something. Whether that be return null, throw, give a fixed value of the integer digits of pi... whatever it does we should validate that it's doing it so that a) we know if it changes (may have doc impact, a bunch of tests that need to be enabled now, etc) and b) if a customer opens an issue it's much easier to say "by design" if we can point at an existing test.

davidsh · 2017-07-06T22:22:18Z

Based on @bartonjs feedback, I pushed another commit.

Key changes:

If callback feature is not supported and trying to be used, then throw PNSE at the property setter. I'm using a message like this: "Using this feature requires Windows 10 Version 1607". The "Version 1607" is the official way we describe "build 14393" a.k.a. "Windows 10 Anniversary Update".
Refactored WinRT callback method to make it more maintainable and safer from improper refactoring changes in the future.
Enabled channel binding test on UAP and validate it returns null information.

…WP (dotnet/corefx#21905) * 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 dotnet/corefx#21904 and revised the test as well. Fixes dotnet/corefx#21627 * Address more PR feedback Commit migrated from dotnet/corefx@e30fdc3

davidsh added area-System.Net.Http os-windows-uwp labels Jul 5, 2017

davidsh added this to the UWP6.0 milestone Jul 5, 2017

davidsh self-assigned this Jul 5, 2017

davidsh requested review from stephentoub, caesar-chen, CIPop, DavidGoll and Diego-Perez-Botero July 5, 2017 22:17

dnfclas added the cla-already-signed label Jul 5, 2017

stephentoub reviewed Jul 5, 2017

View reviewed changes

davidsh requested a review from bartonjs July 5, 2017 22:33

CIPop reviewed Jul 5, 2017

View reviewed changes

Diego-Perez-Botero reviewed Jul 5, 2017

View reviewed changes

Diego-Perez-Botero reviewed Jul 6, 2017

View reviewed changes

stephentoub mentioned this pull request Jul 6, 2017

Wrap CurlHandler cert validation callback exceptions in HttpRequestException #21938

Merged

CIPop approved these changes Jul 6, 2017

View reviewed changes

Diego-Perez-Botero approved these changes Jul 6, 2017

View reviewed changes

Diego-Perez-Botero mentioned this pull request Jul 6, 2017

ClientWebSocket supports client certificates in UWP #21908

Merged

bartonjs reviewed Jul 6, 2017

View reviewed changes

Address more PR feedback

e3657ee

davidsh merged commit e30fdc3 into dotnet:master Jul 7, 2017

davidsh deleted the sslcallback_uwp branch July 7, 2017 00:22

karelz modified the milestones: UWP6.0, 2.1.0 Jul 14, 2017

This was referenced Jan 31, 2020

User-provided SSL callback exceptions are rethrown incorrectly dotnet/runtime#22638

Closed

Interactions between different UAP HttpClient SSL tests causes failures dotnet/runtime#22657

Closed

Fix HttpClientHandler ServerCertificateCustomValidationCallback for UWP #21905

Fix HttpClientHandler ServerCertificateCustomValidationCallback for UWP #21905

Conversation

davidsh commented Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Diego-Perez-Botero Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Diego-Perez-Botero Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

davidsh Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Diego-Perez-Botero Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Diego-Perez-Botero Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh commented Jul 6, 2017

davidsh commented Jul 6, 2017

davidsh commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh commented Jul 5, 2017 •

edited

Loading

davidsh Jul 6, 2017 •

edited

Loading

davidsh Jul 5, 2017 •

edited

Loading

davidsh Jul 6, 2017 •

edited

Loading

davidsh Jul 6, 2017 •

edited

Loading

Diego-Perez-Botero Jul 5, 2017 •

edited

Loading

Diego-Perez-Botero Jul 5, 2017 •

edited

Loading

davidsh Jul 5, 2017 •

edited

Loading

Diego-Perez-Botero Jul 5, 2017 •

edited

Loading

davidsh Jul 5, 2017 •

edited

Loading

Diego-Perez-Botero Jul 6, 2017 •

edited

Loading

davidsh Jul 6, 2017 •

edited

Loading

davidsh Jul 6, 2017 •

edited

Loading

davidsh commented Jul 6, 2017 •

edited

Loading