From 200a75b34ad4221fe2b4c320b6cf67f16698f1e7 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Tue, 5 Aug 2025 10:28:37 +0100 Subject: [PATCH 1/6] Return concrete types from NewNftabler/NewIptabler Signed-off-by: Rob Murray --- daemon/libnetwork/drivers/bridge/bridge_linux.go | 2 +- .../drivers/bridge/internal/iptabler/cleaner.go | 8 ++++---- .../drivers/bridge/internal/iptabler/iptabler.go | 8 ++++---- .../drivers/bridge/internal/iptabler/network.go | 4 ++-- .../drivers/bridge/internal/nftabler/cleaner.go | 4 ++-- .../drivers/bridge/internal/nftabler/network.go | 4 ++-- .../drivers/bridge/internal/nftabler/nftabler.go | 12 ++++++------ 7 files changed, 21 insertions(+), 21 deletions(-) diff --git a/daemon/libnetwork/drivers/bridge/bridge_linux.go b/daemon/libnetwork/drivers/bridge/bridge_linux.go index 84e66a7715..1ac1affed1 100644 --- a/daemon/libnetwork/drivers/bridge/bridge_linux.go +++ b/daemon/libnetwork/drivers/bridge/bridge_linux.go @@ -556,7 +556,7 @@ var newFirewaller = func(ctx context.Context, config firewaller.Config) (firewal // cleaner can't clean up network or port-specific rules that may have been added // to iptables built-in chains. So, if cleanup is needed, give the cleaner to // the nftabler. Then, it'll use it to delete old rules as networks are restored. - fw.(firewaller.FirewallCleanerSetter).SetFirewallCleaner(iptabler.NewCleaner(ctx, config)) + fw.SetFirewallCleaner(iptabler.NewCleaner(ctx, config)) return fw, nil } diff --git a/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go b/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go index 98f3f5f340..b58675ad8b 100644 --- a/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go +++ b/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go @@ -17,7 +17,7 @@ type iptablesCleaner struct { } // NewCleaner checks for iptables rules left behind by an old daemon that was using -// the iptabler. +// the Iptabler. // // If there are old rules present, it deletes as much as possible straight away // (user-defined chains and jumps from the built-in chains). @@ -62,7 +62,7 @@ func (ic iptablesCleaner) DelNetwork(ctx context.Context, nc firewaller.NetworkC } n := network{ config: nc, - ipt: &iptabler{config: ic.config}, + ipt: &Iptabler{config: ic.config}, } if ic.config.IPv4 && nc.Config4.Prefix.IsValid() { _ = deleteLegacyFilterRules(iptables.IPv4, nc.IfName) @@ -77,7 +77,7 @@ func (ic iptablesCleaner) DelNetwork(ctx context.Context, nc firewaller.NetworkC func (ic iptablesCleaner) DelEndpoint(ctx context.Context, nc firewaller.NetworkConfig, epIPv4, epIPv6 netip.Addr) { n := network{ config: nc, - ipt: &iptabler{config: ic.config}, + ipt: &Iptabler{config: ic.config}, } if n.ipt.config.IPv4 && epIPv4.IsValid() { _ = n.filterDirectAccess(ctx, iptables.IPv4, n.config.Config4, epIPv4, false) @@ -90,7 +90,7 @@ func (ic iptablesCleaner) DelEndpoint(ctx context.Context, nc firewaller.Network func (ic iptablesCleaner) DelPorts(ctx context.Context, nc firewaller.NetworkConfig, pbs []types.PortBinding) { n := network{ config: nc, - ipt: &iptabler{config: ic.config}, + ipt: &Iptabler{config: ic.config}, } _ = n.DelPorts(ctx, pbs) } diff --git a/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler.go b/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler.go index 74df0ba8b8..340dae6257 100644 --- a/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler.go +++ b/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler.go @@ -35,12 +35,12 @@ const ( isolationChain2 = "DOCKER-ISOLATION-STAGE-2" ) -type iptabler struct { +type Iptabler struct { config firewaller.Config } -func NewIptabler(ctx context.Context, config firewaller.Config) (firewaller.Firewaller, error) { - ipt := &iptabler{config: config} +func NewIptabler(ctx context.Context, config firewaller.Config) (*Iptabler, error) { + ipt := &Iptabler{config: config} if ipt.config.IPv4 { removeIPChains(ctx, iptables.IPv4) @@ -91,7 +91,7 @@ func NewIptabler(ctx context.Context, config firewaller.Config) (firewaller.Fire return ipt, nil } -func (ipt *iptabler) FilterForwardDrop(ctx context.Context, ipv firewaller.IPVersion) error { +func (ipt *Iptabler) FilterForwardDrop(ctx context.Context, ipv firewaller.IPVersion) error { var iptv iptables.IPVersion switch ipv { case firewaller.IPv4: diff --git a/daemon/libnetwork/drivers/bridge/internal/iptabler/network.go b/daemon/libnetwork/drivers/bridge/internal/iptabler/network.go index f80802fb35..5eef4d6690 100644 --- a/daemon/libnetwork/drivers/bridge/internal/iptabler/network.go +++ b/daemon/libnetwork/drivers/bridge/internal/iptabler/network.go @@ -22,11 +22,11 @@ type ( type network struct { config firewaller.NetworkConfig - ipt *iptabler + ipt *Iptabler cleanFuncs iptablesCleanFuncs } -func (ipt *iptabler) NewNetwork(ctx context.Context, nc firewaller.NetworkConfig) (_ firewaller.Network, retErr error) { +func (ipt *Iptabler) NewNetwork(ctx context.Context, nc firewaller.NetworkConfig) (_ firewaller.Network, retErr error) { n := &network{ ipt: ipt, config: nc, diff --git a/daemon/libnetwork/drivers/bridge/internal/nftabler/cleaner.go b/daemon/libnetwork/drivers/bridge/internal/nftabler/cleaner.go index 7efbfe7a91..c57c36d654 100644 --- a/daemon/libnetwork/drivers/bridge/internal/nftabler/cleaner.go +++ b/daemon/libnetwork/drivers/bridge/internal/nftabler/cleaner.go @@ -11,7 +11,7 @@ import ( "github.com/moby/moby/v2/daemon/libnetwork/internal/nftables" ) -// Cleanup deletes all rules created by nftabler; it's intended to be used +// Cleanup deletes all rules created by Nftabler; it's intended to be used // during startup, to clean up rules created by an old incarnation of the daemon // after switching to a different Firewaller implementation. func Cleanup(ctx context.Context, config firewaller.Config) { @@ -31,6 +31,6 @@ func Cleanup(ctx context.Context, config firewaller.Config) { } } -func (nft *nftabler) SetFirewallCleaner(fc firewaller.FirewallCleaner) { +func (nft *Nftabler) SetFirewallCleaner(fc firewaller.FirewallCleaner) { nft.cleaner = fc } diff --git a/daemon/libnetwork/drivers/bridge/internal/nftabler/network.go b/daemon/libnetwork/drivers/bridge/internal/nftabler/network.go index 996cc86128..b5dd54bf27 100644 --- a/daemon/libnetwork/drivers/bridge/internal/nftabler/network.go +++ b/daemon/libnetwork/drivers/bridge/internal/nftabler/network.go @@ -18,10 +18,10 @@ import ( type network struct { config firewaller.NetworkConfig cleaner func(ctx context.Context) error - fw *nftabler + fw *Nftabler } -func (nft *nftabler) NewNetwork(ctx context.Context, nc firewaller.NetworkConfig) (_ firewaller.Network, retErr error) { +func (nft *Nftabler) NewNetwork(ctx context.Context, nc firewaller.NetworkConfig) (_ firewaller.Network, retErr error) { n := &network{ fw: nft, config: nc, diff --git a/daemon/libnetwork/drivers/bridge/internal/nftabler/nftabler.go b/daemon/libnetwork/drivers/bridge/internal/nftabler/nftabler.go index 9d6f15c3e6..670c35175b 100644 --- a/daemon/libnetwork/drivers/bridge/internal/nftabler/nftabler.go +++ b/daemon/libnetwork/drivers/bridge/internal/nftabler/nftabler.go @@ -45,15 +45,15 @@ const ( rawPreroutingPortsRuleGroup = iota + initialRuleGroup + 1 ) -type nftabler struct { +type Nftabler struct { config firewaller.Config cleaner firewaller.FirewallCleaner table4 nftables.TableRef table6 nftables.TableRef } -func NewNftabler(ctx context.Context, config firewaller.Config) (firewaller.Firewaller, error) { - nft := &nftabler{config: config} +func NewNftabler(ctx context.Context, config firewaller.Config) (*Nftabler, error) { + nft := &Nftabler{config: config} if nft.config.IPv4 { var err error @@ -85,14 +85,14 @@ func NewNftabler(ctx context.Context, config firewaller.Config) (firewaller.Fire return nft, nil } -func (nft *nftabler) getTable(ipv firewaller.IPVersion) nftables.TableRef { +func (nft *Nftabler) getTable(ipv firewaller.IPVersion) nftables.TableRef { if ipv == firewaller.IPv4 { return nft.table4 } return nft.table6 } -func (nft *nftabler) FilterForwardDrop(ctx context.Context, ipv firewaller.IPVersion) error { +func (nft *Nftabler) FilterForwardDrop(ctx context.Context, ipv firewaller.IPVersion) error { table := nft.getTable(ipv) if err := table.Chain(ctx, forwardChain).SetPolicy("drop"); err != nil { return err @@ -101,7 +101,7 @@ func (nft *nftabler) FilterForwardDrop(ctx context.Context, ipv firewaller.IPVer } // init creates the bridge driver's nftables table for IPv4 or IPv6. -func (nft *nftabler) init(ctx context.Context, family nftables.Family) (nftables.TableRef, error) { +func (nft *Nftabler) init(ctx context.Context, family nftables.Family) (nftables.TableRef, error) { // Instantiate the table. table, err := nftables.NewTable(family, dockerTable) if err != nil { From 7dfeee8460191a629a70ef6389dcabfc1943cf15 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Tue, 5 Aug 2025 10:30:55 +0100 Subject: [PATCH 2/6] Drop FirewallCleanerSetter interface Signed-off-by: Rob Murray --- daemon/libnetwork/drivers/bridge/bridge_store.go | 6 +++--- .../drivers/bridge/internal/firewaller/firewaller.go | 6 ------ 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/daemon/libnetwork/drivers/bridge/bridge_store.go b/daemon/libnetwork/drivers/bridge/bridge_store.go index 92a7dc20d1..78dce7e53d 100644 --- a/daemon/libnetwork/drivers/bridge/bridge_store.go +++ b/daemon/libnetwork/drivers/bridge/bridge_store.go @@ -13,7 +13,7 @@ import ( "github.com/containerd/log" "github.com/moby/moby/v2/daemon/internal/otelutil" "github.com/moby/moby/v2/daemon/libnetwork/datastore" - "github.com/moby/moby/v2/daemon/libnetwork/drivers/bridge/internal/firewaller" + "github.com/moby/moby/v2/daemon/libnetwork/drivers/bridge/internal/nftabler" "github.com/moby/moby/v2/daemon/libnetwork/portmapperapi" "github.com/moby/moby/v2/daemon/libnetwork/types" "go.opentelemetry.io/otel" @@ -43,8 +43,8 @@ func (d *driver) initStore() error { // If there's a firewall cleaner, it's done its job by cleaning up rules // belonging to the restored networks. So, drop it. - if fcs, ok := d.firewaller.(firewaller.FirewallCleanerSetter); ok { - fcs.SetFirewallCleaner(nil) + if nft, ok := d.firewaller.(*nftabler.Nftabler); ok { + nft.SetFirewallCleaner(nil) } return nil diff --git a/daemon/libnetwork/drivers/bridge/internal/firewaller/firewaller.go b/daemon/libnetwork/drivers/bridge/internal/firewaller/firewaller.go index 743b03c23e..ce24ee5092 100644 --- a/daemon/libnetwork/drivers/bridge/internal/firewaller/firewaller.go +++ b/daemon/libnetwork/drivers/bridge/internal/firewaller/firewaller.go @@ -110,12 +110,6 @@ type Network interface { DelLink(ctx context.Context, parentIP, childIP netip.Addr, ports []types.TransportPort) } -// FirewallCleanerSetter is an optional interface for a Firewaller. -type FirewallCleanerSetter interface { - // SetFirewallCleaner replaces the FirewallCleaner (possibly with 'nil'). - SetFirewallCleaner(FirewallCleaner) -} - // FirewallCleaner is used to delete rules created by previous incarnations of // the daemon. On startup, once a Firewaller implementation has been selected, if // rules may have been left behind by a different Firewaller implementation, get From 67ffa47090d4682456f4ee3231637ac298d2b9ad Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Tue, 5 Aug 2025 10:51:35 +0100 Subject: [PATCH 3/6] nftables: don't enable IP forwarding For nftables only, never enable IP forwarding on the host. Instead, return an error on network creation if forwarding is not enabled, required by a bridge network, and --ip-forward=true. If IPv4 forwarding is not enabled when the daemon is started with nftables enabled and other config at defaults, the daemon will exit when it tries to create the default bridge. Otherwise, network creation will fail with an error if IPv4/IPv6 forwarding is not enabled when a network is created with IPv4/IPv6. It's the user's responsibility to configure and secure their host when they run Docker with nftables. Signed-off-by: Rob Murray --- .../libnetwork/drivers/bridge/bridge_linux.go | 18 ++++- .../bridge/internal/firewaller/firewaller.go | 3 - .../bridge/internal/firewaller/stub.go | 7 -- .../bridge/internal/iptabler/iptabler.go | 1 + .../bridge/internal/nftabler/nftabler.go | 15 ---- .../drivers/bridge/setup_ip_forwarding.go | 47 +++++++++-- .../bridge/setup_ip_forwarding_test.go | 81 +++++++++++-------- hack/make/.integration-daemon-start | 7 ++ .../internal/testutils/networking/firewall.go | 71 +++------------- .../testutils/networking/l3_segment_linux.go | 2 + .../network/bridge/bridge_linux_test.go | 42 ++++------ integration/networking/bridge_linux_test.go | 16 +++- .../networking/port_mapping_linux_test.go | 16 +++- 13 files changed, 164 insertions(+), 162 deletions(-) diff --git a/daemon/libnetwork/drivers/bridge/bridge_linux.go b/daemon/libnetwork/drivers/bridge/bridge_linux.go index 1ac1affed1..8af622e318 100644 --- a/daemon/libnetwork/drivers/bridge/bridge_linux.go +++ b/daemon/libnetwork/drivers/bridge/bridge_linux.go @@ -880,14 +880,28 @@ func (d *driver) createNetwork(ctx context.Context, config *networkConfiguration config.EnableIPv4 && d.config.EnableIPForwarding, "setupIPv4Forwarding", func(*networkConfiguration, *bridgeInterface) error { - return setupIPv4Forwarding(d.firewaller, d.config.EnableIPTables && !d.config.DisableFilterForwardDrop) + ffd, ok := d.firewaller.(filterForwardDropper) + if !ok { + // The firewaller can't drop non-Docker forwarding. It's up to the user to enable + // forwarding on their host, and configure their firewall appropriately. + return checkIPv4Forwarding() + } + // Enable forwarding and set a default-drop forwarding policy if necessary. + return setupIPv4Forwarding(ffd, d.config.EnableIPTables && !d.config.DisableFilterForwardDrop) }, }, { config.EnableIPv6 && d.config.EnableIPForwarding, "setupIPv6Forwarding", func(*networkConfiguration, *bridgeInterface) error { - return setupIPv6Forwarding(d.firewaller, d.config.EnableIP6Tables && !d.config.DisableFilterForwardDrop) + ffd, ok := d.firewaller.(filterForwardDropper) + if !ok { + // The firewaller can't drop non-Docker forwarding. It's up to the user to enable + // forwarding on their host, and configure their firewall appropriately. + return checkIPv6Forwarding() + } + // Enable forwarding and set a default-drop forwarding policy if necessary. + return setupIPv6Forwarding(ffd, d.config.EnableIP6Tables && !d.config.DisableFilterForwardDrop) }, }, diff --git a/daemon/libnetwork/drivers/bridge/internal/firewaller/firewaller.go b/daemon/libnetwork/drivers/bridge/internal/firewaller/firewaller.go index ce24ee5092..9ab68aaa58 100644 --- a/daemon/libnetwork/drivers/bridge/internal/firewaller/firewaller.go +++ b/daemon/libnetwork/drivers/bridge/internal/firewaller/firewaller.go @@ -76,9 +76,6 @@ type Firewaller interface { // NewNetwork returns an object that can be used to add published ports and legacy // links for a bridge network. NewNetwork(ctx context.Context, nc NetworkConfig) (Network, error) - // FilterForwardDrop sets the default policy of the FORWARD chain in the filter - // table to DROP. - FilterForwardDrop(ctx context.Context, ipv IPVersion) error } // Network can be used to manipulate firewall rules for a bridge network. diff --git a/daemon/libnetwork/drivers/bridge/internal/firewaller/stub.go b/daemon/libnetwork/drivers/bridge/internal/firewaller/stub.go index cf884edd26..f09df48415 100644 --- a/daemon/libnetwork/drivers/bridge/internal/firewaller/stub.go +++ b/daemon/libnetwork/drivers/bridge/internal/firewaller/stub.go @@ -15,7 +15,6 @@ import ( type StubFirewaller struct { Config Networks map[string]*StubFirewallerNetwork - FFD map[IPVersion]bool // filter forward drop } func NewStubFirewaller(config Config) *StubFirewaller { @@ -24,7 +23,6 @@ func NewStubFirewaller(config Config) *StubFirewaller { // A real Firewaller shouldn't hold on to its own networks, the bridge driver is doing that. // But, for unit tests cross-checking the driver, this is useful. Networks: make(map[string]*StubFirewallerNetwork), - FFD: make(map[IPVersion]bool), } } @@ -41,11 +39,6 @@ func (fw *StubFirewaller) NewNetwork(_ context.Context, nc NetworkConfig) (Netwo return nw, nil } -func (fw *StubFirewaller) FilterForwardDrop(_ context.Context, ipv IPVersion) error { - fw.FFD[ipv] = true - return nil -} - type stubFirewallerLink struct { parentIP netip.Addr childIP netip.Addr diff --git a/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler.go b/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler.go index 340dae6257..0a51582e3b 100644 --- a/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler.go +++ b/daemon/libnetwork/drivers/bridge/internal/iptabler/iptabler.go @@ -91,6 +91,7 @@ func NewIptabler(ctx context.Context, config firewaller.Config) (*Iptabler, erro return ipt, nil } +// FilterForwardDrop sets the default policy of the FORWARD chain in the filter table to DROP. func (ipt *Iptabler) FilterForwardDrop(ctx context.Context, ipv firewaller.IPVersion) error { var iptv iptables.IPVersion switch ipv { diff --git a/daemon/libnetwork/drivers/bridge/internal/nftabler/nftabler.go b/daemon/libnetwork/drivers/bridge/internal/nftabler/nftabler.go index 670c35175b..e0fe055bd9 100644 --- a/daemon/libnetwork/drivers/bridge/internal/nftabler/nftabler.go +++ b/daemon/libnetwork/drivers/bridge/internal/nftabler/nftabler.go @@ -85,21 +85,6 @@ func NewNftabler(ctx context.Context, config firewaller.Config) (*Nftabler, erro return nft, nil } -func (nft *Nftabler) getTable(ipv firewaller.IPVersion) nftables.TableRef { - if ipv == firewaller.IPv4 { - return nft.table4 - } - return nft.table6 -} - -func (nft *Nftabler) FilterForwardDrop(ctx context.Context, ipv firewaller.IPVersion) error { - table := nft.getTable(ipv) - if err := table.Chain(ctx, forwardChain).SetPolicy("drop"); err != nil { - return err - } - return nftApply(ctx, table) -} - // init creates the bridge driver's nftables table for IPv4 or IPv6. func (nft *Nftabler) init(ctx context.Context, family nftables.Family) (nftables.TableRef, error) { // Instantiate the table. diff --git a/daemon/libnetwork/drivers/bridge/setup_ip_forwarding.go b/daemon/libnetwork/drivers/bridge/setup_ip_forwarding.go index 4f2845d0f5..0ebd4eb17d 100644 --- a/daemon/libnetwork/drivers/bridge/setup_ip_forwarding.go +++ b/daemon/libnetwork/drivers/bridge/setup_ip_forwarding.go @@ -4,6 +4,7 @@ package bridge import ( "context" + "errors" "fmt" "os" @@ -17,7 +18,24 @@ const ( ipv6ForwardConfAll = "/proc/sys/net/ipv6/conf/all/forwarding" ) -func setupIPv4Forwarding(fw firewaller.Firewaller, wantFilterForwardDrop bool) (retErr error) { +type filterForwardDropper interface { + FilterForwardDrop(context.Context, firewaller.IPVersion) error +} + +func checkIPv4Forwarding() error { + enabled, err := getKernelBoolParam(ipv4ForwardConf) + if err != nil { + return fmt.Errorf("checking IPv4 forwarding: %w", err) + } + if enabled { + return nil + } + // It's the user's responsibility to enable forwarding and secure their host. Or, + // start docker with --ip-forward=false to disable this check. + return errors.New("IPv4 forwarding is disabled: check your host's firewalling and set sysctl net.ipv4.ip_forward=1, or disable this check using daemon option --ip-forward=false") +} + +func setupIPv4Forwarding(ffd filterForwardDropper, wantFilterForwardDrop bool) (retErr error) { changed, err := configureIPForwarding(ipv4ForwardConf, '1') if err != nil { return err @@ -34,16 +52,35 @@ func setupIPv4Forwarding(fw firewaller.Firewaller, wantFilterForwardDrop bool) ( // When enabling ip_forward set the default policy on forward chain to drop. if changed && wantFilterForwardDrop { - if err := fw.FilterForwardDrop(context.TODO(), firewaller.IPv4); err != nil { + if err := ffd.FilterForwardDrop(context.TODO(), firewaller.IPv4); err != nil { return err } } return nil } -func setupIPv6Forwarding(fw firewaller.Firewaller, wantFilterForwardDrop bool) (retErr error) { +func checkIPv6Forwarding() error { + enabledDef, err := getKernelBoolParam(ipv6ForwardConfDefault) + if err != nil { + return fmt.Errorf("checking IPv6 default forwarding: %w", err) + } + enabledAll, err := getKernelBoolParam(ipv6ForwardConfAll) + if err != nil { + return fmt.Errorf("checking IPv6 global forwarding: %w", err) + } + if enabledDef && enabledAll { + return nil + } + + // It's the user's responsibility to enable forwarding and secure their host. Or, + // start docker with --ip-forward=false to disable this check. + return errors.New("IPv6 global forwarding is disabled: check your host's firewalling and set sysctls net.ipv6.conf.all.forwarding=1 and net.ipv6.conf.default.forwarding=1, or disable this check using daemon option --ip-forward=false") +} + +func setupIPv6Forwarding(ffd filterForwardDropper, wantFilterForwardDrop bool) (retErr error) { // Set IPv6 default.forwarding, if needed. - // FIXME(robmry) - is it necessary to set this, setting "all" (below) does the job? + // Setting "all" (below) sets "default" as well, but need to check that "default" is + // set even if "all" is already set. changedDef, err := configureIPForwarding(ipv6ForwardConfDefault, '1') if err != nil { return err @@ -74,7 +111,7 @@ func setupIPv6Forwarding(fw firewaller.Firewaller, wantFilterForwardDrop bool) ( } if (changedAll || changedDef) && wantFilterForwardDrop { - if err := fw.FilterForwardDrop(context.TODO(), firewaller.IPv6); err != nil { + if err := ffd.FilterForwardDrop(context.TODO(), firewaller.IPv6); err != nil { return err } } diff --git a/daemon/libnetwork/drivers/bridge/setup_ip_forwarding_test.go b/daemon/libnetwork/drivers/bridge/setup_ip_forwarding_test.go index 65da1b9f79..7567c169de 100644 --- a/daemon/libnetwork/drivers/bridge/setup_ip_forwarding_test.go +++ b/daemon/libnetwork/drivers/bridge/setup_ip_forwarding_test.go @@ -14,19 +14,20 @@ import ( is "gotest.tools/v3/assert/cmp" ) -type ffdTestFirewaller struct { - ffd firewaller.IPVersion -} - -// NewNetwork is part of interface [firewaller.Firewaller]. -func (f *ffdTestFirewaller) NewNetwork(_ context.Context, _ firewaller.NetworkConfig) (firewaller.Network, error) { - return nil, nil +type ffDropper struct { + ffDrop4 bool + ffDrop6 bool } // FilterForwardDrop is part of interface [firewaller.Firewaller]. Just enough to check // it was called with the expected IPVersion. -func (f *ffdTestFirewaller) FilterForwardDrop(_ context.Context, ipv firewaller.IPVersion) error { - f.ffd = ipv +func (f *ffDropper) FilterForwardDrop(_ context.Context, ipv firewaller.IPVersion) error { + switch ipv { + case firewaller.IPv4: + f.ffDrop4 = true + case firewaller.IPv6: + f.ffDrop6 = true + } return nil } @@ -36,21 +37,16 @@ func TestSetupIPForwarding(t *testing.T) { for _, wantFFD := range []bool{true, false} { t.Run(fmt.Sprintf("wantFFD=%v", wantFFD), func(t *testing.T) { // Disable IP Forwarding if enabled - _, err := configureIPForwarding(ipv4ForwardConf, '0') - assert.NilError(t, err) + setForwarding(t, '0') // Set IP Forwarding - fw := &ffdTestFirewaller{} - err = setupIPv4Forwarding(fw, wantFFD) + ffd := &ffDropper{} + err := setupIPv4Forwarding(ffd, wantFFD) assert.NilError(t, err) // Check what the firewaller was told. - if wantFFD { - assert.Check(t, is.Equal(fw.ffd, firewaller.IPv4)) - } else { - var noVer firewaller.IPVersion - assert.Check(t, is.Equal(fw.ffd, noVer)) - } + assert.Check(t, is.Equal(ffd.ffDrop4, wantFFD)) + assert.Check(t, !ffd.ffDrop6) // Read new setting procSetting, err := os.ReadFile(ipv4ForwardConf) @@ -65,23 +61,15 @@ func TestSetupIP6Forwarding(t *testing.T) { for _, wantFFD := range []bool{true, false} { t.Run(fmt.Sprintf("wantFFD=%v", wantFFD), func(t *testing.T) { - _, err := configureIPForwarding(ipv6ForwardConfDefault, '0') - assert.NilError(t, err) - _, err = configureIPForwarding(ipv6ForwardConfAll, '0') - assert.NilError(t, err) + // Disable IP Forwarding if enabled + setForwarding(t, '0') // Set IP Forwarding - fw := &ffdTestFirewaller{} - err = setupIPv6Forwarding(fw, wantFFD) + ffd := &ffDropper{} + err := setupIPv6Forwarding(ffd, wantFFD) assert.NilError(t, err) - - // Check what the firewaller was told. - if wantFFD { - assert.Check(t, is.Equal(fw.ffd, firewaller.IPv6)) - } else { - var noVer firewaller.IPVersion - assert.Check(t, is.Equal(fw.ffd, noVer)) - } + assert.Check(t, !ffd.ffDrop4) + assert.Check(t, is.Equal(ffd.ffDrop6, wantFFD)) // Read new setting procSetting, err := os.ReadFile(ipv6ForwardConfDefault) @@ -93,3 +81,30 @@ func TestSetupIP6Forwarding(t *testing.T) { }) } } + +func TestCheckForwarding(t *testing.T) { + defer netnsutils.SetupTestOSContext(t)() + + setForwarding(t, '0') + err := checkIPv4Forwarding() + assert.Check(t, is.ErrorContains(err, "IPv4 forwarding is disabled")) + err = checkIPv6Forwarding() + assert.Check(t, is.ErrorContains(err, "IPv6 global forwarding is disabled")) + + setForwarding(t, '1') + err = checkIPv4Forwarding() + assert.Check(t, err) + err = checkIPv6Forwarding() + assert.Check(t, err) +} + +func setForwarding(t *testing.T, val byte) { + for _, sysctl := range []string{ + ipv4ForwardConf, + ipv6ForwardConfDefault, + ipv6ForwardConfAll, + } { + err := os.WriteFile(sysctl, []byte{val, '\n'}, 0o644) + assert.NilError(t, err) + } +} diff --git a/hack/make/.integration-daemon-start b/hack/make/.integration-daemon-start index 78f056356f..76d2efecdf 100644 --- a/hack/make/.integration-daemon-start +++ b/hack/make/.integration-daemon-start @@ -67,6 +67,13 @@ fi dockerd="dockerd" +# When running with the nftables backend, dockerd will not enable IP forwarding (by default, it +# will error on network creation if forwarding is not enabled). +if [ "$DOCKER_FIREWALL_BACKEND" = "nftables" ]; then + sysctl -w net.ipv4.ip_forward=1 > /dev/null + sysctl -w net.ipv6.conf.all.forwarding=1 > /dev/null +fi + if [ -n "$DOCKER_ROOTLESS" ]; then if [ -z "$TEST_SKIP_INTEGRATION_CLI" ]; then echo >&2 '# DOCKER_ROOTLESS requires TEST_SKIP_INTEGRATION_CLI to be set' diff --git a/integration/internal/testutils/networking/firewall.go b/integration/internal/testutils/networking/firewall.go index c7ba4e48ce..0c03087fc3 100644 --- a/integration/internal/testutils/networking/firewall.go +++ b/integration/internal/testutils/networking/firewall.go @@ -1,7 +1,6 @@ package networking import ( - "fmt" "os/exec" "regexp" "strings" @@ -9,45 +8,29 @@ import ( "github.com/moby/moby/v2/testutil/daemon" "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" "gotest.tools/v3/icmd" "gotest.tools/v3/poll" ) -const ( - // The name of the bridge driver's nftables tables. - nftTable = "docker-bridges" - // The name of the filter-FORWARD chain in nftTable. - nftFFChain = "filter-FORWARD" -) - // Find the policy in, for example "Chain FORWARD (policy ACCEPT)". var rePolicy = regexp.MustCompile("policy ([A-Za-z]+)") // SetFilterForwardPolicies sets the default policy for the FORWARD chain in -// the filter tables for both IPv4 and IPv6. The original policy is restored -// using t.Cleanup(). +// the iptables filter tables for both IPv4 and IPv6. The original policy is +// restored using t.Cleanup(). // // There's only one filter-FORWARD policy, so this won't behave well if used by // tests running in parallel in a single network namespace that expect different // behaviour. -func SetFilterForwardPolicies(t *testing.T, firewallBackend string, policy string) { - t.Helper() - if strings.HasPrefix(firewallBackend, "iptables") { - setIptablesFFP(t, policy) - return - } - if strings.HasPrefix(firewallBackend, "nftables") { - setNftablesFFP(t, policy) - return - } - t.Fatalf("unknown firewall backend %s", firewallBackend) -} - -func setIptablesFFP(t *testing.T, policy string) { +func SetFilterForwardPolicies(t *testing.T, policy string) { t.Helper() for _, iptablesCmd := range []string{"iptables", "ip6tables"} { - origPolicy, err := getChainPolicy(t, exec.Command(iptablesCmd, "-L", "FORWARD")) - assert.NilError(t, err, "failed to get iptables policy") + out, err := exec.Command(iptablesCmd, "-L", "FORWARD").Output() + assert.NilError(t, err, "failed to get %s policy", iptablesCmd) + opMatch := rePolicy.FindSubmatch(out) + assert.Assert(t, is.Len(opMatch, 2), "searching for policy: %w", err) + origPolicy := string(opMatch[1]) if origPolicy == policy { continue } @@ -62,42 +45,6 @@ func setIptablesFFP(t *testing.T, policy string) { } } -func setNftablesFFP(t *testing.T, policy string) { - t.Helper() - policy = strings.ToLower(policy) - for _, family := range []string{"ip", "ip6"} { - origPolicy, err := getChainPolicy(t, exec.Command("nft", "list", "chain", family, nftTable, nftFFChain)) - assert.NilError(t, err, "failed to get nftables policy") - if origPolicy == policy { - continue - } - cmd := func(p string) *exec.Cmd { - return exec.Command("nft", "add", "chain", family, nftTable, nftFFChain, "{", "policy", p, ";", "}") - } - if err := cmd(policy).Run(); err != nil { - t.Fatalf("Failed to set %s filter-FORWARD policy: %v", family, err) - } - t.Cleanup(func() { - if err := cmd(origPolicy).Run(); err != nil { - t.Logf("Failed to restore %s filter-FORWARD policy: %v", family, err) - } - }) - } -} - -func getChainPolicy(t *testing.T, cmd *exec.Cmd) (string, error) { - t.Helper() - out, err := cmd.Output() - if err != nil { - return "", fmt.Errorf("getting policy: %w", err) - } - opMatch := rePolicy.FindSubmatch(out) - if len(opMatch) != 2 { - return "", fmt.Errorf("searching for policy: %w", err) - } - return string(opMatch[1]), nil -} - // FirewalldRunning returns true if "firewall-cmd --state" reports "running". func FirewalldRunning() bool { state, err := exec.Command("firewall-cmd", "--state").CombinedOutput() diff --git a/integration/internal/testutils/networking/l3_segment_linux.go b/integration/internal/testutils/networking/l3_segment_linux.go index 26b4a69659..eb923eff2a 100644 --- a/integration/internal/testutils/networking/l3_segment_linux.go +++ b/integration/internal/testutils/networking/l3_segment_linux.go @@ -77,6 +77,8 @@ func (l3 *L3Segment) AddHost(t *testing.T, hostname, nsName, ifname string, addr l3.bridge.MustRun(t, "ip", "link", "set", hostname, "up", "master", l3.bridge.Iface) host.MustRun(t, "ip", "link", "set", host.Iface, "up") host.MustRun(t, "ip", "link", "set", "lo", "up") + host.MustRun(t, "sysctl", "-w", "net.ipv4.ip_forward=1") + host.MustRun(t, "sysctl", "-w", "net.ipv6.conf.all.forwarding=1") for _, addr := range addrs { host.MustRun(t, "ip", "addr", "add", addr.String(), "dev", host.Iface, "nodad") diff --git a/integration/network/bridge/bridge_linux_test.go b/integration/network/bridge/bridge_linux_test.go index 85a89bac13..671d744479 100644 --- a/integration/network/bridge/bridge_linux_test.go +++ b/integration/network/bridge/bridge_linux_test.go @@ -246,6 +246,8 @@ func TestIPRangeAt64BitLimit(t *testing.T) { func TestFilterForwardPolicy(t *testing.T) { skip.If(t, testEnv.IsRootless, "rootless has its own netns") skip.If(t, networking.FirewalldRunning(), "can't use firewalld in host netns to add rules in L3Segment") + skip.If(t, strings.HasPrefix(testEnv.FirewallBackendDriver(), "nftables"), "no policy is set for nftables") + ctx := setupTest(t) // Set up a netns for each test to avoid sysctl and iptables pollution. @@ -305,30 +307,17 @@ func TestFilterForwardPolicy(t *testing.T) { ) host := l3.Hosts[hostname] - getFwdPolicy := func(usingNftables bool, fam string) string { + getFwdPolicy := func(cmd string) string { t.Helper() - if usingNftables { - out := host.MustRun(t, "nft", "list chain "+fam+" docker-bridges filter-FORWARD") - if strings.Contains(out, "policy accept") { - return "ACCEPT" - } - if strings.Contains(out, "policy drop") { - return "DROP" - } - t.Fatalf("Failed to determine nftables filter-FORWARD policy: %s", out) - return "" - } else { - cmd := fam + "tables" - out := host.MustRun(t, cmd, "-S", "FORWARD") - if strings.HasPrefix(out, "-P FORWARD ACCEPT") { - return "ACCEPT" - } - if strings.HasPrefix(out, "-P FORWARD DROP") { - return "DROP" - } - t.Fatalf("Failed to determine %s FORWARD policy: %s", cmd, out) - return "" + out := host.MustRun(t, cmd, "-S", "FORWARD") + if strings.HasPrefix(out, "-P FORWARD ACCEPT") { + return "ACCEPT" } + if strings.HasPrefix(out, "-P FORWARD DROP") { + return "DROP" + } + t.Fatalf("Failed to determine %s FORWARD policy: %s", cmd, out) + return "" } type sysctls struct{ v4, v6def, v6all string } @@ -354,22 +343,21 @@ func TestFilterForwardPolicy(t *testing.T) { d.StartWithBusybox(ctx, t, tc.daemonArgs...) t.Cleanup(func() { d.Stop(t) }) }) - usingNftables := d.FirewallBackendDriver(t) == "nftables" c := d.NewClientT(t) t.Cleanup(func() { c.Close() }) // If necessary, the IPv4 policy should have been updated when the default bridge network was created. - assert.Check(t, is.Equal(getFwdPolicy(usingNftables, "ip"), tc.expPolicy)) + assert.Check(t, is.Equal(getFwdPolicy("iptables"), tc.expPolicy)) // IPv6 policy should not have been updated yet. - assert.Check(t, is.Equal(getFwdPolicy(usingNftables, "ip6"), "ACCEPT")) + assert.Check(t, is.Equal(getFwdPolicy("ip6tables"), "ACCEPT")) assert.Check(t, is.Equal(getSysctls(), sysctls{tc.expForwarding, tc.initForwarding, tc.initForwarding})) // If necessary, creating an IPv6 network should update the sysctls and policy. const netName = "testnetffp" network.CreateNoError(ctx, t, c, netName, network.WithIPv6()) t.Cleanup(func() { network.RemoveNoError(ctx, t, c, netName) }) - assert.Check(t, is.Equal(getFwdPolicy(usingNftables, "ip"), tc.expPolicy)) - assert.Check(t, is.Equal(getFwdPolicy(usingNftables, "ip6"), tc.expPolicy)) + assert.Check(t, is.Equal(getFwdPolicy("iptables"), tc.expPolicy)) + assert.Check(t, is.Equal(getFwdPolicy("ip6tables"), tc.expPolicy)) assert.Check(t, is.Equal(getSysctls(), sysctls{tc.expForwarding, tc.expForwarding, tc.expForwarding})) }) } diff --git a/integration/networking/bridge_linux_test.go b/integration/networking/bridge_linux_test.go index 47ec967f97..919d5d0871 100644 --- a/integration/networking/bridge_linux_test.go +++ b/integration/networking/bridge_linux_test.go @@ -358,7 +358,6 @@ func TestBridgeINCRouted(t *testing.T) { d := daemon.New(t) d.StartWithBusybox(ctx, t) t.Cleanup(func() { d.Stop(t) }) - firewallBackend := d.FirewallBackendDriver(t) c := d.NewClientT(t) t.Cleanup(func() { c.Close() }) @@ -457,10 +456,12 @@ func TestBridgeINCRouted(t *testing.T) { }, } - for _, fwdPolicy := range []string{"ACCEPT", "DROP"} { - networking.SetFilterForwardPolicies(t, firewallBackend, fwdPolicy) + runTests := func(testName, policy string) { networking.FirewalldReload(t, d) - t.Run(fwdPolicy, func(t *testing.T) { + t.Run(testName, func(t *testing.T) { + if policy != "" { + networking.SetFilterForwardPolicies(t, policy) + } for _, tc := range testcases { t.Run(tc.name+"/v4/ping", func(t *testing.T) { t.Parallel() @@ -497,6 +498,13 @@ func TestBridgeINCRouted(t *testing.T) { } }) } + + if strings.HasPrefix(d.FirewallBackendDriver(t), "iptables") { + runTests("iptables-ACCEPT", "ACCEPT") + runTests("iptables-DROP", "DROP") + } else { + runTests("nftables", "") + } } // TestAccessToPublishedPort checks that a container in one network can diff --git a/integration/networking/port_mapping_linux_test.go b/integration/networking/port_mapping_linux_test.go index 231258b4db..1993878d5e 100644 --- a/integration/networking/port_mapping_linux_test.go +++ b/integration/networking/port_mapping_linux_test.go @@ -648,7 +648,6 @@ func TestDirectRoutingOpenPorts(t *testing.T) { d := daemon.New(t) d.StartWithBusybox(ctx, t) t.Cleanup(func() { d.Stop(t) }) - firewallBackend := d.FirewallBackendDriver(t) c := d.NewClientT(t) t.Cleanup(func() { c.Close() }) @@ -770,9 +769,11 @@ func TestDirectRoutingOpenPorts(t *testing.T) { // Run the ping and http tests in two parallel groups, rather than waiting for // ping/http timeouts separately. (The iptables filter-FORWARD policy affects the // whole host, so ACCEPT/DROP tests can't be parallelized). - for _, fwdPolicy := range []string{"ACCEPT", "DROP"} { - networking.SetFilterForwardPolicies(t, firewallBackend, fwdPolicy) - t.Run(fwdPolicy, func(t *testing.T) { + runTests := func(testName, policy string) { + t.Run(testName, func(t *testing.T) { + if policy != "" { + networking.SetFilterForwardPolicies(t, policy) + } for gwMode := range networks { t.Run(gwMode+"/v4/ping", func(t *testing.T) { testPing(t, "ping", networks[gwMode].ipv4, expPingExit[gwMode]) @@ -795,6 +796,13 @@ func TestDirectRoutingOpenPorts(t *testing.T) { } }) } + + if strings.HasPrefix(d.FirewallBackendDriver(t), "iptables") { + runTests("iptables-ACCEPT", "ACCEPT") + runTests("iptables-DROP", "DROP") + } else { + runTests("nftables", "") + } } func TestAcceptFwMark(t *testing.T) { From f802d8a08ea29a95dfa29ab04516ada6788349e4 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Tue, 5 Aug 2025 12:18:09 +0100 Subject: [PATCH 4/6] When cleaning iptables rules, warn on filter-FORWARD DROP Signed-off-by: Rob Murray --- .../drivers/bridge/internal/iptabler/cleaner.go | 7 +++++++ daemon/libnetwork/iptables/iptables.go | 11 +++++++++++ 2 files changed, 18 insertions(+) diff --git a/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go b/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go index b58675ad8b..1de527b08b 100644 --- a/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go +++ b/daemon/libnetwork/drivers/bridge/internal/iptabler/cleaner.go @@ -46,6 +46,13 @@ func NewCleaner(ctx context.Context, config firewaller.Config) firewaller.Firewa _ = t.DeleteJumpRule(iptables.Filter, "FORWARD", DockerForwardChain) _ = deleteLegacyTopLevelRules(ctx, t, ipv) removeIPChains(ctx, ipv) + // The iptables chains will no longer have Docker's ACCEPT rules. So, if the + // filter-FORWARD chain has policy DROP (possibly set by Docker when it enabled + // IP forwarding), packets accepted by nftables chains will still be processed by + // iptables and dropped. It's the user's responsibility to sort that out. + if t.HasPolicy("filter", "FORWARD", iptables.Drop) { + log.G(ctx).WithField("ipv", ipv).Warn("Network traffic for published ports may be dropped, iptables chain FORWARD has policy DROP.") + } return true } cleaned4 := clean(iptables.IPv4, config.IPv4) diff --git a/daemon/libnetwork/iptables/iptables.go b/daemon/libnetwork/iptables/iptables.go index f2d473cc93..dd46c8ef06 100644 --- a/daemon/libnetwork/iptables/iptables.go +++ b/daemon/libnetwork/iptables/iptables.go @@ -3,6 +3,7 @@ package iptables import ( + "bytes" "context" "errors" "fmt" @@ -411,6 +412,16 @@ func (iptable IPTable) SetDefaultPolicy(table Table, chain string, policy Policy return nil } +// HasPolicy returns true if the chain exists and has the given policy. +func (iptable IPTable) HasPolicy(table Table, chain string, policy Policy) bool { + out, err := iptable.Raw("-t", string(table), "-L", chain) + if err != nil { + return false + } + firstLine, _, _ := bytes.Cut(out, []byte("\n")) + return strings.Contains(string(firstLine), "policy "+string(policy)) +} + // AddReturnRule adds a return rule for the chain in the filter table func (iptable IPTable) AddReturnRule(table Table, chain string) error { if iptable.Exists(table, chain, "-j", "RETURN") { From f71e86eed501b1739d520932c8545451c2dd23b1 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Wed, 6 Aug 2025 16:12:10 +0100 Subject: [PATCH 5/6] rootless: enable IP forwarding Signed-off-by: Rob Murray --- contrib/dockerd-rootless.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/contrib/dockerd-rootless.sh b/contrib/dockerd-rootless.sh index f798cd09dd..3ded28f6dd 100755 --- a/contrib/dockerd-rootless.sh +++ b/contrib/dockerd-rootless.sh @@ -199,5 +199,12 @@ else mount_directory /etc/ssl "--rbind" fi + # When running with --firewall-backend=nftables, IP forwarding needs to be enabled + # because the daemon won't enable it. IP forwarding is harmless in the rootless + # netns, there's only a single external interface and only Docker uses the netns. + # So, always enable IPv4 and IPv6 forwarding. + sysctl -w net.ipv4.ip_forward=1 + sysctl -w net.ipv6.conf.all.forwarding=1 + exec "$dockerd" "$@" fi From 2fff6b442e9f39ea01bfd0f1078e220973ead8d5 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Wed, 6 Aug 2025 16:43:36 +0100 Subject: [PATCH 6/6] check-config.sh: report IP forwarding sysctl state Signed-off-by: Rob Murray --- contrib/check-config.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/contrib/check-config.sh b/contrib/check-config.sh index af7d8a7de5..3e910ae956 100755 --- a/contrib/check-config.sh +++ b/contrib/check-config.sh @@ -128,6 +128,16 @@ check_device() { fi } +check_sysctl() { + val=$(sysctl -n $1) + want=$2 + if [ "$val" = "$want" ]; then + wrap_good "sysctl $1" "enabled" + else + wrap_bad "sysctl $1" "disabled" + fi +} + if [ ! -e "$CONFIG" ]; then wrap_warning "warning: $CONFIG does not exist, searching other paths for kernel config ..." for tryConfig in $possibleConfigs; do @@ -343,6 +353,10 @@ if ! is_set EXT4_FS || ! is_set EXT4_FS_POSIX_ACL || ! is_set EXT4_FS_SECURITY; fi echo '- Network Drivers:' +echo " - \"$(wrap_color 'bridge' blue)\":" +check_sysctl net.ipv4.ip_forward 1 | sed 's/^/ - /' +check_sysctl net.ipv6.conf.all.forwarding 1 | sed 's/^/ - /' +check_sysctl net.ipv6.conf.default.forwarding 1 | sed 's/^/ - /' echo " - \"$(wrap_color 'overlay' blue)\":" check_flags VXLAN BRIDGE_VLAN_FILTERING | sed 's/^/ /' echo ' Optional (for encrypted networks):'