Skip to content

fix: count CNAME queries in dry-run estimates - #224

Closed
jiangyuer018 wants to merge 1 commit into
TMHSDigital:mainfrom
jiangyuer018:fix/223-dry-run-cname-query-cost
Closed

jiangyuer018 wants to merge 1 commit into
TMHSDigital:mainfrom
jiangyuer018:fix/223-dry-run-cname-query-cost

Conversation

@jiangyuer018

Copy link
Copy Markdown
Contributor

Description

Fixes #223.

-dry-run now uses the same per-record-type wire-query costs as the resolver. CNAME estimates include the A, AAAA and CNAME queries, so retry and duration estimates use the correct base count.

Related Issue

Fixes #223

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvements
  • Test updates

Checklist

  • I have read the CONTRIBUTING document
  • My code follows the code style of this project
  • I have added/updated appropriate comments to the code
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes (not run: Go 1.26 is unavailable in this environment)
  • I have updated the documentation accordingly (if applicable)

Ethical Considerations

  • This change maintains or improves the security controls in the tool
  • This change follows the educational/legitimate security testing focus of the project
  • I have considered potential misuse cases and taken appropriate steps to prevent them

Additional Information

Added unit coverage for the shared DNS query-cost helper and a simulated dry-run regression covering A, AAAA, CNAME, retries and rate-based duration. git diff --check passes. Local go test -race ./... and make lint could not run because Go/Make are unavailable; this is a draft so CI can verify the change before review.

@TMHSDigital TMHSDigital left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is the right fix. QueriesPerName sums the same queriesPerType table that ResolveTypes reserves rate slots from, and ParseTypes already normalizes and dedupes -type, so the estimate now matches the wire. I ran go test ./... and golangci-lint run ./... on 217ea1f locally: all green, 0 lint issues. I've approved the CI runs.

One thing before this is ready: the old "one query per record type" wording is still in three places.

  • main.go:242: the -max-queries help text. docs/_includes/flags.md is generated from it (and main_docs_test.go checks the two match), so after editing main.go, regenerate it with the Makefile target (see Makefile:44) rather than editing it by hand.
  • README.md:437: "each entry costs one query per record type (two with the default A,AAAA)" needs the CNAME exception too. Without it, the second acceptance criterion on #223 is only half met.

Optional nit: the "same types as the resolver receives" case in TestQueriesPerName passes CNAME twice, which ParseTypes never produces. Either drop it or rename it to say what it pins down (duplicates are not deduped).

Once those land, mark it ready and I'll merge.

@TMHSDigital

Copy link
Copy Markdown
Owner

Thanks for this, and sorry for the churn: a docs pass on main (060e257, #229) also fixed the README -max-queries row, so this PR now conflicts on that one line. main reads:

| `-max-queries <n>` | `0` | Max candidate names to test (0 = unlimited). Each name sends one query per record type per attempt (`CNAME` costs three) |

Your wording is fine too. Keep whichever you prefer when you rebase on main. The Go changes don't conflict, and the checks were green before.

TMHSDigital added a commit that referenced this pull request Nov 14, 2026
The -max-queries help text (and the flag table generated from it) and
the README's wordlist-size note still said one query per record type.
A CNAME lookup sends three, as -dry-run and -rate now count. Wording
from #224.

Refs #223

Co-authored-by: jiangyuer018 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
@TMHSDigital

Copy link
Copy Markdown
Owner

Thank you for this, @jiangyuer018. Your approach was right, and it's now on main, so I'm closing this PR as superseded instead of asking you to rebase again:

  • fd5c6e2 makes -dry-run use the same per-type query cost the rate limiters use. It calls dns.QueriesFor, a helper that a related rate-cap fix (Per-resolver rate cap under-reserves: default types and CNAME exceed it #236) had already exported, which is the same idea as your QueriesPerName. TestDryRunCountsCNAMEQueries pins the count for A,AAAA,CNAME.
  • 2c771ba fixes the wording points from the review: the -max-queries help text, the generated flag table, and the README note on wordlist size. It uses your CNAME wording and lists you as co-author.

Sorry for the churn while this was open, and thanks again for the contribution.

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.

-dry-run counts one query per CNAME lookup; the scan sends three

2 participants