Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Implement configuration support #2186

Merged
merged 1 commit into from
Dec 21, 2017
Merged

Implement configuration support #2186

merged 1 commit into from
Dec 21, 2017

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Nov 28, 2017

This includes the updated design. If you like it then I'll start working on the tests.

Related issues:
#2016 - Config support
#2167 - Default settings
#2166 - UseHttps()
#1290 - Config and code parity
#2188 - Listen on https by default

Major features:

  • KestrelServerOptions.ConfigureEndpointDefaults sets a callback that will be called for every new endpoint no matter what source.
  • KestrelServerOptions.ConfigureHttpsDefaults sets a callback that will be called to configured every new Https adapter. This is primarily useful for endpoints config.
  • KestrelServerOptions.Configure(IConfiguration) creates a builder that reads config and registers endpoints. This includes the .Endpoint method to configure additional settings for individually named endpoints from config.
  • New ListenOptions.UseHttps extensions to provide code and config functional symmetry, and overloads with configureOptions so you don't have to re-invent cert loading when you want to configure other settings.

Supported configurations:

  • No config: Listens on http://localhost:5000 and https://localhost:5001 (if a default cert is available)
  • Configure Urls: Urls can be specified by ASNETCORE_URLS environment variable, Urls command line variable, or Urls top level key in hosting's configuration, or via the UseUrls IWebHostBuilder extension. These can be http or https (if a default cert is available). This may be a semicolon separated list. E.g. "Urls": "http://localhost:5000;http://localhost:5001".
  • Default cert from config: The default certificate can be replaced from configuration. Call serverOptions.Configure(context.Configuration.GetSection("Kestrel")) to specify the configuration section (this will be added to the metapackage). Then add the following config:
{
  "Kestrel": {
    "Certificates": {
       "Default": {
          "Path": "...",
          "Password": ""
       }
    }
  }
}

Certificates can also be loaded from the machine store by specifying the following parameters instead of Path and Password:

  "Subject": "Test Subject", // Required
  "Store": "My", // StoreName defaults to My
  "Location": "CurrentUser/LocalMachine", // StoreLocation defaults to CurrentUser
  "AllowInvalid": true // defaults to false. Setting this to true allows for self-signed or other non-trusted certificates.
  • Change defaults in code: ConfigureEndpointDefaults and ConfigureHttpsDefaults can be used to change default settings for ListenOptions and HttpsConnectionAdapterOptions, including overriding the default certificate specified in the prior scenario.
options.ConfigureEndpointDefaults(opt =>
{
    opt.Protocols = HttpProtocols.Http1;
});

options.ConfigureHttpsDefaults(httpsOptions =>
{
    httpsOptions.SslProtocols = SslProtocols.Tls12;
});
  • Configure endpoints: Named endpoints can be added via configuration with the following format:
  "Kestrel": {
	"Endpoints": {
	   "HTTP": { "Url": "http://*:5000" },
           "HTTPS": {
              "Url": "https://*:5463",
              "Certificate": {
                  "Path": "testCert.pfx",
                  "Password": "testPassword"
              }
          }
    }

Schema details:

  1. The endpoints above are named HTTP and HTTPS. These names are arbitrary but will become useful in the next example. These names are case-insensitive.
  2. The Url parameter is required for each endpoint. The format for this parameter is the same as the top level Urls configuration parameter except that it's limited to a single value.
  3. These endpoints replace those defined in the top level Urls configuration rather than adding to them. Endpoints defined in code via Listen are cumulative with the endpoints defined in this configuration section.
  4. The Certificate section is optional, if not specified then the defaults defined in earlier scenarios will be used. If no defaults are available then the server will throw and fail to start.
  5. The Certificate section supports both the Path&Password and the Subject&Store type of certificates.
  6. Any number of endpoints may be defined in this way so long as they don't cause port conflicts.
  • serverOptions.Configure(context.Configuration.GetSection("Kestrel")) returns a KestrelConfigurationLoader with an .Endpoint(string name, options => { }) method that can be used to supplement configured endpoint's settings.
  serverOptions.Configure(context.Configuration.GetSection("Kestrel"))
    .Endpoint("HTTPS", opt =>
    {
         opt.ListenOptions.Protocols = HttpProtocols.Http1;
         opt.HttpsOptions.SslProtocols = SslProtocols.Tls12;
    });
  • The configuration section for each endpoint is a available on the options in the Endpoint method so that custom settings may be read.
  • Multiple configurations may be loaded by calling serverOptions.Configure(context.Configuration.GetSection("Kestrel")) again with another section. Only the last configuration will be used unless Load is explicitly called on prior instances. The metapackage will not call Load so that it's default configuration section may be replaced.
  • KestrelConfigurationLoader mirrors the Listen family of APIs from KestrelServerOptions as Endpoint overloads so code and config endpoints may be configured in the same place. These overloads do not use names and only consume default settings from configuration.

@Tratcher Tratcher added this to the 2.1.0-preview1 milestone Nov 28, 2017
@Tratcher Tratcher self-assigned this Nov 28, 2017
var url = endpointConfig["Url"];
if (string.IsNullOrEmpty(url))
{
// TODO: Log?
Copy link
Member

Choose a reason for hiding this comment

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

Why not throw in this case? I would log at a minimum though.

{
if (Options.ConfigurationBuilder == null)
{
// The builder has already been built.
Copy link
Member

Choose a reason for hiding this comment

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

When does Build get called twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't expect a given builder to be called twice, but it is a public API. If it was allowed to run twice you'd get duplicate endpoints and port conflicts.

{
public static KestrelConfigBuilder Configure(this KestrelServerOptions options, IConfiguration config)
{
return new KestrelConfigBuilder(options, config); // Assigns itself to options.ConfigurationBuilder
Copy link
Member

Choose a reason for hiding this comment

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

Why not just create a local variable and assign it to options.ConfigurationBuilder in this method instead? It feels slightly better than assigning a partially constructed object even if it's functionally the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it this way because Build then un-assigns itself, this way you can see the whole lifecycle in one class. I can change it.

          "Path": "testCert.pfx",
          "Password": "testPassword"
       }
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I don't mind the comment, but use single line comments instead.

Options.GetHttpsDefaults()(httpsOptions);

var certInfo = new CertificateConfig(endpoint.CertConfig);
if (certInfo.IsFileCert)
Copy link
Member

Choose a reason for hiding this comment

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

Are any errors thrown if a file path and a store location are both specified for the same endpoint?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I don't want to second guess extraneous data. If we have what we need then there's no reason to fail. Nor do I expect people to mix those two up.

Copy link
Member

Choose a reason for hiding this comment

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

I see no reason not to fail if both a file path and a store location is provided.

What's the logic behind choosing file path over the store location when both are specified? If you don't expect people to mix the two, what's wrong with throwing?

/// <summary>
/// Used to flow settings for connection adapters and other extensions.
/// </summary>
public IDictionary<string, object> AdapterData { get; } = new Dictionary<string, object>(0);
Copy link
Member

Choose a reason for hiding this comment

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

Yuck. IFeatureCollection?

Copy link
Member Author

Choose a reason for hiding this comment

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

A dictionary in sheep's clothing... You prefer the typed keys?

Copy link
Member

Choose a reason for hiding this comment

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

This is pretty gross. How is it used?

Copy link
Member

Choose a reason for hiding this comment

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

@davidfowl It's there to allow extension methods defined by connection adapters in separate assemblies to store data between calls to extension methods.

In this case it is used by KestrelServerOptions.ConfigureHttpsDefaults() to store away a default HttpsConnectionAdapterOptions for later use by ListenOptions.UseHttps() and KestrelConfigBuilder.Build().

Copy link
Member

Choose a reason for hiding this comment

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

@davidfowl You're OK with merging this right, I don't exactly have a better alternative right now.

Copy link
Member

Choose a reason for hiding this comment

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

This looks pretty weird, I'm still catching up with this PR and I'll see if I have any better alternatives for what you guys are trying to do

{
public static class KestrelServerOptionsHttpsExtensions
{
public static void ConfigureHttpsDefaults(this KestrelServerOptions serverOptions, Action<HttpsConnectionAdapterOptions> configureOptions)
Copy link
Member

Choose a reason for hiding this comment

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

I would add doc comments to this and ConfigureEndpointDefaults need to be called prior to the Configure/Listen calls it is providing default values for.

<PackageReference Include="Microsoft.Extensions.Logging.Console" Version="$(MicrosoftExtensionsLoggingConsolePackageVersion)" />
</ItemGroup>

<ItemGroup>
<Content Include="../../test/shared/TestCertificates/testCert.pfx" CopyToOutputDirectory="PreserveNewest" />
</ItemGroup>

<ItemGroup>
<Content Update="appsettings.Development.json">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
Copy link
Member

Choose a reason for hiding this comment

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

nit: Make these properties

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather not hand customizes VS generated settings.

});

options.ListenAnyIP(basePort + 3);

options.ListenAnyIP(basePort + 4, listenOptions =>
{
listenOptions.UseHttps(StoreName.My, "aspnet.test", StoreLocation.CurrentUser);
Copy link
Member

Choose a reason for hiding this comment

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

Will this explode?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the cert is missing? Yes

/// Provides a configuration source where endpoints will be loaded from on server start.
/// The default is null.
/// </summary>
public IKestrelConfigBuilder ConfigurationBuilder { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

nit: IKestrelConfigurationBuilder

@danroth27 danroth27 added the https label Dec 8, 2017
@Tratcher Tratcher changed the title [WIP] Implement configuration support Implement configuration support Dec 12, 2017
@Tratcher
Copy link
Member Author

@halter73 Updated with tests and ready to review. Let me know if you want me to trim down the new sample content.

@Tratcher
Copy link
Member Author

Filed https://github.com/aspnet/KestrelHttpServer/issues/2216 to consider what other settings would make sense to read from config. These would all be easy to add on top of the existing design.

@Tratcher Tratcher requested a review from pakrym December 12, 2017 23:33
@Tratcher
Copy link
Member Author

Poke please review this today.

@@ -104,6 +104,7 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Protocols.Abstractions", "s
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "build", "build", "{C2910A13-B2C2-46D8-81D8-7E166F4F5981}"
ProjectSection(SolutionItems) = preProject
build\dependencies.props = build\dependencies.props
Copy link
Member

Choose a reason for hiding this comment

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

👍

/// <returns></returns>
public static KestrelConfigurationBuilder Configure(this KestrelServerOptions options)
{
var builder = new KestrelConfigurationBuilder(options, new ConfigurationBuilder().Build());
Copy link
Member

Choose a reason for hiding this comment

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

There's no Configuration.Empty or similar?

Copy link
Member Author

@Tratcher Tratcher Dec 16, 2017

Choose a reason for hiding this comment

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

Not that I see. @HaoK ?

I'm not concerned about it, this Configure method is only called once or twice at startup.

/// Creates a configuration builder for setting up Kestrel.
/// </summary>
/// <param name="options"></param>
/// <returns></returns>
Copy link
Member

Choose a reason for hiding this comment

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

Nit: remove empty doc comment sections.

{
var env = hostingContext.HostingEnvironment;
config.AddJsonFile("appsettings.json", optional: true)
.AddJsonFile($"appsettings.{env.EnvironmentName}.json", optional: true);
Copy link
Member

Choose a reason for hiding this comment

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

At least one of these appsettings files is required now, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, they're optional in the sample. The config is additive on top of the endpoints defined in code.

options.KestrelServerOptions = context.ServerOptions;
context.ServerOptions.EndpointDefaults(options);

if (!options.ConnectionAdapters.Any(f => f.IsHttps))
Copy link
Member

Choose a reason for hiding this comment

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

So if you don't configure a specific endpoint and the DefaultAddressStrategy is used, you bind to "http://localhost:5000" and "https://localhost:5001". So far so good.

But if you also configure an HTTPS adapter using ConfigureEndpointDefaults you now listen on "https://localhost:5000" and don't listen on port 5001 at all? Seems weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you add HTTPS in ConfigureEndpointDefaults then you end up listening on both "https://localhost:5000" and "https://localhost:5001". This if only skips adding the https adapter twice, it doesn't skip binding to this endpoint.

/// <summary>
/// Returns the default certificate, if available, otherwise null.
/// </summary>
X509Certificate2 Certificate { get; }
Copy link
Member

Choose a reason for hiding this comment

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

When does this need to be set?

Copy link
Member Author

Choose a reason for hiding this comment

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

/// <param name="configureOptions"></param>
public void ConfigureEndpointDefaults(Action<ListenOptions> configureOptions)
{
EndpointDefaults = configureOptions ?? throw new ArgumentNullException(nameof(configureOptions));
Copy link
Member

Choose a reason for hiding this comment

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

Should we allow this to stack? Either way the doc comments should mention whether this overrides previous calls to ConfigureEndpointDefaults.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think more that one default action is required. Comments updated.

}

DisposeCertificates(storeCertificates);
DisposeCertificates(foundCertificates);
Copy link
Member

Choose a reason for hiding this comment

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

After you're done with a X509Store, you seriously have to enumerate X509Store.Certificates and dispose all the ones you didn't use? That's ridiculous if true.

Copy link
Member

Choose a reason for hiding this comment

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

Also isn't everything in foundCertificates also in storeCertificates, so the second call is just double disposing?

Copy link
Member Author

Choose a reason for hiding this comment

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

@javiercn? You've played with the cert store a lot lately.

Note this was copied from Hosting with an added EKU check.
https://github.com/aspnet/Hosting/blob/a2962d54f18dece7ec57ee42f3e565f3727168e1/shared/Microsoft.AspNetCore.Certificates.Configuration.Sources/CertificateStoreLoader.cs#L50

I think you're right about the double dispose, unless Find is making a deep copy for some reason.

Copy link
Member

Choose a reason for hiding this comment

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

@halter73 I think you need to dispose the ones that you don't use. I would have to ask, but I believe that when you open the store and enumerate the certificates you get some unmanaged references to the actual underlying keys (when present) so I believe you need to dispose those.

I believe that the dotnet crypto folks gave us explicit feedback on Cesar's original PR about this. We can probably dig up that PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Native resources would all have finalizers, but it's a good habit to explicitly dispose them when you're done.

}

public bool IsHttps { get; }
public ListenOptions Listener { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Nit: name it ListenOptions.


/// <summary>
/// Bind to given IP address and port.
/// The callback configures endpoint-specific settings.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: If we're going to include the sentence about the configure callback, we should do so for all applicable overloads.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

.Configure()
.Endpoint(IPAddress.Loopback, basePort + 5)
.LocalhostEndpoint(basePort + 6)
.Build();
Copy link
Member

@HaoK HaoK Dec 16, 2017

Choose a reason for hiding this comment

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

So, this I'm guessing Build() is required because that's what triggers the endpoint to actually get added to the options?

Related question, there is no Build() on the Kestrel config section binding one below, so would that just basically be a no-op giving you an unused builder?

I think it might make things simpler if you just add a separate dedicated options for the defaults, i.e. KestralServerDefaultOptions and configure it via options normally. Thent he ConfigureXyzDefault methods would just be configuring this other option instead of manipulating the builder.

You'd still need the existing builder logic to be able to create endpoints from config, but its no longer part of the KestralServerOptions. The server can just get new default options along with the normal options and continue doing the same thing that Build() does. The only change that might be required is all of the defaults configuration would need to be done earlier, as they would need to be in the service container.

I definitely don't have a ton of context in this code or how everything fits together, but using a dedicated options class for the defaults seems to be an potential alternative path to consider

Copy link
Member Author

@Tratcher Tratcher Dec 16, 2017

Choose a reason for hiding this comment

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

Kestrel calls Build on the last builder at Start. The idea is that the metapackage will assign a default configuration but it won't call Build so that that default can be replaced by user code. If you want to add two configurations you need to call Build on at least the first one.

instead of manipulating the builder.
? The ConfigureXDefault methods don't manipulate the builder.

We didn't want to go the route of KestralServerDefaultOptions as that would require duplicating every setting. With the callbacks all settings added to ListenOptions are automatically available as defaults. There are also some things like the connection adapters that are better to create per endpoint.

Copy link
Member

Choose a reason for hiding this comment

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

Requiring them to call build sometimes seems a bit awkward, can't you just expose a list that they can control and remove/add as needed instead... options.Builders.Add instead of options.Configure

RE duplication, you should be able to reuse whatever sub options you want in the defaults, why wouldn't you be able to use the ListenOptions/Adapters for the default options as well. But even if you wanted to have a complete identical surface area, you could just use named options

// Store defaults via "default" name

   var defaults = IOptionsMonitor<KestrelServerOptions>.Get("defaults");
   // ConfigureXDefault methods just pass extra name parameter

Copy link
Member Author

Choose a reason for hiding this comment

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

In practice I expect them to almost never call build, they either use the config defined in the metapackage or they use the one from their own code. Keeping a list does not seem necessary. We'll see what the preview1 feedback is like.

From your examples I'm not sure you understand what defaults we're talking about configuring. It's not that there are multiple KestrelServerOptions with a set of defaults, it's that a given KestrelServerOptions has multiple endpoints / ListenOptions / httpsOptions and you can define defaults for the options on those. The connection adapters are the most tricky here, you'd need to create a fresh one for each endpoint so you can't create one as a default without implementing some kind of cloning logic. With the current model you can re-use all of the extensions off of ListenOptions for creating connection adapters.

Copy link
Member

Choose a reason for hiding this comment

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

So let me try to get on the same page...

So the KestrelServer eventually wants to produce a bunch of endpoints which has options in a ListenOptions instance, (Endpoints are created from these 1:1?)

Https is doing weird stuff which I guess relates to the fact that its configuring listen option instances and doing its own options like stuff. The EndpointDefaults action is basically equivalent to doing a ConfigureAll on ListenOptions if you were using IOptions.

Part of the confusion here I think is that KestrelServerOptions are using IOptions/Configure, while there are a few options types that are used more like EndpointSettings/AdapterSettings and not configured like normal options.

With named endpoints, that matches exactly named options, you can just use IOptions<EndpointOptions>.Get(name) and just bind against config.GetSection("Kestrel:Endpoints"). Your defaults could then just be sugar for services.ConfigureAll(o => o.Protocols = HttpProtocols.Http1)

Assuming you can just hang all Endpoint operations off of .Endpoints, maybe something like? But I'm sure I don't grok what's going on around the adapter/https yet.

   UseKestrel(context, o => {

    // Default wireup (but showing what the sugar methods call)
    o.ConfigureAll<EndpointOptions>(opt => opt.Protocols = HttpProtocols.Http1);
    o.ConfigureAll<HttpsDefaultOptions>(o => o.SslProtocols = SslProtocols.Tls12);

      // just binds all the endpoint options to their names using Configure (and defaults?)
      o.Endpoints.Configure(context.Configuration.GetSection("Kestrel:Endpoints")); 
      o.Endpoints.Add(IPAddress.Loopback, basePort + 5);
      o.Endpoints.AddLocalhost(basePort + 6);
      o.Endpoints.AddNamed("NamedEndpoint", opt =>
                        {
                             opt.Listener.Protocols = HttpProtocols.Http1;
                         })
      o.Endpoints.AddNamed("NamedHttpsEndpoint", opt =>
                         {
                             opt.Https.SslProtocols = SslProtocols.Tls12;
                         });
   }

Copy link
Member Author

Choose a reason for hiding this comment

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

You're still missing quite a bit of context. Stop by if you want to walk through it.

The question I'd originally sent you was if configuration has a static helper for the scenario where I only want an empty placeholder instance like new ConfigurationBuilder().Build()

Copy link
Member

Choose a reason for hiding this comment

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

No because there's always config, its pretty strange to pass in empty configuration to a configuration loader class though... Hence why I think the naming is a bit weird.

Like this doesn't use config at all right?

options
                         .Configure()
                         .Endpoint(IPAddress.Loopback, basePort + 5)
                         .LocalhostEndpoint(basePort + 6)
                         .Load();

Copy link
Member Author

@Tratcher Tratcher Dec 19, 2017

Choose a reason for hiding this comment

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

Damian wanted to mirror the Listen APIs so you could still configure everything in one place. Yes it makes for a slightly odd fraken type.

Copy link
Member

@HaoK HaoK Dec 19, 2017

Choose a reason for hiding this comment

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

That's fine, my main point is that this seems more than just a configuration loader, this is public endpoint management API basically, where the ctor takes an optional config and Load() is basically committing the set of endpoints.

I think building that dedicated API where configuration is just one way to import a chunk of endpoint settings is maybe a simpler way to expose this...

I still think this would be easier to understand if the API just exposed the endpoints on the options. Having to call configure/endpoint/load and then another configure/endpoint block just seems strange.

Seems like it'd be better to let them express it more concisely, without any weird loads.

options
    .LoadConfig(context.Configuration.GetSection("Kestrel") // Order for this shouldn't matter when its called, it just adds bindings using the config section for named endpoints
    .AddEndpoint(IPAddress.Loopback, basePort + 5) // uses defaults
    .AddLocalhostEndpoint(basePort + 6) // uses defaults
    .AddEndpoint("NamedEndpoint", opt => // uses defaults and config section bindings via name
                         {
                             opt.ListenOptions.Protocols = HttpProtocols.Http1;
                         })
   .AddEndpoint("NamedHttpsEndpoint", opt => // uses defaults and  config section bindings via name
                         {
                             opt.HttpsOptions.SslProtocols = SslProtocols.Tls12;
                         }

And you can always just let them Remove/Update existing endpoints.


public IConfigurationSection ConfigSection { get; }

public bool Exists => ConfigSection.GetChildren().Any();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you could consider using the Exists extension method which is similar.
https://github.com/aspnet/Configuration/blob/dev/src/Config.Abstractions/ConfigurationExtensions.cs#L76

You can consider using section.Bind(this) if you wanted to avoid all the ConfigSection property lookups where the name is the property name.


namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal
{
internal class ConfigurationReader
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you could consider just making this into two internal static extension methods on IConfiguration

IConfiguration.ReadCertificates(this IConfiguration)
IConfiguration.ReadEndpoints(this IConfiguration)

Copy link
Member Author

Choose a reason for hiding this comment

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

No need when this is only for internal usage.

@Tratcher
Copy link
Member Author

Note many of the strange design decisions here are due to the assembly separation between K.Core and K.Https. We've opted to merge the https libraries into core and then revisit this PR.

@Tratcher
Copy link
Member Author

Rebased on the new merged assemblies and code consolidated. Ready for a final pass @halter73

/// <summary>
/// Open a socket file descriptor.
/// </summary>
public KestrelConfigurationLoader HandleEndpoint(ulong handle) => HandleEndpoint(handle, _ => { });
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious - did we expose ulongs in the public API before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes these are all mirrors of the existing Listen APIs.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. KestrelServerOptions.ListenHandle(ulong).

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

I would like a test for force upgrading an HTTP endpoint passed in via the IServerAddressesFeature to HTTPS though.

@guardrex
Copy link

@Tratcher

In the initial post above Certificates is a child of Kestrel ...

{
  "Kestrel": {
    "Certificates": {
       "Default": {
          "Path": "...",
          "Password": ""
       }
    }
  }
}

... but in the blog post, it's not ...

https://blogs.msdn.microsoft.com/webdev/2018/02/02/asp-net-core-2-1-roadmap#security

{
  "Kestrel": {
    
  },
  "Certificates": {
    "Default": {
      "Path": "<file>",
      "Password": "<password>"
    },

    "Default": {
      "Subject": "",
      "Store": "",
      "Location": "",
      "AllowInvalid": ""
    }
  }
}

... no love from schema store ... http://json.schemastore.org/appsettings

I assume the blog post is incorrect. cc/ @danroth27

@Tratcher
Copy link
Member Author

@guardrex you're correct. We've already fixed that in another blog post, but it looks like we should update this one too. I left him a comment directly on the that post.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants