Enable IPv4/IPv6 forwarding automatically #2696

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@tianon
Member

tianon commented Nov 14, 2013

After some discussion with @shykes, we decided it makes sense to have this enabled in the core, which fixes one of our longest-standing usability issues.

Since IPv4Forwarding is officially part of the API, I had to insert this code in "runtime.go" to make sure forwarding is enabled before we check whether it is (which is still important to check, because the kernel could still tell us "no"). This way, if the kernel denies our request for forwarding, we'll still get the warning we have in place now.

Of course, if I'm told where it should have gone to make sure it runs before UpdateCapabilities, I'm more than happy to move it, but after digging around, this seemed like the most appropriate place given the constraints.

If the code addition looks good, I'll go ahead and add a new commit to update the two docs that reference ip_forward (Arch and Gentoo install instructions).

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Nov 15, 2013

Contributor

I'm not sure how I like docker changing settings on my host machine and I have no way to stop it. Do we know the security implications for changing these settings?

Contributor

crosbymichael commented Nov 15, 2013

I'm not sure how I like docker changing settings on my host machine and I have no way to stop it. Do we know the security implications for changing these settings?

@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Nov 15, 2013

Member

I believe the biggest justification for it was that the lxc-net upstart script already force-enables ip_forward, and nobody has complained loudly enough for that to change.

I originally suggested that we either bundle this in one of the existing related conditional flags or create a new one, and @shykes said that we should just enable it and can talk about adding a flag when someone comes with a need for it.

This really isn't much different from what Docker already does with your iptables rules, making sure that containers get their associated packets. We talked about playing with the per-interface versions of these settings, but when we've done research into those in the past it's been very erratic and hit-and-miss on which ones exactly need to be enabled to "make it work" like you'd expect.

Member

tianon commented Nov 15, 2013

I believe the biggest justification for it was that the lxc-net upstart script already force-enables ip_forward, and nobody has complained loudly enough for that to change.

I originally suggested that we either bundle this in one of the existing related conditional flags or create a new one, and @shykes said that we should just enable it and can talk about adding a flag when someone comes with a need for it.

This really isn't much different from what Docker already does with your iptables rules, making sure that containers get their associated packets. We talked about playing with the per-interface versions of these settings, but when we've done research into those in the past it's been very erratic and hit-and-miss on which ones exactly need to be enabled to "make it work" like you'd expect.

@shykes

This comment has been minimized.

Show comment
Hide comment
@shykes

shykes Nov 15, 2013

Collaborator

The security implications are minimal. Disabling ip_forward is not a security mechanism - especially since other packages already take liberties with the setting. Firewalling is managed separately by netfilter, and docker is very careful to not  cause side-effects there.

 

​Hypothetically if your host acts as a router to a DMZ with unprotected services, your netfilter is configured to allow untrusted connections to those services, and you are somehow relying solely on ip_forward=0 to disallow it, then docker will remove that protection. But such a setup is already riddled with problems anyway, and if docker doesn't break it, something else will (for example lxc-net).

@solomonstre
@docker

On Thu, Nov 14, 2013 at 9:24 PM, Tianon Gravi notifications@github.com
wrote:

I believe the biggest justification for it was that the lxc-net upstart script already force-enables ip_forward, and nobody has complained loudly enough for that to change.
I originally suggested that we either bundle this in one of the existing related conditional flags or create a new one, and @shykes said that we should just enable it and can talk about adding a flag when someone comes with a need for it.

This really isn't much different from what Docker already does with your iptables rules, making sure that containers get their associated packets. We talked about playing with the per-interface versions of these settings, but when we've done research into those in the past it's been very erratic and hit-and-miss on which ones exactly need to be enabled to "make it work" like you'd expect.

Reply to this email directly or view it on GitHub:
#2696 (comment)

Collaborator

shykes commented Nov 15, 2013

The security implications are minimal. Disabling ip_forward is not a security mechanism - especially since other packages already take liberties with the setting. Firewalling is managed separately by netfilter, and docker is very careful to not  cause side-effects there.

 

​Hypothetically if your host acts as a router to a DMZ with unprotected services, your netfilter is configured to allow untrusted connections to those services, and you are somehow relying solely on ip_forward=0 to disallow it, then docker will remove that protection. But such a setup is already riddled with problems anyway, and if docker doesn't break it, something else will (for example lxc-net).

@solomonstre
@docker

On Thu, Nov 14, 2013 at 9:24 PM, Tianon Gravi notifications@github.com
wrote:

I believe the biggest justification for it was that the lxc-net upstart script already force-enables ip_forward, and nobody has complained loudly enough for that to change.
I originally suggested that we either bundle this in one of the existing related conditional flags or create a new one, and @shykes said that we should just enable it and can talk about adding a flag when someone comes with a need for it.

This really isn't much different from what Docker already does with your iptables rules, making sure that containers get their associated packets. We talked about playing with the per-interface versions of these settings, but when we've done research into those in the past it's been very erratic and hit-and-miss on which ones exactly need to be enabled to "make it work" like you'd expect.

Reply to this email directly or view it on GitHub:
#2696 (comment)

@tianon tianon referenced this pull request Nov 19, 2013

Closed

IPv4 forwarding warning #2396

@ewindisch

This comment has been minimized.

Show comment
Hide comment
@ewindisch

ewindisch Nov 20, 2013

Contributor

Referencing #2396 (comment)

Contributor

ewindisch commented Nov 20, 2013

Referencing #2396 (comment)

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Dec 2, 2013

Contributor

ping @tianon @shykes can you look at @ewindisch 's comment ?

Contributor

crosbymichael commented Dec 2, 2013

ping @tianon @shykes can you look at @ewindisch 's comment ?

@ewindisch

This comment has been minimized.

Show comment
Hide comment
@ewindisch

ewindisch Dec 2, 2013

Contributor

TL;DR version:

  • Re: LXC -- two wrongs doesn't make a right.
  • Have debconf / yast / rpm configure ip_forwarding
  • Clarify or improve documentation
  • Consider (more) net filter management in Docker
  • EXPOSE might be a bug as contracted to the user

I'd prefer just having the installer offer a choice than have docker manipulate this setting itself. However, it wouldn't be too terrible for Docker to make this sysctl change as long as it is optional.

Contributor

ewindisch commented Dec 2, 2013

TL;DR version:

  • Re: LXC -- two wrongs doesn't make a right.
  • Have debconf / yast / rpm configure ip_forwarding
  • Clarify or improve documentation
  • Consider (more) net filter management in Docker
  • EXPOSE might be a bug as contracted to the user

I'd prefer just having the installer offer a choice than have docker manipulate this setting itself. However, it wouldn't be too terrible for Docker to make this sysctl change as long as it is optional.

@ewindisch

This comment has been minimized.

Show comment
Hide comment
@ewindisch

ewindisch Dec 2, 2013

Contributor

Beyond those comments:

It isn't unreasonable that someone might wish to use SDN with Docker. It's almost guaranteed to become a request with OpenStack integration. In that case, the ip_forwarding might not ever happen to the physical devices, but to a GRE or VXLAN tunnel. In this case, I don't believe it would be necessary to have global ip_forwarding.

Configuration of ip_forwarding (or not) might be something that could ultimately become part of configurable network configuration scripts should there become such a feature... I believe discussion has occurred around this within the issues tracker? Xen does this (or did in older versions), allowing network automation to occur within shell scripts managed by the administrator. This allowed for more granular or spectacular network configurations than whatever was default.

Contributor

ewindisch commented Dec 2, 2013

Beyond those comments:

It isn't unreasonable that someone might wish to use SDN with Docker. It's almost guaranteed to become a request with OpenStack integration. In that case, the ip_forwarding might not ever happen to the physical devices, but to a GRE or VXLAN tunnel. In this case, I don't believe it would be necessary to have global ip_forwarding.

Configuration of ip_forwarding (or not) might be something that could ultimately become part of configurable network configuration scripts should there become such a feature... I believe discussion has occurred around this within the issues tracker? Xen does this (or did in older versions), allowing network automation to occur within shell scripts managed by the administrator. This allowed for more granular or spectacular network configurations than whatever was default.

@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Dec 2, 2013

Member

I definitely don't have any gripes against adding a command-line option for disabling this behavior, and suggested it originally but was told we don't want even more flags. :)

I think a command-line flag for this specifically would be consistent with all our other custom-networking-related flags.

/cc @shykes

Member

tianon commented Dec 2, 2013

I definitely don't have any gripes against adding a command-line option for disabling this behavior, and suggested it originally but was told we don't want even more flags. :)

I think a command-line flag for this specifically would be consistent with all our other custom-networking-related flags.

/cc @shykes

@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Dec 2, 2013

Member

(And I definitely agree that eventually, we ought to have SDN, but that's not anywhere near my domain to make that call. :D)

Member

tianon commented Dec 2, 2013

(And I definitely agree that eventually, we ought to have SDN, but that's not anywhere near my domain to make that call. :D)

@creack

This comment has been minimized.

Show comment
Hide comment
@creack

creack Dec 27, 2013

Contributor

Closing for now. Please send a mail to docker-dev's ML for followup/decision.

Contributor

creack commented Dec 27, 2013

Closing for now. Please send a mail to docker-dev's ML for followup/decision.

@creack creack closed this Dec 27, 2013

@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Dec 27, 2013

Member

K, I'll start a thread. Thanks @creack.

Member

tianon commented Dec 27, 2013

K, I'll start a thread. Thanks @creack.

@jpoimboe

This comment has been minimized.

Show comment
Hide comment
@jpoimboe

jpoimboe Jan 28, 2014

Contributor

@tianon Since there seemed to be general agreement on the mailing list, you planning on opening another PR for this? I can do it if you're busy.

Contributor

jpoimboe commented Jan 28, 2014

@tianon Since there seemed to be general agreement on the mailing list, you planning on opening another PR for this? I can do it if you're busy.

@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Jan 28, 2014

Member

Yes! Thanks for the reminder @jpoimboe. I think it's been long enough now. :)

For the record, my new PR will not be setting "net.ipv6.conf.all.forwarding", since I finally got a chance to do some IPv6-related testing and that's pretty much universally wrong to change to any value, and makes a lot more sense when set on individual interfaces appropriately (unlike "net.ipv4.ip_forward", which takes quite a bit of strange incantations to work properly if only set on individual interfaces).

Member

tianon commented Jan 28, 2014

Yes! Thanks for the reminder @jpoimboe. I think it's been long enough now. :)

For the record, my new PR will not be setting "net.ipv6.conf.all.forwarding", since I finally got a chance to do some IPv6-related testing and that's pretty much universally wrong to change to any value, and makes a lot more sense when set on individual interfaces appropriately (unlike "net.ipv4.ip_forward", which takes quite a bit of strange incantations to work properly if only set on individual interfaces).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment