[9front] [Patch] ndb/dns: DNSKEY and OPT RR types

[9front] [Patch] ndb/dns: DNSKEY and OPT RR types

[Jacob Moody]

Hello,



I recently ran in to some issues with pointing an unbound server towards a 9front dns server as its upstream.
The parsing seemed to fail when ndb/dns received a DNSKEY RR from it's own upstream source on behalf of unbound.
This patch catches and stores the DNSKEY from the upstream server to prevent this.



While working on this I upped the max UDP size the server is willing to accept from clients,
as well as the ability to broadcast this new size via EDNS through the OPT RR type when prompted by the client.
The new size of 4096 is based on the suggestion listed in rfc6891.


Thanks,
moody

diff -r 33920ebb68d1 sys/src/cmd/ndb/convDNS2M.c
--- a/sys/src/cmd/ndb/convDNS2M.c	Thu Dec 17 21:02:11 2020 +0100
+++ b/sys/src/cmd/ndb/convDNS2M.c	Thu Dec 17 17:48:13 2020 -0600
@@ -268,6 +268,9 @@
 		for(t = rp->txt; t != nil; t = t->next)
 			STRING(t->p);
 		break;
+	case Topt:
+		BYTES(rp->opt->data, rp->opt->dlen);
+		break;
 	case Tnull:
 		BYTES(rp->null->data, rp->null->dlen);
 		break;
@@ -275,6 +278,7 @@
 		NAME(rp->rmb->name);
 		NAME(rp->rp->name);
 		break;
+	case Tdnskey:
 	case Tkey:
 		USHORT(rp->key->flags);
 		UCHAR(rp->key->proto);
diff -r 33920ebb68d1 sys/src/cmd/ndb/convM2DNS.c
--- a/sys/src/cmd/ndb/convM2DNS.c	Thu Dec 17 21:02:11 2020 +0100
+++ b/sys/src/cmd/ndb/convM2DNS.c	Thu Dec 17 17:48:13 2020 -0600
@@ -441,6 +441,9 @@
 			l = &t->next;
 		}
 		break;
+	case Topt:
+		BYTES(rp->opt->data, rp->opt->dlen);
+		break;
 	case Tnull:
 		BYTES(rp->null->data, rp->null->dlen);
 		break;
@@ -448,6 +451,7 @@
 		rp->rmb = dnlookup(NAME(dname), Cin, 1);
 		rp->rp  = dnlookup(NAME(dname), Cin, 1);
 		break;
+	case Tdnskey:
 	case Tkey:
 		USHORT(rp->key->flags);
 		UCHAR(rp->key->proto);
diff -r 33920ebb68d1 sys/src/cmd/ndb/dn.c
--- a/sys/src/cmd/ndb/dn.c	Thu Dec 17 21:02:11 2020 +0100
+++ b/sys/src/cmd/ndb/dn.c	Thu Dec 17 17:48:13 2020 -0600
@@ -1791,6 +1791,34 @@
 	return rp;
 }

+RR*
+mkopt(void)
+{
+	RR *rp;
+	DN *dp;
+
+	rp = rralloc(Topt);
+
+	dp = emalloc(sizeof(*dp));
+	dp->magic = DNmagic;
+	dp->name = estrdup("");
+	/* class holds our max UDP size */
+	dp->class = Maxudp;
+	dp->rr = nil;
+	dp->referenced = now;
+	dp->next = nil;
+
+	rp->owner = dp;
+	/*
+     * OPT TTL stores RSCODE, VERSION and DNSSEC Flag
+	 * This signals RSCODE = 0, VERSION = 0, and no DNSSEC
+     */
+	rp->ttl = 0;
+	rp->opt->dlen = 0;
+	rp->opt->data = nil;
+	return rp;
+}
+
 void	bytes2nibbles(uchar *nibbles, uchar *bytes, int nbytes);

 /*
@@ -1951,6 +1979,7 @@
 		rp->srv = emalloc(sizeof(*rp->srv));
 		setmalloctag(rp->srv, rp->pc);
 		break;
+	case Tdnskey:
 	case Tkey:
 		rp->key = emalloc(sizeof(*rp->key));
 		setmalloctag(rp->key, rp->pc);
@@ -1963,6 +1992,10 @@
 		rp->sig = emalloc(sizeof(*rp->sig));
 		setmalloctag(rp->sig, rp->pc);
 		break;
+	case Topt:
+		rp->opt = emalloc(sizeof(*rp->opt));
+		setmalloctag(rp->opt, rp->pc);
+		break;
 	case Tnull:
 		rp->null = emalloc(sizeof(*rp->null));
 		setmalloctag(rp->null, rp->pc);
@@ -1994,6 +2027,7 @@
 		memset(rp->srv, 0, sizeof *rp->srv);	/* cause trouble */
 		free(rp->srv);
 		break;
+	case Tdnskey:
 	case Tkey:
 		free(rp->key->data);
 		memset(rp->key, 0, sizeof *rp->key);	/* cause trouble */
@@ -2009,6 +2043,11 @@
 		memset(rp->sig, 0, sizeof *rp->sig);	/* cause trouble */
 		free(rp->sig);
 		break;
+	case Topt:
+		free(rp->opt->data);
+		memset(rp->opt, 0, sizeof *rp->opt);
+		free(rp->opt);
+		break;
 	case Tnull:
 		free(rp->null->data);
 		memset(rp->null, 0, sizeof *rp->null);	/* cause trouble */
diff -r 33920ebb68d1 sys/src/cmd/ndb/dns.h
--- a/sys/src/cmd/ndb/dns.h	Thu Dec 17 21:02:11 2020 +0100
+++ b/sys/src/cmd/ndb/dns.h	Thu Dec 17 17:48:13 2020 -0600
@@ -135,7 +135,7 @@
 	Reserved=	5*Min,

 	/* packet sizes */
-	Maxudp=		512,	/* maximum bytes per udp message sent */
+	Maxudp=		4096,	/* maximum bytes per udp message sent */
 	Maxudpin=	2048,	/* maximum bytes per udp message rcv'd */

 	/* length of domain name hash table */
@@ -171,6 +171,7 @@
 typedef struct Sig	Sig;
 typedef struct Srv	Srv;
 typedef struct Txt	Txt;
+typedef struct Opt	Opt;

 /*
  *  a structure to track a request and any slave process handling it
@@ -236,6 +237,10 @@
 {
 	Block;
 };
+struct Opt
+{
+	Block;
+};

 /*
  *  text strings
@@ -292,6 +297,7 @@
 		Sig	*sig;
 		Null	*null;
 		Txt	*txt;
+		Opt	*opt;
 	};
 };

@@ -485,6 +491,7 @@
 int	tsame(int, int);
 void	unique(RR*);
 void	warning(char*, ...);
+RR*	mkopt(void);

 /* dnarea.c */
 void	refresh_areas(Area*);
diff -r 33920ebb68d1 sys/src/cmd/ndb/dnudpserver.c
--- a/sys/src/cmd/ndb/dnudpserver.c	Thu Dec 17 21:02:11 2020 +0100
+++ b/sys/src/cmd/ndb/dnudpserver.c	Thu Dec 17 17:48:13 2020 -0600
@@ -9,6 +9,7 @@

 static int	udpannounce(char*);
 static void	reply(int, uchar*, DNSmsg*, Request*);
+static void addopt(DNSmsg*, DNSmsg*);

 typedef struct Inprogress Inprogress;
 struct Inprogress
@@ -258,6 +259,7 @@
 				dnnotify(&reqmsg, &repmsg, &req);
 				break;
 			}
+			addopt(&reqmsg, &repmsg);
 			/* send reply on fd to address in buf's udp hdr */
 			reply(fd, buf, &repmsg, &req);
 			freeanswers(&repmsg);
@@ -334,3 +336,18 @@
 	if(write(fd, buf, len) != len)
 		dnslog("error sending reply: %r");
 }
+
+static void
+addopt(DNSmsg *reqmsg, DNSmsg *repmsg)
+{
+	RR *qr, *rr;
+
+	for(qr = reqmsg->ar; qr != nil; qr = qr->next)
+		if(qr->type == Topt){
+			for(rr = repmsg->ar; rr->next != nil; rr = rr->next)
+				;
+			rr->next = mkopt();
+			repmsg->arcount++;
+			break;
+		}
+}

Re: [9front] [Patch] ndb/dns: DNSKEY and OPT RR types

[hiro]

i haven't read the rfc very closely but why would you have a separate
smaller maxudpin with a factor of 1/2 ?

why not just one line maxudp = 4096 ?


On 12/18/20, Jacob Moody <moody@mail.posixcafe.org> wrote:
> Hello,
>
>
>
> I recently ran in to some issues with pointing an unbound server towards a
> 9front dns server as its upstream.
> The parsing seemed to fail when ndb/dns received a DNSKEY RR from it's own
> upstream source on behalf of unbound.
> This patch catches and stores the DNSKEY from the upstream server to prevent
> this.
>
>
>
> While working on this I upped the max UDP size the server is willing to
> accept from clients,
> as well as the ability to broadcast this new size via EDNS through the OPT
> RR type when prompted by the client.
> The new size of 4096 is based on the suggestion listed in rfc6891.
>
>
> Thanks,
> moody
>
> diff -r 33920ebb68d1 sys/src/cmd/ndb/convDNS2M.c
> --- a/sys/src/cmd/ndb/convDNS2M.c	Thu Dec 17 21:02:11 2020 +0100
> +++ b/sys/src/cmd/ndb/convDNS2M.c	Thu Dec 17 17:48:13 2020 -0600
> @@ -268,6 +268,9 @@
>  		for(t = rp->txt; t != nil; t = t->next)
>  			STRING(t->p);
>  		break;
> +	case Topt:
> +		BYTES(rp->opt->data, rp->opt->dlen);
> +		break;
>  	case Tnull:
>  		BYTES(rp->null->data, rp->null->dlen);
>  		break;
> @@ -275,6 +278,7 @@
>  		NAME(rp->rmb->name);
>  		NAME(rp->rp->name);
>  		break;
> +	case Tdnskey:
>  	case Tkey:
>  		USHORT(rp->key->flags);
>  		UCHAR(rp->key->proto);
> diff -r 33920ebb68d1 sys/src/cmd/ndb/convM2DNS.c
> --- a/sys/src/cmd/ndb/convM2DNS.c	Thu Dec 17 21:02:11 2020 +0100
> +++ b/sys/src/cmd/ndb/convM2DNS.c	Thu Dec 17 17:48:13 2020 -0600
> @@ -441,6 +441,9 @@
>  			l = &t->next;
>  		}
>  		break;
> +	case Topt:
> +		BYTES(rp->opt->data, rp->opt->dlen);
> +		break;
>  	case Tnull:
>  		BYTES(rp->null->data, rp->null->dlen);
>  		break;
> @@ -448,6 +451,7 @@
>  		rp->rmb = dnlookup(NAME(dname), Cin, 1);
>  		rp->rp  = dnlookup(NAME(dname), Cin, 1);
>  		break;
> +	case Tdnskey:
>  	case Tkey:
>  		USHORT(rp->key->flags);
>  		UCHAR(rp->key->proto);
> diff -r 33920ebb68d1 sys/src/cmd/ndb/dn.c
> --- a/sys/src/cmd/ndb/dn.c	Thu Dec 17 21:02:11 2020 +0100
> +++ b/sys/src/cmd/ndb/dn.c	Thu Dec 17 17:48:13 2020 -0600
> @@ -1791,6 +1791,34 @@
>  	return rp;
>  }
>
> +RR*
> +mkopt(void)
> +{
> +	RR *rp;
> +	DN *dp;
> +
> +	rp = rralloc(Topt);
> +
> +	dp = emalloc(sizeof(*dp));
> +	dp->magic = DNmagic;
> +	dp->name = estrdup("");
> +	/* class holds our max UDP size */
> +	dp->class = Maxudp;
> +	dp->rr = nil;
> +	dp->referenced = now;
> +	dp->next = nil;
> +
> +	rp->owner = dp;
> +	/*
> +     * OPT TTL stores RSCODE, VERSION and DNSSEC Flag
> +	 * This signals RSCODE = 0, VERSION = 0, and no DNSSEC
> +     */
> +	rp->ttl = 0;
> +	rp->opt->dlen = 0;
> +	rp->opt->data = nil;
> +	return rp;
> +}
> +
>  void	bytes2nibbles(uchar *nibbles, uchar *bytes, int nbytes);
>
>  /*
> @@ -1951,6 +1979,7 @@
>  		rp->srv = emalloc(sizeof(*rp->srv));
>  		setmalloctag(rp->srv, rp->pc);
>  		break;
> +	case Tdnskey:
>  	case Tkey:
>  		rp->key = emalloc(sizeof(*rp->key));
>  		setmalloctag(rp->key, rp->pc);
> @@ -1963,6 +1992,10 @@
>  		rp->sig = emalloc(sizeof(*rp->sig));
>  		setmalloctag(rp->sig, rp->pc);
>  		break;
> +	case Topt:
> +		rp->opt = emalloc(sizeof(*rp->opt));
> +		setmalloctag(rp->opt, rp->pc);
> +		break;
>  	case Tnull:
>  		rp->null = emalloc(sizeof(*rp->null));
>  		setmalloctag(rp->null, rp->pc);
> @@ -1994,6 +2027,7 @@
>  		memset(rp->srv, 0, sizeof *rp->srv);	/* cause trouble */
>  		free(rp->srv);
>  		break;
> +	case Tdnskey:
>  	case Tkey:
>  		free(rp->key->data);
>  		memset(rp->key, 0, sizeof *rp->key);	/* cause trouble */
> @@ -2009,6 +2043,11 @@
>  		memset(rp->sig, 0, sizeof *rp->sig);	/* cause trouble */
>  		free(rp->sig);
>  		break;
> +	case Topt:
> +		free(rp->opt->data);
> +		memset(rp->opt, 0, sizeof *rp->opt);
> +		free(rp->opt);
> +		break;
>  	case Tnull:
>  		free(rp->null->data);
>  		memset(rp->null, 0, sizeof *rp->null);	/* cause trouble */
> diff -r 33920ebb68d1 sys/src/cmd/ndb/dns.h
> --- a/sys/src/cmd/ndb/dns.h	Thu Dec 17 21:02:11 2020 +0100
> +++ b/sys/src/cmd/ndb/dns.h	Thu Dec 17 17:48:13 2020 -0600
> @@ -135,7 +135,7 @@
>  	Reserved=	5*Min,
>
>  	/* packet sizes */
> -	Maxudp=		512,	/* maximum bytes per udp message sent */
> +	Maxudp=		4096,	/* maximum bytes per udp message sent */
>  	Maxudpin=	2048,	/* maximum bytes per udp message rcv'd */
>
>  	/* length of domain name hash table */
> @@ -171,6 +171,7 @@
>  typedef struct Sig	Sig;
>  typedef struct Srv	Srv;
>  typedef struct Txt	Txt;
> +typedef struct Opt	Opt;
>
>  /*
>   *  a structure to track a request and any slave process handling it
> @@ -236,6 +237,10 @@
>  {
>  	Block;
>  };
> +struct Opt
> +{
> +	Block;
> +};
>
>  /*
>   *  text strings
> @@ -292,6 +297,7 @@
>  		Sig	*sig;
>  		Null	*null;
>  		Txt	*txt;
> +		Opt	*opt;
>  	};
>  };
>
> @@ -485,6 +491,7 @@
>  int	tsame(int, int);
>  void	unique(RR*);
>  void	warning(char*, ...);
> +RR*	mkopt(void);
>
>  /* dnarea.c */
>  void	refresh_areas(Area*);
> diff -r 33920ebb68d1 sys/src/cmd/ndb/dnudpserver.c
> --- a/sys/src/cmd/ndb/dnudpserver.c	Thu Dec 17 21:02:11 2020 +0100
> +++ b/sys/src/cmd/ndb/dnudpserver.c	Thu Dec 17 17:48:13 2020 -0600
> @@ -9,6 +9,7 @@
>
>  static int	udpannounce(char*);
>  static void	reply(int, uchar*, DNSmsg*, Request*);
> +static void addopt(DNSmsg*, DNSmsg*);
>
>  typedef struct Inprogress Inprogress;
>  struct Inprogress
> @@ -258,6 +259,7 @@
>  				dnnotify(&reqmsg, &repmsg, &req);
>  				break;
>  			}
> +			addopt(&reqmsg, &repmsg);
>  			/* send reply on fd to address in buf's udp hdr */
>  			reply(fd, buf, &repmsg, &req);
>  			freeanswers(&repmsg);
> @@ -334,3 +336,18 @@
>  	if(write(fd, buf, len) != len)
>  		dnslog("error sending reply: %r");
>  }
> +
> +static void
> +addopt(DNSmsg *reqmsg, DNSmsg *repmsg)
> +{
> +	RR *qr, *rr;
> +
> +	for(qr = reqmsg->ar; qr != nil; qr = qr->next)
> +		if(qr->type == Topt){
> +			for(rr = repmsg->ar; rr->next != nil; rr = rr->next)
> +				;
> +			rr->next = mkopt();
> +			repmsg->arcount++;
> +			break;
> +		}
> +}
>

Re: [9front] [Patch] ndb/dns: DNSKEY and OPT RR types

[cinap_lenrek]

i wonder if the opt record should really be an RR and exposed
to the caching code. it is just a hack to communicate protocol
extensions by query, no? its not really part of the answer (->ar)
to the dns query? in that case attaching Topt records to the
queried domains seems very wrong to me.

--
cinap

Re: [9front] [Patch] ndb/dns: DNSKEY and OPT RR types

[Jacob Moody]

On 12/18/20 9:21 AM, cinap_lenrek@felloff.net wrote:
> i wonder if the opt record should really be an RR and exposed
> to the caching code. it is just a hack to communicate protocol
> extensions by query, no? its not really part of the answer (->ar)
> to the dns query? in that case attaching Topt records to the
> queried domains seems very wrong to me.

Thank you for the feedback.
 From my understanding, the opt record is only used to to communicate the protocol extension.
 From my testing with other DNS servers the opt RR is included in the additional section (->ar)
of the response when the client sends a request that includes it's own opt RR in the additional section.

Are your concerns regarding the caching the fact that opt RR's coming from upstream would be cached?

On 12/18/20 6:25 AM, hiro wrote:

> why not just one line maxudp = 4096 ?


I agree, I will fix that in a revised patch if this behavior is still desired.
If this whole EDNS stuff is undesired I can rip that part of the patch out, but still think
the code should catch the Tdnskey RR's, as having those get caught in the default case was causing some
parsing issues(or perhaps there is a another nice way of handling that, I am not sure)

Thanks,
Moody

Re: [9front] [Patch] ndb/dns: DNSKEY and OPT RR types

[cinap_lenrek]

> Are your concerns regarding the caching the fact that opt RR's coming from upstream would be cached?

I'm concerned because theres some logic that checks
if the answer section is empty (msg->ar == nil).

The current code *does* some filtering on the answers
to avoid cache poisoning or caching unknown record
types.

But i dont want to bet on it that it will filter
hacks like Topt records.

I'd rather keep the opt records separate. One thing
less to worry about. No accidents.

> I agree, I will fix that in a revised patch if this behavior is still desired.

Thanks.

> If this whole EDNS stuff is undesired I can rip that part of the patch out, 

No, this is good work. But i agree that it would be better
as separate patches.

> but still think the code should catch the Tdnskey RR's, 

Yes, absolutely.

> as having those get caught in the default case was causing some
> parsing issues(or perhaps there is a another nice way of handling
> that, I am not sure)
>
> Thanks,
> Moody

--
cinap

Re: [9front] [Patch] ndb/dns: DNSKEY and OPT RR types

[Jacob Moody]

Here is the patch for just dnskey records.

Thanks,
Moody


diff -r 33920ebb68d1 sys/src/cmd/ndb/convDNS2M.c
--- a/sys/src/cmd/ndb/convDNS2M.c	Thu Dec 17 21:02:11 2020 +0100
+++ b/sys/src/cmd/ndb/convDNS2M.c	Sun Dec 20 01:37:50 2020 -0600
@@ -275,6 +275,7 @@
 		NAME(rp->rmb->name);
 		NAME(rp->rp->name);
 		break;
+	case Tdnskey:
 	case Tkey:
 		USHORT(rp->key->flags);
 		UCHAR(rp->key->proto);
diff -r 33920ebb68d1 sys/src/cmd/ndb/convM2DNS.c
--- a/sys/src/cmd/ndb/convM2DNS.c	Thu Dec 17 21:02:11 2020 +0100
+++ b/sys/src/cmd/ndb/convM2DNS.c	Sun Dec 20 01:37:50 2020 -0600
@@ -448,6 +448,7 @@
 		rp->rmb = dnlookup(NAME(dname), Cin, 1);
 		rp->rp  = dnlookup(NAME(dname), Cin, 1);
 		break;
+	case Tdnskey:
 	case Tkey:
 		USHORT(rp->key->flags);
 		UCHAR(rp->key->proto);
diff -r 33920ebb68d1 sys/src/cmd/ndb/dn.c
--- a/sys/src/cmd/ndb/dn.c	Thu Dec 17 21:02:11 2020 +0100
+++ b/sys/src/cmd/ndb/dn.c	Sun Dec 20 01:37:50 2020 -0600
@@ -893,6 +893,7 @@
 		nrp->srv = srv;
 		*srv = *rp->srv;
 		break;
+	case Tdnskey:
 	case Tkey:
 		key = nrp->key;
 		*nrp = *rp;
@@ -1273,6 +1274,7 @@
 	case Trp:
 		fmtprint(&fstr, "\t%s %s", dnname(rp->rmb), dnname(rp->rp));
 		break;
+	case Tdnskey:
 	case Tkey:
 		if (rp->key == nil)
 			fmtprint(&fstr, "\t<null> <null> <null>");
@@ -1413,6 +1415,7 @@
 			idnname(rp->rmb, buf, sizeof(buf)),
 			idnname(rp->rp, buf, sizeof(buf)));
 		break;
+	case Tdnskey:
 	case Tkey:
 		if (rp->key == nil)
 			fmtprint(&fstr, " flags=<null> proto=<null> alg=<null>");
@@ -1951,6 +1954,7 @@
 		rp->srv = emalloc(sizeof(*rp->srv));
 		setmalloctag(rp->srv, rp->pc);
 		break;
+	case Tdnskey:
 	case Tkey:
 		rp->key = emalloc(sizeof(*rp->key));
 		setmalloctag(rp->key, rp->pc);
@@ -1994,6 +1998,7 @@
 		memset(rp->srv, 0, sizeof *rp->srv);	/* cause trouble */
 		free(rp->srv);
 		break;
+	case Tdnskey:
 	case Tkey:
 		free(rp->key->data);
 		memset(rp->key, 0, sizeof *rp->key);	/* cause trouble */

Re: [9front] [Patch] ndb/dns: DNSKEY and OPT RR types

[cinap_lenrek]

looks good! i'll commit.

thank you!

--
cinap

Re: [9front] [Patch] ndb/dns: DNSKEY and OPT RR types

[cinap_lenrek]

that said, i have no problem with chaining the opt fields of
a query or response in a different chain in the message struct.
but chaining it in the msg->ar chain is completely wrong, and
will most likeley break all kinds of assumptions.

--
cinap