From f8fcb3525fc5c4a2bdc1a791b5c7b46a29ef4f04 Mon Sep 17 00:00:00 2001 From: Casey Callendrello Date: Tue, 26 Feb 2019 11:43:35 +0100 Subject: [PATCH] Portmap: append, rather than prepend, entry rules This means that portmapped connections can be more easily controlled / firewalled. --- plugins/meta/portmap/chain.go | 23 ++++++++++++++-------- plugins/meta/portmap/chain_test.go | 2 +- plugins/meta/portmap/portmap.go | 4 ++++ plugins/meta/portmap/portmap_integ_test.go | 6 ++++++ plugins/meta/portmap/portmap_test.go | 1 + 5 files changed, 27 insertions(+), 9 deletions(-) diff --git a/plugins/meta/portmap/chain.go b/plugins/meta/portmap/chain.go index 5ebfe6a31..60bf8a099 100644 --- a/plugins/meta/portmap/chain.go +++ b/plugins/meta/portmap/chain.go @@ -29,6 +29,8 @@ type chain struct { entryRules [][]string // the rules that "point" to this chain rules [][]string // the rules this chain contains + + prependEntry bool // whether or not the entry rules should be prepended } // setup idempotently creates the chain. It will not error if the chain exists. @@ -45,19 +47,19 @@ func (c *chain) setup(ipt *iptables.IPTables) error { } // Add the rules to the chain - for i := len(c.rules) - 1; i >= 0; i-- { - if err := prependUnique(ipt, c.table, c.name, c.rules[i]); err != nil { + for _, rule := range c.rules { + if err := insertUnique(ipt, c.table, c.name, false, rule); err != nil { return err } } // Add the entry rules to the entry chains for _, entryChain := range c.entryChains { - for i := len(c.entryRules) - 1; i >= 0; i-- { + for _, rule := range c.entryRules { r := []string{} - r = append(r, c.entryRules[i]...) + r = append(r, rule...) r = append(r, "-j", c.name) - if err := prependUnique(ipt, c.table, entryChain, r); err != nil { + if err := insertUnique(ipt, c.table, entryChain, c.prependEntry, r); err != nil { return err } } @@ -105,8 +107,9 @@ func (c *chain) teardown(ipt *iptables.IPTables) error { return nil } -// prependUnique will prepend a rule to a chain, if it does not already exist -func prependUnique(ipt *iptables.IPTables, table, chain string, rule []string) error { +// insertUnique will add a rule to a chain if it does not already exist. +// By default the rule is appended, unless prepend is true. +func insertUnique(ipt *iptables.IPTables, table, chain string, prepend bool, rule []string) error { exists, err := ipt.Exists(table, chain, rule...) if err != nil { return err @@ -115,7 +118,11 @@ func prependUnique(ipt *iptables.IPTables, table, chain string, rule []string) e return nil } - return ipt.Insert(table, chain, 1, rule...) + if prepend { + return ipt.Insert(table, chain, 1, rule...) + } else { + return ipt.Append(table, chain, rule...) + } } func chainExists(ipt *iptables.IPTables, tableName, chainName string) (bool, error) { diff --git a/plugins/meta/portmap/chain_test.go b/plugins/meta/portmap/chain_test.go index bff85c12a..d4010b0c3 100644 --- a/plugins/meta/portmap/chain_test.go +++ b/plugins/meta/portmap/chain_test.go @@ -116,8 +116,8 @@ var _ = Describe("chain tests", func() { Expect(err).NotTo(HaveOccurred()) Expect(haveRules).To(Equal([]string{ "-N " + tlChainName, - "-A " + tlChainName + " -d 203.0.113.1/32 -j " + testChain.name, "-A " + tlChainName + ` -m comment --comment "canary value" -j ACCEPT`, + "-A " + tlChainName + " -d 203.0.113.1/32 -j " + testChain.name, })) // Check that the chain and rule was created diff --git a/plugins/meta/portmap/portmap.go b/plugins/meta/portmap/portmap.go index 870552f63..e57622b6d 100644 --- a/plugins/meta/portmap/portmap.go +++ b/plugins/meta/portmap/portmap.go @@ -255,6 +255,10 @@ func genMarkMasqChain(markBit int) chain { table: "nat", name: MarkMasqChainName, entryChains: []string{"POSTROUTING"}, + // Only this entry chain needs to be prepended, because otherwise it is + // stomped on by the masquerading rules created by the CNI ptp and bridge + // plugins. + prependEntry: true, entryRules: [][]string{{ "-m", "comment", "--comment", "CNI portfwd requiring masquerade", diff --git a/plugins/meta/portmap/portmap_integ_test.go b/plugins/meta/portmap/portmap_integ_test.go index 7bd9e0cf0..6d0f4faf0 100644 --- a/plugins/meta/portmap/portmap_integ_test.go +++ b/plugins/meta/portmap/portmap_integ_test.go @@ -163,6 +163,12 @@ var _ = Describe("portmap integration tests", func() { fmt.Fprintf(GinkgoWriter, "hostIP: %s:%d, contIP: %s:%d\n", hostIP, hostPort, contIP, containerPort) + // dump iptables-save output for debugging + cmd = exec.Command("iptables-save") + cmd.Stderr = GinkgoWriter + cmd.Stdout = GinkgoWriter + Expect(cmd.Run()).To(Succeed()) + // Sanity check: verify that the container is reachable directly contOK := testEchoServer(contIP.String(), containerPort, "") diff --git a/plugins/meta/portmap/portmap_test.go b/plugins/meta/portmap/portmap_test.go index d47b483c2..44e2d9367 100644 --- a/plugins/meta/portmap/portmap_test.go +++ b/plugins/meta/portmap/portmap_test.go @@ -323,6 +323,7 @@ var _ = Describe("portmapping configuration", func() { "--mark", "0x20/0x20", "-j", "MASQUERADE", }}, + prependEntry: true, })) }) })