Update CertificateLoader.cs

[simon-curtis]

CertificateLoader loading wrong certificate

Fixes certificate selection by subject name

Description

The check for exact match to certificate subject fails as the forIssuer flag is set to true. This returns the issuer and not the subject this means that if two certificates have a partial match, it will just select the first one that has the search value in the name.

Fixes #49062 (in this specific format)

[ghost]

Thanks for your PR, @simon-curtis. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[martincostello]

I imagine the team would want a test to be added for this if the change is taken 😃

[simon-curtis]

I would tend to agree, but how do you provide a unit test for something to environment specific? I.e. there aren't any tests covering this now.

[amcasey]

I would tend to agree, but how do you provide a unit test for something to environment specific? I.e. there aren't any tests covering this now.

It's possible we have a way to mock the store. Let me see if I can find a test that does that (there may not be one).

[amcasey]

This looks correct to me, but let's see if we can figure out a way to test this.

[amcasey]

Re-running the tests, since the failure is unrelated.
/azp run

[amcasey]

There's one test in HttpsConnectionMiddlewareTests, but it's not great. Given that the original PR didn't add a test and the fix seems self-explanatory (after making it a named argument 😉), I can live without one, assuming there's been some manual validation.

@halter73 @martincostello If you happen to know how to add test coverage for this, feel free to jump in.

[simon-curtis]

Im just trying to load the solution up in Rider, and oh boy it's taking a while.

Is it just the certificate.GetNameInfo(..., false) behaivour we need to test rather then the whole method inside CertificateLoader.LoadFromStoreCert?

[simon-curtis]

Change flag to named argument

[Tratcher]

@amcasey can you do a manual test for this change?

[amcasey]

Im just trying to load the solution up in Rider, and oh boy it's taking a while.

Don't forget about Codespaces. 😉

[amcasey]

Sorry, it took me a little while to remember how to create certs with just the right properties. LGTM

[amcasey]

Any remaining concerns @Tratcher?

[dan-olsen]

@Tratcher Anyway this could be back ported to .NET 8? We only use LTS versions and I would not like to have to continue to have the workaround @simon-curtis mentioned in #49062 until .NET 10.

[ghost]

Hi @dan-olsen. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

[Tratcher]

@dan-olsen it looks like the regression happened in 6.0 (#34582). Given the availability of a workaround, and so few reports of the issue, it's unlikely to qualify for an 8.0 patch. If this is a significant blocker for you, please open a new issue and explain why.