feat: add Microsoft Entra Domain Services graph support - BED-9245 - #3135
feat: add Microsoft Entra Domain Services graph support - BED-9245#3135martinsohn wants to merge 23 commits into
Conversation
Hybrid post-processing treated an empty PostgreSQL containment traversal and a self-referential AZRunsAs edge as fatal, suppressing otherwise valid Entra DS relationships. Treat missing containment as absent evidence and ignore self-loops that cannot identify distinct application and service-principal endpoints. Add regressions for both live-data shapes.
📝 WalkthroughWalkthroughThe pull request adds Microsoft Entra Domain Services support across Azure ingestion, graph schemas, post-processing, edge composition, APIs, generated schemas, searches, and the user interface. It also adds synchronization, management, membership, and related relationship types with integration and UI tests. ChangesEntra Domain Services graph and ingestion
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AzureCollector
participant Graphify
participant PostProcessing
participant GraphAPI
participant WebUI
AzureCollector->>Graphify: ingest Entra Domain Services and role assignments
Graphify->>PostProcessing: provide EntraDS nodes and contributor relationships
PostProcessing->>PostProcessing: resolve contributors and directory roles
PostProcessing->>GraphAPI: create AZManageEntraDS and synchronization relationships
WebUI->>GraphAPI: request Domain Services details and edge composition
GraphAPI->>WebUI: return entity details and composed graph paths
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
api
![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
🧹 Nitpick comments (7)
packages/go/ein/azure_domain_service_test.go (1)
35-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGroup independent variable initializations in
var (...)blocks.
packages/go/ein/azure_domain_service_test.go#L35-L37: GroupingestTimeanddatain avar (...)block.packages/go/ein/azure_domain_service_test.go#L135-L138: GroupresourceIDandprincipalIDin avar (...)block.As per coding guidelines, “When possible, group variable initializations in a
var (...)block and hoist them to the top of the function.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/go/ein/azure_domain_service_test.go` around lines 35 - 37, In TestConvertAzureDomainServiceToNode, group the ingestTime and data initializations in a single var block at the top of the function; likewise, group resourceID and principalID in a var block at the second affected initialization site in packages/go/ein/azure_domain_service_test.go (lines 135-138).Source: Coding guidelines
packages/go/analysis/azure/azure_integration_test.go (1)
1243-1256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the assertions out of the
ReadTransactionclosure.Lines 1245-1253 call
requireandassertinside the transaction callback.require.NoErrorandrequire.Lencallt.FailNow(), which callsruntime.Goexit(). Testify documents thatFailNowmust run on the goroutine that runs the test function. If dawgs executes the callback on another goroutine, the exit aborts that goroutine instead of the test, and the failure is reported incorrectly.The closure also discards
errat Line 1245 instead of returning it to the transaction machinery.Collect the edges inside the closure. Assert after
require.NoError(t, err)at Line 1256.♻️ Proposed refactor
var manageEdges []*graph.Relationship err = suite.GraphDB.ReadTransaction(suite.Context, func(tx graph.Transaction) error { edges, err := ops.FetchRelationships(tx.Relationships().Filter(query.Kind(query.Relationship(), graphAzure.ManageEntraDS))) - require.NoError(t, err) - require.Len(t, edges, 2) - manageEdges = edges - - actualSources := []graph.ID{edges[0].StartID, edges[1].StartID} - assert.ElementsMatch(t, []graph.ID{qualifiedUser.ID, domainServicesContributor.ID}, actualSources) - for _, edge := range edges { - assert.Equal(t, domainService.ID, edge.EndID) - } - return nil + if err != nil { + return err + } + + manageEdges = edges + return nil }) require.NoError(t, err) + require.Len(t, manageEdges, 2) + + actualSources := make([]graph.ID, 0, len(manageEdges)) + for _, edge := range manageEdges { + actualSources = append(actualSources, edge.StartID) + assert.Equal(t, domainService.ID, edge.EndID) + } + assert.ElementsMatch(t, []graph.ID{qualifiedUser.ID, domainServicesContributor.ID}, actualSources)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/go/analysis/azure/azure_integration_test.go` around lines 1243 - 1256, Update the ReadTransaction callback to only fetch and return the relationships error while assigning the resulting edges to manageEdges; remove all require/assert calls from the closure. After require.NoError(t, err), validate manageEdges length, source IDs, and destination IDs on the test goroutine, and ensure the callback returns the FetchRelationships error to the transaction machinery.packages/go/analysis/hybrid/hybrid.go (2)
362-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGroup the four same-typed map parameters into a struct.
addManageEntraDSSyncEdgestakessyncedToEntraDSGroupEdgeMap,entraDSForEdgeMap,manageEntraDSSyncEdgeMap, andmanageEntraDSSyncFilterEdgeMapas four adjacentmap[graph.ID][]graph.IDparameters. The compiler cannot detect a transposed argument at the call site inPostHybrid. A small struct makes each target explicit.♻️ Proposed refactor
type entraDSSyncEdgeMaps struct { syncedToEntraDSGroups map[graph.ID][]graph.ID entraDSFor map[graph.ID][]graph.ID manageEntraDSSync map[graph.ID][]graph.ID manageEntraDSSyncFilter map[graph.ID][]graph.ID } func addManageEntraDSSyncEdges(tx graph.Transaction, adGroups []*graph.Node, entraDSAdminGroupTenantMap map[graph.ID]string, edgeMaps entraDSSyncEdgeMaps) error {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/go/analysis/hybrid/hybrid.go` at line 362, Introduce an entraDSSyncEdgeMaps struct containing the four map[graph.ID][]graph.ID fields, update addManageEntraDSSyncEdges to accept this struct instead of four separate map parameters, and adjust the PostHybrid call site and function references to use the corresponding named fields.
650-655: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPush the end-node filter into the relationship query.
This fetch loads every
AZAddMembersandAZOwnsrelationship in the graph into memory, then discards all of them whose end node is not an Entra DS-synced AZGroup.azGroupToADGroupsalready holds the complete set of eligible end-node IDs at this point. Add the ID filter to the query so the database performs the restriction.⚡ Proposed change
+ azGroupIDs := make([]graph.ID, 0, len(azGroupToADGroups)) + for azGroupID := range azGroupToADGroups { + azGroupIDs = append(azGroupIDs, azGroupID) + } + memberAddEdges, err := ops.FetchRelationships(tx.Relationships().Filterf(func() graph.Criteria { - return query.KindIn(query.Relationship(), azure.AddMembers, azure.Owns) + return query.And( + query.KindIn(query.Relationship(), azure.AddMembers, azure.Owns), + query.InIDs(query.EndID(), azGroupIDs...), + ) }))Confirm that the driver handles a large
InIDslist acceptably before you apply this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/go/analysis/hybrid/hybrid.go` around lines 650 - 655, Update the relationship query used to populate memberAddEdges so it filters end-node IDs using the eligible IDs already held by azGroupToADGroups, alongside the existing azure.AddMembers and azure.Owns kind filter. Use the driver's supported InIDs predicate and confirm it handles the expected list size before applying the change; preserve the existing error handling.packages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSUser/WindowsAbuse.tsx (1)
20-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the component and differentiate the platform guidance.
The component is named
Abuse, but the file isWindowsAbuse.tsx. The body is also identical topackages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSUser/LinuxAbuse.tsx. Rename the component toWindowsAbuse. See the consolidated comment for the duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSUser/WindowsAbuse.tsx` around lines 20 - 50, Rename the component declaration and default export in WindowsAbuse.tsx from Abuse to WindowsAbuse, keeping the existing Windows-specific guidance unchanged.packages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSUser/LinuxAbuse.tsx (1)
20-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the component and remove the duplicated content.
The component is named
Abuse, but the file isLinuxAbuse.tsx. The whole component body is also identical topackages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSUser/WindowsAbuse.tsx. Rename the component toLinuxAbusefor clarity. See the consolidated comment for the duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSUser/LinuxAbuse.tsx` around lines 20 - 50, Rename the component in LinuxAbuse.tsx from Abuse to LinuxAbuse and update its default export accordingly. Remove the duplicated component content by reusing the shared or consolidated implementation used by WindowsAbuse, while preserving the Linux-specific entry point and rendered behavior.packages/go/analysis/hybrid/hybrid_integration_test.go (1)
694-699: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGroup the variable declarations into one
var (...)block.Lines 696-699 declare four variables in four separate statements. Group them into a single
var (...)block at the top of the function.♻️ Proposed refactor
testContext := integration.NewGraphTestContext(t, graphschema.DefaultGraphSchema()) - var syncHarness manageEntraDSSyncHarness - var container *graph.Node - var unrelatedDomainService *graph.Node - var unrelatedDomain *graph.Node + var ( + syncHarness manageEntraDSSyncHarness + container *graph.Node + unrelatedDomainService *graph.Node + unrelatedDomain *graph.Node + )As per coding guidelines: "When possible, group variable initializations in a
var (...)block and hoist them to the top of the function."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/go/analysis/hybrid/hybrid_integration_test.go` around lines 694 - 699, Group the declarations of testContext, syncHarness, container, unrelatedDomainService, and unrelatedDomain into a single var (...) block at the top of TestGetManageEntraDSSyncEdgeComposition, preserving their existing types and initialization behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cue/bh/azure/azure.cue`:
- Around line 1192-1195: The derived relationship lists in
packages/cue/bh/azure/azure.cue are missing EntraDS kinds: at lines 1192-1195,
add SyncedToEntraDSGroup and EntraDSFor to InboundOutboundRelationshipKinds; at
lines 1034-1035, add EntraDSContributor alongside Contributor in both
ControlRelationshipKinds and InboundOutboundRelationshipKinds. Regenerate
packages/go/graphschema/azure/azure.go and
packages/javascript/bh-shared-ui/src/graphSchema.ts.
In `@packages/go/analysis/azure/domain_service.go`:
- Around line 26-40: The Azure *EntityDetails functions currently rely on
unspecified return evaluation order when returning details alongside
db.ReadTransaction. Update all 22 matching EntityDetails functions, including
DomainServiceEntityDetails, to explicitly execute the transaction, capture its
error, and return the populated details with that error afterward.
In `@packages/go/analysis/hybrid/composition.go`:
- Around line 144-158: Update the containment traversal error handling in
GetManageEntraDSSyncEdgeComposition so graph.ErrNoResultsFound is treated like
an empty containmentPaths result: continue to the next correlation path.
Preserve returning other traversal errors unchanged.
In `@packages/go/analysis/hybrid/hybrid_integration_test.go`:
- Around line 449-459: Update the relationship-fetch block around
FetchRelationships to use require.Len for the edges count assertion before
indexing edges[0]. Keep the existing assertion and edge assignment behavior
otherwise unchanged, matching the require-based block later in the test.
- Line 911: Update setupEntraDSGroupMemberHarness to use an unnamed return type
list, then declare azUser, adUser, azGroup, and adGroup inside the function body
while preserving the existing return behavior and documentation.
In `@packages/go/analysis/hybrid/hybrid.go`:
- Around line 343-355: Update addEntraDSAdminGroupTenant to retrieve both name
and tenantID through normalizedNodeProperty, treating missing properties as
absent rather than returning graph.ErrPropertyNotFound; preserve prefix matching
and normalized tenant mapping. Remove the now-redundant uppercase/trim
operations because normalizedNodeProperty handles them. Apply the same
missing-property handling to addNodeToObjectIDMap for objectid.
In `@packages/javascript/bh-shared-ui/src/commonSearchesAGI.ts`:
- Around line 486-488: Update the description associated with the query matching
the AZManageEntraDS edge so it names AZManageEntraDS instead of
ManageEntraDSSync, while preserving the remaining description text.
In `@packages/javascript/bh-shared-ui/src/graphSchema.ts`:
- Around line 1398-1401: Add AzureRelationshipKind.SyncedToEntraDSGroup to the
AzurePathfindingEdges collection alongside the existing Entra DS relationship
kinds, preserving all current entries.
In
`@packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter/edgeCategories.tsx`:
- Around line 227-233: Update the Cross Platform category’s edgeTypes list to
include AzureRelationshipKind.SyncedToEntraDSGroup alongside the existing Azure
relationship kinds, preserving all current entries.
---
Nitpick comments:
In `@packages/go/analysis/azure/azure_integration_test.go`:
- Around line 1243-1256: Update the ReadTransaction callback to only fetch and
return the relationships error while assigning the resulting edges to
manageEdges; remove all require/assert calls from the closure. After
require.NoError(t, err), validate manageEdges length, source IDs, and
destination IDs on the test goroutine, and ensure the callback returns the
FetchRelationships error to the transaction machinery.
In `@packages/go/analysis/hybrid/hybrid_integration_test.go`:
- Around line 694-699: Group the declarations of testContext, syncHarness,
container, unrelatedDomainService, and unrelatedDomain into a single var (...)
block at the top of TestGetManageEntraDSSyncEdgeComposition, preserving their
existing types and initialization behavior.
In `@packages/go/analysis/hybrid/hybrid.go`:
- Line 362: Introduce an entraDSSyncEdgeMaps struct containing the four
map[graph.ID][]graph.ID fields, update addManageEntraDSSyncEdges to accept this
struct instead of four separate map parameters, and adjust the PostHybrid call
site and function references to use the corresponding named fields.
- Around line 650-655: Update the relationship query used to populate
memberAddEdges so it filters end-node IDs using the eligible IDs already held by
azGroupToADGroups, alongside the existing azure.AddMembers and azure.Owns kind
filter. Use the driver's supported InIDs predicate and confirm it handles the
expected list size before applying the change; preserve the existing error
handling.
In `@packages/go/ein/azure_domain_service_test.go`:
- Around line 35-37: In TestConvertAzureDomainServiceToNode, group the
ingestTime and data initializations in a single var block at the top of the
function; likewise, group resourceID and principalID in a var block at the
second affected initialization site in
packages/go/ein/azure_domain_service_test.go (lines 135-138).
In
`@packages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSUser/LinuxAbuse.tsx`:
- Around line 20-50: Rename the component in LinuxAbuse.tsx from Abuse to
LinuxAbuse and update its default export accordingly. Remove the duplicated
component content by reusing the shared or consolidated implementation used by
WindowsAbuse, while preserving the Linux-specific entry point and rendered
behavior.
In
`@packages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSUser/WindowsAbuse.tsx`:
- Around line 20-50: Rename the component declaration and default export in
WindowsAbuse.tsx from Abuse to WindowsAbuse, keeping the existing
Windows-specific guidance unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 953f6764-0e69-4d52-b00c-f817ff34f49e
📒 Files selected for processing (80)
.gitignorecmd/api/src/api/v2/azure.gocmd/api/src/api/v2/azure_test.gocmd/api/src/api/v2/edge.gocmd/api/src/database/migration/extensions/az_graph_schema.sqlcmd/api/src/services/graphify/azure_convertors.gocmd/ui/src/ducks/graph/graphutils.tscmd/ui/src/ducks/graph/types.tspackages/csharp/graphschema/PropertyNames.cspackages/cue/bh/ad/ad.cuepackages/cue/bh/azure/azure.cuepackages/cue/bh/bh.cuepackages/go/analysis/azure/azure_integration_test.gopackages/go/analysis/azure/domain_service.gopackages/go/analysis/azure/entra_domain_services.gopackages/go/analysis/azure/model.gopackages/go/analysis/azure/post.gopackages/go/analysis/edgecomposition/edgecomposition.gopackages/go/analysis/hybrid/composition.gopackages/go/analysis/hybrid/hybrid.gopackages/go/analysis/hybrid/hybrid_integration_test.gopackages/go/analysis/hybrid/hybrid_test.gopackages/go/analysis/post/post_integration_test.gopackages/go/ein/azure.gopackages/go/ein/azure_domain_service.gopackages/go/ein/azure_domain_service_test.gopackages/go/graphschema/ad/ad.gopackages/go/graphschema/azure/azure.gopackages/go/graphschema/azure/azure_test.gopackages/go/graphschema/common/common.gopackages/go/schemagen/generator/sql.gopackages/go/schemagen/generator/typescript.gopackages/go/schemagen/main.gopackages/go/schemagen/model/schema.gopackages/javascript/bh-shared-ui/src/commonSearches.test.tspackages/javascript/bh-shared-ui/src/commonSearchesAGI.tspackages/javascript/bh-shared-ui/src/commonSearchesAGT.tspackages/javascript/bh-shared-ui/src/components/HelpTexts/AZContains/General.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AZContributor/Abuse.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AZContributor/References.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AZEntraDSContributor/AZEntraDSContributor.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AZEntraDSContributor/Abuse.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AZEntraDSContributor/General.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AZEntraDSContributor/Opsec.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AZEntraDSContributor/References.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AZManageEntraDS/AZManageEntraDS.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AZManageEntraDS/Composition.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AZOwner/General.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AZUserAccessAdministrator/Abuse.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AddEntraDSGroupMember/AddEntraDSGroupMember.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AddEntraDSGroupMember/Composition.test.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AddEntraDSGroupMember/Composition.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AddEntraDSGroupMember/General.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AddEntraDSGroupMember/LinuxAbuse.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AddEntraDSGroupMember/Opsec.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AddEntraDSGroupMember/References.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/AddEntraDSGroupMember/WindowsAbuse.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/EntraDSFor/EntraDSFor.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/ManageEntraDSSync/Composition.test.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/ManageEntraDSSync/Composition.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/ManageEntraDSSync/ManageEntraDSSync.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/ManageEntraDSSyncFilter/ManageEntraDSSyncFilter.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSGroup/General.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSGroup/References.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSGroup/SyncedToEntraDSGroup.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSUser/General.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSUser/LinuxAbuse.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSUser/Opsec.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSUser/References.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSUser/SyncedToEntraDSUser.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/SyncedToEntraDSUser/WindowsAbuse.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/index.tsxpackages/javascript/bh-shared-ui/src/graphSchema.tspackages/javascript/bh-shared-ui/src/hooks/useExploreGraph/useExploreGraph.test.tsxpackages/javascript/bh-shared-ui/src/hooks/useExploreGraph/useExploreGraph.tsxpackages/javascript/bh-shared-ui/src/utils/content.tspackages/javascript/bh-shared-ui/src/utils/icons.tspackages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoContent.test.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter/edgeCategories.tsxschemas/valid_edges.json
💤 Files with no reviewable changes (1)
- packages/go/schemagen/generator/typescript.go
| SyncedToEntraDSUser, | ||
| AddEntraDSGroupMember, | ||
| ManageEntraDSSync, | ||
| ManageEntraDSSyncFilter, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Incomplete propagation of new EntraDS kinds into the derived relationship lists in packages/cue/bh/azure/azure.cue. Every new EntraDS relationship kind is registered in RelationshipKinds and PostProcessedRelationships, but three kinds are not carried into the derived membership lists that drive object-control counts and the Explore inbound/outbound panels. One incomplete registration pass explains all three gaps.
packages/cue/bh/azure/azure.cue#L1192-L1195: addSyncedToEntraDSGroupandEntraDSFortoInboundOutboundRelationshipKinds, or add a comment that records why they are excluded whileSyncedToEntraDSUseris included.packages/cue/bh/azure/azure.cue#L1034-L1035: addEntraDSContributortoControlRelationshipKindsandInboundOutboundRelationshipKindsnext toContributor, or confirm the exclusion is intended.packages/go/analysis/azure/entra_domain_services.gotreatsEntraDSContributorandContributoras equivalent control edges at Lines 96 and 327.
Regenerate packages/go/graphschema/azure/azure.go and packages/javascript/bh-shared-ui/src/graphSchema.ts after any change.
📍 Affects 1 file
packages/cue/bh/azure/azure.cue#L1192-L1195(this comment)packages/cue/bh/azure/azure.cue#L1034-L1035
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cue/bh/azure/azure.cue` around lines 1192 - 1195, The derived
relationship lists in packages/cue/bh/azure/azure.cue are missing EntraDS kinds:
at lines 1192-1195, add SyncedToEntraDSGroup and EntraDSFor to
InboundOutboundRelationshipKinds; at lines 1034-1035, add EntraDSContributor
alongside Contributor in both ControlRelationshipKinds and
InboundOutboundRelationshipKinds. Regenerate
packages/go/graphschema/azure/azure.go and
packages/javascript/bh-shared-ui/src/graphSchema.ts.
| func DomainServiceEntityDetails(ctx context.Context, db graph.Database, primaryDisplayKinds graphschema.PrimaryDisplayKinds, objectID string, hydrateCounts bool) (DomainServiceDetails, error) { | ||
| var details DomainServiceDetails | ||
|
|
||
| return details, db.ReadTransaction(ctx, func(tx graph.Transaction) error { | ||
| if node, err := FetchEntityByObjectID(tx, objectID); err != nil { | ||
| return err | ||
| } else { | ||
| details.Node = FromGraphNode(primaryDisplayKinds, node) | ||
| if hydrateCounts { | ||
| details, err = PopulateDomainServiceEntityDetailsCounts(tx, node, details) | ||
| } | ||
| return err | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Determine whether sibling *EntityDetails functions use the same return-operand pattern.
set -euo pipefail
echo "=== EntityDetails functions in the azure analysis package ==="
ast-grep outline packages/go/analysis/azure --items all --type function --match 'EntityDetails'
echo "=== 'return <var>, db.ReadTransaction(' occurrences repo-wide ==="
rg -nP -C2 'return\s+\w+,\s*db\.ReadTransaction\(' --type=goRepository: SpecterOps/BloodHound
Length of output: 2809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== DomainServiceEntityDetails and sibling implementations ==="
for file in packages/go/analysis/azure/{domain_service,service_principal,vm_scale_set,base,management_group,resource_group,application,managed_cluster,tenant,subscription,device,vm,container_registry,role,federated_identity_credentials,key_vault,user,web_app,automation_account,logic_app,group,function_app}.go; do
if [ -f "$file" ]; then
echo "--- $file"
rg -n -A22 -B3 'func [A-Za-z0-9_]*EntityDetails\\(' "$file" || true
fi
done
echo "=== Transaction-return patterns in the package ==="
rg -n -P -C3 'return\\s+[^\\n]*,\\s*(?:[A-Za-z0-9_]+\\.)?ReadTransaction\\(' packages/go/analysis/azure --type=go || true
echo "=== Go tool availability ==="
go version 2>/dev/null || trueRepository: SpecterOps/BloodHound
Length of output: 3494
🌐 Web query:
Go language specification order of evaluation return statement multiple result expressions function call
💡 Result:
In the Go programming language, the order of evaluation for expressions in a return statement (as well as in assignments and function calls) is governed by specific rules defined in the Go Language Specification [1][2]. When evaluating the operands of a return statement containing multiple result expressions, the following behavior applies: 1. Function calls, method calls, and channel communication operations are evaluated in strict lexical left-to-right order [3][4][2]. 2. The evaluation order of other operands (such as variables, constants, or arithmetic expressions that do not contain calls or communication) relative to these function/method/communication operations is not specified [3][4][5][6]. Because the spec does not define a total ordering for all elements, a return statement such as return a, f() (where a is a variable and f() is a function call) does not guarantee whether a is evaluated before or after f() is called [3][4][6]. If f() modifies a, the resulting return value is implementation-dependent and should not be relied upon [7][4][8]. These rules are consistent across assignments, return statements, and argument lists in function calls to allow compilers flexibility in optimization, such as managing register pressure and instruction scheduling [3][4][6]. Because this behavior is intentionally left partially unspecified, it is generally considered best practice to avoid code that depends on the relative evaluation order of mixed side-effect-producing and non-side-effect-producing expressions [4][5].
Citations:
- 1: https://go.dev/ref/spec
- 2: https://go101.org/article/evaluation-orders.html
- 3: spec: evaluation order in assignments return statements golang/go#3852
- 4: https://groups.google.com/g/golang-nuts/c/Q7KVGTFt3nU
- 5: https://stackoverflow.com/questions/71762407/what-is-the-evaluation-order-of-function-arguments
- 6: https://groups.google.com/g/golang-nuts/c/z2Sgt8cD2Is/m/N0IeYvuNCAAJ
- 7: spec: order of evaluation of variables in return statement is not determined golang/go#25609
- 8: https://groups.google.com/g/golang-nuts/c/HQ59BBbhCT0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== EntityDetails implementations ==="
rg -n -U -P -A22 -B3 'func [A-Za-z0-9_]+EntityDetails\([^)]*\)[^{]*\{' packages/go/analysis/azure --type=go || true
echo "=== ReadTransaction return expressions ==="
rg -n -F -C3 'ReadTransaction(ctx' packages/go/analysis/azure --type=go || true
echo "=== Exact return-call patterns ==="
rg -n -F -C2 'return details, db.ReadTransaction(' packages/go/analysis/azure --type=go || true
rg -n -F -C2 'return details, database.ReadTransaction(' packages/go/analysis/azure --type=go || true
echo "=== Local Go specification wording ==="
for spec in /usr/local/go/doc/go_spec.html /usr/share/doc/golang-go/go_spec.html; do
if [ -f "$spec" ]; then
echo "--- $spec"
rg -n -A10 -B5 'order of evaluation|return statement' "$spec" | head -120
fi
doneRepository: SpecterOps/BloodHound
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== EntityDetails return-pattern counts ==="
printf 'same pattern: '
rg -l -F 'return details, db.ReadTransaction(' packages/go/analysis/azure --type=go | wc -l
printf 'explicit txErr pattern: '
rg -l -F 'txErr := db.ReadTransaction(' packages/go/analysis/azure --type=go | wc -l
echo "=== Local Go specification evaluation-order section ==="
spec=/usr/local/go/doc/go_spec.html
if [ -f "$spec" ]; then
rg -n -i -A8 -B8 'order of evaluation' "$spec" | head -80
else
echo "local specification not found"
fi
echo "=== Standalone compiler behavior for a captured return operand ==="
tmpdir=$(mktemp -d)
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/main.go" <<'EOF'
package main
import "fmt"
var value int
func mutate() error {
value = 42
return nil
}
func mixed() (int, error) {
value = 0
return value, mutate()
}
func explicit() (int, error) {
value = 0
err := mutate()
return value, err
}
func main() {
mixedValue, mixedErr := mixed()
explicitValue, explicitErr := explicit()
fmt.Printf("mixed=%d,%v explicit=%d,%v\n", mixedValue, mixedErr, explicitValue, explicitErr)
}
EOF
(cd "$tmpdir" && go run main.go)Repository: SpecterOps/BloodHound
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -u
echo "=== Local Go specification evaluation-order section ==="
spec=/usr/local/go/doc/go_spec.html
if [ -f "$spec" ]; then
rg -n -i -A8 -B8 'order of evaluation' "$spec" | head -80 || true
else
echo "local specification not found"
fi
echo "=== Standalone compiler behavior for a captured return operand ==="
tmpdir=$(mktemp -d)
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/main.go" <<'EOF'
package main
import "fmt"
var value int
func mutate() error {
value = 42
return nil
}
func mixed() (int, error) {
value = 0
return value, mutate()
}
func explicit() (int, error) {
value = 0
err := mutate()
return value, err
}
func main() {
mixedValue, mixedErr := mixed()
explicitValue, explicitErr := explicit()
fmt.Printf("mixed=%d,%v explicit=%d,%v\n", mixedValue, mixedErr, explicitValue, explicitErr)
}
EOF
(cd "$tmpdir" && go run main.go)Repository: SpecterOps/BloodHound
Length of output: 1104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
spec=/usr/local/go/doc/go_spec.html
if [ -f "$spec" ]; then
sed -n '5836,5875p' "$spec"
else
echo "local specification not found"
fiRepository: SpecterOps/BloodHound
Length of output: 1840
Make return evaluation explicit in all Azure *EntityDetails functions. Go leaves the order between details and db.ReadTransaction unspecified. Update the 22 matching functions consistently instead of changing only this function.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/go/analysis/azure/domain_service.go` around lines 26 - 40, The Azure
*EntityDetails functions currently rely on unspecified return evaluation order
when returning details alongside db.ReadTransaction. Update all 22 matching
EntityDetails functions, including DomainServiceEntityDetails, to explicitly
execute the transaction, capture its error, and return the populated details
with that error afterward.
| containmentPaths, err := ops.TraversePaths(tx, ops.TraversalPlan{ | ||
| Root: domain, | ||
| Direction: graph.DirectionOutbound, | ||
| BranchQuery: func() graph.Criteria { | ||
| return query.Kind(query.Relationship(), graphschemaAD.Contains) | ||
| }, | ||
| PathFilter: func(_ *ops.TraversalContext, segment *graph.PathSegment) bool { | ||
| return segment.Node.ID == targetGroup.ID | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } else if containmentPaths.Len() == 0 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle graph.ErrNoResultsFound from the containment traversal.
filterContainedDomainUsers in packages/go/analysis/hybrid/hybrid.go (lines 571-579) documents that PostgreSQL reports an empty traversal as graph.ErrNoResultsFound, while other drivers return an empty PathSet. This traversal uses the same plan but treats every error as fatal. On PostgreSQL, a domain with no matching containment path makes GetManageEntraDSSyncEdgeComposition return an error. The API then responds with HTTP 500 instead of an empty composition.
Treat the empty-traversal error as "no containment evidence" and continue to the next correlation path.
🐛 Proposed fix
containmentPaths, err := ops.TraversePaths(tx, ops.TraversalPlan{
Root: domain,
Direction: graph.DirectionOutbound,
BranchQuery: func() graph.Criteria {
return query.Kind(query.Relationship(), graphschemaAD.Contains)
},
PathFilter: func(_ *ops.TraversalContext, segment *graph.PathSegment) bool {
return segment.Node.ID == targetGroup.ID
},
})
- if err != nil {
+ if errors.Is(err, graph.ErrNoResultsFound) {
+ continue
+ } else if err != nil {
return err
} else if containmentPaths.Len() == 0 {
continue
}📝 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.
| containmentPaths, err := ops.TraversePaths(tx, ops.TraversalPlan{ | |
| Root: domain, | |
| Direction: graph.DirectionOutbound, | |
| BranchQuery: func() graph.Criteria { | |
| return query.Kind(query.Relationship(), graphschemaAD.Contains) | |
| }, | |
| PathFilter: func(_ *ops.TraversalContext, segment *graph.PathSegment) bool { | |
| return segment.Node.ID == targetGroup.ID | |
| }, | |
| }) | |
| if err != nil { | |
| return err | |
| } else if containmentPaths.Len() == 0 { | |
| continue | |
| } | |
| containmentPaths, err := ops.TraversePaths(tx, ops.TraversalPlan{ | |
| Root: domain, | |
| Direction: graph.DirectionOutbound, | |
| BranchQuery: func() graph.Criteria { | |
| return query.Kind(query.Relationship(), graphschemaAD.Contains) | |
| }, | |
| PathFilter: func(_ *ops.TraversalContext, segment *graph.PathSegment) bool { | |
| return segment.Node.ID == targetGroup.ID | |
| }, | |
| }) | |
| if errors.Is(err, graph.ErrNoResultsFound) { | |
| continue | |
| } else if err != nil { | |
| return err | |
| } else if containmentPaths.Len() == 0 { | |
| continue | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/go/analysis/hybrid/composition.go` around lines 144 - 158, Update
the containment traversal error handling in GetManageEntraDSSyncEdgeComposition
so graph.ErrNoResultsFound is treated like an empty containmentPaths result:
continue to the next correlation path. Preserve returning other traversal errors
unchanged.
| db.ReadTransaction(context.Background(), func(tx graph.Transaction) error { | ||
| edges, err := ops.FetchRelationships(tx.Relationships().Filterf(func() graph.Criteria { | ||
| return query.Kind(query.Relationship(), azure.AddEntraDSGroupMember) | ||
| })) | ||
| assert.Nil(t, err) | ||
| assert.Len(t, edges, 1) | ||
| edge = edges[0] | ||
| return nil | ||
| }) | ||
|
|
||
| composition, err := GetAddEntraDSGroupMemberEdgeComposition(context.Background(), db, edge) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use require.Len before indexing edges.
Line 454 uses assert.Len. assert records the failure and continues. If the query returns no relationships, line 455 indexes edges[0] and the test panics with an index-out-of-range error. edge also stays nil and line 459 passes nil into GetAddEntraDSGroupMemberEdgeComposition. The same block at lines 733-734 already uses require. Apply the same approach here.
🐛 Proposed fix
var edge *graph.Relationship
db.ReadTransaction(context.Background(), func(tx graph.Transaction) error {
edges, err := ops.FetchRelationships(tx.Relationships().Filterf(func() graph.Criteria {
return query.Kind(query.Relationship(), azure.AddEntraDSGroupMember)
}))
- assert.Nil(t, err)
- assert.Len(t, edges, 1)
+ require.NoError(t, err)
+ require.Len(t, edges, 1)
edge = edges[0]
return 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.
| db.ReadTransaction(context.Background(), func(tx graph.Transaction) error { | |
| edges, err := ops.FetchRelationships(tx.Relationships().Filterf(func() graph.Criteria { | |
| return query.Kind(query.Relationship(), azure.AddEntraDSGroupMember) | |
| })) | |
| assert.Nil(t, err) | |
| assert.Len(t, edges, 1) | |
| edge = edges[0] | |
| return nil | |
| }) | |
| composition, err := GetAddEntraDSGroupMemberEdgeComposition(context.Background(), db, edge) | |
| db.ReadTransaction(context.Background(), func(tx graph.Transaction) error { | |
| edges, err := ops.FetchRelationships(tx.Relationships().Filterf(func() graph.Criteria { | |
| return query.Kind(query.Relationship(), azure.AddEntraDSGroupMember) | |
| })) | |
| require.NoError(t, err) | |
| require.Len(t, edges, 1) | |
| edge = edges[0] | |
| return nil | |
| }) | |
| composition, err := GetAddEntraDSGroupMemberEdgeComposition(context.Background(), db, edge) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/go/analysis/hybrid/hybrid_integration_test.go` around lines 449 -
459, Update the relationship-fetch block around FetchRelationships to use
require.Len for the edges count assertion before indexing edges[0]. Keep the
existing assertion and edge assignment behavior otherwise unchanged, matching
the require-based block later in the test.
| // (AZAddMembers / AZOwns) between them. When syncUser/syncGroup are true, matching on-prem AD User/Group nodes are | ||
| // created (via ad.AADObjectID) so the corresponding SyncedToEntraDS edges are produced by PostHybrid. Pass an empty | ||
| // kind as controlKind to omit the control edge entirely. Returns the AZUser, on-prem User, AZGroup, on-prem Group. | ||
| func setupEntraDSGroupMemberHarness(t *testing.T, testContext *integration.GraphTestContext, controlKind graph.Kind, syncUser, syncGroup bool) (azUser, adUser, azGroup, adGroup *graph.Node) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the named return parameters.
setupEntraDSGroupMemberHarness declares (azUser, adUser, azGroup, adGroup *graph.Node) as named results. Declare the variables inside the function body instead and keep the return type list unnamed.
♻️ Proposed refactor
-func setupEntraDSGroupMemberHarness(t *testing.T, testContext *integration.GraphTestContext, controlKind graph.Kind, syncUser, syncGroup bool) (azUser, adUser, azGroup, adGroup *graph.Node) {
+func setupEntraDSGroupMemberHarness(t *testing.T, testContext *integration.GraphTestContext, controlKind graph.Kind, syncUser, syncGroup bool) (*graph.Node, *graph.Node, *graph.Node, *graph.Node) {
t.Helper()
+ var (
+ adUser *graph.Node
+ adGroup *graph.Node
+ )
+
tenantID := integration.RandomObjectID(t)
tenant := testContext.NewAzureTenant(tenantID)
azUserObjectID := integration.RandomObjectID(t)
azGroupObjectID := integration.RandomObjectID(t)
- azUser = testContext.NewAzureUser("AZ User", "azuser@specter.dev", "", azUserObjectID, "", tenantID, false)
- azGroup = testContext.NewAzureGroup("AZ Group", azGroupObjectID, tenantID)
+ azUser := testContext.NewAzureUser("AZ User", "azuser@specter.dev", "", azUserObjectID, "", tenantID, false)
+ azGroup := testContext.NewAzureGroup("AZ Group", azGroupObjectID, tenantID)The documentation comment at lines 907-910 already names the returned values, so readability is preserved.
As per coding guidelines: "Do not use named return parameters in user- or agent-written Go code; define all return variables within the function."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/go/analysis/hybrid/hybrid_integration_test.go` at line 911, Update
setupEntraDSGroupMemberHarness to use an unnamed return type list, then declare
azUser, adUser, azGroup, and adGroup inside the function body while preserving
the existing return behavior and documentation.
Source: Coding guidelines
| func addEntraDSAdminGroupTenant(entraDSAdminGroupTenantMap map[graph.ID]string, group *graph.Node) error { | ||
| if name, err := group.Properties.Get(common.Name.String()).String(); err != nil { | ||
| return err | ||
| } else if !strings.HasPrefix(strings.ToUpper(strings.TrimSpace(name)), entraDSAdminGroupNamePrefix) { | ||
| return nil | ||
| } else if tenantID, err := group.Properties.Get(azure.TenantID.String()).String(); err != nil { | ||
| return err | ||
| } else if normalizedTenantID := normalizeObjectID(tenantID); len(normalizedTenantID) != 0 { | ||
| entraDSAdminGroupTenantMap[group.ID] = normalizedTenantID | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Treat a missing name or tenantid property as "not an Entra DS admin group".
addEntraDSAdminGroupTenant returns the raw error from Properties.Get(...).String(). graph.ErrPropertyNotFound therefore propagates out of the tenant loop and aborts the whole PostHybrid read transaction. A single Entra group without a name property then suppresses all hybrid relationships, including the pre-existing SyncedToEntraUser and SyncedToADUser edges.
The file already has normalizedNodeProperty for exactly this case. Use it here.
🛡️ Proposed fix
func addEntraDSAdminGroupTenant(entraDSAdminGroupTenantMap map[graph.ID]string, group *graph.Node) error {
- if name, err := group.Properties.Get(common.Name.String()).String(); err != nil {
- return err
- } else if !strings.HasPrefix(strings.ToUpper(strings.TrimSpace(name)), entraDSAdminGroupNamePrefix) {
- return nil
- } else if tenantID, err := group.Properties.Get(azure.TenantID.String()).String(); err != nil {
- return err
- } else if normalizedTenantID := normalizeObjectID(tenantID); len(normalizedTenantID) != 0 {
- entraDSAdminGroupTenantMap[group.ID] = normalizedTenantID
- }
-
- return nil
+ name, hasName, err := normalizedNodeProperty(group, common.Name.String())
+ if err != nil {
+ return err
+ } else if !hasName || !strings.HasPrefix(name, entraDSAdminGroupNamePrefix) {
+ return nil
+ }
+
+ tenantID, hasTenantID, err := normalizedNodeProperty(group, azure.TenantID.String())
+ if err != nil {
+ return err
+ } else if hasTenantID {
+ entraDSAdminGroupTenantMap[group.ID] = tenantID
+ }
+
+ return nil
}normalizedNodeProperty already uppercases and trims, so the extra strings.ToUpper(strings.TrimSpace(...)) is no longer needed.
addNodeToObjectIDMap at lines 321-329 has the same pattern for objectid. Consider applying the same treatment there.
📝 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 addEntraDSAdminGroupTenant(entraDSAdminGroupTenantMap map[graph.ID]string, group *graph.Node) error { | |
| if name, err := group.Properties.Get(common.Name.String()).String(); err != nil { | |
| return err | |
| } else if !strings.HasPrefix(strings.ToUpper(strings.TrimSpace(name)), entraDSAdminGroupNamePrefix) { | |
| return nil | |
| } else if tenantID, err := group.Properties.Get(azure.TenantID.String()).String(); err != nil { | |
| return err | |
| } else if normalizedTenantID := normalizeObjectID(tenantID); len(normalizedTenantID) != 0 { | |
| entraDSAdminGroupTenantMap[group.ID] = normalizedTenantID | |
| } | |
| return nil | |
| } | |
| func addEntraDSAdminGroupTenant(entraDSAdminGroupTenantMap map[graph.ID]string, group *graph.Node) error { | |
| name, hasName, err := normalizedNodeProperty(group, common.Name.String()) | |
| if err != nil { | |
| return err | |
| } else if !hasName || !strings.HasPrefix(name, entraDSAdminGroupNamePrefix) { | |
| return nil | |
| } | |
| tenantID, hasTenantID, err := normalizedNodeProperty(group, azure.TenantID.String()) | |
| if err != nil { | |
| return err | |
| } else if hasTenantID { | |
| entraDSAdminGroupTenantMap[group.ID] = tenantID | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/go/analysis/hybrid/hybrid.go` around lines 343 - 355, Update
addEntraDSAdminGroupTenant to retrieve both name and tenantID through
normalizedNodeProperty, treating missing properties as absent rather than
returning graph.ErrPropertyNotFound; preserve prefix matching and normalized
tenant mapping. Remove the now-redundant uppercase/trim operations because
normalizedNodeProperty handles them. Apply the same missing-property handling to
addNodeToObjectIDMap for objectid.
| description: | ||
| 'Shows principals that can manage Microsoft Entra Domain Services (Entra DS) synchronization, identified by the ManageEntraDSSync edge, and security settings including NTLM, Kerberos, TLS, LDAP signing, channel binding, and Secure LDAP configuration and certificates.', | ||
| query: `MATCH p = (principal:AZBase)-[:AZManageEntraDS]->(domainService:AZEntraDS)\nRETURN p\nLIMIT 1000`, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the description with the queried edge.
The description states the search identifies principals by the ManageEntraDSSync edge. The query matches AZManageEntraDS instead. Update the description so it names the edge the query uses.
📝 Proposed description fix
- description:
- 'Shows principals that can manage Microsoft Entra Domain Services (Entra DS) synchronization, identified by the ManageEntraDSSync edge, and security settings including NTLM, Kerberos, TLS, LDAP signing, channel binding, and Secure LDAP configuration and certificates.',
+ description:
+ 'Shows principals that can manage Microsoft Entra Domain Services (Entra DS), identified by the AZManageEntraDS edge, including synchronization and security settings such as NTLM, Kerberos, TLS, LDAP signing, channel binding, and Secure LDAP configuration and certificates.',📝 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.
| description: | |
| 'Shows principals that can manage Microsoft Entra Domain Services (Entra DS) synchronization, identified by the ManageEntraDSSync edge, and security settings including NTLM, Kerberos, TLS, LDAP signing, channel binding, and Secure LDAP configuration and certificates.', | |
| query: `MATCH p = (principal:AZBase)-[:AZManageEntraDS]->(domainService:AZEntraDS)\nRETURN p\nLIMIT 1000`, | |
| description: | |
| 'Shows principals that can manage Microsoft Entra Domain Services (Entra DS), identified by the AZManageEntraDS edge, including synchronization and security settings such as NTLM, Kerberos, TLS, LDAP signing, channel binding, and Secure LDAP configuration and certificates.', | |
| query: `MATCH p = (principal:AZBase)-[:AZManageEntraDS]->(domainService:AZEntraDS)\nRETURN p\nLIMIT 1000`, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/bh-shared-ui/src/commonSearchesAGI.ts` around lines 486 -
488, Update the description associated with the query matching the
AZManageEntraDS edge so it names AZManageEntraDS instead of ManageEntraDSSync,
while preserving the remaining description text.
| AzureRelationshipKind.SyncedToEntraDSUser, | ||
| AzureRelationshipKind.AddEntraDSGroupMember, | ||
| AzureRelationshipKind.ManageEntraDSSync, | ||
| AzureRelationshipKind.ManageEntraDSSyncFilter, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add SyncedToEntraDSGroup to Azure pathfinding.
SyncedToEntraDSGroup is declared but is absent from AzurePathfindingEdges. An AddEntraDSGroupMember path can reach an Entra ID group but cannot traverse the synchronization edge into the Entra DS group. This prevents synchronized-group control paths from being found.
Proposed fix
AzureRelationshipKind.SyncedToEntraUser,
+ AzureRelationshipKind.SyncedToEntraDSGroup,
AzureRelationshipKind.AddEntraDSGroupMember,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/bh-shared-ui/src/graphSchema.ts` around lines 1398 -
1401, Add AzureRelationshipKind.SyncedToEntraDSGroup to the
AzurePathfindingEdges collection alongside the existing Entra DS relationship
kinds, preserving all current entries.
| edgeTypes: [ | ||
| AzureRelationshipKind.SyncedToEntraUser, | ||
| AzureRelationshipKind.SyncedToEntraDSUser, | ||
| AzureRelationshipKind.AddEntraDSGroupMember, | ||
| AzureRelationshipKind.ManageEntraDSSync, | ||
| AzureRelationshipKind.ManageEntraDSSyncFilter, | ||
| ], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add SyncedToEntraDSGroup to the Cross Platform category.
schemas/valid_edges.json defines SyncedToEntraDSGroup as an Azure-to-AD post-processing relationship. This category omits it, so users cannot select that new cross-platform relationship through the edge filter. Add AzureRelationshipKind.SyncedToEntraDSGroup here.
Proposed fix
edgeTypes: [
AzureRelationshipKind.SyncedToEntraUser,
AzureRelationshipKind.SyncedToEntraDSUser,
+ AzureRelationshipKind.SyncedToEntraDSGroup,
AzureRelationshipKind.AddEntraDSGroupMember,📝 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.
| edgeTypes: [ | |
| AzureRelationshipKind.SyncedToEntraUser, | |
| AzureRelationshipKind.SyncedToEntraDSUser, | |
| AzureRelationshipKind.AddEntraDSGroupMember, | |
| AzureRelationshipKind.ManageEntraDSSync, | |
| AzureRelationshipKind.ManageEntraDSSyncFilter, | |
| ], | |
| edgeTypes: [ | |
| AzureRelationshipKind.SyncedToEntraUser, | |
| AzureRelationshipKind.SyncedToEntraDSUser, | |
| AzureRelationshipKind.SyncedToEntraDSGroup, | |
| AzureRelationshipKind.AddEntraDSGroupMember, | |
| AzureRelationshipKind.ManageEntraDSSync, | |
| AzureRelationshipKind.ManageEntraDSSyncFilter, | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter/edgeCategories.tsx`
around lines 227 - 233, Update the Cross Platform category’s edgeTypes list to
include AzureRelationshipKind.SyncedToEntraDSGroup alongside the existing Azure
relationship kinds, preserving all current entries.
|
|
||
| PostProcessedRelationships: [ | ||
| ExecuteCommand, | ||
| ManageEntraDS, |
There was a problem hiding this comment.
We should not be adding additional edges to the post-processed list. Any new edges implemented using Post-Processing must utilize the Delta-Change Apply strategy. Please see https://github.com/SpecterOps/BloodHound/pull/2693/changes for an example of how this is done, and feel free to reach out to me for assistance.
StephenHinck
left a comment
StephenHinck
left a comment
There was a problem hiding this comment.
Other requests from ENG aside, adding a block to this PR as it was not implemented on the newer DCA strategy for post-processing. Any additions to post-processing utilize this path for performance reasons.
2 participants
Description
Adds BloodHound graph support for Microsoft Entra Domain Services (Entra DS), connecting data collected from Microsoft Entra ID, Azure Resource Manager, and the managed Active Directory domain.
This PR:
AZEntraDSnode, its properties, valid relationships, and graph schema metadata.objectidandaadobjectidvalues.AZRunsAsrelationships as non-evidence so they do not abort unrelated hybrid post-processing.Graph model
AZEntraDSContributorAZManageEntraDSSyncedToEntraDSUserSyncedToEntraDSGroupAddEntraDSGroupMemberEntraDSForManageEntraDSSyncManageEntraDSSyncFilterMotivation and Context
Relates to ticket https://specterops.atlassian.net/browse/BED-9245
Related work in other BloodHound repos:
Entra DS spans Microsoft Entra ID, Azure Resource Manager, and a Microsoft-managed Active Directory domain. BloodHound previously lacked the graph objects and relationships required to connect these identity and management planes.
The resulting gap prevented users from identifying attack paths where an Entra principal can:
Summary by CodeRabbit