duosecurity / duo_universal_csharp

Duo OIDC-based two-factor authentication for .NET web applications
https://duo.com/docs/duoweb
BSD 3-Clause "New" or "Revised" License
19 stars 8 forks source link

Client does not follow Microsoft's guideance for HttpClient class #36

Open kevincathcart-cas opened 8 months ago

kevincathcart-cas commented 8 months ago

There are some unfortunate limitations that can occur with HttpClient when constructed in the default fashion. That class's defaults are not ideal for either usage model (HttpClient per request, or static/singleton) by default, especially for scenarios with meaningful load. Microsoft has substantial guidance about the use of this class because of these limitations.

This writeup briefly describes the two main issues talked about in Microsoft's guidance, and how they make may impact programs trying to use the Client class either usage model (instance per login request, or static/singleton instance). It does assume the reader has read Microsoft's guidance above.

From Duo's perspective this means potential problems for consumers. There are different problems for the one Client per login request model, and the single Static/Singleton instance.

Potential Issues with Client per Request

There is a limit to how many times you can close and reopen open a TCP connection to a given server in a any given time period (due to TCP's TIME_WAIT state, which lasts 4 minutes by default on Windows, and 60 seconds on Linux). So somebody uses an HttpClient per request under meaningful volume (like a dozen requests per second or so) they may hit problems. (Probably not a huge issue for most Duo Users, as a dozen logins per second continuously would be unlikely, but a brief period of many logins, like at the start of a work day could possibly be problematic).

However, the above is assuming you dispose of the HttpClient when you are done. Unfortunately, Client does not Implement IDisposable, so there is no way to dispose the HttpClient. If you use one per request while failing to dispose, well that also means that you are relying on the garbage collector to garbage collect the connection pool. (By default each HttpClient has its own connection pool, and connections are only closed after being idle for one minute). If you get a bunch of Signins without garbage collection running (or if these objects manage to get promoted to higher garbage collection tiers, which are collected less frequently) this could cause problems.

Microsoft's recommended way to fix this is using IHttpClientFactory, or (for .NET Core/5+ only) using a single static HttpClient instance with PooledConnectionLifetime set to some reasonable value to avoid the second problem.

Potential Issues when using a static/singleton instance of Client

On the other hand, having just a static/singleton HttpClient instance (which is what a static/singleton Client would do) can create issues with DNS changes assuming a enough traffic occurs that some pooled connections never reach their Idle timeout (2 minutes in .NET 5 or older, 1 minute in .NET 6+). While that is still a fair bit of login traffic, it is plausible for heavily used security sensitive apps (i.e. having short login session limits) at a larger enterprise.

There is no way to fix this for .NET Framework users while retaining a single HttpClient instance, so Microsoft's guidance for .NET Framework is to always use IHttpClientFactory. (You can mostly get away with singleton instances if DNS changes are not really a concern though).

zachrybaker commented 3 months ago

Duo, please respond