From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 6402C5A0082 for ; Tue, 8 Nov 2022 07:22:44 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1667888563; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=RRSEN4+9PwTBFjN0Agvrqzjm5+e+J5yVdvivK9lGRAI=; b=Uhdr7vc9q51MbXaNJLHWAGY10ZHfYcW8T8Hu8Jcy6KqkOZ68lFAsC3iTVz3WJ0FP8YO4fu 3YDq2l/2vanYpAFMZGXrSKct4PjmOghfGh5ZGFoooloJUJswU+Lz4edbpxweckSA6P5L7D +lJ2NtLasFe9yjsWzmspJ7YCrAi1vX8= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-556-A3hY-uGrM2WKQwdwhI-fJw-1; Tue, 08 Nov 2022 01:22:42 -0500 X-MC-Unique: A3hY-uGrM2WKQwdwhI-fJw-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 9DAF8862FDC; Tue, 8 Nov 2022 06:22:41 +0000 (UTC) Received: from maya.cloud.tilaa.com (ovpn-208-9.brq.redhat.com [10.40.208.9]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2F3DD2166B29; Tue, 8 Nov 2022 06:22:41 +0000 (UTC) Date: Tue, 8 Nov 2022 07:22:22 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 3/3] udp: Check for answers to forwarded DNS queries before handling local redirects Message-ID: <20221108072222.73615076@elisabeth> In-Reply-To: References: <20221102230443.377446-1-sbrivio@redhat.com> <20221102230443.377446-4-sbrivio@redhat.com> <20221103074251.60b2898b@elisabeth> <20221105082223.4a447ac5@elisabeth> <20221107105121.37735a0b@elisabeth> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: NXAD4KXUTI7EGDUBG5SN5KJMESWAPI7L X-Message-ID-Hash: NXAD4KXUTI7EGDUBG5SN5KJMESWAPI7L X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Paul Holzinger X-Mailman-Version: 3.3.3 Precedence: list List-Id: Development discussion and patches for passt Archived-At: <> Archived-At: List-Archive: <> List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Tue, 8 Nov 2022 16:51:35 +1100 David Gibson wrote: > On Mon, Nov 07, 2022 at 10:51:21AM +0100, Stefano Brivio wrote: > > On Mon, 7 Nov 2022 12:08:59 +1100 > > David Gibson wrote: > > > > > On Sat, Nov 05, 2022 at 08:22:23AM +0100, Stefano Brivio wrote: > > > > On Sat, 5 Nov 2022 12:19:55 +1100 > > > > David Gibson wrote: > > > > > > > > > On Thu, Nov 03, 2022 at 07:42:51AM +0100, Stefano Brivio wrote: > > > > > > On Thu, 3 Nov 2022 14:42:13 +1100 > > > > > > David Gibson wrote: > > > > > > > > > > > > > On Thu, Nov 03, 2022 at 12:04:43AM +0100, Stefano Brivio wrote: > > > > > > > > Now that we allow loopback DNS addresses to be used as targets for > > > > > > > > forwarding, we need to check if DNS answers come from those targets, > > > > > > > > before deciding to eventually remap traffic for local redirects. > > > > > > > > > > > > > > > > Otherwise, the source address won't match the one configured as > > > > > > > > forwarder, which means that the guest or the container will refuse > > > > > > > > those responses. > > > > > > > > > > > > > > > > Signed-off-by: Stefano Brivio > > > > > > > > --- > > > > > > > > udp.c | 17 ++++++++--------- > > > > > > > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > > > > > > > > > > > > > diff --git a/udp.c b/udp.c > > > > > > > > index 4b201d3..7c77e09 100644 > > > > > > > > --- a/udp.c > > > > > > > > +++ b/udp.c > > > > > > > > @@ -680,8 +680,10 @@ static void udp_sock_fill_data_v4(const struct ctx *c, int n, > > > > > > > > src = ntohl(b->s_in.sin_addr.s_addr); > > > > > > > > src_port = ntohs(b->s_in.sin_port); > > > > > > > > > > > > > > > > - if (src >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET || > > > > > > > > - src == INADDR_ANY || src == ntohl(c->ip4.addr_seen)) { > > > > > > > > + if (c->ip4.dns_fwd && src == htonl(c->ip4.dns[0]) && src_port == 53) { > > > > > > > > > > > > > > I guess this is not a newly introduced bug, but for the case of > > > > > > > multiple host nameservers, don't you need to check against everything > > > > > > > in the ip4.dns[] array, not just entry 0? > > > > > > > > > > > > No, because that's the only one we're using as target for forwarded > > > > > > queries -- and DNS answers we want to check here are only the forwarded > > > > > > ones. > > > > > > > > > > *thinks* .. ok, that makes sense. But if that's the case, won't > > > > > ip4.dns[0] be the only entry in ip4.dns[] we use for anything at all? > > > > > Can we drop the table and just keep one entry? > > > > > > > > Now that we have ip{4,6}.dns_send[], yes. > > > > > > Right, that's what I meant. > > > > > > > We could rename .dns_send[] back to .dns[] and change the current > > > > > > Right, I think dns[] is a better name for it. > > > > > > > .dns[] to .own_dns, or .fwd_dns_target, something like that. Other naming > > > > ideas welcome. > > > > > > Yeah, I find the current dns_fwd name not very illuminating either. > > > > > > *thinks* does it even make sense for dns_fwd not to be in dns_send? > > > We're intercepting queries the guest sends to @dns_fwd, so surely we > > > should also be advertising it to the guest. > > > > I wouldn't be so sure of that "surely". In the Podman test case where I > > hit this issue, I use Podman to write to /etc/resolv.conf directly, no > > DHCP/NDP/DHCPv6 involved, and things work. > > > > That doesn't automatically imply a use case for *not* advertising it, > > but we have several ways this can work without advertising anything, so > > there are also chances somebody might not want to advertise that in some > > obscure use case. > > Right, but only the case for not advertising it matters here, and I > don't see one. @dns_fwd (or @dns_match, as we discussed calling it > instead) is explicitly a virtual DNS server available to the guest. > Whatever method the guest does use to configure itself, we should > allow it to discover this via DHCP (or DHCPv6 or NDP). Rather hypothetical: you don't want the guest/container to use a given address as resolver. You know that that address might be in its resolv.conf(5) because you don't have control over the image, wish to override it if possible, and at the same time keep a safety net. Slightly unrelated: we're talking about this in the perspective of getting rid of an explicit @dns_fwd/@dns_match. This would become a flag, indicating we should forward queries originally directed to 1. dns[0]... or 2. anything in dns[]? If it's just about 1. dns[0], we're forcing that address to be the first advertised resolver. If it's about 2. dns[], we're not giving anymore the possibility of forwarding queries originally directed to one a single address. -- Stefano