added service principals in AZAddMembers edge #1975
Conversation
WalkthroughAdds RoleAssignments helpers to identify service principals (optionally filtered by role) and updates Azure post-processing to emit AZAddMembers jobs for service principals in parallel with existing user submissions; replaces fmt-based logs with structured slog calls attaching an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Analyzer
participant RoleAssignments
participant PostQueue as Post-Processing Queue
participant Logger
rect rgb(240,248,255)
note over Analyzer: AddMemberAllGroupsTargetRoles (new parallel SP path)
Analyzer->>RoleAssignments: PrincipalsWithRole(roleTemplateIDs)
RoleAssignments-->>Analyzer: user principals bitmap
Analyzer->>PostQueue: Submit AZAddMembers (users)
alt Parallel SP submission (new)
Analyzer->>RoleAssignments: ServicePrincipalsWithRole(roleTemplateIDs)
RoleAssignments-->>Analyzer: service-principal bitmap
Analyzer->>PostQueue: Submit AZAddMembers (service principals)
PostQueue-->>Analyzer: ack / error
opt on error
Analyzer->>Logger: log submission failure {err, context}
end
end
end
rect rgb(245,255,250)
note over Analyzer: AddMemberGroupNotRoleAssignableTargetRoles (teardown & warnings)
Analyzer->>Analyzer: Check innerGroup.IsAssignableToRole
alt missing property
Analyzer->>Logger: warn missing property {nodeID, property}
else not assignable
Analyzer->>RoleAssignments: ServicePrincipalsWithRole(roleTemplateIDs)
RoleAssignments-->>Analyzer: sp bitmap
Analyzer->>PostQueue: Submit AZAddMembers (SPs -> inner group)
opt on error
Analyzer->>Logger: log submission failure {err, context}
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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). (5)
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: 0
🧹 Nitpick comments (1)
packages/go/analysis/azure/post.go (1)
853-940: Consider refactoring to reduce code duplication.The
addMembersfunction now contains near-identical logic blocks for users and service principals. Consider extracting the common submission logic into a helper function that accepts the principal bitmap as a parameter.Example refactoring approach:
func submitAddMembersEdges( operation analysis.StatTrackedOperation[analysis.CreatePostRelationshipJob], principals cardinality.Duplex[uint64], innerGroupID graph.ID, principalType string, ) error { return operation.Operation.SubmitReader(func(ctx context.Context, tx graph.Transaction, outC chan<- analysis.CreatePostRelationshipJob) error { principals.Each(func(nextID uint64) bool { nextJob := analysis.CreatePostRelationshipJob{ FromID: graph.ID(nextID), ToID: innerGroupID, Kind: azure.AddMembers, } return channels.Submit(ctx, outC, nextJob) }) return nil }) }Then call it for both users and service principals:
// Users with all groups roles if err := submitAddMembersEdges(operation, roleAssignments.UsersWithRole(AddMemberAllGroupsTargetRoles()...), innerGroupID, "users"); err != nil { slog.Error(fmt.Sprintf("Failed to submit post processing job for users with role allowing AZAddMembers edge: %v", err)) } // Service principals with all groups roles if err := submitAddMembersEdges(operation, roleAssignments.ServicePrincipalsWithRole(AddMemberAllGroupsTargetRoles()...), innerGroupID, "service principals"); err != nil { slog.Error(fmt.Sprintf("Failed to submit post processing job for service principals with role allowing AZAddMembers edge: %v", err)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/go/analysis/azure/post.go(2 hunks)packages/go/analysis/azure/role.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/go/analysis/azure/role.go (1)
packages/go/graphschema/azure/azure.go (1)
ServicePrincipal(38-38)
packages/go/analysis/azure/post.go (2)
packages/go/analysis/post.go (1)
CreatePostRelationshipJob(151-157)packages/go/graphschema/azure/azure.go (2)
AddMembers(62-62)IsAssignableToRole(129-129)
⏰ 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-tests
- GitHub Check: run-analysis
🔇 Additional comments (5)
packages/go/analysis/azure/role.go (2)
125-127: LGTM! Service principal query method follows established pattern.The
ServicePrincipals()method correctly mirrors the existingUsers()method implementation, providing consistent access to service principals from role assignments.
156-160: LGTM! Role-filtered service principal query is well-implemented.The
ServicePrincipalsWithRole()method follows the same pattern asUsersWithRole(), correctly filtering principals by role and then intersecting with service principals. This enables the post-processing logic to handle service principals consistently with users.packages/go/analysis/azure/post.go (3)
873-874: LGTM! Error messages improved for clarity.The updated error messages now explicitly reference "AZAddMembers edge" and distinguish between users and service principals, making troubleshooting easier.
Also applies to: 889-890, 913-914, 937-938
876-890: LGTM! Service principal submission path correctly implemented.The parallel submission path for service principals with
AddMemberAllGroupsTargetRolescorrectly mirrors the user logic, enabling AZAddMembers edges from service principals to all groups.
916-938: LGTM! Service principal submission path for non-role-assignable groups correctly implemented.The logic properly checks the
IsAssignableToRoleproperty and only creates edges when the group is not role-assignable, consistent with the user path. The warning log for missing property is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/go/analysis/azure/post.go(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/go/analysis/azure/post.go (2)
packages/go/analysis/post.go (1)
CreatePostRelationshipJob(151-157)packages/go/graphschema/azure/azure.go (2)
AddMembers(62-62)IsAssignableToRole(129-129)
🪛 golangci-lint (2.5.0)
packages/go/analysis/azure/post.go
[major] 244-244: slog: slog.ErrorContext arg "err" should be a string or a slog.Attr (possible missing key or value)
(govet)
[major] 664-664: slog: slog.DebugContext arg "target.ID" should be a string or a slog.Attr (possible missing key or value)
(govet)
[major] 727-727: slog: slog.ErrorContext arg "err" should be a string or a slog.Attr (possible missing key or value)
(govet)
[major] 813-813: slog: slog.Error arg "err" should be a string or a slog.Attr (possible missing key or value)
(govet)
[major] 831-831: slog: slog.Error arg "err" should be a string or a slog.Attr (possible missing key or value)
(govet)
[major] 849-849: slog: slog.Error arg "err" should be a string or a slog.Attr (possible missing key or value)
(govet)
[major] 873-873: slog: slog.Error arg "err" should be a string or a slog.Attr (possible missing key or value)
(govet)
[major] 889-889: slog: slog.Error arg "err" should be a string or a slog.Attr (possible missing key or value)
(govet)
[major] 895-895: slog: slog.WarnContext arg "innerGroup.ID" should be a string or a slog.Attr (possible missing key or value)
(govet)
[major] 913-913: slog: slog.Error arg "err" should be a string or a slog.Attr (possible missing key or value)
(govet)
⏰ 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-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (2)
packages/go/analysis/azure/post.go (2)
876-890: Service principal submission logic looks correct.The parallel submission path for service principals correctly mirrors the existing user logic, allowing AZAddMembers edges from service principals with the
AddMemberAllGroupsTargetRolesto all groups. This achieves the PR objective of extending edge creation to service principals.Note: The error logging on Line 889 is covered by the slog formatting issue flagged separately.
916-938: Service principal submission for non-role-assignable groups implemented correctly.This code block appropriately extends the AZAddMembers edge creation to service principals for groups where
isAssignableToRoleis false, following the same pattern as the user submissions above (lines 892-914).Note: The error logging on Line 937 and the property warning on Line 919 are covered by the slog formatting issue flagged separately.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/go/analysis/azure/post.go (1)
727-727: Remove printf-style format placeholder from structured slog message.The format string contains
%vbut the error is passed as a structured parameter. Remove the%vplaceholder.Apply this diff:
-slog.ErrorContext(ctx, "Error caught during azure ExecuteCommand teardown: %v", slog.String("err", err.Error())) +slog.ErrorContext(ctx, "Error caught during azure ExecuteCommand teardown", slog.String("err", err.Error()))Based on learnings and past review comments.
🧹 Nitpick comments (1)
packages/go/analysis/azure/post.go (1)
916-938: Consider extracting the isRoleAssignable check to reduce duplication.The
isRoleAssignableproperty check is duplicated between the user submission (lines 892-914) and service principal submission (lines 916-938). While the current implementation is correct and maintains parallel structure, extracting this check could improve maintainability.Example refactor:
// Check once before both submissions isRoleAssignable, err := innerGroup.Properties.Get(azure.IsAssignableToRole.String()).Bool() if err != nil && !graph.IsErrPropertyNotFound(err) { // handle error } if graph.IsErrPropertyNotFound(err) { slog.WarnContext(ctx, "Node is missing property", "nodeID", innerGroup.ID, "property", azure.IsAssignableToRole) } // Then use the result in both submission blocks if !isRoleAssignable { // user submission } if !isRoleAssignable { // service principal submission }However, the current approach maintains clear separation and is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/go/analysis/azure/post.go(8 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/azure/post.go
📚 Learning: 2025-06-25T18:24:25.014Z
Learnt from: superlinkx
PR: SpecterOps/BloodHound#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/azure/post.go
📚 Learning: 2025-05-29T18:38:42.769Z
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:38:42.769Z
Learning: In the stbernard package, use structured slog logging with explicit parameter functions like slog.String(), slog.Int(), etc. instead of fmt.Sprintf() with slog for new logs. Example: slog.Warn("message", slog.String("key", value)) rather than slog.Warn(fmt.Sprintf("message %s", value)).
Applied to files:
packages/go/analysis/azure/post.go
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
PR: SpecterOps/BloodHound#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/azure/post.go
🧬 Code graph analysis (1)
packages/go/analysis/azure/post.go (3)
packages/go/analysis/post.go (1)
CreatePostRelationshipJob(151-157)packages/go/graphschema/azure/azure.go (2)
AddMembers(62-62)IsAssignableToRole(129-129)packages/go/analysis/post_operation.go (1)
AtomicPostProcessingStats(86-90)
⏰ 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: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (1)
packages/go/analysis/azure/post.go (1)
876-890: LGTM! Service principal submission logic is correct.The parallel submission path for service principals mirrors the user submission pattern correctly. The implementation properly creates AZAddMembers edges from service principals with the required roles to the group.
irshadaj
force-pushed
the
BED-6439_AZAddMembers
branch
from
c08b6da to
77059f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/go/analysis/azure/post.go(8 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/azure/post.go
📚 Learning: 2025-06-25T18:24:25.014Z
Learnt from: superlinkx
PR: SpecterOps/BloodHound#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/azure/post.go
📚 Learning: 2025-05-29T18:38:42.769Z
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:38:42.769Z
Learning: In the stbernard package, use structured slog logging with explicit parameter functions like slog.String(), slog.Int(), etc. instead of fmt.Sprintf() with slog for new logs. Example: slog.Warn("message", slog.String("key", value)) rather than slog.Warn(fmt.Sprintf("message %s", value)).
Applied to files:
packages/go/analysis/azure/post.go
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
PR: SpecterOps/BloodHound#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/azure/post.go
🧬 Code graph analysis (1)
packages/go/analysis/azure/post.go (3)
packages/go/analysis/post.go (1)
CreatePostRelationshipJob(151-157)packages/go/graphschema/azure/azure.go (2)
AddMembers(62-62)IsAssignableToRole(129-129)packages/go/analysis/post_operation.go (1)
AtomicPostProcessingStats(86-90)
⏰ 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: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (5)
packages/go/analysis/azure/post.go (5)
244-244: LGTM! Structured logging correctly implemented.The teardown error logging now uses structured slog parameters instead of printf-style formatting, which addresses the issues raised in previous reviews.
Based on learnings and past review comments.
Also applies to: 727-727
813-813: LGTM! Error logging properly structured.The job submission error logging correctly uses structured slog parameters, addressing the issues from previous reviews.
Based on learnings and past review comments.
Also applies to: 831-831, 849-849
873-874: LGTM! Service principal support correctly added.The new parallel submission path for service principals mirrors the existing user path and correctly creates
AZAddMembersedges from service principals with the appropriate roles. The error logging uses structured slog parameters as expected.Based on learnings.
Also applies to: 876-890
913-914: LGTM! Service principal support for non-role-assignable groups correctly added.The parallel submission path for service principals with non-role-assignable group roles mirrors the user path and correctly implements the logic. The error logging uses structured slog parameters as expected (aside from line 919 noted separately).
Based on learnings.
Also applies to: 916-938
951-951: LGTM! Teardown error logging correctly structured.The UserRoleAssignments teardown error logging uses structured slog parameters instead of printf-style formatting, consistent with the other changes in this PR.
Based on learnings and past review comments.
Also applies to: 958-958
irshadaj
force-pushed
the
BED-6439_AZAddMembers
branch
from
77059f1 to
a138223
Compare
irshadaj
dismissed
superlinkx’s stale review
Discussed and addressed the logging changes over slack+zoom
(https://github.com/SpecterOps/BloodHound/issues/1842)
Description
Re-opening in a new PR from #1843 to get around merge conflicts and forking.
Modified Azure post-processing logic for creation of the AZAddMembers edge to evaluate all role assignments, not just user role assignments, so that the edge is created from service principals with roles that allow group membership additions.
Also updated error statements to better reflect the failures
Motivation and Context
Resolves https://specterops.atlassian.net/browse/BED-6439
How Has This Been Tested?
Tested in BHCE 8.1.0
Testing instructions included in ticket
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Chores