Updates for network isolation and account key removal (#309)

microsoft · Jul 10, 2024 · 419b963 · 419b963
1 parent 8b4d2a7
commit 419b963
Show file tree

Hide file tree

Showing 32 changed files with 402 additions and 102 deletions.
diff --git a/deployment/bin/deploy b/deployment/bin/deploy
@@ -94,6 +94,31 @@ while [[ "$#" -gt 0 ]]; do case $1 in
 # Check and configure environment #
 ###################################
 
+function on_exit() {
+    # Always disable shared access keys on script exit
+    disable_shared_access_keys
+    # Always remove IP from storage firewalls on script exit
+    remove_ip_from_storage_firewalls
+}
+
+trap on_exit EXIT
+
+# Enable shared access keys on storage accounts that must have properties read
+# [storage_account]=resource_group
+declare -A SAK_STORAGE_ACCOUNTS
+SAK_STORAGE_ACCOUNTS=(
+  ["pctasksteststaging"]="rg-pctaskstest-staging-westeurope"
+  ["stbpctasksteststaging"]="rg-pctaskstest-staging-westeurope"
+)
+
+# Add client IP to firewall for storage accounts that must have properties read
+# [storage_account]=resource_group
+declare -A FW_STORAGE_ACCOUNTS
+FW_STORAGE_ACCOUNTS=(
+    ["pctesttfstate"]="pc-test-manual-resources"
+    ["pctasksteststaging"]="rg-pctaskstest-staging-westeurope"
+)
+
 if [[ -z ${TERRAFORM_DIR} ]]; then
     echo "Must pass in TERRAFORM_DIR with -t"
     exit 1
@@ -109,6 +134,11 @@ fi
 
 setup_env
 
+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
@@ -122,11 +152,12 @@ if [ "${BASH_SOURCE[0]}" = "${0}" ]; then
     require_env "PCTASKS_TASK_KV"
     require_env "PCTASKS_TASK_KV_RESOURCE_GROUP_NAME"
 
-    #########################
-    # Add IP to KV firewall #
-    #########################
+    #######################
+    # Add IP to firewalls #
+    #######################
 
     bin/kv_add_ip
+    add_ip_to_storage_firewalls
 
     #####################
     # Deploy Terraform  #
@@ -137,6 +168,8 @@ if [ "${BASH_SOURCE[0]}" = "${0}" ]; then
 
     if [ -z "${SKIP_FETCH_TF_VARS}${SKIP_TF}" ]; then
 
+        enable_shared_access_keys
+
         if [ -f "${TERRAFORM_DIR}/values.tfvars" ]; then
             mv ${TERRAFORM_DIR}/values.tfvars ${TERRAFORM_DIR}/values.bak.tfvars
         fi
@@ -279,7 +312,6 @@ if [ "${BASH_SOURCE[0]}" = "${0}" ]; then
     pushd ${TERRAFORM_DIR}
     export ACR_NAME=$(tf_output task_acr_name)
     export SA_ACCOUNT_NAME=$(tf_output sa_account_name)
-    export SA_ACCOUNT_KEY=$(tf_output sa_account_key)
     export SA_ACCOUNT_URL=$(tf_output sa_tables_account_url)
     export FUNCTION_APP_NAME=$(tf_output function_app_name)
     export IMAGE_TAG=$(tf_output pctasks_server_image_tag)
@@ -294,7 +326,6 @@ if [ "${BASH_SOURCE[0]}" = "${0}" ]; then
         bin/setup_storage.py \
             "${ACR_NAME}" \
             "${SA_ACCOUNT_NAME}" \
-            "${SA_ACCOUNT_KEY}" \
             --url "${SA_ACCOUNT_URL}" \
             --tag "${IMAGE_TAG}"
     fi

diff --git a/deployment/bin/kv_add_ip b/deployment/bin/kv_add_ip
@@ -28,16 +28,20 @@ if [ "${BASH_SOURCE[0]}" = "${0}" ]; then
 
     cidr=$(get_cidr_range)
 
+    echo "Adding IP $cidr to ${DEPLOY_SECRETS_KV} keyvault firewall allow list..."
     az keyvault network-rule add \
         -g ${DEPLOY_SECRETS_KV_RG_NAME}  \
         -n ${DEPLOY_SECRETS_KV} \
         --ip-address $cidr \
-        --subscription ${ARM_SUBSCRIPTION_ID}
+        --subscription ${ARM_SUBSCRIPTION_ID} \
+        --output none
 
+    echo "Adding IP $cidr to ${PCTASKS_TASK_KV} keyvault firewall allow list..."
     az keyvault network-rule add \
         -g ${PCTASKS_TASK_KV_RESOURCE_GROUP_NAME} \
         -n ${PCTASKS_TASK_KV} \
         --ip-address $cidr \
-        --subscription ${ARM_SUBSCRIPTION_ID}
+        --subscription ${ARM_SUBSCRIPTION_ID} \
+        --output none
 
 fi
diff --git a/deployment/bin/kv_rmv_ip b/deployment/bin/kv_rmv_ip
@@ -32,12 +32,14 @@ if [ "${BASH_SOURCE[0]}" = "${0}" ]; then
         -g ${DEPLOY_SECRETS_KV_RG_NAME} \
         -n ${DEPLOY_SECRETS_KV} \
         --ip-address $cidr \
-        --subscription ${ARM_SUBSCRIPTION_ID}
+        --subscription ${ARM_SUBSCRIPTION_ID} \
+        --output none
 
     az keyvault network-rule remove \
         -g ${PCTASKS_TASK_KV_RESOURCE_GROUP_NAME} \
         -n ${PCTASKS_TASK_KV} \
         --ip-address $cidr \
-        --subscription ${ARM_SUBSCRIPTION_ID}
+        --subscription ${ARM_SUBSCRIPTION_ID} \
+        --output none
 
 fi
diff --git a/deployment/bin/lib b/deployment/bin/lib
@@ -145,3 +145,82 @@ 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..."
+
+    echo "${SAK_STORAGE_ACCOUNTS[@]}"
+
+    for SAK_STORAGE_ACCOUNT in "${!SAK_STORAGE_ACCOUNTS[@]}"; do
+        SAK_RESOURCE_GROUP=${SAK_STORAGE_ACCOUNTS[$SAK_STORAGE_ACCOUNT]}
+
+        echo "   - disabling ${SAK_STORAGE_ACCOUNT} / ${SAK_RESOURCE_GROUP}"
+        az storage account update \
+            --name ${SAK_STORAGE_ACCOUNT} \
+            --resource-group ${SAK_RESOURCE_GROUP} \
+            --allow-shared-key-access false \
+            --subscription ${ARM_SUBSCRIPTION_ID} \
+            --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 \
+            --subscription ${ARM_SUBSCRIPTION_ID} \
+            --output none
+    done
+
+    sleep 10
+}
+
+function add_ip_to_storage_firewalls() {
+    cidr=$(get_cidr_range)
+
+    # Also add the IP to the terraform state storage account
+    for FW_STORAGE_ACCOUNT in "${!FW_STORAGE_ACCOUNTS[@]}"; do
+        FW_RESOURCE_GROUP=${FW_STORAGE_ACCOUNTS[$FW_STORAGE_ACCOUNT]}
+        echo "Adding IP $cidr to ${FW_STORAGE_ACCOUNT} Storage firewall allow list..."
+        az storage account network-rule add \
+            -g ${FW_RESOURCE_GROUP} \
+            -n ${FW_STORAGE_ACCOUNT} \
+            --ip-address $cidr \
+            --subscription ${ARM_SUBSCRIPTION_ID} \
+            --output none
+    done
+
+}
+
+function remove_ip_from_storage_firewalls() {
+    cidr=$(get_cidr_range)
+
+    for FW_STORAGE_ACCOUNT in "${!FW_STORAGE_ACCOUNTS[@]}"; do
+        FW_RESOURCE_GROUP=${FW_STORAGE_ACCOUNTS[$FW_STORAGE_ACCOUNT]}
+        echo "Removing IP $cidr from ${FW_STORAGE_ACCOUNT} Storage firewall allow list..."
+        az storage account network-rule remove \
+            -g ${FW_RESOURCE_GROUP} \
+            -n ${FW_STORAGE_ACCOUNT} \
+            --ip-address $cidr \
+            --subscription ${ARM_SUBSCRIPTION_ID} \
+            --output none
+    done
+}
diff --git a/deployment/bin/setup_storage.py b/deployment/bin/setup_storage.py
@@ -1,10 +1,5 @@
 #!/usr/bin/env python3
 import argparse
-from datetime import datetime, timedelta
-from typing import Optional
-
-from azure.data.tables import generate_table_sas, TableSasPermissions
-from azure.core.credentials import AzureNamedKeyCredential
 
 from pctasks.core.constants import DEFAULT_IMAGE_KEY_TABLE_NAME
 from pctasks.core.models.config import ImageConfig
@@ -14,28 +9,17 @@
 def setup_storage(
     acr: str,
     account_name: str,
-    account_key: str,
-    endpoint_url: Optional[str],
+    endpoint_url: str,
     image_tag: str,
 ) -> None:
     # Setting up image key table.
     table_name = DEFAULT_IMAGE_KEY_TABLE_NAME
-    tables_cred = AzureNamedKeyCredential(name=account_name, key=account_key)
-
-    sas_token = generate_table_sas(
-        credential=tables_cred,
-        table_name=table_name,
-        start=datetime.now(),
-        expiry=datetime.utcnow() + timedelta(hours=24 * 7),
-        permission=TableSasPermissions(
-            add=True, upsert=True, read=True, write=True, update=True
-        ),
-    )
 
-    with ImageKeyEntryTable.from_sas_token(
-        account_url=endpoint_url or f"https://{account_name}.table.core.windows.net",
-        sas_token=sas_token,
+    with ImageKeyEntryTable.from_account_key(
+        account_url=endpoint_url,
+        account_name=account_name,
         table_name=table_name,
+        account_key=None,
     ) as image_key_table:
         image_key_table.set_image(
             "ingest",
@@ -53,9 +37,8 @@ def setup_storage(
     parser = argparse.ArgumentParser(description="Setup deployed PCTasks storage.")
     parser.add_argument("acr", help="The Azure Container Registry name.")
     parser.add_argument("account_name", help="Storage account name.")
-    parser.add_argument("account_key", help="Storage account key.")
     parser.add_argument("--url", help="Endpoint URL.")
     parser.add_argument("--tag", help="Image tag.")
 
     args = parser.parse_args()
-    setup_storage(args.acr, args.account_name, args.account_key, args.url, args.tag)
+    setup_storage(args.acr, args.account_name, args.url, args.tag)
diff --git a/deployment/docker-compose.yml b/deployment/docker-compose.yml
@@ -7,8 +7,8 @@ services:
       dockerfile: deployment/Dockerfile
     environment:
       # For Terraform
-      - ARM_SUBSCRIPTION_ID
-      - ARM_TENANT_ID
+      - ARM_SUBSCRIPTION_ID=${ARM_SUBSCRIPTION_ID:-a84a690d-585b-4c7c-80d9-851a48af5a50}
+      - ARM_TENANT_ID=${ARM_TENANT_ID:-72f988bf-86f1-41af-91ab-2d7cd011db47}
       - ARM_CLIENT_ID
       - ARM_USE_OIDC
       - ARM_OIDC_TOKEN

diff --git a/deployment/helm/deploy-values.template.yaml b/deployment/helm/deploy-values.template.yaml
@@ -55,14 +55,12 @@ pctasks:
     tables:
       account_url: {{ tf.sa_tables_account_url }}
       account_name: {{ tf.sa_account_name }}
-      account_key: {{ tf.sa_account_key }}
       connection_string: {{ tf.sa_connection_string }}
       image_key_table_name: "imagekeys"
 
     blob:
       account_url: {{ tf.sa_blob_account_url }}
       account_name: {{ tf.sa_account_name }}
-      account_key: {{ tf.sa_account_key }}
       log_blob_container: tasklogs
       task_io_blob_container: taskio
 

diff --git a/deployment/helm/published/pctasks-server/templates/deployment.yaml b/deployment/helm/published/pctasks-server/templates/deployment.yaml
@@ -155,8 +155,10 @@ spec:
               value: "{{ .Values.pctasks.run.tables.account_url }}"
             - name: PCTASKS_RUN__TABLES_ACCOUNT_NAME
               value: "{{ .Values.pctasks.run.tables.account_name }}"
+            {{- if .Values.pctasks.run.dev.azurite.enabled }}
             - name: PCTASKS_RUN__TABLES_ACCOUNT_KEY
               value: "{{ .Values.pctasks.run.tables.account_key }}"
+            {{- end }}
             - name: PCTASKS_RUN__IMAGE_KEY_TABLE_NAME
               value: "{{ .Values.pctasks.run.tables.image_key_table_name }}"
 
@@ -165,8 +167,10 @@ spec:
               value: "{{ .Values.pctasks.run.blob.account_url }}"
             - name: PCTASKS_RUN__BLOB_ACCOUNT_NAME
               value: "{{ .Values.pctasks.run.blob.account_name }}"
+            {{- if .Values.pctasks.run.dev.azurite.enabled }}
             - name: PCTASKS_RUN__BLOB_ACCOUNT_KEY
               value: "{{ .Values.pctasks.run.blob.account_key }}"
+            {{- end }}
 
             - name: PCTASKS_RUN__ARGO__HOST
               value: "{{ .Values.pctasks.run.argo.host }}"

diff --git a/deployment/helm/published/pctasks-server/values.yaml b/deployment/helm/published/pctasks-server/values.yaml
@@ -133,13 +133,15 @@ pctasks:
     tables:
       account_url: ""
       account_name: ""
+      # account_key is only used when pctasks.run.dev.azurite.enabled is true
       account_key: ""
       connection_string: ""
       image_key_table_name: "imagekeys"
 
     blob:
       account_url: ""
       account_name: ""
+      # account_key is only used when pctasks.run.dev.azurite.enabled is true
       account_key: ""
       log_blob_container: tasklogs
       task_io_blob_container: taskio

diff --git a/deployment/terraform/batch_pool/main.tf b/deployment/terraform/batch_pool/main.tf
@@ -17,24 +17,18 @@ resource "azurerm_batch_pool" "batch_pool" {
 
   network_configuration {
     subnet_id = var.subnet_id
-    endpoint_configuration {
-      name = "RemoteAccessRule-VM-Deny"
-      # frontend_port_range = "*"
-      frontend_port_range = "1-49999"
-      backend_port        = 22
-      protocol            = "TCP"
-      network_security_group_rules {
-        access                = "Deny"
-        priority              = 150 // Lower than 148
-        source_address_prefix = "*"
-      }
-    }
+    public_ips = []
+
   }
 
   task_scheduling_policy {
     node_fill_type = "Spread"
   }
 
+  # Simplified communication mode is compatible with the NSG policies
+  # on our vnet.
+  target_node_communication_mode = "Simplified"
+
   container_configuration {
     type = "DockerCompatible"
     container_registries {

diff --git a/deployment/terraform/batch_pool/providers.tf b/deployment/terraform/batch_pool/providers.tf
@@ -10,7 +10,7 @@ terraform {
   required_providers {
     azurerm = {
       source  = "hashicorp/azurerm"
-      version = "3.103.1"
+      version = "3.110.0"
     }
   }
 }