Bug #111559 for Net-DNS: 1.04: TSIG not working anymore (TSIG.pm)

Fri Jan 29 20:08:44 2016 jth [...] jth.net - Ticket created

Subject:	1.04: TSIG not working anymore (TSIG.pm)
Date:	Sat, 30 Jan 2016 02:08:13 +0100
To:	bug-Net-DNS [...] rt.cpan.org
From:	"Jørgen Thomsen" <jth [...] jth.net>

It appears to me, that bugs have been introduced into the TSIG handling. 1) algorithm names (HMAC-SHA384 etc) are in upper case, but BIND9 is using lower case causing the case sensitive comparisons in TSIG.pm to fail: e.g. unless ( $signerkey eq $priorkey ) { 2) changing the algorithm name to lower case is not sufficient as several places it is assumed that it is in upper case and forced to upper case or stripped from lower case chars (why strip dashes and '.' in the names beats me and complicates things ?). 3) even when fixing 1) and 2) the verify-procedure is failing with BADSIG (my test case is an AXFR) 4) the extensive use of terminating with BADSIG, BADKEY etc instead of returning a boolean false as described in the documentation should definitely be avoided. A Perl warn could be used for a message to be displayed or found in the webserver log. "The boolean verify method will return true if the hash over the packet data conforms to the data in the TSIG itself" - Jørgen Th.

Sat Jan 30 10:17:54 2016 rwfranks [...] acm.org - Correspondence added

From:

rwfranks [...] acm.org

On Fri Jan 29 20:08:44 2016, jth@jth.net wrote: Show quoted text

> 1) algorithm names (HMAC-SHA384 etc) are in upper case, but BIND9 is > using lower case > causing the case sensitive comparisons in TSIG.pm to fail: > > e.g. unless ( $signerkey eq $priorkey ) {

Noted. This will be fixed in 1.05 and in upcoming dev release. Show quoted text

> 2) changing the algorithm name to lower case is not sufficient as > several places it is > assumed that it is in upper case and forced to upper case or stripped > from lower case > chars (why strip dashes and '.' in the names beats me and complicates > things ?).

The IANA registries list upper case mnemonics and some are inconsistent in use of - (eg DSA-NSEC3-SHA1 and ECC-GOST, but ECDSAP256SHA256). Same lump of code used in TSIG.pm and elsewhere. Show quoted text

> 3) even when fixing 1) and 2) the verify-procedure is failing with > BADSIG (my test case is > an AXFR)

I have added a verified AXFR multi-packet zone transfer to the test suite. This works with BIND 9.10. Show quoted text

> 4) the extensive use of terminating with BADSIG, BADKEY etc instead of > returning a boolean > false as described in the documentation should definitely be > avoided. A Perl warn > could be used for a message to be displayed or found in the > webserver log. > > "The boolean verify method will return true if the hash over the > packet data conforms > to the data in the TSIG itself" >

Which it does. Perl's notion of "true" is something which is not '', 0 or undef. In the general case, AXFR produces a multi-packet response. The "true" objects returned by TSIG verify are used to propagate intermediate results along the chain. The verify section of the documentation for Packet shows how this works. It should be unnecessary to deal directly with the signature RRs.

Sat Jan 30 10:17:54 2016 The RT System itself - Status changed from 'new' to 'open'

Sun Jan 31 12:49:40 2016 jth [...] jth.net - Correspondence added

Subject:	Re: [rt.cpan.org #111559] 1.04: TSIG not working anymore (TSIG.pm)
Date:	Sun, 31 Jan 2016 18:49:06 +0100
To:	bug-Net-DNS [...] rt.cpan.org
From:	"Jørgen Thomsen" <jth [...] jth.net>

On Sat, 30 Jan 2016 10:18:00 -0500,"Dick Franks via RT" <bug-Net-DNS@rt.cpan.org> wrote: Show quoted text

>> 4) the extensive use of terminating with BADSIG, BADKEY etc instead of >> returning a boolean >> false as described in the documentation should definitely be >> avoided. A Perl warn >> could be used for a message to be displayed or found in the >> webserver log. >>

I have to explain this better. Net::DNS is a utility to be used in other programs. It is not a self-contained program. Utilities should NEVER execute a die or croak, as this will terminate the calling program preventing it from handling the erroneous situation. It is a bad programming practice to terminate the calling program. In Perl this probem can be can be circumvented by using eval. However, having to enclose every call to Net::DNS in an eval is clumsy and definitely not a standard way to use utilities. One place I have now changed my 10+ year old code from my @zoneRRs = $resolv->axfr($zoneidna); to my @zoneRRs = (); eval { @zoneRRs = $resolv->axfr($zoneidna); }; warn $@ if $@; to avoid displaying an ugly 500 web server error message to the user and instead handling the case of no RRs returned. I am not so happy of doing this for every call in this program, which has been working well for 10+ years until version 1.04 (except for some tsig problems around version 0.73). So, please, remove all dies and croaks from the code. - Jørgen Th

Sun Jan 31 20:13:27 2016 rwfranks [...] acm.org - Correspondence added

From:

rwfranks [...] acm.org

On Sun Jan 31 12:49:40 2016, jth@jth.net wrote: Show quoted text

> > One place I have now changed my 10+ year old code from > > my @zoneRRs = $resolv->axfr($zoneidna); > > to > > my @zoneRRs = (); > eval { @zoneRRs = $resolv->axfr($zoneidna); }; warn $@ if $@; > > to avoid displaying an ugly 500 web server error message to the user > and instead handling > the case of no RRs returned. > I am not so happy of doing this for every call in this program, which > has been working > well for 10+ years until version 1.04 (except for some tsig problems > around version 0.73).

The whole point of exception handling is to avoid having to wrap tests for error conditions around every subroutine call. A few well placed eval{}s at a suitably high level in the application trap all mishaps in everything from your own code right down into dark corners of packages on which Net::DNS itself depends. More significantly, it also handles unexpected program failures for which there is no possible defence at the application layer. Show quoted text

> So, please, remove all dies and croaks from the code.

I do not propose to do that. A large part of the efficiency improvement in recent years comes from getting a proper grip on error handling. An exception is exactly that, not a normal response to a request. Perhaps you need to concentrate of what the message is telling you rather than rail at the error handling model.

Wed Mar 09 05:23:15 2016 NLNETLABS [...] cpan.org - Status changed from 'open' to 'resolved'