Add SNI Support #2425

Tratcher · 2018-03-23T23:20:36Z

#2357 Here's what Kestrel's SNI support might look like.
This is based on the initial CoreFx design in dotnet/corefx#28278.

shirhatti · 2018-03-24T00:24:26Z

What's the behavior for legacy clients that don't support SNI? I'm assuming since we fallback to using the ServerCertificate if specified or close the connection?

benaadams · 2018-03-24T00:44:32Z

/cc @Drawaes

Drawaes · 2018-03-24T00:53:32Z

To be fair it's not "legacy clients" SNI isn't required and if you go via an IP there is no SNI to compare.

The problem you have of course is if you are multi-homing you can't just use "certificate x" it might be the wrong website. So I think a default should be allowed but not required. It's a MAY include extension.

As an aside I wonder if the possible cipher list could be supplied if you are reading the client hello anyway. That way say a smaller and more secure ECDSA cert could be supplied and an RSA supplied if the client doesn't support it.

Tratcher · 2018-03-24T04:41:54Z

@Drawaes you'll have to ask on dotnet/corefx#28278 about exposing additional parameters. Right now they only give us a name.

@shirhatti The current SslStream PR will invoke the callback with a null name if SNI was not supplied by the client. The developer is free to return a default certificate to continue, or return null or throw to abort. ServerCertificate is always ignored if a selector is provided (SslStream makes them mutually exclusive).

Tratcher · 2018-03-24T04:44:23Z

src/Kestrel.Core/Internal/HttpsConnectionAdapter.cs

+                    var cert = selector(name);
+                    if (cert != null)
+                    {
+                        EnsureCertificateIsAllowedForServerAuth(cert);


@blowdart it's potentially quite expensive to validate EKUs on every new connection. Can we exclude that for the advanced SNI scenario?

Cache the result based on cert thumbprint?

@davidfowl what are the odds you'd allow registering and injecting a MemoryCache here? 😁

Kestrel.Core shouldn't depend on the memory cache.

davidfowl · 2018-03-24T08:14:57Z

src/Kestrel.Core/HttpsConnectionAdapterOptions.cs

+#else
+        {
+            get => null;
+            set => throw new PlatformNotSupportedException("This API requires targeting netcoreapp2.1");


I'm not a huge fan of this throwing. If we made another callback API that let you control the cert per connection what would that look like?

Updated. Now a callback will be allowed even for 2.0. The difference is that for 2.0 we'll always invoke it with a null name before the handshake. The developer won't be able to distinguish between the client and server failing to provide SNI.

krwq · 2018-03-26T19:19:07Z

If server name could not be extracted or is not supported the callback will pass null string

halter73 · 2018-03-27T23:11:45Z

src/Kestrel.Core/Internal/HttpsConnectionAdapter.cs

+                var serverCert = _serverCertificate;
+                if (_serverCertificateSelector != null)
+                {
+                    serverCert = _serverCertificateSelector(null, null);


Do you think we should null check serverCert to get better errors when ServerCertificateSelector is not null but its return value is?

halter73 · 2018-03-27T23:16:07Z

src/Kestrel.Core/Internal/HttpsConnectionAdapter.cs

+            // If a selector is provided then ignore the cert, it may be a default cert.
+            if (selector != null)
+            {
+                // SslStream doesn't allow both.


I'm thinking it should be up to the caller of the HttpsConnectionAdapter ctor to ensure either ServerCertificateSelector or ServerCertificate is set but not both. I think it's better to throw if both are set instead of committing to this behavior of preferring the selector.

In UseHttps, is ServerCertificate already set by the time the config callback is called?

The default certs mess that up. See #2422

halter73 · 2018-03-27T23:19:58Z

src/Kestrel.Core/Internal/HttpsConnectionAdapter.cs

            {
-                throw new ArgumentException(CoreStrings.ServiceCertificateRequired, nameof(options));


lol the good 'ol service certificate.

Tratcher · 2018-03-28T22:08:44Z

src/Kestrel.Core/HttpsConnectionAdapterOptions.cs

+        /// If the server certificate has an Extended Key Usage extension, the usages must include Server Authentication (OID 1.3.6.1.5.5.7.3.1).
+        /// </para>
+        /// </summary>
+        public Func<string, X509Certificate2> ServerCertificateSelector { get; set; }


Consider passing through a state object, either a Kestrel specific object or the SslStream itself. That's what SslStream passes to their callback (as an Object).

So more of a "sender" than a state object? I don't think a state object is necessary since any closures would be allocated only once for the lifetime of the server.

Yes, a sender. Is there any connection specific state we think would be interesting to expose to this callback?

Why not just send the SslStream any properties are likely to be exposed on that in the future and means you won't have to change the kestrel side for every change.

SslStream is largely uninitialized at this stage in the handshake. See dotnet/corefx#28278 (comment)

I agree that is the way it is today. But as we have client hello parsing started it's just a backdoor to other parts being parsed and populated ;) (don't tell anyone) and if these features are populated then there will be no lightup required in kestrel.

Maybe the ConnectionContext would be useful.

Updated to pass through the sender. Trying to pass though per-connection state requires allocating a per-connection closure. We'll follow up in RC to see if that makes sense.

Not that we have to do this now, but a per-connection closure allocation isn't a big deal in the context of initializing an HTTPS connection in Kestrel right now.

Yeah, I realized that looking at the client cert validation code. I added it back for now.

Tratcher · 2018-04-03T18:56:51Z

Everything appears to be working in manual and automated tests. There are a few unrelated flaky test failures. @halter73 ready for a final review.

The last bit is figuring out what CoreFx build to target. @pakrym is waiting for the next prodcon build before updating universe.

davidfowl · 2018-04-03T19:16:21Z

These aren't flaky tests, these look like regressions:

11:53:27 Log Critical[0]: Unexpected exception in HttpConnection.ProcessRequestsAsync. System.ObjectDisposedException: Cannot access a disposed object.
11:53:27 Object name: 'MemoryPoolBlock'.
11:53:27    at System.Buffers.MemoryPoolBlock.Dispose()
11:53:27    at System.IO.Pipelines.BufferSegment.ResetMemory()
11:53:27    at System.IO.Pipelines.Pipe.CompletePipe()
11:53:27    at System.IO.Pipelines.Pipe.CompleteWriter(Exception exception)
11:53:27    at System.IO.Pipelines.Pipe.DefaultPipeWriter.Complete(Exception exception)
11:53:27    at Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal.AdaptedPipeline.<ReadInputAsync>d__16.MoveNext()
11:53:27 --- End of stack trace from previous location where exception was thrown ---
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
11:53:27    at Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal.AdaptedPipeline.<RunAsync>d__14.MoveNext()
11:53:27 --- End of stack trace from previous location where exception was thrown ---
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task task)
11:53:27    at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.HttpConnection.<ProcessRequestsAsync>d__47`1.MoveNext()
11:53:27 Log Critical[0]: ConnectionDispatcher.Execute() 0HLCPIGNNCR8B System.Exception: Unexpected critical error. Log Critical[0]: Unexpected exception in HttpConnection.ProcessRequestsAsync. System.ObjectDisposedException: Cannot access a disposed object.
11:53:27 Object name: 'MemoryPoolBlock'.
11:53:27    at System.Buffers.MemoryPoolBlock.Dispose()
11:53:27    at System.IO.Pipelines.BufferSegment.ResetMemory()
11:53:27    at System.IO.Pipelines.Pipe.CompletePipe()
11:53:27    at System.IO.Pipelines.Pipe.CompleteWriter(Exception exception)
11:53:27    at System.IO.Pipelines.Pipe.DefaultPipeWriter.Complete(Exception exception)
11:53:27    at Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal.AdaptedPipeline.<ReadInputAsync>d__16.MoveNext()
11:53:27 --- End of stack trace from previous location where exception was thrown ---
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
11:53:27    at Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal.AdaptedPipeline.<RunAsync>d__14.MoveNext()
11:53:27 --- End of stack trace from previous location where exception was thrown ---
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task task)
11:53:27    at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.HttpConnection.<ProcessRequestsAsync>d__47`1.MoveNext() ---> System.ObjectDisposedException: Cannot access a disposed object.
11:53:27 Object name: 'MemoryPoolBlock'.
11:53:27    at System.Buffers.MemoryPoolBlock.Dispose()
11:53:27    at System.IO.Pipelines.BufferSegment.ResetMemory()
11:53:27    at System.IO.Pipelines.Pipe.CompletePipe()
11:53:27    at System.IO.Pipelines.Pipe.CompleteWriter(Exception exception)
11:53:27    at System.IO.Pipelines.Pipe.DefaultPipeWriter.Complete(Exception exception)
11:53:27    at Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal.AdaptedPipeline.<ReadInputAsync>d__16.MoveNext()
11:53:27 --- End of stack trace from previous location where exception was thrown ---
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
11:53:27    at Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal.AdaptedPipeline.<RunAsync>d__14.MoveNext()
11:53:27 --- End of stack trace from previous location where exception was thrown ---
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task task)
11:53:27    at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.HttpConnection.<ProcessRequestsAsync>d__47`1.MoveNext()
11:53:27    --- End of inner exception stack trace ---
11:53:27    at Microsoft.AspNetCore.Testing.TestApplicationErrorLogger.Log[TState](LogLevel logLevel, EventId eventId, TState state, Exception exception, Func`3 formatter)
11:53:27    at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.KestrelTrace.Log[TState](LogLevel logLevel, EventId eventId, TState state, Exception exception, Func`3 formatter)
11:53:27    at Microsoft.Extensions.Logging.LoggerExtensions.LogCritical(ILogger logger, EventId eventId, Exception exception, String message, Object[] args)
11:53:27    at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.HttpConnection.<ProcessRequestsAsync>d__47`1.MoveNext()
11:53:27 --- End of stack trace from previous location where exception was thrown ---
11:53:27    at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
11:53:27    at

Tratcher · 2018-04-03T19:17:50Z

@pakrym this is the stuff you're updating right? Is this a matter of mismatched dependencies?

pakrym · 2018-04-03T19:23:06Z

The update is finished in release/2.1. These failures might be an actual issue.

halter73 · 2018-04-03T21:43:29Z

samples/SampleApp/Startup.cs

+                            var localhostCert = CertificateLoader.LoadFromStoreCert("localhost", "My", StoreLocation.CurrentUser, allowInvalid: true);
+                            httpsOptions.ServerCertificateSelector = (features, name) =>
+                            {
+                                // TODO: Name check, multiple certs, null names.


Is this something we actually want to do in this sample?

No plans to do this in the sample, only showing where to do it. I'll remove the TODO.

halter73 · 2018-04-03T21:43:59Z

src/Kestrel.Core/HttpsConnectionAdapterOptions.cs

@@ -29,14 +30,25 @@ public HttpsConnectionAdapterOptions()

        /// <summary>
        /// <para>
-        /// Specifies the server certificate used to authenticate HTTPS connections.
+        /// Specifies the server certificate used to authenticate HTTPS connections. This is ignored if ServerCertificateSelector is set.


I wish we didn't have to set an order of precedence. Are there any alternatives?

There are several layers that can assign default certs, so the best option for now is to ignore it if a selector is provided. See #2422

halter73 · 2018-04-03T21:46:36Z

samples/SampleApp/Startup.cs

+                        listenOptions.UseHttps(httpsOptions =>
+                        {
+                            var localhostCert = CertificateLoader.LoadFromStoreCert("localhost", "My", StoreLocation.CurrentUser, allowInvalid: true);
+                            httpsOptions.ServerCertificateSelector = (features, name) =>


I'm not sure I like this just being a settable property. What about adding a RegisterCertificateSelectorCallback() method that takes a state object as well?

Offline note: this is per endpoint, not per connection. No state is required to reduce allocations.

Tratcher added the blocked Blocked label Mar 23, 2018

Tratcher added this to the 2.1.0-preview2 milestone Mar 23, 2018

Tratcher self-assigned this Mar 23, 2018

Tratcher requested review from halter73, davidfowl and shirhatti March 23, 2018 23:20

Tratcher commented Mar 24, 2018

View reviewed changes

davidfowl reviewed Mar 24, 2018

View reviewed changes

Tratcher force-pushed the tratcher/sni branch from f2818b8 to 1afa20f Compare March 26, 2018 17:16

halter73 reviewed Mar 27, 2018

View reviewed changes

Tratcher commented Mar 28, 2018

View reviewed changes

Tratcher force-pushed the tratcher/sni branch 3 times, most recently from f4ad8ad to 9c8095b Compare April 3, 2018 18:49

Tratcher changed the title ~~[WIP] Outline SNI APIs~~ Outline SNI APIs Apr 3, 2018

Tratcher changed the title ~~Outline SNI APIs~~ Add SNI Support Apr 3, 2018

halter73 reviewed Apr 3, 2018

View reviewed changes

Add SNI support #2357

9ea2c50

Tratcher force-pushed the tratcher/sni branch from 9c8095b to 9ea2c50 Compare April 4, 2018 19:49

Tratcher merged commit 9ea2c50 into release/2.1 Apr 4, 2018

Tratcher deleted the tratcher/sni branch April 4, 2018 20:43

Tratcher removed the blocked Blocked label Apr 5, 2018

		{
		throw new ArgumentException(CoreStrings.ServiceCertificateRequired, nameof(options));

Add SNI Support #2425

Add SNI Support #2425

Uh oh!

Conversation

Tratcher commented Mar 23, 2018

Uh oh!

shirhatti commented Mar 24, 2018

Uh oh!

benaadams commented Mar 24, 2018

Uh oh!

Drawaes commented Mar 24, 2018

Uh oh!

Tratcher commented Mar 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tratcher Mar 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krwq commented Mar 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Apr 3, 2018

Uh oh!

davidfowl commented Apr 3, 2018

Uh oh!

Tratcher commented Apr 3, 2018

Uh oh!

pakrym commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Tratcher commented Mar 24, 2018 •

edited

Loading

Tratcher Mar 24, 2018 •

edited

Loading

Tratcher Apr 3, 2018 •

edited

Loading

halter73 Apr 3, 2018 •

edited

Loading

Tratcher Apr 3, 2018 •

edited

Loading