Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mddddb opened this issue Dec 9, 2022 · 8 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Security
Milestone

Comments

@mddddb
Copy link
Contributor

mddddb commented Dec 9, 2022

Background and motivation

It is currently possible to have a custom remote certificate validation for HttpClient using

public Func<HttpRequestMessage, X509Certificate2?, X509Chain?, SslPolicyErrors, bool>? ServerCertificateCustomValidationCallback
and
public RemoteCertificateValidationCallback? RemoteCertificateValidationCallback { get; set; }

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: #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

@mddddb mddddb added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 9, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 9, 2022
@ghost
Copy link

ghost commented Dec 9, 2022

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 and RemoteCertificateValidationCallback .
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: #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); // 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
Copy link

ghost commented Dec 9, 2022

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

public Func<HttpRequestMessage, X509Certificate2?, X509Chain?, SslPolicyErrors, bool>? ServerCertificateCustomValidationCallback
and
public RemoteCertificateValidationCallback? RemoteCertificateValidationCallback { get; set; }

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: #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); // 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
Copy link
Contributor Author

mddddb commented Dec 20, 2022

@karelz @wfurt @ManickaP

@rzikm
Copy link
Member

rzikm commented Jan 2, 2023

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.

@rzikm rzikm removed the untriaged New issue has not been triaged by the area owner label Jan 2, 2023
@rzikm rzikm modified the milestones: 8.0.0, Future Jan 2, 2023
@wfurt
Copy link
Member

wfurt commented Jan 18, 2023

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
Copy link
Member

simonrozsival commented Jan 18, 2023

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
Copy link
Contributor Author

mddddb commented May 12, 2023

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

@karelz
Copy link
Member

karelz commented May 31, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Security
Projects
None yet
Development

No branches or pull requests

6 participants