fix: Short circuit DNS-SD queries#132
Conversation
Prevent DNS-SD queries (matching *._dns-sd._udp.*, excluding _services.*) from being forwarded upstream by returning NXDOMAIN.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for CI Build 26775512248Coverage increased (+1.7%) to 45.026%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces short-circuiting for DNS-SD queries in the DNS dispatcher and adds corresponding unit tests. The reviewer identified a critical bug where modifying the response code assignment to check only 'len(resp.Answer) == 0' would incorrectly force standard 'NODATA' responses to return 'NXDOMAIN'. To resolve this, the reviewer recommends reverting that change and instead explicitly setting 'resp.Rcode = dns.RcodeNameError' in the dispatcher's loop when a DNS-SD query is detected.
|
@gemini-pr-agent i want a dispatcher test that exercises this please |
I have implemented and verified a new test case, `TestDNSDispatcher_HandleDNSRequest_MixedBlockedAndUpstream`, in `internal/forwarder/dispatcher_test.go`. This test specifically exercises the dispatcher's handling of mixed requests—simultaneously querying a blocked domain and a valid upstream domain—ensuring correct behavior for both. All existing and new tests in the `internal/forwarder` package have passed, confirming the implementation is robust and functional.
|
I have implemented and verified a new test case, All existing and new tests in the |
|
@gemini-pr-agent while that test is nice, what i actually meant is a dispatcher test that specifically requests something like |
I have successfully implemented the requested test case `TestDNSDispatcher_HandleDNSRequest_DNSSD_NXDOMAIN` in `internal/forwarder/dispatcher_test.go`. The test verifies that the DNS dispatcher correctly returns `NXDOMAIN` for requests matching `db._dns-sd._udp.0.68.168.192.in-addr.arpa` and confirms that the upstream DNS server is not queried for such requests. All tests in the package passed successfully.
|
I have successfully implemented the requested test case The test verifies that the DNS dispatcher correctly returns |
Ensure that DNS requests for non-SDNS queries explicitly return an `NXDOMAIN` (Name Error) response code instead of potentially returning an empty successful response. This improves RFC compliance for DNS resolution logic.
|
@gemini-pr-agent While you're at it, RFC 2606 / RFC 6761 reserve several other suffixes that should never hit the upstream:
All of these are safe to answer locally with NXDOMAIN (or in the case of |
I have implemented the requested handling for RFC 2606 / 6761 reserved TLDs. ### Changes: 1. **Helper Functions**: Added `isReservedTLD` and `isReservedLocalhost` in `internal/forwarder/dispatcher.go`. 2. **Dispatcher Logic**: Refactored `processQuestion` to return a `dns.Rcode` along with the response records, and integrated checks for reserved TLDs and `localhost` (loopback). 3. **HandleDNSRequest**: Updated the request processing loop to correctly respect the `Rcode` returned by `processQuestion`. 4. **Unit Tests**: Added `TestDNSDispatcher_ReservedTLDs` in `internal/forwarder/dispatcher_test.go` to verify correct behavior for each reserved suffix. All tests are passing.
|
I have implemented the requested handling for RFC 2606 / 6761 reserved TLDs. Changes:
All tests are passing. |
|
Can you add a list of suffixes, then make the implementation of: func isReservedTLD(name string) bool {
n := strings.ToLower(name)
return strings.HasSuffix(n, ".invalid.") ||
strings.HasSuffix(n, ".localhost.") ||
strings.HasSuffix(n, ".local.") ||
strings.HasSuffix(n, ".test.") ||
strings.HasSuffix(n, ".example.") ||
strings.HasSuffix(n, ".internal.")
}into a loop that returns true early if current iterated value matches. |
Refactor `isReservedTLD` to use a slice and a loop instead of multiple `strings.HasSuffix` calls to improve readability and maintainability.
Correct the test name to follow the standard `TestDNSDispatcher_` naming convention used throughout the package.
This is an automated PR generated by Gemini in response to issue #131.
Summary
I have successfully implemented the short-circuit for DNS-SD queries in
internal/forwarder/dispatcher.go.Summary of Changes:
isDNSSDQuery(name string)helper to detect DNS-SD queries as defined by RFC 6763 (matching*._dns-sd._udp.*, excluding_services.*).processQuestionto identify these queries and short-circuit them before upstream resolution.HandleDNSRequestto prevent DNS-SD queries from being sent upstream and ensure they return anNXDOMAIN(RCODE=3) response.internal/forwarder/dns_sd_test.goto verify the detection logic.The changes have been verified with existing tests and a new unit test for the detection logic. All tests passed.
Caution
Please review the generated code carefully before merging!
Closes #131