Skip to content

Minimal changes to support certificate chain-preloading at startup #24934

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

Merged
merged 1 commit into from
Aug 17, 2020
Merged

Minimal changes to support certificate chain-preloading at startup #24934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 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 @@ -68,6 +68,13 @@ public SniOptionsSelector(
}
}

if (sslOptions.ServerCertificate != null)
{
// This might be do blocking IO but it'll resolve the certificate chain up front before any connections are
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// This might be do blocking IO but it'll resolve the certificate chain up front before any connections are
// This might 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);
Expand Down
6 changes: 6 additions & 0 deletions 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 @@ -39,6 +39,7 @@ internal class HttpsConnectionMiddleware

// The following fields are only set by HttpsConnectionAdapterOptions ctor.
private readonly HttpsConnectionAdapterOptions _options;
private readonly SslStreamCertificateContext _serverCertificateContext;
private readonly X509Certificate2 _serverCertificate;
private readonly Func<ConnectionContext, string, X509Certificate2> _serverCertificateSelector;

Expand Down Expand Up @@ -89,6 +90,10 @@ public HttpsConnectionMiddleware(ConnectionDelegate next, HttpsConnectionAdapter
else
{
EnsureCertificateIsAllowedForServerAuth(_serverCertificate);

// 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(_serverCertificate, additionalCertificates: null);
}

var remoteCertificateValidationCallback = _options.ClientCertificateMode == ClientCertificateMode.NoCertificate ?
Expand Down Expand Up @@ -232,6 +237,7 @@ private Task DoOptionsBasedHandshakeAsync(ConnectionContext context, SslStream s
var sslOptions = new SslServerAuthenticationOptions
{
ServerCertificate = _serverCertificate,
ServerCertificateContext = _serverCertificateContext,
ServerCertificateSelectionCallback = selector,
ClientCertificateRequired = _options.ClientCertificateMode != ClientCertificateMode.NoCertificate,
EnabledSslProtocols = _options.SslProtocols,
Expand Down
5 changes: 2 additions & 3 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 @@ -385,10 +385,9 @@ public void FallsBackToHttpsConnectionAdapterCertificate()
{
{ "www.example.org", new SniConfig() }
};

var fallbackOptions = new HttpsConnectionAdapterOptions
{
ServerCertificate = new X509Certificate2()
ServerCertificate = new X509Certificate2(TestResources.GetCertPath("aspnetdevcert.pfx"), "testPassword")
};

var sniOptionsSelector = new SniOptionsSelector(
Expand Down Expand Up @@ -761,7 +760,7 @@ public X509Certificate2 LoadCertificate(CertificateConfig certInfo, string endpo
return null;
}

var cert = new X509Certificate2();
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.

@halter73 introduced IsTestMock because he wasn't using a real cert here. Something about not wanting to create a large number of real certs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that a real problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

When @halter73 is back, if the test haven't exploded by then can give me some feedback.

Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend storing the test cert in a static like we do for some other tests and removing IsTestMock since there's no longer an issue with this returning invalid certs. I guess it's no big deal if it isn't causing any slowness or reliability issues 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.

The second PR does it. I'll do the work in there.

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