firewall: new plugin which adds allow rules for container IPs to firewalls #75

dcbw · 2017-09-29T21:39:04Z

Distros often have additional rules in the filter table that do things like:

-A FORWARD -j REJECT --reject-with icmp-host-prohibited

docker, for example, gets around this by adding explicit rules to the filter
table's FORWARD chain to allow traffic from the docker0 interface. Do that
for a given host interface too, as a chained plugin. eg:

{
    "cniVersion": "0.3.1",
    "name": "bridge-thing",
    "plugins": [
      {
        "type": "bridge",
        "bridge": "cni0",
        "isGateway": true,
        "ipMasq": true,
        "ipam": {
            "type": "host-local",
            "subnet": "10.88.0.0/16",
            "routes": [
                { "dst": "0.0.0.0/0" }
            ]
        }
      },
      {
        "type": "firewall"
      }
    ]
}

@containernetworking/cni-maintainers @squeed @danwinship

dcbw · 2017-09-29T22:00:26Z

For the record, the code adds rules like:

*filter
:FORWARD ACCEPT [0:0]
:CNI-ADMIN-cni0 - [0:0]
:CNI-FORWARD-cni0 - [0:0]
-A FORWARD -i cni0 ! -o cni0 -m comment --comment "CNI interface cni0 administrator overrides" -j CNI-ADMIN-cni0
-A FORWARD -m comment --comment "CNI interface cni0 private rules" -j CNI-FORWARD-cni0
-A CNI-FORWARD-cni0 -i cni0 ! -o cni0 -j ACCEPT
-A CNI-FORWARD-cni0 -i cni0 -o cni0 -j ACCEPT
-A CNI-FORWARD-cni0 -o cni0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT

squeed · 2017-10-02T12:27:29Z

Man, this has been on my mental to-do list for over a year. I might even add a firewalld mode to it :-)

squeed · 2017-10-02T15:02:22Z

If you're not going to delete, does it make sense to just idempotently create one allow rule per subnet?

squeed · 2017-10-02T15:46:49Z

plugins/meta/iptables-allow/iptables_allow.go

+	}
+
+	var intfName string
+	if iptConf.PrevResult != nil {


This looks like it's relying on ordering to pick the "bridge" interface. Does it make sense to also accept the ifname as a configuration hint?

@squeed yes, I'll do this as an option with fallback to the first PrevResult host interface. This will also make the plugin compatible with 0.1.0 and later too.

dcbw · 2017-10-05T17:19:15Z

If you're not going to delete, does it make sense to just idempotently create one allow rule per subnet?

@squeed We do kinda idempotently create one rule per bridge, which would also work if the bridge changes IP subnets which sometimes happens. If possible I'd like to stick with interface name. Is there a good reason to do IP net instead?

squeed · 2017-10-05T17:21:27Z

Disregard my comment about subnets; I'd misread the code.

squeed · 2017-10-05T17:23:48Z

plugins/meta/iptables-allow/iptables_allow.go

+		return fmt.Errorf("failed to initialize iptables helper: %v", err)
+	}
+
+	adminChainName := fmt.Sprintf("CNI-ADMIN-%s", intf)


Making this name overridable might be useful; admins might want to just have a single chain for interfaces or, as in the case of ptp, not have predictable if (and therefore chain) names.

@squeed you mean just one chain name for all admin overrides, or making it a conf option?

A conf option, of course :-)

dcbw · 2017-10-05T21:41:25Z

@squeed admin chain name override done.

matthewdupre · 2017-10-18T15:44:11Z

plugins/meta/iptables-allow/iptables_allow.go

+func getPrivChainRules(intf string) [][]string {
+	var rules [][]string
+	rules = append(rules, []string{"-i", intf, "!", "-o", intf, "-j", "ACCEPT"})
+	rules = append(rules, []string{"-i", intf, "-o", intf, "-j", "ACCEPT"})


Why do we need two rules here? Can't we just not match on -o at all?

@matthewdupre hmm, I mostly just copied what Docker was doing here; assuming they knew what they were doing.

@matthewdupre consolidated the two rules, you're right we don't need the double-match. The docker code that was adding both was doing it from two unrelated pieces of code which probably aren't aware of each other.

dcbw · 2017-11-20T20:20:34Z

@containernetworking/cni-maintainers any more comments on this?

squeed · 2017-11-21T16:08:52Z

Tested and it works well.

Could you add some mention in the README that the rules are not cleaned up?

I've been thinking about how this would work with just a ptp setup instead of a bridge, especially since we're considering doing that for kubenet. There might be a lot of pollution with no cleanup.

A few ideas for how to solve this:

Add the veths to a devgroup and match on devgroup
Match on CIDR instead of interface.

Thoughts? If you want to add this to a follow-up PR that'd be fine.

squeed · 2017-11-24T15:20:38Z

Nevermind, the solution is simple: just hard-code to match on interface veth+

dcbw · 2017-11-28T17:51:31Z

Nevermind, the solution is simple: just hard-code to match on interface veth+

@squeed can you explain a bit more?

squeed · 2017-11-28T18:23:18Z

So, imagine kubenet moves from bridge to ptp. Now we have one rule per interface, rather than one rule total (for the bridge). Given that this plugin doesn't do delete... things start to look pretty ugly pretty quickly.

I can think of two solutions to this: Add some kind of devicegroup allow mode that adds each interface to a devicegroup, then makes sure there is an allow rule for that group, or, use the "interface" parameter to match on a interface name wildcard. This is a bit of a problem for non-cni veths, though. Probably a bit too fragile.

dcbw · 2018-01-06T03:29:11Z

@squeed ok, how about the newly-pushed approach instead. It implements delete by checking whether the given interface is a master for any other interfaces (eg, a bridge). If so and there are no child/slave devices attached (since presumably the last one just got deleted) then DEL will clean up the iptables rules. Otherwise DEL leaves them alone.

If this seems worthwhile, we probably want to use this same approach in the bridge plugin's IPMasq code, since that also never cleans itself up.

dcbw · 2018-01-19T22:18:46Z

@squeed ok my approach is busted (and so would a devgroup one) because DEL runs plugins in reverse order, and so the bridge still has the port attached. Any suggestions?

Update: so it's not totally busted, because my approach does get the container interface name and we can look at IFLA_LINK on that interface for its peer ifindex, and ignore that peer when checking IFLA_MASTER attributes in the host namespace.

dcbw · 2018-01-22T23:22:26Z

@squeed updated to exclude a container's peer when checking if the bridge has interfaces. Also removed the PrevResult stuff, since on DEL we don't have a PrevResult to pull the host ifname from, since plugins are executed in reverse order.

I guess we could try caching that somewhere from the ADD call, but for now you have to specify the bridge interface for bridge, and you might as well just copy that for iptables-allow since you know it already.

rosenhouse · 2018-01-23T00:45:21Z

@dcbw what do you think about extracting the "find peer interface" logic into a public library function? that way we could use it in #96 too (specifically for this issue )

vadorovsky · 2018-05-28T12:26:06Z

Looks good to me

bboreham · 2018-06-19T09:16:17Z

What if the system has INPUT default policy set to DROP ?

dcbw · 2018-06-19T20:42:23Z

@bboreham AFAICT even docker 1.13 doesn't add anything to the nat+INPUT chain, just FORWARD. So we're doing nothing different here than docker already. I'd imagine INPUT defaulting to DROP would just kill all your return traffic :)

bboreham · 2018-06-19T22:50:39Z

Docker didn't expose services from the container network; the approach was to map ports out to the host network. I'm thinking about Kubernetes, where services do live on the container network.

Imagine a system where the admin has carefully set things up to reject incoming packets from anywhere outside a defined set of rules. Shouldn't our firewall plugin allow for this case, and add rules to the INPUT chain?

dcbw · 2018-06-20T15:18:15Z

@bboreham added #168 for the INPUT chain issue.

rhatdan · 2018-07-12T21:26:17Z

Any progress on this? This is a blocking issue for podman.

vadorovsky · 2018-07-12T21:53:58Z

Is #168 a blocking issue for this PR or should it be solved as a follow-up?

ashcrow · 2018-09-10T13:50:20Z

Any progress here? Our QE folks are blocked from merging new OS level CI tests for podman.

mheon · 2018-09-10T13:51:11Z

@ashcrow containers/podman#1431

ashcrow · 2018-09-10T13:52:16Z

@mheon thanks! I assume that supersedes this PR?

mheon · 2018-09-10T13:54:19Z

@ashcrow It's more of a temporary solution until this can be rewritten and merged, which could be some months still

ashcrow · 2018-09-10T13:54:49Z

@mheon makes sense. Thanks!

rhatdan · 2019-01-10T20:39:43Z

Any update on this?

dcbw · 2019-01-16T16:21:36Z

@rhatdan still waiting on the CHECK changes that @mccv1r0 is working on. It's still in progress.

znmeb · 2019-02-07T05:41:24Z

@dcbw Is there a workaround for this I can implement on Silverblue 29 via firewall-config?

gbraad · 2019-02-15T07:00:55Z

What is the status of this?

dcbw · 2019-04-01T20:16:11Z

plugins/meta/firewall/firewall.go

+	}
+
+	// Tolerate errors if the container namespace has been torn down already
+	containerNS, err := ns.GetNS(args.Netns)


This plugin shouldn't are about the network namespace, right?

dcbw · 2019-04-12T19:15:39Z

This PR was updated and pushed as #290

dcbw force-pushed the ipt-fw-allow-interface branch from 8f2c100 to 1619a37 Compare September 29, 2017 21:58

squeed reviewed Oct 2, 2017

View reviewed changes

squeed mentioned this pull request Oct 5, 2017

Upgrading docker 1.13 on nodes causes outbound container traffic to stop working kubernetes/kubernetes#40182

Closed

squeed reviewed Oct 5, 2017

View reviewed changes

dcbw force-pushed the ipt-fw-allow-interface branch 3 times, most recently from 213212f to 67c2d9d Compare October 5, 2017 21:40

jeffmhastings mentioned this pull request Oct 18, 2017

IPTables rules missing from Flannel/CNI on Kubernetes installation flannel-io/flannel#799

Closed

matthewdupre reviewed Oct 18, 2017

View reviewed changes

dcbw force-pushed the ipt-fw-allow-interface branch from 67c2d9d to 22f3da1 Compare November 7, 2017 22:06

dcbw force-pushed the ipt-fw-allow-interface branch 2 times, most recently from 3bd99fa to 50bbfa1 Compare January 6, 2018 03:27

dcbw force-pushed the ipt-fw-allow-interface branch from 50bbfa1 to 66f5a8a Compare January 6, 2018 03:31

dcbw force-pushed the ipt-fw-allow-interface branch 2 times, most recently from 79b6192 to a0691a7 Compare January 22, 2018 23:21

vadorovsky approved these changes May 28, 2018

View reviewed changes

dcbw requested review from rosenhouse and bboreham June 12, 2018 20:31

mheon mentioned this pull request Jun 20, 2018

no network traffic forwarded when using podman run on Atomic Host systems containers/podman#973

Closed

miabbott mentioned this pull request Jul 24, 2018

introduce podman sanity checks projectatomic/atomic-host-tests#417

Merged

mheon mentioned this pull request Sep 9, 2018

Integrate CNI firewall plugin code containers/podman#1431

Closed

grahamwhaley mentioned this pull request Sep 24, 2018

docs: Clean up k8s with cri-containerd howto kata-containers/documentation#251

Merged

giuseppe mentioned this pull request Oct 2, 2018

cri-o needs cni workaround cri-o/cri-o#1804

Closed

dcbw commented Apr 1, 2019

View reviewed changes

mccv1r0 mentioned this pull request Apr 8, 2019

Cannot access forwarded ports remotely (again?) containers/podman#2748

Closed

dcbw closed this Apr 12, 2019

fiws mentioned this pull request Aug 15, 2019

bridge: no outside connectivity #368

Closed

chanwit mentioned this pull request Sep 16, 2019

add forward rules for the bridge device weaveworks/ignite#427

Closed

firewall: new plugin which adds allow rules for container IPs to firewalls #75

firewall: new plugin which adds allow rules for container IPs to firewalls #75

Conversation

dcbw commented Sep 29, 2017 • edited Loading

dcbw commented Sep 29, 2017

squeed commented Oct 2, 2017

squeed commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbw commented Oct 5, 2017

squeed commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbw commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbw commented Nov 20, 2017

squeed commented Nov 21, 2017

squeed commented Nov 24, 2017

dcbw commented Nov 28, 2017

squeed commented Nov 28, 2017

dcbw commented Jan 6, 2018

dcbw commented Jan 19, 2018 • edited Loading

dcbw commented Jan 22, 2018

rosenhouse commented Jan 23, 2018

vadorovsky commented May 28, 2018

bboreham commented Jun 19, 2018

dcbw commented Jun 19, 2018 • edited Loading

bboreham commented Jun 19, 2018

dcbw commented Jun 20, 2018

rhatdan commented Jul 12, 2018

vadorovsky commented Jul 12, 2018 • edited Loading

ashcrow commented Sep 10, 2018

mheon commented Sep 10, 2018

ashcrow commented Sep 10, 2018

mheon commented Sep 10, 2018

ashcrow commented Sep 10, 2018

rhatdan commented Jan 10, 2019

dcbw commented Jan 16, 2019

znmeb commented Feb 7, 2019

gbraad commented Feb 15, 2019

Choose a reason for hiding this comment

dcbw commented Apr 12, 2019

dcbw commented Sep 29, 2017 •

edited

Loading

dcbw commented Jan 19, 2018 •

edited

Loading

dcbw commented Jun 19, 2018 •

edited

Loading

vadorovsky commented Jul 12, 2018 •

edited

Loading