Update Encrypt property default value to true

[DavoudEshtehari]

• Encrypt default value = true.
• Improving the pre-login process from a client without encryption possibility (TDS Pre-login):
  • Supported values using Native SNI: ENCRYPT_OFF, ENCRYPT_ON, and ENCRYPT_NOT_SUP
  • Supported values using Managed SNI: ENCRYPT_OFF, and ENCRYPT_ON**

Value sent by client	Value returned by server when server is set to ENCRYPT_OFF	Value returned by server when server is set to ENCRYPT_ON	Value returned by server when server is set to ENCRYPT_NOT_SUP
ENCRYPT_OFF	ENCRYPT_OFF	ENCRYPT_REQ	ENCRYPT_NOT_SUP
ENCRYPT_ON	ENCRYPT_ON	ENCRYPT_ON	ENCRYPT_NOT_SUP (connection terminated)
ENCRYPT_NOT_SUP	ENCRYPT_NOT_SUP	ENCRYPT_REQ (connection terminated)	ENCRYPT_NOT_SUP

Complementary explanation

[ErikEJ]

What issue is this solving?

[roji]

Doesn't this require the server to have proper SSL certificates? If so, that seems quite breaking for everyone currently happily not using SSL in internal networks...

[Wraith2]

It depends on the value of TrustServerCertificate see https://docs.microsoft.com/en-us/sql/relational-databases/native-client/features/using-encryption-without-validation?view=sql-server-ver15 by default default is to trust the server so default configuration self signed will still allow access. EnableEncryption just forces the use of ssl and doesn't have implications for client or server certificate validation.

[DavoudEshtehari]

Doesn't this require the server to have proper SSL certificates? If so, that seems quite breaking for everyone currently happily not using SSL in internal networks...

This is all about the client environment configuration and possible security breaches. Also, the customers who enforce the encryption on the server don't face this breach.

[roji]

by default default is to trust the server so default configuration self signed will still allow access

Are you sure about that? Both that table and checking the default value of SqlConnectionStringBuilder.TrustServerCertificate seem to indicate that self-signed certificates aren't trusted by default, and one has to opt in explicitly by setting Trust Server Certificate=true. This makes sense, since by default trusting certificates which cannot be validated wouldn't be very secure.

[DavoudEshtehari]

It depends on the value of TrustServerCertificate see https://docs.microsoft.com/en-us/sql/relational-databases/native-client/features/using-encryption-without-validation?view=sql-server-ver15 by default default is to trust the server so default configuration self signed will still allow access. EnableEncryption just forces the use of ssl and doesn't have implications for client or server certificate validation.

Any connection to the server at least needs a secure login that means both sides of the connection should be possible to encrypt. So, If one side wasn't able to encrypt, establishing a secure connection shouldn't be possible.

Regarding the use of TrustServerCertificate, by attempting to open a connection -before this change- with Encrypt=true and TrustServerCertificate=false you will get an exception which is expected, while if you try it just after disabling all secure protocols on the client it'll connect except using Managed SNI.

[DavoudEshtehari]

What issue is this solving?

Comment's updated.

[Wraith2]

What issue is this solving?

Comment's updated.

Ok. but I think the point was to ask why the change is being made. Is it a top down decision to require encryption at all times?

[DavoudEshtehari]

the point was to ask why the change is being made.

It includes two changes:

1. Replacing the Encrypt default value by true in the connection string.
2. Improving the pre-login process to avoid the probable security breach as it's discussed above.

Is it a top down decision to require encryption at all times?

If you asked for Encrypt default value, the quick answer is yes. But if you asked for the encryption process, I delegate it to @David-Engel.

[David-Engel]

There are two issues being addressed here:

1. Default Encrypt to True. This is for security. Similar to http/https, if the client starts with allowing non-encrypted connections, it will always be susceptible to MITM attacks. Even if the server is configured to require encryption, there can be a MITM altering the server's response to say it doesn't require encryption. The MITM can then proxy the connection. client <-plain text-> MITM <-encrypted-> server = the connection is compromised.

   Security has been encouraging us for years to change the default behavior of client drivers to be secure by default and we have resisted, knowing that it is a breaking change for most users. It's easy enough for devs to add Encrypt = false to all their connection strings, if they need to. We just want to make sure they understand the choice they are making and they are making it deliberately. With cloud computing becoming more and more common, it's not safe to leave the default value of Encrypt equal to false.

2. The less-breaking, but still important, fix here is to ensure connections fail if the client does not have any encryption libraries available and either Encrypt = true or the server requires encryption. SqlClient + native SNI is the only MS driver we've found that will successfully connect in that scenario.

[ErikEJ]

Thanks, makes sense doing this for version 4!

[JRahnama]

LGTM. I wait for other teammate opinion.

[cheenamalhotra]

LGTM, just some minor suggestions.

[Malcolm-Stewart]

Someone mentioned that TrustServerCertificate defaults to true. That is incorrect, unless that was also changed for this driver.
When Encrypt=true, then the certificate is also validated.
This means that changing the default will break many scenarios.
With Encrypt=False, SSL/TLS is still used to encrypt the login packet, but the certificate is not validated.
With Encrypt=true, SSL/TLS is used to encrypt the login packet and all subsequent packets, but the certificate is validated first.
Systems using SQL Server's self-generated certificate will fail after this change and the user will have to buy a certificate or create a self-signed certificate, or change their connection string. If the change was made for scenarios where the user could not modify the connection string to enable encryption, then they are left having to get new certificates.

[Malcolm-Stewart]

Okay, so if the change is only for Microsoft.Data.SqlClient, then that is understandable. If System.Data and ODBC/OLE DB are unchanged, then I do not have any objection to that. I was just worried that legacy apps might get caught up in the change. From: David Engel ***@***.***> Sent: Thursday, August 19, 2021 7:20 PM To: dotnet/SqlClient ***@***.***> Cc: Malcolm Stewart ***@***.***>; Comment ***@***.***> Subject: Re: [dotnet/SqlClient] Update `Encrypt` property default value to true (#1210) Someone mentioned that TrustServerCertificate defaults to true. That is incorrect, unless that was also changed for this driver. I suspect they misspoke. I confuse TrustServerCertificate all the time, thinking of it as "validate server certificate" in my head. When Encrypt=true, then the certificate is also validated. This means that changing the default will break many scenarios. With Encrypt=False, SSL/TLS is still used to encrypt the login packet, but the certificate is not validated. Correct. That will still be the case. With Encrypt=true, SSL/TLS is used to encrypt the login packet and all subsequent packets, but the certificate is validated first. With the default TrustServerCertificate value, correct. Systems using SQL Server's self-generated certificate will fail after this change and the user will have to buy a certificate or create a self-signed certificate, or change their connection string. If the change was made for scenarios where the user could not modify the connection string to enable encryption, then they are left having to get new certificates. Given that applications must be on Microsoft.Data.SqlClient to get this change, that means the application is almost certainly under current development. I don't think there is high risk that someone will be blocked by not being able to change the Encrypt setting for an application that is under current development. If their application is truly stuck, they will likely be on System.Data.SqlClient or an earlier version of MDS forever. Additionally, if this argument continues to keep us from making the change, it will never be made and security will never improve. We do understand it may be disruptive, but security has been pushing for us to do this for years. - You are receiving this because you commented. Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2FSqlClient%2Fpull%2F1210%23issuecomment-902315002&data=04%7C01%7CMalcolm.Stewart%40microsoft.com%7C5763d8d8eb1e4addb78c08d96367ddea%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637650120065312533%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Eb1l18h%2FD9UkwGRydX8JlVPfqcV6oL4ovB1gDJjCDIE%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAPBDW7CXNLXHSSRNKJQKEGLT5WGSBANCNFSM5CCTGNWA&data=04%7C01%7CMalcolm.Stewart%40microsoft.com%7C5763d8d8eb1e4addb78c08d96367ddea%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637650120065312533%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=w41RG5AbH8m3pnkjkLU5JTdjtnbagLjTV%2BwuxQP%2FQh4%3D&reserved=0>. Triage notifications on the go with GitHub Mobile for iOS<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fapps.apple.com%2Fapp%2Fapple-store%2Fid1477376905%3Fct%3Dnotification-email%26mt%3D8%26pt%3D524675&data=04%7C01%7CMalcolm.Stewart%40microsoft.com%7C5763d8d8eb1e4addb78c08d96367ddea%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637650120065322527%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=DOA4596Zr93Rk0LIQpmPUqygzqpld4LNuvAh0JvM5mc%3D&reserved=0> or Android<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26utm_campaign%3Dnotification-email&data=04%7C01%7CMalcolm.Stewart%40microsoft.com%7C5763d8d8eb1e4addb78c08d96367ddea%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637650120065322527%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=fsrF13LR3xh16UCkldxL87%2FwH3zvVlQOwx%2F0NtZMkMo%3D&reserved=0>.

[davidsampson-hv]

This caused downtime for our production Logic Apps talking to SQL as we are using self-signed SSL certs on SQL and hadn't declared trustservercertificates. I am 100% onboard with the change I just wish this had been communicated to users of Logic Apps for example. We have no ability/visibility to track potential changes like this via the underlying Logic Apps stack, we rely on Microsoft to surface and communicate breaking changes in advance, it's clear from this issue that this was known to be a breaking change before it was merged.

I assume as this was merged a year ago but we only experienced the change a couple of weeks ago that Logic Apps have only just updated to this version of dotnet.

[bodzilla]

This caused downtime for our production Logic Apps talking to SQL as we are using self-signed SSL certs on SQL and hadn't declared trustservercertificates. I am 100% onboard with the change I just wish this had been communicated to users of Logic Apps for example. We have no ability/visibility to track potential changes like this via the underlying Logic Apps stack, we rely on Microsoft to surface and communicate breaking changes in advance, it's clear from this issue that this was known to be a breaking change before it was merged.

I assume as this was merged a year ago but we only experienced the change a couple of weeks ago that Logic Apps have only just updated to this version of dotnet.

We have experienced this same type of issue during testing of our own apps which utilise the Serilog MSSQL sink - had it not been for the fact that their changelog included the breaking change link for this lib we would've been in a bit of a pickle, so appreciate both ends declaring this - although it perhaps highlights the need for our end to look deeper into checking dependencies of dependencies before pushing something out for testing.

[xorinzor]

It would've been nice if an example was included in the documentation on how to revert to the original behaviour.

[greengumby]

Using SqlClient V4 I am expecting the connection string to require TrustServerCertificate=true however when using a VM hosted in the Azure Portal connecting to an Azure SQL Database this is not the case. Any ideas why?

[Malcolm-Stewart]

TrustServerCertificate is not required if the server's certificate is implicitly trusted by the client, e.g. it is issued by a certificate authority that has a root certificate already installed in the trusted root store of the client.