dataplat / dbatools

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

[Copy-DbaCredential] Failed to decrypt credentials #9253

Closed 00-schaudhary closed 1 month ago

00-schaudhary commented 4 months ago

Verified issue does not already exist?

I have searched and found no existing issue

What error did you receive?

Copy-DbaCredential -Source primary\sql1 -Destination secondary\sql1 -Name "Test" -verbose

VERBOSE: [10:20:58][Copy-DbaCredential] We will try to use a dedicated admin connection. VERBOSE: [10:20:58][Connect-DbaInstance] String is passed in, will build server object from instance object and other parameters, do some checks and then return the server object VERBOSE: [10:20:58][Connect-DbaInstance] authentication method is 'local integrated' VERBOSE: [10:20:58][Copy-DbaCredential] Decrypting all Credential logins and passwords on ADMIN:primary\sql1 VERBOSE: [10:20:58][Get-DecryptedObject] Querying service master key WARNING: [10:20:58][Copy-DbaCredential] Failed to decrypt credentials on ADMIN:primary\sql1 | You cannot call a method on a null-valued expression.

Steps to Reproduce

Copy-DbaCredential -Source primary\sql1 -Destination secondary\sql1 -Name "Test" -verbose

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

2.0.4

Other details or mentions

No response

What PowerShell host was used when producing this error

Windows PowerShell ISE (powershell_ise.exe)

PowerShell Host Version

Name Value


PSVersion 5.1.20348.1850
PSEdition Desktop
PSCompatibleVersions {1.0, 2.0, 3.0, 4.0...}
BuildVersion 10.0.20348.1850
CLRVersion 4.0.30319.42000
WSManStackVersion 3.0
PSRemotingProtocolVersion 2.3
SerializationVersion 1.1.0.1

SQL Server Edition and Build number

Microsoft SQL Server 2022 (RTM-CU7) (KB5028743) - 16.0.4065.3 (X64) Jul 25 2023 18:03:43 Copyright (C) 2022 Microsoft Corporation Enterprise Edition: Core-based Licensing (64-bit) on Windows Server 2022 Standard 10.0 (Build 20348: )

.NET Framework Version

PSChildName Version


v2.0.50727 2.0.50727.4927 v3.0 3.0.30729.4926 Windows Communication Foundation 3.0.4506.4926 Windows Presentation Foundation 3.0.6920.4902 v3.5 3.5.30729.4926 Client 4.8.04161
Full 4.8.04161
Client 4.0.0.0

00-schaudhary commented 4 months ago

Updated dbtools to 2.1.8. Issue still exists.

andreasjordan commented 3 months ago

The handling of DAC is still not good. In Connect-DbaInstance we bypass a lot of steps and we currently don't ensure an open connection. So based on my local testing, the problem is most like a connection problem.

You can test the query that fails here with the following code:

$dac = Connect-DbaInstance -SqlInstance primary\sql1 -DedicatedAdminConnection
$smkBytes = $dac.Query("SELECT substring(crypt_property,9,len(crypt_property)-8) as smk FROM sys.key_encryptions WHERE key_id=102 and thumbprint=0x0300000001").smk

Please let me know if this code works or not. Then we will work from there.

00-schaudhary commented 3 months ago

Andreas,

I get the below error when I run the commands you have provided.

Exception calling "Query" with "1" argument(s): "You cannot call a method on a null-valued expression." At line:4 char:1

00-schaudhary commented 3 months ago

Also received the error messages shown in the attachment. This is related to the issue in #9204 SQL-Server-dbatools-DAC-Error-Message

andreasjordan commented 3 months ago

So the problem is, that the DAC is not correctly opened.

Can you only run the frist command to open the DAC and see if that triggers one of the error messages in the eventlog?

00-schaudhary commented 3 months ago

If I issue just the first command, I do not get an error in the powershell windows and do not get an error in the SQL log.

No error in the SQL log even if I use the hostname instead of IP. As noted in the other issue, if I use the IP, I do not get an error in the SQL log.

00-schaudhary commented 3 months ago

@andreasjordan Any luck with this?

andreasjordan commented 3 months ago

The main problem is, that using Connect-DbaInstance -DedicatedAdminConnection does not open the connection but only created the SMO object. This way, problems with opening the connection are shown way to late in the code path, resulting in the error message you see.

I don't know way we handled DAC so differently, but I think we have to change that. If we call Connect-DbaInstance inside of our code, we expect an open connection or an exception. and that is not true for DAC. So I will open a PR today to change that.

The second problem is, that Copy-DbaCredential has a fallback to a "normal" connection if DAC fails. Today, this is not in effect, as the DAC is not tested and thus can not fail at that point. After my change to Connect-DbaInstance this will be in effect, but is useless as the command needs the DAC to decrypt the password. I will open a second PR today to let the command fail if the DAC could not be opened.

00-schaudhary commented 3 months ago

Thanks @andreasjordan .

I see 9305 and 9306 are at "All checks have passed" and "Merging is blocked". Is there something I need to do? Not familiar with how this process works.

andreasjordan commented 3 months ago

@potatoqualitee will have a look at the pull requests if she has some spare time. So there is nothing we can do at the moment.

00-schaudhary commented 3 months ago

Sounds good. Thanks.

00-schaudhary commented 2 months ago

@andreasjordan I see potatoqualitee approved the changes for development. I'm assuming it'll be part of the next update? Is there a way for me to find out what that version will be and be notified when it is released?

andreasjordan commented 2 months ago

You can have a look at the release notes. The changes are included in the current release.

00-schaudhary commented 2 months ago

Thanks. I wasn't able to find the release notes but since you said the changes are included in the current release, I update to 2.1.14 using the commands you have provided in another note.

Update-Module -Name dbatools Get-Module -Name dbatools -ListAvailable | Select-Object -Skip 1 | ForEach-Object -Process { Uninstall-Module -Name $.Name -RequiredVersion $.Version }

Opened a new PS window and ran the below command.

$dac = Connect-DbaInstance -SqlInstance -DedicatedAdminConnection $smkBytes = $dac.Query("SELECT substring(crypt_property,9,len(crypt_property)-8) as smk FROM sys.key_encryptions WHERE key_id=102 and thumbprint=0x0300000001").smk

It executes fine the first time. The second time it gives the below error.

Error connecting to []: An existing connection was forcibly closed by the remote host At line:97921 char:9

andreasjordan commented 2 months ago

The error is thrown because the first dac is still open and there canm be only one dac at a time. To show the length of the returned data and to disconnect the dac, add these two lines at the end:

$smkBytes.Length
$dac.ConnectionContext.Disconnect()

This way you should be able to run this over and over again without problems.

But this is only the first step. I will try to work with you on the original issue in the next days.

andreasjordan commented 2 months ago

Just found out that we need another code change. Will keep you updated...

00-schaudhary commented 2 months ago

The error is thrown because the first dac is still open and there canm be only one dac at a time. To show the length of the returned data and to disconnect the dac, add these two lines at the end:

$smkBytes.Length
$dac.ConnectionContext.Disconnect()

This way you should be able to run this over and over again without problems.

But this is only the first step. I will try to work with you on the original issue in the next days.

After adding the 2 extra commands, it works fine.

Understood. I thought this was the fix for the main issue. Ok, let me know if you need anything from my side.

00-schaudhary commented 2 months ago

Just found out that we need another code change. Will keep you updated...

Ok. Thanks.

00-schaudhary commented 1 month ago

Hi - Checking in to see if you were able to make any progress into this?

andreasjordan commented 1 month ago

I am very sorry, but I don't remember what part of the code needs another change. So we have to start from the beginning. Please run your code with the latest version of dbatools and the -Verbose parameter and tell me where you get an error. Then we go from there.

00-schaudhary commented 1 month ago

Sure, please see below. When I ran it the first time, it seemed to go through fine. Second time, it gave me an error.

Copy-DbaCredential -Source primary\sql1 -Destination secondary\sql1 -Name "DB NAME" -verbose

PS C:\Windows\system32> Copy-DbaCredential -Source primary\sql1 -Destination secondary\sql1 -Name "DB NAME" -verbose VERBOSE: [09:34:29][Copy-DbaCredential] We will try to open a dedicated admin connection. VERBOSE: [09:34:29][Connect-DbaInstance] String is passed in, will build server object from instance object and other parameters, do some checks and then return the server object VERBOSE: [09:34:29][Connect-DbaInstance] authentication method is 'local integrated' VERBOSE: [09:34:30][Copy-DbaCredential] Decrypting all Credential logins and passwords on ADMIN:primary\sql1 VERBOSE: [09:34:30][Get-DecryptedObject] Querying service master key VERBOSE: [09:34:30][Resolve-DbaNetworkName] Resolving PRIMARY using .NET.Dns GetHostEntry VERBOSE: [09:34:30][Resolve-DbaNetworkName] Resolving using .NET.Dns GetHostByAddress VERBOSE: [09:34:30][Get-DbaCmObject] Configuration loaded | Cache disabled: False VERBOSE: [09:34:30][Get-DbaCmObject] [primary] Retrieving Management Information VERBOSE: [09:34:30][Get-DbaCmObject] [primary] Accessing computer using Cim over DCOM VERBOSE: [09:34:30][Get-DbaCmObject] [primary] Accessing computer using Cim over DCOM - Success VERBOSE: [09:34:30][Resolve-DbaNetworkName] Resolving primary. using .NET.Dns GetHostEntry VERBOSE: [09:34:30][Resolve-DbaComputerName] Resolved 'primary' to 'PRIMARY' VERBOSE: [09:34:30][Get-DecryptedObject] Decrypt the service master key VERBOSE: [09:34:30][Get-DecryptedObject] Choose the encryption algorithm based on the SMK length - 3DES for 2008, AES for 2012 VERBOSE: [09:34:30][Get-DecryptedObject] Query password information from the Db. VERBOSE: [09:34:30][Get-DecryptedObject] We already have a dac, so we use it. VERBOSE: [09:34:30][Get-DecryptedObject] Go through each row in results VERBOSE: [09:34:30][Copy-DbaCredential] Getting all Credentials that should be processed on ADMIN:primary\sql1 VERBOSE: [09:34:30][Connect-DbaInstance] Server object passed in, will do some checks and then return the original object VERBOSE: [09:34:30][Connect-DbaInstance] String is passed in, will build server object from instance object and other parameters, do some checks and then return the server object VERBOSE: [09:34:30][Connect-DbaInstance] authentication method is 'local integrated' VERBOSE: [09:34:30][Copy-DbaCredential] Starting migration VERBOSE: [09:34:30][Disconnect-DbaInstance] removing from connection hash

PS C:\Windows\system32> Copy-DbaCredential -Source primary\sql1 -Destination secondary\sql1 -Name "DB NAME" -verbose VERBOSE: [09:35:59][Copy-DbaCredential] We will try to open a dedicated admin connection. VERBOSE: [09:35:59][Connect-DbaInstance] String is passed in, will build server object from instance object and other parameters, do some checks and then return the server object VERBOSE: [09:35:59][Connect-DbaInstance] authentication method is 'local integrated' WARNING: [09:35:59][Copy-DbaCredential] Failure | An existing connection was forcibly closed by the remote host

andreasjordan commented 1 month ago

OK, I can reproduce the issue now and want to document my findings:

I only use Copy-DbaCredential in a brand new powershell session. While the command is running, the DAC is opended, and then closed at the end. Some seconds later, the DAC is reopened - probably from a background session of dbatools, probably its the cache that should not be used for DAC.

andreasjordan commented 1 month ago

If we use Connect-DbaInstance with -DedicatedAdminConnection we prevent the Cache from beeing filled. But we also return the server SMO which is used later in the command again with Connect-DbaInstance. This time, the cache code is run and because the connection string includes the ADMIN: the cache opens a new DAC.

I will change Connect-DbaInstance so that the cache is only filled for new connections that are not DACs.

@potatoqualitee - I think this is the root cause for most of the DAC problems we had reported. I will remove some of the code that you introduced to fix these problems because I think it is not needed anymore. But we can add it back in if I'm wrong.

I will do more testing locally, so give me some more days untill I open the pull request for that.