Ignore DNSKEY RRs with incalculable key sizes

[mattias-p]

Purpose

This PR makes us ignore DNSKEY RRs with empty public key fields instead of crashing.

Context

Fixes #110.
Implements the specification updates from zonemaster/zonemaster#1041.

Changes

• Zonemaster::LDNS::RR::DNSKEY::keydata() is updated to work with ldns's internal representation of an empty DNSKEY public key field. (An issue has been created about changing the internal interpretation: DNSKEY RR with only three fields NLnetLabs/ldns#156)
• Zonemaster::LDNS::RR::keysize() is ported from XS to Perl.
• Zonemaster::LDNS::RR::keysize() is fixed to return the -1 sentinel value instead of crashing or returning a negative number of bits.
• Zonemaster::LDNS::Package::answer() is changed to filter out DNSKEY RRs with incalculable key sizes. The old behavior is still available under a new name: Zonemaster::LDNS::Package::answer_unfiltered().

How to test this PR

A test case is provided in #110.

Notes

After this PR when a zone with a single bad DNSKEY whose key size is incalculable, we get the following behaviors:

• DNSSEC03 emits a NO_DNSKEY message.
• DNSSEC07 emits a DS_BUT_NOT_DNSKEY message.
• DNSSEC05 emits one NO_RESPONSE_DNSKEY message for each name server that returned the bad DNSKEY record.
• DNSSEC14 emits one NO_RESPONSE_DNSKEY message for each name server that returned the bad DNSKEY record.

In principle I believe these are reasonable behaviors, but at least some of the tag names are a bit misleading. Especially the word "response" is unfortunate in this context. Perhaps it would also make sense to qualify "dnskey" with the adjective "valid" since there might actually be an RR in the answer section with the DNSKEY type that just happens to be broken enough that it is ignored.

[mattias-p]

I rebased onto the "Split section accessors into XS and Perl parts" commit from #136 in order to make the conflict resolution simpler when merging the second one.

[matsduf]

How does this PR relate to #136? I see similar or identical changes in both.

[matsduf]

This should be fine, but unrelated changes should move to another PR.

[matsduf]

This is not related to this PR as defined in the description. Move this to a separate PR. More of that below.

[ghost]

Nice! Tanks for the detailed description and documentation. I agree it would be nice to have better messages. In the meantime this is good to me.

[tgreenx]

LGTM, a welcome change

[marc-vanderwal]

That’s odd: according to my testing, the unit tests pass, but by following the testing procedure in this PR, I still get crashes.

I saved data-good.txt and data-bad.txt from here in a directory called “data”, merged the PR locally, then built a Docker image for zonemaster-cli. Then I ran the following command:

$ docker run --rm --volume=$PWD/data:/data \
       zonemaster/cli:local \
       --no-ipv6 --test dnssec --restore /data/data-bad.txt nlagriculture.nl
Seconds Level     Message
======= ========= =======
Assertion failed: rd != NULL (./rdata.c: ldns_rdf_size: 26)

Yet I could verify that my Docker image had indeed the modified version of Zonemaster::LDNS.

What’s even stranger, is that running the same test against the current state of develop gives this result:

Seconds Level     Message                                                                  
======= ========= =======                                                                  
   0.67 ERROR     The DNSKEY record with tag 12993 that the DS refers to does not exist in the DNSKEY RRset. Fetched from the nameservers with IP "145.100.177.67;178.22.85.27;185.136.96.82;94.228.142.136".                                                                    
Assertion failed: rd != NULL (./rdata.c: ldns_rdf_size: 26)

Maybe data-{good,bad}.txt are wrong in some way, or have broken DNSKEYs in ways that this pull request hasn’t yet guarded against?

[marc-vanderwal]

It seems that data-bad.txt broke DNSKEY RRs differently than what issue #110 described.

In that file, I found one instance of a DNSKEY record that is completely empty (i.e. with zero RDLENGTH, and thus zero fields out of the required four) instead of only missing its public key.

Out of curiosity, I added the following unit test to t/rr.t:

    # Contains a DNSKEY RR that is completely empty (i.e. has zero
    # fields out of the required 4)
    subtest 'malformed RR: completely empty' => sub {
	my $data = decode_base64( "BleFgAABAAEAAAAADW5sYWdyaWN1bHR1cmUCbmwAAAEAAcAMADAAAQAAAAAAAA==");
	my $p = Zonemaster::LDNS::Packet->new_from_wireformat( $data );
	my ( $rr, @extra ) = $p->answer_unfiltered;
	eq_or_diff \@extra, [], "no extra RRs found";
	if ( !defined $rr ) {
	    BAIL_OUT( "no RR found" );
	}
	is $rr->keydata, "", "empty DNSKEY RR yields empty keydata";
	is $rr->keysize, -1, "empty DNSKEY RR yields keysize of -1";
    }

This triggered the exact assertion failure I saw when running my tests with data-{good,bad}.txt: Assertion failed: rd != NULL (./rdata.c: ldns_rdf_size: 26).

So, in a nutshell:

• LDNS crashes with an assertion failure when decoding an invalid DNSKEY that is missing all its mandatory fields.
• LDNS also crashes with an assertion failure when decoding an invalid DNSKEY that is only missing its fourth and last field (public key).
• This PR works around the second case, but not the first one (but should we?).
• And the test data in data-bad.txt actually triggers the first case.

So, in the end, this PR looks good to me; only the “how to test this PR” was wrong, because the unit tests were sufficient. But it does also mean that someone should fuzz test LDNS.