CheckCertificateRevocation is ignored unless SslProtocols is also specified, causing revocations to not be checked

[jaswope]

As the title states, ConfigurationOptions.CheckCertificateRevocation is ignored unless ConfigurationOptions.SslProtocols is specified, and the resulting behavior is that the revocation check is always skipped. This seems like unexpected behavior, especially given that the default value for ConfigurationOptions.CheckCertificateRevocation is true. At the very least it is inconsistent behavior that can cause surprising bugs to occur when the an application changes to specifying the TLS version.

The fault lies in this block of code:

StackExchange.Redis/src/StackExchange.Redis/ExtensionMethods.cs

Lines 151 to 167 in 40595ca

	internal static void AuthenticateAsClient(this SslStream ssl, string host, SslProtocols? allowedProtocols, bool checkCertificateRevocation)
	{
	if (!allowedProtocols.HasValue)
	{
	//Default to the sslProtocols defined by the .NET Framework
	AuthenticateAsClientUsingDefaultProtocols(ssl, host);
	return;
	}
	var certificateCollection = new X509CertificateCollection();
	ssl.AuthenticateAsClient(host, certificateCollection, allowedProtocols.Value, checkCertificateRevocation);
	}
	private static void AuthenticateAsClientUsingDefaultProtocols(SslStream ssl, string host)
	{
	ssl.AuthenticateAsClient(host);
	}

In line 166 the overload used does not actually check for certificate revocation. Unfortunately the next best overload does not exist pre 4.7, and the default for SslProtocols changed between 4.6 and 4.7. That makes this not a straightforward change, otherwise this would be a pull request rather than an issue.

[mgravell]

Very interesting, and excellent analysis. Thanks - will look.

…

On Mon, 15 Mar 2021, 19:52 Jon Swope, ***@***.***> wrote: As the title states, ConfigurationOptions.CheckCertificateRevocation is ignored unless ConfigurationOptions.SslProtocols is specified, and the resulting behavior is that the revocation check is always skipped. This seems like unexpected behavior, especially given that the default value for ConfigurationOptions.CheckCertificateRevocation is true. At the very least it is inconsistent behavior that can cause surprising bugs to occur when the an application changes to specifying the TLS version. The fault lies in this block of code: https://github.com/StackExchange/StackExchange.Redis/blob/40595caf2a08ecf86ea2cfea7ea0d070d07ffb16/src/StackExchange.Redis/ExtensionMethods.cs#L151-L167 In line 166 the overload <https://docs.microsoft.com/en-us/dotnet/api/system.net.security.sslstream.authenticateasclient?view=net-5.0#System_Net_Security_SslStream_AuthenticateAsClient_System_String_> used does not actually check for certificate revocation. Unfortunately the next best overload <https://docs.microsoft.com/en-us/dotnet/api/system.net.security.sslstream.authenticateasclient?view=net-5.0#System_Net_Security_SslStream_AuthenticateAsClient_System_String_System_Security_Cryptography_X509Certificates_X509CertificateCollection_System_Boolean_> does not exist pre 4.7, and the default for SslProtocols changed between 4.6 and 4.7. That makes this not a straightforward change, otherwise this would be a pull request rather than an issue. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1705>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEHMCVQCYLQFBAUTENTEDTDZQPNANCNFSM4ZHG3GTA> .

[jaswope]

A couple more notes. It looks like the PR that added CheckCertificateRevocation set the default to true in order to not change the existing behavior (which worked this way), and it does indeed look like this split in behavior has been around a long time.

I would hazard to suggest that cert revocation checks should be disabled by default in the current version fix (with the option to enable them if requested), and then enabled by default in the next minor/major version with an accompanying release note about the behavioral change. The reasoning is that I presume most users are not specifying a TLS version, and therefore not actually attempting cert revocation checks. Turning this on by default could cause very hard to diagnose and unexpected issues in enterprise environments that limit outgoing requests (as happened to us).

The reason this is hard to diagnose is because it ends up looking like a redis connect timeout if outgoing requests are blocked without closing the connection. I'm not sure how to make that better though, as I don't think there's a way to tell the difference between revocation server requests timing out and the server you are connecting to timing out besides waiting for the internal ssl connection timeouts to occur and then inspecting the certificate validation callbacks.

[NickCraver]

Looking at this as part of the config issues. While I agree this code is incorrect, we've also seen across the ecosystem dramatic time impacts with actually checking revocations - most browsers today don't do it for this reason.

I'm onboard with fixing the code but I'd actually want to pair it with changing the default to false at the same time, not effectively enabling checks for many users. The truth is that revocations are rarely checked and the problems (especially around performance and reliability) in doing said checks hasn't improved much other the years. Even today, I'm fighting this with NuGet as an example.

I propose in 3.0 we fix this overlap and change the default to false on the actual CheckCertificateRevocation option. For any provider that wants to change this, the current PR #1987 adds a path here.