From 2781401caa29bedbd650ed06b2485535267e8106 Mon Sep 17 00:00:00 2001 From: Lars Kellogg-Stedman Date: Fri, 13 Feb 2015 22:10:14 -0500 Subject: [PATCH] fix various problems with iptables.Exists This modifies iptables.Exists so that it must be called with an explicit table and chain. This allows us (a) to generate an appropriate command line for "iptables -C", which was not previously possible, and (b) it allows us to limit our strings.Contains() search to just the table and chain in question, preventing erroneous matches against unrelated rules. Resolves #10781 Signed-off-by: Lars Kellogg-Stedman Upstream-commit: 3559b4177e611920d87c4dae607c641efb645783 Component: engine --- .../daemon/networkdriver/bridge/driver.go | 33 +++++++-------- .../integration-cli/docker_cli_links_test.go | 8 ++-- components/engine/pkg/iptables/iptables.go | 29 +++++++++----- .../engine/pkg/iptables/iptables_test.go | 40 ++++++++----------- 4 files changed, 56 insertions(+), 54 deletions(-) diff --git a/components/engine/daemon/networkdriver/bridge/driver.go b/components/engine/daemon/networkdriver/bridge/driver.go index 329052be41..0abfdff0c7 100644 --- a/components/engine/daemon/networkdriver/bridge/driver.go +++ b/components/engine/daemon/networkdriver/bridge/driver.go @@ -284,10 +284,11 @@ func setupIPTables(addr net.Addr, icc, ipmasq bool) error { // Enable NAT if ipmasq { - natArgs := []string{"POSTROUTING", "-t", "nat", "-s", addr.String(), "!", "-o", bridgeIface, "-j", "MASQUERADE"} + natArgs := []string{"-s", addr.String(), "!", "-o", bridgeIface, "-j", "MASQUERADE"} - if !iptables.Exists(natArgs...) { - if output, err := iptables.Raw(append([]string{"-I"}, natArgs...)...); err != nil { + if !iptables.Exists(iptables.Nat, "POSTROUTING", natArgs...) { + if output, err := iptables.Raw(append([]string{ + "-t", string(iptables.Nat), "-I", "POSTROUTING"}, natArgs...)...); err != nil { return fmt.Errorf("Unable to enable network bridge NAT: %s", err) } else if len(output) != 0 { return &iptables.ChainError{Chain: "POSTROUTING", Output: output} @@ -296,28 +297,28 @@ func setupIPTables(addr net.Addr, icc, ipmasq bool) error { } var ( - args = []string{"FORWARD", "-i", bridgeIface, "-o", bridgeIface, "-j"} + args = []string{"-i", bridgeIface, "-o", bridgeIface, "-j"} acceptArgs = append(args, "ACCEPT") dropArgs = append(args, "DROP") ) if !icc { - iptables.Raw(append([]string{"-D"}, acceptArgs...)...) + iptables.Raw(append([]string{"-D", "FORWARD"}, acceptArgs...)...) - if !iptables.Exists(dropArgs...) { + if !iptables.Exists(iptables.Filter, "FORWARD", dropArgs...) { log.Debugf("Disable inter-container communication") - if output, err := iptables.Raw(append([]string{"-I"}, dropArgs...)...); err != nil { + if output, err := iptables.Raw(append([]string{"-I", "FORWARD"}, dropArgs...)...); err != nil { return fmt.Errorf("Unable to prevent intercontainer communication: %s", err) } else if len(output) != 0 { return fmt.Errorf("Error disabling intercontainer communication: %s", output) } } } else { - iptables.Raw(append([]string{"-D"}, dropArgs...)...) + iptables.Raw(append([]string{"-D", "FORWARD"}, dropArgs...)...) - if !iptables.Exists(acceptArgs...) { + if !iptables.Exists(iptables.Filter, "FORWARD", acceptArgs...) { log.Debugf("Enable inter-container communication") - if output, err := iptables.Raw(append([]string{"-I"}, acceptArgs...)...); err != nil { + if output, err := iptables.Raw(append([]string{"-I", "FORWARD"}, acceptArgs...)...); err != nil { return fmt.Errorf("Unable to allow intercontainer communication: %s", err) } else if len(output) != 0 { return fmt.Errorf("Error enabling intercontainer communication: %s", output) @@ -326,9 +327,9 @@ func setupIPTables(addr net.Addr, icc, ipmasq bool) error { } // Accept all non-intercontainer outgoing packets - outgoingArgs := []string{"FORWARD", "-i", bridgeIface, "!", "-o", bridgeIface, "-j", "ACCEPT"} - if !iptables.Exists(outgoingArgs...) { - if output, err := iptables.Raw(append([]string{"-I"}, outgoingArgs...)...); err != nil { + outgoingArgs := []string{"-i", bridgeIface, "!", "-o", bridgeIface, "-j", "ACCEPT"} + if !iptables.Exists(iptables.Filter, "FORWARD", outgoingArgs...) { + if output, err := iptables.Raw(append([]string{"-I", "FORWARD"}, outgoingArgs...)...); err != nil { return fmt.Errorf("Unable to allow outgoing packets: %s", err) } else if len(output) != 0 { return &iptables.ChainError{Chain: "FORWARD outgoing", Output: output} @@ -336,10 +337,10 @@ func setupIPTables(addr net.Addr, icc, ipmasq bool) error { } // Accept incoming packets for existing connections - existingArgs := []string{"FORWARD", "-o", bridgeIface, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"} + existingArgs := []string{"-o", bridgeIface, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", "-j", "ACCEPT"} - if !iptables.Exists(existingArgs...) { - if output, err := iptables.Raw(append([]string{"-I"}, existingArgs...)...); err != nil { + if !iptables.Exists(iptables.Filter, "FORWARD", existingArgs...) { + if output, err := iptables.Raw(append([]string{"-I", "FORWARD"}, existingArgs...)...); err != nil { return fmt.Errorf("Unable to allow incoming packets: %s", err) } else if len(output) != 0 { return &iptables.ChainError{Chain: "FORWARD incoming", Output: output} diff --git a/components/engine/integration-cli/docker_cli_links_test.go b/components/engine/integration-cli/docker_cli_links_test.go index 47635a1942..efee8d04e3 100644 --- a/components/engine/integration-cli/docker_cli_links_test.go +++ b/components/engine/integration-cli/docker_cli_links_test.go @@ -132,14 +132,14 @@ func TestLinksIpTablesRulesWhenLinkAndUnlink(t *testing.T) { childIP := findContainerIP(t, "child") parentIP := findContainerIP(t, "parent") - sourceRule := []string{"DOCKER", "-i", "docker0", "-o", "docker0", "-p", "tcp", "-s", childIP, "--sport", "80", "-d", parentIP, "-j", "ACCEPT"} - destinationRule := []string{"DOCKER", "-i", "docker0", "-o", "docker0", "-p", "tcp", "-s", parentIP, "--dport", "80", "-d", childIP, "-j", "ACCEPT"} - if !iptables.Exists(sourceRule...) || !iptables.Exists(destinationRule...) { + sourceRule := []string{"-i", "docker0", "-o", "docker0", "-p", "tcp", "-s", childIP, "--sport", "80", "-d", parentIP, "-j", "ACCEPT"} + destinationRule := []string{"-i", "docker0", "-o", "docker0", "-p", "tcp", "-s", parentIP, "--dport", "80", "-d", childIP, "-j", "ACCEPT"} + if !iptables.Exists("filter", "DOCKER", sourceRule...) || !iptables.Exists("filter", "DOCKER", destinationRule...) { t.Fatal("Iptables rules not found") } dockerCmd(t, "rm", "--link", "parent/http") - if iptables.Exists(sourceRule...) || iptables.Exists(destinationRule...) { + if iptables.Exists("filter", "DOCKER", sourceRule...) || iptables.Exists("filter", "DOCKER", destinationRule...) { t.Fatal("Iptables rules should be removed when unlink") } diff --git a/components/engine/pkg/iptables/iptables.go b/components/engine/pkg/iptables/iptables.go index 010c99b15c..3e083a43ad 100644 --- a/components/engine/pkg/iptables/iptables.go +++ b/components/engine/pkg/iptables/iptables.go @@ -21,6 +21,7 @@ const ( Insert Action = "-I" Nat Table = "nat" Filter Table = "filter" + Mangle Table = "mangle" ) var ( @@ -82,7 +83,7 @@ func NewChain(name, bridge string, table Table) (*Chain, error) { preroute := []string{ "-m", "addrtype", "--dst-type", "LOCAL"} - if !Exists(preroute...) { + if !Exists(Nat, "PREROUTING", preroute...) { if err := c.Prerouting(Append, preroute...); err != nil { return nil, fmt.Errorf("Failed to inject docker in PREROUTING chain: %s", err) } @@ -91,17 +92,17 @@ func NewChain(name, bridge string, table Table) (*Chain, error) { "-m", "addrtype", "--dst-type", "LOCAL", "!", "--dst", "127.0.0.0/8"} - if !Exists(output...) { + if !Exists(Nat, "OUTPUT", output...) { if err := c.Output(Append, output...); err != nil { return nil, fmt.Errorf("Failed to inject docker in OUTPUT chain: %s", err) } } case Filter: - link := []string{"FORWARD", + link := []string{ "-o", c.Bridge, "-j", c.Name} - if !Exists(link...) { - insert := append([]string{string(Insert)}, link...) + if !Exists(Filter, "FORWARD", link...) { + insert := append([]string{string(Insert), "FORWARD"}, link...) if output, err := Raw(insert...); err != nil { return nil, err } else if len(output) != 0 { @@ -242,19 +243,25 @@ func (c *Chain) Remove() error { } // Check if a rule exists -func Exists(args ...string) bool { +func Exists(table Table, chain string, rule ...string) bool { + if string(table) == "" { + table = Filter + } + // iptables -C, --check option was added in v.1.4.11 // http://ftp.netfilter.org/pub/iptables/changes-iptables-1.4.11.txt // try -C // if exit status is 0 then return true, the rule exists - if _, err := Raw(append([]string{"-C"}, args...)...); err == nil { + if _, err := Raw(append([]string{ + "-t", string(table), "-C", chain}, rule...)...); err == nil { return true } - // parse iptables-save for the rule - rule := strings.Replace(strings.Join(args, " "), "-t nat ", "", -1) - existingRules, _ := exec.Command("iptables-save").Output() + // parse "iptables -S" for the rule (this checks rules in a specific chain + // in a specific table) + rule_string := strings.Join(rule, " ") + existingRules, _ := exec.Command("iptables", "-t", string(table), "-S", chain).Output() // regex to replace ips in rule // because MASQUERADE rule will not be exactly what was passed @@ -262,7 +269,7 @@ func Exists(args ...string) bool { return strings.Contains( re.ReplaceAllString(string(existingRules), "?"), - re.ReplaceAllString(rule, "?"), + re.ReplaceAllString(rule_string, "?"), ) } diff --git a/components/engine/pkg/iptables/iptables_test.go b/components/engine/pkg/iptables/iptables_test.go index 8aaf429c94..ced4262ce2 100644 --- a/components/engine/pkg/iptables/iptables_test.go +++ b/components/engine/pkg/iptables/iptables_test.go @@ -39,8 +39,7 @@ func TestForward(t *testing.T) { t.Fatal(err) } - dnatRule := []string{natChain.Name, - "-t", string(natChain.Table), + dnatRule := []string{ "!", "-i", filterChain.Bridge, "-d", ip.String(), "-p", proto, @@ -49,12 +48,11 @@ func TestForward(t *testing.T) { "--to-destination", dstAddr + ":" + strconv.Itoa(dstPort), } - if !Exists(dnatRule...) { + if !Exists(natChain.Table, natChain.Name, dnatRule...) { t.Fatalf("DNAT rule does not exist") } - filterRule := []string{filterChain.Name, - "-t", string(filterChain.Table), + filterRule := []string{ "!", "-i", filterChain.Bridge, "-o", filterChain.Bridge, "-d", dstAddr, @@ -63,12 +61,11 @@ func TestForward(t *testing.T) { "-j", "ACCEPT", } - if !Exists(filterRule...) { + if !Exists(filterChain.Table, filterChain.Name, filterRule...) { t.Fatalf("filter rule does not exist") } - masqRule := []string{"POSTROUTING", - "-t", string(natChain.Table), + masqRule := []string{ "-d", dstAddr, "-s", dstAddr, "-p", proto, @@ -76,7 +73,7 @@ func TestForward(t *testing.T) { "-j", "MASQUERADE", } - if !Exists(masqRule...) { + if !Exists(natChain.Table, "POSTROUTING", masqRule...) { t.Fatalf("MASQUERADE rule does not exist") } } @@ -94,8 +91,7 @@ func TestLink(t *testing.T) { t.Fatal(err) } - rule1 := []string{filterChain.Name, - "-t", string(filterChain.Table), + rule1 := []string{ "-i", filterChain.Bridge, "-o", filterChain.Bridge, "-p", proto, @@ -104,12 +100,11 @@ func TestLink(t *testing.T) { "--dport", strconv.Itoa(port), "-j", "ACCEPT"} - if !Exists(rule1...) { + if !Exists(filterChain.Table, filterChain.Name, rule1...) { t.Fatalf("rule1 does not exist") } - rule2 := []string{filterChain.Name, - "-t", string(filterChain.Table), + rule2 := []string{ "-i", filterChain.Bridge, "-o", filterChain.Bridge, "-p", proto, @@ -118,7 +113,7 @@ func TestLink(t *testing.T) { "--sport", strconv.Itoa(port), "-j", "ACCEPT"} - if !Exists(rule2...) { + if !Exists(filterChain.Table, filterChain.Name, rule2...) { t.Fatalf("rule2 does not exist") } } @@ -133,17 +128,16 @@ func TestPrerouting(t *testing.T) { t.Fatal(err) } - rule := []string{"PREROUTING", - "-t", string(Nat), + rule := []string{ "-j", natChain.Name} rule = append(rule, args...) - if !Exists(rule...) { + if !Exists(natChain.Table, "PREROUTING", rule...) { t.Fatalf("rule does not exist") } - delRule := append([]string{"-D"}, rule...) + delRule := append([]string{"-D", "PREROUTING", "-t", string(Nat)}, rule...) if _, err = Raw(delRule...); err != nil { t.Fatal(err) } @@ -159,17 +153,17 @@ func TestOutput(t *testing.T) { t.Fatal(err) } - rule := []string{"OUTPUT", - "-t", string(natChain.Table), + rule := []string{ "-j", natChain.Name} rule = append(rule, args...) - if !Exists(rule...) { + if !Exists(natChain.Table, "OUTPUT", rule...) { t.Fatalf("rule does not exist") } - delRule := append([]string{"-D"}, rule...) + delRule := append([]string{"-D", "OUTPUT", "-t", + string(natChain.Table)}, rule...) if _, err = Raw(delRule...); err != nil { t.Fatal(err) }