Feature | Adding TDS 8 support

[JRahnama]

TDS8 support is in preview mode for SQL server 2022. This PR is to add client-side support.

Preview MS-TDS spec changes:
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-winprotlp/8a9c667b-2825-46a8-8066-a80681233c33

Behavior:
To use TDS 8, users should specify Encrypt=Strict in the connection string. Strict mode disables TrustServerCertificate (always treated as False in Strict mode). HostNameInCertificate has been added to help some Strict mode scenarios. In the future, the plan is also to add a ServerCertificate setting, which points to a certificate file that will be directly compared to the server certificate for validation. This will allow secure use of any certificate (self-signed, for example) on a connection without opening up the connection to MITM attacks by blanket trusting of all certificates (as TrustServerCertificate=true would do).

The currently proposed code also retains API backwards compatibility in SqlConnectionStringBuilder.Encrypt. Encrypt has changed from a bool to a SqlConnectionEncryptOption object, but implicit conversion operators have been added so that it can still be treated like a bool. Comparing SqlConnectionEncryptOption to a bool will return true for SqlConnectionEncryptOption.Mandatory or SqlConnectionEncryptOption.Strict. Converting a bool to SqlConnectionEncryptOption will produce SqlConnectionEncryptOption.Mandatory for true and SqlConnectionEncryptOption.Optional for false.

[Wraith2]

Is ValidateServerCertificate an instance method that we need to create a delegate for each time we create the connection? If it's static or takes a context parameter we could avoid the allocation.

[JRahnama]

@Wraith2 thanks for looking into this, ValidateServerCertificate is not static and is following the pattern mentioned for RemoteCertificateValidationCallback Delegate. What do you suggest to avoid the allocation?

[Wraith2]

If you could check the sender argument in the callback and find if it's the SNISslStream that'll tell you whether it could be used as the context object here. If it is then you don't need the instance and you can use a static callback.
This isn't really that important and it'll only happen once per connection so don't worry about it too much, I'd say it's more important to get everything working first.

[Wraith2]

Does this need to be a per instance collection? Is if going to be modified by the caller? if it isn't going to be modified we could have a single static instance that we reuse. If it has to be instantiated consider using the ctor that specifies the initial size because the default initial size is 5 and if we know this will almost never grow we waste some memory each time.

[JRahnama]

can you check the new changes I made?

[Wraith2]

Nit: naming would be tds8 since it's a local not a public member. I think I'd probably just call it protocol since tds is the value not the description of the variable that holds it.

[David-Engel]

I would define a static list of client-supported protocols somewhere above (private static clientApplicationProtocols s_Protocols = new List<SslApplicationProtocol>() { new SslApplicationProtocol("tds/8.0") }) that you can reference on line 333 and remove this local variable completely.

EDIT: I now see that Wraith already suggested the same thing in SNITcpHandle.cs. We can use the same static reference here and there.

[JRahnama]

can you check the new changes I made?

[Wraith2]

in SNIPhysicalHandle create a new field private static s_clientApplicationProtocols s_Protocols = new List<SslApplicationProtocol>(1) { new SslApplicationProtocol("tds/8.0") }; as David suggested and then in SNINpHandle and SNITcpHandle use it like this:

SslClientAuthenticationOptions sslClientOptions = new()
{
    TargetHost = _serverNameIndication,
    ApplicationProtocols = SNIPhysicalHandle.s_clientApplicationProtocols ,
    EnabledSslProtocols = SupportedProtocols,
    ClientCertificates = null,
};

[Wraith2]

Looks ok in general.
Have you considered using an int protocol version instead of a bool? It seems sensible to think there may be later versions and make switching on the version cleaner.

[David-Engel]

Not done reviewing, but wanted to submit some partial feedback.

As we discussed offline, we should also try to maintain a backwards compatible connection string, where possible. Specifically, with the new encrypt enum and Optional/Mandatory values, when we output a connection string, we should continue to output Encrypt=true or Encrypt=false for Mondatory/Optional respectively. This will preserve some backwards compatibility between versions in scenarios where an application is passing connection strings around. This is more important since Encrypt is such a common/well-used option.

[David-Engel]

I would define a static list of client-supported protocols somewhere above (private static clientApplicationProtocols s_Protocols = new List<SslApplicationProtocol>() { new SslApplicationProtocol("tds/8.0") }) that you can reference on line 333 and remove this local variable completely.

EDIT: I now see that Wraith already suggested the same thing in SNITcpHandle.cs. We can use the same static reference here and there.

[Wraith2]

This is using an async function and then awaiting it synchronously, if @roji sees you doing this you'll make him sad. See if there is a naturally synchronous AuthenticateAsClient method you can use instead and avoid sync-over-async.

[JRahnama]

there is no AuthenticateAsClient that accepts SslClientAuthenticationOption as an argument in netcore 3.1. It started from net5 and after.

[Wraith2]

Netcore 3.1 goes out of support at the end of this year (it's LTS) and net5 is already unsupported. https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core

You can use compiler ifdefs to use the current pattern only on 3.1 and lower and then use the sync overload from net5 onwards. In this case I think it's acceptable to do that because it will avoid more threading problems.

[David-Engel]

Another partial review. In general, where we were previously passing bool encrypt internally and we are now passing SqlConnectionEncryptionOption encrypt plus bool isTDS8, we could simplify the code by just passing bool encrypt plus bool isTDS8 (or more accurately bool tlsFirst). This would make for fewer code changes related to the change of the encrypt parameter type.

Alternatively, just pass SqlConnectionEncryptionOption encrypt and base all the if (isTDS8) logic off of if (encrypt == SqlConnectionEncryptionOption.Strict).

Since I haven't been all the way through the code, feel free to point to to why either of the above wouldn't work.

[DavoudEshtehari]

Generally, I believe there are some opportunities to add more event source logs, especially in the new paths that would be better if we could include them with this change.