New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a filter chain to allow persistent rules #1675

Merged
merged 1 commit into from May 17, 2017

Conversation

Projects
None yet
7 participants
@wenjianhn
Copy link
Contributor

wenjianhn commented Mar 8, 2017

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

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

This comment has been minimized.

@aboch

aboch Mar 8, 2017

Contributor

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

This comment has been minimized.

@wenjianhn

wenjianhn Mar 9, 2017

Author Contributor

+1.

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

@aboch

This comment has been minimized.

Copy link
Contributor

aboch commented Mar 8, 2017

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

This comment has been minimized.

Copy link
Contributor Author

wenjianhn commented Mar 9, 2017

@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

This comment has been minimized.

Copy link
Contributor

aboch commented Mar 10, 2017

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

@wenjianhn wenjianhn force-pushed the wenjianhn:forward-top branch from 3f20f3a to 4feb41e Mar 13, 2017

@wenjianhn

This comment has been minimized.

Copy link
Contributor Author

wenjianhn commented Mar 13, 2017

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

@aboch

This comment has been minimized.

Copy link
Contributor

aboch commented Mar 13, 2017

@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.

return nil
}

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

This comment has been minimized.

@aboch

aboch Mar 13, 2017

Contributor

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.

This comment has been minimized.

@wenjianhn

wenjianhn Mar 14, 2017

Author Contributor

Nice catch.


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

This comment has been minimized.

@aboch

aboch Mar 13, 2017

Contributor

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

This comment has been minimized.

@wenjianhn

wenjianhn Mar 14, 2017

Author Contributor

Done

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

const userChain = "DOCKER-USER"

This comment has been minimized.

@aboch

aboch Mar 13, 2017

Contributor

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

This comment has been minimized.

@wenjianhn

wenjianhn Mar 14, 2017

Author Contributor

Done. Moved it to controller.go.

@aboch

This comment has been minimized.

Copy link
Contributor

aboch commented Mar 13, 2017

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

@wenjianhn wenjianhn force-pushed the wenjianhn:forward-top branch from 4feb41e to c13d4f9 Mar 14, 2017

@aboch

This comment has been minimized.

Copy link
Contributor

aboch commented Mar 14, 2017

Thanks @wenjianhn

LGTM

@aboch

This comment has been minimized.

Copy link
Contributor

aboch commented Apr 28, 2017

Please rebase

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

@wenjianhn

This comment has been minimized.

Copy link
Contributor Author

wenjianhn commented May 2, 2017

@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

This comment has been minimized.

Copy link
Contributor

aboch commented May 2, 2017

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

@stowns

This comment has been minimized.

Copy link

stowns commented May 12, 2017

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

This comment has been minimized.

Copy link
Contributor

aboch commented May 12, 2017

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

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

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

This comment has been minimized.

@sanimej

sanimej May 15, 2017

Contributor

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

This comment has been minimized.

@wenjianhn

wenjianhn May 15, 2017

Author Contributor

Nice catch. Thanks.

This comment has been minimized.

@sanimej

sanimej May 15, 2017

Contributor

@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()

This comment has been minimized.

@wenjianhn

wenjianhn May 16, 2017

Author Contributor

Done

This comment has been minimized.

@wenjianhn

wenjianhn May 16, 2017

Author Contributor

@sanimej CircleCI timed out. Could you rebuild it?

@sanimej

This comment has been minimized.

Copy link
Contributor

sanimej commented May 15, 2017

@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 wenjianhn force-pushed the wenjianhn:forward-top branch from a60d046 to 266986e May 15, 2017

@wenjianhn

This comment has been minimized.

Copy link
Contributor Author

wenjianhn commented May 15, 2017

@sanimej Sure thing.

Add a filter chain to allow persistent rules
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>

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

@sanimej

This comment has been minimized.

Copy link
Contributor

sanimej commented May 17, 2017

Thanks @wenjianhn. LGTM

@sanimej sanimej merged commit b2bc1a6 into docker:master May 17, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
dco-signed All commits are signed

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

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented May 17, 2017

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

@peterthomassen

This comment has been minimized.

Copy link

peterthomassen commented May 17, 2017

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

This comment has been minimized.

Copy link
Contributor

sanimej commented May 17, 2017

@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

This comment has been minimized.

Copy link
Contributor

mavenugo commented May 17, 2017

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

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jun 7, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment