dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.25k stars 9.95k forks source link

Use IHttpClientFactory in RemoteAuthenticationOptions #10542

Open huan086 opened 5 years ago

huan086 commented 5 years ago

Is your feature request related to a problem? Please describe.

ASP.NET Core 2.1 introduced IHttpClientFactory to solve the problems with creating and disposing HttpClient with every call, and the problems with creating a long-lived HttpClient.

However, long-lived HttpClient is still created within the framework. Specifically, in RemoteAuthenticationOptions.Backchannel and RemoteAuthenticationOptions.BackchannelHttpHandler, these have effectively application lifetime because instances of AuthenticationOptions are cached in the singleton IOptionsMonitorCache.

Describe the solution you'd like

Replace RemoteAuthenticationOptions.Backchannel and RemoteAuthenticationOptions.BackchannelHttpHandler with RemoteAuthenticationOptions.BackchannelFactory, so that whenever an HttpClient is needed, it is created from the BackchannelFactory and disposed.

This would also require a new HttpDocumentRetriever constructor that takes an IHttpClientFactory, because JwtBearerPostConfigureOptions, WsFederationPostConfigureOptions and OpenIdConnectPostConfigureOptions initializes a long-lived ConfigurationManager.

Describe alternatives you've considered

Additional context

The Grand Auth Redesign of 2017 in Core 2.0 predates IHttpClientFactory in Core 2.1.

In Core 2.0, the new SocketsHttpHandler does not solve DNS problem

Exploring DNS issue

Tratcher commented 5 years ago

This would be easy enough for OAuth, but for the IdentityModel based providers (OIDC, WsFed) we'd need updated, refactored dependencies. @brentschmaltz

brentschmaltz commented 5 years ago

@Tratcher see: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1197

How do you prioritize this for this 3.0? Other users have asked for this.

Tratcher commented 5 years ago
brentschmaltz commented 5 years ago

@Tratcher OK, we will add this and make sure you know when it is available.

damianh commented 4 years ago

Also in JwtBearerOptions:

https://github.com/dotnet/aspnetcore/blob/ce0579255104f6ea1902b62c1e21195a7b93f828/src/Security/Authentication/JwtBearer/src/JwtBearerOptions.cs#L65

https://github.com/dotnet/aspnetcore/blob/62351067ff4c1401556725b401478e648b66acdc/src/Security/Authentication/JwtBearer/src/JwtBearerPostConfigureOptions.cs#L53

...and probably several other places.

Tratcher commented 4 years ago

It's the expectation that it would resolve the client per request or only at startup? Per request would be challenging for OIDC and JwtBearer due to their use of ConfigManager.

brentschmaltz commented 4 years ago

@Tratcher our focus is on a 6.4.2 public release. POR is weekly as we find issues. Once 6.4.2 is released, we can address this and other issues.

damianh commented 4 years ago

It's the expectation that it would resolve the client per request or only at startup? Per request would be challenging for OIDC and JwtBearer due to their use of ConfigManager.

Sorry, I'm not confident enough on this topic to make an assertion.

jeffreyrivor commented 4 years ago

.NET Core 2.1 introduced SocketsHttpHandler as the default handler, which is designed to be used in a long-lived singleton-like way and can be made to work. In the documentation for alternatives to IHttpClientFactory, it says that setting a finite SocketsHttpHandler.PooledConnectionLifetime solves the stale DNS problem of long-lived handlers while keeping the performance benefits of pooled resource management. The default value for SocketsHttpHandler.PooledConnectionLifetime is Timeout.InfiniteTimeSpan, so default construction of HttpClient or SocketsHttpHandler won't have the behavior and it has to be manually set.

This should allow us to use an application-lifetime (non-disposed) HttpClient for the Backchannel (or BackchannelHttpHandler) as a workaround.

services.AddAuthentication().AddOpenIdConnect(options =>
{
    // use the handler config
    options.BackchannelHttpHandler = new SocketsHttpHandler
    {
        // recycle pooled sockets every 5 minutes
        PooledConnectionLifetime = TimeSpan.FromMinutes(5),
    };

    // or use the client config
    options.Backchannel = new HttpClient(new SocketsHttpHandler
    {
        PooledConnectionLifetime = TimeSpan.FromMinutes(5),
    }, disposeHandler: false);
});

If you still want to use IHttpClientFactory (it's got really good delegating handler building overloads) and can work within the fact that the Backchannel will be application-lifetime, you could get fancy with using DI services to configure options.

// configure a named HttpClient for use as the backchannel, use SocketsHttpHandler
// configure delegating handler pipeline like Polly here as usual
services.AddHttpClient("backchannel")
    .ConfigurePrimaryHttpMessageHandler(() => new SocketsHttpHandler
    {
        PooledConnectionLifetime = TimeSpan.FromMinutes(5),
    });

// configure the options (match the authentication scheme name used in AddOpenIdConnect)
services.AddOptions<OpenIdConnectOptions>(OpenIdConnectDefaults.AuthenticationScheme)
    .Bind(Configuration.GetSection("AzureAD")) // similar trick to AADv2 tutorials for using settings from appsettings.json
    .Configure<IHttpClientFactory>((options, httpClientFactory) =>
    {
        // use the injected IHttpClientFactory to get the named client
        // note: this only happens once (the options are still bound in singleton fashion)
        options.Backchannel = httpClientFactory.CreateClient("backchannel");

        // configure other options here or in the normal AddOpenIdConnect options lambda
    });

// match the authentication scheme used for the options name
services.AddAuthentication()
    .AddOpenIdConnect(OpenIdConnectDefaults.AuthenticationScheme, options => { /* configure options here or above with backchannel config */ });