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

[Enhancement\Feature Request] - Alignment of code accross modules and Get-All #57

Open
angry-bender opened this issue Mar 5, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@angry-bender
Copy link
Contributor

angry-bender commented Mar 5, 2024

Hey Team,

Just thought id raise an issue for longer term tracking.

I've noticed some of the scripts key functions are not consistent accross the available modules. It might be good to get a review in to ensure each module behaves the same for the following pieces i can think of off the top of my head:

  1. Authentication - GraphApi (When does a user consent? what flags are required, or do they do it before?). Has a user successfully signed into modules before using commands like search-unifiedauditlogs (Noting admins have to grant view-onlyauditlogs in both AzureAD and M365). Where they have it assigned in azure, but not M365, the search-unifiedauditlogs module may not be recognised
  2. Error Handling and checking - Some modules have error handling on the flags that are passed, some do not.
  3. File conventions - Some outputfiles have $date for de-duplication, some do not
  4. Fields - Might be an opportunity to collect fields in the same order between the GraphAPI modules and legacy Get-AzureAD modules

Once these are aligned, it would be awesome to have a get-all script that wraps together the collections (Perhaps with a legacy option for when a user cannot get Graph-API Access approved)

I note this is a big one, and i can try to tap away on it longer term, but unsure if i have the capacity for a bigger undertaking like this :-)

@JoeyInvictus
Copy link
Collaborator

Hi, thank you for the valuable feedback. This is indeed a big one, going require some time and effort. We can try to work on this and chip away at it over the longer term. Mainly, it's just me maintaining this project. So, it's sometimes hard to find time during my regular work. Since it doesn't bring in any money, it often gets less priority than other tasks. However, I will try my best to address the listed issues when I have some available time. Feel free to contribute as well, your pull requests have been great so far! :)

Regarding the points you've raised:

Authentication is indeed a complex issue, especially when dealing with multiple modules requiring authentication like Graph API, AzureAD, ExchangeOnline, and Azaccounts. Excluding Graph API, we attempt a quick check at the start to determine if we can execute the command or any command within the module. If this is not possible, we print a warning indicating that you must authenticate to that module before running the function. We've attempted more in-depth error handling, but encountered challenges, such as:

If you forget to connect to Connect-ExchangeOnline and then attempt to run search-UnifiedAuditLog, you will encounter the following error message:

search-UnifiedAuditLog: The term 'search-UnifiedAuditLog' is not recognized as the name of a cmdlet, function, script file, or operable program.

However, it's important to note that you may also encounter the same error if you have connected but are using an account that lacks the necessary permissions.

So, more in-depth handling of errors seems difficult to do. Therefore, we try to specify in the documentation which modules are required and give a warning when the script can't run the module.

For the Graph API, we attempt to connect with the required scopes for each function separately. We can create a connection script like with the others, but then we would need to import a lot of scopes, which might not be relevant for a user who only needs a subset of the functions. Thinking from the least privileged principle. What would be cool is to create some code that deploys an "acquisition" application in the target tenant with the required permissions and then use this application to acquire all relevant evidence. But this might also be a little overkill if you are just trying to get user info or MFA status.

On this page, we have an overview of each module and what roles/scopes are required: https://microsoft-365-extractor-suite.readthedocs.io/en/latest/installation/Prerequisites.html

It might be good to also include this information in the documentation for each step.

The error handling and checking suggestion is a good one and one we should look into. Over time, people requested new parameters and flags, so it got a little messy (my fault). I am currently testing error handling for the output directory on all functions we have. I will try to do it later for the rest of the flags as well.

For the file convention, I agree and think it would be good to add this to all files to keep it consistent.

For the fields around Graph API modules and the legacy Get-AzureAD modules, I will take a look and try to use the same order. This sounds like something logical to do, haha.

The "get-all" script is something we've been thinking about for a long time. It still feels like something we should create at some point. However, currently, new tasks are being added to the list quicker than I can process them. So, I might have to spend a weekend for this one in the future.

angry-bender added a commit to angry-bender/Microsoft-Extractor-Suite that referenced this issue Mar 19, 2024
Extensive re-write to get similar logic to Get-Ual (as per invictus-ir#57) 

This will enable acquisition of larger tenancies Sign in Logs as it splits the request a single day, as I have found large tenancies end up getting a Out of Memory Error.

Notably, this changes the syntax for $before and $after to the same as the UAL to work. and may need documenation updates
@angry-bender
Copy link
Contributor Author

Hi, thank you for the valuable feedback. This is indeed a big one, going require some time and effort. We can try to work on this and chip away at it over the longer term. Mainly, it's just me maintaining this project. So, it's sometimes hard to find time during my regular work. Since it doesn't bring in any money, it often gets less priority than other tasks. However, I will try my best to address the listed issues when I have some available time. Feel free to contribute as well, your pull requests have been great so far! :)

Regarding the points you've raised:

Authentication is indeed a complex issue, especially when dealing with multiple modules requiring authentication like Graph API, AzureAD, ExchangeOnline, and Azaccounts. Excluding Graph API, we attempt a quick check at the start to determine if we can execute the command or any command within the module. If this is not possible, we print a warning indicating that you must authenticate to that module before running the function. We've attempted more in-depth error handling, but encountered challenges, such as:

If you forget to connect to Connect-ExchangeOnline and then attempt to run search-UnifiedAuditLog, you will encounter the following error message:

search-UnifiedAuditLog: The term 'search-UnifiedAuditLog' is not recognized as the name of a cmdlet, function, script file, or operable program.

However, it's important to note that you may also encounter the same error if you have connected but are using an account that lacks the necessary permissions.

So, more in-depth handling of errors seems difficult to do. Therefore, we try to specify in the documentation which modules are required and give a warning when the script can't run the module.

For the Graph API, we attempt to connect with the required scopes for each function separately. We can create a connection script like with the others, but then we would need to import a lot of scopes, which might not be relevant for a user who only needs a subset of the functions. Thinking from the least privileged principle. What would be cool is to create some code that deploys an "acquisition" application in the target tenant with the required permissions and then use this application to acquire all relevant evidence. But this might also be a little overkill if you are just trying to get user info or MFA status.

On this page, we have an overview of each module and what roles/scopes are required: https://microsoft-365-extractor-suite.readthedocs.io/en/latest/installation/Prerequisites.html

It might be good to also include this information in the documentation for each step.

The error handling and checking suggestion is a good one and one we should look into. Over time, people requested new parameters and flags, so it got a little messy (my fault). I am currently testing error handling for the output directory on all functions we have. I will try to do it later for the rest of the flags as well.

For the file convention, I agree and think it would be good to add this to all files to keep it consistent.

For the fields around Graph API modules and the legacy Get-AzureAD modules, I will take a look and try to use the same order. This sounds like something logical to do, haha.

The "get-all" script is something we've been thinking about for a long time. It still feels like something we should create at some point. However, currently, new tasks are being added to the list quicker than I can process them. So, I might have to spend a weekend for this one in the future.

Thanks Joey,

Very similar scenario here, where I can see a need / feature requirement though, happy to help out where possible. Found one such opportunity today where a bigger tenancy crashed. But completely understand where your coming from on maintenance being a fellow DFIR practitioner.

@JoeyInvictus
Copy link
Collaborator

In our recent update, we aligned some code across the modules:

  • Refined the code for ADSignInLogsGraph, ADAuditLogsGraph, ADAuditLogs and ADSignInLogs to enhance efficiency.
  • Updated parameter names from before/after to EndDate/StartDate across the script for consistency.
  • Converted the MergeCSVOutput parameter to a switch for simplified usage.
  • The timestamp is now prefixed to every output file, ensuring consistency across all functions.

@JoeyInvictus JoeyInvictus added the enhancement New feature or request label Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants