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

Modifying Firewall rules to provide Internet Access to T0/T1 #2327

Open
wants to merge 38 commits into
base: develop
Choose a base branch
from

Conversation

cptanalatriste
Copy link
Contributor

@cptanalatriste cptanalatriste commented Dec 3, 2024

✅ Checklist

  • You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
  • You are targeting the appropriate branch. If you're not certain which one this is, it should be develop.
  • Your branch is up-to-date with the target branch (it probably was when you started, but it may have changed since then).

🚦 Depends on

⤴️ Summary

The proposed approach adds an extra field to the SRE config file (allow_workspace_internet ) and based on its value does the following: 1) If false , business as usual, 2) if true , we remove all the firewall's application rules, we add a network rule allowing connections to the internet, and remove user_rules DNS server configuration.

🌂 Related issues

Closes #2283

🔬 Tests

@cptanalatriste cptanalatriste requested a review from a team as a code owner December 3, 2024 09:09
@jemrobinson jemrobinson marked this pull request as draft December 3, 2024 09:12
Copy link
Contributor

@craddm craddm left a comment

Choose a reason for hiding this comment

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

I can deploy an SRE, but can't actually connect to anything. At the moment, literally only traffic from workspaces is allowed. None of the container services can connect to the internet, so a user can't get to the remote desktop gateway, for example I misdescribed that a bit. Guacamole can't talk to the microsoft Auth servers over the internet, so it can't properly log you in.

@craddm
Copy link
Contributor

craddm commented Dec 11, 2024

Have tested a fresh deployment with internet access enabled, and can confirm it works!

Copy link

github-actions bot commented Dec 11, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  data_safe_haven/commands
  sre.py 57-65
  data_safe_haven/config
  config_sections.py
  sre_config.py
  data_safe_haven/infrastructure/common
  transformations.py
  data_safe_haven/infrastructure/components/wrapped
  log_analytics_workspace.py
  data_safe_haven/infrastructure/programs/sre
  dns_server.py 39
  firewall.py
  monitoring.py
Project Total  

This report was generated by python-coverage-comment-action

@cptanalatriste cptanalatriste marked this pull request as ready for review December 11, 2024 17:28
@cptanalatriste cptanalatriste changed the title [WIP] Modifying Firewall rules to provide Internet Access to T0/T1 Modifying Firewall rules to provide Internet Access to T0/T1 Dec 11, 2024
@JimMadge JimMadge requested review from craddm and jemrobinson January 7, 2025 10:59
.github/workflows/lint_code.yaml Outdated Show resolved Hide resolved
tests/mustache/AdGuardHome.mustache.config.json Outdated Show resolved Hide resolved
data_safe_haven/infrastructure/programs/sre/dns_server.py Outdated Show resolved Hide resolved
data_safe_haven/infrastructure/programs/sre/firewall.py Outdated Show resolved Hide resolved
data_safe_haven/infrastructure/programs/sre/firewall.py Outdated Show resolved Hide resolved
data_safe_haven/infrastructure/programs/sre/firewall.py Outdated Show resolved Hide resolved
tags=tags,
)
for dns_zone_name in AzureDnsZoneNames.ALL
}, # TODO: Check if this works
Copy link
Member

Choose a reason for hiding this comment

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

Is this done?

Comment on lines 138 to 143
# TODO: Be more precise in rule filtering.
allow_internet_collection: list[dict] = [
rule_collection
for rule_collection in network_rule_collections
if rule_collection["name"] == "workspaces-all-allow"
]
Copy link
Member

Choose a reason for hiding this comment

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

Is this done?

@JimMadge
Copy link
Member

JimMadge commented Jan 7, 2025

I don't think I like adding TODO comments to code in a PR. I think they are too easy to miss and sidestep the issue/PR process. I think the two options are,

  1. fix it now
  2. open and issue

What does everyone else think?

data_safe_haven/infrastructure/programs/sre/dns_server.py Outdated Show resolved Hide resolved
network.AzureFirewallApplicationRuleCollectionArgs(
action=network.AzureFirewallRCActionArgs(
type=network.AzureFirewallRCActionType.ALLOW
application_rule_collections: list[
Copy link
Member

Choose a reason for hiding this comment

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

Can we make a new PR that just has the change that moves the application rules to the variable application_rule_collections = [...] without adding/removing/changing any rules? This would make it much easier to review what's actually changing here.

data_safe_haven/infrastructure/programs/sre/firewall.py Outdated Show resolved Hide resolved
data_safe_haven/infrastructure/programs/sre/firewall.py Outdated Show resolved Hide resolved
data_safe_haven/infrastructure/programs/sre/firewall.py Outdated Show resolved Hide resolved
tests/infrastructure/programs/sre/test_firewall.py Outdated Show resolved Hide resolved
tests/mustache/AdGuardHome.mustache.config.json Outdated Show resolved Hide resolved
@JimMadge JimMadge self-assigned this Jan 7, 2025
@JimMadge JimMadge force-pushed the 2283-internet-access-tier0-tier1 branch from 94a5682 to a7f9275 Compare January 9, 2025 15:07
@JimMadge JimMadge requested a review from a team as a code owner January 9, 2025 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Active support for T0/T1
4 participants