microsoft / AzureTRE

An accelerator to help organizations build Trusted Research Environments on Azure.
https://microsoft.github.io/AzureTRE
MIT License
185 stars 145 forks source link

Switch use of storage account keys by Terraform to use Azure AD authentication #4104

Closed marrobi closed 2 weeks ago

marrobi commented 1 month ago

Fixes issue #4103.

This pull request updates the Terraform code to switch from using storage account keys to using Azure AD authentication for operations on storage accounts. This change is necessary due to increasing policy restrictions on the use of storage account keys. The code changes include setting the storage_use_azuread flag to true in the provider block for the AzureRM provider. Additionally, the pull request includes updates to various modules and resources to enable Azure AD authentication.

I've also removed old terraform migrations that delayed the testing, and a legacy cnab-state directory.

I've also included a helper script to increment all porter bundle versions in scenarios such as this.

Finally I fixed a bug with the health services bundle with the TF lock file that prevented it building. Again was needed for testing.

I've tested this myself, but would appreciate others giving it ago on a subscription with a policy banning the use of storage keys.

This does not fix issues of VM's using account keys to use storage and will have to be handled separately.

github-actions[bot] commented 1 month ago

Unit Test Results

0 tests   0 ✅  0s ⏱️ 0 suites  0 💤 0 files    0 ❌

Results for commit a8c63712.

:recycle: This comment has been updated with latest results.

marrobi commented 1 month ago

/test-extended

github-actions[bot] commented 1 month ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11178391410 (with refid eb54370e)

(in response to this comment from @marrobi)

marrobi commented 1 month ago

@tamirkamara are you good to look at this? Also welcome ideas to fix the linting - I'm think the workflows will need updating to remove the tag check from the ./devops/ folder.

marrobi commented 1 month ago

Two issues with this PR still:

tamirkamara commented 1 month ago

/test-extended

github-actions[bot] commented 1 month ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11474790863 (with refid eb54370e)

(in response to this comment from @tamirkamara)

LizaShak commented 3 weeks ago

/test-extended

github-actions[bot] commented 3 weeks ago

Sorry, @LizaShak, only users with write access to the repo can run pr-bot commands.

LizaShak commented 2 weeks ago

/test-extended

github-actions[bot] commented 2 weeks ago

Sorry, @LizaShak, only users with write access to the repo can run pr-bot commands.

LizaShak commented 2 weeks ago

/test-extended

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11650313761 (with refid eb54370e)

(in response to this comment from @LizaShak)

LizaShak commented 2 weeks ago

/test-destroy-env

github-actions[bot] commented 2 weeks ago

Destroying PR test environment (RG: rg-treeb54370e)... (run: https://github.com/microsoft/AzureTRE/actions/runs/11651804943)

github-actions[bot] commented 2 weeks ago

PR test environment destroy complete (RG: rg-treeb54370e)

LizaShak commented 2 weeks ago

/test-extended

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11651853338 (with refid eb54370e)

(in response to this comment from @LizaShak)

LizaShak commented 2 weeks ago

Hi @marrobi ,

The current test execution fails due to:

»»» 🤖 Creating resource group and storage account...
Location    Name
----------  -------------------
eastus      ***
WARNING: A storage account with the provided name *** is found. Will continue to update the existing account.
AccessTier    AllowBlobPublicAccess    AllowCrossTenantReplication    CreationTime                      EnableHttpsTrafficOnly    Kind       Location    MinimumTlsVersion    Name             PrimaryLocation    ProvisioningState    ResourceGroup        StatusOfPrimary
------------  -----------------------  -----------------------------  --------------------------------  ------------------------  ---------  ----------  -------------------  ---------------  -----------------  -------------------  -------------------  -----------------
Hot           False                    False                          2024-10-04T10:36:53.461064+00:00  True                      StorageV2  eastus      TLS1_0               ***  eastus             Succeeded            ***  available

»»» 🔑 Granting Storage Blob Data Contributor role to the current user...
ERROR: /me request is only valid with delegated authentication flow.

Looking at the code the responsible line is:

USER_OBJECT_ID=$(az ad signed-in-user show --query id --output tsv)

I think that the sp used for the deployment needs Microsoft Graph permission of User.Read, who can help us with that?

cc @tamirkamara

marrobi commented 2 weeks ago

@Danny-Cooke-CK @sconck can you help? Thanks.

marrobi commented 2 weeks ago

Hi @marrobi ,

The current test execution fails due to:

»»» 🤖 Creating resource group and storage account...
Location    Name
----------  -------------------
eastus      ***
WARNING: A storage account with the provided name *** is found. Will continue to update the existing account.
AccessTier    AllowBlobPublicAccess    AllowCrossTenantReplication    CreationTime                      EnableHttpsTrafficOnly    Kind       Location    MinimumTlsVersion    Name             PrimaryLocation    ProvisioningState    ResourceGroup        StatusOfPrimary
------------  -----------------------  -----------------------------  --------------------------------  ------------------------  ---------  ----------  -------------------  ---------------  -----------------  -------------------  -------------------  -----------------
Hot           False                    False                          2024-10-04T10:36:53.461064+00:00  True                      StorageV2  eastus      TLS1_0               ***  eastus             Succeeded            ***  available

»»» 🔑 Granting Storage Blob Data Contributor role to the current user...
ERROR: /me request is only valid with delegated authentication flow.

Looking at the code the responsible line is:

USER_OBJECT_ID=$(az ad signed-in-user show --query id --output tsv)

I think that the sp used for the deployment needs Microsoft Graph permission of User.Read, who can help us with that?

cc @tamirkamara

@LizaShak thinking this needs a condition to get signed in user object if using logged in user as per the PR, or use the automation account - ARM_CLIENT_ID's object ID if it is set. Do you agree?

marrobi commented 2 weeks ago

/test-extended

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11661385295 (with refid eb54370e)

(in response to this comment from @marrobi)

marrobi commented 2 weeks ago

/test-extended

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11661905717 (with refid eb54370e)

(in response to this comment from @marrobi)

marrobi commented 2 weeks ago

/test-extended

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11662830499 (with refid eb54370e)

(in response to this comment from @marrobi)

yuvalyaron commented 2 weeks ago

/test-extended

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11666590288 (with refid eb54370e)

(in response to this comment from @yuvalyaron)

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11666956580 (with refid eb54370e)

(in response to this comment from @yuvalyaron)

yuvalyaron commented 2 weeks ago

/test-extended

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11666975985 (with refid eb54370e)

(in response to this comment from @yuvalyaron)

yuvalyaron commented 2 weeks ago

/test-extended

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11667092881 (with refid eb54370e)

(in response to this comment from @yuvalyaron)

yuvalyaron commented 2 weeks ago

/test-extended

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11681571729 (with refid eb54370e)

(in response to this comment from @yuvalyaron)

yuvalyaron commented 2 weeks ago

/test-extended

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11686037125 (with refid eb54370e)

(in response to this comment from @yuvalyaron)

marrobi commented 2 weeks ago

@yuvalyaron looks like these files needs the env vars so the MSI is used: templates/workspace_services/guacamole/user_resources/guacamole-azure-windowsvm/delete_vm_extensions.sh templates/workspace_services/guacamole/user_resources/guacamole-azure-linuxvm/delete_vm_extensions.sh

yuvalyaron commented 2 weeks ago

/test-extended

github-actions[bot] commented 2 weeks ago

:robot: pr-bot :robot:

:runner: Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/11701772694 (with refid eb54370e)

(in response to this comment from @yuvalyaron)