Add "zero config" HTTPS support using local development certificate. #2093

cesarblum · 2017-10-03T18:03:45Z

This change enables automatic HTTPS using a development certificate found in the user store. Any URLs containing the https scheme will be configured with that certificate.

Paging @Eilon since this is a breaking change (previously we would throw upon seeing an https URL).

So now if you set ASPNETCORE_URLS (or use UseUrls()) to e.g. https://localhost:5001, you'll get HTTPS if you add Kestrel with no listen options i.e.

var host = new WebHostBuilder()
    ...
    .UseKestrel()
    ...

This turned out to be more convoluted than I anticipated. I'm hoping someone can suggest a simpler way to achieve the same result.

javiercn · 2017-10-03T18:50:52Z

src/Kestrel/DefaultHttpsProvider.cs

+                store.Open(OpenFlags.ReadOnly);
+
+                var certificates = store.Certificates.OfType<X509Certificate2>();
+                var certificate = certificates


You need to check that it has an OID, a private key and hasn't expired

When do you think you will have dotnet/extensions#263 merged? We shouldn't replicate all that logic here. I should be able to get the certificate by calling CertificateManager.ListCertificates(CertificatePurpose.HTTPS, StoreName.My, StoreLocation.CurrentUser, isValid: true), right?

I'm working on it

javiercn · 2017-10-03T18:55:05Z

Paging @Eilon since this is a breaking change (previously we would throw upon seeing an https URL).

I don't see this as a breaking change. At most a behavior change, and you are weakening the precondition (allowing something that wasn't allowed before) so I don't think this is a breaking change.

muratg · 2017-10-03T19:22:52Z

To me this looks like a minor breaking change, but an acceptable one. If you specify https for your URL, your app would fail to start before this. Now it'll do what you (presumably) wanted anyway.

halter73

Is there a guide on how to setup an X509Store for my current user so I can test this out locally?

halter73 · 2017-10-03T19:59:13Z

src/Kestrel.Core/KestrelServer.cs

@@ -52,6 +54,8 @@ internal KestrelServer(ITransportFactory transportFactory, ServiceContext servic
            Features = new FeatureCollection();
            _serverAddresses = new ServerAddressesFeature();
            Features.Set(_serverAddresses);
+
+            _defaultHttpsProvider = serviceContext.ServerOptions.ApplicationServices.GetService<IDefaultHttpsProvider>();


Don't call GetService for this. Just make IDefaultHttpsProvider a param to the public ctor.

halter73 · 2017-10-03T20:01:41Z

src/Kestrel.Core/Internal/AddressBinder.cs

@@ -19,7 +19,9 @@ internal class AddressBinder
    {
        public static async Task BindAsync(IServerAddressesFeature addresses,
            List<ListenOptions> listenOptions,
+            KestrelServerOptions serverOptions,


It doesn't look like this is actually used by the DefaultHttpsProvider or anything else. I see that you are setting this on ListenOptions before calling ConfigureHttps, but I don't see why that's necessary. Save some lines of code and don't bother plumbing this through here.

It blows up here:

KestrelHttpServer/src/Kestrel.Https/ListenOptionsHttpsExtensions.cs

Line 89 in 83e2084

var loggerFactory = listenOptions.KestrelServerOptions.ApplicationServices.GetRequiredService<ILoggerFactory>();

I may have misunderstood what you said. There's indeed no need to pass it as an arg to BindAsync, since I can get the KestrelServerOptions instance from one of the ListenOptions in the list. I still need that object though, otherwise a NRE is thrown at the line above.

You're right. I forgot about the call to UseHttps().

cesarblum · 2017-10-03T22:03:45Z

@halter73 To get the test cert set up on your machine, clone this PR dotnet/extensions#263 and run the tests. @javiercn can give you more info.

cesarblum · 2017-10-03T23:30:33Z

Injecting IDefaultHttpsProvider in the KestrelServer ctor is a breaking change - I had to add an exception for ApiCheck. However, given these are injected parameters, is that an acceptable breaking change for a minor version? @muratg @Eilon

halter73 · 2017-10-03T23:47:08Z

@CesarBS Couldn't you just add a second ctor, and have the original ctor call the new one with a null IDefaultHttpsProvider? Of course, you'd want to keep the old behavior if IDefaultHttpsProvider is null.

Eilon · 2017-10-04T03:38:51Z

Yeah just do what @halter73 suggested and there's no breaking change at all in terms of source compat, binary compat, runtime behavior, etc. It's nearly always fine to throw fewer exceptions and be more permissive.

davidfowl · 2017-10-04T03:43:30Z

src/Kestrel.Core/Internal/AddressBinder.cs

@@ -31,7 +32,9 @@ internal class AddressBinder
            {
                Addresses = addresses.Addresses,
                ListenOptions = listenOptions,
+                ServerOptions = listenOptions.FirstOrDefault()?.KestrelServerOptions,


This is pretty gross

Tratcher · 2017-10-04T19:10:44Z

New/updated sample?

Tratcher · 2017-10-04T19:30:01Z

src/Kestrel.Core/Internal/AddressBinder.cs

+                if (https)
+                {
+                    options.KestrelServerOptions = context.ServerOptions;
+                    context.DefaultHttpsProvider?.ConfigureHttps(options);


We want this to throw if DefaultHttpsProvider is null, no? You asked for https and you're not getting it.

Tratcher · 2017-10-04T19:33:11Z

src/Kestrel/DefaultHttpsProvider.cs

+
+                if (certificate == null)
+                {
+                    throw new InvalidOperationException("Unable to find ASP.NET Core development certificate.");


This level of magic deserves an FW link explaining what to do when it fails.

@danroth27 @javiercn Do we have an FW link already?

Eilon

Looks good regarding breaking changes (that there aren't any). Had one unrelated comment.

Eilon · 2017-10-04T19:38:18Z

src/Kestrel/DefaultHttpsProvider.cs

+
+                var certificates = store.Certificates.OfType<X509Certificate2>();
+                var certificate = certificates
+                    .FirstOrDefault(c => HasOid(c, AspNetHttpsOid) && !IsExpired(c) /*&& HasPrivateKey(c)*/);


Is the commented code needed?

Yes. Thanks for catching that, I forgot I had commented it out while testing something.

cesarblum · 2017-10-09T23:44:34Z

Updated to use https://github.com/aspnet/Common/blob/b9bc076c782af4a9664b34b561fd91d37e6edda4/shared/Microsoft.AspNetCore.Certificates.Generation.Sources/CertificateManager.cs to find development certificate.

davidfowl · 2017-10-10T05:52:00Z

src/Kestrel/Kestrel.csproj

@@ -12,10 +12,13 @@

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.Hosting" />
+    <PackageReference Include="Microsoft.AspNetCore.Certificates.Generation.Sources" PrivateAssets="All" />
+    <PackageReference Include="System.Security.Cryptography.Cng" />


What's this for?

He’s using the same code we have in tooling to locate the certificate (that’s why it’s a shared source library)

We need that reference for the RSACng type.

cesarblum · 2017-10-12T19:17:09Z

Rebased.

analogrelay

If you don't have time to do this before you leave, I can take it over. One small testing comment and that's it.

analogrelay · 2017-10-13T03:47:58Z

test/Kestrel.Core.Tests/KestrelServerTests.cs

-                    $"HTTPS endpoints can only be configured using {nameof(KestrelServerOptions)}.{nameof(KestrelServerOptions.Listen)}().",
-                    exception.Message);
-                Assert.Equal(1, testLogger.CriticalErrorsLogged);
+                mockDefaultHttpsProvider.Verify(provider => provider.ConfigureHttps(It.IsAny<ListenOptions>()), Times.Once);


Not a fan of tests that verify that a certain method was called. Is there a way to build a simple dummy DefaultHttpsProvider and then verify the certificate on the ListenOptions?

What is the issue with this kind of test? To test with DefaultHttpsProvider, I'd have to make the test project depend on the Kestrel project, instead of just Kestrel.Core.

Hmm. I'll take a bit of a look at it and see if we can make something work. Tests that use mock verification tend to be brittle whenever you change the implementation and they aren't actually testing the desired behavior of the function, they're just testing that the code does what is written. It's a little like a comment over x += 1 that says // Adds 1 to x :). If that's all that we can do, then I suppose it's OK, but I'd like to try and avoid it.

…Binder.

analogrelay · 2017-10-24T19:18:03Z

🆙 📅 Using CertificateManager shared-source package now. Also, added some more logging and exception messages.

javiercn · 2017-10-24T19:47:07Z

src/Kestrel/KestrelStrings.resx

+    <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
+  </resheader>
+  <data name="HttpsUrlProvidedButNoDevelopmentCertificateFound" xml:space="preserve">
+    <value>An 'https' URL was provided, but a development certificate could not be found.</value>


Can you add something along the lines of the text that I sent you on email and add the forwarding link that @danroth27 provided?

javiercn

Everything looks good AFAIK. But we need to get a more descriptive error message when the certificate is not present.
We need to mention what gesture to perform from the command line and a forwarding link to the documentation.

halter73 · 2017-10-24T19:53:21Z

test/Kestrel.Core.Tests/KestrelServerTests.cs

+        [Fact]
+        public void KestrelServerThrowsUsefulExceptionIfDefaultHttpsProviderNotAdded()
+        {
+            var mockDefaultHttpsProvider = new Mock<IDefaultHttpsProvider>();


Nit: It doesn't look like mockDefaultHttpsProvider is used in this or the following two test methods.

yep, copy🍝

halter73 · 2017-10-24T19:57:23Z

test/Kestrel.Core.Tests/KestrelServerTests.cs

@@ -274,6 +315,11 @@ private static KestrelServer CreateServer(KestrelServerOptions options, ILogger
            return new KestrelServer(Options.Create(options), new MockTransportFactory(), new LoggerFactory(new[] { new KestrelTestLoggerProvider(testLogger) }));
        }

+        private static KestrelServer CreateServer(KestrelServerOptions options, IDefaultHttpsProvider defaultHttpsProvider)
+        {
+            return new KestrelServer(Options.Create(options), new MockTransportFactory(), new LoggerFactory(new[] { new KestrelTestLoggerProvider(Mock.Of<ILogger>()) }), defaultHttpsProvider);


Just use the default ctor for KestrelTestLoggerProvider. It makes it easier to see tracing when running random tests and should fail test runs if a critical error is logged through the use of TestApplicationErrorLogger.

Makes sense

Well. After changing that I'm now getting this error:

Failed KestrelServerThrowsUsefulExceptionIfDefaultHttpsProviderNotAdded Error Message: Assert.Throws() Failure Expected: typeof(System.InvalidOperationException) Actual: typeof(System.AggregateException): An error occurred while writing to logger(s). Stack Trace: at Microsoft.Extensions.Logging.Logger.Log[TState](LogLevel logLevel, EventId eventId, TState state, Exception exception, Func`3 formatter) at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.KestrelTrace.Log[TState](LogLevel logLevel, EventId eventId, TState state, Exception exception, Func`3 formatter) in C:\Users\anurse\Code\aspnet\KestrelHttpServer\src\Kestrel.Core\Internal\Infrastructure\KestrelTrace.cs:line 198 at Microsoft.Extensions.Logging.LoggerExtensions.LogCritical(ILogger logger, EventId eventId, Exception exception, String message, Object[] args) at Microsoft.AspNetCore.Server.Kestrel.Core.KestrelServer.<StartAsync>d__24`1.MoveNext() in C:\Users\anurse\Code\aspnet\KestrelHttpServer\src\Kestrel.Core\KestrelServer.cs:line 166 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at System.Runtime.CompilerServices.TaskAwaiter.GetResult() at Microsoft.AspNetCore.Server.Kestrel.Core.Tests.KestrelServerTests.StartDummyApplication(IServer server) in C:\Users\anurse\Code\aspnet\KestrelHttpServer\test\Kestrel.Core.Tests\KestrelServerTests.cs:line 321 at Microsoft.AspNetCore.Server.Kestrel.Core.Tests.KestrelServerTests.<>c__DisplayClass2_0.<KestrelServerThrowsUsefulExceptionIfDefaultHttpsProviderNotAdded>b__0() in C:\Users\anurse\Code\aspnet\KestrelHttpServer\test\Kestrel.Core.Tests\KestrelServerTests.cs:line 62

I think I'm going to put it back for now :)

Ah, it's because the default ctor uses TestApplicationErrorLogger which throws an exception when a Critical message is logged by default. I'm going to change the test to use a NullLogger instance though instead of the Mock.

I changed KestrelTestLoggerProvider to set ThrowOnCriticalErrors to false, since we don't seem to use that behavior anywhere.

halter73 · 2017-10-24T20:01:44Z

src/Kestrel/Internal/DefaultHttpsProvider.cs

+
+        public void ConfigureHttps(ListenOptions listenOptions)
+        {
+            var cert = DefaultCertificateResolver();


Why the Func? Was this replaced in a test at one point? It seems like you could just call FindDevelopmentCertificate directly.

Yeah, I was wondering that too. I suppose so that you could construct it and replace the func, but that doesn't really make sense now that it's pubternal rather than public. It's also a simple enough interface to implement. I'll yank the func.

halter73 · 2017-10-24T20:04:56Z

src/Kestrel/Internal/DefaultHttpsProvider.cs

+                .FirstOrDefault();
+            if (certificate != null)
+            {
+                _logger.LogDebug("Using development certificate: {certificateSubjectName} (Thumbprint: {certificateThumbprint})", certificate.Subject, certificate.Thumbprint);


For common/expected logs, I think it's best to use resource strings.

Hmm, we use literals for all the other logs (see KestrelTrace) so I wasn't sure exactly what the right pattern was here. I thought about adding some LoggerMessage.Define action here but even with that, I've not seen our code use resource strings for that. I guess we could though.

halter73 · 2017-10-24T20:08:03Z

src/Kestrel/Internal/DefaultHttpsProvider.cs

+            }
+            else
+            {
+                _logger.LogDebug("Development certificate could not be found");


Should this be given a higher log level? I'm assuming this would normally cause the application to crash on startup, but in the event the exception is somehow swallowed, I think it's a good idea to log this as an error.

Well, an exception will be thrown by ConfigureHttps, which calls this. But when refactoring to remove the delegate, I'll probably just do exactly as you suggest here :).

analogrelay · 2017-10-24T22:36:28Z

🆙 📅 Responded to feedback, including @javiercn 's comments re messaging.

halter73 · 2017-10-25T18:26:55Z

test/shared/KestrelTestLoggerProvider.cs

@@ -11,7 +11,7 @@ public class KestrelTestLoggerProvider : ILoggerProvider
        private readonly ILogger _testLogger;

        public KestrelTestLoggerProvider()
-            : this(new TestApplicationErrorLogger())
+            : this(new TestApplicationErrorLogger() { ThrowOnCriticalErrors = false })


Don't do this for all tests. For tests where you are expecting a critical errors to be logged, I would just call new KestrelTestLoggerProvider(new TestApplicationErrorLogger() { ThrowOnCriticalErrors = false }).

The idea is that most tests should cause critical errors to be logged, so having this can help us catch issues that might have otherwise been missed.

analogrelay · 2017-10-25T19:26:40Z

🆙 📅 resolved last few pieces of feedback

cesarblum requested review from halter73, davidfowl, Tratcher and javiercn October 3, 2017 18:03

dnfclas added the cla-already-signed label Oct 3, 2017

javiercn reviewed Oct 3, 2017

View reviewed changes

halter73 suggested changes Oct 3, 2017

View reviewed changes

cesarblum force-pushed the cesarbs/magic-https branch 2 times, most recently from acca4ee to c714e4a Compare October 3, 2017 23:29

davidfowl reviewed Oct 4, 2017

View reviewed changes

Tratcher reviewed Oct 4, 2017

View reviewed changes

Eilon reviewed Oct 4, 2017

View reviewed changes

cesarblum force-pushed the cesarbs/magic-https branch 3 times, most recently from ae922be to 256e486 Compare October 9, 2017 23:43

davidfowl reviewed Oct 10, 2017

View reviewed changes

cesarblum requested a review from analogrelay October 12, 2017 19:08

cesarblum force-pushed the cesarbs/magic-https branch from 256e486 to 411af38 Compare October 12, 2017 19:16

analogrelay previously requested changes Oct 13, 2017

View reviewed changes

analogrelay self-assigned this Oct 18, 2017

Cesar Blum Silveira and others added 12 commits October 24, 2017 11:13

Fix test build.

eccd1ea

More cert validation.

3703d0d

Inject IDefaultHttpsProvider in KestrelServer.

ebb2389

Simpler private key check.

fcd542a

Update tests.

fbd6a70

Add new KestrelServer ctor for compat.

e25a2b1

Pass KestrelServerOptions instead of list of ListenOptions to Address…

becf626

…Binder.

Uncomment private key check.

8e5b021

Don't null-condition access to AddressBindContext.DefaultHttpsProvider.

6df6673

add some additional error handling

ea1c66c

comments and such

626a30e

logging and use CertificateManager

77d7952

analogrelay force-pushed the cesarbs/magic-https branch from 36f52b2 to 77d7952 Compare October 24, 2017 18:48

analogrelay added 2 commits October 24, 2017 11:57

fix some (but possibly not all) derps

9ad422b

see, I knew I still derped something

e46a08b

javiercn reviewed Oct 24, 2017

View reviewed changes

javiercn suggested changes Oct 24, 2017

View reviewed changes

halter73 suggested changes Oct 24, 2017

View reviewed changes

analogrelay added 2 commits October 24, 2017 15:22

pr feedback and message updates

3397f60

change test logger provider to not throw on critical

cafaaff

javiercn approved these changes Oct 25, 2017

View reviewed changes

halter73 suggested changes Oct 25, 2017

View reviewed changes

analogrelay added 2 commits October 25, 2017 12:24

constrain use of ThrowOnCriticalErrors

c5cb57f

update error message

48739e5

halter73 approved these changes Oct 25, 2017

View reviewed changes

analogrelay merged commit 8c4bdbc into dev Oct 25, 2017

analogrelay deleted the cesarbs/magic-https branch October 25, 2017 20:59

Add "zero config" HTTPS support using local development certificate. #2093

Add "zero config" HTTPS support using local development certificate. #2093

Uh oh!

Conversation

cesarblum commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

javiercn commented Oct 3, 2017

Uh oh!

muratg commented Oct 3, 2017

Uh oh!

halter73 left a comment

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!

cesarblum commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cesarblum commented Oct 3, 2017

Uh oh!

halter73 commented Oct 3, 2017 • edited by cesarblum Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eilon commented Oct 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Oct 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cesarblum Oct 4, 2017 • 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!

Eilon left a comment

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!

cesarblum commented Oct 9, 2017

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!

cesarblum commented Oct 12, 2017

Uh oh!

analogrelay left a comment

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!

analogrelay commented Oct 24, 2017

Uh oh!

cesarblum commented Oct 3, 2017 •

edited

Loading

cesarblum commented Oct 3, 2017 •

edited

Loading

halter73 commented Oct 3, 2017 •

edited by cesarblum

Loading

cesarblum Oct 4, 2017 •

edited

Loading