[WinHTTP] Certificate caching on WinHttpHandler to eliminate extra call to Custom Certificate Validation #111791

liveans · 2025-01-24T12:06:08Z

This PR goal is improving performance around CustomCertificateValidationCallback.

…om Certificate Validation

dotnet-policy-service · 2025-01-24T12:06:44Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

liveans · 2025-01-24T12:07:04Z

/azp run runtime-libraries-coreclr outerloop-windows

azure-pipelines · 2025-01-24T12:07:14Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs

antonfirsov · 2025-01-28T16:48:59Z

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs

+                            AddressFamily.InterNetwork => new IPAddress(BinaryPrimitives.ReadUInt32LittleEndian(RemoteAddressSpan.Slice(4))),
+                            AddressFamily.InterNetworkV6 => new IPAddress(RemoteAddressSpan.Slice(8, 16).ToArray()),


Since this is running per-request, can we try to avoid the IPAddress allocations (eg. by using a custom struct as a dictionary key), or if we think they are insignificant, run some benchmarks to prove it?

Do you really think it's worth it? Usage of IPAddress simplifies the written code.

Usage of IPAddress simplifies the written code

I'm not sure. Since the addresses are only used as keys in platform-specific logic, we don't even need to parse them or have any logic distinguishing InterNetwork and InterNetworkV6. It's enough to copy the relevant bytes of the span into a struct containing a buffer big enough for both IPv4 and IPv6.

We've discussed this in our offline chat, do you still believe we need to change this into some other structure @antonfirsov ?

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs

rokonec

This is very nice for Prove of concept, but there are few things which needs to be considered before we decide on feasibility of this approach:

decide if unbounded cache can be an memory leak issue (I have hard time to imagine use where it would)
consider different cache key to handle virtual servers infra

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs

…ficate_experiment

liveans · 2025-02-17T13:04:35Z

/azp run runtime-libraries-coreclr outerloop-windows

azure-pipelines · 2025-02-17T13:04:45Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs

src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs

src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs

src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs

Copilot

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs

Copilot · 2025-02-17T20:46:48Z

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs

@@ -56,6 +62,14 @@ private static void RequestCallback(
            {
                switch (internetStatus)
                {
+                    case Interop.WinHttp.WINHTTP_CALLBACK_STATUS_CONNECTED_TO_SERVER:
+                        if (CertificateCachingAppContextSwitchEnabled)
+                        {


Using the null-forgiving operator '!' assumes that Marshal.PtrToStringUni(statusInformation) is never null. Consider adding an explicit null check to prevent a potential exception if the pointer is unexpectedly null.

Suggested change

{

{

if (statusInformation == IntPtr.Zero)

{

throw new InvalidOperationException("statusInformation pointer is null.");

}

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs

wfurt · 2025-02-18T10:34:28Z

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs

+                            ipAddress = addressFamily switch
+                            {
+                                AddressFamily.InterNetwork => new IPAddress(BinaryPrimitives.ReadUInt32LittleEndian(remoteAddressSpan.Slice(4))),
+                                AddressFamily.InterNetworkV6 => new IPAddress(remoteAddressSpan.Slice(8, 16).ToArray()),


nit.
I think we should also try to get ScopeID for IPv6 to be 100% correct. LLA are rare but possible.

liveans · 2025-02-18T19:17:04Z

/azp run runtime-libraries-coreclr outerloop-windows

azure-pipelines · 2025-02-18T19:17:13Z

Azure Pipelines successfully started running 1 pipeline(s).

…ficate_experiment

liveans · 2025-02-18T19:18:27Z

/azp run runtime-libraries-coreclr outerloop-windows

azure-pipelines · 2025-02-18T19:18:39Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/CachedCertificateValue.cs

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs

src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs

liveans · 2025-02-20T17:52:16Z

/azp run runtime-libraries-coreclr outerloop-windows

azure-pipelines · 2025-02-20T17:52:27Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/System.Net.Http.WinHttpHandler/src/System.Net.Http.WinHttpHandler.csproj

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/CachedCertificateValue.cs

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs

src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs

MihaZupan

Assuming we get validation that this saves enough perf to justify the added complexity, this looks reasonable to me.

MihaZupan · 2025-02-20T19:27:34Z

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs

+                                AddressFamily.InterNetwork => new IPAddress(BinaryPrimitives.ReadUInt32LittleEndian(remoteAddressSpan.Slice(4))),
+                                AddressFamily.InterNetworkV6 => new IPAddress(remoteAddressSpan.Slice(8, 16).ToArray()),


It might be worth leaving a link to the relevant docs here for where these offset numbers are coming from.

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs

src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs

Co-authored-by: Miha Zupan <[email protected]>

…ficate_experiment

antonfirsov · 2025-02-24T17:28:27Z

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/CachedCertificateValue.cs

+        public override bool Equals(object? obj)
+        {
+            throw new Exception("Unreachable");
+        }


Is this needed?

antonfirsov · 2025-02-24T17:30:29Z

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs

+                    case Interop.WinHttp.WINHTTP_CALLBACK_STATUS_CONNECTED_TO_SERVER:
+                        if (WinHttpHandler.CertificateCachingAppContextSwitchEnabled)
+                        {
+                            IPAddress connectedToIPAddress = IPAddress.Parse(Marshal.PtrToStringUni(statusInformation)!);


Is it possible to use IPAddress.Parse(ReadOnlySpan<char>) here? Moreover: are we confident this will work well for both IPv4 and IPv6?

Certificate caching on WinHttpHandler to eliminate extra call to Cust…

aed71f0

…om Certificate Validation

dotnet-issue-labeler bot added the area-System.Net.Http label Jan 24, 2025

dotnet-policy-service bot assigned liveans Jan 24, 2025

build-analysis bot mentioned this pull request Jan 24, 2025

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts dotnet/dnceng#2496

Open

3 tasks

MihaZupan reviewed Jan 28, 2025

View reviewed changes

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs Outdated Show resolved Hide resolved

MihaZupan reviewed Jan 28, 2025

View reviewed changes

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs Outdated Show resolved Hide resolved

MihaZupan reviewed Jan 28, 2025

View reviewed changes

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs Outdated Show resolved Hide resolved

antonfirsov reviewed Jan 28, 2025

View reviewed changes

wfurt reviewed Jan 28, 2025

View reviewed changes

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs Outdated Show resolved Hide resolved

rokonec reviewed Jan 29, 2025

View reviewed changes

liveans added 3 commits February 17, 2025 12:35

Review feedback

f08a569

Merge branch 'main' into winhttp_servervalidationcallback_cache_certi…

80ee04c

…ficate_experiment

Review feedback

63de970

Framework compat + Review Feedback

00640e2

MihaZupan reviewed Feb 17, 2025

View reviewed changes

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs Outdated Show resolved Hide resolved

MihaZupan reviewed Feb 17, 2025

View reviewed changes

src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/WinHttpHandlerTest.cs Outdated Show resolved Hide resolved

Implement Timer to clear cache

4ed25ff

MihaZupan reviewed Feb 17, 2025

View reviewed changes

Review feedback

b15191d

liveans marked this pull request as ready for review February 17, 2025 19:22

MihaZupan reviewed Feb 17, 2025

View reviewed changes

MihaZupan requested a review from Copilot February 17, 2025 20:45

Copilot AI reviewed Feb 17, 2025

View reviewed changes

This was referenced Feb 18, 2025

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

[wasm][AOT] emcc : error - received SIGKILL (-9) #89402

Open

wfurt reviewed Feb 18, 2025

View reviewed changes

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs Outdated Show resolved Hide resolved

wfurt reviewed Feb 18, 2025

View reviewed changes

Review Feedback

736944d

Merge branch 'main' into winhttp_servervalidationcallback_cache_certi…

a8e05d0

…ficate_experiment

MihaZupan reviewed Feb 18, 2025

View reviewed changes

This was referenced Feb 18, 2025

Android emulator not booting completely on Helix queue dotnet/dnceng#1448

Open

System.Net.Http.Functional.Tests.HttpClientTest.GetStringAsync_CanBeCanceled CI failure #112304

Open

liveans added 2 commits February 20, 2025 18:40

Review feedback

548e6fd

Fix RemoteExecutor issue and change delay to ms

c365911

MihaZupan reviewed Feb 20, 2025

View reviewed changes

Review feedback

9e00762

MihaZupan approved these changes Feb 20, 2025

View reviewed changes

Apply suggestions from code review

7f8540b

Co-authored-by: Miha Zupan <[email protected]>

build-analysis bot mentioned this pull request Feb 21, 2025

[android] Android.Device_Emulator.JIT.Test failing on emulators with CoreCLR #112633

Open

Merge branch 'main' into winhttp_servervalidationcallback_cache_certi…

e0a9524

…ficate_experiment

build-analysis bot mentioned this pull request Feb 24, 2025

SslStreamNetworkStreamTest failures "The remote certificate is invalid because of errors in the certificate chain: PartialChain" #112856

Closed

antonfirsov reviewed Feb 24, 2025

View reviewed changes

liveans marked this pull request as draft February 24, 2025 22:00

karelz added this to the 10.0.0 milestone Feb 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WinHTTP] Certificate caching on WinHttpHandler to eliminate extra call to Custom Certificate Validation #111791

[WinHTTP] Certificate caching on WinHttpHandler to eliminate extra call to Custom Certificate Validation #111791

liveans commented Jan 24, 2025 •

edited

Loading

dotnet-policy-service bot commented Jan 24, 2025

liveans commented Jan 24, 2025

azure-pipelines bot commented Jan 24, 2025

antonfirsov Jan 28, 2025 •

edited

Loading

liveans Feb 17, 2025

antonfirsov Feb 17, 2025 •

edited

Loading

liveans Feb 20, 2025

rokonec left a comment

liveans commented Feb 17, 2025

azure-pipelines bot commented Feb 17, 2025

Copilot AI left a comment

Copilot AI Feb 17, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

wfurt Feb 18, 2025

liveans commented Feb 18, 2025

azure-pipelines bot commented Feb 18, 2025

liveans commented Feb 18, 2025

azure-pipelines bot commented Feb 18, 2025

liveans commented Feb 20, 2025

azure-pipelines bot commented Feb 20, 2025

MihaZupan left a comment

MihaZupan Feb 20, 2025

antonfirsov Feb 24, 2025

antonfirsov Feb 24, 2025

		AddressFamily.InterNetwork => new IPAddress(BinaryPrimitives.ReadUInt32LittleEndian(RemoteAddressSpan.Slice(4))),
		AddressFamily.InterNetworkV6 => new IPAddress(RemoteAddressSpan.Slice(8, 16).ToArray()),

-                        {
+                        {
+                            if (statusInformation == IntPtr.Zero)
+                            {
+                                throw new InvalidOperationException("statusInformation pointer is null.");
+                            }

[WinHTTP] Certificate caching on WinHttpHandler to eliminate extra call to Custom Certificate Validation #111791

Are you sure you want to change the base?

[WinHTTP] Certificate caching on WinHttpHandler to eliminate extra call to Custom Certificate Validation #111791

Conversation

liveans commented Jan 24, 2025 • edited Loading

dotnet-policy-service bot commented Jan 24, 2025

liveans commented Jan 24, 2025

azure-pipelines bot commented Jan 24, 2025

antonfirsov Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

liveans Feb 17, 2025

Choose a reason for hiding this comment

antonfirsov Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

liveans Feb 20, 2025

Choose a reason for hiding this comment

rokonec left a comment

Choose a reason for hiding this comment

liveans commented Feb 17, 2025

azure-pipelines bot commented Feb 17, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Feb 17, 2025

Choose a reason for hiding this comment

wfurt Feb 18, 2025

Choose a reason for hiding this comment

liveans commented Feb 18, 2025

azure-pipelines bot commented Feb 18, 2025

liveans commented Feb 18, 2025

azure-pipelines bot commented Feb 18, 2025

liveans commented Feb 20, 2025

azure-pipelines bot commented Feb 20, 2025

MihaZupan left a comment

Choose a reason for hiding this comment

MihaZupan Feb 20, 2025

Choose a reason for hiding this comment

antonfirsov Feb 24, 2025

Choose a reason for hiding this comment

antonfirsov Feb 24, 2025

Choose a reason for hiding this comment

liveans commented Jan 24, 2025 •

edited

Loading

antonfirsov Jan 28, 2025 •

edited

Loading

antonfirsov Feb 17, 2025 •

edited

Loading