Skip to content

Add support for loading certificate chains from configuration. #24935

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Add support for loading certificate chains from configuration. #24935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
339bf3a
Add support for loading certificate chains from configuration.
davidfowl Aug 15, 2020
124b365
Actually read the chain path section
davidfowl Sep 14, 2020
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
5 changes: 5 additions & 0 deletions src/Servers/Kestrel/Core/src/HttpsConnectionAdapterOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ public HttpsConnectionAdapterOptions()
/// </summary>
public X509Certificate2 ServerCertificate { get; set; }

/// <summary>
/// Specifies the intermediate certificates in the chain.
/// </summary>
public X509Certificate2Collection ServerCertificateIntermediates { get; set; }
Copy link
Member Author

@davidfowl davidfowl Aug 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The weird thing about this property is that it should really be set with the ServerCertificate in an atomic manner. If the cert is set, then the intermediates should be set as well (null is valid)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. The combined type should probably be defined in the runtime.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a SslStreamCertificateContext property instead of this X509Certificate2Collection?


/// <summary>
/// <para>
/// A callback that will be invoked to dynamically select a server certificate. This is higher priority than ServerCertificate.
Expand Down
28 changes: 19 additions & 9 deletions src/Servers/Kestrel/Core/src/Internal/Certificates/CertificateConfigLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,11 @@ public CertificateConfigLoader(IHostEnvironment hostEnvironment, ILogger<Kestrel
public IHostEnvironment HostEnvironment { get; }
public ILogger<KestrelServer> Logger { get; }

public bool IsTestMock => false;

public X509Certificate2 LoadCertificate(CertificateConfig certInfo, string endpointName)
public (X509Certificate2, X509Certificate2Collection) LoadCertificate(CertificateConfig certInfo, string endpointName)
{
if (certInfo is null)
{
return null;
return (null, null);
}

if (certInfo.IsFileCert && certInfo.IsStoreCert)
Expand All @@ -40,9 +38,21 @@ public X509Certificate2 LoadCertificate(CertificateConfig certInfo, string endpo
else if (certInfo.IsFileCert)
{
var certificatePath = Path.Combine(HostEnvironment.ContentRootPath, certInfo.Path);

X509Certificate2Collection intermediates = null;

if (certInfo.ChainPath != null)
{
var certificateChainPath = Path.Combine(HostEnvironment.ContentRootPath, certInfo.ChainPath);

intermediates = new X509Certificate2Collection();
intermediates.ImportFromPemFile(certificateChainPath);
}

if (certInfo.KeyPath != null)
{
var certificateKeyPath = Path.Combine(HostEnvironment.ContentRootPath, certInfo.KeyPath);

var certificate = GetCertificate(certificatePath);

if (certificate != null)
Expand All @@ -58,10 +68,10 @@ public X509Certificate2 LoadCertificate(CertificateConfig certInfo, string endpo
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
return PersistKey(certificate);
return (PersistKey(certificate), intermediates);
}

return certificate;
return (certificate, intermediates);
}
else
{
Expand All @@ -71,14 +81,14 @@ public X509Certificate2 LoadCertificate(CertificateConfig certInfo, string endpo
throw new InvalidOperationException(CoreStrings.InvalidPemKey);
}

return new X509Certificate2(Path.Combine(HostEnvironment.ContentRootPath, certInfo.Path), certInfo.Password);
return (new X509Certificate2(Path.Combine(HostEnvironment.ContentRootPath, certInfo.Path), certInfo.Password), intermediates);
}
else if (certInfo.IsStoreCert)
{
return LoadFromStoreCert(certInfo);
return (LoadFromStoreCert(certInfo), null);
}

return null;
return (null, null);
}

private static X509Certificate2 PersistKey(X509Certificate2 fullCertificate)
Expand Down
4 changes: 1 addition & 3 deletions src/Servers/Kestrel/Core/src/Internal/Certificates/ICertificateConfigLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Certificates
{
internal interface ICertificateConfigLoader
{
bool IsTestMock { get; }

X509Certificate2 LoadCertificate(CertificateConfig certInfo, string endpointName);
(X509Certificate2, X509Certificate2Collection) LoadCertificate(CertificateConfig certInfo, string endpointName);
}
}
7 changes: 6 additions & 1 deletion src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,9 @@ public override int GetHashCode() => HashCode.Combine(
}

// "CertificateName": {
// "Path": "testCert.pfx",
// "Path": "testCert.pem/pfx",
// "KeyPath": "key.pem",
// "ChainPath": "chain.pem",
// "Password": "testPassword"
// }
internal class CertificateConfig
Expand All @@ -357,6 +359,7 @@ public CertificateConfig(IConfigurationSection configSection)
// Bind explictly to preserve linkability
Path = configSection[nameof(Path)];
KeyPath = configSection[nameof(KeyPath)];
ChainPath = configSection[nameof(ChainPath)];
Password = configSection[nameof(Password)];
Subject = configSection[nameof(Subject)];
Store = configSection[nameof(Store)];
Expand All @@ -380,6 +383,8 @@ internal CertificateConfig()

public string Path { get; set; }

public string ChainPath { get; set; }

public string KeyPath { get; set; }

public string Password { get; set; }
Expand Down
20 changes: 9 additions & 11 deletions src/Servers/Kestrel/Core/src/Internal/SniOptionsSelector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,13 @@ public SniOptionsSelector(
{
var sslOptions = new SslServerAuthenticationOptions
{
ServerCertificate = certifcateConfigLoader.LoadCertificate(sniConfig.Certificate, $"{endpointName}:Sni:{name}"),
EnabledSslProtocols = sniConfig.SslProtocols ?? fallbackHttpsOptions.SslProtocols,
CertificateRevocationCheckMode = fallbackHttpsOptions.CheckCertificateRevocation ? X509RevocationMode.Online : X509RevocationMode.NoCheck,
};

if (sslOptions.ServerCertificate is null)
var (serverCert, intermediates) = certifcateConfigLoader.LoadCertificate(sniConfig.Certificate, $"{endpointName}:Sni:{name}");

if (serverCert is null)
{
if (fallbackHttpsOptions.ServerCertificate is null && _fallbackServerCertificateSelector is null)
{
Expand All @@ -63,21 +64,18 @@ public SniOptionsSelector(

if (_fallbackServerCertificateSelector is null)
{
// Cache the fallback ServerCertificate since there's no fallback ServerCertificateSelector taking precedence.
sslOptions.ServerCertificate = fallbackHttpsOptions.ServerCertificate;
// Cache the fallback ServerCertificate since there's no fallback ServerCertificateSelector taking precedence.
serverCert = fallbackHttpsOptions.ServerCertificate;
intermediates = fallbackHttpsOptions.ServerCertificateIntermediates;
}
}

if (sslOptions.ServerCertificate != null)
if (serverCert != null)
{
// This might be do blocking IO but it'll resolve the certificate chain up front before any connections are
// made to the server
sslOptions.ServerCertificateContext = SslStreamCertificateContext.Create((X509Certificate2)sslOptions.ServerCertificate, additionalCertificates: null);
}

if (!certifcateConfigLoader.IsTestMock && sslOptions.ServerCertificate is X509Certificate2 cert2)
{
HttpsConnectionMiddleware.EnsureCertificateIsAllowedForServerAuth(cert2);
sslOptions.ServerCertificate = serverCert;
sslOptions.ServerCertificateContext = SslStreamCertificateContext.Create(serverCert, intermediates);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be more straightforward to just have developers call this themselves by putting a SslStreamCertificateContext on HttpsConnectionAdapterOptions and preferring it to ServerCertificate similar to the way we treat ServerCertificateSelector.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wherever we load the cert, we're going to be using the context. The callback happens to late. It needs to happen at startup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HttpsConnectionAdapterOptions.ServerCertificateContext or whatever-we-call-it would be set at startup though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does that have to do with the SNI selector? Kestrel should always resolve this at startup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a comparison to point out that even though having both SslStreamCertificateContext and ServerCertificate on HttpsConnectionAdapterOptions would be redundant, ServerCertificateSelector and ServerCertificate are already redundant, so this isn't a new problem if we decide to add SslStreamCertificateContext.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what this has to do with SNI. Exposing SslStreamCertificateContext on HttpsConnectionAdapterOptions is fine and I have no pushback but it has nothing to do with this code path...

}

var clientCertificateMode = sniConfig.ClientCertificateMode ?? fallbackHttpsOptions.ClientCertificateMode;
Expand Down
8 changes: 5 additions & 3 deletions src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,8 +355,10 @@ public void Load()
}

// A cert specified directly on the endpoint overrides any defaults.
httpsOptions.ServerCertificate = CertificateConfigLoader.LoadCertificate(endpoint.Certificate, endpoint.Name)
?? httpsOptions.ServerCertificate;
var (serverCert, intermediates) = CertificateConfigLoader.LoadCertificate(endpoint.Certificate, endpoint.Name);

httpsOptions.ServerCertificate = serverCert ?? httpsOptions.ServerCertificate;
httpsOptions.ServerCertificateIntermediates = intermediates ?? httpsOptions.ServerCertificateIntermediates;

if (httpsOptions.ServerCertificate == null && httpsOptions.ServerCertificateSelector == null)
{
Expand Down Expand Up @@ -417,7 +419,7 @@ private void LoadDefaultCert()
{
if (ConfigurationReader.Certificates.TryGetValue("Default", out var defaultCertConfig))
{
var defaultCert = CertificateConfigLoader.LoadCertificate(defaultCertConfig, "Default");
var (defaultCert, intermediates) = CertificateConfigLoader.LoadCertificate(defaultCertConfig, "Default");
if (defaultCert != null)
{
DefaultCertificateConfig = defaultCertConfig;
Expand Down
2 changes: 1 addition & 1 deletion src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public HttpsConnectionMiddleware(ConnectionDelegate next, HttpsConnectionAdapter

// This might be do blocking IO but it'll resolve the certificate chain up front before any connections are
// made to the server
_serverCertificateContext = SslStreamCertificateContext.Create(certificate, additionalCertificates: null);
_serverCertificateContext = SslStreamCertificateContext.Create(certificate, additionalCertificates: options.ServerCertificateIntermediates);
}

var remoteCertificateValidationCallback = _options.ClientCertificateMode == ClientCertificateMode.NoCertificate ?
Expand Down
8 changes: 3 additions & 5 deletions src/Servers/Kestrel/Core/test/SniOptionsSelectorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,18 +751,16 @@ private class MockCertificateConfigLoader : ICertificateConfigLoader
{
public Dictionary<object, string> CertToPathDictionary { get; } = new Dictionary<object, string>(ReferenceEqualityComparer.Instance);

public bool IsTestMock => true;

public X509Certificate2 LoadCertificate(CertificateConfig certInfo, string endpointName)
public (X509Certificate2, X509Certificate2Collection) LoadCertificate(CertificateConfig certInfo, string endpointName)
{
if (certInfo is null)
{
return null;
return (null, null);
}

var cert = TestResources.GetTestCertificate();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gets called a lot. Maybe we should store this certificate in a static/fixture. We do that in some other test classes that need a lot of valid X509Certificate2s.

CertToPathDictionary.Add(cert, certInfo.Path);
return cert;
return (cert, null);
}
}

Expand Down