Let's unify some code, and add a common implementation of a function
that checks whether we have tried all DNS servers yet, and retries the
transaction if we don't. We already use this same code twice. Let's use
it at some other places too now — basically all cases where we switch to
a new server — with the one case of packet loss, where we too switch
servers, but don#t care how many times we already tried to switch.
This adjusts our feature level handling: when DNSSEC strict mode is on,
let's never lower the feature level below the lowest DNSSEC mode.
Also, when asking whether DNSSEC is supproted, always say yes in strict
mode. This means that error reporting about transactions that fail
because of missing DNSSEC RRs will not report "incompatible-server" but
instead "missing-signature" or suchlike.
The main difference here is that DNSSEC failures become local to a
transaction, instead of propagating into the feature level we reuse for
future transactions. This is beneficial with routers that implement
"mostly a DNS proxy", i.e. that propagate most DNS requests 1:1 to their
upstream servers, but synthesize local answers for a select few domains.
For example, AVM Fritz!Boxes operate that way: they proxy most traffic
1:1 upstream in an DNSSEC-compatible fashion, but synthesize the
"fritz.box" locally, so that it can be used to configure the router.
This local domain cannot be DNSSEC verified, it comes without
signatures. Previously this would mean once that domain was resolved
feature level would be downgraded, and we'd thus fail all future DNSSEC
attempts. With this change, the immediate lookup for "fritz.box" will
fail validation, but for all other unrelated future ones that comes
without prejudice.
(While we are at it, also make a couple of other downgrade paths a bit
tighter.)
Fixes: #10570#14435#6490
The levels have an order, but the order is sometimes a bit arbitrary.
Hence add simple macros to check for specific features and use those, so
that the ordering leaks a bit less into all files.
So far we didn't really handle the case where we can't parse a reply
packet. Since this apparently happens in real-life though, let's add
some minimal logic, to downgrade/restart if we see this.
If the cleanup function returns the appropriate type, use that to reset the
variable. For other functions (usually the foreign ones which return void), add
an explicit value to reset to.
This causes a bit of code churn, but I think it might be worth it. In a
following patch static destructors will be called from a fuzzer, and this
change allows them to be called multiple times. But I think such a change might
help with detecting unitialized code reuse too. We hit various bugs like this,
and things are more obvious when a pointer has been set to NULL.
I was worried whether this change increases text size, but it doesn't seem to:
-Dbuildtype=debug:
before "tree-wide: return NULL from freeing functions":
-rwxrwxr-x 1 zbyszek zbyszek 4117672 Feb 16 14:36 build/libsystemd.so.0.30.0*
-rwxrwxr-x 1 zbyszek zbyszek 4494520 Feb 16 15:06 build/systemd*
after "tree-wide: return NULL from freeing functions":
-rwxrwxr-x 1 zbyszek zbyszek 4117672 Feb 16 14:36 build/libsystemd.so.0.30.0*
-rwxrwxr-x 1 zbyszek zbyszek 4494576 Feb 16 15:10 build/systemd*
now:
-rwxrwxr-x 1 zbyszek zbyszek 4117672 Feb 16 14:36 build/libsystemd.so.0.30.0*
-rwxrwxr-x 1 zbyszek zbyszek 4494640 Feb 16 15:15 build/systemd*
-Dbuildtype=release:
before "tree-wide: return NULL from freeing functions":
-rwxrwxr-x 1 zbyszek zbyszek 5252256 Feb 14 14:47 build-rawhide/libsystemd.so.0.30.0*
-rwxrwxr-x 1 zbyszek zbyszek 1834184 Feb 16 15:09 build-rawhide/systemd*
after "tree-wide: return NULL from freeing functions":
-rwxrwxr-x 1 zbyszek zbyszek 5252256 Feb 14 14:47 build-rawhide/libsystemd.so.0.30.0*
-rwxrwxr-x 1 zbyszek zbyszek 1834184 Feb 16 15:10 build-rawhide/systemd*
now:
-rwxrwxr-x 1 zbyszek zbyszek 5252256 Feb 14 14:47 build-rawhide/libsystemd.so.0.30.0*
-rwxrwxr-x 1 zbyszek zbyszek 1834184 Feb 16 15:16 build-rawhide/systemd*
I would expect that the compiler would be able to elide the setting of a
variable if the variable is never used again. And this seems to be the case:
in optimized builds there is no change in size whatsoever. And the change in
size in unoptimized build is negligible.
Something strange is happening with size of libsystemd: it's bigger in
optimized builds. Something to figure out, but unrelated to this patch.
I started working on this because I wanted to change how
DEFINE_TRIVIAL_CLEANUP_FUNC is defined. Even independently of that change, it's
nice to make make things more consistent and predictable.
Let's preferably route traffic for reverse lookups to LLMNR/mDNS/DNS on
the matching interface if the IP address is in the local subnet. Also,
if looking up an IP address of our own host, let's avoid doing
LLMNR/mDNS at all.
This is useful if "~." is a routing domain to DNS, as it means, local
reverse lookups still go to LLMNR/mDNS, too.
Fixes: #16243#10081
_gc() does cleanup if it is possible. So far it returned a bool to
signal if it succeeded (false on success). When working on the resolved
code I had to look at the definition every time, because the (arguably
reversed) calling convention is unobvious. So let's return a pointer
(non-NULL: gc has not been done, NULL: gc has been done).
This fits nicely with the standard to return a pointer from all free
functions obviously.
Let's make things more debuggable: when debug logging is on, let's
say which client is asking for our services.
This is helpful for easily figuring out which local process might
interfere with your debugging sessions by issuing additional requests
while you try to debug a request (I am looking at you, geoclue!).
Let's introduce a new flag that indicates whether the response was
acquired in "confidential" mode, i.e. via encrypted DNS-over-TLS, or
synthesized locally.
Fixes: #12859
Let's use the same flags type we use for client communication, i.e.
instead of "bool answer_authenticated", let's use "uint64_t
answer_query_flags", with the SD_RESOLVED_AUTHENTICATED flag.
This is mostly just search/replace, i.e. a refactoring, no change in
behaviour.
This becomes useful once in a later commit SD_RESOLVED_CONFIDENTIAL is
added to indicate resolution that either were encrypted (DNS-over-TLS)
or never left the local system.
When low-level RR resolution is requested from "resolvectl query" via
"--type=" or "--class=" no search domain logic is applied and no IDNA
translation.
Explain this in detail in the documentation, and also mentions this when
users attempt to resolve single-label names or names with international
characters in the output.
I believe the current behaviour is correct, but it is indeed surprising.
Hence the documentation and output improvement.
Fixes: #11325#10737
The "socket graveyard" shall contain sockets we have sent a question out
of, but not received a reply. If we'd close thus sockets immediately
when we are not interested anymore, we'd trigger ICMP port unreachable
messages once we after all *do* get a reply. Let's avoid that, by
leaving the fds open for a bit longer, until a timeout is reached or a
reply datagram received.
Fixes: #17421
When a transaction fails and we decide to switch DNS servers, don#t do
so unconditionally. Check if the current DNS server is still the same as
when the transaction was initiated. And if not, do not do anything.
That should reduce the number of redundant DNS server switches if many
parallel transactions fail simultaneously (which is pretty likely if
DNSSEC is on).
Fixes: #17040
Let's reuse accept_link_local_reverse_lookups() at one more place, where
we check for the list of link local reverase address domains. Since we
don't actually accept the domains here (but rather the opposite, not
accept), let's rename the function a bit more generically with accept_ →
match_.
While we are at it invert the if branches, to make things more easily
understandable: filter out the unwatnted stuff and have the "all good"
state as main codepath.
Let's suppress repeated stub queries coming in, to minimize resource
usage. Many DNS clients are pretty aggressive regarding repeating DNS
requests, hence let's find them and suppress the follow-ups should we
need more time to fulfill the queries.
A previous commit made sure that when one of our own packets is looped
back to us, we ignore it. But let's go one step further, and refuse
operation if we notice the server we talk to is our own. This way we
won't generate unnecessary traffic and can return a cleaner error.
Fixes: #17413
Let's be more precise in naming this function, after all this doesn#t
actually check if the packet is really ours, but just that the source IP
address is a local one. Hence name it that way.
(This is preparation to add a helper that checks if packet belongs to
local transaction later on)
Let's add some overflow checks. Also, if 0 records are reserved, use
this as indication that a copy shall be done and do not grow the answer
beyond the current size.
Apparently, there are plenty routers in place that report an incorrect
RR count in the packets: they declare more RRs than are actually
included.
Let's accept these responses, but let's downgrade them to baseline, i.e.
let's suppress OPT in this case: if they don't even get the RR count
right, let's operate on the absolute baseline, and not bother with
anything fancier such as EDNS.
Prompted-by: https://github.com/systemd/systemd/issues/12841#issuecomment-724063973Fixes: #3980
Most likely fixes: #12841
LLMNR/mDNS don't support DNSSEC, hence there's no point in routing any
lookups asking for DNSSEC there.
This speeds up looking up DNSSEC RRs for top-level domains, since we
don't have to wait for LLMNR to complete.
The implementation is a bit ugly because we set the output variable
twice. But we were already doing this on error, so this is not
significantly worse. Doing this allows us to avoid goto's, and the
compiler should be able to figure this out and only set once.
As suggested in https://github.com/systemd/systemd/pull/11484#issuecomment-775288617.
This does not touch anything exposed in src/systemd. Changing the defines there
would be a compatibility break.
Note that tests are broken after this commit. They will be fixed in the next one.
Some folks argue that NOTIMP should only be returned if a specific QTYPE
is not supported. While I don#t think the RFCs are too clear about that,
let's use REFUSED instead, which appears to be a less controversial
choice of error code.
Prompted-by: #17218
Benjamin Robin
Lennart Poettering
Zbigniew Jędrzejewski-Szmek
Yu Watanabe