[certs] Full chain support followup

[HaoK]

See #41944 (comment)

Some followup questions/work:

from @bartonjs

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.

From @davidfowl

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.

[davidfowl]

I think you're doing the right things in the PR but we need to test (in this follow up) a PFX file with the full chain.

[davidfowl]

This is what we want to solve, this exact problem https://stackoverflow.com/questions/65926215/kestrel-fails-tls-handshake-after-attempt-to-download-intermediate-certificate-f

I can't figure out why it's attempting to download this certificate, considering the same certificate is embedded in the PKCS#12 PFX bundle that is bound to Kestrel. Am I supposed to load either the root CA or intermediate into the CA trust folder in file system? (Ex. /usr/local/share/ca-certificates/ - I can't load the intermediate there, only the CA?)

@bartonjs Can you help with the exact code we need to load the full chain from a pfx file?

[bartonjs]

Loading a PFX as a collection is easy:

X509Certificate2Collection coll = new X509Certificate2Collection();
coll.Import(pfx, pwd, flags);

(instead of new X509Certificate2(pfx, pwd, flags)).

But that's just an arbitrary collection. The cert that the single-cert constructor would pick is either the first or last one that has cert.HasPrivateKey:true (there's a stack/reversal somewhere, so I don't remember off the top of my head if it's first or last in the collection ordering). If there aren't any then a) it won't work for your scenarios and b) the ctor would then fall back to the first (or last) cert. (If the collection is empty then the single cert ctor would throw at this point).

So let's go with "Single":

X509Certificate2 target = coll.Single(c => c.HasPrivateKey);
coll.Remove(target);

You can now assume that the remainder of coll is related to the certificate and is part of the chain... or throw the collection into X509ChainPolicy.ExtraStore and build an X509Chain to provably trim it down. Assuming you're just going to pass it into SslStreamCertificateContext, it'll do that for you, so Single + Remove is probably all you need.

[HaoK]

So just to make sure I understand, we think the code is fine as is, I just need to add a test that loads the PFX into ServerCertificateChain using the above:

X509Certificate2Collection coll = new X509Certificate2Collection();
coll.Import(pfx, pwd, flags);
X509Certificate2 target = coll.Single(c => c.HasPrivateKey);
coll.Remove(target);
// HttpsConnectionAdapterOptions.ServerCertificateChain = coll, and verify that X509ChainPolicy.ExtraStore contains the certs in coll

[HaoK]

Note, it looks like the test already is testing this format where clientCertificate which has the privateKey is not part of the clientChain. So should I add a test in the other direction where clientChain is included in the clientChain as well and verify thing s?

    public async Task ServerCertificateChainInExtraStore()
    {
        var streams = new List<SslStream>();
        CertHelper.CleanupCertificates(nameof(ServerCertificateChainInExtraStore));
        // clientCertificate hasPrivateKey = true and is not part of clientChain
        (var clientCertificate, var clientChain) = CertHelper.GenerateCertificates(nameof(ServerCertificateChainInExtraStore), longChain: true, serverCertificate: false);

[davidfowl]

@HaoK it seems like we need to always import the collection when we get a pfx. We don’t do this today do we?

[HaoK]

I'm not sure I understand since the HttpsConnectionAdapterOptions.ServerCertificateChain property is a X509Certificate2Collection, so if they need to import something from a pfx, they can just do this code themselves right?

Or am I misunderstanding the ask, are we supposed to detect if the cert file is a pfx chain and do stuff automatically to load it into the SslStreamCertificateContext?

[bartonjs]

If no keypath value is specified then you load certinfo.Path/certinfo.Password at

https://github.com/dotnet/aspnetcore/blob/main/src/Servers/Kestrel/Core/src/Internal/Certificates/CertificateConfigLoader.cs#L74

The target scenario there is, presumably, that certinfo.Path is specifying a PFX. My understanding of David's comments are that he would like that PFX to be loaded as a collection, so something similar to

fullChain.Import(Path.Combine(HostEnvironment.ContentRootPath, certInfo.Path!), certInfo.Password);
X509Certificate2 target = fullChain.Single(c => c.HasPrivateKey);
fullChain.Remove(target);
return (target, fullChain);

Depending on how you want/allow PFX loads to combine with a chain-PEM file you might want to clear the collection before doing the Import.

[HaoK]

Ah okay, that makes sense, thanks for the clarification @bartonjs I'll start on the PR now