dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.16k stars 4.72k forks source link

[API Proposal]: Provide async callback for Remote certification validation in HttpClient #79441

Open mddddb opened 1 year ago

mddddb commented 1 year ago

Background and motivation

It is currently possible to have a custom remote certificate validation for HttpClient using https://github.com/dotnet/runtime/blob/4cbe6f99d23e04c56a89251d49de1b0f14000427/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs#L261 and https://github.com/dotnet/runtime/blob/a5f3676cc71e176084f0f7f1f6beeecd86fbeafc/src/libraries/System.Net.Security/src/System/Net/Security/SslClientAuthenticationOptions.cs#L26 However, it is currently impossible to have async validation without sync-over-async, since the signature of the delegate is as below: public delegate bool RemoteCertificateValidationCallback(object sender, X509Certificate? certificate, X509Chain? chain, SslPolicyErrors sslPolicyErrors);

I.e.

services
    .AddHttpClient("someEndpointClient")
    .ConfigureHttpMessageHandlerBuilder(builder =>
    {
        var handler = (HttpClientHandler)builder.PrimaryHandler;
        handler.ServerCertificateCustomValidationCallback = (httpRequestMessage, certificate, chain, sslPolicyErrors) =>
        {
            Task<bool> someValidationTask = ...;
            return someValidationTask.GetAwaiter().GetResult(); // sync-over-async
        };
    });

Original discussion: https://github.com/dotnet/runtime/discussions/78761

API Proposal

namespace System.Net.Security;

public delegate ValueTask<bool> RemoteCertificateValidationAsyncCallback(object sender, X509Certificate? certificate, X509Chain? chain, SslPolicyErrors sslPolicyErrors);
namespace System.Net.Security

public class SslClientAuthenticationOptions
{
    ...

    public RemoteCertificateValidationAsyncCallback? RemoteCertificateValidationAsyncCallback { get; set; }
}
namespace System.Net.Http

public partial class HttpClientHandler : HttpMessageHandler
{
    ...

    public Func<HttpRequestMessage, X509Certificate2?, X509Chain?, SslPolicyErrors, ValueTask<bool>>? ServerCertificateCustomValidationAsyncCallback { get; set; }
}

API Usage

services
    .AddHttpClient("someEndpointClient")
    .ConfigureHttpMessageHandlerBuilder(builder =>
    {
        var handler = (HttpClientHandler)builder.PrimaryHandler;
        handler.ServerCertificateCustomValidationAsyncCallback = async (httpRequestMessage, certificate, chain, sslPolicyErrors) =>
        {
            Task<bool> someValidationTask = ...;
            return await someValidationTask.ConfigireAwait(false);
        };
    });

Alternative Designs

No response

Risks

Replacing the current sync callback is a breaking change. But introducing a new one and having 2 callbacks for the same reason is not a good solution either

ghost commented 1 year ago

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

Issue Details
### Background and motivation It is currently possible to have a custom remote certificate validation using [ServerCertificateCustomValidationCallback](https://github.com/dotnet/runtime/blob/4cbe6f99d23e04c56a89251d49de1b0f14000427/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs#L261) and [RemoteCertificateValidationCallback ](https://github.com/dotnet/runtime/blob/a5f3676cc71e176084f0f7f1f6beeecd86fbeafc/src/libraries/System.Net.Security/src/System/Net/Security/SslClientAuthenticationOptions.cs#L26). However, it is currently impossible to have async validation without sync-over-async, since the signature of the delegate is as below: `public delegate bool RemoteCertificateValidationCallback(object sender, X509Certificate? certificate, X509Chain? chain, SslPolicyErrors sslPolicyErrors);` I.e. ```c# services .AddHttpClient("someEndpointClient") .ConfigureHttpMessageHandlerBuilder(builder => { var handler = (HttpClientHandler)builder.PrimaryHandler; handler.ServerCertificateCustomValidationCallback = (httpRequestMessage, certificate, chain, sslPolicyErrors) => { Task someValidationTask = ...; return someValidationTask.GetAwaiter().GetResult(); // sync-over-async }; }); ``` Original discussion: https://github.com/dotnet/runtime/discussions/78761 ### API Proposal ```csharp namespace System.Net.Security; public delegate ValueTask RemoteCertificateValidationAsyncCallback(object sender, X509Certificate? certificate, X509Chain? chain, SslPolicyErrors sslPolicyErrors); ``` ```csharp namespace System.Net.Security public class SslClientAuthenticationOptions { ... public RemoteCertificateValidationAsyncCallback? RemoteCertificateValidationAsyncCallback { get; set; } } ``` ```csharp namespace System.Net.Http public partial class HttpClientHandler : HttpMessageHandler { ... public Func>? ServerCertificateCustomValidationAsyncCallback { get; set; } } ``` ### API Usage ```c# services .AddHttpClient("someEndpointClient") .ConfigureHttpMessageHandlerBuilder(builder => { var handler = (HttpClientHandler)builder.PrimaryHandler; handler.ServerCertificateCustomValidationAsyncCallback = async (httpRequestMessage, certificate, chain, sslPolicyErrors) => { Task someValidationTask = ...; return await someValidationTask.ConfigireAwait(false); // sync-over-async }; }); ``` ### Alternative Designs _No response_ ### Risks Replacing the current sync callback is a breaking change. But introducing a new one and having 2 callbacks for the same reason is not a good solution either
Author: mddddb
Assignees: -
Labels: `api-suggestion`, `area-System.Net.Http`, `untriaged`
Milestone: -
ghost commented 1 year ago

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

Issue Details
### Background and motivation It is currently possible to have a custom remote certificate validation for HttpClient using https://github.com/dotnet/runtime/blob/4cbe6f99d23e04c56a89251d49de1b0f14000427/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs#L261 and https://github.com/dotnet/runtime/blob/a5f3676cc71e176084f0f7f1f6beeecd86fbeafc/src/libraries/System.Net.Security/src/System/Net/Security/SslClientAuthenticationOptions.cs#L26 However, it is currently impossible to have async validation without sync-over-async, since the signature of the delegate is as below: `public delegate bool RemoteCertificateValidationCallback(object sender, X509Certificate? certificate, X509Chain? chain, SslPolicyErrors sslPolicyErrors);` I.e. ```c# services .AddHttpClient("someEndpointClient") .ConfigureHttpMessageHandlerBuilder(builder => { var handler = (HttpClientHandler)builder.PrimaryHandler; handler.ServerCertificateCustomValidationCallback = (httpRequestMessage, certificate, chain, sslPolicyErrors) => { Task someValidationTask = ...; return someValidationTask.GetAwaiter().GetResult(); // sync-over-async }; }); ``` Original discussion: https://github.com/dotnet/runtime/discussions/78761 ### API Proposal ```csharp namespace System.Net.Security; public delegate ValueTask RemoteCertificateValidationAsyncCallback(object sender, X509Certificate? certificate, X509Chain? chain, SslPolicyErrors sslPolicyErrors); ``` ```csharp namespace System.Net.Security public class SslClientAuthenticationOptions { ... public RemoteCertificateValidationAsyncCallback? RemoteCertificateValidationAsyncCallback { get; set; } } ``` ```csharp namespace System.Net.Http public partial class HttpClientHandler : HttpMessageHandler { ... public Func>? ServerCertificateCustomValidationAsyncCallback { get; set; } } ``` ### API Usage ```c# services .AddHttpClient("someEndpointClient") .ConfigureHttpMessageHandlerBuilder(builder => { var handler = (HttpClientHandler)builder.PrimaryHandler; handler.ServerCertificateCustomValidationAsyncCallback = async (httpRequestMessage, certificate, chain, sslPolicyErrors) => { Task someValidationTask = ...; return await someValidationTask.ConfigireAwait(false); // sync-over-async }; }); ``` ### Alternative Designs _No response_ ### Risks Replacing the current sync callback is a breaking change. But introducing a new one and having 2 callbacks for the same reason is not a good solution either
Author: mddddb
Assignees: -
Labels: `api-suggestion`, `area-System.Net.Security`, `untriaged`
Milestone: -
mddddb commented 1 year ago

@karelz @wfurt @ManickaP

rzikm commented 1 year ago

API proposal LGTM, the only question is whether we want to have an async certificate selection callback as well, while we are at it. Thoughts? @wfurt.

Not critical for 8.0, but would be nice to get it in an LTS release. Putting to Future for now.

wfurt commented 1 year ago

I think the changes in HttpClientHandler are possibly problematic. AFAIK we did not change the shape for long time for compat reasons. It would also expose it for all platform handlers and I'm not sure if that really doable.

SocketsHttpHandler already expose SslClientAuthenticationOptions so it can be used directly.

Having two almost same functions is not great. But we can make them mutual exclusive in validation.

Simple workaround IMHO would be letting handshake always to continue and do extra validation after it is done. In practice, this what we do anyway (the callback runs after handshake is completed at TLS level) at the moment I would assume failure would be rare case (e.g. mostly you connecting to valid sites)

Note that with 7.0 one can influence the default validation via CertificateChainPolicy e.g. disable all online check to avoid interference.

simonrozsival commented 1 year ago

I don't think we would be able to implement the async callback in AndroidMessageHandler and NSUrlSessionHandler without blocking. I suggest making this API specific to SocketsHttpHandler.

mddddb commented 1 year ago

@karelz @wfurt @ManickaP, just curious, is there any update on the planning for this?

karelz commented 1 year ago

@mddddb we currently do not have plans for implementing it. It will not fit into 8.0 for sure. We can revisit it for 9.0+, however given the troubles it brings and the low demand (no upvotes on top post), I may be cut eventually.