Bind Kestrel options to config by default (#30) #44

cesarblum · 2017-04-24T17:22:32Z

dnfclas · 2017-04-24T17:22:37Z

@CesarBS,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

davidfowl · 2017-04-24T17:24:39Z

src/Microsoft.AspNetCore/WebHost.cs

@@ -148,6 +150,7 @@ public static IWebHostBuilder CreateDefaultBuilder(string[] args)

                    if (env.IsDevelopment())
                    {
+                        Console.WriteLine(env.ApplicationName);


I assume this is just left over.

davidfowl · 2017-04-24T17:26:09Z

src/Microsoft.AspNetCore/Microsoft.AspNetCore.csproj

@@ -11,7 +11,7 @@
  </PropertyGroup>

  <ItemGroup>
-   <PackageReference Include="@(MetaPackagePackageReference)" />
+    <PackageReference Include="@(MetaPackagePackageReference)" />


Undo this change?

Indentation was wrong.

davidfowl · 2017-04-24T17:27:09Z

src/Microsoft.AspNetCore/KestrelServerOptionsSetup.cs

+    /// </summary>
+    public class KestrelServerOptionsSetup : IConfigureOptions<KestrelServerOptions>
+    {
+        private IServiceProvider _services;


This should be IConfiguration, not IServiceProvider

davidfowl · 2017-04-24T17:27:18Z

src/Microsoft.AspNetCore/KestrelServerOptionsSetup.cs

+        /// Creates a new instance of <see cref="KestrelServerOptionsSetup"/>.
+        /// </summary>
+        /// <param name="services">An <seealso cref="IServiceProvider"/> instance.</param>
+        public KestrelServerOptionsSetup(IServiceProvider services)


See comment above. Same thing. Inject the IConfiguration directly.

davidfowl · 2017-04-24T17:28:23Z

src/Microsoft.AspNetCore/CertificateLoader.cs

+            var sourceKind = certificateConfiguration.GetValue<string>("Source");
+
+            CertificateSource certificateSource;
+            switch (sourceKind)


Is this supposed to be case sensitive?

cc @javiercn

No, avoid treating config as case sensitive.

davidfowl · 2017-04-24T17:34:36Z

src/Microsoft.AspNetCore/CertificateLoader.cs

+        public static Dictionary<string, X509Certificate2> LoadAll(IConfiguration configurationRoot)
+        {
+            var certificates = configurationRoot.GetSection("Certificates");
+            var loadedCertificates = new Dictionary<string, X509Certificate2>();


This looks like it could be a single LINQ statement.

davidfowl · 2017-04-24T17:43:49Z

src/Microsoft.AspNetCore/KestrelServerOptionsSetup.cs

+            IConfigurationSection endPoint,
+            Dictionary<string, X509Certificate2> certificates)
+        {
+            options.Listen(IPAddress.Parse(endPoint.GetValue<string>("Address")), endPoint.GetValue<int>("Port"), listenOptions =>


This needs to be more failure tolerant. TryParse with a decent exception message

Tratcher · 2017-04-24T18:07:47Z

samples/AppSettings/Program.cs

+{
+    public class Program
+    {
+        public static void Main(string[] args)


Why not use the existing sample?

Settings would apply to all the scenarios there. I want to keep things separate for clarity.

Tratcher · 2017-04-24T18:10:34Z

src/Microsoft.AspNetCore/CertificateLoader.cs

+            var sourceKind = certificateConfiguration.GetValue<string>("Source");
+
+            CertificateSource certificateSource;
+            switch (sourceKind)


No, avoid treating config as case sensitive.

Tratcher · 2017-04-24T18:13:38Z

src/Microsoft.AspNetCore/CertificateLoader.cs

+
+            public override X509Certificate2 Load()
+            {
+                if (!Enum.TryParse(StoreLocation, true, out StoreLocation storeLocation))


named params

Tratcher · 2017-04-24T18:15:12Z

src/Microsoft.AspNetCore/CertificateLoader.cs

+                using (var store = new X509Store(StoreName, storeLocation))
+                {
+                    store.Open(OpenFlags.ReadOnly);
+                    var foundCertificate = store.Certificates.Find(X509FindType.FindBySubjectName, Subject, validOnly: false)


Is there an EKU check anywhere?

Why validOnly: false? Do we want people to be able to bind to expired certs?

FindBySubjectName does strstr. If you want exact match you'll need to post-process it.

store.Certificates returns unique X509Certificate2 objects, and Find returns unique X509Certificate2 objects. If you would like to reduce Finalizations you should dispose all of the members of store.Certificates (which you need to save as a local, since reading it twice just makes the problem worse).

Similary, if you cascade call Find you should dispose all the previous values.

@javiercn Since this was originally your code, what are your thoughts here?

@blowdart self-signed certificates. I would change that to HostingEnvironment.IsDevelopment() if its available

Another option is to just load the certificates and filter using Linq, or have an extra configuration value per certificate that indicates if its ok to load invalid (so that you can again load self-signed certificates for development)

Yet another option is to make the check true and force the user to put the certificate (in case its self-signed) in the trusted certificate store for .NET. (Which varies across platforms)

Talked to @danroth27 and we decided to go with a config key (AllowInvalid).

Tratcher · 2017-04-24T18:31:01Z

samples/AppSettings/appsettings.json

+        "Port": 8081
+      },
+      "HttpV6": {
+        "Address": "[::1]",


Brackets not required

Tratcher · 2017-04-24T18:41:36Z

src/Microsoft.AspNetCore/KestrelServerOptionsSetup.cs

+                X509Certificate2 endPointCertificate = null;
+                if (certificateName != null)
+                {
+                    endPointCertificate = certificates[certificateName];


KeyNotFoundException? You can give a better error.

Tratcher · 2017-04-24T18:43:38Z

src/Microsoft.AspNetCore/KestrelServerOptionsSetup.cs

+
+                    if (certificate.GetChildren().Any())
+                    {
+                        var password = _configurationRoot[$"Kestrel:EndPoints:{endPoint.Key}:Certificate:Password"];


is this not already part of the certificate config section? even if it's coming from a different config source it should get merged.

davidfowl · 2017-04-24T19:32:05Z

samples/SampleApp/SampleApp.csproj

@@ -1,6 +1,6 @@
 <Project Sdk="Microsoft.NET.Sdk.Web">

-  <Import Project="..\..\build\common.props" />
+  <Import Project="..\..\build\dependencies.props" />


@natemcmaster pointed out we don't normally import common.props here, only dependencies.props. This was causing issues with user secrets because common.props has GenerateUserSecretsAttribute set to false.

javiercn · 2017-04-24T20:07:39Z

src/Microsoft.AspNetCore/CertificateLoader.cs

+            {
+                try
+                {
+                    var loadedCertificate = new X509Certificate2(Path, _password);


var loadedCertificate = new X509Certificate2(Path, _password, flags);

cesarblum · 2017-04-24T22:19:30Z

src/Microsoft.AspNetCore/CertificateLoader.cs

+                var certificate = TryLoad(X509KeyStorageFlags.DefaultKeySet, out var error)
+                    ?? TryLoad(X509KeyStorageFlags.UserKeySet, out error)
+    #if NETCOREAPP2_0
+                    ?? TryLoad(X509KeyStorageFlags.EphemeralKeySet, out error)


@blowdart Is this the proper order to attempt to load the cert? cc @javiercn

What are you trying to do here? @bartonjs because he understands Ephemeral :)

I read this as:

Load perphemeral, as MachineKeySet as the PFX says so, or as UserKeySet if it has no opinion on the matter.

Load perphemeral as UserKeySet (in case the key was MachineKeySet and the current process does not have write access to the machine key store).

Load ephemeral (in case the current process has no persisted key write access at all, such as low rights with no user profile)

"perphemeral"? 🤣

Is there an advantage in trying "perphemeral" first, if ephermeral is there? There's no real need to persist the private key anyhere.

Yup, I wordsmithed.

Assuming ephemeral works for your needs, it's probably the best choice. It takes disk IO and permissions completely out of play.

The only concern is that if you pass the cert to something which directly calls CertGetCertificateContextProperty with CERT_KEY_PROV_INFO_PROP_ID they will be confused. But it avoids the whole we-leak-a-file-to-the-hard-drive-if-the-process-crashes annoyance of perphemeral (and the disk I/O // wear-and-tear).

EphemeralKeySet doesn't work on macOS, though, so you'll still want to fall back to a perphemeral mode there.

I'll put the call with EphemeralKeySet first then, with a fallback to UserKeySet if that fails. Does that sound right?

cesarblum · 2017-04-24T23:15:59Z

src/Microsoft.AspNetCore/CertificateLoader.cs

+                    if (!HasServerAuthEKU(certificate))
+                    {
+                        throw new InvalidOperationException(
+                            $"The certificate file at '{Path}' does not contain an Enhanced Key Usage field with the OID 1.3.6.1.5.5.7.3.1 (Server Authentication)");


@blowdart Is the wording correct?

cesarblum · 2017-04-24T23:16:02Z

src/Microsoft.AspNetCore/CertificateLoader.cs

+                    if (foundCertificate == null)
+                    {
+                        throw new InvalidOperationException(
+                            $"No valid certificate found for {Subject} in store {StoreName} under {StoreLocation}. Check that the certificate contains the Enhanced Key Usage field with the OID 1.3.6.1.5.5.7.3.1 (Server Authentication)");


@blowdart Is the wording correct?

cesarblum · 2017-04-24T23:17:37Z

@danroth27 Should the EKU check be mandatory as I've done it here? Or should we add a flag to optionally disable it?

Should we actually move the EKU check to Kestrel? @blowdart @davidfowl Thoughts?

danroth27 · 2017-04-24T23:42:06Z

Yeah, if we're going to check the EKUs at all it should be done lower in the stack. Let's remove this check from here.

cesarblum · 2017-04-24T23:54:00Z

Removed EKU check. Needs further discussion.

davidfowl · 2017-04-25T00:06:49Z

src/Microsoft.AspNetCore/CertificateLoader.cs

+                        .OrderByDescending(certificate => certificate.NotAfter)
+                        .FirstOrDefault();
+
+    #if NET46


Tratcher · 2017-04-25T01:24:20Z

src/Microsoft.AspNetCore/KestrelServerOptionsSetup.cs

+            }
+
+            int port;
+            if (!int.TryParse(portValue, out port))


culture, style

cesarblum · 2017-04-25T18:14:54Z

@halter73 Added you as a reviewer.

blowdart · 2017-04-25T18:22:35Z

@danroth27 It's not a question of IF we perform an EKU check. We should/must. So if it's removed from here, then where is the corresponding issue in kestrel to perform the check there?

…ert.

…tore.

…unctionalTests.

cesarblum · 2017-04-26T22:47:28Z

@JunTaoLuo @Tratcher Can you guys review the functional tests I've just added?

JunTaoLuo

Functional test looks good. I only skimmed the rest of the code change though.

Tratcher

This still needs work.

Tratcher · 2017-04-27T16:28:48Z

src/Microsoft.AspNetCore/CertificateLoader.cs

+
+                if (error != null)
+                {
+                    throw error;


Rethrowing this exception looses the original stack trace. You should wrap it or use ExceptionDispatchInfo to capture and rethrow it.

Tratcher · 2017-04-27T16:32:22Z

src/Microsoft.AspNetCore/KestrelServerOptionsSetup.cs

+            var portValue = endPoint.GetValue<string>("Port");
+
+            IPAddress address;
+            if (!IPAddress.TryParse(addressValue, out address))


Wait, I can't bind to * or localhost? That's a non-starter.

cesarblum requested review from davidfowl, danroth27, JunTaoLuo and javiercn April 24, 2017 17:22

dnfclas added the cla-already-signed label Apr 24, 2017

cesarblum force-pushed the cesarbs/30 branch from 67b4904 to fcf0429 Compare April 24, 2017 17:23

davidfowl reviewed Apr 24, 2017

View reviewed changes

cesarblum force-pushed the cesarbs/30 branch from fcf0429 to 0dacf19 Compare April 24, 2017 17:24

davidfowl reviewed Apr 24, 2017

View reviewed changes

Tratcher reviewed Apr 24, 2017

View reviewed changes

davidfowl reviewed Apr 24, 2017

View reviewed changes

javiercn reviewed Apr 24, 2017

View reviewed changes

cesarblum commented Apr 24, 2017

View reviewed changes

cesarblum force-pushed the cesarbs/30 branch from 3e6432b to ab4b21f Compare April 24, 2017 23:53

davidfowl reviewed Apr 25, 2017

View reviewed changes

Tratcher reviewed Apr 25, 2017

View reviewed changes

src/Microsoft.AspNetCore/KestrelServerOptionsSetup.cs

}

int port;

if (!int.TryParse(portValue, out port))

Copy link

Member

Tratcher Apr 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

culture, style

cesarblum requested a review from halter73 April 25, 2017 18:14

cesarblum force-pushed the cesarbs/30 branch from f66e889 to c3b4ab4 Compare April 26, 2017 21:26

Cesar Blum Silveira added 14 commits April 26, 2017 14:32

Bind Kestrel options to config by default (#30).

81d5f0d

Inject IConfiguration into KestrelServerOptionsSetup.

a7528d2

Load all certs in single LINQ statement.

ecaa630

Better errors for invalid IP addresses and ports.

f3d7cf5

Parse port as int.

fefe595

Endpoint changes to sample.

50e7904

PR feedback.

48a59b6

Pass flags to X509Certificate2 ctor.

e16e2af

No need to call X509Store.Close().

290035c

No default value for password arg in CertificateLoader.Load().

04c8f6f

Throw if no cert found in store.

a4744a6

Add new sample to sln.

59626a7

PR feedback: more validation for subject + config for valid/invalid c…

23a16aa

…ert.

Use FindBySubjectDistinguishedName when loading certificates from a s…

b50c22b

…tore.

cesarblum force-pushed the cesarbs/30 branch from c3b4ab4 to b50c22b Compare April 26, 2017 21:34

cesarblum changed the base branch from rel/1.1.0-preview1 to rel/2.0.0-preview1 April 26, 2017 21:34

muratg approved these changes Apr 26, 2017

View reviewed changes

Cesar Blum Silveira added 2 commits April 26, 2017 15:46

Add functional tests.

6ea143c

Rename directory Microsoft.AspNetCore.Tests to Microsoft.AspNetCore.F…

ee4959c

…unctionalTests.

nit: 'fix' new sample's UserSecretsId

88ef88e

JunTaoLuo approved these changes Apr 26, 2017

View reviewed changes

Don't dispose certificate found in store.

9a1051d

cesarblum merged commit 21de306 into rel/2.0.0-preview1 Apr 26, 2017

cesarblum deleted the cesarbs/30 branch April 26, 2017 23:47

Tratcher reviewed Apr 27, 2017

View reviewed changes

Tratcher mentioned this pull request Apr 27, 2017

Kestrel config binder should understand * and localhost #73

Closed

danroth27 mentioned this pull request Jun 1, 2017

Kestrel config binder should understand * and localhost aspnet/KestrelHttpServer#1879

Closed

muratg mentioned this pull request Aug 18, 2017

Kestrel config binder should understand * and localhost aspnet/AspNetCoreModule#136

Closed

Bind Kestrel options to config by default (#30) #44

Bind Kestrel options to config by default (#30) #44

Uh oh!

Conversation

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

Uh oh!

Uh oh!

dnfclas commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cesarblum Apr 24, 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!

Choose a reason for hiding this comment

Uh oh!

davidfowl Apr 24, 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!

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!

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

cesarblum commented Apr 24, 2017 •

edited

Loading

cesarblum Apr 24, 2017 •

edited

Loading

davidfowl Apr 24, 2017 •

edited

Loading

cesarblum commented Apr 24, 2017 •

edited

Loading