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 AZAddOwner relationship creation #687

Closed
wants to merge 1 commit into from

Conversation

cntC0d3
Copy link

@cntC0d3 cntC0d3 commented Jul 8, 2024

Add AZAddOwner relationship between users with Hybrid Identity Administrator, Partner Tier1 Support, Partner Tier2 Support, or Directory Synchronization Accounts roles and all Service Principals and Apps within the tenant

Description

Add logic to analysis/azure/post to perform post-processing analysis for AZAddOwner edges in the UserRoleAssignments function. This was completed by adding a new addOwners function that fetches all ServicePrincipals and Apps within the tenant and proceeds to create the edge between users with the following roles:

  • Hybrid Identity Administrator
  • Partner Tier1 Support
  • Partner Tier2 Support
  • Directory Synchronization Accounts

Motivation and Context

This PR addresses: Issue #686

Fully implements documented behavior for AZAddOwner relationships per documentation.

How Has This Been Tested?

Verifying issue:

  1. Collect Azurehound data for tenant with users assigned with at least one of the above role.s
  2. Upload data to bloodhound server and wait for ingestion.
  3. Run the following cypher command against data:
MATCH p=()-[r:AZAddOwner]->() RETURN p LIMIT 25
  1. verify no relationships exist.

Verifying fix.

  1. Rerun steps 1-3 above.
  2. Verify expected relationships between known users with role and all apps/service principals within the same tenant.

Screenshots (optional):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

…strator, Partner Tier1 Support, Partner Tier2 Support, or Directory Synchronization Accounts roles and all Service Principals and Apps within the tenant
Copy link

github-actions bot commented Jul 8, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@cntC0d3
Copy link
Author

cntC0d3 commented Jul 9, 2024

I have read the CLA Document and I hereby sign the CLA

@cntC0d3
Copy link
Author

cntC0d3 commented Jul 9, 2024

recheck

@StephenHinck
Copy link
Collaborator

Hey, @cntC0d3 - thanks for the PR! We're prepping a release right now and won't be able to get a full review before this one, but we'll be sure to get this reviewed here in the next couple of weeks!

@JonasBK
Copy link
Collaborator

JonasBK commented Jul 25, 2024

Hey @cntC0d3,

Thanks for the PR! It looks good - great job 🙌

We have however started to model the permissions of roles directly out of the AZRole nodes instead of out of the principals with the role assignment. See AZResetPassword as example:
image
In that way, we reduce the total number of edges created (better performance) and it becomes clear how a given principal has a permission through a role assignment.

Are you up for modifying your AZAddOwner implementation to match that pattern?

The documentation for the AZAddOwner edge has to be updated to match the new pattern. I'll take care of that.

@cntC0d3
Copy link
Author

cntC0d3 commented Jul 25, 2024

@JonasBK - yeah I can knock that out in the next couple days. 😃

@JonasBK
Copy link
Collaborator

JonasBK commented Jul 25, 2024

Awesome! 🙌

@slokie-so slokie-so added question Further information is requested needs more info This issue requires more information work in progress This pull request is a work in progress and should not be merged blocked This pull request cannot be completed and should not be merged ticketed Ticket has been created internally for tracking labels Jul 31, 2024
@cntC0d3
Copy link
Author

cntC0d3 commented Aug 23, 2024

@JonasBK - since Microsoft decided to fix many of the issues associated with this edge I haven't had much motivation to continue working this. Closing PR.

@cntC0d3 cntC0d3 closed this Aug 23, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 2024
@JonasBK
Copy link
Collaborator

JonasBK commented Aug 28, 2024

Hey @cntC0d3,

Talked with Andy about it. It is only Directory Synchronization Accounts that will loose the ability to add owners to SPs and Apps to our understanding. So your PR is still relevant for the Hybrid Identity Administrator, Partner Tier1 Support, and Partner Tier2 Support roles.

Let me know if you are still interested in fixing up the PR. All cool if not - then we will get it fixed :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocked This pull request cannot be completed and should not be merged needs more info This issue requires more information question Further information is requested ticketed Ticket has been created internally for tracking work in progress This pull request is a work in progress and should not be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants