Kestrel: Support full cert chain #41944

HaoK · 2022-05-31T20:31:47Z

TBD: tests

HaoK · 2022-05-31T20:35:28Z

@davidfowl does this sorta resemble what you had in mind? I tried to take what was in #21513 and #24935

davidfowl · 2022-06-01T05:37:14Z

Does full chain include the leaf cert or is it just the intermediates? Looking at this PR I did a while back it seems like they were intermediates? Maybe we can support both but that means doing some logic to see if the full chain includes the leaf (maybe?)

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

HaoK · 2022-06-01T21:02:05Z

Does full chain include the leaf cert or is it just the intermediates

@davidfowl I'm not sure, but it looks like you were involved in the API design of the X509Certificate2Collection.ImportFromPemFile from here: dotnet/runtime#31944

I'm still trying to make sense of this stuff, but in case this refreshes your memory faster than I get there...

Link to the PR that adds these APIs is here: https://github.com/dotnet/runtime/pull/38280/files#diff-dbecad4bd3d918d3af329011f49c4fc79e1347d86f9bd51d15674ec48fd9f33cR262

HaoK · 2022-06-01T21:08:04Z

So from the tests in the PR: https://github.com/dotnet/runtime/pull/38280/files#diff-43d4431a84a8bd471a7795198aaa4570de224ad144ef8633be53dbb179540172R1473

I think we would get all the certs right? So the leaf would be included, why would we only want the intermediates?

davidfowl · 2022-06-03T08:19:22Z

@HaoK it was common in other servers and what you end up getting when you use lets encrypt or certs in the non pfx format.

e.g. When you run Lets encrypt + certbot this is what I ended up with

/etc/letsencrypt/live/<domain>/privkey.pem
/etc/letsencrypt/live/<domain>/cert.pem
/etc/letsencrypt/live/<domain>/chain.pem

The chain file didn't include cert.pem (the leaf cert). So we don't want customers to have to do any more processing of these files once they get them. Before we natively support PEM, they had to run something to turn it into a pfx (Which sucked). Now we support PEM and they can pass the private key and cert, and the chain is downloaded at startup (it used to be on the first connection). This change completes the cycle and now lets you specify either the fullchain or the leaf+ intermediates.

I believe its also possible to output a fullchain.pem, so maybe that is what we tell people to use?

MarkCiliaVincenti · 2022-06-29T10:14:28Z

I had worked around the issue and created a library at https://github.com/MarkCiliaVincenti/TlsCertificateLoader

HaoK · 2022-08-04T18:24:55Z

So I created a test cert chain (root_ca -> intermediate_1 -> intermediate_2 -> leaf.com (which is the bundled certs) using smallstep/cli (which was amazingly easy)

This PR when loading the leaf.com crt bundle as the FullChain results in:

leaf.com (signed by intermediate 2)
intermediate_2 (signed by intermediate 1)

@davidfowl does this match what your expectations would be?

I added a test in the SniOptionsSelectorTests to verify the cert chain, I'm not sure if we have other tests which exercise certs, @halter73 can you point to places/suggest what other tests should I add for this?

HaoK · 2022-08-04T18:44:10Z

Does full chain include the leaf cert

It appears so, at least the smallstep cli --bundle command does.

cc @blowdart to take a look at all this too

blowdart · 2022-08-04T18:48:47Z

Chain shouldn't include the actual cert for the site, but it's not a well defined term at all, so you can't make the assumption.

Windows exports are for the leaf and you can then include the chain with it, openssl and CAs provide the chain as a separate bundle.

src/Servers/Kestrel/shared/test/TestResources.cs

halter73 · 2022-08-04T20:18:07Z

I added a test in the SniOptionsSelectorTests to verify the cert chain, I'm not sure if we have other tests which exercise certs, @halter73 can you point to places/suggest what other tests should I add for this?

KestrelConfigurationLoaderTests has a bunch of cert related tests. There's also HttpsConnectionMiddlewareTests.

src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs

HaoK · 2022-08-08T16:52:44Z

@bartonjs would you be able to take a look and see if this PR looks good overall? We'd love sign off from a cert expert that this looks good.

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

bartonjs · 2022-08-08T18:14:22Z

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

@@ -37,6 +37,9 @@ public CertificateConfigLoader(IHostEnvironment hostEnvironment, ILogger<Kestrel
        else if (certInfo.IsFileCert)
        {
            var certificatePath = Path.Combine(HostEnvironment.ContentRootPath, certInfo.Path!);
+            var fullChain = new X509Certificate2Collection();
+            fullChain.ImportFromPemFile(certificatePath);
+
            if (certInfo.KeyPath != null)
            {
                var certificateKeyPath = Path.Combine(HostEnvironment.ContentRootPath, certInfo.KeyPath);


In this if block, you could just make certificate be fullChain[0] (assuming it's non-empty), and then you could remove certificate from the collection; depending on what public API guarantees you're making.

The two halves of that sentence are:

We've already done the work of loading the certificate instance, why go open the file and process the contents a second time?

The perf of a chain build will be marginally faster if you remove unnecessary elements. Since the target certificate is already specified as the target it won't need to be found from the collection. By removing it early you save a "nope, not the one I'm looking for, next!"

If you don't remove it, because you want the "full" chain to be in the HttpsOptions.ServerCertificateChain collection, then you have to decide if you want to have the same instance in the property and the collection. If "yes" to the full chain and "no" to the same instance... then leave the code as-is 😄.

And the reason I put "full" in quotes is that I don't think Let's Encrypt puts their root cert in that file, so the "full" chain is really "the chain except the root". But it's as "full" as that file is, I suppose.

@adityamandaleeka @blowdart @davidfowl @halter73 I can't say I have anywhere near enough context for this feature request/area to be able to personally decide how this should work...

Do we want to just vote or something? Personally I like what @bartonjs is suggesting, i.e. we just use fullChain[0] and remove the cert from the collection.

Yea remove it. A chain is everything above the leaf in my mind. Need to document carefully though.

I did a bunch of research here a year ago and I can dig up my notes. When you get the certs from certbot for lets encrypt you can get both the fullchain.pem or the chain.pem + cert.pem (https://community.letsencrypt.org/t/generating-cert-pem-chain-pem-and-fullchain-pem-from-order-certificate/78376/6).

We should also support loading the full chai from PFX files if it is there. NGINX supports the full chain as well (https://serverfault.com/questions/987612/nginx-ssl-config-for-cert-pem-and-chain-pem).

I'd like us to support both providing just the intermediates without the leaf cert and the full chain and we can remove the leaf cert (assuming this is easy). That makes us a bit more friendly.

I'm not sure I understand the nuances here. I updated the PR to Jeremy's suggestion so we remove the cert from the chain if we have a key path.

Is pfx support something different or does that just work?

I'd like us to support both providing just the intermediates without the leaf cert and the full chain and we can remove the leaf cert (assuming this is easy). That makes us a bit more friendly.

The PR currently only supports full chain right? So is the intermediates only scenario we don't assume fullChain[0] is the leaf?

There are some differences in using the cert from the chain vs the current GetCertificate method, as a bunch of tests now fail. So I think I'm just leave this code alone for now, and I will file a new issue to track @davidfowl and @bartonjs suggestions which I'd imagine we can take in rc2 still. I just want to try and get something in for rc1 this week

@bartonjs so the MacOS error is: "Status: One or more certificates required to validate this certificate cannot be found." This is a bit weird since this works fine on ubuntu and windows, but I know we've skipped on MacOS many of our other cert tests today... I'm tempted to just skip this on macOS and file an issue to look at this later, unless this is something that is indicative of something seriously being broken

I filed #43193 to track resolving the full chain/intermediate/removing a cert.

I think many tests were all failing because the new test I copied from runtime was nuking all of our test certs which broke all the other tests as part of its cleanup, I scoped its cleanup to only the certs it was creating and things seem happier now.

HaoK · 2022-08-08T18:21:23Z

src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs

+            // Verify we can construct full chain
+            if (chain.ChainElements.Count < clientChain.Count)
+            {
+                throw new InvalidOperationException($"chain cannot be built {chain.ChainElements.Count}");


@bartonjs Hrm we seem to hit an error on helix with this test on mac OSX only, should I just skip?

System.InvalidOperationException : chain cannot be built 1

https://dev.azure.com/dnceng/public/_build/results?buildId=1929785&view=ms.vss-test-web.build-test-results-tab&runId=49884456&resultId=121131&paneView=debug

Cert chains generally behave the same on macOS as other platforms. You'll probably benefit from printing out what the chain looked like and what all chain status values turned up (at the chain overall, and at the individual elements)

Here's what the long cert chain looks like on windows when it's passing in the debugger in a sample run:

▶ | [0] | {[Subject] CN=A Revocation Test CA 2, O=ServerCertificateChainInExtraStore [Issuer] CN=A Revocation Test CA 1, O=ServerCertificateChainInExtraStore [Serial Number] 00FCC2A15A96F406D0 [Not Before] 8/10/2022 1:09:16 AM [Not After] 11/14/2022 12:03:16 AM [Thumbprint] B70A0E006EF6E77280D355054ED33285901EA315 } | System.Security.Cryptography.X509Certificates.X509Certificate2 ▶ | [1] | {[Subject] CN=A Revocation Test CA 1, O=ServerCertificateChainInExtraStore [Issuer] CN=A Revocation Test CA 0, O=ServerCertificateChainInExtraStore [Serial Number] 00F11BE02C645700DD [Not Before] 8/10/2022 1:08:16 AM [Not After] 11/14/2022 12:04:16 AM [Thumbprint] 7EC341DBC3D9AE8A8E5F372C79440DA446C50616 } | System.Security.Cryptography.X509Certificates.X509Certificate2 ▶ | [2] | {[Subject] CN=A Revocation Test CA 0, O=ServerCertificateChainInExtraStore [Issuer] CN=A Revocation Test Root, O=ServerCertificateChainInExtraStore [Serial Number] 00F2E046173643F2E2 [Not Before] 8/10/2022 1:07:16 AM [Not After] 11/14/2022 12:05:16 AM [Thumbprint] 5C2B53E144C96402F1ECA65A1CBBE77B234C6CCD } | System.Security.Cryptography.X509Certificates.X509Certificate2 ▶ | [3] | {[Subject] CN=A Revocation Test Root, O=ServerCertificateChainInExtraStore [Issuer] CN=A Revocation Test Root, O=ServerCertificateChainInExtraStore [Serial Number] 3901199FF26B8536 [Not Before] 8/10/2022 1:06:16 AM [Not After] 11/14/2022 12:06:16 AM [Thumbprint] 649BAB9F7B5BBABEA7394373448360E7F686A338 } | System.Security.Cryptography.X509Certificates.X509Certificate2

Its actually failing now on Windows and OSX, but passes on Ubuntu??

Windows failure:

System.InvalidOperationException : chain cannot be built ChainStatus: A certificate chain could not be built to a trusted root authority. Subject: CN=ServerCertificateChainInExtraStore, O=ServerCertificateChainInExtraStore Issuer: CN=A Revocation Test CA 2, O=ServerCertificateChainInExtraStore Status:

OSX failure

System.InvalidOperationException : chain cannot be built ChainStatus: One or more certificates required to validate this certificate cannot be found. Subject: CN=ServerCertificateChainInExtraStore, O=ServerCertificateChainInExtraStore Issuer: CN=A Revocation Test CA 2, O=ServerCertificateChainInExtraStore Status: One or more certificates required to validate this certificate cannot be found. 111

Filed #43273 to track figuring out what's going on with OSX. Simplified the test to just check that any certs were added to the extra store, since runtime already has tests covering the lower level apis anyways, so we don't need to the test code that verifies that the generated test certs chain can be built successfully

ghost · 2022-09-06T20:02:19Z

@HaoK, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!

ghost added the area-runtime label May 31, 2022

halter73 reviewed Jun 1, 2022

View reviewed changes

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

halter73 reviewed Jun 1, 2022

View reviewed changes

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

HaoK force-pushed the haok/certs branch from 49f4179 to 9fdf46e Compare August 3, 2022 23:16

HaoK commented Aug 4, 2022

View reviewed changes

src/Servers/Kestrel/shared/test/TestResources.cs Outdated Show resolved Hide resolved

HaoK commented Aug 6, 2022

View reviewed changes

src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs Outdated Show resolved Hide resolved

HaoK marked this pull request as ready for review August 8, 2022 16:51

HaoK requested review from Tratcher, BrennanConroy and JamesNK as code owners August 8, 2022 16:51

HaoK requested a review from bartonjs August 8, 2022 16:53

bartonjs reviewed Aug 8, 2022

View reviewed changes

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

bartonjs reviewed Aug 8, 2022

View reviewed changes

HaoK commented Aug 8, 2022

View reviewed changes

HaoK and others added 5 commits August 9, 2022 19:29

Kestrel: Support full cert chain

0bb35c9

Update doc comments (per blowdart)

21a26a7

Update CertificateConfigLoader.cs

db88bda

Cleanup

c571af2

Fixup

1ba7ca4

HaoK and others added 10 commits August 9, 2022 19:29

Fix warnings

510a195

Cleanup

9e7d947

Remove offline

e18c53d

Dump chainElement when it fails to build in test

2311399

Remove cert from chain, also print more useful status

54da27a

Cleanup

6db0573

Undo changes to full chain

4a0fc6f

See if offline is causing test failures

5091181

Update SniOptionsSelector.cs

03e2f51

Remove offline

b8a66c1

HaoK force-pushed the haok/certs branch from 4da0c45 to b8a66c1 Compare August 10, 2022 02:30

HaoK added 2 commits August 9, 2022 22:50

Test

e8369cf

Tweak test cleanup

f949d16

HaoK mentioned this pull request Aug 10, 2022

Full chain support followup #43193

Open

HaoK and others added 6 commits August 10, 2022 12:46

Skip on mac for now

0a25765

Add more logging

5760a8d

Try a long chain

d543da6

Print issuer and subject

91c423b

Simplify test

70028d6

Skip on OSX

ff756dd

HaoK mentioned this pull request Aug 14, 2022

Figure out why FullChain test fails on OSX #43273

Open

HaoK enabled auto-merge (squash) August 14, 2022 04:12

davidfowl approved these changes Aug 14, 2022

View reviewed changes

HaoK merged commit c1d2ac7 into main Aug 14, 2022

HaoK deleted the haok/certs branch August 14, 2022 04:51

ghost added this to the 7.0-rc1 milestone Aug 14, 2022

adityamandaleeka added the blog-candidate Consider mentioning this in the release blog post label Sep 6, 2022

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

Kestrel: Support full cert chain #41944

Kestrel: Support full cert chain #41944

HaoK commented May 31, 2022 •

edited

Loading

HaoK commented May 31, 2022

davidfowl commented Jun 1, 2022

HaoK commented Jun 1, 2022 •

edited

Loading

HaoK commented Jun 1, 2022

davidfowl commented Jun 3, 2022 •

edited

Loading

MarkCiliaVincenti commented Jun 29, 2022

HaoK commented Aug 4, 2022 •

edited

Loading

HaoK commented Aug 4, 2022

blowdart commented Aug 4, 2022

halter73 commented Aug 4, 2022 •

edited

Loading

HaoK commented Aug 8, 2022 •

edited

Loading

bartonjs Aug 8, 2022

HaoK Aug 8, 2022

blowdart Aug 8, 2022

davidfowl Aug 9, 2022

HaoK Aug 9, 2022

HaoK Aug 9, 2022

HaoK Aug 10, 2022

HaoK Aug 8, 2022

bartonjs Aug 8, 2022

HaoK Aug 12, 2022

HaoK Aug 14, 2022 •

edited

Loading

ghost commented Sep 6, 2022

Kestrel: Support full cert chain #41944

Kestrel: Support full cert chain #41944

Conversation

HaoK commented May 31, 2022 • edited Loading

HaoK commented May 31, 2022

davidfowl commented Jun 1, 2022

HaoK commented Jun 1, 2022 • edited Loading

HaoK commented Jun 1, 2022

davidfowl commented Jun 3, 2022 • edited Loading

MarkCiliaVincenti commented Jun 29, 2022

HaoK commented Aug 4, 2022 • edited Loading

HaoK commented Aug 4, 2022

blowdart commented Aug 4, 2022

halter73 commented Aug 4, 2022 • edited Loading

HaoK commented Aug 8, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK Aug 14, 2022 • edited Loading

Choose a reason for hiding this comment

ghost commented Sep 6, 2022

HaoK commented May 31, 2022 •

edited

Loading

HaoK commented Jun 1, 2022 •

edited

Loading

davidfowl commented Jun 3, 2022 •

edited

Loading

HaoK commented Aug 4, 2022 •

edited

Loading

halter73 commented Aug 4, 2022 •

edited

Loading

HaoK commented Aug 8, 2022 •

edited

Loading

HaoK Aug 14, 2022 •

edited

Loading