Add ALPN support to System.Net.Security.SslStream API

[jubarat]

Rationale

In order to initiate HTTP/2 connection over TLS, application protocol must be negotiated (References: rfc7540 section-3.3,rfc7301). ALPN support is available in SChannel for Windows Server 2012 R2, Windows 8.1 or later. Support in SslStream is a popular feature request: See here and #15077.

Client and server applications need a way to specify their supported application protocol list and be able to query the negotiated protocol.

Proposed API change

    public partial class SslStream : AuthenticatedStream
    {
        ...
        public virtual void AuthenticateAsClient(string targetHost, System.Security.Cryptography.X509Certificates.X509CertificateCollection clientCertificates, System.Security.Authentication.SslProtocols enabledSslProtocols, bool checkCertificateRevocation, string[] applicationProtocols);
        public virtual System.Threading.Tasks.Task AuthenticateAsClientAsync(string targetHost, System.Security.Cryptography.X509Certificates.X509CertificateCollection clientCertificates, System.Security.Authentication.SslProtocols enabledSslProtocols, bool checkCertificateRevocation, string[] applicationProtocols);
        public virtual void AuthenticateAsServer(System.Security.Cryptography.X509Certificates.X509Certificate serverCertificate, bool clientCertificateRequired, System.Security.Authentication.SslProtocols enabledSslProtocols, bool checkCertificateRevocation, string[] applicationProtocols);
        public virtual System.Threading.Tasks.Task AuthenticateAsServerAsync(System.Security.Cryptography.X509Certificates.X509Certificate serverCertificate, bool clientCertificateRequired, System.Security.Authentication.SslProtocols enabledSslProtocols, bool checkCertificateRevocation, string[] applicationProtocols);
        public virtual string NegotiatedApplicationProtocol;
    }

(New method overloads of AuthenticateAsClient/AuthenticateAsServer (+their async versions) are being added having new parameter string[] applicationProtocols. New property string NegotiatedApplicationProtocol is added as well).

Full view of SslStream API containing the proposed changes is in PR.

Example usage

Client:

SslStream sslStream = new SslStream(serverStream);
string[] applicationProtocols = new[] { "h2", "http/1.1" };
sslStream.AuthenticateAsClient(certificate.Subject, new X509CertificateCollection(), SslProtocols.Tls12, false, applicationProtocols);
string protocol = sslStream.NegotiatedApplicationProtocol;

Server:

SslStream sslStream = new SslStream(serverStream)
string[] applicationProtocols = new[] { "h2", "http/1.1" };
sslStream.AuthenticateAsServer(certificate, false, SslProtocols.Tls12, false, applicationProtocols);
string protocol = sslStream.NegotiatedApplicationProtocol;

Details

Client advertises list of protocols in descending order of preference. After the TLS handshake is complete the client queries the TLS stack for negotiated application protocol.

Similar way server side application provides its own list of supported protocols (again in descending order of preference). The TLS stack makes the application protocol match internally. After the handshake completes the server application queries the negotiated application protocol. Note: This behavior is supported by SChannel and is compatible with OpenSSL ALPN API. Unlike OpenSSL the SChannel does not expose possibility of querying client's advertised list of protocols therefore this API proposal does not contain server side protocol selecting based on callback.

AuthenticateAsClient/AuthenticateAsServer methods

• null value passed to applicationProtocols parameter means the caller is not requesting application protocol negotiation. This is also the behavior of existing method overloads.
• If both client and server specified their application protocol list and there was no match then AuthenticateAsServer throws AuthenticationException with inner exception having OS native error code. At this point the TLS handshake is aborted so the client gets IOException due to server application closing the transport connection.
• An alternative approach how to pass application protocol list instead of AuthenticateAs… methods could be as parameter of SslStream constructor.
• In following cases ArgumentException is thrown:
  • Empty protocol list.
  • Any element of protocol list is null.
  • Any element has more than 255 characters.
  • The full protocol list serialized size is more than 32767 bytes.

NegotiatedApplicationProtocol property

• If successfully negotiated then it returns the negotiated application protocol string.
• If peer did not advertise ALPN TLS extension or applicationProtocols was null then this property returns null.
• Throws InvalidOperationException if calling before AuthenticateAsClient or AuthenticateAsServer completes.

Examples of API behavior are in functional tests of PR.

Pull request

dotnet/corefx#4685

[davidsh]

cc: @CIPop @SidharthNabar @stephentoub @terrajobst

[davidsh]

ref: #15077

[joshfree]

@davidsh what additional changes are needed before we review the API proposal? Granted I expect changes to come from the review itself.

/cc @terrajobst @KrzysztofCwalina

[davidsh]

cc: @ericstj

The proposed additions to the SslStream class can NOT be done as currently proposed. It will break unification with Desktop.

We either have to do it as extension methods and then still probably have to do it as a separate contract. In fact, part of the API proposal involves adding a virtual property (which I don’t understand why we would do that as virtual). But, in general, dding properties cannot be represented like extension methods since there are no extension properties.

So, this API proposal will need to be revised.

[terrajobst]

@jubarat, thanks for proposal! I second @davidsh's concerns around being able to bring them to desktop.

Could either of you comment on whether they have to be virtual? Would it be possible to come up with a design that could provide the functionality on top, e.g. by providing a new type that wraps or inherits SslStream or by providing extension methods?

[jubarat]

The reason why I made the property virtual because all existing properties of SslStream except TransportContext are already virtual. I can guess only why they were designed virtual - possibly to allow inheriting from SslStream and customize all aspects of it. This feature does not need virtual property for itself so I can remove it.

I think inheritance from user perspective would not look too good for this feature since it feels like adding a new feature/extension to existing class rather having a new class with is a relationship. Also users would need to construct an inherited class instead of SslStream. Do my concerns make sense ?

I would like to know more about the Desktop unification especially what are the implications of adding new methods to existing API classes in corefx. Is there any document about this topic ?

[stephentoub]

@terrajobst, the milestone for this was changed to Future, and there's a PR open related to it. Should the PR be closed? Left open?

[karelz]

@CIPop has some work done in the space as well. We need to reconcile it with SNI work and figure out how to push it forward. It needs non-trivial work to even figure out the plan.

[ThatRendle]

Suggestion: if there are issues with breaking unification with Desktop by changing an existing class, that might make this a good opportunity to introduce a completely new set of classes using the more-correct Tls prefix instead of Ssl.

[CIPop]

Tls prefix instead of Ssl

I'd be all for that but I don't think it will be OK with our API review board. Unfortunately we're kind of stuck with SslStream doing TLS...

[karelz]

Note that renaming APIs just because we can, could easily lead to gazillion of APIs where no one understands which one is the recommended. Decisions like that should be the last resort, after we exhausted all other options. Sometimes, it is the right thing to do, we just have to be careful to think it through. Once you ship it in Framework, you live it it for decade(s) -- that's our key learning from shipping .NET Framework (Desktop).

That said, this particular issue is troublesome and it is not the best example of API we handled the right way (especially given we got contributor @jubarat interested in it who submitted PR 1.5 years ago and we failed to do our part :( - I think this is probably one of the worst case in CoreFX repo :().
It makes me sad and a little bit embarrassed. On the other hand, there is hope, as I believe we're doing much better now, especially in the last 6 months, not missing on contributions like this one.
We want to deliver this work (we have even internal team pushing on it), we just have higher priorities to finish (2.0/2.1 in general). Also note that Networking area was historically under lots of pressure and we didn't scale well - which is luckily changing with our recent additional funding and hires on the team! (yay!)
If there is a contributor interested in helping out with this issue, I promise we will not mishandle it as we did the original PR. I am more than happy to push on the API review, and babysit overall smoother ride -- just let me know if that's the case.

Thank you and please accept our sincere apologies for mishandling this issue (apologies go especially to @jubarat). We were young in the open-source world back then, and this issue slipped through :( ... we will learn from our mistake and do better in future.

[ThatRendle]

@karelz Thank you for the response. Rather than feeling sad or embarrassed, you should give yourselves credit for the massive transformation you have achieved in Microsoft's approach to open source. Developing something as huge as a .NET implementation in the open, on GitHub, is an enormous undertaking, and I know that your teams are not as well-staffed as many people assume they are.

I appreciate that this will not make it into the 2.0 release, but maybe it could be added to the 2.1 milestone, so that HTTP/2 support can be added to Kestrel by the end of the year?

[karelz]

Thanks for your kind words. It is encouraging that community and customers appreciate our work, thank you!

2.1 milestone is currently tracking only work for .NET Standard 2.0 for UWP. We will likely add there work based on feedback (i.e. more customer complaints, blockers, etc.). However, at this point, I prefer to have things in 2.0, or in Future, unlesss they are UWP related - in which case 2.1 is fine.
This particular thing is on top of our Networking backlog (the 'wishlist' label also captures that), so the chances are, we will push it into 2.1, but I don't want to promise it yet. Moreover, I can't promise it on behalf of the entire Networking v-team ... it needs to be discussed ;-)

Also, to be super-honest: Networking v-team is overbooked for both 2.0 and 2.1 (despite the fact we had 1 person joining us few weeks ago and 2 more are joining by early May - incl. our new Linux Networking expert).
There's just TONS of things to do - port APIs, reliability, perf, new APIs, long-term vision for high-performance networking stack, etc.
Also we took shortcuts in porting APIs from Desktop (because of time constraints, while taking usage numbers into account), so we expect we might get some feedback after we release 2.0 and we might need to reprioritize some of that work back into 2.1 to unblock more scenarios.
Taking all that into account, I am not comfortable to say at this moment we will make it happen in 2.1 without external contribution. We try to make milestones 95% accurate, so I don't want to add it into 2.1. I think keeping it in Future has better chance to raise concerns from community and customers about the timelines and help us collect even more feedback how much blocking the work is ... which is something that could potentially lead to reprioritization and making commitment in 2.1 (the more feedback and votes, the better chance).

[justcoding121]

Not to Hijack this. For those who end up here and can't wait to for support for long waiting Server Name Indication (SNI) and ALPN support for SslStream we found a hack to support both. Should work cross platform, we do it by Peeking and modifying underlying network stream that SslStream uses. Published in below project.

https://github.com/justcoding121/StreamExtended

PS: ALPN is not supported

[cesarblum]

@justcoding121 Have you tested it end-to-end? We tried a similar approach in Kestrel thinking we could just inject the ALPN extension in the response, but that causes the TLS handshake to fail because it affects the message hashes that the client and the server compare at the end of the handshake.

[justcoding121]

@CesarBS I think I opened the bottle before the party :-) I was investigating why it failed and should be the very same reason.

[aidapsibr]

Dropping a link to the user-voice as well: https://visualstudio.uservoice.com/forums/121579-visual-studio-ide/suggestions/6264363-add-support-for-alpn-to-system-net-security-sslstr

[Tratcher]

Adding yet more overloads of AuthenticateAsClient/Server seems excessive, especially with all the possible parameter permutations. The proposed string[] applicationProtocols parameter is symmetrical on the client and server APIs so why not make it a property configured prior to Authenticate instead?

        public virtual IList<string> ApplicationProtocols { get; }
        public virtual string NegotiatedApplicationProtocol { get; }

Usage:

SslStream sslStream = new SslStream(serverStream);
sslStream.ApplicationProtocols.Add("h2");
sslStream.ApplicationProtocols.Add("http/1.1");
sslStream.AuthenticateAsClient/Server/Async(...)
string protocol = sslStream.NegotiatedApplicationProtocol;

[CIPop]

why not make it a property configured prior to Authenticate instead

I agree with the property idea. I also think it should be somehow symmetric with the SNI server side API (#9608).
We should design the two API surfaces together.

Updated: we're already doing that here: dotnet/corefx#23077

[geoffkizer]

See latest proposal at #23157