Add a filter chain to allow persistent rules #1675

wenjianhn · 2017-03-08T09:01:45Z

Allow users to configure firewall policies in way that persists
docker operations/restarts. Docker will not delete or modify any
pre-existing rules from the DOCKER-FORWARD-TOP filter chain. This
allows the user to create in advance any rules required to further
restrict access from/to the containers.

Fixes moby/moby/issues/29184
Fixes moby/moby/issues/23987
Related to moby/moby/issues/24848

Signed-off-by: Jacob Wen jian.w.wen@oracle.com

aboch · 2017-03-08T16:15:54Z

drivers/bridge/setup_ip_tables.go

@@ -13,6 +13,7 @@ import (
 const (
 	DockerChain    = "DOCKER"
 	IsolationChain = "DOCKER-ISOLATION"
+	fwdTopChain    = "DOCKER-FORWARD-TOP"


I would call this DOCKER-USER to specify it is left to the user to manage.

+1.

In service_linux.go:
// In the filter table FORWARD chain first rule should be to jump to INGRESS-CHAIN

aboch · 2017-03-08T16:25:41Z

Thanks @wenjianhn for the change.

I think the new DOCKE-USER chain, not being driver specific, should be installed by the libnetwork core and not bridge driver. Can you please move the logic into controller.go at the end of New() after the network drivers initialization is done.
Feel free to make ensureJumpRule and addReturnRule methods of the iptables pkg.

wenjianhn · 2017-03-09T10:28:49Z

@aboch Looks like we also need to use APPEND instead of INSERT when add rules in the drivers.
Otherwise, "-A FORWARD -j DOCKER-USER" will be in the bottom.

https://github.com/wenjianhn/libnetwork/tree/draft/docker-user-chain
Rules created by the above patch:

# Generated by iptables-save v1.4.21 on Thu Mar  9 09:57:38 2017
*filter
:INPUT ACCEPT [0:0]
:FORWARD ACCEPT [0:0]
:OUTPUT ACCEPT [0:0]
:DOCKER - [0:0]
:DOCKER-ISOLATION - [0:0]
:DOCKER-USER - [0:0]
-A FORWARD -j DOCKER-ISOLATION
-A FORWARD -o docker0 -j DOCKER
-A FORWARD -o docker0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
-A FORWARD -i docker0 ! -o docker0 -j ACCEPT
-A FORWARD -i docker0 -o docker0 -j ACCEPT
-A FORWARD -j DOCKER-USER
-A DOCKER-ISOLATION -j RETURN
-A DOCKER-USER -j RETURN
COMMIT
# Completed on Thu Mar  9 09:57:38 2017

aboch · 2017-03-10T23:48:57Z

For that, you would call in iptables.EnsureJumpRule("FORWARD", dockerUserChain) in controller.go after the call to c.addNetwork(network)

wenjianhn · 2017-03-13T08:21:38Z

@aboch The CI failed. It doesn't seem to be related to my changes.
Could you rebuild it?

aboch · 2017-03-13T14:14:46Z

@wenjianhn

test -z "$(golint ./... | grep -Ev 'vendor|.pb.go:' | tee /dev/stderr)"
iptables/iptables.go:469:1: exported function AddReturnRule should have comment or be unexported
iptables/iptables.go:487:1: comment on exported function EnsureJumpRule should be of the form "EnsureJumpRule ..."

those are the reason for the failure. Please add the comments.

aboch · 2017-03-13T16:05:17Z

iptables/iptables.go

+		return nil
+	}
+
+	err := RawCombinedOutput(append([]string{"-I", chain}, args...)...)


Logically this should be a "-A", the return rule is anyway expected to appear at the end of the chain.
The original function was coded with the notion it would be used right after chain creation, as it was a bridge private function.
Now that the function is a public method, we should not assume anything like that.

I know this is from the existing code.
But given we are touching it now, I think it is the right time to fix this.

Nice catch.

aboch · 2017-03-13T16:08:33Z

service_linux.go

+
+	err = iptables.EnsureJumpRule("FORWARD", userChain)
+	if err != nil {
+		logrus.Warnf("Failed to create the jump rule for %s: %v", userChain, err)


Failed to ensure the jump rule to %s: %v...

aboch · 2017-03-13T16:11:22Z

service_linux.go

@@ -260,6 +260,7 @@ func (sb *sandbox) rmLBBackend(ip, vip net.IP, fwMark uint32, ingressPorts []*Po
 	}
 }

+const userChain = "DOCKER-USER"


Let's keep this file for service related things only.
Please move this const and the arrangeUserFilterRule() function to controller.go or network.go

Done. Moved it to controller.go.

aboch · 2017-03-13T16:53:35Z

Thanks @wenjianhn
Also please invoke arrangeUserFilterRule() at https://github.com/docker/libnetwork/blob/master/controller.go#L746 after the calling arrangeUserFilterRule()

aboch · 2017-03-14T22:13:45Z

Thanks @wenjianhn

LGTM

aboch · 2017-04-28T17:00:48Z

Please rebase

wenjianhn · 2017-05-02T06:45:30Z

@aboch The test fail. It doesn't seem to be related to my changes.

go build -o "bin/dnet-$GOOS-$GOARCH" ./cmd/dnet
# github.com/docker/libnetwork/vendor/github.com/vishvananda/netns
vendor/github.com/vishvananda/netns/netns.go:27: undefined: syscall.Stat_t
vendor/github.com/vishvananda/netns/netns.go:28: undefined: syscall.Fstat
vendor/github.com/vishvananda/netns/netns.go:31: undefined: syscall.Fstat
vendor/github.com/vishvananda/netns/netns.go:39: undefined: syscall.Stat_t
vendor/github.com/vishvananda/netns/netns.go:43: undefined: syscall.Fstat
vendor/github.com/vishvananda/netns/netns.go:57: cannot use int(*ns) (type int) as type syscall.Handle in argument to syscall.Close
Makefile:53: recipe for target 'cross-local' failed
make: *** [cross-local] Error 2
make: *** [circle-ci-cross] Error 2
make circle-ci returned exit code 2

aboch · 2017-05-02T16:41:57Z

Yes, weird cross-compilation issue. No changes in that vendoring.
I will rebuild the ci.

stowns · 2017-05-12T15:47:01Z

Any updates on this issue? We're currently having to use a custom shell script for starting/restarting docker stacks to solve this problem.

aboch · 2017-05-12T16:06:22Z

@wenjianhn Please move the current definition of arrangeFilterRules into a linux specific file and add stubs for it for non linux platforms

sanimej · 2017-05-15T07:00:52Z

controller.go

@@ -841,6 +845,7 @@ addToStore:
 	if !c.isDistributedControl() {
 		c.Lock()
 		arrangeIngressFilterRule()
+		arrangeUserFilterRule()
 		c.Unlock()
 	}



Instead of calling arrangeUserFilterRule two times can you move it after the if !c.isDistributedControl() { block ?

Nice catch. Thanks.

@wenjianhn arrangeIngressFilterRule() is done inside the !c.isDistributedControl() because it applies only in the swarm mode. But the user chain is needed in non-swarm mode as well. Can you add this after the if !c.isDistributedControl() block

c.Lock() arrangeUserFilterRule() c.Unlock()

@sanimej CircleCI timed out. Could you rebuild it?

sanimej · 2017-05-15T07:14:01Z

@wenjianhn This needs another rebase and couple of minor comments to be addressed. Can you please take care of that and we can merge it in time for 17.06. Sorry about the delay in the review.

wenjianhn · 2017-05-15T07:52:24Z

@sanimej Sure thing.

Allow users to configure firewall policies in a way that persists docker operations/restarts. Docker will not delete or modify any pre-existing rules from the DOCKER-USER filter chain. This allows the user to create in advance any rules required to further restrict access from/to the containers. Fixes moby/moby#29184 Fixes moby/moby#23987 Related to moby/moby#24848 Signed-off-by: Jacob Wen <jian.w.wen@oracle.com>

sanimej · 2017-05-17T05:47:54Z

Thanks @wenjianhn. LGTM

thaJeztah · 2017-05-17T10:42:48Z

@sanimej will this be in a vendor bump for 17.06?

peterthomassen · 2017-05-17T15:12:57Z

Thanks a lot for making this happen!

From the code comments, I gather that one has to add rules to the DOCKER-USER chain if they should be persistent.

Is that correct?
Is there already documentation for this? (I could not find any.)

sanimej · 2017-05-17T17:20:12Z

@thaJeztah We will do a vendor in later in the day.

@peterthomassen Yes, any custom rules you want to run before the docker added rules should be in the DOCKER-USER chain. This will be included in the 17.06 release notes/documentation.

mavenugo · 2017-05-17T17:21:14Z

@sanimej I have asked @aboch to take care of the vendoring with an existing open moby PR.

thaJeztah · 2017-06-07T12:59:41Z

ping @mstanleyjones this was included in docker 17.06 (through commit moby/moby@46392f2, which is part of moby/moby#32981), however, because it was part of another "changelog" issue, probably may not have been clear as a separate feature.

Given that this is a much-requested feature, we should include this somewhere in the documentation

Extra information: $ docker run -d -p 7777:6379 --name data1 redis $ docker run -d -p 8888:6379 --name data2 redis $ sudo iptables -N DOCKER-USER-redis1 $ sudo iptables -A DOCKER-USER-redis1 -s 192.168.56.0/24 -p tcp -m tcp -j RETURN $ sudo iptables -A DOCKER-USER-redis1 -j REJECT --reject-with icmp-port-unreachable $ sudo iptables -N DOCKER-USER-redis2 $ sudo iptables -A DOCKER-USER-redis2 -s 10.0.24.0/24 -p tcp -m tcp -j RETURN $ sudo iptables -A DOCKER-USER-redis2 -j REJECT --reject-with icmp-port-unreachable $ sudo iptables -A DOCKER-USER -i eth0 -p tcp -m conntrack --ctorigdstport 7777 -j DOCKER-USER-redis1 $ sudo iptables -A DOCKER-USER -i eth0 -p tcp -m conntrack --ctorigdstport 8888 -j DOCKER-USER-redis2 "I think an example like this belongs in the docs as it probably covers what 99% of users are looking for: the ability to expose ports using `-p` but still be able to control traffic to them using common filters like `-s`." Note that --ctorigdstport matches the original destination port of the first packet of the connection, not the packet being filtered. So the dropping rule will also drop responses to the outgoing connections from Docker to the world on 5000-9999! Add --ctdir ORIGINAL to the DROP rule to match only incoming packets. See github.com/moby/moby/issues/22054#issuecomment-466663033 You can also specify which chains docker should use. For example, in the filter table, specify another chain instead of `FORWARD`. This allows you to use traditional tools to manage the firewall and decide when to pass control to docker. Information pulled from: moby/moby#33567 https://unrouted.io/2017/08/15/docker-firewall/ moby/libnetwork#1675

azeemdin · 2019-05-28T13:07:56Z

I have "iptables": false in /etc/docker/daemon.json. I am using docker swarm mode. But docker is adding DOCKER-INGRESS iptables rules. Is there any workaround to avoid this?

aboch reviewed Mar 8, 2017

View reviewed changes

wenjianhn force-pushed the forward-top branch from 3f20f3a to 4feb41e Compare March 13, 2017 07:53

aboch reviewed Mar 13, 2017

View reviewed changes

wenjianhn force-pushed the forward-top branch from 4feb41e to c13d4f9 Compare March 14, 2017 06:47

This was referenced Mar 15, 2017

Docker and Iptables false moby/moby#28241

Closed

Docker and Firewalls such as IPTables moby/moby#29312

Closed

aboch mentioned this pull request Mar 29, 2017

Use IPVS for nating instead of iptables #1379

Closed

wenjianhn force-pushed the forward-top branch from c13d4f9 to ab114c5 Compare May 2, 2017 05:10

wenjianhn force-pushed the forward-top branch from ab114c5 to a60d046 Compare May 15, 2017 06:48

sanimej reviewed May 15, 2017

View reviewed changes

wenjianhn force-pushed the forward-top branch from a60d046 to 266986e Compare May 15, 2017 07:48

wenjianhn force-pushed the forward-top branch from 266986e to 0067b3a Compare May 16, 2017 02:25

sanimej merged commit b2bc1a6 into moby:master May 17, 2017

wenjianhn deleted the forward-top branch May 17, 2017 05:48

This was referenced May 17, 2017

docker does not respect previously setup FORWARD rules moby/moby#29184

Closed

Docker should not update FORWARD chain on startup moby/moby#23987

Closed

stszap mentioned this pull request May 24, 2017

Set iptables rules at startup rancher/os#446

Closed

thaJeztah mentioned this pull request Jun 7, 2017

Enable custom firewall rules moby/moby#33567

Open

mdlinville mentioned this pull request Jun 7, 2017

Document filter chain for persistent iptables rules docker/docs#3518

Closed

paigehargrave mentioned this pull request Feb 26, 2019

Attempt to add static iptables rules for DOCKER-USER on CentOS 7 docker/docs#8357

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a filter chain to allow persistent rules #1675

Add a filter chain to allow persistent rules #1675

wenjianhn commented Mar 8, 2017

aboch Mar 8, 2017

wenjianhn Mar 9, 2017

aboch commented Mar 8, 2017

wenjianhn commented Mar 9, 2017

aboch commented Mar 10, 2017

wenjianhn commented Mar 13, 2017

aboch commented Mar 13, 2017

aboch Mar 13, 2017

wenjianhn Mar 14, 2017

aboch Mar 13, 2017

wenjianhn Mar 14, 2017

aboch Mar 13, 2017

wenjianhn Mar 14, 2017

aboch commented Mar 13, 2017

aboch commented Mar 14, 2017

aboch commented Apr 28, 2017

wenjianhn commented May 2, 2017

aboch commented May 2, 2017

stowns commented May 12, 2017

aboch commented May 12, 2017 •

edited

Loading

sanimej May 15, 2017

wenjianhn May 15, 2017

sanimej May 15, 2017

wenjianhn May 16, 2017

wenjianhn May 16, 2017

sanimej commented May 15, 2017

wenjianhn commented May 15, 2017

sanimej commented May 17, 2017

thaJeztah commented May 17, 2017

peterthomassen commented May 17, 2017

sanimej commented May 17, 2017

mavenugo commented May 17, 2017

thaJeztah commented Jun 7, 2017

azeemdin commented May 28, 2019

Add a filter chain to allow persistent rules #1675

Add a filter chain to allow persistent rules #1675

Conversation

wenjianhn commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboch commented Mar 8, 2017

wenjianhn commented Mar 9, 2017

aboch commented Mar 10, 2017

wenjianhn commented Mar 13, 2017

aboch commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboch commented Mar 13, 2017

aboch commented Mar 14, 2017

aboch commented Apr 28, 2017

wenjianhn commented May 2, 2017

aboch commented May 2, 2017

stowns commented May 12, 2017

aboch commented May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanimej commented May 15, 2017

wenjianhn commented May 15, 2017

sanimej commented May 17, 2017

thaJeztah commented May 17, 2017

peterthomassen commented May 17, 2017

sanimej commented May 17, 2017

mavenugo commented May 17, 2017

thaJeztah commented Jun 7, 2017

azeemdin commented May 28, 2019

aboch commented May 12, 2017 •

edited

Loading