Azurehound enhancement - Collection of Registry Values from Intune #139
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 Microsoft Intune device management and security analysis capabilities. New models, enums, and client methods enable listing Intune devices, collecting compliance and registry data, executing PowerShell scripts, and analyzing device security. Several new CLI commands provide access to these features. Additionally, a large auto-generated mock client is removed. Changes
Sequence Diagram(s)Intune Device Registry Data Collection FlowsequenceDiagram
participant User
participant CLI
participant AzureClient
participant IntuneAPI
User->>CLI: Run "intune-registry-analysis" command
CLI->>AzureClient: ListIntuneDevices()
AzureClient->>IntuneAPI: GET /managedDevices
IntuneAPI-->>AzureClient: List of devices
AzureClient-->>CLI: Stream devices
loop For each device
CLI->>AzureClient: CollectRegistryDataFromDevice(deviceID)
AzureClient->>IntuneAPI: ExecuteRegistryCollectionScript(deviceID)
IntuneAPI-->>AzureClient: ScriptExecution info
AzureClient->>IntuneAPI: WaitForScriptCompletion(scriptID)
IntuneAPI-->>AzureClient: ScriptExecutionResult (with registry data)
AzureClient-->>CLI: RegistryData
end
CLI->>CLI: Analyze registry and compliance data
CLI->>User: Output security analysis/report
Intune Device Compliance Listing FlowsequenceDiagram
participant User
participant CLI
participant AzureClient
participant IntuneAPI
User->>CLI: Run "list-intune-compliance" command
CLI->>AzureClient: ListIntuneManagedDevices()
AzureClient->>IntuneAPI: GET /managedDevices
IntuneAPI-->>AzureClient: Stream devices
AzureClient-->>CLI: Stream devices
CLI->>AzureClient: GetIntuneDeviceCompliance(deviceID)
AzureClient->>IntuneAPI: GET /managedDevices/{id}/deviceCompliancePolicyStates
IntuneAPI-->>AzureClient: ComplianceState(s)
AzureClient-->>CLI: ComplianceState(s)
CLI->>User: Output compliance info
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code Graph Analysis (1)client/client.go (10)
🔇 Additional comments (2)
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (8)
cmd/list-intune-devices.go (1)
28-48: Good implementation with minor improvement opportunityThe command implementation follows established patterns and handles errors appropriately. The output formatting provides useful device information.
Consider adding a
--countflag to only show the device count without detailed listing for performance in large environments.client/intune_devices.go (1)
16-20: Consider making default parameters more explicitThe
setDefaultParamsfunction modifies parameters in-place, which could be unexpected for callers.Consider documenting the default behavior or making it more explicit:
+// setDefaultParams sets default pagination parameters if not specified +// Default Top value is 999 to retrieve most devices in a single request func setDefaultParams(params *query.GraphParams) { if params.Top == 0 { params.Top = 999 } }cmd/list-intune-compliance.go (1)
24-32: Consider making the compliance state version configurable.The
Versionfield is hardcoded to1. Consider making this configurable or documenting the versioning strategy to ensure compatibility with future changes.Add a version parameter or use a constant:
+const DefaultComplianceStateVersion = 1 + func createBasicComplianceState(device intune.ManagedDevice, suffix string) intune.ComplianceState { return intune.ComplianceState{ Id: device.Id + suffix, DeviceId: device.Id, DeviceName: device.DeviceName, State: device.ComplianceState, - Version: 1, + Version: DefaultComplianceStateVersion, } }client/intune_registry.go (1)
224-226: Consider configurable polling interval with exponential backoff.The polling interval is hardcoded to 30 seconds. For better efficiency and responsiveness, consider making this configurable and implementing exponential backoff.
-func (s *azureClient) WaitForScriptCompletion(ctx context.Context, scriptID string, maxWaitTime time.Duration) (*azure.RegistryData, error) { +func (s *azureClient) WaitForScriptCompletion(ctx context.Context, scriptID string, maxWaitTime time.Duration, pollInterval time.Duration) (*azure.RegistryData, error) { timeout := time.After(maxWaitTime) - ticker := time.NewTicker(30 * time.Second) + if pollInterval <= 0 { + pollInterval = 30 * time.Second + } + ticker := time.NewTicker(pollInterval) defer ticker.Stop()models/azure/intune.go (1)
30-30: Document handling of sensitive device identifiers.The
UDIDfield contains sensitive device identifier information. Ensure proper handling, logging policies, and potential redaction requirements are documented.Consider implementing a redaction interface for sensitive fields to ensure they are not accidentally logged or exposed in non-secure contexts.
cmd/list-intune-registry-analysis.go (3)
361-371: Fix error message formatting.The error messages are missing newline characters, which will cause output formatting issues.
- fmt.Printf("Starting device analysis without script execution...") + fmt.Printf("Starting device analysis without script execution...\n") // ... later in the code ... if deviceResult.Error != nil { - fmt.Printf("Error getting device: %v", deviceResult.Error) + fmt.Printf("Error getting device: %v\n", deviceResult.Error) continue }
418-418: Make the sync staleness threshold configurable.The 7-day threshold for determining stale device sync is hardcoded. This should be configurable to accommodate different organizational policies.
+const DefaultSyncStalenessThresholdDays = 7 + // Check for old sync dates -if time.Since(device.LastSyncDateTime) > 7*24*time.Hour { +if time.Since(device.LastSyncDateTime) > time.Duration(DefaultSyncStalenessThresholdDays)*24*time.Hour {
444-445: Fix error message formatting in registry analysis function.Missing newlines in Printf statements will cause output formatting issues.
- fmt.Printf("Starting real registry security analysis...") + fmt.Printf("Starting real registry security analysis...\n") // ... and later ... - fmt.Printf("Error collecting registry data: %v", registryResult.Error) + fmt.Printf("Error collecting registry data: %v\n", registryResult.Error) - fmt.Printf("Analyzed device %s: %d findings, risk score %d", + fmt.Printf("Analyzed device %s: %d findings, risk score %d\n", analysis.Device.DeviceName, len(analysis.SecurityFindings), analysis.RiskScore) - fmt.Printf("Registry analysis completed: %d successful, %d errors", successCount, errorCount) + fmt.Printf("Registry analysis completed: %d successful, %d errors\n", successCount, errorCount)Also applies to: 465-468
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignore(1 hunks)client/client.go(3 hunks)client/intune_devices.go(1 hunks)client/intune_registry.go(1 hunks)client/mocks/client.go(0 hunks)cmd/list-intune-compliance.go(1 hunks)cmd/list-intune-devices.go(1 hunks)cmd/list-intune-registry-analysis.go(1 hunks)enums/intune.go(1 hunks)models/azure/intune.go(1 hunks)models/azure/intune_security.go(1 hunks)models/intune/models.go(1 hunks)
💤 Files with no reviewable changes (1)
- client/mocks/client.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
client/intune_devices.go (4)
client/query/params.go (2)
GraphParams(109-120)Top(42-42)client/client.go (1)
AzureResult(96-99)models/intune/models.go (3)
ManagedDevice(12-25)ComplianceState(28-36)ConfigurationState(46-54)constants/misc.go (1)
GraphApiVersion(43-43)
enums/intune.go (2)
enums/kind.go (1)
Kind(20-20)models/intune/models.go (1)
ComplianceState(28-36)
🔇 Additional comments (15)
.gitignore (1)
235-238: LGTM - Appropriate gitignore additionsThe additions properly exclude GitHub workflow files and PowerShell scripts that should not be version controlled.
cmd/list-intune-devices.go (1)
21-26: LGTM - Well-structured command definitionThe command uses proper Cobra patterns with clear usage description and appropriate flags.
models/intune/models.go (3)
11-25: Excellent model designThe
ManagedDevicestruct comprehensively captures all essential device attributes with proper JSON serialization tags and appropriate field types.
27-43: Well-structured compliance modelsThe
ComplianceStateandComplianceSettingStatestructs properly model the hierarchical compliance information with appropriate nesting and field types.
45-61: Consistent configuration state modelingThe
ConfigurationStateandConfigurationSettingStatestructs follow the same pattern as compliance models, ensuring consistency in the API design.client/intune_devices.go (3)
22-34: Excellent async implementationThe method properly uses channels for async processing and follows the established pattern with proper path construction and parameter handling.
36-48: Good compliance data retrieval implementationThe method correctly constructs the device-specific API path and follows the same async pattern as the device listing method.
50-62: Consistent configuration data retrievalThe method maintains consistency with the other methods in terms of API patterns and async handling.
enums/intune.go (5)
3-9: Well-defined Kind constantsThe Kind constants follow proper naming conventions and provide clear identification for different Intune resource types.
11-21: Comprehensive compliance state enumerationThe
ComplianceStatetype and constants cover all standard Intune compliance states, providing type safety and clear documentation.
23-32: Complete enrollment type coverageThe
EnrollmentTypeenumeration covers the key Windows and general enrollment methods used in Intune deployments.
34-42: Appropriate management agent typesThe
ManagementAgentenumeration properly represents the different management agents that can be used with Intune.
44-52: Standard operating system enumerationThe
OperatingSystemconstants cover the primary platforms supported by Intune with proper casing conventions.cmd/list-intune-compliance.go (1)
171-173: GraphParams does not require initializationAfter verifying the
GraphParamsdefinition inclient/query/params.go, all of its fields are optional query parameters. Passing an emptyGraphParams{}toGetIntuneDeviceCompliancesimply results in no extra parameters—this is the intended behavior. No changes are needed.models/azure/intune_security.go (1)
8-18: LGTM!The
DeviceSecurityAnalysisstruct is well-structured with appropriate field types and JSON tags.
cmd/list-intune-compliance.go
Outdated
|
|
||
| func collectDeviceCompliance(ctx context.Context, client client.AzureClient, devices <-chan intune.ManagedDevice, out chan<- interface{}) { | ||
| var ( | ||
| streams = pipeline.Demux(ctx.Done(), devices, config.ColStreamCount.Value().(int)) |
There was a problem hiding this comment.
Handle type assertion safely to prevent panic.
The type assertion config.ColStreamCount.Value().(int) could panic if the value is not an integer. Use the two-value form to handle this safely.
var (
- streams = pipeline.Demux(ctx.Done(), devices, config.ColStreamCount.Value().(int))
+ streamCount, ok := config.ColStreamCount.Value().(int)
+ streams []<-chan intune.ManagedDevice
wg sync.WaitGroup
)
+
+if !ok || streamCount <= 0 {
+ streamCount = 1 // fallback to sequential processing
+}
+streams = pipeline.Demux(ctx.Done(), devices, streamCount)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/list-intune-compliance.go at line 140, the type assertion on
config.ColStreamCount.Value() to int is done unsafely and may cause a panic if
the value is not an int. Change this to use the two-value form of type assertion
to check if the assertion succeeded. If it fails, handle the error or provide a
default integer value before passing it to pipeline.Demux to prevent runtime
panics.
|
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 |
2 participants
Summary by CodeRabbit
New Features
Chores