Remove storage account keys from deployment (#224)

* Remove storage account keys from table access * pcfuncs don't use keys * Deploy and tests * Contrib * Remove temp * Remove verbosity * Move settings validation to startup check
microsoft · Jun 24, 2024 · 9beb59b · 9beb59b
1 parent 3b1e9f7
commit 9beb59b
Show file tree

Hide file tree

Showing 25 changed files with 221 additions and 143 deletions.
diff --git a/deployment/README.md b/deployment/README.md
@@ -40,25 +40,27 @@ Container Registry repo where you published your local images:
 - `ACR_TILER_REPO`
 - `IMAGE_TAG`
 
-__Note:__ Remember to bring down your resources after testing with `terraform destroy`!
+**Note:** Remember to bring down your resources after testing with `terraform destroy`!
 
 ## Loading configuration data
 
 Configuration data is stored in Azure Storage Tables. Use the `pcapis` command line interface that is installed with the `pccommon` package to load data. For example:
 
+```console
+>  az login # Use an account that has "Storage Table Data Contributor" on the account
+>  pcapis load -t collection --account pctapissatyasa  --table collectionconfig --file pccommon/tests/data-files/collection_config.json
 ```
->  pcapis load -t collection --sas "${SAS_TOKEN}" --account pctapissatyasa  --table collectionconfig --file pccommon/tests/data-files/collection_config.json
-```
+
 To dump a single collection config, use:
 
-```
->  pcapis dump -t collection --sas "${SAS_TOKEN}" --account pctapissatyasa  --table collectionconfig --id naip
+```console
+>  pcapis dump -t collection  --account pctapissatyasa  --table collectionconfig --id naip
 ```
 
 For container configs, you must also specify the container account name used as the Partition Key:
 
-```
->  pcapis dump -t collection --sas "${SAS_TOKEN}" --account pctapissatyasa  --table containerconfig --id naip --container-account naipeuwest
+```console
+>  pcapis dump -t collection --account pctapissatyasa  --table containerconfig --id naip --container-account naipeuwest
 ```
 
 Using the `load` command on a single dump file for either config will update the single row.
diff --git a/deployment/bin/deploy b/deployment/bin/deploy
@@ -49,9 +49,30 @@ 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
 
 if [[ -z ${TERRAFORM_DIR} ]]; then
     echo "Must pass in TERRAFORM_DIR with -t"
@@ -91,6 +112,18 @@ 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
+
         terraform init --upgrade
 
         if [ "${PLAN_ONLY}" ]; then
@@ -142,7 +175,8 @@ if [ "${BASH_SOURCE[0]}" = "${0}" ]; then
         --kube-context "${KUBE_CONTEXT}" \
         --wait \
         --timeout 2m0s \
-        -f ${DEPLOY_VALUES_FILE}
+        -f ${DEPLOY_VALUES_FILE} \
+        --debug
 
     echo "================"
     echo "==== Tiler ====="
@@ -154,7 +188,8 @@ if [ "${BASH_SOURCE[0]}" = "${0}" ]; then
         --kube-context "${KUBE_CONTEXT}" \
         --wait \
         --timeout 2m0s \
-        -f ${DEPLOY_VALUES_FILE}
+        -f ${DEPLOY_VALUES_FILE} \
+        --debug
 
     echo "=================="
     echo "==== Ingress ====="

diff --git a/deployment/bin/kv_add_ip b/deployment/bin/kv_add_ip
@@ -32,6 +32,7 @@ if [ "${BASH_SOURCE[0]}" = "${0}" ]; then
         -g ${KEY_VAULT_RESOURCE_GROUP_NAME}  \
         -n ${KEY_VAULT_NAME} \
         --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,6 +32,7 @@ if [ "${BASH_SOURCE[0]}" = "${0}" ]; then
         -g ${KEY_VAULT_RESOURCE_GROUP_NAME}  \
         -n ${KEY_VAULT_NAME} \
         --ip-address $cidr \
-        --subscription ${ARM_SUBSCRIPTION_ID}
+        --subscription ${ARM_SUBSCRIPTION_ID} \
+        --output none
 
 fi
diff --git a/deployment/helm/deploy-values.template.yaml b/deployment/helm/deploy-values.template.yaml
@@ -42,6 +42,11 @@ stac:
     replicaCount: "{{ tf.stac_replica_count }}"
     podAnnotations:
       "pc/gitsha": "{{ env.GIT_COMMIT }}"
+    useWorkloadIdentity: true
+    serviceAccount:
+      annotations:
+        "azure.workload.identity/client-id": {{ tf.cluster_stac_identity_client_id }}
+        "azure.workload.identity/tenant-id": {{ tf.tenant_id }}
 
   appRootPath: "/stac"
   port: "80"
@@ -86,7 +91,6 @@ tiler:
 
 storage:
   account_name: "{{ tf.storage_account_name }}"
-  account_key: "{{ tf.storage_account_key }}"
   collection_config_table_name: "{{ tf.collection_config_table_name }}"
   container_config_table_name: "{{ tf.container_config_table_name }}"
   ip_exception_config_table_name: "{{ tf.ip_exception_config_table_name }}"

diff --git a/deployment/helm/published/planetary-computer-stac/templates/_helpers.tpl b/deployment/helm/published/planetary-computer-stac/templates/_helpers.tpl
@@ -42,6 +42,7 @@ app.kubernetes.io/instance: {{ .Release.Name }}
 Common labels
 */}}
 {{- define "pcstac.labels" -}}
+azure.workload.identity/use: {{ .Values.stac.deploy.useWorkloadIdentity | quote}}
 helm.sh/chart: {{ include "pcstac.chart" . }}
 {{ include "pcstac.selectorLabels" . }}
 {{- if .Chart.AppVersion }}

diff --git a/deployment/helm/published/planetary-computer-stac/templates/deployment.yaml b/deployment/helm/published/planetary-computer-stac/templates/deployment.yaml
@@ -19,7 +19,7 @@ spec:
         {{- toYaml . | nindent 8 }}
       {{- end }}
       labels:
-        {{- include "pcstac.selectorLabels" . | nindent 8 }}
+        {{- include "pcstac.labels" . | nindent 8 }}
     spec:
       {{- with .Values.stac.deploy.imagePullSecrets }}
       imagePullSecrets:
@@ -89,20 +89,14 @@ spec:
               value: "{{ .Values.stac.debug }}"
             - name: "PCAPIS_COLLECTION_CONFIG__ACCOUNT_NAME"
               value: "{{ .Values.storage.account_name }}"
-            - name: "PCAPIS_COLLECTION_CONFIG__ACCOUNT_KEY"
-              value: "{{ .Values.storage.account_key }}"
             - name: "PCAPIS_COLLECTION_CONFIG__TABLE_NAME"
               value: "{{ .Values.storage.collection_config_table_name }}"
             - name: "PCAPIS_CONTAINER_CONFIG__ACCOUNT_NAME"
               value: "{{ .Values.storage.account_name }}"
-            - name: "PCAPIS_CONTAINER_CONFIG__ACCOUNT_KEY"
-              value: "{{ .Values.storage.account_key }}"
             - name: "PCAPIS_CONTAINER_CONFIG__TABLE_NAME"
               value: "{{ .Values.storage.container_config_table_name }}"
             - name: "PCAPIS_IP_EXCEPTION_CONFIG__ACCOUNT_NAME"
               value: "{{ .Values.storage.account_name }}"
-            - name: "PCAPIS_IP_EXCEPTION_CONFIG__ACCOUNT_KEY"
-              value: "{{ .Values.storage.account_key }}"
             - name: "PCAPIS_IP_EXCEPTION_CONFIG__TABLE_NAME"
               value: "{{ .Values.storage.ip_exception_config_table_name }}"
             - name: "PCAPIS_REDIS_HOSTNAME"

diff --git a/deployment/helm/published/planetary-computer-stac/templates/serviceaccount.yaml b/deployment/helm/published/planetary-computer-stac/templates/serviceaccount.yaml
@@ -6,7 +6,7 @@ metadata:
   name: {{ include "pcstac.serviceAccountName" . }}
   labels:
     {{- include "pcstac.labels" . | nindent 4 }}
-  {{- with .Values.serviceAccount.annotations }}
+  {{- with .Values.stac.deploy.serviceAccount.annotations }}
   annotations:
     {{- toYaml . | nindent 4 }}
   {{- end }}

diff --git a/deployment/helm/published/planetary-computer-stac/values.yaml b/deployment/helm/published/planetary-computer-stac/values.yaml
@@ -56,6 +56,10 @@ stac:
     affinity: {}
     autoscaling:
       enabled: false
+    useWorkloadIdentity: false
+    serviceAccount:
+      annotations: {}
+
 
   cert:
     privateKeySecretRef: "letsencrypt-staging"

diff --git a/deployment/helm/published/planetary-computer-tiler/templates/deployment.yaml b/deployment/helm/published/planetary-computer-tiler/templates/deployment.yaml
@@ -85,20 +85,14 @@ spec:
               value: "{{ .Values.tiler.default_max_items_per_tile}}"
             - name: "PCAPIS_COLLECTION_CONFIG__ACCOUNT_NAME"
               value: "{{ .Values.storage.account_name }}"
-            - name: "PCAPIS_COLLECTION_CONFIG__ACCOUNT_KEY"
-              value: "{{ .Values.storage.account_key }}"
             - name: "PCAPIS_COLLECTION_CONFIG__TABLE_NAME"
               value: "{{ .Values.storage.collection_config_table_name }}"
             - name: "PCAPIS_CONTAINER_CONFIG__ACCOUNT_NAME"
               value: "{{ .Values.storage.account_name }}"
-            - name: "PCAPIS_CONTAINER_CONFIG__ACCOUNT_KEY"
-              value: "{{ .Values.storage.account_key }}"
             - name: "PCAPIS_CONTAINER_CONFIG__TABLE_NAME"
               value: "{{ .Values.storage.container_config_table_name }}"
             - name: "PCAPIS_IP_EXCEPTION_CONFIG__ACCOUNT_NAME"
               value: "{{ .Values.storage.account_name }}"
-            - name: "PCAPIS_IP_EXCEPTION_CONFIG__ACCOUNT_KEY"
-              value: "{{ .Values.storage.account_key }}"
             - name: "PCAPIS_IP_EXCEPTION_CONFIG__TABLE_NAME"
               value: "{{ .Values.storage.ip_exception_config_table_name }}"
             - name: "PCAPIS_TABLE_VALUE_TTL"

diff --git a/deployment/terraform/resources/aks.tf b/deployment/terraform/resources/aks.tf
@@ -26,6 +26,10 @@ resource "azurerm_kubernetes_cluster" "pc" {
     vm_size        = "Standard_DS2_v2"
     node_count     = var.aks_node_count
     vnet_subnet_id = azurerm_subnet.node_subnet.id
+
+    upgrade_settings {
+      max_surge = "10%"
+    }
   }
 
   identity {
@@ -74,9 +78,31 @@ resource "azurerm_kubernetes_cluster" "pc" {
   }
 }
 
+resource "azurerm_user_assigned_identity" "stac" {
+  name                = "id-${local.prefix}-stac"
+  location            = var.region
+  resource_group_name = azurerm_resource_group.pc.name
+}
+
+resource "azurerm_federated_identity_credential" "stac" {
+  name                = "federated-id-${local.prefix}"
+  resource_group_name = azurerm_resource_group.pc.name
+  audience            = ["api://AzureADTokenExchange"]
+  issuer              = azurerm_kubernetes_cluster.pc.oidc_issuer_url
+  subject             = "system:serviceaccount:pc:planetary-computer-stac"
+  parent_id           = azurerm_user_assigned_identity.stac.id
+  timeouts {}
+}
+
+resource "azurerm_role_assignment" "cluster-stac-identity-storage-access" {
+  scope                = azurerm_storage_account.pc.id
+  role_definition_name = "Storage Table Data Reader"
+  principal_id         = azurerm_user_assigned_identity.stac.principal_id
+}
+
 # Workload Identity for tiler access to the Azure Maps account
 resource "azurerm_user_assigned_identity" "tiler" {
-  name                = "id-${local.prefix}"
+  name                = "id-${local.prefix}-tiler"
   location            = var.region
   resource_group_name = azurerm_resource_group.pc.name
 }
@@ -98,6 +124,12 @@ resource "azurerm_role_assignment" "cluster-identity-maps-render-token" {
 
 }
 
+resource "azurerm_role_assignment" "cluster-tiler-identity-storage-access" {
+  scope                = azurerm_storage_account.pc.id
+  role_definition_name = "Storage Table Data Reader"
+  principal_id         = azurerm_user_assigned_identity.tiler.principal_id
+}
+
 # add the role to the identity the kubernetes cluster was assigned
 resource "azurerm_role_assignment" "network" {
   scope                = azurerm_resource_group.pc.id

diff --git a/deployment/terraform/resources/functions.tf b/deployment/terraform/resources/functions.tf
@@ -46,7 +46,7 @@ resource "azurerm_function_app" "pcfuncs" {
   os_type = "linux"
   version = "~4"
   site_config {
-    linux_fx_version          = "PYTHON|3.8"
+    linux_fx_version          = "PYTHON|3.9"
     use_32_bit_worker_process = false
     ftps_state                = "Disabled"
 

diff --git a/deployment/terraform/resources/output.tf b/deployment/terraform/resources/output.tf
@@ -55,6 +55,10 @@ output "cluster_tiler_identity_client_id" {
   value = azurerm_user_assigned_identity.tiler.client_id
 }
 
+output "cluster_stac_identity_client_id" {
+  value = azurerm_user_assigned_identity.stac.client_id
+}
+
 ## Ingress
 
 output "ingress_ip" {
@@ -104,10 +108,6 @@ output "storage_account_name" {
   value = azurerm_storage_account.pc.name
 }
 
-output "storage_account_key" {
-  value = azurerm_storage_account.pc.primary_access_key
-}
-
 output "collection_config_table_name" {
   value = azurerm_storage_table.collectionconfig.name
 }

diff --git a/deployment/terraform/resources/providers.tf b/deployment/terraform/resources/providers.tf
@@ -1,4 +1,4 @@
-provider azurerm {
+provider "azurerm" {
   features {}
   use_oidc = true
 }
@@ -9,9 +9,9 @@ terraform {
   required_providers {
     azurerm = {
       source  = "hashicorp/azurerm"
-      version = "3.97.1"
+      version = "3.108.0"
     }
   }
 }
 
-data "azurerm_client_config" "current" {}
+data "azurerm_client_config" "current" {}
diff --git a/deployment/terraform/resources/storage_account.tf b/deployment/terraform/resources/storage_account.tf
@@ -6,8 +6,21 @@ resource "azurerm_storage_account" "pc" {
   account_replication_type        = "LRS"
   min_tls_version                 = "TLS1_2"
   allow_nested_items_to_be_public = false
+
+  # Disabling shared access keys breaks terraform's ability to do subsequent
+  # resource fetching during terraform plan. As a result, this property is
+  # ignored and managed outside of this apply session, via the deploy script.
+  # https://github.com/hashicorp/terraform-provider-azurerm/issues/25218
+
+  # shared_access_key_enabled = false
+  lifecycle {
+    ignore_changes = [
+      shared_access_key_enabled,
+    ]
+  }
 }
 
+
 # Tables
 
 resource "azurerm_storage_table" "collectionconfig" {

diff --git a/pc-stac.dev.env b/pc-stac.dev.env
@@ -15,17 +15,14 @@ USE_API_HYDRATE=TRUE
 # Azure Storage
 PCAPIS_COLLECTION_CONFIG__ACCOUNT_URL=http://azurite:10002/devstoreaccount1
 PCAPIS_COLLECTION_CONFIG__ACCOUNT_NAME=devstoreaccount1
-PCAPIS_COLLECTION_CONFIG__ACCOUNT_KEY=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==
 PCAPIS_COLLECTION_CONFIG__TABLE_NAME=collectionconfig
 
 PCAPIS_CONTAINER_CONFIG__ACCOUNT_URL=http://azurite:10002/devstoreaccount1
 PCAPIS_CONTAINER_CONFIG__ACCOUNT_NAME=devstoreaccount1
-PCAPIS_CONTAINER_CONFIG__ACCOUNT_KEY=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==
 PCAPIS_CONTAINER_CONFIG__TABLE_NAME=containerconfig
 
 PCAPIS_IP_EXCEPTION_CONFIG__ACCOUNT_URL=http://azurite:10002/devstoreaccount1
 PCAPIS_IP_EXCEPTION_CONFIG__ACCOUNT_NAME=devstoreaccount1
-PCAPIS_IP_EXCEPTION_CONFIG__ACCOUNT_KEY=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==
 PCAPIS_IP_EXCEPTION_CONFIG__TABLE_NAME=ipexceptionlist
 
 # Disable config and stac caching in development by setting TTL to 1 second

diff --git a/pc-tiler.dev.env b/pc-tiler.dev.env
@@ -25,17 +25,14 @@ VECTORTILE_SA_BASE_URL=https://pcvectortiles.blob.core.windows.net
 # Azure Storage
 PCAPIS_COLLECTION_CONFIG__ACCOUNT_URL=http://azurite:10002/devstoreaccount1
 PCAPIS_COLLECTION_CONFIG__ACCOUNT_NAME=devstoreaccount1
-PCAPIS_COLLECTION_CONFIG__ACCOUNT_KEY=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==
 PCAPIS_COLLECTION_CONFIG__TABLE_NAME=collectionconfig
 
 PCAPIS_CONTAINER_CONFIG__ACCOUNT_URL=http://azurite:10002/devstoreaccount1
 PCAPIS_CONTAINER_CONFIG__ACCOUNT_NAME=devstoreaccount1
-PCAPIS_CONTAINER_CONFIG__ACCOUNT_KEY=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==
 PCAPIS_CONTAINER_CONFIG__TABLE_NAME=containerconfig
 
 PCAPIS_IP_EXCEPTION_CONFIG__ACCOUNT_URL=http://azurite:10002/devstoreaccount1
 PCAPIS_IP_EXCEPTION_CONFIG__ACCOUNT_NAME=devstoreaccount1
-PCAPIS_IP_EXCEPTION_CONFIG__ACCOUNT_KEY=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==
 PCAPIS_IP_EXCEPTION_CONFIG__TABLE_NAME=ipexceptionlist
 
 # Disable config and stac caching in development by setting TTL to 1 second