Skip to content

Introduce KestrelServerOptions.CheckDefaultCertificate #48057

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 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Servers/Kestrel/Core/src/CoreStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -734,4 +734,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l
<data name="NeedHttpsConfigurationToBindHttpsAddresses" xml:space="preserve">
<value>Call UseKestrelHttpsConfiguration() on IWebHostBuilder to automatically enable HTTPS when an https:// address is used.</value>
</data>
<data name="NeedConfigurationToRetrieveServerCertificate" xml:space="preserve">
<value>Call KestrelConfigurationLoader.Load() before retrieving the server certificate.</value>
</data>
</root>
6 changes: 3 additions & 3 deletions src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public class KestrelConfigurationLoader
{
private readonly IHttpsConfigurationService _httpsConfigurationService;

private bool _loaded;
internal bool IsLoaded { get; private set; }

internal KestrelConfigurationLoader(
KestrelServerOptions options,
Expand Down Expand Up @@ -234,12 +234,12 @@ internal void ApplyHttpsDefaults(HttpsConnectionAdapterOptions httpsOptions)
/// </summary>
public void Load()
{
if (_loaded)
if (IsLoaded)
{
// The loader has already been run.
return;
}
_loaded = true;
IsLoaded = true;

Reload();

Expand Down
23 changes: 23 additions & 0 deletions src/Servers/Kestrel/Core/src/KestrelServerOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -585,4 +585,27 @@ public void ListenNamedPipe(string pipeName, Action<ListenOptions> configure)
configure(listenOptions);
CodeBackedListenOptions.Add(listenOptions);
}

/// <summary>
/// Return true if there is a default or development server certificate.
/// Should not be called before the configuration is loaded, if there is one.
/// </summary>
/// <returns>True if there is a default or development server certificate, false otherwise.</returns>
/// <exception cref="InvalidOperationException">If there is a configuration and it has not been loaded.</exception>
public bool CheckDefaultCertificate()
{
if (ConfigurationLoader is { IsLoaded: false })
Copy link
Member

Choose a reason for hiding this comment

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

Can this call ConfigurationLoader.Load() instead of throwing?

Copy link
Member Author

Choose a reason for hiding this comment

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

@halter73 Suggested doing it this way because invoking Load ourselves would put the burden on us to reload on change.

Copy link
Member

Choose a reason for hiding this comment

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

We'd have to reload if CheckDefaultCertificate were called again? That seems reasonable.

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 understood him to say that config changes between the first call to Load and bind-time would be lost since bind wouldn't attempt to reload and already loaded configuration.

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.e. other consumers of the configuration might see stale values.)

Copy link
Member

Choose a reason for hiding this comment

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

@halter73 Suggested doing it this way because invoking Load ourselves would put the burden on us to reload on change.

We should capture the reload token during the first call to Load regardless. If we start calling Load ourselves in more places, we should fix the bug at the same time. Otherwise, we risk not picking up new endpoints or certa added to Kestrel config between the manual call to Load and a later call to StartAsync. We could always call Reload during StarAsync, but it'd be more efficient to only reload if the configuration has actually changed.

That said, this is a pretty narrow edge case and an existing big.

{
throw new InvalidOperationException(CoreStrings.NeedConfigurationToRetrieveServerCertificate);
}

// We consider calls to `CheckDefaultCertificate` to be a clear expression of user intent to pull in HTTPS configuration support
EnableHttpsConfiguration();

var httpsOptions = new HttpsConnectionAdapterOptions();
ApplyHttpsDefaults(httpsOptions);
ApplyDefaultCertificate(httpsOptions);

return httpsOptions.ServerCertificate is not null;
}
}
3 changes: 2 additions & 1 deletion src/Servers/Kestrel/Core/src/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#nullable enable
Microsoft.AspNetCore.Server.Kestrel.Core.Features.ISslStreamFeature
Microsoft.AspNetCore.Server.Kestrel.Core.Features.ISslStreamFeature.SslStream.get -> System.Net.Security.SslStream!
Microsoft.AspNetCore.Server.Kestrel.Core.KestrelServerOptions.CheckDefaultCertificate() -> bool
Microsoft.AspNetCore.Server.Kestrel.Core.KestrelServerOptions.ListenNamedPipe(string! pipeName) -> void
Microsoft.AspNetCore.Server.Kestrel.Core.KestrelServerOptions.ListenNamedPipe(string! pipeName, System.Action<Microsoft.AspNetCore.Server.Kestrel.Core.ListenOptions!>! configure) -> void
Microsoft.AspNetCore.Server.Kestrel.Core.ListenOptions.PipeName.get -> string?
Microsoft.AspNetCore.Server.Kestrel.Core.ListenOptions.PipeName.get -> string?
23 changes: 23 additions & 0 deletions src/Servers/Kestrel/Kestrel/test/HttpsConfigurationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,29 @@ public async Task LoadEndpointCertificate(string address, bool useKestrelHttpsCo
}
}

[Fact]
public async Task CheckDefaultCertificateJustWorks()
{
var hostBuilder = new WebHostBuilder()
.UseKestrelCore()
.ConfigureKestrel(serverOptions =>
{
var testCertificate = new X509Certificate2(Path.Combine("shared", "TestCertificates", "aspnetdevcert.pfx"), "testPassword");
serverOptions.TestOverrideDefaultCertificate = testCertificate;

Assert.True(serverOptions.CheckDefaultCertificate());
})
.Configure(app => { });

var host = hostBuilder.Build();

// Binding succeeds
await host.StartAsync();
await host.StopAsync();

Assert.True(host.Services.GetRequiredService<IHttpsConfigurationService>().IsInitialized);
}

[Fact]
public async Task UseHttpsJustWorks()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,56 @@ void CheckListenOptions(X509Certificate2 expectedCert)
}
}

[Fact]
public void CheckDefaultCertificate_DevelopmentCertificate()
{
try
{
var serverOptions = CreateServerOptions();
var certificate = new X509Certificate2(TestResources.GetCertPath("aspnetdevcert.pfx"), "testPassword", X509KeyStorageFlags.Exportable);
var bytes = certificate.Export(X509ContentType.Pkcs12, "1234");
var path = GetCertificatePath();
Directory.CreateDirectory(Path.GetDirectoryName(path));
File.WriteAllBytes(path, bytes);

var config = new ConfigurationBuilder().AddInMemoryCollection(new[]
{
new KeyValuePair<string, string>("Certificates:Development:Password", "1234"),
}).Build();

serverOptions.Configure(config);
serverOptions.ConfigurationLoader.Load();

Assert.True(serverOptions.CheckDefaultCertificate());
}
finally
{
if (File.Exists(GetCertificatePath()))
{
File.Delete(GetCertificatePath());
}
}
}

[Fact]
public void CheckDefaultCertificate_DefaultCertificate()
{
var certificate = new X509Certificate2(TestResources.TestCertificatePath, "testPassword", X509KeyStorageFlags.Exportable);

var serverOptions = CreateServerOptions();

var config = new ConfigurationBuilder().AddInMemoryCollection(new[]
{
new KeyValuePair<string, string>("Certificates:Default:Path", TestResources.TestCertificatePath),
new KeyValuePair<string, string>("Certificates:Default:Password", "testPassword")
}).Build();

serverOptions.Configure(config);
serverOptions.ConfigurationLoader.Load();

Assert.True(serverOptions.CheckDefaultCertificate());
}

[Fact]
public void ConfigureEndpoint_ThrowsWhen_The_PasswordIsMissing()
{
Expand Down
53 changes: 53 additions & 0 deletions src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,59 @@ await sslStream.AuthenticateAsClientAsync("127.0.0.1", clientCertificates: null,
Assert.True(onAuthenticateCalled, "onAuthenticateCalled");
}

[Fact]
public void CheckDefaultCertificate_ConfigurationNotLoaded()
{
var serverOptions = CreateServerOptions();
serverOptions.TestOverrideDefaultCertificate = _x509Certificate2;

serverOptions.Configure();

Assert.Throws<InvalidOperationException>(() => serverOptions.CheckDefaultCertificate());
}

[Fact]
public void CheckDefaultCertificate_ConfigurationLoaded()
{
var serverOptions = CreateServerOptions();
serverOptions.TestOverrideDefaultCertificate = _x509Certificate2;

serverOptions.Configure();
serverOptions.ConfigurationLoader.Load();

Assert.True(serverOptions.CheckDefaultCertificate());
}

[Fact]
public void CheckDefaultCertificate_NoConfiguration()
{
var serverOptions = CreateServerOptions();
serverOptions.TestOverrideDefaultCertificate = _x509Certificate2;

Assert.True(serverOptions.CheckDefaultCertificate());
}

[Fact]
public void CheckDefaultCertificate_NoCertificate()
{
var serverOptions = CreateServerOptions();
serverOptions.IsDevelopmentCertificateLoaded = true; // Prevent the system default from being loaded

Assert.False(serverOptions.CheckDefaultCertificate());
}

[Fact]
public void CheckDefaultCertificate_FromHttpsDefaults()
{
var serverOptions = CreateServerOptions();
serverOptions.ConfigureHttpsDefaults(httpsOptions =>
{
httpsOptions.ServerCertificate = _x509Certificate2;
});

Assert.True(serverOptions.CheckDefaultCertificate());
}

private class HandshakeErrorLoggerProvider : ILoggerProvider
{
public HttpsConnectionFilterLogger FilterLogger { get; set; } = new HttpsConnectionFilterLogger();
Expand Down