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

cptanalatriste · 2024-12-03T09:09:10Z

✅ 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.

(@JimMadge After my changes)

allow_workspace_internet configuration value in the sre section (default false)
When allowed, compared to the default rules,
- A network rule is added to allow outbound internet access from workspaces
- The application rules to allow specific outbound access (snap, apt) are removed
Add new variable to templates and documentation
Add warnings about possible data leakage when allow_workspace_internet is true.

🌂 Related issues

Closes #2283

🔬 Tests

(@JimMadge After my changes)

Outbound internet access from workspace when allow_workspace_internet is true

No outbound internet access from workspace when allow_workspace_internet is false

data_safe_haven/infrastructure/programs/sre/firewall.py

tests/infrastructure/programs/sre/test_firewall.py

craddm

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.

data_safe_haven/infrastructure/programs/sre/firewall.py

craddm · 2024-12-11T11:36:57Z

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

github-actions · 2024-12-11T16:58:57Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (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}

.github/workflows/lint_code.yaml

tests/mustache/AdGuardHome.mustache.config.json

data_safe_haven/infrastructure/programs/sre/dns_server.py

data_safe_haven/infrastructure/programs/sre/firewall.py

tests/infrastructure/programs/sre/test_firewall.py

data_safe_haven/infrastructure/programs/sre/firewall.py

JimMadge · 2025-01-07T11:50:19Z

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,

fix it now
open and issue

What does everyone else think?

data_safe_haven/infrastructure/programs/sre/dns_server.py

data_safe_haven/infrastructure/programs/sre/firewall.py

data_safe_haven/resources/dns_server/AdGuardHome.mustache.yaml

tests/infrastructure/programs/sre/test_firewall.py

tests/mustache/AdGuardHome.mustache.config.json

jemrobinson

Looks broadly OK but I have some suggested changes.

jemrobinson · 2025-01-10T15:40:58Z

.github/scripts/expand_mustache.sh

+    filename="${filename%.*}"
+    test_config=".github/resources/$filename.config.json"
+
+    if [ -e "$test_config" ]; then


Would it be better to do this for all mustache files we want to expand? It's nicer than the hack of replacing all the mustache markers with array or variable that's used in the other if branch?

jemrobinson · 2025-01-10T15:44:33Z

data_safe_haven/infrastructure/programs/sre/firewall.py

-                network.AzureFirewallApplicationRuleCollectionArgs(
-                    action=network.AzureFirewallRCActionArgs(
-                        type=network.AzureFirewallRCActionType.ALLOW
+        application_rule_collections_common = [


Can we confirm that these rules are identical to the previous rules except where deliberately changed? This sounds difficult. One way to convince ourselves could be to make a separate PR from develop that just makes the change of putting the rules into the application_rule_collections_common list without changing the rules themselves. It would then be easier to see what changes are being made here.

jemrobinson · 2025-01-10T15:46:44Z

data_safe_haven/infrastructure/programs/sre/firewall.py

+
+        if props.allow_workspace_internet:
+            application_rule_collections = application_rule_collections_common
+            network_rule_collections = [


Can you add a comment explaining why this needs to be a network rule not an application rule? I think possibly because application rules only apply to HTTP(S) but maybe I'm wrong.

jemrobinson · 2025-01-10T15:50:37Z

data_safe_haven/resources/dns_server/AdGuardHome.mustache.yaml

+{{#allow_workspace_internet}}
+user_rules: []
+{{/allow_workspace_internet}}
+{{^allow_workspace_internet}}
 user_rules:
  # https://github.com/AdguardTeam/AdGuardHome/wiki/Hosts-Blocklists#adblock-style-syntax
  - "*.*"
  {{#filter_allow}}
  - "@@||{{.}}"
  {{/filter_allow}}
+{{/allow_workspace_internet}}


Would this logic be better as:

user_rules: # https://github.com/AdguardTeam/AdGuardHome/wiki/Hosts-Blocklists#adblock-style-syntax {{#filter_block}} - "{{.}}" {{/filter_block}} {{#filter_allow}} - "@@||{{.}}" {{/filter_allow}}

with the default being: filter_block = ["*.*"], filter_allow = ["some", "urls", "here"] and the allow-all being filter_block = [], filter_allow = ["*.*"]? I'm not totally sure whether this would work - thoughts?

Alternatively, we generate the @@|| prefixes in the calling script and just have

user_rules: # https://github.com/AdguardTeam/AdGuardHome/wiki/Hosts-Blocklists#adblock-style-syntax {{#filters}} - "{{.}}" {{/filters}}

which might be the simplest/cleanest of all.

jemrobinson · 2025-01-10T15:52:45Z

tests/infrastructure/programs/sre/conftest.py

+# Mock configuration.
+
+
+class DataSafeHavenMocks(pulumi.runtime.Mocks):
+    def new_resource(self, args: pulumi.runtime.MockResourceArgs):


Suggested change

# Mock configuration.

class DataSafeHavenMocks(pulumi.runtime.Mocks):

def new_resource(self, args: pulumi.runtime.MockResourceArgs):

class DataSafeHavenMocks(pulumi.runtime.Mocks):

"""Configuration for Pulumi mocks"""

def new_resource(self, args: pulumi.runtime.MockResourceArgs):

cptanalatriste added 5 commits November 29, 2024 15:54

[WIP] Adding an extra flag to config-SRE

2daf019

[WIP] Setting up firewall tests

d0bd20c

[WIP] The component test does not work

7325672

Tests seem to be working

a5a1301

[WIP] My tests break other tests

4ba2d7e

cptanalatriste requested a review from a team as a code owner December 3, 2024 09:09

jemrobinson marked this pull request as draft December 3, 2024 09:12

jemrobinson reviewed Dec 3, 2024

View reviewed changes

data_safe_haven/infrastructure/programs/sre/firewall.py Outdated Show resolved Hide resolved

jemrobinson reviewed Dec 3, 2024

View reviewed changes

tests/infrastructure/programs/sre/test_firewall.py Outdated Show resolved Hide resolved

tests/infrastructure/programs/sre/test_firewall.py Outdated Show resolved Hide resolved

cptanalatriste added 4 commits December 4, 2024 16:21

[WIP] Using network rules for allowing internet access

57e5fdc

[WIP] Enable Mocks globally, and fixing tests broken by this

3f469a8

Remove unnecessary white space

c49a227

Merge branch 'develop' into 2283-internet-access-tier0-tier1

4baa8bd

craddm requested changes Dec 5, 2024

View reviewed changes

data_safe_haven/infrastructure/programs/sre/firewall.py Outdated Show resolved Hide resolved

data_safe_haven/infrastructure/programs/sre/firewall.py Show resolved Hide resolved

cptanalatriste added 3 commits December 9, 2024 11:21

Fixing bug

a57a8bb

[WIP] Enabling al sources on the network rule

d1e4f23

Fixing tests, and relaxing DNS rules when internet is allowed

6a0c4d7

Fixing firewall tests

5e46b73

cptanalatriste marked this pull request as ready for review December 11, 2024 17:28

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

cptanalatriste added 3 commits December 19, 2024 12:18

Supporting custom tests for Mustache templates

3394d92

Removing trailing space

08e2006

Removing trailing spaces from GitHub Action YAML file

21d8a55

JimMadge requested review from craddm and jemrobinson January 7, 2025 10:59

JimMadge reviewed Jan 7, 2025

View reviewed changes

data_safe_haven/infrastructure/programs/sre/firewall.py Outdated Show resolved Hide resolved

jemrobinson requested changes Jan 7, 2025

View reviewed changes

JimMadge self-assigned this Jan 7, 2025

JimMadge added 18 commits January 8, 2025 10:12

Move mustache template expansion file

6d21bd7

Move mustache expansion to a script

8cfc0b6

Fix issues in mustache script

92c0cad

Tidy mustache template completion

5e68a5f

Remove TODO

de3912c

Remove unnecessary type hint

39964dd

Add newlines at end of files

0453580

Remove delimiter option

83ce861

Tidy import in firewall tests

0748733

Tidy allow internet logic

da89565

Restore application collection rules

265c7e0

Use workspaces subnet for allow internet rule

3127086

Make rule collection names more consistent

357fc57

Collect common application rules

b25ee64

Avoid sponge

5fa8e9c

Add missing dummy template values

f842c2a

Correct source addresses

b136798

Fix firewall tests

a7f9275

JimMadge force-pushed the 2283-internet-access-tier0-tier1 branch from 94a5682 to a7f9275 Compare January 9, 2025 15:07

Add allow_workspace_internet parameter to template

ef05a6c

JimMadge requested a review from a team as a code owner January 9, 2025 15:47

JimMadge added 3 commits January 9, 2025 15:56

Add tests for template tiers

f995e01

Add warning to config template

b6811c4

Add CLI warning and confirmation

39549f9

JimMadge requested review from jemrobinson and JimMadge January 10, 2025 15:24

jemrobinson requested changes Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cptanalatriste commented Dec 3, 2024 •

edited by JimMadge

Loading

craddm left a comment •

edited

Loading

craddm commented Dec 11, 2024

github-actions bot commented Dec 11, 2024 •

edited

Loading

JimMadge commented Jan 7, 2025

jemrobinson left a comment

jemrobinson Jan 10, 2025

jemrobinson Jan 10, 2025

jemrobinson Jan 10, 2025

jemrobinson Jan 10, 2025

jemrobinson Jan 10, 2025

jemrobinson Jan 10, 2025

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

Are you sure you want to change the base?

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

Conversation

cptanalatriste commented Dec 3, 2024 • edited by JimMadge Loading

✅ Checklist

🚦 Depends on

⤴️ Summary

🌂 Related issues

🔬 Tests

craddm left a comment • edited Loading

Choose a reason for hiding this comment

craddm commented Dec 11, 2024

github-actions bot commented Dec 11, 2024 • edited Loading

Coverage report

JimMadge commented Jan 7, 2025

jemrobinson left a comment

Choose a reason for hiding this comment

jemrobinson Jan 10, 2025

Choose a reason for hiding this comment

jemrobinson Jan 10, 2025

Choose a reason for hiding this comment

jemrobinson Jan 10, 2025

Choose a reason for hiding this comment

jemrobinson Jan 10, 2025

Choose a reason for hiding this comment

jemrobinson Jan 10, 2025

Choose a reason for hiding this comment

jemrobinson Jan 10, 2025

Choose a reason for hiding this comment

cptanalatriste commented Dec 3, 2024 •

edited by JimMadge

Loading

craddm left a comment •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading