From cef5180db0cdc86e8e7f3874247cb78af447320d Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 18 Apr 2023 14:02:56 -0700 Subject: [PATCH 1/2] Log if a server certificate lacks the subjectAlternativeName extensions CommonName was deprecated in favor of subjectAlternativeName so there's a good chance of getting a browser security warning if it's missing. --- .../Kestrel/Core/src/CertificateLoader.cs | 3 +++ .../Core/src/HttpsConfigurationService.cs | 14 ++++++---- .../Core/src/Internal/SniOptionsSelector.cs | 6 +++-- src/Servers/Kestrel/Core/src/KestrelServer.cs | 11 ++++++-- .../Middleware/HttpsConnectionMiddleware.cs | 17 ++++++++---- .../CertificateLoaderTests.cs | 11 ++++++++ .../HttpsConnectionMiddlewareTests.cs | 27 +++++++++++++++++++ 7 files changed, 75 insertions(+), 14 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/CertificateLoader.cs b/src/Servers/Kestrel/Core/src/CertificateLoader.cs index 4b581e740b6f..ffbfd7080e8a 100644 --- a/src/Servers/Kestrel/Core/src/CertificateLoader.cs +++ b/src/Servers/Kestrel/Core/src/CertificateLoader.cs @@ -106,6 +106,9 @@ internal static bool IsCertificateAllowedForServerAuth(X509Certificate2 certific internal static bool DoesCertificateHaveAnAccessiblePrivateKey(X509Certificate2 certificate) => certificate.HasPrivateKey; + internal static bool DoesCertificateHaveASubjectAlternativeName(X509Certificate2 certificate) + => certificate.Extensions.OfType().Any(); + private static void DisposeCertificates(X509Certificate2Collection? certificates, X509Certificate2? except) { if (certificates != null) diff --git a/src/Servers/Kestrel/Core/src/HttpsConfigurationService.cs b/src/Servers/Kestrel/Core/src/HttpsConfigurationService.cs index 8e6185d4b642..013833a6463e 100644 --- a/src/Servers/Kestrel/Core/src/HttpsConfigurationService.cs +++ b/src/Servers/Kestrel/Core/src/HttpsConfigurationService.cs @@ -24,9 +24,11 @@ internal sealed class HttpsConfigurationService : IHttpsConfigurationService private bool _isInitialized; private TlsConfigurationLoader? _tlsConfigurationLoader; - private Action? _populateMultiplexedTransportFeatures; + private Action>? _populateMultiplexedTransportFeatures; private Func? _useHttpsWithDefaults; + private ILogger? _httpsLogger; + /// /// Create an uninitialized . /// To initialize it later, call . @@ -67,6 +69,7 @@ public void Initialize( _tlsConfigurationLoader = new TlsConfigurationLoader(hostEnvironment, serverLogger, httpsLogger); _populateMultiplexedTransportFeatures = PopulateMultiplexedTransportFeaturesWorker; _useHttpsWithDefaults = UseHttpsWithDefaultsWorker; + _httpsLogger = httpsLogger; } /// @@ -100,7 +103,7 @@ public ListenOptions UseHttpsWithSni(ListenOptions listenOptions, HttpsConnectio public void PopulateMultiplexedTransportFeatures(FeatureCollection features, ListenOptions listenOptions) { EnsureInitialized(CoreStrings.NeedHttpsConfigurationToUseHttp3); - _populateMultiplexedTransportFeatures.Invoke(features, listenOptions); + _populateMultiplexedTransportFeatures.Invoke(features, listenOptions, _httpsLogger); } /// @@ -114,7 +117,7 @@ public ListenOptions UseHttpsWithDefaults(ListenOptions listenOptions) /// If this instance has not been initialized, initialize it if possible and throw otherwise. /// /// If initialization is not possible. - [MemberNotNull(nameof(_useHttpsWithDefaults), nameof(_tlsConfigurationLoader), nameof(_populateMultiplexedTransportFeatures))] + [MemberNotNull(nameof(_useHttpsWithDefaults), nameof(_tlsConfigurationLoader), nameof(_populateMultiplexedTransportFeatures), nameof(_httpsLogger))] private void EnsureInitialized(string uninitializedError) { if (!_isInitialized) @@ -132,18 +135,19 @@ private void EnsureInitialized(string uninitializedError) Debug.Assert(_useHttpsWithDefaults is not null); Debug.Assert(_tlsConfigurationLoader is not null); Debug.Assert(_populateMultiplexedTransportFeatures is not null); + Debug.Assert(_httpsLogger is not null); } /// /// The initialized implementation of . /// - internal static void PopulateMultiplexedTransportFeaturesWorker(FeatureCollection features, ListenOptions listenOptions) + internal static void PopulateMultiplexedTransportFeaturesWorker(FeatureCollection features, ListenOptions listenOptions, ILogger logger) { // HttpsOptions or HttpsCallbackOptions should always be set in production, but it's not set for InMemory tests. // The QUIC transport will check if TlsConnectionCallbackOptions is missing. if (listenOptions.HttpsOptions != null) { - var sslServerAuthenticationOptions = HttpsConnectionMiddleware.CreateHttp3Options(listenOptions.HttpsOptions); + var sslServerAuthenticationOptions = HttpsConnectionMiddleware.CreateHttp3Options(listenOptions.HttpsOptions, logger); features.Set(new TlsConnectionCallbackOptions { ApplicationProtocols = sslServerAuthenticationOptions.ApplicationProtocols ?? new List { SslApplicationProtocol.Http3 }, diff --git a/src/Servers/Kestrel/Core/src/Internal/SniOptionsSelector.cs b/src/Servers/Kestrel/Core/src/Internal/SniOptionsSelector.cs index c1606c0bb743..376916398a80 100644 --- a/src/Servers/Kestrel/Core/src/Internal/SniOptionsSelector.cs +++ b/src/Servers/Kestrel/Core/src/Internal/SniOptionsSelector.cs @@ -20,6 +20,7 @@ internal sealed class SniOptionsSelector private const string WildcardPrefix = "*."; private readonly string _endpointName; + private readonly ILogger _logger; private readonly Func? _fallbackServerCertificateSelector; private readonly Action? _onAuthenticateCallback; @@ -37,6 +38,7 @@ public SniOptionsSelector( ILogger logger) { _endpointName = endpointName; + _logger = logger; _fallbackServerCertificateSelector = fallbackHttpsOptions.ServerCertificateSelector; _onAuthenticateCallback = fallbackHttpsOptions.OnAuthenticate; @@ -75,7 +77,7 @@ public SniOptionsSelector( if (!certifcateConfigLoader.IsTestMock && sslOptions.ServerCertificate is X509Certificate2 cert2) { - HttpsConnectionMiddleware.EnsureCertificateIsAllowedForServerAuth(cert2); + HttpsConnectionMiddleware.EnsureCertificateIsAllowedForServerAuth(cert2, _logger); } var clientCertificateMode = sniConfig.ClientCertificateMode ?? fallbackHttpsOptions.ClientCertificateMode; @@ -158,7 +160,7 @@ public SniOptionsSelector( if (fallbackCertificate != null) { - HttpsConnectionMiddleware.EnsureCertificateIsAllowedForServerAuth(fallbackCertificate); + HttpsConnectionMiddleware.EnsureCertificateIsAllowedForServerAuth(fallbackCertificate, _logger); } sslOptions.ServerCertificate = fallbackCertificate; diff --git a/src/Servers/Kestrel/Core/src/KestrelServer.cs b/src/Servers/Kestrel/Core/src/KestrelServer.cs index c65552f9a06f..596dcf9fed6f 100644 --- a/src/Servers/Kestrel/Core/src/KestrelServer.cs +++ b/src/Servers/Kestrel/Core/src/KestrelServer.cs @@ -35,7 +35,7 @@ public KestrelServer(IOptions options, IConnectionListener options, new[] { transportFactory ?? throw new ArgumentNullException(nameof(transportFactory)) }, Array.Empty(), - new SimpleHttpsConfigurationService(), + new SimpleHttpsConfigurationService(loggerFactory.CreateLogger()), loggerFactory, new KestrelMetrics(new DummyMeterFactory())); } @@ -78,6 +78,13 @@ private sealed class DummyMeterFactory : IMeterFactory private sealed class SimpleHttpsConfigurationService : IHttpsConfigurationService { + private readonly ILogger _httpsLogger; + + public SimpleHttpsConfigurationService(ILogger httpsLogger) + { + _httpsLogger = httpsLogger; + } + public bool IsInitialized => true; public void Initialize(IHostEnvironment hostEnvironment, ILogger serverLogger, ILogger httpsLogger) @@ -87,7 +94,7 @@ public void Initialize(IHostEnvironment hostEnvironment, ILogger public void PopulateMultiplexedTransportFeatures(FeatureCollection features, ListenOptions listenOptions) { - HttpsConfigurationService.PopulateMultiplexedTransportFeaturesWorker(features, listenOptions); + HttpsConfigurationService.PopulateMultiplexedTransportFeaturesWorker(features, listenOptions, _httpsLogger); } public ListenOptions UseHttpsWithDefaults(ListenOptions listenOptions) diff --git a/src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs b/src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs index 8cf4f843798a..008b47773371 100644 --- a/src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs +++ b/src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs @@ -90,7 +90,7 @@ public HttpsConnectionMiddleware(ConnectionDelegate next, HttpsConnectionAdapter { Debug.Assert(_serverCertificate != null); - EnsureCertificateIsAllowedForServerAuth(_serverCertificate); + EnsureCertificateIsAllowedForServerAuth(_serverCertificate, _logger); var certificate = _serverCertificate; if (!certificate.HasPrivateKey) @@ -314,7 +314,7 @@ private Task DoOptionsBasedHandshakeAsync(ConnectionContext context, SslStream s var cert = _serverCertificateSelector(context, name); if (cert != null) { - EnsureCertificateIsAllowedForServerAuth(cert); + EnsureCertificateIsAllowedForServerAuth(cert, _logger); } return cert!; }; @@ -452,12 +452,16 @@ private static async ValueTask ServerOptionsCall return sslOptions; } - internal static void EnsureCertificateIsAllowedForServerAuth(X509Certificate2 certificate) + internal static void EnsureCertificateIsAllowedForServerAuth(X509Certificate2 certificate, ILogger logger) { if (!CertificateLoader.IsCertificateAllowedForServerAuth(certificate)) { throw new InvalidOperationException(CoreStrings.FormatInvalidServerCertificateEku(certificate.Thumbprint)); } + else if (!CertificateLoader.DoesCertificateHaveASubjectAlternativeName(certificate)) + { + logger.NoSubjectAlternativeName(certificate.Thumbprint); + } } private static X509Certificate2? ConvertToX509Certificate2(X509Certificate? certificate) @@ -514,7 +518,7 @@ private static bool IsWindowsVersionIncompatibleWithHttp2() return false; } - internal static SslServerAuthenticationOptions CreateHttp3Options(HttpsConnectionAdapterOptions httpsOptions) + internal static SslServerAuthenticationOptions CreateHttp3Options(HttpsConnectionAdapterOptions httpsOptions, ILogger logger) { if (httpsOptions.OnAuthenticate != null) { @@ -539,7 +543,7 @@ internal static SslServerAuthenticationOptions CreateHttp3Options(HttpsConnectio var cert = httpsOptions.ServerCertificateSelector(null, host); if (cert != null) { - EnsureCertificateIsAllowedForServerAuth(cert); + EnsureCertificateIsAllowedForServerAuth(cert, logger); } return cert!; }; @@ -596,6 +600,9 @@ public static void FoundCertWithPrivateKey(this ILogger logger, string? storeName, Exception exception); + [LoggerMessage(9, LogLevel.Information, "Certificate with thumbprint {Thumbprint} lacks the subjectAlternativeName (SAN) extension and may not be accepted by browsers.", EventName = "NoSubjectAlternativeName")] + public static partial void NoSubjectAlternativeName(this ILogger logger, string thumbprint); + public static void FailedToOpenStore(this ILogger logger, StoreLocation storeLocation, Exception exception) { var storeLocationString = storeLocation == StoreLocation.LocalMachine ? nameof(StoreLocation.LocalMachine) : nameof(StoreLocation.CurrentUser); diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/CertificateLoaderTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/CertificateLoaderTests.cs index 92590965d398..d3d02064a6d4 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/CertificateLoaderTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/CertificateLoaderTests.cs @@ -53,4 +53,15 @@ public void IsCertificateAllowedForServerAuth_RejectsCertificatesMissingServerEk Assert.False(CertificateLoader.IsCertificateAllowedForServerAuth(cert)); } + + [Theory] + [InlineData("aspnetdevcert.pfx", true)] + [InlineData("no_extensions.pfx", false)] + public void DoesCertificateHaveASubjectAlternativeName(string testCertName, bool hasSan) + { + var certPath = TestResources.GetCertPath(testCertName); + TestOutputHelper.WriteLine("Loading " + certPath); + var cert = new X509Certificate2(certPath, "testPassword"); + Assert.Equal(hasSan, CertificateLoader.DoesCertificateHaveASubjectAlternativeName(cert)); + } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs index b5c62a11a3b6..f5d468283325 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs @@ -23,6 +23,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Metrics; using Moq; @@ -1341,6 +1342,26 @@ public void ThrowsForCertificatesMissingServerEku(string testCertName) Assert.Equal(CoreStrings.FormatInvalidServerCertificateEku(cert.Thumbprint), ex.Message); } + [Theory] + [InlineData("no_extensions.pfx")] + public void LogsForCertificateMissingSubjectAlternativeName(string testCertName) + { + var certPath = TestResources.GetCertPath(testCertName); + TestOutputHelper.WriteLine("Loading " + certPath); + var cert = new X509Certificate2(certPath, "testPassword"); + Assert.False(CertificateLoader.DoesCertificateHaveASubjectAlternativeName(cert)); + + var testLogger = new TestApplicationErrorLogger(); + CreateMiddleware( + new HttpsConnectionAdapterOptions + { + ServerCertificate = cert, + }, + testLogger); + + Assert.Single(testLogger.Messages.Where(log => log.EventId == 9)); + } + [ConditionalTheory] [InlineData(HttpProtocols.Http1)] [InlineData(HttpProtocols.Http2)] @@ -1442,6 +1463,12 @@ private static HttpsConnectionMiddleware CreateMiddleware(X509Certificate2 serve }); } + private static HttpsConnectionMiddleware CreateMiddleware(HttpsConnectionAdapterOptions options, TestApplicationErrorLogger testLogger = null) + { + var loggerFactory = testLogger is null ? (ILoggerFactory)NullLoggerFactory.Instance : new LoggerFactory(new[] { new KestrelTestLoggerProvider(testLogger) }); + return new HttpsConnectionMiddleware(context => Task.CompletedTask, options, loggerFactory, new KestrelMetrics(new TestMeterFactory())); + } + private static HttpsConnectionMiddleware CreateMiddleware(HttpsConnectionAdapterOptions options) { return new HttpsConnectionMiddleware(context => Task.CompletedTask, options, new KestrelMetrics(new TestMeterFactory())); From ba6fbb9377b4c52b0539fd7267fc3ed9cf216c15 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 19 Apr 2023 18:30:53 -0700 Subject: [PATCH 2/2] Address minor PR feedback --- src/Servers/Kestrel/Core/src/Internal/SniOptionsSelector.cs | 2 +- src/Servers/Kestrel/Core/src/KestrelServer.cs | 6 +++--- .../Core/src/Middleware/HttpsConnectionMiddleware.cs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/SniOptionsSelector.cs b/src/Servers/Kestrel/Core/src/Internal/SniOptionsSelector.cs index 376916398a80..7bc2940945b5 100644 --- a/src/Servers/Kestrel/Core/src/Internal/SniOptionsSelector.cs +++ b/src/Servers/Kestrel/Core/src/Internal/SniOptionsSelector.cs @@ -77,7 +77,7 @@ public SniOptionsSelector( if (!certifcateConfigLoader.IsTestMock && sslOptions.ServerCertificate is X509Certificate2 cert2) { - HttpsConnectionMiddleware.EnsureCertificateIsAllowedForServerAuth(cert2, _logger); + HttpsConnectionMiddleware.EnsureCertificateIsAllowedForServerAuth(cert2, logger); } var clientCertificateMode = sniConfig.ClientCertificateMode ?? fallbackHttpsOptions.ClientCertificateMode; diff --git a/src/Servers/Kestrel/Core/src/KestrelServer.cs b/src/Servers/Kestrel/Core/src/KestrelServer.cs index 596dcf9fed6f..cd8284a87304 100644 --- a/src/Servers/Kestrel/Core/src/KestrelServer.cs +++ b/src/Servers/Kestrel/Core/src/KestrelServer.cs @@ -35,7 +35,7 @@ public KestrelServer(IOptions options, IConnectionListener options, new[] { transportFactory ?? throw new ArgumentNullException(nameof(transportFactory)) }, Array.Empty(), - new SimpleHttpsConfigurationService(loggerFactory.CreateLogger()), + new SimpleHttpsConfigurationService(loggerFactory), loggerFactory, new KestrelMetrics(new DummyMeterFactory())); } @@ -80,9 +80,9 @@ private sealed class SimpleHttpsConfigurationService : IHttpsConfigurationServic { private readonly ILogger _httpsLogger; - public SimpleHttpsConfigurationService(ILogger httpsLogger) + public SimpleHttpsConfigurationService(ILoggerFactory loggerFactory) { - _httpsLogger = httpsLogger; + _httpsLogger = loggerFactory.CreateLogger(); } public bool IsInitialized => true; diff --git a/src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs b/src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs index 008b47773371..3488024f2294 100644 --- a/src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs +++ b/src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs @@ -601,7 +601,7 @@ public static void FoundCertWithPrivateKey(this ILogger logger, string? storeName, Exception exception); [LoggerMessage(9, LogLevel.Information, "Certificate with thumbprint {Thumbprint} lacks the subjectAlternativeName (SAN) extension and may not be accepted by browsers.", EventName = "NoSubjectAlternativeName")] - public static partial void NoSubjectAlternativeName(this ILogger logger, string thumbprint); + public static partial void NoSubjectAlternativeName(this ILogger logger, string thumbprint); public static void FailedToOpenStore(this ILogger logger, StoreLocation storeLocation, Exception exception) {