diff --git a/CHANGELOG.md b/CHANGELOG.md index bd34fe1a83..66556d35a6 100644 --- 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 index df12433297..f6b7e267c1 100644 --- 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 index 09973394f8..01191a5e83 100644 --- 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 index 9b084a6099..4b2ce7df3d 100644 --- 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 index 047f263bcb..31661bfc31 100644 --- 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 index 0edc38156d..3e6e94ce1f 100644 --- 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 index a5984d87cb..f2c3b3227a 100644 --- 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,11 +76,6 @@ 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) @@ -100,19 +83,23 @@ def get_required_permission(airlock_request: AirlockRequest) -> ContainerSasPerm 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 index 6f7f141c11..abca21c60d 100644 --- 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 index 87f72b5f03..7a1a48ce1b 100644 --- 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 index 6eeabb51bf..dbb4d3175b 100644 --- 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 index f658d0a64e..5a4bb1d414 100644 --- 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 index 45eb766c61..2586d4ed41 100644 --- 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 index f93ada3245..f6310739e0 100644 --- 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 index 4c7b50c9ff..1c646e384f 100644 --- 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 }