Add support for loading certificate chains from configuration. #24935

davidfowl · 2020-08-15T17:09:22Z

Adds a ServerCertificateIntermediates property to HttpsConnectionAdapterOptions
Adds a Chain property to configuration. This only supports PEM certificates.

TODO: Add tests

Fixes #21513
Contributes to #21512

src/Servers/Kestrel/Core/src/Internal/Certificates/CertificateConfigLoader.cs

bartonjs

Looks good aside from the one confusing line

davidfowl · 2020-08-17T04:37:29Z

src/Servers/Kestrel/Core/src/HttpsConnectionAdapterOptions.cs

+        /// <summary>
+        /// Specifies the intermediate certificates in the chain.
+        /// </summary>
+        public X509Certificate2Collection ServerCertificateIntermediates { get; set; }


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)

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

Should we add a SslStreamCertificateContext property instead of this X509Certificate2Collection?

davidfowl · 2020-08-17T04:38:51Z

@bartonjs if we wanted to support resolving the chain for PKCS #12 files (pfx) would I call Import on the pfx itself?

bartonjs · 2020-08-17T05:01:09Z

if we wanted to support resolving the chain for PKCS #12 files (pfx) would I call Import on the pfx itself?

X509Certificate2Collection coll = new X509Certificate2Collection();
coll.Import(pfxPath, pfxPwd, flags);

If there's exactly one cert that returns true for HasPrivateKey, you can hold its reference as the target cert and remove it from the collection (removing it isn't strictly required, I guess...).

halter73 · 2020-08-18T21:24:31Z

src/Servers/Kestrel/Core/test/SniOptionsSelectorTests.cs

                }

-                var cert = new X509Certificate2();
+                var cert = TestResources.GetTestCertificate();


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.

halter73 · 2020-08-18T21:29:04Z

src/Servers/Kestrel/Core/src/Internal/ConfigurationReader.cs

@@ -346,6 +346,8 @@ internal class SniConfig

    // "CertificateName": {
    //     "Path": "testCert.pfx",
+    //     "KeyPath": "",
+    //     "ChainPath": "",


Nit: Use fake .pem files to show the expected file type for KeyPath/ChainPath.

halter73 · 2020-08-18T21:35:32Z

src/Servers/Kestrel/Core/src/Internal/SniOptionsSelector.cs

+                    // This might be do blocking IO but it'll resolve the certificate chain up front before any connections are
+                    // made to the server
+                    sslOptions.ServerCertificate = serverCert;
+                    sslOptions.ServerCertificateContext = SslStreamCertificateContext.Create(serverCert, intermediates);


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.

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

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

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

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.

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

src/Servers/Kestrel/Core/src/Internal/Certificates/ICertificateConfigLoader.cs

ghost · 2020-08-24T16:14:18Z

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

javiercn

This looks good!

There's missing tests and stuff, but other than that the changes are fine I think!

Pilchie · 2020-08-31T22:11:42Z

👀

dougbu · 2020-11-09T23:23:32Z

@davidfowl this PR is blocking me deleting the old release/5.0-rc2 branch. Could you please close it❔

davidfowl · 2020-11-09T23:26:58Z

As long as you don't delete it

dougbu · 2020-11-09T23:33:44Z

As long as you don't delete it

If you need the branch, probably safest to rebase it before I burn the release/5.0-rc2 branch. I don't think GitHub removes branches when left dangling after a PR is closed but the PR does protect the branch a bit.

dougbu · 2020-11-13T18:57:58Z

🆗 @davidfowl I'll do the rebase to unblock myself. Should this PR target master now❔

davidfowl · 2020-11-13T20:45:52Z

Yep

- Adds a ServerCertificateIntermediates property to HttpsConnectionAdapterOptions - Adds a Chain property to configuration. This only supports PEM certificates. - Import intermediates if chain path specified

dougbu · 2020-11-13T20:51:01Z

Done

ghost added the area-servers label Aug 15, 2020

davidfowl added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 15, 2020

davidfowl requested review from halter73, Tratcher, javiercn and bartonjs August 15, 2020 19:46

bartonjs reviewed Aug 16, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Certificates/CertificateConfigLoader.cs Outdated Show resolved Hide resolved

bartonjs approved these changes Aug 16, 2020

View reviewed changes

davidfowl commented Aug 17, 2020

View reviewed changes

Base automatically changed from davidfowl/cert-preload to master August 17, 2020 19:20

halter73 reviewed Aug 18, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Certificates/ICertificateConfigLoader.cs Show resolved Hide resolved

davidfowl added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Aug 24, 2020

davidfowl force-pushed the davidfowl/cert-chain-support branch from 3e874e6 to cec7d75 Compare August 24, 2020 16:31

davidfowl changed the base branch from master to release/5.0 August 24, 2020 16:31

davidfowl added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 24, 2020

javiercn reviewed Aug 26, 2020

View reviewed changes

davidfowl force-pushed the davidfowl/cert-chain-support branch from cec7d75 to e00b3e3 Compare August 28, 2020 17:38

davidfowl mentioned this pull request Sep 1, 2020

Add async ServerOptionsSelectionCallback UseHttps overload #25390

Merged

davidfowl force-pushed the davidfowl/cert-chain-support branch from e00b3e3 to f098e95 Compare September 12, 2020 06:19

davidfowl changed the base branch from release/5.0 to release/5.0-rc2 September 12, 2020 06:20

runfoapp bot mentioned this pull request Sep 29, 2020

Test initial triage jaredpar/runfo#50

Closed

davidfowl mentioned this pull request Oct 21, 2020

Kestrel stops serving https (http2) requests after reboot under load #21183

Closed

davidfowl added 2 commits November 13, 2020 12:49

Add support for loading certificate chains from configuration.

339bf3a

- Adds a ServerCertificateIntermediates property to HttpsConnectionAdapterOptions - Adds a Chain property to configuration. This only supports PEM certificates. - Import intermediates if chain path specified

Actually read the chain path section

124b365

dougbu force-pushed the davidfowl/cert-chain-support branch from 567dee9 to 124b365 Compare November 13, 2020 20:50

dougbu changed the base branch from release/5.0-rc2 to master November 13, 2020 20:50

shirhatti linked an issue Nov 16, 2020 that may be closed by this pull request

Kestrel stops serving https (http2) requests after reboot under load #21183

Closed

Base automatically changed from master to main January 22, 2021 01:32

davidfowl closed this Mar 12, 2021

davidfowl mentioned this pull request Mar 23, 2021

Support specifying a full certificate chain in HttpsConnectionAdapterOptions #21513

Closed

dougbu deleted the davidfowl/cert-chain-support branch August 21, 2021 22:36

davidfowl mentioned this pull request Sep 7, 2021

Kestrel on Debian/Ubuntu not sending private intermediate cert #36202

Closed

HaoK mentioned this pull request May 31, 2022

Kestrel: Support full cert chain #41944

Merged

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for loading certificate chains from configuration. #24935

Add support for loading certificate chains from configuration. #24935

davidfowl commented Aug 15, 2020 •

edited

Loading

bartonjs left a comment

davidfowl Aug 17, 2020 •

edited

Loading

halter73 Aug 18, 2020

halter73 Aug 18, 2020

davidfowl commented Aug 17, 2020

bartonjs commented Aug 17, 2020

halter73 Aug 18, 2020

halter73 Aug 18, 2020

halter73 Aug 18, 2020

davidfowl Aug 18, 2020

halter73 Aug 18, 2020

davidfowl Aug 18, 2020

halter73 Aug 18, 2020

davidfowl Aug 19, 2020

ghost commented Aug 24, 2020

javiercn left a comment

Pilchie commented Aug 31, 2020

dougbu commented Nov 9, 2020

davidfowl commented Nov 9, 2020

dougbu commented Nov 9, 2020

dougbu commented Nov 13, 2020

davidfowl commented Nov 13, 2020

dougbu commented Nov 13, 2020

Add support for loading certificate chains from configuration. #24935

Add support for loading certificate chains from configuration. #24935

Conversation

davidfowl commented Aug 15, 2020 • edited Loading

bartonjs left a comment

Choose a reason for hiding this comment

davidfowl Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Aug 17, 2020

bartonjs commented Aug 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 24, 2020

javiercn left a comment

Choose a reason for hiding this comment

Pilchie commented Aug 31, 2020

dougbu commented Nov 9, 2020

davidfowl commented Nov 9, 2020

dougbu commented Nov 9, 2020

dougbu commented Nov 13, 2020

davidfowl commented Nov 13, 2020

dougbu commented Nov 13, 2020

davidfowl commented Aug 15, 2020 •

edited

Loading

davidfowl Aug 17, 2020 •

edited

Loading