[Bug] Test-DbaWindowsLogin searches by SAMAccountName string instead of ObjectSID

[carpewil]

Report

Host used

• powershell.exe
• ISE
• VS Code
• Other (please specify)

Errors Received

replace THIS text WITH the OUTPUT of -- $ERROR[0] | SELECT *

Steps to Reproduce

Expected Behavior

Actual Behavior

Environmental information

🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨
Please provide the output of the below script

[pscustomobject]@{
'PowerShell Version' = $PSVersionTable.PSVersion.ToString()
'dbatools latest installed' = (Get-InstalledModule -Name dbatools).Version
'Culture of OS' = (Get-Culture)
} | fl -force

SQL Server:

/* REPLACE WITH output of @@VERSION */

/* REPLACE WITH output of @@LANGUAGE */

[carpewil]

No error was received, just false-positive results in a few cases where SAMAccountName had changed in AD

[niphlod]

@carpewil : can you please elaborate a bit on the topic ? Are you having laying around the same samaccountname with different SIDs ?

[carpewil]

@niphlod Obviously you cannot have multiple accounts with the same SAMAccountName at the same time. Let me provide a couple of situations where the cmdlet provided false positives for orphaned SQL logins. The first was with an AD user account. Our naming convention for SAMAccountName uses a portion of the users last name and a portion of the first name. We have a developer who got married a couple of years after starting with company. When her last name changed after her marriage it triggered a process that also changed her SAMAccountName to match. Her account was not deleted and recreated. Therefore the objectSID stayed the same. So let's say her original SAMAccountName was DOMAIN\SMITSUS and that it was changed to DOMAIN\JONESUS after her marriage. Her AD account had been added as a SQL login on a couple of SQL instances using her original SAMAccountName of DOMAIN\SMITSUS. Even after her SAMAccountName changed DOMAIN\JONESUS the SQL logins continued to function because SQL actually stores the objectSID of the account. However, when I ran the cmdlet it searched for the string value of SMITSUS against DOMAIN and did not find a match indicating it was orphaned. If it had searched for the objectSID instead it would have found her account even though the SAMAccountName value had changed.
I also had a scenario with an AD security group that had been setup as a SQL login on multiple instances. Let's call it DOMAIN\DEV_ADMINS. We had decided to deprecate the use of the security group because it's use had not followed best practice. But we could not just get rid of the group overnight due to its extended use in the past. We therefore renamed the group in AD to indicate it was Deprecated (DOMAIN\DEV_ADMINS - Deprecated). When running the cmdlet it searched DOMAIN for the literal value of DEV_ADMINS, which I didn't find and therefore said the SQL login was orphaned even though the group still existed and the SQL login still functioned.
Hope this helps better explain things.

[niphlod]

Yeah, gotcha, perfectly clear, thanks a lot for taking the time to explain your scenario. Il mer 14 lug 2021, 12:17 carpewil ***@***.***> ha scritto:

…

@niphlod <https://github.com/niphlod> Obviously you cannot have multiple accounts with the same SAMAccountName at the same time. Let me provide a couple of situations where the cmdlet provided false positives for orphaned SQL logins. The first was with an AD user account. Our naming convention for SAMAccountName uses a portion of the users last name and a portion of the first name. We have a developer who got married a couple of years after starting with company. When her last name changed after her marriage it triggered a process that also changed her SAMAccountName to match. Her account was not deleted and recreated. Therefore the objectSID stayed the same. So let's say her original SAMAccountName was DOMAIN\SMITSUS and that it was changed to DOMAIN\JONESUS after her marriage. Her AD account had been added as a SQL login on a couple of SQL instances using her original SAMAccountName of DOMAIN\SMITSUS. Even after her SAMAccountName changed DOMAIN\JONESUS the SQL logins continued to function because SQL actually stores the objectSID of the account. However, when I ran the cmdlet it searched for the string value of SMITSUS against DOMAIN and did not find a match indicating it was orphaned. If it had searched for the objectSID instead it would have found her account even though the SAMAccountName value had changed. I also had a scenario with an AD security group that had been setup as a SQL login on multiple instances. Let's call it DOMAIN\DEV_ADMINS. We had decided to deprecate the use of the security group because it's use had not followed best practice. But we could not just get rid of the group overnight due to its extended use in the past. We therefore renamed the group in AD to indicate it was Deprecated (DOMAIN\DEV_ADMINS - Deprecated). When running the cmdlet it searched DOMAIN for the literal value of DEV_ADMINS, which I didn't find and therefore said the SQL login was orphaned even though the group still existed and the SQL login still functioned. Hope this helps better explain things. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7528 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA52B4XWYXNXUCD2HDCDPLTXVP5DANCNFSM5AHC4UIQ> .

[carpewil]

I had found a SQL function that converted the varbinary value that SQL stores for the Windows (AD) SID back to a valid AD objectSID value. I took it and cobbled together the PS function below to convert the value to an AD objectSID which can then be used to for the account lookup instead of the SAMAccountName string value. Im sure there's a more efficient way to write this. But hopefully it helps. Function SQLSID2ADSID { param( [Parameter( Position=0, Mandatory=$true, ValueFromPipeline=$true) ] [Byte[]]$Bin) # Convert the varbinary value to a Hex string $HexStr = -join ($Bin | foreach { "{0:X2}" -f $_ }) # Test to make sure its a valid value, SID value that corresponds to an AD SID and not for a SID for SQL account If ($HexStr.Length % 4 -ne 0) { Return "Error: Invalid Value $HexStr.ToString()" } # Conver the string to an array $HexArr = $HexStr -split '(..)' -ne '' # First part of Windows SID value, 'S-1' [string]$StringID = "S-" + [uint32]$HexArr[0] # Append second part of Windows SID value, 'S-1-5' $val = 0 For ($i=2; $i -le 7; $i++) { $val = $val + [uint32]$HexArr[$i] } $StringID = $StringID + "-" + $val # Now it gets tricky because the remaining parts are stored in reverse order # Prep some variables we'll use to parse through the rest of the hex array [int]$j = 8 [int]$x = $HexArr.Length - 1 $RevArr = @() # There are 5 remaining sections of the Windows SID # Each section of the SID is made of 4 elements of the hex array # We string together the elements into their own array, $RevArr For ($j=1; $j -le 5; $j++) { $HexVal = $null For ($i=1; $i -le 4; $i++) { $HexVal = $HexVal + $HexArr[$x] $x = $x - 1 } $RevArr += [uint32]"0x$HexVal" } # Now process the new array in reverse order and concatenate # the values onto the Windows SID For ($i = $RevArr.Length - 1; $i -ge 0; $i--){ $StringID = $StringID + "-" + $RevArr[$i] } Return $StringID } #Connect to the SQL instance and get a SQL SID value $Conn = New-Object System.Data.SqlClient.SqlConnection $Conn.ConnectionString = "Integrated Security=true;Server=mytestpc;Database=master;" $Command = New-Object System.Data.SqlClient.SqlCommand $Command.CommandText = "SELECT sid FROM sys.server_principals WHERE principal_id = 259" $Command.Connection = $Conn $TableListSA = New-Object System.Data.SqlClient.SqlDataAdapter $Command.CommandText, $Conn.ConnectionString $TableListSA.SelectCommand = $Command $TableListDS = New-Object System.Data.DataSet $TableListSA.Fill($TableListDS) | Out-Null $Row = $TableListDS.Tables[0].Rows[0] #Pass the SQL SID value to the function $ADSID = SQLSID2ADSID $Row["sid"] #Display the results $ADSID

________________________________ From: Simone Bizzotto ***@***.***> Sent: Wednesday, July 14, 2021 5:49 AM To: sqlcollaborative/dbatools ***@***.***> Cc: carpewil ***@***.***>; Mention ***@***.***> Subject: Re: [sqlcollaborative/dbatools] [Bug] Test-DbaWindowsLogin searches by SAMAccountName string instead of ObjectSID (#7528) Yeah, gotcha, perfectly clear, thanks a lot for taking the time to explain your scenario. Il mer 14 lug 2021, 12:17 carpewil ***@***.***> ha scritto:

@niphlod <https://github.com/niphlod> Obviously you cannot have multiple accounts with the same SAMAccountName at the same time. Let me provide a couple of situations where the cmdlet provided false positives for orphaned SQL logins. The first was with an AD user account. Our naming convention for SAMAccountName uses a portion of the users last name and a portion of the first name. We have a developer who got married a couple of years after starting with company. When her last name changed after her marriage it triggered a process that also changed her SAMAccountName to match. Her account was not deleted and recreated. Therefore the objectSID stayed the same. So let's say her original SAMAccountName was DOMAIN\SMITSUS and that it was changed to DOMAIN\JONESUS after her marriage. Her AD account had been added as a SQL login on a couple of SQL instances using her original SAMAccountName of DOMAIN\SMITSUS. Even after her SAMAccountName changed DOMAIN\JONESUS the SQL logins continued to function because SQL actually stores the objectSID of the account. However, when I ran the cmdlet it searched for the string value of SMITSUS against DOMAIN and did not find a match indicating it was orphaned. If it had searched for the objectSID instead it would have found her account even though the SAMAccountName value had changed. I also had a scenario with an AD security group that had been setup as a SQL login on multiple instances. Let's call it DOMAIN\DEV_ADMINS. We had decided to deprecate the use of the security group because it's use had not followed best practice. But we could not just get rid of the group overnight due to its extended use in the past. We therefore renamed the group in AD to indicate it was Deprecated (DOMAIN\DEV_ADMINS - Deprecated). When running the cmdlet it searched DOMAIN for the literal value of DEV_ADMINS, which I didn't find and therefore said the SQL login was orphaned even though the group still existed and the SQL login still functioned. Hope this helps better explain things. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7528 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA52B4XWYXNXUCD2HDCDPLTXVP5DANCNFSM5AHC4UIQ> .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fsqlcollaborative%2Fdbatools%2Fissues%2F7528%23issuecomment-879790686&data=04%7C01%7C%7C1ad05f2c315b4eabeec308d946b50403%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637618565532668582%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=1Jth7gKoW86XDqLnTUWWPqJp31WSzw2YyPSQ2wt8YYg%3D&reserved=0>, or unsubscribe<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAUXPWDMQHW4IU342NML274DTXVTSPANCNFSM5AHC4UIQ&data=04%7C01%7C%7C1ad05f2c315b4eabeec308d946b50403%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637618565532668582%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=aOpmt0x93fzQuw215g8y57e45EMJu%2BUvhYQ%2BiC8n3IE%3D&reserved=0>.

[niphlod]

theoretically this is already done by SMO, but thanks for the input in case it'll be a problem

[niphlod]

hi @carpewil , would you give #7668 a try ?

[carpewil]

Wasn't 100% certain what I needed to do so let me explain the steps I took. Using your link I opened the raw text file for the Test-DbaWindowsLogin function, copied it and pasted into the Test-DbaWindowsLogin.ps1 file on my machine. I then imported the dbatools module and ran the following along with results: PS C:\WINDOWS\system32> Test-DbaWindowsLogin -SqlInstance MYINSTANCE -Login "DOMAIN\MYGROUP" WARNING: [09:05:20][Test-DbaWindowsLogin] AD Searcher Error for MYGROUP on [MYINSTANCE] DisabledInSQLServer : False Domain : DOMAIN Enabled : Found : False LockedOut : Login : MYGROUP PasswordExpired : PasswordNotRequired : Server : MYINSTANCE Type : Group Looks like it's still not finding the renamed group. It looks like the hang up for me maybe on line 159: $u = Get-DbaADObject -ADObject "$domain\$SIDForAD" -Type User -IdentityType Sid -EnableException Couple of things with this. First, I do not have a Get-DbaADObject cmdlet (The term 'get-dbaadobject' is not recognized as the name of a cmdlet...) nor do I see it listed in the Command Index. Second possible issue, I noticed that you have hardcoded the -Type parameter as "User". Since I dont have the cmdlet Im just guessing here, but what happens when Im testing for the existence of an AD security group (which I am)?

…

________________________________ From: Simone Bizzotto ***@***.***> Sent: Monday, August 2, 2021 7:54 AM To: sqlcollaborative/dbatools ***@***.***> Cc: carpewil ***@***.***>; Mention ***@***.***> Subject: Re: [sqlcollaborative/dbatools] [Bug] Test-DbaWindowsLogin searches by SAMAccountName string instead of ObjectSID (#7528) hi @carpewil<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcarpewil&data=04%7C01%7C%7C108429d2ae9348de2e7b08d955b4a23e%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637635056574973852%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=NGNeJBjeGBYyDxOb8Iz26JRBypEedy%2BUCN6sWGVP%2FuM%3D&reserved=0> , would you give #7668<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fsqlcollaborative%2Fdbatools%2Fpull%2F7668&data=04%7C01%7C%7C108429d2ae9348de2e7b08d955b4a23e%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637635056574983804%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=epZsdVowYj5eQ8AAoD8qgOLpnOo6T12DAACncjb%2BIAw%3D&reserved=0> a try ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fsqlcollaborative%2Fdbatools%2Fissues%2F7528%23issuecomment-891000818&data=04%7C01%7C%7C108429d2ae9348de2e7b08d955b4a23e%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637635056574983804%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=wGZMGvRH5upWZjHbRHtOo3FRMM72u0QKqTLSc2tV%2F9c%3D&reserved=0>, or unsubscribe<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAUXPWDL3EKRODYH6LCJKJSTT22IPNANCNFSM5AHC4UIQ&data=04%7C01%7C%7C108429d2ae9348de2e7b08d955b4a23e%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637635056574993784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=EPFSCJoVe%2BvznoN1sy%2BY2OLyjPgCa1gN%2BpyOoMXcQR0%3D&reserved=0>.

[niphlod]

line 253 searches for group, line 159 for users . Get-DbaADObject is an internal function, you can find it under the internals folder.

BTW, replacing the function code works but only if you have a .git folder in the module path. dbatools speeds things up with allcommands.ps1 otherwise. If a .git folder is found where dbatools.psd1 lives, allcommands.ps1 is discarded and the whole source code is loaded. This slows down the import process performance wise, but it's the way to allow testing modified functions.