Use delegated key when generating SAS token in API (#2460)

* Use delegated key when generating SAS token in API * Upgrade AP version * Upgrade versions * Add to release notes * Remove logging * CR changes * CR changes * Rename account details * Fix lint * Upgrade api version. Co-authored-by: Liza Shakury <lishakur@Lizas-MacBook-Pro.local>
microsoft · Aug 16, 2022 · 3a64455 · 3a64455
1 parent d996a70
commit 3a64455
Show file tree

Hide file tree

Showing 14 changed files with 103 additions and 81 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,7 @@ ENHANCEMENTS:
 * 'CreationTime' field was added to Airlock requests ([#2432](https://github.com/microsoft/AzureTRE/issues/2432))
 * Bundles mirror Terraform plugins when built ([#2446](https://github.com/microsoft/AzureTRE/pull/2446))
 * 'Get all Airlock requests' endpoint supports filtering ([#2433](https://github.com/microsoft/AzureTRE/pull/2433)).
+* API uses user delagation key when generating SAS token for airlock requests. ([#2390](https://github.com/microsoft/AzureTRE/issues/2390))
 
 BUG FIXES:
 

diff --git a/airlock_processor/_version.py b/airlock_processor/_version.py
@@ -1 +1 @@
-__version__ = "0.4.2"
+__version__ = "0.4.3"
diff --git a/airlock_processor/shared_code/blob_operations.py b/airlock_processor/shared_code/blob_operations.py
@@ -79,7 +79,7 @@ def copy_data(source_account_name: str, destination_account_name: str, request_i
 def get_credential() -> DefaultAzureCredential:
     managed_identity = os.environ.get("MANAGED_IDENTITY_CLIENT_ID")
     if managed_identity:
-        logging.info("using the Airlock processor's managed identity to get storage management client")
+        logging.info("using the Airlock processor's managed identity to get credentials.")
     return DefaultAzureCredential(managed_identity_client_id=os.environ["MANAGED_IDENTITY_CLIENT_ID"],
                                   exclude_shared_token_cache_credential=True) if managed_identity else DefaultAzureCredential()
 

diff --git a/api_app/_version.py b/api_app/_version.py
@@ -1 +1 @@
-__version__ = "0.4.12"
+__version__ = "0.4.13"
diff --git a/api_app/api/routes/airlock.py b/api_app/api/routes/airlock.py
@@ -18,13 +18,12 @@
 from services.authentication import get_current_workspace_owner_or_researcher_user_or_airlock_manager, get_current_workspace_owner_or_researcher_user, get_current_airlock_manager_user
 
 from .airlock_resource_helpers import save_airlock_review, save_and_publish_event_airlock_request, \
-    update_status_and_publish_event_airlock_request, RequestAccountDetails, enrich_requests_with_allowed_actions, get_airlock_requests_by_user_and_workspace
+    update_status_and_publish_event_airlock_request, enrich_requests_with_allowed_actions, get_airlock_requests_by_user_and_workspace
 
-from services.airlock import get_storage_management_client, validate_user_allowed_to_access_storage_account, \
-    get_account_and_rg_by_request, get_airlock_request_container_sas_token, validate_request_status
+from services.airlock import validate_user_allowed_to_access_storage_account, \
+    get_account_by_request, get_airlock_request_container_sas_token, validate_request_status
 
 airlock_workspace_router = APIRouter(dependencies=[Depends(get_current_workspace_owner_or_researcher_user_or_airlock_manager)])
-storage_client = get_storage_management_client()
 
 
 # airlock
@@ -102,6 +101,6 @@ async def get_airlock_container_link(workspace=Depends(get_deployed_workspace_by
                                      user=Depends(get_current_workspace_owner_or_researcher_user)) -> AirlockRequestTokenInResponse:
     validate_user_allowed_to_access_storage_account(user, airlock_request)
     validate_request_status(airlock_request)
-    request_account_details: RequestAccountDetails = get_account_and_rg_by_request(airlock_request, workspace)
-    container_url = get_airlock_request_container_sas_token(storage_client, request_account_details, airlock_request)
+    account_name: str = get_account_by_request(airlock_request, workspace)
+    container_url = get_airlock_request_container_sas_token(account_name, airlock_request)
     return AirlockRequestTokenInResponse(containerUrl=container_url)
diff --git a/api_app/api/routes/airlock_resource_helpers.py b/api_app/api/routes/airlock_resource_helpers.py
@@ -18,15 +18,6 @@
 from services.authentication import get_access_service
 
 
-class RequestAccountDetails:
-    account_name: str
-    account_rg: str
-
-    def __init__(self, account_name, account_rg):
-        self.account_name = account_name
-        self.account_rg = account_rg
-
-
 async def save_and_publish_event_airlock_request(airlock_request: AirlockRequest, airlock_request_repo: AirlockRequestRepository, user: User, workspace: Workspace):
 
     # First check we have some email addresses so we can notify people.

diff --git a/api_app/services/airlock.py b/api_app/services/airlock.py
@@ -1,11 +1,9 @@
+import logging
 from datetime import datetime, timedelta
 
-from azure.mgmt.storage import StorageManagementClient
-from azure.storage.blob import generate_container_sas, ContainerSasPermissions
+from azure.storage.blob import generate_container_sas, ContainerSasPermissions, BlobServiceClient
 from fastapi import HTTPException
 from starlette import status
-
-from api.routes.airlock_resource_helpers import RequestAccountDetails
 from core import config
 from azure.identity import DefaultAzureCredential
 from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus
@@ -15,53 +13,43 @@
 from resources import strings, constants
 
 
-def get_storage_management_client():
-    token_credential = DefaultAzureCredential(managed_identity_client_id=config.MANAGED_IDENTITY_CLIENT_ID,
-                                              exclude_shared_token_cache_credential=True)
-    return StorageManagementClient(credential=token_credential, subscription_id=config.SUBSCRIPTION_ID)
+def get_credential() -> DefaultAzureCredential:
+    managed_identity = config.MANAGED_IDENTITY_CLIENT_ID
+    if managed_identity:
+        logging.info("Using managed identity credentials.")
+    return DefaultAzureCredential(managed_identity_client_id=config.MANAGED_IDENTITY_CLIENT_ID,
+                                  exclude_shared_token_cache_credential=True) if managed_identity else DefaultAzureCredential()
 
 
-def get_account_and_rg_by_request(airlock_request: AirlockRequest, workspace: Workspace) -> RequestAccountDetails:
+def get_account_by_request(airlock_request: AirlockRequest, workspace: Workspace) -> str:
     tre_id = config.TRE_ID
     short_workspace_id = workspace.id[-4:]
     if airlock_request.requestType == constants.IMPORT_TYPE:
         if airlock_request.status == AirlockRequestStatus.Draft:
-            return RequestAccountDetails(constants.STORAGE_ACCOUNT_NAME_IMPORT_EXTERNAL.format(tre_id),
-                                         constants.CORE_RESOURCE_GROUP_NAME.format(tre_id))
+            return constants.STORAGE_ACCOUNT_NAME_IMPORT_EXTERNAL.format(tre_id)
         elif airlock_request.status == AirlockRequestStatus.Submitted:
-            return RequestAccountDetails(constants.STORAGE_ACCOUNT_NAME_IMPORT_INPROGRESS.format(tre_id),
-                                         constants.CORE_RESOURCE_GROUP_NAME.format(tre_id))
+            return constants.STORAGE_ACCOUNT_NAME_IMPORT_INPROGRESS.format(tre_id)
         elif airlock_request.status == AirlockRequestStatus.InReview:
-            return RequestAccountDetails(constants.STORAGE_ACCOUNT_NAME_IMPORT_INPROGRESS.format(tre_id),
-                                         constants.CORE_RESOURCE_GROUP_NAME.format(tre_id))
+            return constants.STORAGE_ACCOUNT_NAME_IMPORT_INPROGRESS.format(tre_id)
         elif airlock_request.status == AirlockRequestStatus.Approved:
-            return RequestAccountDetails(constants.STORAGE_ACCOUNT_NAME_IMPORT_APPROVED.format(short_workspace_id),
-                                         constants.WORKSPACE_RESOURCE_GROUP_NAME.format(tre_id, short_workspace_id))
+            return constants.STORAGE_ACCOUNT_NAME_IMPORT_APPROVED.format(short_workspace_id)
         elif airlock_request.status == AirlockRequestStatus.Rejected:
-            return RequestAccountDetails(constants.STORAGE_ACCOUNT_NAME_IMPORT_REJECTED.format(tre_id),
-                                         constants.CORE_RESOURCE_GROUP_NAME.format(tre_id))
+            return constants.STORAGE_ACCOUNT_NAME_IMPORT_REJECTED.format(tre_id)
         elif airlock_request.status == AirlockRequestStatus.Blocked:
-            return RequestAccountDetails(constants.STORAGE_ACCOUNT_NAME_IMPORT_BLOCKED.format(tre_id),
-                                         constants.CORE_RESOURCE_GROUP_NAME.format(tre_id))
+            return constants.STORAGE_ACCOUNT_NAME_IMPORT_BLOCKED.format(tre_id)
     else:
         if airlock_request.status == AirlockRequestStatus.Draft:
-            return RequestAccountDetails(constants.STORAGE_ACCOUNT_NAME_EXPORT_INTERNAL.format(short_workspace_id),
-                                         constants.WORKSPACE_RESOURCE_GROUP_NAME.format(tre_id, short_workspace_id))
+            return constants.STORAGE_ACCOUNT_NAME_EXPORT_INTERNAL.format(short_workspace_id)
         elif airlock_request.status in AirlockRequestStatus.Submitted:
-            return RequestAccountDetails(constants.STORAGE_ACCOUNT_NAME_EXPORT_INPROGRESS.format(short_workspace_id),
-                                         constants.WORKSPACE_RESOURCE_GROUP_NAME.format(tre_id, short_workspace_id))
+            return constants.STORAGE_ACCOUNT_NAME_EXPORT_INPROGRESS.format(short_workspace_id)
         elif airlock_request.status == AirlockRequestStatus.InReview:
-            return RequestAccountDetails(constants.STORAGE_ACCOUNT_NAME_EXPORT_INPROGRESS.format(short_workspace_id),
-                                         constants.WORKSPACE_RESOURCE_GROUP_NAME.format(tre_id, short_workspace_id))
+            return constants.STORAGE_ACCOUNT_NAME_EXPORT_INPROGRESS.format(short_workspace_id)
         elif airlock_request.status == AirlockRequestStatus.Approved:
-            return RequestAccountDetails(constants.STORAGE_ACCOUNT_NAME_EXPORT_APPROVED.format(tre_id),
-                                         constants.CORE_RESOURCE_GROUP_NAME.format(tre_id))
+            return constants.STORAGE_ACCOUNT_NAME_EXPORT_APPROVED.format(tre_id)
         elif airlock_request.status == AirlockRequestStatus.Rejected:
-            return RequestAccountDetails(constants.STORAGE_ACCOUNT_NAME_EXPORT_REJECTED.format(short_workspace_id),
-                                         constants.WORKSPACE_RESOURCE_GROUP_NAME.format(tre_id, short_workspace_id))
+            return constants.STORAGE_ACCOUNT_NAME_EXPORT_REJECTED.format(short_workspace_id)
         elif airlock_request.status == AirlockRequestStatus.Blocked:
-            return RequestAccountDetails(constants.STORAGE_ACCOUNT_NAME_EXPORT_BLOCKED.format(short_workspace_id),
-                                         constants.WORKSPACE_RESOURCE_GROUP_NAME.format(tre_id, short_workspace_id))
+            return constants.STORAGE_ACCOUNT_NAME_EXPORT_BLOCKED.format(short_workspace_id)
 
 
 def validate_user_allowed_to_access_storage_account(user: User, airlock_request: AirlockRequest):
@@ -88,31 +76,30 @@ def validate_request_status(airlock_request: AirlockRequest):
         return
 
 
-def get_storage_account_key(storage_client: StorageManagementClient, request_account_details: RequestAccountDetails):
-    return storage_client.storage_accounts.list_keys(request_account_details.account_rg,
-                                                     request_account_details.account_name).keys[0].value
-
-
 def get_required_permission(airlock_request: AirlockRequest) -> ContainerSasPermissions:
     if airlock_request.status == AirlockRequestStatus.Draft:
         return ContainerSasPermissions(read=True, write=True, list=True, delete=True)
     else:
         return ContainerSasPermissions(read=True, list=True)
 
 
-def get_airlock_request_container_sas_token(storage_client: StorageManagementClient,
-                                            request_account_details: RequestAccountDetails,
+def get_airlock_request_container_sas_token(account_name: str,
                                             airlock_request: AirlockRequest):
-    account_key = get_storage_account_key(storage_client, request_account_details)
-    required_permission = get_required_permission(airlock_request)
+    blob_service_client = BlobServiceClient(account_url=get_account_url(account_name),
+                                            credential=get_credential())
     expiry = datetime.utcnow() + timedelta(hours=config.AIRLOCK_SAS_TOKEN_EXPIRY_PERIOD_IN_HOURS)
+    udk = blob_service_client.get_user_delegation_key(datetime.utcnow(), expiry)
+    required_permission = get_required_permission(airlock_request)
 
-    # TODO: use user delegated key  https://github.com/microsoft/AzureTRE/issues/2185
     token = generate_container_sas(container_name=airlock_request.id,
-                                   account_name=request_account_details.account_name,
-                                   account_key=account_key,
+                                   account_name=account_name,
+                                   user_delegation_key=udk,
                                    permission=required_permission,
                                    expiry=expiry)
 
     return "https://{}.blob.core.windows.net/{}?{}" \
-        .format(request_account_details.account_name, airlock_request.id, token)
+        .format(account_name, airlock_request.id, token)
+
+
+def get_account_url(account_name: str) -> str:
+    return f"https://{account_name}.blob.core.windows.net/"
diff --git a/templates/core/terraform/airlock/identity.tf b/templates/core/terraform/airlock/identity.tf
@@ -51,9 +51,9 @@ resource "azurerm_role_assignment" "airlock_blob_data_contributor" {
 
 # This might be considered redundent since we give Virtual Machine Contributor
 # at the subscription level, but best to be explicit.
-resource "azurerm_role_assignment" "api_reader_data_access" {
-  count                = length(local.api_sa_reader_data_access)
-  scope                = local.api_sa_reader_data_access[count.index]
-  role_definition_name = "Reader and Data Access"
+resource "azurerm_role_assignment" "api_sa_data_contributor" {
+  count                = length(local.api_sa_data_contributor)
+  scope                = local.api_sa_data_contributor[count.index]
+  role_definition_name = "Storage Blob Data Contributor"
   principal_id         = var.api_principal_id
 }
diff --git a/templates/core/terraform/airlock/locals.tf b/templates/core/terraform/airlock/locals.tf
@@ -48,7 +48,7 @@ locals {
     azurerm_storage_account.sa_import_blocked.id
   ]
 
-  api_sa_reader_data_access = [
+  api_sa_data_contributor = [
     azurerm_storage_account.sa_import_external.id,
     azurerm_storage_account.sa_import_in_progress.id,
     azurerm_storage_account.sa_export_approved.id

diff --git a/templates/core/terraform/airlock/storage_accounts.tf b/templates/core/terraform/airlock/storage_accounts.tf
@@ -20,6 +20,33 @@ resource "azurerm_storage_account" "sa_import_external" {
   lifecycle { ignore_changes = [tags] }
 }
 
+data "azurerm_private_dns_zone" "blobcore" {
+  name                = "privatelink.blob.core.windows.net"
+  resource_group_name = var.resource_group_name
+}
+
+resource "azurerm_private_endpoint" "stg_import_external_pe" {
+  name                = "stg-ex-import-blob-${var.tre_id}"
+  location            = var.location
+  resource_group_name = var.resource_group_name
+  subnet_id           = var.airlock_storage_subnet_id
+  tags                = var.tre_core_tags
+
+  lifecycle { ignore_changes = [tags] }
+
+  private_dns_zone_group {
+    name                 = "private-dns-zone-group-stg-export-app"
+    private_dns_zone_ids = [data.azurerm_private_dns_zone.blobcore.id]
+  }
+
+  private_service_connection {
+    name                           = "psc-stgeximport-${var.tre_id}"
+    private_connection_resource_id = azurerm_storage_account.sa_import_external.id
+    is_manual_connection           = false
+    subresource_names              = ["Blob"]
+  }
+}
+
 # 'Approved' export
 resource "azurerm_storage_account" "sa_export_approved" {
   name                     = local.export_approved_storage_name
@@ -42,6 +69,28 @@ resource "azurerm_storage_account" "sa_export_approved" {
   lifecycle { ignore_changes = [tags] }
 }
 
+resource "azurerm_private_endpoint" "stg_export_approved_pe" {
+  name                = "stg-app-export-blob-${var.tre_id}"
+  location            = var.location
+  resource_group_name = var.resource_group_name
+  subnet_id           = var.airlock_storage_subnet_id
+  tags                = var.tre_core_tags
+
+  lifecycle { ignore_changes = [tags] }
+
+  private_dns_zone_group {
+    name                 = "private-dns-zone-group-stg-export-app"
+    private_dns_zone_ids = [data.azurerm_private_dns_zone.blobcore.id]
+  }
+
+  private_service_connection {
+    name                           = "psc-stgappexport-${var.tre_id}"
+    private_connection_resource_id = azurerm_storage_account.sa_export_approved.id
+    is_manual_connection           = false
+    subresource_names              = ["Blob"]
+  }
+}
+
 # 'In-Progress' storage account
 resource "azurerm_storage_account" "sa_import_in_progress" {
   name                            = local.import_in_progress_storage_name
@@ -67,11 +116,6 @@ resource "azurerm_storage_account" "sa_import_in_progress" {
   lifecycle { ignore_changes = [tags] }
 }
 
-data "azurerm_private_dns_zone" "blobcore" {
-  name                = "privatelink.blob.core.windows.net"
-  resource_group_name = var.resource_group_name
-}
-
 resource "azurerm_private_endpoint" "stg_import_inprogress_pe" {
   name                = "stg-ip-import-blob-${var.tre_id}"
   location            = var.location

diff --git a/templates/core/version.txt b/templates/core/version.txt
@@ -1 +1 @@
-__version__ = "0.4.14"
+__version__ = "0.4.15"
diff --git a/templates/workspaces/base/porter.yaml b/templates/workspaces/base/porter.yaml
@@ -1,6 +1,6 @@
 ---
 name: tre-workspace-base
-version: 0.3.23
+version: 0.3.24
 description: "A base Azure TRE workspace"
 dockerfile: Dockerfile.tmpl
 registry: azuretre

diff --git a/templates/workspaces/base/terraform/airlock/locals.tf b/templates/workspaces/base/terraform/airlock/locals.tf
@@ -28,7 +28,7 @@ locals {
     azurerm_storage_account.sa_export_blocked.id
   ]
 
-  api_reader_data_access = [
+  api_sa_data_contributor = [
     azurerm_storage_account.sa_import_approved.id,
     azurerm_storage_account.sa_export_internal.id,
     azurerm_storage_account.sa_export_inprogress.id

diff --git a/templates/workspaces/base/terraform/airlock/storage_accounts.tf b/templates/workspaces/base/terraform/airlock/storage_accounts.tf
@@ -269,9 +269,9 @@ resource "azurerm_role_assignment" "airlock_blob_data_contributor" {
 
 # This might be considered redundent since we give Virtual Machine Contributor
 # at the subscription level, but best to be explicit.
-resource "azurerm_role_assignment" "api_reader_data_access" {
-  count                = length(local.api_reader_data_access)
-  scope                = local.api_reader_data_access[count.index]
-  role_definition_name = "Reader and Data Access"
+resource "azurerm_role_assignment" "api_sa_data_contributor" {
+  count                = length(local.api_sa_data_contributor)
+  scope                = local.api_sa_data_contributor[count.index]
+  role_definition_name = "Storage Blob Data Contributor"
   principal_id         = data.azurerm_user_assigned_identity.api_id.principal_id
 }