Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop handling a domain if all nameservers don't provide sufficient glue records when they should #417

Merged
merged 6 commits into from
Aug 13, 2024

Conversation

phillip-stephens
Copy link
Contributor

Description

Wanted to get a specific review on this since it seems to core logic that affects more than just IPv6 lookups.

The issue that caused me to look into this was trying to get A records for esrg.stanford.edu from Stanford.edu nameservers over IPv6

$ dig -t A esrg.stanford.edu @avallone.stanford.edu -6

; <<>> DiG 9.10.6 <<>> -t A esrg.stanford.edu @avallone.stanford.edu -6
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 20243
;; flags: qr rd; QUERY: 1, ANSWER: 0, AUTHORITY: 2, ADDITIONAL: 3
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 1232
;; QUESTION SECTION:
;esrg.stanford.edu.		IN	A

;; AUTHORITY SECTION:
esrg.stanford.edu.	1800	IN	NS	dns-02.esrg.stanford.edu.
esrg.stanford.edu.	1800	IN	NS	dns-01.esrg.stanford.edu.

;; ADDITIONAL SECTION:
dns-02.esrg.stanford.edu. 1800	IN	A	171.67.68.26
dns-01.esrg.stanford.edu. 1800	IN	A	171.67.68.25

;; Query time: 84 msec
;; SERVER: 2620:6c:40c0:0:204:63:224:53#53(2620:6c:40c0:0:204:63:224:53)
;; WHEN: Mon Aug 12 12:17:07 EDT 2024
;; MSG SIZE  rcvd: 120

We can't proceed to look up dns-01.esrg.stanford.edu any further, and without an AAAA record to continue IPv6 iteration, we cannot proceed.

Solution

  • Add a new NONEEDEDGLUE status that is returned if a given name is beneath the current layer (esrg.stanford.edu is beneath Stanford.edu and the glue doesn't contain a next hop.)
  • If all authorities in iterateOnAuthorities return this, we'll terminate the iteration

Testing

esrg.stanford.edu lookup

ipv6 base branch

$ echo "esrg.stanford.edu" | ./zdns A --6 --iterative
{"data":{"protocol":"","resolver":""},"duration":15.001968875,"name":"esrg.stanford.edu","status":"TIMEOUT","timestamp":"2024-08-12T12:22:51-04:00"}

ipv6-recursive-base

$ echo "esrg.stanford.edu" | ./zdns A --6 --iterative
{"data":{"protocol":"","resolver":""},"duration":0.21923125,"name":"esrg.stanford.edu","status":"NONEEDEDGLUE","timestamp":"2024-08-12T12:23:26-04:00"}

Ensuring this doesn't break known working domains

main

Benchmarking ZDNS, Resolving 2000 domains... 100% 
Benchmark took:                                                       23.12s
Min resolution time:                                                 27.35µs
Max resolution time:                                                  15.00s
Average resolution time:                                            744.78ms

Domains resolved successfully:                                     1830/2000
Domains that timed out:                                                  152
Domains that failed:                                                      18

ipv6-recursive-base

~/zdns on  phillip/ipv6-recursive-base! ⌚ 16:24:40
$ make benchmark
go build -o zdns
cd ./benchmark && go run main.go stats.go
Benchmarking ZDNS, Resolving 2000 domains... 100% 
Benchmark took:                                                       23.98s
Min resolution time:                                                 28.17µs
Max resolution time:                                                  15.00s

Domains resolved successfully:                                     1803/2000
Domains that timed out:                                                  180
Domains that failed:                                                      17

@phillip-stephens phillip-stephens marked this pull request as ready for review August 12, 2024 17:39
@phillip-stephens phillip-stephens requested a review from a team as a code owner August 12, 2024 17:39
@zakird zakird merged commit c4ca392 into phillip/ipv6 Aug 13, 2024
3 checks passed
@zakird zakird deleted the phillip/ipv6-recursive-base branch August 13, 2024 19:15
zakird pushed a commit that referenced this pull request Aug 14, 2024
* handled ipv6 nses in the ResolverConfig

* added v6 local addrs to config

* added most basic IPv6 NS test

* added comments explaining we'll mix IPv4 and IPv6 addresses in the CLI side and distinguish later

* CLI side done

* init LocalAddr arrays in new RC and return error if ipv6only and we can't get an IPv6 address

* compiles with IPv6 support

* remove ipv6 todo

* fix bug in parsing IPv6 resolv.conf

* populate root servers with IPv6 if applicable

* fixed population bug with ipv6 roots

* fixed seg fault with a guard condition, need to fix root cause tho

* added thread-saftey for PopulateAndValidate and fixed some lack of IPv6 support in authority/additional iteration

* added a few sanity tests for IPv6

* fixed bug by not deleting unneeded local addrs and nameservers

* fixed executable name for testing

* loopback handling in ExternalLoopback

* fixed compile issues in tests

* fixed bug with proper IPv6 detection and make populateAndValidate idempotent

* fixed issue with copying ns arrays in resolver init

* added new --4 and --6 flags to disambiguate from the lookup A and AAAA flags

* fix up unit tests, messed them up by misunderstanding the lookup-ipv4 flag

* fix up IPv6 test so it doesn't run on non-IPv6 supported hosts

* forgot to rename the ZDNS exe back

* moved ipv6 tests into their own workflow

* renamed zdns exe

* cleaned up ipv6 tests

* added loopback test for ipv6

* elevate warning msg about not being able to find IPv6 socket to warning or else it's hard to identify

* add fix for using root servers if in iterative mode in CLI

* added details to loopback warning msg

* lookup AAAA for nameservers in extract authority

* added prefer ipv4 and ipv6 options

* better handling around the ipv4 preference validation, better UX, more tests

* lint

* hack to get IPv6 test to pass on hosts that don't support IPv6

* a lil more hack

* added comment to explain hack in unit tests

* fixed compile issues in non-test code

* tests passing

* lint

* remove tests with loopback IPv6, we can't have IPv6 loopback NSs

* only add name servers if IP mode supports it

* cleanup and fixed some bugs from merge

* loopback cli hack and added more string sanitization on listing --name-servers

* don't overwrite cli provided external NS'

* don't overwrite cli provided external NS bug

* compiling

* fixed up tests and added null checks to ExternalLookup. Removed test trying to use IPv6 loopback nameserver, since we don't support that

* cleaned up unneeded changes in integration_tests

* spelling

* use new concat

* avoid redundent check and remove todo

* Stop handling a domain if all nameservers don't provide sufficient glue records when they should (#417)

* stop handling a domain if the nameserver that should provide glue records doesn't

* add rfc comment

* lint

* updated ipv6 integration test

* update comment

* better err msg if user specifies IPv6 mode on non IPv6 capable machine

* infer IP support thru nameservers, use --4/6 as IPvX only

* disallow both --4 and --6

* tests and lints

* fixed up ipv6 tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants