diff options
author | Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> | 2021-03-25 21:31:27 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-03-25 21:31:27 +0100 |
commit | 7eafbd4270a6ee29371f98b9aa1eeef7ab4560e0 (patch) | |
tree | bf43b16c2cb8140a9f0dfae4dd7909545be2a575 | |
parent | 6f4c93259e4fbe0ba92c525259fb3b964a82c982 (diff) | |
parent | 915ba31cfd6b87e9f3dd31f2bf5bd0b5c252738e (diff) | |
download | systemd-7eafbd4270a6ee29371f98b9aa1eeef7ab4560e0.tar.gz |
Merge pull request #19112 from poettering/more-stub-fixes
resolved: two more tweaks to the stub
-rw-r--r-- | src/resolve/resolved-bus.c | 20 | ||||
-rw-r--r-- | src/resolve/resolved-dns-query.c | 82 | ||||
-rw-r--r-- | src/resolve/resolved-dns-query.h | 5 | ||||
-rw-r--r-- | src/resolve/resolved-dns-stub.c | 175 | ||||
-rw-r--r-- | src/resolve/resolved-varlink.c | 8 |
5 files changed, 173 insertions, 117 deletions
diff --git a/src/resolve/resolved-bus.c b/src/resolve/resolved-bus.c index 032ed0256b..c3624669ce 100644 --- a/src/resolve/resolved-bus.c +++ b/src/resolve/resolved-bus.c @@ -195,14 +195,14 @@ static void bus_method_resolve_hostname_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = sd_bus_reply_method_errorf(q->bus_request, BUS_ERROR_CNAME_LOOP, "CNAME loop detected, or CNAME resolving disabled on '%s'", dns_query_string(q)); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; r = sd_bus_message_new_method_return(q->bus_request, &reply); @@ -486,14 +486,14 @@ static void bus_method_resolve_address_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = sd_bus_reply_method_errorf(q->bus_request, BUS_ERROR_CNAME_LOOP, "CNAME loop detected, or CNAME resolving disabled on '%s'", dns_query_string(q)); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; r = sd_bus_message_new_method_return(q->bus_request, &reply); @@ -660,14 +660,14 @@ static void bus_method_resolve_record_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = sd_bus_reply_method_errorf(q->bus_request, BUS_ERROR_CNAME_LOOP, "CNAME loop detected, or CNAME resolving disabled on '%s'", dns_query_string(q)); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; r = sd_bus_message_new_method_return(q->bus_request, &reply); @@ -1107,8 +1107,8 @@ static void resolve_service_hostname_complete(DnsQuery *q) { return; } - r = dns_query_process_cname(q); - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + r = dns_query_process_cname_many(q); + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; /* This auxiliary lookup is finished or failed, let's see if all are finished now. */ @@ -1180,14 +1180,14 @@ static void bus_method_resolve_service_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = sd_bus_reply_method_errorf(q->bus_request, BUS_ERROR_CNAME_LOOP, "CNAME loop detected, or CNAME resolving disabled on '%s'", dns_query_string(q)); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; question = dns_query_question_for_protocol(q, q->answer_protocol); diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c index 8bc0607983..3a908a6b54 100644 --- a/src/resolve/resolved-dns-query.c +++ b/src/resolve/resolved-dns-query.c @@ -1036,7 +1036,7 @@ static int dns_query_cname_redirect(DnsQuery *q, const DnsResourceRecord *cname) return 0; } -int dns_query_process_cname(DnsQuery *q) { +int dns_query_process_cname_one(DnsQuery *q) { _cleanup_(dns_resource_record_unrefp) DnsResourceRecord *cname = NULL; DnsQuestion *question; DnsResourceRecord *rr; @@ -1046,6 +1046,22 @@ int dns_query_process_cname(DnsQuery *q) { assert(q); + /* Processes a CNAME redirect if there's one. Returns one of three values: + * + * CNAME_QUERY_MATCH → direct RR match, caller should just use the RRs in this answer (and not + * bother with any CNAME/DNAME stuff) + * + * CNAME_QUERY_NOMATCH → no match at all, neither direct nor CNAME/DNAME, caller might decide to + * restart query or take things as NODATA reply. + * + * CNAME_QUERY_CNAME → no direct RR match, but a CNAME/DNAME match that we now followed for one step. + * + * The function might also return a failure, in particular -ELOOP if we encountered too many + * CNAMEs/DNAMEs in a chain or if following CNAMEs/DNAMEs was turned off. + * + * Note that this function doesn't actually restart the query. The caller can decide to do that in + * case of CNAME_QUERY_CNAME, though. */ + if (!IN_SET(q->state, DNS_TRANSACTION_SUCCESS, DNS_TRANSACTION_NULL)) return DNS_QUERY_NOMATCH; @@ -1112,17 +1128,63 @@ int dns_query_process_cname(DnsQuery *q) { if (r < 0) return r; - /* Let's see if the answer can already answer the new redirected question */ - r = dns_query_process_cname(q); - if (r != DNS_QUERY_NOMATCH) - return r; + return DNS_QUERY_CNAME; /* Tell caller that we did a single CNAME/DNAME redirection step */ +} - /* OK, it cannot, let's begin with the new query */ - r = dns_query_go(q); - if (r < 0) - return r; +int dns_query_process_cname_many(DnsQuery *q) { + int r; - return DNS_QUERY_RESTARTED; /* We restarted the query for a new cname */ + assert(q); + + /* Follows CNAMEs through the current packet: as long as the current packet can fulfill our + * redirected CNAME queries we keep going, and restart the query once the current packet isn't good + * enough anymore. It's a wrapper around dns_query_process_cname_one() and returns the same values, + * but with extended semantics. Specifically: + * + * DNS_QUERY_MATCH → as above + * + * DNS_QUERY_CNAME → we ran into a CNAME/DNAME redirect that we could not answer from the current + * message, and thus restarted the query to resolve it. + * + * DNS_QUERY_NOMATCH → we reached the end of CNAME/DNAME chain, and there are no direct matches nor a + * CNAME/DNAME match. i.e. this is a NODATA case. + * + * Note that this function will restart the query for the caller if needed, and that's the case + * DNS_QUERY_CNAME is returned. + */ + + r = dns_query_process_cname_one(q); + if (r != DNS_QUERY_CNAME) + return r; /* The first redirect is special: if it doesn't answer the question that's no + * reason to restart the query, we just accept this as a NODATA answer. */ + + for (;;) { + r = dns_query_process_cname_one(q); + if (r < 0 || r == DNS_QUERY_MATCH) + return r; + if (r == DNS_QUERY_NOMATCH) { + /* OK, so we followed one or more CNAME/DNAME RR but the existing packet can't answer + * this. Let's restart the query hence, with the new question. Why the different + * handling than the first chain element? Because if the server answers a direct + * question with an empty answer then this is a NODATA response. But if it responds + * with a CNAME chain that ultimately is incomplete (i.e. a non-empty but truncated + * CNAME chain) then we better follow up ourselves and ask for the rest of the + * chain. This is particular relevant since our cache will store CNAME/DNAME + * redirects that we learnt about for lookups of certain DNS types, but later on we + * can reuse this data even for other DNS types, but in that case need to follow up + * with the final lookup of the chain ourselves with the RR type we ourselves are + * interested in. */ + r = dns_query_go(q); + if (r < 0) + return r; + + return DNS_QUERY_CNAME; + } + + /* So we found a CNAME that the existing packet already answers, again via a CNAME, let's + * continue going then. */ + assert(r == DNS_QUERY_CNAME); + } } DnsQuestion* dns_query_question_for_protocol(DnsQuery *q, DnsProtocol protocol) { diff --git a/src/resolve/resolved-dns-query.h b/src/resolve/resolved-dns-query.h index 5d96cc06f8..39bf341d68 100644 --- a/src/resolve/resolved-dns-query.h +++ b/src/resolve/resolved-dns-query.h @@ -112,7 +112,7 @@ struct DnsQuery { enum { DNS_QUERY_MATCH, DNS_QUERY_NOMATCH, - DNS_QUERY_RESTARTED, + DNS_QUERY_CNAME, }; DnsQueryCandidate* dns_query_candidate_ref(DnsQueryCandidate*); @@ -129,7 +129,8 @@ int dns_query_make_auxiliary(DnsQuery *q, DnsQuery *auxiliary_for); int dns_query_go(DnsQuery *q); void dns_query_ready(DnsQuery *q); -int dns_query_process_cname(DnsQuery *q); +int dns_query_process_cname_one(DnsQuery *q); +int dns_query_process_cname_many(DnsQuery *q); void dns_query_complete(DnsQuery *q, DnsTransactionState state); diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index b6d14b9305..177b88e025 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -161,89 +161,40 @@ static int dns_stub_collect_answer_by_question( DnsQuestion *question, bool with_rrsig) { /* Add RRSIG RR matching each RR */ - _cleanup_(dns_resource_key_unrefp) DnsResourceKey *redirected_key = NULL; - unsigned n_cname_redirects = 0; DnsAnswerItem *item; int r; assert(reply); - /* Copies all RRs from 'answer' into 'reply', if they match 'question'. There might be direct and - * indirect matches (i.e. via CNAME/DNAME). If they have an indirect one, remember where we need to - * go, and restart the loop */ + /* Copies all RRs from 'answer' into 'reply', if they match 'question'. */ - for (;;) { - _cleanup_(dns_resource_key_unrefp) DnsResourceKey *next_redirected_key = NULL; - - DNS_ANSWER_FOREACH_ITEM(item, answer) { - DnsResourceKey *k = NULL; - - if (redirected_key) { - /* There was a redirect in this packet, let's collect all matching RRs for the redirect */ - r = dns_resource_key_match_rr(redirected_key, item->rr, NULL); - if (r < 0) - return r; - - k = redirected_key; - } else if (question) { - /* We have a question, let's see if this RR matches it */ - r = dns_question_matches_rr(question, item->rr, NULL); - if (r < 0) - return r; - - k = question->keys[0]; - } else - r = 1; /* No question, everything matches */ - - if (r == 0) { - _cleanup_free_ char *target = NULL; - - /* OK, so the RR doesn't directly match. Let's see if the RR is a matching - * CNAME or DNAME */ - - assert(k); - - r = dns_resource_record_get_cname_target(k, item->rr, &target); - if (r == -EUNATCH) - continue; /* Not a CNAME/DNAME or doesn't match */ - if (r < 0) - return r; - - /* Oh, wow, this is a redirect. Let's remember where this points, and store - * it in 'next_redirected_key'. Once we finished iterating through the rest - * of the RR's we'll start again, with the redirected RR key. */ - - n_cname_redirects++; - if (n_cname_redirects > CNAME_REDIRECT_MAX) /* don't loop forever */ - return -ELOOP; - - dns_resource_key_unref(next_redirected_key); - - /* There can only be one CNAME per name, hence no point in storing more than one here */ - next_redirected_key = dns_resource_key_new(k->class, k->type, target); - if (!next_redirected_key) - return -ENOMEM; - } - - /* Mask the section info, we want the primary answers to always go without section info, so - * that it is added to the answer section when we synthesize a reply. */ + DNS_ANSWER_FOREACH_ITEM(item, answer) { - r = reply_add_with_rrsig( - reply, - item->rr, - item->ifindex, - item->flags & ~DNS_ANSWER_MASK_SECTIONS, - item->rrsig, - with_rrsig); + /* We have a question, let's see if this RR matches it */ + r = dns_question_matches_rr(question, item->rr, NULL); + if (r < 0) + return r; + if (!r) { + /* Maybe there's a CNAME/DNAME in here? If so, that's an answer too */ + r = dns_question_matches_cname_or_dname(question, item->rr, NULL); if (r < 0) return r; + if (!r) + continue; } - if (!next_redirected_key) - break; + /* Mask the section info, we want the primary answers to always go without section + * info, so that it is added to the answer section when we synthesize a reply. */ - dns_resource_key_unref(redirected_key); - redirected_key = TAKE_PTR(next_redirected_key); + r = reply_add_with_rrsig( + reply, + item->rr, + item->ifindex, + item->flags & ~DNS_ANSWER_MASK_SECTIONS, + item->rrsig, + with_rrsig); + if (r < 0) + return r; } return 0; @@ -323,27 +274,27 @@ static int dns_stub_assign_sections( if (r < 0) return r; - /* Include all RRs that originate from the answer or authority sections, and aren't listed in the + /* Include all RRs that originate from the authority sections, and aren't already listed in the * answer section, in the authority section */ r = dns_stub_collect_answer_by_section( &q->reply_authoritative, q->answer, - DNS_ANSWER_SECTION_ANSWER, + DNS_ANSWER_SECTION_AUTHORITY, q->reply_answer, NULL, edns0_do); if (r < 0) return r; + + /* Include all RRs that originate from the answer or additional sections in the additional section + * (except if already listed in the other two sections). Also add all RRs with no section marking. */ r = dns_stub_collect_answer_by_section( - &q->reply_authoritative, + &q->reply_additional, q->answer, - DNS_ANSWER_SECTION_AUTHORITY, - q->reply_answer, NULL, + DNS_ANSWER_SECTION_ANSWER, + q->reply_answer, q->reply_authoritative, edns0_do); if (r < 0) return r; - - /* Include all RRs that originate from the additional sections in the additional section (except if - * already listed in the other two sections). Also add all RRs with no section marking. */ r = dns_stub_collect_answer_by_section( &q->reply_additional, q->answer, @@ -771,21 +722,63 @@ static void dns_stub_query_complete(DnsQuery *q) { switch (q->state) { - case DNS_TRANSACTION_SUCCESS: - r = dns_query_process_cname(q); - if (r == -ELOOP) { /* CNAME loop, let's send what we already have */ - log_debug_errno(r, "Detected CNAME loop, returning what we already have."); - (void) dns_stub_send_reply(q, q->answer_rcode); - break; - } - if (r < 0) { - log_debug_errno(r, "Failed to process CNAME: %m"); - break; + case DNS_TRANSACTION_SUCCESS: { + bool first = true; + + for (;;) { + int cname_result; + + cname_result = dns_query_process_cname_one(q); + if (cname_result == -ELOOP) { /* CNAME loop, let's send what we already have */ + log_debug_errno(r, "Detected CNAME loop, returning what we already have."); + (void) dns_stub_send_reply(q, q->answer_rcode); + break; + } + if (cname_result < 0) { + log_debug_errno(cname_result, "Failed to process CNAME: %m"); + break; + } + + if (cname_result == DNS_QUERY_NOMATCH) { + /* This answer doesn't contain any RR that would answer our question + * positively, i.e. neither directly nor via CNAME. */ + + if (first) /* We never followed a CNAME and the answer doesn't match our + * question at all? Then this is final, the empty answer is the + * answer. */ + break; + + /* Otherwise, we already followed a CNAME once within this packet, and the + * packet doesn't answer our question. In that case let's restart the query, + * now with the redirected question. We'll */ + r = dns_query_go(q); + if (r < 0) + log_debug_errno(r, "Failed to restart query: %m"); + + return; + } + + r = dns_stub_assign_sections( + q, + dns_query_question_for_protocol(q, DNS_PROTOCOL_DNS), + dns_stub_reply_with_edns0_do(q)); + if (r < 0) { + log_debug_errno(r, "Failed to assign sections: %m"); + dns_query_free(q); + return; + } + + if (cname_result == DNS_QUERY_MATCH) /* A match? Then we are done, let's return what we got */ + break; + + /* We followed a CNAME. and collected the RRs that answer the redirected question + * successfully. Let's not try to do this again. */ + assert(cname_result == DNS_QUERY_CNAME); + first = false; } - if (r == DNS_QUERY_RESTARTED) - return; _fallthrough_; + } case DNS_TRANSACTION_RCODE_FAILURE: (void) dns_stub_send_reply(q, q->answer_rcode); diff --git a/src/resolve/resolved-varlink.c b/src/resolve/resolved-varlink.c index 77e75b8a8d..27d8c8967e 100644 --- a/src/resolve/resolved-varlink.c +++ b/src/resolve/resolved-varlink.c @@ -158,14 +158,14 @@ static void vl_method_resolve_hostname_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = varlink_error(q->varlink_request, "io.systemd.Resolve.CNAMELoop", NULL); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; question = dns_query_question_for_protocol(q, q->answer_protocol); @@ -395,14 +395,14 @@ static void vl_method_resolve_address_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = varlink_error(q->varlink_request, "io.systemd.Resolve.CNAMELoop", NULL); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; question = dns_query_question_for_protocol(q, q->answer_protocol); |