Enable secure DNS in DNS forwarder#5458
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors DNS types to plain nym-dns::DnsConfig, migrates resolver forwarding to hickory NameServerConfig, adds AllowedDns for firewall endpoint classification, updates platform DNS monitors and macOS PF rules to use endpoints, and adapts the tunnel state machine and DNS handler to the new APIs. ChangesDNS Configuration Refactoring
Estimated code review effort: Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
0bc17c3 to
0333c12
Compare
d36cbd4 to
cdcd605
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nym-vpn-core/crates/nym-firewall/src/net.rs`:
- Around line 313-324: The partition currently uses endpoints.iter() producing
Vec<&Endpoint> which mismatches AllowedDns fields that expect Vec<Endpoint>;
change the iterator to consume ownership by using
endpoints.into_iter().partition(...) so partition returns owned Endpoint values,
and update the closure to accept owned Endpoint (keep the predicate
nym_firewall_config::is_local_address(&ep.address.ip())). Populate
non_tunnel_dns and tunnel_dns with those owned vectors.
In `@nym-vpn-core/crates/nym-vpn-lib/src/service/vpn_service.rs`:
- Around line 1458-1460: handle_get_default_dns currently maps
DEFAULT_DNS_SERVERS_CONFIG per-protocol entries to a Vec<IpAddr>, producing
duplicates; change handle_get_default_dns to deduplicate IPs before returning
(e.g. collect mapped IPs into a HashSet or filter while preserving order by
pushing only unseen IPs into the result Vec) so the returned Vec<IpAddr>
contains each resolver once; reference DEFAULT_DNS_SERVERS_CONFIG and the async
fn handle_get_default_dns to locate and update the logic.
In `@nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/dns_handler.rs`:
- Line 4: Run rustfmt (cargo fmt) on this file to apply canonical formatting:
let rustfmt reorder the import to what it expects (nym_dns::{DnsConfig,
DnsMonitor} or similar) and collapse the DnsHandler::set(...) call into the
single-line form rustfmt produces; after running cargo fmt for the crate, review
the changes (notably the nym_dns import and DnsHandler::set usage) and commit
them so CI formatting checks pass.
In `@nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/mod.rs`:
- Around line 230-249: tunnel_dns() currently returns duplicate resolver IPs
when DnsOptions::Default is used because DEFAULT_DNS_SERVERS_CONFIG contains one
entry per transport; update tunnel_dns (the match arm for DnsOptions::Default
and the branch that handles empty custom addrs) to produce a deduplicated,
stable list of IpAddr values (e.g., collect into a HashSet or use sort+dedup)
before applying the IPv6 filter and returning; ensure you still respect
self.enable_ipv6 and preserve the existing behavior for custom non-empty lists.
- Around line 289-297: allowed_default_dns_endpoints() currently maps
ProtocolConfig::H3 to nym_firewall::TransportProtocol::Tcp but H3 represents
DoH3 over QUIC (UDP); update the mapping inside the ns.connections.iter().map
closure (the match on conn.protocol) so that ProtocolConfig::H3 { .. } is
treated like the QUIC/UDP branch and returns
nym_firewall::TransportProtocol::Udp instead of Tcp, ensuring H3 nameservers are
allowed over UDP in the firewall allowlist.
🪄 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: 884c6a81-8ecb-4087-9312-ee2dcfa944c1
⛔ Files ignored due to path filters (1)
nym-vpn-core/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
nym-vpn-core/CHANGELOG.mdnym-vpn-core/README.mdnym-vpn-core/crates/nym-dns/src/android.rsnym-vpn-core/crates/nym-dns/src/lib.rsnym-vpn-core/crates/nym-dns/src/macos.rsnym-vpn-core/crates/nym-firewall/Cargo.tomlnym-vpn-core/crates/nym-firewall/src/lib.rsnym-vpn-core/crates/nym-firewall/src/macos.rsnym-vpn-core/crates/nym-firewall/src/net.rsnym-vpn-core/crates/nym-vpn-lib/Cargo.tomlnym-vpn-core/crates/nym-vpn-lib/src/lib.rsnym-vpn-core/crates/nym-vpn-lib/src/resolver/mod.rsnym-vpn-core/crates/nym-vpn-lib/src/service/vpn_service.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/dns_handler.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/mod.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/connected_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/connecting_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/error_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/offline_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/tunnel_monitor.rs
💤 Files with no reviewable changes (2)
- nym-vpn-core/README.md
- nym-vpn-core/crates/nym-firewall/Cargo.toml
2fb5e76 to
beab404
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nym-vpn-core/crates/nym-firewall/src/macos.rs`:
- Around line 503-521: The current loops over dns_config.tunnel_dns() and
dns_config.non_tunnel_dns() incorrectly filter endpoints to port()==53, which
removes connected-state allow rules for secure DNS on 853/443; remove the
port()==53 guard so you still call
get_allow_tunnel_dns_rules_when_connected(exit_tunnel, dns_endpoint) for every
tunnel_dns() entry and get_allow_local_dns_rules_when_connected(exit_tunnel,
dns_endpoint) for every non_tunnel_dns() entry (retain the port==53 check only
where get_block_dns_rules() requires it), ensuring encrypted upstreams still get
explicit allow rules when connected and split tunneling is enabled.
🪄 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: 98768969-f0c3-482c-979a-dbc6cdc4c217
⛔ Files ignored due to path filters (1)
nym-vpn-core/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
nym-vpn-core/CHANGELOG.mdnym-vpn-core/README.mdnym-vpn-core/crates/nym-dns/src/android.rsnym-vpn-core/crates/nym-dns/src/lib.rsnym-vpn-core/crates/nym-dns/src/macos.rsnym-vpn-core/crates/nym-firewall/Cargo.tomlnym-vpn-core/crates/nym-firewall/src/lib.rsnym-vpn-core/crates/nym-firewall/src/macos.rsnym-vpn-core/crates/nym-firewall/src/net.rsnym-vpn-core/crates/nym-vpn-lib/Cargo.tomlnym-vpn-core/crates/nym-vpn-lib/src/lib.rsnym-vpn-core/crates/nym-vpn-lib/src/resolver/mod.rsnym-vpn-core/crates/nym-vpn-lib/src/service/vpn_service.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/dns_handler.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/mod.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/connected_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/connecting_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/error_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/offline_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/tunnel_monitor.rs
💤 Files with no reviewable changes (2)
- nym-vpn-core/README.md
- nym-vpn-core/crates/nym-firewall/Cargo.toml
✅ Files skipped from review due to trivial changes (1)
- nym-vpn-core/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (13)
- nym-vpn-core/crates/nym-firewall/src/net.rs
- nym-vpn-core/crates/nym-dns/src/android.rs
- nym-vpn-core/crates/nym-vpn-lib/src/service/vpn_service.rs
- nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/dns_handler.rs
- nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/offline_state.rs
- nym-vpn-core/crates/nym-vpn-lib/src/lib.rs
- nym-vpn-core/crates/nym-firewall/src/lib.rs
- nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/error_state.rs
- nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/tunnel_monitor.rs
- nym-vpn-core/crates/nym-dns/src/lib.rs
- nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/mod.rs
- nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/connected_state.rs
- nym-vpn-core/crates/nym-vpn-lib/src/resolver/mod.rs
| // Do not create rules for any other ports but 53 because "block all dns" targets port 53 specifically | ||
| for dns_endpoint in dns_config.tunnel_dns() { | ||
| if dns_endpoint.address.port() == 53 { | ||
| rules.push(self.get_allow_tunnel_dns_rules_when_connected( | ||
| exit_tunnel, | ||
| dns_endpoint, | ||
| )?); | ||
| } | ||
| } | ||
| for server in dns_config.non_tunnel_config() { | ||
| rules.append( | ||
| &mut self.get_allow_local_dns_rules_when_connected(exit_tunnel, *server)?, | ||
| ); | ||
| for dns_endpoint in dns_config.non_tunnel_dns() { | ||
| if dns_endpoint.address.port() == 53 { | ||
| rules.append( | ||
| &mut self.get_allow_local_dns_rules_when_connected( | ||
| exit_tunnel, | ||
| dns_endpoint, | ||
| )?, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't skip connected-state rules for non-53 DNS endpoints.
Line 503 now gates both dns_config.tunnel_dns() and dns_config.non_tunnel_dns() on port() == 53. That drops the explicit allow rules for encrypted upstreams on 853/443: non-tunnel secure DNS has no later pass rule in the normal connected path, and secure tunnel DNS loses its only explicit exit-tunnel allow rule when split tunneling is enabled. get_block_dns_rules() explains why port 53 needs special handling, but not why the other DNS endpoints can be omitted entirely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nym-vpn-core/crates/nym-firewall/src/macos.rs` around lines 503 - 521, The
current loops over dns_config.tunnel_dns() and dns_config.non_tunnel_dns()
incorrectly filter endpoints to port()==53, which removes connected-state allow
rules for secure DNS on 853/443; remove the port()==53 guard so you still call
get_allow_tunnel_dns_rules_when_connected(exit_tunnel, dns_endpoint) for every
tunnel_dns() entry and get_allow_local_dns_rules_when_connected(exit_tunnel,
dns_endpoint) for every non_tunnel_dns() entry (retain the port==53 check only
where get_block_dns_rules() requires it), ensuring encrypted upstreams still get
explicit allow rules when connected and split tunneling is enabled.
There was a problem hiding this comment.
This must be fixed now
There was a problem hiding this comment.
@pronebird, here is the fix. Both port guards should be removed — the comment reasoning is actually backwards: precisely because get_block_dns_rules() only targets port 53, non-53 upstreams (DoT/853, DoH/443) are not blocked by it, but they also have no explicit pass rule anywhere else in the connected path. For non_tunnel_dns this causes those requests to fall through to the catch-all drop. For tunnel_dns the impact is worse in split-tunnel mode where the explicit tunnel DNS rule is the only pass on the tunnel interface.
// In FirewallPolicy::Connected branch of get_policy_specific_rules
// Allow tunnel DNS on any port; the block-dns rules only target port 53
// so non-53 upstreams (DoT/853, DoH/443) require explicit rules here too.
// When split tunneling is active the general tunnel-allow rule is absent,
// making these per-endpoint rules the only pass on the tunnel interface.
for dns_endpoint in dns_config.tunnel_dns() {
rules.push(self.get_allow_tunnel_dns_rules_when_connected(
exit_tunnel,
dns_endpoint,
)?);
}
// Non-tunnel DNS must also be allowed for every port: the block-dns rules
// only drop port 53, but non-tunnel traffic never passes through the tunnel
// interface allow-all rule, so port-853/443 endpoints need an explicit pass.
for dns_endpoint in dns_config.non_tunnel_dns() {
rules.append(
&mut self
.get_allow_local_dns_rules_when_connected(exit_tunnel, dns_endpoint)?,
);
}The only place the port-53 gate is legitimately needed is inside get_block_dns_rules() itself (which already hard-codes :53). The per-endpoint allow rules must be emitted for every port, especially now that this PR enables encrypted upstream DNS.
ʕ •ᴥ•ʔ on it
beab404 to
32e2bcd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/dns_handler.rs (1)
4-4:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix import ordering to pass CI.
The pipeline is failing because
cargo fmtexpects the imports in alphabetical order:{DnsConfig, DnsMonitor}instead of{DnsMonitor, DnsConfig}.Proposed fix
-use nym_dns::{DnsMonitor, DnsConfig}; +use nym_dns::{DnsConfig, DnsMonitor};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/dns_handler.rs` at line 4, The import list in dns_handler.rs is out of alphabetical order; update the use statement to import DnsConfig before DnsMonitor (i.e., change use nym_dns::{DnsMonitor, DnsConfig}; to use nym_dns::{DnsConfig, DnsMonitor};) so cargo fmt/Cargo CI ordering checks pass and references to DnsMonitor and DnsConfig remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/dns_handler.rs`:
- Line 4: The import list in dns_handler.rs is out of alphabetical order; update
the use statement to import DnsConfig before DnsMonitor (i.e., change use
nym_dns::{DnsMonitor, DnsConfig}; to use nym_dns::{DnsConfig, DnsMonitor};) so
cargo fmt/Cargo CI ordering checks pass and references to DnsMonitor and
DnsConfig remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4eeb3d0-5789-4289-b22e-fc77a8373f80
⛔ Files ignored due to path filters (1)
nym-vpn-core/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
nym-vpn-core/CHANGELOG.mdnym-vpn-core/README.mdnym-vpn-core/crates/nym-dns/src/android.rsnym-vpn-core/crates/nym-dns/src/lib.rsnym-vpn-core/crates/nym-dns/src/macos.rsnym-vpn-core/crates/nym-firewall/Cargo.tomlnym-vpn-core/crates/nym-firewall/src/lib.rsnym-vpn-core/crates/nym-firewall/src/macos.rsnym-vpn-core/crates/nym-firewall/src/net.rsnym-vpn-core/crates/nym-vpn-lib/Cargo.tomlnym-vpn-core/crates/nym-vpn-lib/src/lib.rsnym-vpn-core/crates/nym-vpn-lib/src/resolver/mod.rsnym-vpn-core/crates/nym-vpn-lib/src/service/vpn_service.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/dns_handler.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/mod.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/connected_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/connecting_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/error_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/offline_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/tunnel_monitor.rs
💤 Files with no reviewable changes (2)
- nym-vpn-core/crates/nym-firewall/Cargo.toml
- nym-vpn-core/README.md
✅ Files skipped from review due to trivial changes (1)
- nym-vpn-core/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (12)
- nym-vpn-core/crates/nym-dns/src/android.rs
- nym-vpn-core/crates/nym-vpn-lib/Cargo.toml
- nym-vpn-core/crates/nym-vpn-lib/src/service/vpn_service.rs
- nym-vpn-core/crates/nym-firewall/src/net.rs
- nym-vpn-core/crates/nym-dns/src/macos.rs
- nym-vpn-core/crates/nym-vpn-lib/src/lib.rs
- nym-vpn-core/crates/nym-firewall/src/macos.rs
- nym-vpn-core/crates/nym-firewall/src/lib.rs
- nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/error_state.rs
- nym-vpn-core/crates/nym-vpn-lib/src/resolver/mod.rs
- nym-vpn-core/crates/nym-dns/src/lib.rs
- nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/connecting_state.rs
32e2bcd to
52a3a5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/mod.rs`:
- Around line 232-259: The defaults closure and the subsequent handling of
self.dns can produce duplicate IPs; ensure the returned addrs vector contains
unique resolver IPs before filtering by IPv4/IPv6. Modify the defaults() closure
(or deduplicate right after building addrs) to remove duplicate IpAddr entries
while preserving order (e.g., track seen IPs with a HashSet and push only first
occurrences), then continue with addrs.retain(...) as before; update references
to defaults, DnsOptions::Default, DnsOptions::Custom, and the addrs variable
accordingly.
🪄 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: 1737a449-9d5f-46f6-9eb5-ec08dff29cd7
⛔ Files ignored due to path filters (1)
nym-vpn-core/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
nym-vpn-core/CHANGELOG.mdnym-vpn-core/README.mdnym-vpn-core/crates/nym-dns/src/android.rsnym-vpn-core/crates/nym-dns/src/lib.rsnym-vpn-core/crates/nym-dns/src/macos.rsnym-vpn-core/crates/nym-firewall/Cargo.tomlnym-vpn-core/crates/nym-firewall/src/lib.rsnym-vpn-core/crates/nym-firewall/src/macos.rsnym-vpn-core/crates/nym-firewall/src/net.rsnym-vpn-core/crates/nym-vpn-lib/Cargo.tomlnym-vpn-core/crates/nym-vpn-lib/src/lib.rsnym-vpn-core/crates/nym-vpn-lib/src/resolver/mod.rsnym-vpn-core/crates/nym-vpn-lib/src/service/vpn_service.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/dns_handler.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/mod.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/connected_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/connecting_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/error_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/offline_state.rsnym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/tunnel_monitor.rs
💤 Files with no reviewable changes (2)
- nym-vpn-core/README.md
- nym-vpn-core/crates/nym-firewall/Cargo.toml
✅ Files skipped from review due to trivial changes (1)
- nym-vpn-core/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (14)
- nym-vpn-core/crates/nym-vpn-lib/src/service/vpn_service.rs
- nym-vpn-core/crates/nym-vpn-lib/Cargo.toml
- nym-vpn-core/crates/nym-firewall/src/net.rs
- nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/error_state.rs
- nym-vpn-core/crates/nym-dns/src/macos.rs
- nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/connected_state.rs
- nym-vpn-core/crates/nym-dns/src/android.rs
- nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/dns_handler.rs
- nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/offline_state.rs
- nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/tunnel_monitor.rs
- nym-vpn-core/crates/nym-vpn-lib/src/tunnel_state_machine/states/connecting_state.rs
- nym-vpn-core/crates/nym-dns/src/lib.rs
- nym-vpn-core/crates/nym-firewall/src/macos.rs
- nym-vpn-core/crates/nym-vpn-lib/src/resolver/mod.rs
743f362 to
f668933
Compare
f668933 to
4854b8f
Compare
|



Ticket
JIRA-NYM-1198
Description
TBD
Checklist:
Screenshots (optional, if UI related)
This change is
Summary by CodeRabbit
New Features
Documentation
Refactor