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: SocketsHttpHandler better certificate APIs #41191

Open
davidfowl opened this issue Aug 21, 2020 · 6 comments
Open

API Proposal: SocketsHttpHandler better certificate APIs #41191

davidfowl opened this issue Aug 21, 2020 · 6 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented Aug 21, 2020

Background and Motivation

Today SocketsHttpHandler exposes certificate settings via SslOptions (which is of type SslClientAuthenticationOptions). We can use these APIs to handle server certificate validation and client certificate selection. The only issue is that these callbacks don't provide the context of the HttpRequestMessage which can make it hard to determine what logic should be run (especially in multitenant scenarios). We should add 2 callbacks to the SocketsHttpHandler for these scenarios, one to handler client cert selection and the other to handle server certificate validation. HttpClientHandler has a server certificate selection callback for this but nothing for client cert selection.

Proposed API

namespace System.Net.Http
{
    public class SocketsHttpHandler
    {
+      public Func<HttpRequestMessage, X509Certificate2, X509Chain, SslPolicyErrors, bool> ServerCertificateValidationCallback { get; set; }
+      public Func<HttpRequestMessage, X509CertificateCollection, X509Certificate, string[], X509Certificate2> ClientCertificateSelectionCallback { get; set; }
    }
}

Usage Examples

var handler = new SocketsHttpHandler();
handler.ServerCertificateCustomValidationCallback = (message, cert, chain, errors) =>
{
    var tenant = FindTenant(message);
    return tenant.CheckServerCert(cert, chain, errors);
};

handler.ClientCertificateSelectionCallback = (message, localCertificates, remoteCertificate, acceptableIssuers) =>
{
    var tenant = FindTenant(message);
    return tenant.ClientCertificate;
};
@davidfowl davidfowl added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 21, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Net.Http untriaged New issue has not been triaged by the area owner labels Aug 21, 2020
@ghost
Copy link

ghost commented Aug 21, 2020

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

@geoffkizer
Copy link
Contributor

Can you give me an example of what you'd do in FindTenant above that would rely on info that you can't get from the current callbacks?

I'm fine with adding these if we have scenarios for it. But I always worry that this sort of thing can get you into trouble. The SSL connection will be reused for lots of different requests, and if you don't understand the implications of this you could shoot yourself in the foot.

@davidfowl
Copy link
Member Author

The scenario came from an internal customer that was trying to manage multitenant resources with a single SocketsHttpHandler instance. We ended up with a terrible workaround that had to use private reflection to grab the SocketsHttpHandler out of the HttpClientHandler instance. Currently, the callback that lives directly on SslOptions doesn't expose the host name but SslStream has the target host in .NET 5 so that's good (they were on 3.1).

HttpClientHandler does expose the HttpRequestMessage so this feature exists today if you use that handler but not the SocketsHttpClientHandler directly.

The reason to expose the HttpRequestMessage is because that's the only means by which people have to roundtrip state between their code and framework callbacks. I can stash the tenant information in Properties and read it later. I understand the fear but I think since HttpClientHandler already exposes the request, it makes sense to expose it on the other handler.

@geoffkizer
Copy link
Contributor

I can stash the tenant information in Properties and read it later.

Yeah that makes sense. That said, I wonder if there is a safer mechanism to convey that kind of information.

@davidfowl davidfowl mentioned this issue Aug 23, 2020
2 tasks
@scalablecory scalablecory removed the untriaged New issue has not been triaged by the area owner label Aug 30, 2020
@scalablecory scalablecory added this to the Future milestone Aug 30, 2020
@scalablecory
Copy link
Contributor

If we add ClientCertificateSelectionCallback, we should make it async to allow loading from e.g. databases.

@ekuhlmann23
Copy link

Are there any plans to expose such callbacks?

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.Http
Projects
None yet
Development

No branches or pull requests

5 participants