Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 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
26 changes: 26 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,30 @@ 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 HasDefaultOrDevelopmentCertificate
Copy link
Member

Choose a reason for hiding this comment

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

Not super enthusiastic about a property getter having side effects.

Copy link
Member

@Tratcher Tratcher May 4, 2023

Choose a reason for hiding this comment

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

Yeah, make it a method.

bool CheckForDefaultCertificate()

The development certificate counts as a default and is explained in the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

What side-effect are we worried about? Enabling https config?

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 familiar with the bool CheckFoo() naming convention - personally, I'd expect that method to throw if the condition weren't satisfied. I take it we use that elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed and made it a method, but I'd still like to understand more about side-effects.

Copy link
Member Author

Choose a reason for hiding this comment

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

Incidentally, now that it doesn't actually return the cert, I would be fine with this throwing if https configuration isn't enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

The point of the method is to avoid throwing for known, common conditions. Exceptions should only be used for unexpected conditions.

Sorry, I'm not sure which part of the thread you're responding to. If it was about my expectation that CheckFoo would throw, that was an intuition about the naming convention and not a comment on how this member (whatever name and shape it ends up taking) should behave.

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 a friendly guard method, letting you check the state in a controlled way to avoid an exception later. It should not throw an exception.

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 Asked me to have it throw in that case. Would you like me to work up a version that loads eagerly and then compensates if the configuration changes?

Copy link
Member

Choose a reason for hiding this comment

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

If you're getting conflicting advice we should hold off until we can discuss it in a meeting, API review or otherwise.

{
get
{
if (ConfigurationLoader is { IsLoaded: false })
{
throw new InvalidOperationException(CoreStrings.NeedConfigurationToRetrieveServerCertificate);
}

// We consider calls to `HasDefaultOrDevelopmentCertificate.get` 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.HasDefaultOrDevelopmentCertificate.get -> 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 HasDefaultOrDevelopmentCertificateJustWorks()
{
var hostBuilder = new WebHostBuilder()
.UseKestrelCore()
.ConfigureKestrel(serverOptions =>
{
var testCertificate = new X509Certificate2(Path.Combine("shared", "TestCertificates", "aspnetdevcert.pfx"), "testPassword");
serverOptions.TestOverrideDefaultCertificate = testCertificate;

Assert.True(serverOptions.HasDefaultOrDevelopmentCertificate);
})
.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 HasDefaultOrDevelopmentCertificate_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.HasDefaultOrDevelopmentCertificate);
}
finally
{
if (File.Exists(GetCertificatePath()))
{
File.Delete(GetCertificatePath());
}
}
}

[Fact]
public void HasDefaultOrDevelopmentCertificate_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.HasDefaultOrDevelopmentCertificate);
}

[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 HasDefaultOrDevelopmentCertificate_ConfigurationNotLoaded()
{
var serverOptions = CreateServerOptions();
serverOptions.TestOverrideDefaultCertificate = _x509Certificate2;

serverOptions.Configure();

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

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

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

Assert.True(serverOptions.HasDefaultOrDevelopmentCertificate);
}

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

Assert.True(serverOptions.HasDefaultOrDevelopmentCertificate);
}

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

Assert.False(serverOptions.HasDefaultOrDevelopmentCertificate);
}

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

Assert.True(serverOptions.HasDefaultOrDevelopmentCertificate);
}

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