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 graph tier0 permission validation #645

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

HenrikPiecha
Copy link

Added test where app registrations are checked for having tier-0 graph permissions with a risk of having a direct or indirect path to Global Admin and full tenant takeover.
This test was inspired by the Community Post in "Entra 🆔 News #80 → This week in Microsoft Entra" from "Emilien Socchi".

Further information: https://github.com/emiliensocchi/azure-tiering/tree/main/Microsoft%20Graph%20application%20permissions#tier-0

@HenrikPiecha HenrikPiecha requested review from a team as code owners January 29, 2025 13:23
Copy link
Contributor

@SamErde SamErde left a comment

Choose a reason for hiding this comment

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

This PR should only contain changes related to the test description:

Added test where app registrations are checked for having tier-0 graph permissions with a risk of having a direct or indirect path to Global Admin and full tenant takeover.

This will make review, approval, and merging easier to safely manage.

@HenrikPiecha HenrikPiecha force-pushed the add-graph-tier0-permission-validation branch from 2a5d7cd to 465e55e Compare February 5, 2025 10:08
@HenrikPiecha
Copy link
Author

Removed commits that are handeled in separate PR.

@@ -0,0 +1,53 @@
Ensure no graph application has permissions with a risk of having a direct or indirect path to Global Admin and full tenant takeover.

This test if any application has tier-0 graph permissions with a risk of having a direct or indirect path to Global Admin and full tenant takeover.
Copy link
Contributor

Choose a reason for hiding this comment

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

Grammar? "This test checks if any application..."

Copy link
Author

Choose a reason for hiding this comment

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

Has been adjusted.

Copy link
Contributor

@SamErde SamErde left a comment

Choose a reason for hiding this comment

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

Nice test. I'm curious how the report could advise admins that might have an app with required permissions (grey area!), or if exceptions might ever be a possibility.

Corrected grammar issue.
-> "This test 'checks' if any application..."
@HenrikPiecha
Copy link
Author

Well... there are a lot of scenarios where at least permissions with an indirect path to a global administrator are required. Maester itself requires “RoleEligibilitySchedule.ReadWrite.Directory”, which also has a known indirect path to a global administrator.
This test should make the criticality of these permissions transparent to admins. They must assess whether an existing application can receive these privileges or not.

Should I extend the test-description with this information?

@SamErde
Copy link
Contributor

SamErde commented Feb 5, 2025

Well... there are a lot of scenarios where at least permissions with an indirect path to a global administrator are required. Maester itself requires “RoleEligibilitySchedule.ReadWrite.Directory”, which also has a known indirect path to a global administrator. This test should make the criticality of these permissions transparent to admins. They must assess whether an existing application can receive these privileges or not.

Should I extend the test-description with this information?

I do like the idea of putting that concept into the description, so admins don't get the impression that it is an all or nothing decision for 100% of real-world scenarios. 👍

Added conceptional description for valid use-cases into test description: 
"There are several use cases where Tier-0 permissions with an indirect attack path are required. For example, Maester itself requires the permission “RoleEligibilitySchedule.ReadWrite.Directory” to properly validate the PIM assignments. Nevertheless, an administrator should question the use of these permissions and check whether less critical permissions are also sufficient. Applications that are provided by third-party vendors that do have Tier-0 permissions with direct or indirect attack paths should strictly be questioned and monitored. "
@HenrikPiecha
Copy link
Author

Updated test description as discussed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants