Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TRE core should use private endpoint to access TRE management storage account #4353 #4360

Merged

Conversation

ashis-kar91
Copy link
Collaborator

@ashis-kar91 ashis-kar91 commented Feb 11, 2025

Resolves #4353

What is being addressed

  • Access to TRE management storage account was allowed from all networks, which is a security risk. TRE core used to access the management storage account via public network.
  • TRE management storage account should use a private endpoint to restrict public network access and ensure data remains within a private network.
  • Local TRE deployments will fail by making the storage account completely private. Hence Allow public access from specific IP addresses during deployment.

How is this addressed

The changes include the addition of a private endpoint for the TRE management storage account from the resource processor subnet, updates to network rules for the storage account to deny public access, and the introduction of a new script to handle adding and removing network exceptions for the management storage account during deployments.

Security Enhancements:

  • core/terraform/resource_processor/vmss_porter/main.tf: Add a new resource azurerm_private_endpoint for the TRE management storage account to use a private endpoint. This ensures secure access to the management storage account from TRE core resource processor subnet.
  • Set default action to "Deny" and bypass to "AzureServices" for the TRE management storage account (devops/terraform/bootstrap.sh) [1] [2].
  • Add network rules to deny public access and allow only specific IPs for the TRE management storage account (devops/terraform/main.tf).

Deployment Script Improvements:

  • Add script mgmtstorage_add_network_exception.sh to handle adding and removing network exceptions for the management storage account (devops/scripts/mgmtstorage_add_network_exception.sh).
  • Update deployment scripts to source the new mgmtstorage_add_network_exception.sh script (core/terraform/deploy.sh, core/terraform/destroy.sh, devops/terraform/deploy.sh, devops/terraform/destroy.sh) [1] [2] [3] [4].

Version Updates:

  • Update version information in core/version.txt and devops/version.txt [1] [2].

Changelog update:

  • Deny public access to the TRE management storage account and add private endpoint for TRE core (CHANGELOG.md).

@github-actions github-actions bot added the external PR from an external contributor label Feb 11, 2025
@ashis-kar91 ashis-kar91 force-pushed the ashiskar/add-mgmt-blob-private-endpoint branch from c939ed2 to 13f7c81 Compare February 11, 2025 17:33
Copy link

github-actions bot commented Feb 11, 2025

Unit Test Results

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

Results for commit 7dc7822.

♻️ This comment has been updated with latest results.

@ashis-kar91 ashis-kar91 marked this pull request as draft February 11, 2025 17:46
@ashis-kar91 ashis-kar91 marked this pull request as ready for review February 11, 2025 17:48
@marrobi marrobi requested a review from jonnyry February 11, 2025 17:50
@yuvalyaron yuvalyaron self-requested a review February 11, 2025 22:14
@marrobi
Copy link
Member

marrobi commented Feb 12, 2025

@ashis-kar91 thanks, I might be confused, but when deploy say, using IP 1.2.3.4, first time, that IP gets added to the firewall for the state storage account. Next time, the agent has 6.7.8.9, how does the agent access the terraform state store?

I think the storage firewall needs to be amended using the CLI prior to the terraform commands running, similar to #4260 with the KeyVault.

@jonnyry do you concur?

Copy link
Member

@marrobi marrobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting request changes on, until are clear on #4360 (comment)

@ashis-kar91
Copy link
Collaborator Author

@ashis-kar91 thanks, I might be confused, but when deploy say, using IP 1.2.3.4, first time, that IP gets added to the firewall for the state storage account. Next time, the agent has 6.7.8.9, how does the agent access the terraform state store?

I think the storage firewall needs to be amended using the CLI prior to the terraform commands running, similar to #4260 with the KeyVault.

@jonnyry do you concur?

Sounds good to me. Working on it.

@jonnyry
Copy link
Collaborator

jonnyry commented Feb 12, 2025

@ashis-kar91 thanks, I might be confused, but when deploy say, using IP 1.2.3.4, first time, that IP gets added to the firewall for the state storage account. Next time, the agent has 6.7.8.9, how does the agent access the terraform state store?

I think the storage firewall needs to be amended using the CLI prior to the terraform commands running, similar to #4260 with the KeyVault.

@jonnyry do you concur?

Yes - since the storage account is holding tfstate, terraform will need data plane access during the plan phase. The first run should succeed, second and subsequent runs will fail - since the IP added on the previous run will not match the current GitHub runner's IP.

If GitHub runners had nice fixed IPs that would be a different matter :-)

@ashis-kar91 ashis-kar91 force-pushed the ashiskar/add-mgmt-blob-private-endpoint branch 3 times, most recently from a7984e6 to 7cb8dca Compare February 14, 2025 22:22
@ashis-kar91
Copy link
Collaborator Author

@marrobi, @jonnyry,
Made the changes to dynamically add and remove network exceptions. Also resolved all the pending comments.
Please review again.

Copy link
Member

@marrobi marrobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will let the tests check the operation.

@marrobi
Copy link
Member

marrobi commented Feb 17, 2025

/test

Copy link

🤖 pr-bot 🤖

⚠️ When using /test on external PRs, the SHA of the checked commit must be specified

(in response to this comment from @marrobi)

@marrobi
Copy link
Member

marrobi commented Feb 17, 2025

/test 1f41328

Copy link

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/13367453953 (with refid 76feaa49)

(in response to this comment from @marrobi)

@marrobi
Copy link
Member

marrobi commented Feb 17, 2025

@ashis-kar91 tests have failed on the new script - https://github.com/microsoft/AzureTRE/actions/runs/13367453953

Copy link
Collaborator

@jonnyry jonnyry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comments.

Also, do any of the other core/terraform shell scripts that access the backend tfstate container, require your add network exception script adding? E.g. migrate.sh or outputs.sh. I haven't had chance to look at the flows, but I'm not sure all of them are called within the standard deployment flow.

Copy link

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/13443111269 (with refid 76feaa49)

(in response to this comment from @marrobi)

@ashis-kar91 ashis-kar91 force-pushed the ashiskar/add-mgmt-blob-private-endpoint branch from 25e661a to 2dbc4a4 Compare February 20, 2025 19:52
@marrobi
Copy link
Member

marrobi commented Feb 20, 2025

/test-destroy-env

Copy link

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

Copy link

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

@marrobi
Copy link
Member

marrobi commented Feb 20, 2025

/test 4abe2c9

Copy link

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/13445711529 (with refid 76feaa49)

(in response to this comment from @marrobi)

Ashis Kar added 10 commits February 21, 2025 13:14
… and allow access from TRE core and local IP
…ing deployment. Also changed subnet for resource processor private endpoint
…e group and storage account names, and improve storage accessibility check logic.
- Improved IP presence check in network rules with clearer logic.
- Added error handling for storage container listing.
- Explicit mention of public network access for the storage account in bootstrap and main Terraform configurations.
…ion.sh for improved network exception handling
@ashis-kar91 ashis-kar91 force-pushed the ashiskar/add-mgmt-blob-private-endpoint branch from 63133aa to 5dc89a4 Compare February 21, 2025 13:14
@ashis-kar91
Copy link
Collaborator Author

/test 5dc89a4

Copy link

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/13457698967 (with refid 76feaa49)

(in response to this comment from @ashis-kar91)

@ashis-kar91
Copy link
Collaborator Author

/test 1cddc7c

Copy link

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/13460780708 (with refid 76feaa49)

(in response to this comment from @ashis-kar91)

@ashis-kar91
Copy link
Collaborator Author

Are we good to merge @marrobi, @jonnyry?

…f private endpoint for TRE management storage account
@marrobi
Copy link
Member

marrobi commented Feb 24, 2025

/test-force-approve

Passed: https://github.com/microsoft/AzureTRE/actions/runs/13460780708

@marrobi marrobi enabled auto-merge (squash) February 24, 2025 10:25
Copy link

🤖 pr-bot 🤖

✅ Marking tests as complete (for commit 7dc7822)

(in response to this comment from @marrobi)

@marrobi marrobi merged commit 475ea48 into microsoft:main Feb 24, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external PR from an external contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TRE core should use private endpoint to access TRE management storage account
4 participants