Azurehound enhancement - Local group user rights from Intune #140
Conversation
…etron-azurehound-enhancement-metronlabs Files are causing a conflict because newly created files were added but the deleted files were not removed.
# Test the current implementation Build the app using: go build -ldflags="-s -w -X github.com/bloodhoundad/azurehound/v2/constants.Version=dev-intune" Then test using added JWT token after these commands: ./azurehound list intune-devices --jwt JWT_TOKEN ./azurehound list intune-data --jwt JWT_TOKEN ./azurehound list intune-compliance --jwt JWT_TOKEN
Add 1. client id 2. client secret & 3. tenant id for the script to work.
…the deployed script instead of mock data
This reverts commit fd673c1.
This reverts commit 97867d2.
This reverts commit 160a712.
WalkthroughThis update introduces comprehensive support for Microsoft Intune device management, registry and session analysis, and Azure AD group membership/role assignment collection, with BloodHound integration. It adds new data models, client methods, and CLI commands for device compliance, registry, and session security analysis. Additionally, it removes a legacy mock client and updates Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant AzureClient
participant MicrosoftGraphAPI
User->>CLI: Run intune-compliance/device/registry/session/group-membership command
CLI->>AzureClient: Connect and request data (e.g., devices, compliance, sessions, groups)
AzureClient->>MicrosoftGraphAPI: Query for Intune devices, compliance, registry, sign-ins, groups, roles
MicrosoftGraphAPI-->>AzureClient: Return requested data
AzureClient-->>CLI: Stream results (devices, compliance, registry, session, group/role data)
CLI->>CLI: Analyze/process data (risk, findings, BloodHound format)
CLI-->>User: Display results and/or export BloodHound JSON
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (9)
client/intune_devices.go (1)
16-20: Consider lowering the default Top value for API compatibility.The default Top value of 999 might exceed Microsoft Graph API limits, which typically range from 100-500. Consider using a more conservative default like 100 or 200 to avoid potential API errors.
func setDefaultParams(params *query.GraphParams) { if params.Top == 0 { - params.Top = 999 + params.Top = 100 } }cmd/list-intune-compliance.go (1)
24-32: Consider makingcreateBasicComplianceStateunexported if only used internally.The function is exported but appears to only be used within this package. If it's not intended for external use, consider renaming it to
createBasicComplianceState(lowercase first letter).-func createBasicComplianceState(device intune.ManagedDevice, suffix string) intune.ComplianceState { +func createBasicComplianceState(device intune.ManagedDevice, suffix string) intune.ComplianceState {cmd/list-group-membership.go (1)
177-182: Add file existence check to prevent accidental overwrites.The function creates a file without checking if it already exists, which could lead to data loss.
filename := fmt.Sprintf("bloodhound_azuread_focused_%s.json", time.Now().Format("20060102_150405")) + +// Check if file exists +if _, err := os.Stat(filename); err == nil { + return fmt.Errorf("file %s already exists", filename) +} + file, err := os.Create(filename)client/intune_sessions_direct.go (1)
183-308: Consider refactoring this large function for better maintainability.This function is over 125 lines long and handles multiple responsibilities. Consider breaking it down into smaller, focused functions.
Consider extracting these logical sections into separate functions:
- Device info creation (lines 188-206)
- Session processing (lines 208-254)
- Suspicious activity detection (lines 256-269)
- Session data assembly (lines 272-307)
Example:
func (s *azureClient) createDeviceSessionData(deviceKey string, signIns []SignInEvent) azure.DeviceSessionData { deviceInfo := s.extractDeviceInfo(deviceKey, signIns) sessions, users, adminCount := s.processSignInSessions(signIns) suspiciousActivities := s.detectSuspiciousActivities(signIns) return s.assembleSessionData(deviceInfo, sessions, users, adminCount, suspiciousActivities) }models/azure/graph_data.go (1)
182-193: Consider using structured error types for better error handling.The
GraphDataCollectionResultstructure is well-designed. However, storing errors as strings in theErrorsfield might limit error handling capabilities.Consider defining a structured error type for more detailed error information:
+// CollectionError represents an error during data collection +type CollectionError struct { + Entity string `json:"entity"` + Operation string `json:"operation"` + Message string `json:"message"` + Timestamp time.Time `json:"timestamp"` +} type GraphDataCollectionResult struct { GroupMemberships []GroupMembershipData `json:"groupMemberships"` UserRoleAssignments []UserRoleData `json:"userRoleAssignments"` DeviceAccess []DeviceAccessData `json:"deviceAccess"` SignInActivity []SignIn `json:"signInActivity"` CollectionTime time.Duration `json:"collectionTime"` TotalGroups int `json:"totalGroups"` TotalUsers int `json:"totalUsers"` TotalDevices int `json:"totalDevices"` TotalSignIns int `json:"totalSignIns"` - Errors []string `json:"errors"` + Errors []CollectionError `json:"errors"` }client/client.go (1)
206-233: Consider grouping related methods together in the interface.The new interface methods are well-designed and follow existing patterns. However,
ValidateScriptDeployment(line 222) appears between collection methods and device methods, disrupting the logical grouping.Consider reordering to group script-related methods together:
// High-level Collection Methods CollectGroupMembershipData(ctx context.Context) <-chan AzureResult[azure.GroupMembershipData] CollectUserRoleAssignments(ctx context.Context) <-chan AzureResult[azure.UserRoleData] CollectDeviceAccessData(ctx context.Context) <-chan AzureResult[azure.DeviceAccessData] - ValidateScriptDeployment(ctx context.Context) error GetAzureADOrganization(ctx context.Context, selectCols []string) (*azure.Organization, error) + // Script Management Methods + ValidateScriptDeployment(ctx context.Context) error ListIntuneDevices(ctx context.Context, params query.GraphParams) <-chan AzureResult[azure.IntuneDevice] ExecuteRegistryCollectionScript(ctx context.Context, deviceID string) (*azure.ScriptExecution, error)client/intune_registry.go (1)
29-31: Use a named constant for the default page size.The magic number
999should be defined as a constant for better maintainability and clarity.+const DefaultGraphPageSize = 999 + func (s *azureClient) ListIntuneDevices(ctx context.Context, params query.GraphParams) <-chan AzureResult[azure.IntuneDevice] { var ( out = make(chan AzureResult[azure.IntuneDevice]) path = fmt.Sprintf("/%s/deviceManagement/managedDevices", constants.GraphApiVersion) ) if params.Top == 0 { - params.Top = 999 + params.Top = DefaultGraphPageSize }cmd/list-intune-registry-analysis.go (2)
45-67: Consider terminal compatibility for emoji display.The heavy use of emojis might not render properly in all terminal environments. Consider providing a flag to disable emoji output or detecting terminal capabilities.
+var noEmoji = flag.Bool("no-emoji", false, "Disable emoji in output") + func displayAnalysisResults(results []azure.DeviceSecurityAnalysis) { - fmt.Printf("\n=== INTUNE DEVICE SECURITY ANALYSIS RESULTS ===\n\n") + if *noEmoji { + fmt.Printf("\n=== INTUNE DEVICE SECURITY ANALYSIS RESULTS ===\n\n") + } else { + fmt.Printf("\n=== INTUNE DEVICE SECURITY ANALYSIS RESULTS ===\n\n") + }
418-431: Make the sync staleness threshold configurable.The 7-day threshold for determining stale sync is hardcoded. This should be configurable as different organizations may have different requirements.
+const DefaultSyncStalenessThreshold = 7 * 24 * time.Hour + // Check for old sync dates -if time.Since(device.LastSyncDateTime) > 7*24*time.Hour { +syncThreshold := DefaultSyncStalenessThreshold +if envThreshold := os.Getenv("INTUNE_SYNC_STALENESS_HOURS"); envThreshold != "" { + if hours, err := strconv.Atoi(envThreshold); err == nil { + syncThreshold = time.Duration(hours) * time.Hour + } +} +if time.Since(device.LastSyncDateTime) > syncThreshold {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.gitignore(1 hunks)client/client.go(3 hunks)client/intune_devices.go(1 hunks)client/intune_groups_direct.go(1 hunks)client/intune_registry.go(1 hunks)client/intune_sessions_direct.go(1 hunks)client/mocks/client.go(0 hunks)cmd/list-group-membership.go(1 hunks)cmd/list-intune-compliance.go(1 hunks)cmd/list-intune-devices.go(1 hunks)cmd/list-intune-registry-analysis.go(1 hunks)cmd/list-intune-session-analysis.go(1 hunks)enums/intune.go(1 hunks)models/azure/graph_data.go(1 hunks)models/azure/intune.go(1 hunks)models/azure/intune_security.go(1 hunks)models/azure/intune_sessions.go(1 hunks)models/intune/models.go(1 hunks)
💤 Files with no reviewable changes (1)
- client/mocks/client.go
🔇 Additional comments (19)
.gitignore (1)
235-238: LGTM - appropriate ignore rules for CI/CD files.The additions properly exclude GitHub workflow files and PowerShell scripts from version control, which aligns with the new Intune functionality being introduced.
models/intune/models.go (1)
11-61: LGTM - well-structured data models.The Intune data models are properly defined with appropriate JSON tags, field types, and follow Go conventions. The structure aligns well with Microsoft Graph API response schemas.
client/intune_devices.go (1)
22-62: LGTM - solid API implementation.The client methods follow Microsoft Graph API patterns correctly with proper async operations, error handling, and API path construction.
enums/intune.go (1)
1-52: LGTM - well-defined enumerations for Intune integration.The enumerations are properly structured with appropriate type definitions and constants that align with Microsoft Graph API values. The naming conventions are consistent and follow Go best practices.
cmd/list-intune-compliance.go (1)
138-166: Excellent concurrent processing implementation!The use of
pipeline.Demuxfor work distribution andsync.WaitGroupfor synchronization is well-implemented. Proper panic recovery in each goroutine ensures robustness.client/intune_sessions_direct.go (1)
46-59: Good error threshold implementation!The error counting with a threshold of 5 prevents infinite loops while still allowing for some transient failures. This is a robust pattern for API interactions.
models/azure/intune_sessions.go (1)
1-273: Well-structured and comprehensive data models!The session data models are thorough, well-documented, and properly tagged for JSON serialization. The hierarchical structure and clear separation of concerns make the models easy to understand and maintain.
client/intune_groups_direct.go (1)
66-225: Excellent implementation of data collection functions!All three collection functions (
CollectGroupMembershipData,CollectUserRoleAssignments,CollectDeviceAccessData) follow a consistent pattern with:
- Proper channel management with
defer close(out)- Graceful error handling that continues processing on individual failures
- Clear separation of concerns
- Good use of existing client methods
This implementation is robust and maintainable.
models/azure/graph_data.go (2)
1-84: Well-structured Graph API data models.The data structures are properly designed with appropriate field types, JSON tags, and proper handling of optional fields. Good use of
json.RawMessagefor flexible member types andtime.Timefor temporal data.
86-180: BloodHound integration structures are well-designed.The BloodHound-specific data models follow consistent patterns and properly represent entity relationships. Good separation of concerns between core data and relationship representations.
cmd/list-intune-session-analysis.go (2)
17-64: Command setup and implementation follow best practices.Good use of Cobra framework, proper context handling, and appropriate error handling with exit codes.
66-112: Robust session data collection with good error handling.The async collection pattern with error tracking is well-implemented. The detailed error message when no devices are analyzed helps users troubleshoot permission issues.
models/azure/intune_security.go (2)
1-176: Comprehensive and well-structured security configuration models.The security configuration structures provide excellent coverage of Intune security features including BitLocker, Windows Defender, Firewall profiles, and compliance reporting. Good use of typed fields where appropriate and flexible maps for dynamic settings.
215-406: Well-designed BloodHound entity structures with proper naming.Good practice in prefixing user and group structs with "BloodHound" to avoid naming conflicts with existing Azure types. The comprehensive property sets and relationship structures properly model Active Directory security contexts.
models/azure/intune.go (3)
9-59: Comprehensive IntuneDevice model with appropriate field types.The
IntuneDevicestruct provides excellent coverage of device attributes with proper typing. Good use ofint64for byte sizes andtime.Timefor temporal data.
107-163: Well-structured registry data models with security analysis.Good design with separation of concerns between raw registry data, security indicators, and summary statistics. The optional error field in
RegistryEntryusing a pointer is appropriate for error handling.
166-246: Excellent security finding models for threat analysis.The security finding structures provide comprehensive modeling of security issues, escalation vectors, and remediation guidance. Good inclusion of MITRE ATT&CK mappings in
SecurityFinding. TheEscalationVectormodel is particularly well-designed for privilege escalation analysis.client/client.go (1)
40-61: Well-designed SignInEvent structure for security telemetry.The
SignInEventstruct provides comprehensive coverage of sign-in data with appropriate nested structures for status and device details. Good for security analysis and audit trails.cmd/list-intune-registry-analysis.go (1)
481-596: Well-structured security analysis with comprehensive checks.The security analysis implementation is thorough with proper severity categorization, MITRE ATT&CK mappings, and escalation vector identification. The evidence collection and recommendations are helpful for remediation.
Consider adding more security checks in the future, such as:
- Checking for disabled Windows Defender
- Analyzing firewall rules
- Detecting exposed RDP/SMB services
cmd/list-intune-devices.go
Outdated
| func listIntuneDevices(ctx context.Context, azClient client.AzureClient) ([]azure.IntuneDevice, error) { | ||
| var ( | ||
| out = make([]azure.IntuneDevice, 0) | ||
| devices = azClient.ListIntuneDevices(ctx, query.GraphParams{}) | ||
| count = 0 | ||
| ) | ||
|
|
||
| for result := range devices { | ||
| if result.Error != nil { | ||
| return nil, result.Error | ||
| } else { | ||
| count++ | ||
| out = append(out, result.Ok) | ||
| } | ||
| } | ||
|
|
||
| return out, nil | ||
| } |
There was a problem hiding this comment.
Critical: Method name and type mismatch issues.
The implementation has several critical issues:
-
Method name mismatch: Line 53 calls
azClient.ListIntuneDevices()but the actual method inclient/intune_devices.goisListIntuneManagedDevices() -
Type mismatch: The function returns
[]azure.IntuneDevicebut the client method returnsintune.ManagedDevicefrom the channel -
Field access errors: Lines 42-46 access fields that may not exist on the
azure.IntuneDevicetype
Apply this diff to fix the issues:
-func listIntuneDevices(ctx context.Context, azClient client.AzureClient) ([]azure.IntuneDevice, error) {
+func listIntuneDevices(ctx context.Context, azClient client.AzureClient) ([]intune.ManagedDevice, error) {
var (
- out = make([]azure.IntuneDevice, 0)
- devices = azClient.ListIntuneDevices(ctx, query.GraphParams{})
+ out = make([]intune.ManagedDevice, 0)
+ devices = azClient.ListIntuneManagedDevices(ctx, query.GraphParams{})
count = 0
)And add the missing import:
import (
"context"
"fmt"
"github.com/bloodhoundad/azurehound/v2/client"
"github.com/bloodhoundad/azurehound/v2/client/query"
"github.com/bloodhoundad/azurehound/v2/models/azure"
+ "github.com/bloodhoundad/azurehound/v2/models/intune"
"github.com/spf13/cobra"
)📝 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 listIntuneDevices(ctx context.Context, azClient client.AzureClient) ([]azure.IntuneDevice, error) { | |
| var ( | |
| out = make([]azure.IntuneDevice, 0) | |
| devices = azClient.ListIntuneDevices(ctx, query.GraphParams{}) | |
| count = 0 | |
| ) | |
| for result := range devices { | |
| if result.Error != nil { | |
| return nil, result.Error | |
| } else { | |
| count++ | |
| out = append(out, result.Ok) | |
| } | |
| } | |
| return out, nil | |
| } | |
| // --- imports (add the new package) --- | |
| import ( | |
| "context" | |
| "fmt" | |
| "github.com/bloodhoundad/azurehound/v2/client" | |
| "github.com/bloodhoundad/azurehound/v2/client/query" | |
| "github.com/bloodhoundad/azurehound/v2/models/azure" | |
| "github.com/bloodhoundad/azurehound/v2/models/intune" | |
| "github.com/spf13/cobra" | |
| ) | |
| // --- function updated to use the correct client method and types --- | |
| func listIntuneDevices(ctx context.Context, azClient client.AzureClient) ([]intune.ManagedDevice, error) { | |
| var ( | |
| out = make([]intune.ManagedDevice, 0) | |
| devices = azClient.ListIntuneManagedDevices(ctx, query.GraphParams{}) | |
| count = 0 | |
| ) | |
| for result := range devices { | |
| if result.Error != nil { | |
| return nil, result.Error | |
| } else { | |
| count++ | |
| out = append(out, result.Ok) | |
| } | |
| } | |
| return out, nil | |
| } |
🤖 Prompt for AI Agents
In cmd/list-intune-devices.go around lines 50 to 67, the function calls
azClient.ListIntuneDevices but the correct method name is
ListIntuneManagedDevices; update the call accordingly. Also, the function
returns a slice of azure.IntuneDevice but the channel yields
intune.ManagedDevice types, so change the return type and variable types to use
intune.ManagedDevice to match. Finally, review and correct any field accesses to
ensure they match the intune.ManagedDevice struct fields instead of
azure.IntuneDevice. Add the necessary import for the intune package as well.
client/intune_registry.go
Outdated
| // Only collect from Windows devices that are compliant | ||
| if !strings.Contains(strings.ToLower(device.OperatingSystem), "windows") || | ||
| device.ComplianceState != "compliant" { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider collecting data from non-compliant devices for security analysis.
Limiting registry collection to only compliant devices might miss critical security issues on non-compliant devices, which are potentially more vulnerable.
-// Only collect from Windows devices that are compliant
-if !strings.Contains(strings.ToLower(device.OperatingSystem), "windows") ||
- device.ComplianceState != "compliant" {
+// Only collect from Windows devices
+if !strings.Contains(strings.ToLower(device.OperatingSystem), "windows") {
continue
}
+
+// Log but don't skip non-compliant devices
+if device.ComplianceState != "compliant" {
+ log.Printf("Warning: Collecting from non-compliant device %s", device.DeviceName)
+}📝 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.
| // Only collect from Windows devices that are compliant | |
| if !strings.Contains(strings.ToLower(device.OperatingSystem), "windows") || | |
| device.ComplianceState != "compliant" { | |
| continue | |
| } | |
| // Only collect from Windows devices | |
| if !strings.Contains(strings.ToLower(device.OperatingSystem), "windows") { | |
| continue | |
| } | |
| // Log but don't skip non-compliant devices | |
| if device.ComplianceState != "compliant" { | |
| log.Printf("Warning: Collecting from non-compliant device %s", device.DeviceName) | |
| } |
🤖 Prompt for AI Agents
In client/intune_registry.go around lines 308 to 312, the current code skips
data collection from non-compliant Windows devices. Modify the condition to
collect registry data from all Windows devices regardless of their compliance
state by removing the compliance check, ensuring data from potentially
vulnerable devices is included for security analysis.
| "id": fmt.Sprintf("assignment-%s-%d", deviceID, time.Now().Unix()), | ||
| "target": map[string]interface{}{ | ||
| "@odata.type": "#microsoft.graph.deviceManagementScriptGroupAssignment", | ||
| "deviceAndAppManagementAssignmentFilterId": nil, | ||
| "deviceAndAppManagementAssignmentFilterType": "none", | ||
| "groupId": nil, | ||
| "targetGroupId": deviceID, // Target specific device | ||
| }, | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix incorrect device targeting in script assignment.
Setting targetGroupId to a device ID appears incorrect. Microsoft Graph typically expects group IDs for group assignments. For device-specific targeting, you should use a different assignment type.
🌐 Web query:
What is the correct Microsoft Graph API format for assigning Intune scripts to individual devices rather than groups?
💡 Result:
The Microsoft Graph API currently does not support direct assignment of Intune device management scripts to individual devices. Instead, scripts can only be assigned to Azure AD groups, which means devices must be added to a group first, and then the script is assigned to that group[1][8][10].
Key Points:
-
Assignment Endpoint: The API uses the
deviceManagementScriptGroupAssignmentresource to assign a script to a group. The relevant endpoint and required JSON look like this:POST https://graph.microsoft.com/beta/deviceManagement/deviceManagementScripts/{scriptId}/groupAssignments { "@odata.type": "#microsoft.graph.deviceManagementScriptGroupAssignment", "targetGroupId": "<Azure AD Group ObjectId>" }The only target supported is a group—there is no parameter to assign directly to a device[1][8].
-
Workaround: To assign a script to a specific device:
- Find or create an Azure AD group (such as a security group).
- Add the device to the group. This can be done using Graph API by adding the device’s object ID to the group’s members list[2].
- Assign the script to the group using the API above.
-
No Supported API for Direct Device Assignment: There is no official, documented API action for assigning Intune scripts directly to a
managedDeviceIdor device resource. All assignment mechanisms require use of group membership as the intermediary[1][8][10].
Example (Assign Script to Group):
{
"@odata.type": "#microsoft.graph.deviceManagementScriptGroupAssignment",
"targetGroupId": "your-group-object-id"
}POST this to:
https://graph.microsoft.com/beta/deviceManagement/deviceManagementScripts/{scriptId}/groupAssignments
Replace your-group-object-id with the group containing the target device(s)[1].
Summary: Intune script assignments via Microsoft Graph API must target Azure AD groups, not individual devices. To assign a script to a single device, add that device to a group and assign the script to the group[1][2][8][10].
Citations:
- 1: https://learn.microsoft.com/en-us/graph/api/resources/intune-devices-devicemanagementscriptgroupassignment?view=graph-rest-beta
- 2: https://www.anoopcnair.com/microsoft-intune-devices-to-group-membership/
- 3: https://github.com/microsoftgraph/powershell-intune-samples/blob/master/DeviceConfiguration/DeviceManagementScript_Add_Assign.ps1
- 4: https://techcommunity.microsoft.com/discussions/microsoft-intune/managing-intune-device-categories-via-graph/4007591
- 5: https://learn.microsoft.com/en-us/intune/intune-service/developer/intune-graph-apis
- 6: https://www.anoopcnair.com/intune-policy-assignment-classification/
- 7: https://learn.microsoft.com/en-us/answers/questions/2153820/how-do-you-re-assign-a-primary-user-to-an-intune-d
- 8: https://github.com/microsoftgraph/powershell-intune-samples/blob/master/DeviceConfiguration/Readme.md
- 9: https://www.youtube.com/watch?v=5t6oYg8rNqk
- 10: https://learn.microsoft.com/en-us/graph/api/resources/intune-graph-overview?view=graph-rest-1.0
Use Azure AD Group Assignments for Intune Scripts and Log Original Errors
Microsoft Graph only supports deviceManagementScriptGroupAssignment to Azure AD groups—not direct device IDs. To target a single device you must:
- Create or reuse an Azure AD group and add the device’s object ID to it.
- Assign the script with the group’s object ID:
"target": map[string]interface{}{ "@odata.type": "#microsoft.graph.deviceManagementScriptGroupAssignment", "targetGroupId": "<azureADGroupObjectId>", },
- Before falling back on the alternate assignment method, log the original error from the first
Assigncall.
Please update:
• client/intune_registry.go, lines 135–143:
Replace
- "targetGroupId": deviceID, // Target specific device
+ "targetGroupId": assignmentGroupID, // use Azure AD group containing the device• client/intune_registry.go, around lines 125–158:
Insert logging of the initial error, for example:
if err != nil {
logrus.WithError(err).Error("failed to assign script to Azure AD group")
// existing fallback logic…
}🤖 Prompt for AI Agents
In client/intune_registry.go around lines 135 to 143, replace the current
assignment target that uses a device ID with an Azure AD group assignment by
setting "targetGroupId" to the Azure AD group's object ID instead of the device
ID, as Microsoft Graph only supports group assignments. Additionally, around
lines 125 to 158, add logging of the original error from the first Assign call
using logrus.WithError(err).Error before executing any fallback assignment logic
to ensure the initial failure is recorded for debugging.
| if deviceData.Device.OSVersion != "" && len(deviceData.Device.OSVersion) > 0 { | ||
| // Add informational finding about OS version | ||
| finding := azure.SecurityFinding{ | ||
| ID: "OS_VERSION_INFO", | ||
| Title: "Operating System Information", | ||
| Severity: "INFO", | ||
| Category: "Information", | ||
| Description: "Device OS version recorded for security posture assessment", | ||
| Evidence: []string{fmt.Sprintf("OS: %s, Version: %s", deviceData.Device.OperatingSystem, deviceData.Device.OSVersion)}, | ||
| Recommendations: []string{"Ensure OS is up to date with latest security patches"}, | ||
| MITREAttack: []string{}, | ||
| } | ||
| analysis.SecurityFindings = append(analysis.SecurityFindings, finding) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
OS version check only records information without actual validation.
The function name suggests it checks for "old OS versions" but it only records the OS information without validating if it's actually outdated.
-// Check for old OS versions (basic heuristic)
-if deviceData.Device.OSVersion != "" && len(deviceData.Device.OSVersion) > 0 {
- // Add informational finding about OS version
+// Check for old OS versions
+if deviceData.Device.OSVersion != "" {
+ // Parse version and check if it's outdated
+ if isOSVersionOutdated(deviceData.Device.OperatingSystem, deviceData.Device.OSVersion) {
+ finding := azure.SecurityFinding{
+ ID: "OUTDATED_OS_VERSION",
+ Title: "Outdated Operating System Version",
+ Severity: "HIGH",
+ Category: "Vulnerability Management",
+ Description: "Device is running an outdated OS version that may have unpatched vulnerabilities",
+ Evidence: []string{fmt.Sprintf("OS: %s, Version: %s", deviceData.Device.OperatingSystem, deviceData.Device.OSVersion)},
+ Recommendations: []string{"Update to the latest OS version", "Apply all security patches"},
+ MITREAttack: []string{"T1190"},
+ }
+ analysis.SecurityFindings = append(analysis.SecurityFindings, finding)
+ analysis.RiskScore += 20
+ } else {
+ // Keep the informational finding for up-to-date versionsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/list-intune-registry-analysis.go around lines 617 to 630, the code only
records the OS version information without validating if the OS version is
outdated, which contradicts the function's intent. Update the logic to compare
the device's OS version against a defined threshold for old versions, and only
create and append a security finding if the OS version is considered outdated.
This involves parsing the OS version, implementing the comparison, and adjusting
the finding's severity and description accordingly.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (14)
cmd/list-group-membership.go (1)
304-342: Good implementation of privileged group detection with word boundary checks.The implementation properly addresses the past review comments by using exact matching and word boundary checks to avoid false positives.
client/intune_sessions_direct.go (1)
159-194: Excellent implementation of role-based admin detection.The function properly queries Azure AD for actual role assignments instead of using string matching, which addresses the past review comments about admin detection being too simplistic.
cmd/list-intune-session-analysis.go (3)
472-474: Good security practice with restrictive file permissions.The implementation correctly uses 0600 permissions as suggested in past reviews, ensuring only the owner can read/write the sensitive session data.
128-138: Risk scoring values still lack documentation or configurability.The risk scoring continues to use arbitrary fixed values (20 points for admin sessions, 10 for high volume, 30 for suspicious activities) without documentation or configurability as suggested in past reviews.
Consider implementing the suggested constants:
+// Risk scoring configuration +const ( + // AdminSessionRiskPoints - Elevated risk due to privileged access + AdminSessionRiskPoints = 20 + // HighVolumeRiskPoints - Potential session sprawl or compromise + HighVolumeRiskPoints = 10 + // SuspiciousActivityRiskPoints - Indicators of potential compromise + SuspiciousActivityRiskPoints = 30 + // HighVolumeThreshold - Number of sessions considered high volume + HighVolumeThreshold = 5 +) + func createSimpleAnalysis(deviceData azure.DeviceSessionData) azure.DeviceSessionAnalysis { // ... existing code ... // Calculate risk score riskScore := 0 if adminSessions > 0 { - riskScore += adminSessions * 20 + riskScore += adminSessions * AdminSessionRiskPoints } - if totalSessions > 5 { - riskScore += 10 + if totalSessions > HighVolumeThreshold { + riskScore += HighVolumeRiskPoints } if len(deviceData.SessionData.SecurityIndicators.SuspiciousActivities) > 0 { - riskScore += 30 + riskScore += SuspiciousActivityRiskPoints }
301-351: Risk score analysis remains tightly coupled to scoring logic.The function continues to duplicate and reverse-engineer the risk scoring logic from
createSimpleAnalysis, creating maintenance issues as noted in past reviews.Consider implementing the suggested RiskComponents approach to decouple analysis from scoring:
+// RiskComponents tracks individual risk factors +type RiskComponents struct { + AdminSessions int + HighVolume bool + SuspiciousActivity bool +}Then modify
azure.DeviceSessionAnalysisto include RiskComponents and update both functions accordingly.models/azure/intune_security.go (1)
204-212: Good resolution of struct naming conflict.The struct has been properly renamed to
BloodHoundIntuneDataWrapperas suggested in past reviews, avoiding compilation errors from duplicate definitions.client/intune_registry.go (7)
28-31: Good implementation of dynamic configuration loading.The script configuration is now properly loaded from environment variables as suggested in past reviews, providing flexibility without requiring code changes.
115-129: Robust composite ID parsing implementation.The implementation properly addresses past reviews by using "|" as separator and
SplitNwith limit 2, correctly handling device IDs that contain hyphens.
263-326: Well-implemented polling with exponential backoff.The implementation properly addresses past reviews with:
- Configurable initial polling interval from environment
- Exponential backoff with multiplier
- Maximum interval cap to prevent excessive delays
293-296: Proper JSON unmarshal error handling implemented.The implementation correctly addresses past reviews by logging the unmarshal error and returning a descriptive error message instead of silently ignoring failures.
352-354: Excellent implementation of concurrency control.The semaphore pattern using a buffered channel properly limits concurrent API calls as suggested in past reviews, preventing API overload while improving performance.
364-367: Good decision to include all Windows devices.The implementation correctly addresses past reviews by collecting registry data from all Windows devices regardless of compliance state, ensuring security analysis includes potentially vulnerable non-compliant devices.
190-224: Group assignment implementation is incomplete.While error logging has been added as requested, the group assignment still sets
groupIdtonil. As noted in past reviews and the code comment, Microsoft Graph requires an Azure AD group ID for script assignments.The current implementation will fail because
groupIdisnil. You need to either:
- Create or use an existing Azure AD group containing the target device
- Document this as a known limitation and rely solely on the fallback method
Consider implementing a helper function to create device-specific groups or maintaining a pool of assignment groups.
cmd/list-intune-registry-analysis.go (1)
641-654: OS version check only records information without actual validation.The function creates an informational finding about OS version without validating if it's actually outdated, which contradicts the intent of checking for "old OS versions."
🧹 Nitpick comments (5)
cmd/list-group-membership.go (1)
67-82: Consider implementing error count limits to prevent memory issues.The error collection in
result.Errorshas no upper bound. If there are thousands of groups with errors, this could consume significant memory.Consider implementing an error limit and aggregation:
// Collect Group Memberships (focused on relevant groups) fmt.Printf("🏢 Collecting Azure AD groups and memberships...\n") groupResults := azClient.CollectGroupMembershipData(ctx) +const maxErrors = 100 +errorCount := 0 for groupResult := range groupResults { if groupResult.Error != nil { - result.Errors = append(result.Errors, fmt.Sprintf("Group error: %v", groupResult.Error)) + errorCount++ + if len(result.Errors) < maxErrors { + result.Errors = append(result.Errors, fmt.Sprintf("Group error: %v", groupResult.Error)) + } } else { // Only include privileged groups or groups with members if isPrivilegedGroup(groupResult.Ok.Group.DisplayName) || len(groupResult.Ok.Members) > 0 { result.GroupMemberships = append(result.GroupMemberships, groupResult.Ok) result.TotalGroups++ } } } +if errorCount > maxErrors { + result.Errors = append(result.Errors, fmt.Sprintf("... and %d more errors", errorCount-maxErrors)) +}cmd/list-intune-registry-analysis.go (4)
384-412: Improve error handling and logging consistency.The function uses
fmt.Printffor error messages and status updates, which doesn't provide consistent logging levels. Consider using a structured logging approach.- fmt.Printf("Error getting device: %v", deviceResult.Error) + log.Printf("Error getting device: %v", deviceResult.Error)- fmt.Printf("Starting device analysis without script execution...") + log.Printf("Starting device analysis without script execution...")- fmt.Printf("Analyzed %d devices based on compliance data", len(results)) + log.Printf("Analyzed %d devices based on compliance data", len(results))
528-528: Consider using constants for risk score values.The risk scores are hardcoded magic numbers scattered throughout the code. Consider defining constants to improve maintainability and clarity.
+const ( + RiskScoreUACDisabled = 25 + RiskScoreAutoAdminLogon = 40 + RiskScoreWeakServicePerms = 15 + RiskScoreSuspiciousStartup = 10 + RiskScoreNonCompliant = 15 + RiskScoreStaleSync = 10 + + MaxRiskScore = 100 +)Then use these constants instead of hardcoded values:
- analysis.RiskScore += 25 + analysis.RiskScore += RiskScoreUACDisabledAlso applies to: 558-558, 588-588, 609-609
400-403: Consider more robust OS filtering.The current filtering only checks for "windows" in the OS name. Consider adding validation for empty/null values and more specific criteria.
- if !strings.Contains(strings.ToLower(device.OperatingSystem), "windows") { + if device.OperatingSystem == "" || !strings.Contains(strings.ToLower(device.OperatingSystem), "windows") {
442-455: Consider making sync time threshold configurable.The 7-day threshold for stale sync detection is hardcoded. Consider making this configurable or using a named constant.
+const StaleDeviceSyncThreshold = 7 * 24 * time.Hour + - if time.Since(device.LastSyncDateTime) > 7*24*time.Hour { + if time.Since(device.LastSyncDateTime) > StaleDeviceSyncThreshold {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
client/intune_groups_direct.go(1 hunks)client/intune_registry.go(1 hunks)client/intune_sessions_direct.go(1 hunks)cmd/list-group-membership.go(1 hunks)cmd/list-intune-devices.go(1 hunks)cmd/list-intune-registry-analysis.go(1 hunks)cmd/list-intune-session-analysis.go(1 hunks)models/azure/intune_security.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/list-intune-devices.go
- client/intune_groups_direct.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
client/intune_sessions_direct.go (5)
client/query/params.go (4)
GraphParams(109-120)Top(42-42)Format(30-30)Select(37-37)client/client.go (2)
AzureResult(120-123)SignInEvent(40-61)models/azure/intune_sessions.go (8)
DeviceSessionData(9-16)ActiveSession(28-40)LoggedOnUser(43-54)SuspiciousActivity(68-76)SessionData(19-25)SessionSecurityInfo(57-65)SessionDataSummary(79-87)CredentialExposure(129-138)constants/misc.go (3)
GraphApiVersion(43-43)DisplayName(29-29)Description(30-30)models/azure/intune.go (3)
IntuneDevice(9-59)DeviceInfo(126-132)SecurityIndicators(144-149)
cmd/list-intune-registry-analysis.go (2)
models/azure/intune.go (6)
SecurityFinding(203-212)IntuneDevice(9-59)EscalationVector(215-225)DeviceRegistryData(108-115)RegistryData(118-123)SecurityIndicators(144-149)client/client.go (1)
AzureClient(271-287)
🔇 Additional comments (4)
cmd/list-group-membership.go (1)
31-32: Good use of signal.NotifyContext for proper signal handling.The implementation correctly uses
signal.NotifyContextwith bothos.Interruptandsyscall.SIGTERM, which ensures proper cleanup on signal reception.client/intune_sessions_direct.go (2)
23-96: Well-structured session data collection with proper error handling.The implementation includes:
- Appropriate error threshold to prevent excessive API calls
- Helpful diagnostic messages when no logs are found
- Proper resource cleanup with channel closure
289-298: Proper integration of role-based admin checking in session processing.The code correctly uses
isAdminUserByRolesinstead of string matching, ensuring accurate admin privilege detection for each sign-in event.cmd/list-intune-registry-analysis.go (1)
461-502: Good error handling implementation.The function properly tracks success and error counts, provides meaningful feedback, and returns appropriate errors when no devices can be analyzed successfully.
| file, err := os.Create(filename) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create export file: %w", err) | ||
| } | ||
| defer file.Close() |
There was a problem hiding this comment.
Use restrictive file permissions for exported data.
The exported JSON file contains potentially sensitive group membership and role assignment data but is created with default permissions (likely 0644).
// Write to file
filename := fmt.Sprintf("bloodhound_azuread_focused_%s.json", time.Now().Format("20060102_150405"))
-file, err := os.Create(filename)
+file, err := os.OpenFile(filename, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600)
if err != nil {
return fmt.Errorf("failed to create export file: %w", err)
}📝 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.
| file, err := os.Create(filename) | |
| if err != nil { | |
| return fmt.Errorf("failed to create export file: %w", err) | |
| } | |
| defer file.Close() | |
| // Write to file | |
| filename := fmt.Sprintf("bloodhound_azuread_focused_%s.json", time.Now().Format("20060102_150405")) | |
| file, err := os.OpenFile(filename, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600) | |
| if err != nil { | |
| return fmt.Errorf("failed to create export file: %w", err) | |
| } | |
| defer file.Close() |
🤖 Prompt for AI Agents
In cmd/list-group-membership.go around lines 180 to 184, the exported JSON file
is created with default permissions, which may expose sensitive group membership
and role assignment data. Modify the os.Create call to use os.OpenFile with
explicit restrictive permissions such as 0600 to ensure only the owner can read
and write the file. This change will enhance security by limiting access to the
exported data.
| for _, memberRaw := range groupData.Members { | ||
| var member map[string]interface{} | ||
| json.Unmarshal(memberRaw, &member) | ||
|
|
||
| if memberID, ok := member["id"].(string); ok { | ||
| memberName := "" | ||
| memberType := "User" | ||
|
|
||
| if displayName, ok := member["displayName"].(string); ok { | ||
| memberName = displayName | ||
| } | ||
| if odataType, ok := member["@odata.type"].(string); ok { | ||
| memberType = extractTypeFromOData(odataType) | ||
| } | ||
|
|
||
| membership := azure.BloodHoundGroupMembership{ | ||
| GroupId: groupData.Group.Id, | ||
| GroupName: groupData.Group.DisplayName, | ||
| MemberId: memberID, | ||
| MemberName: memberName, | ||
| MemberType: memberType, | ||
| RelationshipType: "MemberOf", | ||
| } | ||
| bloodhoundData.GroupMemberships = append(bloodhoundData.GroupMemberships, membership) | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle JSON unmarshal errors instead of silently skipping.
JSON unmarshal errors are not handled, which could lead to silently missing group members/owners data.
// Convert memberships
for _, memberRaw := range groupData.Members {
var member map[string]interface{}
- json.Unmarshal(memberRaw, &member)
+ if err := json.Unmarshal(memberRaw, &member); err != nil {
+ fmt.Printf("Warning: Failed to unmarshal member data: %v\n", err)
+ continue
+ }
if memberID, ok := member["id"].(string); ok {Apply the same pattern for owner unmarshaling at lines 247-249.
🤖 Prompt for AI Agents
In cmd/list-group-membership.go around lines 219 to 244, the JSON unmarshal
errors for group members are ignored, which can cause missing data without any
indication. Modify the code to check the error returned by json.Unmarshal and
handle it appropriately, such as logging the error or returning it, instead of
silently skipping. Apply the same error handling pattern to the owner
unmarshaling code at lines 247 to 249 to ensure consistency and proper error
reporting.
| func init() { | ||
| listRootCmd.AddCommand(listIntuneRegistryAnalysisCmd) | ||
| } |
There was a problem hiding this comment.
Fix duplicate init() functions.
There are two init() functions defined in the same file, which will cause a compilation error. Consolidate them into a single function.
-func init() {
- listRootCmd.AddCommand(listIntuneRegistryAnalysisCmd)
-}
-
-var listIntuneRegistryAnalysisCmd = &cobra.Command{
- Use: "intune-registry-analysis",
- Long: "Performs security analysis on collected registry data and formats for BloodHound",
- Run: listIntuneRegistryAnalysisCmdImpl,
- SilenceUsage: true,
-}
-
-func listIntuneRegistryAnalysisCmdImpl(cmd *cobra.Command, args []string) {
- ctx, stop := context.WithCancel(cmd.Context())
- defer stop()
-
- azClient := connectAndCreateClient()
-
- // Add flag to choose between real and simulated analysis
- useRealAnalysis, _ := cmd.Flags().GetBool("real-analysis")
-
- var analysisResults []azure.DeviceSecurityAnalysis
- var err error
-
- if useRealAnalysis {
- fmt.Printf("🔍 Performing real registry security analysis...\n")
- // Validate script deployment first
- if err := azClient.ValidateScriptDeployment(ctx); err != nil {
- fmt.Printf("⚠️ Script validation failed: %v\n", err)
- fmt.Printf("ℹ️ Falling back to device compliance analysis\n")
- analysisResults, err = performDeviceAnalysisWithoutScripts(ctx, azClient)
- } else {
- analysisResults, err = performRealRegistrySecurityAnalysis(ctx, azClient)
- }
- } else {
- fmt.Printf("ℹ️ Performing device compliance analysis (no registry scripts)\n")
- analysisResults, err = performDeviceAnalysisWithoutScripts(ctx, azClient)
- }
-
- if err != nil {
- exit(err)
- }
-
- displayAnalysisResults(analysisResults)
-}
-
-// Add flag to command initialization
-func init() {
- listRootCmd.AddCommand(listIntuneRegistryAnalysisCmd)
- listIntuneRegistryAnalysisCmd.Flags().Bool("real-analysis", false, "Perform real registry analysis using deployed scripts")
-}
+var listIntuneRegistryAnalysisCmd = &cobra.Command{
+ Use: "intune-registry-analysis",
+ Long: "Performs security analysis on collected registry data and formats for BloodHound",
+ Run: listIntuneRegistryAnalysisCmdImpl,
+ SilenceUsage: true,
+}
+
+func init() {
+ listRootCmd.AddCommand(listIntuneRegistryAnalysisCmd)
+ listIntuneRegistryAnalysisCmd.Flags().Bool("real-analysis", false, "Perform real registry analysis using deployed scripts")
+}
+
+func listIntuneRegistryAnalysisCmdImpl(cmd *cobra.Command, args []string) {
+ ctx, stop := context.WithCancel(cmd.Context())
+ defer stop()
+
+ azClient := connectAndCreateClient()
+
+ // Add flag to choose between real and simulated analysis
+ useRealAnalysis, _ := cmd.Flags().GetBool("real-analysis")
+
+ var analysisResults []azure.DeviceSecurityAnalysis
+ var err error
+
+ if useRealAnalysis {
+ fmt.Printf("🔍 Performing real registry security analysis...\n")
+ // Validate script deployment first
+ if err := azClient.ValidateScriptDeployment(ctx); err != nil {
+ fmt.Printf("⚠️ Script validation failed: %v\n", err)
+ fmt.Printf("ℹ️ Falling back to device compliance analysis\n")
+ analysisResults, err = performDeviceAnalysisWithoutScripts(ctx, azClient)
+ } else {
+ analysisResults, err = performRealRegistrySecurityAnalysis(ctx, azClient)
+ }
+ } else {
+ fmt.Printf("ℹ️ Performing device compliance analysis (no registry scripts)\n")
+ analysisResults, err = performDeviceAnalysisWithoutScripts(ctx, azClient)
+ }
+
+ if err != nil {
+ exit(err)
+ }
+
+ displayAnalysisResults(analysisResults)
+}Also applies to: 62-65
🤖 Prompt for AI Agents
In cmd/list-intune-registry-analysis.go around lines 16 to 18 and also lines 62
to 65, there are multiple init() functions defined which causes a compilation
error. Combine the code from both init() functions into a single init() function
to avoid duplication and ensure all initialization logic is preserved within
that one function.
| if useRealAnalysis { | ||
| fmt.Printf("🔍 Performing real registry security analysis...\n") | ||
| // Validate script deployment first | ||
| if err := azClient.ValidateScriptDeployment(ctx); err != nil { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify client interface methods exist.
The code calls methods that may not be defined in the AzureClient interface: ValidateScriptDeployment, ListIntuneDevices, and CollectRegistryDataFromAllDevices. This will cause compilation errors.
Also applies to: 390-390, 471-471
🏁 Script executed:
#!/bin/bash
# Verify if these methods exist in the AzureClient interface
ast-grep --pattern $'type AzureClient interface {
$$$
}'Length of output: 1277
AzureClient interface is missing required methods
The calls to the AzureClient in cmd/list-intune-registry-analysis.go reference three methods that aren’t defined on the interface:
• ValidateScriptDeployment(ctx context.Context) error
• ListIntuneDevices(ctx context.Context, params query.GraphParams) …
• CollectRegistryDataFromAllDevices(ctx context.Context) …
Current interface in client/client.go:271–287 only defines Intune methods named:
ListIntuneManagedDevices(ctx context.Context, params query.GraphParams) <-chan AzureResult[intune.ManagedDevice]
GetIntuneDeviceCompliance(...)
GetIntuneDeviceConfiguration(...)Actions:
- Either rename your calls to match the existing
ListIntuneManagedDevicesmethod - Or add the following signatures to the AzureClient interface:
ValidateScriptDeployment(ctx context.Context) error ListIntuneDevices(ctx context.Context, params query.GraphParams) <-chan AzureResult[intune.ManagedDevice] CollectRegistryDataFromAllDevices(ctx context.Context) <-chan AzureResult[YourRegistryType]
- Afterwards, implement these methods on your concrete AzureClient
🤖 Prompt for AI Agents
In cmd/list-intune-registry-analysis.go at line 42, the AzureClient interface is
missing the methods ValidateScriptDeployment, ListIntuneDevices, and
CollectRegistryDataFromAllDevices that are being called. To fix this, update the
AzureClient interface in client/client.go (lines 271-287) by adding these method
signatures with the correct return types, then implement these methods in the
concrete AzureClient struct. Alternatively, rename the calls in
list-intune-registry-analysis.go to match the existing interface methods like
ListIntuneManagedDevices if that fits your use case.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation