Proposed SslStream additions for ALPN

[Drawaes]

Latest Proposal

https://github.com/dotnet/corefx/issues/23177#issuecomment-332995338

namespace System.Net.Security
{
public partial class SslStream
{
    public Task AuthenticateAsServerAsync(SslServerAuthenticationOptions options, CancellationToken cancellation) { }
    public Task AuthenticateAsClientAsync(SslClientAuthenticationOptions options, CancellationToken cancellation) { }
    
    public SslApplicationProtocol NegotiatedApplicationProtocol { get; }
}

public class SslServerAuthenticationOptions
{
    public bool AllowRenegotiation { get; set; }
    public X509Certificate ServerCertificate { get; set; }
    public bool ClientCertificateRequired { get; set; }
    public SslProtocols EnabledSslProtocols { get; set; }
    public X509RevocationMode CheckCertificateRevocation { get; set; }
    public IList<SslApplicationProtocol> ApplicationProtocols { get; set; }
    public RemoteCertificateValidationCallback RemoteCertificateValidationCallback { get; set; }
    public EncryptionPolicy EncryptionPolicy { get; set; }
}

public class SslClientAuthenticationOptions
{
    public bool AllowRenegotiation { get; set; }
    public string TargetHost { get; set; }
    public X509CertificateCollection ClientCertificates { get; set; }
    public LocalCertificateSelectionCallback LocalCertificateSelectionCallback { get; set; }
    public SslProtocols EnabledSslProtocols { get; set; }
    public X509RevocationMode CheckCertificateRevocation { get; set; }
    public IList<SslApplicationProtocol> ApplicationProtocols { get; set; }
    public RemoteCertificateValidationCallback RemoteCertificateValidationCallback { get; set; }
    public EncryptionPolicy EncryptionPolicy { get; set; }
}

public struct SslApplicationProtocol : IEquatable<SslApplicationProtocol>
{
    public static readonly SslApplicationProtocol Http2;
    public static readonly SslApplicationProtocol Http11;
    // Adding other IANA values, is left based on customer input.

    public SslApplicationProtocol(byte[] protocol) { }

    public SslApplicationProtocol(string protocol) { } 
  
    public ReadOnlyMemory<byte> Protocol { get; }

    public bool Equals(SslApplicationProtocol other) { }
    public override bool Equals(object obj) { }
    public override int GetHashCode() { }
    public override string ToString() { } 
    public static bool operator ==(SslApplicationProtocol left, SslApplicationProtocol right) { }
    public static bool operator !=(SslApplicationProtocol left, SslApplicationProtocol right) { }
}
}

Previous Proposal

Change log:

• Updated = Removed results object to keep new objects down
• Updated = Meeting notes
• Updated = Removed string overload as there is no clear way in code to tell the user that it's utf-8 and only adds a potential trap
• Updated = Put that string overload back under protest, but decision was made in a review meeting

References #23107

Rationale

Server Name Indication and Application Layer Protocol Negotiation are two TLS extensions that are missing currently from SslStream. They are needed urgently to support Http/2 (ALPN) and the ability to run Kestrel as an edge server (SNI). There are also many other customer applications that require this, including Clients being able to use HTTP/2 (Mananed HttpClient has an Http/2 support open issue).

These have been outstanding for a long period of time, for a number of reasons. One major reason is that any change will cause an increase in overloading of the Authenticate methods which have become unwieldy and are brittle when adding new options.

There will be more options coming with other TLS extensions available now (max fragment size for restricted memory clients for instance) and having a mechanism to add these without major API changes seems like a sensible change.

Proposed API Change

Originally I suggested overloading the Authenticate methods, however discussions in #23107 have changed my mind. Thanks to @Tratcher for this concept

public partial class SslStream
{
   public Task AuthenticateAsServerAsync(SslServerAuthenticationOptions options, CancellationToken cancellation) { }
   public Task AuthenticateAsClientAsync(SslClientAuthenticationOptions options, CancellationToken cancellation) { }
    
   public SslApplicationProtocol NegotiatedApplicationProtocol {get;}
}

public class SslServerAuthenticationOptions
{
   public bool AllowRenegotiation { get; set; }
   public X509Certificate ServerCertificate { get; set; }
   public bool ClientCertificateRequired { get; set; }
   public SslProtocols EnabledSslProtocols { get; set; }
   public X509RevocationMode CheckCertificateRevocation { get; set; }
   public IList<SslApplicationProtocol> ApplicationProtocols { get; set; }
   public RemoteCertificateValidationCallback RemoteCertificateValidationCallback { get; set; }
   public EncryptionPolicy EncryptionPolicy { get; set; }
}

public class SslClientAuthenticationOptions
{
   public bool AllowRenegotiation { get; set; }
   public string TargetHost { get; set; }
   public X509CertificateCollection ClientCertificates { get; set; }
   public LocalCertificateSelectionCallback LocalCertificateSelectionCallback { get; set; }
   public SslProtocols EnabledSslProtocols { get; set; }
   public X509RevocationMode CheckCertificateRevocation { get; set; }
   public IList<SslApplicationProtocol> ApplicationProtocols { get; set; }
   public RemoteCertificateValidationCallback RemoteCertificateValidationCallback { get; set; }
   public EncryptionPolicy EncryptionPolicy { get; set; }
}

public struct SslApplicationProtocol : IEquatable<SslApplicationProtocol>
{
    public static readonly SslApplicationProtocol Http2;
    public static readonly SslApplicationProtocol Http11;
    // Adding other IANA values, is left based on customer input.

    public SslApplicationProtocol(byte[] protocol) { }

   public SslApplicationProtocol(string protocol) { } // assumes utf-8 and public override 
  
    public ReadOnlyMemory<byte> Protocol { get; }

    public bool Equals(SslApplicationProtocol other) { }
    public override bool Equals(object obj) { }
    public override int GetHashCode() { }
    public override string ToString() { } // For debugger. utf-8 or a string representation of the raw bytes. E.g. "0x68 0x74 0x74 0x70 0x2f 0x31 0x2e 0x31"
    public static bool operator ==(SslApplicationProtocol left, SslApplicationProtocol right) { }
    public static bool operator !=(SslApplicationProtocol left, SslApplicationProtocol right) { }
}

Meeting Notes 22-Sep-2017

1. Add ToString() to SslApplicationProtocol to get string version of the bytes.
2. Add string ctor to SslApplicationProtocol for usability, this will assume it's utf8 string.
3. ReadOnlyMemory is not immutable, we need to copy the bytes in the ctor, so taking a byte[] instead of ReadOnlyMemory

Meeting Notes 5-Sep-2017

1. Use the updated API proposal above with options bags on the Authenticate methods.
2. Introduce an ApplicationProtocol type so the ALPN values can be correctly defined as raw bytes, have shared constants, and have equality operators.
3. Disallow mixing calls between the old constructors and the new methods. Only the minimal constructors taking the inner stream and ownership bool will be supported.
4. There is still some pending discussion on the factory approach.

Further Notes

1. This will mean future options can be added without causing binary compatibility issues (increasing properties on concrete classes rather than changing overloads)
2. Non async methods shouldn't be supported for the new methods as it is an async operation under the hood so hiding the threads goes against current framework thinking. Consumers can wrap the async methods with blocking if they so wish (see modern HttpClient )
3. I snuck max fragment in for the client, but I am happy to have this dropped if it is a sticking point
4. Cancellation tokens are there for both methods as per current framework thinking
5. ValueTask wasn't considered because there will be very few times this doesn't cause async operations, but if the new standard is to use ValueTask that is fine as well
6. A static list of the Http 1.1, Http/2 and Http/2 over TLS should be provided to stop users having to look it up.
7. Helper methods on the auth results should be provided so users don't have to look up what the string representations are in this case I added one for IsHttp2.

Potential Implementation Issues

There are now a number of parameters that could be modified during an connection being created. One of these is the certificate dictionary. If the consumer changes these by accidentally reusing the config options for a differently configured connection you could run into a security issue. One option is to have a builder but this is frowned upon as an API pattern and instead mostly used in higher level libraries/frameworks (ASP.NET for instance). This is solvable via taking an internal snapshot of the options but will need to be considered in any threat modelling.

Example Usage

var options = new ServerAuthenticationOptions()
{
    EnabledProtocols = SslProtocols.Tls12 | SslPRotocols.Tls11,
    ApplicationProtocols = new List<ApplicationProtocol>()
    {
         ApplicationProtocol.Http2,
         ApplicationProtocol.Http11
    }
};
var secureStream = new SslStream(innerStream);
await secureStream(options, token);

References

#17677 SNI
#15813 ALPN
#23107 Previous API proposal
RFC 7301 ALPN
RFC 3546 SNI and Max fragment
IANA ALPN Protocol Ids

[EDIT] Updated formatting by @karelz

[karelz]

cc @geoffkizer @Priya91 @wfurt

[Priya91]

@Drawaes What is the motivation behind adding options bag separately, this doesn't work well with the current API design in SslStream. Instead of introducing so many new types, I think exposing overloads of AuthenticateAs[Client|Server]Async methods to have application protocols that they would like to handshake for will be simple and get the job done.

[Drawaes]

Because you also need another overload for SNI or overloads. So now you have a larger matrix. The referenced previous issue #23107 was originally going this route however the number of overloads is becoming excessive. There are more extensions that have yet to manifest or be considered both with TLS 1.2 and TLS 1.3 around the corner.

Consider ZeroRtt data, that should be surfaced as an API which will need yet another overload. I guess the question is how many overloads is too many?

[Drawaes]

I would be happy for the return types to be dropped and maybe the client and server args to be merged (not the best but hey) but just adding protocolIds doesn't cover SNI. It merely kicks the can a little down the road. It also makes it difficult for end users a raw protocol list is a bit unfriendly without any helper methods.

[Priya91]

Reading through the RFCs, this is the proposal that I have,

namespace System.Net.Security
{
    public partial struct TlsExtension
    {
        public TlsExtension(TlsExtensionType extensionType, Span<byte> extensionData) { throw null; }
        public TlsExtensionType ExtensionType { get => throw null; }
        public Span<byte> ExtensionData { get => throw null; }
    }
    public enum TlsExtensionType
    {
        ServerName = 0,
        Alpn = 16
    }
    public partial class SslStream : AuthenticatedStream
    {
        public virtual void AuthenticateAsClient(string targetHost, System.Security.Cryptography.X509Certificates.X509CertificateCollection clientCertificates, System.Security.Authentication.SslProtocols enabledSslProtocols, bool checkCertificateRevocation, IList<TlsExtension> tlsExtension) { throw null; }
        public virtual void AuthenticateAsServer(System.Security.Cryptography.X509Certificates.X509Certificate serverCertificate, bool clientCertificateRequired, System.Security.Authentication.SslProtocols enabledSslProtocols, bool checkCertificateRevocation, IList<TlsExtension> tlsExtension) { throw null; }
        public virtual Task AuthenticateAsClientAsync(string targetHost, System.Security.Cryptography.X509Certificates.X509CertificateCollection clientCertificates, System.Security.Authentication.SslProtocols enabledSslProtocols, bool checkCertificateRevocation, IList<TlsExtension> tlsExtension) { throw null; }
        public virtual Task AuthenticateAsServerAsync(System.Security.Cryptography.X509Certificates.X509Certificate serverCertificate, bool clientCertificateRequired, System.Security.Authentication.SslProtocols enabledSslProtocols, bool checkCertificateRevocation, IList<TlsExtension> tlsExtension) { throw null; }
    }
}

For the ExtensionData, we can provide structure to it by maybe having static methods on TlsExtension like,

public partial struct TlsExtension
{
    public static Span<byte> GetAlpnExtensionData(IList<string> protocols) { throw null; }
    public static Span<byte> GetServerNameExtensionData(IList<string> serverNames) { throw null; }
}

cc @geoffkizer @stephentoub @davidsh @Caesar1995 @CIPop

@CIPop Since you've already looked at implementing Alpn, do you have any thoughts on how we should define the APIs here..

[Drawaes]

Wait, but can I only add a single extension here? What if I want ALPN and SNI (which is a very common case on the server side). Also how do I provide multiple certificates? SNI's normal use is this

Client says I am connecting to www.mycooldomain.com

Server says okay I expect either www.mycooldomain.com or www.mylesscooldomain.com, seeing as you picked the first I shall provide certificate A to you.

[Drawaes]

You would need a (xxxxxxxxxx, params TlsExtension[] extensions)

And if you do that you now preclude any future overloads, and get into trouble. And it still doesn't provide for the ServerName -> Certificate mapping.

[Drawaes]

Also this may become a thing, #22507 if there was an options bag it could be added easily, with an overload it will be messy.

[geoffkizer]

I'm a little concerned that the options bag approach is turning a relatively constrained feature add (ALPN) into a bigger deal (fix the SslStream API).

[Priya91]

Wait, but can I only add a single extension here?

The overloads can be made to use an IList for that case.

Also how do I provide multiple certificates?

I'm not sure how to address this with the current design, let me think about it more. I'll have to investigate how openssl/schannel provide support Tls extensions, and see if we can emulate that design. I agree that with more extensions being added, we should design an api that's extensible.

[Priya91]

Instead of bundling all the options in a new options bag, wouldn't it be better to identify the specific set of settings that's required for each extension that's supported by Sslstream, and add a property for it on the TlsExtension struct? So the TlsExtension will contain all settings required for performing that extension in the handshake.

[Drawaes]

So you want an OptionsBag but to leave the current options out of it. Sure that's a possibility. If you want to understand SChannel and OpenSsl for SNI on the server side there are two different approaches (of course they are different :P )

OpenSsl You provide a callback. When SNI is detected it calls the callback. With your SSL object, you can then read the SNI from the callback params and you "switch" the SSL_CTX (context) that it is attached to (hence you normally make and pool the contexts with all the certs upfront, which by the way is something that should be done anyway, but that is a different issue for another day).

For SChannel you supply a list of certificates when you make the Authentication Object, rather than one, and it picks based on the SNI.

For ALPN you provide the extension to Schannel as an extra token buffer. You basically write it itself. Then there is a property you can inspect to find the one agreed on.

For OpenSsl you once again set the protocols, and on the server side again you get a callback.

So I am not sure the implementation really gives us much help here, basically for every new extension it is hand coded in OpenSsl and they add specific methods/callbacks for them.

[Drawaes]

@geoffkizer in your response, you have ignored SNI which is pretty important, for a number of reasons (I have internal ones) but @Tratcher will give you some I am sure as well.

Plus as a final thing, SslStream doesn't have a cancellation token, which is a bit rubbish, handshakes can easily be stalled by a client and today there is no way of stopping it other than killing the underlying stream, so the upper application needs access to the network stream.

[Drawaes]

Updated to drop results.

[Drawaes]

References to other frameworks
GO - Options Bag
NodeJs - Options Bag
Jetty - ContextFactory