Add privileged service principals table to AAD report #1467
Conversation
dagarwal-mitre
added
the
enhancement
| $PrivilegedServicePrincipals = @{} | ||
|
|
||
| #PrivilegedObjects is an array because of the tracker.trycommand, and so the first index is the hashtable | ||
| foreach ($key in $PrivilegedObjects[0].Keys) { |
There was a problem hiding this comment.
Are we certain that $PrivilegedObjects will always contain at least one entry? Otherwise accessing the 0th index may throw an error if an entry does not exist.
Either way, I also think it would help to see some error handling to check if $PrivilegedObjects[0].Keys is not null prior to entering the loop.
| @@ -270,13 +270,33 @@ function New-Report { | |||
| # Create a section header for the licensing information | |||
| $LicensingHTML = "<h2>Tenant Licensing Information</h2>" + $LicenseTable | |||
|
|
|||
| # Create a section for privileged service principals | |||
| $privilegedServicePrincipalsTable = $SettingsExport.privileged_service_principals.psobject.properties | ForEach-Object { | |||
There was a problem hiding this comment.
Logic in AADProvider:
If $PrivilegedServicePrincipals is $null, then it equals {}. Otherwise, set equal to $PrivilegedServicePrincipals
Then in the AADProvider json set "privileged_service_principals" equal to $PrivilegedServicePrincipals
If "privileged_service_principals" = {}, then $principal.DisplayName, $principal.ServicePrincipalId, etc. are nonexistent properties. Can we add some error handling here?
We could check $SettingsExport.privileged_service_principals.PSObject.Count > 0 or some other condition.
| @@ -274,6 +291,9 @@ function Get-PrivilegedUser { | |||
| if ($Objecttype -eq "user") { | |||
| LoadObjectDataIntoPrivilegedUserHashtable -RoleName $Role.DisplayName -PrivilegedUsers $PrivilegedUsers -ObjectId $User.Id -TenantHasPremiumLicense $TenantHasPremiumLicense -M365Environment $M365Environment -Objecttype "user" | |||
| } | |||
| elseif ($Objecttype -eq "servicePrincipal") { | |||
| LoadObjectDataIntoPrivilegedUserHashtable -RoleName $Role.DisplayName -PrivilegedUsers $PrivilegedUsers -ObjectId $User.Id -TenantHasPremiumLicense $TenantHasPremiumLicense -M365Environment $M365Environment -Objecttype "serviceprincipal" | |||
There was a problem hiding this comment.
Can we add a new unit test to handle $Objecttype="serviceprincipal" in LoadObjectDataIntoPrivilegedUserHashTable.Tests.ps1?
| @@ -1,5 +1,5 @@ | |||
| Import-Module -Name $PSScriptRoot/../ExportEXOProvider.psm1 -Function Get-ScubaSpfRecord, Get-ScubaDkimRecord, Get-ScubaDmarcRecord | |||
| Import-Module -Name $PSScriptRoot/../ExportAADProvider.psm1 -Function Get-PrivilegedRole, Get-PrivilegedUser | |||
| Import-Module -Name $PSScriptRoot/../ExportAADProvider.psm1 -Function Get-PrivilegedRole, Get-PrivilegedUser, Get-PrivilegedServicePrincipal | |||
There was a problem hiding this comment.
I don't see Get-PrivilegedServicePrincipal as a function in your branch's ExportAADProvider.psm1.
| @@ -437,7 +474,107 @@ function LoadObjectDataIntoPrivilegedUserHashtable { | |||
| } | |||
|
|
|||
| } | |||
|
|
|||
| # function LoadObjectDataIntoPrivilegedUserHashtable { | |||
There was a problem hiding this comment.
Remove comment blocks
dagarwal-mitre
force-pushed
the
934_service_principals_to_aad_report
branch
from
74932fa to
62dafde
Compare
dagarwal-mitre
force-pushed
the
934_service_principals_to_aad_report
branch
from
62dafde to
7200154
Compare
🗣 Description
Wanted to add privileged service principals to the report to make users aware of these high privileged principals within their tenants
💭 Motivation and context
#934
🧪 Testing
Test with different tenants and ensure the Privileged Service Principals table is produced as expected for the AAD report and verify the verify
privileged_service_principalsis added in ScubaResults.json.✅ 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 branchbutton 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
Demonstrate changes to the team for questions and comments.
(Note: Only required for issues of size
Mediumor larger)✅ Post-merge checklist