fix: reliably check DNSSEC AD flag with dedicated resolver (micro-fix)#7139
fix: reliably check DNSSEC AD flag with dedicated resolver (micro-fix)#7139omichauhan-lgtm wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughDNSSEC validation now uses an isolated resolver with explicit public nameservers and EDNS (DO bit), queries the SOA record, and determines DNSSEC enabled only if the SOA response includes the Authenticated Data (AD) flag; function signature/invocation updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Scanner
participant IsolatedResolver
participant PublicNameserver
participant DNSZone
Scanner->>IsolatedResolver: build resolver (nameservers + EDNS DO)
IsolatedResolver->>PublicNameserver: DNS query: SOA for domain
PublicNameserver->>DNSZone: resolve SOA (validate chain)
DNSZone-->>PublicNameserver: SOA response (may include AD)
PublicNameserver-->>IsolatedResolver: return response
IsolatedResolver-->>Scanner: response with flags (check AD)
Scanner->>Scanner: set enabled=true if AD else enabled=false + issue
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tools/src/aden_tools/tools/dns_security_scanner/dns_security_scanner.py (1)
201-223: Minor cleanups: unusedresolverarg and duplicateexceptarms.
- The
resolverparameter is no longer used inside_check_dnssec. Either consume it (e.g., as the timeout source / fallback per the comment above) or drop it from the signature for clarity.- Lines 220–223 split
NoAnswerfrom(NXDOMAIN, DNSException)but both arms dopass; collapse into a singleexceptclause.♻️ Proposed cleanup
- except dns.resolver.NoAnswer: - pass - except (dns.resolver.NXDOMAIN, dns.exception.DNSException): - pass + except (dns.resolver.NoAnswer, dns.resolver.NXDOMAIN, dns.exception.DNSException): + pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/src/aden_tools/tools/dns_security_scanner/dns_security_scanner.py` around lines 201 - 223, The _check_dnssec function currently ignores its resolver parameter and has two separate except blocks that both pass; either use the passed-in resolver (e.g., use resolver.timeout, resolver.nameservers as a fallback, or call resolver.resolve when appropriate) or remove the resolver argument from the function signature to avoid confusion, and collapse the exception handling into a single except that catches dns.resolver.NoAnswer, dns.resolver.NXDOMAIN, and dns.exception.DNSException (or a common base) and handles them consistently (currently a pass) so the function is concise and the intent is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/src/aden_tools/tools/dns_security_scanner/dns_security_scanner.py`:
- Around line 207-217: The current check in dns_security_scanner.py that
inspects answers.response.flags & dns.flags.AD on sec_resolver (which uses plain
UDP nameservers ['8.8.8.8','1.1.1.1']) is only a best-effort signal and not
cryptographic proof; update the dnssec_enabled() implementation and its
docstring to (1) clearly state that the AD flag over plain UDP is not
channel-authenticated and is only advisory, and (2) provide two stronger
options: a) perform DNS-over-TLS/HTTPS queries using dns.query.tls or
dns.query.https (or dns.nameserver.DoTNameserver/DoHNameserver) to ensure
channel integrity, or b) perform local DNSSEC validation of RRsets via
dns.dnssec.validate and document that full chain-of-trust requires managing
trust anchors; modify the code paths around sec_resolver.resolve(domain, "SOA")
and the AD-flag check to either use DoT/DoH resolver or call dns.dnssec.validate
on received RRsets when opting for local validation, and ensure the docstring
describes the trade-offs and recommended default.
- Around line 204-214: The code creates a new sec_resolver and hardcodes public
nameservers without honoring the caller's timeout/lifetime and without a
fallback, causing false negatives; update dns_security_scanner.py to (1) set
sec_resolver.timeout and sec_resolver.lifetime from the passed-in resolver (or
accept caller-configured values) before calling sec_resolver.resolve(domain,
"SOA"), (2) avoid blindly overwriting the provided resolver—use the passed-in
resolver as the primary and only create sec_resolver when needed, (3) implement
a fallback attempt to the system/default resolver if the public validators
(sec_resolver.nameservers = ['8.8.8.8','1.1.1.1']) time out or raise
dns.exception.DNSException, and (4) treat inability to reach any validating
resolver as a distinct error/issue (e.g., return or raise a specific
"validation_unavailable" result) instead of marking DNSSEC as disabled; also
remove the redundant except dns.resolver.NoAnswer handler since NoAnswer is
under dns.exception.DNSException.
---
Nitpick comments:
In `@tools/src/aden_tools/tools/dns_security_scanner/dns_security_scanner.py`:
- Around line 201-223: The _check_dnssec function currently ignores its resolver
parameter and has two separate except blocks that both pass; either use the
passed-in resolver (e.g., use resolver.timeout, resolver.nameservers as a
fallback, or call resolver.resolve when appropriate) or remove the resolver
argument from the function signature to avoid confusion, and collapse the
exception handling into a single except that catches dns.resolver.NoAnswer,
dns.resolver.NXDOMAIN, and dns.exception.DNSException (or a common base) and
handles them consistently (currently a pass) so the function is concise and the
intent is clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb87686c-84e2-4c63-974a-5cbc75166795
📒 Files selected for processing (1)
tools/src/aden_tools/tools/dns_security_scanner/dns_security_scanner.py
| # Create an isolated resolver to avoid polluting the default one | ||
| sec_resolver = dns.resolver.Resolver() | ||
|
|
||
| # Use known validating nameservers (Google, Cloudflare) | ||
| sec_resolver.nameservers = ['8.8.8.8', '1.1.1.1'] | ||
|
|
||
| # Use EDNS to request DNSSEC validation (DO flag) and increase packet size | ||
| sec_resolver.use_edns(0, dns.flags.DO, 4096) | ||
|
|
||
| # Query for SOA records (mandatory for all domains) | ||
| answers = sec_resolver.resolve(domain, "SOA") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
dnspython Resolver default timeout and lifetime values
💡 Result:
The dnspython dns.resolver.Resolver class has default values of timeout=2.0 seconds (time to wait for a response from a server) and lifetime=5.0 seconds (total time to spend trying to get an answer to the question).
Citations:
- 1: https://github.com/rthalley/dnspython/blob/master/dns/resolver.py
- 2: https://github.com/rthalley/dnspython/blob/master/doc/resolver-class.rst
🏁 Script executed:
head -230 tools/src/aden_tools/tools/dns_security_scanner/dns_security_scanner.py | tail -35Repository: aden-hive/hive
Length of output: 1301
🏁 Script executed:
python3 << 'EOF'
# Check if dns.resolver.NoAnswer is a subclass of dns.exception.DNSException
# by inspecting dnspython's exception hierarchy
import dns.resolver
import dns.exception
# Check NoAnswer's MRO (method resolution order)
print("dns.resolver.NoAnswer MRO:")
print(dns.resolver.NoAnswer.__mro__)
print()
# Check if NoAnswer is a subclass of DNSException
is_subclass = issubclass(dns.resolver.NoAnswer, dns.exception.DNSException)
print(f"Is NoAnswer a subclass of DNSException? {is_subclass}")
print()
# Check other exceptions
print("dns.resolver.NXDOMAIN MRO:")
print(dns.resolver.NXDOMAIN.__mro__)
EOFRepository: aden-hive/hive
Length of output: 174
🏁 Script executed:
# Find requirements or dependencies file
find . -name "requirements*.txt" -o -name "pyproject.toml" -o -name "setup.py" | head -5Repository: aden-hive/hive
Length of output: 185
🏁 Script executed:
# Check the main pyproject.toml and tools pyproject.toml for dnspython version
grep -i "dnspython" ./pyproject.toml ./tools/pyproject.toml ./tools/src/pyproject.toml 2>/dev/nullRepository: aden-hive/hive
Length of output: 105
🌐 Web query:
dnspython 2.4.0 exception hierarchy NoAnswer NXDOMAIN DNSException
💡 Result:
In dnspython 2.4.0, all exceptions subclass dns.exception.DNSException, the abstract base class for dnspython exceptions. dns.resolver.NoAnswer and dns.resolver.NXDOMAIN are both subclasses of DNSException, defined in the dns.resolver module. NoAnswer is raised when the DNS response indicates the query name exists but has no RRset of the requested type/class (if raise_on_no_answer=True). NXDOMAIN is raised when the query name does not exist. This hierarchy has been consistent since at least version 1.14.0 and remains the same in later versions like 2.7+ where source code explicitly shows class NoAnswer(dns.exception.DNSException):.
Citations:
- 1: https://dnspython.readthedocs.io/en/latest/exceptions.html
- 2: https://www.dnspython.org/docs/1.16.0/dns.resolver.NoAnswer-class.html
- 3: https://www.dnspython.org/docs/1.16.0/dns.exception.DNSException-class.html
- 4: https://www.dnspython.org/docs/1.14.0/dns.exception.DNSException-class.html
- 5: https://www.dnspython.org/docs/1.14.0/class-tree.html
- 6: https://dnspython.readthedocs.io/en/2.7/_modules/dns/resolver.html
- 7: https://dnspython.readthedocs.io/en/2.7/resolver-class.html
- 8: https://github.com/rthalley/dnspython/blob/master/dns/resolver.py
Hardcoded public resolvers + missing timeout can re-introduce false negatives.
In egress-restricted environments (corporate networks, air-gapped hosts, sandboxes) outbound UDP/53 to 8.8.8.8/1.1.1.1 is frequently blocked. When that happens, the SOA query will hit dnspython's default lifetime (~5s) and raise a Timeout/LifetimeTimeout, which is swallowed by the DNSException handler and reported as enabled: False — the same false-negative class issue #5112 set out to fix, just for a different population of users.
Two concrete gaps:
sec_resolver.timeout/sec_resolver.lifetimeare not set, so this check no longer respects the caller-configured timeout onresolver.- There is no fallback path when both public validators are unreachable; the scanner cannot distinguish "DNSSEC not enabled" from "could not reach a validating resolver".
Consider: (1) propagating the caller's timeout/lifetime, (2) attempting the system resolver as a fallback (it's already configured by the caller and may itself be a validating resolver), and (3) surfacing a distinct issue when validation could not be performed at all so the risk grader doesn't penalize unrelated network failures.
Also minor: the resolver parameter is passed but never used (a new sec_resolver is created instead), and the first except dns.resolver.NoAnswer block is redundant since NoAnswer is a subclass of dns.exception.DNSException.
🛠️ Suggested adjustment
- # Create an isolated resolver to avoid polluting the default one
- sec_resolver = dns.resolver.Resolver()
-
- # Use known validating nameservers (Google, Cloudflare)
- sec_resolver.nameservers = ['8.8.8.8', '1.1.1.1']
-
- # Use EDNS to request DNSSEC validation (DO flag) and increase packet size
- sec_resolver.use_edns(0, dns.flags.DO, 4096)
-
- # Query for SOA records (mandatory for all domains)
- answers = sec_resolver.resolve(domain, "SOA")
-
- # Verify the Authenticated Data (AD) flag is present
- if answers.response.flags & dns.flags.AD:
- return {"enabled": True, "issues": []}
+ # Isolated resolver pinned to known validating nameservers, with the
+ # caller's timeout budget preserved.
+ sec_resolver = dns.resolver.Resolver()
+ sec_resolver.nameservers = ["8.8.8.8", "1.1.1.1"]
+ sec_resolver.timeout = resolver.timeout
+ sec_resolver.lifetime = resolver.lifetime
+ sec_resolver.use_edns(0, dns.flags.DO, 4096)
+
+ answers = sec_resolver.resolve(domain, "SOA")
+ if answers.response.flags & dns.flags.AD:
+ return {"enabled": True, "issues": []}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/src/aden_tools/tools/dns_security_scanner/dns_security_scanner.py`
around lines 204 - 214, The code creates a new sec_resolver and hardcodes public
nameservers without honoring the caller's timeout/lifetime and without a
fallback, causing false negatives; update dns_security_scanner.py to (1) set
sec_resolver.timeout and sec_resolver.lifetime from the passed-in resolver (or
accept caller-configured values) before calling sec_resolver.resolve(domain,
"SOA"), (2) avoid blindly overwriting the provided resolver—use the passed-in
resolver as the primary and only create sec_resolver when needed, (3) implement
a fallback attempt to the system/default resolver if the public validators
(sec_resolver.nameservers = ['8.8.8.8','1.1.1.1']) time out or raise
dns.exception.DNSException, and (4) treat inability to reach any validating
resolver as a distinct error/issue (e.g., return or raise a specific
"validation_unavailable" result) instead of marking DNSSEC as disabled; also
remove the redundant except dns.resolver.NoAnswer handler since NoAnswer is
under dns.exception.DNSException.
| # Use known validating nameservers (Google, Cloudflare) | ||
| sec_resolver.nameservers = ['8.8.8.8', '1.1.1.1'] | ||
|
|
||
| # Use EDNS to request DNSSEC validation (DO flag) and increase packet size | ||
| sec_resolver.use_edns(0, dns.flags.DO, 4096) | ||
|
|
||
| # Query for SOA records (mandatory for all domains) | ||
| answers = sec_resolver.resolve(domain, "SOA") | ||
|
|
||
| # Verify the Authenticated Data (AD) flag is present | ||
| if answers.response.flags & dns.flags.AD: |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
dnspython AD flag DNSSEC validation trust channel DoT DoH
💡 Result:
dnspython provides DNSSEC signature validation for individual RRsets via dns.dnssec.validate and validate_rrsig, requiring the cryptography library (pip install dnspython[dnssec]). This validates RRSIGs against DNSKEYs but does not perform full chain-of-trust validation from root trust anchors; users must fetch and provide keys manually or implement recursive validation. The stub resolver (dns.resolver.Resolver) supports DNSSEC-aware queries by setting use_edns(0, dns.flags.DO) to request DNSSEC data and checking response.flags & dns.flags.AD to verify if the upstream resolver performed validation (AD flag indicates authenticated data). Example from Mailu project: import dns.resolver import dns.flags resolver = dns.resolver.Resolver resolver.use_edns(0, dns.flags.DO, 1232) resolver.nameservers = ['8.8.8.8'] answer = resolver.resolve('example.com', 'A') if answer.response.flags & dns.flags.AD: print("Upstream validated DNSSEC") The resolver does not perform validation itself; AD flag presence confirms the recursive resolver (e.g., 8.8.8.8) did. dnspython supports secure channels: DNS-over-TLS (DoT) via dns.query.tls or dns.nameserver.DoTNameserver, DNS-over-HTTPS (DoH) via dns.query.https or dns.nameserver.DoHNameserver (requires pip install dnspython[doh] for requests/httpx). Resolver can use these by setting nameservers to DoT/DoH objects (recent versions). These protect query privacy/integrity; DNSSEC validation occurs at the upstream resolver if supported. To check DNSSEC protection programmatically: Query with DO bit to authoritative NS, validate DNSKEY self-signature using dns.dnssec.validate, and check AD from validating resolver. No built-in full trust chain validation; see open issue #414.
Citations:
- 1: https://dnspython.readthedocs.io/en/latest/dnssec.html
- 2: DNSSEC - Is the AD Flag in start.py (Admin) the right thing to do? Mailu/Mailu#2619
- 3: https://github.com/rthalley/dnspython/blob/master/dns/dnssec.py
- 4: https://groups.google.com/g/dnspython-users/c/U8YR2QHLsG8
- 5: https://dnspython.readthedocs.io/en/2.7/resolver-nameserver.html
- 6: https://stackoverflow.com/questions/26137036/programmatically-check-if-domains-are-dnssec-protected
- 7: https://www.dnspython.org/docs/1.16.0/dns.resolver.Resolver-class.html
- 8: https://github.com/rthalley/dnspython/blob/master/dns/resolver.py
- 9: https://dnspython.readthedocs.io/en/2.7/resolver-functions.html
Use DoT/DoH or local validation for cryptographic DNSSEC assurance, not the AD flag over plain UDP.
The AD flag from an upstream resolver (8.8.8.8/1.1.1.1) only indicates the resolver performed DNSSEC validation; it provides no protection against on-path modification of that flag. Plain UDP/53 cannot guarantee channel integrity, making this check best-effort rather than cryptographic proof.
For stronger validation:
- Use DNS-over-TLS (
dns.query.tlsordns.nameserver.DoTNameserver) or DNS-over-HTTPS (dns.query.httpsordns.nameserver.DoHNameserver) for channel security - Perform local DNSSEC validation using
dns.dnssec.validateon responses; however, this validates individual RRsets but does not automatically perform full chain-of-trust validation from root trust anchors
At minimum, document in the dnssec_enabled() docstring that the AD flag check is only a best-effort signal and does not provide cryptographic assurance without a trusted channel.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/src/aden_tools/tools/dns_security_scanner/dns_security_scanner.py`
around lines 207 - 217, The current check in dns_security_scanner.py that
inspects answers.response.flags & dns.flags.AD on sec_resolver (which uses plain
UDP nameservers ['8.8.8.8','1.1.1.1']) is only a best-effort signal and not
cryptographic proof; update the dnssec_enabled() implementation and its
docstring to (1) clearly state that the AD flag over plain UDP is not
channel-authenticated and is only advisory, and (2) provide two stronger
options: a) perform DNS-over-TLS/HTTPS queries using dns.query.tls or
dns.query.https (or dns.nameserver.DoTNameserver/DoHNameserver) to ensure
channel integrity, or b) perform local DNSSEC validation of RRsets via
dns.dnssec.validate and document that full chain-of-trust requires managing
trust anchors; modify the code paths around sec_resolver.resolve(domain, "SOA")
and the AD-flag check to either use DoT/DoH resolver or call dns.dnssec.validate
on received RRsets when opting for local validation, and ensure the docstring
describes the trade-offs and recommended default.
|
Thanks for the review @coderabbitai! I've removed the unused |
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/src/aden_tools/tools/dns_security_scanner/dns_security_scanner.py (1)
220-220: Remove redundant exception classes from except tuple.Both
dns.resolver.NoAnsweranddns.resolver.NXDOMAINare subclasses ofdns.exception.DNSException. This pattern appears 7 times throughout the file (lines 130, 171, 192, 220, 236, 245, 253) and can be simplified to catch justdns.exception.DNSException.♻️ Optional cleanup
- except (dns.resolver.NoAnswer, dns.resolver.NXDOMAIN, dns.exception.DNSException): + except dns.exception.DNSException: pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/src/aden_tools/tools/dns_security_scanner/dns_security_scanner.py` at line 220, Replace the redundant exception tuple "except (dns.resolver.NoAnswer, dns.resolver.NXDOMAIN, dns.exception.DNSException):" with a single "except dns.exception.DNSException:" wherever it appears (the occurrences that reference dns.resolver.NoAnswer and dns.resolver.NXDOMAIN alongside dns.exception.DNSException); this removes duplicate subclass catches and leaves the broad DNSException catch in functions/blocks that currently use that except clause.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/src/aden_tools/tools/dns_security_scanner/dns_security_scanner.py`:
- Line 220: Replace the redundant exception tuple "except
(dns.resolver.NoAnswer, dns.resolver.NXDOMAIN, dns.exception.DNSException):"
with a single "except dns.exception.DNSException:" wherever it appears (the
occurrences that reference dns.resolver.NoAnswer and dns.resolver.NXDOMAIN
alongside dns.exception.DNSException); this removes duplicate subclass catches
and leaves the broad DNSException catch in functions/blocks that currently use
that except clause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f14e14d-3012-4afd-afc8-4ad1ab970a59
📒 Files selected for processing (1)
tools/src/aden_tools/tools/dns_security_scanner/dns_security_scanner.py
1 participant
Fixes #5112.
The _check_dnssec\ tool was returning false positives because the system resolver doesn't always validate DNSSEC, or strips the AD flag. This PR ensures we:
Summary by CodeRabbit