Record type APL: adjust error handling and tests

[tsellers-r7]

Summary

This PR:

• Removes an error when processing APL record types where the address is not aligned with the prefix (netmask).
• Enhances testing so as to test for a specific Error with the goal of making it easier to understand which conditions were being tested.

Reasoning

When processing an APL record where the address is not on the prefix (netmask) boundary the current code will emit the error dns: invalid APL address length and cease processing further dns.RR records.

dns/msg_helpers.go

Lines 795 to 798 in 1630ffe

	network := ipnet.IP.Mask(ipnet.Mask)
	if !network.Equal(ipnet.IP) {
	return APLPrefix{}, len(msg), &Error{err: "invalid APL address length"}
	}

For example, if the response includes 192.155.84.18/24 this would be an error because the correct boundary for a /24 CIDR would be 192.155.84.0 instead of .18.

In my opinion this response should not be treated as a fatal processing error because:

• Per RFC3120 there is no requirement for the address to be on the netmask boundary
• This is not a packet decoding issue, this data accurately reflects what was sent on the wire
• This is user controlled data and users make mistakes. It would be similar to an incorrectly formatted SPF record that is valid on the wire but won't perform as expected.
• The failure here prevents other records in an ANY response from being processed.

While testing the changes I noticed that, in addition to the test I expected to fail, another test was now failing:

dns/msg_helpers_test.go

Lines 408 to 410 in 1630ffe

	"address with extra byte",
	[]byte{0x00, 0x01, 0x10, 0x03, 192, 0, 2},
	},

I wasn't sure what condition that test was trying to check for so I reworked the tests to check for specific Errors. This enabled me to ensured that the major failure paths were being tested and added a bit of clarity that some of the test names were missing.

Testing / Example output

For all testing below I've used the q example from the master branch of github.com/miekg/exdns. Testing was performed with go version go1.17 darwin/amd64 on macOS Catalina. The problem was originally noticed while running on Ubuntu 20.04 LTS.

Example output - Current Code

APL record lookup

$ go run q.go --fallback @8.8.8.8  APL  IN ip4.loopy.limon.baker.org
;; dns: invalid APL address length

ANY record lookup

$ go run q.go --fallback @8.8.8.8  ANY  IN ip4.loopy.limon.baker.org
;; dns: invalid APL address length

Example output - PR Code

APL record lookup

$ go run q.go --fallback @8.8.8.8  APL  IN ip4.loopy.limon.baker.org
;; opcode: QUERY, status: NOERROR, id: 33976
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 0

;; QUESTION SECTION:
;ip4.loopy.limon.baker.org.	IN	 APL

;; ANSWER SECTION:
ip4.loopy.limon.baker.org.	1795	IN	APL	1:192.155.84.18/24

;; query time: 45093 µs, server: 8.8.8.8:53(udp), size: 87 bytes

ANY record lookup

$ go run q.go --fallback @8.8.8.8  ANY  IN ip4.loopy.limon.baker.org
;; opcode: QUERY, status: NOERROR, id: 11923
;; flags: qr rd ra; QUERY: 1, ANSWER: 7, AUTHORITY: 0, ADDITIONAL: 0

;; QUESTION SECTION:
;ip4.loopy.limon.baker.org.	IN	 ANY

;; ANSWER SECTION:
ip4.loopy.limon.baker.org.	1800	IN	APL	1:192.155.84.18/24
ip4.loopy.limon.baker.org.	1800	IN	A	192.155.84.18
ip4.loopy.limon.baker.org.	1800	IN	SSHFP	1 1 629D772D9EA6EB31B9E3C96D3982E02E6D5986E2
ip4.loopy.limon.baker.org.	1800	IN	SSHFP	1 2 70CC2CFC17EBA6AF8039A7246E091E727FF5DE109C3ACD8D61754048EA704A34
ip4.loopy.limon.baker.org.	1800	IN	SSHFP	2 1 E0F51B2F5D7C9D2F8C9FF9556088111C0A8908EB
ip4.loopy.limon.baker.org.	1800	IN	SSHFP	2 2 37BABEDDE292754BA3174472DD7DF91B8C047FFE98B5D3247792DD7F9B1CDF3C
ip4.loopy.limon.baker.org.	1800	IN	PTR	loopy.limon.baker.org.

;; query time: 167087 µs, server: 8.8.8.8:53(udp), size: 448 bytes

Example output - dig

APL record lookup

$ dig @8.8.8.8 APL ip4.loopy.limon.baker.org

; <<>> DiG 9.10.6 <<>> @8.8.8.8 APL ip4.loopy.limon.baker.org
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 24973
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 512
;; QUESTION SECTION:
;ip4.loopy.limon.baker.org.	IN	APL

;; ANSWER SECTION:
ip4.loopy.limon.baker.org. 1800	IN	APL	1:192.155.84.18/24

;; Query time: 1120 msec
;; SERVER: 8.8.8.8#53(8.8.8.8)
;; WHEN: Tue Sep 28 08:50:52 CDT 2021
;; MSG SIZE  rcvd: 74

ANY record lookup

$ dig @8.8.8.8 ANY ip4.loopy.limon.baker.org

; <<>> DiG 9.10.6 <<>> @8.8.8.8 ANY ip4.loopy.limon.baker.org
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 34224
;; flags: qr rd ra; QUERY: 1, ANSWER: 7, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 512
;; QUESTION SECTION:
;ip4.loopy.limon.baker.org.	IN	ANY

;; ANSWER SECTION:
ip4.loopy.limon.baker.org. 1800	IN	APL	1:192.155.84.18/24
ip4.loopy.limon.baker.org. 1800	IN	A	192.155.84.18
ip4.loopy.limon.baker.org. 1800	IN	SSHFP	2 1 E0F51B2F5D7C9D2F8C9FF9556088111C0A8908EB
ip4.loopy.limon.baker.org. 1800	IN	SSHFP	1 2 70CC2CFC17EBA6AF8039A7246E091E727FF5DE109C3ACD8D61754048 EA704A34
ip4.loopy.limon.baker.org. 1800	IN	SSHFP	2 2 37BABEDDE292754BA3174472DD7DF91B8C047FFE98B5D3247792DD7F 9B1CDF3C
ip4.loopy.limon.baker.org. 1800	IN	SSHFP	1 1 629D772D9EA6EB31B9E3C96D3982E02E6D5986E2
ip4.loopy.limon.baker.org. 1800	IN	PTR	loopy.limon.baker.org.

;; Query time: 80 msec
;; SERVER: 8.8.8.8#53(8.8.8.8)
;; WHEN: Tue Sep 28 08:49:55 CDT 2021
;; MSG SIZE  rcvd: 264

[tsellers-r7]

CC @fcelda since you opened the original PR to add APL support and might have an opinion on this.

[miekg]

can't really tell if this is correct or not, but looks good to me

[fcelda]

Thank you for detailed description, @tsellers-r7.

Sections 4.1 and 4.2 of RFC 3123 is very clear that the trailing zero bytes of the address must not be included:

Trailing zero octets do not bear any information (e.g., there is no semantic difference between 10.0.0.0/16 and 10/16) in an address prefix, so the shortest possible AFDLENGTH can be used to encode it. (...) Therefore the sender MUST NOT include trailing zero octets in the AFDPART regardless of the value of PREFIX. This includes cases in which AFDLENGTH times 8 results in a value less than PREFIX. The AFDPART is padded with zero bits to match a full octet boundary.

However, I agree with you that we may be less strict when parsing the record from wire format. So yes, your changes look good to me as well and I think this PR can be merged. 👍

As a side note, reading that paragraph from the RFC carefully again, I'm not sure the implementation for generating the wire format for APL follows that spec correctly. I think that that for 10.0.0.0/16 we currently generate two-byte address ([]byte{10, 0}) while we probably should include just the first byte ([]byte{10}).

[tsellers-r7]

What are the next steps on this PR? Are there some changes you'd like to see?

[fcelda]

I suggest we merge it as it is.