{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/corefx/issues/23177","repository_url":"https://api.github.com/repos/dotnet/corefx","labels_url":"https://api.github.com/repos/dotnet/corefx/issues/23177/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/corefx/issues/23177/comments","events_url":"https://api.github.com/repos/dotnet/corefx/issues/23177/events","html_url":"https://github.com/dotnet/corefx/issues/23177","id":249804954,"number":23177,"title":"Proposed SslStream additions for ALPN and SNI Take 2","user":{"login":"Drawaes","id":16406969,"avatar_url":"https://avatars2.githubusercontent.com/u/16406969?v=4","gravatar_id":"","url":"https://api.github.com/users/Drawaes","html_url":"https://github.com/Drawaes","followers_url":"https://api.github.com/users/Drawaes/followers","following_url":"https://api.github.com/users/Drawaes/following{/other_user}","gists_url":"https://api.github.com/users/Drawaes/gists{/gist_id}","starred_url":"https://api.github.com/users/Drawaes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Drawaes/subscriptions","organizations_url":"https://api.github.com/users/Drawaes/orgs","repos_url":"https://api.github.com/users/Drawaes/repos","events_url":"https://api.github.com/users/Drawaes/events{/privacy}","received_events_url":"https://api.github.com/users/Drawaes/received_events","type":"User","site_admin":false},"labels":[{"id":153714228,"url":"https://api.github.com/repos/dotnet/corefx/labels/api-needs-work","name":"api-needs-work","color":"207de5","default":false},{"id":497898777,"url":"https://api.github.com/repos/dotnet/corefx/labels/area-System.Net.Security","name":"area-System.Net.Security","color":"d4c5f9","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":{"url":"https://api.github.com/repos/dotnet/corefx/milestones/12","html_url":"https://github.com/dotnet/corefx/milestone/12","labels_url":"https://api.github.com/repos/dotnet/corefx/milestones/12/labels","id":2323293,"number":12,"title":"2.1.0","description":".NET Core 2.1, next major release. See [official release dates](https://github.com/dotnet/core/blob/master/roadmap.md) for details.","creator":{"login":"karelz","id":5776068,"avatar_url":"https://avatars2.githubusercontent.com/u/5776068?v=4","gravatar_id":"","url":"https://api.github.com/users/karelz","html_url":"https://github.com/karelz","followers_url":"https://api.github.com/users/karelz/followers","following_url":"https://api.github.com/users/karelz/following{/other_user}","gists_url":"https://api.github.com/users/karelz/gists{/gist_id}","starred_url":"https://api.github.com/users/karelz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/karelz/subscriptions","organizations_url":"https://api.github.com/users/karelz/orgs","repos_url":"https://api.github.com/users/karelz/repos","events_url":"https://api.github.com/users/karelz/events{/privacy}","received_events_url":"https://api.github.com/users/karelz/received_events","type":"User","site_admin":false},"open_issues":343,"closed_issues":2106,"state":"open","created_at":"2017-02-15T02:14:11Z","updated_at":"2017-08-31T21:41:56Z","due_on":null,"closed_at":null},"comments":158,"created_at":"2017-08-12T10:48:18Z","updated_at":"2017-08-31T22:36:55Z","closed_at":null,"author_association":"COLLABORATOR","body":"---\r\n**Updated = Removed results object to keep new objects down**\r\n---\r\nReferences #23077 \r\n# Rationale\r\n---\r\nServer 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). \r\n\r\nThese 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. \r\n\r\nThere 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. \r\n\r\n# Proposed API Change\r\n---\r\nOriginally I suggested overloading the Authenticate methods, however discussions in #23077 have changed my mind. Thanks to @Tratcher for this concept\r\n\r\n``` csharp\r\npublic partial class SslStream\r\n{\r\n   public Task AuthenticateAsServerAsync(ServerAuthenticationOptions options, CancellationToken cancellation) { }\r\n   public Task AuthenticateAsClientAsync(ClientAuthenticationOptions options, CancellationToken cancellation) { }\r\n    \r\n   public string ServerIndicationName {get;}\r\n   public string ProtocolId {get;}\r\n}\r\n\r\npublic class SharedAuthenticationOptions\r\n{\r\n    public SslProtocols? EnabledSslProtocols { get; set; }\r\n    public bool CheckCertificateRevocation { get; set; }\r\n    public IList<string> ApplicationProtocols { get; }\r\n    public RemoteCertificateValidationCallback RemoteCertificateValidationCallback { get; set; }\r\n    public EncryptionPolicy? EncryptionPolicy { get; set; }\r\n}\r\n\r\npublic class ServerAuthenticationOptions : SharedAuthenticationOptions\r\n{\r\n   public X509Certificate ServerCertificate { get; set; }\r\n   public IDictionary<string, X509Certificate> ServerCertificates { get; set; }\r\n   public bool ClientCertificateRequired { get; set; }\r\n}\r\n\r\npublic class ClientAuthenticationOptions : SharedAuthOptions\r\n{\r\n   public string TargetHost { get; set; }\r\n   public X509CertificateCollection ClientCertificates { get; set; }\r\n   public LocalCertificateSelectionCallback UserCertificateSelectionCallback { get; set; }\r\n   public int MaximumFragementSize {get;set;}\r\n}\r\n\r\n```\r\n\r\n# Further Notes\r\n---\r\n1. This will mean future options can be added without causing binary compatibility issues (increasing properties on concrete classes rather than changing overloads)\r\n1. 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 )\r\n1. I snuck max fragment in for the client, but I am happy to have this dropped if it is a sticking point\r\n1. Cancellation tokens are there for both methods as per current framework thinking\r\n1. 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\r\n1. 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.\r\n1. 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.\r\n\r\n# Potential Implementation Issues\r\n---\r\nThere 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.\r\n\r\n# Example Usage\r\n---\r\n\r\n```\r\nvar options = new ServerAuthenticationOptions()\r\n{\r\n    EnabledProtocols = SslProtocols.Tls12 | SslPRotocols.Tls11,\r\n    ApplicationProtocols = new List<string>()\r\n    {\r\n         \"h2\",\r\n         \"http1.1\"\r\n    }\r\n};\r\nvar secureStream = new SslStream(innerStream);\r\nvar result = await secureStream(options, token);\r\n```\r\n\r\n# References \r\n---\r\n#9608 SNI\r\n#4721 ALPN\r\n#23077 Previous API proposal\r\n[RFC 7301 ALPN](https://tools.ietf.org/rfc/rfc7301.txt)\r\n[RFC 3546 SNI and Max fragment](https://www.ietf.org/rfc/rfc3546.txt)\r\n[IANA ALPN Protocol Ids](https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids)\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/dotnet/corefx/issues/comments/326438792","html_url":"https://github.com/dotnet/corefx/issues/23177#issuecomment-326438792","issue_url":"https://api.github.com/repos/dotnet/corefx/issues/23177","id":326438792,"user":{"login":"wfurt","id":14356188,"avatar_url":"https://avatars2.githubusercontent.com/u/14356188?v=4","gravatar_id":"","url":"https://api.github.com/users/wfurt","html_url":"https://github.com/wfurt","followers_url":"https://api.github.com/users/wfurt/followers","following_url":"https://api.github.com/users/wfurt/following{/other_user}","gists_url":"https://api.github.com/users/wfurt/gists{/gist_id}","starred_url":"https://api.github.com/users/wfurt/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wfurt/subscriptions","organizations_url":"https://api.github.com/users/wfurt/orgs","repos_url":"https://api.github.com/users/wfurt/repos","events_url":"https://api.github.com/users/wfurt/events{/privacy}","received_events_url":"https://api.github.com/users/wfurt/received_events","type":"User","site_admin":false},"created_at":"2017-08-31T22:36:55Z","updated_at":"2017-08-31T22:36:55Z","author_association":"MEMBER","body":"I can put it on TODO list to investigate. And yes, I agree that would be much more viable. \r\nThe open question would be behavior on older versions. It would be nice IMHO if everything works, just ALPN would throw exception for unsupported platform. "}}
{"actor":{"display_login":"wfurt"}}