From 95f1bcc2497a0da39e87001ffa0ea39d9bf9476b Mon Sep 17 00:00:00 2001 From: Alessandro Boch Date: Mon, 14 Dec 2015 21:57:56 -0800 Subject: [PATCH] Change isolation rules for multiple bridge networks - From subnet to interface Signed-off-by: Alessandro Boch --- libnetwork/drivers/bridge/bridge.go | 48 ++++---- libnetwork/drivers/bridge/bridge_test.go | 32 +++-- libnetwork/drivers/bridge/setup_ip_tables.go | 110 +++++++++++++----- .../drivers/bridge/setup_ip_tables_test.go | 2 +- libnetwork/drivers/overlay/filter.go | 34 +++--- libnetwork/iptables/iptables.go | 10 ++ 6 files changed, 147 insertions(+), 89 deletions(-) diff --git a/libnetwork/drivers/bridge/bridge.go b/libnetwork/drivers/bridge/bridge.go index 04f5a4c81b..b2beef9ed6 100644 --- a/libnetwork/drivers/bridge/bridge.go +++ b/libnetwork/drivers/bridge/bridge.go @@ -106,12 +106,13 @@ type bridgeNetwork struct { } type driver struct { - config *configuration - network *bridgeNetwork - natChain *iptables.ChainInfo - filterChain *iptables.ChainInfo - networks map[string]*bridgeNetwork - store datastore.DataStore + config *configuration + network *bridgeNetwork + natChain *iptables.ChainInfo + filterChain *iptables.ChainInfo + isolationChain *iptables.ChainInfo + networks map[string]*bridgeNetwork + store datastore.DataStore sync.Mutex } @@ -244,15 +245,15 @@ func (n *bridgeNetwork) registerIptCleanFunc(clean iptableCleanFunc) { n.iptCleanFuncs = append(n.iptCleanFuncs, clean) } -func (n *bridgeNetwork) getDriverChains() (*iptables.ChainInfo, *iptables.ChainInfo, error) { +func (n *bridgeNetwork) getDriverChains() (*iptables.ChainInfo, *iptables.ChainInfo, *iptables.ChainInfo, error) { n.Lock() defer n.Unlock() if n.driver == nil { - return nil, nil, types.BadRequestErrorf("no driver found") + return nil, nil, nil, types.BadRequestErrorf("no driver found") } - return n.driver.natChain, n.driver.filterChain, nil + return n.driver.natChain, n.driver.filterChain, n.driver.isolationChain, nil } func (n *bridgeNetwork) getNetworkBridgeName() string { @@ -282,26 +283,16 @@ func (n *bridgeNetwork) getEndpoint(eid string) (*bridgeEndpoint, error) { // from each of the other networks func (n *bridgeNetwork) isolateNetwork(others []*bridgeNetwork, enable bool) error { n.Lock() - thisV4 := n.bridge.bridgeIPv4 - thisV6 := getV6Network(n.config, n.bridge) + thisIface := n.config.BridgeName n.Unlock() // Install the rules to isolate this networks against each of the other networks for _, o := range others { o.Lock() - otherV4 := o.bridge.bridgeIPv4 - otherV6 := getV6Network(o.config, o.bridge) + otherIface := o.config.BridgeName o.Unlock() - - if !types.CompareIPNet(thisV4, otherV4) { - // It's ok to pass a.b.c.d/x, iptables will ignore the host subnet bits - if err := setINC(thisV4.String(), otherV4.String(), enable); err != nil { - return err - } - } - - if thisV6 != nil && otherV6 != nil && !types.CompareIPNet(thisV6, otherV6) { - if err := setINC(thisV6.String(), otherV6.String(), enable); err != nil { + if thisIface != otherIface { + if err := setINC(thisIface, otherIface, enable); err != nil { return err } } @@ -347,9 +338,11 @@ func (c *networkConfiguration) conflictsWithNetworks(id string, others []*bridge func (d *driver) configure(option map[string]interface{}) error { var ( - config *configuration - err error - natChain, filterChain *iptables.ChainInfo + config *configuration + err error + natChain *iptables.ChainInfo + filterChain *iptables.ChainInfo + isolationChain *iptables.ChainInfo ) genericData, ok := option[netlabel.GenericData] @@ -378,7 +371,7 @@ func (d *driver) configure(option map[string]interface{}) error { } if config.EnableIPTables { - natChain, filterChain, err = setupIPChains(config) + natChain, filterChain, isolationChain, err = setupIPChains(config) if err != nil { return err } @@ -387,6 +380,7 @@ func (d *driver) configure(option map[string]interface{}) error { d.Lock() d.natChain = natChain d.filterChain = filterChain + d.isolationChain = isolationChain d.config = config d.Unlock() diff --git a/libnetwork/drivers/bridge/bridge_test.go b/libnetwork/drivers/bridge/bridge_test.go index e039a53e61..37da29451d 100644 --- a/libnetwork/drivers/bridge/bridge_test.go +++ b/libnetwork/drivers/bridge/bridge_test.go @@ -275,7 +275,7 @@ func TestCreateMultipleNetworks(t *testing.T) { } // Verify the network isolation rules are installed, each network subnet should appear 4 times - verifyV4INCEntries(d.networks, 4, t) + verifyV4INCEntries(d.networks, 6, t) config4 := &networkConfiguration{BridgeName: "net_test_4"} genericOption[netlabel.GenericData] = config4 @@ -284,10 +284,10 @@ func TestCreateMultipleNetworks(t *testing.T) { } // Now 6 times - verifyV4INCEntries(d.networks, 6, t) + verifyV4INCEntries(d.networks, 12, t) d.DeleteNetwork("1") - verifyV4INCEntries(d.networks, 4, t) + verifyV4INCEntries(d.networks, 6, t) d.DeleteNetwork("2") verifyV4INCEntries(d.networks, 2, t) @@ -300,19 +300,29 @@ func TestCreateMultipleNetworks(t *testing.T) { } func verifyV4INCEntries(networks map[string]*bridgeNetwork, numEntries int, t *testing.T) { - out, err := iptables.Raw("-L", "FORWARD") + out, err := iptables.Raw("-nvL", IsolationChain) if err != nil { t.Fatal(err) } - for _, nw := range networks { - nt := types.GetIPNetCopy(nw.bridge.bridgeIPv4) - nt.IP = nt.IP.Mask(nt.Mask) - re := regexp.MustCompile(nt.String()) - matches := re.FindAllString(string(out[:]), -1) - if len(matches) != numEntries { - t.Fatalf("Cannot find expected inter-network isolation rules in IP Tables:\n%s", string(out[:])) + + found := 0 + for _, x := range networks { + for _, y := range networks { + if x == y { + continue + } + re := regexp.MustCompile(fmt.Sprintf("%s %s", x.config.BridgeName, y.config.BridgeName)) + matches := re.FindAllString(string(out[:]), -1) + if len(matches) != 1 { + t.Fatalf("Cannot find expected inter-network isolation rules in IP Tables:\n%s", string(out[:])) + } + found++ } } + + if found != numEntries { + t.Fatalf("Cannot find expected number (%d) of inter-network isolation rules in IP Tables:\n%s\nFound %d", numEntries, string(out[:]), found) + } } type testInterface struct { diff --git a/libnetwork/drivers/bridge/setup_ip_tables.go b/libnetwork/drivers/bridge/setup_ip_tables.go index 6b1c5dcb11..2d2442f295 100644 --- a/libnetwork/drivers/bridge/setup_ip_tables.go +++ b/libnetwork/drivers/bridge/setup_ip_tables.go @@ -11,35 +11,52 @@ import ( // DockerChain: DOCKER iptable chain name const ( - DockerChain = "DOCKER" + DockerChain = "DOCKER" + IsolationChain = "DOCKER-ISOLATION" ) -func setupIPChains(config *configuration) (*iptables.ChainInfo, *iptables.ChainInfo, error) { +func setupIPChains(config *configuration) (*iptables.ChainInfo, *iptables.ChainInfo, *iptables.ChainInfo, error) { // Sanity check. if config.EnableIPTables == false { - return nil, nil, fmt.Errorf("Cannot create new chains, EnableIPTable is disabled") + return nil, nil, nil, fmt.Errorf("cannot create new chains, EnableIPTable is disabled") } hairpinMode := !config.EnableUserlandProxy natChain, err := iptables.NewChain(DockerChain, iptables.Nat, hairpinMode) if err != nil { - return nil, nil, fmt.Errorf("Failed to create NAT chain: %s", err.Error()) + return nil, nil, nil, fmt.Errorf("failed to create NAT chain: %v", err) } defer func() { if err != nil { if err := iptables.RemoveExistingChain(DockerChain, iptables.Nat); err != nil { - logrus.Warnf("Failed on removing iptables NAT chain on cleanup: %v", err) + logrus.Warnf("failed on removing iptables NAT chain on cleanup: %v", err) } } }() - filterChain, err := iptables.NewChain(DockerChain, iptables.Filter, hairpinMode) + filterChain, err := iptables.NewChain(DockerChain, iptables.Filter, false) if err != nil { - return nil, nil, fmt.Errorf("Failed to create FILTER chain: %s", err.Error()) + return nil, nil, nil, fmt.Errorf("failed to create FILTER chain: %v", err) + } + defer func() { + if err != nil { + if err := iptables.RemoveExistingChain(DockerChain, iptables.Filter); err != nil { + logrus.Warnf("failed on removing iptables FILTER chain on cleanup: %v", err) + } + } + }() + + isolationChain, err := iptables.NewChain(IsolationChain, iptables.Filter, false) + if err != nil { + return nil, nil, nil, fmt.Errorf("failed to create FILTER isolation chain: %v", err) } - return natChain, filterChain, nil + if err := addReturnRule(IsolationChain); err != nil { + return nil, nil, nil, err + } + + return natChain, filterChain, isolationChain, nil } func (n *bridgeNetwork) setupIPTables(config *networkConfiguration, i *bridgeInterface) error { @@ -72,7 +89,7 @@ func (n *bridgeNetwork) setupIPTables(config *networkConfiguration, i *bridgeInt return setupIPTablesInternal(config.BridgeName, maskedAddrv4, config.EnableICC, config.EnableIPMasquerade, hairpinMode, false) }) - natChain, filterChain, err := n.getDriverChains() + natChain, filterChain, _, err := n.getDriverChains() if err != nil { return fmt.Errorf("Failed to setup IP tables, cannot acquire chain info %s", err.Error()) } @@ -86,6 +103,11 @@ func (n *bridgeNetwork) setupIPTables(config *networkConfiguration, i *bridgeInt if err != nil { return fmt.Errorf("Failed to program FILTER chain: %s", err.Error()) } + + if err := ensureJumpRule("FORWARD", IsolationChain); err != nil { + return err + } + n.registerIptCleanFunc(func() error { return iptables.ProgramChain(filterChain, config.BridgeName, hairpinMode, false) }) @@ -166,10 +188,8 @@ func programChainRule(rule iptRule, ruleDescr string, insert bool) error { } if condition { - if output, err := iptables.Raw(append(prefix, rule.args...)...); err != nil { + if err := iptables.RawCombinedOutput(append(prefix, rule.args...)...); err != nil { return fmt.Errorf("Unable to %s %s rule: %s", operation, ruleDescr, err.Error()) - } else if len(output) != 0 { - return &iptables.ChainError{Chain: rule.chain, Output: output} } } @@ -190,20 +210,16 @@ func setIcc(bridgeIface string, iccEnable, insert bool) error { iptables.Raw(append([]string{"-D", chain}, acceptArgs...)...) if !iptables.Exists(table, chain, dropArgs...) { - if output, err := iptables.Raw(append([]string{"-A", chain}, dropArgs...)...); err != nil { + if err := iptables.RawCombinedOutput(append([]string{"-A", chain}, dropArgs...)...); err != nil { return fmt.Errorf("Unable to prevent intercontainer communication: %s", err.Error()) - } else if len(output) != 0 { - return fmt.Errorf("Error disabling intercontainer communication: %s", output) } } } else { iptables.Raw(append([]string{"-D", chain}, dropArgs...)...) if !iptables.Exists(table, chain, acceptArgs...) { - if output, err := iptables.Raw(append([]string{"-I", chain}, acceptArgs...)...); err != nil { + if err := iptables.RawCombinedOutput(append([]string{"-I", chain}, acceptArgs...)...); err != nil { return fmt.Errorf("Unable to allow intercontainer communication: %s", err.Error()) - } else if len(output) != 0 { - return fmt.Errorf("Error enabling intercontainer communication: %s", output) } } } @@ -224,11 +240,11 @@ func setIcc(bridgeIface string, iccEnable, insert bool) error { } // Control Inter Network Communication. Install/remove only if it is not/is present. -func setINC(network1, network2 string, enable bool) error { +func setINC(iface1, iface2 string, enable bool) error { var ( table = iptables.Filter - chain = "FORWARD" - args = [2][]string{{"-s", network1, "-d", network2, "-j", "DROP"}, {"-s", network2, "-d", network1, "-j", "DROP"}} + chain = IsolationChain + args = [2][]string{{"-i", iface1, "-o", iface2, "-j", "DROP"}, {"-i", iface2, "-o", iface1, "-j", "DROP"}} ) if enable { @@ -236,10 +252,8 @@ func setINC(network1, network2 string, enable bool) error { if iptables.Exists(table, chain, args[i]...) { continue } - if output, err := iptables.Raw(append([]string{"-I", chain}, args[i]...)...); err != nil { - return fmt.Errorf("unable to add inter-network communication rule: %s", err.Error()) - } else if len(output) != 0 { - return fmt.Errorf("error adding inter-network communication rule: %s", string(output)) + if err := iptables.RawCombinedOutput(append([]string{"-I", chain}, args[i]...)...); err != nil { + return fmt.Errorf("unable to add inter-network communication rule: %v", err) } } } else { @@ -247,13 +261,51 @@ func setINC(network1, network2 string, enable bool) error { if !iptables.Exists(table, chain, args[i]...) { continue } - if output, err := iptables.Raw(append([]string{"-D", chain}, args[i]...)...); err != nil { - return fmt.Errorf("unable to remove inter-network communication rule: %s", err.Error()) - } else if len(output) != 0 { - return fmt.Errorf("error removing inter-network communication rule: %s", string(output)) + if err := iptables.RawCombinedOutput(append([]string{"-D", chain}, args[i]...)...); err != nil { + return fmt.Errorf("unable to remove inter-network communication rule: %v", err) } } } return nil } + +func addReturnRule(chain string) error { + var ( + table = iptables.Filter + args = []string{"-j", "RETURN"} + ) + + if iptables.Exists(table, chain, args...) { + return nil + } + + err := iptables.RawCombinedOutput(append([]string{"-I", chain}, args...)...) + if err != nil { + return fmt.Errorf("unable to add return rule in %s chain: %s", chain, err.Error()) + } + + return nil +} + +// Ensure the jump rule is on top +func ensureJumpRule(fromChain, toChain string) error { + var ( + table = iptables.Filter + args = []string{"-j", toChain} + ) + + if iptables.Exists(table, fromChain, args...) { + err := iptables.RawCombinedOutput(append([]string{"-D", fromChain}, args...)...) + if err != nil { + return fmt.Errorf("unable to remove jump to %s rule in %s chain: %s", toChain, fromChain, err.Error()) + } + } + + err := iptables.RawCombinedOutput(append([]string{"-I", fromChain}, args...)...) + if err != nil { + return fmt.Errorf("unable to insert jump to %s rule in %s chain: %s", toChain, fromChain, err.Error()) + } + + return nil +} diff --git a/libnetwork/drivers/bridge/setup_ip_tables_test.go b/libnetwork/drivers/bridge/setup_ip_tables_test.go index 12fe67be28..983f225f36 100644 --- a/libnetwork/drivers/bridge/setup_ip_tables_test.go +++ b/libnetwork/drivers/bridge/setup_ip_tables_test.go @@ -104,7 +104,7 @@ func assertIPTableChainProgramming(rule iptRule, descr string, t *testing.T) { func assertChainConfig(d *driver, t *testing.T) { var err error - d.natChain, d.filterChain, err = setupIPChains(d.config) + d.natChain, d.filterChain, d.isolationChain, err = setupIPChains(d.config) if err != nil { t.Fatal(err) } diff --git a/libnetwork/drivers/overlay/filter.go b/libnetwork/drivers/overlay/filter.go index 0607548140..87b0c48aa0 100644 --- a/libnetwork/drivers/overlay/filter.go +++ b/libnetwork/drivers/overlay/filter.go @@ -12,16 +12,6 @@ const globalChain = "DOCKER-OVERLAY" var filterOnce sync.Once -func rawIPTables(args ...string) error { - if output, err := iptables.Raw(args...); err != nil { - return fmt.Errorf("unable to add overlay filter: %v", err) - } else if len(output) != 0 { - return fmt.Errorf("unable to add overlay filter: %s", string(output)) - } - - return nil -} - func chainExists(cname string) bool { if _, err := iptables.Raw("-L", cname); err != nil { return false @@ -31,12 +21,14 @@ func chainExists(cname string) bool { } func setupGlobalChain() { - if err := rawIPTables("-N", globalChain); err != nil { - logrus.Debugf("could not create global overlay chain: %v", err) + if err := iptables.RawCombinedOutput("-N", globalChain); err != nil { + logrus.Errorf("could not create global overlay chain: %v", err) + return } - if err := rawIPTables("-A", globalChain, "-j", "RETURN"); err != nil { - logrus.Debugf("could not install default return chain in the overlay global chain: %v", err) + if err := iptables.RawCombinedOutput("-A", globalChain, "-j", "RETURN"); err != nil { + logrus.Errorf("could not install default return chain in the overlay global chain: %v", err) + return } } @@ -49,21 +41,21 @@ func setNetworkChain(cname string, remove bool) error { opt := "-N" // In case of remove, make sure to flush the rules in the chain if remove && exists { - if err := rawIPTables("-F", cname); err != nil { + if err := iptables.RawCombinedOutput("-F", cname); err != nil { return fmt.Errorf("failed to flush overlay network chain %s rules: %v", cname, err) } opt = "-X" } if (!remove && !exists) || (remove && exists) { - if err := rawIPTables(opt, cname); err != nil { + if err := iptables.RawCombinedOutput(opt, cname); err != nil { return fmt.Errorf("failed network chain operation %q for chain %s: %v", opt, cname, err) } } if !remove { if !iptables.Exists(iptables.Filter, cname, "-j", "DROP") { - if err := rawIPTables("-A", cname, "-j", "DROP"); err != nil { + if err := iptables.RawCombinedOutput("-A", cname, "-j", "DROP"); err != nil { return fmt.Errorf("failed adding default drop rule to overlay network chain %s: %v", cname, err) } } @@ -91,12 +83,12 @@ func setFilters(cname, brName string, remove bool) error { for _, chain := range []string{"OUTPUT", "FORWARD"} { exists := iptables.Exists(iptables.Filter, chain, "-j", globalChain) if exists { - if err := rawIPTables("-D", chain, "-j", globalChain); err != nil { + if err := iptables.RawCombinedOutput("-D", chain, "-j", globalChain); err != nil { return fmt.Errorf("failed to delete overlay hook in chain %s while moving the hook: %v", chain, err) } } - if err := rawIPTables("-I", chain, "-j", globalChain); err != nil { + if err := iptables.RawCombinedOutput("-I", chain, "-j", globalChain); err != nil { return fmt.Errorf("failed to insert overlay hook in chain %s: %v", chain, err) } } @@ -105,7 +97,7 @@ func setFilters(cname, brName string, remove bool) error { // Insert/Delete the rule to jump to per-bridge chain exists := iptables.Exists(iptables.Filter, globalChain, "-o", brName, "-j", cname) if (!remove && !exists) || (remove && exists) { - if err := rawIPTables(opt, globalChain, "-o", brName, "-j", cname); err != nil { + if err := iptables.RawCombinedOutput(opt, globalChain, "-o", brName, "-j", cname); err != nil { return fmt.Errorf("failed to add per-bridge filter rule for bridge %s, network chain %s: %v", brName, cname, err) } } @@ -115,7 +107,7 @@ func setFilters(cname, brName string, remove bool) error { return nil } - if err := rawIPTables(opt, cname, "-i", brName, "-j", "ACCEPT"); err != nil { + if err := iptables.RawCombinedOutput(opt, cname, "-i", brName, "-j", "ACCEPT"); err != nil { return fmt.Errorf("failed to add overlay filter rile for network chain %s, bridge %s: %v", cname, brName, err) } diff --git a/libnetwork/iptables/iptables.go b/libnetwork/iptables/iptables.go index be7725f85d..170ea1353e 100644 --- a/libnetwork/iptables/iptables.go +++ b/libnetwork/iptables/iptables.go @@ -312,6 +312,7 @@ func Exists(table Table, chain string, rule ...string) bool { // parse "iptables -S" for the rule (this checks rules in a specific chain // in a specific table) ruleString := strings.Join(rule, " ") + ruleString = chain + " " + ruleString existingRules, _ := exec.Command(iptablesPath, "-t", string(table), "-S", chain).Output() return strings.Contains(string(existingRules), ruleString) @@ -351,3 +352,12 @@ func Raw(args ...string) ([]byte, error) { return output, err } + +// RawCombinedOutput inernally calls the Raw function and returns a non nil +// error if Raw returned a non nil error or a non empty output +func RawCombinedOutput(args ...string) error { + if output, err := Raw(args...); err != nil || len(output) != 0 { + return fmt.Errorf("%s (%v)", string(output), err) + } + return nil +}