Azurehound enhancement - Active Sessions #142
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.
Removed Unused Methods. Commented Code
WalkthroughAdds broad Intune and sign-in log support: new Intune models/enums, client methods for device, script, compliance and session collection, CLI commands for Intune device/compliance/registry/session analysis, fixes field-casing, removes an obsolete mock and a directory object model, and updates .gitignore. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant AzureClient
participant MSGraph
User->>CLI: run "intune-registry-analysis" / "intune-session-analysis"
CLI->>AzureClient: request devices / sign-in logs / script operations
AzureClient->>MSGraph: List devices / Get script / Trigger execution / List signInLogs
MSGraph-->>AzureClient: paginated results / execution status / logs
AzureClient-->>CLI: stream device/session results (channels)
CLI->>CLI: analyze results (registry parsing, risk scoring)
CLI-->>User: display summary, per-device findings, optional export
sequenceDiagram
participant AzureClient
participant MSGraph
rect rgba(200,230,255,0.6)
AzureClient->>MSGraph: GetDeployedScriptID(scriptName)
MSGraph-->>AzureClient: script metadata (ID)
AzureClient->>MSGraph: TriggerScriptExecution(scriptID, deviceID)
MSGraph-->>AzureClient: execution acknowledged
AzureClient->>MSGraph: GetScriptExecutionResults(scriptID) (polling)
MSGraph-->>AzureClient: execution output / status
end
AzureClient->>AzureClient: parse JSON output -> RegistryData
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
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: 16
🧹 Nitpick comments (13)
client/intune_devices.go (1)
26-26: Consider using a buffered channel for better performanceUsing an unbuffered channel may cause the producer goroutine to block frequently. Consider adding a small buffer to improve throughput.
-out = make(chan AzureResult[intune.ManagedDevice]) +out = make(chan AzureResult[intune.ManagedDevice], 10)models/azure/intune.go (3)
33-33: Consider consistent casing for WiFi in JSON tagThe JSON tag
"wiFiMacAddress"uses unusual mixed casing. Consider using"wifiMacAddress"for consistency with common naming conventions.- WiFiMacAddress string `json:"wiFiMacAddress"` + WiFiMacAddress string `json:"wifiMacAddress"`
104-104: Consider documenting expected Settings structureThe
Settingsfield usesmap[string]interface{}which is acceptable for variable configuration data. However, consider adding documentation about expected key-value pairs or common settings for better maintainability.LastModifiedDateTime time.Time `json:"lastModifiedDateTime"` + // Settings contains device-specific configuration values. + // Common keys include: [list common keys here] Settings map[string]interface{} `json:"settings"`
140-140: *Consider using string instead of string for Error fieldUsing
*stringfor the Error field is unusual. Consider using a regularstringwith an empty string representing no error, or define a custom error type.Values map[string]interface{} `json:"values"` Accessible bool `json:"accessible"` - Error *string `json:"error"` + Error string `json:"error,omitempty"`models/azure/intune_sessions.go (2)
67-67: Consider using constants or enums for TokenPrivilegesThe
TokenPrivilegesfield uses[]stringwhich could lead to typos or inconsistent privilege names. Consider defining constants or an enum type for common Windows privileges.// Example privilege constants const ( PrivilegeSeDebug = "SeDebugPrivilege" PrivilegeSeBackup = "SeBackupPrivilege" PrivilegeSeRestore = "SeRestorePrivilege" // ... more privileges )
285-285: Consider structured format for TechnicalDetailsThe
TechnicalDetailsfield is a simple string which might be limiting for complex technical information. Consider using a structured format or allowing markdown/JSON content.EstimatedEffort string `json:"estimatedEffort"` MITREMitigations []string `json:"mitreMitigations"` - TechnicalDetails string `json:"technicalDetails"` + // TechnicalDetails supports markdown or JSON formatted content + TechnicalDetails string `json:"technicalDetails"`cmd/list-intune-session-analysis.go (1)
1-1: Add unit tests for the commandThe comment indicates this is "without test command". Production code should include comprehensive test coverage.
Would you like me to help create unit tests for this command implementation?
client/intune_registry.go (2)
148-158: Log the primary method failure before fallbackWhen the assignment method fails, the error is silently swallowed and fallback is attempted. Consider logging the primary failure for debugging purposes.
_, err := s.msgraph.Post(ctx, path, body, query.GraphParams{}, map[string]string{ "Content-Type": "application/json", }) if err != nil { + // Log the assignment error for debugging + // log.Printf("Assignment method failed: %v, trying device action method", err) // If assignment method fails, try direct device action return s.triggerScriptViaDeviceAction(ctx, scriptID, deviceID) }
225-225: Make polling interval configurableThe hard-coded 30-second polling interval might be too long for interactive use or too frequent for long-running scripts. Consider making it configurable.
-ticker := time.NewTicker(30 * time.Second) +// Allow configurable polling interval with sensible default +pollInterval := 30 * time.Second +if customInterval := os.Getenv("INTUNE_POLL_INTERVAL"); customInterval != "" { + if duration, err := time.ParseDuration(customInterval); err == nil && duration >= 5*time.Second { + pollInterval = duration + } +} +ticker := time.NewTicker(pollInterval)cmd/list-intune-registry-analysis.go (2)
33-34: Address the TODO: Script validation is being skippedThe comment indicates that script validation is being skipped temporarily. This should be implemented or properly documented if intentionally omitted.
Would you like me to implement the script validation logic or create an issue to track this TODO?
362-362: Add newlines to Printf statements for proper output formattingThe Printf statements are missing newline characters which could cause output to run together.
Apply this diff to fix the formatting:
- fmt.Printf("Starting device analysis without script execution...") + fmt.Printf("Starting device analysis without script execution...\n")- fmt.Printf("Analyzed %d devices based on compliance data", len(results)) + fmt.Printf("Analyzed %d devices based on compliance data\n", len(results))Also applies to: 386-386
client/intune_sessions_direct.go (2)
321-366: Consider enhancing role detection heuristicsThe current pattern-based detection is a reasonable fallback approach. However, consider these improvements:
- Document that these are heuristic methods and may have false positives
- Consider adding configurable pattern lists
- Log when falling back to heuristic detection
Add documentation to clarify the heuristic nature:
-// Helper functions for role detection +// Helper functions for role detection using heuristic pattern matching +// Note: These are fallback methods when API-based detection is unavailable +// and may produce false positives func isAdminUserHeuristic(upn string) bool {
418-422: Optimize regex compilation for GUID detectionThe regex pattern is compiled on every function call, which is inefficient. Also, the error from
MatchStringis ignored.Move the regex compilation to a package-level variable:
+var guidRegex = regexp.MustCompile(`^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$`) + // Helper function for GUID detection func isGUIDLike(s string) bool { - guidPattern := `^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$` - matched, _ := regexp.MatchString(guidPattern, s) - return matched + return guidRegex.MatchString(s) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.gitignore(1 hunks)client/client.go(3 hunks)client/intune_devices.go(1 hunks)client/intune_registry.go(1 hunks)client/intune_sessions_direct.go(1 hunks)client/mocks/client.go(0 hunks)client/user_roles.go(1 hunks)cmd/list-app-owners.go(1 hunks)cmd/list-app-role-assignments.go(2 hunks)cmd/list-device-owners.go(1 hunks)cmd/list-group-members.go(1 hunks)cmd/list-group-owners.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)cmd/list-role-assignments.go(1 hunks)cmd/list-service-principal-owners.go(1 hunks)enums/intune.go(1 hunks)models/azure/directory_object.go(0 hunks)models/azure/intune.go(1 hunks)models/azure/intune_security.go(1 hunks)models/azure/intune_sessions.go(1 hunks)models/azure/organization.go(1 hunks)models/intune/models.go(1 hunks)
💤 Files with no reviewable changes (2)
- models/azure/directory_object.go
- client/mocks/client.go
🧰 Additional context used
🧬 Code Graph Analysis (13)
cmd/list-group-members.go (1)
pipeline/pipeline.go (1)
Send(35-42)
cmd/list-role-assignments.go (1)
pipeline/pipeline.go (1)
Send(35-42)
cmd/list-group-owners.go (1)
pipeline/pipeline.go (1)
Send(35-42)
cmd/list-device-owners.go (1)
pipeline/pipeline.go (1)
Send(35-42)
cmd/list-app-owners.go (1)
models/app-owner.go (1)
AppOwner(24-27)
cmd/list-service-principal-owners.go (1)
pipeline/pipeline.go (1)
Send(35-42)
cmd/list-app-role-assignments.go (1)
client/query/params.go (1)
GraphParams(109-120)
client/intune_devices.go (4)
client/query/params.go (2)
GraphParams(109-120)Top(42-42)client/client.go (1)
AzureResult(120-123)models/intune/models.go (3)
ManagedDevice(12-25)ComplianceState(28-36)ConfigurationState(46-54)constants/misc.go (1)
GraphApiVersion(43-43)
cmd/list-intune-compliance.go (7)
models/intune/models.go (1)
ManagedDevice(12-25)panicrecovery/panic_recovery.go (2)
HandleBubbledPanic(14-26)PanicRecovery(29-33)client/client.go (1)
AzureClient(253-273)pipeline/pipeline.go (1)
Demux(109-141)config/config.go (1)
ColStreamCount(308-317)cmd/utils.go (1)
NewAzureWrapper(437-442)enums/intune.go (1)
KindAZIntuneCompliance(8-8)
models/azure/intune.go (4)
enums/intune.go (3)
OperatingSystem(45-45)EnrollmentType(24-24)ManagementAgent(35-35)constants/misc.go (3)
DisplayName(29-29)Description(30-30)Name(28-28)models/azure/device.go (1)
Device(28-131)models/azure/user.go (1)
User(25-467)
models/azure/intune_security.go (7)
models/azure/device.go (1)
Device(28-131)models/azure/intune.go (7)
IntuneDevice(9-59)SecurityFinding(203-212)EscalationVector(215-225)BloodHoundDeviceData(166-176)DeviceCompliancePolicy(87-94)DeviceConfiguration(97-105)RegistryFinding(189-200)constants/misc.go (4)
DisplayName(29-29)Description(30-30)Version(25-25)Name(28-28)enums/generic_enabled_disabled.go (1)
Enabled(23-23)models/ingest-request.go (1)
Meta(25-29)enums/intune.go (1)
OperatingSystem(45-45)enums/parameter_type.go (1)
ObjectType(28-28)
models/azure/intune_sessions.go (2)
models/azure/device.go (1)
Device(28-131)models/azure/intune.go (3)
IntuneDevice(9-59)DeviceInfo(126-132)SecurityIndicators(144-149)
enums/intune.go (2)
enums/kind.go (1)
Kind(20-20)models/intune/models.go (1)
ComplianceState(28-36)
🔇 Additional comments (25)
.gitignore (1)
235-238: LGTM - Appropriate ignore rules for CI/CD and sensitive files.These ignore rules properly exclude GitHub workflow files and the PowerShell token script from version control, which is correct for maintaining security and avoiding unnecessary clutter in the repository.
enums/intune.go (5)
4-9: LGTM - Well-structured Intune Kind constants.The Kind constants follow the established pattern and use descriptive names that clearly indicate their purpose in the Intune domain.
11-21: LGTM - Comprehensive compliance state enumeration.The ComplianceState enum covers all standard Microsoft Intune compliance states, including the grace period state which is important for compliance workflows.
23-32: LGTM - Complete enrollment type coverage.The EnrollmentType enum includes all major Windows enrollment scenarios including Azure AD join, auto-enrollment, and co-management, which provides comprehensive coverage for device enrollment methods.
34-42: LGTM - Appropriate management agent types.The ManagementAgent enum covers the key management agents used in Microsoft device management, including the fallback "unknown" value for handling unexpected cases.
44-52: LGTM - Standard operating system enumeration.The OperatingSystem enum includes the major platforms supported by Microsoft Intune with correct casing conventions (iOS, macOS).
cmd/list-group-members.go (1)
91-91: LGTM - Correct field name fix.The change from
group.Idtogroup.IDproperly aligns with Go naming conventions for acronyms and ensures the correct field is accessed from the models.Group struct.cmd/list-group-owners.go (1)
81-81: LGTM - Consistent field name correction.The change from
group.Idtogroup.IDmaintains consistency with Go naming conventions and ensures proper field access from the models.Group struct.cmd/list-device-owners.go (1)
80-80: LGTM - Proper field name correction.The change from
device.Idtodevice.IDcorrectly follows Go naming conventions for acronyms and ensures the proper field is accessed from the models.Device struct.cmd/list-role-assignments.go (1)
82-82: Field name correction aligns with Go naming conventions.The change from
role.Idtorole.IDproperly follows Go naming conventions where acronyms should be capitalized (ID, not Id).cmd/list-app-owners.go (2)
84-84: Correct field name usage in client method call.The change from
app.Data.Idtoapp.Data.IDensures proper access to the application identifier field following Go naming conventions.
90-90: Consistent field name usage in struct assignment.The change from
app.Data.Idtoapp.Data.IDmaintains consistency with the corrected field naming convention in the AppOwner struct assignment.cmd/list-service-principal-owners.go (1)
81-81: Field name correction follows Go naming conventions.The change from
servicePrincipal.IdtoservicePrincipal.IDproperly follows Go naming conventions where acronyms should be capitalized.models/azure/organization.go (2)
130-130: Correct field name usage in tenant ID formatting.The change from
s.Idtos.IDensures proper access to the organization identifier field following Go naming conventions in the tenant ID formatting.
132-132: Consistent field name usage in TenantId assignment.The change from
s.Idtos.IDmaintains consistency with the corrected field naming convention in the TenantId field assignment.cmd/list-app-role-assignments.go (2)
101-101: Correct field name usage in client method call.The change from
servicePrincipal.IdtoservicePrincipal.IDensures proper access to the service principal identifier field following Go naming conventions.
119-119: Consistent field name usage in logging statement.The change from
servicePrincipal.IdtoservicePrincipal.IDmaintains consistency with the corrected field naming convention in the logging statement.models/intune/models.go (1)
1-61: Well-structured data modelsThe Intune data models are properly defined with appropriate JSON tags and field types that align with Microsoft Graph API responses.
client/client.go (1)
34-34: Import path validated: intune package exists with required typesThe import
"github.com/bloodhoundad/azurehound/v2/models/intune"is correct. Themodels/intune/models.gofile defines theManagedDevice,ComplianceState, andConfigurationStatestructs as expected. No further action required.models/azure/intune_sessions.go (1)
8-20: No duplicate DirectoryRole or DirectoryObject definitions foundI searched the entire codebase and confirmed these types only exist in models/azure/intune_sessions.go. There are no conflicting definitions elsewhere, so it’s safe to add them here.
cmd/list-intune-registry-analysis.go (3)
45-68: LGTM!The function is well-structured with proper error handling for empty results and clear display flow.
290-358: LGTM!The summary statistics calculation is comprehensive and well-organized. The function properly handles empty results and aggregates data effectively.
480-596: Excellent security analysis implementation!The function comprehensively checks for various security issues with proper MITRE ATT&CK mapping and detailed escalation vectors. The risk scoring logic is well-balanced.
models/azure/intune_security.go (1)
1-407: Well-structured security data models!The file provides comprehensive data structures for Intune security analysis and BloodHound integration. Good practices observed:
- Proper use of JSON tags for serialization
- Clear naming conventions
- Appropriate use of time.Time for timestamps
- Smart naming to avoid conflicts (BloodHoundUser, BloodHoundGroup)
- Comprehensive security modeling with MITRE ATT&CK integration
client/intune_sessions_direct.go (1)
23-90: Excellent session data collection implementation!The function handles errors gracefully, provides helpful diagnostic messages when no sign-in logs are found, and follows proper Go patterns with channels and goroutines.
cmd/list-intune-devices.go
Outdated
| ctx, stop := context.WithCancel(cmd.Context()) | ||
| defer stop() | ||
|
|
||
| azClient := connectAndCreateClient() |
There was a problem hiding this comment.
Add error handling for client creation
The connectAndCreateClient() function likely returns an error as a second value. Add proper error handling to ensure graceful failure if the client cannot be created.
-azClient := connectAndCreateClient()
+azClient, err := connectAndCreateClient()
+if err != nil {
+ exit(fmt.Errorf("failed to create Azure client: %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.
| azClient := connectAndCreateClient() | |
| azClient, err := connectAndCreateClient() | |
| if err != nil { | |
| exit(fmt.Errorf("failed to create Azure client: %w", err)) | |
| } |
🤖 Prompt for AI Agents
In cmd/list-intune-devices.go at line 32, the call to connectAndCreateClient()
is missing error handling. Update the code to capture both the client and error
returned by connectAndCreateClient(), then check if the error is non-nil. If
there is an error, handle it appropriately by logging the error and exiting or
returning early to prevent further execution with an invalid client.
| riskScore := 0 | ||
| if adminSessions > 0 { | ||
| riskScore += adminSessions * 20 | ||
| } | ||
| if totalSessions > 5 { | ||
| riskScore += 10 | ||
| } | ||
| if len(deviceData.SessionData.SecurityIndicators.SuspiciousActivities) > 0 { | ||
| riskScore += 30 | ||
| } | ||
|
|
||
| analysis.RiskScore = riskScore | ||
|
|
||
| // Set security posture | ||
| switch { | ||
| case riskScore >= 60: | ||
| analysis.SecurityPosture = "High_Risk" | ||
| case riskScore >= 30: | ||
| analysis.SecurityPosture = "Moderate" | ||
| case riskScore >= 10: | ||
| analysis.SecurityPosture = "Low_Risk" | ||
| default: | ||
| analysis.SecurityPosture = "Secure" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use constants for risk scoring thresholds
The risk scoring logic uses magic numbers that make the code difficult to maintain and understand. Define constants for better clarity and maintainability.
+const (
+ // Risk score components
+ RiskScorePerAdminSession = 20
+ RiskScoreHighSessionCount = 10
+ RiskScoreSuspiciousActivity = 30
+
+ // Risk thresholds
+ RiskThresholdHigh = 60
+ RiskThresholdModerate = 30
+ RiskThresholdLow = 10
+
+ // Session count threshold
+ SessionCountThresholdHigh = 5
+)
// Calculate risk score
riskScore := 0
if adminSessions > 0 {
- riskScore += adminSessions * 20
+ riskScore += adminSessions * RiskScorePerAdminSession
}
-if totalSessions > 5 {
- riskScore += 10
+if totalSessions > SessionCountThresholdHigh {
+ riskScore += RiskScoreHighSessionCount
}
if len(deviceData.SessionData.SecurityIndicators.SuspiciousActivities) > 0 {
- riskScore += 30
+ riskScore += RiskScoreSuspiciousActivity
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/list-intune-session-analysis.go around lines 129 to 152, the risk scoring
logic uses hardcoded numeric values (magic numbers) for thresholds and score
increments. Define descriptive constants for these values at the top of the file
or in a dedicated constants section, then replace all occurrences of these magic
numbers with the corresponding constants to improve code clarity and
maintainability.
| // performRealRegistrySecurityAnalysis performs actual registry data collection and analysis | ||
| func performRealRegistrySecurityAnalysis(ctx context.Context, azClient client.AzureClient) ([]azure.DeviceSecurityAnalysis, error) { | ||
| var ( | ||
| out = make([]azure.DeviceSecurityAnalysis, 0) | ||
| successCount = 0 | ||
| errorCount = 0 | ||
| ) | ||
|
|
||
| fmt.Printf("Starting real registry security analysis...") | ||
|
|
||
| // Use the real registry collection function from your client | ||
| deviceRegistryData := azClient.CollectRegistryDataFromAllDevices(ctx) | ||
|
|
||
| for registryResult := range deviceRegistryData { | ||
| if registryResult.Error != nil { | ||
| fmt.Printf("Error collecting registry data: %v", registryResult.Error) | ||
| errorCount++ | ||
| continue | ||
| } | ||
|
|
||
| // Perform real security analysis on the collected registry data | ||
| analysis := performBasicDeviceSecurityAnalysis(registryResult.Ok) | ||
|
|
||
| // Enhance the analysis with additional checks | ||
| enhanceSecurityAnalysis(&analysis, registryResult.Ok) | ||
|
|
||
| out = append(out, analysis) | ||
| successCount++ | ||
|
|
||
| fmt.Printf("Analyzed device %s: %d findings, risk score %d", | ||
| analysis.Device.DeviceName, | ||
| len(analysis.SecurityFindings), | ||
| analysis.RiskScore) | ||
| } | ||
|
|
||
| fmt.Printf("Registry analysis completed: %d successful, %d errors", successCount, errorCount) | ||
|
|
||
| if successCount == 0 && errorCount > 0 { | ||
| return nil, fmt.Errorf("failed to analyze any devices successfully (%d errors)", errorCount) | ||
| } | ||
|
|
||
| return out, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused function: performRealRegistrySecurityAnalysis
This function appears to be the actual registry security analysis implementation but is never called. The main command only uses performDeviceAnalysisWithoutScripts. This should either be integrated into the command flow or removed if not needed.
Consider either:
- Updating the main command to use this function when scripts are available
- Removing this dead code if it's no longer needed
- Adding a command flag to toggle between simplified and real analysis modes
🤖 Prompt for AI Agents
In cmd/list-intune-registry-analysis.go around lines 436 to 478, the function
performRealRegistrySecurityAnalysis is defined but never called, making it dead
code. To fix this, either integrate this function into the main command flow by
replacing or conditionally calling it instead of
performDeviceAnalysisWithoutScripts, or add a command-line flag to toggle
between the simplified and real analysis modes and call this function
accordingly. If the function is no longer needed, remove it to clean up the
codebase.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
client/user_roles.go (2)
41-70: URL encoding properly implementedGood implementation addressing the previous review feedback - the userPrincipalName is now properly URL encoded using
url.QueryEscape, and the stable v1.0 endpoint is used.
138-161: Correct OData filter syntax implementedThe Filter field now correctly contains only the filter expression without the "$filter=" prefix, addressing the previous review feedback.
client/intune_sessions_direct.go (1)
221-222: Propagate the parent context for role detectionThe enhanced role detection functions should use the parent function's context instead of
context.Background()to preserve cancellation and deadlines.-isAdmin := s.isAdminUserEnhanced(context.Background(), signIn.UserPrincipalName) -isService := s.isServiceUserEnhanced(context.Background(), signIn.UserPrincipalName) +isAdmin := s.isAdminUserEnhanced(ctx, signIn.UserPrincipalName) +isService := s.isServiceUserEnhanced(ctx, signIn.UserPrincipalName)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
client/intune_devices.go(1 hunks)client/intune_registry.go(1 hunks)client/intune_sessions_direct.go(1 hunks)client/user_roles.go(1 hunks)cmd/list-intune-compliance.go(1 hunks)cmd/list-intune-devices.go(1 hunks)models/azure/intune.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/intune_devices.go
- client/intune_registry.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
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(23-30)ActiveSession(42-54)LoggedOnUser(57-68)SuspiciousActivity(82-90)SessionData(33-39)SessionSecurityInfo(71-79)SessionDataSummary(93-101)CredentialExposure(143-152)constants/misc.go (3)
GraphApiVersion(43-43)DisplayName(29-29)Description(30-30)models/azure/intune.go (3)
IntuneDevice(36-86)DeviceInfo(153-159)SecurityIndicators(171-176)
🔇 Additional comments (10)
cmd/list-intune-devices.go (2)
35-39: Good error handling implementation!The error handling for client creation has been properly implemented, addressing the previous review comment.
57-72: Clean implementation with proper error handling!The function correctly handles the channel iteration and the previously flagged unused
countvariable has been removed.cmd/list-intune-compliance.go (3)
34-39: Good implementation avoiding global variables!The flags are properly defined without using global variables, addressing the previous review comment.
63-66: Excellent use of local flag retrieval!The implementation correctly retrieves flag values locally, avoiding global state issues.
173-185: Correct control flow implementation!The function properly returns after sending the fallback compliance data, addressing the previous review comment about incorrect use of
continue.models/azure/intune.go (2)
8-33: Excellent type safety improvement!The
ChromeOSDeviceInfois now properly typed as a slice of the defined struct instead of[]interface{}, addressing the previous review comment and improving type safety.Also applies to: 68-68
254-261: Good security practice for sensitive data!The
ClientSecretfield is properly excluded from JSON serialization with thejson:"-"tag, and a warning comment has been added, addressing the previous security concern.client/user_roles.go (3)
15-32: Good implementation of package-level admin role definitionsThe extraction of admin role names to a package-level variable improves maintainability and addresses the previous review feedback effectively.
72-107: Well-structured admin detection with proper fallbackExcellent implementation with API-first approach and heuristic fallback. The function properly distinguishes between directory roles and groups using OData types.
210-237: Robust GUID validation implementationWell-implemented GUID validation with proper format checking (8-4-4-4-12) and hexadecimal character validation.
| // Helper function to create client with proper error handling | ||
| func connectAndCreateClientWithError() (client.AzureClient, error) { | ||
| // This function should be implemented to return both client and error | ||
| // For now, we'll assume connectAndCreateClient() exists and wrap it | ||
| // In a real implementation, you'd modify the original function or create a new one | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| // Convert panic to error if the original function panics | ||
| panic(fmt.Errorf("failed to create client: %v", r)) | ||
| } | ||
| }() | ||
|
|
||
| azClient := connectAndCreateClient() | ||
| return azClient, nil | ||
| } |
There was a problem hiding this comment.
Fix panic recovery logic
The panic recovery converts the panic to another panic instead of returning an error, defeating the purpose of recovery.
-defer func() {
- if r := recover(); r != nil {
- // Convert panic to error if the original function panics
- panic(fmt.Errorf("failed to create client: %v", r))
- }
-}()
-
-azClient := connectAndCreateClient()
-return azClient, nil
+var azClient client.AzureClient
+var err error
+
+defer func() {
+ if r := recover(); r != nil {
+ err = fmt.Errorf("failed to create client: %v", r)
+ }
+}()
+
+azClient = connectAndCreateClient()
+return azClient, 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.
| // Helper function to create client with proper error handling | |
| func connectAndCreateClientWithError() (client.AzureClient, error) { | |
| // This function should be implemented to return both client and error | |
| // For now, we'll assume connectAndCreateClient() exists and wrap it | |
| // In a real implementation, you'd modify the original function or create a new one | |
| defer func() { | |
| if r := recover(); r != nil { | |
| // Convert panic to error if the original function panics | |
| panic(fmt.Errorf("failed to create client: %v", r)) | |
| } | |
| }() | |
| azClient := connectAndCreateClient() | |
| return azClient, nil | |
| } | |
| // Helper function to create client with proper error handling | |
| func connectAndCreateClientWithError() (client.AzureClient, error) { | |
| var azClient client.AzureClient | |
| var err error | |
| defer func() { | |
| if r := recover(); r != nil { | |
| err = fmt.Errorf("failed to create client: %v", r) | |
| } | |
| }() | |
| azClient = connectAndCreateClient() | |
| return azClient, err | |
| } |
🤖 Prompt for AI Agents
In cmd/list-intune-devices.go around lines 74 to 88, the panic recovery block
currently re-panics with an error instead of returning it, which defeats the
purpose of recovery. Modify the function to catch the panic and return it as an
error instead of panicking again. You can achieve this by using a named error
return variable and setting it inside the deferred function when a panic occurs,
then returning the client and error normally.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/intune_sessions_direct.go (1)
367-373: Potential integer overflow in getIdleTime functionThe function casts
duration.Hours()directly to int without bounds checking, which could potentially cause issues with very large durations.Consider adding bounds checking or using a more robust time formatting approach:
func getIdleTime(logonTime time.Time) string { duration := time.Since(logonTime) + if duration < 0 { + duration = 0 + } hours := int(duration.Hours()) + if hours > 9999 { + hours = 9999 + } minutes := int(duration.Minutes()) % 60 seconds := int(duration.Seconds()) % 60 return fmt.Sprintf("%02d:%02d:%02d", hours, minutes, seconds) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/intune_sessions_direct.go(1 hunks)cmd/list-intune-devices.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/list-intune-devices.go
🔇 Additional comments (12)
client/intune_sessions_direct.go (12)
1-13: Package structure and imports look goodThe imports are well-organized and include all necessary dependencies for the functionality.
15-20: Helper function follows established patternsThe
setDefaultSessionParamsfunction correctly follows the AzureHound pattern and sets appropriate defaults for sign-in logs.
22-89: Main collection function implements robust error handlingThe
CollectSessionDataDirectlyfunction properly handles errors with a threshold-based approach and provides informative feedback. The context propagation toprocessSignInLogsSimplecorrectly addresses the past review comment.
95-96: Good fix for OData injection vulnerabilityThe sanitization of
userPrincipalNameby escaping single quotes properly addresses the security concern raised in past reviews.
121-122: Consistent security fix appliedThe same OData injection protection is correctly applied to
deviceIdparameter.
191-191: Context parameter correctly added to function signatureThe function signature now includes the
context.Contextparameter as suggested in past reviews, enabling proper context propagation.
226-228: Context propagation implemented correctlyThe context is now properly passed to the enhanced role detection functions instead of using
context.Background(), addressing the past review comment.
182-183: Function call updated with context parameterThe call to
createDeviceSessionDatanow properly passes the context parameter, completing the fix for context propagation.
143-155: Excellent adherence to AzureHound patternsThe
listSignInLogsfunction correctly follows the established AzureHound pattern by usinggetAzureObjectListand proper path construction.
273-284: Comprehensive suspicious activity detectionThe risk detection logic properly identifies high-risk sign-ins and creates structured suspicious activity records with appropriate metadata.
288-324: Well-structured session data creationThe session data structure creation is comprehensive and includes all necessary security indicators, summaries, and device information.
334-340: Potential panic in getUsernameFromUPN functionThe function splits the UPN but doesn't validate that the split produces at least one element before accessing
parts[0].Apply this fix to prevent potential index out of bounds:
func getUsernameFromUPN(upn string) string { if upn == "" { return "Unknown" } parts := strings.Split(upn, "@") + if len(parts) == 0 { + return "Unknown" + } return parts[0] }Likely an incorrect or invalid review comment.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores