Skip to content

Make TLS & QUIC Pay-for-Play #47209

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

Closed
wants to merge 8 commits into from
Closed

Conversation

amcasey
Copy link
Member

@amcasey amcasey commented Mar 15, 2023

This draft PR is intended as context for a discussion and is not ready to commit.

As discussed offline, the way to think about disabling TLS is that Kestrel won't do things automatically for you, but there are things you can do yourself even without opting in. I think of it as disabling TLS configuration - Kestrel won't call UseHttps on your behalf (e.g. because you have specified an https address) or load certificates from IConfiguration or CertificateManager. However, you remain free to specify a certificate in code and call UseHttps explicitly. This is enough to allow the cryptography types to be trimmed and seems more defensible than having Kestrel throw whenever you get to close to TLS.

In contrast, QUIC support is either on or off.
Opting out effectively means that Kestrel won't call UseQuic on your behalf.

Big changes:

  1. Introduce UseKestrelSlim, which doesn't use QUIC or TLS configuration.
  2. Introduce UseHttpsConfiguration, which opts in to TLS configuration (QUIC uses the existing UseQuic extension method).
  3. Split out the TLS portion of KestrelConfigurationLoader into a DI component, ITlsConfigurationLoader.
  4. Break AddressBinder's direct dependency on UseHttps (called for addresses that parse as https).
  5. Split TransportManager into multiplexed and non-multiplexed DI components.
  6. Make KestrelServerImpl a DI component.
  7. Make ServiceContext a DI component.
  8. Throw when a failure is caused by a missing opt-in. (In particular, distinguish between lack of QUIC support and lack of QUIC opt-in.)

Open Questions:

  1. Is there a better way to have UseQuic and UseHttpsConfiguration coordinate across assembly boundaries than introducing types like MultiplexedConnectionMarkerService?
  2. Is this a sensible place to cut off TLS support?

Originally based on @eerhardt's bffa15c

This draft PR is intended as context for a discussion and is not ready to commit.

As discussed offline, the way to think about disabling TLS is that Kestrel won't do things automatically for you, but there are things you can do yourself _even without opting in_.
I think of it as disabling TLS _configuration_ - Kestrel won't call `UseHttps` on your behalf (e.g. because you have specified an https address) or load certificates from `IConfiguration` or `CertificateManager`.
However, you remain free to specify a certificate in code and call `UseHttps` explicitly.
This is enough to allow the cryptography types to be trimmed and seems more defensible than having Kestrel throw whenever you get to close to TLS.

In contrast, QUIC support is either on or off.
Opting out effectively means that Kestrel won't call `UseQuic` on your behalf.

Big changes:

1. Introduce `UseKestrelSlim`, which doesn't use QUIC or TLS configuration.
1. Introduce `UseHttpsConfiguration`, which opts in to TLS configuration (QUIC uses the existing `UseQuic` extension method).
1. Split out the TLS portion of `KestrelConfigurationLoader` into a DI component, `ITlsConfigurationLoader`.
1. Break `AddressBinder`'s direct dependency on `UseHttps` (called for addresses that parse as https).
1. Split `TransportManager` into multiplexed and non-multiplexed DI components.
1. Make `KestrelServerImpl` a DI component.
1. Make `ServiceContext` a DI component.
1. Throw when a failure is caused by a missing opt-in.  (In particular, distinguish between lack of QUIC support and lack of QUIC opt-in.)

Open Questions:

1. Is there a better way to have `UseQuic` and `UseHttpsConfiguration` coordinate across assembly boundaries than introducing types like `MultiplexedConnectionMarkerService`?
1. Is this a sensible place to cut off TLS support?

Originally based on @eerhardt's dotnet@bffa15c
@ghost ghost added the area-runtime label Mar 15, 2023
@davidfowl
Copy link
Member

davidfowl commented Mar 15, 2023

The slim name really bugs me. I think our current naming scheme of Add/Use{Thing}Core already exists and we should continue to use it. I’ll take a deeper look at the refactoring soon 👍🏾

@amcasey
Copy link
Member Author

amcasey commented Mar 15, 2023

The slim name really bugs me. I think our current naming scheme of Add/Use{Thing}Core already exists and we should continue to use it. I’ll take a deeper look at the refactoring soon 👍🏾

I don't feel strongly about "slim", but I did consciously choose it over "core" because leaving out exactly these two things didn't feel "core" to me. I expect vigorous discussion. 😉


Task StopAsync(CancellationToken cancellationToken);
Task StopEndpointsAsync(List<EndpointConfig> endpointsToStop, CancellationToken cancellationToken);
}
Copy link
Member

@halter73 halter73 Mar 16, 2023

Choose a reason for hiding this comment

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

Instead of adding IMultiplexedTransportManager and ITransportManager, could we register IMultiplexedHttpsFeatureFactory? I know it's internal, but it seems like the smaller change unless there's some other benefit to splitting things up.

internal interface IMultiplexedHttpsFeatureFactory
{
    void PopulateHttpsFeatures(IFeatureCollection features, ListenOptions listenOptions);
}

We could have the default implementation throw an InvalidOperationException similar to InvalidUseHttpsHelper.

Edit: It might make sense just to combine these:

internal interface IUseHttpsHelper
{
    ListenOptions UseHttps(ListenOptions listenOptions);
    void PopulateHttpsFeatures(ListenOptions listenOptions, IFeatureCollection features);
}

Or we could add an ListenOptions.Features and have UseHttps(ListenOptions listenOptions) populate it and keep the one method.

I'm a big fan of limiting the number of services even if they're internal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I'm not sure I understand this suggestion. (I had assumed I could go study IMultiplexedHttpsFeatureFactory, but I think you just invented it?) I think you might be suggesting that we add the pay-for-play functionality to a (the?) feature collection, rather than the DI container?

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented in #47454.

@amcasey
Copy link
Member Author

amcasey commented Mar 21, 2023

@davidfowl I'm getting ready to polish and add tests. Any major notes before I do?

@amcasey
Copy link
Member Author

amcasey commented Mar 22, 2023

@halter73 How's 36e2106 as a way to enable explicit UseHttps calls without UseHttpsconfiguration?

@@ -163,33 +163,28 @@ public static ListenOptions UseHttps(this ListenOptions listenOptions, Action<Ht
{
ArgumentNullException.ThrowIfNull(configureOptions);

// TODO (acasey): apply to other overloads?
listenOptions.KestrelServerOptions.EnableTlsConfigurationLoading();
Copy link
Member

Choose a reason for hiding this comment

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

// TODO (acasey): apply to other overloads?

Would we do this to enable configuring things like default SslProtocols from config? I think that makes sense. It seems like the least-breaking thing to do.

On all the other overloads, the actual cert will be unchanged by config, 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.

Agreed that doing it in all overloads makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

This covers exactly the overloads that don't say This does not use default certificates or other defaults specified via config or KestrelServerOptions.ConfigureHttpsDefaults(Action{HttpsConnectionAdapterOptions. I can't decide whether it feels more intuitive to make UseHttps imply UseHttpsConfiguration in all cases or to respect the fact that some overloads are independent of IConfiguration. I'm leaning towards the latter.

Comment on lines 50 to 53
if (_loaded)
{
Reload();
}
Copy link
Member

Choose a reason for hiding this comment

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

Given that if this is called, we know that UseHttps() is going to read the default HTTPS config immediately via ApplyHttpsDefaults and ApplyDefaultCertificate, wouldn't it be better to always reload?

That might be enough to close #45801 with this PR. It's not as good as delaying the configuration reading all the way until binding, but it fixes the primary issue without breaking dotnet-monitor detecting the lack of default cert by catching InvalidOperationExceptions.

Suggested change
if (_loaded)
{
Reload();
}
Reload();

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'm not sure I follow - what would be the benefit of reloading here, rather than where we were about to do it anyway? When we were discussing #45801, I thought it was important to defer the code-based configuration, rather than to move up the IConfiguration-based configuration.

Copy link
Member

Choose a reason for hiding this comment

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

When we were discussing #45801, I thought it was important to defer the code-based configuration, rather than to move up the IConfiguration-based configuration.

I still think it's better in principle to delay the code-based configuration to avoid extra config loading. But we tried that, and it broke dotnet-monitor.

I'm just trying to be realistic, and in the absence of an easy path forward to delay reading the configuration for UseHttps(), recognize that moving up the configuration loading in this case where we know it's going to be eagerly read is still better than not despite knowing it will get reloaded again later.

In a perfect world, we wouldn't be eagerly reading config, but we are. If and when we do delay reading config, I fully support removing the extraneous load.

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 that case, I think I'd rather leave it the way it is and then make a second change after this is merged removing the _loaded check and explaining how that mitigates #45801. I think that will be much clearer if/when someone tries to clean it up.

Copy link
Member

Choose a reason for hiding this comment

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

Are we going to test this by manually calling kso.ConfigurationLoader!.Load() before calling kso.Listen(..., lo => UseHttps())? Because that's the only way this code ever does anything at all, right?

It seems weird to me to touch this code but not try to make the common case work when it's less code just because it happens to fix another existing bug. That said, if you think this is going to cause controversy or unnecessary friction with this PR, fine. I just don't want to leave this completely unfixed in .NET 8, and putting it off always increases that risk.

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 wasn't aware that _loaded would always be false here. Can I assert that instead of checking it?

I thought the case being fixed in #45801 was explicitly calling UseHttps when running in a docker container. I assumed that was less common that configuring it by setting the address because I didn't think it had ever worked and it would have been weird to ship with the common case broken. But maybe you meant something else by "common case"?

Besides the logical tidiness of having separate commits for separate intended outcomes, the reason I'm inclined to separate them is that both the pay-for-play change and the configuration-loading-sequence change seem likely to have unintended consequences, especially in codebases with very specific expectations, and I think investigating and fixing those problems will be easier if the commits are separate.

@amcasey
Copy link
Member Author

amcasey commented Mar 23, 2023

I have two concerns with this approach to making UseHttps just work.

  1. It only lights up part of UseHttpsConfiguration. In particular, we still won't automatically call UseHttps from AddressBinder and we'll be missing the cert functionality for HTTP/3 (which could matter if the user has already called UseQuic).
  2. There's no corresponding way to light up UseQuic, e.g. because we've detected use of Protocol.Http3.

@@ -20,15 +19,11 @@ public static class WebHostBuilderQuicExtensions
/// <returns>The <see cref="IWebHostBuilder"/>.</returns>
public static IWebHostBuilder UseQuic(this IWebHostBuilder hostBuilder)
{
if (QuicListener.IsSupported)
return hostBuilder.ConfigureServices(services =>
Copy link
Member Author

Choose a reason for hiding this comment

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

As @JamesNK mentioned here, this breaks all the checks that assume the presence of a multiplexed factory indicates QUIC support and a transport that can potentially bind (e.g. here)

Copy link
Member

Choose a reason for hiding this comment

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

That's why we're going to use check IConnectionListenerFactorySelector.CanBind rather than the presence of the IMultiplexedConnectionListenerFactory to determine whether QUIC is supported where necessary, 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.

How? Wouldn't you need to pass in an endpoint?

Copy link
Member

@halter73 halter73 Mar 27, 2023

Choose a reason for hiding this comment

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

The check is here, right? With a similar check just below it in the same method.

if (hasHttp3 && _multiplexedTransportFactories.Count == 0 && !(hasHttp1 || hasHttp2))
{
throw new InvalidOperationException("This platform doesn't support QUIC or HTTP/3.");
}

So options.EndPoint, I'd assume. If none of the _multiplexedTransportFactories implement IConnectionListenerFactorySelector and say they can bind to the endpoint, then HTTP/3 isn't going to work.

@amcasey
Copy link
Member Author

amcasey commented Apr 4, 2023

Superseded by #47454, which attempts to incorporate the feedback on this PR.

@amcasey amcasey closed this Apr 4, 2023
@amcasey 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants