Bug #115558 for Net-DNS: Net::DNS::Nameserver does not allow EDNS replies

Thu Jun 23 13:59:25 2016 bjorn [...] mork.no - Ticket created

Subject:	Net::DNS::Nameserver does not allow EDNS replies
Date:	Thu, 23 Jun 2016 19:59:08 +0200
To:	bug-Net-DNS [...] rt.cpan.org
From:	Bjørn Mork <bjorn [...] mork.no>

Hello, I have implemented a small server with DNSSEC support, but have been unable to figure out any way to make it return the required OPT record. The problem is that Net::DNS::Nameserver::make_reply() unconditionally calls my $reply = $query->reply(); without any possibility to set the optional $UDPmax argument to ->reply(). So ->reply() calls ->edns->size($UDPmax) with $UDPmax undefined: $reply->edns->size($UDPmax) if $query->edns->_specified; and the result is no Net::DNS::RR::OPT. I propose either just set a default bufsize and unconditionally enabled EDNS0 support. There is really no place for a server without it anymore... Or make it optional, like the proposed patch does. But even with the EDNS0 OPT in place, there is still no way to set the "DO" flag on replies. I propose adding that to the existing headermask and leave it to the ReplyHandler implementation to set it as appropriate. This is also done in the attached patch. Thanks for your excellent DNS implementation. This is extremely useful for implementing special purpose DNS services. Bjørn

Message body is not shown because sender requested not to inline it.

Sat Jun 25 23:22:03 2016 jmaslak [...] antelope.net - Correspondence added

On Thu Jun 23 13:59:25 2016, bjorn@mork.no wrote: Show quoted text

> and the result is no Net::DNS::RR::OPT. I propose either just set a > default bufsize and unconditionally enabled EDNS0 support. There is > really no place for a server without it anymore... Or make it optional, > like the proposed patch does.

I'm just another Perl hacker here, with no influence on the module author. I'm just another person who has used this module and found it very helpful. I would suggest some changes to this part. I'd suggest adding $reply to @arglist that is passed to the handler functions, so that full control over $reply can be done by user code if needed. I'm thinking it should be reasonably safe to add it to the end of @arglist. This has the added benefit of being able to override any other behavior (not just the size, but any EDNS options that you might want to set, not to mention possibly setting different versions of EDNS and the like). Alternatively, expose $reply->edns by itself. Show quoted text

> But even with the EDNS0 OPT in place, there is still no way to set the > "DO" flag on replies. I propose adding that to the existing headermask > and leave it to the ReplyHandler implementation to set it as > appropriate. This is also done in the attached patch.

I like this. :) I think this might be able to accomplish a lot of what you desire (but not necessarily adding an EDNS option) if it was extended a bit. I.E. support do, cd, z, and EDNS size (which can also be accessed as $header->size()) via this mechanism. Perhaps add your "$header->do(1) if $headermask->{ad};" and similar for do, cd, and z, which would look substantially identical. Size would have to take a value and pass that to $header->size(). You could probably even do the same thing as size with edns flags in general and perhaps even EDNS options (I.E. perhaps a flag value of "edns_option" could be an arrayref containing option number and option data pairs - I initially thought maybe a hashref with the key being the edns option, but I suspect it's possible in the future to have send the same option more than once). I too would like to thank the author for his work, which makes it pretty simple to write DNS servers.

Sat Jun 25 23:22:04 2016 The RT System itself - Status changed from 'new' to 'open'

Sun Jun 26 11:11:51 2016 rwfranks [...] acm.org - Correspondence added

From:

rwfranks [...] acm.org

On Sat Jun 25 23:22:03 2016, JMASLAK wrote: Show quoted text

> On Thu Jun 23 13:59:25 2016, bjorn@mork.no wrote: >

> > and the result is no Net::DNS::RR::OPT. I propose either just set a > > default bufsize and unconditionally enabled EDNS0 support. There is > > really no place for a server without it anymore... Or make it > > optional, > > like the proposed patch does.

Firstly, may I underline the point that Nameserver.pm is *not* a production grade implementation, never was, never will be. It is a plaything for testing only. Show quoted text

> I would suggest some changes to this part. I'd suggest adding $reply > to @arglist that is passed to the handler functions, so that full > control over $reply can be done by user code if needed. I'm thinking > it should be reasonably safe to add it to the end of @arglist. This > has the added benefit of being able to override any other behavior > (not just the size, but any EDNS options that you might want to set,

No need to do that to set UDP size and DO flag. Show quoted text

> > But even with the EDNS0 OPT in place, there is still no way to set > > the > > "DO" flag on replies. I propose adding that to the existing > > headermask > > and leave it to the ReplyHandler implementation to set it as > > appropriate. This is also done in the attached patch.

> > I like this. :) I think this might be able to accomplish a lot of > what you desire (but not necessarily adding an EDNS option) if it was > extended a bit.

This suggestion appeals to my code reduction instincts, even if the rest of me is unmoved. $headermask = { DO => 1, size => 12345 }

Subject:

Nameserver.pm-patch

Download Nameserver.pm-patch
application/octet-stream 845b

Message body not shown because it is not plain text.

Sun Jun 26 11:47:10 2016 bjorn [...] mork.no - Correspondence added

Subject:	Re: [rt.cpan.org #115558] Net::DNS::Nameserver does not allow EDNS replies
Date:	Sun, 26 Jun 2016 17:46:42 +0200
To:	"Dick Franks via RT" <bug-Net-DNS [...] rt.cpan.org>
From:	Bjørn Mork <bjorn [...] mork.no>

"Dick Franks via RT" <bug-Net-DNS@rt.cpan.org> writes: Show quoted text

> On Sat Jun 25 23:22:03 2016, JMASLAK wrote:

>> On Thu Jun 23 13:59:25 2016, bjorn@mork.no wrote: >>

>> > and the result is no Net::DNS::RR::OPT. I propose either just set a >> > default bufsize and unconditionally enabled EDNS0 support. There is >> > really no place for a server without it anymore... Or make it >> > optional, >> > like the proposed patch does.

> > Firstly, may I underline the point that Nameserver.pm is *not* a > production grade implementation, never was, never will be. It is a > plaything for testing only.

Sure, like all good code :) Show quoted text

> This suggestion appeals to my code reduction instincts, even if the rest of me is unmoved. > > $headermask = { DO => 1, size => 12345 } >

.. Show quoted text

> *** /home/rwf/svn/net-dns/lib/Net/DNS/Nameserver.pm 2015-10-05 09:25:49.640428000 +0100 > --- Nameserver.pm 2016-06-26 15:42:48.763186288 +0100 > *************** > *** 251,265 **** > $reply->{additional} = [@$add] if $add; > } > > ! if ( !defined($headermask) ) { > ! $header->ra(1); > ! $header->ad(0); > ! } else { > ! $header->opcode( $headermask->{opcode} ) if $headermask->{opcode}; > > ! $header->aa(1) if $headermask->{aa}; > ! $header->ra(1) if $headermask->{ra}; > ! $header->ad(1) if $headermask->{ad}; > } > > $header->print if $self->{Verbose} && defined $headermask; > --- 251,260 ---- > $reply->{additional} = [@$add] if $add; > } > > ! $headermask ||= { ra => 1, ad => 0 }; > > ! while ( my ( $key, $value ) = each %$headermask ) { > ! $header->$key($value); > } > > $header->print if $self->{Verbose} && defined $headermask;

Nice! I like that. Works for me. Bjørn

Wed Jun 29 04:18:21 2016 bjorn [...] mork.no - Correspondence added

Subject:	Re: [rt.cpan.org #115558] Net::DNS::Nameserver does not allow EDNS replies
Date:	Wed, 29 Jun 2016 10:17:44 +0200
To:	"Dick Franks via RT" <bug-Net-DNS [...] rt.cpan.org>
From:	Bjørn Mork <bjorn [...] mork.no>

Just an additional thing to consider before deciding on the API here: The proposed solution still does not allow adding any ENDS options. For fun, I thought I might as well add NSID to my small server. But you need the $reply->edns Net::DNS::RR::OPT object for that. So I added the rather ugly $header->edns->options(@{delete $headermask->{options}}) if exists($headermask->{options}); in Nameserver.pm:make_reply(). But this overloading of the $headermask is starting to feel odd. Better alternatives are wanted. You might want to consider how this feature can be supported before deciding on the DO/size support. The headermask support does not make much sense if the edns object becomes directly available in the reply_handler. Yes, yes, noone *needs* any EDNS options. I know :) Bjørn

Wed Jun 29 11:46:12 2016 rwfranks [...] acm.org - Correspondence added

From:

rwfranks [...] acm.org

On Wed Jun 29 04:18:21 2016, bjorn@mork.no wrote: Show quoted text

> Just an additional thing to consider before deciding on the API here: > The proposed solution still does not allow adding any ENDS options. > > For fun, I thought I might as well add NSID to my small server. But > you > need the $reply->edns Net::DNS::RR::OPT object for that. So I added > the > rather ugly > > $header->edns->options(@{delete $headermask->{options}}) if > exists($headermask->{options}); > > in Nameserver.pm:make_reply(). But this overloading of the $headermask > is starting to feel odd. Better alternatives are wanted. You might > want to consider how this feature can be supported before deciding on > the DO/size support. The headermask support does not make much sense > if > the edns object becomes directly available in the reply_handler.

The DO and size support was a nil-cost extension of what was already there. Otherwise, it would likely not be done at all. Show quoted text

> Yes, yes, noone *needs* any EDNS options. I know :)

If nobody needs it, why waste time and effort on making special provision for it? Anyway, the reply handler is free to insert an OPT RR with options into the additional section which should be good enough for test purposes. As I noted at the start of this thread, Nameserver.pm falls a long way short of being a proper nameserver implementation.

Thu Nov 03 08:56:08 2016 NLNETLABS [...] cpan.org - Correspondence added

Will be in the 1.07 release

Thu Nov 03 08:56:09 2016 NLNETLABS [...] cpan.org - Status changed from 'open' to 'resolved'

Wed Dec 28 04:22:10 2016 ABBYPAN [...] cpan.org - Correspondence added

I encountered the same OPT problem as bjorn. the patch is addon: dns-reply-missing-edns.diff debug trace as follows: 1. Nameserver.pm sub tcp_connection / udp_connection call my $reply = $self->make_reply( $query, $sock->peerhost, $conn ); to make reply packet then call $reply->data to send reply packet data 2. $self->make_reply sub make_reply() my $reply = $query->reply(); init the reply packet 2.1 Packet.pm : sub reply() $reply->edns->size($UDPmax) if $query->edns->_specified; set edns size 2.2 Packet.pm : sub edns() sub edns { my $self = shift; my $link = \$self->{xedns}; ($$link) = grep $_->isa(qw(Net::DNS::RR::OPT)), @{$self->{additional}} unless $$link; $$link = new Net::DNS::RR( type => 'OPT' ) unless $$link; } if the query contains a EDNS OPT, when my $reply = $query->reply(); first call $reply->edns->size($UDPmax), $self->{xedns} is null, $link is set to $self->{xedns}'s ref address. because $self->{additional} is null, $$link is set to a new empty OPT. that means, $self->{xedns} is set to this new empty OPT. next time call $reply->edns $link = \$self->{xedns} is ref to the empty OPT which created in $reply->edns->size($UDPmax) initiation. $$link will always be true, next two lines won't work: ($$link) = grep $_->isa(qw(Net::DNS::RR::OPT)), @{$self->{additional}} unless $$link; $$link = new Net::DNS::RR( type => 'OPT' ) unless $$link; $self->edns is always the empty OPT. 3. $reply->data $self->edns is empty OPT, $edns->_specified always 0, no edns OPT will be unshift to @addl, that is why EDNS OPT is missing in reply packet. sub data { &encode; } sub encode { my ( $self, $size ) = @_; # uncoverable pod my $edns = $self->edns; # EDNS support my @addl = grep !$_->isa('Net::DNS::RR::OPT'), @{$self->{additional}}; unshift( @addl, $edns ) if $edns->_specified; $self->{additional} = \@addl; # ... }

Subject:

dns-reply-missing-edns.patch

--- Net-DNS-1.06_05/lib/Net/DNS/Packet.pm 2016-11-12 11:23:47.000000000 +0800 +++ Net-DNS-1.06_05.fix/lib/Net/DNS/Packet.pm 2016-12-28 16:13:02.577195268 +0800 @@ -237,10 +237,12 @@ =cut sub edns { - my $self = shift; - my $link = \$self->{xedns}; - ($$link) = grep $_->isa(qw(Net::DNS::RR::OPT)), @{$self->{additional}} unless $$link; - $$link = new Net::DNS::RR( type => 'OPT' ) unless $$link; + my $self = shift; + my $link ; + $link = \$self->{xedns} if(exists $self->{xedns}); + ($$link) = grep $_->isa(qw(Net::DNS::RR::OPT)), @{$self->{additional}} unless ($link and $$link); + $$link = new Net::DNS::RR( type => 'OPT' ) unless ($link and $$link); + return $$link; }

Thu Dec 29 11:56:30 2016 rwfranks [...] acm.org - Correspondence added

From:

rwfranks [...] acm.org

On Wed Dec 28 04:22:10 2016, ABBYPAN wrote: Show quoted text

> I encountered the same OPT problem as bjorn. > > the patch is addon: dns-reply-missing-edns.diff >

Patch, as written, breaks a few install tests. However, your diagnosis was correct. Thanks for doing the work. Solution will be in 1.07 soon to be released.

Sun Jan 01 05:09:00 2017 ABBYPAN [...] cpan.org - Correspondence added

RT-Send-CC:

rwfranks [...] acm.org

thanks a lot, v1.07 works now. 在2016-十二月-29 11:56:30 星期四时，rwfranks@acm.org写到： Show quoted text

> On Wed Dec 28 04:22:10 2016, ABBYPAN wrote:

> > I encountered the same OPT problem as bjorn. > > > > the patch is addon: dns-reply-missing-edns.diff > >

> Patch, as written, breaks a few install tests. > > However, your diagnosis was correct. Thanks for doing the work. > > Solution will be in 1.07 soon to be released. >