Skip to content

Commit

Permalink
Log if a server certificate lacks the subjectAlternativeName extensio…
Browse files Browse the repository at this point in the history 
…ns (#47678)

* 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.

* Address minor PR feedback
  • Loading branch information
@amcasey
amcasey authored Apr 20, 2023
1 parent d34fefb commit 0c42c03
Show file tree
Hide file tree
Showing 7 changed files with 75 additions and 14 deletions.
3 changes: 3 additions & 0 deletions src/Servers/Kestrel/Core/src/CertificateLoader.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<X509SubjectAlternativeNameExtension>().Any();

private static void DisposeCertificates(X509Certificate2Collection? certificates, X509Certificate2? except)
{
if (certificates != null)
Expand Down
14 changes: 9 additions & 5 deletions src/Servers/Kestrel/Core/src/HttpsConfigurationService.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@ internal sealed class HttpsConfigurationService : IHttpsConfigurationService
private bool _isInitialized;

private TlsConfigurationLoader? _tlsConfigurationLoader;
private Action<FeatureCollection, ListenOptions>? _populateMultiplexedTransportFeatures;
private Action<FeatureCollection, ListenOptions, ILogger<HttpsConnectionMiddleware>>? _populateMultiplexedTransportFeatures;
private Func<ListenOptions, ListenOptions>? _useHttpsWithDefaults;

private ILogger<HttpsConnectionMiddleware>? _httpsLogger;

/// <summary>
/// Create an uninitialized <see cref="HttpsConfigurationService"/>.
/// To initialize it later, call <see cref="Initialize"/>.
Expand Down Expand Up @@ -67,6 +69,7 @@ public void Initialize(
_tlsConfigurationLoader = new TlsConfigurationLoader(hostEnvironment, serverLogger, httpsLogger);
_populateMultiplexedTransportFeatures = PopulateMultiplexedTransportFeaturesWorker;
_useHttpsWithDefaults = UseHttpsWithDefaultsWorker;
_httpsLogger = httpsLogger;
}

/// <inheritdoc/>
Expand Down Expand Up @@ -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);
}

/// <inheritdoc/>
Expand All @@ -114,7 +117,7 @@ public ListenOptions UseHttpsWithDefaults(ListenOptions listenOptions)
/// If this instance has not been initialized, initialize it if possible and throw otherwise.
/// </summary>
/// <exception cref="InvalidOperationException">If initialization is not possible.</exception>
[MemberNotNull(nameof(_useHttpsWithDefaults), nameof(_tlsConfigurationLoader), nameof(_populateMultiplexedTransportFeatures))]
[MemberNotNull(nameof(_useHttpsWithDefaults), nameof(_tlsConfigurationLoader), nameof(_populateMultiplexedTransportFeatures), nameof(_httpsLogger))]
private void EnsureInitialized(string uninitializedError)
{
if (!_isInitialized)
Expand All @@ -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);
}

/// <summary>
/// The initialized implementation of <see cref="PopulateMultiplexedTransportFeatures"/>.
/// </summary>
internal static void PopulateMultiplexedTransportFeaturesWorker(FeatureCollection features, ListenOptions listenOptions)
internal static void PopulateMultiplexedTransportFeaturesWorker(FeatureCollection features, ListenOptions listenOptions, ILogger<HttpsConnectionMiddleware> 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> { SslApplicationProtocol.Http3 },
Expand Down
6 changes: 4 additions & 2 deletions src/Servers/Kestrel/Core/src/Internal/SniOptionsSelector.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ internal sealed class SniOptionsSelector
private const string WildcardPrefix = "*.";

private readonly string _endpointName;
private readonly ILogger<HttpsConnectionMiddleware> _logger;

private readonly Func<ConnectionContext, string?, X509Certificate2?>? _fallbackServerCertificateSelector;
private readonly Action<ConnectionContext, SslServerAuthenticationOptions>? _onAuthenticateCallback;
Expand All @@ -37,6 +38,7 @@ public SniOptionsSelector(
ILogger<HttpsConnectionMiddleware> logger)
{
_endpointName = endpointName;
_logger = logger;

_fallbackServerCertificateSelector = fallbackHttpsOptions.ServerCertificateSelector;
_onAuthenticateCallback = fallbackHttpsOptions.OnAuthenticate;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -158,7 +160,7 @@ public SniOptionsSelector(

if (fallbackCertificate != null)
{
HttpsConnectionMiddleware.EnsureCertificateIsAllowedForServerAuth(fallbackCertificate);
HttpsConnectionMiddleware.EnsureCertificateIsAllowedForServerAuth(fallbackCertificate, _logger);
}

sslOptions.ServerCertificate = fallbackCertificate;
Expand Down
11 changes: 9 additions & 2 deletions src/Servers/Kestrel/Core/src/KestrelServer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public KestrelServer(IOptions<KestrelServerOptions> options, IConnectionListener
options,
new[] { transportFactory ?? throw new ArgumentNullException(nameof(transportFactory)) },
Array.Empty<IMultiplexedConnectionListenerFactory>(),
new SimpleHttpsConfigurationService(),
new SimpleHttpsConfigurationService(loggerFactory),
loggerFactory,
new KestrelMetrics(new DummyMeterFactory()));
}
Expand Down Expand Up @@ -78,6 +78,13 @@ private sealed class DummyMeterFactory : IMeterFactory

private sealed class SimpleHttpsConfigurationService : IHttpsConfigurationService
{
private readonly ILogger<HttpsConnectionMiddleware> _httpsLogger;

public SimpleHttpsConfigurationService(ILoggerFactory loggerFactory)
{
_httpsLogger = loggerFactory.CreateLogger<HttpsConnectionMiddleware>();
}

public bool IsInitialized => true;

public void Initialize(IHostEnvironment hostEnvironment, ILogger<KestrelServer> serverLogger, ILogger<HttpsConnectionMiddleware> httpsLogger)
Expand All @@ -87,7 +94,7 @@ public void Initialize(IHostEnvironment hostEnvironment, ILogger<KestrelServer>

public void PopulateMultiplexedTransportFeatures(FeatureCollection features, ListenOptions listenOptions)
{
HttpsConfigurationService.PopulateMultiplexedTransportFeaturesWorker(features, listenOptions);
HttpsConfigurationService.PopulateMultiplexedTransportFeaturesWorker(features, listenOptions, _httpsLogger);
}

public ListenOptions UseHttpsWithDefaults(ListenOptions listenOptions)
Expand Down
17 changes: 12 additions & 5 deletions src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public HttpsConnectionMiddleware(ConnectionDelegate next, HttpsConnectionAdapter
{
Debug.Assert(_serverCertificate != null);

EnsureCertificateIsAllowedForServerAuth(_serverCertificate);
EnsureCertificateIsAllowedForServerAuth(_serverCertificate, _logger);

var certificate = _serverCertificate;
if (!certificate.HasPrivateKey)
Expand Down Expand Up @@ -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!;
};
Expand Down Expand Up @@ -452,12 +452,16 @@ private static async ValueTask<SslServerAuthenticationOptions> ServerOptionsCall
return sslOptions;
}

internal static void EnsureCertificateIsAllowedForServerAuth(X509Certificate2 certificate)
internal static void EnsureCertificateIsAllowedForServerAuth(X509Certificate2 certificate, ILogger<HttpsConnectionMiddleware> 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)
Expand Down Expand Up @@ -514,7 +518,7 @@ private static bool IsWindowsVersionIncompatibleWithHttp2()
return false;
}

internal static SslServerAuthenticationOptions CreateHttp3Options(HttpsConnectionAdapterOptions httpsOptions)
internal static SslServerAuthenticationOptions CreateHttp3Options(HttpsConnectionAdapterOptions httpsOptions, ILogger<HttpsConnectionMiddleware> logger)
{
if (httpsOptions.OnAuthenticate != null)
{
Expand All @@ -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!;
};
Expand Down Expand Up @@ -596,6 +600,9 @@ public static void FoundCertWithPrivateKey(this ILogger<HttpsConnectionMiddlewar
[LoggerMessage(8, LogLevel.Debug, "Failed to open certificate store {StoreName}.", EventName = "FailToOpenStore")]
public static partial void FailedToOpenStore(this ILogger<HttpsConnectionMiddleware> 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<HttpsConnectionMiddleware> logger, string thumbprint);

public static void FailedToOpenStore(this ILogger<HttpsConnectionMiddleware> logger, StoreLocation storeLocation, Exception exception)
{
var storeLocationString = storeLocation == StoreLocation.LocalMachine ? nameof(StoreLocation.LocalMachine) : nameof(StoreLocation.CurrentUser);
Expand Down
11 changes: 11 additions & 0 deletions src/Servers/Kestrel/test/InMemory.FunctionalTests/CertificateLoaderTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
27 changes: 27 additions & 0 deletions src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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()));
Expand Down

0 comments on commit 0c42c03

Please sign in to comment.