From 7abc89cd957433be916465a778e06e5488ba2dd8 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 21 Feb 2023 11:09:07 -0800 Subject: [PATCH 1/9] Make certificate precedence explicit 1. Values from the user 2. Values set explicitly for test purposes 3. Values from IConfiguration 4. Values from CertificateManager Note that these are all stored in separate places, so it's possible to "undo" changes if one goes away. Also, make clearing of the IConfiguration cert on reload explicit. --- .../Core/src/KestrelConfigurationLoader.cs | 6 +- .../Kestrel/Core/src/KestrelServerOptions.cs | 97 ++++++++++++------- .../Kestrel/Core/test/KestrelServerTests.cs | 2 +- .../test/KestrelConfigurationLoaderTests.cs | 4 +- .../BindTests/AddressRegistrationTests.cs | 2 +- .../InMemory.FunctionalTests/HttpsTests.cs | 12 +-- 6 files changed, 75 insertions(+), 48 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs index e81340454f76..caaeedfdc744 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. @@ -273,6 +274,7 @@ public void Load() Options.ConfigurationBackedListenOptions.Clear(); DefaultCertificateConfig = null; + DefaultCertificate = null; ConfigurationReader = new ConfigurationReader(Configuration); @@ -404,7 +406,7 @@ private void LoadDefaultCert() if (defaultCert != null) { DefaultCertificateConfig = defaultCertConfig; - Options.DefaultCertificate = defaultCert; + DefaultCertificate = defaultCert; } } else @@ -414,7 +416,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..2980a7f170b7 100644 --- a/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs +++ b/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs @@ -153,7 +153,15 @@ public class KestrelServerOptions /// /// The default 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? DefaultCertificate { get; private set; } + + /// + /// Allow tests to explicitly set the default certificate. + /// + internal X509Certificate2? TestOverrideDefaultCertificate { get; set; } /// /// Has the default dev certificate load been attempted? @@ -234,7 +242,25 @@ internal void ApplyDefaultCert(HttpsConnectionAdapterOptions httpsOptions) return; } - EnsureDefaultCert(); + if (TestOverrideDefaultCertificate is X509Certificate2 certificateFromTest) + { + httpsOptions.ServerCertificate = certificateFromTest; + return; + } + + if (ConfigurationLoader?.DefaultCertificate is X509Certificate2 certificateFromLoader) + { + httpsOptions.ServerCertificate = certificateFromLoader; + return; + } + + if (!IsDevCertLoaded) + { + IsDevCertLoaded = true; + Debug.Assert(DefaultCertificate is null); + var logger = ApplicationServices!.GetRequiredService>(); + DefaultCertificate = GetDevelopmentCertificateFromStore(logger); + } httpsOptions.ServerCertificate = DefaultCertificate; } @@ -280,46 +306,45 @@ 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 null; + } + + return cert; + } + catch + { + logger.UnableToLocateDevelopmentCertificate(); + return null; } } diff --git a/src/Servers/Kestrel/Core/test/KestrelServerTests.cs b/src/Servers/Kestrel/Core/test/KestrelServerTests.cs index 7b6d155b5103..2c8580ae56d7 100644 --- a/src/Servers/Kestrel/Core/test/KestrelServerTests.cs +++ b/src/Servers/Kestrel/Core/test/KestrelServerTests.cs @@ -54,7 +54,7 @@ public void StartWithInvalidAddressThrows() public void StartWithHttpsAddressConfiguresHttpsEndpoints() { var options = CreateServerOptions(); - options.DefaultCertificate = TestResources.GetTestCertificate(); + options.TestOverrideDefaultCertificate = TestResources.GetTestCertificate(); using (var server = CreateServer(options)) { 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..ba9a066d451b 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.DefaultCertificate); // Not used since configuration cert is present } finally { @@ -390,7 +390,7 @@ public void ConfigureEndpoint_CanLoadPemCertificates(string certificateFile, str }).Load(); Assert.True(ran1); - Assert.NotNull(serverOptions.DefaultCertificate); + Assert.Null(serverOptions.DefaultCertificate); // Not used since configuration cert is present } [Fact] 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/HttpsTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs index c78429ea1e9a..6692946a2e88 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs @@ -44,7 +44,7 @@ private KestrelServerOptions CreateServerOptions() public void UseHttpsDefaultsToDefaultCert() { var serverOptions = CreateServerOptions(); - serverOptions.DefaultCertificate = _x509Certificate2; + serverOptions.TestOverrideDefaultCertificate = _x509Certificate2; serverOptions.ListenLocalhost(5000, options => { @@ -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(); From a89fe8d4c1edf57ce7b44c9224c4e9ac9fe0d8bc Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 21 Feb 2023 16:39:50 -0800 Subject: [PATCH 2/9] Don't update ConfigurationBackedLoaders until reload succeeds Otherwise, a configuration error - e.g. a bad endpoint certificate password - could cause it to be left in a bad state, causing issues during subsequent reloads. --- .../Kestrel/Core/src/KestrelConfigurationLoader.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs index caaeedfdc744..50a4a8f2dc04 100644 --- a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs +++ b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs @@ -271,8 +271,8 @@ public void Load() { var endpointsToStop = Options.ConfigurationBackedListenOptions.ToList(); var endpointsToStart = new List(); + var endpointsToReuse = new List(); - Options.ConfigurationBackedListenOptions.Clear(); DefaultCertificateConfig = null; DefaultCertificate = null; @@ -352,7 +352,7 @@ public void Load() if (matchingBoundEndpoints.Count > 0) { endpointsToStop.RemoveAll(o => o.EndpointConfig == endpoint); - Options.ConfigurationBackedListenOptions.AddRange(matchingBoundEndpoints); + endpointsToReuse.AddRange(matchingBoundEndpoints); continue; } @@ -392,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); } From 9438e040d4d6b19cbf6acba0005bf6362bad1858 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 22 Feb 2023 12:17:40 -0800 Subject: [PATCH 3/9] Test certificate updates on config reload --- .../test/KestrelConfigurationLoaderTests.cs | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs index ba9a066d451b..fd251d063503 100644 --- a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs +++ b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs @@ -252,6 +252,80 @@ 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(); + _ = serverOptions.ConfigurationLoader.Reload(); + + Assert.Null(serverOptions.ConfigurationLoader.DefaultCertificate); + + void CheckCertificates(X509Certificate2 expectedCert) + { + var httpsOptions = new HttpsConnectionAdapterOptions(); + serverOptions.ApplyDefaultCert(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_ThrowsWhen_The_PasswordIsMissing() { From 631512c0621affcb827f19b67d15c33167cf5813 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 22 Feb 2023 12:46:10 -0800 Subject: [PATCH 4/9] Test setting a bad certificate password in config --- .../test/KestrelConfigurationLoaderTests.cs | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs index fd251d063503..1ad8c49c898d 100644 --- a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs +++ b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs @@ -326,6 +326,51 @@ void CheckCertificates(X509Certificate2 expectedCert) } } + [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() { From 612904df93ecd6c0579126a0b9c16e8edcc5561d Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 23 Feb 2023 12:53:10 -0800 Subject: [PATCH 5/9] Hack around possible absence of dev cert in store --- .../Kestrel/test/KestrelConfigurationLoaderTests.cs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs index 1ad8c49c898d..c10b7d789198 100644 --- a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs +++ b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs @@ -305,7 +305,18 @@ public void DevelopmentCertificateCanBeRemoved() // Remove Development certificate serverOptions.ConfigurationLoader.Configuration = new ConfigurationBuilder().AddInMemoryCollection(endpointConfig).Build(); - _ = serverOptions.ConfigurationLoader.Reload(); + try + { + _ = serverOptions.ConfigurationLoader.Reload(); + } + catch (InvalidOperationException e) + { + // Since there are no longer any IConfiguration certificates, we'll fall back to the certificate store. + // Unfortunately, the state of the store varies by box (usually there is a cert on dev boxes, but not + // on CI boxes) and modifying the state of the real store at test-time seems inappropriate. An alternative + // approach would be make it possible to replace CertificateManager.Instance in individual tests, but that + // seems like overkill since we only want to confirm that the configuration loader no longer has a cert. + } Assert.Null(serverOptions.ConfigurationLoader.DefaultCertificate); From 0ecec6d59a95731d87c6bdb5a32d0cc8fc8159e6 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 23 Feb 2023 14:15:58 -0800 Subject: [PATCH 6/9] Use IsDevCertLoaded to bypass the cert store entirely --- .../test/KestrelConfigurationLoaderTests.cs | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs index c10b7d789198..8c0960f9f3b6 100644 --- a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs +++ b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs @@ -305,18 +305,15 @@ public void DevelopmentCertificateCanBeRemoved() // Remove Development certificate serverOptions.ConfigurationLoader.Configuration = new ConfigurationBuilder().AddInMemoryCollection(endpointConfig).Build(); - try - { - _ = serverOptions.ConfigurationLoader.Reload(); - } - catch (InvalidOperationException e) - { - // Since there are no longer any IConfiguration certificates, we'll fall back to the certificate store. - // Unfortunately, the state of the store varies by box (usually there is a cert on dev boxes, but not - // on CI boxes) and modifying the state of the real store at test-time seems inappropriate. An alternative - // approach would be make it possible to replace CertificateManager.Instance in individual tests, but that - // seems like overkill since we only want to confirm that the configuration loader no longer has a cert. - } + + // 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.IsDevCertLoaded = true; + Assert.Null(serverOptions.DefaultCertificate); + + // 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); From 7e5e531348e009d0b2e23d9adc9a059430cb5ba1 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 24 Feb 2023 11:40:56 -0800 Subject: [PATCH 7/9] Don't drop untrusted certs --- src/Servers/Kestrel/Core/src/KestrelServerOptions.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs b/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs index 2980a7f170b7..41cbb13584de 100644 --- a/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs +++ b/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs @@ -336,7 +336,6 @@ internal void Serialize(Utf8JsonWriter writer) if (!CertificateManager.Instance.IsTrusted(cert)) { logger.DeveloperCertificateNotTrusted(); - return null; } return cert; From 7d142a29fd774c303df94bd6a1229641ea33f57b Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 24 Feb 2023 16:15:47 -0800 Subject: [PATCH 8/9] Fix CanReadAndWriteWithHttpsConnectionMiddlewareWithPemCertificate --- .../InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs | 1 + 1 file changed, 1 insertion(+) 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) From 3c6f8147b5bbaff517940296788d5d51144126fe Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 28 Feb 2023 13:50:23 -0800 Subject: [PATCH 9/9] Rename certificate members for consistency --- .../Core/src/KestrelConfigurationLoader.cs | 2 +- .../Kestrel/Core/src/KestrelServerOptions.cs | 22 +++++++++---------- .../Core/src/ListenOptionsHttpsExtensions.cs | 4 ++-- .../Kestrel/Core/test/KestrelServerTests.cs | 2 +- .../test/KestrelConfigurationLoaderTests.cs | 14 ++++++------ .../InMemory.FunctionalTests/HttpsTests.cs | 12 +++++----- 6 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs index 50a4a8f2dc04..297fd606148b 100644 --- a/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs +++ b/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs @@ -338,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; diff --git a/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs b/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs index 41cbb13584de..3aa1ca76607d 100644 --- a/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs +++ b/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs @@ -151,12 +151,12 @@ 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. /// /// /// Getter exposed for testing. /// - internal X509Certificate2? DefaultCertificate { get; private set; } + internal X509Certificate2? DevelopmentCertificate { get; private set; } /// /// Allow tests to explicitly set the default certificate. @@ -166,7 +166,7 @@ public class KestrelServerOptions /// /// 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. @@ -235,7 +235,7 @@ internal void ApplyHttpsDefaults(HttpsConnectionAdapterOptions httpsOptions) HttpsDefaults(httpsOptions); } - internal void ApplyDefaultCert(HttpsConnectionAdapterOptions httpsOptions) + internal void ApplyDefaultCertificate(HttpsConnectionAdapterOptions httpsOptions) { if (httpsOptions.ServerCertificate != null || httpsOptions.ServerCertificateSelector != null) { @@ -254,15 +254,15 @@ internal void ApplyDefaultCert(HttpsConnectionAdapterOptions httpsOptions) return; } - if (!IsDevCertLoaded) + if (!IsDevelopmentCertificateLoaded) { - IsDevCertLoaded = true; - Debug.Assert(DefaultCertificate is null); + IsDevelopmentCertificateLoaded = true; + Debug.Assert(DevelopmentCertificate is null); var logger = ApplicationServices!.GetRequiredService>(); - DefaultCertificate = GetDevelopmentCertificateFromStore(logger); + DevelopmentCertificate = GetDevelopmentCertificateFromStore(logger); } - httpsOptions.ServerCertificate = DefaultCertificate; + httpsOptions.ServerCertificate = DevelopmentCertificate; } internal void Serialize(Utf8JsonWriter writer) @@ -279,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"); 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"); diff --git a/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs b/src/Servers/Kestrel/Kestrel/test/KestrelConfigurationLoaderTests.cs index 8c0960f9f3b6..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.Null(serverOptions.DefaultCertificate); // Not used since configuration cert is present + Assert.Null(serverOptions.DevelopmentCertificate); // Not used since configuration cert is present } finally { @@ -308,8 +308,8 @@ public void DevelopmentCertificateCanBeRemoved() // 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.IsDevCertLoaded = true; - Assert.Null(serverOptions.DefaultCertificate); + 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. @@ -320,7 +320,7 @@ public void DevelopmentCertificateCanBeRemoved() void CheckCertificates(X509Certificate2 expectedCert) { var httpsOptions = new HttpsConnectionAdapterOptions(); - serverOptions.ApplyDefaultCert(httpsOptions); + serverOptions.ApplyDefaultCertificate(httpsOptions); Assert.Equal(expectedCert.SerialNumber, httpsOptions.ServerCertificate.SerialNumber); Assert.Equal(expectedCert.SerialNumber, serverOptions.ConfigurationLoader.DefaultCertificate.SerialNumber); } @@ -517,7 +517,7 @@ public void ConfigureEndpoint_CanLoadPemCertificates(string certificateFile, str }).Load(); Assert.True(ran1); - Assert.Null(serverOptions.DefaultCertificate); // Not used since configuration cert is present + Assert.Null(serverOptions.DevelopmentCertificate); // Not used since configuration cert is present } [Fact] @@ -541,7 +541,7 @@ public void ConfigureEndpointDevelopmentCertificateGetsIgnoredIfPasswordIsNotCor .Configure(config) .Load(); - Assert.Null(serverOptions.DefaultCertificate); + Assert.Null(serverOptions.DevelopmentCertificate); } finally { @@ -572,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/InMemory.FunctionalTests/HttpsTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs index 6692946a2e88..3ee91abd9d8b 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsTests.cs @@ -51,7 +51,7 @@ public void UseHttpsDefaultsToDefaultCert() 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]