diff --git a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs index e81340454f76..297fd606148b 100644 --- a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs +++ b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs @@ -77,6 +77,7 @@ internal KestrelConfigurationLoader( private IList EndpointsToAdd { get; } = new List(); private CertificateConfig? DefaultCertificateConfig { get; set; } + internal X509Certificate2? DefaultCertificate { get; set; } /// /// Specifies a configuration Action to run when an endpoint with the given name is loaded from configuration. @@ -270,9 +271,10 @@ public void Load() { var endpointsToStop = Options.ConfigurationBackedListenOptions.ToList(); var endpointsToStart = new List(); + var endpointsToReuse = new List(); - Options.ConfigurationBackedListenOptions.Clear(); DefaultCertificateConfig = null; + DefaultCertificate = null; ConfigurationReader = new ConfigurationReader(Configuration); @@ -336,7 +338,7 @@ public void Load() if (httpsOptions.ServerCertificate == null && httpsOptions.ServerCertificateSelector == null) { // Fallback - Options.ApplyDefaultCert(httpsOptions); + Options.ApplyDefaultCertificate(httpsOptions); // Ensure endpoint is reloaded if it used the default certificate and the certificate changed. endpoint.Certificate = DefaultCertificateConfig; @@ -350,7 +352,7 @@ public void Load() if (matchingBoundEndpoints.Count > 0) { endpointsToStop.RemoveAll(o => o.EndpointConfig == endpoint); - Options.ConfigurationBackedListenOptions.AddRange(matchingBoundEndpoints); + endpointsToReuse.AddRange(matchingBoundEndpoints); continue; } @@ -390,9 +392,16 @@ public void Load() listenOptions.EndpointConfig = endpoint; endpointsToStart.Add(listenOptions); - Options.ConfigurationBackedListenOptions.Add(listenOptions); } + // Update ConfigurationBackedListenOptions after everything else has been processed so that + // it's left in a good state (i.e. its former state) if something above throws an exception. + // Note that this isn't foolproof - we could run out of memory or something - but it covers + // exceptions resulting from user misconfiguration (e.g. bad endpoint cert password). + Options.ConfigurationBackedListenOptions.Clear(); + Options.ConfigurationBackedListenOptions.AddRange(endpointsToReuse); + Options.ConfigurationBackedListenOptions.AddRange(endpointsToStart); + return (endpointsToStop, endpointsToStart); } @@ -404,7 +413,7 @@ private void LoadDefaultCert() if (defaultCert != null) { DefaultCertificateConfig = defaultCertConfig; - Options.DefaultCertificate = defaultCert; + DefaultCertificate = defaultCert; } } else @@ -414,7 +423,7 @@ private void LoadDefaultCert() { Logger.LocatedDevelopmentCertificate(certificate); DefaultCertificateConfig = certificateConfig; - Options.DefaultCertificate = certificate; + DefaultCertificate = certificate; } } } diff --git a/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs b/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs index c216af615b8d..3aa1ca76607d 100644 --- a/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs +++ b/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs @@ -151,14 +151,22 @@ public class KestrelServerOptions private Action HttpsDefaults { get; set; } = _ => { }; /// - /// The default server certificate for https endpoints. This is applied lazily after HttpsDefaults and user options. + /// The development server certificate for https endpoints. This is applied lazily after HttpsDefaults and user options. /// - internal X509Certificate2? DefaultCertificate { get; set; } + /// + /// Getter exposed for testing. + /// + internal X509Certificate2? DevelopmentCertificate { get; private set; } + + /// + /// Allow tests to explicitly set the default certificate. + /// + internal X509Certificate2? TestOverrideDefaultCertificate { get; set; } /// /// Has the default dev certificate load been attempted? /// - internal bool IsDevCertLoaded { get; set; } + internal bool IsDevelopmentCertificateLoaded { get; set; } /// /// Internal AppContext switch to toggle the WebTransport and HTTP/3 datagrams experiemental features. @@ -227,16 +235,34 @@ internal void ApplyHttpsDefaults(HttpsConnectionAdapterOptions httpsOptions) HttpsDefaults(httpsOptions); } - internal void ApplyDefaultCert(HttpsConnectionAdapterOptions httpsOptions) + internal void ApplyDefaultCertificate(HttpsConnectionAdapterOptions httpsOptions) { if (httpsOptions.ServerCertificate != null || httpsOptions.ServerCertificateSelector != null) { return; } - EnsureDefaultCert(); + if (TestOverrideDefaultCertificate is X509Certificate2 certificateFromTest) + { + httpsOptions.ServerCertificate = certificateFromTest; + return; + } + + if (ConfigurationLoader?.DefaultCertificate is X509Certificate2 certificateFromLoader) + { + httpsOptions.ServerCertificate = certificateFromLoader; + return; + } - httpsOptions.ServerCertificate = DefaultCertificate; + if (!IsDevelopmentCertificateLoaded) + { + IsDevelopmentCertificateLoaded = true; + Debug.Assert(DevelopmentCertificate is null); + var logger = ApplicationServices!.GetRequiredService>(); + DevelopmentCertificate = GetDevelopmentCertificateFromStore(logger); + } + + httpsOptions.ServerCertificate = DevelopmentCertificate; } internal void Serialize(Utf8JsonWriter writer) @@ -253,8 +279,8 @@ internal void Serialize(Utf8JsonWriter writer) writer.WritePropertyName(nameof(AllowResponseHeaderCompression)); writer.WriteBooleanValue(AllowResponseHeaderCompression); - writer.WritePropertyName(nameof(IsDevCertLoaded)); - writer.WriteBooleanValue(IsDevCertLoaded); + writer.WritePropertyName(nameof(IsDevelopmentCertificateLoaded)); + writer.WriteBooleanValue(IsDevelopmentCertificateLoaded); writer.WriteString(nameof(RequestHeaderEncodingSelector), RequestHeaderEncodingSelector == DefaultHeaderEncodingSelector ? "default" : "configured"); writer.WriteString(nameof(ResponseHeaderEncodingSelector), ResponseHeaderEncodingSelector == DefaultHeaderEncodingSelector ? "default" : "configured"); @@ -280,46 +306,44 @@ internal void Serialize(Utf8JsonWriter writer) writer.WriteEndArray(); } - private void EnsureDefaultCert() + private static X509Certificate2? GetDevelopmentCertificateFromStore(ILogger logger) { - if (DefaultCertificate == null && !IsDevCertLoaded) + try { - IsDevCertLoaded = true; // Only try once - var logger = ApplicationServices!.GetRequiredService>(); - try + var cert = CertificateManager.Instance.ListCertificates(StoreName.My, StoreLocation.CurrentUser, isValid: true, requireExportable: false) + .FirstOrDefault(); + + if (cert is null) { - DefaultCertificate = CertificateManager.Instance.ListCertificates(StoreName.My, StoreLocation.CurrentUser, isValid: true, requireExportable: false) - .FirstOrDefault(); - - if (DefaultCertificate != null) - { - var status = CertificateManager.Instance.CheckCertificateState(DefaultCertificate, interactive: false); - if (!status.Success) - { - // Display a warning indicating to the user that a prompt might appear and provide instructions on what to do in that - // case. The underlying implementation of this check is specific to Mac OS and is handled within CheckCertificateState. - // Kestrel must NEVER cause a UI prompt on a production system. We only attempt this here because Mac OS is not supported - // in production. - Debug.Assert(status.FailureMessage != null, "Status with a failure result must have a message."); - logger.DeveloperCertificateFirstRun(status.FailureMessage); - - // Prevent binding to HTTPS if the certificate is not valid (avoid the prompt) - DefaultCertificate = null; - } - else if (!CertificateManager.Instance.IsTrusted(DefaultCertificate)) - { - logger.DeveloperCertificateNotTrusted(); - } - } - else - { - logger.UnableToLocateDevelopmentCertificate(); - } + logger.UnableToLocateDevelopmentCertificate(); + return null; } - catch + + var status = CertificateManager.Instance.CheckCertificateState(cert, interactive: false); + if (!status.Success) { - logger.UnableToLocateDevelopmentCertificate(); + // Display a warning indicating to the user that a prompt might appear and provide instructions on what to do in that + // case. The underlying implementation of this check is specific to Mac OS and is handled within CheckCertificateState. + // Kestrel must NEVER cause a UI prompt on a production system. We only attempt this here because Mac OS is not supported + // in production. + Debug.Assert(status.FailureMessage != null, "Status with a failure result must have a message."); + logger.DeveloperCertificateFirstRun(status.FailureMessage); + + // Prevent binding to HTTPS if the certificate is not valid (avoid the prompt) + return null; } + + if (!CertificateManager.Instance.IsTrusted(cert)) + { + logger.DeveloperCertificateNotTrusted(); + } + + return cert; + } + catch + { + logger.UnableToLocateDevelopmentCertificate(); + return null; } } diff --git a/src/Servers/Kestrel/Core/src/ListenOptionsHttpsExtensions.cs b/src/Servers/Kestrel/Core/src/ListenOptionsHttpsExtensions.cs index 175b8bed8163..3463a56828e7 100644 --- a/src/Servers/Kestrel/Core/src/ListenOptionsHttpsExtensions.cs +++ b/src/Servers/Kestrel/Core/src/ListenOptionsHttpsExtensions.cs @@ -166,7 +166,7 @@ public static ListenOptions UseHttps(this ListenOptions listenOptions, Action().Addresses.Add("https://127.0.0.1:0"); @@ -70,7 +70,7 @@ public void StartWithHttpsAddressConfiguresHttpsEndpoints() public void KestrelServerThrowsUsefulExceptionIfDefaultHttpsProviderNotAdded() { var options = CreateServerOptions(); - options.IsDevCertLoaded = true; // Prevent the system default from being loaded + options.IsDevelopmentCertificateLoaded = true; // Prevent the system default from being loaded using (var server = CreateServer(options, throwOnCriticalErrors: false)) { server.Features.Get().Addresses.Add("https://127.0.0.1:0"); diff --git a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs index 36683b4c81c0..8f683984f907 100644 --- a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs +++ b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs @@ -241,7 +241,7 @@ public void ConfigureEndpointDevelopmentCertificateGetsLoadedWhenPresent() }).Load(); Assert.True(ran1); - Assert.NotNull(serverOptions.DefaultCertificate); + Assert.Null(serverOptions.DevelopmentCertificate); // Not used since configuration cert is present } finally { @@ -252,6 +252,133 @@ public void ConfigureEndpointDevelopmentCertificateGetsLoadedWhenPresent() } } + [Fact] + public void DevelopmentCertificateCanBeRemoved() + { + try + { + var serverOptions = CreateServerOptions(); + + var devCert = new X509Certificate2(TestResources.GetCertPath("aspnetdevcert.pfx"), "testPassword", X509KeyStorageFlags.Exportable); + var devCertBytes = devCert.Export(X509ContentType.Pkcs12, "1234"); + var devCertPath = GetCertificatePath(); + Directory.CreateDirectory(Path.GetDirectoryName(devCertPath)); + File.WriteAllBytes(devCertPath, devCertBytes); + + var defaultCertPath = TestResources.TestCertificatePath; + var defaultCert = TestResources.GetTestCertificate(); + Assert.NotEqual(devCert.SerialNumber, defaultCert.SerialNumber); // Need to be able to distinguish them + + var endpointConfig = new[] + { + new KeyValuePair("Endpoints:End1:Url", "https://*:5001"), + }; + var devCertConfig = new[] + { + new KeyValuePair("Certificates:Development:Password", "1234"), + }; + var defaultCertConfig = new[] + { + new KeyValuePair("Certificates:Default:path", defaultCertPath), + new KeyValuePair("Certificates:Default:Password", "testPassword"), + }; + + var config = new ConfigurationBuilder().AddInMemoryCollection(endpointConfig.Concat(devCertConfig)).Build(); + + serverOptions.Configure(config).Load(); + + CheckCertificates(devCert); + + // Add Default certificate + serverOptions.ConfigurationLoader.Configuration = new ConfigurationBuilder().AddInMemoryCollection(endpointConfig.Concat(devCertConfig).Concat(defaultCertConfig)).Build(); + _ = serverOptions.ConfigurationLoader.Reload(); + + // Default is preferred to Development + CheckCertificates(defaultCert); + + // Remove Default certificate + serverOptions.ConfigurationLoader.Configuration = new ConfigurationBuilder().AddInMemoryCollection(endpointConfig.Concat(devCertConfig)).Build(); + _ = serverOptions.ConfigurationLoader.Reload(); + + // Back to Development + CheckCertificates(devCert); + + // Remove Development certificate + serverOptions.ConfigurationLoader.Configuration = new ConfigurationBuilder().AddInMemoryCollection(endpointConfig).Build(); + + // With all of the configuration certs removed, the only place left to check is the CertificateManager. + // We don't want to depend on machine state, so we cheat and say we already looked. + serverOptions.IsDevelopmentCertificateLoaded = true; + Assert.Null(serverOptions.DevelopmentCertificate); + + // Since there are no configuration certs and we bypassed the CertificateManager, there will be an + // exception about not finding any certs at all. + Assert.Throws(() => serverOptions.ConfigurationLoader.Reload()); + + Assert.Null(serverOptions.ConfigurationLoader.DefaultCertificate); + + void CheckCertificates(X509Certificate2 expectedCert) + { + var httpsOptions = new HttpsConnectionAdapterOptions(); + serverOptions.ApplyDefaultCertificate(httpsOptions); + Assert.Equal(expectedCert.SerialNumber, httpsOptions.ServerCertificate.SerialNumber); + Assert.Equal(expectedCert.SerialNumber, serverOptions.ConfigurationLoader.DefaultCertificate.SerialNumber); + } + } + finally + { + if (File.Exists(GetCertificatePath())) + { + File.Delete(GetCertificatePath()); + } + } + } + + [Fact] + public void ConfigureEndpoint_RecoverFromBadPassword() + { + var serverOptions = CreateServerOptions(); + + var configRoot = new ConfigurationBuilder().AddInMemoryCollection(new[] + { + new KeyValuePair("Endpoints:End1:Url", "https://*:5001"), + new KeyValuePair("Endpoints:End1:Certificate:Path", TestResources.TestCertificatePath), + new KeyValuePair("Endpoints:End1:Certificate:Password", "testPassword") + }).Build(); + var configProvider = configRoot.Providers.Single(); + + var testCertificate = TestResources.GetTestCertificate(); + + var otherCertificatePath = TestResources.GetCertPath("aspnetdevcert.pfx"); + var otherCertificate = new X509Certificate2(otherCertificatePath, "testPassword"); + + serverOptions.Configure(configRoot).Load(); + CheckListenOptions(testCertificate); + + // Update cert but use incorrect password + configProvider.Set("Endpoints:End1:Certificate:Path", otherCertificatePath); + configProvider.Set("Endpoints:End1:Certificate:Password", "badPassword"); + + // Fails to load certificate because password is bad + Assert.ThrowsAny(() => serverOptions.ConfigurationLoader.Reload()); + + // ConfigurationBackedListenOptions still contains prior value + CheckListenOptions(testCertificate); + + // Correct password + configProvider.Set("Endpoints:End1:Certificate:Password", "testPassword"); + _ = serverOptions.ConfigurationLoader.Reload(); + + // ConfigurationBackedListenOptions contains new value + CheckListenOptions(otherCertificate); + + void CheckListenOptions(X509Certificate2 expectedCert) + { + var listenOptions = Assert.Single(serverOptions.ConfigurationBackedListenOptions); + Assert.Equal(expectedCert.SerialNumber, listenOptions.HttpsOptions.ServerCertificate.SerialNumber); + } + } + [Fact] public void ConfigureEndpoint_ThrowsWhen_The_PasswordIsMissing() { @@ -390,7 +517,7 @@ public void ConfigureEndpoint_CanLoadPemCertificates(string certificateFile, str }).Load(); Assert.True(ran1); - Assert.NotNull(serverOptions.DefaultCertificate); + Assert.Null(serverOptions.DevelopmentCertificate); // Not used since configuration cert is present } [Fact] @@ -414,7 +541,7 @@ public void ConfigureEndpointDevelopmentCertificateGetsIgnoredIfPasswordIsNotCor .Configure(config) .Load(); - Assert.Null(serverOptions.DefaultCertificate); + Assert.Null(serverOptions.DevelopmentCertificate); } finally { @@ -445,7 +572,7 @@ public void ConfigureEndpointDevelopmentCertificateGetsIgnoredIfPfxFileDoesNotEx .Configure(config) .Load(); - Assert.Null(serverOptions.DefaultCertificate); + Assert.Null(serverOptions.DevelopmentCertificate); } finally { diff --git a/src/Servers/Kestrel/test/BindTests/AddressRegistrationTests.cs b/src/Servers/Kestrel/test/BindTests/AddressRegistrationTests.cs index 2ee418207fa8..9c265e115a46 100644 --- a/src/Servers/Kestrel/test/BindTests/AddressRegistrationTests.cs +++ b/src/Servers/Kestrel/test/BindTests/AddressRegistrationTests.cs @@ -506,7 +506,7 @@ private async Task RegisterDefaultServerAddresses_Success(IEnumerable ad { if (mockHttps) { - options.DefaultCertificate = TestResources.GetTestCertificate(); + options.TestOverrideDefaultCertificate = TestResources.GetTestCertificate(); } }) .Configure(ConfigureEchoAddress); diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs index 68929b4f167a..ca4a64b0ac7a 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs @@ -70,6 +70,7 @@ public async Task CanReadAndWriteWithHttpsConnectionMiddlewareWithPemCertificate var logger = serviceProvider.GetRequiredService>(); var httpsLogger = serviceProvider.GetRequiredService>(); var loader = new KestrelConfigurationLoader(options, configuration, env.Object, reloadOnChange: false, logger, httpsLogger); + options.ConfigurationLoader = loader; // Since we're constructing it explicitly, we have to hook it up explicitly loader.Load(); void ConfigureListenOptions(ListenOptions listenOptions) diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs index c78429ea1e9a..3ee91abd9d8b 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs @@ -44,14 +44,14 @@ private KestrelServerOptions CreateServerOptions() public void UseHttpsDefaultsToDefaultCert() { var serverOptions = CreateServerOptions(); - serverOptions.DefaultCertificate = _x509Certificate2; + serverOptions.TestOverrideDefaultCertificate = _x509Certificate2; serverOptions.ListenLocalhost(5000, options => { options.UseHttps(); }); - Assert.False(serverOptions.IsDevCertLoaded); + Assert.False(serverOptions.IsDevelopmentCertificateLoaded); serverOptions.ListenLocalhost(5001, options => { @@ -61,7 +61,7 @@ public void UseHttpsDefaultsToDefaultCert() Assert.Null(opt.ServerCertificate); }); }); - Assert.False(serverOptions.IsDevCertLoaded); + Assert.False(serverOptions.IsDevelopmentCertificateLoaded); } [Fact] @@ -115,8 +115,8 @@ public void ConfigureHttpsDefaultsNeverLoadsDefaultCert() }); }); // Never lazy loaded - Assert.False(serverOptions.IsDevCertLoaded); - Assert.Null(serverOptions.DefaultCertificate); + Assert.False(serverOptions.IsDevelopmentCertificateLoaded); + Assert.Null(serverOptions.DevelopmentCertificate); } [Fact] @@ -143,8 +143,8 @@ public void ConfigureCertSelectorNeverLoadsDefaultCert() }); }); // Never lazy loaded - Assert.False(serverOptions.IsDevCertLoaded); - Assert.Null(serverOptions.DefaultCertificate); + Assert.False(serverOptions.IsDevelopmentCertificateLoaded); + Assert.Null(serverOptions.DevelopmentCertificate); } [ConditionalFact] @@ -409,7 +409,7 @@ public async Task HandshakeTimesOutAndIsLoggedAsDebugWithAsyncCallback() public async Task Http3_UseHttpsNoArgsWithDefaultCertificate_UseDefaultCertificate() { var serverOptions = CreateServerOptions(); - serverOptions.DefaultCertificate = _x509Certificate2; + serverOptions.TestOverrideDefaultCertificate = _x509Certificate2; IFeatureCollection bindFeatures = null; var multiplexedConnectionListenerFactory = new MockMultiplexedConnectionListenerFactory(); @@ -494,7 +494,7 @@ public async Task Http3_ConfigureHttpsDefaults_Works() public async Task Http1And2And3_NoUseHttps_MultiplexBindNotCalled() { var serverOptions = CreateServerOptions(); - serverOptions.DefaultCertificate = _x509Certificate2; + serverOptions.TestOverrideDefaultCertificate = _x509Certificate2; var bindCalled = false; var multiplexedConnectionListenerFactory = new MockMultiplexedConnectionListenerFactory(); @@ -528,7 +528,7 @@ public async Task Http1And2And3_NoUseHttps_MultiplexBindNotCalled() public async Task Http3_NoUseHttps_Throws() { var serverOptions = CreateServerOptions(); - serverOptions.DefaultCertificate = _x509Certificate2; + serverOptions.TestOverrideDefaultCertificate = _x509Certificate2; var bindCalled = false; var multiplexedConnectionListenerFactory = new MockMultiplexedConnectionListenerFactory(); @@ -564,7 +564,7 @@ public async Task Http3_NoUseHttps_Throws() public async Task Http3_ServerOptionsSelectionCallback_Works() { var serverOptions = CreateServerOptions(); - serverOptions.DefaultCertificate = _x509Certificate2; + serverOptions.TestOverrideDefaultCertificate = _x509Certificate2; IFeatureCollection bindFeatures = null; var multiplexedConnectionListenerFactory = new MockMultiplexedConnectionListenerFactory(); @@ -610,7 +610,7 @@ public async Task Http3_ServerOptionsSelectionCallback_Works() public async Task Http3_TlsHandshakeCallbackOptions_Works() { var serverOptions = CreateServerOptions(); - serverOptions.DefaultCertificate = _x509Certificate2; + serverOptions.TestOverrideDefaultCertificate = _x509Certificate2; IFeatureCollection bindFeatures = null; var multiplexedConnectionListenerFactory = new MockMultiplexedConnectionListenerFactory();