Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed Schema Validation with parameter Identity from IntuneSettingCatalogASRRulesPolicyWindows10 #3990

Merged
merged 24 commits into from
Feb 5, 2024

Conversation

MKlingner
Copy link

Pull Request (PR) description

add mandatory for parameter Identity in IntuneSettingCatalogASRRulesPolicyWindows10

This Pull Request (PR) fixes the following issues

@ricmestre
Copy link
Contributor

@MKlingner have you seen my comments in the issue? I didn't send any PR like yours because the resource has another bigger issue afterwards so no, this doesn't fix the resource

@ricmestre
Copy link
Contributor

Additionally your fix is wrong, instead of making it mandatory in the resource module you actually need to change Identity in the schema itself from Key to Write so that it's not mandatory so that the policy then can be used in cloning scenarios without having to modify the Identity field manually.

@ricmestre
Copy link
Contributor

Are you able to create the policy in a real deployment? The error message indicates there is something wrong in graph, most likely the http request is not being done correctly/with the proper properties/values.

@MKlingner MKlingner marked this pull request as draft December 4, 2023 05:57
@MKlingner
Copy link
Author

The new fix is able to create a new policy and update an existing one. Finding a policy by DisplayName and updating with the correct id is also fixed. There was also a problem with policies without any assignments.
I'm using the functions from M365DSCDRGUtil to translate the groupDisplayName to a real groupId. Because of the given structure with an additional target in the root, it doesn't look quiet elegant.

@MKlingner MKlingner marked this pull request as ready for review December 5, 2023 09:52
@ricmestre
Copy link
Contributor

@William-Francillette I didn't have time to test this diff in a real deployment yet, but since this touches some of your code could you please check if it makes sense and if your other resources might need to be updated as well?

@Cyanic-Cloud
Copy link

This would be a great win to get sorted 😅

@ricmestre
Copy link
Contributor

@Cyanic-Cloud Yep, but this PR won't solve it since the problem is when making the HTTP request to Graph inside New-IntuneDeviceConfigurationPolicy and this isn't solved here.

@William-Francillette Could you please have a look into this?

@Cyanic-Cloud
Copy link

Reckon this will be fixed before the holidays? :)

@ricmestre
Copy link
Contributor

ricmestre commented Dec 19, 2023

@MKlingner I created a new policy, exported it, deleted the policy and tried to recreate it but it always fails, it seems the Graph request doesn't like how this property is sent since if I remove it the policy then gets created.

attacksurfacereductiononlyexclusions = @("C:\Test1","C:\Test2","C:\Test3");

But there are still several problems after that, it then fails with "Bad request" on Update-DeviceConfigurationPolicyAssignment, I have 2 assignments in the policy, which only appears in Event Viewer and looking at the code there are also 2 other problems in Test-TargetResource and to save you the trouble you just need to apply the patch below. Basically if policy is Absent in blueprint and non-existent in the tenant the condition then will always fail and report non-desired state, also any other tests with an Identity randomly generated or from another tenant will also fail because property Identity is not being removed.

diff --git a/Modules/Microsoft365DSC/DSCResources/MSFT_IntuneSettingCatalogASRRulesPolicyWindows10/MSFT_IntuneSettingCatalogASRRulesPolicyWindows10.psm1 b/Modules/Microsoft365DSC/DSCResources/MSFT_IntuneSettingCatalogASRRulesPolicyWindows10/MSFT_IntuneSettingCatalogASRRulesPolicyWindows10.psm1
index a059ab308..006b52d19 100644
--- a/Modules/Microsoft365DSC/DSCResources/MSFT_IntuneSettingCatalogASRRulesPolicyWindows10/MSFT_IntuneSettingCatalogASRRulesPolicyWindows10.psm1
+++ b/Modules/Microsoft365DSC/DSCResources/MSFT_IntuneSettingCatalogASRRulesPolicyWindows10/MSFT_IntuneSettingCatalogASRRulesPolicyWindows10.psm1
@@ -452,13 +452,7 @@ function Set-TargetResource
     #endregion
 
     $currentPolicy = Get-TargetResource @PSBoundParameters
-    $PSBoundParameters.Remove('Ensure') | Out-Null
-    $PSBoundParameters.Remove('Credential') | Out-Null
-    $PSBoundParameters.Remove('ApplicationId') | Out-Null
-    $PSBoundParameters.Remove('TenantId') | Out-Null
-    $PSBoundParameters.Remove('ApplicationSecret') | Out-Null
-    $PSBoundParameters.Remove('CertificateThumbprint') | Out-Null
-    $PSBoundParameters.Remove('ManagedIdentity') | Out-Null
+    $PSBoundParameters = Remove-M365DSCAuthenticationParameter -BoundParameters $PSBoundParameters
 
     $templateReferenceId = 'e8c053d6-9f95-42b1-a7f1-ebfd71c67a4b_1'
 
@@ -706,19 +700,13 @@ function Test-TargetResource
     Write-Verbose -Message "Testing configuration of Endpoint Protection Attack Surface Protection rules Policy {$DisplayName}"
 
     $CurrentValues = Get-TargetResource @PSBoundParameters
+    $ValuesToCheck = ([Hashtable]$PSBoundParameters).clone()
+    $ValuesToCheck = Remove-M365DSCAuthenticationParameter -BoundParameters $ValuesToCheck
+    $ValuesToCheck.Remove('Identity') | Out-Null
 
-    Write-Verbose -Message "Current Values: $(Convert-M365DscHashtableToString -Hashtable $CurrentValues)"
-    Write-Verbose -Message "Target Values: $(Convert-M365DscHashtableToString -Hashtable $PSBoundParameters)"
-
-    $ValuesToCheck = $PSBoundParameters
-    $ValuesToCheck.Remove('Credential') | Out-Null
-    $ValuesToCheck.Remove('ApplicationId') | Out-Null
-    $ValuesToCheck.Remove('TenantId') | Out-Null
-    $ValuesToCheck.Remove('ApplicationSecret') | Out-Null
-
-    if ($CurrentValues.Ensure -eq 'Absent')
+    if ($CurrentValues.Ensure -ne $PSBoundParameters.Ensure)
     {
-        Write-Verbose -Message 'The policy was not found'
+        Write-Verbose -Message "Test-TargetResource returned $false"
         return $false
     }
     #region Assignments
@@ -780,6 +768,9 @@ function Test-TargetResource
     $ValuesToCheck.Remove('Assignments') | Out-Null
     #endregion
 
+    Write-Verbose -Message "Current Values: $(Convert-M365DscHashtableToString -Hashtable $CurrentValues)"
+    Write-Verbose -Message "Target Values: $(Convert-M365DscHashtableToString -Hashtable $PSBoundParameters)"
+
     $TestResult = Test-M365DSCParameterState -CurrentValues $CurrentValues `
         -Source $($MyInvocation.MyCommand.Source) `
         -DesiredValues $PSBoundParameters `

@MKlingner
Copy link
Author

@ricmestre I have digged into this rabbit hole a little bit:

  1. It is not possible to exclude all rules from a policy all at once. This is described here. I don't know why the SettingTemplate is telling us, that there is something like a global AttackSurfaceReductionOnlyExclusions.
    Nevertheless it is possible to get the exclusion by single rules:
'#microsoft.graph.deviceManagementConfigurationGroupSettingCollectionInstance'
{
    foreach ($settingInstance in $setting.AdditionalProperties.groupSettingCollectionValue.children)
    {
        $settingName = $settingInstance.settingDefinitionId.split('_') | Select-Object -Last 1
        [String]$settingValue = $settingInstance.choiceSettingValue.value.split('_') | Select-Object -Last 1
        $returnHashtable.Add($settingName, $settingValue)

        foreach($settingValueChildInstance in $settingInstance.choiceSettingValue.children)
        {
            switch($settingValueChildInstance.'@odata.type')
            {
                '#microsoft.graph.deviceManagementConfigurationSimpleSettingCollectionInstance'
                {
                    $settingValueChildName = $settingValueChildInstance.settingDefinitionId.split('_') | Select-Object -Last 1
                    $settingValueChildValues = @()
                    foreach($settingValueChildInstanceValue in $settingValueChildInstance.simpleSettingCollectionValue)
                    {
                        switch ($settingValueChildInstanceValue.'@odata.type')
                        {
                            '#microsoft.graph.deviceManagementConfigurationStringSettingValue'
                            {
                                $settingValueChildValues += $settingValueChildInstanceValue.value
                            }
                            Default
                            {
                                Write-Warning -Message "Unknown setting child value type: $($settingValueChildInstanceValue.'@odata.type')"
                            }
                        }
                    }
                    $returnHashtable.Add("$($settingName)_$($settingValueChildName)", $settingValueChildValues)
                }
                Default
                {
                    Write-Warning -Message "Unknown setting child type: $($settingValueChildInstanceValue.'@odata.type')"
                }
            }
        }
    }
}

But...
2. It is not possible to set this exclusions by Powershell (currently).

Therefore I would suggest to remove AttackSurfaceReductionOnlyExclusions from the schema and show a warning (or throw an error) everytime a exclusion is defined in a rule.

@Cyanic-Cloud
Copy link

Anymore progress here? I was going to log my own issue but want to see if anymore progress can be made here

@NikCharlebois
Copy link
Collaborator

Can you please take a look at the failing Unit tests? These need to complete successfully before we finalize the code review and merge. Thanks

@MKlingner
Copy link
Author

The failing Unit Tests are not caused by the Intune Module i've changed. A Merge with the current Dev-Branch should fix the tests.

@MKlingner
Copy link
Author

@NikCharlebois Could you approve my PR? All Unit Tests run smoothly on my local machine.

@NikCharlebois NikCharlebois merged commit 8b78553 into microsoft:Dev Feb 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IntuneSettingCatalogASRRulesPolicyWindows10: Cannot be deployed
4 participants