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

Add PIM for groups support to AAD provider #794

Conversation

tkol2022
Copy link
Collaborator

@tkol2022 tkol2022 commented Jan 11, 2024

🗣 Description

This pull request modifies the AAD provider's Get-PrivilegedUser function to support getting user to PIM Group eligible assignments which is a current gap in our code. The PR also addresses a longstanding bug where users would sometimes get a duplicate role in their privileged_users node in the JSON. I also changed the connection module to request the required MS Graph permission scope since we are introducing a new cmdlet and I added the permission to the respective sections in the README file.

Closes #757
Closes #703

🧪 Testing

I tested this by creating test users and groups in all the test tenants that support PIM to ensure that the AAD code picks up the users that are assigned to a PIM group as Eligible. No updates were necessary to the functional tests because nothing changed in the JSON structure that would affect the Rego. I spoke with Crutch and we might want to add Powershell unit tests in the future that will exercise the code in the Get-PrivilegedUser and Get-PrivilegedRole functions of the AAD provider since those are what the types of changes associated with with this PR were associated with.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep! (kind of - i snuck in a separate bug fix)
  • All future TODOs are captured in issues, which are referenced
    in code comments.
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • 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

  • Revert dependencies to default branches.
  • Finalize version.

✅ Post-merge checklist

  • Create a release.

@tkol2022 tkol2022 added bug This issue or pull request addresses broken functionality enhancement This issue or pull request will add new or improve existing functionality labels Jan 11, 2024
@tkol2022 tkol2022 added this to the Flipper milestone Jan 11, 2024
@tkol2022 tkol2022 self-assigned this Jan 11, 2024
@tkol2022
Copy link
Collaborator Author

How to test this PR in your own tenant

**These instructions assume that you are going to test in a Sandia tenant, but if you are a reviewer on this PR I would read this anyway since it describes how I tested these code changes myself.

** It is important when following these setup instructions that you pay special attention to where assignments are made as "Active" or "Eligible" depending on the setup step. You need to make sure that you set that up exactly as described here so that when you run ScubaGear it will exercise the new code fragments that I added for the enhancement. Also, there are assignments that are made to roles and assignments that are made to groups. When assigning to groups, we will be doing that from the PIM for Groups portal page and NOT the Entra ID portal (this may be clear in the instructions but I wanted to call it out anyway). You can change the name of the groups to remove the "GCC High" portion if you want.

Entra ID portal
Create four new security groups named Ted's GCC High PIM Test Group, Ted's 2 GCC High PIM Test Group, Ted's 3 GCC High PIM Test Group, Ted's 4 GCC High PIM Test Group
Make sure to select the option that roles can be assigned to the group

Create 4 new test users with the convention TestPIMUser1 (display name = Test PIM User 1)
When you create the user make sure their account is disabled. We will not be logging in as these users, just using them for group membership testing.

PIM portal
Click Manage > Groups
Click Discover groups at the top of the page
Select the four new groups that were added an then click the Manage groups button at the top. This will "onboard" the groups into PIM.

Entra ID portal
Click Manage > Roles and administrators
Select the role Sharepoint Administrator
Click Add assignments
Assign the group Ted's GCC High PIM Test Group to the role as an "Active" assignment
Repeat the steps above but assign the group Ted's 2 GCC High PIM Test Group as an "Eligible" assignment

Repeat the steps above for the Hybrid Identity Administrator role but assign the group named Ted's 3 GCC High PIM Test Group as an "Active" assignment
Repeat the steps above for the Hybrid Identity Administrator role but assign the group named Ted's 4 GCC High PIM Test Group as an "Eligible" assignment

PIM portal
Click Manage > Groups
Select the group Ted's GCC High PIM Test Group
When the group opens up, select Manage > Assignments
Click Add assignments
Under the Select Role dropdown box, select Member
Under Select member(s) select Test PIM User 1
Click Next and then Assign the user as "Eligible"

When assigning users to the groups in PIM, you can also select users from the tenant that already have other administrative roles assigned to them to provide the new AAD code with more potential states for testing purposes. Look at my sample user assignment to groups in PIM distribution below. In this example the users Test PIM User 1 through 4 will only have a single role assigned to them if you follow my instructions, but the preexisting users Ted and Nanda already have other privileged roles assigned. If you select other users to add to the testing make sure that those users do not have an existing assignment to the roles Sharepoint Administrator or Hybrid Administrator so as not to skew the test.

Ted's GCC High PIM Test Group
Test PIM User 1
Ted Kolovos

Ted's 2 GCC High PIM Test Group
Test PIM User 2
Nanda

Ted's 3 GCC High PIM Test Group
Test PIM User 3

Ted's 4 GCC High PIM Test Group
Test PIM User 4

Testing and what do we expect to see when running ScubaGear?

  • Once you setup the tenant as per the instructions above, execute ScubaGear and examine the ProviderSettingsExport.json
  • Find each of the users that were assigned above inside the privileged_users node in the JSON
  • Review each user's respective "roles" in the JSON and make sure that they match the distribution below (yours may vary slightly depending on which additional users you included for the test in your environment)

The following users should have the Sharepoint Administrator role
Test PIM User 1
Ted Kolovos
Test PIM User 2
Nanda

The following users should have the Hybrid Identity Administrator role
Test PIM User 3
Test PIM User 4

@schrolla
Copy link
Collaborator

Per first item in the pre-approval checklist, this PR needs to be updated to have a descriptive and human-readable title. The title of the PR becomes the subject of the final commit message that describes the enhancement in the commit history and is used to inform next release change log. Prefer use of present tense verb followed by a short description of the change.

@tkol2022 tkol2022 changed the title 757 update AADprovider to include PIM for groups data in provider JSON Add PIM for groups support to AAD provider Jan 12, 2024
Copy link
Collaborator

@buidav buidav left a comment

Choose a reason for hiding this comment

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

FYSA this review might take a bit longer, as I encountered some issues that I'm still trying to figure out the root causes.

First issue that I unintentionally discovered. I forgot to add the
PrivilegedEligibilitySchedule.Read.AzureADGroup scope to my Graph application which caused Get-MgBetaIdentityGovernancePrivilegedAccessGroupEligibilityScheduleInstance to fail when I authenticated.
However, I did not get an error message thrown from ScubaGear. Something is suppressing the error message that should've been thrown.

Second issue is still occurring after following and double checking the instructions above. The Test PIM users are not showing up inside privileged_users key inside the SNL Test tenant. Only after I assign the test users to the PIM group in the PIM portal as active instead of eligible do they show up in the privileged_users key.

Also, have some small style guide diligence comments.

@schrolla schrolla force-pushed the 757-update-aad-provider-to-include-pim-for-groups-data-in-json branch from 3963f34 to f5bf26f Compare January 19, 2024 15:53
@schrolla schrolla changed the base branch from flipper to main January 19, 2024 15:55
@schrolla schrolla force-pushed the 757-update-aad-provider-to-include-pim-for-groups-data-in-json branch from f5bf26f to 1c91a5f Compare January 19, 2024 15:56
@schrolla
Copy link
Collaborator

@tkol2022 This PR has been updated to retarget the main branch and the associated feature branch has been rebased onto the main branch as well. If you have a local copy of the branch, please update it from the remote via a git pull --rebase. If you have any questions, please let me know.

@tkol2022 tkol2022 linked an issue Jan 19, 2024 that may be closed by this pull request
2 tasks
Copy link
Collaborator

@buidav buidav left a comment

Choose a reason for hiding this comment

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

My original errors are purely latency related. See comments below for another error encountered and a suggested fix.

Copy link
Contributor

@ssatyapal123 ssatyapal123 left a comment

Choose a reason for hiding this comment

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

Verified on G5 that all users were added as noted in the test description. Added another test user "Test PIM User 5" to "Ted's 2 PIM Group" and verified that user was also added to the privileged_users array with the SharePoint role.

Copy link
Collaborator

@buidav buidav left a comment

Choose a reason for hiding this comment

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

Tested in the E5 test tenant. Users show up in the JSON as expected after following the instructions. However, there was a significant time delay (at least in the E5 tenant I tested in) when the Eligible assigned users start appearing the data. A thing to keep in mind when testing this setting in the future.

Tests in G5 and GCC High come up with no errors. (Did not create groups myself since I see that there are already groups there.)

Good to go with a quick reminder:

@tkol2022 reminder to add the PrivilegedEligibilitySchedule.Read.AzureADGroup scope to our Service Principals for both regular and functional testing SPs in the various test tenants and consent to the new permission.

tkol2022 and others added 5 commits January 24, 2024 13:03
Co-authored-by: David Bui <105074908+buidav@users.noreply.github.com>
Co-authored-by: David Bui <105074908+buidav@users.noreply.github.com>
Co-authored-by: David Bui <105074908+buidav@users.noreply.github.com>
@tkol2022 tkol2022 force-pushed the 757-update-aad-provider-to-include-pim-for-groups-data-in-json branch from 9828a79 to e1405b8 Compare January 24, 2024 18:03
@nanda-katikaneni nanda-katikaneni merged commit 562d397 into main Jan 25, 2024
3 checks passed
@nanda-katikaneni nanda-katikaneni deleted the 757-update-aad-provider-to-include-pim-for-groups-data-in-json branch January 25, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue or pull request addresses broken functionality enhancement This issue or pull request will add new or improve existing functionality
Projects
None yet
5 participants