[PATCH 0/2] Broaden DNS forwarding

[PATCH 0/2] Broaden DNS forwarding

[David Gibson]

When looking through outstanding bugs in various trackers, I noticed
this one:
	https://github.com/containers/podman/issues/23239

Now that the flow table is merged, this is very easy to fix, so,
here's a fix.  While we're at it, handle encrypted DNS on port 853 as
well, which Red Hat certainly seems to be interested in for one.

David Gibson (2):
  fwd: Refactor tests in fwd_nat_from_tap() for clarity
  fwd: Broaden what we consider for DNS specific forwarding rules

 fwd.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

-- 
2.45.2

[PATCH 1/2] fwd: Refactor tests in fwd_nat_from_tap() for clarity

[David Gibson]

Currently, we start by handling the common case, where we don't translate
the destination address, then we modify the tgt side for the special cases.
In the process we do comparisons on the tentatively set fields in tgt,
which obscures the fact that tgt should be an essentially pure function of
ini, and risks people examining fields of tgt that are not yet initialized.

To make this clearer, do all our tests on 'ini', constructing tgt from
scratch on that basis.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 fwd.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/fwd.c b/fwd.c
index 8c1f3d91..bd16ac42 100644
--- a/fwd.c
+++ b/fwd.c
@@ -169,22 +169,22 @@ void fwd_scan_ports_init(struct ctx *c)
 uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
 			 const struct flowside *ini, struct flowside *tgt)
 {
-	tgt->eaddr = ini->faddr;
-	tgt->eport = ini->fport;
-
-	if (proto == IPPROTO_UDP && tgt->eport == 53 &&
-	    inany_equals4(&tgt->eaddr, &c->ip4.dns_match)) {
+	if (proto == IPPROTO_UDP && ini->fport == 53 &&
+	    inany_equals4(&ini->faddr, &c->ip4.dns_match)) {
 		tgt->eaddr = inany_from_v4(c->ip4.dns_host);
-	} else if (proto == IPPROTO_UDP && tgt->eport == 53 &&
-		   inany_equals6(&tgt->eaddr, &c->ip6.dns_match)) {
+	} else if (proto == IPPROTO_UDP && ini->fport == 53 &&
+		   inany_equals6(&ini->faddr, &c->ip6.dns_match)) {
 		tgt->eaddr.a6 = c->ip6.dns_host;
-	} else if (!c->no_map_gw) {
-		if (inany_equals4(&tgt->eaddr, &c->ip4.gw))
-			tgt->eaddr = inany_loopback4;
-		else if (inany_equals6(&tgt->eaddr, &c->ip6.gw))
+	} else if (!c->no_map_gw && inany_equals4(&ini->faddr, &c->ip4.gw)) {
+		tgt->eaddr = inany_loopback4;
+	} else if (!c->no_map_gw && inany_equals6(&ini->faddr, &c->ip6.gw)) {
 			tgt->eaddr = inany_loopback6;
+	} else {
+		tgt->eaddr = ini->faddr;
 	}
 
+	tgt->eport = ini->fport;
+
 	/* The relevant addr_out controls the host side source address.  This
 	 * may be unspecified, which allows the kernel to pick an address.
 	 */
-- 
@@ -169,22 +169,22 @@ void fwd_scan_ports_init(struct ctx *c)
 uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
 			 const struct flowside *ini, struct flowside *tgt)
 {
-	tgt->eaddr = ini->faddr;
-	tgt->eport = ini->fport;
-
-	if (proto == IPPROTO_UDP && tgt->eport == 53 &&
-	    inany_equals4(&tgt->eaddr, &c->ip4.dns_match)) {
+	if (proto == IPPROTO_UDP && ini->fport == 53 &&
+	    inany_equals4(&ini->faddr, &c->ip4.dns_match)) {
 		tgt->eaddr = inany_from_v4(c->ip4.dns_host);
-	} else if (proto == IPPROTO_UDP && tgt->eport == 53 &&
-		   inany_equals6(&tgt->eaddr, &c->ip6.dns_match)) {
+	} else if (proto == IPPROTO_UDP && ini->fport == 53 &&
+		   inany_equals6(&ini->faddr, &c->ip6.dns_match)) {
 		tgt->eaddr.a6 = c->ip6.dns_host;
-	} else if (!c->no_map_gw) {
-		if (inany_equals4(&tgt->eaddr, &c->ip4.gw))
-			tgt->eaddr = inany_loopback4;
-		else if (inany_equals6(&tgt->eaddr, &c->ip6.gw))
+	} else if (!c->no_map_gw && inany_equals4(&ini->faddr, &c->ip4.gw)) {
+		tgt->eaddr = inany_loopback4;
+	} else if (!c->no_map_gw && inany_equals6(&ini->faddr, &c->ip6.gw)) {
 			tgt->eaddr = inany_loopback6;
+	} else {
+		tgt->eaddr = ini->faddr;
 	}
 
+	tgt->eport = ini->fport;
+
 	/* The relevant addr_out controls the host side source address.  This
 	 * may be unspecified, which allows the kernel to pick an address.
 	 */
-- 
2.45.2

Re: [PATCH 1/2] fwd: Refactor tests in fwd_nat_from_tap() for clarity

[Stefano Brivio]

On Wed, 24 Jul 2024 16:20:57 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently, we start by handling the common case, where we don't translate
> the destination address, then we modify the tgt side for the special cases.
> In the process we do comparisons on the tentatively set fields in tgt,
> which obscures the fact that tgt should be an essentially pure function of
> ini, and risks people examining fields of tgt that are not yet initialized.
> 
> To make this clearer, do all our tests on 'ini', constructing tgt from
> scratch on that basis.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  fwd.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/fwd.c b/fwd.c
> index 8c1f3d91..bd16ac42 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -169,22 +169,22 @@ void fwd_scan_ports_init(struct ctx *c)
>  uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
>  			 const struct flowside *ini, struct flowside *tgt)
>  {
> -	tgt->eaddr = ini->faddr;
> -	tgt->eport = ini->fport;
> -
> -	if (proto == IPPROTO_UDP && tgt->eport == 53 &&
> -	    inany_equals4(&tgt->eaddr, &c->ip4.dns_match)) {

These blocks are all one-liners now and curly brackets could go away, I
guess?

> +	if (proto == IPPROTO_UDP && ini->fport == 53 &&
> +	    inany_equals4(&ini->faddr, &c->ip4.dns_match)) {
>  		tgt->eaddr = inany_from_v4(c->ip4.dns_host);
> -	} else if (proto == IPPROTO_UDP && tgt->eport == 53 &&
> -		   inany_equals6(&tgt->eaddr, &c->ip6.dns_match)) {
> +	} else if (proto == IPPROTO_UDP && ini->fport == 53 &&
> +		   inany_equals6(&ini->faddr, &c->ip6.dns_match)) {
>  		tgt->eaddr.a6 = c->ip6.dns_host;
> -	} else if (!c->no_map_gw) {
> -		if (inany_equals4(&tgt->eaddr, &c->ip4.gw))
> -			tgt->eaddr = inany_loopback4;
> -		else if (inany_equals6(&tgt->eaddr, &c->ip6.gw))
> +	} else if (!c->no_map_gw && inany_equals4(&ini->faddr, &c->ip4.gw)) {
> +		tgt->eaddr = inany_loopback4;
> +	} else if (!c->no_map_gw && inany_equals6(&ini->faddr, &c->ip6.gw)) {
>  			tgt->eaddr = inany_loopback6;

Resulting excess tab here.

> +	} else {
> +		tgt->eaddr = ini->faddr;
>  	}
>  
> +	tgt->eport = ini->fport;
> +
>  	/* The relevant addr_out controls the host side source address.  This
>  	 * may be unspecified, which allows the kernel to pick an address.
>  	 */

-- 
Stefano

Re: [PATCH 1/2] fwd: Refactor tests in fwd_nat_from_tap() for clarity

[David Gibson]

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

On Wed, Jul 24, 2024 at 08:38:59AM +0200, Stefano Brivio wrote:
> On Wed, 24 Jul 2024 16:20:57 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Currently, we start by handling the common case, where we don't translate
> > the destination address, then we modify the tgt side for the special cases.
> > In the process we do comparisons on the tentatively set fields in tgt,
> > which obscures the fact that tgt should be an essentially pure function of
> > ini, and risks people examining fields of tgt that are not yet initialized.
> > 
> > To make this clearer, do all our tests on 'ini', constructing tgt from
> > scratch on that basis.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  fwd.c | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fwd.c b/fwd.c
> > index 8c1f3d91..bd16ac42 100644
> > --- a/fwd.c
> > +++ b/fwd.c
> > @@ -169,22 +169,22 @@ void fwd_scan_ports_init(struct ctx *c)
> >  uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
> >  			 const struct flowside *ini, struct flowside *tgt)
> >  {
> > -	tgt->eaddr = ini->faddr;
> > -	tgt->eport = ini->fport;
> > -
> > -	if (proto == IPPROTO_UDP && tgt->eport == 53 &&
> > -	    inany_equals4(&tgt->eaddr, &c->ip4.dns_match)) {
> 
> These blocks are all one-liners now and curly brackets could go away, I
> guess?

Good point, done.

> > +	if (proto == IPPROTO_UDP && ini->fport == 53 &&
> > +	    inany_equals4(&ini->faddr, &c->ip4.dns_match)) {
> >  		tgt->eaddr = inany_from_v4(c->ip4.dns_host);
> > -	} else if (proto == IPPROTO_UDP && tgt->eport == 53 &&
> > -		   inany_equals6(&tgt->eaddr, &c->ip6.dns_match)) {
> > +	} else if (proto == IPPROTO_UDP && ini->fport == 53 &&
> > +		   inany_equals6(&ini->faddr, &c->ip6.dns_match)) {
> >  		tgt->eaddr.a6 = c->ip6.dns_host;
> > -	} else if (!c->no_map_gw) {
> > -		if (inany_equals4(&tgt->eaddr, &c->ip4.gw))
> > -			tgt->eaddr = inany_loopback4;
> > -		else if (inany_equals6(&tgt->eaddr, &c->ip6.gw))
> > +	} else if (!c->no_map_gw && inany_equals4(&ini->faddr, &c->ip4.gw)) {
> > +		tgt->eaddr = inany_loopback4;
> > +	} else if (!c->no_map_gw && inany_equals6(&ini->faddr, &c->ip6.gw)) {
> >  			tgt->eaddr = inany_loopback6;
> 
> Resulting excess tab here.

Fixed.

> > +	} else {
> > +		tgt->eaddr = ini->faddr;
> >  	}
> >  
> > +	tgt->eport = ini->fport;
> > +
> >  	/* The relevant addr_out controls the host side source address.  This
> >  	 * may be unspecified, which allows the kernel to pick an address.
> >  	 */
> 

-- 
David Gibson (he or they)	| 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 --]

[PATCH 2/2] fwd: Broaden what we consider for DNS specific forwarding rules

[David Gibson]

passt/pasta has options to redirect DNS requests from the guest to a
different server address on the host side.  Currently, however, only UDP
packets to port 53 are considered "DNS requests".  This ignores DNS
requests over TCP - less common, but certainly possible.  It also ignores
encrypted DNS requests on port 853.

Extend the DNS forwarding logic to handle both of those cases.

Link: https://github.com/containers/podman/issues/23239

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 fwd.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/fwd.c b/fwd.c
index bd16ac42..3f864f85 100644
--- a/fwd.c
+++ b/fwd.c
@@ -156,6 +156,20 @@ void fwd_scan_ports_init(struct ctx *c)
 	}
 }
 
+/**
+ * is_dns_flow() - Determine if flow appears to be a DNS request
+ * @proto:	Protocol (IP L4 protocol number)
+ * @ini:	Flow address information of the initiating side
+ *
+ * Return: true if the flow appears to be directed at a dns server, that is a
+ *         TCP or UDP flow to port 53 (domain) or port 853 (domain-s)
+ */
+static bool is_dns_flow(uint8_t proto, const struct flowside *ini)
+{
+	return ((proto == IPPROTO_UDP) || (proto == IPPROTO_TCP)) &&
+		((ini->fport == 53) || (ini->fport == 853));
+}
+
 /**
  * fwd_nat_from_tap() - Determine to forward a flow from the tap interface
  * @c:		Execution context
@@ -169,10 +183,10 @@ void fwd_scan_ports_init(struct ctx *c)
 uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
 			 const struct flowside *ini, struct flowside *tgt)
 {
-	if (proto == IPPROTO_UDP && ini->fport == 53 &&
+	if (is_dns_flow(proto, ini) &&
 	    inany_equals4(&ini->faddr, &c->ip4.dns_match)) {
 		tgt->eaddr = inany_from_v4(c->ip4.dns_host);
-	} else if (proto == IPPROTO_UDP && ini->fport == 53 &&
+	} else if (is_dns_flow(proto, ini) &&
 		   inany_equals6(&ini->faddr, &c->ip6.dns_match)) {
 		tgt->eaddr.a6 = c->ip6.dns_host;
 	} else if (!c->no_map_gw && inany_equals4(&ini->faddr, &c->ip4.gw)) {
-- 
@@ -156,6 +156,20 @@ void fwd_scan_ports_init(struct ctx *c)
 	}
 }
 
+/**
+ * is_dns_flow() - Determine if flow appears to be a DNS request
+ * @proto:	Protocol (IP L4 protocol number)
+ * @ini:	Flow address information of the initiating side
+ *
+ * Return: true if the flow appears to be directed at a dns server, that is a
+ *         TCP or UDP flow to port 53 (domain) or port 853 (domain-s)
+ */
+static bool is_dns_flow(uint8_t proto, const struct flowside *ini)
+{
+	return ((proto == IPPROTO_UDP) || (proto == IPPROTO_TCP)) &&
+		((ini->fport == 53) || (ini->fport == 853));
+}
+
 /**
  * fwd_nat_from_tap() - Determine to forward a flow from the tap interface
  * @c:		Execution context
@@ -169,10 +183,10 @@ void fwd_scan_ports_init(struct ctx *c)
 uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
 			 const struct flowside *ini, struct flowside *tgt)
 {
-	if (proto == IPPROTO_UDP && ini->fport == 53 &&
+	if (is_dns_flow(proto, ini) &&
 	    inany_equals4(&ini->faddr, &c->ip4.dns_match)) {
 		tgt->eaddr = inany_from_v4(c->ip4.dns_host);
-	} else if (proto == IPPROTO_UDP && ini->fport == 53 &&
+	} else if (is_dns_flow(proto, ini) &&
 		   inany_equals6(&ini->faddr, &c->ip6.dns_match)) {
 		tgt->eaddr.a6 = c->ip6.dns_host;
 	} else if (!c->no_map_gw && inany_equals4(&ini->faddr, &c->ip4.gw)) {
-- 
2.45.2

Re: [PATCH 0/2] Broaden DNS forwarding

[Stefano Brivio]

On Wed, 24 Jul 2024 16:20:56 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> When looking through outstanding bugs in various trackers, I noticed
> this one:
> 	https://github.com/containers/podman/issues/23239

...right, that was also pretty much the first candidate in my list of
flow table related low-hanging fruits.

The series looks good to me, except for a nit on 1/2 and one missing
bit: we should update the man page as well (for --dns-forward).

-- 
Stefano