dataplat / dbatools

🚀 SQL Server automation and instance migrations have never been safer, faster or freer
https://dbatools.io
MIT License
2.44k stars 796 forks source link

Connect-DbaInstance - ignores -Encrypt False #9113

Closed ksl28 closed 11 months ago

ksl28 commented 1 year ago

Verified issue does not already exist?

I have searched and found no existing issue

What error did you receive?

The Connect-DbaInstance seems to ignore the -Encrypt parameter, if the value is False. It seems there is missing an else statement on line 788.

https://github.com/dataplat/dbatools/blob/c253ff5e44dfb6193c5cc63e3e898143371d9fa3/public/Connect-DbaInstance.ps1#L788C1-L791C22

Steps to Reproduce

$a = Connect-DbaInstance -SqlInstance "sqlinstance" -EncryptConnection false
$a.ConnectionContext.ConnectionString

The ConnectionString will always be Encrypt=True

Please confirm that you are running the most recent version of dbatools

2.1.1

Other details or mentions

No response

What PowerShell host was used when producing this error

PowerShell Core (pwsh.exe)

PowerShell Host Version

Name Value


PSVersion 7.3.7 PSEdition Core GitCommitId 7.3.7 OS Microsoft Windows 10.0.22621 Platform Win32NT PSCompatibleVersions {1.0, 2.0, 3.0, 4.0…} PSRemotingProtocolVersion 2.3 SerializationVersion 1.1.0.1 WSManStackVersion 3.0

SQL Server Edition and Build number

Microsoft SQL Server 2022 (RTM) - 16.0.1000.6 (X64) Oct 8 2022 05:58:25 Copyright (C) 2022 Microsoft Corporation Developer Edition (64-bit) on Windows Server 2022 Standard 10.0 (Build 20348: ) (Hypervisor)

.NET Framework Version

.NET 7.0.11

andreasjordan commented 1 year ago

Will provide a fix in a few hours.

andreasjordan commented 1 year ago

This is not so easy, @potatoqualitee please also have a look at this.

The parameter EncryptConnection is currently of type [string], gets it default from Get-DbatoolsConfigValue -FullName 'sql.connection.encrypt' with a default of "Mandatory".

The parameter is used to set the property "EncryptConnection" of [Microsoft.SqlServer.Management.Common.SqlConnectionInfo]. Problem is: This property is of type [bool], thus can only be $true or $false, but never "Mandatory".

I would suggest we introduce a braking change and get this correct in any place:

This change is only breaking for people that used (or tried to use) the parameter. For those who only changed the config, this change will not be breaking, but only fix the issue.

Currently, no unencrypted connection can be made - so we should fix this.

wsmelton commented 1 year ago

The Mandatory is a valid value for the SQLClient, thats why it is used.

https://learn.microsoft.com/en-us/dotnet/api/microsoft.data.sqlclient.sqlconnectionencryptoption?view=sqlclient-dotnet-standard-5.1

https://github.com/dataplat/dbatools/blob/73ff74e4efe569025bd4c544837ff71b790c8ed7/private/configurations/settings/sql.ps1#L19C1-L21

andreasjordan commented 1 year ago

But we don't use Microsoft.Data.SqlClient and so we have no place to put the "Mandatory" inside of Connect-DbaInstance.

wsmelton commented 1 year ago

The Connect-DbaInstance does indeed reference the SqlClient.

https://github.com/dataplat/dbatools/blob/73ff74e4efe569025bd4c544837ff71b790c8ed7/public/Connect-DbaInstance.ps1#L653

If we are saying the consumption of the Encryption parameter is not using it, then it won't be a breaking change and is simple bug to address in a patch release.

andreasjordan commented 1 year ago

This is not the code path we are talking about here.

wsmelton commented 1 year ago

Fully aware, so if the code path you are referencing DOES NOT touch the SQL Client then there is NO breaking change to address. Just fix the issue as a normal patch release.

andreasjordan commented 1 year ago

I want to change the type of the parameter from "string" to "bool" - that is the breaking part...

wsmelton commented 1 year ago

Why do you need to change the type? Just ignore the mandatory value and convert to bool when setting the property.

wsmelton commented 1 year ago

Mandatory needs to be treated however it is treated in current logic (i.e., Mandatory=true) Otherwise [bool]$Encryption should work (don't have laptop).

The only thing that would warrant breaking change is if we are changing the expected behavior that currently exists.

andreasjordan commented 1 year ago

[bool]'False' is $true. So we need some if-else-logic.

"SqlConnectionEncryptOption" can be $null or one of the following: "Mandatory", "Optional", "Strict".

Quote from the docs you referenced: "When converting to a boolean, Mandatory, Strict , and null convert to true and Optional converts false."

Quote from the docs of the command: "If this switch is enabled, SQL Server uses SSL encryption for all data sent between the client and server if the server has a certificate installed. For more information, see Connection String Syntax."

I could provide a quick fix to allow "Optional" and/or "False" to be treated as $false - but I would rather fix all of the issues. As we try to be simple and as in the end we only need a boolean, we should change this to a switch paramater. If someone wants to use the values from SqlConnectionEncryptOption, they can build a connection string and use this as SqlInstance and it will be used.

ksl28 commented 11 months ago

Hi :) I feel like i might have poked the hornest nest here - sorry for that.

From a consumer point of view, that should be changed to a [bool] parameter - string dosent make sense here.

ksl28 commented 11 months ago

@potatoqualitee - Thanks for the update, but i believe a new bug was introduced - or i am using it the wrong way.

In my head this should generate an error, stating that the connection could not be made, because the instance requires an encrypted connection.

image

Is that a bug, or have i misunderstood something?

Thanks! :)

andreasjordan commented 11 months ago

Will test that...

andreasjordan commented 11 months ago

@ksl28 - what version of SQL Server do you use?

SQL Server 2022 has new setting: image

If I use "Force Strict Encryption", then a connection via dbatools is only possible unsing the connection string syntac likeConnect-DbaInstance -SqlInstance "Data Source=SQL01;Integrated Security=True;Encrypt=Strict".

If I use SQL Server 2019, "Force Encrytion" does not work. I can open both encrypted and unencrypted connection: image

But I think that is not a bug in dbatools.

Will have a deeper look into that in the next days...

ksl28 commented 11 months ago

Hi @andreasjordan

I am using SQL Server 2022 Developer edition, and my settings looks like the one on your screenshot.

Just some background info for my use case - i basically want to test if a SQL Instance allows me to connect unencrypted or not. But for half of the SQL servers i do not have access to the Windows OS below - so i cant use Get-DbaForceNetworkEncryption

I will try with a SQL server 2019 tomorrow, but need to get a certificate issued first.

Thanks for the help so far! :)

andreasjordan commented 11 months ago

This is basiccaly the code we use inside on Connect-DbaInstance:

$serverName = 'sql01'
$EncryptConnection = $true
$sqlConnectionInfo = New-Object -TypeName Microsoft.SqlServer.Management.Common.SqlConnectionInfo -ArgumentList $serverName
$sqlConnectionInfo.EncryptConnection = $EncryptConnection
#$sqlConnectionInfo.StrictEncryption = $EncryptConnection
$serverConnection = New-Object -TypeName Microsoft.SqlServer.Management.Common.ServerConnection -ArgumentList $sqlConnectionInfo
$server = New-Object -TypeName Microsoft.SqlServer.Management.Smo.Server -ArgumentList $serverConnection
$null = $server.ConnectionContext.ExecuteWithResults("SELECT 'dbatools is opening a new connection'")

Using my version 2022 with strict encryption, this failes. But if I include the commented line $sqlConnectionInfo.StrictEncryption = $EncryptConnection the connection is successful.

But with my version 2019, I must not use this option.

I will do futher investigations to find a solution that works best with dbatools...