Add resolver for default bridge, remove default nameservers by robmry · Pull Request #47602 · moby/moby

robmry · 2024-03-20T19:34:29Z

Reverted until we sort out buildkit's DNS...
- see Revert internal resolver for default bridge #48020

addresses [EU GDPR] [Feature Request] Fallback-DNS (Google) should be removed as it is incompatible with GDPR, PCI-DSS, most company policies, doesn't follow secure coding practices etc. #47541

- What I did

Internal resolver for default bridge network

Until now, containers on the default bridge network have been configured to talk directly to external DNS servers - their resolv.conf files have either been populated with nameservers from the host's resolv.conf, or with servers from '--dns' (or with Google's nameservers as a fallback).

This change makes the internal bridge more like other networks by using the internal resolver. But, the internal resolver is not populated with container names or aliases - it's only for external DNS lookups.

Containers on the default network, on a host that has a loopback resolver (like systemd's on 127.0.0.53) will now use that resolver via the internal resolver. So, the logic used to find systemd's current set of resolvers is no longer needed by the daemon.

Legacy links work just as they did before, using '/etc/hosts' and magic.

No default nameservers for internal resolver

Don't fall-back to Google's DNS servers in a network that has an internal resolver.

Now the default bridge uses the internal resolver, the only reason a network started by the daemon should end up without any upstream servers is if the host's resolv.conf doesn't list any. In this case, the '--dns' option can be used to explicitly configure nameservers for a container if necessary.

(Note that buildkit's containers do not have an internal resolver, so they will still set up Google's nameservers if the host has no resolvers that can be used in the container's namespace.)

- How I did it

Start the internal resolver, unless the network is 'host', 'container' or 'none'.

Deleted a bunch of code - but not the 'resolvconf' code that configures the resolver for a no-internal-resolver network, because that's still needed by buildkit.

- How to verify it

Now the default bridge has an internal resolver, there's a danger we could accidentally populate DNS names for its containers - added a regression test to catch that.

- Description for the changelog

Run an internal resolver on the default bridge network to forward DNS requests
to external resolvers, even if they are on localhost addresses, or IPv6 addresses when
the default bridge does not have IPv6 connectivity. To preserve existing behavior, the
internal resolver on the default bridge will not resolve container names, unlike the
resolver on user-defined networks.

Do not use Google's DNS servers as a fallback when no external DNS servers are
supplied in configuration via `--dns` or available from the host's `resolv.conf`.

thaJeztah · 2024-03-21T12:15:27Z

@robmry this is probably changelog-worthy? (and may need "impact/docs" as well?); if so, can you add those labels and a description for the change-log?

robmry · 2024-03-21T12:51:00Z

@robmry this is probably changelog-worthy? (and may need "impact/docs" as well?); if so, can you add those labels and a description for the change-log?

Will do ... I haven't filled in any of the description yet, the PR's still draft - wanted the full test run before committing!

integration-cli/docker_cli_run_test.go

thaJeztah · 2024-05-29T12:04:00Z

integration-cli/docker_cli_run_test.go

+	if actual != "nameserver 127.0.0.11 options ndots:3" {
+		c.Fatalf("expected 'nameserver 127.0.0.11 options ndots:3', but says: %q", actual)
+	}
+	if !strings.Contains(out, "ExtServers: [1.1.1.1]") {


Nothing urgent, but I saw the 1.1.1.1 and for a second thought we still set some default somewhere; but it looks like this is the IP we set as part of the test.

It made me thinking though; should we start practicing "best practice" and use designated example IP-addresses for these; similar to how we use designated example domains? See https://datatracker.ietf.org/doc/html/rfc5737

Yes, a tidy-up of this and other old tests would be good. (I don't think it needs to be tied to this change though.)

For sure; nothing urgent! My curiosity here was if we depended on those resolvers to make the test work (and in that case possibly having CI instability if those resolvers would be unreachable). That doesn't appear to be the case here (we only check the content of the file), but I guess having "designated" IP-addresses could possibly help taking away that confusion

LOL, that said; it's much easier to recognise "example.com" than one of those IP-addresses (we could have a const for them but that may be a bit too much.

thaJeztah · 2024-05-29T12:43:52Z

daemon/config/config_linux.go

-	// ResolvConf is the path to the configuration of the host resolver
-	ResolvConf string `json:"resolv-conf,omitempty"`


For a minute, I thought that an undocumented feature was sneaked in through e353e7e, because all the code seems to indicate that this option is configurable through daemon.json;

it's an exported field, and it has a json label ("resolv-conf")

setupResolvConf checks if the value is set before applying resolvconf.Path

moby/daemon/daemon_linux.go

Lines 139 to 149 in 2ebf191

// setupResolvConf sets the appropriate resolv.conf file if not specified

// When systemd-resolved is running the default /etc/resolv.conf points to

// localhost. In this case fetch the alternative config file that is in a

// different path so that containers can use it

// In all the other cases fallback to the default one

func setupResolvConf(config *config.Config) {

if config.ResolvConf != "" {

return

}

config.ResolvConf = resolvconf.Path()

}

But it looks like we're saved here by the fact that there's no flag defined for the daemon;

mkdir -p /etc/docker cp /etc/resolv.conf /etc/docker/resolv.conf echo '# THIS IS CUSTOM' >> /etc/docker/resolv.conf echo '{"debug": true, "resolv-conf":"/etc/docker/resolv.conf"}' > /etc/docker/daemon.json dockerd --validate unable to configure the Docker daemon with file /etc/docker/daemon.json: the following directives don't match any configuration option: resolv-conf

That said; there's one place this field is currently used, and that's when reloading the daemon config; in that case the daemon logs print the reloaded configuration; is that a problem? Should we still have it visible what the daemon used as resolv.conf ?

kill -s SIGHUP $(pidof dockerd) INFO[2024-05-29T12:40:23.968890416Z] Reloaded configuration: {"pidfile":"/var/run/docker.pid","data-root":"/var/lib/docker","exec-root":"/var/run/docker","group":"docker","max-concurrent-downloads":3,"max-concurrent-uploads":5,"max-download-attempts":5,"shutdown-timeout":15,"debug":true,"hosts":["unix:///var/run/docker.sock"],"log-level":"info","log-format":"text","swarm-default-advertise-addr":"","swarm-raft-heartbeat-tick":0,"swarm-raft-election-tick":0,"metrics-addr":"","host-gateway-ip":"172.18.0.1","log-driver":"json-file","mtu":1500,"ip":"0.0.0.0","icc":true,"iptables":true,"ip-forward":true,"ip-masq":true,"userland-proxy":true,"userland-proxy-path":"/usr/local/bin/docker-proxy","default-address-pools":{"Values":null},"network-control-plane-mtu":1500,"experimental":false,"containerd":"/var/run/docker/containerd/containerd.sock","builder":{"GC":{},"Entitlements":{}},"containerd-namespace":"moby","containerd-plugin-namespace":"plugins.moby","default-runtime":"runc","seccomp-profile":"builtin","default-shm-size":67108864,"default-ipc-mode":"private","default-cgroupns-mode":"host","resolv-conf":"/etc/resolv.conf","proxies":{}}

Sorry - I missed this, or forgot to reply.

It's quite a while since I looked at this PR, so had to remind myself ... before this change, if the host's resolver was systemd's 127.0.0.53 we couldn't point at it from a container's resolv.conf on the default network. So, in that case, we based our config on systemd's alternative resolv.conf.

Now, even on the default network the internal resolver can use the systemd resolver directly, we don't need to know about its upstreams - and we only ever read /etc/resolv.conf (ignoring the undocumented env-var that's intended for tests to use).

So, I don't think continuing to log it would be very useful.

corhere

Should the description be updated to auto-close #47541? Or do we want to leave that issue open until Buildkit is also free from the Google DNS fallback?

corhere · 2024-06-04T18:46:45Z

daemon/container_operations_unix.go

-		// as on the host itself. The container gets a copy of these files.
+	// In host-mode networking, the container does not have its own networking
+	// namespace, so `/etc/hosts` should be the same as on the host itself. Setting
+	// OptionOriginHostsPath means the container will get a copy of the host's file.


The host's hosts file?

Suggested change

// OptionOriginHostsPath means the container will get a copy of the host's file.

// OptionOriginHostsPath means the container will get a copy of `/etc/hosts` from the host filesystem.

corhere · 2024-06-04T18:53:18Z

integration/networking/resolvconf_test.go

+	// Expect the external DNS server to respond to a request from the container.
+	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(), network.DNSRespAddr))
+
+	// Expect the external DNS server to respond to a request from the container
+	// for the container's own name - it won't be recognised as a container name
+	// because there's no service resolution on the default bridge.
+	res, err = container.Exec(ctx, c, ctrId, []string{"nslookup", ctrName})
+	assert.NilError(t, err)
+	assert.Check(t, is.Equal(res.ExitCode, 0))
+	assert.Check(t, is.Contains(res.Stdout(), network.DNSRespAddr))
+
+	// Check that inspect output has no DNSNames for the container.
+	inspect := container.Inspect(ctx, t, c, ctrId)
+	net, ok := inspect.NetworkSettings.Networks[networktypes.NetworkBridge]
+	assert.Check(t, ok, "expected to find bridge network in inspect output")
+	assert.Check(t, is.Nil(net.DNSNames))


These smell like three distinct subtests. It would be nice if the pass/fail status was reported independently for each, to make it easier to narrow down the root cause.

robmry · 2024-06-05T08:50:58Z

Should the description be updated to auto-close #47541? Or do we want to leave that issue open until Buildkit is also free from the Google DNS fallback?

I think it should wait for us to do something about buidkit too ... I've added a comment. (Also, don't want to imply that this change is required for compliance with GDPR or PCI-DSS. Internal advice is that it isn't.)

akerouanton · 2024-06-05T14:02:41Z

libnetwork/sandbox_dns_unix.go

 }

-// Called when an endpoint has joined the sandbox.
-func (sb *Sandbox) updateDNS(ipv6Enabled bool) error {


The windows implementation of this method can also be deleted. See

moby/libnetwork/sandbox_dns_windows.go

Lines 25 to 27 in c6aaabc

func (sb *Sandbox) updateDNS(ipv6Enabled bool) error {

return nil

}

I think the following TODO comment can be updated / removed:

moby/libnetwork/sandbox_dns_unix.go

Lines 366 to 371 in c6aaabc

// TODO(robmry) - I think that's probably accidental, I can't find a reason for it,

// and the old resolvconf.Build() function wrote the file but not the hash, which

// is surprising. But, before fixing it, a guard/flag needs to be added to

// sb.updateDNS() to make sure that when an endpoint joins a sandbox that already

// has an internal resolver, the container's resolv.conf is still (re)configured

// for an internal resolver.

Thank you - done.

Until now, containers on the default bridge network have been configured to talk directly to external DNS servers - their resolv.conf files have either been populated with nameservers from the host's resolv.conf, or with servers from '--dns' (or with Google's nameservers as a fallback). This change makes the internal bridge more like other networks by using the internal resolver. But, the internal resolver is not populated with container names or aliases - it's only for external DNS lookups. Containers on the default network, on a host that has a loopback resolver (like systemd's on 127.0.0.53) will now use that resolver via the internal resolver. So, the logic used to find systemd's current set of resolvers is no longer needed by the daemon. Legacy links work just as they did before, using '/etc/hosts' and magic. (Buildkit does not use libnetwork, so it can't use the internal resolver. But it does use libnetwork/resolvconf's logic to configure resolv.conf. So, code to set up resolv.conf for a legacy networking without an internal resolver can't be removed yet.) Signed-off-by: Rob Murray <[email protected]>

Don't fall-back to Google's DNS servers in a network that has an internal resolver. Now the default bridge uses the internal resolver, the only reason a network started by the daemon should end up without any upstream servers is if the host's resolv.conf doesn't list any. In this case, the '--dns' option can be used to explicitly configure nameservers for a container if necessary. (Note that buildkit's containers do not have an internal resolver, so they will still set up Google's nameservers if the host has no resolvers that can be used in the container's namespace.) Signed-off-by: Rob Murray <[email protected]>

akerouanton

LGTM

robmry force-pushed the internal_resolver_for_default_bridge branch from 59b678a to 6b8bdde Compare March 21, 2024 10:20

robmry self-assigned this Mar 21, 2024

robmry added kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/networking Networking area/networking/dns Networking labels Mar 21, 2024

robmry changed the title ~~Internal resolver for default bridge~~ Add resolver for default bridge, remove default nameservers Mar 21, 2024

robmry added impact/changelog impact/documentation labels Mar 21, 2024

robmry marked this pull request as ready for review March 21, 2024 19:20

robmry requested review from akerouanton and corhere March 21, 2024 19:21

robmry mentioned this pull request Mar 21, 2024

[EU GDPR] [Feature Request] Fallback-DNS (Google) should be removed as it is incompatible with GDPR, PCI-DSS, most company policies, doesn't follow secure coding practices etc. #47541

Open

robmry added this to the 27.0.0 milestone Apr 22, 2024

akerouanton reviewed May 29, 2024

View reviewed changes

integration-cli/docker_cli_run_test.go Outdated Show resolved Hide resolved

robmry force-pushed the internal_resolver_for_default_bridge branch 2 times, most recently from eab8b25 to 38b34a7 Compare May 29, 2024 09:05

thaJeztah reviewed May 29, 2024

View reviewed changes

corhere approved these changes Jun 4, 2024

View reviewed changes

robmry force-pushed the internal_resolver_for_default_bridge branch from 38b34a7 to 6f00ac7 Compare June 5, 2024 08:42

robmry force-pushed the internal_resolver_for_default_bridge branch from 6f00ac7 to d44269e Compare June 5, 2024 11:03

akerouanton reviewed Jun 5, 2024

View reviewed changes

robmry force-pushed the internal_resolver_for_default_bridge branch from d44269e to 630930b Compare June 5, 2024 16:02

robmry added 2 commits June 5, 2024 20:27

robmry force-pushed the internal_resolver_for_default_bridge branch from 630930b to d365702 Compare June 5, 2024 19:27

akerouanton approved these changes Jun 5, 2024

View reviewed changes

akerouanton merged commit f3f20c3 into moby:master Jun 6, 2024

robmry deleted the internal_resolver_for_default_bridge branch June 13, 2024 18:58

This was referenced Jun 17, 2024

docker 27.0.0~rc.1 does not use host dns #48015

Closed

Revert internal resolver for default bridge #48020

Merged

Use internal resolver for default bridge #48027

Open

		// ResolvConf is the path to the configuration of the host resolver
		ResolvConf string `json:"resolv-conf,omitempty"`

	// setupResolvConf sets the appropriate resolv.conf file if not specified
	// When systemd-resolved is running the default /etc/resolv.conf points to
	// localhost. In this case fetch the alternative config file that is in a
	// different path so that containers can use it
	// In all the other cases fallback to the default one
	func setupResolvConf(config *config.Config) {
	if config.ResolvConf != "" {
	return
	}
	config.ResolvConf = resolvconf.Path()
	}

	// OptionOriginHostsPath means the container will get a copy of the host's file.
	// OptionOriginHostsPath means the container will get a copy of `/etc/hosts` from the host filesystem.

	func (sb *Sandbox) updateDNS(ipv6Enabled bool) error {
	return nil
	}

	// TODO(robmry) - I think that's probably accidental, I can't find a reason for it,
	// and the old resolvconf.Build() function wrote the file but not the hash, which
	// is surprising. But, before fixing it, a guard/flag needs to be added to
	// sb.updateDNS() to make sure that when an endpoint joins a sandbox that already
	// has an internal resolver, the container's resolv.conf is still (re)configured
	// for an internal resolver.

Conversation

robmry commented Mar 20, 2024 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Mar 21, 2024

Uh oh!

robmry commented Mar 21, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robmry commented Jun 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akerouanton left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robmry commented Mar 20, 2024 •

edited by thaJeztah

Loading