Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade Microsoft.Data.SqlClient dependency to version 4.1 or 5.0 #85

Closed
shueybubbles opened this issue Mar 21, 2022 · 8 comments
Closed
Assignees

Comments

@shueybubbles
Copy link
Collaborator

dotnet/SqlClient#1210 changed the default connection string value of Encrypt to be true instead of false.

Therefore, any SMO-based application that requires Encrypt=false but isn't currently setting it explicitly in the connection string could break if moved to M.D.S 4.1 and doesn't update the connection string appropriately.

@shueybubbles shueybubbles changed the title Upgrade Microsoft.Data.SqlClient dependency to version 4.1 Upgrade Microsoft.Data.SqlClient dependency to version 4.1 or 5.0 Mar 21, 2022
@ErikEJ
Copy link

ErikEJ commented Aug 10, 2022

For TDS 8 support with SQL 2022, 5.0 is required

@shueybubbles
Copy link
Collaborator Author

@ErikEJ @Matteo-T To preserve backward compatibility with existing SMO apps, I propose we add a new boolean property in ConnectionSettings called StrictEncryption.

If StrictEncryption is true it will use Strict for the connection string regardless of the value of EncryptConnection. Otherwise it will use the current code with the boolean conversion provided by SqlConnectionStringBuilder.

Is that reasonable?

Note that the current code defaults to a false value for Encrypt so anyone using these SMO constructors instead of a SqlConnection instance to create their ServerConnection should continue to observe the same behavior as before and not need to set TrustServerCertificate explicitly.

@shueybubbles shueybubbles self-assigned this Aug 15, 2022
@ErikEJ
Copy link

ErikEJ commented Aug 15, 2022

Would it be more future proof to use an enum - or YAGNI ??

@shueybubbles
Copy link
Collaborator Author

An enum could work but it's not particularly future proof, in that if SqlClient adds some new variation SMO would still have to add a case statement mapping for it. The most future proof model would be to use a string and use reflection to find the name of the corresponding static SqlConnectionEncryptOption instance having that name.

I have to keep these ConnectionSettings properties decoupled from Microsoft.Data.SqlClient types because we still provide SMO builds to Sql Server that use System.Data.SqlClient connections.

@shueybubbles
Copy link
Collaborator Author

Open questions for the community:

  1. Should we keep building for netcoreapp3.1 and NetStandard 2.0?
  2. Should we add a Net5.0 build?
  3. Should we add a Net6.0 build?
  4. Should we break up the SMO 17 nuget packages into a Core package and separate packages for Assessment and Notebook? (see How should the SMO nuget packages be structured? #46)

@shueybubbles
Copy link
Collaborator Author

OOF. Fun fact- SMO has so many layers of ConnectionXXX settings objects with their own suites of duplicate properties that certain key SqlConnectionStringBuilder properties are not being propagated from one connection to another during a copy. The change of default to Encrypt=true surfaced the fact that TrustServerCertificate is not copied from one connection to another. Do SMO apps only run in environments with Encrypt=false or does everyone actually deploy valid trusted certs to their Sql Server instances?

@shueybubbles
Copy link
Collaborator Author

17 has upgraded to MDS 5

@potatoqualitee
Copy link

I mostly see unencrypted until I have to encrypt them. I will ask on twitter. I imagine this will be a huge pain for people but i personally think its worth it. https was a pain until tools were built to make it way easy and even free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants