[PATCH v2 0/2] Broaden DNS forwarding

[PATCH v2 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.

Changes since v1:
 * Fix some stylistic errors in 1/2
 * Update man page to reflect new behaviour in 2/2

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   | 39 ++++++++++++++++++++++++++-------------
 passt.1 | 10 +++++-----
 2 files changed, 31 insertions(+), 18 deletions(-)

-- 
2.45.2

[PATCH v2 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 | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/fwd.c b/fwd.c
index 8c1f3d91..c323aba7 100644
--- a/fwd.c
+++ b/fwd.c
@@ -169,21 +169,20 @@ 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))
-			tgt->eaddr = inany_loopback6;
-	}
+	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,21 +169,20 @@ 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))
-			tgt->eaddr = inany_loopback6;
-	}
+	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

[PATCH v2 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 ++++++++++++++++--
 passt.1 | 10 +++++-----
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/fwd.c b/fwd.c
index c323aba7..dea36f6c 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))
diff --git a/passt.1 b/passt.1
index abc13d51..81789cc4 100644
--- a/passt.1
+++ b/passt.1
@@ -244,11 +244,11 @@ usage of DNS addresses altogether.
 
 .TP
 .BR \-\-dns-forward " " \fIaddr
-Map \fIaddr\fR (IPv4 or IPv6) as seen from guest or namespace to the first
-configured DNS resolver (with corresponding IP version). Mapping is limited to
-UDP traffic directed to port 53, and DNS answers are translated back with a
-reverse mapping.
-This option can be specified zero to two times (once for IPv4, once for IPv6).
+Map \fIaddr\fR (IPv4 or IPv6) as seen from guest or namespace to the
+first configured DNS resolver (with corresponding IP version). Maps
+only UDP and TCP traffic to port 53 or port 853.  Replies are
+translated back with a reverse mapping.  This option can be specified
+zero to two times (once for IPv4, once for IPv6).
 
 .TP
 .BR \-S ", " \-\-search " " \fIlist
-- 
@@ -244,11 +244,11 @@ usage of DNS addresses altogether.
 
 .TP
 .BR \-\-dns-forward " " \fIaddr
-Map \fIaddr\fR (IPv4 or IPv6) as seen from guest or namespace to the first
-configured DNS resolver (with corresponding IP version). Mapping is limited to
-UDP traffic directed to port 53, and DNS answers are translated back with a
-reverse mapping.
-This option can be specified zero to two times (once for IPv4, once for IPv6).
+Map \fIaddr\fR (IPv4 or IPv6) as seen from guest or namespace to the
+first configured DNS resolver (with corresponding IP version). Maps
+only UDP and TCP traffic to port 53 or port 853.  Replies are
+translated back with a reverse mapping.  This option can be specified
+zero to two times (once for IPv4, once for IPv6).
 
 .TP
 .BR \-S ", " \-\-search " " \fIlist
-- 
2.45.2

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

[Paul Holzinger]

Hi,

On 24/07/2024 09:51, David Gibson wrote:
> 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.
The question here is if it handles DoT should it handle DoH as well, 
i.e. https (443)?
>
> Link: https://github.com/containers/podman/issues/23239
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Tested-by: Paul Holzinger <pholzing@redhat.com>

I tested both dns over tcp and dns over tls with dig.

-- 
Paul

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

[Stefano Brivio]

On Wed, 24 Jul 2024 11:41:44 +0200
Paul Holzinger <pholzing@redhat.com> wrote:

> Hi,
> 
> On 24/07/2024 09:51, David Gibson wrote:
> > 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.  
>
> The question here is if it handles DoT should it handle DoH as well, 
> i.e. https (443)?

We don't have a flexible interface, yet, to finely configure outbound
traffic redirections, so the user couldn't enable or disable this at
will. So I'm wondering if there's any use case that we risk breaking
with that.

The most confusing case I can think of is a host with a local resolver
with a loopback address (for example, the usual 127.0.0.53 from
systemd-resolved). Without --no-map-gw (or with Podman's --map-gw), we
will, by default, use the address of the default gateway (which maps to
the host) as implied --dns-forward option.

If we now match on HTTPS as well, HTTPS traffic that's supposed to
reach the host (because there's an HTTPS server there) will anyway reach
the host, even if we mishandle it as DNS traffic somehow.

So I don't actually see an issue with that, but given that users can't
disable just HTTPS (this should be easier to implement with the flow
table, but it will surely be a while before we get to that), we should
think quite hard if there's any possibility of breakage before going
ahead with it.

> > Link: https://github.com/containers/podman/issues/23239
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>  
> 
> Tested-by: Paul Holzinger <pholzing@redhat.com>
> 
> I tested both dns over tcp and dns over tls with dig.

Thanks!

-- 
Stefano

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

[David Gibson]

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

On Wed, Jul 24, 2024 at 04:30:50PM +0200, Stefano Brivio wrote:
> On Wed, 24 Jul 2024 11:41:44 +0200
> Paul Holzinger <pholzing@redhat.com> wrote:
> 
> > Hi,
> > 
> > On 24/07/2024 09:51, David Gibson wrote:
> > > 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.  
> >
> > The question here is if it handles DoT should it handle DoH as well, 
> > i.e. https (443)?

My first inclination was, no, because for traffic to port 443 we can't
be confident it's actually DNS.  But, then again, maybe going to an
address marked as a DNS server address is good enough?  I'm not sure.

> We don't have a flexible interface, yet, to finely configure outbound
> traffic redirections, so the user couldn't enable or disable this at
> will. So I'm wondering if there's any use case that we risk breaking
> with that.
> 
> The most confusing case I can think of is a host with a local resolver
> with a loopback address (for example, the usual 127.0.0.53 from
> systemd-resolved). Without --no-map-gw (or with Podman's --map-gw), we
> will, by default, use the address of the default gateway (which maps to
> the host) as implied --dns-forward option.
> 
> If we now match on HTTPS as well, HTTPS traffic that's supposed to
> reach the host (because there's an HTTPS server there) will anyway reach
> the host, even if we mishandle it as DNS traffic somehow.
> 
> So I don't actually see an issue with that, but given that users can't
> disable just HTTPS (this should be easier to implement with the flow
> table, but it will surely be a while before we get to that), we should
> think quite hard if there's any possibility of breakage before going
> ahead with it.

Yeah, that argument inclines me back towards "no" for DoH, at least
for the time being.

> > > Link: https://github.com/containers/podman/issues/23239
> > >
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>  
> > 
> > Tested-by: Paul Holzinger <pholzing@redhat.com>
> > 
> > I tested both dns over tcp and dns over tls with dig.
> 
> Thanks!
> 

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

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

[Paul Holzinger]

On 25/07/2024 06:44, David Gibson wrote:
> On Wed, Jul 24, 2024 at 04:30:50PM +0200, Stefano Brivio wrote:
>> On Wed, 24 Jul 2024 11:41:44 +0200
>> Paul Holzinger <pholzing@redhat.com> wrote:
>>
>>> Hi,
>>>
>>> On 24/07/2024 09:51, David Gibson wrote:
>>>> 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.
>>> The question here is if it handles DoT should it handle DoH as well,
>>> i.e. https (443)?
> My first inclination was, no, because for traffic to port 443 we can't
> be confident it's actually DNS.  But, then again, maybe going to an
> address marked as a DNS server address is good enough?  I'm not sure.
>
>> We don't have a flexible interface, yet, to finely configure outbound
>> traffic redirections, so the user couldn't enable or disable this at
>> will. So I'm wondering if there's any use case that we risk breaking
>> with that.
>>
>> The most confusing case I can think of is a host with a local resolver
>> with a loopback address (for example, the usual 127.0.0.53 from
>> systemd-resolved). Without --no-map-gw (or with Podman's --map-gw), we
>> will, by default, use the address of the default gateway (which maps to
>> the host) as implied --dns-forward option.
>>
>> If we now match on HTTPS as well, HTTPS traffic that's supposed to
>> reach the host (because there's an HTTPS server there) will anyway reach
>> the host, even if we mishandle it as DNS traffic somehow.
>>
>> So I don't actually see an issue with that, but given that users can't
>> disable just HTTPS (this should be easier to implement with the flow
>> table, but it will surely be a while before we get to that), we should
>> think quite hard if there's any possibility of breakage before going
>> ahead with it.
> Yeah, that argument inclines me back towards "no" for DoH, at least
> for the time being.
Ok, I agree.
>
>>>> Link: https://github.com/containers/podman/issues/23239
>>>>
>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> Tested-by: Paul Holzinger <pholzing@redhat.com>
>>>
>>> I tested both dns over tcp and dns over tls with dig.
>> Thanks!
>>
-- 
Paul Holzinger

Re: [PATCH v2 0/2] Broaden DNS forwarding

[Stefano Brivio]

On Wed, 24 Jul 2024 17:51:10 +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
> 
> 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.
> 
> Changes since v1:
>  * Fix some stylistic errors in 1/2
>  * Update man page to reflect new behaviour in 2/2
> 
> David Gibson (2):
>   fwd: Refactor tests in fwd_nat_from_tap() for clarity
>   fwd: Broaden what we consider for DNS specific forwarding rules

Applied.

-- 
Stefano