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

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

It's a quick proof of concept.

…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).

MihaZupan · 2025-01-28T17:10:37Z

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

@@ -20,6 +23,8 @@ internal static class WinHttpRequestCallback
        public static Interop.WinHttp.WINHTTP_STATUS_CALLBACK StaticCallbackDelegate =
            new Interop.WinHttp.WINHTTP_STATUS_CALLBACK(WinHttpCallback);

+        private static ConcurrentDictionary<IPAddress, string> s_cachedCertificates = new();


This should be specific to each WinHttpHandler instance as cert validation callbacks may be different (either an instance collection, or the handler being included as the key).

We should also be clearing old entries from this to avoid a memory leak.

NIT: Also consider to invalidate instance level cache if validation callback is changed. I know it is not recommended to change handler after it has been wired to HttpClient but if it is possible we might consider to handle it.

MihaZupan · 2025-01-28T17:13:55Z

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

+                    }
+                }
+
+                if (ipAddress is not null && s_cachedCertificates.TryGetValue(ipAddress, out string? thumbprint) && thumbprint == serverCertificate.Thumbprint)


Is it possible to obtain the thumbprint from the certHandle without allocating the cert?
If so, this could be moved up to avoid some more overhead.
Otherwise, this is also fine assuming that building the chain is the bulk of wasted compute.

and thumbprint use obsolete SHA-1. We should avoid it IMHO for new code. We may try to hash the whole cert content e.g. raw bytes.

I suppose we shouldn't need hashing here at all. We have the certs, we can just always do a SequenceEqual on the raw bytes.

MihaZupan · 2025-01-28T17:14:58Z

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

+                        ref infoSize))
+                    {
+                        ReadOnlySpan<byte> RemoteAddressSpan = new ReadOnlySpan<byte>(connectionInfo.RemoteAddress, 128);
+                        AddressFamily addressFamily = (AddressFamily)BitConverter.ToInt16(RemoteAddressSpan.ToArray(), 0);


Suggested change

AddressFamily addressFamily = (AddressFamily)BitConverter.ToInt16(RemoteAddressSpan.ToArray(), 0);

AddressFamily addressFamily = (AddressFamily)BitConverter.ToInt16(RemoteAddressSpan);

Oh I guess this is due to Framework? :/

In that case we can at least do .Slice(0, 2).ToArray() instead to avoid allocating the full 128 bytes

Can it be just Unsafe.ReadUnaligned<short>(ref MemoryMarshal.GetReference(remoteAddressSpan))? (Assuming we don't need to also check endianness here.)

Why not to use simple bit manipulation: RemoteAddressSpan[0] + (RemoteAddressSpan[0]<<8) normally I would recommend BinaryPrimitives.ReadInt16LittleEndian but I not sure we can afford package dependency here for framework.

The important thing is to avoid unnecessary allocations, it doesn't really matter how. (The code I quoted is BinaryPrimitives.ReadInt16LittleEndian's implementation.)

antonfirsov · 2025-01-28T16:39:00Z

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

+                        (IntPtr)pConnectionInfo,
+                        ref infoSize))
+                    {
+                        ReadOnlySpan<byte> RemoteAddressSpan = new ReadOnlySpan<byte>(connectionInfo.RemoteAddress, 128);


Local variables should be lower case: remoteAddressSpan

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?

antonfirsov · 2025-01-28T17:13:56Z

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

+                        ref infoSize))
+                    {
+                        ReadOnlySpan<byte> RemoteAddressSpan = new ReadOnlySpan<byte>(connectionInfo.RemoteAddress, 128);
+                        AddressFamily addressFamily = (AddressFamily)BitConverter.ToInt16(RemoteAddressSpan.ToArray(), 0);


We should avoid the ToArray() allocation. Shouldn't we deal with endiannes here too and use BinaryPrimitives?

antonfirsov · 2025-01-28T17:19:26Z

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

+                    else
+                    {


On what Windows versions is WINHTTP_OPTION_CONNECTION_INFO supported? Can we disable WinHttpQueryOption queries forever if it fails once?

Or run it against a fake handle to create a feature flag: https://github.com/dotnet/runtime/blob/f1901a00dfcfbcf0a4f8b6aa7459fec30846d1ab/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpTrailersHelper.cs

I don't think there is a windows version which doesn't support this feature and we support as .NET, this should be pretty safe.

WINHTTP_OPTION_CONNECTION_INFO says that this Applies to: Windows XP with SP2 and later; Windows 2003 with SP1 and later.

So we're safe here.

Might worth to add a comment.

antonfirsov · 2025-01-28T17:23:35Z

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

+                            AddressFamily.InterNetworkV6 => new IPAddress(RemoteAddressSpan.Slice(8, 16).ToArray()),
+                            _ => null
+                        };
+                        if (ipAddress != null && NetEventSource.Log.IsEnabled()) NetEventSource.Info(state, $"ipAddress: {ipAddress}");


Do we really expect non-IP address families here? Or can we just do Debug.Assert() for it and avoid further checks?

wfurt · 2025-01-28T18:02:36Z

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

@@ -56,6 +61,11 @@ private static void RequestCallback(
            {
                switch (internetStatus)
                {
+                    case Interop.WinHttp.WINHTTP_CALLBACK_STATUS_CONNECTED_TO_SERVER:
+                        IPAddress connectedToIPAddress = IPAddress.Parse(Marshal.PtrToStringUni(statusInformation)!);


This may be problematic in general as there can be many virtual servers and certificates on single IP address.

If you're talking about the same ip-port pair, yes, but we can extend this solution to cover sni-port too, the only thing we can do at that moment is, once we have a connection to this ip address, we still need to clear all cache for this ip address, because we can't extract more info other than ip address in this callback.

Is a (host, port) pair really sufficient to identify connections? In SocketsHttpHandler our connection key includes much more info.

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs

Lines 520 to 529 in bfa0276

internal readonly struct HttpConnectionKey : IEquatable<HttpConnectionKey>

{

public readonly HttpConnectionKind Kind;

public readonly string? Host;

public readonly int Port;

public readonly string? SslHostName; // null if not SSL

public readonly Uri? ProxyUri;

public readonly string Identity;

public HttpConnectionKey(HttpConnectionKind kind, string? host, int port, string? sslHostName, Uri? proxyUri, string identity)

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

rokonec · 2025-01-29T13:26:22Z

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

+                        ref infoSize))
+                    {
+                        ReadOnlySpan<byte> RemoteAddressSpan = new ReadOnlySpan<byte>(connectionInfo.RemoteAddress, 128);
+                        AddressFamily addressFamily = (AddressFamily)BitConverter.ToInt16(RemoteAddressSpan.ToArray(), 0);


Why not to use simple bit manipulation: RemoteAddressSpan[0] + (RemoteAddressSpan[0]<<8) normally I would recommend BinaryPrimitives.ReadInt16LittleEndian but I not sure we can afford package dependency here for framework.

rokonec · 2025-01-29T17:47:08Z

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

@@ -20,6 +23,8 @@ internal static class WinHttpRequestCallback
        public static Interop.WinHttp.WINHTTP_STATUS_CALLBACK StaticCallbackDelegate =
            new Interop.WinHttp.WINHTTP_STATUS_CALLBACK(WinHttpCallback);

+        private static ConcurrentDictionary<IPAddress, string> s_cachedCertificates = new();


NIT: Also consider to invalidate instance level cache if validation callback is changed. I know it is not recommended to change handler after it has been wired to HttpClient but if it is possible we might consider to handle it.

rokonec · 2025-01-29T17:57:57Z

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

+                    }
+                }
+
+                if (ipAddress is not null && s_cachedCertificates.TryGetValue(ipAddress, out string? thumbprint) && thumbprint == serverCertificate.Thumbprint)


If cache key stays as IPAddress and not some more specific unique connection hash/id, there is change that IP of virtual servers will cause huge degradation in perf. For example if give IP serve http://foo/ and http://bar/ client which will alternate between foo and bar it will always cross invalidate foo and bar caches. For example if in this setup every call will be like:

1) http://foo/ensure-connected 2) http://bar/resource/verb?op=x

It will always call validation callback.
I believe we shall seek for cache key which has better affinity to particular endpoint, for example hash(request_host+cert_thumbprint).

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

antonfirsov reviewed Jan 28, 2025

View reviewed changes

wfurt reviewed Jan 28, 2025

View reviewed changes

rokonec reviewed Jan 29, 2025

View reviewed changes

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

dotnet-policy-service bot commented Jan 24, 2025

liveans commented Jan 24, 2025

azure-pipelines bot commented Jan 24, 2025

MihaZupan Jan 28, 2025

rokonec Jan 29, 2025

MihaZupan Jan 28, 2025

wfurt Jan 28, 2025

MihaZupan Jan 29, 2025

MihaZupan Jan 28, 2025

MihaZupan Jan 28, 2025

liveans Jan 28, 2025

MihaZupan Jan 28, 2025 •

edited

Loading

antonfirsov Jan 28, 2025

rokonec Jan 29, 2025

antonfirsov Jan 29, 2025

antonfirsov Jan 28, 2025

antonfirsov Jan 28, 2025 •

edited

Loading

antonfirsov Jan 28, 2025

antonfirsov Jan 28, 2025

antonfirsov Jan 28, 2025

liveans Jan 29, 2025 •

edited

Loading

antonfirsov Jan 29, 2025

antonfirsov Jan 28, 2025 •

edited

Loading

wfurt Jan 28, 2025

liveans Jan 29, 2025

antonfirsov Jan 29, 2025

rokonec left a comment

rokonec Jan 29, 2025

rokonec Jan 29, 2025

rokonec Jan 29, 2025

	AddressFamily addressFamily = (AddressFamily)BitConverter.ToInt16(RemoteAddressSpan.ToArray(), 0);
	AddressFamily addressFamily = (AddressFamily)BitConverter.ToInt16(RemoteAddressSpan);

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

	internal readonly struct HttpConnectionKey : IEquatable<HttpConnectionKey>
	{
	public readonly HttpConnectionKind Kind;
	public readonly string? Host;
	public readonly int Port;
	public readonly string? SslHostName; // null if not SSL
	public readonly Uri? ProxyUri;
	public readonly string Identity;

	public HttpConnectionKey(HttpConnectionKind kind, string? host, int port, string? sslHostName, Uri? proxyUri, string identity)

[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

dotnet-policy-service bot commented Jan 24, 2025

liveans commented Jan 24, 2025

azure-pipelines bot commented Jan 24, 2025

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

MihaZupan Jan 28, 2025 • 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

antonfirsov Jan 28, 2025 • 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

liveans Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Jan 28, 2025 • 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

rokonec left a 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

MihaZupan Jan 28, 2025 •

edited

Loading

antonfirsov Jan 28, 2025 •

edited

Loading

liveans Jan 29, 2025 •

edited

Loading

antonfirsov Jan 28, 2025 •

edited

Loading