From 24f53eba7f71dd60f0d56fc9450a53f080b05cdd Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Tue, 22 Oct 2024 13:55:50 +0100 Subject: [PATCH 1/5] Move integration/network/bridge_test.go to bridge subdir Also make it Linux-only, as the bridge driver is Linux only and all of the tests had skips for Windows. Signed-off-by: Rob Murray --- .../bridge_linux_test.go} | 9 +-- integration/network/bridge/main_test.go | 56 +++++++++++++++++++ 2 files changed, 57 insertions(+), 8 deletions(-) rename integration/network/{bridge_test.go => bridge/bridge_linux_test.go} (91%) create mode 100644 integration/network/bridge/main_test.go diff --git a/integration/network/bridge_test.go b/integration/network/bridge/bridge_linux_test.go similarity index 91% rename from integration/network/bridge_test.go rename to integration/network/bridge/bridge_linux_test.go index d3849349f5..7b455f8d01 100644 --- a/integration/network/bridge_test.go +++ b/integration/network/bridge/bridge_linux_test.go @@ -1,4 +1,4 @@ -package network +package bridge import ( "context" @@ -19,7 +19,6 @@ import ( ) func TestCreateWithMultiNetworks(t *testing.T) { - skip.If(t, testEnv.DaemonInfo.OSType == "windows") skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.44"), "requires API v1.44") ctx := setupTest(t) @@ -49,9 +48,6 @@ func TestCreateWithMultiNetworks(t *testing.T) { } func TestCreateWithIPv6DefaultsToULAPrefix(t *testing.T) { - // On Windows, network creation fails with this error message: Error response from daemon: this request is not supported by the 'windows' ipam driver - skip.If(t, testEnv.DaemonInfo.OSType == "windows") - ctx := setupTest(t) apiClient := testEnv.APIClient() @@ -73,7 +69,6 @@ func TestCreateWithIPv6DefaultsToULAPrefix(t *testing.T) { } func TestCreateWithIPv6WithoutEnableIPv6Flag(t *testing.T) { - skip.If(t, testEnv.DaemonInfo.OSType == "windows") // d.Start fails on Windows with `protocol not available` ctx := setupTest(t) d := daemon.New(t) @@ -103,7 +98,6 @@ func TestCreateWithIPv6WithoutEnableIPv6Flag(t *testing.T) { // Check that it's possible to create IPv6 networks with a 64-bit ip-range, // in 64-bit and bigger subnets, with and without a gateway. func Test64BitIPRange(t *testing.T) { - skip.If(t, testEnv.DaemonInfo.OSType == "windows", "no bridge or IPv6 on Windows") ctx := setupTest(t) c := testEnv.APIClient() @@ -139,7 +133,6 @@ func Test64BitIPRange(t *testing.T) { // Demonstrate a limitation of the IP address allocator, it can't // allocate the last address in range that ends on a 64-bit boundary. func TestIPRangeAt64BitLimit(t *testing.T) { - skip.If(t, testEnv.DaemonInfo.OSType == "windows", "no bridge or IPv6 on Windows") ctx := setupTest(t) c := testEnv.APIClient() diff --git a/integration/network/bridge/main_test.go b/integration/network/bridge/main_test.go new file mode 100644 index 0000000000..373d9afe82 --- /dev/null +++ b/integration/network/bridge/main_test.go @@ -0,0 +1,56 @@ +package bridge // import "github.com/docker/docker/integration/network/bridge" + +import ( + "context" + "os" + "testing" + + "github.com/docker/docker/testutil" + "github.com/docker/docker/testutil/environment" + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/codes" +) + +var ( + testEnv *environment.Execution + baseContext context.Context +) + +func TestMain(m *testing.M) { + shutdown := testutil.ConfigureTracing() + ctx, span := otel.Tracer("").Start(context.Background(), "integration/network/bridge.TestMain") + baseContext = ctx + + var err error + testEnv, err = environment.New(ctx) + if err != nil { + span.SetStatus(codes.Error, err.Error()) + span.End() + shutdown(ctx) + panic(err) + } + + err = environment.EnsureFrozenImagesLinux(ctx, testEnv) + if err != nil { + span.SetStatus(codes.Error, err.Error()) + span.End() + shutdown(ctx) + panic(err) + } + + testEnv.Print() + code := m.Run() + if code != 0 { + span.SetStatus(codes.Error, "m.Run() returned non-zero exit code") + } + span.End() + shutdown(ctx) + os.Exit(code) +} + +func setupTest(t *testing.T) context.Context { + ctx := testutil.StartSpan(baseContext, t) + environment.ProtectAll(ctx, t, testEnv) + t.Cleanup(func() { testEnv.Clean(ctx, t) }) + return ctx +} From a4d74c638301dfae3f84afd041ed7ac8ecb00212 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Tue, 29 Oct 2024 10:04:49 +0000 Subject: [PATCH 2/5] Move setupIPv6Forwarding() to setup_ip_forwarding.go Signed-off-by: Rob Murray --- .../drivers/bridge/setup_ip_forwarding.go | 28 +++++++++++++++++ libnetwork/drivers/bridge/setup_ipv6_linux.go | 31 ------------------- 2 files changed, 28 insertions(+), 31 deletions(-) diff --git a/libnetwork/drivers/bridge/setup_ip_forwarding.go b/libnetwork/drivers/bridge/setup_ip_forwarding.go index a29ac07785..c7fe15030b 100644 --- a/libnetwork/drivers/bridge/setup_ip_forwarding.go +++ b/libnetwork/drivers/bridge/setup_ip_forwarding.go @@ -72,3 +72,31 @@ func setupIPForwarding(enableIPTables bool, enableIP6Tables bool) error { return nil } + +func setupIPv6Forwarding(config *networkConfiguration, i *bridgeInterface) error { + // Get current IPv6 default forwarding setup + ipv6ForwardDataDefault, err := os.ReadFile(ipv6ForwardConfDefault) + if err != nil { + return fmt.Errorf("Cannot read IPv6 default forwarding setup: %v", err) + } + // Enable IPv6 default forwarding only if it is not already enabled + if ipv6ForwardDataDefault[0] != '1' { + if err := os.WriteFile(ipv6ForwardConfDefault, []byte{'1', '\n'}, ipv6ForwardConfPerm); err != nil { + log.G(context.TODO()).Warnf("Unable to enable IPv6 default forwarding: %v", err) + } + } + + // Get current IPv6 all forwarding setup + ipv6ForwardDataAll, err := os.ReadFile(ipv6ForwardConfAll) + if err != nil { + return fmt.Errorf("Cannot read IPv6 all forwarding setup: %v", err) + } + // Enable IPv6 all forwarding only if it is not already enabled + if ipv6ForwardDataAll[0] != '1' { + if err := os.WriteFile(ipv6ForwardConfAll, []byte{'1', '\n'}, ipv6ForwardConfPerm); err != nil { + log.G(context.TODO()).Warnf("Unable to enable IPv6 all forwarding: %v", err) + } + } + + return nil +} diff --git a/libnetwork/drivers/bridge/setup_ipv6_linux.go b/libnetwork/drivers/bridge/setup_ipv6_linux.go index 688c652064..c03290d858 100644 --- a/libnetwork/drivers/bridge/setup_ipv6_linux.go +++ b/libnetwork/drivers/bridge/setup_ipv6_linux.go @@ -1,12 +1,9 @@ package bridge import ( - "context" "fmt" "net/netip" "os" - - "github.com/containerd/log" ) // Standard link local prefix @@ -49,31 +46,3 @@ func setupGatewayIPv6(config *networkConfiguration, i *bridgeInterface) error { return nil } - -func setupIPv6Forwarding(config *networkConfiguration, i *bridgeInterface) error { - // Get current IPv6 default forwarding setup - ipv6ForwardDataDefault, err := os.ReadFile(ipv6ForwardConfDefault) - if err != nil { - return fmt.Errorf("Cannot read IPv6 default forwarding setup: %v", err) - } - // Enable IPv6 default forwarding only if it is not already enabled - if ipv6ForwardDataDefault[0] != '1' { - if err := os.WriteFile(ipv6ForwardConfDefault, []byte{'1', '\n'}, ipv6ForwardConfPerm); err != nil { - log.G(context.TODO()).Warnf("Unable to enable IPv6 default forwarding: %v", err) - } - } - - // Get current IPv6 all forwarding setup - ipv6ForwardDataAll, err := os.ReadFile(ipv6ForwardConfAll) - if err != nil { - return fmt.Errorf("Cannot read IPv6 all forwarding setup: %v", err) - } - // Enable IPv6 all forwarding only if it is not already enabled - if ipv6ForwardDataAll[0] != '1' { - if err := os.WriteFile(ipv6ForwardConfAll, []byte{'1', '\n'}, ipv6ForwardConfPerm); err != nil { - log.G(context.TODO()).Warnf("Unable to enable IPv6 all forwarding: %v", err) - } - } - - return nil -} From 5823b05c97daa0142dcc5f5d71f372c08a3b7881 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Wed, 6 Nov 2024 11:51:26 +0000 Subject: [PATCH 3/5] Modify IP forwarding and filter-FORWARD DROP setup. Before this change, for IPv4: - sysctl net.ipv4.ip_forward was enabled during bridge driver initialisation, if: - not already enabled - ip-forward=true, and - iptables=true. - the filter-FORWARD chain's policy was set to DROP, if the daemon updated the sysctl. - if setting the policy failed, the sysctl change was reverted. But, for IPv6: - sysctls net.ipv6.conf.[default|all].forwarding were both enabled when creating the first IPv6-enabled network, if: - they weren't already enabled, - ip-forward=true, and - ip6tables=true. - the filter-FORWARD chain's policy was set to DROP when creating an IPv4 enabled bridge network (inc. the default bridge), if: - ip6tables=true. - (this happened whether or not the daemon would ever enable IPv6 forwarding, or even create an IPv6 network.) The bridge driver no longer needs the default policy to be DROP to implement its own port-filtering rules. But, enabling IP forwarding without setting the filter-FORWARD policy to DROP would potentially be a security risk. This change aligns IPv4 and IPv6 behaviours: - only try to set the sysctls when creating a bridge network that needs them (for IPv4, that's still during daemon init because the default bridge is IPv4 enabled). - only check/set the filter-FORWARD policy after updating sysctls. - if the filter-FORWARD policy can't be set, treat it as an error and revert sysctl changes. We enabled ip6tables by default in 27.0. Setting the filter-FORWARD policy to DROP even when no IPv6 enabled network was created caused issues for some users. In particular, those running with iptables=false suddenly got the IPv6 DROP policy enabled (which broke unrelated services on the host). This change solves that by only setting the policy when necessary. Signed-off-by: Rob Murray --- .../iptablesdoc/generated/new-daemon.md | 6 +- .../iptablesdoc/templates/new-daemon.md | 6 +- libnetwork/drivers/bridge/bridge_linux.go | 23 ++- .../drivers/bridge/setup_ip_forwarding.go | 157 ++++++++++-------- .../bridge/setup_ip_forwarding_test.go | 46 ++--- libnetwork/drivers/bridge/setup_ipv6_linux.go | 8 +- 6 files changed, 117 insertions(+), 129 deletions(-) diff --git a/integration/network/bridge/iptablesdoc/generated/new-daemon.md b/integration/network/bridge/iptablesdoc/generated/new-daemon.md index 0a68fda235..06343900a9 100644 --- a/integration/network/bridge/iptablesdoc/generated/new-daemon.md +++ b/integration/network/bridge/iptablesdoc/generated/new-daemon.md @@ -68,8 +68,10 @@ Table `filter`: The FORWARD chain's policy shown above is ACCEPT. However: - For IPv4, [setupIPForwarding][1] sets the POLICY to DROP if the sysctl - net.ipv4.ip_forward was not set to '1', and the daemon set it itself. - - For IPv6, the policy is always DROP. + net.ipv4.ip_forward was not set to '1', and the daemon set it itself when + an IPv4-enabled bridge network was created. + - For IPv6, similar, but for sysctls "/proc/sys/net/ipv6/conf/default/forwarding" + and "/proc/sys/net/ipv6/conf/all/forwarding". [1]: https://github.com/moby/moby/blob/cff4f20c44a3a7c882ed73934dec6a77246c6323/libnetwork/drivers/bridge/setup_ip_forwarding.go#L44 diff --git a/integration/network/bridge/iptablesdoc/templates/new-daemon.md b/integration/network/bridge/iptablesdoc/templates/new-daemon.md index 330b36c7d0..aac3fb0b89 100644 --- a/integration/network/bridge/iptablesdoc/templates/new-daemon.md +++ b/integration/network/bridge/iptablesdoc/templates/new-daemon.md @@ -17,8 +17,10 @@ Table `filter`: The FORWARD chain's policy shown above is ACCEPT. However: - For IPv4, [setupIPForwarding][1] sets the POLICY to DROP if the sysctl - net.ipv4.ip_forward was not set to '1', and the daemon set it itself. - - For IPv6, the policy is always DROP. + net.ipv4.ip_forward was not set to '1', and the daemon set it itself when + an IPv4-enabled bridge network was created. + - For IPv6, similar, but for sysctls "/proc/sys/net/ipv6/conf/default/forwarding" + and "/proc/sys/net/ipv6/conf/all/forwarding". [1]: https://github.com/moby/moby/blob/cff4f20c44a3a7c882ed73934dec6a77246c6323/libnetwork/drivers/bridge/setup_ip_forwarding.go#L44 diff --git a/libnetwork/drivers/bridge/bridge_linux.go b/libnetwork/drivers/bridge/bridge_linux.go index 5b74112ab0..7d830b563a 100644 --- a/libnetwork/drivers/bridge/bridge_linux.go +++ b/libnetwork/drivers/bridge/bridge_linux.go @@ -552,14 +552,6 @@ func (d *driver) configure(option map[string]interface{}) error { } } - if config.EnableIPForwarding { - err = setupIPForwarding(config.EnableIPTables, config.EnableIP6Tables) - if err != nil { - log.G(context.TODO()).Warn(err) - return err - } - } - if config.Rootless { var err error pdc, err = newPortDriverClient(context.TODO()) @@ -890,8 +882,6 @@ func (d *driver) createNetwork(config *networkConfiguration) (err error) { bridgeSetup.queueStep(setupMTU) } - enableIPv6Forwarding := config.EnableIPv6 && d.config.EnableIPForwarding - // Module br_netfilter needs to be loaded with net.bridge.bridge-nf-call-ip[6]tables // enabled to implement icc=false, or DNAT when the userland-proxy is disabled. enableBrNfCallIptables := !config.EnableICC || !d.config.EnableUserlandProxy @@ -914,8 +904,17 @@ func (d *driver) createNetwork(config *networkConfiguration) (err error) { // existing device. {config.EnableIPv4 && bridgeAlreadyExists && !config.InhibitIPv4, setupVerifyAndReconcileIPv4}, - // Enable IPv6 Forwarding - {enableIPv6Forwarding, setupIPv6Forwarding}, + // Enable IP Forwarding + {config.EnableIPv4 && d.config.EnableIPForwarding, + func(*networkConfiguration, *bridgeInterface) error { + return setupIPv4Forwarding(d.config.EnableIPTables) + }, + }, + {config.EnableIPv6 && d.config.EnableIPForwarding, + func(*networkConfiguration, *bridgeInterface) error { + return setupIPv6Forwarding(d.config.EnableIP6Tables) + }, + }, // Setup Loopback Addresses Routing {!d.config.EnableUserlandProxy, setupLoopbackAddressesRouting}, diff --git a/libnetwork/drivers/bridge/setup_ip_forwarding.go b/libnetwork/drivers/bridge/setup_ip_forwarding.go index c7fe15030b..ebac3dd366 100644 --- a/libnetwork/drivers/bridge/setup_ip_forwarding.go +++ b/libnetwork/drivers/bridge/setup_ip_forwarding.go @@ -12,91 +12,106 @@ import ( ) const ( - ipv4ForwardConf = "/proc/sys/net/ipv4/ip_forward" - ipv4ForwardConfPerm = 0o644 + ipv4ForwardConf = "/proc/sys/net/ipv4/ip_forward" + ipv6ForwardConfDefault = "/proc/sys/net/ipv6/conf/default/forwarding" + ipv6ForwardConfAll = "/proc/sys/net/ipv6/conf/all/forwarding" ) -func configureIPForwarding(enable bool) error { - var val byte - if enable { - val = '1' +func setupIPv4Forwarding(wantFilterForwardDrop bool) (retErr error) { + changed, err := configureIPForwarding(ipv4ForwardConf, '1') + if err != nil { + return err } - return os.WriteFile(ipv4ForwardConf, []byte{val, '\n'}, ipv4ForwardConfPerm) + if changed { + defer func() { + if retErr != nil { + if _, err := configureIPForwarding(ipv4ForwardConf, '0'); err != nil { + log.G(context.TODO()).WithError(err).Error("Cannot disable IPv4 forwarding") + } + } + }() + } + + // When enabling ip_forward set the default policy on forward chain to drop. + if changed && wantFilterForwardDrop { + if err := setFilterForwardDrop(iptables.IPv4); err != nil { + return err + } + } + return nil } -func setupIPForwarding(enableIPTables bool, enableIP6Tables bool) error { - // Get current IPv4 forward setup - ipv4ForwardData, err := os.ReadFile(ipv4ForwardConf) +func setupIPv6Forwarding(wantFilterForwardDrop bool) (retErr error) { + // Set IPv6 default.forwarding, if needed. + // FIXME(robmry) - is it necessary to set this, setting "all" (below) does the job? + changedDef, err := configureIPForwarding(ipv6ForwardConfDefault, '1') if err != nil { - return fmt.Errorf("Cannot read IP forwarding setup: %v", err) + return err } - - // Enable IPv4 forwarding only if it is not already enabled - if ipv4ForwardData[0] != '1' { - // Enable IPv4 forwarding - if err := configureIPForwarding(true); err != nil { - return fmt.Errorf("Enabling IP forwarding failed: %v", err) - } - // When enabling ip_forward set the default policy on forward chain to - // drop only if the daemon option iptables is not set to false. - if enableIPTables { - iptable := iptables.GetIptable(iptables.IPv4) - if err := iptable.SetDefaultPolicy(iptables.Filter, "FORWARD", iptables.Drop); err != nil { - if err := configureIPForwarding(false); err != nil { - log.G(context.TODO()).Errorf("Disabling IP forwarding failed, %v", err) + if changedDef { + defer func() { + if retErr != nil { + if _, err := configureIPForwarding(ipv6ForwardConfDefault, '0'); err != nil { + log.G(context.TODO()).WithError(err).Error("Cannot disable IPv6 default.forwarding") } - return err } - iptables.OnReloaded(func() { - log.G(context.TODO()).Debug("Setting the default DROP policy on firewall reload") - if err := iptable.SetDefaultPolicy(iptables.Filter, "FORWARD", iptables.Drop); err != nil { - log.G(context.TODO()).Warnf("Setting the default DROP policy on firewall reload failed, %v", err) + }() + } + + // Set IPv6 all.forwarding, if needed. + changedAll, err := configureIPForwarding(ipv6ForwardConfAll, '1') + if err != nil { + return err + } + if changedAll { + defer func() { + if retErr != nil { + if _, err := configureIPForwarding(ipv6ForwardConfAll, '0'); err != nil { + log.G(context.TODO()).WithError(err).Error("Cannot disable IPv6 all.forwarding") } - }) + } + }() + } + + if (changedAll || changedDef) && wantFilterForwardDrop { + if err := setFilterForwardDrop(iptables.IPv6); err != nil { + return err } } - // add only iptables rules - forwarding is handled by setupIPv6Forwarding in setup_ipv6 - if enableIP6Tables { - iptable := iptables.GetIptable(iptables.IPv6) + return nil +} + +func configureIPForwarding(file string, val byte) (changed bool, _ error) { + data, err := os.ReadFile(file) + if err != nil || len(data) == 0 { + return false, fmt.Errorf("cannot read IP forwarding setup from '%s': %w", file, err) + } + if len(data) == 0 { + return false, fmt.Errorf("cannot read IP forwarding setup from '%s': 0 bytes", file) + } + if data[0] == val { + return false, nil + } + if err := os.WriteFile(file, []byte{val, '\n'}, 0o644); err != nil { + return false, fmt.Errorf("failed to set IP forwarding '%s' = '%c': %w", file, val, err) + } + return true, nil +} + +func setFilterForwardDrop(ipv iptables.IPVersion) error { + iptable := iptables.GetIptable(ipv) + if err := iptable.SetDefaultPolicy(iptables.Filter, "FORWARD", iptables.Drop); err != nil { + return err + } + iptables.OnReloaded(func() { + log.G(context.TODO()).WithFields(log.Fields{"ipv": ipv}).Debug("Setting the default DROP policy on firewall reload") if err := iptable.SetDefaultPolicy(iptables.Filter, "FORWARD", iptables.Drop); err != nil { - log.G(context.TODO()).Warnf("Setting the default DROP policy on firewall reload failed, %v", err) + log.G(context.TODO()).WithFields(log.Fields{ + "error": err, + "ipv": ipv, + }).Warn("Failed to set the default DROP policy on firewall reload") } - iptables.OnReloaded(func() { - log.G(context.TODO()).Debug("Setting the default DROP policy on firewall reload") - if err := iptable.SetDefaultPolicy(iptables.Filter, "FORWARD", iptables.Drop); err != nil { - log.G(context.TODO()).Warnf("Setting the default DROP policy on firewall reload failed, %v", err) - } - }) - } - - return nil -} - -func setupIPv6Forwarding(config *networkConfiguration, i *bridgeInterface) error { - // Get current IPv6 default forwarding setup - ipv6ForwardDataDefault, err := os.ReadFile(ipv6ForwardConfDefault) - if err != nil { - return fmt.Errorf("Cannot read IPv6 default forwarding setup: %v", err) - } - // Enable IPv6 default forwarding only if it is not already enabled - if ipv6ForwardDataDefault[0] != '1' { - if err := os.WriteFile(ipv6ForwardConfDefault, []byte{'1', '\n'}, ipv6ForwardConfPerm); err != nil { - log.G(context.TODO()).Warnf("Unable to enable IPv6 default forwarding: %v", err) - } - } - - // Get current IPv6 all forwarding setup - ipv6ForwardDataAll, err := os.ReadFile(ipv6ForwardConfAll) - if err != nil { - return fmt.Errorf("Cannot read IPv6 all forwarding setup: %v", err) - } - // Enable IPv6 all forwarding only if it is not already enabled - if ipv6ForwardDataAll[0] != '1' { - if err := os.WriteFile(ipv6ForwardConfAll, []byte{'1', '\n'}, ipv6ForwardConfPerm); err != nil { - log.G(context.TODO()).Warnf("Unable to enable IPv6 all forwarding: %v", err) - } - } - + }) return nil } diff --git a/libnetwork/drivers/bridge/setup_ip_forwarding_test.go b/libnetwork/drivers/bridge/setup_ip_forwarding_test.go index 26be998080..a07b675a29 100644 --- a/libnetwork/drivers/bridge/setup_ip_forwarding_test.go +++ b/libnetwork/drivers/bridge/setup_ip_forwarding_test.go @@ -3,51 +3,27 @@ package bridge import ( - "bytes" "os" "testing" + + "github.com/docker/docker/internal/testutils/netnsutils" + "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" ) func TestSetupIPForwarding(t *testing.T) { - // Read current setting and ensure the original value gets restored - procSetting := readCurrentIPForwardingSetting(t) - defer reconcileIPForwardingSetting(t, procSetting) + defer netnsutils.SetupTestOSContext(t)() // Disable IP Forwarding if enabled - if bytes.Equal(procSetting, []byte("1\n")) { - writeIPForwardingSetting(t, []byte{'0', '\n'}) - } + _, err := configureIPForwarding(ipv4ForwardConf, '0') + assert.NilError(t, err) // Set IP Forwarding - if err := setupIPForwarding(true, true); err != nil { - t.Fatalf("Failed to setup IP forwarding: %v", err) - } + err = setupIPv4Forwarding(true) + assert.NilError(t, err) // Read new setting - procSetting = readCurrentIPForwardingSetting(t) - if !bytes.Equal(procSetting, []byte("1\n")) { - t.Fatal("Failed to effectively setup IP forwarding") - } -} - -func readCurrentIPForwardingSetting(t *testing.T) []byte { procSetting, err := os.ReadFile(ipv4ForwardConf) - if err != nil { - t.Fatalf("Can't execute test: Failed to read current IP forwarding setting: %v", err) - } - return procSetting -} - -func writeIPForwardingSetting(t *testing.T, chars []byte) { - err := os.WriteFile(ipv4ForwardConf, chars, ipv4ForwardConfPerm) - if err != nil { - t.Fatalf("Can't execute or cleanup after test: Failed to reset IP forwarding: %v", err) - } -} - -func reconcileIPForwardingSetting(t *testing.T, original []byte) { - current := readCurrentIPForwardingSetting(t) - if !bytes.Equal(original, current) { - writeIPForwardingSetting(t, original) - } + assert.NilError(t, err) + assert.Check(t, is.DeepEqual(procSetting, []byte{'1', '\n'})) } diff --git a/libnetwork/drivers/bridge/setup_ipv6_linux.go b/libnetwork/drivers/bridge/setup_ipv6_linux.go index c03290d858..521d013302 100644 --- a/libnetwork/drivers/bridge/setup_ipv6_linux.go +++ b/libnetwork/drivers/bridge/setup_ipv6_linux.go @@ -9,12 +9,6 @@ import ( // Standard link local prefix var linkLocalPrefix = netip.MustParsePrefix("fe80::/64") -const ( - ipv6ForwardConfPerm = 0o644 - ipv6ForwardConfDefault = "/proc/sys/net/ipv6/conf/default/forwarding" - ipv6ForwardConfAll = "/proc/sys/net/ipv6/conf/all/forwarding" -) - func setupBridgeIPv6(config *networkConfiguration, i *bridgeInterface) error { procFile := "/proc/sys/net/ipv6/conf/" + config.BridgeName + "/disable_ipv6" ipv6BridgeData, err := os.ReadFile(procFile) @@ -23,7 +17,7 @@ func setupBridgeIPv6(config *networkConfiguration, i *bridgeInterface) error { } // Enable IPv6 on the bridge only if it isn't already enabled if ipv6BridgeData[0] != '0' { - if err := os.WriteFile(procFile, []byte{'0', '\n'}, ipv6ForwardConfPerm); err != nil { + if err := os.WriteFile(procFile, []byte{'0', '\n'}, 0o644); err != nil { return fmt.Errorf("Unable to enable IPv6 addresses on bridge: %v", err) } } From 3cadadb4eb639ae265ab86585106643f2789352e Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Mon, 21 Oct 2024 18:53:29 +0100 Subject: [PATCH 4/5] Add daemon option --ip-forward-no-drop The daemon no longer depends on the iptables/ip6tables filter-FORWARD chain's policy being DROP in order to implement its port filtering rules. However, if the daemon enables IP forwarding in the host's system config, by default it will set the policy to DROP to avoid potential security issues for other applications/networks. If docker does need to enable IP forwarding, but other applications on the host require filter-FORWARD's policies to be ACCEPT, this option can be used to tell the daemon to leave the policy unchanged. (Equivalent to enabling IP forwarding before starting the daemon, but without needing to do that.) Signed-off-by: Rob Murray --- cmd/dockerd/config_unix.go | 3 ++- daemon/config/config_linux.go | 13 +++++++------ daemon/daemon_unix.go | 13 +++++++------ libnetwork/drivers/bridge/bridge_linux.go | 17 +++++++++-------- man/dockerd.8.md | 14 ++++++++++++-- 5 files changed, 37 insertions(+), 23 deletions(-) diff --git a/cmd/dockerd/config_unix.go b/cmd/dockerd/config_unix.go index c9f240ebfd..6074f2c995 100644 --- a/cmd/dockerd/config_unix.go +++ b/cmd/dockerd/config_unix.go @@ -27,7 +27,8 @@ func installConfigFlags(conf *config.Config, flags *pflag.FlagSet) { flags.Var(opts.NewNamedUlimitOpt("default-ulimits", &conf.Ulimits), "default-ulimit", "Default ulimits for containers") flags.BoolVar(&conf.BridgeConfig.EnableIPTables, "iptables", true, "Enable addition of iptables rules") flags.BoolVar(&conf.BridgeConfig.EnableIP6Tables, "ip6tables", true, "Enable addition of ip6tables rules") - flags.BoolVar(&conf.BridgeConfig.EnableIPForward, "ip-forward", true, "Enable net.ipv4.ip_forward") + flags.BoolVar(&conf.BridgeConfig.EnableIPForward, "ip-forward", true, "Enable IP forwarding in system configuration") + flags.BoolVar(&conf.BridgeConfig.DisableFilterForwardDrop, "ip-forward-no-drop", false, "Do not set the filter-FORWARD policy to DROP when enabling IP forwarding") flags.BoolVar(&conf.BridgeConfig.EnableIPMasq, "ip-masq", true, "Enable IP masquerading") flags.BoolVar(&conf.BridgeConfig.EnableIPv6, "ipv6", false, "Enable IPv6 networking") flags.StringVar(&conf.BridgeConfig.IP, "bip", "", "Specify network bridge IP") diff --git a/daemon/config/config_linux.go b/daemon/config/config_linux.go index a12177af1a..18ca1007bd 100644 --- a/daemon/config/config_linux.go +++ b/daemon/config/config_linux.go @@ -41,12 +41,13 @@ const ( type BridgeConfig struct { DefaultBridgeConfig - EnableIPTables bool `json:"iptables,omitempty"` - EnableIP6Tables bool `json:"ip6tables,omitempty"` - EnableIPForward bool `json:"ip-forward,omitempty"` - EnableIPMasq bool `json:"ip-masq,omitempty"` - EnableUserlandProxy bool `json:"userland-proxy,omitempty"` - UserlandProxyPath string `json:"userland-proxy-path,omitempty"` + EnableIPTables bool `json:"iptables,omitempty"` + EnableIP6Tables bool `json:"ip6tables,omitempty"` + EnableIPForward bool `json:"ip-forward,omitempty"` + DisableFilterForwardDrop bool `json:"ip-forward-no-drop,omitempty"` + EnableIPMasq bool `json:"ip-masq,omitempty"` + EnableUserlandProxy bool `json:"userland-proxy,omitempty"` + UserlandProxyPath string `json:"userland-proxy-path,omitempty"` } // DefaultBridgeConfig stores all the parameters for the default bridge network. diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 1ddacd9562..31a1d3f47d 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -915,12 +915,13 @@ func setHostGatewayIP(controller *libnetwork.Controller, config *config.Config) func driverOptions(config *config.Config) nwconfig.Option { return nwconfig.OptionDriverConfig("bridge", options.Generic{ netlabel.GenericData: options.Generic{ - "EnableIPForwarding": config.BridgeConfig.EnableIPForward, - "EnableIPTables": config.BridgeConfig.EnableIPTables, - "EnableIP6Tables": config.BridgeConfig.EnableIP6Tables, - "EnableUserlandProxy": config.BridgeConfig.EnableUserlandProxy, - "UserlandProxyPath": config.BridgeConfig.UserlandProxyPath, - "Rootless": config.Rootless, + "EnableIPForwarding": config.BridgeConfig.EnableIPForward, + "DisableFilterForwardDrop": config.BridgeConfig.DisableFilterForwardDrop, + "EnableIPTables": config.BridgeConfig.EnableIPTables, + "EnableIP6Tables": config.BridgeConfig.EnableIP6Tables, + "EnableUserlandProxy": config.BridgeConfig.EnableUserlandProxy, + "UserlandProxyPath": config.BridgeConfig.UserlandProxyPath, + "Rootless": config.Rootless, }, }) } diff --git a/libnetwork/drivers/bridge/bridge_linux.go b/libnetwork/drivers/bridge/bridge_linux.go index 7d830b563a..86488cdac9 100644 --- a/libnetwork/drivers/bridge/bridge_linux.go +++ b/libnetwork/drivers/bridge/bridge_linux.go @@ -51,12 +51,13 @@ type ( // configuration info for the "bridge" driver. type configuration struct { - EnableIPForwarding bool - EnableIPTables bool - EnableIP6Tables bool - EnableUserlandProxy bool - UserlandProxyPath string - Rootless bool + EnableIPForwarding bool + DisableFilterForwardDrop bool + EnableIPTables bool + EnableIP6Tables bool + EnableUserlandProxy bool + UserlandProxyPath string + Rootless bool } // networkConfiguration for network specific configuration @@ -907,12 +908,12 @@ func (d *driver) createNetwork(config *networkConfiguration) (err error) { // Enable IP Forwarding {config.EnableIPv4 && d.config.EnableIPForwarding, func(*networkConfiguration, *bridgeInterface) error { - return setupIPv4Forwarding(d.config.EnableIPTables) + return setupIPv4Forwarding(d.config.EnableIPTables && !d.config.DisableFilterForwardDrop) }, }, {config.EnableIPv6 && d.config.EnableIPForwarding, func(*networkConfiguration, *bridgeInterface) error { - return setupIPv6Forwarding(d.config.EnableIP6Tables) + return setupIPv6Forwarding(d.config.EnableIP6Tables && !d.config.DisableFilterForwardDrop) }, }, diff --git a/man/dockerd.8.md b/man/dockerd.8.md index 3bb226a212..96c0c9033c 100644 --- a/man/dockerd.8.md +++ b/man/dockerd.8.md @@ -44,6 +44,7 @@ dockerd - Enable daemon mode [**--insecure-registry**[=*[]*]] [**--ip**[=*0.0.0.0*]] [**--ip-forward**[=**true**]] +[**--ip-forward-no-drop**[=**true**]] [**--ip-masq**[=**true**]] [**--iptables**[=**true**]] [**--ipv6**] @@ -289,11 +290,20 @@ unix://[/path/to/socket] to use. has no effect. This setting will also enable IPv6 forwarding if you have both - **--ip-forward=true** and **--fixed-cidr-v6** set. Note that this may reject - Router Advertisements and interfere with the host's existing IPv6 + **--ip-forward=true** and an IPv6 enabled bridge network. Note that this + may reject Router Advertisements and interfere with the host's existing IPv6 configuration. For more information, consult the documentation about "Advanced Networking - IPv6". +**--ip-forward-no-drop**=**true**|**false** + When **false**, the default, if Docker enables IP forwarding itself (see + **--ip-forward**), and **--iptables** or **--ip6tables** are enabled, it + also sets the default policy for the FORWARD chain in the iptables or + ip6tables filter table to DROP. + + When **true**, and when IP forwarding is already enabled, Docker does + not modify the default policy of the FORWARD chain. + **--ip-masq**=**true**|**false** Enable IP masquerading for bridge's IP range. Default is **true**. From 3dea9fd4e61c4a1afa39165783065cd9b83fd113 Mon Sep 17 00:00:00 2001 From: Rob Murray Date: Tue, 22 Oct 2024 11:43:48 +0100 Subject: [PATCH 5/5] Add TestFilterForwardPolicy Signed-off-by: Rob Murray --- .../network/bridge/bridge_linux_test.go | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) diff --git a/integration/network/bridge/bridge_linux_test.go b/integration/network/bridge/bridge_linux_test.go index 7b455f8d01..7d5da6f905 100644 --- a/integration/network/bridge/bridge_linux_test.go +++ b/integration/network/bridge/bridge_linux_test.go @@ -2,6 +2,7 @@ package bridge import ( "context" + "fmt" "net/netip" "strings" "testing" @@ -12,9 +13,11 @@ import ( "github.com/docker/docker/api/types/versions" ctr "github.com/docker/docker/integration/internal/container" "github.com/docker/docker/integration/internal/network" + "github.com/docker/docker/internal/testutils/networking" "github.com/docker/docker/testutil" "github.com/docker/docker/testutil/daemon" "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" "gotest.tools/v3/skip" ) @@ -193,3 +196,123 @@ func TestIPRangeAt64BitLimit(t *testing.T) { }) } } + +// TestFilterForwardPolicy tests that, if the daemon enables IP forwarding on the +// host, it also sets the iptables filter-FORWARD policy to DROP (unless it's +// told not to). +func TestFilterForwardPolicy(t *testing.T) { + skip.If(t, testEnv.IsRootless, "rootless has its own netns") + ctx := setupTest(t) + + // Set up a netns for each test to avoid sysctl and iptables pollution. + addr4 := netip.MustParseAddr("192.168.125.1") + addr6 := netip.MustParseAddr("fd76:c828:41f9::1") + l3 := networking.NewL3Segment(t, "test-ffp", + netip.PrefixFrom(addr4, 24), + netip.PrefixFrom(addr6, 64), + ) + t.Cleanup(func() { l3.Destroy(t) }) + + testcases := []struct { + name string + initForwarding string + daemonArgs []string + expForwarding string + expPolicy string + }{ + { + name: "enable forwarding", + initForwarding: "0", + expForwarding: "1", + expPolicy: "DROP", + }, + { + name: "forwarding already enabled", + initForwarding: "1", + expForwarding: "1", + expPolicy: "ACCEPT", + }, + { + name: "no drop", + initForwarding: "0", + daemonArgs: []string{"--ip-forward-no-drop"}, + expForwarding: "1", + expPolicy: "ACCEPT", + }, + { + name: "no forwarding", + initForwarding: "0", + daemonArgs: []string{"--ip-forward=false"}, + expForwarding: "0", + expPolicy: "ACCEPT", + }, + } + + for i, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + ctx := testutil.StartSpan(ctx, t) + + // Create a netns for this test. + addr4, addr6 = addr4.Next(), addr6.Next() + hostname := fmt.Sprintf("docker%d", i) + l3.AddHost(t, hostname, hostname+"-host", "eth0", + netip.PrefixFrom(addr4, 24), + netip.PrefixFrom(addr6, 64), + ) + host := l3.Hosts[hostname] + + getFwdPolicy := func(cmd string) string { + t.Helper() + out := host.Run(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 } + getSysctls := func() sysctls { + t.Helper() + return sysctls{ + host.Run(t, "sysctl", "-n", "net.ipv4.ip_forward")[:1], + host.Run(t, "sysctl", "-n", "net.ipv6.conf.default.forwarding")[:1], + host.Run(t, "sysctl", "-n", "net.ipv6.conf.all.forwarding")[:1], + } + } + + // Initial settings for IP forwarding params. + host.Run(t, "sysctl", "-w", "net.ipv4.ip_forward="+tc.initForwarding) + host.Run(t, "sysctl", "-w", "net.ipv6.conf.all.forwarding="+tc.initForwarding) + + // Start the daemon in its own network namespace. + var d *daemon.Daemon + host.Do(t, func() { + // Run without OTel because there's no routing from this netns for it - which + // means the daemon doesn't shut down cleanly, causing the test to fail. + d = daemon.New(t, daemon.WithEnvVars("OTEL_EXPORTER_OTLP_ENDPOINT=")) + d.StartWithBusybox(ctx, t, tc.daemonArgs...) + t.Cleanup(func() { d.Stop(t) }) + }) + 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("iptables"), tc.expPolicy)) + // IPv6 policy should not have been updated yet. + 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("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})) + }) + } +}