CheckTrustedIssuer: Fixes for invalid chains by NickCraver · Pull Request #2665 · StackExchange/StackExchange.Redis (original) (raw)

The "right" way to do this in .NET 5+ is to use CustomTrustStore. But unfortunately it does not work for your needs since it is based on a trusted root, not a trusted issuer.

The reason I point this out is that AllowUnknownCertificateAuthority can be unreliable. Android, for example, does not work with AllowUnknownCertificateAuthority (reference: https://github.com/dotnet/runtime/blob/80b1e143c686c9548ee5930f2466805a76f095bd/src/libraries/System.Security.Cryptography/tests/X509Certificates/DynamicChainTests.cs#L198). The Android chain builder just doesn't give back partial or untrusted chains. This is "fine" since it doesn't cause you to fail open (that is, not a security issue) but someone could run in to a situation where TrustIssuer doesn't work.

Who is calling Redis from Android, Kevin?

Well, you never know, plus there are other platform quirks about returning partial chains.

So, should I use CustomTrustStore?

Initially I was going to recommend it for .NET 5+, but seeing how your implementation is based on trusting an arbitrary anchor, not a root, it makes it difficult to recommend, or would otherwise have different behavior.

You still aren't looking at the pull request Kevin

Oh, right. Let's take a look. Well, it builds a chain and permits exactly one error, AUCA, which is fine. If it's valid, then see if any of the certificates in the chain match the "trusted" certificate by thumbprint. If nothing matches, then "false".

That all seems fine.

Suggestions

1. It is possible for X509Chain.Build to throw CryptographicException in very rare scenarios. You may want to catch it and return false instead of letting the exception bubble up.
2. X509Chain is disposable. You may want to dispose of it instead of leaving it up to the GC.