Re: [PATCH 3/3] udp: Check for answers to forwarded DNS queries before handling local redirects

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5533 bytes --]

On Tue, Nov 08, 2022 at 07:22:22AM +0100, Stefano Brivio wrote:
> On Tue, 8 Nov 2022 16:51:35 +1100
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <sbrivio@redhat.com>
> > > > > > > > > ---
> > > > > > > > >  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.

Yeah, I guess.  Seems pretty contrived.

> 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.

Hmm... yes, those are fair points.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]