From df9578498f3f566409fcb71229d9fc99e4ab0568 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 20 Apr 2020 11:02:39 +0200 Subject: [PATCH 1/7] resolve: allow setting the log level dynamically as in pid1 This is useful to raise the log level for a single transaction or a few, without affecting other state of the resolved as a restart would. The log level can only be set, I didn't bother with having the ability to restore the original as in pid1. --- man/org.freedesktop.resolve1.xml | 7 +++++ man/resolvectl.xml | 1 + man/systemctl.xml | 2 +- src/resolve/resolvectl.c | 43 +++++++++++++++++++++++++ src/resolve/resolved-bus.c | 54 +++++++++++++++++++++++++++++++- 5 files changed, 105 insertions(+), 2 deletions(-) diff --git a/man/org.freedesktop.resolve1.xml b/man/org.freedesktop.resolve1.xml index 83ab0ed3d9..3a01a3a8ee 100644 --- a/man/org.freedesktop.resolve1.xml +++ b/man/org.freedesktop.resolve1.xml @@ -145,6 +145,9 @@ node /org/freedesktop/resolve1 { readonly as DNSSECNegativeTrustAnchors = ['...', ...]; @org.freedesktop.DBus.Property.EmitsChangedSignal("false") readonly s DNSStubListener = '...'; + @org.freedesktop.DBus.Property.EmitsChangedSignal("false") + @org.freedesktop.systemd1.Privileged("true") + readwrite s LogLevel = '...'; }; interface org.freedesktop.DBus.Peer { ... }; interface org.freedesktop.DBus.Introspectable { ... }; @@ -460,6 +463,10 @@ node /org/freedesktop/resolve1 { which DNS is configured and for the system-wide settings if there are any. Note that systemd-resolved assumes DNSSEC is supported by DNS servers until it verifies that this is not the case. Thus, the reported value may initially be true, until the first transactions are executed. + + The LogLevel property shows the (maximum) log level of the manager, with the + same values as the option described in + systemd1. diff --git a/man/resolvectl.xml b/man/resolvectl.xml index 594e22c03f..75be5fe072 100644 --- a/man/resolvectl.xml +++ b/man/resolvectl.xml @@ -175,6 +175,7 @@ automatically, an explicit reverting is not necessary in that case. + diff --git a/man/systemctl.xml b/man/systemctl.xml index fe8b77423a..30880b4110 100644 --- a/man/systemctl.xml +++ b/man/systemctl.xml @@ -1067,7 +1067,7 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err - + log-level [LEVEL] If no argument is given, print the current log level of the manager. If an diff --git a/src/resolve/resolvectl.c b/src/resolve/resolvectl.c index f20e8c44b8..831901e9b4 100644 --- a/src/resolve/resolvectl.c +++ b/src/resolve/resolvectl.c @@ -2516,6 +2516,48 @@ static int verb_revert_link(int argc, char **argv, void *userdata) { return 0; } +static int verb_log_level(int argc, char *argv[], void *userdata) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + sd_bus *bus = userdata; + int r; + + assert(bus); + + if (argc == 1) { + _cleanup_free_ char *level = NULL; + + r = sd_bus_get_property_string( + bus, + "org.freedesktop.resolve1", + "/org/freedesktop/resolve1", + "org.freedesktop.resolve1.Manager", + "LogLevel", + &error, + &level); + if (r < 0) + return log_error_errno(r, "Failed to get log level: %s", bus_error_message(&error, r)); + + puts(level); + + } else { + assert(argc == 2); + + r = sd_bus_set_property( + bus, + "org.freedesktop.resolve1", + "/org/freedesktop/resolve1", + "org.freedesktop.resolve1.Manager", + "LogLevel", + &error, + "s", + argv[1]); + if (r < 0) + return log_error_errno(r, "Failed to set log level: %s", bus_error_message(&error, r)); + } + + return 0; +} + static void help_protocol_types(void) { if (arg_legend) puts("Known protocol types:"); @@ -3190,6 +3232,7 @@ static int native_main(int argc, char *argv[], sd_bus *bus) { { "dnssec", VERB_ANY, 3, 0, verb_dnssec }, { "nta", VERB_ANY, VERB_ANY, 0, verb_nta }, { "revert", VERB_ANY, 2, 0, verb_revert_link }, + { "log-level", VERB_ANY, 2, 0, verb_log_level }, {} }; diff --git a/src/resolve/resolved-bus.c b/src/resolve/resolved-bus.c index 0f60c42963..badb0ee739 100644 --- a/src/resolve/resolved-bus.c +++ b/src/resolve/resolved-bus.c @@ -16,6 +16,7 @@ #include "socket-netlink.h" #include "stdio-util.h" #include "strv.h" +#include "syslog-util.h" #include "user-util.h" #include "utf8.h" @@ -1835,6 +1836,57 @@ static int bus_method_unregister_service(sd_bus_message *message, void *userdata return call_dnssd_method(m, message, bus_dnssd_method_unregister, error); } +static int property_get_log_level( + sd_bus *bus, + const char *path, + const char *interface, + const char *property, + sd_bus_message *reply, + void *userdata, + sd_bus_error *error) { + + _cleanup_free_ char *t = NULL; + int r; + + assert(bus); + assert(reply); + + r = log_level_to_string_alloc(log_get_max_level(), &t); + if (r < 0) + return r; + + return sd_bus_message_append(reply, "s", t); +} + +static int property_set_log_level( + sd_bus *bus, + const char *path, + const char *interface, + const char *property, + sd_bus_message *value, + void *userdata, + sd_bus_error *error) { + + const char *t; + int r; + + assert(bus); + assert(value); + + r = sd_bus_message_read(value, "s", &t); + if (r < 0) + return r; + + r = log_level_from_string(t); + if (r < 0) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid log level '%s'", t); + + log_info("Setting log level to %s.", t); + log_set_max_level(r); + + return 0; +} + static const sd_bus_vtable resolve_vtable[] = { SD_BUS_VTABLE_START(0), SD_BUS_PROPERTY("LLMNRHostname", "s", NULL, offsetof(Manager, llmnr_hostname), SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE), @@ -1853,7 +1905,7 @@ static const sd_bus_vtable resolve_vtable[] = { SD_BUS_PROPERTY("DNSSECNegativeTrustAnchors", "as", bus_property_get_ntas, 0, 0), SD_BUS_PROPERTY("DNSStubListener", "s", bus_property_get_dns_stub_listener_mode, offsetof(Manager, dns_stub_listener_mode), 0), - + SD_BUS_WRITABLE_PROPERTY("LogLevel", "s", property_get_log_level, property_set_log_level, 0, 0), SD_BUS_METHOD_WITH_NAMES("ResolveHostname", "isit", From 7f25507647a15989fe74200504b64d29a77607b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 20 Apr 2020 11:03:11 +0200 Subject: [PATCH 2/7] man: add forgotten tags around a paragaph in resolve1(5) --- man/org.freedesktop.resolve1.xml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/man/org.freedesktop.resolve1.xml b/man/org.freedesktop.resolve1.xml index 3a01a3a8ee..57496b8f2f 100644 --- a/man/org.freedesktop.resolve1.xml +++ b/man/org.freedesktop.resolve1.xml @@ -409,10 +409,10 @@ node /org/freedesktop/resolve1 { Properties - LLMNRHostname contains the hostname currently exposed on the network via LLMNR. It - usually follows the system hostname as may be queried via + LLMNRHostname contains the hostname currently exposed on the network via + LLMNR. It usually follows the system hostname as may be queried via gethostname3, - but may differ if a conflict is detected on the network. + but may differ if a conflict is detected on the network. DNS contains an array of all DNS servers currently used by systemd-resolved. It contains similar information as the DNS server data written to From 6daebf9e4a964a47ef46709a4782d7768a483d87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 20 Apr 2020 12:41:49 +0200 Subject: [PATCH 3/7] TODO: add a hypothetical --- TODO | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/TODO b/TODO index 9c40362b09..b2fb1262af 100644 --- a/TODO +++ b/TODO @@ -821,6 +821,10 @@ Features: * teach ConditionKernelCommandLine= globs or regexes (in order to match foobar={no,0,off}) +* Add ConditionDirectoryNotEmpty= handle non-absoute paths as a search path or add + ConditionConfigSearchPathNotEmpty= or different syntax? See the discussion starting at + https://github.com/systemd/systemd/pull/15109#issuecomment-607740136. + * BootLoaderSpec: Clarify that the kernel has to be in $BOOT. Clarify that the boot loader should be installed to the ESP. Define a way how an installer can figure out whether a BLS compliant boot loader From e53b8cc5216742f919dff35d9c86397b06d5f33d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 20 Apr 2020 13:52:35 +0200 Subject: [PATCH 4/7] resolved: return org.freedesktop.resolve1.DnsError.NXDOMAIN on LLMNR resolution failure Fixes #14922. --- src/resolve/resolved-dns-transaction.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index fb54d160da..5898308d5f 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -1350,7 +1350,16 @@ static int dns_transaction_prepare(DnsTransaction *t, usec_t ts) { } if (t->n_attempts >= TRANSACTION_ATTEMPTS_MAX(t->scope->protocol)) { - dns_transaction_complete(t, DNS_TRANSACTION_ATTEMPTS_MAX_REACHED); + DnsTransactionState result; + + if (t->scope->protocol == DNS_PROTOCOL_LLMNR) + /* If we didn't find anything on LLMNR, it's not an error, but a failure to resolve + * the name. */ + result = DNS_TRANSACTION_NOT_FOUND; + else + result = DNS_TRANSACTION_ATTEMPTS_MAX_REACHED; + + dns_transaction_complete(t, result); return 0; } From 5c35cd5f47a17719773fcf02cafa79ed15f44d68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 20 Apr 2020 14:14:09 +0200 Subject: [PATCH 5/7] resolved: include actual path in error message An error with a full path is immediately clear. OTOH, a user might not be familiar with concenpt like "private resolv.conf". I opted to use %s-formatting for the path, because the code is much easier to read this way. Any difference in t speed of execution is not important. --- src/resolve/resolved-resolv-conf.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/resolve/resolved-resolv-conf.c b/src/resolve/resolved-resolv-conf.c index f5fc13563d..763fc09740 100644 --- a/src/resolve/resolved-resolv-conf.c +++ b/src/resolve/resolved-resolv-conf.c @@ -355,35 +355,35 @@ int manager_write_resolv_conf(Manager *m) { r = fopen_temporary_label(PRIVATE_UPLINK_RESOLV_CONF, PRIVATE_UPLINK_RESOLV_CONF, &f_uplink, &temp_path_uplink); if (r < 0) - return log_warning_errno(r, "Failed to open private resolv.conf file for writing: %m"); + return log_warning_errno(r, "Failed to open new %s for writing: %m", PRIVATE_UPLINK_RESOLV_CONF); (void) fchmod(fileno(f_uplink), 0644); r = fopen_temporary_label(PRIVATE_STUB_RESOLV_CONF, PRIVATE_STUB_RESOLV_CONF, &f_stub, &temp_path_stub); if (r < 0) - return log_warning_errno(r, "Failed to open private stub-resolv.conf file for writing: %m"); + return log_warning_errno(r, "Failed to open new %s for writing: %m", PRIVATE_STUB_RESOLV_CONF); (void) fchmod(fileno(f_stub), 0644); r = write_uplink_resolv_conf_contents(f_uplink, dns, domains); if (r < 0) { - log_error_errno(r, "Failed to write private resolv.conf contents: %m"); + log_error_errno(r, "Failed to write new %s: %m", PRIVATE_UPLINK_RESOLV_CONF); goto fail; } if (rename(temp_path_uplink, PRIVATE_UPLINK_RESOLV_CONF) < 0) { - r = log_error_errno(errno, "Failed to move private resolv.conf file into place: %m"); + r = log_error_errno(errno, "Failed to move new %s into place: %m", PRIVATE_UPLINK_RESOLV_CONF); goto fail; } r = write_stub_resolv_conf_contents(f_stub, dns, domains); if (r < 0) { - log_error_errno(r, "Failed to write private stub-resolv.conf contents: %m"); + log_error_errno(r, "Failed to write new %s: %m", PRIVATE_STUB_RESOLV_CONF); goto fail; } if (rename(temp_path_stub, PRIVATE_STUB_RESOLV_CONF) < 0) { - r = log_error_errno(errno, "Failed to move private stub-resolv.conf file into place: %m"); + r = log_error_errno(errno, "Failed to move new %s into place: %m", PRIVATE_STUB_RESOLV_CONF); goto fail; } From 965228a846146f368f185ed9af5273b484ba8b5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 20 Apr 2020 14:27:44 +0200 Subject: [PATCH 6/7] resolve: when writing of private resolv.confs fails, do not remove old copies All callers ignore the return value. This is almost entirely theoretical, since writing to /run is unlikely to fail..., but the user is almost certainly better off keeping the old copy around and having working dns resolution with an out-of-date dns server list than having having a dangling /etc/resolv.conf symlink. --- src/resolve/resolved-resolv-conf.c | 33 ++++++++++++++---------------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/src/resolve/resolved-resolv-conf.c b/src/resolve/resolved-resolv-conf.c index 763fc09740..c060ccd714 100644 --- a/src/resolve/resolved-resolv-conf.c +++ b/src/resolve/resolved-resolv-conf.c @@ -359,41 +359,38 @@ int manager_write_resolv_conf(Manager *m) { (void) fchmod(fileno(f_uplink), 0644); - r = fopen_temporary_label(PRIVATE_STUB_RESOLV_CONF, PRIVATE_STUB_RESOLV_CONF, &f_stub, &temp_path_stub); - if (r < 0) - return log_warning_errno(r, "Failed to open new %s for writing: %m", PRIVATE_STUB_RESOLV_CONF); - - (void) fchmod(fileno(f_stub), 0644); - r = write_uplink_resolv_conf_contents(f_uplink, dns, domains); if (r < 0) { log_error_errno(r, "Failed to write new %s: %m", PRIVATE_UPLINK_RESOLV_CONF); goto fail; } - if (rename(temp_path_uplink, PRIVATE_UPLINK_RESOLV_CONF) < 0) { - r = log_error_errno(errno, "Failed to move new %s into place: %m", PRIVATE_UPLINK_RESOLV_CONF); + r = fopen_temporary_label(PRIVATE_STUB_RESOLV_CONF, PRIVATE_STUB_RESOLV_CONF, &f_stub, &temp_path_stub); + if (r < 0) { + log_warning_errno(r, "Failed to open new %s for writing: %m", PRIVATE_STUB_RESOLV_CONF); goto fail; } + (void) fchmod(fileno(f_stub), 0644); + r = write_stub_resolv_conf_contents(f_stub, dns, domains); if (r < 0) { log_error_errno(r, "Failed to write new %s: %m", PRIVATE_STUB_RESOLV_CONF); goto fail; } - if (rename(temp_path_stub, PRIVATE_STUB_RESOLV_CONF) < 0) { + if (rename(temp_path_uplink, PRIVATE_UPLINK_RESOLV_CONF) < 0) + r = log_error_errno(errno, "Failed to move new %s into place: %m", PRIVATE_UPLINK_RESOLV_CONF); + + if (rename(temp_path_stub, PRIVATE_STUB_RESOLV_CONF) < 0) r = log_error_errno(errno, "Failed to move new %s into place: %m", PRIVATE_STUB_RESOLV_CONF); - goto fail; + + fail: + if (r < 0) { + /* Something went wrong, perform cleanup... */ + (void) unlink(temp_path_uplink); + (void) unlink(temp_path_stub); } - return 0; - -fail: - (void) unlink(PRIVATE_UPLINK_RESOLV_CONF); - (void) unlink(temp_path_uplink); - (void) unlink(PRIVATE_STUB_RESOLV_CONF); - (void) unlink(temp_path_stub); - return r; } From ca8b81d923f34ec4bdaafe472f083059873b9793 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 20 Apr 2020 15:22:18 +0200 Subject: [PATCH 7/7] resolve: when the stub listener is disabled, symlink stub-resolv.conf to resolv.conf When the stub listener is disabled, stub-resolv.conf is useless. Instead of warning about this, let's just make stub-resolv.conf point to the private resolv.conf file. (The original bug report asked for "mirroring", but I think a symlink is nicer than a copy because it is easier to see that a redirection was made.) Fixes #14700. --- src/resolve/resolved-resolv-conf.c | 65 ++++++++++++++---------------- 1 file changed, 31 insertions(+), 34 deletions(-) diff --git a/src/resolve/resolved-resolv-conf.c b/src/resolve/resolved-resolv-conf.c index c060ccd714..6afc193f69 100644 --- a/src/resolve/resolved-resolv-conf.c +++ b/src/resolve/resolved-resolv-conf.c @@ -9,6 +9,7 @@ #include "dns-domain.h" #include "fd-util.h" #include "fileio.h" +#include "fs-util.h" #include "ordered-set.h" #include "resolved-conf.h" #include "resolved-dns-server.h" @@ -27,41 +28,30 @@ #define PRIVATE_STATIC_RESOLV_CONF ROOTLIBEXECDIR "/resolv.conf" int manager_check_resolv_conf(const Manager *m) { - const char *path; - struct stat st; - int r; + struct stat st, own; assert(m); /* This warns only when our stub listener is disabled and /etc/resolv.conf is a symlink to - * PRIVATE_STATIC_RESOLV_CONF or PRIVATE_STUB_RESOLV_CONF. */ + * PRIVATE_STATIC_RESOLV_CONF. */ if (m->dns_stub_listener_mode != DNS_STUB_LISTENER_NO) return 0; - r = stat("/etc/resolv.conf", &st); - if (r < 0) { + if (stat("/etc/resolv.conf", &st) < 0) { if (errno == ENOENT) return 0; return log_warning_errno(errno, "Failed to stat /etc/resolv.conf: %m"); } - FOREACH_STRING(path, - PRIVATE_STUB_RESOLV_CONF, - PRIVATE_STATIC_RESOLV_CONF) { - - struct stat own; - - /* Is it symlinked to our own uplink file? */ - if (stat(path, &own) >= 0 && - st.st_dev == own.st_dev && - st.st_ino == own.st_ino) { - log_warning("DNSStubListener= is disabled, but /etc/resolv.conf is a symlink to %s " - "which expects DNSStubListener= to be enabled.", path); - return -EOPNOTSUPP; - } - } + /* Is it symlinked to our own uplink file? */ + if (stat(PRIVATE_STATIC_RESOLV_CONF, &own) >= 0 && + st.st_dev == own.st_dev && + st.st_ino == own.st_ino) + return log_warning_errno(SYNTHETIC_ERRNO(EOPNOTSUPP), + "DNSStubListener= is disabled, but /etc/resolv.conf is a symlink to " + PRIVATE_STATIC_RESOLV_CONF " which expects DNSStubListener= to be enabled."); return 0; } @@ -365,26 +355,33 @@ int manager_write_resolv_conf(Manager *m) { goto fail; } - r = fopen_temporary_label(PRIVATE_STUB_RESOLV_CONF, PRIVATE_STUB_RESOLV_CONF, &f_stub, &temp_path_stub); - if (r < 0) { - log_warning_errno(r, "Failed to open new %s for writing: %m", PRIVATE_STUB_RESOLV_CONF); - goto fail; - } + if (m->dns_stub_listener_mode != DNS_STUB_LISTENER_NO) { + r = fopen_temporary_label(PRIVATE_STUB_RESOLV_CONF, PRIVATE_STUB_RESOLV_CONF, &f_stub, &temp_path_stub); + if (r < 0) { + log_warning_errno(r, "Failed to open new %s for writing: %m", PRIVATE_STUB_RESOLV_CONF); + goto fail; + } - (void) fchmod(fileno(f_stub), 0644); + (void) fchmod(fileno(f_stub), 0644); - r = write_stub_resolv_conf_contents(f_stub, dns, domains); - if (r < 0) { - log_error_errno(r, "Failed to write new %s: %m", PRIVATE_STUB_RESOLV_CONF); - goto fail; + r = write_stub_resolv_conf_contents(f_stub, dns, domains); + if (r < 0) { + log_error_errno(r, "Failed to write new %s: %m", PRIVATE_STUB_RESOLV_CONF); + goto fail; + } + + if (rename(temp_path_stub, PRIVATE_STUB_RESOLV_CONF) < 0) + r = log_error_errno(errno, "Failed to move new %s into place: %m", PRIVATE_STUB_RESOLV_CONF); + + } else { + r = symlink_atomic(basename(PRIVATE_UPLINK_RESOLV_CONF), PRIVATE_STUB_RESOLV_CONF); + if (r < 0) + log_error_errno(r, "Failed to symlink %s: %m", PRIVATE_STUB_RESOLV_CONF); } if (rename(temp_path_uplink, PRIVATE_UPLINK_RESOLV_CONF) < 0) r = log_error_errno(errno, "Failed to move new %s into place: %m", PRIVATE_UPLINK_RESOLV_CONF); - if (rename(temp_path_stub, PRIVATE_STUB_RESOLV_CONF) < 0) - r = log_error_errno(errno, "Failed to move new %s into place: %m", PRIVATE_STUB_RESOLV_CONF); - fail: if (r < 0) { /* Something went wrong, perform cleanup... */