Skip to content

Log if a server certificate lacks the subjectAlternativeName extensions #47678

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Servers/Kestrel/Core/src/CertificateLoader.cs
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
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
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
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
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
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));
}
}
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