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

Enhance AAD Provider and Rego Code to automate checks for policy MS.AAD.3.3v1 #1014

Merged
merged 24 commits into from
Apr 30, 2024

Conversation

ssatyapal123
Copy link
Contributor

@ssatyapal123 ssatyapal123 commented Mar 20, 2024

🗣 Description

Updated AAD Rego code to check for MS.AAD.3.3v1: If Phishing-resistant MFA has not been enforced yet and Microsoft Authenticator is enabled, it SHALL be configured to show login context information.

Closes #410
Closes #1020

Changes in detail:

  • To code this issue for policy 3.3, we needed to retrieve the authentication method configurations that were already being returned by an existing ScubaGear cmdlet Get-MgBetaPolicyAuthenticationMethodPolicy for policies 3.4 & 3.5 but the JSON structure had to be modified to support all three policies.
  • The Rego was updated for policies 3.4 and 3.5 to read from the adjusted JSON structure. Unit tests and functional tests were adjusted accordingly.
  • Rego policy 3.5 was adjusted since it was coded wrong - when the policy migration state is not migrationComplete it should have a Criticality of Shall/Not-Implemented to align with the baseline requirement. This results in an N/A in the report Result column.
  • Products.Tests.ps1 was modified to support the case when a policy (e.g. 3.3) is N/A but has a tailored Details message instead of the generic "This product does not currently have the capability to check compliance for this policy" which only makes sense in some scenarios but not for policy 3.3.
  • The Rego for new policy implementation 3.3 was coded. Unit tests and functional tests were coded as well.

💭 Motivation and context

Previously MSAAD.3.1v1 was not being checked by ScubaGear. This change adds automation by checking if MS Authenticator has the appropriate context information enabled.

🧪 Testing

Tested against E5/G5 tenant to confirm 3.3v1 passes/fails when the settings for Microsoft Authenticator are changed.

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

@ssatyapal123 ssatyapal123 added the enhancement This issue or pull request will add new or improve existing functionality label Mar 20, 2024
@ssatyapal123 ssatyapal123 added this to the Halibut milestone Mar 20, 2024
@ssatyapal123 ssatyapal123 self-assigned this Mar 20, 2024
@tkol2022
Copy link
Collaborator

@ssatyapal123

  • You need to go through the pre-approval checklist at the top of the PR
  • Your branch is out of date with the main branch. Rebase.
  • Functional tests are missing so create a separate issue and we will add those in the next release

image

@ssatyapal123 ssatyapal123 force-pushed the 410-aad-rego-33v1-authenticator-context-v2 branch from ab8b168 to 0d7f483 Compare March 21, 2024 14:34
@tkol2022
Copy link
Collaborator

tkol2022 commented Mar 21, 2024

Test Results against E5 (bug found)

The policy produced a fail because the tenant has the "Show geographic location" disabled in the MS Authenticator settings which is correct but the message in the Details column of the report is not related to that. Needs to be fixed.

image

image

When the tenant is compliant and the test passes, the message in the report is not correct either. It is referencing policy 3.2 but your new code is for policy 3.3.

image

@tkol2022 tkol2022 changed the title Add Rego Code to Support Automated Checks for AAD Policy 3.3v1 Enhance AAD Provider and Rego Code to automate checks for policy MS.AAD.3.3v1 Mar 22, 2024
@tkol2022 tkol2022 assigned Sloane4 and unassigned ssatyapal123 Mar 24, 2024
@schrolla schrolla force-pushed the 410-aad-rego-33v1-authenticator-context-v2 branch from 98c8ae3 to b4129cb Compare March 25, 2024 15:58
@tkol2022 tkol2022 force-pushed the 410-aad-rego-33v1-authenticator-context-v2 branch from b4129cb to 681200c Compare March 25, 2024 19:27
@tkol2022 tkol2022 assigned tkol2022 and unassigned Sloane4 Mar 25, 2024
@tkol2022 tkol2022 marked this pull request as draft March 27, 2024 20:01
@tkol2022 tkol2022 force-pushed the 410-aad-rego-33v1-authenticator-context-v2 branch 2 times, most recently from bed3aa5 to cd3dcc9 Compare April 15, 2024 22:23
@tkol2022 tkol2022 marked this pull request as ready for review April 15, 2024 22:36
@tkol2022 tkol2022 requested review from mitchelbaker-cisa and Sloane4 and removed request for tkol2022 and Sloane4 April 15, 2024 22:36
@tkol2022 tkol2022 requested review from dagarwal-mitre and james-garriss and removed request for mitchelbaker-cisa and Sloane4 April 29, 2024 17:12
@tkol2022 tkol2022 force-pushed the 410-aad-rego-33v1-authenticator-context-v2 branch from f265c05 to 2c57498 Compare April 29, 2024 17:22
Copy link
Collaborator

@james-garriss james-garriss left a comment

Choose a reason for hiding this comment

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

Nice work.

@tkol2022
Copy link
Collaborator

Nice work.

thx

@tkol2022
Copy link
Collaborator

@nanda-katikaneni - ready for merge

@nanda-katikaneni nanda-katikaneni merged commit b6d3bbc into main Apr 30, 2024
15 checks passed
@nanda-katikaneni nanda-katikaneni deleted the 410-aad-rego-33v1-authenticator-context-v2 branch April 30, 2024 18:41
@tkol2022 tkol2022 mentioned this pull request May 2, 2024
1 task
ahuynhMITRE added a commit that referenced this pull request May 6, 2024
* (New) Add Licensing Information to AAD Report (#1011)

* Copied Changes form broken branch for new PR

* removed trailing whitespace

* Removed more trailing white space

* updated licensing table to have unique id and added case in smoke test for licensing table

* remove white space

* added check for licensing information to functional tests

* added smoke test for licensing

* Add a UTC timestamp to the Provider JSON (#1009)

* add a quick timestamp

* add milliseconds

* timestamp_zulu

* add timestampzulu to metadata in mergejson

* fix unit tests

* fix bug found in previous PR

* Add config file for GitHub Pages (#1001)

* add YML for theme

* add config for pages

* lint

* put regal yaml back

* Create a workflow for bumping the ScubaGear module version (#989)

* create workflow for automated version bumping

* clean up whitespace

* bump checkout version

* fix variable substitution and regex match output

* remove testing variable

* Fix the Handful of AAD unit tests that are broken (#1019)

* fixed unit tests :)

* Resolved Linter Warnings

---------

Co-authored-by: Devesh Agarwal <dagarwal@mitre.org>

* Remove default vars from test configs (#996)

* minor edits to aad.5.3v1 and aad5.4v1 instructions (#1028)

* uncenter the text (#1031)

* Add option to generate a config file template (#984)

* config file generation

* Adding defender

* Final generate config file

* Ready for pull request

* moved to support module

* added to manifest file

* pascal case and whitespace

* linter hacks

* linter test

* "unused" variables

* linter hacks part II

* linter hacks part III

* Unit Test added

* added default value for unit testing

various syntax fixes as well

* fixed unit test, mock converting to yaml

* fixed unit tests?

* fixed?

* more debugging

* debugging

* Update README.md for generating config

* Update ScubaGear version to 1.2.0 (#1029)

Co-authored-by: GitHub Action <action@github.com>

* Fix bug with msaad52v1 only admins consent to apps (#1043)

* uploading for Cassey debugging session

* modified Rego policy 5.2 and unit tests to account for Microsoft updates to tenant output data

* modified 5.2 functional tests based on Microsoft changes to JSON output

* Update sample reports to latest version output examples (#1058)

* Fix publish package errors (#1042)

* inject write-error

* comment out publish

* show manifest error

* remove fail silently

* debug path version

* fix id typo

* fixed list

* fix needs

* fix release prep again

* write prereleasetag

* get types

* fix typo

* fix typos

* set tags as array

* reset tags

* debug w string

* remove array

* gc manifest

* get childitem

* write module path

* Debug manifest

* debug more

* typecast to string

* debug string concat

* display hashtable

* more debug details

* tweak debugging

* update install w prerelease

* commenting

* install required version

* add debug

* add debug

* add version

* use 1 if

* hard code

* install only

* force it

* remove install

* add debug statements

* see params

* fix parameter set

* fix comment typo

* use binding

* add comments

* fix @

* remove cmdlet

* uncomm param set

* remove param sets

* split into 2

* fix push paths

* add env back

* clean debug statement

* fix pipeline issues

* add output test

* use write-output

* clean up minor

* back to previous version

* use output

* more output

* use debug

* more output

* fix ps lint

* test returning false

* return to old error

* hardcode params

* fix step name

* improve debug statements

* back to write host

* debug prerelease

* changed info to host

* switch tag version

* write manifest out

* update version

* fix temp print

* bump to 8

* add import back

* require version

* bump to 10

* bump vers

* without tag

* bump tag

* use find mod

* fix unit test

* bump tag

* trivial change to trigger tests

* hide error

* v 03

* fix pipeline

* return false

* add write error

* remove details

* be silent

* be false

* cleanup

* unhardcode

* Exclude Write-Host locally

* commented out push trigger

* comment out push trigger

* update description

* Add a Workflow to check for OPA version updates (#1004)

* add parseable language

* add OPA Update workflow

* remove white space

* grammar

* Update Pull Request Description

Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>

---------

Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>

* Defender functional test plan fixes (#1057)

* Remove IsNotChecked flag from G3 3.1 Non-compliant - No defender license test

* Move impersonation protection tests from standard to g5 variant

* Fix Markdown in readme (#1068)

* fix table

* fix links

* minor changes

* Add Policy Group Names to ScubaResults.json (#1041)

* package Report Results by control group

* finish comment

* upgrade the ScubaGear ScubaResults metadata with even more context

* make comment clearer

* make another comment clearer

* Bump OPA version from v0.61.0 to v0.63.0 (#1070)

* Bump OPA version from v0.61.0 to  v0.63.0

* also add support for 0.62.1

* add omit comments

* add back in the space

* add back in the link

---------

Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: buidav <105074908+buidav@users.noreply.github.com>

* Add GitLeaks to CI/CD Pipeline (#1066)

* add if

* update pipeline

* remove push

* set permissions

* Enhance AAD Provider and Rego Code to automate checks for policy MS.AAD.3.3v1 (#1014)

* Update provider json for  auth method config

* Added new json for MS auth feature settings

* Combined authentication method calls into one json object

* Updates to JSON for aad 3.4 and 3.5

* updates to aad 3.3 rego

* Include all auth method configs in json export

* Updates to aad 3.3 to check msauth settings

* Updates to unit tests for aad 3.4

* Update AADConfig.rego 3.3v1 status check

* fix N/A criticality for 3.5 and adjust unit and functional tests for new JSON structure

* changed JSON structure for 3.4, 3.5 because it had redundant objects and updated all tests

* fix run unit test

* revised 3.3 unit tests

* update 3.3 code & added unit tests

* refactored names and added comments to 3.3 policy

* refactored 3.4 to reduce redundant code and bad variable naming

* streamlined code for 3.5 to reduce redundancy and returned ActualValue which was missing

* renamed MFAPolicies to PhishingResistantMFAPolicies for accuracy

* initial version 3.3 functional test wip

* added functional test cases

* removed NotCheckedDetails import since linter complained

* made AuthenticationPolicyMigrationIsComplete shorter due to linter complaints

* changed AuthenticationPolicyMigrationIsComplete from boolean assignment to if due to linter

* removed redundant call to Get-MgBetaPolicyAuthenticationMethodPolicy which was left by mistake

---------

Co-authored-by: Ted Kolovos <107076927+tkol2022@users.noreply.github.com>
Co-authored-by: Sloane4 <cdiaz@mitre.org>

* Bump OPA version from v0.63.0 to  v0.64.1 (#1079)

Co-authored-by: GitHub Action <action@github.com>

* fixed functional test for 5.3 so that it only produces a single EnableAdminConsentRequests field (#1081)

---------

Co-authored-by: Devesh Agarwal <147526644+dagarwal-mitre@users.noreply.github.com>
Co-authored-by: David Bui <105074908+buidav@users.noreply.github.com>
Co-authored-by: James Garriss <52328727+james-garriss@users.noreply.github.com>
Co-authored-by: Ted Kolovos <107076927+tkol2022@users.noreply.github.com>
Co-authored-by: Devesh Agarwal <dagarwal@mitre.org>
Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>
Co-authored-by: amart241 <106177534+amart241@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: Shanti Satyapal <78565245+ssatyapal123@users.noreply.github.com>
Co-authored-by: Sloane4 <cdiaz@mitre.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue or pull request will add new or improve existing functionality
Projects
None yet
6 participants