feat: Local Group Post-Processing Rework - BED-7147 #2253
Conversation
WalkthroughComprehensive refactoring replacing PathAggregator-based group expansion with a new LocalGroupData structure throughout AD post-processing. Introduces LocalGroupData, CanRDPData, and CanRDPComputerData to centralize group membership and RDP data. Updates ADCS ESC implementations and post-processing functions to use reachability caches. Removes ResolveAllGroupMemberships; updates go.mod dependencies including dawgs v0.3.2→v0.4.2. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/go/analysis/ad/esc_shared.go (1)
303-334: Dead code and always-false condition - logic appears incomplete.The
isGroupvariable is declared at line 314 but is never set totruebecause the code that would set it (lines 316-320) is commented out. This means:
isGroupis alwaysfalse- The condition
if !isGroupat line 322 is alwaystrue- Every member is unconditionally added to the bitmap
If filtering out groups is no longer needed, simplify the code by removing the dead logic. If it is needed, uncomment and fix the implementation.
♻️ Option 1: Remove dead code if groups should be included
func expandNodeSliceToBitmapWithoutGroups(nodes []*graph.Node, groupExpansions *algo.ReachabilityCache) cardinality.Duplex[uint64] { var ( bitmap = cardinality.NewBitmap64() scratch = cardinality.NewBitmap64() ) for _, controller := range nodes { if controller.Kinds.ContainsOneOf(ad.Group) { groupExpansions.OrReachability(controller.ID.Uint64(), graph.DirectionInbound, scratch) - - scratch.Each(func(memberID uint64) bool { - isGroup := false - - // groupExpansions.Digraph.EachAdjacentNode(memberID, graph.DirectionInbound, func(adjacent uint64) bool { - // // If the expanded member of the controller group has an adjacent inbound node then it must be a group - // isGroup = true - // return false - // }) - - if !isGroup { - bitmap.Add(memberID) - } - - return true - }) + bitmap.Or(scratch) + scratch.Clear() } else { bitmap.Add(controller.ID.Uint64()) } } return bitmap }cmd/api/src/daemons/datapipe/datapipe.go (1)
72-104: Remove early return or guard it with a feature flag.Lines 73-74 introduce an early return that makes the entire daemon loop (lines 76-103) unreachable dead code. This disables critical pipeline operations:
- Timer initialization and cleanup
- Pruning ticker
- Pipeline lifecycle (
Start,PruneData,DeleteData,IngestTasks)- Context cancellation handling
There is no feature flag, build tag, or TODO comment explaining the temporary nature. If this is intentional for testing the LocalGroupData rework, apply a feature flag to control it consistently with the existing pattern used elsewhere in datapipe (e.g.,
GetFlagByKeyin analysis.go).packages/go/analysis/ad/post.go (1)
209-226: Bug:FetchAdminGroupsuses relationship properties in a node query.The filter uses
query.StartPropertyandquery.EndPropertywhich are relationship-specific, but the query is ontx.Nodes(). These properties don't apply to node queries and will likely cause unexpected behavior or empty results.Proposed fix
func FetchAdminGroups(ctx context.Context, db graph.Database) (cardinality.Duplex[uint64], error) { computerNodeIds := cardinality.NewBitmap64() return computerNodeIds, db.ReadTransaction(ctx, func(tx graph.Transaction) error { return tx.Nodes().Filter(query.And( - query.Or( - query.StringEndsWith(query.StartProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), - query.StringEndsWith(query.EndProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), - ), + query.Kind(query.Node(), ad.LocalGroup), + query.StringEndsWith(query.NodeProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), )).FetchIDs(func(cursor graph.Cursor[graph.ID]) error { for id := range cursor.Chan() { computerNodeIds.Add(id.Uint64()) } return nil }) }) }
🤖 Fix all issues with AI agents
In `@cmd/api/src/analysis/ad/post.go`:
- Around line 26-61: The Post function in ad.post.go immediately returns,
disabling all AD post-processing (DeleteTransitEdges, PostDCSync,
PostLocalGroups, PostADCS, PostNTLM) and leaving parameters adcsEnabled,
citrixEnabled, ntlmEnabled, compositionCounter unused; add a single-line TODO
comment (e.g., "// TODO: BED-7147 - AD post-processing disabled pending Local
Group Post-Processing Rework; re-enable
DeleteTransitEdges/PostDCSync/PostLocalGroups/PostADCS/PostNTLM and restore use
of adcsEnabled/citrixEnabled/ntlmEnabled/compositionCounter when BED-7147 is
complete or gate via a feature flag") immediately before the early return in
Post to explain the temporary state and next steps for re-enabling.
In `@cmd/api/src/analysis/membership_integration_test.go`:
- Line 17: The file's integration-only build constraint is accidentally
commented out ("// //go:build integration"), so remove the extra leading "// "
to restore the directive (make the first non-blank line read "//go:build
integration"), ensure there's a blank line after the build tag per Go rules, and
save—this will prevent the integration tests in membership_integration_test.go
from being built in normal test runs.
In `@packages/go/analysis/ad/ad.go`:
- Around line 476-533: The function expandNodeSetGroupMemberships is currently
unused; either remove it entirely or retain it with a short comment explaining
its intended future purpose and why it must remain (e.g., reserved for group
expansion optimizations), and fix the typo in the comment/inline string from
"exlcuded" to "excluded" so the message is correct; be sure to adjust any
import/unused warnings if you remove the function.
In `@packages/go/analysis/ad/local_groups.go`:
- Around line 126-128: The slog.Error call currently uses an empty message which
makes logs unclear; replace the empty string in the slog.Error(...) call with a
concise, descriptive message (e.g., "failed to <operation>" reflecting the
surrounding operation) and keep the error field (use slog.String("err",
err.Error()) or slog.Any("err", err)) so the log includes both context and the
error details—update the slog.Error invocation in local_groups.go where
slog.Error("", slog.String("err", err.Error())) is present.
- Line 206: The worker loop uses runtime.NumCPU()/2 and can yield zero workers;
change the loop to compute a worker count variable (e.g., workers :=
runtime.NumCPU()/2), clamp it to at least 1 (if workers < 1 { workers = 1 }),
and then iterate for workerID := 0; workerID < workers; workerID++ so the loop
in local_groups.go never runs with zero workers (mirror the same fix applied to
PostCanRDP).
- Line 58: The for-loop that spawns workers uses runtime.NumCPU()/2 which can be
zero on single-core systems, so change the worker count calculation to ensure at
least one worker (e.g. set workers := runtime.NumCPU()/2; if workers < 1 {
workers = 1 }) and iterate for workerID := 0; workerID < workers; workerID++ to
guarantee the computerC channel is drained and avoid hangs when channels.Submit
blocks; update any related comments and use the same workerID/computerC symbols
in local_groups.go.
In `@packages/go/analysis/ad/post.go`:
- Around line 557-581: FetchNodePropertyMap currently ignores the error returned
by tx.Nodes().Filter(criteria).Query(...); capture the Query return value (e.g.
err := tx.Nodes().Filter(criteria).Query(...)) and if err != nil return nil, err
so query execution errors are propagated instead of silently returning a
possibly incomplete nodePropertyMap; keep the existing map population logic
inside the callback and only return nodePropertyMap, nil when Query succeeds.
- Around line 494-502: FetchLocalGroupMembership currently ignores the
groupSuffix argument and always uses canRDPData.RemoteDesktopUsersLocalGroup.ID;
either remove the unused groupSuffix parameter from the signature or use it to
select the correct local group before calling container.AdjacentNodes (e.g.,
look up the appropriate group ID from canRDPData using the suffix such as a
LocalGroupsBySuffix map or selector and then pass that group's ID.Uint64() into
container.AdjacentNodes instead of RemoteDesktopUsersLocalGroup.ID); ensure the
returned cardinality.NewBitmap64With still uses the adjacent nodes from the
chosen group.
🧹 Nitpick comments (3)
packages/go/analysis/ad/local_groups.go (1)
61-95: UnusedworkerIDparameter in goroutines.The
workerIDis passed to each worker goroutine but never used inside. Consider either removing the parameter or using it for logging/debugging to aid in troubleshooting.Also applies to: 104-129, 209-239, 245-297
packages/go/analysis/ad/adcs.go (1)
36-36: Consider adding nil check forlocalGroupDataparameter.The function accepts
localGroupData *LocalGroupDatabut doesn't validate it before passing to downstream functions. If a caller passesnil, this will cause panics in the ESC processing functions when accessingLocalGroupData.GroupMembershipCache.Proposed fix
func PostADCS(ctx context.Context, db graph.Database, localGroupData *LocalGroupData, adcsEnabled bool) (*analysis.AtomicPostProcessingStats, ADCSCache, error) { + if localGroupData == nil { + return &analysis.AtomicPostProcessingStats{}, ADCSCache{}, fmt.Errorf("localGroupData cannot be nil") + } var cache = NewADCSCache()cmd/api/src/test/integration/harnesses.go (1)
5534-5543: Make the “Membership” comment unambiguous (or drop the numeric mapping).The
// 1: 1, 3, 4, 5style lines aren’t self-describing and are easy to desync from the actual edges below. Consider rewriting as explicit, verifiable statements (e.g., “User1 is transitively member of Group1 via Group4, and Group2 via Group3”) or remove the numeric mapping entirely.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
cmd/api/src/analysis/ad/post.gocmd/api/src/analysis/membership_integration_test.gocmd/api/src/analysis/post_integration_test.gocmd/api/src/daemons/datapipe/datapipe.gocmd/api/src/test/integration/harnesses.gogo.modpackages/go/analysis/ad/ad.gopackages/go/analysis/ad/adcs.gopackages/go/analysis/ad/adcscache.gopackages/go/analysis/ad/esc1.gopackages/go/analysis/ad/esc10.gopackages/go/analysis/ad/esc13.gopackages/go/analysis/ad/esc3.gopackages/go/analysis/ad/esc4.gopackages/go/analysis/ad/esc6.gopackages/go/analysis/ad/esc9.gopackages/go/analysis/ad/esc_shared.gopackages/go/analysis/ad/local_groups.gopackages/go/analysis/ad/membership.gopackages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/owns.gopackages/go/analysis/ad/post.go
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
packages/go/analysis/ad/esc13.gopackages/go/analysis/ad/esc10.gopackages/go/analysis/ad/esc3.gocmd/api/src/analysis/membership_integration_test.gopackages/go/analysis/ad/owns.gopackages/go/analysis/ad/esc9.gopackages/go/analysis/ad/ad.gocmd/api/src/analysis/post_integration_test.gopackages/go/analysis/ad/esc_shared.gopackages/go/analysis/ad/esc1.gopackages/go/analysis/ad/esc6.gopackages/go/analysis/ad/esc4.gopackages/go/analysis/ad/ntlm.gogo.modpackages/go/analysis/ad/adcs.go
📚 Learning: 2025-07-10T14:31:01.781Z
Learnt from: JonasBK
Repo: SpecterOps/BloodHound PR: 1671
File: packages/go/analysis/ad/esc16.go:65-69
Timestamp: 2025-07-10T14:31:01.781Z
Learning: In BloodHound ADCS ESC analysis functions (PostADCSESC1, PostADCSESC4, PostADCSESC6, PostADCSESC9, PostADCSESC10, PostADCSESC13, PostADCSESC16, etc.), the established pattern for handling cert template validation errors and user filtering errors is to use slog.WarnContext to log warnings and continue processing with the next template, rather than returning errors to the caller. This pattern is consistently applied across all ADCS ESC edge implementations.
Applied to files:
packages/go/analysis/ad/esc13.gopackages/go/analysis/ad/esc10.gopackages/go/analysis/ad/esc3.gopackages/go/analysis/ad/esc9.gopackages/go/analysis/ad/esc1.gopackages/go/analysis/ad/esc6.gopackages/go/analysis/ad/esc4.go
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
packages/go/analysis/ad/esc13.gopackages/go/analysis/ad/ntlm.go
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
packages/go/analysis/ad/owns.gopackages/go/analysis/ad/adcs.go
📚 Learning: 2025-07-10T14:33:20.317Z
Learnt from: JonasBK
Repo: SpecterOps/BloodHound PR: 1671
File: cmd/api/src/analysis/ad/adcs_integration_test.go:3687-3687
Timestamp: 2025-07-10T14:33:20.317Z
Learning: When reviewing Go code, functions defined in one file within a package are accessible from other files in the same package. Before flagging missing functions as compilation errors, check if they exist in other files within the same package directory.
Applied to files:
packages/go/analysis/ad/owns.go
📚 Learning: 2025-06-25T18:24:25.014Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T18:24:25.014Z
Learning: In BloodHound Go code, for error types in slog structured logging, prefer using slog.String("key", err.Error()) over slog.Any("key", err). The explicit string conversion with err.Error() is preferred over using slog.Any() for error types.
Applied to files:
packages/go/analysis/ad/owns.go
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/analysis/post_integration_test.go
📚 Learning: 2025-05-27T16:58:33.295Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Applied to files:
cmd/api/src/analysis/post_integration_test.go
📚 Learning: 2025-06-17T22:37:36.389Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1595
File: cmd/api/src/api/v2/saved_queries_test.go:2594-2594
Timestamp: 2025-06-17T22:37:36.389Z
Learning: In Go table-driven tests, there's a distinction between main test function parallelism and subtest parallelism. Main test functions can safely use t.Parallel() for performance benefits, but individual subtests within table-driven tests may need to run sequentially to avoid race conditions with mocks, deferred functions, or shared resources.
Applied to files:
cmd/api/src/analysis/post_integration_test.go
📚 Learning: 2025-06-11T20:49:35.177Z
Learnt from: kpom-specter
Repo: SpecterOps/BloodHound PR: 1562
File: cmd/api/src/daemons/datapipe/datapipe.go:100-120
Timestamp: 2025-06-11T20:49:35.177Z
Learning: In BloodHound's datapipe architecture, only the datapipe (e.g., `WithDatapipeStatus`) is responsible for mutating the datapipe status; pipeline actions themselves do not alter the status and instead communicate any job-level state elsewhere.
Applied to files:
cmd/api/src/daemons/datapipe/datapipe.go
📚 Learning: 2025-05-23T19:56:13.423Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/BloodHound PR: 1440
File: packages/go/analysis/ad/ntlm.go:123-138
Timestamp: 2025-05-23T19:56:13.423Z
Learning: In the BloodHound NTLM analysis code (packages/go/analysis/ad/ntlm.go), the `isProtectedComputer` function intentionally fails open (returns false/unprotected) when the Protected Users group cannot be retrieved, maintaining consistency with the original logic patterns in the codebase.
Applied to files:
packages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/post.go
📚 Learning: 2025-06-30T22:58:54.834Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1629
File: go.mod:18-18
Timestamp: 2025-06-30T22:58:54.834Z
Learning: Go 1.24 supports patch versions in the go directive of go.mod files. Syntax like "go 1.24.4" is valid and specifies the minimum Go version required to build the module, including the patch level.
Applied to files:
go.mod
📚 Learning: 2025-08-26T18:13:36.643Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1820
File: .github/workflows/audit-release.yml:0-0
Timestamp: 2025-08-26T18:13:36.643Z
Learning: The go tool command supports custom/third-party binaries as of Go 1.24, allowing invocation of custom CLI tools like "go tool stbernard" rather than being limited to only built-in Go toolchain binaries.
Applied to files:
go.mod
🧬 Code graph analysis (14)
packages/go/analysis/ad/local_groups.go (6)
cmd/api/src/database/db.go (1)
Database(73-192)packages/go/analysis/ad/post.go (5)
LocalGroupData(620-625)CanRDPComputerData(719-726)FetchCanRDPEntityBitmapForComputer(527-555)FetchCanRDPData(693-699)FetchComputerLocalGroupIDBySIDSuffix(452-477)packages/go/analysis/post_operation.go (2)
AtomicPostProcessingStats(86-90)NewAtomicPostProcessingStats(92-98)packages/go/analysis/post.go (1)
CreatePostRelationshipJob(151-157)packages/go/graphschema/ad/ad.go (4)
CanRDP(68-68)AdminTo(66-66)CanPSRemote(67-67)ExecuteDCOM(69-69)packages/go/analysis/analysis.go (1)
MaximumDatabaseParallelWorkers(36-36)
packages/go/analysis/ad/esc13.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(620-625)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(537-698)
packages/go/analysis/ad/esc10.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(620-625)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)
packages/go/analysis/ad/esc3.go (3)
packages/go/analysis/ad/post.go (1)
LocalGroupData(620-625)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(537-698)
packages/go/analysis/ad/owns.go (1)
packages/go/analysis/ad/post.go (1)
LocalGroupData(620-625)
packages/go/analysis/ad/esc9.go (3)
packages/go/analysis/ad/post.go (1)
LocalGroupData(620-625)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)packages/go/schemagen/tsgen/tsgen.go (1)
ID(216-220)
cmd/api/src/analysis/post_integration_test.go (2)
packages/go/analysis/ad/post.go (1)
FetchLocalGroupData(627-661)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(537-698)
cmd/api/src/daemons/datapipe/datapipe.go (1)
cmd/api/src/model/datapipestatus.go (1)
DatapipeStatusAnalyzing(26-26)
packages/go/analysis/ad/esc1.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(620-625)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(537-698)
packages/go/analysis/ad/esc6.go (2)
packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(537-698)
packages/go/analysis/ad/esc4.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(620-625)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(537-698)
packages/go/analysis/ad/ntlm.go (1)
packages/go/analysis/ad/post.go (1)
LocalGroupData(620-625)
packages/go/analysis/ad/adcs.go (4)
packages/go/analysis/ad/post.go (1)
LocalGroupData(620-625)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)packages/go/analysis/ad/esc1.go (1)
PostADCSESC1(35-64)packages/go/analysis/ad/esc13.go (1)
PostADCSESC13(36-78)
packages/go/analysis/ad/post.go (2)
packages/go/analysis/ad/wellknown/suffix.go (2)
AdministratorsSIDSuffix(83-83)RemoteDesktopUsersSIDSuffix(86-86)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(537-698)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
🔇 Additional comments (59)
cmd/api/src/analysis/post_integration_test.go (5)
17-17: Build tag has been disabled - verify if intentional.The
//go:build serial_integrationdirective is now double-commented (// //go:build), which turns it into a regular comment and removes the build constraint entirely. These tests will now run in all builds instead of only during serial integration testing.If intentional, consider removing the line entirely. If not, restore the original:
-// //go:build serial_integration +//go:build serial_integration
84-92: LGTM - refactored to use new LocalGroupData API.The test correctly uses the new pattern: fetch
LocalGroupDatafirst, then compute cross-product node sets. The parameter rename tographDBis consistent with the new API usage.Note: The
txparameter is unused in this callback. If the transaction isn't needed for any assertions, consider whether the test could use a simpler callback signature (if the test framework supports it).
101-109: LGTM - consistent refactoring pattern.The changes align with the LocalGroupData API refactor applied across all cross-product tests.
118-126: LGTM - consistent with other cross-product tests.
135-143: LGTM - consistent refactoring with appropriate multi-assertion verification.cmd/api/src/analysis/membership_integration_test.go (1)
83-91: LGTM - Test correctly refactored to use ReachabilityCache pattern.The test properly invokes
algo.FetchReachabilityCachewith appropriate relationship kinds (ad.MemberOf,ad.MemberOfLocalGroup) and validates membership cardinalities usingReachExclusivewithgraph.DirectionInbound. The expected cardinality values (3, 1, 1, 1, 2) are correct: DomainGroupA has 3 transitive members (JohnUser, RDPDomainUsersGroup, RohanUser); DomainGroupB, C, and D each have 1 direct member; DomainGroupE has 2 members through group nesting (DomainGroupD + IrshadUser).packages/go/analysis/ad/membership.go (1)
81-108: LGTM - Simplified direction reversal.The inline call to
direction.Reverse()at line 93 is a clean simplification that maintains the same behavior while removing the unnecessary intermediate variable. The overallFetchPathMembersfunction logic remains correct.packages/go/analysis/ad/local_groups.go (1)
30-56: LGTM - Batch write pattern is well-structured.Both
PostCanRDPandPostLocalGroupsuse a consistent pattern for batching relationship writes with proper channel coordination and statistics tracking. The deferred channel closures and wait group synchronization ensure clean shutdown.Also applies to: 177-203
packages/go/analysis/ad/esc_shared.go (1)
394-399: LGTM - LocalGroupData integration in getVictimBitmap.The function correctly uses
localGroupData.GroupMembershipCacheto expand group memberships, aligning with the broader refactor fromPathAggregatortoLocalGroupData.go.mod (1)
50-50: Dependency update verified: v0.4.1 provides the required ReachabilityCache API.The bump from
v0.3.2tov0.4.1introducesalgo.ReachabilityCacheand is confirmed compatible. The codebase actively usesFetchReachabilityCache()andFetchFilteredReachabilityCache()functions, and the version is available in the Go proxy registry.packages/go/analysis/ad/esc1.go (1)
35-35: LGTM - Signature migration to LocalGroupData.The function signature correctly migrates from
impact.PathAggregatorto*LocalGroupData, and the call toCalculateCrossProductNodeSetsis properly updated to passlocalGroupDataas the first argument, matching the new signature inad/ad.go.Also applies to: 48-48
packages/go/analysis/ad/esc13.go (1)
36-36: LGTM - Consistent signature migration.The signature change from
impact.PathAggregatorto*LocalGroupDatais consistent with the broader refactor. The updated call toCalculateCrossProductNodeSetscorrectly passeslocalGroupDataas the first argument. Error handling continues to follow the established pattern of logging warnings and continuing to the next template. Based on learnings, this is the expected pattern for ADCS ESC analysis functions.Also applies to: 53-53
packages/go/analysis/ad/owns.go (2)
38-45: LGTM - LocalGroupData integration in PostOwnsAndWriteOwner.The function correctly accepts
*LocalGroupDataand extractsGroupMembershipCachefor use inFetchAdminGroupIds. This aligns with the broader migration fromPathAggregatortoLocalGroupData.
196-213: OrReachability method signature and behavior confirmed.The
OrReachabilitymethod exists in thealgo.ReachabilityCachetype with the expected signature and correctly ORs the inbound reachability set (all members of the group) intoadminIds. The implementation properly expands group memberships through the reachability cache.packages/go/analysis/ad/adcscache.go (1)
30-30: LGTM - Timing instrumentation addition.The deferred timing wrapper using
util.SLogMeasureFunctionfollows the standard Go pattern for measuring function execution time. The double invocation()()correctly captures the start time immediately while deferring the duration logging. This usage is consistent with similar instrumentation patterns in the codebase (e.g.,local_groups.go,post.go).packages/go/analysis/ad/esc9.go (4)
35-35: LGTM! Signature updated to useLocalGroupData.The function signature change from
groupExpansions impact.PathAggregatortolocalGroupData *LocalGroupDataaligns with the broader refactor across the AD post-processing modules. This centralizes group membership data into a single container structure.
84-84: LGTM! Consistent signature change forPostADCSESC9b.The signature update mirrors
PostADCSESC9a, maintaining consistency across the ESC9 implementations.
102-102: LGTM! Consistent usage oflocalGroupDatain ESC9b.The
getVictimBitmapcall correctly uses the newlocalGroupDataparameter.
53-53: Parameter propagation togetVictimBitmapis correct.The function signature in
esc_shared.go(line 394) confirmsgetVictimBitmapaccepts*LocalGroupDataas the first parameter, and the call site correctly passeslocalGroupDataalong with all other parameters in the correct order and types. The logic flow is maintained with the updated data structure.packages/go/analysis/ad/esc3.go (3)
38-38: LGTM! Signature updated forPostADCSESC3.The function signature change is consistent with the broader refactor, replacing
groupExpansions impact.PathAggregatorwithlocalGroupData *LocalGroupData.
91-97: LGTM! Cross-product calculation updated for enrollment agent restrictions case.The
CalculateCrossProductNodeSetscall correctly passeslocalGroupDataas the first argument, with the remaining node slice arguments preserved. ThedelegatedAgents.Slice()is appropriately included when enrollment agent restrictions apply.
109-114: LGTM! Cross-product calculation updated for non-restricted case.The call correctly uses
localGroupDataand maintains the same node slice arguments for computing the cross-product when enrollment agent restrictions do not apply.packages/go/analysis/ad/esc10.go (4)
36-36: LGTM! Signature updated forPostADCSESC10a.The function signature change aligns with the broader refactor, replacing the
groupExpansionsparameter withlocalGroupData *LocalGroupData.
54-54: LGTM! Victim bitmap calculation updated.The
getVictimBitmapcall correctly useslocalGroupDatato compute the victim bitmap for ESC10a.
84-84: LGTM! Consistent signature change forPostADCSESC10b.The signature update mirrors
PostADCSESC10a, maintaining consistency across the ESC10 implementations.
102-102: LGTM! Consistent usage oflocalGroupDatain ESC10b.The
getVictimBitmapcall correctly uses the newlocalGroupDataparameter.packages/go/analysis/ad/ad.go (7)
32-32: LGTM! Import added foralgopackage.The
algoimport is required for accessingalgo.ReachabilityCache, which is used withinLocalGroupData.GroupMembershipCache.
537-537: LGTM! Signature updated forCalculateCrossProductNodeSets.The function now accepts
localGroupData *LocalGroupDataas its first parameter, centralizing access to group membership data and excluded shortcut groups.
562-603: LGTM! Cross-product logic refactored to useLocalGroupData.The loop correctly:
- Checks
localGroupData.ExcludedShortcutGroups.Contains(entityID)to skip excluded groups (Auth Users/Everyone)- Uses
localGroupData.GroupMembershipCache.Reach()to expand group memberships- Iterates over excluded nodes to detect if any are reachable from the current entity
The early-break optimization when
nodeExcludedis true prevents unnecessary processing.
614-624: LGTM! Edge case handling for single remaining set.When all but one nodeset contains excluded groups (Auth Users/Everyone), returning just that single first-degree set is the correct behavior.
631-631: LGTM! Reachability expansion viaOrReachability.Using
localGroupData.GroupMembershipCache.OrReachabilityto accumulate reachable nodes intounrolledRefSetis consistent with the new data structure pattern.
639-657: LGTM! Efficient cardinality calculation with eager bitmap clearing.The pattern of using a temporary bitmap, computing cardinality, and immediately clearing it prevents memory accumulation during the iteration. This is a good practice for processing large node sets.
682-682: LGTM! XOR-based membership removal.Using
XorReachabilityto remove all members of a matched group fromunrolledRefSetcorrectly implements the shortcut optimization that avoids redundant membership checks.packages/go/analysis/ad/esc4.go (6)
35-35: LGTM! Signature updated forPostADCSESC4.The function signature change is consistent with the broader refactor across all ESC modules.
63-67: LGTM! Cross-product for path 2a updated.The call correctly uses
localGroupDatafor computing principals that control the cert template.
71-76: LGTM! Cross-product for path 2b updated.The call correctly computes the cross-product for principals with
Enroll/AllExtendedRights+GenericWritecombinations.
88-94: LGTM! Cross-product for path 2d updated.The four-way cross-product for
Enroll/AllExtendedRights+WritePKINameFlag+WritePKIEnrollmentFlagcorrectly useslocalGroupData.
99-104: LGTM! Cross-product for path 2e updated.The conditional cross-product when
enrolleeSuppliesSubjectis true correctly useslocalGroupData.
111-116: LGTM! Cross-product for path 2f updated.The conditional cross-product when
requiresManagerApprovalis false correctly useslocalGroupData.packages/go/analysis/ad/adcs.go (1)
109-207: LGTM - Consistent refactoring of ESC processing calls.The
localGroupDataparameter is correctly propagated to allPostADCSESC*functions. The error handling patterns and control flow remain unchanged, ensuring behavioral consistency with the previous implementation.packages/go/analysis/ad/esc6.go (2)
37-79: LGTM - ESC6a refactored to useLocalGroupData.The signature change and
CalculateCrossProductNodeSetscall update are consistent with the broader refactoring pattern. The error handling viaslog.WarnContextcontinues to follow the established ADCS ESC pattern (per retrieved learnings).
82-127: LGTM - ESC6b refactored consistently with ESC6a.The changes mirror the ESC6a refactoring, maintaining consistency across both ESC6 variants. The
HasUPNCertMappingInForestcheck remains in place for the ESC6b-specific domain filtering.packages/go/analysis/ad/ntlm.go (6)
41-47: LGTM -NTLMCachestruct updated to useLocalGroupData.The field change from
GroupExpansions impact.PathAggregatortoLocalGroupData *LocalGroupDataaligns with the broader refactoring. The getter methods remain unchanged, preserving the cache's API.
64-125: LGTM -NewNTLMCachecorrectly propagatesLocalGroupData.The function properly passes
localGroupData.GroupMembershipCachetoFetchProtectedUsersMappedToDomainsand stores the fullLocalGroupDatareference for later use. The fail-open behavior for protected users (per retrieved learnings) is maintained.
128-213: LGTM -PostNTLMsignature and cache creation updated.The function correctly passes
localGroupDatatoNewNTLMCache. The early return for disabled NTLM feature and the nested error handling remain unchanged.
586-593: LGTM - Group expansion inPostCoerceAndRelayNTLMToSMBusesOrReachability.The bitmap expansion correctly uses
LocalGroupData.GroupMembershipCache.OrReachabilityfor group membership resolution, replacing the previousPathAggregatorapproach.
856-881: LGTM -FetchProtectedUsersMappedToDomainsacceptsReachabilityCachedirectly.The function correctly uses
OrReachabilityto populate the protected users bitmap. Taking*algo.ReachabilityCachedirectly (rather than fullLocalGroupData) is appropriate since only the cache is needed.
441-448: The functionexpandNodeSliceToBitmapWithoutGroupsexists with the correct signature accepting*algo.ReachabilityCacheas its second parameter.packages/go/analysis/ad/post.go (10)
39-72: LGTM -PostSyncLAPSPasswordupdated to useLocalGroupData.The function correctly passes
localGroupDatatogetLAPSSyncers. The domain iteration and relationship creation logic remain unchanged.
74-104: LGTM -PostDCSyncupdated to useLocalGroupData.Consistent with
PostSyncLAPSPassword, the function correctly propagateslocalGroupDatatogetDCSyncers.
187-207: LGTM - NewFetchComputerIDsfunction.Clean implementation using
FetchIDsfor efficient ID-only retrieval. The measurement logging is helpful for performance monitoring.
288-303: LGTM -getLAPSSyncersupdated to useLocalGroupData.The function correctly passes
localGroupDatatoCalculateCrossProductNodeSetsfor computing the cross-product of GetChanges and GetChangesInFilteredSet principals.
305-320: LGTM -getDCSyncersupdated to useLocalGroupData.Consistent with
getLAPSSyncers, the cross-product calculation now useslocalGroupData.
425-477: LGTM - Refactored local group lookup functions.Both
FetchComputerLocalGroupBySIDSuffixandFetchComputerLocalGroupIDBySIDSuffixnow use explicit query/scan patterns with proper resource cleanup viaresults.Close().
620-625: LGTM -LocalGroupDatastruct definition.The struct consolidates the necessary data for local group post-processing:
Computers: bitmap of computer IDsGroupMembershipCache: reachability cache for group membershipExcludedShortcutGroups: Auth Users/Everyone for shortcut handlingLocalGroupMembershipDigraph: directed graph for local group membershipThe TODO comment on
ExcludedShortcutGroupsshould be addressed in a follow-up.
627-661: LGTM -FetchLocalGroupDatabuilds the consolidated data structure.The function correctly:
- Fetches excluded shortcut groups (Auth Users/Everyone)
- Fetches computer IDs
- Builds the reachability cache for MemberOf and MemberOfLocalGroup relationships
- Builds the directed graph for local group membership
701-755: LGTM -CanRDPDataandCanRDPComputerDatatypes.The type hierarchy is well-structured:
CanRDPDataextendsLocalGroupDatawith URA-specific dataCanRDPComputerDataextendsCanRDPDatawith per-computer contextHasRemoteInteractiveLogonRightmethod provides efficient lookup via digraph
771-810: LGTM - Refactored RDP processing functions.
FetchRemoteDesktopUsersBitmapForComputerandProcessRDPWithUracorrectly use the newCanRDPComputerDatastructure. TheProcessRDPWithUraimplementation:
- Gets RDP local group members via reachability cache
- Gets RIL entities from the digraph
- Performs cross-product validation
The logic is clear and efficient.
cmd/api/src/test/integration/harnesses.go (1)
5544-5548: Code changes are correct and require no test updates.The new relationships (
User1 -> Group3andUser1 -> Group4) properly expand the transitive membership coverage. Tests inpost_integration_test.gothat depend onShortcutHarnessonly verify the presence of specific group relationships (e.g., thatGroup3is reachable via cross-product calculation), not exact membership counts. These presence-based assertions will continue to pass with the expanded relationship set.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
zinic
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/api/src/daemons/datapipe/datapipe.go (1)
72-104: Critical: Early return leaves daemon loop unreachable.The
returnon line 74 makes all code from line 76 onwards dead code. The daemon will only execute a singleAnalyzecall and immediately exit, bypassing:
- The normal
Startpipeline action- Scheduled pruning, purging, and ingesting
- The main event loop and graceful shutdown handling
If this is intentional for the rework, the unreachable code (lines 76–103) should be removed. If accidental (e.g., debugging code left in), remove lines 73–75 to restore normal daemon behavior.
Option A: Remove early return if this was debugging code
func (s *Daemon) Start(ctx context.Context) { - s.WithDatapipeStatus(ctx, model.DatapipeStatusAnalyzing, s.pipeline.Analyze) - return - var ( datapipeLoopTimer = time.NewTimer(s.startDelay)Option B: Remove dead code if early return is intentional
func (s *Daemon) Start(ctx context.Context) { s.WithDatapipeStatus(ctx, model.DatapipeStatusAnalyzing, s.pipeline.Analyze) - return - - var ( - datapipeLoopTimer = time.NewTimer(s.startDelay) - pruningTicker = time.NewTicker(pruningInterval) - ) - - defer datapipeLoopTimer.Stop() - defer pruningTicker.Stop() - - s.WithDatapipeStatus(ctx, model.DatapipeStatusStarting, s.pipeline.Start) - - for { - select { - case <-pruningTicker.C: - s.WithDatapipeStatus(ctx, model.DatapipeStatusPruning, s.pipeline.PruneData) - - case <-datapipeLoopTimer.C: - s.WithDatapipeStatus(ctx, model.DatapipeStatusPurging, s.pipeline.DeleteData) - - s.WithDatapipeStatus(ctx, model.DatapipeStatusIngesting, s.pipeline.IngestTasks) - - s.WithDatapipeStatus(ctx, model.DatapipeStatusAnalyzing, s.pipeline.Analyze) - - datapipeLoopTimer.Reset(s.tickInterval) - - case <-ctx.Done(): - return - } - } }packages/go/analysis/ad/esc_shared.go (1)
303-334: Remove dead variable and commented code; addscratch.Clear()between iterations.The
isGroupvariable is initialized tofalsebut the code block that would set it totrueis commented out (lines 316-320), making the condition on line 322 always evaluate totrue. This removes the group filtering logic entirely. Additionally, thescratchbitmap is reused across loop iterations without being cleared, causing data from previous controller iterations to accumulate. Each iteration should start with an emptyscratch.Remove the dead variable, commented code, and add
scratch.Clear()after thescratch.Each()call to ensure proper state between iterations.🔧 Proposed fix
func expandNodeSliceToBitmapWithoutGroups(nodes []*graph.Node, groupExpansions *algo.ReachabilityCache) cardinality.Duplex[uint64] { var ( bitmap = cardinality.NewBitmap64() scratch = cardinality.NewBitmap64() ) for _, controller := range nodes { if controller.Kinds.ContainsOneOf(ad.Group) { groupExpansions.OrReachability(controller.ID.Uint64(), graph.DirectionInbound, scratch) scratch.Each(func(memberID uint64) bool { - isGroup := false - - // groupExpansions.Digraph.EachAdjacentNode(memberID, graph.DirectionInbound, func(adjacent uint64) bool { - // // If the expanded member of the controller group has an adjacent inbound node then it must be a group - // isGroup = true - // return false - // }) - - if !isGroup { - bitmap.Add(memberID) - } - + bitmap.Add(memberID) return true }) + scratch.Clear() } else { bitmap.Add(controller.ID.Uint64()) } } return bitmap }packages/go/analysis/ad/post.go (1)
209-226: Bug: Using relationship query helpers in a node query.
query.StartPropertyandquery.EndPropertyare designed for relationship queries (to access properties of the start/end nodes of a relationship). When used in atx.Nodes().Filter(...)context, this won't produce the expected results. Usequery.NodePropertyinstead.Proposed fix
func FetchAdminGroups(ctx context.Context, db graph.Database) (cardinality.Duplex[uint64], error) { computerNodeIds := cardinality.NewBitmap64() return computerNodeIds, db.ReadTransaction(ctx, func(tx graph.Transaction) error { return tx.Nodes().Filter(query.And( - query.Or( - query.StringEndsWith(query.StartProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), - query.StringEndsWith(query.EndProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), - ), + query.StringEndsWith(query.NodeProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), )).FetchIDs(func(cursor graph.Cursor[graph.ID]) error { for id := range cursor.Chan() { computerNodeIds.Add(id.Uint64()) } return nil }) }) }
🤖 Fix all issues with AI agents
In `@packages/go/analysis/ad/local_groups.go`:
- Around line 237-244: The switch on nextJob.groupSuffix (cases
adminGroupSuffix, psRemoteGroupSuffix, dcomGroupSuffix) can leave edgeKind unset
for unknown suffixes; add a default case that handles unexpected values by
logging the unexpected nextJob.groupSuffix (or returning an error) and
skipping/continuing rather than using an uninitialized edgeKind, so that
invalid/empty edge kinds are never created.
- Around line 113-146: The early return when localGroupData.FetchCanRDPData(ctx,
graphDB) fails leaves the post goroutine (the one that reads from postC started
earlier) blocked because it never receives a close; fix by either moving the
post goroutine startup (the goroutine that reads from postC) so it is launched
only after FetchCanRDPData succeeds, or ensure all channels the post goroutine
consumes (postC and any upstream channels like workC/computerC) are closed on
every exit path (e.g., add a defer to close those channels before calling
FetchCanRDPData or before returning on error); update the code paths around
localGroupData.FetchCanRDPData, channels.Receive, channels.Submit, and the postC
goroutine accordingly so no goroutine can block on an unclosed channel on early
return.
In `@packages/go/analysis/ad/post.go`:
- Around line 452-477: FetchComputerLocalGroupIDBySIDSuffix currently returns a
zero graph.ID when no rows match; update the query callback (or post-query
handling) to return graph.ErrNoResultsFound instead of silently returning nil
error when results.Next() is false — e.g., inside the anonymous Query callback
for FetchComputerLocalGroupIDBySIDSuffix, if results.Next() is false return
graph.ErrNoResultsFound (or set queryErr accordingly) so callers receive a clear
no-results error similar to FetchComputerLocalGroupBySIDSuffix.
- Around line 772-778: FetchRemoteDesktopUsersBitmapForComputer calls
FetchRemoteDesktopUsersLocalGroupMembership with three arguments but that
function only accepts two; update the call in
FetchRemoteDesktopUsersBitmapForComputer to match
FetchRemoteDesktopUsersLocalGroupMembership’s signature (pass the two expected
parameters—e.g., computer and wellknown.RemoteDesktopUsersSIDSuffix.String()—and
drop canRDPData), or alternatively modify
FetchRemoteDesktopUsersLocalGroupMembership to accept the third parameter
(canRDPData) if its logic requires it; ensure ProcessRDPWithUra remains
unchanged and that parameter names (computer, canRDPData, rdpGroup,
wellknown.RemoteDesktopUsersSIDSuffix) are used consistently.
♻️ Duplicate comments (1)
packages/go/analysis/ad/post.go (1)
557-582: LGTM - Query error handling is now correct.The error from
tx.Nodes().Filter(criteria).Query(...)is properly captured and returned. This addresses the concern from the previous review.
🧹 Nitpick comments (2)
cmd/api/src/analysis/membership_integration_test.go (1)
77-92: Consider renaming the test to reflect the new API.The implementation correctly adapts to the new
algo.FetchReachabilityCacheAPI and usesReachExclusivefor membership assertions. However, the test nameTestResolveAllGroupMembershipsreferences the removedResolveAllGroupMembershipsfunction.Suggested rename
-func TestResolveAllGroupMemberships(t *testing.T) { +func TestFetchReachabilityCache(t *testing.T) {packages/go/analysis/ad/post.go (1)
621-626: TODO comment needs clarification.There's a
TODO: Document mecomment forExcludedShortcutGroups. Consider adding documentation explaining the purpose of this field and which groups it represents.Would you like me to help draft documentation for this field, or should I open an issue to track this?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
cmd/api/src/analysis/ad/post.gocmd/api/src/analysis/membership_integration_test.gocmd/api/src/analysis/post_integration_test.gocmd/api/src/daemons/datapipe/datapipe.gocmd/api/src/test/integration/harnesses.gogo.modpackages/go/analysis/ad/ad.gopackages/go/analysis/ad/adcs.gopackages/go/analysis/ad/adcscache.gopackages/go/analysis/ad/esc1.gopackages/go/analysis/ad/esc10.gopackages/go/analysis/ad/esc13.gopackages/go/analysis/ad/esc3.gopackages/go/analysis/ad/esc4.gopackages/go/analysis/ad/esc6.gopackages/go/analysis/ad/esc9.gopackages/go/analysis/ad/esc_shared.gopackages/go/analysis/ad/local_groups.gopackages/go/analysis/ad/membership.gopackages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/owns.gopackages/go/analysis/ad/post.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/api/src/test/integration/harnesses.go
- packages/go/analysis/ad/esc9.go
- packages/go/analysis/ad/esc13.go
- packages/go/analysis/ad/esc1.go
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-06-25T18:24:25.014Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T18:24:25.014Z
Learning: In BloodHound Go code, for error types in slog structured logging, prefer using slog.String("key", err.Error()) over slog.Any("key", err). The explicit string conversion with err.Error() is preferred over using slog.Any() for error types.
Applied to files:
packages/go/analysis/ad/local_groups.gopackages/go/analysis/ad/owns.go
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
packages/go/analysis/ad/local_groups.gopackages/go/analysis/ad/owns.gopackages/go/analysis/ad/adcs.go
📚 Learning: 2025-05-27T16:58:33.295Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Applied to files:
cmd/api/src/analysis/membership_integration_test.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
cmd/api/src/analysis/membership_integration_test.gopackages/go/analysis/ad/esc_shared.gopackages/go/analysis/ad/owns.gopackages/go/analysis/ad/ad.gopackages/go/analysis/ad/esc4.gopackages/go/analysis/ad/adcs.gopackages/go/analysis/ad/esc6.gopackages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/esc10.gopackages/go/analysis/ad/esc3.gogo.mod
📚 Learning: 2025-07-10T14:33:20.317Z
Learnt from: JonasBK
Repo: SpecterOps/BloodHound PR: 1671
File: cmd/api/src/analysis/ad/adcs_integration_test.go:3687-3687
Timestamp: 2025-07-10T14:33:20.317Z
Learning: When reviewing Go code, functions defined in one file within a package are accessible from other files in the same package. Before flagging missing functions as compilation errors, check if they exist in other files within the same package directory.
Applied to files:
packages/go/analysis/ad/owns.go
📚 Learning: 2025-07-10T14:31:01.781Z
Learnt from: JonasBK
Repo: SpecterOps/BloodHound PR: 1671
File: packages/go/analysis/ad/esc16.go:65-69
Timestamp: 2025-07-10T14:31:01.781Z
Learning: In BloodHound ADCS ESC analysis functions (PostADCSESC1, PostADCSESC4, PostADCSESC6, PostADCSESC9, PostADCSESC10, PostADCSESC13, PostADCSESC16, etc.), the established pattern for handling cert template validation errors and user filtering errors is to use slog.WarnContext to log warnings and continue processing with the next template, rather than returning errors to the caller. This pattern is consistently applied across all ADCS ESC edge implementations.
Applied to files:
packages/go/analysis/ad/esc4.gocmd/api/src/analysis/ad/post.gopackages/go/analysis/ad/esc6.gopackages/go/analysis/ad/esc10.gopackages/go/analysis/ad/esc3.go
📚 Learning: 2025-05-23T19:56:13.423Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/BloodHound PR: 1440
File: packages/go/analysis/ad/ntlm.go:123-138
Timestamp: 2025-05-23T19:56:13.423Z
Learning: In the BloodHound NTLM analysis code (packages/go/analysis/ad/ntlm.go), the `isProtectedComputer` function intentionally fails open (returns false/unprotected) when the Protected Users group cannot be retrieved, maintaining consistency with the original logic patterns in the codebase.
Applied to files:
cmd/api/src/analysis/ad/post.gopackages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/post.go
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
packages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/post.go
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/analysis/post_integration_test.go
📚 Learning: 2025-06-11T20:49:35.177Z
Learnt from: kpom-specter
Repo: SpecterOps/BloodHound PR: 1562
File: cmd/api/src/daemons/datapipe/datapipe.go:100-120
Timestamp: 2025-06-11T20:49:35.177Z
Learning: In BloodHound's datapipe architecture, only the datapipe (e.g., `WithDatapipeStatus`) is responsible for mutating the datapipe status; pipeline actions themselves do not alter the status and instead communicate any job-level state elsewhere.
Applied to files:
cmd/api/src/daemons/datapipe/datapipe.go
📚 Learning: 2025-06-30T22:58:54.834Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1629
File: go.mod:18-18
Timestamp: 2025-06-30T22:58:54.834Z
Learning: Go 1.24 supports patch versions in the go directive of go.mod files. Syntax like "go 1.24.4" is valid and specifies the minimum Go version required to build the module, including the patch level.
Applied to files:
go.mod
📚 Learning: 2025-08-26T18:13:36.643Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1820
File: .github/workflows/audit-release.yml:0-0
Timestamp: 2025-08-26T18:13:36.643Z
Learning: The go tool command supports custom/third-party binaries as of Go 1.24, allowing invocation of custom CLI tools like "go tool stbernard" rather than being limited to only built-in Go toolchain binaries.
Applied to files:
go.mod
🧬 Code graph analysis (8)
packages/go/analysis/ad/local_groups.go (5)
packages/go/analysis/ad/post.go (3)
LocalGroupData(621-626)CanRDPComputerData(720-727)FetchCanRDPEntityBitmapForComputer(527-555)packages/go/analysis/post_operation.go (3)
AtomicPostProcessingStats(86-90)NewAtomicPostProcessingStats(92-98)NewPropertiesWithLastSeen(166-171)packages/go/analysis/post.go (1)
CreatePostRelationshipJob(151-157)packages/go/graphschema/ad/ad.go (4)
CanRDP(68-68)AdminTo(66-66)CanPSRemote(67-67)ExecuteDCOM(69-69)packages/go/analysis/analysis.go (1)
MaximumDatabaseParallelWorkers(36-36)
packages/go/analysis/ad/owns.go (1)
packages/go/analysis/ad/post.go (1)
LocalGroupData(621-626)
packages/go/analysis/ad/ad.go (1)
packages/go/analysis/ad/post.go (1)
LocalGroupData(621-626)
packages/go/analysis/ad/esc6.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(621-626)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(477-638)
packages/go/analysis/ad/esc10.go (3)
packages/go/analysis/post.go (1)
CreatePostRelationshipJob(151-157)packages/go/analysis/ad/post.go (1)
LocalGroupData(621-626)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)
packages/go/analysis/ad/esc3.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(621-626)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(477-638)
cmd/api/src/daemons/datapipe/datapipe.go (1)
cmd/api/src/model/datapipestatus.go (1)
DatapipeStatusAnalyzing(26-26)
packages/go/analysis/ad/post.go (2)
packages/go/analysis/ad/wellknown/suffix.go (2)
AdministratorsSIDSuffix(83-83)RemoteDesktopUsersSIDSuffix(86-86)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(477-638)
🪛 GitHub Actions: Run Go Unit Tests
packages/go/analysis/ad/post.go
[error] 774-774: FetchRemoteDesktopUsersLocalGroupMembership called with too many arguments.
[error] Build failed due to compilation error in post.go (too many arguments in call).
🪛 golangci-lint (2.5.0)
packages/go/analysis/ad/post.go
[major] 774-774: : # github.com/specterops/bloodhound/packages/go/analysis/ad
packages/go/analysis/ad/post.go:774:112: too many arguments in call to FetchRemoteDesktopUsersLocalGroupMembership
have (graph.ID, string, *CanRDPComputerData)
want (graph.ID, *CanRDPComputerData)
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (53)
go.mod (2)
87-87: LGTM: Indirect dependency updates align with direct updates.These indirect dependency updates appear to be automatically pulled in by Go modules due to the direct dependency changes. The golang.org/x/* library updates are routine maintenance patches.
Also applies to: 276-276, 278-278, 280-281
21-21: Dependency updates verified and approved.All versions exist and are secure:
github.com/jackc/pgx/v5v5.8.0 includes patches for CVE-2024-27289 and CVE-2024-27304github.com/specterops/dawgsv0.4.1 (minor bump from v0.3.2) has no known security advisories- Remaining updates (
cuelang.org/go,golang.org/x/*,github.com/axiomhq/hyperloglog) all verified with no advisoriesThe
specterops/dawgsminor version update is appropriate for supporting the local group post-processing refactor mentioned in the PR.packages/go/analysis/ad/adcscache.go (1)
30-30: LGTM! Clean timing instrumentation addition.The deferred timing wrapper pattern is idiomatic and correctly uses double parentheses to invoke the returned cleanup function. This provides consistent duration logging for cache building operations.
Also applies to: 76-77
packages/go/analysis/ad/membership.go (1)
92-104: LGTM! Clean simplification.Calling
direction.Reverse()directly eliminates the unnecessary intermediate variable and related error handling, making the code more concise.cmd/api/src/analysis/post_integration_test.go (2)
79-93: LGTM! Test correctly adapted to new LocalGroupData API.The test properly exercises the new two-step flow:
FetchLocalGroupDatafollowed byCalculateCrossProductNodeSets. Therequire.Nilfprovides better error context on failure.
96-145: Remaining cross-product tests follow consistent pattern.All four cross-product tests (
TestCrossProduct,TestCrossProductAuthUsers,TestCrossProductEveryone,TestCrossProductEveryone2) correctly implement the new LocalGroupData workflow with consistent error handling and assertions.cmd/api/src/analysis/ad/post.go (1)
29-67: Comprehensive refactor to LocalGroupData-based workflow.The Post function is now properly wired with the new LocalGroupData abstraction replacing the previous groupExpansions/PathAggregator approach. Key observations:
- All processing steps correctly receive
localGroupData- The
adcsCacheis properly threaded fromPostADCStoPostNTLM- All enabled flags (
adcsEnabled,citrixEnabled,ntlmEnabled) andcompositionCounterare correctly utilized- Stats aggregation is complete for all processing steps
The fail-fast error handling with early returns is appropriate for this sequential processing pipeline.
packages/go/analysis/ad/owns.go (3)
24-24: LGTM - Import added for algo package.The new import supports the
*algo.ReachabilityCachetype used inFetchAdminGroupIds.
38-48: LGTM - Function signature updated to use LocalGroupData.The migration from direct
groupExpansionsparameter tolocalGroupData *LocalGroupDatais consistent with the broader refactor. TheGroupMembershipCacheis correctly extracted fromlocalGroupDatawhen callingFetchAdminGroupIds.
196-214: No action required — direction is correct.
DirectionInboundis the correct direction for expanding group membership. In AD graph semantics, "MemberOf" edges point from members to groups, so following DirectionInbound on a group correctly identifies all its members. This pattern is consistent across the codebase (ad.go, ntlm.go, esc_shared.go) and validated by integration tests that confirmReachExclusive(groupID, DirectionInbound)returns the group's members.Likely an incorrect or invalid review comment.
packages/go/analysis/ad/esc3.go (3)
38-38: LGTM - Function signature updated for LocalGroupData.The parameter change from
groupExpansions impact.PathAggregatortolocalGroupData *LocalGroupDataaligns with the broader refactor across ADCS ESC functions.
91-97: LGTM - CalculateCrossProductNodeSets call updated.The call correctly passes
localGroupDataas the first parameter, with the remaining enrollers/agents parameters preserved.
109-114: LGTM - Non-delegated path CalculateCrossProductNodeSets call updated.Consistent with the delegated agents path above, this call correctly passes
localGroupDataas the first parameter.packages/go/analysis/ad/esc4.go (6)
35-35: LGTM - Function signature updated for LocalGroupData.The parameter change aligns with the broader ESC function refactor pattern.
63-67: LGTM - 2a scenario: Controllers cross-product updated.The call correctly passes
localGroupDatafor principals controlling the cert template.
71-77: LGTM - 2b scenario: Generic Write + Enroll cross-product updated.Consistent parameter usage for the enrollment rights combination check.
88-94: LGTM - 2d scenario: PKI flags cross-product updated.The four-way cross-product for PKI flag combinations is correctly updated.
99-105: LGTM - 2e scenario: EnrolleeSuppliesSubject path updated.Correctly passes
localGroupDatawhenenrolleeSuppliesSubjectis true.
111-117: LGTM - 2f scenario: Manager approval path updated.Correctly passes
localGroupDatawhenrequiresManagerApprovalis false.packages/go/analysis/ad/esc10.go (4)
36-36: LGTM - PostADCSESC10a signature updated.The parameter change from
groupExpansions impact.PathAggregatortolocalGroupData *LocalGroupDatais consistent with other ESC functions.
54-54: LGTM - getVictimBitmap call updated in ESC10a.The call correctly passes
localGroupDataas the first parameter for victim bitmap construction.
84-84: LGTM - PostADCSESC10b signature updated.Consistent with PostADCSESC10a's parameter change.
102-102: LGTM - getVictimBitmap call updated in ESC10b.Consistent parameter usage with ESC10a.
packages/go/analysis/ad/esc_shared.go (2)
30-30: LGTM - Import added for algo package.Supports the
*algo.ReachabilityCachetype used inexpandNodeSliceToBitmapWithoutGroups.
394-399: LGTM - getVictimBitmap updated to use LocalGroupData.The function now correctly extracts
GroupMembershipCachefromlocalGroupDatafor both template and ECA bitmap expansions.packages/go/analysis/ad/adcs.go (3)
36-36: LGTM! Signature updated to use LocalGroupData.The function signature change from
groupExpansions impact.PathAggregatortolocalGroupData *LocalGroupDataaligns with the broader refactor replacing PathAggregator-based expansion with LocalGroupData-driven logic.
109-109: LGTM! Internal helper signature updated consistently.The
processEnterpriseCAWithValidCertChainToDomainsignature change mirrors the parent function and correctly propagateslocalGroupDatato all downstreamPostADCSESC*calls.
119-207: LGTM! All PostADCSESC calls consistently updated.*All ESC post-processing calls (ESC1, ESC3, ESC4, ESC6a, ESC6b, ESC9a, ESC9b, ESC10a, ESC10b, ESC13) correctly receive
localGroupDatainstead of the previousgroupExpansionsparameter.packages/go/analysis/ad/local_groups.go (5)
32-43: LGTM! Well-structured parallel processing setup.The function initializes proper synchronization primitives (WaitGroups, channels) for a multi-stage pipeline: work distribution → computer data fetching → CanRDP bitmap computation → relationship posting.
73-73: Good fix for the zero-worker edge case.Using
runtime.NumCPU()/2+1ensures at least one worker is always spawned, preventing potential hangs on single-core systems.
164-190: LGTM! Clean setup for PostLocalGroups.The function properly defines group suffix constants and initializes the multi-stage pipeline with appropriate synchronization primitives.
221-221: Good fix for the zero-worker edge case (consistent with PostCanRDP).Using
runtime.NumCPU()/2+1ensures at least one worker is spawned.
263-314: Database transaction errors are now logged.Line 312 uses
util.SLogErrorto log transaction errors, addressing the concern from the past review about silently ignoring errors.packages/go/analysis/ad/esc6.go (4)
37-37: LGTM! Signature updated to use LocalGroupData.The function signature change aligns with the broader refactor. The implementation correctly uses
localGroupDatafor theCalculateCrossProductNodeSetscall.
59-59: LGTM! CalculateCrossProductNodeSets call updated.The call correctly passes
localGroupDataas the first argument, aligning with the updated function signature inad.go.
82-82: LGTM! PostADCSESC6b signature updated consistently.The signature change mirrors PostADCSESC6a for consistency across the ESC6 implementation.
104-104: LGTM! CalculateCrossProductNodeSets call updated for ESC6b.Consistent with the ESC6a implementation.
packages/go/analysis/ad/ad.go (6)
476-481: LGTM! Signature updated to use LocalGroupData.The function signature change from
groupExpansions impact.PathAggregatortolocalGroupData *LocalGroupDataaligns with the broader refactor. The function maintains the same contract of requiring at least 2 node slices.
499-543: LGTM! Unrolling logic refactored to use LocalGroupData.The logic correctly:
- Uses
localGroupData.ExcludedShortcutGroupsto check if nodes should be excluded (Auth Users/Everyone)- Uses
localGroupData.GroupMembershipCache.Reach()to get inbound reachability for groups- Checks if any reachable entities are in the excluded set
- Properly handles the exclusion flag to skip entire node sets when Auth Users/Everyone is found
554-564: LGTM! Simplified handling for edge cases.The logic correctly handles:
- When all node sets contain Auth Users/Everyone: return all first-degree principals
- When only one node set remains without Auth Users/Everyone: return that set directly
567-575: LGTM! Reference set population updated.Uses
localGroupData.GroupMembershipCache.OrReachabilityto populate the unrolled reference set, maintaining equivalent semantics to the previous PathAggregator-based approach.
579-597: LGTM! Cardinality-based sorting refactored.The temporary bitmap approach correctly:
- Uses
GroupMembershipCache.OrReachabilityto compute cardinality- Clears the bitmap after each use to avoid accumulation
- Builds a map of group IDs to their cardinalities for sorting
611-627: LGTM! Final cross-product logic updated.Uses
localGroupData.GroupMembershipCache.XorReachabilityfor efficient set difference operations when processing shortcut groups.packages/go/analysis/ad/ntlm.go (8)
41-47: LGTM! NTLMCache struct updated to use LocalGroupData.The struct field change from
GroupExpansions impact.PathAggregatortoLocalGroupData *LocalGroupDataaligns with the broader refactor.
64-64: LGTM! NewNTLMCache signature updated.The function now accepts
localGroupData *LocalGroupDataand stores it in the cache for use by downstream operations.
75-75: LGTM! FetchProtectedUsersMappedToDomains call updated.Correctly passes
localGroupData.GroupMembershipCacheto the updated function.
128-128: LGTM! PostNTLM signature updated.The function now accepts
localGroupData *LocalGroupDataand passes it through toNewNTLMCache.
443-444: LGTM! Bitmap expansion updated to use LocalGroupData.The
expandNodeSliceToBitmapWithoutGroupscalls now usentlmCache.LocalGroupData.GroupMembershipCacheinstead of the previous GroupExpansions.
586-593: LGTM! PostCoerceAndRelayNTLMToSMB updated.The admin principal expansion now uses
ntlmCache.LocalGroupData.GroupMembershipCache.OrReachabilityfor group membership unrolling.
856-856: LGTM! FetchProtectedUsersMappedToDomains signature updated.The function now accepts
*algo.ReachabilityCachedirectly instead of the full PathAggregator, providing a more focused interface.
869-871: LGTM! Protected users bitmap population updated.Uses
groupExpansions.OrReachabilityto populate the protected users bitmap, maintaining equivalent semantics.packages/go/analysis/ad/post.go (2)
780-811: LGTM - Clear two-stage RDP entity resolution.The implementation correctly handles both direct RIL entities with RDP group membership and members of expanded RIL groups. The comments clearly explain each stage of processing.
527-555: LGTM - Well-structured RDP entity resolution with clear branching.The logic correctly handles the various combinations of URA enforcement, Citrix enablement, and RDP group processing. The early exit when DAU group is missing is a good optimization.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| func FetchComputerLocalGroupBySIDSuffix(tx graph.Transaction, computer graph.ID, groupSuffix string) (*graph.Node, error) { | ||
| if rel, err := tx.Relationships().Filter(query.And( | ||
| query.StringEndsWith(query.StartProperty(common.ObjectID.String()), groupSuffix), | ||
| query.Kind(query.Relationship(), ad.LocalToComputer), | ||
| query.InIDs(query.EndID(), computer), | ||
| )).First(); err != nil { | ||
| return nil, err | ||
| } else { | ||
| return ops.FetchNode(tx, rel.StartID) | ||
| } | ||
| var ( | ||
| groupNode graph.Node | ||
| queryErr = tx.Relationships().Filter(query.And( | ||
| query.StringEndsWith(query.StartProperty(common.ObjectID.String()), groupSuffix), | ||
| query.Kind(query.Relationship(), ad.LocalToComputer), | ||
| query.InIDs(query.EndID(), computer), | ||
| )).Query( | ||
| func(results graph.Result) error { | ||
| if results.Next() { | ||
| if err := results.Scan(&groupNode); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| results.Close() | ||
| return results.Error() | ||
| }, | ||
| query.Returning( | ||
| query.Start(), | ||
| ), | ||
| ) | ||
| ) | ||
|
|
||
| return &groupNode, queryErr | ||
| } |
There was a problem hiding this comment.
Missing "not found" error when query returns no results.
When results.Next() returns false on the first call, groupNode remains a zero-value and the function returns &groupNode, nil. Callers (e.g., line 743) don't check for a zero-value node, which could cause downstream issues when accessing RemoteDesktopUsersLocalGroup.ID.
Proposed fix - return error when no results found
func FetchComputerLocalGroupBySIDSuffix(tx graph.Transaction, computer graph.ID, groupSuffix string) (*graph.Node, error) {
var (
- groupNode graph.Node
- queryErr = tx.Relationships().Filter(query.And(
+ groupNode *graph.Node
+ queryErr = tx.Relationships().Filter(query.And(
query.StringEndsWith(query.StartProperty(common.ObjectID.String()), groupSuffix),
query.Kind(query.Relationship(), ad.LocalToComputer),
query.InIDs(query.EndID(), computer),
)).Query(
func(results graph.Result) error {
if results.Next() {
- if err := results.Scan(&groupNode); err != nil {
+ var node graph.Node
+ if err := results.Scan(&node); err != nil {
return err
}
+ groupNode = &node
}
results.Close()
return results.Error()
},
query.Returning(
query.Start(),
),
)
)
- return &groupNode, queryErr
+ if queryErr != nil {
+ return nil, queryErr
+ }
+ if groupNode == nil {
+ return nil, graph.ErrNoResultsFound
+ }
+ return groupNode, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func FetchComputerLocalGroupBySIDSuffix(tx graph.Transaction, computer graph.ID, groupSuffix string) (*graph.Node, error) { | |
| if rel, err := tx.Relationships().Filter(query.And( | |
| query.StringEndsWith(query.StartProperty(common.ObjectID.String()), groupSuffix), | |
| query.Kind(query.Relationship(), ad.LocalToComputer), | |
| query.InIDs(query.EndID(), computer), | |
| )).First(); err != nil { | |
| return nil, err | |
| } else { | |
| return ops.FetchNode(tx, rel.StartID) | |
| } | |
| var ( | |
| groupNode graph.Node | |
| queryErr = tx.Relationships().Filter(query.And( | |
| query.StringEndsWith(query.StartProperty(common.ObjectID.String()), groupSuffix), | |
| query.Kind(query.Relationship(), ad.LocalToComputer), | |
| query.InIDs(query.EndID(), computer), | |
| )).Query( | |
| func(results graph.Result) error { | |
| if results.Next() { | |
| if err := results.Scan(&groupNode); err != nil { | |
| return err | |
| } | |
| } | |
| results.Close() | |
| return results.Error() | |
| }, | |
| query.Returning( | |
| query.Start(), | |
| ), | |
| ) | |
| ) | |
| return &groupNode, queryErr | |
| } | |
| func FetchComputerLocalGroupBySIDSuffix(tx graph.Transaction, computer graph.ID, groupSuffix string) (*graph.Node, error) { | |
| var ( | |
| groupNode *graph.Node | |
| queryErr = tx.Relationships().Filter(query.And( | |
| query.StringEndsWith(query.StartProperty(common.ObjectID.String()), groupSuffix), | |
| query.Kind(query.Relationship(), ad.LocalToComputer), | |
| query.InIDs(query.EndID(), computer), | |
| )).Query( | |
| func(results graph.Result) error { | |
| if results.Next() { | |
| var node graph.Node | |
| if err := results.Scan(&node); err != nil { | |
| return err | |
| } | |
| groupNode = &node | |
| } | |
| results.Close() | |
| return results.Error() | |
| }, | |
| query.Returning( | |
| query.Start(), | |
| ), | |
| ) | |
| ) | |
| if queryErr != nil { | |
| return nil, queryErr | |
| } | |
| if groupNode == nil { | |
| return nil, graph.ErrNoResultsFound | |
| } | |
| return groupNode, nil | |
| } |
🧰 Tools
🪛 GitHub Actions: Run Go Unit Tests
[error] Build failed due to compilation error in post.go (too many arguments in call).
| func FetchComputerLocalGroupIDBySIDSuffix(tx graph.Transaction, computer graph.ID, groupSuffix string) (graph.ID, error) { | ||
| var ( | ||
| startID graph.ID | ||
| queryErr = tx.Relationships().Filter(query.And( | ||
| query.StringEndsWith(query.StartProperty(common.ObjectID.String()), groupSuffix), | ||
| query.Kind(query.Relationship(), ad.LocalToComputer), | ||
| query.InIDs(query.EndID(), computer), | ||
| )).Query( | ||
| func(results graph.Result) error { | ||
| if results.Next() { | ||
| if err := results.Scan(&startID); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| results.Close() | ||
| return results.Error() | ||
| }, | ||
| query.Returning( | ||
| query.StartID(), | ||
| ), | ||
| ) | ||
| ) | ||
|
|
||
| return startID, queryErr | ||
| } |
There was a problem hiding this comment.
Similar issue: returns zero ID when no results found.
Same pattern as FetchComputerLocalGroupBySIDSuffix - when no results are found, a zero graph.ID is returned without an error. Consider returning graph.ErrNoResultsFound when no matching local group exists.
🧰 Tools
🪛 GitHub Actions: Run Go Unit Tests
[error] Build failed due to compilation error in post.go (too many arguments in call).
🤖 Prompt for AI Agents
In `@packages/go/analysis/ad/post.go` around lines 452 - 477,
FetchComputerLocalGroupIDBySIDSuffix currently returns a zero graph.ID when no
rows match; update the query callback (or post-query handling) to return
graph.ErrNoResultsFound instead of silently returning nil error when
results.Next() is false — e.g., inside the anonymous Query callback for
FetchComputerLocalGroupIDBySIDSuffix, if results.Next() is false return
graph.ErrNoResultsFound (or set queryErr accordingly) so callers receive a clear
no-results error similar to FetchComputerLocalGroupBySIDSuffix.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/api/src/daemons/datapipe/datapipe.go (1)
76-103: Dead code after early return.All code from line 76 onwards is unreachable due to the unconditional
returnon line 74. This includes:
- Timer initialization (lines 76-79)
- Deferred cleanup (lines 81-82)
- The
pipeline.Startcall (line 84)- The entire daemon loop handling pruning, deletion, ingestion, and periodic analysis (lines 86-103)
If the early return was intentional for testing, this dead code should be removed. If the early return was accidental, remove lines 73-75 instead to restore normal daemon operation.
packages/go/analysis/ad/esc_shared.go (1)
309-331: Misleading function name:expandNodeSliceToBitmapWithoutGroupsincludes groups.The function name suggests groups are excluded, but the group detection logic (lines 316-320) is commented out, causing
isGroupto always befalse. This means all expanded members—including groups—are added to the bitmap.While the caller in
getVictimBitmapexplicitly expects this behavior ("we don't do group shortcutting here"), the function name is misleading. This creates a maintenance risk: future developers may incorrectly assume the function excludes groups based on its name alone.Either rename the function to reflect that it includes groups, or implement the group filtering logic properly.
packages/go/analysis/ad/post.go (1)
209-226: Usequery.NodeProperty()instead ofquery.StartProperty()andquery.EndProperty()for node queries.
FetchAdminGroupsqueriestx.Nodes()but usesquery.StartProperty()andquery.EndProperty()in the filter criteria. These property accessors are only for relationship queries, not node queries. Usequery.NodeProperty()instead, and remove the unnecessaryORlogic since a node has a single ObjectID, not start/end variants. This pattern is used correctly throughout the codebase (e.g., inad.goandagt.go).Proposed fix
func FetchAdminGroups(ctx context.Context, db graph.Database) (cardinality.Duplex[uint64], error) { computerNodeIds := cardinality.NewBitmap64() return computerNodeIds, db.ReadTransaction(ctx, func(tx graph.Transaction) error { return tx.Nodes().Filter(query.And( - query.Or( - query.StringEndsWith(query.StartProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), - query.StringEndsWith(query.EndProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), - ), + query.StringEndsWith(query.NodeProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), )).FetchIDs(func(cursor graph.Cursor[graph.ID]) error { for id := range cursor.Chan() { computerNodeIds.Add(id.Uint64()) } return nil }) }) }
🤖 Fix all issues with AI agents
In `@cmd/api/src/analysis/ad/post.go`:
- Around line 32-52: Tests call removed/renamed APIs: remove any invocation of
adAnalysis.ExpandAllRDPLocalGroups and update calls to
adAnalysis.FetchCanRDPEntityBitmapForComputer to the new signature by
constructing a CanRDPComputerData value (populate fields that used to be passed:
tx/computerID/groupExpansions) and calling
FetchCanRDPEntityBitmapForComputer(computerData, enforceURA, citrixEnabled);
adjust test setup/mocks to provide the CanRDPComputerData instance and the two
boolean flags rather than the old parameter set.
In `@cmd/api/src/daemons/datapipe/datapipe.go`:
- Around line 73-75: The early return after calling s.WithDatapipeStatus(ctx,
model.DatapipeStatusAnalyzing, s.pipeline.Analyze) short-circuits the daemon so
the main loop (pruning, ingestion, periodic analysis, context cancellation
handling) never runs; remove that return and restore the surrounding daemon loop
so the call to WithDatapipeStatus/Analyze runs as part of the periodic/looped
workflow (or gate it behind a test flag), ensuring the loop that performs
pruning, ingestion and context.Done handling continues to execute.
In `@packages/go/analysis/ad/local_groups.go`:
- Around line 32-42: Tests use removed/changed RDP APIs: update integration
tests in cmd/api/src/analysis/analysis_integration_test.go to use the new
signatures by removing any calls to ExpandAllRDPLocalGroups and replacing
FetchCanRDPEntityBitmapForComputer(tx, computerID, groupExpansions, enforceURA,
citrixEnabled) with the new call FetchCanRDPEntityBitmapForComputer(computerData
*CanRDPComputerData, enforceURA bool, citrixEnabled bool); construct or obtain a
*CanRDPComputerData for the computer under test (populating fields previously
provided by groupExpansions/tx) and pass enforceURA/citrix flags as before, then
adjust assertions to reflect any difference in returned bitmap semantics after
removing ExpandAllRDPLocalGroups.
In `@packages/go/analysis/ad/post.go`:
- Around line 535-563: The test calls still use the old signature passing (tx,
computerID, groupExpansions, enforceURA, citrixEnabled); update each of the
seven callers in cmd/api/src/analysis/analysis_integration_test.go to build a
CanRDPComputerData instance (populating fields used by
FetchCanRDPEntityBitmapForComputer such as Computer, CanRDPData,
RemoteDesktopUsersLocalGroup, DAUGroup, LocalGroupData,
RemoteInteractiveLogonRightEntities and any helper state like ComputersWithURA)
and then call FetchCanRDPEntityBitmapForComputer(computerData, enforceURA,
citrixEnabled) with the two boolean flags; ensure helper test setup that
previously produced tx/computerID/groupExpansions now returns or is converted
into the matching CanRDPComputerData structure so callers match the new
signature.
♻️ Duplicate comments (2)
packages/go/analysis/ad/post.go (2)
456-485: Similar issue: Returns zero ID when no results found.Same pattern as
FetchComputerLocalGroupBySIDSuffix- when no results are found, a zerograph.IDis returned without an error. Consider returninggraph.ErrNoResultsFoundwhen no matching local group exists.Proposed fix
func FetchComputerLocalGroupIDBySIDSuffix(tx graph.Transaction, computer graph.ID, groupSuffix string) (graph.ID, error) { var ( - startID graph.ID + startID graph.ID + found bool err = tx.Relationships().Filter(query.And( // ... query unchanged ... )).Query( func(results graph.Result) error { if results.Next() { if err := results.Scan(&startID); err != nil { return err } + found = true } results.Close() return results.Error() }, // ... returning unchanged ... ) ) if err != nil { return 0, err } + if !found { + return 0, graph.ErrNoResultsFound + } return startID, nil }
425-454: Bug: Returns zero-value node when no results found.When the query returns no results,
groupNoderemains a zero-valuegraph.Node, and&groupNodeis returned withnilerror. Callers (e.g., line 751 inComputerData) don't check for a zero-value node, which could cause incorrect behavior when accessingRemoteDesktopUsersLocalGroup.ID.Proposed fix - return error when no results found
func FetchComputerLocalGroupBySIDSuffix(tx graph.Transaction, computer graph.ID, groupSuffix string) (*graph.Node, error) { var ( - groupNode graph.Node + groupNode *graph.Node err = tx.Relationships().Filter(query.And( query.StringEndsWith(query.StartProperty(common.ObjectID.String()), groupSuffix), query.Kind(query.Relationship(), ad.LocalToComputer), query.InIDs(query.EndID(), computer), )).Query( func(results graph.Result) error { if results.Next() { - if err := results.Scan(&groupNode); err != nil { + var node graph.Node + if err := results.Scan(&node); err != nil { return err } + groupNode = &node } results.Close() return results.Error() }, query.Returning( query.Start(), ), ) ) if err != nil { return nil, err } + if groupNode == nil { + return nil, graph.ErrNoResultsFound + } - return &groupNode, nil + return groupNode, nil }
🧹 Nitpick comments (6)
packages/go/analysis/ad/local_groups.go (1)
274-308: Consider logging unexpected errors from group lookups.The pattern of checking
err == nilbefore proceeding is reasonable when the group may not exist. However, this silently ignores all errors, including unexpected ones (e.g., database connection issues).Consider distinguishing between "group not found" (expected) and other errors (unexpected):
♻️ Suggested improvement
- if localAdminGroupErr == nil { + if localAdminGroupErr == nil { numGroupsProcessed.Add(1) channels.Submit(ctx, reachC, reachJob{ targetComputer: computerID, targetGroup: localAdminGroup.Uint64(), groupSuffix: adminGroupSuffix, }) + } else if !graph.IsErrNotFound(localAdminGroupErr) { + slog.Warn("Unexpected error fetching admin group", slog.String("err", localAdminGroupErr.Error())) }cmd/api/src/analysis/post_integration_test.go (1)
88-91: Consider renamingexcludedGroupstolocalGroupDatafor clarity.The variable is named
excludedGroups, butFetchLocalGroupDatareturns a*LocalGroupDatastruct containing multiple fields (Computers, GroupMembershipCache, ExcludedShortcutGroups, LocalGroupMembershipDigraph). The current name only reflects one of those fields.♻️ Suggested rename
- excludedGroups, err := ad2.FetchLocalGroupData(context.Background(), graphDB) + localGroupData, err := ad2.FetchLocalGroupData(context.Background(), graphDB) require.Nilf(t, err, "%v", err) - results := ad2.CalculateCrossProductNodeSets(excludedGroups, firstSet, secondSet) + results := ad2.CalculateCrossProductNodeSets(localGroupData, firstSet, secondSet)This same rename should be applied consistently across all four test functions (Lines 88-91, 105-108, 122-125, 139-142).
packages/go/analysis/ad/ntlm.go (1)
64-83: Consider adding nil check forlocalGroupDataparameter.If
localGroupDatais nil, dereferencinglocalGroupData.GroupMembershipCacheon line 75 will cause a panic. While callers should provide a valid pointer, a defensive nil check at the function entry would improve robustness.Proposed defensive check
func NewNTLMCache(ctx context.Context, db graph.Database, localGroupData *LocalGroupData) (NTLMCache, error) { + if localGroupData == nil { + return NTLMCache{}, fmt.Errorf("localGroupData cannot be nil") + } + var ( ntlmCache = NTLMCache{} unprotectedComputerCache = make(map[string]cardinality.Duplex[uint64])packages/go/analysis/ad/post.go (2)
592-627: Minor redundancy in property check.The query already filters with
query.Equals(query.NodeProperty(ad.HasURA.String()), true)at line 601, but line 612 again checksif propertyValuebefore adding to the bitmap. This double-check is unnecessary since the filter guarantees onlytruevalues reach this point.Simplified version
if err := tx.Nodes().Filter( query.And( query.Kind(query.Node(), ad.Computer), query.Equals(query.NodeProperty(ad.HasURA.String()), true), ), ).Query(func(results graph.Result) error { for results.Next() { var ( - nodeID graph.ID - propertyValue bool + nodeID graph.ID ) - if err := results.Scan(&nodeID, &propertyValue); err != nil { + if err := results.Scan(&nodeID); err != nil { return err - } else if propertyValue { - nodesWithURA.Add(nodeID.Uint64()) } + nodesWithURA.Add(nodeID.Uint64()) } return results.Error() }, query.Returning( query.NodeID(), - query.NodeProperty(ad.HasURA.String()), )); err != nil {
629-670: LGTM! Consider documentingExcludedShortcutGroups.The
LocalGroupDatastructure andFetchLocalGroupDataimplementation are well-designed. The TODO comment on line 632 forExcludedShortcutGroupsshould be addressed in a follow-up.Would you like me to open an issue to track the documentation of
ExcludedShortcutGroups?go.mod (1)
87-87: LGTM: Indirect dependency updates.The indirect dependency updates appear reasonable and are automatically managed by the Go module system. The golang.org/x package pseudo-versions are recent (early January 2026) and align with typical Go toolchain updates.
For additional assurance, you may optionally verify there are no security advisories for the golang.org/x packages:
#!/bin/bash # Optional: Check golang.org/x packages for vulnerabilities echo "=== Checking golang.org/x packages ===" for pkg in "golang.org/x/net" "golang.org/x/sys" "golang.org/x/exp" "golang.org/x/crypto"; do echo "--- $pkg ---" gh api graphql -f query=" { securityVulnerabilities(first: 3, ecosystem: GO, package: \"$pkg\") { nodes { advisory { summary severity } vulnerableVersionRange } } }" echo doneAlso applies to: 276-276, 278-278, 280-280, 281-281
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
cmd/api/src/analysis/ad/post.gocmd/api/src/analysis/membership_integration_test.gocmd/api/src/analysis/post_integration_test.gocmd/api/src/daemons/datapipe/datapipe.gocmd/api/src/test/integration/harnesses.gogo.modpackages/go/analysis/ad/ad.gopackages/go/analysis/ad/adcs.gopackages/go/analysis/ad/adcscache.gopackages/go/analysis/ad/esc1.gopackages/go/analysis/ad/esc10.gopackages/go/analysis/ad/esc13.gopackages/go/analysis/ad/esc3.gopackages/go/analysis/ad/esc4.gopackages/go/analysis/ad/esc6.gopackages/go/analysis/ad/esc9.gopackages/go/analysis/ad/esc_shared.gopackages/go/analysis/ad/local_groups.gopackages/go/analysis/ad/membership.gopackages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/owns.gopackages/go/analysis/ad/post.go
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/go/analysis/ad/esc1.go
- cmd/api/src/test/integration/harnesses.go
- packages/go/analysis/ad/esc3.go
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/analysis/post_integration_test.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
packages/go/analysis/ad/ad.gopackages/go/analysis/ad/esc13.gogo.modpackages/go/analysis/ad/esc_shared.gopackages/go/analysis/ad/adcs.gopackages/go/analysis/ad/esc6.gopackages/go/analysis/ad/esc10.gocmd/api/src/analysis/membership_integration_test.gopackages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/esc9.gopackages/go/analysis/ad/esc4.gopackages/go/analysis/ad/owns.go
📚 Learning: 2025-06-11T20:49:35.177Z
Learnt from: kpom-specter
Repo: SpecterOps/BloodHound PR: 1562
File: cmd/api/src/daemons/datapipe/datapipe.go:100-120
Timestamp: 2025-06-11T20:49:35.177Z
Learning: In BloodHound's datapipe architecture, only the datapipe (e.g., `WithDatapipeStatus`) is responsible for mutating the datapipe status; pipeline actions themselves do not alter the status and instead communicate any job-level state elsewhere.
Applied to files:
cmd/api/src/daemons/datapipe/datapipe.go
📚 Learning: 2025-07-10T14:31:01.781Z
Learnt from: JonasBK
Repo: SpecterOps/BloodHound PR: 1671
File: packages/go/analysis/ad/esc16.go:65-69
Timestamp: 2025-07-10T14:31:01.781Z
Learning: In BloodHound ADCS ESC analysis functions (PostADCSESC1, PostADCSESC4, PostADCSESC6, PostADCSESC9, PostADCSESC10, PostADCSESC13, PostADCSESC16, etc.), the established pattern for handling cert template validation errors and user filtering errors is to use slog.WarnContext to log warnings and continue processing with the next template, rather than returning errors to the caller. This pattern is consistently applied across all ADCS ESC edge implementations.
Applied to files:
packages/go/analysis/ad/esc13.gopackages/go/analysis/ad/esc6.gopackages/go/analysis/ad/esc10.gopackages/go/analysis/ad/esc9.gocmd/api/src/analysis/ad/post.gopackages/go/analysis/ad/esc4.go
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
packages/go/analysis/ad/esc13.gopackages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/post.go
📚 Learning: 2025-06-30T22:58:54.834Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1629
File: go.mod:18-18
Timestamp: 2025-06-30T22:58:54.834Z
Learning: Go 1.24 supports patch versions in the go directive of go.mod files. Syntax like "go 1.24.4" is valid and specifies the minimum Go version required to build the module, including the patch level.
Applied to files:
go.mod
📚 Learning: 2025-08-26T18:13:36.643Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1820
File: .github/workflows/audit-release.yml:0-0
Timestamp: 2025-08-26T18:13:36.643Z
Learning: The go tool command supports custom/third-party binaries as of Go 1.24, allowing invocation of custom CLI tools like "go tool stbernard" rather than being limited to only built-in Go toolchain binaries.
Applied to files:
go.mod
📚 Learning: 2025-06-25T18:24:25.014Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T18:24:25.014Z
Learning: In BloodHound Go code, for error types in slog structured logging, prefer using slog.String("key", err.Error()) over slog.Any("key", err). The explicit string conversion with err.Error() is preferred over using slog.Any() for error types.
Applied to files:
packages/go/analysis/ad/local_groups.gopackages/go/analysis/ad/owns.go
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
packages/go/analysis/ad/local_groups.gopackages/go/analysis/ad/adcs.gopackages/go/analysis/ad/owns.go
📚 Learning: 2025-05-23T19:56:13.423Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/BloodHound PR: 1440
File: packages/go/analysis/ad/ntlm.go:123-138
Timestamp: 2025-05-23T19:56:13.423Z
Learning: In the BloodHound NTLM analysis code (packages/go/analysis/ad/ntlm.go), the `isProtectedComputer` function intentionally fails open (returns false/unprotected) when the Protected Users group cannot be retrieved, maintaining consistency with the original logic patterns in the codebase.
Applied to files:
packages/go/analysis/ad/local_groups.gopackages/go/analysis/ad/ntlm.gocmd/api/src/analysis/ad/post.gopackages/go/analysis/ad/post.go
📚 Learning: 2025-05-27T16:58:33.295Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Applied to files:
cmd/api/src/analysis/membership_integration_test.go
📚 Learning: 2025-07-10T14:33:20.317Z
Learnt from: JonasBK
Repo: SpecterOps/BloodHound PR: 1671
File: cmd/api/src/analysis/ad/adcs_integration_test.go:3687-3687
Timestamp: 2025-07-10T14:33:20.317Z
Learning: When reviewing Go code, functions defined in one file within a package are accessible from other files in the same package. Before flagging missing functions as compilation errors, check if they exist in other files within the same package directory.
Applied to files:
packages/go/analysis/ad/owns.go
🧬 Code graph analysis (11)
cmd/api/src/analysis/post_integration_test.go (2)
packages/go/analysis/ad/post.go (1)
FetchLocalGroupData(636-670)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(477-638)
packages/go/analysis/ad/ad.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(629-634)packages/go/graphschema/ad/ad.go (3)
Group(31-31)LocalGroup(36-36)Contains(54-54)
packages/go/analysis/ad/esc13.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(629-634)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(477-638)
packages/go/analysis/ad/esc_shared.go (1)
packages/go/analysis/ad/post.go (1)
LocalGroupData(629-634)
packages/go/analysis/ad/local_groups.go (2)
packages/go/analysis/ad/post.go (4)
LocalGroupData(629-634)CanRDPComputerData(728-735)FetchCanRDPData(702-708)FetchCanRDPEntityBitmapForComputer(535-563)packages/go/analysis/analysis.go (1)
MaximumDatabaseParallelWorkers(36-36)
packages/go/analysis/ad/esc6.go (3)
packages/go/analysis/ad/post.go (1)
LocalGroupData(629-634)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(477-638)
packages/go/analysis/ad/esc10.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(629-634)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)
packages/go/analysis/ad/ntlm.go (1)
packages/go/analysis/ad/post.go (1)
LocalGroupData(629-634)
packages/go/analysis/ad/esc9.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(629-634)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)
cmd/api/src/analysis/ad/post.go (1)
packages/go/analysis/ad/post.go (2)
FetchLocalGroupData(636-670)PostDCSync(74-104)
packages/go/analysis/ad/owns.go (1)
packages/go/analysis/ad/post.go (1)
LocalGroupData(629-634)
🪛 golangci-lint (2.5.0)
packages/go/analysis/ad/local_groups.go
[major] 42-42: : # github.com/specterops/bloodhound/cmd/api/src/analysis_test [github.com/specterops/bloodhound/cmd/api/src/analysis.test]
cmd/api/src/analysis/analysis_integration_test.go:42:38: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:46:130: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:65:38: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:70:129: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:97:129: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:126:37: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:131:129: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:154:38: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:159:146: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:175:146: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:175:146: too many errors
(typecheck)
cmd/api/src/analysis/ad/post.go
[major] 42-42: : # github.com/specterops/bloodhound/cmd/api/src/analysis_test [github.com/specterops/bloodhound/cmd/api/src/analysis.test]
cmd/api/src/analysis/analysis_integration_test.go:42:38: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:46:130: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:65:38: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:70:129: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:97:129: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:126:37: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:131:129: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:154:38: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:159:146: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:175:146: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:175:146: too many errors
(typecheck)
packages/go/analysis/ad/owns.go
[major] 42-42: : # github.com/specterops/bloodhound/cmd/api/src/analysis_test [github.com/specterops/bloodhound/cmd/api/src/analysis.test]
cmd/api/src/analysis/analysis_integration_test.go:42:38: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:46:130: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:65:38: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:70:129: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:97:129: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:126:37: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:131:129: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:154:38: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:159:146: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:175:146: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:175:146: too many errors
(typecheck)
packages/go/analysis/ad/post.go
[major] 42-42: : # github.com/specterops/bloodhound/cmd/api/src/analysis_test [github.com/specterops/bloodhound/cmd/api/src/analysis.test]
cmd/api/src/analysis/analysis_integration_test.go:42:38: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:46:130: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:65:38: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:70:129: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:97:129: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:126:37: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:131:129: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:154:38: undefined: adAnalysis.ExpandAllRDPLocalGroups
cmd/api/src/analysis/analysis_integration_test.go:159:146: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:175:146: too many arguments in call to adAnalysis.FetchCanRDPEntityBitmapForComputer
have (graph.Transaction, graph.ID, unknown type, bool, bool)
want ("github.com/specterops/bloodhound/packages/go/analysis/ad".CanRDPComputerData, bool, bool)
cmd/api/src/analysis/analysis_integration_test.go:175:146: too many errors
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-go-unit-tests
🔇 Additional comments (44)
packages/go/analysis/ad/membership.go (1)
92-94: LGTM!Clean simplification using
direction.Reverse()directly instead of a separate variable. The logic remains equivalent and the code is more concise.packages/go/analysis/ad/esc_shared.go (1)
394-419: LGTM on signature update.The function signature change to accept
*LocalGroupDataaligns with the broader refactor. The internal logic correctly useslocalGroupData.GroupMembershipCachefor the bitmap expansions.packages/go/analysis/ad/local_groups.go (1)
32-166: Well-structured concurrent processing pipeline.The function correctly addresses previous concerns:
- Worker count uses
runtime.NumCPU()/2+1ensuring at least one worker (line 81)- Channel closing is properly sequenced after all producers finish
- Database transaction errors are now logged (lines 146-148)
- Error from
FetchCanRDPDatais checked before starting goroutines, avoiding orphaned channel readersThe three-stage pipeline (work fetchers → computer processors → batch writer) with proper wait group coordination is clean.
packages/go/analysis/ad/adcscache.go (1)
75-77: LGTM!Good addition of timing instrumentation using
util.SLogMeasureFunction. The deferred call pattern correctly measures the entireBuildCacheexecution time.cmd/api/src/analysis/membership_integration_test.go (1)
77-92: LGTM!The test correctly adapts to the new
algo.FetchReachabilityCacheAPI:
- Uses
query.KindIn(query.Relationship(), ad.MemberOf, ad.MemberOfLocalGroup)to filter relevant relationship types- Assertions updated to use
ReachExclusive(..., graph.DirectionInbound).Cardinality()which returns the count of nodes that can reach each groupThe expected cardinalities remain unchanged, validating the new implementation maintains behavioral parity.
packages/go/analysis/ad/esc13.go (2)
36-36: LGTM - Function signature updated to use LocalGroupData.The signature change from
groupExpansions impact.PathAggregatortolocalGroupData *LocalGroupDataaligns with the broader refactoring to use a unified data carrier for cross-product calculations.
53-53: LGTM - CalculateCrossProductNodeSets call updated correctly.The call site properly passes
localGroupDataas the first argument, consistent with the new function signature inad.go. The remaining arguments (ecaEnrollers,cache.GetCertTemplateEnrollers(template.ID)) are unchanged, preserving the cross-product logic.cmd/api/src/analysis/post_integration_test.go (1)
84-93: LGTM - Test properly migrated to new LocalGroupData flow.The test correctly:
- Uses the updated callback signature with
graphDB- Fetches local group data before cross-product calculation
- Passes the data to
CalculateCrossProductNodeSets- Validates results with appropriate assertions
packages/go/analysis/ad/esc4.go (2)
35-35: LGTM - Function signature updated to use LocalGroupData.The signature change from
groupExpansions impact.PathAggregatortolocalGroupData *LocalGroupDatais consistent with the pattern applied across all ESC post-processing functions.
62-117: LGTM - All CalculateCrossProductNodeSets call sites correctly updated.All five cross-product calculations (2a, 2b, 2d, 2e, 2f) properly pass
localGroupDataas the first argument while preserving the remaining node set arguments. The logic flow and OR operations withprincipalsbitmap remain unchanged.cmd/api/src/analysis/ad/post.go (1)
29-67: LGTM - Post-processing pipeline properly refactored to use LocalGroupData.The orchestration flow is well-structured:
- Deletes transit edges first
- Fetches
localGroupDataonce and reuses it across all subsequent Post* calls- Sequential execution with proper error propagation at each step
- Stats aggregation at the end
This addresses the previous review concern about disabled post-processing - the pipeline now executes all steps correctly.
packages/go/analysis/ad/owns.go (2)
38-45: LGTM - PostOwnsAndWriteOwner properly migrated to LocalGroupData.The function correctly:
- Accepts
localGroupData *LocalGroupDataparameter- Extracts
GroupMembershipCacheand passes it toFetchAdminGroupIdsThis maintains the separation of concerns while adapting to the new data carrier pattern.
196-213: LGTM - FetchAdminGroupIds refactored to use ReachabilityCache.The function now:
- Accepts
*algo.ReachabilityCacheinstead of the previous PathAggregator- Uses
OrReachabilityto accumulate group members intoadminIdsThe
OrReachabilitycall on line 208 correctly expands each admin group (DA/EA) by adding all principals that can reach that group via inbound membership edges, which is the intended behavior for determining admin group membership.packages/go/analysis/ad/esc9.go (2)
35-35: Signature and parameter migration look correct for ESC9a.The function signature update to accept
*LocalGroupDataand the correspondinggetVictimBitmapcall are consistent with the broader refactor. The error handling pattern usingslog.WarnContextandcontinuealigns with the established ADCS ESC pattern. Based on learnings, this follows the expected convention.Also applies to: 53-53
84-84: Signature and parameter migration look correct for ESC9b.Consistent with ESC9a changes. The
localGroupDataparameter is correctly propagated togetVictimBitmap.Also applies to: 102-102
packages/go/analysis/ad/esc10.go (2)
36-36: Signature and parameter migration look correct for ESC10a.The migration from
groupExpansionstolocalGroupDatais correctly implemented. ThegetVictimBitmapcall properly receives the new parameter.Also applies to: 54-54
84-84: Signature and parameter migration look correct for ESC10b.Consistent with ESC10a changes and the broader LocalGroupData refactor pattern.
Also applies to: 102-102
packages/go/analysis/ad/ad.go (5)
500-543: Unrolling logic correctly migrated to LocalGroupData.The refactored code:
- Uses
localGroupData.ExcludedShortcutGroups.Contains()to check for Auth Users/Everyone directly- Retrieves group membership via
localGroupData.GroupMembershipCache.Reach()- Correctly breaks out of entity processing when an excluded group is found in the reachability set
The nested iteration at lines 523-529 efficiently short-circuits when an excluded node is detected.
554-564: Edge case handling simplified and correct.The conditional structure properly handles:
- Empty
firstDegreeSets(all contain excluded groups) → return all first-degree entities- Single
firstDegreeSet→ return that set directly- Two or more sets → proceed with cross-product calculation
579-597: Cardinality mapping uses proper bitmap reuse pattern.The
tempBitmapis correctly cleared after each use (line 590) to prevent cardinality accumulation across iterations. This is a good memory-efficient pattern.
611-627: Group shortcutting logic correctly uses XorReachability.The use of
XorReachabilityat line 622 properly removes group members from the reference set when a group is matched, preventing redundant checks of already-covered entities.
477-481: All call sites ofCalculateCrossProductNodeSetshave been correctly updated with the new signature.The function no longer requires a
graph.Transactionparameter. All 22 references across production code (esc1.go, esc3.go, esc4.go, esc6.go, esc13.go, post.go) and tests (post_integration_test.go) properly passlocalGroupDataas the first parameter, which is fetched viaFetchLocalGroupData(). This decouples the cross-product calculation from database access, improving the design.packages/go/analysis/ad/adcs.go (3)
36-36: PostADCS entry point correctly updated.The signature change to accept
*LocalGroupDatainstead ofimpact.PathAggregatoraligns with the broader refactor. The function correctly passes this to downstream processing.
109-109: Internal helper signature correctly updated.The
processEnterpriseCAWithValidCertChainToDomainfunction signature properly includeslocalGroupDatato propagate it to all ESC processors.
119-207: All ESC post-processing calls consistently updated.All 11 ADCS ESC processors (ESC1, ESC3, ESC4, ESC6a, ESC6b, ESC9a, ESC9b, ESC10a, ESC10b, ESC13) correctly receive
localGroupData. ThePostGoldenCertcall at line 111 appropriately does not receivelocalGroupDataas it doesn't require group expansion data.packages/go/analysis/ad/esc6.go (2)
37-37: PostADCSESC6a correctly migrated to LocalGroupData.The signature change and
CalculateCrossProductNodeSetscall are properly updated. The call now omits thetxparameter (which was removed from the function signature) and correctly passeslocalGroupDataas the first argument.Also applies to: 59-59
82-82: PostADCSESC6b correctly migrated to LocalGroupData.Consistent with ESC6a changes. The function signature and
CalculateCrossProductNodeSetscall are properly aligned with the new LocalGroupData-based API.Also applies to: 104-104
packages/go/analysis/ad/ntlm.go (6)
27-27: LGTM!Import addition is appropriate for the new
algo.ReachabilityCachetype usage.
41-47: LGTM!The struct field change from
GroupExpansionstoLocalGroupDataaligns with the refactoring objective to centralize group membership data.
128-166: LGTM!The signature change and internal usage of
localGroupDataare consistent with the refactoring pattern.
443-444: LGTM!The bitmap expansion now correctly uses the
GroupMembershipCachefromLocalGroupData.
586-591: LGTM!The group membership expansion correctly uses
OrReachabilityfrom theGroupMembershipCache.
856-871: LGTM!The function signature and implementation correctly use
*algo.ReachabilityCachefor group expansion, maintaining consistency with the new data carrier approach.packages/go/analysis/ad/post.go (10)
29-36: LGTM!New imports are necessary for the introduced
LocalGroupData,CanRDPDatastructures, and associated helper functions.
39-47: LGTM!The signature change to accept
*LocalGroupDatais consistent with the refactoring pattern.
74-83: LGTM!Consistent with the
PostSyncLAPSPasswordrefactoring pattern.
187-207: LGTM!Clean implementation with appropriate error handling and timing measurement for observability.
288-303: LGTM!The signature change and usage of
CalculateCrossProductNodeSetswithLocalGroupDatais consistent with the refactoring.
502-510: Potential nil pointer dereference risk.This function accesses
canRDPData.RemoteDesktopUsersLocalGroup.IDwithout checking ifRemoteDesktopUsersLocalGroupis nil. WhileComputerData(line 751) fetches this, ifFetchComputerLocalGroupBySIDSuffixreturns a zero-value node (as noted in the earlier issue), this could lead to incorrect behavior. This is a downstream effect of theFetchComputerLocalGroupBySIDSuffixissue.
565-590: LGTM!The query error handling has been fixed - the error from
Query()is now properly captured and returned.
737-764:ComputerDatadoesn't detect missing Remote Desktop Users group.At line 751-755, if
FetchComputerLocalGroupBySIDSuffixreturns a zero-value node (when no group is found), the error will be nil andcanRDPData.RemoteDesktopUsersLocalGroupwill point to an empty node. This could lead to incorrect behavior in downstream processing. Once theFetchComputerLocalGroupBySIDSuffixfix is applied to returngraph.ErrNoResultsFound, this code should handle that error appropriately.
780-786: LGTM!Clean delegation pattern based on the
skipURAflag.
788-819: LGTM!The two-phase RDP entity processing logic is well-structured:
- Direct RIL entities with RDP group membership are added immediately
- Group members are expanded and checked for the correct permissions
The implementation correctly uses
GroupMembershipCache.Reachfor group expansion.go.mod (1)
21-21: Dependency versions are published and security-verified; breaking change concern unfounded for dawgs.The updates to
github.com/specterops/dawgs v0.4.1andgithub.com/jackc/pgx/v5 v5.8.0are both published versions (as of Jan 15 and Dec 26, 2025, respectively). The dawgs v0.4.1 release notes show no breaking changes—only new Reachability tooling and code cleanup. pgx v5.8.0 exists on the Go module proxy but lacks official release notes on its GitHub releases page. No security advisories found forcuelang.org/go v0.15.3or thegolang.org/xupdates; crypto v0.47.0 includes fixes for CVE-2025-47914 and CVE-2025-58181 from v0.45.0+.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/api/src/analysis/analysis_integration_test.go`:
- Line 17: The integration test build tag line is commented out ("// //go:build
integration") so the tests run during normal unit tests; edit
analysis_integration_test.go to remove the extra leading slashes so the line
reads "//go:build integration" (ensure it is the very first non-blank line
before package declaration) so the integration build tag is active and the tests
are excluded from regular `go test`.
In `@packages/go/analysis/ad/ad.go`:
- Around line 559-571: The slog.Info statements that print members of
nodeSlices/firstDegreeSets/unrolledSets/checkSet (calls in the loop and final
slog.Info) should be removed or demoted to debug-level logging to avoid emitting
large data blobs; replace slog.Info(...) with slog.Debug(...) and wrap them in a
debug-enabled guard using slog.Default().Enabled(context.Background(),
slog.LevelDebug) (or remove entirely) so the heavy Slice() outputs are only
produced when debug logging is enabled for functions manipulating nodeSlices,
firstDegreeSets, unrolledSets, and checkSet.
♻️ Duplicate comments (3)
packages/go/analysis/ad/post.go (3)
456-485: Zero ID returned when no results found.Similar to
FetchComputerLocalGroupBySIDSuffix, this function returns0forstartIDwhen no results are found, without indicating an error. Callers cannot distinguish between "no local group found" and "local group with ID 0".🔧 Proposed fix - return error when no results found
func FetchComputerLocalGroupIDBySIDSuffix(tx graph.Transaction, computer graph.ID, groupSuffix string) (graph.ID, error) { var ( - startID graph.ID + startID graph.ID + found bool err = tx.Relationships().Filter(query.And( query.StringEndsWith(query.StartProperty(common.ObjectID.String()), groupSuffix), query.Kind(query.Relationship(), ad.LocalToComputer), query.InIDs(query.EndID(), computer), )).Query( func(results graph.Result) error { if results.Next() { if err := results.Scan(&startID); err != nil { return err } + found = true } results.Close() return results.Error() }, query.Returning( query.StartID(), ), ) ) if err != nil { return 0, err } + if !found { + return 0, graph.ErrNoResultsFound + } return startID, nil }
425-454: Zero-value node returned when no results found.When
results.Next()returns false on the first call,groupNoderemains a zero-valuegraph.Nodeand the function returns&groupNode, nil. This can cause issues downstream - for example at line 757 wherecomputerData.RemoteDesktopUsersLocalGroupis assigned from this function and later accessed at line 505 viaRemoteDesktopUsersLocalGroup.ID.🔧 Proposed fix - return error when no results found
func FetchComputerLocalGroupBySIDSuffix(tx graph.Transaction, computer graph.ID, groupSuffix string) (*graph.Node, error) { var ( - groupNode graph.Node + groupNode *graph.Node err = tx.Relationships().Filter(query.And( query.StringEndsWith(query.StartProperty(common.ObjectID.String()), groupSuffix), query.Kind(query.Relationship(), ad.LocalToComputer), query.InIDs(query.EndID(), computer), )).Query( func(results graph.Result) error { if results.Next() { - if err := results.Scan(&groupNode); err != nil { + var node graph.Node + if err := results.Scan(&node); err != nil { return err } + groupNode = &node } results.Close() return results.Error() }, query.Returning( query.Start(), ), ) ) if err != nil { return nil, err } - return &groupNode, nil + if groupNode == nil { + return nil, graph.ErrNoResultsFound + } + return groupNode, nil }
754-758: Error handling for missing RemoteDesktopUsersLocalGroup needs verification.
FetchComputerLocalGroupBySIDSuffixreturns a pointer to a zero-value node (not nil) when the Remote Desktop Users local group doesn't exist. The function always returns(&groupNode, nil)at line 453, even when no query results are found. This meanscomputerData.RemoteDesktopUsersLocalGroupwill point to a node with an empty/zero ID.When accessed downstream at line 505 (
canRDPData.RemoteDesktopUsersLocalGroup.ID.Uint64()), this will use an invalid (zero) ID value, causingcontainer.AdjacentNodes()to retrieve incorrect membership data. Confirm whether this should instead return nil on missing group or if the callers need explicit nil checks.
🧹 Nitpick comments (2)
cmd/api/src/analysis/ad/ntlm_integration_test.go (1)
154-156: Inconsistent variable naming:grouplocalGroupDataappears to be a typo.The variable is named
localGroupDatain testsTestPostNTLMRelayADCSandTestNTLMRelayToADCSComposition(lines 49, 96), butgrouplocalGroupDatahere and in several other test functions. This inconsistency suggests a find-replace artifact. Consider standardizing tolocalGroupDatathroughout for readability.♻️ Suggested fix for consistent naming
- grouplocalGroupData, computers, _, authenticatedUsers, err := fetchNTLMPrereqs(db) + localGroupData, computers, _, authenticatedUsers, err := fetchNTLMPrereqs(db) require.NoError(t, err) - ntlmCache, err := ad2.NewNTLMCache(context.Background(), db, grouplocalGroupData) + ntlmCache, err := ad2.NewNTLMCache(context.Background(), db, localGroupData)cmd/api/src/analysis/ad/adcs_integration_test.go (1)
66-81: Same variable naming inconsistency as in ntlm_integration_test.go.The variable
grouplocalGroupDatais used throughout this file (and appears ~70+ times in the changed lines). Consider a bulk rename tolocalGroupDatafor consistency with the function's internal naming and with other test files. The actual data flow is correct.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/api/src/analysis/ad/ad_integration_test.gocmd/api/src/analysis/ad/adcs_integration_test.gocmd/api/src/analysis/ad/ntlm_integration_test.gocmd/api/src/analysis/analysis_integration_test.gopackages/go/analysis/ad/ad.gopackages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/post.go
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/analysis/ad/ad_integration_test.go
📚 Learning: 2025-05-23T19:56:13.423Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/BloodHound PR: 1440
File: packages/go/analysis/ad/ntlm.go:123-138
Timestamp: 2025-05-23T19:56:13.423Z
Learning: In the BloodHound NTLM analysis code (packages/go/analysis/ad/ntlm.go), the `isProtectedComputer` function intentionally fails open (returns false/unprotected) when the Protected Users group cannot be retrieved, maintaining consistency with the original logic patterns in the codebase.
Applied to files:
cmd/api/src/analysis/analysis_integration_test.gocmd/api/src/analysis/ad/ntlm_integration_test.gopackages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/post.go
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
cmd/api/src/analysis/analysis_integration_test.gopackages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/post.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
cmd/api/src/analysis/ad/ntlm_integration_test.gopackages/go/analysis/ad/ad.gocmd/api/src/analysis/ad/adcs_integration_test.gopackages/go/analysis/ad/ntlm.go
📚 Learning: 2025-08-28T16:43:43.961Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.
Applied to files:
cmd/api/src/analysis/ad/ntlm_integration_test.gocmd/api/src/analysis/ad/adcs_integration_test.go
📚 Learning: 2025-05-27T16:58:33.295Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Applied to files:
packages/go/analysis/ad/post.go
🧬 Code graph analysis (6)
cmd/api/src/analysis/ad/ad_integration_test.go (2)
packages/go/analysis/ad/post.go (2)
PostSyncLAPSPassword(39-72)PostDCSync(74-104)packages/go/analysis/ad/owns.go (1)
PostOwnsAndWriteOwner(38-164)
cmd/api/src/analysis/analysis_integration_test.go (1)
packages/go/analysis/ad/post.go (2)
FetchCanRDPData(705-711)FetchCanRDPEntityBitmapForComputer(536-564)
cmd/api/src/analysis/ad/ntlm_integration_test.go (1)
packages/go/analysis/ad/ntlm.go (3)
NewNTLMCache(63-124)FetchLDAPSigningCache(892-968)FetchProtectedUsersMappedToDomains(855-880)
packages/go/analysis/ad/ad.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(630-635)packages/go/graphschema/ad/ad.go (3)
Group(31-31)LocalGroup(36-36)Contains(54-54)
cmd/api/src/analysis/ad/adcs_integration_test.go (4)
packages/go/analysis/ad/post.go (2)
LocalGroupData(630-635)FetchLocalGroupData(637-671)packages/go/analysis/ad/esc1.go (1)
PostADCSESC1(35-64)packages/go/analysis/ad/esc6.go (2)
PostADCSESC6a(37-80)PostADCSESC6b(82-127)packages/go/analysis/ad/esc13.go (1)
PostADCSESC13(36-78)
packages/go/analysis/ad/ntlm.go (1)
packages/go/analysis/ad/post.go (1)
LocalGroupData(630-635)
🪛 GitHub Actions: Run Go Unit Tests
cmd/api/src/analysis/ad/ad_integration_test.go
[error] 1-1: Test command failed with exit code 1
cmd/api/src/analysis/analysis_integration_test.go
[error] 1-1: Test command failed with exit code 1
cmd/api/src/analysis/ad/ntlm_integration_test.go
[error] 1-1: Test command failed with exit code 1
packages/go/analysis/ad/ad.go
[error] 1-1: Test command failed with exit code 1
cmd/api/src/analysis/ad/adcs_integration_test.go
[error] 1-1: Test command failed with exit code 1
packages/go/analysis/ad/ntlm.go
[error] 1-1: Test command failed with exit code 1
packages/go/analysis/ad/post.go
[error] 1-1: Test command failed with exit code 1
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (17)
cmd/api/src/analysis/analysis_integration_test.go (1)
42-57: LGTM - API migration to FetchCanRDPData pattern.The test correctly uses the new
FetchCanRDPDataAPI and retrieves per-computer data viacanRDPData.ComputerData(tx, computerID)before callingFetchCanRDPEntityBitmapForComputer. This pattern is consistently applied throughout the file.packages/go/analysis/ad/ad.go (1)
477-497: LGTM - Clean signature refactor to LocalGroupData.The function signature change from
(tx graph.Transaction, groupExpansions impact.PathAggregator, nodeSlices ...[]*graph.Node)to(localGroupData *LocalGroupData, nodeSlices ...[]*graph.Node)is a sensible simplification that removes the transaction dependency and centralizes group membership data access.packages/go/analysis/ad/ntlm.go (3)
40-46: LGTM - NTLMCache struct updated to use LocalGroupData.The struct field change from
GroupExpansions impact.PathAggregatortoLocalGroupData *LocalGroupDataaligns with the PR's goal of centralizing group membership data access through the new LocalGroupData abstraction.
63-82: LGTM - NewNTLMCache correctly wired to LocalGroupData.The function signature and internal wiring correctly propagates
localGroupDatatoFetchProtectedUsersMappedToDomainsand assigns it tontlmCache.LocalGroupData.
855-879: LGTM - FetchProtectedUsersMappedToDomains updated to use LocalGroupData.The implementation correctly uses
localGroupData.GroupMembershipCache.OrReachabilityto populate the protected users bitmap, replacing the previous PathAggregator-based approach.packages/go/analysis/ad/post.go (3)
630-671: LGTM - Well-structured LocalGroupData type and fetching logic.The
LocalGroupDatastruct cleanly encapsulates the data needed for local group post-processing. TheFetchLocalGroupDatafunction properly handles errors and sequentially builds up the required caches.
713-767: LGTM - CanRDPData and CanRDPComputerData provide clean per-computer data access.The nested struct design with
CanRDPDataembeddingLocalGroupDataandCanRDPComputerDataembeddingCanRDPDataprovides a clean hierarchy for accessing RDP-related data at different scopes.
791-821: LGTM - ProcessRDPWithUra logic is clear.The two-pass approach (first checking direct RIL entities, then expanding group memberships) is well-structured and the comments clearly explain the logic.
cmd/api/src/analysis/ad/ad_integration_test.go (4)
1144-1165: LGTM - Test properly updated to use FetchLocalGroupData API.The test correctly fetches
localGroupDataand passes it toPostSyncLAPSPassword. The error handling pattern is appropriate for integration tests.
1175-1196: LGTM - TestDCSync properly migrated to new API.Consistent with the other test updates, using
FetchLocalGroupDataand passing the result toPostDCSync.
1207-1212: LGTM - TestOwnsWriteOwnerPriorCollectorVersions properly updated.The test correctly uses the new
FetchLocalGroupDataandPostOwnsAndWriteOwnersignatures.
1410-1415: LGTM - TestOwnsWriteOwner properly updated.Consistent migration to the new LocalGroupData-based API.
cmd/api/src/analysis/ad/ntlm_integration_test.go (2)
49-51: LGTM!The refactor correctly retrieves
localGroupDataviafetchNTLMPrereqsand passes it toNewNTLMCache. The variable naming is consistent here withlocalGroupData.
613-630: LGTM!The
fetchNTLMPrereqsfunction has been correctly updated to useFetchLocalGroupDataand return*ad2.LocalGroupData. The error handling and return value flow are properly maintained.cmd/api/src/analysis/ad/adcs_integration_test.go (3)
41-55: LGTM!The
FetchADCSPrereqsfunction correctly fetcheslocalGroupDatausingad2.FetchLocalGroupDataand returns it as the first value. The cache building and other data fetching remain unchanged.
231-232: Good pattern for tests that don't needlocalGroupData.Correctly discards the unused
localGroupDatareturn value sincePostGoldenCertdoesn't require it.
1-39: Function signatures and imports are correct; no issues found with the code structure shown.The
FetchLocalGroupDataandPostADCSESC*function signatures match their usage throughout the test file. All calls pass parameters in the correct order and types:
FetchLocalGroupData(context.Background(), db)matches the definitionfunc(context.Context, graph.Database) (*LocalGroupData, error)- All
PostADCSESC*calls (ESC1, ESC3, ESC4, ESC6a, ESC6b, ESC9a, ESC9b, ESC10a, ESC10b, ESC13) follow the expected signature pattern and are invoked correctlyImports (lines 21-38) include all necessary packages. The code shown is structurally sound with no compilation errors evident.
Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| //go:build integration | ||
| // //go:build integration |
There was a problem hiding this comment.
Critical: Build tag is commented out, causing integration tests to run during unit tests.
The build tag // //go:build integration has an extra // prefix that comments it out. This means these integration tests will run during regular unit test execution, which is likely causing the pipeline failure.
🔧 Proposed fix
-// //go:build integration
+//go:build integration📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // //go:build integration | |
| //go:build integration |
🤖 Prompt for AI Agents
In `@cmd/api/src/analysis/analysis_integration_test.go` at line 17, The
integration test build tag line is commented out ("// //go:build integration")
so the tests run during normal unit tests; edit analysis_integration_test.go to
remove the extra leading slashes so the line reads "//go:build integration"
(ensure it is the very first non-blank line before package declaration) so the
integration build tag is active and the tests are excluded from regular `go
test`.
237a7dd to
3283c57
Compare
zinic
changed the title
zinic
force-pushed
the
BED-7147
branch
2 times, most recently
from
221d877 to
b328188
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/api/src/analysis/ad/adcs_integration_test.go (1)
41-53: Handle BuildCache errors in FetchADCSPrereqs.
BuildCachereturns an error but it’s ignored here, which can mask setup failures and yield misleading test results.🔧 Proposed fix
- cache.BuildCache(context.Background(), db, enterpriseCertAuthorities, certTemplates) - return localGroupData, cache.GetEnterpriseCertAuthorities(), cache.GetCertTemplates(), cache.GetDomains(), cache, nil + if err := cache.BuildCache(context.Background(), db, enterpriseCertAuthorities, certTemplates); err != nil { + return nil, nil, nil, nil, ad2.ADCSCache{}, err + } + return localGroupData, cache.GetEnterpriseCertAuthorities(), cache.GetCertTemplates(), cache.GetDomains(), cache, nilpackages/go/analysis/ad/post.go (1)
209-226: Fix incorrect property accessors and misleading variable name inFetchAdminGroups.This function uses
query.StartPropertyandquery.EndPropertyon a node query (tx.Nodes()), but these are relationship-oriented accessors meant fortx.Relationships()filters. For node queries, usequery.NodePropertyinstead. Additionally, the variable namecomputerNodeIdsis misleading for a function namedFetchAdminGroups—it should beadminGroupIds.Proposed fix
func FetchAdminGroups(ctx context.Context, db graph.Database) (cardinality.Duplex[uint64], error) { - computerNodeIds := cardinality.NewBitmap64() + adminGroupIds := cardinality.NewBitmap64() - return computerNodeIds, db.ReadTransaction(ctx, func(tx graph.Transaction) error { + return adminGroupIds, db.ReadTransaction(ctx, func(tx graph.Transaction) error { return tx.Nodes().Filter(query.And( - query.Or( - query.StringEndsWith(query.StartProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), - query.StringEndsWith(query.EndProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), - ), + query.StringEndsWith(query.NodeProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), )).FetchIDs(func(cursor graph.Cursor[graph.ID]) error { for id := range cursor.Chan() { - computerNodeIds.Add(id.Uint64()) + adminGroupIds.Add(id.Uint64()) } return nil }) }) }
🤖 Fix all issues with AI agents
In `@packages/go/analysis/ad/local_groups.go`:
- Around line 275-308: The three calls to FetchComputerLocalGroupIDBySIDSuffix
(localAdminGroup, localPSRemoteGroup, localDCOMGroup) currently ignore their
errors (localAdminGroupErr, localPSRemoteGroupErr, localDCOMGroupErr) and
silently skip processing; change the logic to propagate or surface these errors
instead of swallowing them—e.g., for each fetch, if err != nil wrap/return the
error (or accumulate and return a combined error) with context like which
computerID and suffix failed so callers can see failures; update the surrounding
function to return an error (or accept/return a multi-error) and remove the
silent-nil-only behavior before calling numGroupsProcessed.Add and
channels.Submit for reachJob submissions.
♻️ Duplicate comments (2)
packages/go/analysis/ad/post.go (2)
425-454: Bug: Returns pointer to zero-value node when no results found.This issue was previously flagged. When
results.Next()returns false on the first call,groupNoderemains a zero-valuegraph.Node, and the function returns&groupNode, nil. Callers (e.g.,ComputerDataat line 754) don't check for a zero-value node, which will cause issues when accessingRemoteDesktopUsersLocalGroup.ID(it will be 0).Proposed fix - return error when no results found
func FetchComputerLocalGroupBySIDSuffix(tx graph.Transaction, computer graph.ID, groupSuffix string) (*graph.Node, error) { var ( - groupNode graph.Node + groupNode *graph.Node err = tx.Relationships().Filter(query.And( query.StringEndsWith(query.StartProperty(common.ObjectID.String()), groupSuffix), query.Kind(query.Relationship(), ad.LocalToComputer), query.InIDs(query.EndID(), computer), )).Query( func(results graph.Result) error { if results.Next() { - if err := results.Scan(&groupNode); err != nil { + var node graph.Node + if err := results.Scan(&node); err != nil { return err } + groupNode = &node } results.Close() return results.Error() }, query.Returning( query.Start(), ), ) ) if err != nil { return nil, err } - return &groupNode, nil + if groupNode == nil { + return nil, graph.ErrNoResultsFound + } + return groupNode, nil }
456-485: Similar issue: Returns zero ID when no results found.Same pattern as
FetchComputerLocalGroupBySIDSuffix- when no results are found, a zerograph.IDis returned without an error. Callers cannot distinguish between "no group found" and "group found with ID 0" (though ID 0 is typically invalid). Consider returninggraph.ErrNoResultsFound.
🧹 Nitpick comments (5)
cmd/api/src/analysis/membership_integration_test.go (1)
77-95: Test refactor looks correct for the new reachability model.The updated test properly validates the SCC-based reachability behavior, and the explanatory comments (lines 87-89) clearly document why origin members are included in the cardinality counts. The consistent use of
graph.DirectionInboundacross all assertions aligns with the newLocalGroupData/ReachabilityCacheapproach.
Consider addressing the deprecated test utility.
Static analysis flags
integration.NewGraphTestContextas deprecated (SA1019). The deprecation note references updated integration test standards. This is a minor concern since it affects existing test infrastructure, but consider updating this test to use the newer patterns when feasible.packages/go/analysis/ad/ntlm.go (1)
63-83: Add defensive checks for nil LocalGroupData.
LocalGroupData(and itsGroupMembershipCache) is now a required dependency; a nil pointer would panic in several paths. An early guard yields a clearer error and avoids hard crashes.💡 Proposed guard
func NewNTLMCache(ctx context.Context, db graph.Database, localGroupData *LocalGroupData) (NTLMCache, error) { var ( ntlmCache = NTLMCache{} unprotectedComputerCache = make(map[string]cardinality.Duplex[uint64]) allUnprotectedComputerCache = cardinality.NewBitmap64() ) + if localGroupData == nil || localGroupData.GroupMembershipCache == nil { + return ntlmCache, fmt.Errorf("local group data is required") + } return ntlmCache, db.ReadTransaction(ctx, func(tx graph.Transaction) error { // Fetch all nodes where the node is a Group and is an Authenticated User if innerAuthenticatedUsersCache, err := FetchAuthUsersMappedToDomains(tx); err != nil { return err @@ func FetchProtectedUsersMappedToDomains(ctx context.Context, db graph.Database, localGroupData *LocalGroupData) (map[string]cardinality.Duplex[uint64], error) { protectedUsers := make(map[string]cardinality.Duplex[uint64]) + if localGroupData == nil || localGroupData.GroupMembershipCache == nil { + return protectedUsers, fmt.Errorf("local group data is required") + } err := db.ReadTransaction(ctx, func(tx graph.Transaction) error {Also applies to: 855-870
packages/go/analysis/ad/post.go (3)
502-511: Remove commented-out dead code.Line 510 contains commented-out code that should be removed to keep the codebase clean.
Proposed fix
func FetchRemoteDesktopUsersLocalGroupMembership(computer graph.ID, canRDPData *CanRDPComputerData) cardinality.Duplex[uint64] { adjacentNodes := container.AdjacentNodes( canRDPData.LocalGroupMembershipDigraph, canRDPData.RemoteDesktopUsersLocalGroup.ID.Uint64(), graph.DirectionInbound, ) return cardinality.NewBitmap64With(adjacentNodes...) - // return canRDPData.GroupMembershipCache.Reach(canRDPData.RemoteDesktopUsersLocalGroup.ID.Uint64(), graph.DirectionInbound) }
630-635: Address the TODO comment for documentation.Line 633 has a TODO comment
// TODO: Document meforExcludedShortcutGroups. Consider adding documentation explaining what this field represents and how it's used in cross-product calculations.Would you like me to help draft documentation for this field based on its usage in
CalculateCrossProductNodeSets?
685-689: Redundant fetch ofExcludedShortcutGroups.
ExcludedShortcutGroupsis already populated inLocalGroupData(line 644 inFetchLocalGroupData), and sinceCanRDPDataembedsLocalGroupData, it inherits this field. Fetching it again here is redundant and performs an unnecessary database query.Proposed fix - remove redundant fetch
func (s *LocalGroupData) FetchCanRDPData(ctx context.Context, graphDB graph.Database) (*CanRDPData, error) { components := &CanRDPData{ LocalGroupData: *s, } if err := graphDB.ReadTransaction(ctx, func(tx graph.Transaction) error { if computersWithURA, err := FetchComputersWithURA(tx); err != nil { return err } else { components.ComputersWithURA = computersWithURA } - if excludedGroups, err := FetchAuthUsersAndEveryoneGroups(tx); err != nil { - return err - } else { - components.ExcludedShortcutGroups = excludedGroups.IDBitmap() - } - return nil }); err != nil { return components, err }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
cmd/api/src/analysis/ad/ad_integration_test.gocmd/api/src/analysis/ad/adcs_integration_test.gocmd/api/src/analysis/ad/ntlm_integration_test.gocmd/api/src/analysis/ad/post.gocmd/api/src/analysis/analysis_integration_test.gocmd/api/src/analysis/membership_integration_test.gocmd/api/src/analysis/post_integration_test.gocmd/api/src/daemons/datapipe/datapipe.gocmd/api/src/test/integration/harnesses.gogo.modpackages/go/analysis/ad/ad.gopackages/go/analysis/ad/adcs.gopackages/go/analysis/ad/adcscache.gopackages/go/analysis/ad/esc1.gopackages/go/analysis/ad/esc10.gopackages/go/analysis/ad/esc13.gopackages/go/analysis/ad/esc3.gopackages/go/analysis/ad/esc4.gopackages/go/analysis/ad/esc6.gopackages/go/analysis/ad/esc9.gopackages/go/analysis/ad/esc_shared.gopackages/go/analysis/ad/local_groups.gopackages/go/analysis/ad/membership.gopackages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/owns.gopackages/go/analysis/ad/post.go
💤 Files with no reviewable changes (1)
- cmd/api/src/daemons/datapipe/datapipe.go
🚧 Files skipped from review as they are similar to previous changes (8)
- cmd/api/src/analysis/ad/post.go
- packages/go/analysis/ad/esc6.go
- packages/go/analysis/ad/esc4.go
- packages/go/analysis/ad/adcscache.go
- cmd/api/src/test/integration/harnesses.go
- packages/go/analysis/ad/esc10.go
- cmd/api/src/analysis/post_integration_test.go
- packages/go/analysis/ad/esc13.go
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
packages/go/analysis/ad/adcs.gopackages/go/analysis/ad/local_groups.gopackages/go/analysis/ad/owns.gopackages/go/analysis/ad/ad.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
packages/go/analysis/ad/adcs.gopackages/go/analysis/ad/esc_shared.gopackages/go/analysis/ad/esc9.gopackages/go/analysis/ad/esc1.gocmd/api/src/analysis/ad/ntlm_integration_test.gocmd/api/src/analysis/membership_integration_test.gocmd/api/src/analysis/ad/adcs_integration_test.gogo.modpackages/go/analysis/ad/owns.gopackages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/ad.gopackages/go/analysis/ad/esc3.go
📚 Learning: 2025-07-10T14:31:01.781Z
Learnt from: JonasBK
Repo: SpecterOps/BloodHound PR: 1671
File: packages/go/analysis/ad/esc16.go:65-69
Timestamp: 2025-07-10T14:31:01.781Z
Learning: In BloodHound ADCS ESC analysis functions (PostADCSESC1, PostADCSESC4, PostADCSESC6, PostADCSESC9, PostADCSESC10, PostADCSESC13, PostADCSESC16, etc.), the established pattern for handling cert template validation errors and user filtering errors is to use slog.WarnContext to log warnings and continue processing with the next template, rather than returning errors to the caller. This pattern is consistently applied across all ADCS ESC edge implementations.
Applied to files:
packages/go/analysis/ad/adcs.gopackages/go/analysis/ad/esc9.gopackages/go/analysis/ad/esc1.gopackages/go/analysis/ad/ad.gopackages/go/analysis/ad/esc3.go
📚 Learning: 2025-06-25T18:24:25.014Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T18:24:25.014Z
Learning: In BloodHound Go code, for error types in slog structured logging, prefer using slog.String("key", err.Error()) over slog.Any("key", err). The explicit string conversion with err.Error() is preferred over using slog.Any() for error types.
Applied to files:
packages/go/analysis/ad/local_groups.gopackages/go/analysis/ad/owns.go
📚 Learning: 2025-05-23T19:56:13.423Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/BloodHound PR: 1440
File: packages/go/analysis/ad/ntlm.go:123-138
Timestamp: 2025-05-23T19:56:13.423Z
Learning: In the BloodHound NTLM analysis code (packages/go/analysis/ad/ntlm.go), the `isProtectedComputer` function intentionally fails open (returns false/unprotected) when the Protected Users group cannot be retrieved, maintaining consistency with the original logic patterns in the codebase.
Applied to files:
packages/go/analysis/ad/local_groups.gocmd/api/src/analysis/ad/ntlm_integration_test.gopackages/go/analysis/ad/ntlm.gocmd/api/src/analysis/analysis_integration_test.gopackages/go/analysis/ad/post.go
📚 Learning: 2025-05-27T16:58:33.295Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Applied to files:
packages/go/analysis/ad/local_groups.gocmd/api/src/analysis/membership_integration_test.gocmd/api/src/analysis/analysis_integration_test.gopackages/go/analysis/ad/post.go
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
packages/go/analysis/ad/local_groups.go
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/analysis/ad/ad_integration_test.go
📚 Learning: 2025-08-28T16:43:43.961Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.
Applied to files:
cmd/api/src/analysis/ad/ntlm_integration_test.gocmd/api/src/analysis/ad/adcs_integration_test.go
📚 Learning: 2025-06-30T22:58:54.834Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1629
File: go.mod:18-18
Timestamp: 2025-06-30T22:58:54.834Z
Learning: Go 1.24 supports patch versions in the go directive of go.mod files. Syntax like "go 1.24.4" is valid and specifies the minimum Go version required to build the module, including the patch level.
Applied to files:
go.mod
📚 Learning: 2025-08-26T18:13:36.643Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1820
File: .github/workflows/audit-release.yml:0-0
Timestamp: 2025-08-26T18:13:36.643Z
Learning: The go tool command supports custom/third-party binaries as of Go 1.24, allowing invocation of custom CLI tools like "go tool stbernard" rather than being limited to only built-in Go toolchain binaries.
Applied to files:
go.mod
📚 Learning: 2025-07-10T14:33:20.317Z
Learnt from: JonasBK
Repo: SpecterOps/BloodHound PR: 1671
File: cmd/api/src/analysis/ad/adcs_integration_test.go:3687-3687
Timestamp: 2025-07-10T14:33:20.317Z
Learning: When reviewing Go code, functions defined in one file within a package are accessible from other files in the same package. Before flagging missing functions as compilation errors, check if they exist in other files within the same package directory.
Applied to files:
packages/go/analysis/ad/owns.go
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
packages/go/analysis/ad/ntlm.gocmd/api/src/analysis/analysis_integration_test.gopackages/go/analysis/ad/post.go
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
packages/go/analysis/ad/ad.go
🧬 Code graph analysis (12)
packages/go/analysis/ad/esc_shared.go (1)
packages/go/analysis/ad/post.go (1)
LocalGroupData(630-635)
packages/go/analysis/ad/local_groups.go (5)
packages/go/analysis/ad/post.go (4)
LocalGroupData(630-635)CanRDPComputerData(731-738)FetchCanRDPEntityBitmapForComputer(536-564)FetchComputerLocalGroupIDBySIDSuffix(456-485)packages/go/analysis/post_operation.go (3)
AtomicPostProcessingStats(86-90)NewAtomicPostProcessingStats(92-98)NewPropertiesWithLastSeen(166-171)packages/go/analysis/post.go (1)
CreatePostRelationshipJob(151-157)packages/go/graphschema/ad/ad.go (4)
CanRDP(68-68)AdminTo(66-66)CanPSRemote(67-67)ExecuteDCOM(69-69)packages/go/analysis/analysis.go (1)
MaximumDatabaseParallelWorkers(36-36)
packages/go/analysis/ad/esc9.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(630-635)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)
packages/go/analysis/ad/esc1.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(630-635)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(477-638)
cmd/api/src/analysis/ad/ad_integration_test.go (2)
packages/go/analysis/ad/post.go (3)
FetchLocalGroupData(637-671)PostSyncLAPSPassword(39-72)PostDCSync(74-104)packages/go/analysis/ad/owns.go (1)
PostOwnsAndWriteOwner(38-164)
cmd/api/src/analysis/ad/ntlm_integration_test.go (2)
packages/go/analysis/ad/ntlm.go (3)
NewNTLMCache(63-124)FetchLDAPSigningCache(892-968)FetchProtectedUsersMappedToDomains(855-880)packages/go/analysis/ad/post.go (1)
LocalGroupData(630-635)
cmd/api/src/analysis/ad/adcs_integration_test.go (4)
packages/go/analysis/ad/post.go (2)
LocalGroupData(630-635)FetchLocalGroupData(637-671)packages/go/analysis/ad/esc1.go (1)
PostADCSESC1(35-64)packages/go/analysis/ad/esc9.go (2)
PostADCSESC9a(35-82)PostADCSESC9b(84-128)packages/go/analysis/ad/esc6.go (2)
PostADCSESC6a(37-80)PostADCSESC6b(82-127)
packages/go/analysis/ad/owns.go (1)
packages/go/analysis/ad/post.go (1)
LocalGroupData(630-635)
packages/go/analysis/ad/ntlm.go (1)
packages/go/analysis/ad/post.go (1)
LocalGroupData(630-635)
packages/go/analysis/ad/esc3.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(630-635)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(477-638)
cmd/api/src/analysis/analysis_integration_test.go (1)
packages/go/analysis/ad/post.go (2)
FetchCanRDPData(705-711)FetchCanRDPEntityBitmapForComputer(536-564)
packages/go/analysis/ad/post.go (5)
packages/go/analysis/post.go (1)
CreatePostRelationshipJob(151-157)packages/go/graphschema/common/common.go (2)
Nodes(33-35)ObjectID(52-52)packages/go/analysis/ad/wellknown/suffix.go (2)
AdministratorsSIDSuffix(83-83)RemoteDesktopUsersSIDSuffix(86-86)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(477-638)packages/go/analysis/ad/queries.go (1)
FetchAuthUsersAndEveryoneGroups(1892-1902)
🪛 golangci-lint (2.5.0)
cmd/api/src/analysis/membership_integration_test.go
[minor] 78-78: SA1019: integration.NewGraphTestContext is deprecated: this suite of integration utils is deprecated and should be avoided Integration tests should be updated to reflect the latest standards. See commit a6cc430#diff-e6bcb50873ade3cf33cef4e3e0ff566fb8ac1367b4ade36f4511bc2172a760e1 for implementation guidance. Additional detailed information can be found in Confluence.
(staticcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (26)
cmd/api/src/analysis/membership_integration_test.go (1)
30-30: LGTM!Import correctly added to support the new
algo.FetchReachabilityCachefunction used in the refactored test.cmd/api/src/analysis/analysis_integration_test.go (3)
42-50: Per-computer CanRDP test wiring looks good.Fetching CanRDPData once and deriving per-computer data inside the read transaction matches the new API flow.
68-76: URA enforced/unenforced paths are updated cleanly.Also applies to: 100-106, 134-143
165-174: Citrix path adjustments look consistent with the new CanRDP data model.Also applies to: 189-195, 214-222
go.mod (2)
21-60: No review notes for this hunk.
87-281: No review notes for this hunk.packages/go/analysis/ad/local_groups.go (1)
32-165: PostCanRDP pipeline sequencing looks solid.Stages are closed in the right order, and the data flow is easy to follow.
cmd/api/src/analysis/ad/ad_integration_test.go (1)
1144-1147: LocalGroupData wiring in post-processing tests looks consistent.Also applies to: 1175-1178, 1207-1210, 1410-1413
packages/go/analysis/ad/membership.go (1)
92-94: Simplified direction handling is clear and correct.packages/go/analysis/ad/esc1.go (1)
35-35: LGTM - Signature updated to use LocalGroupData.The function signature change from
impact.PathAggregatorto*LocalGroupDataand the corresponding update toCalculateCrossProductNodeSetscall align with the broader refactor. The cross-product calculation now correctly passeslocalGroupDataas the first parameter, consistent with the updatedCalculateCrossProductNodeSetssignature inad/ad.go.Also applies to: 48-48
packages/go/analysis/ad/esc3.go (1)
38-38: LGTM - Consistent migration to LocalGroupData.The signature change and both
CalculateCrossProductNodeSetscall sites are correctly updated:
- Line 38: Function now accepts
*LocalGroupDatainstead ofimpact.PathAggregator- Lines 91-97: Delegated agents path correctly passes
localGroupDatawith the additionaldelegatedAgents.Slice()parameter- Lines 109-114: Non-delegated path correctly passes
localGroupDataThe cross-product computation logic is preserved while switching to the new data carrier.
Also applies to: 91-97, 109-114
packages/go/analysis/ad/owns.go (1)
38-38: LGTM - Clean migration to ReachabilityCache-based expansion.The changes correctly adapt the admin group resolution:
PostOwnsAndWriteOwnernow accepts*LocalGroupDataand extractsGroupMembershipCachefor the downstream callFetchAdminGroupIdssignature updated to accept*algo.ReachabilityCachedirectly- The
OrReachcall (Line 208) replaces the previousCardinalityapproach, accumulating all principals with inbound reachability to admin groups intoadminIdsThe logic correctly identifies Domain Admins/Enterprise Admins and their transitive members by leveraging the reachability cache's graph-based expansion.
Also applies to: 45-45, 196-208
packages/go/analysis/ad/esc9.go (1)
35-35: LGTM - ESC9a/9b updated to use LocalGroupData.Both functions are correctly updated:
PostADCSESC9aandPostADCSESC9bsignatures now accept*LocalGroupData- The
getVictimBitmapcalls passlocalGroupDataas the first argument, matching the updated signature inesc_shared.goThe established error handling pattern (using
slog.WarnContextand continuing) is preserved. Based on learnings, this is the correct pattern for ADCS ESC analysis functions.Also applies to: 53-53, 84-84, 102-102
packages/go/analysis/ad/esc_shared.go (2)
394-399: Signature update looks correct.The
getVictimBitmapfunction correctly accepts*LocalGroupDataand extractsGroupMembershipCachefor the downstreamexpandNodeSliceToBitmapWithoutGroupscalls. The logic for combining template and ECA enrollers based on special group enrollment is preserved.Note: The output depends on
expandNodeSliceToBitmapWithoutGroups, which has the commented-out group filtering logic flagged above.
303-334: The design is correct as implemented. The comment "Expand controllers for the eca + template completely because we don't do group shortcutting here" explicitly indicates that full expansion of all group members (including nested groups) is intentional. The function nameexpandNodeSliceToBitmapWithoutGroupsmeans the result bitmap contains expanded members without the group nodes themselves—not that groups should be filtered from the result.The commented-out code that checks if members are groups was intentionally disabled because the analysis requires complete expansion without optimization shortcuts. This is a deliberate design choice, not incomplete refactoring.
cmd/api/src/analysis/ad/ntlm_integration_test.go (2)
49-52: LGTM: NTLM integration tests now use LocalGroupData-backed cache.Call sites consistently pass
LocalGroupDataintoNewNTLMCacheand protected user lookups, matching the new API flow.Also applies to: 96-99, 154-157, 230-233, 282-285, 354-361, 434-441, 510-517, 565-572
613-630: LGTM: fetchNTLMPrereqs now centralizes LocalGroupData retrieval.Using
FetchLocalGroupDatain the test helper keeps setup aligned with the production data pipeline.packages/go/analysis/ad/adcs.go (1)
36-70: LGTM: LocalGroupData is threaded through ADCS post-processing.Passing
LocalGroupDatainto ESC handlers keeps the ADCS pipeline consistent with the new local-group cache model.Also applies to: 109-207
cmd/api/src/analysis/ad/adcs_integration_test.go (1)
66-3711: LGTM: ESC integration tests now pass LocalGroupData into handlers.Call sites are consistently updated to the new LocalGroupData-based signatures across ESC test cases.
packages/go/analysis/ad/ntlm.go (1)
40-46: LGTM: LocalGroupData cache now powers NTLM group expansion.Using
LocalGroupData.GroupMembershipCachefor template/ECA and SMB admin expansion keeps NTLM analysis aligned with the new local-group data flow.Also applies to: 442-444, 588-589
packages/go/analysis/ad/ad.go (1)
475-507: No defensive guard needed—LocalGroupDatais always initialized at call sites.
FetchLocalGroupDataeither returns a fully initialized*LocalGroupDatawith all fields set (includingGroupMembershipCache) or returns(nil, error). All callers consistently check for error before using the result:if localGroupData, err := adAnalysis.FetchLocalGroupData(ctx, db); err != nil { return &aggregateStats, err } else { // localGroupData is guaranteed non-nil here }The proposed guard is redundant given this pattern is enforced across the entire codebase.
packages/go/analysis/ad/post.go (5)
39-72: LGTM! Signature updates align with LocalGroupData refactoring.The
PostSyncLAPSPasswordandPostDCSyncfunctions correctly propagate the*LocalGroupDataparameter togetLAPSSyncersandgetDCSyncersrespectively. The operation flow remains consistent with the previous implementation.Also applies to: 74-104
536-564: LGTM! Function signature and logic are correct.The signature change to accept
*CanRDPComputerDatais properly implemented. The function correctly handles the citrix-enabled and URA scenarios, using the embeddedLocalGroupDatafor cross-product calculations.
566-591: LGTM! Query error is now properly captured and returned.The previous issue where the query error was ignored has been addressed. The error is now assigned to the
errvariable and returned along with the result.
754-758: Dependent on fix forFetchComputerLocalGroupBySIDSuffix.This code will silently receive a pointer to a zero-value node when no Remote Desktop Users local group exists for the computer, due to the bug in
FetchComputerLocalGroupBySIDSuffix. Once that function is fixed to returngraph.ErrNoResultsFound, this call site should handle that error appropriately (either as an error or by settingRemoteDesktopUsersLocalGroupto nil).
791-821: LGTM! ProcessRDPWithUra logic is correct.The refactored implementation properly uses the cached digraphs and reachability cache for RDP permission checking. The two-phase approach (direct RIL entity check, then expanded group membership check) is sound.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| localAdminGroup, localAdminGroupErr = FetchComputerLocalGroupIDBySIDSuffix(tx, graph.ID(computerID), adminGroupSuffix) | ||
| localPSRemoteGroup, localPSRemoteGroupErr = FetchComputerLocalGroupIDBySIDSuffix(tx, graph.ID(computerID), psRemoteGroupSuffix) | ||
| localDCOMGroup, localDCOMGroupErr = FetchComputerLocalGroupIDBySIDSuffix(tx, graph.ID(computerID), dcomGroupSuffix) | ||
| ) | ||
|
|
||
| if localAdminGroupErr == nil { | ||
| numGroupsProcessed.Add(1) | ||
|
|
||
| channels.Submit(ctx, reachC, reachJob{ | ||
| targetComputer: computerID, | ||
| targetGroup: localAdminGroup.Uint64(), | ||
| groupSuffix: adminGroupSuffix, | ||
| }) | ||
| } | ||
|
|
||
| if localPSRemoteGroupErr == nil { | ||
| numGroupsProcessed.Add(1) | ||
|
|
||
| channels.Submit(ctx, reachC, reachJob{ | ||
| targetComputer: computerID, | ||
| targetGroup: localPSRemoteGroup.Uint64(), | ||
| groupSuffix: psRemoteGroupSuffix, | ||
| }) | ||
| } | ||
|
|
||
| if localDCOMGroupErr == nil { | ||
| numGroupsProcessed.Add(1) | ||
|
|
||
| channels.Submit(ctx, reachC, reachJob{ | ||
| targetComputer: computerID, | ||
| targetGroup: localDCOMGroup.Uint64(), | ||
| groupSuffix: dcomGroupSuffix, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Don’t swallow local group lookup errors.
If FetchComputerLocalGroupIDBySIDSuffix fails, the error is ignored and edges are silently skipped. Returning the error makes failures visible and avoids partial post-processing.
🔧 Suggested fix
- if localAdminGroupErr == nil {
+ if localAdminGroupErr == nil {
numGroupsProcessed.Add(1)
channels.Submit(ctx, reachC, reachJob{
targetComputer: computerID,
targetGroup: localAdminGroup.Uint64(),
groupSuffix: adminGroupSuffix,
})
+ } else {
+ return localAdminGroupErr
}
- if localPSRemoteGroupErr == nil {
+ if localPSRemoteGroupErr == nil {
numGroupsProcessed.Add(1)
channels.Submit(ctx, reachC, reachJob{
targetComputer: computerID,
targetGroup: localPSRemoteGroup.Uint64(),
groupSuffix: psRemoteGroupSuffix,
})
+ } else {
+ return localPSRemoteGroupErr
}
- if localDCOMGroupErr == nil {
+ if localDCOMGroupErr == nil {
numGroupsProcessed.Add(1)
channels.Submit(ctx, reachC, reachJob{
targetComputer: computerID,
targetGroup: localDCOMGroup.Uint64(),
groupSuffix: dcomGroupSuffix,
})
+ } else {
+ return localDCOMGroupErr
}🤖 Prompt for AI Agents
In `@packages/go/analysis/ad/local_groups.go` around lines 275 - 308, The three
calls to FetchComputerLocalGroupIDBySIDSuffix (localAdminGroup,
localPSRemoteGroup, localDCOMGroup) currently ignore their errors
(localAdminGroupErr, localPSRemoteGroupErr, localDCOMGroupErr) and silently skip
processing; change the logic to propagate or surface these errors instead of
swallowing them—e.g., for each fetch, if err != nil wrap/return the error (or
accumulate and return a combined error) with context like which computerID and
suffix failed so callers can see failures; update the surrounding function to
return an error (or accept/return a multi-error) and remove the silent-nil-only
behavior before calling numGroupsProcessed.Add and channels.Submit for reachJob
submissions.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/go/analysis/ad/post.go (1)
206-223: UseNodePropertyfilters when querying nodes.
FetchAdminGroupsusesquery.StartProperty/query.EndPropertyin atx.Nodes()filter. These accessors are relationship-scoped and don't apply to node queries, causing incorrect filtering. Usequery.NodePropertyinstead, as shown in similar patterns throughout the codebase (e.g.,owns.go).🔧 Proposed fix
return computerNodeIds, db.ReadTransaction(ctx, func(tx graph.Transaction) error { return tx.Nodes().Filter(query.And( - query.Or( - query.StringEndsWith(query.StartProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), - query.StringEndsWith(query.EndProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), - ), + query.StringEndsWith(query.NodeProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), )).FetchIDs(func(cursor graph.Cursor[graph.ID]) error { for id := range cursor.Chan() { computerNodeIds.Add(id.Uint64()) }
🤖 Fix all issues with AI agents
In `@packages/go/analysis/ad/esc_shared.go`:
- Around line 303-327: The expandNodeSliceToBitmapWithoutGroups function
currently never filters out group IDs because the group detection block is
commented out; restore the group-detection logic in
expandNodeSliceToBitmapWithoutGroups by using the graph adjacency iteration on
groupExpansions.Digraph (or the appropriate Digraph instance) to check each
memberID for inbound adjacent nodes and set isGroup = true when any inbound
adjacent node exists, then only add non-group memberIDs to bitmap; if the Local
Group Post-Processing rework intentionally removed group filtering, instead
rename the function and update callers (e.g., getVictimBitmap) to reflect that
it includes groups so behavior is explicit.
In `@packages/go/analysis/ad/local_groups.go`:
- Line 182: The deferred call currently defers the ContextMeasure function
itself instead of its returned closure, so change the defer in PostLocalGroups
to invoke the returned function; e.g. call measure.ContextMeasure(ctx,
slog.LevelInfo, "PostLocalGroups") and immediately call the result (defer
measure.ContextMeasure(... )()) or assign the returned func to a variable and
defer that (e.g. end := measure.ContextMeasure(...); defer end()) so the timing
closure actually runs.
♻️ Duplicate comments (3)
packages/go/analysis/ad/local_groups.go (1)
277-309: Still swallowing local group lookup errors.The error returns from
FetchComputerLocalGroupIDBySIDSuffixare ignored, which can silently skip edges and hide collection gaps. This was already raised earlier.packages/go/analysis/ad/post.go (2)
363-393: Missing not-found handling for local group lookup.
FetchComputerLocalGroupBySIDSuffixreturns a zero-value node when no rows match. This was already flagged earlier; please ensure the fix is applied here too.
395-426: Missing not-found handling for local group ID lookup.
FetchComputerLocalGroupIDBySIDSuffixstill returns zerograph.IDwhen no rows match. This was already flagged earlier; please ensure the fix is applied here too.
🧹 Nitpick comments (4)
cmd/api/src/analysis/ad/ntlm_integration_test.go (1)
154-157: Consider renaminggrouplocalGroupDatatolocalGroupDatafor clarity.
This is a minor readability nit across multiple tests.♻️ Suggested rename (apply similarly in each test)
- grouplocalGroupData, computers, _, authenticatedUsers, err := fetchNTLMPrereqs(db) + localGroupData, computers, _, authenticatedUsers, err := fetchNTLMPrereqs(db) require.NoError(t, err) - ntlmCache, err := ad2.NewNTLMCache(context.Background(), db, grouplocalGroupData) + ntlmCache, err := ad2.NewNTLMCache(context.Background(), db, localGroupData)Also applies to: 230-233, 282-285, 354-361, 434-441, 510-517, 565-572
packages/go/analysis/ad/owns.go (1)
196-209: Add a defensive nil guard forgroupExpansionsto avoid panics.
If a caller passes a nil cache,OrReachwill panic; returning an error makes failures explicit.🛡️ Suggested guard
func FetchAdminGroupIds(ctx context.Context, db graph.Database, groupExpansions *algo.ReachabilityCache) (cardinality.Duplex[uint64], error) { adminIds := cardinality.NewBitmap64() + + if groupExpansions == nil { + return adminIds, errors.New("groupExpansions is nil") + } return adminIds, db.ReadTransaction(ctx, func(tx graph.Transaction) error {packages/go/analysis/ad/ntlm.go (1)
63-83: Guard against nilLocalGroupDatabefore dereference.
FetchProtectedUsersMappedToDomains(and the call chain fromNewNTLMCache) assumes a non-nillocalGroupDataandGroupMembershipCache. If a caller passes nil, this will panic. Consider failing fast with a clear error.✅ Suggested guard
func FetchProtectedUsersMappedToDomains(ctx context.Context, db graph.Database, localGroupData *LocalGroupData) (map[string]cardinality.Duplex[uint64], error) { + if localGroupData == nil || localGroupData.GroupMembershipCache == nil { + return nil, fmt.Errorf("localGroupData.GroupMembershipCache is required") + } protectedUsers := make(map[string]cardinality.Duplex[uint64])Also applies to: 855-870
cmd/api/src/analysis/membership_integration_test.go (1)
78-78: Migrate away from deprecated integration.NewGraphTestContext.Staticcheck flags this helper as deprecated; prefer the newer integration test utilities to avoid future breakage.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
cmd/api/src/analysis/ad/ad_integration_test.gocmd/api/src/analysis/ad/adcs_integration_test.gocmd/api/src/analysis/ad/ntlm_integration_test.gocmd/api/src/analysis/ad/post.gocmd/api/src/analysis/analysis_integration_test.gocmd/api/src/analysis/membership_integration_test.gocmd/api/src/analysis/post_integration_test.gocmd/api/src/daemons/datapipe/datapipe.gogo.modpackages/go/analysis/ad/ad.gopackages/go/analysis/ad/adcs.gopackages/go/analysis/ad/adcscache.gopackages/go/analysis/ad/esc1.gopackages/go/analysis/ad/esc10.gopackages/go/analysis/ad/esc13.gopackages/go/analysis/ad/esc3.gopackages/go/analysis/ad/esc4.gopackages/go/analysis/ad/esc6.gopackages/go/analysis/ad/esc9.gopackages/go/analysis/ad/esc_shared.gopackages/go/analysis/ad/local_groups.gopackages/go/analysis/ad/membership.gopackages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/owns.gopackages/go/analysis/ad/post.go
💤 Files with no reviewable changes (1)
- cmd/api/src/daemons/datapipe/datapipe.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/analysis/post_integration_test.go
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-05-27T16:58:33.295Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Applied to files:
cmd/api/src/analysis/membership_integration_test.gocmd/api/src/analysis/ad/post.gopackages/go/analysis/ad/local_groups.gocmd/api/src/analysis/analysis_integration_test.gopackages/go/analysis/ad/post.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
cmd/api/src/analysis/membership_integration_test.gopackages/go/analysis/ad/adcscache.gopackages/go/analysis/ad/esc13.gocmd/api/src/analysis/ad/post.gopackages/go/analysis/ad/esc1.gopackages/go/analysis/ad/owns.gopackages/go/analysis/ad/esc_shared.gocmd/api/src/analysis/ad/ntlm_integration_test.gopackages/go/analysis/ad/esc6.gopackages/go/analysis/ad/esc4.gopackages/go/analysis/ad/ntlm.gopackages/go/analysis/ad/esc10.gopackages/go/analysis/ad/adcs.gocmd/api/src/analysis/ad/adcs_integration_test.gopackages/go/analysis/ad/esc9.gopackages/go/analysis/ad/esc3.gopackages/go/analysis/ad/ad.gogo.mod
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
packages/go/analysis/ad/adcscache.gopackages/go/analysis/ad/local_groups.gopackages/go/analysis/ad/owns.gopackages/go/analysis/ad/adcs.gopackages/go/analysis/ad/ad.go
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
packages/go/analysis/ad/adcscache.gopackages/go/analysis/ad/ad.go
📚 Learning: 2025-06-25T18:24:25.014Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T18:24:25.014Z
Learning: In BloodHound Go code, for error types in slog structured logging, prefer using slog.String("key", err.Error()) over slog.Any("key", err). The explicit string conversion with err.Error() is preferred over using slog.Any() for error types.
Applied to files:
packages/go/analysis/ad/adcscache.gopackages/go/analysis/ad/local_groups.gopackages/go/analysis/ad/owns.go
📚 Learning: 2025-07-10T14:31:01.781Z
Learnt from: JonasBK
Repo: SpecterOps/BloodHound PR: 1671
File: packages/go/analysis/ad/esc16.go:65-69
Timestamp: 2025-07-10T14:31:01.781Z
Learning: In BloodHound ADCS ESC analysis functions (PostADCSESC1, PostADCSESC4, PostADCSESC6, PostADCSESC9, PostADCSESC10, PostADCSESC13, PostADCSESC16, etc.), the established pattern for handling cert template validation errors and user filtering errors is to use slog.WarnContext to log warnings and continue processing with the next template, rather than returning errors to the caller. This pattern is consistently applied across all ADCS ESC edge implementations.
Applied to files:
packages/go/analysis/ad/esc13.gocmd/api/src/analysis/ad/post.gopackages/go/analysis/ad/esc1.gopackages/go/analysis/ad/esc6.gopackages/go/analysis/ad/esc4.gopackages/go/analysis/ad/esc10.gopackages/go/analysis/ad/esc9.gopackages/go/analysis/ad/esc3.gopackages/go/analysis/ad/ad.go
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
packages/go/analysis/ad/esc13.gopackages/go/analysis/ad/ntlm.gocmd/api/src/analysis/analysis_integration_test.gopackages/go/analysis/ad/post.go
📚 Learning: 2025-05-23T19:56:13.423Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/BloodHound PR: 1440
File: packages/go/analysis/ad/ntlm.go:123-138
Timestamp: 2025-05-23T19:56:13.423Z
Learning: In the BloodHound NTLM analysis code (packages/go/analysis/ad/ntlm.go), the `isProtectedComputer` function intentionally fails open (returns false/unprotected) when the Protected Users group cannot be retrieved, maintaining consistency with the original logic patterns in the codebase.
Applied to files:
cmd/api/src/analysis/ad/post.gopackages/go/analysis/ad/local_groups.gocmd/api/src/analysis/ad/ntlm_integration_test.gopackages/go/analysis/ad/ntlm.gocmd/api/src/analysis/analysis_integration_test.gopackages/go/analysis/ad/post.go
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
packages/go/analysis/ad/local_groups.go
📚 Learning: 2025-07-10T14:33:20.317Z
Learnt from: JonasBK
Repo: SpecterOps/BloodHound PR: 1671
File: cmd/api/src/analysis/ad/adcs_integration_test.go:3687-3687
Timestamp: 2025-07-10T14:33:20.317Z
Learning: When reviewing Go code, functions defined in one file within a package are accessible from other files in the same package. Before flagging missing functions as compilation errors, check if they exist in other files within the same package directory.
Applied to files:
packages/go/analysis/ad/owns.go
📚 Learning: 2025-08-28T16:43:43.961Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.
Applied to files:
cmd/api/src/analysis/ad/ntlm_integration_test.gocmd/api/src/analysis/ad/adcs_integration_test.go
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/analysis/ad/ad_integration_test.go
📚 Learning: 2025-06-30T22:58:54.834Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1629
File: go.mod:18-18
Timestamp: 2025-06-30T22:58:54.834Z
Learning: Go 1.24 supports patch versions in the go directive of go.mod files. Syntax like "go 1.24.4" is valid and specifies the minimum Go version required to build the module, including the patch level.
Applied to files:
go.mod
📚 Learning: 2025-08-26T18:13:36.643Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1820
File: .github/workflows/audit-release.yml:0-0
Timestamp: 2025-08-26T18:13:36.643Z
Learning: The go tool command supports custom/third-party binaries as of Go 1.24, allowing invocation of custom CLI tools like "go tool stbernard" rather than being limited to only built-in Go toolchain binaries.
Applied to files:
go.mod
🧬 Code graph analysis (15)
cmd/api/src/analysis/membership_integration_test.go (2)
cmd/api/src/test/integration/harnesses.go (1)
HarnessDetails(10107-10221)packages/go/graphschema/ad/ad.go (1)
MemberOfLocalGroup(81-81)
packages/go/analysis/ad/adcscache.go (1)
packages/go/bhlog/measure/measure.go (1)
ContextMeasure(36-45)
packages/go/analysis/ad/esc13.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(539-551)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(477-638)
cmd/api/src/analysis/ad/post.go (2)
packages/go/analysis/post.go (1)
DeleteTransitEdges(164-202)packages/go/analysis/ad/post.go (2)
FetchLocalGroupData(554-588)PostDCSync(73-103)
packages/go/analysis/ad/esc1.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(539-551)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(477-638)
packages/go/analysis/ad/esc_shared.go (2)
packages/go/graphschema/ad/ad.go (1)
Group(31-31)packages/go/analysis/ad/post.go (1)
LocalGroupData(539-551)
cmd/api/src/analysis/ad/ntlm_integration_test.go (2)
packages/go/analysis/ad/ntlm.go (3)
NewNTLMCache(63-124)FetchLDAPSigningCache(892-968)FetchProtectedUsersMappedToDomains(855-880)packages/go/analysis/ad/post.go (2)
LocalGroupData(539-551)FetchLocalGroupData(554-588)
cmd/api/src/analysis/ad/ad_integration_test.go (2)
packages/go/analysis/ad/post.go (3)
FetchLocalGroupData(554-588)PostSyncLAPSPassword(38-71)PostDCSync(73-103)packages/go/analysis/ad/owns.go (1)
PostOwnsAndWriteOwner(38-164)
packages/go/analysis/ad/esc4.go (3)
packages/go/analysis/ad/post.go (1)
LocalGroupData(539-551)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(477-638)
packages/go/analysis/ad/ntlm.go (1)
packages/go/analysis/ad/post.go (1)
LocalGroupData(539-551)
packages/go/analysis/ad/esc10.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(539-551)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)
packages/go/analysis/ad/adcs.go (7)
packages/go/analysis/ad/post.go (1)
LocalGroupData(539-551)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)packages/go/analysis/ad/esc1.go (1)
PostADCSESC1(35-64)packages/go/analysis/ad/esc6.go (2)
PostADCSESC6a(37-80)PostADCSESC6b(82-127)packages/go/analysis/ad/esc9.go (2)
PostADCSESC9a(35-82)PostADCSESC9b(84-128)packages/go/analysis/ad/esc10.go (2)
PostADCSESC10a(36-82)PostADCSESC10b(84-127)packages/go/analysis/ad/esc13.go (1)
PostADCSESC13(36-78)
packages/go/analysis/ad/esc9.go (2)
packages/go/analysis/ad/post.go (1)
LocalGroupData(539-551)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)
packages/go/analysis/ad/esc3.go (3)
packages/go/analysis/ad/post.go (1)
LocalGroupData(539-551)packages/go/analysis/ad/adcscache.go (1)
ADCSCache(33-55)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(477-638)
cmd/api/src/analysis/analysis_integration_test.go (2)
packages/go/analysis/ad/post.go (3)
CanRDPData(627-635)FetchLocalGroupData(554-588)FetchCanRDPEntityBitmapForComputer(470-498)cmd/api/src/test/integration/harnesses.go (1)
RDPHarnessWithCitrix(579-602)
🪛 golangci-lint (2.5.0)
cmd/api/src/analysis/membership_integration_test.go
[minor] 78-78: SA1019: integration.NewGraphTestContext is deprecated: this suite of integration utils is deprecated and should be avoided Integration tests should be updated to reflect the latest standards. See commit a6cc430#diff-e6bcb50873ade3cf33cef4e3e0ff566fb8ac1367b4ade36f4511bc2172a760e1 for implementation guidance. Additional detailed information can be found in Confluence.
(staticcheck)
cmd/api/src/analysis/analysis_integration_test.go
[minor] 45-45: SA1019: integration.NewGraphTestContext is deprecated: this suite of integration utils is deprecated and should be avoided Integration tests should be updated to reflect the latest standards. See commit a6cc430#diff-e6bcb50873ade3cf33cef4e3e0ff566fb8ac1367b4ade36f4511bc2172a760e1 for implementation guidance. Additional detailed information can be found in Confluence.
(staticcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-go-unit-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (39)
packages/go/analysis/ad/adcscache.go (1)
75-76: BuildCache timing instrumentation looks good.Nice, low‑impact observability addition.
packages/go/analysis/ad/membership.go (1)
92-94: Inline reverse direction is fine.This simplifies the traversal without changing behavior.
cmd/api/src/analysis/ad/ad_integration_test.go (1)
1144-1147: LGTM — tests now align with LocalGroupData-based APIs.Good update to the new post-processing flow.
Also applies to: 1175-1178, 1207-1210, 1410-1413
go.mod (1)
21-60: Verify compatibility of dependency bumps via integration tests rather than release notes.The versions
github.com/specterops/dawgs v0.4.2andgithub.com/jackc/pgx/v5 v5.8.0are valid Go module entries (confirmed in go.sum), but public release notes are not available. SinceLocalGroupDataunderwent a recent refactor (BED-7147), run the full integration test suite to confirm no behavioral regressions with these upgraded dependencies, particularly for graph schema operations and database access patterns.Also applies to: 87-87, 276-281
packages/go/analysis/ad/esc1.go (1)
35-35: LGTM — LocalGroupData threaded into ESC1 cross-product.Signature and call-site are aligned with the new LocalGroupData-based cross-product API.
Also applies to: 48-48
packages/go/analysis/ad/esc13.go (1)
36-36: LGTM — ESC13 cross-product now uses LocalGroupData.The signature update and call-site wiring match the updated CalculateCrossProductNodeSets API.
Also applies to: 53-53
packages/go/analysis/ad/esc3.go (1)
38-38: LGTM — ESC3 paths updated consistently for LocalGroupData.Both cross-product call sites and the function signature are aligned with the new API.
Also applies to: 91-97, 109-114
cmd/api/src/analysis/ad/post.go (1)
31-65: LGTM — centralized LocalGroupData reuse in AD post-processing.Fetching LocalGroupData once and reusing it across downstream steps keeps the data flow consistent.
packages/go/analysis/ad/esc4.go (1)
35-35: LGTM — ESC4 cross-product calls aligned with LocalGroupData contract.Signature and all affected call sites now match the updated CalculateCrossProductNodeSets API.
Also applies to: 63-67, 71-76, 88-94, 99-104, 111-116
cmd/api/src/analysis/ad/ntlm_integration_test.go (2)
49-52: LocalGroupData-based NTLM cache setup looks consistent.Also applies to: 96-99
613-630: fetchNTLMPrereqs now centralizes LocalGroupData retrieval — LGTM.packages/go/analysis/ad/owns.go (1)
38-45: LocalGroupData is now wired into Owns/WriteOwner processing as expected.packages/go/analysis/ad/esc9.go (1)
35-54: Victim bitmap now consistently derives from LocalGroupData.Also applies to: 84-103
packages/go/analysis/ad/ad.go (1)
477-623: Cross-product computation now consistently uses LocalGroupData — looks good.cmd/api/src/analysis/analysis_integration_test.go (2)
36-41: Helper to build CanRDPData keeps the tests DRY.
50-58: Per-computer CanRDP data flow updates look consistent.Also applies to: 76-84, 110-114, 142-151, 173-182, 197-204, 222-230
packages/go/analysis/ad/esc_shared.go (2)
27-31: Import update only.
394-399: LocalGroupData integration looks consistent.The victim bitmap now flows through LocalGroupData’s membership cache as expected.
packages/go/analysis/ad/adcs.go (1)
36-71: LocalGroupData propagation is consistent.Passing LocalGroupData through PostADCS into all ESC handlers matches the refactor direction.
Also applies to: 109-207
cmd/api/src/analysis/ad/adcs_integration_test.go (2)
41-53: Prereq helper updated cleanly.Fetching LocalGroupData in the shared helper aligns with the new API surface.
66-3692: Test call sites updated consistently.All ESC post-processing invocations now thread LocalGroupData through; the refactor looks uniform across the integration suite.
packages/go/analysis/ad/esc6.go (1)
37-59: ESC6 LocalGroupData wiring looks good.Signature updates and cross-product calls are consistent with the refactor.
Also applies to: 82-104
packages/go/analysis/ad/esc10.go (1)
36-55: ESC10 LocalGroupData integration looks consistent.Victim bitmap computation now correctly flows through LocalGroupData.
Also applies to: 84-103
packages/go/analysis/ad/ntlm.go (3)
40-46: LGTM: LocalGroupData cached on NTLMCache.The new cache field wiring is straightforward and aligns with the refactor.
126-166: LGTM: PostNTLM now threads LocalGroupData through cache initialization.
442-444: LGTM: Group expansion switched to LocalGroupData reachability cache.Also applies to: 588-589
packages/go/analysis/ad/post.go (11)
26-103: LGTM: PostSyncLAPSPassword/PostDCSync now consume LocalGroupData.
186-204: LGTM: FetchComputerIDs added with timing instrumentation.
285-316: LGTM: LAPS/DCSync cross-product now uses LocalGroupData.
428-442: LGTM: local group lookup by name.
444-498: LGTM: RDP local group membership + CanRDP bitmap computation.
500-535: LGTM: FetchComputersWithURA bitmap caching.
537-588: LGTM: LocalGroupData structure and fetcher wiring.
590-635: LGTM: CanRDPData aggregation from LocalGroupData.
637-666: LGTM: RemoteInteractiveLogonRight cache lookup + per-computer data struct.
668-697: LGTM: FetchCanRDPComputerData flow is consistent.
699-738: LGTM: RDP bitmap selection and URA processing.cmd/api/src/analysis/membership_integration_test.go (2)
30-30: No issues to note for this import change.
83-94: Verify that ReachOfComponentContainingMember is available in DAWGS v0.4.2 and includes the origin member with DirectionInbound.The method
ReachOfComponentContainingMembercould not be located in DAWGS v0.4.1 (the latest public release) or in any publicly available source code. Additionally, go.mod specifies v0.4.2, which does not have a public release or tag on GitHub—only v0.4.1 exists as of Jan 15, 2026. Confirm:
- Whether v0.4.2 is an internal pre-release or if a different version should be used
- The actual method name on
algo.ReachabilityCacheif it differs fromReachOfComponentContainingMember- Whether DirectionInbound includes the origin member as the code comment claims
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| func expandNodeSliceToBitmapWithoutGroups(nodes []*graph.Node, groupExpansions *algo.ReachabilityCache) cardinality.Duplex[uint64] { | ||
| var ( | ||
| bitmap = cardinality.NewBitmap64() | ||
| scratch = cardinality.NewBitmap64() | ||
| ) | ||
|
|
||
| for _, controller := range nodes { | ||
| if controller.Kinds.ContainsOneOf(ad.Group) { | ||
| groupExpansions.Cardinality(controller.ID.Uint64()).(cardinality.Duplex[uint64]).Each(func(id uint64) bool { | ||
| //Check group expansions against each id, if cardinality is 0 than its not a group | ||
| if groupExpansions.Cardinality(id).Cardinality() == 0 { | ||
| bitmap.Add(id) | ||
| groupExpansions.OrReach(controller.ID.Uint64(), graph.DirectionInbound, scratch) | ||
|
|
||
| scratch.Each(func(memberID uint64) bool { | ||
| isGroup := false | ||
|
|
||
| // groupExpansions.Digraph.EachAdjacentNode(memberID, graph.DirectionInbound, func(adjacent uint64) bool { | ||
| // // If the expanded member of the controller group has an adjacent inbound node then it must be a group | ||
| // isGroup = true | ||
| // return false | ||
| // }) | ||
|
|
||
| if !isGroup { | ||
| bitmap.Add(memberID) | ||
| } | ||
|
|
||
| return true | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat packages/go/analysis/ad/esc_shared.goRepository: SpecterOps/BloodHound
Length of output: 15054
🏁 Script executed:
rg "expandNodeSliceToBitmapWithoutGroups" -B 5 -A 5Repository: SpecterOps/BloodHound
Length of output: 3164
🏁 Script executed:
rg "WithoutGroups" --type go -B 2 -A 2Repository: SpecterOps/BloodHound
Length of output: 1823
🏁 Script executed:
git log --all --oneline -S "expandNodeSliceToBitmapWithoutGroups" -- packages/go/analysis/ad/Repository: SpecterOps/BloodHound
Length of output: 195
🏁 Script executed:
git log --all -p --follow -S "isGroup := false" -- packages/go/analysis/ad/esc_shared.go | head -100Repository: SpecterOps/BloodHound
Length of output: 3786
Group filtering is disabled due to commented-out detection logic.
isGroup is never set because the group detection block is commented out, so group IDs are added to the "WithoutGroups" bitmap. This contradicts the function name and can inflate ESC victim expansion results when used in getVictimBitmap() and similar calculations. Confirm whether this is intentional from the recent Local Group Post-Processing rework, and either restore the group filtering or update the function name and callers to reflect the new behavior.
🤖 Prompt for AI Agents
In `@packages/go/analysis/ad/esc_shared.go` around lines 303 - 327, The
expandNodeSliceToBitmapWithoutGroups function currently never filters out group
IDs because the group detection block is commented out; restore the
group-detection logic in expandNodeSliceToBitmapWithoutGroups by using the graph
adjacency iteration on groupExpansions.Digraph (or the appropriate Digraph
instance) to check each memberID for inbound adjacent nodes and set isGroup =
true when any inbound adjacent node exists, then only add non-group memberIDs to
bitmap; if the Local Group Post-Processing rework intentionally removed group
filtering, instead rename the function and update callers (e.g.,
getVictimBitmap) to reflect that it includes groups so behavior is explicit.
| groupSuffix string | ||
| } | ||
|
|
||
| defer measure.ContextMeasure(ctx, slog.LevelInfo, "PostLocalGroups") |
There was a problem hiding this comment.
Fix deferred measure invocation so timing actually runs.
Right now the deferred call schedules ContextMeasure itself instead of the returned closure, so no duration is logged.
🔧 Proposed fix
- defer measure.ContextMeasure(ctx, slog.LevelInfo, "PostLocalGroups")
+ defer measure.ContextMeasure(ctx, slog.LevelInfo, "PostLocalGroups")()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defer measure.ContextMeasure(ctx, slog.LevelInfo, "PostLocalGroups") | |
| defer measure.ContextMeasure(ctx, slog.LevelInfo, "PostLocalGroups")() |
🤖 Prompt for AI Agents
In `@packages/go/analysis/ad/local_groups.go` at line 182, The deferred call
currently defers the ContextMeasure function itself instead of its returned
closure, so change the defer in PostLocalGroups to invoke the returned function;
e.g. call measure.ContextMeasure(ctx, slog.LevelInfo, "PostLocalGroups") and
immediately call the result (defer measure.ContextMeasure(... )()) or assign the
returned func to a variable and defer that (e.g. end :=
measure.ContextMeasure(...); defer end()) so the timing closure actually runs.
Description
TODO
Motivation and Context
Resolves BED-7147
Why is this change required? What problem does it solve?
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.