Cache risk-rating SID lookups by SamErde · Pull Request #277 · jakehildreth/Locksmith

SamErde · 2026-05-06T09:18:17Z

Summary

Eliminates repeated Active Directory LDAP queries for the same SID during a Locksmith scan run.

Problem

Set-RiskRating had seven call sites all performing Get-ADObject -Filter { objectSid -eq $SID } | Select-Object objectClass. Common principals appear in many template ACLs and were re-queried on every issue.

Solution

New Get-SidObjectClass helper with $script:SidObjectClassCache in Private/Set-RiskRating.ps1. Transient ADWS errors are not cached (key stays absent for retry); errors re-emitted via Write-Error -ErrorRecord $_ so callers can detect failures and $ErrorActionPreference is honoured. All 7 call sites replaced.

Scope

Caches repeated SID lookups in Set-RiskRating.ps1
Preserves all risk score semantics
Invoke-Locksmith.ps1 manually updated for parity (Build/Build-Module.ps1 is NOT run in CI)
No Pester tests, no remediation/TLS changes, no broad refactoring

Static analysis

PSScriptAnalyzer: no new warnings introduced.

Replace seven identical Get-ADObject | Select-Object objectClass calls in Private\Set-RiskRating.ps1 with a new Get-SidObjectClass helper that wraps the query in a script-scoped hashtable cache (\). During a Locksmith scan, the same SID (Domain Users, Authenticated Users, Domain Computers, etc.) appears across many issues. Previously each issue evaluation issued a separate LDAP query for each SID. With the cache, every SID is resolved at most once per module session. Changes: - Add Get-SidObjectClass private helper at top of Set-RiskRating.ps1 - Replace all 7 Get-ADObject lookup sites with Get-SidObjectClass calls - Preserve exact return types and semantics for all call sites: - Pattern A (6 sites): returns PSCustomObject with objectClass property - Pattern B (1 site, ESC5): extracts .objectClass from the PSCustomObject - No changes to risk scoring logic, remediation, or test coverage Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This PR reduces repeated Active Directory lookups during a Locksmith scan by introducing a script-scoped SID→objectClass cache and routing existing Get-ADObject calls through a new helper in Set-RiskRating.

Changes:

Added Get-SidObjectClass helper that caches Get-ADObject SID lookups in a script-scoped hashtable.
Replaced 7 repeated Get-ADObject ... | Select-Object objectClass call sites with Get-SidObjectClass to reuse cached results.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Use -ErrorAction Stop with try/catch so that a transient ADWS error does not populate the cache with . Legitimate 'not found' (SID absent from AD) still returns and caches normally; transient errors emit Write-Warning and leave the key absent, allowing retry on the next call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…nvoke-Locksmith.ps1 Invoke-Locksmith.ps1 is the manually-maintained monolithic script that must stay in parity with the Private/*.ps1 sources. Apply the same Get-SidObjectClass helper (with try/catch error-handling) and replace the same 7 Get-ADObject | Select-Object objectClass call sites that were already applied to Private\Set-RiskRating.ps1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

SamErde

@copilot please review

Copilot · 2026-05-06T13:34:32Z

@copilot please review

The current state of the PR looks good. Both issues from the previous review have been resolved:

Transient error caching (commit a2961c4): Get-ADObject is now wrapped in try/catch with -ErrorAction Stop. Only successful results (including genuine "not found" $null) are stored in the cache; transient ADWS errors emit a Write-Warning and leave the key absent so the next call retries.
Return type semantics: All 7 call sites preserve the same return type as the original Get-ADObject | Select-Object objectClass calls — 6 sites receive the PSCustomObject directly (Pattern A), and the ESC5 loop appends .objectClass to extract the raw value (Pattern B), matching the pre-existing behaviour exactly.

No new issues found.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Replace Write-Warning with Write-Error -ErrorRecord $_ in the Get-SidObjectClass catch block so that AD lookup failures surface on the error stream and callers' $ErrorActionPreference (including Stop) is respected. Failed lookups are still not cached so each SID can be retried on the next call. Applied to both Private\Set-RiskRating.ps1 and Invoke-Locksmith.ps1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

jakehildreth · 2026-07-03T15:04:29Z

@SamErde Oooh, yes. I have a need for speed. But... can you separate the new function into its own file in /Private? tyilu

SamErde · 2026-07-03T15:07:53Z

@SamErde Oooh, yes. I have a need for speed. But... can you separate the new function into its own file in /Private? tyilu

Of course! That's a great catch.

jakehildreth · 2026-07-03T15:58:33Z

Sorry it took so long to review everything!

Copilot

🟢 Approval recommended

The changes are localized to lookup plumbing, preserve existing semantics, and the only remaining feedback is minor comment-based help wording clarification.

Review details

Files reviewed: 2/2 changed files
Comments generated: 2
Review effort level: Lite

SamErde · 2026-09-01T15:29:29Z

+        .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.


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

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

SamErde marked this pull request as ready for review May 6, 2026 09:25

Copilot AI review requested due to automatic review settings May 6, 2026 09:25

Copilot started reviewing on behalf of SamErde May 6, 2026 09:26 View session

SamErde marked this pull request as draft May 6, 2026 09:27

Copilot AI reviewed May 6, 2026

View reviewed changes

Comment thread Private/Set-RiskRating.ps1

Comment thread Private/Set-RiskRating.ps1 Outdated

SamErde requested a review from Copilot May 6, 2026 09:41

Copilot started reviewing on behalf of SamErde May 6, 2026 09:42 View session

SamErde marked this pull request as ready for review May 6, 2026 09:58

Copilot AI reviewed May 6, 2026

SamErde requested a review from Copilot May 6, 2026 13:33

SamErde commented May 6, 2026

View reviewed changes

Copilot started work on behalf of SamErde May 6, 2026 13:33 View session

Copilot started reviewing on behalf of SamErde May 6, 2026 13:34 View session

Copilot finished work on behalf of SamErde May 6, 2026 13:35

Copilot AI reviewed May 6, 2026

View reviewed changes

Comment thread Private/Set-RiskRating.ps1 Outdated

Comment thread Invoke-Locksmith.ps1

SamErde requested a review from Copilot May 6, 2026 13:56

Copilot started reviewing on behalf of SamErde May 6, 2026 13:57 View session

Copilot AI reviewed May 6, 2026

View reviewed changes

SamErde mentioned this pull request May 6, 2026

Fix pre-existing PSCustomObject vs string comparison at Set-RiskRating call sites #279

Open

Merge branch 'main' into samerde/cache-risk-sid-lookups

06d99b8

SamErde requested a lite review from Copilot September 1, 2026 15:21

SamErde self-assigned this Sep 1, 2026

SamErde added the enhancement New feature or request label Sep 1, 2026

Copilot started reviewing on behalf of SamErde September 1, 2026 15:22 View session

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Potential fix for pull request finding

ba9c8af

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

SamErde merged commit 7e3bdbd into main Sep 1, 2026
6 checks passed

SamErde deleted the samerde/cache-risk-sid-lookups branch September 1, 2026 15:36

Uh oh!

Conversation

SamErde commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Solution

Scope

Static analysis

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

SamErde left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented May 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

jakehildreth commented Jul 3, 2026

Uh oh!

SamErde commented Jul 3, 2026

Uh oh!

jakehildreth commented Jul 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

SamErde Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SamErde commented May 6, 2026 •

edited

Loading