BED-5021 -- Citrix CanRDP rework #1945
Conversation
WalkthroughReplaces FetchRemoteDesktopUsersBitmapForComputer with FetchCanRDPEntityBitmapForComputer (adds enforceURA and includeCitrix flags), refactors post.go control flow to handle Citrix/URA branching and cross-product semantics, updates integration tests to the new API and expectations, and refreshes the Citrix RDP harness JSON (reindexed nodes and added Group G). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test as Integration Test
participant API as FetchCanRDPEntityBitmapForComputer
participant Local as FetchComputerLocalGroupBySIDSuffix
participant URA as uraEnabled check
participant DAU as DirectAccessUsers lookup
participant RIL as FetchBaseRILEntities
participant Proc as ProcessRDPWithUra
Test->>API: call(computerID, groupExpansions, enforceURA, includeCitrix)
API->>URA: determine uraEnabled
API->>Local: fetch RDP local group
alt includeCitrix == false
API->>Proc: ProcessRDPWithUra(uraEnabled)
Proc-->>API: rdpEntities
API-->>Test: return rdpEntities
else includeCitrix == true
API->>DAU: fetch Direct Access Users
alt DAU exists and uraEnabled == false
API-->>Test: return crossProduct(RDP, DAU)
else DAU exists and uraEnabled == true
API->>RIL: fetch base RIL entities
API-->>Test: return crossProduct(RDP, DAU, baseRIL)
else DAU not found
API->>Proc: ProcessRDPWithUra(uraEnabled)
Proc-->>API: rdpEntities
API-->>Test: return rdpEntities
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 1
🧹 Nitpick comments (5)
cmd/api/src/test/integration/harnesses/citrixRDPHarness.json (2)
291-300: Align DAU group naming with analyzer lookup; remove trailing space.Analyzer fetches the Citrix group by exact name "Direct Access Users". The harness caption currently reads "Direct Access Users Group " (note the trailing space). If the loader maps captions to names, this could miss the lookup. Please ensure the underlying node’s name equals "Direct Access Users" or adjust the caption to match.
Apply this diff if captions drive naming:
- "caption": "Direct Access Users Group ", + "caption": "Direct Access Users",If captions don’t drive naming, confirm that the node’s stored name property is exactly "Direct Access Users".
514-522: Avoid pre-seeding CanRDP edges in the harness to prevent masking analyzer behavior.Since tests validate analyzer-derived bitmaps, pre-created CanRDP edges can confuse future readers and may mask logic errors if additional tests start consuming edges. Consider removing these three CanRDP relationships.
- { - "id": "n21", - "fromId": "n13", - "toId": "n3", - "type": "CanRDP", - "properties": {}, - "style": { - "arrow-color": "#f44e3b" - } - }, ... - { - "id": "n24", - "fromId": "n15", - "toId": "n3", - "type": "CanRDP", - "properties": {}, - "style": { - "arrow-color": "#f44e3b" - } - }, ... - { - "id": "n32", - "fromId": "n12", - "toId": "n3", - "type": "CanRDP", - "properties": {}, - "style": { - "arrow-color": "#f44e3b" - } - },Also applies to: 540-548, 606-614
cmd/api/src/analysis/analysis_integration_test.go (3)
60-60: Rename test to reflect the new API.Keeps test naming aligned with the public API being exercised.
-func TestFetchRemoteDesktopUsersBitmapForComputer(t *testing.T) { +func TestFetchCanRDPEntityBitmapForComputer(t *testing.T) {
158-172: Add a citrix-enabled, unenforced-URA case to exercise the two-set cross product.Currently citrix is only tested with enforceURA=true. Add a branch with enforceURA=false, includeCitrix=true to validate the cross product of RDP and DAU (no RIL). Assert presence of entities in both groups (e.g., Irshad, Uli, DomainGroupC) and absence of entities only in one set (e.g., Rohan, if not in RDP).
Example snippet to add in this test before creating the RIL edge:
require.Nil(t, db.ReadTransaction(context.Background(), func(tx graph.Transaction) error { rdpEnabledEntityIDBitmap, err := adAnalysis.FetchCanRDPEntityBitmapForComputer(tx, harness.RDPHarnessWithCitrix.Computer.ID, groupExpansions, false, true) require.Nil(t, err) // Cross product of RDP ∩ DAU (no RIL gating) require.True(t, rdpEnabledEntityIDBitmap.Contains(harness.RDPHarnessWithCitrix.IrshadUser.ID.Uint64())) require.True(t, rdpEnabledEntityIDBitmap.Contains(harness.RDPHarnessWithCitrix.UliUser.ID.Uint64())) require.True(t, rdpEnabledEntityIDBitmap.Contains(harness.RDPHarnessWithCitrix.DomainGroupC.ID.Uint64())) require.False(t, rdpEnabledEntityIDBitmap.Contains(harness.RDPHarnessWithCitrix.RohanUser.ID.Uint64())) // if not in RDP return nil }))
149-200: Optional: Add a test for Citrix-enabled fallback when DAU group is absent.When citrixEnabled=true but "Direct Access Users" doesn’t exist, code should fall back to ProcessRDPWithUra. Consider a small dedicated test using the non-Citrix RDP harness with citrixEnabled=true, asserting parity with the citrixDisabled path.
Example new test function (outline):
func TestFetchRDPEntityBitmapForComputer_CitrixEnabled_NoDAUGroupFallsBack(t *testing.T) { testContext := integration.NewGraphTestContext(t, schema.DefaultGraphSchema()) testContext.DatabaseTestWithSetup(func(h *integration.HarnessDetails) error { h.RDP.Setup(testContext) // no DAU group in this harness return nil }, func(h integration.HarnessDetails, db graph.Database) { groupExpansions, err := adAnalysis.ExpandAllRDPLocalGroups(context.Background(), db) require.NoError(t, err) require.NoError(t, db.ReadTransaction(context.Background(), func(tx graph.Transaction) error { // Citrix flag on, but DAU missing => fallback behavior should match citrix=false got, err := adAnalysis.FetchCanRDPEntityBitmapForComputer(tx, h.RDP.Computer.ID, groupExpansions, true, true) require.NoError(t, err) want, err := adAnalysis.FetchCanRDPEntityBitmapForComputer(tx, h.RDP.Computer.ID, groupExpansions, true, false) require.NoError(t, err) require.Equal(t, want.Cardinality(), got.Cardinality()) // Optionally compare contents here. return nil })) }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cmd/api/src/test/integration/harnesses/citrixRDPHarness.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
cmd/api/src/analysis/analysis_integration_test.go(5 hunks)cmd/api/src/test/integration/harnesses/citrixRDPHarness.json(25 hunks)packages/go/analysis/ad/post.go(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-23T19:56:13.423Z
Learnt from: definitelynotagoblin
PR: SpecterOps/BloodHound#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.go
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
PR: SpecterOps/BloodHound#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/post.go
🧬 Code graph analysis (2)
cmd/api/src/analysis/analysis_integration_test.go (2)
packages/go/analysis/ad/post.go (1)
FetchCanRDPEntityBitmapForComputer(636-676)cmd/api/src/test/integration/harnesses.go (1)
RDPHarnessWithCitrix(579-601)
packages/go/analysis/ad/post.go (2)
packages/go/analysis/ad/wellknown/suffix.go (1)
RemoteDesktopUsersSIDSuffix(86-86)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(478-623)
⏰ 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: build-ui
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (6)
cmd/api/src/test/integration/harnesses/citrixRDPHarness.json (1)
616-622: LGTM: Rohan → Direct Access Users membership adds the intended Citrix coverage.This addition correctly reflects the nested-member case that was previously missed and aligns with the cross-product logic in post-processing.
cmd/api/src/analysis/analysis_integration_test.go (5)
46-49: LGTM: Correct migration to FetchCanRDPEntityBitmapForComputer.Parameter order matches the new signature (tx, computerID, groupExpansions, enforceURA, includeCitrix). Good.
70-84: LGTM: Enforced URA path validates expanded RIL entities.Assertions reflect the nested-membership fix (e.g., Rohan present). Good coverage.
96-116: LGTM: Unenforced URA short-circuit to first-degree RDP members.Matches the early-return semantics when citrix is disabled and URA isn’t enforced.
184-197: LGTM: Citrix + enforced URA validates three-way cross product.Comment and assertions match the new cross-product semantics.
22-35: All call sites migrated to new API: no remainingFetchRemoteDesktopUsersBitmapForComputerreferences; onlyFetchCanRDPEntityBitmapForComputeris used.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/api/src/test/integration/harnesses.go (2)
596-596: Field addition looks fine; consider clarifying intent with a brief commentAdd a short comment explaining why DomainGroupG exists (e.g., to validate cross‑product/dup‑suppression behavior when a domain group is in both local groups). This will help future readers understand the scenario.
678-680: Add documentation for Group G dual membership
Add a comment above the two NewRelationship calls explaining that Group G is intentionally in both RDPLocalGroup and DirectAccessUsersGroup to exercise CanRDP cross-product logic and duplicate suppression under Citrix. Tests already verify this in analysis_integration_test.go (lines 187, 213).+ // Group G is intentionally present in both local groups to exercise CanRDP cross-product logic + // and duplicate suppression when Citrix is enabled.packages/go/analysis/ad/post.go (1)
652-668: Optional: reuse the skip flag before fetching RIL entitiesWhen the RDP group already holds the Remote Interactive Logon right (
canSkipURAProcessing == true), the extra lookup ofFetchRemoteInteractiveLogonRightEntitiesand the 3-way cross-product are redundant—the 2-way cross-product of RDP × DAU is sufficient and cheaper. You could branch oncanSkipURAProcessingbefore hitting the database again.- } else if baseRilEntities, err := FetchRemoteInteractiveLogonRightEntities(tx, computer); err != nil { + } else if canSkipURAProcessing { + // The RDP group itself satisfies the RIL requirement; intersecting RDP and DAU memberships is enough + return CalculateCrossProductNodeSets(tx, localGroupExpansions, []*graph.Node{rdpGroup}, []*graph.Node{dauGroup}), nil + } else if baseRilEntities, err := FetchRemoteInteractiveLogonRightEntities(tx, computer); err != nil { return nil, err } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cmd/api/src/test/integration/harnesses/citrixRDPHarness.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
cmd/api/src/analysis/analysis_integration_test.go(8 hunks)cmd/api/src/test/integration/harnesses.go(3 hunks)cmd/api/src/test/integration/harnesses/citrixRDPHarness.json(25 hunks)packages/go/analysis/ad/post.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/test/integration/harnesses/citrixRDPHarness.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
PR: SpecterOps/BloodHound#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/post.go
📚 Learning: 2025-05-23T19:56:13.423Z
Learnt from: definitelynotagoblin
PR: SpecterOps/BloodHound#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.go
🧬 Code graph analysis (3)
cmd/api/src/test/integration/harnesses.go (3)
packages/go/lab/arrows/graph.go (1)
Node(38-55)packages/go/graphschema/ad/ad.go (1)
MemberOfLocalGroup(81-81)cmd/api/src/test/integration/graph.go (1)
DefaultRelProperties(34-36)
packages/go/analysis/ad/post.go (3)
packages/go/analysis/ad/wellknown/suffix.go (1)
RemoteDesktopUsersSIDSuffix(86-86)packages/go/analysis/ad/ad.go (1)
CalculateCrossProductNodeSets(478-623)packages/go/analysis/impact/aggregator.go (1)
PathAggregator(38-42)
cmd/api/src/analysis/analysis_integration_test.go (2)
packages/go/analysis/ad/post.go (1)
FetchCanRDPEntityBitmapForComputer(636-673)cmd/api/src/test/integration/harnesses.go (1)
RDPHarnessWithCitrix(579-602)
🔇 Additional comments (2)
cmd/api/src/test/integration/harnesses.go (1)
636-636: LGTM: Group initializationThe new group is created consistently with the other domain groups.
cmd/api/src/analysis/analysis_integration_test.go (1)
174-188: Citrix cross-product coverage looks solidNice job validating the non-URA Citrix path—checking that both nested users (Rohan) and groups without RIL privilege (DomainGroupG) now receive CanRDP edges proves the regression is gone.
ddlees
left a comment
ddlees
left a comment
There was a problem hiding this comment.
I appreciate all the inline documentation. 🙇
Description
The
CanRDPedge is created from the cross product of the members of a computer's "Remote Desktop Users" local group and any entities with aRemoteInteractiveLogonPrivilegeedge to that computer. The citrix configuration setting adds another constraint that the source entity must also have membership to the computer's "Direct Access Users" group, which is a group created by citrix.Before citrix functionality was added, there was some shortcutting logic in place that prevented unrolling the "Remote Desktop Users" membership in cases where the group itself had a
RemoteInteractiveLogonPrivilegeedge. This logic was kept in place after the citrix flag was set up, and as a result there were cases where an entity with nested membership in "Remote Desktop Users" would not get aCanRDPedge despite meeting all 3 requirements.This PR updates our
CanRDPlogic to use ourCalculateCrossProductNodeSets()function to properly unroll group memberships and create shortcuts where possible. This should have the added advantage of removing some redundant edges from nested members of the "Direct Access Users" citrix group.Motivation and Context
Resolves BED-5021
There were a handful of reports from customers of
CanRDPedges not being created where expected when the citrix configuration setting was enabled.How Has This Been Tested?
Updated existing test cases to match our new behavior. The "Rohan" user in our test harness, which has nested membership in the "Remote Desktop Users" group, should now get a
CanRDPedge to the computer. Also added an additional test case to cover the potential scenario that citrix is enabled but URA is not enforced.Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests