Core key vault firewall should not be set to "Allow public access from all networks" #4260

jonnyry · 2025-01-07T21:49:04Z

Resolves #4250

What is being addressed

Changes the core key vault firewall from Allow public access from all networks to Allow public access from specific virtual networks and IP addresses
Adds an IP exception to the key vault firewall for the deployment machine's internet IP (or the PUBLIC_DEPLOYMENT_IP_ADDRESS variable if set) during deployment
Removes the IP exception at the end of deployment (whether deployment succeeds or fails)

How is this addressed

Two new scripts add and remove the keyvault deployment IP exception:
- devops/scripts/kv_add_network_exception.sh
- devops/scripts/kv_remove_network_exception.sh
They are called from the following scenarios in order to provider access to KV:
- core/terraform/deploy.sh
- core/terraform/scripts/letsencrypt.sh
- devops/scripts/destroy_env_no_terraform.sh
- core/terraform/destroy.sh
- devops/scripts/key_vault_list.sh
- devops/scripts/set_contributor_sp_secrets.sh
The remove script uses a bash trap so that it runs regardless of whether the preceeding code fails or not, to ensure the IP exception is removed

A bug in azurerm provider was encountered which required the use of a terraform provisioner:

A create provisioner on azurerm_key_vault was required to work around an azurerm provider bug which means if a key vault is being re-created (it was previously soft deleted), the network acls are not updated. This can be removed when the bug is fixed, or a different workaround found.

Updates since inital commit (as discussed with @marrobi):

Remove use of tags and null provisioner to add tag.

…m all networks" microsoft#4250

github-actions · 2025-01-07T21:49:45Z

Unit Test Results

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

Results for commit 4a1b8b8.

♻️ This comment has been updated with latest results.

jonnyry · 2025-01-07T22:24:37Z

/test 8af920d

github-actions · 2025-01-07T22:24:54Z

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/12660338621 (with refid 26f9d939)

(in response to this comment from @jonnyry)

jonnyry · 2025-01-07T23:37:17Z

/test-extended 8af920d

github-actions · 2025-01-07T23:37:34Z

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/12661150197 (with refid 26f9d939)

(in response to this comment from @jonnyry)

marrobi · 2025-01-08T08:25:47Z

core/terraform/keyvault.tf

+#
+resource "null_resource" "add_deployment_tag" {
+  triggers = {
+    always_run = timestamp()


Why does this always need to run? Once it's added once, it shouldn't get removed?

The intention was so if the tag is removed in Azure, it will always be readded.

However as discussed, have removed the use of tags altogether, so the provisioner has been removed.

marrobi · 2025-01-08T08:28:06Z

core/terraform/scripts/letsencrypt.sh

Can the Storage Account rules in this script be handles the same way?

Yes certainly - planning to have a look at storage accounts after this.

jonnyry · 2025-01-08T11:02:04Z

/destroy-test-env

github-actions · 2025-01-08T11:02:18Z

🤖 pr-bot 🤖

/destroy-test-env is not recognised as a valid command.

You can use the following commands:
    /test - build, deploy and run smoke tests on a PR
    /test-extended - build, deploy and run smoke & extended tests on a PR
    /test-extended-aad - build, deploy and run smoke & extended AAD tests on a PR
    /test-shared-services - test the deployment of shared services on a PR build
    /test-force-approve - force approval of the PR tests (i.e. skip the deployment checks)
    /test-destroy-env - delete the validation environment for a PR (e.g. to enable testing a deployment from a clean start after previous tests)
    /help - show this help

(in response to this comment from @jonnyry)

jonnyry · 2025-01-08T11:02:28Z

/test-destroy-env

github-actions · 2025-01-08T11:03:15Z

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

jonnyry · 2025-01-08T11:23:57Z

/test 2970a5d

github-actions · 2025-01-08T11:24:13Z

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/12669597448 (with refid 26f9d939)

(in response to this comment from @jonnyry)

jonnyry · 2025-01-08T12:11:22Z

/test 272589f

github-actions · 2025-01-08T12:11:35Z

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/12670289419 (with refid 26f9d939)

(in response to this comment from @jonnyry)

jonnyry · 2025-01-08T12:15:04Z

/test bf9fd32

github-actions · 2025-01-08T12:15:21Z

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/12670349633 (with refid 26f9d939)

(in response to this comment from @jonnyry)

jonnyry · 2025-01-08T12:18:58Z

/test-destroy-env

github-actions · 2025-01-08T12:19:42Z

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

github-actions · 2025-01-08T12:46:56Z

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

jonnyry · 2025-01-08T13:04:41Z

/test dcb0b8f

github-actions · 2025-01-08T13:04:55Z

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/12671159667 (with refid 26f9d939)

(in response to this comment from @jonnyry)

jonnyry · 2025-01-08T13:45:19Z

/test dcb0b8f

github-actions · 2025-01-08T13:45:34Z

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/12671848713 (with refid 26f9d939)

(in response to this comment from @jonnyry)

tamirkamara · 2025-01-08T13:51:53Z

CHANGELOG.md

I don't have time to go over this the next few days and guess @marrobi is the same. Just wanted to point out we now have 2 vaults being used from the deployer point of view.

When CMK is enabled another vault is created in the mgmt resource group

jonnyry · 2025-01-08T14:33:34Z

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/12671848713 (with refid 26f9d939)

(in response to this comment from @jonnyry)

Notes on test run starting with an empty environment:

KV exception added here:

https://github.com/microsoft/AzureTRE/actions/runs/12671848713/job/35314921879#step:3:432

Adding deployment network exception to key vault kv-***...
 Core resource group rg-*** not found

KV exception removed here:

https://github.com/microsoft/AzureTRE/actions/runs/12671848713/job/35314921879#step:3:8259

Removing deployment network exception to key vault kv-***...
 Deployment network exception removed

jonnyry · 2025-01-08T15:52:45Z

/test 135be76

github-actions · 2025-01-08T15:53:00Z

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/12674163834 (with refid 26f9d939)

(in response to this comment from @jonnyry)

jonnyry · 2025-01-08T16:26:13Z

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/12674163834 (with refid 26f9d939)

(in response to this comment from @jonnyry)

Notes on test run starting with an existing TRE:

KV exception added here:

https://github.com/microsoft/AzureTRE/actions/runs/12674163834/job/35322577601#step:3:456

 Adding deployment network exception to key vault kv-***...
 Keyvault kv-*** is now accessible

KV exception removed here:

 Removing deployment network exception to key vault kv-***...
 Deployment network exception removed

https://github.com/microsoft/AzureTRE/actions/runs/12674163834/job/35322577601#step:3:1181

Core key vault firewall should not be set to "Allow public access fro…

e0753da

…m all networks" microsoft#4250

jonnyry added 5 commits January 7, 2025 21:51

Linting

e33f235

Update core version

c9af674

Linting

9f1ef68

Linting

1fefc9f

Linting

8af920d

marrobi reviewed Jan 8, 2025

View reviewed changes

jonnyry added 4 commits January 8, 2025 10:41

Simplified: Remove use of azure tags, make specific to key vault

8c24844

Merge branch 'main' into jr/upstream-main/93-close-keyvault-firewall

357891f

Update core version

f6ed85a

Linting

dcb0b8f

jonnyry force-pushed the jr/upstream-main/93-close-keyvault-firewall branch from 2970a5d to dcb0b8f Compare January 8, 2025 12:00

jonnyry force-pushed the jr/upstream-main/93-close-keyvault-firewall branch from bf9fd32 to dcb0b8f Compare January 8, 2025 12:25

jonnyry requested a review from tamirkamara January 8, 2025 13:04

tamirkamara reviewed Jan 8, 2025

View reviewed changes

Update to deal with scenario where TRE_ID is not available

135be76

Remove unused null provisioner from .terraform.lock.hcl

4a1b8b8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core key vault firewall should not be set to "Allow public access from all networks" #4260

Core key vault firewall should not be set to "Allow public access from all networks" #4260

jonnyry commented Jan 7, 2025 •

edited

Loading

github-actions bot commented Jan 7, 2025 •

edited

Loading

jonnyry commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

jonnyry commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

marrobi Jan 8, 2025

jonnyry Jan 8, 2025

marrobi Jan 8, 2025

jonnyry Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

tamirkamara Jan 8, 2025

tamirkamara Jan 8, 2025

jonnyry commented Jan 8, 2025 •

edited

Loading

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

Core key vault firewall should not be set to "Allow public access from all networks" #4260

Are you sure you want to change the base?

Core key vault firewall should not be set to "Allow public access from all networks" #4260

Conversation

jonnyry commented Jan 7, 2025 • edited Loading

Resolves #4250

What is being addressed

How is this addressed

github-actions bot commented Jan 7, 2025 • edited Loading

Unit Test Results

jonnyry commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

jonnyry commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

marrobi Jan 8, 2025

Choose a reason for hiding this comment

jonnyry Jan 8, 2025

Choose a reason for hiding this comment

marrobi Jan 8, 2025

Choose a reason for hiding this comment

jonnyry Jan 8, 2025

Choose a reason for hiding this comment

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

tamirkamara Jan 8, 2025

Choose a reason for hiding this comment

tamirkamara Jan 8, 2025

Choose a reason for hiding this comment

jonnyry commented Jan 8, 2025 • edited Loading

jonnyry commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

jonnyry commented Jan 8, 2025

jonnyry commented Jan 7, 2025 •

edited

Loading

github-actions bot commented Jan 7, 2025 •

edited

Loading

jonnyry commented Jan 8, 2025 •

edited

Loading