Add "Encrypt=false" to SQL Server connectionString sample

[hishamco]

Many of us are facing an issue when they supply SQL Server connectionString, more specifically after the SQL Server uses the secure connection. I knew it's documented but it's good to show it in the setup screen instead of spending minutes or hour to figure out why the connection valid

Fixes #15201

[MikeAlhayek]

I don't think this is a good idea as it may encourage bad practice for production. The same string should not include this parameter.

Maybe we should find a way to show a warning on the screen if the connection failed due to the security issue instead

[hishamco]

I don't think this is a good idea as it may encourage bad practice for production. The same string should not include this parameter.

Why it's bad practice, it's just a sample nothing more, but anyone can immediately recognize that he SHOULD pass the Encrypt parameter

Maybe we should find a way to show a warning on the screen if the connection failed due to the security issue instead

If this parameter not supplied the validator will show the message, there's already a bug that the author spent hours to figure out that he should add the new parameter

[MikeAlhayek]

I think it’s a bad idea because some users may start copying/pasting the sample without paying much attention. A warning if the connection fails due to security sounds like a better idea to me.

[hishamco]

I understand what you mean, believe me, or not I spent several minutes to figure out this, don't forget when you submit the page a validator will show a message and probably you want to show another warning and enter both the connection string and password again which is time-consuming

Again what we provide is just a sample nothing more, so let us hear the folks feedback and I'm open to any suggestion :)

[sebastienros]

What about changing the hint text to explain this might be necessary to set this attribute?

[hishamco]

We could, but by adding it to the hint we required the user to copy the property and set it within the connection string with the specified value. It would be nice if it's one step, change the Encrypt like User Id and Password

[Skrypt]

@hishamco Don't add the Encrypt=false to the connection string. Add something that explains about the Encrypt param in the hint .cshtml file.

;Encrypt=true or false parameter may be required by your server to be added. We strongly recommend using Encrypt=true on production servers.

[sebastienros]

Just a reminder about why issues happen with Microsoft.Data.SqlClient:

Starting in v4.0, default encryption settings were made more secure, requiring opt-in to non-encrypted connections. Encrypt defaults to true and the driver will always validate the server certificate based on TrustServerCertificate. (Previously, server certificates would only be validated if Encrypt was also true.)

If you need to turn off encryption, you must specify Encrypt=false. If you use encryption with a self-signed certificate on the server, you must specify TrustServerCertificate=true.

In v5.0, SqlConnectionStringBuilder.Encrypt is no longer a bool. It's a SqlConnectionEncryptOption with multiple values to support Strict encryption mode (TDS 8.0). It uses implicit conversion operators to remain code-backwards compatible, but it was a binary breaking change, requiring a recompile of applications.

So by default encryption is enabled, and the driver will always validate the server certificate. Locally you won't have a valid certificate, to either set TrustServerCertificate=true if you still want data encryption, or Encrypt=false to disable encryption altogether.

This text in the doc might be sufficient, and a link to the doc in the hint. Also a better exception message when the connection is created and fails from the expected exception (I thought we had done that already).

[Skrypt]

@sebastienros can you share the link to that documentation. I tried finding it without success earlier.

[MikeAlhayek]

@Skrypt I think @sebastienros was referring to https://docs.orchardcore.net/en/latest/docs/releases/1.5.0/#yessql-breaking-changes

Also, if you look at the logs you'll get explanation. But I think in this case, we should catch this special exception in the DatabaseValidation class and when this issue occur, we should print out a hit using notification so the use know what to do.

[Piedone]

I'm with the others above; don't encourage lack of security in a sample without further explanation.

[sebastienros]

I think this is the official doc for the topic. https://learn.microsoft.com/en-us/sql/connect/ado-net/encryption-and-certificate-validation?view=sql-server-ver16

I found the blurb I pasted in the release changelog of the library.

[hishamco]

@MikeAlhayek should I close this after your PR #15210?

[MikeAlhayek]

yes. I am closing it.