Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor tunnel state machine DNS configuration #6783

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

dlon
Copy link
Member

@dlon dlon commented Sep 10, 2024

This PR makes the tunnel state machine DNS configuration more flexible:

enum DnsConfig {
    /// Use tunnel config gateway addresses
    Default,
    /// Use the specified addresses for DNS resolution
    Override {
        /// Addresses to configure on the tunnel interface
        tunnel_config: Vec<IpAddr>,
        /// Addresses to allow on non-tunnel interfaces
        non_tunnel_config: Vec<IpAddr>,
    },
}

Previously, the configuration consisted simply of a list of IPs (Option<Vec<IpAddr>>), and whether an IP was configured for the tunnel device or not was decided based on whether IPs were private/public.

Motivation: This makes it possible to ensure that certain private IPs, like IPv6 content blockers, are only allowed on the tunnel interface, without making the talpid universe aware of these IP ranges.


This change is Reviewable

@dlon dlon force-pushed the refactor-tunnel-dns-config branch from 470ab57 to c4ab446 Compare September 16, 2024 08:05
@dlon dlon marked this pull request as ready for review September 16, 2024 08:09
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 23 of 23 files at r1, 11 of 11 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dlon)


mullvad-daemon/src/dns.rs line 17 at r1 (raw file):

/// Return the resolvers as a vector of `IpAddr`s. Returns `None` when no special resolvers
/// are requested and the tunnel default gateway should be used.

This comment is outdated

Code quote:

/// Return the resolvers as a vector of `IpAddr`s. Returns `None` when no special resolvers
/// are requested and the tunnel default gateway should be used.

mullvad-daemon/src/dns.rs line 108 at r1 (raw file):

    }

    // Public IPs should be tunneled, private IPs should not be, except gateway?

Question mark?

Code quote:

// Public IPs should be tunneled, private IPs should not be, except gateway?

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

        v.extend(self.non_tunnel_config);
        v
    }

Nit This could easily be generalized and hide inner representation of how addresses are stored in ResolvedDnsConfig

    /// Consume `self` and return an iterator of all addresses
    pub fn addresses(self) -> impl Iterator<Item = IpAddr> {
        let mut v = self.tunnel_config;
        v.extend(self.non_tunnel_config);
        v.into_iter()
    }

Code quote:

    /// Consume `self` and return a vector of all addresses
    pub fn addresses(self) -> Vec<IpAddr> {
        let mut v = self.tunnel_config;
        v.extend(self.non_tunnel_config);
        v
    }

Copy link
Member Author

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 19 of 23 files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)


mullvad-daemon/src/dns.rs line 17 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

This comment is outdated

Done.

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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

@dlon dlon force-pushed the refactor-tunnel-dns-config branch from 92f7fd4 to aa871d5 Compare September 18, 2024 09:00
@dlon dlon force-pushed the refactor-tunnel-dns-config branch from aa871d5 to beb0637 Compare September 18, 2024 09:01
@dlon dlon merged commit 5ff831f into main Sep 18, 2024
47 checks passed
@dlon dlon deleted the refactor-tunnel-dns-config branch September 18, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants