-
Notifications
You must be signed in to change notification settings - Fork 225
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 to handle nested PIM groups and refactor Get-PrivilegedUser to reduce code duplication #1310
Enhance AAD provider to handle nested PIM groups and refactor Get-PrivilegedUser to reduce code duplication #1310
Conversation
Testing Instructions for Reviewers*** Important: See my separate comment on the security impacts of testing this PR.At a high level these are the scenarios that require testing:
*Note: I added warning output statements that will show up in your Powershell console when you are testing. This will help you observe the objects (users and groups) that ScubaGear is processing so you can track your testing scenarios. Overall Scenario 1 - AAD group is assigned to a PIM group causes the tool to crash (nested groups)Review issue #1306 to understand the scope of the problem and see screenshots here and here. Follow the steps to reproduce the problem in #1306 and document the results in the comment template associated with your name. Overall Scenario 2 - AAD user or group is deleted before running ScubaGear causes the tool to crashReview issue #1305 to understand the scope of the problem and follow the steps described there to reproduce the problem and document the results in the comment template associated with your name. Overall Scenario 3 - Regression test AAD to ensure that ScubaGear still produces correct results for users / groups assigned to highly privileged roles, including groups that are PIM groupsFor regression testing scenarios, follow the comment template associated with your name. |
Security Guidance for testing this PRSince testing this PR requires making assignments of users and groups to highly privileged roles, this introduces operational security risks to the tenants that you will use for testing. Follow this guidance to reduce risks. I sent you an email with the subject "managing risks with test accounts" that describes the paradigm that I setup which uses a special Entra Id group and conditional access policy that I associate with test users to handle operational risks. Read that for specifics. I plan to setup the same paradigm in all our test tenants so that you can leverage it.
*Note: we might want to leave some of the test objects (users and groups) that you created in Entra Id for the long term, if we plan to use those objects to perpetually test the AAD provider in ScubaGear (via automated or manual testing). We can discuss this at the stand-up. |
Nanda - Overall Scenario 1 - Test Results - AAD group is assigned to a PIM group causes the tool to crash (nested groups)
|
Nanda - Overall Scenario 2 - Test Results - AAD user or group is deleted before running ScubaGear causes the tool to crash
|
Nanda - Overall Scenario 3 - Regression test AAD to ensure that ScubaGear still produces correct results for users / groups assigned to highly privileged roles
|
@mitchelbaker-cisa @buidav I provided three template comments above with @nanda-katikaneni name on them. You can copy paste those comments to create your own template. Feel free to tweak it for what works best for you. Use the slack to communicate across the review team or with me if you have questions or comments. |
You need to run Initialize-Scuba before executing the fix branch to install the Powershell dependency module for a new Cmdlet that was introduced in this fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few questions. But mainly I think you need to add some unit tests for your new function with mock data. Please do that.
@mitchelbaker-cisa @nanda-katikaneni @buidav I corrected a flaw in how the recursion count was being processed so make sure to test that the recursion stops after 2 levels deep in the tree. I added a temporary print statement to help you keep track of which level in the recursion tree is processing while ScubaGear is executing. |
@gdasher I added some new unit tests including one for recursion that helped identify a flaw in how I was incrementing the recursion count. Please check them out. |
…ps assigned to PIM groups
…-MgBetaDirectoryObject
…iled error stacktrace to Get-PrivilegedRole
…oadObjectDataIntoPrivilegedUserHashtable
… privileged roles from hashtable to array
5bbe424
to
29a6421
Compare
Tested high-level scenarios#1 (AAD group is assigned to a PIM group causes the tool to crash) and scenario#2 (AAD user or group is deleted before running ScubaGear causes the tool to crash) and their individual test cases on G5 and E5 tenants. For all test cases with main branch, AAD testing crashes with 404 error for the "highly privileged access" group. With fix branch, no crashes and test results as expected. Also ran regression tests and compared json's with main and fix branches - results as expected. TBD: full functional test runs on fix branch after the rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the testing scenarios 1-3 behave as expected. I have the last 8 scenarios to do with the fix applied but will be finished with that soon.
Overall Scenario 1 - Test Results - AAD group is assigned to a PIM group causes the tool to crash (nested groups)
Branch | Tenant | Scenario | Expected Results | Actual Results |
---|---|---|---|---|
main | G5 | Scenario 1 - Nested group assignment in PIM for Groups (Active assigned to role) | crash 404 error | 404 (NotFound), Errorcode: Request_ResourceNotFound |
main | G5 | Scenario 2 - Nested group assignment in PIM for Groups (Eligible assigned to role) | crash 404 error | 404 (NotFound), Errorcode: Request_ResourceNotFound |
main | G5 | Scenario 3 - Nested group assignment in PIM for Groups (PIM group circular reference) | crash 404 error | 404 (NotFound), Errorcode: Request_ResourceNotFound |
fix | G5 | Scenario 1 - Nested group assignment in PIM for Groups (Active assigned to role) | no crash | no crash, reports generated |
fix | G5 | Scenario 2 - Nested group assignment in PIM for Groups (Eligible assigned to role) | no crash | no crash, reports generated |
fix | G5 | Scenario 3 - Nested group assignment in PIM for Groups (PIM group circular reference) | no crash | no crash, reports generated |
Overall Scenario 2 - Test Results - AAD user or group is deleted before running ScubaGear causes the tool to crash
Branch | Tenant | Scenario | Expected Results | Actual Results |
---|---|---|---|---|
main | G5 | Scenario 1 - Deleted user (Active role assignment) | crash 404 error | 404 (NotFound), Errorcode: Request_ResourceNotFound |
main | G5 | Scenario 2 - Deleted user (Eligible role assignment) | crash 404 error | 404 (NotFound), Errorcode: Request_ResourceNotFound |
main | G5 | Scenario 3 - Deleted group (Active role assignment) | crash 404 error | 404 (NotFound), Errorcode: Request_ResourceNotFound |
main | G5 | Scenario 4 - Deleted group (Eligible role assignment) | crash 404 error | 404 (NotFound), Errorcode: Request_ResourceNotFound |
fix | G5 | Scenario 1 - Deleted user (Active role assignment) | no crash | no crash, reports generated |
fix | G5 | Scenario 2 - Deleted user (Eligible role assignment) | no crash | no crash, reports generated |
fix | G5 | Scenario 3 - Deleted group (Active role assignment) | no crash | no crash, reports generated; tried with owner role and same result |
fix | G5 | Scenario 4 - Deleted group (Eligible role assignment) | no crash | no crash, reports generated |
Overall Scenario 3 - Regression test AAD to ensure that ScubaGear still produces correct results for users / groups assigned to highly privileged roles
Branch | Tenant | Scenario | Expected Results | Actual Results |
---|---|---|---|---|
main | G5 | Scenario 1 - User is assigned to privileged role as Active | the user is included in the provider JSON | users with active assignment of sharepoint admin included in provider JSON |
main | G5 | Scenario 2 - User is assigned to privileged role as Eligible | the user is included in the provider JSON | users with eligible assignment of sharepoint admin included in provider JSON |
main | G5 | Scenario 3 - Regular Group is assigned to privileged role as Active | all users that are members of the group are included in the provider JSON | two users with no role assignment, but active assigned to a regular group with sharepoint admin; both users included under privileged_users |
main | G5 | Scenario 4 - Regular Group is assigned to privileged role as Eligible | all users that are members of the group are included in the provider JSON | two users with no role assignment, but eligible assigned to a regular group with sharepoint admin; both users included under privileged_users |
main | G5 | Scenario 5 - PIM group is assigned to privileged role as Active - User is assigned to PIM group as Eligible | the user is included in the provider JSON | two users with no role assignment, but eligible assigned to a PIM group with sharepoint admin as active; both users included under privileged_users |
main | G5 | Scenario 6 - PIM group is assigned to privileged role as Active - User is assigned to PIM group as Active | the user is included in the provider JSON | two users with no role assignment, but active assigned to a PIM group with sharepoint admin as active; both users included under privileged_users |
main | G5 | Scenario 7 - PIM group is assigned to privileged role as Eligible - User is assigned to PIM group as Eligible | the user is included in the provider JSON | two users with no role assignment, but eligible assigned to a PIM group with sharepoint admin as eligible; both users included under privileged_users |
main | G5 | Scenario 8 - PIM group is assigned to privileged role as Eligible - User is assigned to PIM group as Active | the user is included in the provider JSON | two users with no role assignment, but active assigned to a PIM group with sharepoint admin as eligible; both users included under privileged_users |
fix | G5 | Scenario 1 - User is assigned to privileged role as Active | the user is included in the provider JSON | users with active assignment of sharepoint admin included in provider JSON |
fix | G5 | Scenario 2 - User is assigned to privileged role as Eligible | the user is included in the provider JSON | users with eligible assignment of sharepoint admin included in provider JSON |
fix | G5 | Scenario 3 - Regular Group is assigned to privileged role as Active | all users that are members of the group are included in the provider JSON | two users with no role assignment, but active assigned to a regular group with sharepoint admin; both users included under privileged_users |
fix | G5 | Scenario 4 - Regular Group is assigned to privileged role as Eligible | all users that are members of the group are included in the provider JSON | two users with no role assignment, but eligible assigned to a regular group with sharepoint admin; both users included under privileged_users |
fix | G5 | Scenario 5 - PIM group is assigned to privileged role as Active - User is assigned to PIM group as Eligible | the user is included in the provider JSON | two users with no role assignment, but eligible assigned to a PIM group with sharepoint admin as active; both users included under privileged_users |
fix | G5 | Scenario 6 - PIM group is assigned to privileged role as Active - User is assigned to PIM group as Active | the user is included in the provider JSON | two users with no role assignment, but active assigned to a PIM group with sharepoint admin as active; both users included under privileged_users |
fix | G5 | Scenario 7 - PIM group is assigned to privileged role as Eligible - User is assigned to PIM group as Eligible | the user is included in the provider JSON | two users with no role assignment, but eligible assigned to a PIM group with sharepoint admin as eligible; both users included under privileged_users |
fix | G5 | Scenario 8 - PIM group is assigned to privileged role as Eligible - User is assigned to PIM group as Active | the user is included in the provider JSON | two users with no role assignment, but active assigned to a PIM group with sharepoint admin as eligible; both users included under privileged_users |
...ng/Unit/PowerShell/Providers/AADProvider/LoadObjectDataIntoPrivilegedUserHashtable.Tests.ps1
Show resolved
Hide resolved
Summary of results for E5 and G5 Tenants: Overall Scenario 1 - Test Results for E5 and G5 tenants - AAD group is assigned to a PIM group causes the tool to crash (nested groups)
Overal Scenario 2 - Test Results for E5 and G5 tenants - AAD user or group is deleted before running ScubaGear causes the tool to crash
Overall scenario # 3 is done with only E5 tenant - expected results for most (will add after couple more tests); regression functional tests on the E5 tenant with fix branch worked fine (G5 functional tests tbd). Based on the review/testing so far, only suggestion is to remove the debug print statements before merge - while its really useful in debugging the nested PIM groups, the command line execution for aad has become very chatty. As an alternative, consider on-demand debugging feature/flag for a future release. |
...ng/Unit/PowerShell/Providers/AADProvider/LoadObjectDataIntoPrivilegedUserHashtable.Tests.ps1
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great Ted, please refer to my comment above on completed test scenarios and their results. Last ask from me is to create an issue to capture how we might approach building functional tests for handling nested PIM groups and deleted users/groups.
Overall Scenario 3 - Test results for E5 tenant - regression test AAD to ensure that ScubaGear still produces correct results for users / groups assigned to highly privileged roles
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No additional comments.
Created #1340 - Looking for someone to lead that issue to improve the functional testing of PIM functionality. |
@nanda-katikaneni ready to go |
…d because it was causing errors since powershell doesn't have the graph modules loaded
@nanda-katikaneni I fixed the unit tests. Ready for merge. |
🗣 Description
The general impetus for this PR was a publicly reported edge case where the AAD provider crashes when an admin assigns a nested group to an AAD PIM group as an Eligible assignment. In order to implement the fix, I determined that augmenting the current code in Get-PrivilegedUser was a bad idea because it had already become hard to maintain with lots of duplication, so I refactored Get-PrivilegedUser to call a new helper function named LoadObjectDataIntoPrivilegedUserHashtable which takes a user or a group as an argument, fetches the metadata about the object and loads the metadata into the PrivilegedUsers hashtable which is used to form the privileged_users section of the provider JSON document. The other major defect that this PR addresses is that the code now handles a scenario where a user or a group is deleted from the directory in the hours before running ScubaGear, this can cause ScubaGear to crash if the object was Eligible assigned to a PIM group.
Both of the defects described above, the nested groups and the deleted objects cause ScubaGear to crash with a 404 (NotFound) Request_ResourceNotFound error, even though the root cause is completely unrelated.
*I have some Write-Warning statements dispersed in the code right now which are only to assist the pull request reviewers with testing the various scenarios. These will be removed.
Iterative list of code changes:
Closes #1288
Closes #1306
Closes #1305
Closes #1303
ToDo
🧪 Testing
Due to the size of the PR description, I provide testing instructions as detailed comments. Each reviewer should have a comment template with multiple test cases associated with your name.
✅ Pre-approval checklist
✅ 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