Skip to content

Preload default Kestrel config before running Kestrel's configureOptions callback #28120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
halter73 opened this issue Nov 24, 2020 · 5 comments
Labels
affected-few This issue impacts only small number of customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. feature-kestrel help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team severity-minor This label is used by an internal tool
Milestone

Comments

@halter73
Copy link
Member

This issue is inspired by an email from @wiktork. From the email:

I’m [working] on dotnet-monitor. The tool uses Asp.net Core to host a web server that acts as a control plane for diagnostic tooling. We are currently working on the security work for this tool, in particular supporting https.

I have a scenario where I want to do the following:

  • Bind https://localhost:52323

  • Bind http://localhost:52525

  • Notice one is https, and the other is http.

  • Note the two endpoints serve different information (sensitive data over https, non-sensitive data over http).

  • If there is no default certificate (one generated by dotnet dev-certs) or one specified via Kestrel:Certificates:Default, I would like to successfully bind and listen on the http endpoint, but gracefully fail on the https endpoint.

My initial take on this was to use ConfigureKestrel:

//Simplified code
ConfigureKestrel((context, options) =>
{
        try
        {
               //If https
               options.ListenLocalhost(url.Port, listenOptions => listenOptions.UseHttps() )
        }
        catch (InvalidOperationException e)
        {
            //This binding failure is typically due to missing default certificate
            console.Error.WriteLine($"Unable to bind to {url}. Dotnet-monitor functionality will be limited.");
            console.Error.WriteLine(e.Message);
        }

This allows me to selectively UseHttps on an endpoint, but this doesn’t work because the default certificate has not been loaded yet. From my understanding of the code, that happens post configuration:

  1. All configuration occurs (my UseHttps code always throws, indicating there is no default certificate)
  2. KestrelConfigurationLoader.Load will actually load the default certificate into CertificateManager
  3. Binding/Running server

At this point I could pass along the certificate myself if it’s present to UseHttps, but I don’t really want to replicate all the default certificate logic from asp.net core. Do you guys have any suggestions on how to accomplish this?

The workaround I provided in the email thread was as follows:

It's too bad that UseHttps() doesn't automatically use the default certificate from config. UseHttps() will use the installed development certificate if there's one available. I would love to fix it, but this is the unfortunate side-effect of being able change the config section Kestrel reads from in the ConfigureKestrel callback. Fortunately, since you can load the configuration early yourself inside the ConfigureKestrel callback, the fix should be a one-liner. If you add

options.Configure(context.Configuration.GetSection("Kestrel")).Load();

before

options.ListenLocalhost(url.Port, listenOptions => listenOptions.UseHttps());

does that fix your issue?

While I expect this workaround of manually loading config at the start of the ConfigureKestrel callback works, it's not at all obvious that this should be necessary and is an overall bad experience.

If we instead preload default Kestrel config before running configureKestrel's callback should allow the KestrelServerOptions.Listen() and ListenOptions.UseHttps() methods to pick up "Default"/"Development" certificates and "EndpointDefaults" from config. Since ConfigureWebDefaults is responsible for binding the KestrelConfigurationLoader the the "Kestrel" subsection of config here, this is also where we should do the preloading.

This is a behavioral breaking change, but maybe we can make this less risky by loading just the defaults, and not the endpoints before ConfigureKestrel's callback is run.

@Tratcher
Copy link
Member

We used to load the dev cert first but that was changed by aspnet/KestrelHttpServer#2465 due to the reasons outlined in aspnet/KestrelHttpServer#2422 (startup perf, log noise even when not using the cert).

There may be a middle ground we can reach without regressing those scenarios.

@davidfowl
Copy link
Member

Startup performance is also a focus on .NET 6 so we want to do what we can to find that middleground and not regress performance here.

@halter73 halter73 added this to the Next sprint planning milestone Nov 25, 2020
@ghost
Copy link

ghost commented Nov 25, 2020

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@halter73
Copy link
Member Author

Triage note: The dev cert already works in this scenario. This is about the default cert defined in config.

@jkotalik jkotalik added affected-few This issue impacts only small number of customers bug This issue describes a behavior which is not expected - a bug. severity-minor This label is used by an internal tool labels Dec 4, 2020 — with ASP.NET Core Issue Ranking
@adityamandaleeka adityamandaleeka added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Jul 14, 2021
@ghost
Copy link

ghost commented Sep 13, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

amcasey added a commit to amcasey/aspnetcore that referenced this issue May 3, 2023
...as a way to retrieve the server certificate eagerly (assuming the configuration has been loaded), rather than having to wait until bind-time.

Fixes dotnet#28120
amcasey added a commit to amcasey/aspnetcore that referenced this issue May 4, 2023
...as a way to determine whether a certificate is available before invoking `KestrelServerOptions.Listen` and possibly failing in `ListenOptions.UseHttps`.

Fixes dotnet#28120
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
@mkArtakMSFT mkArtakMSFT added help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team and removed help wanted Up for grabs. We would accept a PR to help resolve this issue labels Oct 28, 2023
@amcasey amcasey modified the milestones: .NET 8 Planning, Backlog Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-few This issue impacts only small number of customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. feature-kestrel help candidate Indicates that the issues may be a good fit for community to help with. Requires work from eng. team severity-minor This label is used by an internal tool
Projects
None yet
8 participants
@halter73 @davidfowl @adityamandaleeka @Tratcher @jkotalik @amcasey @mkArtakMSFT and others