starred/hickory-dns

Fork 0

mirror of https://github.com/hickory-dns/hickory-dns.git synced 2026-04-25 11:15:54 +03:00

[GH-ISSUE #2428] On 0.25.0-alpha.2, setting validate=true does not validate DNSSEC #987

New issue

Closed

opened 2026-03-16 01:10:49 +03:00 by kerem · 12 comments

kerem commented

2026-03-16 01:10:49 +03:00

Owner

Originally created by @msrd0 on GitHub (Sep 5, 2024).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/2428

Describe the bug
DNSSEC isn't checked properly.

To Reproduce
Start hickory-dns with the following config:

listen_port = 53
listen_addrs_ipv4 = ["127.0.0.1"]
listen_addrs_ipv6 = ["::1"]

[[zones]]
zone = "."
zone_type = "Forward"

[zones.stores]
type = "forward"

[zones.stores.options]
validate = true

[[zones.stores.name_servers]]
socket_addr = "1.1.1.1:853"
protocol = "tls"
tls_dns_name = "cloudflare-dns.com"
trust_nx_responses = false

[[zones.stores.name_servers]]
socket_addr = "1.0.0.1:853"
protocol = "tls"
tls_dns_name = "cloudflare-dns.com"
trust_nx_responses = false

# dns101.comcast.net, for querying dnssec-failed.org
[[zones.stores.name_servers]]
socket_addr = "69.252.250.103:53"
protocol = "udp"
trust_nx_responses = false

Then, query dnssec-failed.org and observe that a record is returned. Note that cloudflare (1.1.1.1) does DNSSEC validation, so I added a nameserver from the whois response to be sure that hickory actually receives a record to validate. https://dnsviz.net/d/dnssec-failed.org/dnssec/ shows that the domain has a bogus dnssec record.

Expected behavior
I expect to (a) not receive any records, and (b) expect to receive some error indication like a SERVFAIL response.

System:

OS: I use arch btw
Architecture: x86_64
Version: Linux 6.10.7-arch1-1
rustc version: 1.80.1 (376290515 2024-07-16)

Version:
Crate: hickory-dns binary
Version: 0.25.0-alpha.2

Originally created by @msrd0 on GitHub (Sep 5, 2024). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/2428 **Describe the bug** DNSSEC isn't checked properly. **To Reproduce** Start hickory-dns with the following config: ```toml listen_port = 53 listen_addrs_ipv4 = ["127.0.0.1"] listen_addrs_ipv6 = ["::1"] [[zones]] zone = "." zone_type = "Forward" [zones.stores] type = "forward" [zones.stores.options] validate = true [[zones.stores.name_servers]] socket_addr = "1.1.1.1:853" protocol = "tls" tls_dns_name = "cloudflare-dns.com" trust_nx_responses = false [[zones.stores.name_servers]] socket_addr = "1.0.0.1:853" protocol = "tls" tls_dns_name = "cloudflare-dns.com" trust_nx_responses = false # dns101.comcast.net, for querying dnssec-failed.org [[zones.stores.name_servers]] socket_addr = "69.252.250.103:53" protocol = "udp" trust_nx_responses = false ``` Then, query `dnssec-failed.org` and observe that a record is returned. Note that cloudflare (1.1.1.1) does DNSSEC validation, so I added a nameserver from the whois response to be sure that hickory actually receives a record to validate. https://dnsviz.net/d/dnssec-failed.org/dnssec/ shows that the domain has a bogus dnssec record. **Expected behavior** I expect to (a) not receive any records, and (b) expect to receive some error indication like a SERVFAIL response. **System:** - OS: I use arch btw - Architecture: x86_64 - Version: Linux 6.10.7-arch1-1 - rustc version: 1.80.1 (376290515 2024-07-16) **Version:** Crate: hickory-dns binary Version: 0.25.0-alpha.2

kerem

2026-03-16 01:10:49 +03:00

closed this issue
added the
bug

trust

ops
labels

kerem commented

2026-03-16 01:11:05 +03:00

Author

Owner

@msrd0 commented on GitHub (Nov 25, 2024):

This problem is still present in 0.25.0-alpha.3

@msrd0 commented on GitHub (Nov 25, 2024): This problem is still present in 0.25.0-alpha.3

kerem commented

2026-03-16 01:11:10 +03:00

Author

Owner

@bluejekyll commented on GitHub (Mar 2, 2025):

@divergentdave do you know if this is still an issue?

@bluejekyll commented on GitHub (Mar 2, 2025): @divergentdave do you know if this is still an issue?

kerem commented

2026-03-16 01:11:15 +03:00

Author

Owner

@divergentdave commented on GitHub (Mar 2, 2025):

I changed zone_type = "Forward" zone_type = "External", and trust_nx_responses to trust_negative_responses, and I can reproduce this. (with cargo run --features dnssec-aws-lc-rs,tls-aws-lc-rs --package hickory-dns -- -c configs/issue_2428.toml -p 12345 -d and cargo run --features dnssec-aws-lc-rs,tls-aws-lc-rs --package hickory-dns -- -c configs/issue_2428.toml -p 12345 -d) See the tail end of the logs below. Validation is happening, but the validation error apparently isn't getting translated into a SERVFAIL response. Note that the stub resolver has not received the same level of attention lately as the recursive resolver. (Also, while this config works for demonstrating this issue, note that combining authoritative name servers and recursive resolvers in the list of upstreams for a type = "forward" stub resolver will lead to strange results.)

1740942326:DEBUG:hickory_proto::dnssec::dnssec_dns_handle:301:validating message_response: 0, with 2 trust_anchors
1740942326:DEBUG:hickory_proto::dnssec::dnssec_dns_handle:417:failed to verify: dnssec-failed.org. record_type: A: rrsigs were not able to be verified: dnssec-failed.org., type: A
1740942326:DEBUG:hickory_proto::error:494:response: ; header 41181:RESPONSE:RD,AA:NoError:QUERY:2/0/1
; edns version: 0 dnssec_ok: true z_flags: 0 max_payload: 4096 opts: 0
; query
;; dnssec-failed.org. IN A
; answers 2
dnssec-failed.org. 300 IN A 96.99.227.255
dnssec-failed.org. 300 IN RRSIG A RSASHA1 2 300 1741963881 1740494781 44973 dnssec-failed.org. 5rFBB4naA3X2q1EXKuX+7U5TINhA3toviJkgpiZA2nnQmOhPpWrZ+9i73B4vPxAZ3mnvCAD4Ftpy6v83rOr5g5uALLdkQ4shgMuO88M2Vw2abzzbEf5ZLgWXHwfUe7xC3PPI3hbbO0HAsVhirbJhueOaaPxGZagZ+/BaGd+dQ/I=
; nameservers 0
; additionals 1

1740942326:DEBUG:hickory_server::server::response_handler:107:response: 27907 response_code: No Error

@divergentdave commented on GitHub (Mar 2, 2025): I changed `zone_type = "Forward"` `zone_type = "External"`, and `trust_nx_responses` to `trust_negative_responses`, and I can reproduce this. (with `cargo run --features dnssec-aws-lc-rs,tls-aws-lc-rs --package hickory-dns -- -c configs/issue_2428.toml -p 12345 -d` and `cargo run --features dnssec-aws-lc-rs,tls-aws-lc-rs --package hickory-dns -- -c configs/issue_2428.toml -p 12345 -d`) See the tail end of the logs below. Validation is happening, but the validation error apparently isn't getting translated into a SERVFAIL response. Note that the stub resolver has not received the same level of attention lately as the recursive resolver. (Also, while this config works for demonstrating this issue, note that combining authoritative name servers and recursive resolvers in the list of upstreams for a `type = "forward"` stub resolver will lead to strange results.) ``` 1740942326:DEBUG:hickory_proto::dnssec::dnssec_dns_handle:301:validating message_response: 0, with 2 trust_anchors 1740942326:DEBUG:hickory_proto::dnssec::dnssec_dns_handle:417:failed to verify: dnssec-failed.org. record_type: A: rrsigs were not able to be verified: dnssec-failed.org., type: A 1740942326:DEBUG:hickory_proto::error:494:response: ; header 41181:RESPONSE:RD,AA:NoError:QUERY:2/0/1 ; edns version: 0 dnssec_ok: true z_flags: 0 max_payload: 4096 opts: 0 ; query ;; dnssec-failed.org. IN A ; answers 2 dnssec-failed.org. 300 IN A 96.99.227.255 dnssec-failed.org. 300 IN RRSIG A RSASHA1 2 300 1741963881 1740494781 44973 dnssec-failed.org. 5rFBB4naA3X2q1EXKuX+7U5TINhA3toviJkgpiZA2nnQmOhPpWrZ+9i73B4vPxAZ3mnvCAD4Ftpy6v83rOr5g5uALLdkQ4shgMuO88M2Vw2abzzbEf5ZLgWXHwfUe7xC3PPI3hbbO0HAsVhirbJhueOaaPxGZagZ+/BaGd+dQ/I= ; nameservers 0 ; additionals 1 1740942326:DEBUG:hickory_server::server::response_handler:107:response: 27907 response_code: No Error ```

kerem commented

2026-03-16 01:11:20 +03:00

Author

Owner

@bluejekyll commented on GitHub (Mar 2, 2025):

thanks for the quick response.

@bluejekyll commented on GitHub (Mar 2, 2025): thanks for the quick response.

kerem commented

2026-03-16 01:11:26 +03:00

Author

Owner

@divergentdave commented on GitHub (Mar 4, 2025):

I opened PR #2830 to add a test for this. Fixing it will be involved, because there are a lot of little pieces missing.

The impl Authority for ForwardAuthority block does not override can_validate_dnssec(), so this inherits the default implementation that always returns false. I think this should be returning self.resolver.options().validate.
build_forwarded_response() has been written to work with the recursive resolver, but it is also used with the forward authority. Some implicit assumptions that are true of lookups returned by the recursor are no longer true in this case, where lookups are constructed from responses right off the network.
EmptyLookup may be used in some cases, such as when there is no NS or SOA record in the response. (I initially forgot to add an SOA record to the zone in my test, and then kept it that way when I noticed this code path) The impl LookupObject for EmptyLookup block does not override the dnssec_summary() method, and thus inherits the default implementation that always returns Insecure. This effectively defeats the prior negative response validation.
Generally build_forwarded_response() has a lot of edge cases, and we should make sure all code paths through it are sound with respect to DNSSEC. If the requirements for validating resolvers and validating stub resolvers are too different, we may want to revisit using the same function for both use cases.

@divergentdave commented on GitHub (Mar 4, 2025): I opened PR #2830 to add a test for this. Fixing it will be involved, because there are a lot of little pieces missing. * The `impl Authority for ForwardAuthority` block does not override `can_validate_dnssec()`, so this inherits the default implementation that always returns false. I think this should be returning `self.resolver.options().validate`. * `build_forwarded_response()` has been written to work with the recursive resolver, but it is also used with the forward authority. Some implicit assumptions that are true of lookups returned by the recursor are no longer true in this case, where lookups are constructed from responses right off the network. * `EmptyLookup` may be used in some cases, such as when there is no NS or SOA record in the response. (I initially forgot to add an SOA record to the zone in my test, and then kept it that way when I noticed this code path) The `impl LookupObject for EmptyLookup` block does not override the `dnssec_summary()` method, and thus inherits the default implementation that always returns `Insecure`. This effectively defeats the prior negative response validation. * Generally `build_forwarded_response()` has a lot of edge cases, and we should make sure all code paths through it are sound with respect to DNSSEC. If the requirements for validating resolvers and validating stub resolvers are too different, we may want to revisit using the same function for both use cases.

kerem commented

2026-03-16 01:11:31 +03:00

Author

Owner

@djc commented on GitHub (Mar 4, 2025):

Given this, I don't think we should block the release on this. @bluejekyll?

@djc commented on GitHub (Mar 4, 2025): Given this, I don't think we should block the release on this. @bluejekyll?

kerem commented

2026-03-16 01:11:36 +03:00

Author

Owner

@msrd0 commented on GitHub (Mar 4, 2025):

If I may add my 2 cents here, I believe that a release with a non-working validate option would be a mistake. If you decide to release without working dnssec validation, please remove the validate option and only re-introduce it once validation works.

@msrd0 commented on GitHub (Mar 4, 2025): If I may add my 2 cents here, I believe that a release with a non-working `validate` option would be a mistake. If you decide to release without working dnssec validation, please remove the `validate` option and only re-introduce it once validation works.

kerem commented

2026-03-16 01:11:41 +03:00

Author

Owner

@djc commented on GitHub (Mar 4, 2025):

If this is not a regression compared to 0.24, 0.25.0 will not be worse (on this particular aspect) than the releases that are already out there. I definitely agree that we should fix this -- if it is not a regression I don't think we should hold back all the other improvements we made to DNSSEC (and everything else) in the past year.

@djc commented on GitHub (Mar 4, 2025): If this is not a regression compared to 0.24, 0.25.0 will not be worse (on this particular aspect) than the releases that are already out there. I definitely agree that we should fix this -- if it is not a regression I don't think we should hold back all the other improvements we made to DNSSEC (and everything else) in the past year.

kerem commented

2026-03-16 01:11:46 +03:00

Author

Owner

@msrd0 commented on GitHub (Mar 4, 2025):

If this is not a regression compared to 0.24

It is. 0.24 blocked all records without valid dnssec, including those with no dnssec at all (which I don't believe is indended behaviour, see #2429). In the 0.25 alpha versions I tested the option appears as a complete no-op.

@msrd0 commented on GitHub (Mar 4, 2025): > If this is not a regression compared to 0.24 It is. 0.24 blocked all records without valid dnssec, including those with no dnssec at all (which I don't believe is indended behaviour, see #2429). In the 0.25 alpha versions I tested the option appears as a complete no-op.

kerem commented

2026-03-16 01:11:51 +03:00

Author

Owner

@bluejekyll commented on GitHub (Mar 5, 2025):

If I have a moment I'd like to see not if I can get this working to at least match the old behavior, if not the correct current behavior.

@bluejekyll commented on GitHub (Mar 5, 2025): If I have a moment I'd like to see not if I can get this working to at least match the old behavior, if not the correct current behavior.

kerem commented

2026-03-16 01:11:56 +03:00

Author

Owner

@divergentdave commented on GitHub (Mar 12, 2025):

Here's a CFG of the existing build_forwarded_response() function.

Some more observations:

We should be using RCODE=REFUSED in the case where RD=0, rather than returning an empty response with NOERROR.
We can only transmit a Proof::Bogus to the DnssecSummary logic if we have a record to hang the proof off of, or a ProtoErrorKind::Nsec. Thus, we should make sure we test behavior when the response from the upstream resolver has empty sections. (though this might be covered by existing logic turning an empty response into an error).
AuthLookup::dnssec_summary() returns a fallback of DnssecSummary::Insecure if there are no records. This feels risky, compared to using Bogus.

@divergentdave commented on GitHub (Mar 12, 2025): Here's a CFG of the existing `build_forwarded_response()` function. ![Image](https://github.com/user-attachments/assets/8a2c765f-751b-4ca9-9da6-d74b069fa7c3) Some more observations: * We should be using `RCODE=REFUSED` in the case where `RD=0`, rather than returning an empty response with `NOERROR`. * We can only transmit a `Proof::Bogus` to the `DnssecSummary` logic if we have a record to hang the proof off of, or a `ProtoErrorKind::Nsec`. Thus, we should make sure we test behavior when the response from the upstream resolver has empty sections. (though this might be covered by existing logic turning an empty response into an error). * `AuthLookup::dnssec_summary()` returns a fallback of `DnssecSummary::Insecure` if there are no records. This feels risky, compared to using `Bogus`.

kerem commented

2026-03-16 01:12:01 +03:00

Author

Owner

@msrd0 commented on GitHub (Mar 14, 2025):

Thanks for fixing this 😻

@msrd0 commented on GitHub (Mar 14, 2025): Thanks for fixing this 😻

kerem referenced this issue

2026-03-16 02:28:55 +03:00

[PR #987] [MERGED] Mark socket2 as optional, it does not work on the wasm32-unknown-unknown platform. #1839