Skip to content

Commit 95ed45c

Browse files
amcaseyamcasey
authored
Tidy up some irregularities in certificate reloading (#46819)
* 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. * 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. * Test certificate updates on config reload * Test setting a bad certificate password in config * Hack around possible absence of dev cert in store * Use IsDevCertLoaded to bypass the cert store entirely * Don't drop untrusted certs * Fix CanReadAndWriteWithHttpsConnectionMiddlewareWithPemCertificate * Rename certificate members for consistency
1 parent e9ced2f commit 95ed45c

File tree

8 files changed

+230
-69
lines changed

8 files changed

+230
-69
lines changed

‎src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ internal KestrelConfigurationLoader(
7777
private IList<Action> EndpointsToAdd { get; } = new List<Action>();
7878

7979
private CertificateConfig? DefaultCertificateConfig { get; set; }
80+
internal X509Certificate2? DefaultCertificate { get; set; }
8081

8182
/// <summary>
8283
/// Specifies a configuration Action to run when an endpoint with the given name is loaded from configuration.
@@ -270,9 +271,10 @@ public void Load()
270271
{
271272
var endpointsToStop = Options.ConfigurationBackedListenOptions.ToList();
272273
var endpointsToStart = new List<ListenOptions>();
274+
var endpointsToReuse = new List<ListenOptions>();
273275

274-
Options.ConfigurationBackedListenOptions.Clear();
275276
DefaultCertificateConfig = null;
277+
DefaultCertificate = null;
276278

277279
ConfigurationReader = new ConfigurationReader(Configuration);
278280

@@ -336,7 +338,7 @@ public void Load()
336338
if (httpsOptions.ServerCertificate == null && httpsOptions.ServerCertificateSelector == null)
337339
{
338340
// Fallback
339-
Options.ApplyDefaultCert(httpsOptions);
341+
Options.ApplyDefaultCertificate(httpsOptions);
340342

341343
// Ensure endpoint is reloaded if it used the default certificate and the certificate changed.
342344
endpoint.Certificate = DefaultCertificateConfig;
@@ -350,7 +352,7 @@ public void Load()
350352
if (matchingBoundEndpoints.Count > 0)
351353
{
352354
endpointsToStop.RemoveAll(o => o.EndpointConfig == endpoint);
353-
Options.ConfigurationBackedListenOptions.AddRange(matchingBoundEndpoints);
355+
endpointsToReuse.AddRange(matchingBoundEndpoints);
354356
continue;
355357
}
356358

@@ -390,9 +392,16 @@ public void Load()
390392
listenOptions.EndpointConfig = endpoint;
391393

392394
endpointsToStart.Add(listenOptions);
393-
Options.ConfigurationBackedListenOptions.Add(listenOptions);
394395
}
395396

397+
// Update ConfigurationBackedListenOptions after everything else has been processed so that
398+
// it's left in a good state (i.e. its former state) if something above throws an exception.
399+
// Note that this isn't foolproof - we could run out of memory or something - but it covers
400+
// exceptions resulting from user misconfiguration (e.g. bad endpoint cert password).
401+
Options.ConfigurationBackedListenOptions.Clear();
402+
Options.ConfigurationBackedListenOptions.AddRange(endpointsToReuse);
403+
Options.ConfigurationBackedListenOptions.AddRange(endpointsToStart);
404+
396405
return (endpointsToStop, endpointsToStart);
397406
}
398407

@@ -404,7 +413,7 @@ private void LoadDefaultCert()
404413
if (defaultCert != null)
405414
{
406415
DefaultCertificateConfig = defaultCertConfig;
407-
Options.DefaultCertificate = defaultCert;
416+
DefaultCertificate = defaultCert;
408417
}
409418
}
410419
else
@@ -414,7 +423,7 @@ private void LoadDefaultCert()
414423
{
415424
Logger.LocatedDevelopmentCertificate(certificate);
416425
DefaultCertificateConfig = certificateConfig;
417-
Options.DefaultCertificate = certificate;
426+
DefaultCertificate = certificate;
418427
}
419428
}
420429
}

‎src/Servers/Kestrel/Core/src/KestrelServerOptions.cs‎

Lines changed: 66 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -151,14 +151,22 @@ public class KestrelServerOptions
151151
private Action<HttpsConnectionAdapterOptions> HttpsDefaults { get; set; } = _ => { };
152152

153153
/// <summary>
154-
/// The default server certificate for https endpoints. This is applied lazily after HttpsDefaults and user options.
154+
/// The development server certificate for https endpoints. This is applied lazily after HttpsDefaults and user options.
155155
/// </summary>
156-
internal X509Certificate2? DefaultCertificate { get; set; }
156+
/// <remarks>
157+
/// Getter exposed for testing.
158+
/// </remarks>
159+
internal X509Certificate2? DevelopmentCertificate { get; private set; }
160+
161+
/// <summary>
162+
/// Allow tests to explicitly set the default certificate.
163+
/// </summary>
164+
internal X509Certificate2? TestOverrideDefaultCertificate { get; set; }
157165

158166
/// <summary>
159167
/// Has the default dev certificate load been attempted?
160168
/// </summary>
161-
internal bool IsDevCertLoaded { get; set; }
169+
internal bool IsDevelopmentCertificateLoaded { get; set; }
162170

163171
/// <summary>
164172
/// Internal AppContext switch to toggle the WebTransport and HTTP/3 datagrams experiemental features.
@@ -227,16 +235,34 @@ internal void ApplyHttpsDefaults(HttpsConnectionAdapterOptions httpsOptions)
227235
HttpsDefaults(httpsOptions);
228236
}
229237

230-
internal void ApplyDefaultCert(HttpsConnectionAdapterOptions httpsOptions)
238+
internal void ApplyDefaultCertificate(HttpsConnectionAdapterOptions httpsOptions)
231239
{
232240
if (httpsOptions.ServerCertificate != null || httpsOptions.ServerCertificateSelector != null)
233241
{
234242
return;
235243
}
236244

237-
EnsureDefaultCert();
245+
if (TestOverrideDefaultCertificate is X509Certificate2 certificateFromTest)
246+
{
247+
httpsOptions.ServerCertificate = certificateFromTest;
248+
return;
249+
}
250+
251+
if (ConfigurationLoader?.DefaultCertificate is X509Certificate2 certificateFromLoader)
252+
{
253+
httpsOptions.ServerCertificate = certificateFromLoader;
254+
return;
255+
}
238256

239-
httpsOptions.ServerCertificate = DefaultCertificate;
257+
if (!IsDevelopmentCertificateLoaded)
258+
{
259+
IsDevelopmentCertificateLoaded = true;
260+
Debug.Assert(DevelopmentCertificate is null);
261+
var logger = ApplicationServices!.GetRequiredService<ILogger<KestrelServer>>();
262+
DevelopmentCertificate = GetDevelopmentCertificateFromStore(logger);
263+
}
264+
265+
httpsOptions.ServerCertificate = DevelopmentCertificate;
240266
}
241267

242268
internal void Serialize(Utf8JsonWriter writer)
@@ -253,8 +279,8 @@ internal void Serialize(Utf8JsonWriter writer)
253279
writer.WritePropertyName(nameof(AllowResponseHeaderCompression));
254280
writer.WriteBooleanValue(AllowResponseHeaderCompression);
255281

256-
writer.WritePropertyName(nameof(IsDevCertLoaded));
257-
writer.WriteBooleanValue(IsDevCertLoaded);
282+
writer.WritePropertyName(nameof(IsDevelopmentCertificateLoaded));
283+
writer.WriteBooleanValue(IsDevelopmentCertificateLoaded);
258284

259285
writer.WriteString(nameof(RequestHeaderEncodingSelector), RequestHeaderEncodingSelector == DefaultHeaderEncodingSelector ? "default" : "configured");
260286
writer.WriteString(nameof(ResponseHeaderEncodingSelector), ResponseHeaderEncodingSelector == DefaultHeaderEncodingSelector ? "default" : "configured");
@@ -280,46 +306,44 @@ internal void Serialize(Utf8JsonWriter writer)
280306
writer.WriteEndArray();
281307
}
282308

283-
private void EnsureDefaultCert()
309+
private static X509Certificate2? GetDevelopmentCertificateFromStore(ILogger<KestrelServer> logger)
284310
{
285-
if (DefaultCertificate == null && !IsDevCertLoaded)
311+
try
286312
{
287-
IsDevCertLoaded = true; // Only try once
288-
var logger = ApplicationServices!.GetRequiredService<ILogger<KestrelServer>>();
289-
try
313+
var cert = CertificateManager.Instance.ListCertificates(StoreName.My, StoreLocation.CurrentUser, isValid: true, requireExportable: false)
314+
.FirstOrDefault();
315+
316+
if (cert is null)
290317
{
291-
DefaultCertificate = CertificateManager.Instance.ListCertificates(StoreName.My, StoreLocation.CurrentUser, isValid: true, requireExportable: false)
292-
.FirstOrDefault();
293-
294-
if (DefaultCertificate != null)
295-
{
296-
var status = CertificateManager.Instance.CheckCertificateState(DefaultCertificate, interactive: false);
297-
if (!status.Success)
298-
{
299-
// Display a warning indicating to the user that a prompt might appear and provide instructions on what to do in that
300-
// case. The underlying implementation of this check is specific to Mac OS and is handled within CheckCertificateState.
301-
// Kestrel must NEVER cause a UI prompt on a production system. We only attempt this here because Mac OS is not supported
302-
// in production.
303-
Debug.Assert(status.FailureMessage != null, "Status with a failure result must have a message.");
304-
logger.DeveloperCertificateFirstRun(status.FailureMessage);
305-
306-
// Prevent binding to HTTPS if the certificate is not valid (avoid the prompt)
307-
DefaultCertificate = null;
308-
}
309-
else if (!CertificateManager.Instance.IsTrusted(DefaultCertificate))
310-
{
311-
logger.DeveloperCertificateNotTrusted();
312-
}
313-
}
314-
else
315-
{
316-
logger.UnableToLocateDevelopmentCertificate();
317-
}
318+
logger.UnableToLocateDevelopmentCertificate();
319+
return null;
318320
}
319-
catch
321+
322+
var status = CertificateManager.Instance.CheckCertificateState(cert, interactive: false);
323+
if (!status.Success)
320324
{
321-
logger.UnableToLocateDevelopmentCertificate();
325+
// Display a warning indicating to the user that a prompt might appear and provide instructions on what to do in that
326+
// case. The underlying implementation of this check is specific to Mac OS and is handled within CheckCertificateState.
327+
// Kestrel must NEVER cause a UI prompt on a production system. We only attempt this here because Mac OS is not supported
328+
// in production.
329+
Debug.Assert(status.FailureMessage != null, "Status with a failure result must have a message.");
330+
logger.DeveloperCertificateFirstRun(status.FailureMessage);
331+
332+
// Prevent binding to HTTPS if the certificate is not valid (avoid the prompt)
333+
return null;
322334
}
335+
336+
if (!CertificateManager.Instance.IsTrusted(cert))
337+
{
338+
logger.DeveloperCertificateNotTrusted();
339+
}
340+
341+
return cert;
342+
}
343+
catch
344+
{
345+
logger.UnableToLocateDevelopmentCertificate();
346+
return null;
323347
}
324348
}
325349

‎src/Servers/Kestrel/Core/src/ListenOptionsHttpsExtensions.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ public static ListenOptions UseHttps(this ListenOptions listenOptions, Action<Ht
166166
var options = new HttpsConnectionAdapterOptions();
167167
listenOptions.KestrelServerOptions.ApplyHttpsDefaults(options);
168168
configureOptions(options);
169-
listenOptions.KestrelServerOptions.ApplyDefaultCert(options);
169+
listenOptions.KestrelServerOptions.ApplyDefaultCertificate(options);
170170

171171
if (options.ServerCertificate == null && options.ServerCertificateSelector == null)
172172
{
@@ -181,7 +181,7 @@ internal static bool TryUseHttps(this ListenOptions listenOptions)
181181
{
182182
var options = new HttpsConnectionAdapterOptions();
183183
listenOptions.KestrelServerOptions.ApplyHttpsDefaults(options);
184-
listenOptions.KestrelServerOptions.ApplyDefaultCert(options);
184+
listenOptions.KestrelServerOptions.ApplyDefaultCertificate(options);
185185

186186
if (options.ServerCertificate == null && options.ServerCertificateSelector == null)
187187
{

‎src/Servers/Kestrel/Core/test/KestrelServerTests.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ public void StartWithInvalidAddressThrows()
5454
public void StartWithHttpsAddressConfiguresHttpsEndpoints()
5555
{
5656
var options = CreateServerOptions();
57-
options.DefaultCertificate = TestResources.GetTestCertificate();
57+
options.TestOverrideDefaultCertificate = TestResources.GetTestCertificate();
5858
using (var server = CreateServer(options))
5959
{
6060
server.Features.Get<IServerAddressesFeature>().Addresses.Add("https://127.0.0.1:0");
@@ -70,7 +70,7 @@ public void StartWithHttpsAddressConfiguresHttpsEndpoints()
7070
public void KestrelServerThrowsUsefulExceptionIfDefaultHttpsProviderNotAdded()
7171
{
7272
var options = CreateServerOptions();
73-
options.IsDevCertLoaded = true; // Prevent the system default from being loaded
73+
options.IsDevelopmentCertificateLoaded = true; // Prevent the system default from being loaded
7474
using (var server = CreateServer(options, throwOnCriticalErrors: false))
7575
{
7676
server.Features.Get<IServerAddressesFeature>().Addresses.Add("https://127.0.0.1:0");

0 commit comments

Comments
 (0)