GNOME Bugzilla – Bug 762540
RFE: please consider pushing DNS information into systemd-resolved
Last modified: 2016-09-08 10:55:19 UTC
It would be great if NetworkManager could push the DNS information it acquires for the various links it manages into systemd-resolved if that's around. This way, systemd-resolved can manage DNS data properly per-interface, and thus nicely make multiple private zones on multiple interfaces available at the same time. Even better would be if NM would allow DNSSEC configuration in its connection UI, which is as easy as adding a drop-down box with three settings "yes", "no", "allow-downgrade". Also nice would be if NM would disabled DNSSEC unconditionally while probing for captive portals, and resetting this setting as soon as detection of that is complete (after all some captive portal systems modify DNS to redirect traffic to their portal, which conflicts with DNSSEC). Currently systemd-networkd already pushes this information into systemd-resolved, and it would be great if NM would do the same. The APIs for all of these are pretty straight-forward I think, and fully documented here: https://wiki.freedesktop.org/www/Software/systemd/writing-network-configuration-managers/ https://wiki.freedesktop.org/www/Software/systemd/resolved/ Note that this bus API of systemd-resolved is relatively new, only available with v229 and newer. Thus to implement and test this you need a very recent distro that already has v229 (such as rawhide). Note that it should be relatively straightforward to push DNS info into resolved and then fall back to direct /etc/resolv.conf management should it not be available, in order to maintain full compatibility with all systems.
DNSSEC configuration knobs would also be useful for unbound and dnssec-trigger integration. Unbound is a local DNS/DNSSEC server with many capabilities that, together with dnssec-trigger and NetworkManager provides a solution for both split view DNS (aka per-connection or per-interface DNS) and DNSSEC. In addition to avoiding DNSSEC for connectivity probes it is important to actually probe the DNS servers of the specific connection. Otherwise you might get a false positive from another connection and then switch to the new connection while it is still locked up behind a captive portal. For completeness, unbound and dnssec-trigger are reconfigured from NetworkManager's unbound plugin (ignoring the old dispatcher.d based solution), so the support for resolved could be done using a new plugin. When both unbound and resolved are in the system, NetworkManager can use the unbound plugin to configure dnssec-trigger/unbound and resolved would then be configured to query the local unbound. That way you can avoid the need to configure multiple distinct services from NetworkManager and leave any other issues like resolved cache flushing up to dnssec-trigger/unbound integration bits.
resolved cache flushing? resolved maintains separate per-connection caches, which require no explict flushing hence. If an interface shows up, it will have a cache of its own, and if an interface goes away again (or loses its configuration) its cache is destroyed. Caches from an interface A are not tainted by requests made on any interface B, they are maintained fully separately. Hence explicit cache flushing is simply not necessary. In fact much of resolved's logic is maintained per-interface. This means that clients can actually specify the interface index they want a look-up to be made on. Thus, if NM wants to ensure that the captive portal detection checks are done with the DNS servers of specific interfaces, that's trivially easy, simply specify the ifindex when resolving the name. See https://wiki.freedesktop.org/www/Software/systemd/writing-resolver-clients/ for details. Anyway, this RFE is really about hooking up NM directly with resolved, precisely because resolved maintains state, logic, cache and configuration strictly per-interface: we want that NM pushes its per-link DNS configuration into resolved, and we don't lose this *per-link* information half-way. If we wouldn't care about the per-link DNS config info, then /etc/resolv.conf would be sufficient, after all, since /etc/resolv.conf can carry DNS info just fine, but it cannot carry per-link DNS info.
Created attachment 333841 [details] [review] Add systemd-resolved backend Add initial DNS backend that pushes DNS information into systemd-resolved. Backend is choosen by default if the systems resolv.conv is setup to pointing to one of the standard resolved locations. This doesn't handle global dns configuration. Signed-off-by: Sjoerd Simons <sjoerd@luon.net>
fwiw the issue with global dns configration is two-fold. First of, resolved doesn't really have a concept of a global dns in the first place. This could be worked around by setting the global dns servers as the dns servers for any link managed by NetworkManager (unsure how that'd behave nicely when using VPNs but it's the intended semantic). Secondly, NetworkManager has a concept of global dns domains where one can configure a specific list of nameservers for a set of domains. This concept simply doesn't exist in Resolved. Both DNS servers and domain are tied to links, but there is no direct correlation between a DNS server and a domain.
Review of attachment 333841 [details] [review]: Hi, thanks for the patch. Overall, looks very good! Could you try to follow more the coding style of NetworkManager? e.g. - space before '(' + info = g_file_query_info(f, - indent first with tabs, then spaces (in case of line breaks) for example: + info = g_file_query_info(f, + G_FILE_ATTRIBUTE_STANDARD_IS_SYMLINK","\ - no { } blocks for single line + if (NM_IS_IP4_CONFIG (data->config)) { + ifindex = nm_ip4_config_get_ifindex (data->config); + } else i ::: src/dns-manager/nm-dns-manager.c @@ +1621,3 @@ + NULL, NULL); + + if (g_file_info_get_is_symlink(info)) { Must handle that @info may be %NULL. @@ +1624,3 @@ + int i; + + for (i = 0; !ret && resolved_paths[i] != NULL; i++) { ret = _nm_utils_strv_find_first (resolved_paths, G_N_ELEMENTS (resolvd_paths), g_file_info_get_symlink_target(info)) >= 0; ? @@ +1683,3 @@ + if ((!mode && _resolvconf_resolved_managed()) + || nm_streq0 (mode, "systemd-resolved")) { usually, we indent such things differently: \t\tif ( (!mode && _resolvconf_resolved_managed()) \t\t || nm_streq0 (mode, "systemd-resolved")) { ::: src/dns-manager/nm-dns-systemd-resolved.c @@ +30,3 @@ +#include <linux/if.h> + +#include "nm-dns-systemd-resolved.h" could we move this up immediately after "nm-default.h" @@ +128,3 @@ + } + + ic->configs = g_list_append(ic->configs, data->config); is the order here relevant? Maybe g_list_prepend()? If the order is relevant, maybe rename ic->configs to ic->configs_reverse and explicitly track them in reverse. @@ +223,3 @@ + g_variant_builder_end(&dns), + G_DBUS_CALL_FLAGS_NONE, + -1, NULL, call_done, self); Does not take a reference on @self nor passes a cancellable (using a cancellable would be preferred in general). So during call_done(), @self could be already destroyed. In your case, call_done() only logs a message, thus if you ensure that @self is not dereferenced, you don't need to do that. Meaning, fix call_done(): - NMDnsSystemdResolved *self = NM_DNS_SYSTEMD_RESOLVED (user_data); + NMDnsSystemdResolved *self = (NMDnsSystemdResolved) user_data; and add comments to _NMLOG() and call_done() that they must not follow the @self pointer. @@ +293,3 @@ + g_return_if_fail (connection); + + priv->resolve = g_dbus_proxy_new_sync (connection, could we do it async? Preferably pass a cancellable
(In reply to Thomas Haller from comment #5) > Review of attachment 333841 [details] [review] [review]: > > Hi, > > thanks for the patch. Overall, looks very good! > > > Could you try to follow more the coding style of NetworkManager? Is there a indent or clang-format definition for NM's style ooi ? (my bad too many different projects with different coding styles) > @@ +128,3 @@ > + } > + > + ic->configs = g_list_append(ic->configs, data->config); > > is the order here relevant? Maybe g_list_prepend()? > If the order is relevant, maybe rename ic->configs to ic->configs_reverse > and explicitly track them in reverse. N is typically 2 (at most), I don't really think the complexity of g_list_append is that problematic here ;) > > @@ +293,3 @@ > + g_return_if_fail (connection); > + > + priv->resolve = g_dbus_proxy_new_sync (connection, > > could we do it async? Preferably pass a cancellable Probably importantly it needs some error-checking :). I guess i could make it async by lazily creating the proxy at update(rather then at init time). Though that will make the plugin a bit less elegant as it then needs to cache the updates so they can be applied after the dbus proxy is setup. So possible if deemed worthwhile (if we pass the don't autostart at construction flag i wouldn't expect the proxy creation actually needing bus interaction as NM is already on the sytsem bus).
Fwiw the other feature we can't do with the resolved backend at the moment is the DNS server prioritisation introduced in 1.4. Otoh this seems rather redundant as resolved handles the use-case of multiple DNS servers (and multiple active links) in a much nicer way anyway.
(In reply to Sjoerd Simons from comment #6) > Is there a indent or clang-format definition for NM's style ooi ? (my bad > too many different projects with different coding styles) For Emacs users there is: https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/contrib/editors/networkmanager-style.el
(In reply to Sjoerd Simons from comment #6) > (In reply to Thomas Haller from comment #5) > I guess i could make it async [...] if deemed worthwhile When NM starts it creates a lot of proxy instances. There are many possibilities to improve startup there, but new code should not introduce additional delay.
(In reply to Sjoerd Simons from comment #7) > Fwiw the other feature we can't do with the resolved backend at the moment > is the DNS server prioritisation introduced in 1.4. Otoh this seems rather > redundant as resolved handles the use-case of multiple DNS servers (and > multiple active links) in a much nicer way anyway. The priority logic is useful only in non-parallel resolvers like the libc one. Other plugins (like dnsmasq) will simply contact all the servers in parallel. The priority can also be used to allow a connection to set its own name servers in an exclusive way (i.e. the servers from other connections are ignored). This is useful especially for VPNs and can be achieved by setting the property to a negative value. Plugins don't have to care about this as the logic is handled in nm-dns-manager.c.
Created attachment 334850 [details] [review] Add systemd-resolved backend Add initial DNS backend that pushes DNS information into systemd-resolved. Backend is choosen by default if the systems resolv.conv is setup to pointing to one of the standard resolved locations. This doesn't handle global dns configuration. Signed-off-by: Sjoerd Simons <sjoerd@luon.net> Changes since previous patch: * Make init async * Thread cancellable through to the async update calls * Change identation to better match NM style
(In reply to Sjoerd Simons from comment #11) > Created attachment 334850 [details] [review] [review] > Add systemd-resolved backend Hi Sjoerd, looks mostly good to me. Thanks. I pushed it to th/systemd-resolved-bgo762540 for easier review and added a few fixes. https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=th/systemd-resolved-bgo762540 I would be fine with merging th/systemd-resolved-bgo762540 as is. +1.
(In reply to Thomas Haller from comment #12) > I would be fine with merging th/systemd-resolved-bgo762540 as is. > > +1. LGTM. I've added a couple of commits, feel free to squash them if you prefer.
merged to upstream master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=177830831c09b5bb3073da8afd0f804f9f767adc