Skip to content

Cache risk-rating SID lookups #277

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.

Sign up for GitHub

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

Merged
SamErde merged 6 commits into from
Sep 1, 2026
Merged

Cache risk-rating SID lookups #277

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3e486f9
Add session-scoped SID-to-objectClass cache in Set-RiskRating
SamErde May 6, 2026
a2961c4
fix: avoid caching transient AD lookup errors in Get-SidObjectClass
SamErde May 6, 2026
1e7d0c7
fix: mirror Get-SidObjectClass helper and call-site replacements in I…
SamErde May 6, 2026
d384518
fix: use Write-Error to re-emit AD lookup failures to the error stream
SamErde May 6, 2026
06d99b8
Merge branch 'main' into samerde/cache-risk-sid-lookups
SamErde Sep 1, 2026
ba9c8af
Potential fix for pull request finding
SamErde Sep 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 62 additions & 7 deletions Invoke-Locksmith.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3496,6 +3496,61 @@ function Set-AdditionalTemplateProperty {
}
}

function Get-SidObjectClass {
<#
.SYNOPSIS
Returns the AD objectClass for a given SID, using a session-scoped cache to avoid repeated LDAP queries.

.DESCRIPTION
Comment thread
SamErde marked this conversation as resolved.
Wraps Get-ADObject with a script-scoped hashtable cache so that repeated SID-to-objectClass lookups
within a single Locksmith scan run hit Active Directory only once per unique SID. Common principals
such as Domain Users, Authenticated Users, and Domain Computers appear across many issues and
benefit most from this cache.

.PARAMETER Sid
The SID string to look up.

.OUTPUTS
PSCustomObject with an objectClass property, or $null if the SID is not found in AD.

.NOTES
The cache ($script:SidObjectClassCache) persists for the lifetime of the module session. It is
intentionally not cleared between issues so that common principals are resolved only once per
scan run.
Comment on lines +3504 to +3519

@SamErde SamErde Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This might be worth a follow-up look because the module is the primary use case for Locksmith (assumption about real-world usage).


.EXAMPLE
$objectClassInfo = Get-SidObjectClass -Sid 'S-1-5-21-...-512'
#>
[CmdletBinding()]
[OutputType([PSCustomObject])]
param (
[Parameter(Mandatory)]
[string]$Sid
)

if ($null -eq $script:SidObjectClassCache) {
$script:SidObjectClassCache = @{}
}

if (-not $script:SidObjectClassCache.ContainsKey($Sid)) {
try {
# Use -ErrorAction Stop so transient ADWS/AD errors throw rather than producing
# $null output. Only cache a result (including $null for a genuine "not found")
# on success; leave the key absent on error so the next call can retry.
$result = Get-ADObject -Filter { objectSid -eq $Sid } -ErrorAction Stop |
Select-Object objectClass
$script:SidObjectClassCache[$Sid] = $result
} catch {
# Re-emit to the error stream so callers can detect/handle failures and so
# $ErrorActionPreference is honoured. The key is intentionally left absent
# so a subsequent call can retry the AD query.
Write-Error -ErrorRecord $_
}
}

return $script:SidObjectClassCache[$Sid]
}

function Set-RiskRating {
<#
.SYNOPSIS
Expand Down Expand Up @@ -3565,7 +3620,7 @@ function Set-RiskRating {
if ($Issue.Technique -eq 'ESC7') {
# If an Issue can be tied to a principal, the principal's objectClass impacts the Issue's risk
$SID = $Issue.IdentityReferenceSID.ToString()
$IdentityReferenceObjectClass = Get-ADObject -Filter { objectSid -eq $SID } | Select-Object objectClass
$IdentityReferenceObjectClass = Get-SidObjectClass -Sid $SID

if ($Issue.IdentityReferenceSID -match $UnsafeUsers) {
# Authenticated Users, Domain Users, Domain Computers etc. are very risky
Expand Down Expand Up @@ -3627,7 +3682,7 @@ function Set-RiskRating {

# If an Issue can be tied to a principal, the principal's objectClass impacts the Issue's risk
$SID = $Issue.IdentityReferenceSID.ToString()
$IdentityReferenceObjectClass = Get-ADObject -Filter { objectSid -eq $SID } | Select-Object objectClass
$IdentityReferenceObjectClass = Get-SidObjectClass -Sid $SID


if ($Issue.IdentityReferenceSID -match $UnsafeUsers) {
Expand Down Expand Up @@ -3686,7 +3741,7 @@ function Set-RiskRating {
}
}
$escSID = $esc.IdentityReferenceSID.ToString()
$escIdentityReferenceObjectClass = Get-ADObject -Filter { objectSid -eq $escSID } | Select-Object objectClass
$escIdentityReferenceObjectClass = Get-SidObjectClass -Sid $escSID
if ($escSID -match $SafeUsers) {
# Safe Users are admins. Authenticating as an admin is bad.
$Principals += $esc.IdentityReference.Value
Expand Down Expand Up @@ -3737,7 +3792,7 @@ function Set-RiskRating {
}
}
$escSID = $esc.IdentityReferenceSID.ToString()
$escIdentityReferenceObjectClass = Get-ADObject -Filter { objectSid -eq $escSID } | Select-Object objectClass
$escIdentityReferenceObjectClass = Get-SidObjectClass -Sid $escSID
if ($escSID -match $SafeUsers) {
# Safe Users are admins. Authenticating as an admin is bad.
$Principals += $esc.IdentityReference.Value
Expand Down Expand Up @@ -3792,7 +3847,7 @@ function Set-RiskRating {
}
}
$escSID = $esc.IdentityReferenceSID.ToString()
$escIdentityReferenceObjectClass = Get-ADObject -Filter { objectSid -eq $escSID } | Select-Object objectClass
$escIdentityReferenceObjectClass = Get-SidObjectClass -Sid $escSID
if ($escSID -match $UnsafeUsers) {
# Unsafe Users are large groups.
$Principals += $esc.IdentityReference.Value
Expand Down Expand Up @@ -3830,7 +3885,7 @@ function Set-RiskRating {
}
}
$escSID = $esc.IdentityReferenceSID.ToString()
$escIdentityReferenceObjectClass = Get-ADObject -Filter { objectSid -eq $escSID } | Select-Object objectClass
$escIdentityReferenceObjectClass = Get-SidObjectClass -Sid $escSID
if ($escSID -match $UnsafeUsers) {
# Unsafe Users are large groups.
$Principals += $esc.IdentityReference.Value
Expand Down Expand Up @@ -3872,7 +3927,7 @@ function Set-RiskRating {
}
}
$OtherIssueSID = $OtherIssue.IdentityReferenceSID.ToString()
$OtherIssueIdentityReferenceObjectClass = (Get-ADObject -Filter { objectSid -eq $OtherIssueSID } | Select-Object objectClass).objectClass
$OtherIssueIdentityReferenceObjectClass = (Get-SidObjectClass -Sid $OtherIssueSID).objectClass
if ($OtherIssueSID -match $UnsafeUsers) {
# Unsafe Users are large groups.
$Principals += $OtherIssue.IdentityReference.Value
Expand Down
69 changes: 62 additions & 7 deletions Private/Set-RiskRating.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,58 @@
function Get-SidObjectClass {
<#
.SYNOPSIS
Returns the AD objectClass for a given SID, using a session-scoped cache to avoid repeated LDAP queries.

.DESCRIPTION
Wraps Get-ADObject with a script-scoped hashtable cache so that repeated SID-to-objectClass lookups
within a single Locksmith scan run hit Active Directory only once per unique SID. Common principals
such as Domain Users, Authenticated Users, and Domain Computers appear across many issues and
benefit most from this cache.

.PARAMETER Sid
The SID string to look up.

.OUTPUTS
PSCustomObject with an objectClass property, or $null if the SID is not found in AD.

.NOTES
The cache ($script:SidObjectClassCache) persists for the lifetime of the module session. It is
intentionally not cleared between issues so that common principals are resolved only once per
scan run.
Comment thread
Copilot marked this conversation as resolved.
Outdated

.EXAMPLE
$objectClassInfo = Get-SidObjectClass -Sid 'S-1-5-21-...-512'
#>
[CmdletBinding()]
[OutputType([PSCustomObject])]
param (
[Parameter(Mandatory)]
[string]$Sid
)

if ($null -eq $script:SidObjectClassCache) {
$script:SidObjectClassCache = @{}
}

if (-not $script:SidObjectClassCache.ContainsKey($Sid)) {
try {
# Use -ErrorAction Stop so transient ADWS/AD errors throw rather than producing
# $null output. Only cache a result (including $null for a genuine "not found")
# on success; leave the key absent on error so the next call can retry.
$result = Get-ADObject -Filter { objectSid -eq $Sid } -ErrorAction Stop |
Select-Object objectClass
$script:SidObjectClassCache[$Sid] = $result
} catch {
# Re-emit to the error stream so callers can detect/handle failures and so
# $ErrorActionPreference is honoured. The key is intentionally left absent
# so a subsequent call can retry the AD query.
Write-Error -ErrorRecord $_
}
}

return $script:SidObjectClassCache[$Sid]
Comment thread
SamErde marked this conversation as resolved.
}

function Set-RiskRating {
<#
.SYNOPSIS
Expand Down Expand Up @@ -67,7 +122,7 @@ function Set-RiskRating {
if ($Issue.Technique -eq 'ESC7') {
# If an Issue can be tied to a principal, the principal's objectClass impacts the Issue's risk
$SID = $Issue.IdentityReferenceSID.ToString()
$IdentityReferenceObjectClass = Get-ADObject -Filter { objectSid -eq $SID } | Select-Object objectClass
$IdentityReferenceObjectClass = Get-SidObjectClass -Sid $SID

if ($Issue.IdentityReferenceSID -match $UnsafeUsers) {
# Authenticated Users, Domain Users, Domain Computers etc. are very risky
Expand Down Expand Up @@ -126,7 +181,7 @@ function Set-RiskRating {

# If an Issue can be tied to a principal, the principal's objectClass impacts the Issue's risk
$SID = $Issue.IdentityReferenceSID.ToString()
$IdentityReferenceObjectClass = Get-ADObject -Filter { objectSid -eq $SID } | Select-Object objectClass
$IdentityReferenceObjectClass = Get-SidObjectClass -Sid $SID


if ($Issue.IdentityReferenceSID -match $UnsafeUsers) {
Expand Down Expand Up @@ -180,7 +235,7 @@ function Set-RiskRating {
}
}
$escSID = $esc.IdentityReferenceSID.ToString()
$escIdentityReferenceObjectClass = Get-ADObject -Filter { objectSid -eq $escSID } | Select-Object objectClass
$escIdentityReferenceObjectClass = Get-SidObjectClass -Sid $escSID
if ($escSID -match $SafeUsers) {
# Safe Users are admins. Authenticating as an admin is bad.
$Principals += $esc.IdentityReference.Value
Expand Down Expand Up @@ -227,7 +282,7 @@ function Set-RiskRating {
}
}
$escSID = $esc.IdentityReferenceSID.ToString()
$escIdentityReferenceObjectClass = Get-ADObject -Filter { objectSid -eq $escSID } | Select-Object objectClass
$escIdentityReferenceObjectClass = Get-SidObjectClass -Sid $escSID
if ($escSID -match $SafeUsers) {
# Safe Users are admins. Authenticating as an admin is bad.
$Principals += $esc.IdentityReference.Value
Expand Down Expand Up @@ -278,7 +333,7 @@ function Set-RiskRating {
}
}
$escSID = $esc.IdentityReferenceSID.ToString()
$escIdentityReferenceObjectClass = Get-ADObject -Filter { objectSid -eq $escSID } | Select-Object objectClass
$escIdentityReferenceObjectClass = Get-SidObjectClass -Sid $escSID
if ($escSID -match $UnsafeUsers) {
# Unsafe Users are large groups.
$Principals += $esc.IdentityReference.Value
Expand Down Expand Up @@ -314,7 +369,7 @@ function Set-RiskRating {
}
}
$escSID = $esc.IdentityReferenceSID.ToString()
$escIdentityReferenceObjectClass = Get-ADObject -Filter { objectSid -eq $escSID } | Select-Object objectClass
$escIdentityReferenceObjectClass = Get-SidObjectClass -Sid $escSID
if ($escSID -match $UnsafeUsers) {
# Unsafe Users are large groups.
$Principals += $esc.IdentityReference.Value
Expand Down Expand Up @@ -354,7 +409,7 @@ function Set-RiskRating {
}
}
$OtherIssueSID = $OtherIssue.IdentityReferenceSID.ToString()
$OtherIssueIdentityReferenceObjectClass = (Get-ADObject -Filter { objectSid -eq $OtherIssueSID } | Select-Object objectClass).objectClass
$OtherIssueIdentityReferenceObjectClass = (Get-SidObjectClass -Sid $OtherIssueSID).objectClass
if ($OtherIssueSID -match $UnsafeUsers) {
# Unsafe Users are large groups.
$Principals += $OtherIssue.IdentityReference.Value
Expand Down