[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

[HaoK]

We discussed this in triage, and given that this is no longer a test only change, and is an incremental improvement, we felt like this is a better fit for 8.0 rather than trying to get this in for rc2.

[davidfowl]

Why? Isn’t it a simple change? I consider it part of the overall fix to support full certificate chains

[HaoK]

Well, the current proposal for a change from @bartonjs is not what I would consider simple in the PR and we have no test coverage for the code (the tests mock this interface to supply the certs directly to the tests), which means we'd be have to rely on manual testing code or build a reasonable set of tests as part of this PR at the same time. It seems a bit of a stretch to say this meets the rc2 bar was our main concern in triage

[davidfowl]

What happens when you put the same pfx file as both the leaf cert and the chain?

[bartonjs]

Hm, looking at the code, I'm not sure a PFX will work at all...

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

Lines 37 to 43 in 0cde903

	else if (certInfo.IsFileCert)
	{
	var certificatePath = Path.Combine(HostEnvironment.ContentRootPath, certInfo.Path!);
	var fullChain = new X509Certificate2Collection();
	fullChain.ImportFromPemFile(certificatePath);
	if (certInfo.KeyPath != null)

The first thing that it does with the cert-as-a-file is load it as a multi-pem. If it's a PFX that'll throw.

If certInfo.Path is supposed to be able to be a PFX there's trouble here.

[davidfowl]

Hmm I think this is a regression then.

[HaoK]

That's not good, so we might have broken pfx entirely? I guess we just need a variant of this for pfx certs CanReadAndWriteWithHttpsConnectionMiddlewareWithPemCertificate() test:

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

Line 54 in c1d2ac7

public async Task CanReadAndWriteWithHttpsConnectionMiddlewareWithPemCertificate()

with a pfx to make sure that works, I'll try that on Tuesday

[davidfowl]

I was thinking about this and hoping that we have coverage for this at least 😄. Thanks @HaoK !

[HaoK]

Well, I copied the CanReadAndWriteWithHttpsConnectionMiddlewareWithPemCertificate() test and swapped in a pfx, the good news is that the original code we had works, and it still works with @bartonjs new code too.

The new test is here: https://github.com/dotnet/aspnetcore/pull/43719/files#diff-b50fac2e180288a9c83f5636d8d2e03f9d09b05b96051c056857f95a3417110bR103

So I'm not sure whether that means we should take the change or not for rc2?

[davidfowl]

Can you also verify it in an running app outside of this repo? If yes then we can skip for RC2 yeah

[HaoK]

So I verified that I could create a new rc2 app and pointed it at once of our test pfx files and it would startup fine via dotnet run:

    "Certificates": {
      "Default": {
        "Path": "C:\\Github\\aspnetcore\\src\\Servers\\Kestrel\\shared\\test\\TestCertificates\\aspnetdevcert.pfx",
        "Password": "pwd"
      }
    },

Verified that the wrong password would throw, and the right password would start up fine. Is this what you wanted me to double check?

[davidfowl]

Yep!

[erichiller]

Just checking, is the plan for this fix to be in .net7-rc2 ? I just spent a long time trying to figure this out before coming across this issue. It is quite confusing that chains work with pem and not with pfx.

[HaoK]

No, its too late for this to get into 7.0, the plan is for this to go into 8.0. Just to confirm what you are seeing on 7.0, a pfx chain won't work, but you aren't having issues using non chain pfx certs

[erichiller]

No, its too late for this to get into 7.0, the plan is for this to go into 8.0. Just to confirm what you are seeing on 7.0, a pfx chain won't work, but you aren't having issues using non chain pfx certs

Correct

However, I just found one interesting thing, if I have a chain-containing pem certificate file and load it in DefaultCertificates, then the chaining does not work. Exact same configuration properties copied to an endpoint and it does work.

Chaining does work in this configuration

{
    "Kestrel": {
        "Endpoints": {
            "Https": {
                "Url": "https://*:5007",
                "Certificate": { // here chaining does work 
                    "Path": "./cert.pem",
                    "KeyPath": "./key.pem"
                }
            }
        }
    }
}

Chaining doesn't work in this configuration

{
    "Kestrel": {
        "Endpoints": {
            "Https": {
                "Url": "https://*:5007",
            }
        },
        "Certificates": { 
            "Default": {
                "Path": "./cert.pem",
                "KeyPath": "./key.pem"
            }
        }
    }
}

[HaoK]

Thanks for that report @erichiller seems like a bug

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[msoler8785]

The workaround described by @erichiller doesn't seem to be working for me on 8.0.1.

[amcasey]

The workaround described by @erichiller doesn't seem to be working for me on 8.0.1.

Did it formerly work for you and stop working in 8.0.1 or are you just providing details about what you tried?

[msoler8785]

The workaround described by @erichiller doesn't seem to be working for me on 8.0.1.

Did it formerly work for you and stop working in 8.0.1 or are you just providing details about what you tried?

I'm just posting details about what I have tried and 8.0.1 seems to be the latest mainline release. I have tried this on older versions as well and nothing seems to work.

[LiamLamb]

The workaround described by @erichiller doesn't seem to be working for me on 8.0.1.

Did it formerly work for you and stop working in 8.0.1 or are you just providing details about what you tried?

I'm just posting details about what I have tried and 8.0.1 seems to be the latest mainline release. I have tried this on older versions as well and nothing seems to work.

I've had the same experience unfortunately. I've tried several workarounds, like the one mentioned above on related GitHub issues with no success.

It's a bit unclear to me what the plan is with this and when we can expect a fix. For reference, I am using PKCS12 certificates for both client and server.

I am a bit surprised that this is an issue in a release as mature as .NET 8.0, given it's a pretty important part of TLS as far as I understand 🫤.

I am also surprised this doesn't have more attention from the community in that regard, as surely many people have stumbled on this when setting up their PKI between .NET apps?

[amcasey]

It's a bit unclear to me what the plan is with this and when we can expect a fix. For reference, I am using PKCS12 certificates for both client and server.

I apologize for the lack of activity. We've had some team changes reducing our expertise in this area and, obviously, this is the sort of thing you only want to change very carefully and thoroughly. It hasn't fallen through the cracks, but I also can't promise near-term progress.

[rmmason]

I have just wasted a couple of days trying to figure out my issues with this. In the end I came across #52511 which detailed exactly what I was seeing and a workaround.

I'm not sure why fixing these issues are taking so long when this should be considered core basic behaviour that should just work?

Maybe you consider at least reopening #52511 and getting this solved if this is easy compared to the wider changes you are planning?