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

A new script to add and remove the keyvault deployment IP exception:
- devops/scripts/kv_add_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
The 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.
Delete the following scripts as they're no longer used:

devops/scripts/key_vault_list.sh
devops/scripts/set_contributor_sp_secrets.sh

Refactor the add and remove scripts into a single script

…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 67c2b2a.

♻️ 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: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

tamirkamara · 2025-01-19T07:33:06Z

CHANGELOG.md

There's a pending ask to enable the deployer to access resources such as these keyvaults over private network only.

Will this make this approach obsolete?

If not, this means we will need to do all of this in a conditional way. Right?

That would mean all TRE deployers would need to switch to private self hosted runners right? If so, yes this would be obsolete.

If we want to support both deployment patterns - deployment from GitHub hosted runners + deployment from private self hosted runners with KV set to private networking - then yes we'd need to do it conditionally (in order to prevent the KV from being fully public).

I guess it depends on whether implementing keyvaults private networking only means switching off the ability to use Github hosted runners - is that the plan?

The goal I'm referring to that resources will be accessed via private network only which means private runners.

The question for you, is weather this PR comes from a similar place but doesn't go as far yet and just limits which public IPs can access. Or, in a situation where private agents / network is done, will you still need this method of limiting public IPs

Yep when the codebase switches to deployment from private runners ONLY, then this change won't be needed anymore.

To clarify, in your deployment/usecase you would also like to use private runners?

Although are a lot of scripts to maintain...

Not sure there is another way?

Was thinking the add & remove scripts could be consolidated down to a single script, so there's only one script to call when you need KV access.

Also there's a couple of scripts I added the add/remove calls to that don't seem to be called from anywhere (they could possibly be removed?):

devops/scripts/key_vault_list.sh

devops/scripts/set_contributor_sp_secrets.sh

I think they can go. I've definitively not used either in recent times. @tamirkamara ?

If can get it into a single script might be cleaner?

OK simplified version is checked in:

Single add_kv_network_exception.sh script which does both the add & remove on exit

Deleted old scripts key_vault_list.sh & set_contributor_sp_secrets.sh

Needs a good re-test.

@marrobi @tamirkamara WDYT?

Re testing, have run the following scenarios on a local CICD build:

Deploy to empty environment.

Deploy to existing TRE.

Certificate renewal.

Destroy TRE

jonnyry · 2025-02-06T12:03:58Z

/test dee1c14

github-actions · 2025-02-06T12:04:15Z

🤖 pr-bot 🤖

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

(in response to this comment from @jonnyry)

Copilot reviewed 1 out of 11 changed files in this pull request and generated no comments.

Files not reviewed (10)

core/terraform/deploy.sh: Language not supported
core/terraform/destroy.sh: Language not supported
core/terraform/keyvault.tf: Language not supported
core/terraform/locals.tf: Language not supported
core/terraform/scripts/letsencrypt.sh: Language not supported
core/version.txt: Language not supported
devops/scripts/destroy_env_no_terraform.sh: Language not supported
devops/scripts/key_vault_list.sh: Language not supported
devops/scripts/kv_add_network_exception.sh: Language not supported
devops/scripts/set_contributor_sp_secrets.sh: Language not supported

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

jonnyry added 2 commits January 8, 2025 16:30

Remove unused null provisioner from .terraform.lock.hcl

4a1b8b8

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

d7ce398

tamirkamara reviewed Jan 19, 2025

View reviewed changes

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

e9833c4

microsoft deleted a comment from github-actions bot Jan 19, 2025

jonnyry mentioned this pull request Feb 4, 2025

Defender for Cloud Security Recommendations #4303

Open

21 tasks

jonnyry added 8 commits February 5, 2025 14:08

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

2fa2a95

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

56d91d1

Remove unused scripts: key_vault_list.sh & set_contributor_sp_secrets.sh

6251320

Refactor as per @marrobi

de97183

Update letsencrypt.sh

bd786c4

Update kv_add_network_exception.sh

cc2551e

Update kv_add_network_exception.sh

417c195

Update kv_add_network_exception.sh

dee1c14

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

67c2b2a

marrobi requested a review from Copilot February 7, 2025 19:48

Copilot AI reviewed Feb 7, 2025

View reviewed changes

marrobi mentioned this pull request Feb 7, 2025

Ensure storage rule is removed if lets encrypt process fails #472

Open

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonnyry Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonnyry Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonnyry commented Feb 6, 2025

github-actions bot commented Feb 6, 2025

Choose a reason for hiding this comment

jonnyry commented Jan 7, 2025 •

edited

Loading

github-actions bot commented Jan 7, 2025 •

edited

Loading

jonnyry commented Jan 8, 2025 •

edited

Loading

jonnyry Jan 19, 2025 •

edited

Loading

jonnyry Feb 6, 2025 •

edited

Loading