libnet: Don't forward to upstream resolvers on internal nw

Commit cbc2a71 makes `connect` syscall fail fast when a container is
only attached to an internal network. Thanks to that, if such a
container tries to resolve an "external" domain, the embedded resolver
returns an error immediately instead of waiting for a timeout.

This commit makes sure the embedded resolver doesn't even try to forward
to upstream servers.

Co-authored-by: Albin Kerouanton <albinker@gmail.com>
Signed-off-by: Rob Murray <rob.murray@docker.com>

integration/networking/resolvconf_test.go

@@ -1,6 +1,7 @@
 package networking
 
 import (
+	"net"
 	"os"
 	"strings"
 	"testing"
@@ -9,30 +10,39 @@ import (
 	"github.com/docker/docker/integration/internal/container"
 	"github.com/docker/docker/integration/internal/network"
 	"github.com/docker/docker/testutil/daemon"
+	"github.com/miekg/dns"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
 	"gotest.tools/v3/skip"
 )
 
-// Regression test for https://github.com/moby/moby/issues/46968
-func TestResolvConfLocalhostIPv6(t *testing.T) {
-	// No "/etc/resolv.conf" on Windows.
-	skip.If(t, testEnv.DaemonInfo.OSType == "windows")
-
-	ctx := setupTest(t)
-
-	// Write a resolv.conf that only contains a loopback address.
+// writeTempResolvConf writes a resolv.conf that only contains a single
+// nameserver line, with address addr.
+// It returns the name of the temp file.
+func writeTempResolvConf(t *testing.T, addr string) string {
+	t.Helper()
 	// Not using t.TempDir() here because in rootless mode, while the temporary
 	// directory gets mode 0777, it's a subdir of an 0700 directory owned by root.
 	// So, it's not accessible by the daemon.
 	f, err := os.CreateTemp("", "resolv.conf")
 	assert.NilError(t, err)
-	defer os.Remove(f.Name())
+	t.Cleanup(func() { os.Remove(f.Name()) })
 	err = f.Chmod(0644)
 	assert.NilError(t, err)
-	f.Write([]byte("nameserver 127.0.0.53\n"))
+	f.Write([]byte("nameserver " + addr + "\n"))
+	return f.Name()
+}
+
+// Regression test for https://github.com/moby/moby/issues/46968
+func TestResolvConfLocalhostIPv6(t *testing.T) {
+	// No "/etc/resolv.conf" on Windows.
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows")
 
-	d := daemon.New(t, daemon.WithEnvVars("DOCKER_TEST_RESOLV_CONF_PATH="+f.Name()))
+	ctx := setupTest(t)
+
+	tmpFileName := writeTempResolvConf(t, "127.0.0.53")
+
+	d := daemon.New(t, daemon.WithEnvVars("DOCKER_TEST_RESOLV_CONF_PATH="+tmpFileName))
 	d.StartWithBusybox(ctx, t, "--experimental", "--ip6tables")
 	defer d.Stop(t)
 
@@ -56,7 +66,7 @@ func TestResolvConfLocalhostIPv6(t *testing.T) {
 		Force: true,
 	})
 
-	output := strings.ReplaceAll(result.Stdout.String(), f.Name(), "RESOLV.CONF")
+	output := strings.ReplaceAll(result.Stdout.String(), tmpFileName, "RESOLV.CONF")
 	assert.Check(t, is.Equal(output, `# Generated by Docker Engine.
 # This file can be edited; Docker Engine will not make further changes once it
 # has been modified.
@@ -70,3 +80,112 @@ options ndots:0
 # Option ndots from: internal
 `))
 }
+
+const dnsRespAddr = "10.11.12.13"
+
+// startDaftDNS starts and returns a really, really daft DNS server that only
+// responds to type-A requests, and always with address dnsRespAddr.
+func startDaftDNS(t *testing.T, addr string) *dns.Server {
+	serveDNS := func(w dns.ResponseWriter, query *dns.Msg) {
+		if query.Question[0].Qtype == dns.TypeA {
+			resp := &dns.Msg{}
+			resp.SetReply(query)
+			answer := &dns.A{
+				Hdr: dns.RR_Header{
+					Name:   query.Question[0].Name,
+					Rrtype: dns.TypeA,
+					Class:  dns.ClassINET,
+					Ttl:    600,
+				},
+			}
+			answer.A = net.ParseIP(dnsRespAddr)
+			resp.Answer = append(resp.Answer, answer)
+			_ = w.WriteMsg(resp)
+		}
+	}
+
+	conn, err := net.ListenUDP("udp", &net.UDPAddr{
+		IP:   net.ParseIP(addr),
+		Port: 53,
+	})
+	assert.NilError(t, err)
+
+	server := &dns.Server{Handler: dns.HandlerFunc(serveDNS), PacketConn: conn}
+	go func() {
+		_ = server.ActivateAndServe()
+	}()
+
+	return server
+}
+
+// Check that when a container is connected to an internal network, DNS
+// requests sent to daemon's internal DNS resolver are not forwarded to
+// an upstream resolver listening on a localhost address.
+// (Assumes the host does not already have a DNS server on 127.0.0.1.)
+func TestInternalNetworkDNS(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows", "No resolv.conf on Windows")
+	skip.If(t, testEnv.IsRootless, "Can't use resolver on host in rootless mode")
+	ctx := setupTest(t)
+
+	// Start a DNS server on the loopback interface.
+	server := startDaftDNS(t, "127.0.0.1")
+	defer server.Shutdown()
+
+	// Set up a temp resolv.conf pointing at that DNS server, and a daemon using it.
+	tmpFileName := writeTempResolvConf(t, "127.0.0.1")
+	d := daemon.New(t, daemon.WithEnvVars("DOCKER_TEST_RESOLV_CONF_PATH="+tmpFileName))
+	d.StartWithBusybox(ctx, t, "--experimental", "--ip6tables")
+	defer d.Stop(t)
+
+	c := d.NewClientT(t)
+	defer c.Close()
+
+	intNetName := "intnet"
+	network.CreateNoError(ctx, t, c, intNetName,
+		network.WithDriver("bridge"),
+		network.WithInternal(),
+	)
+	defer network.RemoveNoError(ctx, t, c, intNetName)
+
+	extNetName := "extnet"
+	network.CreateNoError(ctx, t, c, extNetName,
+		network.WithDriver("bridge"),
+	)
+	defer network.RemoveNoError(ctx, t, c, extNetName)
+
+	// Create a container, initially with external connectivity.
+	// Expect the external DNS server to respond to a request from the container.
+	ctrId := container.Run(ctx, t, c, container.WithNetworkMode(extNetName))
+	defer c.ContainerRemove(ctx, ctrId, containertypes.RemoveOptions{Force: true})
+	res, err := container.Exec(ctx, c, ctrId, []string{"nslookup", "test.example"})
+	assert.NilError(t, err)
+	assert.Check(t, is.Equal(res.ExitCode, 0))
+	assert.Check(t, is.Contains(res.Stdout(), dnsRespAddr))
+
+	// Connect the container to the internal network as well.
+	// External DNS should still be used.
+	err = c.NetworkConnect(ctx, intNetName, ctrId, nil)
+	assert.NilError(t, err)
+	res, err = container.Exec(ctx, c, ctrId, []string{"nslookup", "test.example"})
+	assert.NilError(t, err)
+	assert.Check(t, is.Equal(res.ExitCode, 0))
+	assert.Check(t, is.Contains(res.Stdout(), dnsRespAddr))
+
+	// Disconnect from the external network.
+	// Expect no access to the external DNS.
+	err = c.NetworkDisconnect(ctx, extNetName, ctrId, true)
+	assert.NilError(t, err)
+	res, err = container.Exec(ctx, c, ctrId, []string{"nslookup", "test.example"})
+	assert.NilError(t, err)
+	assert.Check(t, is.Equal(res.ExitCode, 1))
+	assert.Check(t, is.Contains(res.Stdout(), "SERVFAIL"))
+
+	// Reconnect the external network.
+	// Check that the external DNS server is used again.
+	err = c.NetworkConnect(ctx, extNetName, ctrId, nil)
+	assert.NilError(t, err)
+	res, err = container.Exec(ctx, c, ctrId, []string{"nslookup", "test.example"})
+	assert.NilError(t, err)
+	assert.Check(t, is.Equal(res.ExitCode, 0))
+	assert.Check(t, is.Contains(res.Stdout(), dnsRespAddr))
+}

libnetwork/endpoint.go

@@ -569,8 +569,13 @@ func (ep *Endpoint) sbJoin(sb *Sandbox, options ...EndpointOption) (err error) {
 		return sb.setupDefaultGW()
 	}
 
-	moveExtConn := sb.getGatewayEndpoint() != extEp
+	currentExtEp := sb.getGatewayEndpoint()
+	// Enable upstream forwarding if the sandbox gained external connectivity.
+	if sb.resolver != nil {
+		sb.resolver.SetForwardingPolicy(currentExtEp != nil)
+	}
 
+	moveExtConn := currentExtEp != extEp
 	if moveExtConn {
 		if extEp != nil {
 			log.G(context.TODO()).Debugf("Revoking external connectivity on endpoint %s (%s)", extEp.Name(), extEp.ID())
@@ -764,6 +769,11 @@ func (ep *Endpoint) sbLeave(sb *Sandbox, force bool) error {
 
 	// New endpoint providing external connectivity for the sandbox
 	extEp = sb.getGatewayEndpoint()
+	// Disable upstream forwarding if the sandbox lost external connectivity.
+	if sb.resolver != nil {
+		sb.resolver.SetForwardingPolicy(extEp != nil)
+	}
+
 	if moveExtConn && extEp != nil {
 		log.G(context.TODO()).Debugf("Programming external connectivity on endpoint %s (%s)", extEp.Name(), extEp.ID())
 		extN, err := extEp.getNetworkFromStore()

libnetwork/resolver.go

@@ -9,6 +9,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/containerd/log"
@@ -75,7 +76,7 @@ type Resolver struct {
 	tcpListen     *net.TCPListener
 	err           error
 	listenAddress string
-	proxyDNS      bool
+	proxyDNS      atomic.Bool
 	startCh       chan struct{}
 	logger        *log.Entry
 
@@ -85,15 +86,17 @@ type Resolver struct {
 
 // NewResolver creates a new instance of the Resolver
 func NewResolver(address string, proxyDNS bool, backend DNSBackend) *Resolver {
-	return &Resolver{
+	r := &Resolver{
 		backend:       backend,
-		proxyDNS:      proxyDNS,
 		listenAddress: address,
 		err:           fmt.Errorf("setup not done yet"),
 		startCh:       make(chan struct{}, 1),
 		fwdSem:        semaphore.NewWeighted(maxConcurrent),
 		logInverval:   rate.Sometimes{Interval: logInterval},
 	}
+	r.proxyDNS.Store(proxyDNS)
+
+	return r
 }
 
 func (r *Resolver) log(ctx context.Context) *log.Entry {
@@ -194,6 +197,12 @@ func (r *Resolver) SetExtServers(extDNS []extDNSEntry) {
 	}
 }
 
+// SetForwardingPolicy re-configures the embedded DNS resolver to either enable or disable forwarding DNS queries to
+// external servers.
+func (r *Resolver) SetForwardingPolicy(policy bool) {
+	r.proxyDNS.Store(policy)
+}
+
 // NameServer returns the IP of the DNS resolver for the containers.
 func (r *Resolver) NameServer() string {
 	return r.listenAddress
@@ -421,7 +430,7 @@ func (r *Resolver) serveDNS(w dns.ResponseWriter, query *dns.Msg) {
 		return
 	}
 
-	if r.proxyDNS {
+	if r.proxyDNS.Load() {
 		// If the user sets ndots > 0 explicitly and the query is
 		// in the root domain don't forward it out. We will return
 		// failure and let the client retry with the search domain

libnetwork/sandbox_dns_unix.go

@@ -44,10 +44,11 @@ func (sb *Sandbox) finishInitDNS() error {
 func (sb *Sandbox) startResolver(restore bool) {
 	sb.resolverOnce.Do(func() {
 		var err error
-		// The embedded resolver is always started with proxyDNS set as true, even when the sandbox is only attached to
-		// an internal network. This way, it's the driver responsibility to make sure `connect` syscall fails fast when
-		// no external connectivity is available (eg. by not setting a default gateway).
-		sb.resolver = NewResolver(resolverIPSandbox, true, sb)
+		// The resolver is started with proxyDNS=false if the sandbox does not currently
+		// have a gateway. So, if the Sandbox is only connected to an 'internal' network,
+		// it will not forward DNS requests to external resolvers. The resolver's
+		// proxyDNS setting is then updated as network Endpoints are added/removed.
+		sb.resolver = NewResolver(resolverIPSandbox, sb.getGatewayEndpoint() != nil, sb)
 		defer func() {
 			if err != nil {
 				sb.resolver = nil