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

Update EXO code to align to revised emerald baseline #527

Merged

Conversation

buidav
Copy link
Collaborator

@buidav buidav commented Sep 7, 2023

🗣 Description

💭 Motivation and context

Closes #429
Additionally Resolves #562, #563, #564, #560, #571

Addresses the EXO half of #124. Teams half still needs to be addressed.

🧪 Testing

Tested on E5, G3, G5, GCC High Tenants.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All future TODOs are captured in issues, which are referenced
    in code comments.
  • All relevant type-of-change labels have been added.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.

✅ Pre-merge checklist

✅ Post-merge checklist

@buidav buidav added the enhancement This issue or pull request will add new or improve existing functionality label Sep 7, 2023
@buidav buidav added this to the Emerald milestone Sep 7, 2023
@buidav buidav self-assigned this Sep 7, 2023
@adhilto
Copy link
Collaborator

adhilto commented Sep 15, 2023

Also addresses EXO half of #124.

Copy link
Collaborator

@adhilto adhilto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Just two (hopefully) minor comments.

Rego/EXOConfig.rego Outdated Show resolved Hide resolved
Rego/EXOConfig.rego Show resolved Hide resolved
Rego/EXOConfig.rego Outdated Show resolved Hide resolved
Rego/EXOConfig.rego Show resolved Hide resolved
Rego/EXOConfig.rego Outdated Show resolved Hide resolved
Rego/EXOConfig.rego Show resolved Hide resolved
Rego/EXOConfig.rego Outdated Show resolved Hide resolved
Rego/EXOConfig.rego Show resolved Hide resolved
Rego/EXOConfig.rego Show resolved Hide resolved
@buidav buidav force-pushed the 429-update-exo-code-to-align-to-revised-baseline branch from e757cc1 to 6a7c46a Compare September 20, 2023 00:27
@tkol2022
Copy link
Collaborator

While working on the functional tests I came across a couple of conditions that I think might be worth resolving for Rego policy check 7.1 "External sender warnings SHALL be implemented". Both of these cases represent what I would consider non-compliant states according to the baseline policy and its implementation instructions. If you feel these are better scoped to a separate issue, then feel free to copy/paste them into a separate new one.

  • The first non-compliant state is if the user were to implement a different "Do the following" action for the rule from what we have in the baseline. The baseline instructs the user to implement the "Prepend the subject of the message with" action but I found that I could get ScubaGear to produce a Pass even when I implemented a different action: "Set the message header".
    image
    image

  • The second non-compliant state is if the user implements the correct action but puts a blank (or empty single space) into the message prefix input box. This would cause the email to be delivered to the user's email box and since the only thing prepended to the subject is a blank, the user wouldn't have any indicator that the message was from an external sender. I'm wondering if we should check that there is at least some text in the prefix input configuration.
    image
    image

@tkol2022
Copy link
Collaborator

tkol2022 commented Oct 2, 2023

Baseline document updates

Based on the writing the automated functional tests for EXO, below are some baseline document updates that I noticed. Tagging @ahuynhMITRE and @schrolla for awareness.

  • Not sure if this instruction belongs here for 12.1. Step 8 to ensure Turn on safe list is not selected belongs with policy 12.2.

image

@buidav buidav requested a review from tkol2022 October 3, 2023 15:31
@tkol2022 tkol2022 removed the request for review from adhilto October 3, 2023 15:44
@buidav buidav force-pushed the 429-update-exo-code-to-align-to-revised-baseline branch from 4872db8 to 408967c Compare October 3, 2023 23:32
Copy link
Collaborator

@tkol2022 tkol2022 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the EXO code extensively when writing the automated functional test YAML file. Bugs found have been added as separate issues. I also just added #571 based on a comment here.

@buidav
Copy link
Collaborator Author

buidav commented Oct 5, 2023

@tkol2022 @Sloane4
Added code/baseline updates to resolve the following issues

3.1 #564
Updated baseline that DKIM Should be enabled for all domains.
Updated rego code as well to cover this

4.3 #562
added check that reports@dmarc.cyber.dhs.gov is specifically in the rua field

4.4 #563
added check that rua field has at least 2 emails and the ruf field has at least 1 email

6.2 #560
remove the external org sharing instructions in the M365 admin center from the baseline.
This setting is to be explored in #469

7.1 #571
Added condition check that the rule should be using PrependSubject (not null)
and that there is at least 1 character in the PrependSubject field.

@buidav
Copy link
Collaborator Author

buidav commented Oct 11, 2023

@Sloane4 addressed comments. Rereview changes.
@tkol2022 there was additional bug with DMARC that a spent a few extra hours smushing for 4.3 and 4.4.
Any false positives for DKIM/DMARC should hopefully now be gone.

@buidav buidav force-pushed the 429-update-exo-code-to-align-to-revised-baseline branch from 9ea294c to fc7fb44 Compare October 11, 2023 00:14
@buidav
Copy link
Collaborator Author

buidav commented Oct 13, 2023

@nanda-katikaneni this is ready to merge
@ahuynhMITRE there are EXO baseline document changes in this PR

@nanda-katikaneni nanda-katikaneni merged commit 8cf3f45 into emerald Oct 16, 2023
6 checks passed
@buidav buidav deleted the 429-update-exo-code-to-align-to-revised-baseline branch October 24, 2023 04:46
schrolla pushed a commit that referenced this pull request Nov 2, 2023
* commit these changes first

* fix unit tests

* fix Policy group 6 check, improve error handling for policy 12, update unit tests

* address feedback round 1

* fix dkim dmarc rua ruf fields

* fix variable naming

* more variable naming

* get rid of m365 admin center instr

* more unit tests

* additional unit tests

* address comments

* force DMARC rdata to always be an array and fail if domain if NxDomain is returned

* fix missing email in policy
schrolla pushed a commit that referenced this pull request Nov 2, 2023
* commit these changes first

* fix unit tests

* fix Policy group 6 check, improve error handling for policy 12, update unit tests

* address feedback round 1

* fix dkim dmarc rua ruf fields

* fix variable naming

* more variable naming

* get rid of m365 admin center instr

* more unit tests

* additional unit tests

* address comments

* force DMARC rdata to always be an array and fail if domain if NxDomain is returned

* fix missing email in policy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment