Limited bridge netfilter application. #2497

tomkcook · 2020-01-07T15:48:42Z

GordonTheTurtle · 2020-01-07T15:48:45Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "limited-bridge-netfilter" [email protected]:tomkcook/libnetwork.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

arkodg · 2020-01-07T17:39:28Z

Thanks for raising a PR @tomkcook
Can you please

Sign your PR using the steps mentioned above
Elaborate what you did, and why you did it in the commit message (by sharing the links you had shared in the Issue)

arkodg · 2020-01-07T17:40:52Z

drivers/bridge/setup_bridgenetfiltering.go

@@ -64,11 +64,12 @@ func checkBridgeNetFiltering(config *networkConfiguration, i *bridgeInterface) e
 if err != nil {
 logrus.Warnf("failed to check %s forwarding: %v", ipVerName, err)
 } else if enabled {
- enabled, err := getKernelBoolParam(getBridgeNFKernelParam(ipVer))
+ bridgeName := i.Link.Attrs().Name


I see that we already have something similar in https://github.com/docker/libnetwork/blob/feeff4f0a3fd2a2bb19cf67c826082c66ffaaed9/drivers/bridge/setup_bridgenetfiltering.go#L55
Can we use any one way of deriving the bridgeName and reference it everywhere else

Done. It got mixed up with the requested signoff and commit comment changes so has ended up in the same commit, apologies.

libnetwork uses the br-netfilter module to allow filtering of packets passing through a bridge. To do so, it sets /proc/sys/net/bridge/bridge-nf-call-ip[6]tables to 1, forcing iptables for every bridge on the system, whether this is desired or not. This overrides anything set in /etc/sysctl.conf. This change prevents libnetwork from setting the system-wide /proc/sys/net/bridge/bridge-nf-call-ip[6]tables and instead sets /sys/class/net/<bridge-name>/bridge/nf_call_ip[6]tables for each bridge which has ICC disabled. Note that this does introduce a change in the behaviour of docker. For a default network configuration, with the existing behaviour, both `docker_gwbridge` and `docker0` bridges have iptables enabled while this change results in `docker_gwbridge` having iptables enabled but `docker0` having iptables disabled, because ICC is enabled by default. As far as I can tell, iptables should not be enabled on the `docker0` bridge when ICC is enabled (the code which implements this seems to assume that iptables is enabled per-bridge and not systemwide) so I think this change is correct, but it is still a change in behaviour. Signed-off-by: Tom Cook <[email protected]>

tomkcook · 2020-01-08T17:27:02Z

Thanks for raising a PR @tomkcook
Can you please

1. Sign your PR using the steps mentioned above

2. Elaborate what you did, and why you did it in the commit message (by sharing the links you had shared in the Issue)

Done.

arkodg · 2020-01-08T18:10:33Z

these changes look good @tomkcook
we might need to remove the check you brought up earlier https://github.com/docker/libnetwork/issues/2488#issuecomment-571746499
thoughts ? @euanh @selansen @thaJeztah

thaJeztah · 2020-01-14T12:57:30Z

drivers/bridge/setup_bridgenetfiltering.go

 switch ipVer {
 case ipv4:
- return "/proc/sys/net/bridge/bridge-nf-call-iptables"
+ return fmt.Sprintf("/sys/class/net/%s/bridge/nf_call_iptables", bridgeName)


Should probably use path.Join()

Suggested change

return fmt.Sprintf("/sys/class/net/%s/bridge/nf_call_iptables", bridgeName)

return path.Join("/sys/class/net", bridgeName, "bridge/nf_call_iptables")

I'm not exactly clear how one is better than the other; both versions still have assumptions about what the path separator is and, anyway, the whole thing is linux-specific. Does swapping to path.Join improve this in some way?

We're constructing a path, so we should use a function that's designed for that.

Looking at this, we should also validate bridgeName before use: this value is passed as a label, and we likely don't want ../../ to be used

thaJeztah · 2020-01-14T12:58:08Z

drivers/bridge/setup_bridgenetfiltering.go

 case ipv6:
- return "/proc/sys/net/bridge/bridge-nf-call-ip6tables"
+ return fmt.Sprintf("/sys/class/net/%s/bridge/nf_call_ip6tables", bridgeName)


Same here;

Suggested change

return fmt.Sprintf("/sys/class/net/%s/bridge/nf_call_ip6tables", bridgeName)

return path.Join("/sys/class/net", bridgeName, "bridge/nf_call_ip6tables")

thaJeztah · 2020-01-14T13:01:59Z

we might need to remove the check you brought up earlier moby/moby#47127 (comment)

Would that remove the --icc=false option entirely?

arkodg · 2020-01-14T18:25:02Z

@thaJeztah no, only suggesting we rewrite it to

{d.config.EnableIPTables, setupBridgeNetFiltering},)

thaJeztah · 2020-01-22T11:49:55Z

Unrelated to this PR directly, but I noticed that the i argument to checkBridgeNetFiltering() doesn't seem to be used;

func checkBridgeNetFiltering(config *networkConfiguration, i *bridgeInterface) error {

It was added in https://github.com/docker/libnetwork/pull/336/files, and looks to be because it needs to satisfy the setupStep interface; https://github.com/docker/libnetwork/blob/9f2286349b58b00cf98bd36aa3f78763d52e8c63/drivers/bridge/setup.go#L3 and passed a value during the setup; https://github.com/docker/libnetwork/blob/9f2286349b58b00cf98bd36aa3f78763d52e8c63/drivers/bridge/setup.go#L15-L22

(could probably be changed to _ *bridgeInterface to make it more explicit that it's unused)

arkodg · 2020-02-21T23:50:20Z

@tomkcook can incorporate https://github.com/docker/libnetwork/issues/2488#issuecomment-571746499 (remove the icc check) in this PR

cpuguy83 · 2021-06-18T22:27:23Z

Note we have migrated this codebase over to github.com/moby/moby/libnetwork.
We are not accepting PR's on this repo anymore except for backports to be included in moby 20.10

GordonTheTurtle added the dco/no label Jan 7, 2020

arkodg reviewed Jan 7, 2020

View reviewed changes

arkodg added the status/2-needs-code-review label Jan 7, 2020

tomkcook force-pushed the limited-bridge-netfilter branch from 2605b47 to 5aa4008 Compare January 8, 2020 17:17

GordonTheTurtle removed the dco/no label Jan 8, 2020

tomkcook force-pushed the limited-bridge-netfilter branch from 5aa4008 to 446b688 Compare January 8, 2020 17:25

thaJeztah requested changes Jan 14, 2020

View reviewed changes

corhere added the carry-to-mobymoby label Jan 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limited bridge netfilter application. #2497

Limited bridge netfilter application. #2497

tomkcook commented Jan 7, 2020

GordonTheTurtle commented Jan 7, 2020

arkodg commented Jan 7, 2020

arkodg Jan 7, 2020

tomkcook Jan 8, 2020

tomkcook commented Jan 8, 2020

arkodg commented Jan 8, 2020

thaJeztah Jan 14, 2020

tomkcook Jan 21, 2020

thaJeztah Jan 22, 2020

thaJeztah Jan 14, 2020

thaJeztah commented Jan 14, 2020

arkodg commented Jan 14, 2020

thaJeztah commented Jan 22, 2020

arkodg commented Feb 21, 2020

cpuguy83 commented Jun 18, 2021

	return fmt.Sprintf("/sys/class/net/%s/bridge/nf_call_iptables", bridgeName)
	return path.Join("/sys/class/net", bridgeName, "bridge/nf_call_iptables")

Limited bridge netfilter application. #2497

Are you sure you want to change the base?

Limited bridge netfilter application. #2497

Conversation

tomkcook commented Jan 7, 2020

GordonTheTurtle commented Jan 7, 2020

arkodg commented Jan 7, 2020

arkodg Jan 7, 2020

Choose a reason for hiding this comment

tomkcook Jan 8, 2020

Choose a reason for hiding this comment

tomkcook commented Jan 8, 2020

arkodg commented Jan 8, 2020

thaJeztah Jan 14, 2020

Choose a reason for hiding this comment

tomkcook Jan 21, 2020

Choose a reason for hiding this comment

thaJeztah Jan 22, 2020

Choose a reason for hiding this comment

thaJeztah Jan 14, 2020

Choose a reason for hiding this comment

thaJeztah commented Jan 14, 2020

arkodg commented Jan 14, 2020

thaJeztah commented Jan 22, 2020

arkodg commented Feb 21, 2020

cpuguy83 commented Jun 18, 2021