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.

Sign up for GitHub

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

Jump to bottom

Update ScubaGear authentication documentation for additional GCC High, Defender, and SharePoint details #1557

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

Update ScubaGear authentication documentation for additional GCC High, Defender, and SharePoint details #1557

wants to merge 12 commits into from
+26 −8

Conversation

buidav
Copy link
Collaborator

@buidav buidav commented Feb 6, 2025 •
edited
Loading

🗣 Description

  • This PR updates our documentation relating to multiple small but related authentication things.
    • Adds documentation for details necessary for ScubaGear non-interactive auth in GCC High
    • 'Lowers' the necessary interactive auth role from SharePoint admin to Global Reader
    • 'Lowers' the MS Graph permission noted in SharePoint non-interactive auth to Organization.Read.All as noted from Create a JSON file to store permission scopes required for ScubaGear #1380
    • Adds an explanation for why we include permissions with bundled in write privileges to our non-interactive permissions table.
    • Also fixed some extra white space stuff. I saw from a custom extension.

💭 Motivation and context

Closes #265
Closes #1555
Closes #1556

🧪 Testing

  • Screenshots and explanations are located in the various issues being closed by this PR.
  • Testing was done by assigning SPs or creating new user accounts to authenticate into our Tests tenants.
  • Reviewers are not meant to recreate all of the experiments noted in those issues from scratch. Just check that the language added to the documentation is good and accurate.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • PR targets the correct parent branch (e.g., main or release-name) for merge.
  • Changes are limited to single related goals - eschew scope creep!
  • Changes are sized such that they do not touch excessive number of files.
  • All future TODOs are captured in issues, which are referenced in code comments.
  • These code changes follow the ScubaGear content style guide.
  • Related issues these changes resolve are linked preferably via closing keywords.
  • All relevant type-of-change labels added.
  • All relevant project fields are set.
  • All relevant repo and/or project documentation updated to reflect these changes.

✅ Pre-merge checklist

  • PR passed smoke test check.

  • Feature branch has been rebased against changes from parent branch, as needed

    Use Rebase branch button 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 Medium or larger)

✅ Post-merge checklist

  • Feature branch deleted after merge to clean up repository.
  • Verified that all checks pass on parent branch (e.g., main or release-name) after merge.

@buidav buidav self-assigned this Feb 6, 2025
@buidav buidav linked an issue Feb 6, 2025 that may be closed by this pull request
GCC High: Unable to authenticate to Defender (Connect-IPPSSession) via app-only auth (Service Principal) #265
Open
@buidav buidav added the enhancement This issue or pull request will add new or improve existing functionality label Feb 6, 2025
@buidav buidav force-pushed the 265-update-permissions-documentation branch from e56fd57 to 2b4ac2f Compare February 6, 2025 23:01
@buidav buidav added this to the Lionfish milestone Feb 6, 2025
@buidav buidav marked this pull request as ready for review February 6, 2025 23:10
@buidav buidav added the documentation This issue or pull request improves or adds to documentation label Feb 6, 2025
james-garriss
james-garriss approved these changes Feb 7, 2025
View reviewed changes
Copy link
Collaborator

@james-garriss james-garriss left a comment

Choose a reason for hiding this comment

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

Some opportunities for grammar and clarification improvements (see below), but nothing critical. Approved.

NOTE: I only reviewed the documentation. I did not attempt to run and test ScubaGear with the permission changes.

docs/prerequisites/noninteractive.md Outdated
* Associate a [certificate with a service principal](https://learn.microsoft.com/en-us/cli/azure/azure-cli-sp-tutorial-3)

> **Note**: Take note of the AppId and the name of your tenant, as these values will be required to execute ScubaGear in non-interactive mode.
The minimum permissions and roles that must be assigned to the service principal are listed in the table below.

> **Important**: Permissions that have 'write' privileges are included in the Power Platform & SharePoint application permissions. Those permissions are the minimum required by ScubaGear to be able to read admin center configuration settings. The mix of read & write privileges instead of fine-grained read-only privileges is an unfortunate access limitation of the underlying configuration setting APIs for these services. ScubaGear itself does not exercise the use of the write privileges for it's assessments.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Replace 'write' (single quotes) with "write" (double quotes).
Replace "Power Platform & SharePoint" with "Power Platform and SharePoint".
Replace "read & write" with "read and write".
Replace "it's" (in the last sentence) with "its".

You state that the "problem" here is that the APIs don't have the read-only privs we would prefer to use. I would like to see a link or two to the Microsoft documentation pages that list the (less than ideal) privs, so that the reader can see for themselves that the problem is Microsoft, not ScubaGear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added double quotes instead of single.
Added some MS Learn links to the "problems". Though I wish there was a better documentation that spells out plainly that someone with those permissions could hijack the entire service if they wanted to.

docs/prerequisites/noninteractive.md Outdated
| Microsoft Teams | | Global Reader |

> **Note** Additional details necessary for specifically GCC High non-interactive authentication are detailed in [this section](#additional-gcc-high-details) below.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove "specifically". It seems to be both unnecessary and grammatically confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

docs/prerequisites/noninteractive.md Outdated

## Additional GCC High details

This section is to note specific additional non-interactive authentication details to successfully run ScubaGear against a GCC High tenant.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This section contains additional, non-interactive authentication details that are required to successfully run ScubaGear against a GCC High tenant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rewording suggestion taken.

docs/prerequisites/noninteractive.md Outdated
Comment on lines 92 to 95
Additional details for running ScubaGear to assess Defender for Office 365 in a GCC High tenant.

`Exchange.ManageAsApp` must be added as an application permission from BOTH the `Microsoft Exchange Online Protection` and the `Office 365 Exchange Online` APIs.
This is touched upon briefly in a GCC High application manifest writer's note in this section of the [Exchange Online App Only Auth MS Learn documentation](https://learn.microsoft.com/en-us/powershell/exchange/app-only-auth-powershell-v2?view=exchange-ps#modify-the-app-manifest-to-assign-api-permissions).
Copy link
Collaborator

Choose a reason for hiding this comment

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

When running ScubaGear to assess Defender for Office 365 in a GCC High tenant, the Exchange.ManageAsApp must be added as an application permission from both the Microsoft Exchange Online Protection and the Office 365 Exchange Online APIs. This is mentioned in a GCC High application manifest writer's note in this section of the Exchange Online App Only Auth MS Learn documentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rewording suggestion taken.

docs/prerequisites/noninteractive.md Outdated
Comment on lines 99 to 101
Additional details for running ScubaGear to assess SharePoint Online in a GCC High tenant.

The `Sites.FullControl.All` application permission must be added from the GCC High unique `Office 365 SharePoint Online` API rather than the commercial unique `SharePoint` API located in commercial/government community cloud tenants.
Copy link
Collaborator

Choose a reason for hiding this comment

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

When running ScubaGear to assess SharePoint Online in a GCC High tenant, the Sites.FullControl.All application permission must be added from the GCC High-unique Office 365 SharePoint Online API rather than the commercial-unique SharePoint API located in commercial/government community cloud tenants.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rewording suggestion taken.

tkol2022
tkol2022 approved these changes Feb 7, 2025
View reviewed changes
Copy link
Collaborator

@tkol2022 tkol2022 left a comment

Choose a reason for hiding this comment

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

I made some tweaks. I wanted to point out that the name of the respective M365 application is missing from the permissions table in the noninteractive.md file, but Microsoft is adding that with their upcoming PR so we don't have to worry about that right now. I put a screenshot of their table with the new API NAME and API APPID columns for reference.

image

@buidav buidav force-pushed the 265-update-permissions-documentation branch from eaf510e to 0e69730 Compare February 7, 2025 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@james-garriss james-garriss james-garriss approved these changes

@tkol2022 tkol2022 tkol2022 approved these changes

Assignees

@buidav buidav

Labels
documentation This issue or pull request improves or adds to documentation enhancement This issue or pull request will add new or improve existing functionality
Projects
None yet
Milestone
Lionfish
3 participants
@buidav @james-garriss @tkol2022