Add gateway mode "nat-unprotected" #48597

robmry · 2024-10-07T17:29:25Z

- What I did

Add gateway mode nat-unprotected for use in option com.docker.network.bridge.gateway_mode_ipv[46].

A nat-unprotected network:

has masquerading and port publishing to the host (like the default nat gateway mode).
has no port/protocol filtering for direct access to the container's address from remote hosts (unlike nat).

nat-unprotected mode can be used to restore the old default behaviour for IPv4 when the filter-FORWARD chain's default policy was "ACCEPT". (Before #48594, the daemon would only set the policy to "DROP" when it set sysctl "ip_forward" itself. Now, it never touches the default policy.)

https://docs.docker.com/engine/network/packet-filtering-firewalls/#direct-routing will need an update.

- How I did it

In the DOCKER chain, where nat mode appends a default-DROP rule for any packets not accepted by the per-port/protocol rules, nat-unprotected appends a default-ACCEPT rule (needed in case the filter-FORWARD chain's default policy is DROP).

Because the default is ACCEPT, no per-port/protocol rules to ACCEPT packets for the published ports are created. (If the userland proxy is enabled, it is still started.)

- How to verify it

New tests.

- Description for the changelog

- bridge driver options `com.docker.network.bridge.gateway_mode_ipv4` and `com.docker.network.bridge.gateway_mode_ipv6` now accept mode `nat-unprotected`.
  - `nat-unprotected` is similar to the default `nat` mode, but no per port/protocol iptables rules are set up.
  - this means any port on a container can be accessed by direct-routing from a remote host.

akerouanton · 2024-11-22T12:04:36Z

libnetwork/drivers/bridge/bridge_linux.go

 	return n.config.GwModeIPv4.routed(), n.config.GwModeIPv6.routed()
 }

+func (n *bridgeNetwork) getUnprotected(v iptables.IPVersion) bool {


I'm pretty sure we'll find this name confusing some time in the future. Maybe isNATUnprotected would be more explicit?

Suggested change

func (n *bridgeNetwork) getUnprotected(v iptables.IPVersion) bool {

func (n *bridgeNetwork) isNATUnprotected(v iptables.IPVersion) bool {

Or maybe you can introduce a more generic method: func (n *bridgeNetwork) isGwMode(gwm gwMode) bool 🤔

Hmm, yes - I see your point! I think I had routed-unprotected in-mind (which doesn't exist, but might at some point) ... so, this method would return true for *-unprotected.

I'll change it to isGwModeUnprotected.

akerouanton · 2024-11-22T12:17:44Z

libnetwork/drivers/bridge/bridge_linux.go

-	gwModeRouted  gwMode = "routed"
+	gwModeDefault   gwMode = ""
+	gwModeNAT       gwMode = "nat"
+	gwModeNATUnprot gwMode = "nat-unprotected"


I think we need to make nat-unprotected the default for existing networks -- only new networks should use nat. At least, that's what it should be for Docker Desktop (and DD doesn't have a way to 'migrate' Docker objects during upgrades).

Noting our discussion here ...

Setting nat-unprotected for existing IPv4 networks would be right if the host had previously been running with the filter-FORWARD chain's policy set to ACCEPT ... which happens when sysctl ip_forward is enabled before the daemon starts.

But, when it's set to DROP, and for IPv6 - only published ports would be exposed. So, we'd silently be making things worse in many cases by defaulting to nat-unprotected.

So, the change to the default nat mode will break a few setups by blocking remote access to unpublished ports, and anyone who's relying on that will have to re-create their networks with the new mode - but, at least it's fail-safe, and this change will block remote access for users (possibly the majority) who hadn't realised those ports were open.

akerouanton

LGTM

robmry · 2024-11-28T15:52:30Z

Rebased - which got rid of the first commit, it was just gofumpt reformatting.

robmry · 2024-11-28T16:54:14Z

Unrelated-looking test failure ...

=== FAIL: github.com/docker/docker/integration/container TestLogs/driver_json-file/tty/only_stdout (60.43s)
    logs_test.go:133: assertion failed: error is not nil: error during connect: Post "http://%2F%2F.%2Fpipe%2Fdocker_engine/v1.48/containers/create": EOF

thaJeztah · 2024-11-28T16:02:23Z

integration/network/bridge/iptablesdoc/templates/usernet-portmap-natunprot.md

+
+	docker network create \
+	  -o com.docker.network.bridge.name=bridge1 \
+	  -o com.docker.network.bridge.gateway_mode_ipv4=nat-unprotected \


The reason we need an option to explicitly set this for IPv4/IPv6 is that we changed the default for IPv6 already (but not IPv4 for backward-compat), correct?

(Mostly trying to get my head around what's needed to make the overall experience more aligned in future, and possibly reduce some complexity in codepaths)

Defaults haven't changed ... in 27.0 (#47871) we introduced the IPv4 and IPv6 gateway mode options - nat became a new name for the old behaviour, and routed mode was added. The default is still nat for both IPv4 and IPv6.

For IPv4, NAT is normal, so you might want to publish ports to the host. For IPv6, direct routing is preferred and, if you have the routing set up in your network, you might want to disable NAT. So, they're configurable separately.

In 28.0, we're changing the default behaviour (nat mode) by blocking access to unpublished ports from other hosts on the same layer-2 network that have set up a route to the container network ("direct routing"). Previously, these ports were only blocked if the filter-FORWARD chain's default policy was DROP. But, docker only sets to DROP if it enables IP forwarding itself.

The old behaviour for a host with filter-FORWARD policy ACCEPT is used as a feature, so we're adding nat-unprotected as a way to restore the old nat behaviour.

It's possible to use nat-unprotected for IPv6, but its filter-FORWARD policy was always set to DROP when ip6tables was enabled - so the new restrictions in nat mode are less likely to be breaking changes.

thaJeztah · 2024-11-28T17:35:41Z

libnetwork/drivers/bridge/bridge_linux.go

+	n.Lock()
+	defer n.Unlock()


Mostly curious; we have so much locking everywhere; do we have good insight what exactly is it synchronising? (was contemplating if it's purely n.config, and if we should have an accessor for that to return it 🤔

Yes, I think it's protecting concurrent access to n.config ... an accessor to fetch a copy of the whole of config might be better - but most of the locking is probably unnecessary because the config won't change once created.

But, when adding new accessors, I'm sticking with the scheme others us - it needs proper review and tidy-up and, for me, the cost of acquiring the mutex is outweighed by the risk of making it crashy by missing something!

thaJeztah · 2024-11-28T17:42:41Z

libnetwork/drivers/bridge/port_mapping_linux.go

-	if err := setPerPortForwarding(b, v, bridgeName, enable); err != nil {
-		return err
+
+	if !n.isGwModeUnprotected(v) {


Really nit-picky, but I had to look 2 levels deep to see the exact details;

we need isGwModeUnprotected to select the gateway mode for either IPv4 or IPv6

n.config.GwModeIPv6 / n.config.GwModeIPv4 are a gwMode, which is effectively a glorified string, but we started to dig in to adding gwMode.something() bool accessors, which just compare it

Wondering if it would make sense to have a n.gatewayMode(v iptables.IPVersion) gwMode method, which would allow comparing it where we need;

if n.gateWayMode(v) == gwModeNATUnprot { // ... }

or for cases where there's multiple code-paths;

switch n.gateWayMode(v) { case gwModeNAT: // .... case gwModeNATUnprot: // ... }

If we end up adding a routed-unprotected mode, this code will want to know whether the mode is nat-unprotected or routed-unprotected ... it could explicitly check for the mode being any one of the ones it's interested in - but I think collecting the logic into one place is better. Less danger of missing-a-bit when things change. But, can change it if you like?

I've unwrapped it a bit ... now there's a bridgeNetwork.gwMode() that returns a gwMode, and the caller checks unprotected() themselves.

(Similarly, could replace getNATDisabled() with gwMode(), although it'd mean acquiring the mutex an extra time to get both modes.)

thaJeztah · 2024-11-28T17:50:26Z

libnetwork/drivers/bridge/setup_ip_tables_linux.go

+	// that can route directly to the container's address (by setting up a
+	// route via the host's address).
+	action := "DROP"
+	if n.isGwModeUnprotected(ipVersion) {


Related to my earlier pondering about locking; this takes a lock here (not really important, just .. thinking out loud I guess)

thaJeztah · 2024-11-28T17:50:45Z

libnetwork/drivers/bridge/setup_ip_tables_linux.go

+
+	// Append to the filter table's DOCKER chain (the default rule must follow
+	// per-port ACCEPT rules, which will be inserted at the top of the chain).
+	if err := appendOrDelChainRule(rule, "DEFAULT FWD", true); err != nil {


But interestingly (regarding my "locking" pondering); n.registerIptCleanFunc actually mutates state of the network and does not have any synchronisation.

Right - it's a mess ... but, not this PR's mess!

thaJeztah · 2024-11-28T17:53:03Z

libnetwork/drivers/bridge/setup_ip_tables_linux.go

+	// Append to the filter table's DOCKER chain (the default rule must follow
+	// per-port ACCEPT rules, which will be inserted at the top of the chain).
+	if err := appendOrDelChainRule(rule, "DEFAULT FWD", true); err != nil {
+		return fmt.Errorf("failed to add default-drop rule: %s", err.Error())


ugh; I see a bunch more further up, and this was probably existing code, but we really shouldn't use err.Error() for these, and either wrap the error (if we need to be able to access the wrapped one) or just format;

Suggested change

return fmt.Errorf("failed to add default-drop rule: %s", err.Error())

return fmt.Errorf("failed to add default-drop rule: %w", err)

or (if we want to mask the underlying error)

Suggested change

return fmt.Errorf("failed to add default-drop rule: %s", err.Error())

return fmt.Errorf("failed to add default-drop rule: %v", err)

I guess it was a copy and paste of an existing block - I'll change it.

Signed-off-by: Rob Murray <[email protected]>

Same as "nat" mode, there's masquerading and port mapping from the host - but no port/protocol filtering for direct access to the container's address from remote hosts. This is the old default behaviour for IPv4 when the filter-FORWARD chain's default policy was "ACCEPT" (the daemon would only set it to "DROP" when it set sysctl "ip_forward" itself, but it didn't set up DROP rules for unpublished ports). Now, port filtering doesn't depend on the filter-FORWARD policy. So, this mode is added as a way to restore the old/surprising/insecure behaviour for anyone who's depending on it. Networks will need to be re-created with this new gateway mode. Signed-off-by: Rob Murray <[email protected]>

laurazard

LGTM

thaJeztah

LGTM

@tag

## Description Updates for moby 28.0 networking. ## Related issues or tickets Series of commits ... - Fix description of 'inhibit_ipv4' - Not changed in moby 28.0, updated to clarify difference from (new) IPv6-only networks. - Updates to default bridge address config - moby/moby#48319 - Describe IPv6-only network config - moby/moby#48271 - docker/cli#5599 - Update description of gateway modes - moby/moby#48594 - moby/moby#48596 - moby/moby#48597 - Describe gateway selection in the networking overview. - docker/cli#5664 - Describe gateway mode `isolated` - moby/moby#49262 ## Reviews   - [ ] Technical review - [ ] Editorial review - [ ] Product review --------- Signed-off-by: Rob Murray <[email protected]>

robmry added this to the 28.0.0 milestone Oct 7, 2024

robmry self-assigned this Oct 7, 2024

robmry force-pushed the nat-unprotected branch 6 times, most recently from be62866 to 393d4a0 Compare October 11, 2024 10:55

robmry mentioned this pull request Oct 11, 2024

Create static iptables rules during bridge driver init #48640

Merged

robmry mentioned this pull request Oct 23, 2024

Make containers on routed-mode networks accessible from other bridge networks #48596

Merged

robmry force-pushed the nat-unprotected branch 2 times, most recently from 826284d to a284fda Compare October 24, 2024 14:09

robmry mentioned this pull request Nov 4, 2024

[epic] iptables and port publishing improvements for moby 28.0 #48815

Closed

11 tasks

robmry force-pushed the nat-unprotected branch 2 times, most recently from b11455a to 7d85aa8 Compare November 19, 2024 17:20

robmry marked this pull request as ready for review November 19, 2024 19:03

robmry requested a review from akerouanton November 19, 2024 19:03

akerouanton reviewed Nov 22, 2024

View reviewed changes

robmry force-pushed the nat-unprotected branch from 7d85aa8 to 594541d Compare November 22, 2024 15:56

akerouanton approved these changes Nov 28, 2024

View reviewed changes

robmry force-pushed the nat-unprotected branch from 594541d to 8d02388 Compare November 28, 2024 15:51

thaJeztah reviewed Nov 28, 2024

View reviewed changes

Wrap errors rather than using err.Error()

07dff11

Signed-off-by: Rob Murray <[email protected]>

robmry force-pushed the nat-unprotected branch from 8d02388 to 027a336 Compare November 28, 2024 19:08

robmry force-pushed the nat-unprotected branch from 027a336 to 0aba672 Compare November 28, 2024 19:33

laurazard approved these changes Nov 29, 2024

View reviewed changes

thaJeztah approved these changes Nov 29, 2024

View reviewed changes

thaJeztah merged commit 24fd633 into moby:master Nov 29, 2024
140 checks passed

robmry deleted the nat-unprotected branch December 3, 2024 11:12

robmry mentioned this pull request Dec 11, 2024

Updates for moby 28.0 networking docker/docs#21612

Merged

3 tasks

	func (n *bridgeNetwork) getUnprotected(v iptables.IPVersion) bool {
	func (n *bridgeNetwork) isNATUnprotected(v iptables.IPVersion) bool {

	return fmt.Errorf("failed to add default-drop rule: %s", err.Error())
	return fmt.Errorf("failed to add default-drop rule: %w", err)

Add gateway mode "nat-unprotected" #48597

Add gateway mode "nat-unprotected" #48597

Uh oh!

Conversation

robmry commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robmry Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akerouanton left a comment

Choose a reason for hiding this comment

Uh oh!

robmry commented Nov 28, 2024

Uh oh!

robmry commented Nov 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurazard left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robmry commented Oct 7, 2024 •

edited

Loading

robmry Nov 22, 2024 •

edited

Loading