diff --git a/public/Connect-DbaInstance.ps1 b/public/Connect-DbaInstance.ps1 index bf14528bc45..adcff010838 100644 --- a/public/Connect-DbaInstance.ps1 +++ b/public/Connect-DbaInstance.ps1 @@ -752,35 +752,134 @@ function Connect-DbaInstance { } elseif ($copyContext) { $isNewConnection = $true $connContext = $inputObject.ConnectionContext.Copy() + # A ServerConnection that was built from a SqlConnection has its connection string set + # explicitly, and SMO then refuses to let some of its properties be assigned: + # Property cannot be changed or read after a connection string has been set. + # DatabaseName, NonPooledConnection and ServerInstance are all in that group, which is + # why -Database, -NonPooledConnection and -DedicatedAdminConnection used to fail on a + # server that was created from a SqlConnection. Those settings are collected here and + # put into the connection string further down instead, where the same three are + # Initial Catalog, Pooling and Data Source. No readable property tells the two kinds of + # context apart - every one of them reads fine on both - so we find out by trying. + $connectionStringKeyword = @{ } + $databaseNeedsSwitch = $false if ($ApplicationIntent) { $connContext.ApplicationIntent = $ApplicationIntent } if ($NonPooledConnection) { - $connContext.NonPooledConnection = $true + try { + $connContext.NonPooledConnection = $true + } catch { + $connectionStringKeyword["Pooling"] = $false + } } if (Test-Bound -Parameter StatementTimeout) { $connContext.StatementTimeout = $StatementTimeout } - if ($DedicatedAdminConnection -and $inputObject.ConnectionContext.ServerInstance -notmatch '^ADMIN:') { + if ($DedicatedAdminConnection -and $inputObject.ConnectionContext.ServerInstance -notmatch "^ADMIN:") { if ($instance.IsLocalHost) { # Use localhost to avoid multiple IP resolution on multi-homed servers (issue #10151) - if ($instance.InstanceName -ne 'MSSQLSERVER') { - $connContext.ServerInstance = "ADMIN:localhost\$($instance.InstanceName)" + if ($instance.InstanceName -ne "MSSQLSERVER") { + $dedicatedAdminServerInstance = "ADMIN:localhost\$($instance.InstanceName)" } else { - $connContext.ServerInstance = "ADMIN:localhost" + $dedicatedAdminServerInstance = "ADMIN:localhost" } # Trust the server certificate because 'localhost' may not match the certificate CN (e.g., FQDN), issue #10254 $connContext.TrustServerCertificate = $true } else { - $connContext.ServerInstance = 'ADMIN:' + $connContext.ServerInstance + $dedicatedAdminServerInstance = "ADMIN:$($connContext.ServerInstance)" + } + try { + $connContext.ServerInstance = $dedicatedAdminServerInstance + } catch { + $connectionStringKeyword["Data Source"] = $dedicatedAdminServerInstance + if ($instance.IsLocalHost) { + # TrustServerCertificate was assigned above, and on a context with a fixed + # connection string that assignment does not reach the string - the property + # reads back as True while the string still says False. Since the string is + # what the new connection is built from, the same has to be put in there, or + # the localhost DAC fails on a certificate that does not match. See #10254. + $connectionStringKeyword["Trust Server Certificate"] = $true + } + } + try { + $connContext.NonPooledConnection = $true + } catch { + $connectionStringKeyword["Pooling"] = $false } - $connContext.NonPooledConnection = $true } if ($Database) { - # Save StatementTimeout because it might be reset on GetDatabaseConnection - $savedStatementTimeout = $connContext.StatementTimeout - $connContext = $connContext.GetDatabaseConnection($Database, $false) - $connContext.StatementTimeout = $savedStatementTimeout + # We set DatabaseName instead of calling GetDatabaseConnection on purpose. + # GetDatabaseConnection opens the connection on the copy we are holding and returns + # a *different* ConnectionContext, so the server we build below never owns that + # connection. Disconnect-DbaInstance can only reach the context of the server it is + # given, so nothing ever closed it: the session stayed open for the life of the + # process, holding a shared lock on the database. On model that is enough to make a + # later CREATE DATABASE on the same instance fail on the exclusive lock. + # Setting DatabaseName keeps the connection with the context we hand to the server, + # which is also what the connection string code paths of this command already do. + # It does not reset StatementTimeout either, so the save and restore that + # GetDatabaseConnection needed is gone with it. + try { + $connContext.DatabaseName = $Database + } catch { + # Falling back to GetDatabaseConnection here would bring back the very leak this + # change is about. The database is switched on the connection of the copy further + # down instead, which needs no new connection and therefore cannot lose anything. + $databaseNeedsSwitch = $true + } + } + if ($connectionStringKeyword.Count -gt 0) { + # Rebuilding the connection string means opening a new connection from it, and that is + # only safe when the string still carries what it takes to log in. Once a SqlConnection + # has been opened with Persist Security Info=False, SqlClient hides the password, so + # the string we can read back no longer has one and the new connection would fail with + # "Login failed". Rejecting that is better than handing back a server that cannot be + # used - and the caller can always pass the instance name and a SqlCredential instead. + $connectionStringBuilder = New-Object -TypeName Microsoft.Data.SqlClient.SqlConnectionStringBuilder -ArgumentList $connContext.ConnectionString + $usesSqlLogin = -not $connectionStringBuilder["Integrated Security"] -and $connectionStringBuilder["User ID"] + if ($usesSqlLogin -and -not $connectionStringBuilder["Password"] -and -not $connContext.SqlConnectionObject.Credential) { + Stop-Function -Message "Cannot apply the requested settings to [$instance]: they need a new connection, and the password of the SQL Server login is no longer readable from the connection that was passed in. Connect with the instance name and -SqlCredential instead, or pass a SqlConnection that uses Persist Security Info=True." -Target $instance -Continue + } + + if ($databaseNeedsSwitch) { + # A new connection is opened anyway, so the database belongs in the string. As + # Initial Catalog it is part of the pool key, which is what setting DatabaseName + # achieves as well, so the reason #9505 forced a non pooled connection still holds. + $connectionStringKeyword["Initial Catalog"] = $Database + $databaseNeedsSwitch = $false + } + if ($ApplicationIntent) { + # Assigned as a property above, which does not reach a fixed connection string + # either, so the context would report ReadOnly while the connection routes as it + # always did. + $connectionStringKeyword["ApplicationIntent"] = $ApplicationIntent + } + + $changedKeywords = $connectionStringKeyword.Keys -join ", " + Write-Message -Level Debug -Message "Connection context does not accept property assignments, using the connection string for: $changedKeywords" + # The copy has to be disconnected first: setting the connection string of an open + # connection does not move it. The copy is a session of its own - a different SPID + # than the server that was passed in - so this does not touch the caller. + $connContext.Disconnect() + foreach ($keyword in $connectionStringKeyword.Keys) { + # PowerShell routes property assignments on a connection string builder through its + # dictionary indexer, so the keyword has to be used rather than the property name: + # $connectionStringBuilder.InitialCatalog fails with "Keyword not supported". + $connectionStringBuilder[$keyword] = $connectionStringKeyword[$keyword] + } + $connContext.ConnectionString = $connectionStringBuilder.ConnectionString + } + if ($databaseNeedsSwitch) { + # Nothing else needs a new connection, so the database is switched on the one the copy + # already holds. That keeps every part of the authentication that lives on the + # SqlConnection rather than in its string, which rebuilding the string would lose. + # The copy is a session of its own, so this is not the leak of #10555 - the caller + # keeps its own database. + if ($connContext.SqlConnectionObject.State -ne "Open") { + $connContext.Connect() + } + $connContext.SqlConnectionObject.ChangeDatabase($Database) } $server = New-Object -TypeName Microsoft.SqlServer.Management.Smo.Server -ArgumentList $connContext if ($Database -and $server.ConnectionContext.CurrentDatabase -ne $Database) { diff --git a/tests/Connect-DbaInstance.Tests.ps1 b/tests/Connect-DbaInstance.Tests.ps1 index 28c83a7dd92..c8218b95304 100644 --- a/tests/Connect-DbaInstance.Tests.ps1 +++ b/tests/Connect-DbaInstance.Tests.ps1 @@ -7,6 +7,12 @@ param( BeforeDiscovery { $script:hasCredentialSspiProvider = $null -ne ("Dataplat.Dbatools.Connection.NetworkCredentialSspiContextProvider" -as [type]) + # A dedicated admin connection to an instance on the machine running the tests takes a different path + # than one to a remote instance: it goes to ADMIN:localhost and forces TrustServerCertificate, because + # the certificate of the instance does not have to match "localhost" (#10254). That path can only be + # exercised where the instance really is local, which is the case on the CI runners and not in a lab + # of remote instances. The value decides a Skip, so it has to exist at discovery time. + $script:instanceIsLocalHost = ([DbaInstanceParameter]$TestConfig.InstanceMulti1).IsLocalHost } Describe $CommandName -Tag UnitTests { @@ -525,6 +531,130 @@ Describe $CommandName -Tag IntegrationTests { } } + Context "connection is properly cloned from a connection that was created from a SqlConnection" { + BeforeAll { + # A ServerConnection that was built from a SqlConnection has its connection string set, and + # SMO then refuses assignments to DatabaseName, NonPooledConnection and ServerInstance, so + # cloning such a server has to go through the connection string instead. CI never noticed + # because it covers SqlConnection to Server and Server to another Database, but never the + # two of them chained. See #10584. + [Microsoft.Data.SqlClient.SqlConnection]$sqlConnectionToClone = "Data Source=$($TestConfig.InstanceMulti1);Integrated Security=True;Encrypt=False;Trust Server Certificate=True" + $serverFromSqlConnection = Connect-DbaInstance -SqlInstance $sqlConnectionToClone + } + + AfterAll { + $null = $serverFromSqlConnection | Disconnect-DbaInstance + } + + It "clones when using parameter Database" { + $serverClone = Connect-DbaInstance -SqlInstance $serverFromSqlConnection -Database tempdb + $serverClone.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "tempdb" + $serverFromSqlConnection.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "master" + $null = $serverClone | Disconnect-DbaInstance + } + + It "clones when using parameter Database together with NonPooledConnection" { + $serverClone = Connect-DbaInstance -SqlInstance $serverFromSqlConnection -Database tempdb -NonPooledConnection + $serverClone.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "tempdb" + $serverClone.ConnectionContext.ConnectionString | Should -Match "Pooling=False" + $null = $serverClone | Disconnect-DbaInstance + } + + It "clones when using parameter DedicatedAdminConnection" { + $serverClone = Connect-DbaInstance -SqlInstance $serverFromSqlConnection -DedicatedAdminConnection + $serverClone.ConnectionContext.ConnectionString | Should -Match "ADMIN:" + # The connection context cannot be asked here, because ServerInstance is one of the properties + # that cannot be assigned on such a context, so ask the session which endpoint it is on. + $dacQuery = "SELECT COUNT(*) FROM sys.dm_exec_sessions AS s JOIN sys.endpoints AS e ON e.endpoint_id = s.endpoint_id WHERE e.is_admin_endpoint = 1 AND s.session_id = @@SPID" + $serverClone.ConnectionContext.ExecuteScalar($dacQuery) | Should -Be 1 + $null = $serverClone | Disconnect-DbaInstance + } + + It "keeps the forced certificate trust of a local dedicated admin connection" -Skip:(-not $script:instanceIsLocalHost) { + # A local DAC goes to ADMIN:localhost and forces TrustServerCertificate, because the + # certificate of the instance does not have to match "localhost" (#10254). On a context whose + # connection string is fixed, assigning that property succeeds and reads back as True while + # the string still says False - and the string is what the new connection is built from. So + # the trust has to be put into the string as well, or the local DAC fails on the certificate. + # Starts from Trust Server Certificate=False on purpose: with True the assertion would pass + # even if the command did nothing at all. + $localDacConnectionString = "Data Source=$($TestConfig.InstanceMulti1);Integrated Security=True;Encrypt=False;Trust Server Certificate=False" + [Microsoft.Data.SqlClient.SqlConnection]$localDacConnection = $localDacConnectionString + $serverForLocalDac = Connect-DbaInstance -SqlInstance $localDacConnection + try { + $serverClone = Connect-DbaInstance -SqlInstance $serverForLocalDac -DedicatedAdminConnection + + # Read back through a builder rather than matching the string: a connection string builder + # keeps the spelling it was given, so the same setting reads as "Trust Server Certificate" + # or "TrustServerCertificate" depending on how the caller wrote it. + $cloneStringBuilder = New-Object -TypeName Microsoft.Data.SqlClient.SqlConnectionStringBuilder -ArgumentList $serverClone.ConnectionContext.ConnectionString + $cloneStringBuilder["Trust Server Certificate"] | Should -BeTrue + $cloneStringBuilder["Data Source"] | Should -Match "^ADMIN:localhost" + $null = $serverClone | Disconnect-DbaInstance + } finally { + $null = $serverForLocalDac | Disconnect-DbaInstance + } + } + } + + Context "connection is properly cloned from an open SQL authenticated SqlConnection" { + BeforeAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + # Once a SqlConnection has been opened with Persist Security Info=False, SqlClient hides the + # password: the connection string that can be read back no longer carries one. Anything that + # rebuilds the connection string from it therefore produces a connection that cannot log in, + # so the database has to be switched on the connection that already exists. See #10584. + $sqlAuthLogin = "dbatoolsci_clone_$(Get-Random)" + $sqlAuthPassword = "dbatools.IO_$(Get-Random)" + $splatSqlAuthLogin = @{ + SqlInstance = $TestConfig.InstanceMulti1 + Login = $sqlAuthLogin + Password = (ConvertTo-SecureString -String $sqlAuthPassword -AsPlainText -Force) + Force = $true + } + $null = New-DbaLogin @splatSqlAuthLogin + $null = Set-DbaLogin -SqlInstance $TestConfig.InstanceMulti1 -Login $sqlAuthLogin -AddRole sysadmin + + $sqlAuthConnectionString = "Data Source=$($TestConfig.InstanceMulti1);Initial Catalog=master;User ID=$sqlAuthLogin;Password=$sqlAuthPassword;Persist Security Info=False;Encrypt=False;Trust Server Certificate=True" + $sqlAuthConnection = New-Object -TypeName Microsoft.Data.SqlClient.SqlConnection -ArgumentList $sqlAuthConnectionString + $sqlAuthConnection.Open() + $serverFromSqlAuth = Connect-DbaInstance -SqlInstance $sqlAuthConnection + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + AfterAll { + $PSDefaultParameterValues["*-Dba*:EnableException"] = $true + + $null = $serverFromSqlAuth | Disconnect-DbaInstance + $sqlAuthConnection.Close() + $null = Remove-DbaLogin -SqlInstance $TestConfig.InstanceMulti1 -Login $sqlAuthLogin -Force -ErrorAction SilentlyContinue + + $PSDefaultParameterValues.Remove("*-Dba*:EnableException") + } + + It "hides the password from the connection string, which is what makes this case hard" { + $serverFromSqlAuth.ConnectionContext.ConnectionString | Should -Not -Match "Password=" + } + + It "clones when using parameter Database" { + $serverClone = Connect-DbaInstance -SqlInstance $serverFromSqlAuth -Database tempdb + $serverClone.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "tempdb" + $serverClone.ConnectionContext.ExecuteScalar("select suser_sname()") | Should -Be $sqlAuthLogin + $serverFromSqlAuth.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "master" + $null = $serverClone | Disconnect-DbaInstance + } + + It "refuses instead of returning a server that cannot log in" { + # This one needs a new connection, and the password for it is gone, so the command has to say + # so rather than hand back something that fails on first use. Connect-DbaInstance throws by + # default, which is why this is not a warning. + { Connect-DbaInstance -SqlInstance $serverFromSqlAuth -Database tempdb -NonPooledConnection } | + Should -Throw -ExpectedMessage "*no longer readable*" + } + } + Context "connection is properly cloned from an existing connection" { BeforeAll { $server = Connect-DbaInstance -SqlInstance $TestConfig.InstanceMulti1 @@ -540,12 +670,42 @@ Describe $CommandName -Tag IntegrationTests { $serverClone.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "tempdb" } + It "hands the clone a connection that Disconnect-DbaInstance can close" { + # The clone used to be built with GetDatabaseConnection, which opened the connection on an + # intermediate copy and returned a different context. The clone therefore did not own its + # connection and Disconnect-DbaInstance closed nothing, so every call left a session parked + # in the database holding a shared lock on it. + # Repeated on purpose. With pooling, a sleeping session that stays behind is legitimate - it + # belongs to the pool and the next call reuses it. What must not happen is that their number + # grows with every call, which is what an orphaned connection looks like: nothing can reuse + # it and nothing can close it. Against the old implementation the count went up on almost + # every cycle, so a single cycle is not enough to tell the two apart. + $countPerCycle = @() + foreach ($cycle in 1..5) { + $serverParked = Connect-DbaInstance -SqlInstance $server -Database tempdb + $serverParked.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "tempdb" + $null = $serverParked | Disconnect-DbaInstance + $countPerCycle += @(Get-DbaProcess -SqlInstance $server -Database tempdb | Where-Object Program -match "dbatools").Count + } + + # Every cycle has to land on the same number, not just the first and the last. A sequence like + # 4, 5, 4, 5, 4 starts and ends the same way and still means the pool is churning. + ($countPerCycle | Select-Object -Unique).Count | Should -Be 1 + } + It "clones when using parameter ApplicationIntent" { $serverClone = Connect-DbaInstance -SqlInstance $server -ApplicationIntent ReadOnly $server.ConnectionContext.ApplicationIntent | Should -BeNullOrEmpty $serverClone.ConnectionContext.ApplicationIntent | Should -Be "ReadOnly" } + It "clones when using parameter Database together with NonPooledConnection" { + $serverClone = Connect-DbaInstance -SqlInstance $server -Database tempdb -NonPooledConnection + $serverClone.ConnectionContext.ExecuteScalar("select db_name()") | Should -Be "tempdb" + $serverClone.ConnectionContext.NonPooledConnection | Should -Be $true + $null = $serverClone | Disconnect-DbaInstance + } + It "clones when using parameter NonPooledConnection" { $serverClone = Connect-DbaInstance -SqlInstance $server -NonPooledConnection $server.ConnectionContext.NonPooledConnection | Should -Be $false