-
Notifications
You must be signed in to change notification settings - Fork 122
Feature 21955 - Manage the local administrators on Microsoft Entra joined devices #708
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
base: main
Are you sure you want to change the base?
Conversation
|
@merill @ramical
The current test fails organizations that implemented this recommendation. |
alexandair
left a comment
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.
| **Remediation action** | ||
|
|
||
| - [Manage the local administrators on Microsoft Entra joined devices](https://learn.microsoft.com/entra/identity/devices/assign-local-admin?wt.mc_id=zerotrustrecommendations_automation_content_cnl_csasci#manage-the-microsoft-entra-joined-device-local-administrator-role) | ||
| - [Manage the local administrators on Microsoft Entra joined devices](https://learn.microsoft.com/en-us/entra/identity/devices/assign-local-admin) |
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.
Per doc's guidance we should not include locales so I suggest not including the /en-US part
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.
I don't know who and why has changed that URL. Tracking parts of the URL are added during the build process.
Btw, most (if not all) of the specs are using "en-us" locale.
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.
In the docx, the link has en-us, Could you please let me know what I should keep there instead of en-us?
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.
I always remove "en-us" when I code.
However, I think that links in that section are overwritten by build process anyway. @merill can confirm that.
ramical
left a comment
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.
Added a couple of comments. Once you address, pls check in
@ramical Any comment on this? |
|
Fair point and thanks for connecting the dogts. This is a great cross-pillar scenario where we can use some advice from our Intune specialist. @Clay-Microsoft wondering if you can weigh in? From Entra, we have policy plane to inject local admins to entra devices and seems that intune does too. So what's the best practice here? |
@Clay-Microsoft - Need you inputs on this. |
|
Response from Clay - "Account protection policies via Intune are generally the preferred and better path. That is what we recommend." |
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.
Pull request overview
This PR rewrites the assessment logic for Test-Assessment.21955 to check if local administrators on Microsoft Entra joined devices are properly managed by querying role assignments from the database instead of querying a device registration policy API endpoint.
Changes:
- Replaced Graph API policy check with database query for Microsoft Entra Joined Device Local Administrator role assignments
- Added detailed reporting tables showing active (permanent) and eligible role assignments with user/group details
- Updated documentation to provide more comprehensive security threat context
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/powershell/tests/Test-Assessment.21955.ps1 | Rewrote assessment logic to query vwRole database view for role assignments instead of device registration policy, added detailed reporting with tables for permanent and eligible members |
| src/powershell/tests/Test-Assessment.21955.md | Expanded threat description with detailed attack scenario explanation, updated documentation link |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
alexandair
left a comment
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.
@praneeth-0000
Please, address my feedback.
| [ZtTest( | ||
| Category = 'Devices', | ||
| ImplementationCost = 'Low', | ||
| MinimumLicense = ('Free'), |
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.
License Requirement Mismatch:
The code specifies MinimumLicense = 'Free', but the official Microsoft documentation for managing "Microsoft Entra Joined Device Local Administrator" states:
"This option requires Microsoft Entra ID P1 or P2 licenses."
Additionally, the test logic checks for Eligible assignments (PIM), which definitely requires an Entra ID P2 license. Since the passing condition allows for either permanent (P1) or eligible (P2) assignments, the minimum license to effectively use the feature being tested is at least P1.
Someone from Identity team needs to confirm this.
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.
Hi @SagarSathe , previously the 21955 spec contained the minimum license as free but after the spec was re written they are not mentioned in docx. Like @alexandair mentioned, this has to be updated based on new logic.
| MinimumLicense = ('Free'), | ||
| Pillar = 'Identity', | ||
| RiskLevel = 'High', | ||
| SfiPillar = 'Protect tenants and isolate production systems', |
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.
The spec specifies "Secrets or Protect identities".
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.
Hi @SagarSathe , need confirmation on SFI Pillar, previous version of Spec 21955 mentioned "Protect tenants and isolate production systems" after the revamp the details were not present in the spec.
Rewrote the assessment logic as per new spec
Closes #678