Remove Apple services workarounds for unaffected macOS versions

[MarkusPettersson98]

This PR gates the NAT filter rules that we introduced in #6911 to only the affected macOS versions, since we don't need them otherwise. They were added to mitigate the problem of Apple services not working properly together with the Mullvad VPN app as reported in #6521.

This PR additionally disables our local DNS resolver by default for the same macOS versions, which has been enabled since #6890. We found an issue with some services such as Orbstack setting the pf flag skip on loopback, which would mess with our redirect of DNS to our local DNS resolver. Hopefully this can help fix #7058 (at least partially).

---

This change is 

[linear]

DES-1359 Remove macOS Apple app fix (NAT firewall rules) for macOS >=15.1

[dlon]

Reviewed 3 of 6 files at r1, 5 of 6 files at r3, all commit messages.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)

---

talpid-core/src/resolver.rs line 48 at r1 (raw file):

Not

Nit: note

It could probably be rephrased to clarify that the setting doesn't affect the error/blocked state.

---

talpid-core/src/dns/macos.rs line 81 at r1 (raw file):

    }

    /// Construct a [DnsConfig] from the arguments and apply the desired addresses to all network services.

Is DnsConfig supposed to be DnsSettings?

[dlon]

Reviewed 1 of 6 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)

[MarkusPettersson98]

Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @dlon)

---

talpid-core/src/resolver.rs line 48 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Not

Nit: note

It could probably be rephrased to clarify that the setting doesn't affect the error/blocked state.

Done

---

talpid-core/src/dns/macos.rs line 81 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Is DnsConfig supposed to be DnsSettings?

Done

[dlon]

Reviewed 6 of 6 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[dlon]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[hulthe]

Reviewed 3 of 6 files at r5, 1 of 2 files at r6.
Reviewable status: 2 of 6 files reviewed, 5 unresolved discussions (waiting on @dlon and @MarkusPettersson98)

---

talpid-core/src/tunnel_state_machine/connected_state.rs line 173 at r7 (raw file):

        // On macOS, configure only the local DNS resolver
        #[cfg(target_os = "macos")]
        // We do not want to forward DNS queries to *our* local resolver the DNS config

"if the DNS config is a loopback address"?

---

talpid-core/src/tunnel_state_machine/connected_state.rs line 182 at r7 (raw file):

                .map_err(BoxedError::new)?;
        } else {
            log::debug!("Enabling DNS forwarding");

🪒🐂 IMO the log statements shouldn't say "Enabling DNS forwarding", but instead "Enabling local DNS resolver", as forwarding queries isn't the point, it's an implementation detail.

---

talpid-core/src/tunnel_state_machine/connected_state.rs line 184 at r7 (raw file):

            log::debug!("Enabling DNS forwarding");
            let local_resolver_config = dns_config.addresses().collect();
            // Forward all DNS requests to our local DNS resolver

I think this comment is incorrect

AFAICT, enable_forward tells the local resolver to start forwarding queries to some external DNS resolver, i.e. nothing is forwarded to the local resolver.

---

talpid-core/src/tunnel_state_machine/connecting_state.rs line 56 at r7 (raw file):

    ) -> (Box<dyn TunnelState>, TunnelStateTransition) {
        #[cfg(target_os = "macos")]
        if *LOCAL_DNS_RESOLVER {

Do we not want to check dns_config.is_loopback() here as well?

---

talpid-core/src/firewall/macos.rs line 981 at r7 (raw file):

        let _ = self
            .pf
            .try_remove_anchor(ANCHOR_NAME, pfctl::AnchorKind::Nat);

Have you considered ignoring only some error kinds?

        self.pf
            .try_remove_anchor(ANCHOR_NAME, pfctl::AnchorKind::Nat)
            .or_else(|e| match e.kind() {
                pfctl::ErrorKind::AnchorDoesNotExist => Ok(()),
                _ => Err(e),
            })?;

[MarkusPettersson98]

Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on @dlon, @hulthe, and @MarkusPettersson98)

---

talpid-core/src/tunnel_state_machine/connected_state.rs line 173 at r7 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

"if the DNS config is a loopback address"?

Done.

---

talpid-core/src/tunnel_state_machine/connected_state.rs line 182 at r7 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

🪒🐂 IMO the log statements shouldn't say "Enabling DNS forwarding", but instead "Enabling local DNS resolver", as forwarding queries isn't the point, it's an implementation detail.

Done

---

talpid-core/src/tunnel_state_machine/connected_state.rs line 184 at r7 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

I think this comment is incorrect

AFAICT, enable_forward tells the local resolver to start forwarding queries to some external DNS resolver, i.e. nothing is forwarded to the local resolver.

Done

---

talpid-core/src/tunnel_state_machine/connecting_state.rs line 56 at r7 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Do we not want to check dns_config.is_loopback() here as well?

No, this was the behaviour before 2024.6, so we just revert to that if LOCAL_DNS_RESOLVER is false:)

[MarkusPettersson98]

Reviewable status: 2 of 6 files reviewed, 2 unresolved discussions (waiting on @dlon and @hulthe)

---

talpid-core/src/firewall/macos.rs line 981 at r7 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Have you considered ignoring only some error kinds?

        self.pf
            .try_remove_anchor(ANCHOR_NAME, pfctl::AnchorKind::Nat)
            .or_else(|e| match e.kind() {
                pfctl::ErrorKind::AnchorDoesNotExist => Ok(()),
                _ => Err(e),
            })?;

Turns out this was already the case with try_remove_anchor, it ignores pfctl::Error::AnchorDoesNotExist error! Added a comment to explain that we rely on this behaviour

[hulthe]

Reviewed 1 of 6 files at r5, 1 of 2 files at r6, 2 of 3 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

talpid-core/src/firewall/macos.rs line 981 at r7 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Turns out this was already the case with try_remove_anchor, it ignores pfctl::Error::AnchorDoesNotExist error! Added a comment to explain that we rely on this behaviour

5/7 perfect code

---

talpid-core/src/tunnel_state_machine/connecting_state.rs line 56 at r7 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

No, this was the behaviour before 2024.6, so we just revert to that if LOCAL_DNS_RESOLVER is false:)

coolio

[hulthe]

Reviewed 1 of 5 files at r12, 4 of 4 files at r13, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[hulthe]

Reviewed 1 of 1 files at r14, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved