From caf6a9c2e68f7e74684249d112e2b083f77ac3a9 Mon Sep 17 00:00:00 2001 From: Marcus Robinson Date: Wed, 14 Sep 2022 17:02:33 +0100 Subject: [PATCH] Create groups for AAD app reg roles (#2532) --- CHANGELOG.md | 1 + api_app/_version.py | 2 +- .../aad/create_application_administrator.sh | 19 ++++++-- devops/scripts/create_aad_assets.sh | 4 ++ docs/tre-admins/auth.md | 1 + .../identities/application_admin.md | 4 +- templates/core/.env.sample | 3 ++ templates/core/version.txt | 2 +- templates/workspaces/base/.env.sample | 1 + templates/workspaces/base/parameters.json | 6 +++ templates/workspaces/base/porter.yaml | 8 +++- .../workspaces/base/template_schema.json | 6 +++ .../workspaces/base/terraform/aad/aad.tf | 48 +++++++++++++++++++ .../base/terraform/aad/variables.tf | 1 + .../workspaces/base/terraform/variables.tf | 6 +++ .../workspaces/base/terraform/workspace.tf | 2 + 16 files changed, 106 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d91aed7e8..3c026b1a19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ ENHANCEMENTS: * Airlock requests contain a field with information about the files that were submitted ([#2504](https://github.com/microsoft/AzureTRE/pull/2504)) * UI - Operations and notifications stability improvements ([[#2530](https://github.com/microsoft/AzureTRE/pull/2530)) * UI - Initial implemetation of Workspace Airlock Request View ([#2512](https://github.com/microsoft/AzureTRE/pull/2512)) +* Add ability to automatically create Azure AD groups for each application role. Requires API version 0.4.30 or later ([#2532](https://github.com/microsoft/AzureTRE/pull/2532)) * Add `is_expsed_externally` option to Azure ML Workspace Service ([#2548](https://github.com/microsoft/AzureTRE/pull2548)) * Azure ML workspace service assigns Azure ML Data Scientist role to Workspace Researchers ([#2539](https://github.com/microsoft/AzureTRE/pull/2539)) * UI is deployed by default ([#2554](https://github.com/microsoft/AzureTRE/pull/2554)) diff --git a/api_app/_version.py b/api_app/_version.py index 905119c354..8400830cc1 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.4.32" +__version__ = "0.4.33" diff --git a/devops/scripts/aad/create_application_administrator.sh b/devops/scripts/aad/create_application_administrator.sh index 508bff8a6f..8f61ee8e31 100755 --- a/devops/scripts/aad/create_application_administrator.sh +++ b/devops/scripts/aad/create_application_administrator.sh @@ -110,8 +110,17 @@ fi msGraphAppId="00000003-0000-0000-c000-000000000000" msGraphObjectId=$(az ad sp show --id ${msGraphAppId} --query "id" --output tsv --only-show-errors) -applicationPermissionId=$(az ad sp show --id ${msGraphAppId} --query "appRoles[?value=='${applicationPermission}'].id" --output tsv --only-show-errors) -roleApplicationPermission=$(get_msgraph_role "${applicationPermission}") +# split permissions into array +IFS=',' read -ra applicationPermissions <<< "${applicationPermission}" + +applicationPermissionIds=() +roleApplicationPermissions=() +for permission in "${applicationPermissions[@]}"; do # access each element of array + applicationPermissionIds+=("$(az ad sp show --id "${msGraphAppId}" --query "appRoles[?value=='${permission}'].id" --output tsv --only-show-errors)") + roleApplicationPermissions+=("$(get_msgraph_role "${permission}")") +done + +printf -v roleApplicationPermissionsJson '%s,' "${roleApplicationPermissions[@]}" appDefinition=$(jq -c . << JSON { @@ -121,7 +130,7 @@ appDefinition=$(jq -c . << JSON { "resourceAppId": "${msGraphAppId}", "resourceAccess": [ - ${roleApplicationPermission} + ${roleApplicationPermissionsJson%,} ] }] } @@ -153,7 +162,9 @@ spId=$(az ad sp list --filter "appId eq '${appId}'" --query '[0].id' --output ts if [[ $grantAdminConsent -eq 1 ]]; then echo "Granting admin consent for '${appName}' app (service principal ID ${spId}) - NOTE: Directory admin privileges required for this step" wait_for_new_service_principal "${spId}" - grant_admin_consent "${spId}" "$msGraphObjectId" "${applicationPermissionId}" + for applicationPermissionId in "${applicationPermissionIds[@]}"; do # access each element of array + grant_admin_consent "${spId}" "$msGraphObjectId" "${applicationPermissionId}" + done fi echo "APPLICATION_ADMIN_CLIENT_ID=\"${appId}\"" > "devops/auth.env" diff --git a/devops/scripts/create_aad_assets.sh b/devops/scripts/create_aad_assets.sh index a7ba55f375..a5dafe9e9b 100755 --- a/devops/scripts/create_aad_assets.sh +++ b/devops/scripts/create_aad_assets.sh @@ -29,6 +29,10 @@ if [ "${AUTO_WORKSPACE_APP_REGISTRATION:-}" == true ]; then APPLICATION_PERMISSION="Application.ReadWrite.All" fi +if [ "${AUTO_WORKSPACE_GROUP_CREATION:-}" == true ]; then + APPLICATION_PERMISSION="Application.ReadWrite.All,Directory.Read.All,Group.ReadWrite.All" +fi + # Create the identity that is able to administer other applications "$DIR/aad/create_application_administrator.sh" \ --name "${TRE_ID}" \ diff --git a/docs/tre-admins/auth.md b/docs/tre-admins/auth.md index 131fba1e89..e32131933b 100644 --- a/docs/tre-admins/auth.md +++ b/docs/tre-admins/auth.md @@ -15,6 +15,7 @@ The automation utilises a `make` command, which reads a few environment variable |AAD_TENANT_ID|The tenant id of where your AAD identities will be placed. This can be different to the tenant where your Azure resources are created.| | LOCATION | Where your Azure assets will be provisioned (eg. westeurope). This is used to add a redirect URI from the Swagger UI to the API Application. |AUTO_WORKSPACE_APP_REGISTRATION| Default of `false`. Setting this to true grants the `Application.ReadWrite.All` permission to the *Application Admin* identity. This identity is used to manage other AAD applications that it owns, e.g. Workspaces. If you do not set this, the identity will have `Application.ReadWrite.OwnedBy`. Further information can be found [here](./identities/application_admin.md). +|AUTO_WORKSPACE_GROUP_CREATION| Default of `false`. Setting this to true grants the `Directory.Read.All` and `Group.ReadWrite.All` permission to the *Application Admin* identity. This identity can then create security groups aligned to each applciation role. Active Directory licencing implications need to be considered as Group assignment is a [premium feature](https://docs.microsoft.com/en-us/azure/architecture/multitenant-identity/app-roles#roles-using-azure-ad-app-roles). ## Create Authentication assets You can build all of the Identity assets by running the following at the command line diff --git a/docs/tre-admins/identities/application_admin.md b/docs/tre-admins/identities/application_admin.md index df045faa2b..07bb469f14 100644 --- a/docs/tre-admins/identities/application_admin.md +++ b/docs/tre-admins/identities/application_admin.md @@ -11,6 +11,8 @@ This application does not have any roles defined. | --- | -- | -----| --------- | | Application.ReadWrite.OwnedBy | Application | Yes | This user has `Application.ReadWrite.OwnedBy` as a minimum permission for it to function. If the tenant is managed by a customer administrator, then this user must be added to the **Owners** of every workspace that is created. This will allow TRE to manage the AAD Application. This will be a manual process for the Tenant Admin. | | Application.ReadWrite.All | Application | Yes | If the AAD Administrator has delegated AAD administrative operations to the TRE, then this user should be granted `Application.ReadWrite.All`. This will allow the user to create workspace applications and administer any applications in the tenant. There will be no need for the Tenant Admin to oversee the Tenant. | +| Directory.Read.All | Application | Yes | This permission is required to read User details from Azure Active Directory. This is requried if Azure AD groups are to be created automatically by the TRE. | +| Group.ReadWrite.All | Application | Yes | This permission is required to create and update Azure AD groups. This is requried if Azure AD groups are to be created automatically by the TRE. | '*' See the difference between [delegated and application permission](https://docs.microsoft.com/graph/auth/auth-concepts#delegated-and-application-permissions) types. See [Microsoft Graph permissions reference](https://docs.microsoft.com/graph/permissions-reference) for more details. @@ -26,7 +28,7 @@ This user is currently only used from the Porter bundles hosted on the Resource | -------- | ----------- | | `--name` | This is used to put a friendly name to the Application that can be seen in the portal. It is typical to use the name of your TRE instance. | | `--admin-consent` | If you have the appropriate permission to grant admin consent, then pass in this argument. If you do not, you will have to ask an AAD Admin to consent after you have created the identity. Consent is required for this permission. -| `--application-permission` | This should be either `Application.ReadWrite.All` or `Application.ReadWrite.OwnedBy` | +| `--application-permission` | This is a comma seperated list of the permissions that need to be assigned. For exampler `Application.ReadWrite.All,Directory.Read.All,Group.ReadWrite.All` | | `--reset-password` | Optional, default is 0. When run in a headless fashion, 1 is passed in to always reset the password. | ## Environment Variables diff --git a/templates/core/.env.sample b/templates/core/.env.sample index eb2c57a046..a485aec7c1 100644 --- a/templates/core/.env.sample +++ b/templates/core/.env.sample @@ -14,6 +14,9 @@ AAD_TENANT_ID=__CHANGE_ME__ # When this is false, the AAD Application will need creating manually. AUTO_WORKSPACE_APP_REGISTRATION=true +# Setting AUTO_WORKSPACE_GROUP_CREATION to true will create an identity with `Directory.Read.All` and `Group.ReadWrite.All` +AUTO_WORKSPACE_GROUP_CREATION=false + # If your local machine/build agent cannot get the public IP # address from https://ipecho.net/plain, then you can circumvent # this by setting this Environment variable. This blockage can diff --git a/templates/core/version.txt b/templates/core/version.txt index 9e6207df0a..1cc3baa708 100644 --- a/templates/core/version.txt +++ b/templates/core/version.txt @@ -1 +1 @@ -__version__ = "0.4.24" +__version__ = "0.4.25" diff --git a/templates/workspaces/base/.env.sample b/templates/workspaces/base/.env.sample index e7793021f5..0d895d2cc2 100644 --- a/templates/workspaces/base/.env.sample +++ b/templates/workspaces/base/.env.sample @@ -6,6 +6,7 @@ AUTH_TENANT_ID="__CHANGE_ME__" # These are passed in if Terraform will create the Workspace AAD Application REGISTER_AAD_APPLICATION=true +CREATE_AAD_GROUPS=true AUTH_CLIENT_ID="__CHANGE_ME__" AUTH_CLIENT_SECRET="__CHANGE_ME__" WORKSPACE_OWNER_OBJECT_ID="__CHANGE_ME__" diff --git a/templates/workspaces/base/parameters.json b/templates/workspaces/base/parameters.json index 42329886b4..ccaf94536d 100755 --- a/templates/workspaces/base/parameters.json +++ b/templates/workspaces/base/parameters.json @@ -64,6 +64,12 @@ "env": "REGISTER_AAD_APPLICATION" } }, + { + "name": "create_aad_groups", + "source": { + "env": "CREATE_AAD_GROUPS" + } + }, { "name": "client_id", "source": { diff --git a/templates/workspaces/base/porter.yaml b/templates/workspaces/base/porter.yaml index 6fe481c466..65d1f8a8c3 100644 --- a/templates/workspaces/base/porter.yaml +++ b/templates/workspaces/base/porter.yaml @@ -1,6 +1,6 @@ --- name: tre-workspace-base -version: 0.3.28 +version: 0.3.30 description: "A base Azure TRE workspace" dockerfile: Dockerfile.tmpl registry: azuretre @@ -59,6 +59,10 @@ parameters: type: boolean default: false description: "Whether this bundle should register the workspace in AAD" + - name: create_aad_groups + type: boolean + default: false + description: "Whether this bundle should create AAD groups for the workspace app roles" - name: workspace_owner_object_id type: string description: "The object id of the user that will be granted WorkspaceOwner after it is created." @@ -148,6 +152,7 @@ install: shared_storage_quota: "{{ bundle.parameters.shared_storage_quota }}" enable_local_debugging: "{{ bundle.parameters.enable_local_debugging }}" register_aad_application: "{{ bundle.parameters.register_aad_application }}" + create_aad_groups: "{{ bundle.parameters.create_aad_groups }}" auth_client_id: "{{ bundle.credentials.auth_client_id }}" auth_client_secret: "{{ bundle.credentials.auth_client_secret }}" auth_tenant_id: "{{ bundle.credentials.auth_tenant_id }}" @@ -241,6 +246,7 @@ uninstall: shared_storage_quota: "{{ bundle.parameters.shared_storage_quota }}" enable_local_debugging: "{{ bundle.parameters.enable_local_debugging }}" register_aad_application: "{{ bundle.parameters.register_aad_application }}" + create_aad_groups: "{{ bundle.parameters.create_aad_groups }}" auth_client_id: "{{ bundle.credentials.auth_client_id }}" auth_client_secret: "{{ bundle.credentials.auth_client_secret }}" auth_tenant_id: "{{ bundle.credentials.auth_tenant_id }}" diff --git a/templates/workspaces/base/template_schema.json b/templates/workspaces/base/template_schema.json index db8f728f5a..35a8b0db4e 100644 --- a/templates/workspaces/base/template_schema.json +++ b/templates/workspaces/base/template_schema.json @@ -65,6 +65,12 @@ "default": true, "title": "Enable Airlock", "description": "If enabled, allows imports and exports for the workspace." + }, + "create_aad_groups": { + "type": "boolean", + "title": "Create AAD Groups for each workspace role", + "description": "Create AAD Groups for the workspace roles, requires `auto_create`. If this is set to true, the workspace will create new AAD Groups.", + "default": false } }, "uiSchema": { diff --git a/templates/workspaces/base/terraform/aad/aad.tf b/templates/workspaces/base/terraform/aad/aad.tf index 0c35da6518..c18ed4b5b3 100644 --- a/templates/workspaces/base/terraform/aad/aad.tf +++ b/templates/workspaces/base/terraform/aad/aad.tf @@ -124,3 +124,51 @@ resource "azuread_app_role_assignment" "workspace_owner" { principal_object_id = var.workspace_owner_object_id resource_object_id = azuread_service_principal.workspace.object_id } + +resource "azuread_group" "workspace_owners" { + count = var.create_aad_groups ? 1 : 0 + display_name = "${var.workspace_resource_name_suffix} Workspace Owners" + owners = [var.workspace_owner_object_id] + security_enabled = true +} + +resource "azuread_group" "workspace_researchers" { + count = var.create_aad_groups ? 1 : 0 + display_name = "${var.workspace_resource_name_suffix} Workspace Researchers" + owners = [var.workspace_owner_object_id] + security_enabled = true +} + +resource "azuread_group" "workspace_airlock_managers" { + count = var.create_aad_groups ? 1 : 0 + display_name = "${var.workspace_resource_name_suffix} Airlock Managers" + owners = [var.workspace_owner_object_id] + security_enabled = true +} + +resource "azuread_group_member" "workspace_owner" { + count = var.create_aad_groups ? 1 : 0 + group_object_id = azuread_group.workspace_owners[count.index].id + member_object_id = var.workspace_owner_object_id +} + +resource "azuread_app_role_assignment" "workspace_owners_group" { + count = var.create_aad_groups ? 1 : 0 + app_role_id = azuread_service_principal.workspace.app_role_ids["WorkspaceOwner"] + principal_object_id = azuread_group.workspace_owners[count.index].id + resource_object_id = azuread_service_principal.workspace.object_id +} + +resource "azuread_app_role_assignment" "workspace_researchers_group" { + count = var.create_aad_groups ? 1 : 0 + app_role_id = azuread_service_principal.workspace.app_role_ids["WorkspaceResearcher"] + principal_object_id = azuread_group.workspace_researchers[count.index].id + resource_object_id = azuread_service_principal.workspace.object_id +} + +resource "azuread_app_role_assignment" "workspace_airlock_managers_group" { + count = var.create_aad_groups ? 1 : 0 + app_role_id = azuread_service_principal.workspace.app_role_ids["AirlockManager"] + principal_object_id = azuread_group.workspace_airlock_managers[count.index].id + resource_object_id = azuread_service_principal.workspace.object_id +} diff --git a/templates/workspaces/base/terraform/aad/variables.tf b/templates/workspaces/base/terraform/aad/variables.tf index 83a5fc217c..dd750dc3ac 100644 --- a/templates/workspaces/base/terraform/aad/variables.tf +++ b/templates/workspaces/base/terraform/aad/variables.tf @@ -5,3 +5,4 @@ variable "tre_workspace_tags" {} variable "aad_redirect_uris_b64" { type = string # list of objects like [{"name": "my uri 1", "value": "https://..."}, {}] } +variable "create_aad_groups" {} diff --git a/templates/workspaces/base/terraform/variables.tf b/templates/workspaces/base/terraform/variables.tf index c4a618cdbc..cbf26fb16e 100644 --- a/templates/workspaces/base/terraform/variables.tf +++ b/templates/workspaces/base/terraform/variables.tf @@ -47,6 +47,12 @@ variable "register_aad_application" { description = "Create an AAD application automatically for the Workspace." } +variable "create_aad_groups" { + type = bool + default = false + description = "Create AAD groups automatically for the Workspace Application Roles." +} + variable "enable_airlock" { type = bool description = "Controls the deployment of Airlock resources in the workspace." diff --git a/templates/workspaces/base/terraform/workspace.tf b/templates/workspaces/base/terraform/workspace.tf index 3594462c8c..042e6c4766 100644 --- a/templates/workspaces/base/terraform/workspace.tf +++ b/templates/workspaces/base/terraform/workspace.tf @@ -34,6 +34,8 @@ module "aad" { workspace_resource_name_suffix = local.workspace_resource_name_suffix workspace_owner_object_id = var.workspace_owner_object_id aad_redirect_uris_b64 = var.aad_redirect_uris_b64 + create_aad_groups = var.create_aad_groups + depends_on = [ azurerm_key_vault_access_policy.deployer, azurerm_key_vault_access_policy.resource_processor,