Proposed SslStream additions for ALPN and SNI

[Drawaes]

Rationale

---

SNI and ALPN have been hanging around for a while as requirements and are becoming more urgent for Kestrel as HTTP/2 approaches (ALPN) and it becomes an edge server (SNI). There was an initial API proposal in issue #15813 however this stalled from my reading because of two issues

1. It would break compat with desktop
2. It would make the AuthenticateAsXXXXX method overloads become unwieldy

So I think the need has been established just not the API Design and therefore the implementation.

#Proposed API change

I would propose adding extension methods for SslStream.

public static SslStreamExtensions
{
    public delegate void ApplicationLayerProtocolCallback(object sender, string matchedProtocol);
    public delegate bool ServerNameIndiciationCallback(object sender, string serverName, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors);

    public void SetApplicationLayerProtocolNegotiation(this SslStream self, string[] supportedProtocols, ApplicationLayerProtocolNegotiationCallback callback) {}

    public void SetServerNameIndiciation(this SslStream self, Dictionary<string, X509Certificate> serverCertificates, ServerNameIndiciationCallback callback) {}
}

One other potential idea is to move this off to a "Builder" method for SslStream. This might help in the server scenario as multiple SslStreams could be built from it and components that each can share could be reused (for instance SSL_CTX in openssl is only required 1x for the same settings rather than per connection).

Example Usage

---

var supportedProtocols = new string[] {"http1.1", "h2"};
var supportedServerNames = new Dictionary<string,X509Certificate>()
{
    "www.myanimatedgifsrock.com" , new X509Certificate("secretpath", "secretpassword"),
    "www.immoslesscoolgifs.com", new X509Certificate("expiredcertpath", "secretpassword2"),
};
var SslStream stream = new SslStream(networkStream);
stream.SetApplicationLayerProtocolNegotiation(supportedProtocols, MyAlpnHandler);
stream.SetServerNameIndiciation(supportedServerNames, MySniHandler);

//normal use resumes

Referenced Issues

---

#17677 SNI
#15813 ALPN

Potential Issues

---

The callback for SNI should need to (and does in above) provide the ability to validate the certificate. Consideration would need to be given to the concept that if the SNI callback provides this what happens if a callback is provided in the Authenticate method. Also should there be separate extension methods for client and for server? Maybe but really you probably want both to have the chance to decide if they are happy with the otherside based on the servername that they tried to connect to.

The other issue with this pattern is that there is no "state" held in the SslStream. Once the callback has been called there is no real way to retrieve this information later and it would be up to the user to associate this information with the SslStream and carry it around. To this end perhaps an extension method/methods to later retrieve this information might be considered such as below

public static bool TryGetApplicationLayerProtocol(this SslStream self, out string protocol);
public static bool TryGetServerName(this SslStream self, out string serverName);

Thanks :)

[davidsh]

Our newest thinking (which has evolved a lot over a year +) on API additions is that we will add them to 'netcoreapp' and not directly to 'netstandard'. The latest 'netcore' architecture supports this well. We have been doing this for several months now. Our time, we will add it to .NET Framework so that it can be part of a future 'netstandard'.

The best API shape for this functionality is to add it as direct methods/properties with perhaps new API overloads. Thus we prefer not adding extension methods for this.

cc: @stephentoub

[Tratcher]

I agree SNI needs a callback model. The one you've defined seems to be trying to copy the client cert validation callback and I don't think those two scenarios are directly related. The dictionary approach is also overly prescriptive, there's no telling how many domains I may be trying to handle, and I may need to do wildcard matching. How about:

public virtual Task AuthenticateAsServerAsync(Func<string, Task<X509Certificate>> serverCertificateCallback, ...);

For clients not using SNI the string parameter would be string.Empty. Returning null would indicate no matching certificate could be found (causing AuthenticateAsServerAsync to fail).

[Drawaes]

The callback is for the client cert. The server side could get a different cert depending on each host.

I am not sure on Schannel you can supply the server side cert from a callback. Hence it will be a fixed list.

[Drawaes]

@davidsh , okay that's confusing as the other API proposal froze for that exact reason :P I am more than happy if it isn't extension methods :)

As for more overloads on those methods... really? They are awful already and if they keep adding extensions you will end up with

AuthenticateAsServerAsync(
	X509Certificate serverCertificate,
	bool clientCertificateRequired,
	SslProtocols enabledSslProtocols,
	bool checkCertificateRevocation,
        string[] protocols,
        IEnumerable<string, x509certificate> supportedServerNames,
        bool SniMatchRequired,
        etc....
  
)

Which is just going to get ugly, SslStream is really a complex little protocol component and to tring to configure the entire thing through one method seems... well not wonderful

[Tratcher]

Is it time to make some objects to contain all of the options and results?

public Task<ServerAuthResults> AuthenticateAsServerAsync(ServerAuthOptions options);
public Task<ClientAuthResults> AuthenticateAsClientAsync(ClientAuthOptions options);

[Drawaes]

@Tratcher I am not sure my answer is clear. So let me try to rephrase

There are two things going on with SNI,

1. you need to decide what cert to send to the client
2. you (may) need to validate the client cert/connection and that could/should be different per "host" that you can connect to.

Now what you want is for 1 you can provide a managed function that will return the correct server cert. Which while is a nice idea (I would like that too) I am pretty sure that isn't going to fly with SChannel. I am looking at the header to reacquaint myself with it but I am 75% sure you have to provide a static array upfront (FYI you can provide a function for OpenSsl... just saying Windows.... ;))

As for the client section.... if we don't have to use extension methods then I say just make the option of the normal client validation callback or one that includes the server name that was validated.

As for your point above ^^^^ 100% agree, lets do that if we can... but can we make the options "built" or something like that. Because to be honest there is a bunch of optimizations that can be made behind the scenes if those "options" stay the same per connection (like say if you are a webserver making loads of connections ;) ).

But here is the kicker.... if you kill the options, you kill the connection in that case. Then again that can be hidden from the user, and if the SslStream holds a copy they won't go away anyway... but you would need to check when the last stream stopped using them potentially....

Not sure now :) I am leaning towards a factory/builder to be honest... I have been thinking about this for 12 months and have written it 5 different ways and keep coming back to that, although that could be achieved by making a hidden one in the background based on the options object.

[Tratcher]

How well does the builder handle the disparity between client and server options? Do you need a server builder that builds a ServerSslStream and likewise for the client to keep the options sensible?

[Drawaes]

I would say they can be pretty close to the same if not the same. I have mostly prototyped the server side here around what it would look like for aspnetcore. I need to think about the client side some more. I would say if you want ALPN in soon, then maybe the SNI can be dropped for now and some simple methods can be added for that. Kick the can down the road so to speak (the internals of SslStream are like a history lesson in .net code with many can kicking sessions).

[iamcarbon]

We should consider dynamic certificate issuance & renewal scenarios when designing this API. This would enable certificates to be acquired through Let's Encrypt or any other dynamic source in real-time.

For example:

ValueTask<X509Certificate2>  GetServerCertificateAsync(string serverName) {
    // lookup locally & ensure the certificate is still valid, otherwise --
    // fetch from provider (i.e. Let's Encrypt & wait (requests should remain in-flight until this method returns)
}

[Drawaes]

Again as far as I am aware you can't dynamically provide certs for schannel. You need an upfront list of certs.

[iamcarbon]

@Drawaes Keeping my hopes alive that schannel has some way of dynamically providing certificates as it has the potential to dramatically reduce the developer and operational burdens of setting up and maintaining SSL certificates. But if it can't be done -- it can't be done!

[Drawaes]

There is no reason if there is a config object as suggested above that it could take care of the list. It's good to have dreams (FYI the openssl side can do a callback).

[Tratcher]

Here's a more complete version of my options proposal above.

    public partial class SslStream
    {
        public ServerAuthResults AuthenticateAsServer(ServerAuthOptions options) { }
        public Task<ServerAuthResults> AuthenticateAsServerAsync(ServerAuthOptions options) { }
        public ClientAuthResults AuthenticateAsClient(ClientAuthOptions options) { }
        public Task<ClientAuthResults> AuthenticateAsClientAsync(ClientAuthOptions options) { }
    }

    public class SharedAuthOptions
    {
        public SslProtocols? EnabledSslProtocols { get; set; }
        public bool CheckCertificateRevocation { get; set; }
        public IList<string> ApplicationProtocols { get; }
        public RemoteCertificateValidationCallback RemoteCertificateValidationCallback { get; set; }
        public EncryptionPolicy? EncryptionPolicy { get; set; }
    }

    public class ServerAuthOptions : SharedAuthOptions
    {
        public X509Certificate ServerCertificate { get; set; }
        public IDictionary<string, X509Certificate> ServerCertificates { get; set; }
        public Func<string, Task<X509Certificate>> ServerCertificateCallback { get; set; } // Not supported on Windows?
        public bool ClientCertificateRequired { get; set; }
    }

    public class ClientAuthOptions : SharedAuthOptions
    {
        public string TargetHost { get; set; }
        public X509CertificateCollection ClientCertificates { get; set; }
        public LocalCertificateSelectionCallback UserCertificateSelectionCallback { get; set; }
    }

    public class SharedAuthResults
    {
        public string NegotiatedApplicationProtocol { get; }
        // These and many other results are already exposed directly on SslStream
        public X509Certificate LocalCertificate { get; }
        public X509Certificate RemoteCertificate { get; }
    }

    public class ServerAuthResults : SharedAuthResults
    {
    }

    public class ClientAuthResults : SharedAuthResults
    {
    }

Observations:

• public delegate X509Certificate LocalCertificateSelectionCallback(object sender, string targetHost, X509CertificateCollection localCertificates, X509Certificate remoteCertificate, string[] acceptableIssuers); could almost be re-used for the callback form of SNI.
• I included settings previously specified in the constructors as it simplifies overall usage.
• This cuts way down on permutations, and leaves most parameters optional. Enums can become nullable to allow for using system defaults.
• Most of the properties on SslStream are symmetrical negotiation results that would have belonged on SharedAuthResults, but we don't need to duplicate them at this point. NegotiatedApplicationProtocol is the only new result so we may not need the result objects.

[jnm2]

@Drawaes Did you mean "additions" or "editions" in the title?

[Drawaes]

Fixed :)

[Matthias247]

@Tratcher If you go the way with the Options structures (which I think is a good way for APIs with many configuration options) I would recommend to go one step further: Remove all setters from the Options and create them through some additional Builders.

That has several advantages:

• Options can be truly immutable and savely be shared between multiple auth handlers. Otherwise users might make errors like mutating the options between two calls to AuthenticateAsXyz() which might cause undefined behavior if the still running async methods use them.
• You can have methods on the builders which set several properties at once if they are related, or if setting one property should always require setting another to a default value.

You can even make Options completely opaque by removing all getters from the public API. That allows for implementing them in most optimized way for the framework and restricts the API surface to the builders.

[Drawaes]

100% agree with above. Today the Unix pal is creating things that could be shared resources... I have been thinking about this and the issue comes around destruction of the shared resources but that is an implementation detail.

[stephentoub]

If we go with an options bag approach, it'd be nice to include a CancellationToken on the base options type, so that the authentication operations can be canceled (likely leaving the connection in a bad state, but that's fine).

[Tratcher]

Cancellation is a good add, but putting it on the options bag seems a little strange. The defacto pattern has been Task MyMethodAsync(parameters..., CancellationToken cancellation = CancealltionToken.None);

Would you apply that cancellation to the sync APIs as well?

[stephentoub]

My higher-level point was just that we should add cancellation support... I'm not tied to any particular approach for doing so.

[Tratcher]

Updated with async cancellation and dropping the result objects.

    public partial class SslStream
    {
        public string NegotiatedApplicationProtocol { get; }
        public ServerAuthResults AuthenticateAsServer(ServerAuthOptions options) { }
        public Task AuthenticateAsServerAsync(ServerAuthOptions options, CancellationToken cancellationToken) { }
        public ClientAuthResults AuthenticateAsClient(ClientAuthOptions options) { }
        public Task AuthenticateAsClientAsync(ClientAuthOptions options, CancellationToken cancellationToken) { }
    }

    public class SharedAuthOptions
    {
        public SslProtocols? EnabledSslProtocols { get; set; }
        public bool CheckCertificateRevocation { get; set; }
        public IList<string> ApplicationProtocols { get; }
        public RemoteCertificateValidationCallback RemoteCertificateValidationCallback { get; set; }
        public EncryptionPolicy? EncryptionPolicy { get; set; }
    }

    public class ServerAuthOptions : SharedAuthOptions
    {
        public X509Certificate ServerCertificate { get; set; }
        public IDictionary<string, X509Certificate> ServerCertificates { get; set; }
        public Func<string, Task<X509Certificate>> ServerCertificateCallback { get; set; } // Not supported on Windows?
        public bool ClientCertificateRequired { get; set; }
    }

    public class ClientAuthOptions : SharedAuthOptions
    {
        public string TargetHost { get; set; }
        public X509CertificateCollection ClientCertificates { get; set; }
        public LocalCertificateSelectionCallback UserCertificateSelectionCallback { get; set; }
    }

[Drawaes]

As said earlier by someone else I have a problem with this being mutable. What if someone changes the supported protocol during a handshake or the ceritifactes list? If this is the way to go it will either need to be copied internally... or it will need to has a builder etc and be immutable once built.

[Tratcher]

That primarily depends on the reusability. If we say it's reusable then yes it needs to be immutable with a builder. If it's not reusable then people have no need to modify it after the operation starts and you can avoid creating extra builder APIs.
We've used the non-reusable options approach elsewhere and haven't had any trouble with users modifying them out of band.

[Drawaes]

Sure. But this is security. Someone could change or substitute certificates in user code say. It's a can of worms to have it immutable. Today for instance the Sslstream takes a handle from Tue cert and ups the ref count on the unmanaged handle.

So even if client code does something to the cert it's not an issue. Say you have an option object with safe settings (tls 1.2 only) but you need to connect to some old service in your code and the user accidently reuses that options and down sets the ssl version. Thus changing what the server supports.

So I think if the mutable options approach is taken then the content will need to be cloned including an up reference and new handle for each cert.

Or a builder. I am keen for either. I think the options object is a good approach

[Tratcher]

Standard usability vs the-ability-to-shoot-yourself-in-the-foot argument. As it's unrelated to the actual SSL features I'll defer to corefx's API design guidance.

[Drawaes]

Sure as I said I am happy either way, with implementation being easier one way but possible for both. It's more a heads up that there are risks with the mutable version that will need to be mitigated.

[Drawaes]

Closed due to new proposal. It's not very internet cool, but @Tratcher changed my mind so I have ditched my original thinking :)

dotnet/corefx#23077

[benaadams]

You self referenced; think you meant new one is https://github.com/dotnet/corefx/issues/23177

Interesting is exactly 100 issues on :)

[Drawaes]