From 69098c7014e638918675e041a14ba1128c07885b Mon Sep 17 00:00:00 2001 From: Matt McFarland Date: Mon, 24 Jun 2024 14:42:18 -0400 Subject: [PATCH] Storage access fixes (#225) * Use AzureAD auth for terraform backend Move away from using shared key credentials for the backend auth in both CI and local dev. * Test deploy * CLI + OIDC * Debug * Remove debug --- .github/workflows/cicd.yml | 6 +-- deployment/README.md | 2 + deployment/bin/deploy | 45 +++++++------------ deployment/bin/lib | 40 +++++++++++++++++ deployment/docker-compose.yml | 4 +- deployment/terraform/resources/providers.tf | 5 +++ .../terraform/resources/storage_account.tf | 5 +++ deployment/terraform/staging/main.tf | 1 + 8 files changed, 73 insertions(+), 35 deletions(-) diff --git a/.github/workflows/cicd.yml b/.github/workflows/cicd.yml index c33e87be..ea8068ec 100644 --- a/.github/workflows/cicd.yml +++ b/.github/workflows/cicd.yml @@ -32,7 +32,7 @@ jobs: - name: Get image tag id: get_image_tag - run: + run: case "${GITHUB_REF}" in *tags*) echo "tag=${GITHUB_REF/refs\/tags\//}" >> $GITHUB_OUTPUT ; @@ -57,7 +57,7 @@ jobs: - build_and_publish steps: - uses: actions/checkout@v3 - + - name: Log in with Azure uses: azure/login@v1 with: @@ -86,4 +86,4 @@ jobs: ARM_CLIENT_ID: ${{ fromJSON(secrets.SECURE_AZURE_CREDENTIALS).clientId }} ARM_SUBSCRIPTION_ID: ${{ fromJSON(secrets.SECURE_AZURE_CREDENTIALS).subscriptionId }} ARM_TENANT_ID: ${{ fromJSON(secrets.SECURE_AZURE_CREDENTIALS).tenantId }} - ARM_USE_OIDC: true \ No newline at end of file + ARM_USE_OIDC: true diff --git a/deployment/README.md b/deployment/README.md index 6a536597..cd33bb2b 100644 --- a/deployment/README.md +++ b/deployment/README.md @@ -10,6 +10,8 @@ The logic for the deployment workflow is encapsulated in the [bin/deploy](bin/de scripts/console --deploy ``` +To have access to the remote backend terraform state, the identity (App Registration in CI, or local corp credential if local) will need to have the `Storage Blob Data Owner` role on the `pctesttfstate` storage account. + ## Manual resources ### Deployment secrets Key Vault diff --git a/deployment/bin/deploy b/deployment/bin/deploy index b84c04c6..1d689580 100755 --- a/deployment/bin/deploy +++ b/deployment/bin/deploy @@ -49,30 +49,20 @@ while [[ "$#" -gt 0 ]]; do case $1 in ;; esac done -disable_shared_access_keys() { - echo "Disabling shared access key on storage account..." - az storage account update \ - --name ${SAK_STORAGE_ACCOUNT} \ - --resource-group ${SAK_RESOURCE_GROUP} \ - --allow-shared-key-access false \ - --output none - - if [ $? -ne 0 ]; then - echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!" - echo "WARNING: Failed to turn off shared key access on the storage account." - echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!" - exit 2 - fi -} - # Always disable shared access keys on script exit trap disable_shared_access_keys EXIT ################################### # Check and configure environment # ################################### -SAK_STORAGE_ACCOUNT=pctapisstagingsa -SAK_RESOURCE_GROUP=pct-apis-westeurope-staging_rg + +# Enable shared access keys on storage accounts that must have properties read +# [storage_account]=resource_group +declare -A SAK_STORAGE_ACCOUNTS +SAK_STORAGE_ACCOUNTS=( + ["pctapisstagingsa"]="pct-apis-westeurope-staging_rg" + ["pcfilestest"]="pc-test-manual-resources" +) if [[ -z ${TERRAFORM_DIR} ]]; then echo "Must pass in TERRAFORM_DIR with -t" @@ -94,6 +84,12 @@ setup_env echo "===== Running Deploy =====" echo "IMAGE_TAG: ${IMAGE_TAG}" +if [ -z "$ARM_CLIENT_ID" ]; then + export ARM_CLIENT_ID=$(az account show --query user.name -o tsv) + echo "Using Azure CLI auth with username: ${ARM_CLIENT_ID}" +fi + + # --------------------------------------------------- if [ "${BASH_SOURCE[0]}" = "${0}" ]; then @@ -113,16 +109,7 @@ if [ "${BASH_SOURCE[0]}" = "${0}" ]; then if [[ "${SKIP_TF}" != 1 ]]; then echo "Deploying infrastructure with Terraform..." - echo "Enabling shared key access for storage account..." - # Terraform isn't able to read all resources from a storage account if shared key access is disabled - # so while we're deploying, we need to enable it. Since we haven't run TF yet, we don't have the name of the account - # so they are hardcoded here. This is a temporary workaround until this is resolved - # https://github.com/hashicorp/terraform-provider-azurerm/issues/25218 - az storage account update \ - --name ${SAK_STORAGE_ACCOUNT} \ - --resource-group ${SAK_RESOURCE_GROUP} \ - --allow-shared-key-access true \ - --output none + enable_shared_access_keys terraform init --upgrade @@ -176,7 +163,6 @@ if [ "${BASH_SOURCE[0]}" = "${0}" ]; then --wait \ --timeout 2m0s \ -f ${DEPLOY_VALUES_FILE} \ - --debug echo "================" echo "==== Tiler =====" @@ -189,7 +175,6 @@ if [ "${BASH_SOURCE[0]}" = "${0}" ]; then --wait \ --timeout 2m0s \ -f ${DEPLOY_VALUES_FILE} \ - --debug echo "==================" echo "==== Ingress =====" diff --git a/deployment/bin/lib b/deployment/bin/lib index 329e682b..5710dbce 100755 --- a/deployment/bin/lib +++ b/deployment/bin/lib @@ -131,3 +131,43 @@ function get_cidr_range() { IFS='.' read -r -a ip_parts <<< "$runnerIpAddress" echo "${ip_parts[0]}.${ip_parts[1]}.0.0/16" } + +function disable_shared_access_keys() { + echo "Disabling shared access key on storage account..." + + for SAK_STORAGE_ACCOUNT in "${!SAK_STORAGE_ACCOUNTS[@]}"; do + SAK_RESOURCE_GROUP=${SAK_STORAGE_ACCOUNTS[$SAK_STORAGE_ACCOUNT]} + + az storage account update \ + --name ${SAK_STORAGE_ACCOUNT} \ + --resource-group ${SAK_RESOURCE_GROUP} \ + --allow-shared-key-access false \ + --output none + + if [ $? -ne 0 ]; then + echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!" + echo "WARNING: Failed to turn off shared key access on the storage account." + echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!" + exit 2 + fi + done +} + +function enable_shared_access_keys() { + echo "Enabling shared key access for storage account..." + # Terraform isn't able to read all resources from a storage account if shared key access is disabled + # so while we're deploying, we need to enable it. Since we haven't run TF yet, we don't have the name of the account + # so they are hardcoded here. This is a temporary workaround until this is resolved + # https://github.com/hashicorp/terraform-provider-azurerm/issues/25218 + + for SAK_STORAGE_ACCOUNT in "${!SAK_STORAGE_ACCOUNTS[@]}"; do + SAK_RESOURCE_GROUP=${SAK_STORAGE_ACCOUNTS[$SAK_STORAGE_ACCOUNT]} + + echo " - enabling ${SAK_STORAGE_ACCOUNT} / ${SAK_RESOURCE_GROUP}" + az storage account update \ + --name ${SAK_STORAGE_ACCOUNT} \ + --resource-group ${SAK_RESOURCE_GROUP} \ + --allow-shared-key-access true \ + --output none + done +} diff --git a/deployment/docker-compose.yml b/deployment/docker-compose.yml index 1e20e1a0..da67cbbe 100644 --- a/deployment/docker-compose.yml +++ b/deployment/docker-compose.yml @@ -8,11 +8,11 @@ services: environment: - ACR_STAC_REPO=${ACR_STAC_REPO:-pccomponentstest.azurecr.io/planetary-computer-apis/stac} - ACR_TILER_REPO=${ACR_TILER_REPO:-pccomponentstest.azurecr.io/planetary-computer-apis/tiler} - - IMAGE_TAG + - IMAGE_TAG=${IMAGE_TAG:-latest} - GIT_COMMIT - ARM_SUBSCRIPTION_ID=${ARM_SUBSCRIPTION_ID:-a84a690d-585b-4c7c-80d9-851a48af5a50} - - ARM_TENANT_ID + - ARM_TENANT_ID=${ARM_TENANT_ID:-72f988bf-86f1-41af-91ab-2d7cd011db47} - ARM_CLIENT_ID - ARM_USE_OIDC - ARM_OIDC_TOKEN diff --git a/deployment/terraform/resources/providers.tf b/deployment/terraform/resources/providers.tf index 3dc2d08c..2404234d 100644 --- a/deployment/terraform/resources/providers.tf +++ b/deployment/terraform/resources/providers.tf @@ -1,6 +1,11 @@ provider "azurerm" { features {} use_oidc = true + + # This could be used instead of temporarily enabling shared key access once + # this issue is resolved. + # https://github.com/hashicorp/terraform-provider-azurerm/issues/23142 + # storage_use_azuread = true } terraform { diff --git a/deployment/terraform/resources/storage_account.tf b/deployment/terraform/resources/storage_account.tf index b14c4e6e..411270d8 100644 --- a/deployment/terraform/resources/storage_account.tf +++ b/deployment/terraform/resources/storage_account.tf @@ -37,3 +37,8 @@ resource "azurerm_storage_table" "ipexceptionlist" { name = "ipexceptionlist" storage_account_name = azurerm_storage_account.pc.name } + +resource "azurerm_storage_table" "blobstoragebannedip" { + name = "blobstoragebannedip" + storage_account_name = azurerm_storage_account.pc.name +} diff --git a/deployment/terraform/staging/main.tf b/deployment/terraform/staging/main.tf index 269a2401..75567650 100644 --- a/deployment/terraform/staging/main.tf +++ b/deployment/terraform/staging/main.tf @@ -31,6 +31,7 @@ terraform { container_name = "pc-test-api" key = "pqe-apis.tfstate" use_oidc = true + use_azuread_auth = true } }