Skip to content

when enabling ip forwarding set the default forward policy to drop #1526

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.

Sign up for GitHub

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

Merged
mavenugo merged 1 commit into from
Nov 10, 2016
Merged

when enabling ip forwarding set the default forward policy to drop #1526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions drivers/bridge/bridge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,13 +380,6 @@ func (d *driver) configure(option map[string]interface{}) error {
return &ErrInvalidDriverConfig{}
}

if config.EnableIPForwarding {
err = setupIPForwarding()
if err != nil {
return err
}
}

if config.EnableIPTables {
Copy link
Copy Markdown
Contributor

@aboch aboch Nov 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the new logic is to change the FORWARD chain default policy only if config.EnableIPTables==true, I am thinking we do not need to move this code, neither make any change in SetupForwarding().
We only need to add in SetupIPChain(configuration)

if config.EnableIPForwarding {
    if err := iptables.SetDefaultPolicy(iptables.Filter, "FORWARD", iptables.Drop); err != nil {
        return fmt.Errorf("%v. IP forwarding was set to 1", err)
    }
}

Also this will guarantee the action will take place when the firewall is reloaded:
iptables.OnReloaded(..., setupIPChains)

Copy link
Copy Markdown
Author

@sanimej sanimej Nov 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config.EnableIPForwarding is set based on the daemon flag and not by checking the sysctl ip_forward. By doing the way you are suggesting we will call SetDefaultPolicy even if ip_forward is already enabled; ie: ip_forward might be enabled and the default policy set be ACCEPT. Docker changing that to DROP is incorrect.

Copy link
Copy Markdown
Contributor

@aboch aboch Nov 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that's right. Thanks.

if _, err := os.Stat("/proc/sys/net/bridge"); err != nil {
if out, err := exec.Command("modprobe", "-va", "bridge", "br_netfilter").CombinedOutput(); err != nil {
Expand All @@ -402,6 +395,14 @@ func (d *driver) configure(option map[string]interface{}) error {
iptables.OnReloaded(func() { logrus.Debugf("Recreating iptables chains on firewall reload"); setupIPChains(config) })
}

if config.EnableIPForwarding {
err = setupIPForwarding(config.EnableIPTables)
if err != nil {
logrus.Warn(err)
return err
}
}

d.Lock()
d.natChain = natChain
d.filterChain = filterChain
Expand Down
34 changes: 30 additions & 4 deletions drivers/bridge/setup_ip_forwarding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package bridge

import (
"fmt"
log "github.com/Sirupsen/logrus"
"github.com/docker/libnetwork/iptables"
"io/ioutil"
)

Expand All @@ -10,7 +12,15 @@ const (
ipv4ForwardConfPerm = 0644
)

func setupIPForwarding() error {
func configureIPForwarding(enable bool) error {
var val byte
if enable {
val = '1'
}
return ioutil.WriteFile(ipv4ForwardConf, []byte{val, '\n'}, ipv4ForwardConfPerm)
}

func setupIPForwarding(enableIPTables bool) error {
// Get current IPv4 forward setup
ipv4ForwardData, err := ioutil.ReadFile(ipv4ForwardConf)
if err != nil {
Expand All @@ -20,10 +30,26 @@ func setupIPForwarding() error {
// Enable IPv4 forwarding only if it is not already enabled
if ipv4ForwardData[0] != '1' {
// Enable IPv4 forwarding
if err := ioutil.WriteFile(ipv4ForwardConf, []byte{'1', '\n'}, ipv4ForwardConfPerm); err != nil {
return fmt.Errorf("Setup IP forwarding failed: %v", err)
if err := configureIPForwarding(true); err != nil {
return fmt.Errorf("Enabling IP forwarding failed: %v", err)
}
// When enabling ip_forward set the default policy on forward chain to
// drop only if the daemon option iptables is not set to false.
if !enableIPTables {
return nil
}
if err := iptables.SetDefaultPolicy(iptables.Filter, "FORWARD", iptables.Drop); err != nil {
if err := configureIPForwarding(false); err != nil {
log.Errorf("Disabling IP forwarding failed, %v", err)
}
return err
}
iptables.OnReloaded(func() {
log.Debugf("Setting the default DROP policy on firewall reload")
if err := iptables.SetDefaultPolicy(iptables.Filter, "FORWARD", iptables.Drop); err != nil {
log.Warnf("Settig the default DROP policy on firewall reload failed, %v", err)
}
})
}

return nil
}
2 changes: 1 addition & 1 deletion drivers/bridge/setup_ip_forwarding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ func TestSetupIPForwarding(t *testing.T) {
}

// Set IP Forwarding
if err := setupIPForwarding(); err != nil {
if err := setupIPForwarding(true); err != nil {
t.Fatalf("Failed to setup IP forwarding: %v", err)
}

Expand Down
15 changes: 15 additions & 0 deletions iptables/iptables.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ import (
// Action signifies the iptable action.
type Action string

// Policy is the default iptable policies
type Policy string

// Table refers to Nat, Filter or Mangle.
type Table string

Expand All @@ -32,6 +35,10 @@ const (
Filter Table = "filter"
// Mangle table is used for mangling the packet.
Mangle Table = "mangle"
// Drop is the default iptables DROP policy
Drop Policy = "DROP"
// Accept is the default iptables ACCEPT policy
Accept Policy = "ACCEPT"
)

var (
Expand Down Expand Up @@ -422,6 +429,14 @@ func GetVersion() (major, minor, micro int, err error) {
return
}

// SetDefaultPolicy sets the passed default policy for the table/chain
func SetDefaultPolicy(table Table, chain string, policy Policy) error {
if err := RawCombinedOutput("-t", string(table), "-P", chain, string(policy)); err != nil {
return fmt.Errorf("setting default policy to %v in %v chain failed: %v", policy, chain, err)
}
return nil
}

func parseVersionNumbers(input string) (major, minor, micro int) {
re := regexp.MustCompile(`v\d*.\d*.\d*`)
line := re.FindString(input)
Expand Down