Proper EDNS handling and cleaner NOERROR response in DNSSERVER #11411
Conversation
|
|
👋 Hello Kolkman, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
| @@ -1,5 +1,5 @@ | |||
| name=DNSServer | |||
| version=3.2.0 | |||
| version=3.3.0 | |||
There was a problem hiding this comment.
this version is updated with every new release. Please revert
There was a problem hiding this comment.
Reverted the change with new commit
| @@ -9,6 +9,23 @@ | |||
| #define DNS_OFFSET_DOMAIN_NAME DNS_HEADER_SIZE // Offset in bytes to reach the domain name labels in the DNS message | |||
| #define DNS_DEFAULT_PORT 53 | |||
|
|
|||
| #define DNS_SOA_MNAME_LABEL "ns" | |||
| #define DNS_SOA_RNAME_LABEL "esp32" | |||
There was a problem hiding this comment.
what are MNAME and RNAME? How are their values chosen?
There was a problem hiding this comment.
MNAME and RNAME are values defined in RFC1035 section 3.1.13.
Tne MNAME is the name of the primary nameserver the request came from - that value is only used in the context of dynamic DNS updates RFC 2136. In this contexts it is purely informational. I chose the 'ns' label as that is sort of customary for nameservers.
The RNAME is used to construct the email address of the administrator for the zone. The way things are coded now that is esp32@local. There nothing in the DNS protocol that uses that value, it is only for network administrator information siganling purpose.
libraries/DNSServer/src/DNSServer.h
Outdated
| // definition of RFC1035 3.1 | ||
| // for instance, a postfix of example.com would be defined as: | ||
| // #define DNS_SOA_POSTFIX_LABEL {'\7', 'e', 'x', 'a', 'm', 'p', 'l', 'e', '\3', 'c', 'o', 'm', '\0'} | ||
| #define DNS_SOA_POSTFIX_LABEL {'\5', 'l', 'o', 'c', 'a', 'l', '\0'} |
There was a problem hiding this comment.
.local is used by MDNS. Is that the best value?
There was a problem hiding this comment.
Excellent question, because it surfaces a dilemma that I had.
Here are the considerations:
- The MNAME and RNAME are purely informational still you do not want something to be used on the real Internet.
- But, suppose that somebody queries a DNSSERVER on an ESP and decides to complain to the admin who is indicated in the RNAME. Then you don't want that mail to end up with an existing person somewhere on the Internet.
- The domains in the MNAME and RNAME could be based on the queryname, but that would be problematic for wildcard resolution, where the administrator email might become [email protected], so I figure that is a bad choice.
- One has to pick a value that is never going to appear in the real DNS. The IANA maintains a registry of those.
- I think
invalidmight be a good choice. RFC 6761 says that trying to resolveinvalidmust always expected to fail. However - folk seeing 'invalid' in their soa record during troubleshooting might misinterpret what is happening. - .local demonstrates that something local is going on - which is the most likely place where ESP32s get deployed.
As said, there is nothing in the DNS (except for dynamic updates) that will use the M- and RNAME values, hence no interference with MDNS - the values are purely used in the context of local troubleshooting.
reverting version number update - as it is done automatically
|
@Kolkman small typo reported by the CI: https://github.com/espressif/arduino-esp32/actions/runs/15443489960/job/43466846177?pr=11411#step:9:29 |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the DNSServer to properly ignore EDNS OPT records and return an RFC-compliant "No Data" (NODATA) response with an SOA record when the queried name exists but the type does not match.
- Ignore the additional section (EDNS OPT) by zeroing
ARCountso queries with OPT still match. - Branch
_handleUDPto callreplyWithNoAnswfor non-A/ANY queries, implementing NODATA per RFC2308. - Introduce SOA constants and implement
replyWithNoAnswto emit an authority SOA RR with minimal TTL.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| libraries/DNSServer/src/DNSServer.h | Added SOA‐related macros and declared replyWithNoAnsw method. |
| libraries/DNSServer/src/DNSServer.cpp | Updated _handleUDP logic, ignore EDNS in requestIncludesOnlyQuestion, implemented replyWithNoAnsw. |
Comments suppressed due to low confidence (2)
libraries/DNSServer/src/DNSServer.h:199
- [nitpick] The method name
replyWithNoAnswis abbreviated and may be unclear; consider renaming toreplyWithNoAnswerorreplyWithNoDatafor clarity.
inline void replyWithNoAnsw(AsyncUDPPacket &req, DNSHeader &dnsHeader, DNSQuestion &dnsQuestion);
libraries/DNSServer/src/DNSServer.cpp:128
- [nitpick] Add a unit or integration test to verify that queries with an OPT record (non-zero ARCount) are correctly ignored and still receive a response.
dnsHeader.ARCount = 0; // We assume that if ARCount !=0 there is a EDNS OPT packet, just ignore
| dnsHeader.ARCount = 0; // We assume that if ARCount !=0 there is a EDNS OPT packet, just ignore | ||
| return ntohs(dnsHeader.QDCount) == 1 && dnsHeader.ANCount == 0 && dnsHeader.NSCount == 0; // && dnsHeader.ARCount == 0; |
There was a problem hiding this comment.
Mutating dnsHeader.ARCount directly can lead to unexpected side effects since the header is reused for reply. Consider checking ARCount in a local variable or on a copy instead of overwriting the original field.
| dnsHeader.ARCount = 0; // We assume that if ARCount !=0 there is a EDNS OPT packet, just ignore | |
| return ntohs(dnsHeader.QDCount) == 1 && dnsHeader.ANCount == 0 && dnsHeader.NSCount == 0; // && dnsHeader.ARCount == 0; | |
| uint16_t arCount = dnsHeader.ARCount; // Store ARCount in a local variable | |
| return ntohs(dnsHeader.QDCount) == 1 && dnsHeader.ANCount == 0 && dnsHeader.NSCount == 0 && arCount == 0; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
there was a bit of dead code here (line 129), I removed it.
Co pilot seemed to be hallucinating for its other suggestions
Test Results 76 files 76 suites 12m 44s ⏱️ Results for commit 557d9c7. ♻️ This comment has been updated with latest results. |
Description of Change
Bugs
The code solves two bugs:
BUG 1: When trouble shooting a dnsserver with the 'dig' (one of the main DNS troubleshooting tools) the server doesn't answer because it doesn't handle EDNS (OPT records in the additional section).
That has been fixed, the additional section in the query can safely be ignored.
(mostly change around line 123)
BUG 2: The code also returns an A record when the QUERY Type not 'A' (or 'ANY').
This has been fixed too. For an A and ANY query type the server returns an A record.
(mostly change around line 114)
Feature
The code continues to return a 'SERVFAIL' when no match with QNAME occurs. However, with the fix of bug 2 we created a proper 'No Data' response, (RFC2308 2.2) for queries that have a match for the name, but not for the type. The function DNSServer::replyWithNoAnsw implements that functionality.
The server now behaves somewhat more conform the DNS protocl specifications.
(most of the code changes)
Tests scenarios
I tested this code on an ESP32 and WireShark as protocol analyzer - there is no reason to suspect that it does not work on other platforms (to which I have no access).
Related links