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

Add the option to turn on or of encryption on MsSql Server Connection #2007

Merged
merged 3 commits into from
Feb 26, 2022

Conversation

preardon
Copy link
Member

@preardon preardon commented Feb 26, 2022

@iancooper This is what is required to upgrade to Microsoft.Data.Sql > 4.0.0

  • Removes a few unnecessary references
  • Add an option to specify if sql connection encryption should be on or not

#1867

Copy link
Member

@iancooper iancooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks for fixing this

@preardon
Copy link
Member Author

@iancooper this is a weird one for me, Microsoft have changed the default behaviour of this library from 4.0.0 to default encryption to on (previously off), this issue with this of course is that unless you have your servers setup with trusted certificates then this will fail.

The real question here is should we offer this override to connection string that is passed in to turn this on/off. There are two other options here

  1. Do nothing, and advise our users that if they are not connecting to a SqlServer with a trusted connection add "Encrypt=false;" to their connection strings, which feels like a bit of a breaking change to me (however I imagine this will effect the rest of their Sql connections as we have just forced them to upgrade to 4.1.0 (Entity Framework Core only required > 2.1.4)
  2. Completely change the MsSqlConfiguration Class so that you don't pass in connection string, but rather the elements and place the ConnectionStringBuilder in there, however this is a bit of a breaking change as well

I suppose what I've done here feels like the least breaking change, but I don't love the fact that if you are already passing in a Connection String with Encrypt=True and upgrade to this version we will turn it off unless you specify that it should be on

I would appreciate your thoughts :) I think after typing this novella, I'm leaning towards throwing most of the PR away and going with the Do Nothing approach.

@iancooper
Copy link
Member

I'd say that your argument for (1) and document makes sense. If everyone using 4 has to take this step, which will make their connection more secure, then presumably they will be doing this elsewhere as they say.

The XML comments are handy for documenting this, as it shows up in Intellisense

@preardon
Copy link
Member Author

Ok, I'll undo my changes and put something in the comments

@preardon preardon merged commit 17e483b into BrighterCommand:master Feb 26, 2022
@preardon preardon deleted the issue/1867-MsSql-Encryption branch February 26, 2022 22:17
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

Successfully merging this pull request may close these issues.

2 participants