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

[Priya91]

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.

The SslStream uses openssl and schannel, for whatever APIs we expose, we need to be able to plug it into these pal layers. So it is worth thinking about implementation here, and getting ideas from already used frameworks

[Drawaes]

Sure I agree, I was merely pointing out that they are both diametrically opposed unfortunately. Life would be too easy otherwise :)

[Drawaes]

FYI, further investigation it seems you can provide a callback for SNI on SChannel. Which means that if a callback is surfaced on the SslStream then the scenario of something like Let's Encrypt providing a dynamic certificate is possible for both SslStream and OpenSsl. So either a list or a callback would be ideal, if only one can be provided then the callback would be preferable.

[geoffkizer]

it seems you can provide a callback for SNI on SChannel

Can you provide a link to the API for this?

[geoffkizer]

@Drawaes I'm not ignoring SNI or cancellation. I'm simply observing that the only must-have feature here for 2.1 is ALPN.

That said, @stephentoub and I chatted a bit today, and we agree that the options bag approach here seems like the right one, despite the additional work and risk associated with it.

[geoffkizer]

I think it would be useful to break this proposal down into a couple different parts:

(1) New AuthenticateAsClient/Server APIs that take option bags and a cancellation token, as well as the definition of the option bags that match existing APIs. We should be clear about the principles used here. I believe we plan to take both constructor args and method args and put them on the options bag. Is that correct?
(2) Proposal to extend this model for ALPN
(3) Proposal to extend this model for SNI

edited to add:
(4) Proposal to extend this model for MaxFragment handling

[geoffkizer]

   public string ServerIndicationName {get;}
   public string ProtocolId {get;}

I would suggest:
"ServerNameIndication" (or even just "ServerName")
"ApplicationProtocolName"

[geoffkizer]

   public X509Certificate ServerCertificate { get; set; }
   public IDictionary<string, X509Certificate> ServerCertificates { get; set; }

Based on above discussion, it sounded like we were moving toward a callback model here. Is that correct? What does this look like?

[geoffkizer]

    public SslProtocols? EnabledSslProtocols { get; set; }
    public EncryptionPolicy? EncryptionPolicy { get; set; }

These should not be nullable; they should just have appropriate defaults.

[geoffkizer]

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

We should define these explicitly as part of the ALPN proposal.

Do we actually need http2 without TLS? Are there any other defined protocol names we should include?

[geoffkizer]

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.

Here's my suggestion: Make these objects freeze-on-use.

[geoffkizer]

For options like RemoteCertificateValidationCallback, which I can specify today in the constructor, what happens if I pass something to the constructor but then leave this null on the options bag? Do we use the value from the options bag (null), or do we use the value from the constructor? Similarly for other constructor args.

[Drawaes]

Okay I am good with splitting SNI and ALPN to be separate to the options bag change. I will launch two sub issues for those and answer the specific questions there for those and discuss only the options bag concept here.

[Drawaes]

How does freeze on use manifest? Is there an internal flag and if you try to set something after that it throws? If so what's the exception appropriate here? What about if the certificates ends up being a list or dictionary? We have to freeze the list/dictionary as well.... how is that done? (I like the idea just not sure how it is implemented).

The other option is to have a hashcode or compare that checks the settings + the certs etc (child objects) and checks an internal cache. If it doesn't exist duplicate and put it in there. There is already an internal credentials cache.

You could ref count simply to remove items. Then on a client it wouldn't be much burden and on the server it rarely would drop to zero unless your server is not doing much in which case a class allocation or two won't matter?

Thoughts? I would rather sort out some details before updating the proposal again.

[stephentoub]

Why is it a problem to have the options bag be mutable? It's no different from anything else: you give this to the API, and the API expects it to be in a consistent state for the duration of the operation... if you change it while the operation is in flight, that's a bug, no different from any other case where you mutate something while a called method depends on it (e.g. the buffers passed to read/write). If you want to treat it as immutable and just never change it after it's created, you can. If you want to treat it as mutable such that you can modify it between uses, you can.

[Priya91]

@Drawaes The options bag IList<SslApplicationProtocol> property can be changed, but the SslApplicaitonProtocol struct protocol value is immutable. For now, we are assuming SslApplicationProtocol use lies only in the options bag, but this type could be used outside of options bag in other scenarios in user code.

[Drawaes]

@Priya91 I 100% understand the design, but it seems arbitrary that the ALPN is protected by immutability using copies, but not the list, or anything else in SslStream. So then we have arbitary immutability and inconsistancy, and we have an asymmetrical API that exposes a ReadOnlyMemory on the Protocol property but no way to use this to make a new one?

Anyway, if it's the design you all want, so be it.

[KrzysztofCwalina]

@Drawaes, we won't be exposing constant/pre-cooked options bags. We do want to have constant SslApplicationProtocols exposed.

@benaadams, I agree that ReadOnlySpan would be more flexible. I am not sure we need this flexibility. It will make the APIs harder to use (Span is less known than byte[], Encoding.UTF8.GetBytes returns array, etc.).

Having said that, I am not pushing for any design here (in fact I think it's a bit hair splitting and we should just let the developer responsible for this feature, i.e. @Priya91, make the call) . I was just commenting that the ctor must copy regardless what we pass in, and so the argument that we should use ReadOnlyMemory because it's readonly does not seem to be valid.

[Priya91]

and we have an asymmetrical API that exposes a ReadOnlyMemory on the Protocol property but no way to use this to make a new one?

I don't understand this statement, The readonlymemory can be used to make a new SslApplicationProtocol object, new SslApplicationProtocol(readonlymemory.toarray()).

[benaadams]

The readonlymemory can be used to make a new SslApplicationProtocol object, new SslApplicationProtocol(readonlymemory.toarray()).

It can. However you are taking an immutable item ReadOnlyMemory<byte> allocating to convert it into a mutable item byte[] and then passing the mutable item into the constructor, so it can copy it again and turn it into an immutable ReadOnlyMemory<byte> .

What I'm suggesting is taking the parameter as ReadOnlyMemory<byte> conveys the intent of the function correctly in a new way that we couldn't do previously with arrays. It says the function that takes this does not intend to modify it. Whereas a function taking an array suggests no such guarantee; the parameter may be changed for the callee after the call.

I'm not suggesting to remove the byte[] .ctor; as that would probably just confuse people; just to add an additional .ctor that can directly take the property the type exposes and create another one, rather than having to convert it to something else; or translate it first.

Also you are probably never actually going to do
new SslApplicationProtocol(proto.Protocol));
or
new SslApplicationProtocol(proto.Protocol.ToArray()));
As if you have the protocol why would you create an identical one; so mostly its that the api is not self consistent that bothers me. However its a minor point.

[benaadams]

However I do think interpreting a string input as UFT8 is problematic and likewise without a string .ctor you could easily do

new SslApplicationProtocol(Encoding.ASCII.GetBytes("MyProtocol"));
new SslApplicationProtocol(Encoding.UTF8.GetBytes("MyProtocol"));
new SslApplicationProtocol(Encoding.BigEndianUnicode.GetBytes("MyProtocol"));
new SslApplicationProtocol(Encoding.Unicode.GetBytes("MyProtocol"));
new SslApplicationProtocol(Encoding.UTF7.GetBytes("MyProtocol"));
new SslApplicationProtocol(Encoding.UTF32.GetBytes("MyProtocol"));
new SslApplicationProtocol(Encoding.GetEncoding("iso-8859-5").GetBytes("MyProtocol"));

Where it is explicit what the encoding is, as string in .NET is a 16-bit format; that can also quite happily store binary data and not a UFT8 format.

I suppose in the same vein you could do

byte[] protocol = new byte[str.Length * 2];
int i = 0;
foreach (char ch in str)
{
    protocol[i] = (byte)(ch & 0xFF);
    protocol[i+1] = (byte)((ch >> 8) & 0xFF);
    i += 2;
}
new SslApplicationProtocol(protocol);

To maintain string's natural format - I just think an api that takes string an automatically assumes a different output encoding is asking for trouble - is baking a conversion assumption into the api.

The issue with .NET apis as demonstrated by .NET Standard 2.0 (and amazing it is) - is that they can never be changed once they are live.

A string overload can always be added later if there is demand; however once there it can never be taken away or its behavior changed.

[benaadams]

With a ReadOnlyMemory .ctor + https://github.com/dotnet/corefx/issues/24293 then you'd be able to avoid the foreach char loop with the no-alloc

new SslApplicationProtocol(new ReadOnlyMemory<char>(str).AsBytes())

(ignoring defensive copy in .ctor)

[Priya91]

@benaadams What are the use-cases for someone to do this?

I'm not suggesting to remove the byte[] .ctor; as that would probably just confuse people; just to add an additional .ctor that can directly take the property the type exposes and create another one, rather than having to convert it to something else; or translate it first.

[Priya91]

Whether it is byte[] or Readonlymemory it has to be copied in the constructor, and a readonlymemory is just a byte[] that doesn't expose means to modify the underlying byte[]. So I don't see any extra value in readonlymemory in the constructor. I also don't understand the asymmetrical API point, what scenarios are affected by this?

Regarding the string constructor overload, it's provided for usability and convenience of using new alpn values that are not currently constants, or for custom pre-agreed new values that the clients and servers may use. It is not our goal to provide equality for an sslaplicationprotocol object created with another tostring() to match, that scenario is not a use case for this api. It is similar to how the encoding apis don't match with getstring and getbytes for incompatible encoding values.

[karelz]

We should converge the discussion on this issue and get to consensus, it is dragging for quite long :(.

@Tratcher public SslApplicationProtocol(string protocol) { } // assumes utf-8

What does it mean "utf-8"?
We have UTF-16 encoding of strings, so that is what we're going to receive. We will have to convert it internally to UTF-8 string and store in "byte[]" that we are passing into.
ToString will interpret underlying "byte[]" as UTF-8 string. If that fails, it will be string representation of the raw bytes, e.g. "0x68 0x74 0x74 0x70 0x2f 0x31 0x2e 0x31".

[Priya91]

This is the latest ALPN proposal from the discussion on this thread. Posting that here for the api-review team.

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) { }
}
}

[benaadams]

We have UTF-16 encoding of strings, so that is what we're going to receive. We will have to convert it internally to UTF-8 string and store in "byte[]" that we are passing into.

And its not conveyed in the api that this is a baked in auto-conversion e.g.

public SslApplicationProtocol(string protocol)
  : this(Encoding.UTF8.GetBytes(protocol))
{ }

Atm its at best a side comment in this Issue.

The spec does not say it must be a UTF-8 string interpretation - it says it could be

Identification Sequence: The precise set of octet values that identifies the protocol. This could be the UTF-8 encoding [RFC3629] of the protocol name.

[benaadams]

How common will it be that users will be making their own protocols so need a convenience constructor that does an opinionated conversion from string to UTF-8 bytes?

What subset of those users will not understand the Encoding apis so need help to do

var protocol0 = new SslApplicationProtocol(Encoding.UTF8.GetBytes(protocol))

Equally anyone straying into the early non-ascii with accents; will they go for the extra byte UTF8 creates for

// 13 bytes in UTF-8, 12 in Windows-1250/ISO-8859-2
var protocol0 = new SslApplicationProtocol("můj protokol");
// 22 bytes in UTF-8, 21 in Windows-1252/ ISO 8859-1
var protocol1 = new SslApplicationProtocol("doppelgängerprotokoll");
// 23 bytes in UTF-8, 12 in Windows-1251/ ISO 8859-5
var protocol2 = new SslApplicationProtocol("мой протокол");

Or will they prefer to use Latin 2 or Windows 1250/1251/1252 encodings from System.Text.Encoding.CodePages which saves a byte and each char is still 1 byte; whereas in UTF8 its 2 bytes?

I don't think the string overload is a good one - its trouble, especially with the pushback on using utf8 as a compact string representation from people using Latin character sets as they use their localized 8-byte format https://github.com/dotnet/coreclr/issues/7083

[benaadams]

All the current IANA ALPN Protocol Ids are ASCII, so there are no actual examples or precedent of UTF-8 in actual use:

"http/1.1"
"spdy/1"
"spdy/2"
"spdy/3"
"stun.turn"
"stun.nat-discovery"
"h2"
"h2c"
"webrtc"
"c-webrtc"
"ftp"
"imap"
"pop3"
"managesieve"

[Tratcher]

@karelz Yes I meant that the string would be converted to bytes using the utf-8 encoding.

This string overload is not a critical piece of the API and does not merit the amount of time already spent on it. It provides a slight improvement in usability over the other overload, that's all. Keep it or delete it, but either way it's time to move on.

[benaadams]

Whether it is byte[] or Readonlymemory it has to be copied in the constructor, and a readonlymemory is just a byte[] that doesn't expose means to modify the underlying byte[]. So I don't see any extra value in readonlymemory in the constructor.

Its a good point. As you say it needs to be copied anyway, so ReadOnlySpan<byte> would have more value than ReadOnlyMemory<byte> as it can also be stack memory - and you can always get one from a ReadOnlyMemory<byte>

[Drawaes]

Okay it's upto the dev implementing it and MS who has to support it in the end. Mostly I wanted an options bag and was moonshotting for a builder so I am pretty happy I got what I wanted. I will make one last comment on the constructor then someone at MS can make a decision and they are welcome to update the top issue.

So my final word.. as repeated often API design is forever. Why take a string and utf8 It? UTF8 strings might be around the corner anyway. What's the likelihood of someone actually needing the overload right now? It can be added later if needed you can always add you can't takeaway.

Second read-only memory .... arrays implicitly cast to memory anyway right ? So make it read-only memory and add an array if you need it. If the concern is familiarity well if you are making your own ALPN protocol I would hope a certain level of depth of knowledge anyways.

Anyway let me know what you want above.....

Incase It's not clear 1 read-only memory constructor only is my recommendation.

[karelz]

@benaadams The spec does not say it must be a UTF-8 string interpretation - it says it could be

Correct. The spec is intentionally unclear from unknown reasons, we believe that the UTF-8 is strongly hinted. It is our interpretation of the spec.
BTW: We discussed also ASCII. If anyone needs UTF-16 or other encodings, they can use directly byte[] overload.
Practically, we do not think anyone will use anything different than the IANA pre-defined ASCII/UTF-8 strings.

How common will it be that users will be making their own protocols

Users can use it for all the IANA protocols which we do not include as convenient constants. It is also result of a long internal discussion about using only string as the underlying data instead of byte[] (which is more feature proof). Adding string overload is convenience for usability.

This string overload is not a critical piece of the API and does not merit the amount of time already spent on it. ... either way it's time to move on.

Agreed 💯!

@Drawaes UTF8 strings might be around the corner anyway.

I don't think it is a good idea to make APIs dependent on each other. We don't know if/when UTF-8 strings get into the platform.
I view the convenience string API as way to pass ASCII (or UTF-8) string, as per spec guidance.

BTW: I will Update top post with copy & link to latest version of the API from @Priya91.

[benaadams]

The spec is intentionally unclear from unknown reasons, we believe that the UTF-8 is strongly hinted. It is our interpretation of the spec.

It isn't, it doesn't use a RFC2119 term (which the spec also references)

2. Requirements Language

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
"SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
document are to be interpreted as described in [RFC2119].

Its says "could" which is not one of these terms; rather than "RECOMMENDED" or even "MAY" - its more of an example usage - to clarify that even though its a binary field, text is probably a sensible option.

If you want to bake this conversion in; I would suggest being explicit in the api in some way e.g.

new SslApplicationProtocol(string uf8Protocol)

I think the people specifying their own protocols can cope with the onerous extra step of using an explicit Encoder for the string rather than having an ambiguous convince string overload .ctor to make it easier.

Anyway; I've raised my objections, made suggestions - won't object to the final decision.

[karelz]

API review: Approved as latest proposal. We discussed the alternatives suggested above, but didn't feel we should take them.

[Drawaes]

Okay cool, looking forward to it being in corefx