Make TLS & QUIC Pay-for-Play (Redux) #47454

amcasey · 2023-03-27T23:36:55Z

Based on the feedback on #47209, I've prepared this substantially different approach. Whereas #47209 focuses on providing a describable set of restrictions and guardrails to prevent their violation, this PR focuses on making a best effort to improve trimmed size while breaking as few scenarios as possible.

What's disabled when you create a slim builder? You'll find out at bind-time if you crossed the line and a helpful error message will tell you the one-liner (either IWebHostBuilder.UseHttpsConfiguration or IWebHostBuilder.UseQuic) to add to fix it. What does that mean, in practice? Kestrel won't call UseHttps on your behalf - in particular, setting your URL to https won't "just work" - and there won't be any QUIC transports.

Presently, IWebHostBuilder.UseQuic doesn't imply IWebHostBuilder.UseHttpsConfiguration, but it could if we're willing to slightly clutter the public API (i.e. with a marker service in Connection.Abstractions).

There are exactly two new DI services, both internal:

IHttpsConfigurationHelper is always present, but throws until it is explicitly enabled.
IEnabler is a declarative way to enable IHttpsConfigurationHelper (vs a method call). I suspect there's a better way to do this in DI, but I'm not familiar enough with the available APIs.

There are two new public APIs (plus overloads):

UseKestrelSlim (name TBD) doesn't call UseQuic or IHttpsConfigurationHelper.Enable.
UseHttpsConfiguration (effectively) calls IHttpsConfiguration.Enable.

I have more validation to do, but the trimmed size is as in #47209 and the tests pass.

Fixes #47567
Fixes #46681

amcasey · 2023-03-27T23:37:47Z

FYI @halter73 @JamesNK This potentially replaces #47209. I believe it incorporates the feedback provided on that PR.

src/Servers/Kestrel/Core/src/ITlsConfigurationLoader.cs

src/Servers/Kestrel/Core/src/HttpsConnectionAdapterOptions.cs

src/DefaultBuilder/src/WebHost.cs

src/Servers/Kestrel/Core/src/ListenOptionsHttpsExtensions.cs

src/Servers/Kestrel/Core/src/TlsConfigurationLoader.cs

src/Servers/Kestrel/Core/src/HttpsConfigurationHelper.cs

amcasey · 2023-03-28T04:15:52Z

/azp run

azure-pipelines · 2023-03-28T04:16:10Z

Azure Pipelines successfully started running 3 pipeline(s).

amcasey · 2023-03-29T00:33:57Z

I did manage to get the template tests to run locally, but they pass. I'll keep investigating, but I don't expect a substantial product change (i.e. I think it's safe to review).

Edit: I don't know what it was doing, but it wasn't running the tests. I debugged in CI and now the tests are green.

amcasey · 2023-03-29T18:01:58Z

974b528 fixed most tests. The issue was that ConfigureWebDefaults was effectively calling UseKestrelSlim, rather than UseKestrel. I'm not sure why some tests are still failing, but I've added more instrumentation to investigate.

amcasey · 2023-03-30T16:29:30Z

The remaining test failure appears to be induced by my logging code, so I'm ignoring it for now.

Edit: It was. Resolved.

src/Servers/Kestrel/Core/src/IHttpsConfigurationHelper.cs

src/Servers/Kestrel/Core/src/ITlsConfigurationLoader.cs

src/Servers/Kestrel/Core/src/Internal/KestrelServerImpl.cs

src/Servers/Kestrel/Core/src/KestrelServer.cs

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs

src/Servers/Kestrel/Kestrel/src/WebHostBuilderKestrelExtensions.cs

src/Servers/Kestrel/Core/src/HttpsConfigurationHelper.cs

amcasey · 2023-03-31T17:00:16Z

Force push drops all debugging commits and, hopefully, fixes CI.

src/Servers/Kestrel/Kestrel/src/WebHostBuilderKestrelExtensions.cs

ghost · 2023-04-05T01:43:36Z

@amcasey, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!

src/Servers/Kestrel/Core/src/IHttpsConfigurationService.cs

src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs

davidfowl · 2023-04-05T04:54:56Z

src/Servers/Kestrel/Core/src/KestrelServer.cs

+        public void ApplyHttpsConfiguration(
+            HttpsConnectionAdapterOptions httpsOptions,
+            EndpointConfig endpoint,
+            KestrelServerOptions serverOptions,
+            CertificateConfig? defaultCertificateConfig,
+            ConfigurationReader configurationReader)
+        {
+            throw new NotImplementedException(); // Not actually required by this impl
+        }
+
+        public ListenOptions UseHttpsWithSni(ListenOptions listenOptions, HttpsConnectionAdapterOptions httpsOptions, EndpointConfig endpoint)
+        {
+            throw new NotImplementedException(); // Not actually required by this impl
+        }
+
+        public CertificateAndConfig? LoadDefaultCertificate(ConfigurationReader configurationReader)
+        {
+            throw new NotImplementedException(); // Not actually required by this impl
+        }
+    }


I feel like I'm missing something. This is saying that these methods aren't called when KestrelServer is newed up manually?

Correct, KestrelServer and KestrelServerImpl only pass the IHttpsConfigurationService to TransportManager and AddressBinder, so they don't need the parts of the interface that are exclusively for KestrelConfigurationLoader.

davidfowl · 2023-04-05T04:57:05Z

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs

+        var HttpsConfigurationService = ApplicationServices.GetRequiredService<IHttpsConfigurationService>();
+
+        if (!HttpsConfigurationService.IsInitialized)
+        {
+            var hostEnvironment = ApplicationServices.GetRequiredService<IHostEnvironment>();
+            var logger = ApplicationServices.GetRequiredService<ILogger<KestrelServer>>();
+            var httpsLogger = ApplicationServices.GetRequiredService<ILogger<HttpsConnectionMiddleware>>();
+            HttpsConfigurationService.Initialize(hostEnvironment, logger, httpsLogger);
+        }


Usually, there's a single call to some entry point service that resolved dependencies. This feels very manual. Is there a way to avoid these checks to IsInitialized or does that happen opportunistically in various places?

This is the pattern I came up with for making it possible to light up this functionality either during DI setup or afterwards (esp when calling ListenOptions.UseHttps). Alternatives welcome.

To elaborate a bit, there's always an IHttpsConfigurationService, but all of its methods throw unless you either register an HttpsConfigurationService.IInitializer (as in UseHttpsConfiguration) or explicitly invoke Initialize (as here, in service of ListenOptions.UseHttps). Since UseHttps is called after the DI container is sealed, there needed to be a different mechanism for enabling the functionality in that code path. (Eric and Stephen both felt strongly that UseHttps should "just work".)

amcasey · 2023-04-10T23:49:47Z

Looks like I was once again sabotaged by my local dev certs. I'll update the tests.

amcasey · 2023-04-11T23:31:19Z

Rename "UseKestrelSlim" to "UseKestrelCore"?

`CreateSlimBuilder` now calls `UseKestrelSlim` (name TBD) to create a Kestrel server that doesn't automatically support TLS or QUIC. If you invoke `ListenOptions.UseHttps`, TLS will "just work" but, if you want to enable https using `ASPNETCORE_URLS`, you'll need to invoke the new `WebHostBuilder.UseHttpsConfiguration` extension method. Quic is enabled using the existing `WebHostBuilder.UseQuic` extension method. Squash: Break direct dependency of AddressBinder on ListenOptionsHttpsExtensions.UseHttps Factor out the part of TransportManager that depends on https Introduce KestrelServerOptions.HasServerCertificateOrSelector for convenience Factor TlsConfigurationLoader out of KestrelConfigurationLoader Introduce but don't consume IHttpsConfigurationHelper Consume IHttpsConfigurationHelper - tests failing Fix most tests Fix KestrelServerTests Fix remaining tests Respect IHttpsConfigurationHelper in ApplyDefaultCertificate Introduce UseKestrelSlim Delete unused TryUseHttps Enable HttpsConfiguration when UseHttps is called Introduce UseHttpsConfiguration Drop incomplete test implementation of IHttpsConfigurationHelper Tidy up test diffs Fix AOT trimming by moving enable call out of ctor Fix some tests Simplify HttpsConfigurationHelper ctor for more convenient testing Improve error message Don't declare Enabler transient Fix tests other than KestrelConfigurationLoaderTests Correct HttpsConfigurationHelper Add IEnabler interface to break direct dependency Restore UseKestrel call in WebHost.ConfigureWebDefaults Stop registering an https address in ApiTemplateTest boolean -> bool HttpsConfigurationHelper -> HttpsConfigurationService HttpsConfigurationService.Enable -> Initialize ITlsConfigurationLoader.ApplyHttpsDefaults -> ApplyHttpsConfiguration ITlsConfigurationLoader.UseHttps -> UseHttpsWithSni IHttpsConfigurationService.UseHttps -> UseHttpsWithDefaults Inline ITlsConfigurationLoader in IHttpsConfigurationService Document IHttpsConfigurationService Document new public APIs in WebHostBuilderKestrelExtensions Clean up TODOs Improve error text recommending UseQuic Co-authored-by: James Newton-King <[email protected]> Add assert message Clarify comment on assert Fix typo in doc comment Co-authored-by: Aditya Mandaleeka <[email protected]> Fix typo in doc comment Co-authored-by: Aditya Mandaleeka <[email protected]> Fix typo in doc comment Co-authored-by: Aditya Mandaleeka <[email protected]> Don't use regions Correct casing Replace record with readonly struct Test AddressBinder exception Test an endpoint address from config Test certificate loading Bonus: use dynamic ports to improve reliability Test Quic with UseKestrelSlim Test the interaction of UseHttps and UseHttpsConfiguration Test different UseHttps overloads Add more detail to doc comment Set TestOverrideDefaultCertificate in the tests that expect it

amcasey · 2023-04-12T22:11:38Z

Force push is a squash and a rebase on main (mostly a merge with #46834).

src/Servers/Kestrel/Core/src/CoreStrings.resx

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs

src/Servers/Kestrel/Core/src/KestrelServer.cs

JamesNK · 2023-04-13T11:27:36Z

I played around a bit on a branch - 6e10637

MemberNotNull can definitely be used to remove some bangs.
Can TlsConfigurationLoad be passed from DI, or does that bring in types we don't want?
Is it possible to write a test to verify that TLS and friends are trimmed when not enabled? We have benchmarks that measure size, but something that will be caught in CI is needed. Otherwise, I think this will be too easy to regress accidentally.

Co-authored-by: James Newton-King <[email protected]>

amcasey · 2023-04-13T17:46:49Z

I played around a bit on a branch - 6e10637

I'll have a look.

Edit: I only see MemberNotNull and TlsConfigurationLoader - did I miss anything?

MemberNotNull can definitely be used to remove some bangs.

I didn't know about that attribute. Neat.

Edit: done.

Can TlsConfigurationLoad be passed from DI, or does that bring in types we don't want?

It was a DI service in #47209, but Stephen asked me not to clutter the service collection.

Edit: It's definitely cleaner, but we really need to finally land this. That won't affect the public surface area, so we can refactor in a follow-up PR if there's still appetite.

Is it possible to write a test to verify that TLS and friends are trimmed when not enabled? We have benchmarks that measure size, but something that will be caught in CI is needed. Otherwise, I think this will be too easy to regress accidentally.

It is ridiculously easy to regress this. I think @mitchdenny is working on AOT template tests that might provide this coverage?

JamesNK · 2023-04-13T23:30:03Z

You also need the Debug.Assert calls from my branch to fix the new warnings.

If it's very easy to undo this improvement, we need unit tests. @eerhardt does dotnet/runtime have a way to assert types aren't accidentally being brought in that we could use? Would publishing an app with native AOT and then doing Type.GetType(xxx) work?

…itialized

JamesNK

LGTM.

Need a way to automate testing this to prevent accidentally regressing the next time we need to change TLS cert logic.

This will be non-trivial. Can happen in a follow-up PR.

src/Servers/Kestrel/Core/test/KestrelServerTests.cs

eerhardt · 2023-04-14T00:47:23Z

You also need the Debug.Assert calls from my branch to fix the new warnings.

If it's very easy to undo this improvement, we need unit tests. @eerhardt does dotnet/runtime have a way to assert types aren't accidentally being brought in that we could use? Would publishing an app with native AOT and then doing Type.GetType(xxx) work?

Yes, we do that in 2 places (that I know of) in dotnet/runtime:

Invariant Globalization:

https://github.com/dotnet/runtime/blob/e4743ec9474728f2b4e120fd70a3f729e9204c32/src/libraries/System.Runtime/tests/TrimmingTests/InvariantGlobalizationTrue.cs#L31-L67

System.Text.Json:

https://github.com/dotnet/runtime/blob/e4743ec9474728f2b4e120fd70a3f729e9204c32/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/IsReflectionEnabledByDefaultFalse.cs#L54-L66

Note there is a bit of infrastructure to support this. We have a "TrimmingTests" leg that publishes all these EXE projects for trimming. And then we execute the published executable and ensure the exit code is 100. If it isn't, the test fails. That is all captured here:

https://github.com/dotnet/runtime/blob/e4743ec9474728f2b4e120fd70a3f729e9204c32/eng/testing/linker/trimmingTests.targets#L151

Note that ASP.NET has a backup system (https://github.com/aspnet/Benchmarks/tree/main/src/BenchmarksApps) which publish fully trimmed apps and measure the output size. We also have a regression bot that logs an issue if those apps increase by more than 2%.

eerhardt · 2023-04-14T14:45:51Z

ghost added the area-runtime label Mar 27, 2023

amcasey commented Mar 27, 2023

View reviewed changes

amcasey force-pushed the PayForPlay branch from c9ad0df to e727e55 Compare March 29, 2023 22:24

amcasey force-pushed the PayForPlay branch from e9a9518 to 2667248 Compare March 31, 2023 00:37

JamesNK reviewed Mar 31, 2023

View reviewed changes

amcasey force-pushed the PayForPlay branch from 2667248 to b8018e6 Compare March 31, 2023 16:59

amcasey marked this pull request as ready for review April 4, 2023 17:35

amcasey requested review from Tratcher, halter73, BrennanConroy and mgravell as code owners April 4, 2023 17:35

This was referenced Apr 4, 2023

Make TLS & QUIC Pay-for-Play #47209

Closed

Introduce trim-friendly APIs for controlling HTTPS Configuration support #47567

Closed

adityamandaleeka reviewed Apr 5, 2023

View reviewed changes

src/Servers/Kestrel/Kestrel/src/WebHostBuilderKestrelExtensions.cs Outdated Show resolved Hide resolved

adityamandaleeka reviewed Apr 5, 2023

View reviewed changes

src/Servers/Kestrel/Kestrel/src/WebHostBuilderKestrelExtensions.cs Outdated Show resolved Hide resolved

adityamandaleeka reviewed Apr 5, 2023

View reviewed changes

src/Servers/Kestrel/Kestrel/src/WebHostBuilderKestrelExtensions.cs Outdated Show resolved Hide resolved

adityamandaleeka reviewed Apr 5, 2023

View reviewed changes

src/Servers/Kestrel/Kestrel/src/WebHostBuilderKestrelExtensions.cs Outdated Show resolved Hide resolved

adityamandaleeka added the blog-candidate Consider mentioning this in the release blog post label Apr 5, 2023

davidfowl reviewed Apr 5, 2023

View reviewed changes

src/Servers/Kestrel/Core/src/IHttpsConfigurationService.cs Outdated Show resolved Hide resolved

davidfowl reviewed Apr 5, 2023

View reviewed changes

src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs Outdated Show resolved Hide resolved

davidfowl reviewed Apr 5, 2023

View reviewed changes

adityamandaleeka approved these changes Apr 7, 2023

View reviewed changes

amcasey force-pushed the PayForPlay branch from 2ae1607 to 7843321 Compare April 12, 2023 22:11

JamesNK reviewed Apr 13, 2023

View reviewed changes

src/Servers/Kestrel/Core/src/CoreStrings.resx Outdated Show resolved Hide resolved

src/Servers/Kestrel/Core/src/KestrelServerOptions.cs Outdated Show resolved Hide resolved

src/Servers/Kestrel/Core/src/KestrelServer.cs Show resolved Hide resolved

Improve assert message

7aea16b

Co-authored-by: James Newton-King <[email protected]>

Adopt MemberNotNullAttribute

7b94e52

amcasey added 5 commits April 13, 2023 16:36

Assert that members are non-null to suppress CS8774

a269b08

UseHttpsConfiguration -> UseKestrelHttpsConfiguration

367d052

UseKestrelSlim -> UseKestrelCore

7696f09

Drop convenience overloads of UseKestrelCore

d798e05

Use more explicit error strings in HttpsConfigurationService.EnsureIn…

6236efd

…itialized

JamesNK approved these changes Apr 14, 2023

View reviewed changes

amcasey enabled auto-merge (squash) April 14, 2023 00:09

JamesNK reviewed Apr 14, 2023

View reviewed changes

src/Servers/Kestrel/Core/test/KestrelServerTests.cs Outdated Show resolved Hide resolved

Update src/Servers/Kestrel/Core/test/KestrelServerTests.cs

7d44b2b

amcasey merged commit f1bbdd4 into dotnet:main Apr 14, 2023

ghost added this to the 8.0-preview4 milestone Apr 14, 2023

amcasey deleted the PayForPlay branch April 14, 2023 02:49

This comment was marked as off-topic.

Sign in to view

amcasey mentioned this pull request Apr 18, 2023

Log if a server certificate lacks the subjectAlternativeName extensions #47678

Merged

4 tasks

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make TLS & QUIC Pay-for-Play (Redux) #47454

Make TLS & QUIC Pay-for-Play (Redux) #47454

amcasey commented Mar 27, 2023 •

edited

Loading

amcasey commented Mar 27, 2023

amcasey commented Mar 28, 2023

azure-pipelines bot commented Mar 28, 2023

amcasey commented Mar 29, 2023 •

edited

Loading

amcasey commented Mar 29, 2023

amcasey commented Mar 30, 2023 •

edited

Loading

amcasey commented Mar 31, 2023

ghost commented Apr 5, 2023

davidfowl Apr 5, 2023

amcasey Apr 5, 2023

davidfowl Apr 5, 2023

amcasey Apr 5, 2023

amcasey Apr 5, 2023 •

edited

Loading

amcasey commented Apr 10, 2023

amcasey commented Apr 11, 2023

amcasey commented Apr 12, 2023

JamesNK commented Apr 13, 2023

amcasey commented Apr 13, 2023 •

edited

Loading

JamesNK commented Apr 13, 2023

JamesNK left a comment

eerhardt commented Apr 14, 2023

eerhardt commented Apr 14, 2023

This comment was marked as off-topic.

Make TLS & QUIC Pay-for-Play (Redux) #47454

Make TLS & QUIC Pay-for-Play (Redux) #47454

Conversation

amcasey commented Mar 27, 2023 • edited Loading

amcasey commented Mar 27, 2023

amcasey commented Mar 28, 2023

azure-pipelines bot commented Mar 28, 2023

amcasey commented Mar 29, 2023 • edited Loading

amcasey commented Mar 29, 2023

amcasey commented Mar 30, 2023 • edited Loading

amcasey commented Mar 31, 2023

ghost commented Apr 5, 2023

davidfowl Apr 5, 2023

Choose a reason for hiding this comment

amcasey Apr 5, 2023

Choose a reason for hiding this comment

davidfowl Apr 5, 2023

Choose a reason for hiding this comment

amcasey Apr 5, 2023

Choose a reason for hiding this comment

amcasey Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

amcasey commented Apr 10, 2023

amcasey commented Apr 11, 2023

amcasey commented Apr 12, 2023

JamesNK commented Apr 13, 2023

amcasey commented Apr 13, 2023 • edited Loading

JamesNK commented Apr 13, 2023

JamesNK left a comment

Choose a reason for hiding this comment

eerhardt commented Apr 14, 2023

eerhardt commented Apr 14, 2023

This comment was marked as off-topic.

amcasey commented Mar 27, 2023 •

edited

Loading

amcasey commented Mar 29, 2023 •

edited

Loading

amcasey commented Mar 30, 2023 •

edited

Loading

amcasey Apr 5, 2023 •

edited

Loading

amcasey commented Apr 13, 2023 •

edited

Loading