Allow policies to be omitted from ScubaGear

[adhilto]

🗣 Description

Make it so that policies can be omitted from the ScubaGear reports.

💭 Motivation and context

Closes #740.
Closes #739.
Closes #738.

🧪 Testing

I'd recommend that the reviewers start by reading the documentation I added for this feature, to get a good overview of the approach taken: https://github.com/cisagov/ScubaGear/blob/739-allow-any-given-policy-to-be-ignored/docs/configuration/configuration.md#omit-policies.

Testing I've done:

• Added 8 new Pester cases to cover these changes
• Manually tested without a config file to ensure normal executions is unchanged
• Manually tested excluding several policies without expiration dates
• Manually tested excluding policies without specifying the rationale
• Manually tested excluding policies with expirations date, both in the past and future
• Manually tested excluding policies with a malformed expiration date
• Manually tested generating a config file with policy omissions via New-Config, including some edge cases, such as malformed policy ids, variations in capitalization, and unexpected product names in the policy IDs.

✅ 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.

[tkol2022]

Feedback on example config file

The example MS.TEAMS.6.1v1 provided in the sample config file is confusing with respect to the sample rationale. If the service is provided by M365 Defender and the Defender baseline checks for the implementation of this policy, then what is the purpose of having this policy in Teams to begin with? I believe this has a good chance of confusing users.
Instead I recommend you provide an example rationale such as "The DLP capability required for Teams is implemented by third party product X which ScubaGear does not have the ability to check" or something like that.

I also recommend that you trim the number of example policy IDs that you have. I think by the time the user gets to policy 7.1 in the example config they will get the point so you could get rid of examples 7.2, 8.1, 8.2 and we wouldn't lose anything.

[adhilto]

Feedback on example config file

The example MS.TEAMS.6.1v1 provided in the sample config file is confusing with respect to the sample rationale. If the service is provided by M365 Defender and the Defender baseline checks for the implementation of this policy, then what is the purpose of having this policy in Teams to begin with? I believe this has a good chance of confusing users. Instead I recommend you provide an example rationale such as "The DLP capability required for Teams is implemented by third party product X which ScubaGear does not have the ability to check" or something like that.

I also recommend that you trim the number of example policy IDs that you have. I think by the time the user gets to policy 7.1 in the example config they will get the point so you could get rid of examples 7.2, 8.1, 8.2 and we wouldn't lose anything.

Done

[tkol2022]

I made a few tweaks to the configuration.md because i felt the term "excluded" could get conflated with AAD conditional access policy exclusions. we should stick with the term "omitted" and omissions.

[tkol2022]

In configuration.md, I think we can delete this paragraph. It is confusing and I don't think it adds anything. Do you think anyone is likely to send policy ids as command line arguments to New-Config? It is easier to just modify the yaml config file in my opinion.

"Policy omissions can be provided to the New-Config function as a comma-separated list of policy IDs under the OmitPolicy parameter. However, the New-Config function does not allow you to specify the rationales or expiration dates via the commandline; these must be manually entered into the resulting config file."

[tkol2022]

The expiration date feature is a nice touch!

[tkol2022]

What are your thoughts on changing the following?
Test disabled by user.
to
Test omitted by user.

[tkol2022]

Bug

If the user accidentally provides two Rationale fields ScubaGear crashes. The same problem occurs if you have more than one omit policy entry for the same policy id.

[tkol2022]

Will the following line perform a case-insensitive match? If not, should we make is so that case does not matter?

if ($Config.OmitPolicy.psobject.properties.name -Contains $ControlId)

I tested this. It ignores case which is great for flexibility. No change needed.

[tkol2022]

Test summary

• Tested prior date, today and future date for the Expiration field and the tool behaved as expected.
• Tested single quote in Rationale field and tool behaved as expected.
• Tested rationale missing and tool behaved as expected.
• Tested malformed date and the tool behaved as expected.
• Tested without rationale and without date and the tool behaved as expected.
• The only test that produced a bug was when you have more than one of the same policy or you have multiple rationales. See separate comment with details.

[tkol2022]

I found one bug and left some comments for a couple of other tweaks. If you can address those I think we are in good shape. Nice work. Overall the system behaved as expected under various conditions. Also I asked a related question on Slack which was answered by the team about how the downstream system interprets the outputs from this new functionality.

[mitchelbaker-cisa]

Testing:

Ran ScubaGear without a config file, runs as expected.
Ran ScubaGear with a config file:

• with valid policy ID, rationale, expiration
• with invalid policy ID
• with null Expiration value (results in skipped omission for given policy)
• with non-string Expiration value (results in skipped omission for given policy)

Verified "OmitPolicy" is included in ProviderSettingsExport
Verified Result: "Omitted" is in individual report json and in combined json with -mergeJson flag

Left one comment to consider but otherwise looks good.

[adhilto]

In configuration.md, I think we can delete this paragraph. It is confusing and I don't think it adds anything. Do you think anyone is likely to send policy ids as command line arguments to New-Config? It is easier to just modify the yaml config file in my opinion.

"Policy omissions can be provided to the New-Config function as a comma-separated list of policy IDs under the OmitPolicy parameter. However, the New-Config function does not allow you to specify the rationales or expiration dates via the commandline; these must be manually entered into the resulting config file."

I agree that it's probably easier to reference the example config and modify their config file manually. I'm ok with deleting the paragraph.

[adhilto]

Bug

If the user accidentally provides two Rationale fields ScubaGear crashes. The same problem occurs if you have more than one omit policy entry for the same policy id.

I think this bug is out of scope for this PR, I recommend opening a separate issue for it. This bug is in the orchestrator and/or the ScubaConfig.psm1 LoadConfig function. The YAML specification actually does not allow duplicate keys, so in your test, you've engineered invalid yaml. The problem this surfaces is that the orchestrator does not catch that invalid yaml was provided. The first error message in your screenshot is a bit cryptic, but the fix might be to wrap this line (in ScubaConfig.psm1) in a try/catch:

Additionally, for whatever reason, this if in Orchestraror.psm1 failed to detect that the config failed to load.

[adhilto]

What are your thoughts on changing the following? Test disabled by user. to Test omitted by user.

I'm 100% ok with either. If you have a preference for "Test omitted" I'll change it to that.

[tkol2022]

Additionally, for whatever reason, this if in Orchestraror.psm1 failed to detect that the config failed to load.

I opened an issue. BTW, from what I can tell, the reason that code in Orchestrator does not work as expected is because when the LoadConfig function produces an error, Orchestrator will not execute the if statement you mentioned. Instead it will go to the nearest catch () block, right?

[adhilto]

Additionally, for whatever reason, this if in Orchestraror.psm1 failed to detect that the config failed to load.

I opened an issue. BTW, from what I can tell, the reason that code in Orchestrator does not work as expected is because when the LoadConfig function produces an error, Orchestrator will not execute the if statement you mentioned. Instead it will go to the nearest catch () block, right?

Honestly, I'm not really sure what will happen. Unless I'm just not looking deep enough, this block of code isn't within a try at all. Definitely more digging needed here.

[adhilto]

What are your thoughts on changing the following? Test disabled by user. to Test omitted by user.

Changed

[adhilto]

@nanda-katikaneni Looks like this one is good to go