Mandatory client-secret when creating a workspace (#1924)

* Mandatory client_secret when creating workspace * Debugging settings * azure rm version * Update templates/workspaces/base/.env.sample Co-authored-by: Marcus Robinson <marrobi@microsoft.com> * Update templates/workspaces/base/.env.sample Co-authored-by: Marcus Robinson <marrobi@microsoft.com> * Update templates/workspaces/base/terraform/variables.tf Co-authored-by: Marcus Robinson <marrobi@microsoft.com>
microsoft · May 28, 2022 · bee34e1 · bee34e1
1 parent 350d8a0
commit bee34e1
Show file tree

Hide file tree

Showing 24 changed files with 156 additions and 106 deletions.
diff --git a/api_app/_version.py b/api_app/_version.py
@@ -1 +1 @@
-__version__ = "0.3.4"
+__version__ = "0.3.5"
diff --git a/api_app/models/schemas/workspace.py b/api_app/models/schemas/workspace.py
@@ -72,7 +72,8 @@ class Config:
                 "properties": {
                     "display_name": "the workspace display name",
                     "description": "workspace description",
-                    "client_id": "9d52b04f-89cf-47b4-868a-e12be7133b36",
+                    "client_id": "<WORKSPACE_CLIENT_ID>",
+                    "client_secret": "<WORKSPACE_CLIENT_SECRET>",
                     "address_space_size": "small"
                 }
             }

diff --git a/api_app/models/schemas/workspace_template.py b/api_app/models/schemas/workspace_template.py
@@ -18,6 +18,7 @@ def get_sample_workspace_template_object(template_name: str = "tre-workspace-bas
             "display_name": Property(type="string"),
             "description": Property(type="string"),
             "client_id": Property(type="string"),
+            "client_secret": Property(type="string"),
             "address_space_size": Property(
                 type="string",
                 default="small",

diff --git a/api_app/schemas/azuread.json b/api_app/schemas/azuread.json
@@ -12,6 +12,11 @@
             "type": "string",
             "title": "Application (Client) ID",
             "description": "AAD App registration of the workspace"
-        }
+        },
+        "client_secret": {
+          "type": "string",
+          "title": "Application (Client) Secret",
+          "description": "AAD App registration of the workspace. This value will be copied into Key Vault."
+      }
     }
 }
diff --git a/api_app/services/aad_authentication.py b/api_app/services/aad_authentication.py
@@ -187,7 +187,7 @@ def _get_app_sp_graph_data(self, client_id: str) -> dict:
         return graph_data
 
     # This method is called when you create a workspace and you already have an AAD App Registration
-    # to link it to. You pass in teh client_id and go and get the extra information you need from AAD
+    # to link it to. You pass in the client_id and go and get the extra information you need from AAD
     # If the client_id is `auto_create`, then these values will be written by Terraform.
     def _get_app_auth_info(self, client_id: str) -> dict:
         graph_data = self._get_app_sp_graph_data(client_id)

diff --git a/docs/tre-admins/setup-instructions/installing-base-workspace.md b/docs/tre-admins/setup-instructions/installing-base-workspace.md
@@ -56,7 +56,8 @@ Go to `https://<azure_tre_fqdn>/api/docs` and use POST `/api/workspaces` with th
   "properties": {
     "display_name": "manual-from-swagger",
     "description": "workspace for team X",
-    "client_id": "WORKSPACE_API_CLIENT_ID",
+    "client_id":"<WORKSPACE_API_CLIENT_ID>",
+    "client_secret":"<WORKSPACE_API_CLIENT_SECRET>",
     "address_space_size": "medium"
   }
 }

diff --git a/...tre-admins/setup-instructions/installing-workspace-service-and-user-resource.md b/...tre-admins/setup-instructions/installing-workspace-service-and-user-resource.md
@@ -74,8 +74,6 @@ Now that we have published and registered both workspace service and user resour
       "properties": {
         "display_name":"Virtual Desktop",
         "description":"Create virtual desktops for running research workloads",
-        "ws_client_id":"<WORKSPACE_API_CLIENT_ID>",
-        "ws_client_secret":"<WORKSPACE_API_CLIENT_SECRET>",
         "is_exposed_externally":true,
         "guac_disable_copy":true,
         "guac_disable_paste":true

diff --git a/docs/tre-templates/workspace-services/guacamole.md b/docs/tre-templates/workspace-services/guacamole.md
@@ -18,13 +18,6 @@ Service Tags:
 
 When deploying a Guacamole service into a workspace the following properties need to be configured.
 
-### Required Properties
-
-| Property | Options | Description |
-| -------- | ------- | ----------- |
-| `ws_client_id` | Valid client ID of the Workspace App Registration. | The OpenID client ID which should be submitted to the OpenID service when necessary. This value is typically provided to you by the OpenID service when OpenID credentials are generated for your application. |
-| `ws_client_secret` | Valid client secret. |
-
 ### Optional Properties
 
 | Property | Options | Description |

diff --git a/docs/tre-templates/workspaces/base.md b/docs/tre-templates/workspaces/base.md
@@ -11,6 +11,17 @@ The base workspace template contains the following resources:
 - Network Security Group
 - App Service Plan
 
+## Workspace Configuration
+
+When deploying a workspace the following properties need to be configured.
+
+### Required Properties
+
+| Property | Options | Description |
+| -------- | ------- | ----------- |
+| `client_id` | Valid client ID of the Workspace App Registration. | The OpenID client ID which should be submitted to the OpenID service when necessary. This value is typically provided to you by the OpenID service when OpenID credentials are generated for your application. |
+| `client_secret` | Valid client secret. |
+
 ## Azure Trusted Services
 *Azure Trusted Services* are allowed to connect to both the key vault and storage account provsioned within the workspace. If this is undesirable additonal resources without this setting configured can be deployed.
 

diff --git a/e2e_tests/test_performance.py b/e2e_tests/test_performance.py
@@ -63,7 +63,8 @@ async def test_bulk_updates_to_ensure_each_resource_updated_in_series(admin_toke
                 "display_name": "E2E test guacamole service",
                 "description": "",
                 "address_space_size": "small",
-                "client_id": f"{config.TEST_WORKSPACE_APP_ID}"
+                "client_id": f"{config.TEST_WORKSPACE_APP_ID}",
+                "client_secret": f"{config.TEST_WORKSPACE_APP_SECRET}"
             }
         }
 
@@ -79,9 +80,7 @@ async def test_bulk_updates_to_ensure_each_resource_updated_in_series(admin_toke
             "templateName": "tre-service-guacamole",
             "properties": {
                 "display_name": "Workspace service test",
-                "description": "",
-                "ws_client_id": f"{config.TEST_WORKSPACE_APP_ID}",
-                "ws_client_secret": f"{config.TEST_WORKSPACE_APP_SECRET}"
+                "description": ""
             }
         }
 

diff --git a/e2e_tests/test_workspace_services.py b/e2e_tests/test_workspace_services.py
@@ -18,7 +18,8 @@ async def test_create_guacamole_service_into_base_workspace(admin_token, verify)
             "display_name": "E2E test guacamole service",
             "description": "workspace for E2E",
             "address_space_size": "small",
-            "client_id": f"{config.TEST_WORKSPACE_APP_ID}"
+            "client_id": f"{config.TEST_WORKSPACE_APP_ID}",
+            "client_secret": f"{config.TEST_WORKSPACE_APP_SECRET}"
         }
     }
 
@@ -29,9 +30,7 @@ async def test_create_guacamole_service_into_base_workspace(admin_token, verify)
         "templateName": "tre-service-guacamole",
         "properties": {
             "display_name": "Workspace service test",
-            "description": "Workspace service for E2E test",
-            "ws_client_id": f"{config.TEST_WORKSPACE_APP_ID}",
-            "ws_client_secret": f"{config.TEST_WORKSPACE_APP_SECRET}"
+            "description": "Workspace service for E2E test"
         }
     }
 
@@ -77,9 +76,7 @@ async def test_create_guacamole_service_into_aad_workspace(admin_token, workspac
         "templateName": "tre-service-guacamole",
         "properties": {
             "display_name": "Workspace service test",
-            "description": "Workspace service for E2E test",
-            "ws_client_id": f"{config.TEST_WORKSPACE_APP_ID}",
-            "ws_client_secret": f"{config.TEST_WORKSPACE_APP_SECRET}"
+            "description": "Workspace service for E2E test"
         }
     }
 

diff --git a/templates/workspace_services/guacamole/.env.sample b/templates/workspace_services/guacamole/.env.sample
@@ -1,10 +1,20 @@
 # GUID to identify the workspace service
 ID=__CHANGE_ME__
+TRE_RESOURCE_ID=__SAME_AS_ID__
 
 # GUID to identify the workspace bundle
 WORKSPACE_ID="__CHANGE_ME__"
 
 # Guacamole image tag to use (version in templates\workspace\services\guacamole\version.txt)
 GUACAMOLE_IMAGE_TAG="__CHANGE_ME__"
-WS_CLIENT_ID="__CHANGE_ME__"
-WS_CLIENT_SECRET="__CHANGE_ME__"
+
+ARM_USE_MSI=false
+GUAC_DISABLE_COPY=true
+GUAC_DISABLE_PASTE=false
+GUAC_ENABLE_DRIVE=true
+GUAC_DRIVE_NAME="transfer"
+GUAC_DRIVE_PATH="/guac-transfer"
+GUAC_DISABLE_DOWNLOAD=true
+IS_EXPOSED_EXTERNALLY=false
+image_name="guac-server"
+image_tag=""
diff --git a/templates/workspace_services/guacamole/parameters.json b/templates/workspace_services/guacamole/parameters.json
@@ -33,18 +33,6 @@
         "env": "MGMT_RESOURCE_GROUP_NAME"
       }
     },
-    {
-      "name": "ws_client_id",
-      "source": {
-        "env": "WS_CLIENT_ID"
-      }
-    },
-    {
-      "name": "ws_client_secret",
-      "source": {
-        "env": "WS_CLIENT_SECRET"
-      }
-    },
     {
       "name": "tfstate_container_name",
       "source": {

diff --git a/templates/workspace_services/guacamole/porter.yaml b/templates/workspace_services/guacamole/porter.yaml
@@ -1,6 +1,6 @@
 ---
 name: tre-service-guacamole
-version: 0.3.0
+version: 0.3.2
 description: "An Azure TRE service for Guacamole"
 registry: azuretre
 dockerfile: Dockerfile.tmpl
@@ -73,18 +73,6 @@ parameters:
     description:
       "Determines if the web app will be available over public/internet
       or private networks"
-  - name: ws_client_id
-    type: string
-    env: WS_CLIENT_ID
-    description:
-      "The WS client ID which should be submitted to the OpenID
-       service when necessary. This value is typically provided to you
-       when you create the ws application"
-  - name: ws_client_secret
-    type: string
-    env: WS_CLIENT_SECRET
-    description:
-      "The WS client secret"
   # the following are added automatically by the resource processor
   - name: id
     type: string
@@ -139,8 +127,6 @@ install:
         guac_drive_path: "{{ bundle.parameters.guac_drive_path }}"
         guac_disable_download: "{{ bundle.parameters.guac_disable_download }}"
         is_exposed_externally: "{{ bundle.parameters.is_exposed_externally }}"
-        ws_client_id: "{{ bundle.parameters.ws_client_id }}"
-        ws_client_secret: "{{ bundle.parameters.ws_client_secret }}"
         tre_resource_id: "{{ bundle.parameters.id }}"
       backendConfig:
         resource_group_name:
@@ -181,8 +167,6 @@ uninstall:
         guac_drive_path: "{{ bundle.parameters.guac_drive_path }}"
         guac_disable_download: "{{ bundle.parameters.guac_disable_download }}"
         is_exposed_externally: "{{ bundle.parameters.is_exposed_externally }}"
-        ws_client_id: "{{ bundle.parameters.ws_client_id }}"
-        ws_client_secret: "{{ bundle.parameters.ws_client_secret }}"
         tre_resource_id: "{{ bundle.parameters.id }}"
       backendConfig:
         resource_group_name:

diff --git a/templates/workspace_services/guacamole/template_schema.json b/templates/workspace_services/guacamole/template_schema.json
@@ -39,18 +39,6 @@
       "type": "boolean",
       "title": "Expose externally",
       "description": "Is the Guacamole service exposed outside of the vnet"
-    },
-    "ws_client_id": {
-      "$id": "#/properties/ws_client_id",
-      "type": "string",
-      "title": "workspace app client id",
-      "description": "The workspace app client ID which should be submitted to the OpenID service when necessary. This value is typically provided to you by the OpenID service when OpenID credentials are generated for your application."
-    },
-    "ws_client_secret": {
-      "$id": "#/properties/ws_client_secret",
-      "type": "string",
-      "title": "workspace app client secret",
-      "description": "The workspace app client secret which should be submitted to the OpenID service when necessary. This value is typically provided to you by the OpenID service when OpenID credentials are generated for your application."
     }
   }
 }
diff --git a/templates/workspace_services/guacamole/terraform/.terraform.lock.hcl b/templates/workspace_services/guacamole/terraform/.terraform.lock.hcl
diff --git a/templates/workspace_services/guacamole/terraform/firewall.tf b/templates/workspace_services/guacamole/terraform/firewall.tf
@@ -6,7 +6,9 @@ data "azurerm_firewall" "fw" {
   resource_group_name = "rg-${var.tre_id}"
 }
 
-resource "null_resource" "az_login" {
+resource "null_resource" "az_login_msi" {
+  count = var.arm_use_msi ? 1 : 0
+
   provisioner "local-exec" {
     command = "az login --identity -u '${data.azurerm_client_config.current.client_id}'"
   }
@@ -15,6 +17,17 @@ resource "null_resource" "az_login" {
   }
 }
 
+resource "null_resource" "az_login_spn" {
+  count = var.arm_use_msi ? 0 : 1
+
+  provisioner "local-exec" {
+    command = "az login --service-principal -u '${var.arm_client_id}' -p '${var.arm_client_secret}' -t '${var.arm_tenant_id}'"
+  }
+  triggers = {
+    timestamp = timestamp()
+  }
+}
+
 data "external" "rule_priorities" {
   program = ["bash", "-c", "./get_firewall_priorities.sh"]
 
@@ -24,7 +37,8 @@ data "external" "rule_priorities" {
     service_resource_name_suffix = local.service_resource_name_suffix
   }
   depends_on = [
-    null_resource.az_login
+    null_resource.az_login_msi,
+    null_resource.az_login_spn
   ]
 }
 

diff --git a/templates/workspace_services/guacamole/terraform/main.tf b/templates/workspace_services/guacamole/terraform/main.tf
@@ -39,6 +39,16 @@ data "azurerm_key_vault_secret" "aad_tenant_id" {
   key_vault_id = data.azurerm_key_vault.ws.id
 }
 
+data "azurerm_key_vault_secret" "workspace_client_id" {
+  name         = "workspace-client-id"
+  key_vault_id = data.azurerm_key_vault.ws.id
+}
+
+data "azurerm_key_vault_secret" "workspace_client_secret" {
+  name         = "workspace-client-secret"
+  key_vault_id = data.azurerm_key_vault.ws.id
+}
+
 data "azurerm_subnet" "web_apps" {
   name                 = "WebAppsSubnet"
   virtual_network_name = data.azurerm_virtual_network.ws.name

diff --git a/templates/workspace_services/guacamole/terraform/variables.tf b/templates/workspace_services/guacamole/terraform/variables.tf
@@ -16,5 +16,3 @@ variable "guac_drive_path" {}
 variable "guac_disable_download" {}
 variable "is_exposed_externally" {}
 variable "tre_resource_id" {}
-variable "ws_client_id" {}
-variable "ws_client_secret" {}
diff --git a/templates/workspace_services/guacamole/terraform/web_app.tf b/templates/workspace_services/guacamole/terraform/web_app.tf
@@ -47,11 +47,11 @@ resource "azurerm_app_service" "guacamole" {
     GUAC_DRIVE_NAME       = "${var.guac_drive_name}"
     GUAC_DRIVE_PATH       = "${var.guac_drive_path}"
     GUAC_DISABLE_DOWNLOAD = "${var.guac_disable_download}"
-    AUDIENCE              = "${var.ws_client_id}"
+    AUDIENCE              = "@Microsoft.KeyVault(SecretUri=${data.azurerm_key_vault_secret.workspace_client_id.id})"
     ISSUER                = local.issuer
 
-    OAUTH2_PROXY_CLIENT_ID       = "@Microsoft.KeyVault(SecretUri=${azurerm_key_vault_secret.workspace_client_id.id})"
-    OAUTH2_PROXY_CLIENT_SECRET   = "@Microsoft.KeyVault(SecretUri=${azurerm_key_vault_secret.workspace_client_secret.id})"
+    OAUTH2_PROXY_CLIENT_ID       = "@Microsoft.KeyVault(SecretUri=${data.azurerm_key_vault_secret.workspace_client_id.id})"
+    OAUTH2_PROXY_CLIENT_SECRET   = "@Microsoft.KeyVault(SecretUri=${data.azurerm_key_vault_secret.workspace_client_secret.id})"
     OAUTH2_PROXY_REDIRECT_URI    = "https://${local.webapp_name}.azurewebsites.net/oauth2/callback"
     OAUTH2_PROXY_EMAIL_DOMAIN    = "\"*\"" # oauth proxy will allow all email domains only when the value is "*"
     OAUTH2_PROXY_OIDC_ISSUER_URL = "https://login.microsoftonline.com/${local.aad_tenant_id}/v2.0"
@@ -80,11 +80,6 @@ resource "azurerm_app_service" "guacamole" {
     type         = "UserAssigned"
     identity_ids = [azurerm_user_assigned_identity.guacamole_id.id]
   }
-
-  depends_on = [
-    azurerm_key_vault_secret.workspace_client_id,
-    azurerm_key_vault_secret.workspace_client_secret
-  ]
 }
 
 resource "azurerm_monitor_diagnostic_setting" "guacamole" {
@@ -222,22 +217,3 @@ resource "azurerm_key_vault_access_policy" "guacamole_policy" {
 
   secret_permissions = ["Get", "List", ]
 }
-
-resource "azurerm_key_vault_secret" "workspace_client_id" {
-  name         = "workspace-client-id"
-  value        = var.ws_client_id
-  key_vault_id = data.azurerm_key_vault.ws.id
-  depends_on = [
-    azurerm_key_vault_access_policy.guacamole_policy
-  ]
-}
-
-resource "azurerm_key_vault_secret" "workspace_client_secret" {
-  name         = "workspace-client-secret"
-  value        = var.ws_client_secret
-  key_vault_id = data.azurerm_key_vault.ws.id
-  depends_on = [
-    azurerm_key_vault_access_policy.guacamole_policy
-  ]
-}
-