Enhanced Defender license warnings for policy groups 2 and 4

[Dylan-MITRE]

🗣 Description

Add licenses check for policy groups 2 and 4 in addition to the current check, make the report details more precise. And let user be more aware of the missing licenses.

💭 Motivation and context

closes #599

🧪 Testing

Run Scuba for Defender
Change the json file mainly "defender_license" to "false"
Run Scuba for Defender again
Will see warning about Defender license missing during the run
Will see notes on reports indicates license missing

Note: the check for policy group 2 and 4 will still pass or fail based on the environment setting because we are modifying the json file from an tenant with required licenses. The result is still be accurate based on the environment information. Note that, normally it will fail if they don't have license due to missing information from the provider JSON.

✅ Pre-approval checklist

• This PR has an informative and human-readable title.
• PR targets the correct parent branch (e.g., main or release-name) for merge.
• Changes are limited to a single goal - eschew scope creep!
• Changes are sized such that they do not touch excessive number of files.
• All future TODOs are captured in issues, which are referenced in code comments.
• These code changes follow the ScubaGear content style guide.
• Related issues these changes resolve are linked preferably via closing keywords.
• All relevant type-of-change labels added.
• All relevant project fields are set.
• All relevant repo and/or project documentation updated to reflect these changes.
• Unit tests added/updated to cover PowerShell and Rego changes.
• Functional tests added/updated to cover PowerShell and Rego changes.
• All relevant functional tests passed.
• All automated checks (e.g., linting, static analysis, unit/smoke tests) passed.

✅ Pre-merge checklist

• PR passed smoke test check.

• Feature branch has been rebased against changes from parent branch, as needed

  Use Rebase branch button below or use this reference to rebase from the command line.

• Resolved all merge conflicts on branch

• Notified merge coordinator that PR is ready for merge via comment mention

✅ Post-merge checklist

• Feature branch deleted after merge to clean up repository.
• Verified that all checks pass on parent branch (e.g., main or release-name) after merge.

[schrolla]

The licensing relationship to policy group 4, particularly 4.2 is a bit more granular and tricky than just adding a warning. This might require some additional thinking about how best to report status on a policy item that contains a mix of both license required and non-license required items. Technically, without the license a tenant cannot meet the requirement, but without details about specific locations the admin doesn't have sufficient feedback to determine if the issue is due to license, DLP policy config, or a combination of both. See full comments below.

[schrolla]

Getting closer, but recommend additional code to better delineate which locations are not meeting the baseline rather than a generic message. See comments below.

[schrolla]

As is, I don't believe this string provides the necessary additional details. It lists all the locations from the baseline policy, regardless of those in a current DLP policy. The goal is to provide the specific locations not present in an otherwise matching DLP policy. For example, if a tenant had a DLP policy with all the sensitive info types for 4.1, but only set to locations Exchange, OneDrive, and SharePoint, then the report details should indicate the policy name that matches 4.1 and which locations it is missing (Devices, Teams). Additionally, there could be multiple DLP policies matching 4.1 but missing locations. So it might be a set of DLP policies and listing the missing locations for each. Although at least a set of locations, even it if doesn't call out specific policies.

Not easy, but those are the details that would be useful to the user.

[Sloane4]

While I agree, there is also the complexity of the number of policies. There could be multiple policies covering different parts or multiple policies covering one part like teams. De tangling that and accounting for that in the code can become complex & result in to long of a string fast. So I believe we opted for putting the additional information about policies, their state, & relation to the products in the results json under ActualValue

[schrolla]

That's a good point. I think the missing bit here in the report details then is a string that tells them where to look for additional details and confirming those details are present in ActualValue. So I'm going to rework this accordingly.

[schrolla]

@Sloane4 Updated error message to include a line pointing users to the ActualValue field in the TestResults.json file. The filename and path are taken from the scuba_config key so they will be correct for any given invocation.

[schrolla]

This utility method was intentionally vague about licenses because it is used by both policy group 1 (which relies on Office365 Plan 1 or 2) and policy group 4 (which relies on either DLP for Teams and DLP for Endpoint). So you can't replace the generic string directly with Office365 Plan 1 or 2. To provide more specific messaging, you would need to have the function take the policy ID and change the string based on the policy group, perhaps with the old generic default for groups other than 1 and 4 as a backup.

[Sloane4]

You can chain the functions, you just have to update the previous uses of the functions

# If a defender license is not present, assume failure and
# replace the message with the warning for Office 365 license
ApplyLicenseWarning(_, "Office") := concat(" ", [FAIL, LicenseWarning]) if {
    input.defender_license == false
    LicenseWarning := concat(" ", [
        "**NOTE: Either you do not have sufficient permissions or",
        "your tenant does not have a license for Microsoft Defender",
        "for Office 365 Plan 1, which is required for this feature.**"
    ])
}

# If a defender license is not present, assume failure and
# replace the message with the warning for DLP license
ApplyLicenseWarning(_, "Dlp") := concat(" ", [FAIL, LicenseWarning]) if {
    input.defender_license == false
    LicenseWarning := concat(" ", [
        "**NOTE: Either you do not have sufficient permissions or",
        "your tenant does not have a license for Microsoft Defender",
        "for DLP for Teams or DLP for Endpoint, which is required for this feature.**"
    ])
}

This way you don't have to make another method for every license.

Example use case: "ReportDetails": ApplyLicenseWarning(Status, "Office")

[schrolla]

It got a bit more complex than that given the variety of scenarios. But the updated results are such that the error message ends up including just the right information for any given scenario and only points out the specific license requirement for the specific missing locations. See implementation in the DefenderConfig.rego for 4.2.

[Dylan-MITRE]

Also added fix for powershell bugs #976

[Sloane4]

If this is triggered for unsuccessful commands, why are they added to successful commands as well?

[schrolla]

Similar to the section above it for ATP related commands, it is a case where we don't want the auto-generated error in the report about the unsuccessful commands because we handle this case with the dlp_license flag instead and have the Rego provide a more detailed DLP license warning error instead. The missing commands are how we detect the condition. By adding it to both unsuccessful and successful, we prevent the CreateReport module from adding a generic warning about missing commands that would indicate a code bug.

[Sloane4]

could this be shortened to simple set arithmetic? We already have error_policies to work off of, so we can just subtract a set of all names form the resulting error_policies set.

[schrolla]

Yes, see the assignment to PresentLocations on line 631 in the 4.2 test. Uses set arithmetic as you indicated.

[Sloane4]

You can chain the functions, you just have to update the previous uses of the functions

# If a defender license is not present, assume failure and
# replace the message with the warning for Office 365 license
ApplyLicenseWarning(_, "Office") := concat(" ", [FAIL, LicenseWarning]) if {
    input.defender_license == false
    LicenseWarning := concat(" ", [
        "**NOTE: Either you do not have sufficient permissions or",
        "your tenant does not have a license for Microsoft Defender",
        "for Office 365 Plan 1, which is required for this feature.**"
    ])
}

# If a defender license is not present, assume failure and
# replace the message with the warning for DLP license
ApplyLicenseWarning(_, "Dlp") := concat(" ", [FAIL, LicenseWarning]) if {
    input.defender_license == false
    LicenseWarning := concat(" ", [
        "**NOTE: Either you do not have sufficient permissions or",
        "your tenant does not have a license for Microsoft Defender",
        "for DLP for Teams or DLP for Endpoint, which is required for this feature.**"
    ])
}

This way you don't have to make another method for every license.

Example use case: "ReportDetails": ApplyLicenseWarning(Status, "Office")

[Sloane4]

While I agree, there is also the complexity of the number of policies. There could be multiple policies covering different parts or multiple policies covering one part like teams. De tangling that and accounting for that in the code can become complex & result in to long of a string fast. So I believe we opted for putting the additional information about policies, their state, & relation to the products in the results json under ActualValue