Update CNAME resolution

[tgreenx]

Purpose

This PR slightly updates the CNAME resolution of Engine's recursor. See the Changes section below.
Updated test scenarios are defined in zonemaster/zonemaster#1477.

Context

zonemaster/zonemaster#1477

Changes

• Zonemaster::Engine::Recursor::_resolve_cname() now returns undef when the CNAME resolution fails, along with new message tags: CNAME_UNRESOLVABLE (ERROR level) and CNAME_TO_NODATA (DEBUG level)
• Update unit tests:
  • Move the content of scenario TOO-LONG-CNAME-CHAIN to a new scenario TOO-MANY-CNAME,
  • Update scenario TOO-LONG-CNAME-CHAIN to check for tag CNAME_CHAIN_TOO_LONG instead
  • Add scenario UNRESOLVABLE-CNAME
  • Add scenario ``CNAME-CHAIN-TO-NODATA`

How to test this PR

Unit tests are updated and should pass.

[marc-vanderwal]

So far, so good.

[marc-vanderwal]

The CNAME_UNRESOLVABLE tag is output in some cases where the final query for a CNAME chain gives NODATA. That case can happen fairly frequently and should simply be treated like a negative response (i.e. NXDOMAIN), not like a resolution failure (as if we had gotten SERVFAIL).

I’d suggest a different name for that tag. How about CNAME_TO_NODATA?

[matsduf]

I’d suggest a different name for that tag. How about CNAME_TO_NODATA?

It think that is a good suggestion. Should there be another tag, CNAME_TO_NXDOMAIN?

For me CNAME_UNRESOLVABLE sounds like we cannot resolve the CNAME record.

[marc-vanderwal]

Yes, a CNAME_TO_NXDOMAIN might make sense too.

In any case, I don’t think the ERROR level is appropriate for these tags because they do not indicate a problem so severe that the zone is unresolvable.

[matsduf]

@tgreenx, will you fix the conflict?

[tgreenx]

@tgreenx, will you fix the conflict?

@matsduf @marc-vanderwal done, please re-review.

[tgreenx]

I’d suggest a different name for that tag. How about CNAME_TO_NODATA?

Added.

I’d suggest a different name for that tag. How about CNAME_TO_NODATA?

It think that is a good suggestion. Should there be another tag, CNAME_TO_NXDOMAIN?

Not needed, NXDOMAIN responses are already handled by the calling method (_recurse()).

[matsduf]

Even if name "is_in_bailiwick" it can be out of zone. Name can belong to a child zone (or several delegations down).

[matsduf]

I think the tag CNAME_UNRESOLVABLE should be defined or at least described. Probably together with CNAME_TO_NODATA and other tags.

[marc-vanderwal]

In order to be consistent with the Zonemaster::Engine::Nameserver->query() method, I think we should return undef if and only if we had a timeout. Now we hit this case when the packet isn’t something we expect, e.g. a FORMERR response. We should still return that packet to the caller.

There’s a subtle bug that I missed when reviewing. I’ll propose a change.

[marc-vanderwal]

If we are following a chain of aliases (CNAMEs) and at some point, the owner name of the next item in the chain is in a different case than the previous item’s CNAME’s RDATA, then we erroneously report a broken chain. There’s an example in the wild of that.

This line should be:

        $cnames{lc( $rr_owner )} = $rr_target;

[matsduf]

Isn't clearer to write:

$rr_target = lc( $rr_target );
$rr_owner = lc( $rr_owner );

$seen_targets{ $rr_target } = 1;
$forbidden_targets{ $rr_owner } = 1;
$cnames{ $rr_owner } = $rr_target;