Use encrypted dns proxy in `mullvad-ios` #6846

MarkusPettersson98 · 2024-09-23T14:31:51Z

This change is

pinkisemils · 2024-09-24T14:07:57Z

We believe this is finally ready for review. With these changes, I am introducing an FFI to be used by the iOS codebase. It also adds a kind of a retry strategy. This was a joint effort between @MarkusPettersson98 and me.

Whilst in the weeds, we found that the timeout in the resolver config for hickory-resolver isn't actually taken into account - we should probably report it as an error to them.

faern

As discussed in the office, please see this document for the latest spec. I believe the only difference from the code you propose here, is how we pick XOR over plain: https://mullvad.atlassian.net/wiki/spaces/CLIEN/pages/3736502288/Encrypted+DNS+proxy

Reviewable status: 0 of 9 files reviewed, all discussions resolved

faern

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

mullvad-encrypted-dns-proxy/src/config/mod.rs line 79 at r1 (raw file):

    pub obfuscation: Option<ObfuscationConfig>,

    pub r#type: ProxyType,

As discussed in the office: Remove this field and instead derive proxy type (to prefer XOR over plain) via the obfuscation field above instead.

dlon

Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)

mullvad-ios/src/encrypted_dns_proxy.rs line 154 at r2 (raw file):

#[no_mangle]
pub unsafe extern "C" fn encrytped_dns_proxy_free(ptr: *mut EncryptedDnsProxyState) {
    mem::forget(unsafe { Box::from_raw(ptr) })

Should forget be called here?

pinkisemils

Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on @dlon and @MarkusPettersson98)

mullvad-ios/src/encrypted_dns_proxy.rs line 154 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should forget be called here?

No, we should not. This was most certainly not my best judgement call.

MarkusPettersson98

Reviewed 5 of 9 files at r1, 3 of 4 files at r2, all commit messages.
Reviewable status: 0 of 9 files reviewed, 6 unresolved discussions (waiting on @dlon and @faern)

mullvad-encrypted-dns-proxy/src/config/mod.rs line 79 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

As discussed in the office: Remove this field and instead derive proxy type (to prefer XOR over plain) via the obfuscation field above instead.

Done

mullvad-ios/src/encrypted_dns_proxy.rs line 113 at r3 (raw file):

        self.tried_configurations.insert(config.clone());
        Some(config)
    }

Nit What do you think about appying some DRY? Nice to not need the explicit early return in the middle of the function imho.

    fn next_configuration(&mut self) -> Option<ProxyConfig> {
        let mut difference = self.configurations.difference(&self.tried_configurations);
        let config = difference
            // prefer obfuscated proxy configurations.
            .find(|config| config.obfuscation.is_some())
            .or_else(|| difference.next())
            .cloned()?;
        self.tried_configurations.insert(config.clone());
        Some(config)
    }

Code quote:

    fn next_configuration(&mut self) -> Option<ProxyConfig> {
        if let Some(xor_config) = self
            .configurations
            .difference(&self.tried_configurations)
            // prefer obfuscated proxy configurations.
            .find(|config| config.obfuscation.is_some())
            .cloned()
        {
            self.tried_configurations.insert(xor_config.clone());
            return Some(xor_config);
        }

        let config = self
            .configurations
            .difference(&self.tried_configurations)
            .next()
            .cloned()?;
        self.tried_configurations.insert(config.clone());
        Some(config)
    }

mullvad-ios/src/encrypted_dns_proxy.rs line 159 at r3 (raw file):

/// `proxy_handle` will only contain valid values if the return value is zero. It is still valid to
/// deallocate the memory.
///

Nit: Trailing blank line

Code quote:

/// `proxy_handle` will only contain valid values if the return value is zero. It is still valid to
/// deallocate the memory.
///

mullvad-ios/src/encrypted_dns_proxy.rs line 170 at r3 (raw file):

            log::error!("Cannot instantiate a tokio runtime: {}", err);
            return -1;
        }

Do we want to promote -1 error code to a variant of the Error enum defined in this module? Currently, just looking at the conversion from Error to an error code / i32 implies that there is a variant missing

impl From<Error> for i32 {
    fn from(err: Error) -> Self {
        match err {
            Error::BindLocalSocket(_) => -2,
            Error::GetBindAddr(_) => -3,
            Error::Forwarder(_) => -4,
            Error::FetchConfig(_) => -5,
            Error::NoConfigs => -6,
        }
    }
}

Let's say we add Error::Runtime, we could get rid of the 'magic' error value from the encrypted_dns_proxy_start function

#[derive(Debug)]
pub enum Error {
    /// Failed to initialize the tokio runtime.
    Runtime,
    /// Failed to bind a local listening socket, the one that will be forwarded through the proxy
    BindLocalSocket(io::Error),
    /// Failed to get local listening address of the local listening socket.
    GetBindAddr(io::Error),
    /// Failed to initialize forwarder.
    Forwarder(io::Error),
    /// Failed to fetch new.
    FetchConfig(config_resolver::Error),
    /// Failed to initialize with a valid configuration.
    NoConfigs,
}

impl From<Error> for i32 {
    fn from(err: Error) -> Self {
        match err {
            Error::Runtime => -1,
            Error::BindLocalSocket(_) => -2,
            Error::GetBindAddr(_) => -3,
            Error::Forwarder(_) => -4,
            Error::FetchConfig(_) => -5,
            Error::NoConfigs => -6,
        }
    }
}

...

pub unsafe extern "C" fn encrypted_dns_proxy_start(
    encrypted_dns_proxy: *mut EncryptedDnsProxyState,
    proxy_handle: *mut ProxyHandle,
) -> i32 {
    let handle = match crate::mullvad_ios_runtime() {
        Ok(handle) => handle,
        Err(err) => {
            log::error!("Cannot instantiate a tokio runtime: {}", err);
            return Error::Runtime.into();
        }
    };
...

Code quote:

        Err(err) => {
            log::error!("Cannot instantiate a tokio runtime: {}", err);
            return -1;
        }

mullvad-ios/src/encrypted_dns_proxy.rs line 188 at r3 (raw file):

}

/// SAFETY:

Nit:

/// # Safety

Code quote:

/// SAFETY:

faern

Reviewable status: 0 of 9 files reviewed, 6 unresolved discussions (waiting on @dlon and @MarkusPettersson98)

mullvad-encrypted-dns-proxy/src/config/mod.rs line 40 at r3 (raw file):

/// Type of a proxy configuration. Derived from the 2nd hextet of an IPv6 address in network byte
/// order. E.g. an IPv6 address such as `7f7f:2323::`  would have a proxy type value of `0x2323`.
#[derive(PartialEq, Debug, Eq, Hash, Clone)]

These extra impls can be removed now, when we decided to not expose this type publicly. The crate does not need those impls internally.

faern

Reviewed 2 of 4 files at r2.
Reviewable status: 2 of 9 files reviewed, 6 unresolved discussions (waiting on @dlon and @MarkusPettersson98)

faern

Reviewable status: 2 of 9 files reviewed, 7 unresolved discussions (waiting on @dlon and @MarkusPettersson98)

mullvad-ios/src/encrypted_dns_proxy.rs line 16 at r3 (raw file):

    /// random configurations every time.
    configurations: HashSet<ProxyConfig>,
    tried_configurations: HashSet<ProxyConfig>,

Emils and I have touched on this topic in the office. I just want to point out that we want the exact same proxy selection algorithm on all other platforms. So if you decide to merge this into mullvad-ios now it has to be moved later. We are not going to duplicate the entire selection algorithm.

faern

Reviewable status: 2 of 9 files reviewed, 7 unresolved discussions (waiting on @dlon and @MarkusPettersson98)

mullvad-ios/src/encrypted_dns_proxy.rs line 113 at r3 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Nit What do you think about appying some DRY? Nice to not need the explicit early return in the middle of the function imho.

    fn next_configuration(&mut self) -> Option<ProxyConfig> {
        let mut difference = self.configurations.difference(&self.tried_configurations);
        let config = difference
            // prefer obfuscated proxy configurations.
            .find(|config| config.obfuscation.is_some())
            .or_else(|| difference.next())
            .cloned()?;
        self.tried_configurations.insert(config.clone());
        Some(config)
    }

Computing the difference fewer times would indeed be nice. Not because of the computational cost, but because it's currently done three times and it's a lot of code and the difference is never given a name. If it's given a name it can be easier to read, and less code as well.

If the insertion into tried_configurations can be in a single place that would also be very nice.

faern

Reviewable status: 2 of 9 files reviewed, 8 unresolved discussions (waiting on @dlon and @MarkusPettersson98)

mullvad-ios/src/encrypted_dns_proxy.rs line 65 at r3 (raw file):

        self.fetch_configs().await?;
        if self.should_reset() {
            self.reset();

IMO, the reset is part of the selection algorithm which is "owned" by the next_configuration method. I feel like it would be more logical if the reset happens there.

MarkusPettersson98

Reviewable status: 2 of 9 files reviewed, 8 unresolved discussions (waiting on @dlon and @faern)

mullvad-encrypted-dns-proxy/src/config/mod.rs line 40 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

These extra impls can be removed now, when we decided to not expose this type publicly. The crate does not need those impls internally.

Done.

mullvad-ios/src/encrypted_dns_proxy.rs line 16 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Emils and I have touched on this topic in the office. I just want to point out that we want the exact same proxy selection algorithm on all other platforms. So if you decide to merge this into mullvad-ios now it has to be moved later. We are not going to duplicate the entire selection algorithm.

Yes, that makes sense. I also think it makes sense to have it in mullvad-ios for now and move it later.

buggmagnet

Reviewed 2 of 9 files at r1, 1 of 4 files at r2.
Reviewable status: 4 of 9 files reviewed, 12 unresolved discussions (waiting on @dlon, @faern, and @MarkusPettersson98)

mullvad-encrypted-dns-proxy/src/config_resolver.rs line 65 at r3 (raw file):

pub async fn resolve_default_config() -> Result<Vec<config::ProxyConfig>, Error> {
    resolve_configs(&default_resolvers(), "frakta.eu").await

nit:
If we ever want to this be usable in staging (or other) environments, we should either let the call site (via the FFI) pass the domain to resolve, or use a configuration flag to modify this.

But I'm okay with this for a v1, just food for thought here.

mullvad-ios/Cargo.toml line 13 at r3 (raw file):

workspace = true

[target.'cfg(any(target_os = "ios", target_os = "macos"))'.dependencies]

nit:
This shouldn't be necessary right ?

mullvad-ios/src/encrypted_dns_proxy.rs line 27 at r3 (raw file):

    /// Failed to initialize forwarder.
    Forwarder(io::Error),
    /// Failed to fetch new.

Failed to fetch a new configuration I imagine ?

mullvad-ios/src/encrypted_dns_proxy.rs line 77 at r3 (raw file):

            .await
            .map_err(Error::Forwarder)?;
        let join_handle = Box::new(tokio::spawn(async move {

Will this be created on the shared tokio runtime that was gotten in encrypted_dns_proxy_start ?

buggmagnet

It would be nice to separate the FFI from the implementation from a design point of view, especially if we want to have this shared with other clients. They do not need for example, the shared rust runtime from iOS

Reviewable status: 4 of 9 files reviewed, 12 unresolved discussions (waiting on @dlon, @faern, and @MarkusPettersson98)

MarkusPettersson98

Reviewable status: 4 of 9 files reviewed, 12 unresolved discussions (waiting on @buggmagnet, @dlon, and @faern)

mullvad-ios/src/encrypted_dns_proxy.rs line 113 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Computing the difference fewer times would indeed be nice. Not because of the computational cost, but because it's currently done three times and it's a lot of code and the difference is never given a name. If it's given a name it can be easier to read, and less code as well.

If the insertion into tried_configurations can be in a single place that would also be very nice.

As Emils pointed out to me, find may exhaust the iterator if there are no obfuscator configs, thus calling next on the iterator at that point will yield None. Here is an example to prove that:

#[test]
fn test_differences() {
    use std::collections::HashSet;
    let mut set = HashSet::new();
    set.insert(1);
    set.insert(2);
    let mut other_set = HashSet::new();
    other_set.insert(1);
    let mut difference = set.difference(&other_set);

    difference.find(|x| **x == 3);
    assert_eq!(difference.next(), Some(2).as_ref())
}

It is solvable by recomputing the difference in the closure passed to or_else. But we will still potentially recompute it as we do in the current implementation, FYI

MarkusPettersson98

We will figure this out when we implement this for other clients. For now, we've decided to keep it as is for simplicity.

Reviewable status: 3 of 9 files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @faern)

mullvad-encrypted-dns-proxy/src/config_resolver.rs line 65 at r3 (raw file):

Previously, buggmagnet wrote…

nit:
If we ever want to this be usable in staging (or other) environments, we should either let the call site (via the FFI) pass the domain to resolve, or use a configuration flag to modify this.

But I'm okay with this for a v1, just food for thought here.

That's an interesting idea, but out of scope for this iteration :)

mullvad-ios/src/encrypted_dns_proxy.rs line 27 at r3 (raw file):

Previously, buggmagnet wrote…

Failed to fetch a new configuration I imagine ?

Yes!

mullvad-ios/src/encrypted_dns_proxy.rs line 65 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

IMO, the reset is part of the selection algorithm which is "owned" by the next_configuration method. I feel like it would be more logical if the reset happens there.

Correct, implemented

mullvad-ios/src/encrypted_dns_proxy.rs line 77 at r3 (raw file):

Previously, buggmagnet wrote…

Will this be created on the shared tokio runtime that was gotten in encrypted_dns_proxy_start ?

Yes

mullvad-ios/Cargo.toml line 13 at r3 (raw file):

Previously, buggmagnet wrote…

nit:
This shouldn't be necessary right ?

You're right

buggmagnet

Sounds good to me @MarkusPettersson98 👍

Reviewable status: 3 of 9 files reviewed, 5 unresolved discussions (waiting on @faern and @MarkusPettersson98)

buggmagnet

Reviewed 1 of 1 files at r4, 1 of 3 files at r6.
Reviewable status: 5 of 9 files reviewed, 5 unresolved discussions (waiting on @faern and @MarkusPettersson98)

mullvad-ios/src/encrypted_dns_proxy.rs line 195 at r6 (raw file):

/// # Safety
/// `proxy_config` must be a valid pointer to a `ProxyHandle` as initialized by

We should mention thread safety here too

buggmagnet

Reviewed all commit messages.
Reviewable status: 5 of 9 files reviewed, 5 unresolved discussions (waiting on @faern and @MarkusPettersson98)

faern · 2024-09-25T12:49:21Z

mullvad-ios/src/encrypted_dns_proxy.rs

+    async fn fetch_configs(&mut self) -> Result<(), Error> {
+        match mullvad_encrypted_dns_proxy::config_resolver::resolve_default_config().await {
+            Ok(new_configs) => {
+                self.configurations = HashSet::from_iter(new_configs.into_iter());


This part is important for the random order promises we make. If self.configurations were not re-created but rather the same HashSet was cleared and re-filled, then we would be stuck in trying proxies in the same order every time. Maybe this line should document that it's important for that reason?

faern

Reviewable status: 4 of 9 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @MarkusPettersson98)

mullvad-ios/src/encrypted_dns_proxy.rs line 113 at r3 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

As Emils pointed out to me, find may exhaust the iterator if there are no obfuscator configs, thus calling next on the iterator at that point will yield None. Here is an example to prove that:
#[test]
fn test_differences() {
    use std::collections::HashSet;
    let mut set = HashSet::new();
    set.insert(1);
    set.insert(2);
    let mut other_set = HashSet::new();
    other_set.insert(1);
    let mut difference = set.difference(&other_set);

    difference.find(|x| **x == 3);
    assert_eq!(difference.next(), Some(2).as_ref())
}
It is solvable by recomputing the difference in the closure passed to or_else. But we will still potentially recompute it as we do in the current implementation, FYI

Avoidable without using the currently proposed code style. But too many reviewers here, so I'll see myself out.

github-actions · 2024-09-25T14:13:15Z

🚨 End to end tests failed. Please check the failed workflow run.

faern · 2024-09-26T06:49:35Z

mullvad-encrypted-dns-proxy/src/config_resolver.rs

-        self.0.source()
+        match self {
+            Self::ResolutionError(ref err) => Some(err),
+            Self::Timeout(ref err) => Some(err),


We can't both return the inner error as our source and use the inner errors Display implementation up in our Display implementation. That makes the error stack have two entries that are identical. It will end up like this:

Error: Timeout reached Caused by: Timeout reached

If you use an inner error as your own message in Display then in your implementation of Error::source you need to return inner_error.source() as your source. To cut out this repetition.

MarkusPettersson98 marked this pull request as draft September 23, 2024 14:32

pinkisemils force-pushed the ios-use-encrypted-dns-proxy branch from bf15762 to 1922341 Compare September 23, 2024 14:35

buggmagnet added the iOS Issues related to iOS label Sep 23, 2024

pinkisemils requested review from Serock3 and buggmagnet September 24, 2024 14:00

MarkusPettersson98 marked this pull request as ready for review September 24, 2024 14:09

faern reviewed Sep 24, 2024

View reviewed changes

faern requested changes Sep 24, 2024

View reviewed changes

dlon reviewed Sep 24, 2024

View reviewed changes

pinkisemils force-pushed the ios-use-encrypted-dns-proxy branch from bd268b7 to 38aeb2b Compare September 24, 2024 17:45

pinkisemils reviewed Sep 24, 2024

View reviewed changes

MarkusPettersson98 commented Sep 25, 2024

View reviewed changes

faern requested changes Sep 25, 2024

View reviewed changes

faern reviewed Sep 25, 2024

View reviewed changes

faern requested changes Sep 25, 2024

View reviewed changes

faern reviewed Sep 25, 2024

View reviewed changes

MarkusPettersson98 commented Sep 25, 2024

View reviewed changes

buggmagnet requested review from dlon and faern September 25, 2024 08:07

buggmagnet reviewed Sep 25, 2024

View reviewed changes

MarkusPettersson98 commented Sep 25, 2024

View reviewed changes

buggmagnet reviewed Sep 25, 2024

View reviewed changes

pinkisemils force-pushed the ios-use-encrypted-dns-proxy branch from dfe49e8 to 196e4a1 Compare September 25, 2024 11:26

buggmagnet reviewed Sep 25, 2024

View reviewed changes

pinkisemils force-pushed the ios-use-encrypted-dns-proxy branch from 196e4a1 to 456ed2d Compare September 25, 2024 11:51

buggmagnet previously approved these changes Sep 25, 2024

View reviewed changes

pinkisemils dismissed buggmagnet’s stale review via 6c1db7c September 25, 2024 12:34

pinkisemils force-pushed the ios-use-encrypted-dns-proxy branch from 456ed2d to 6c1db7c Compare September 25, 2024 12:34

faern reviewed Sep 25, 2024

View reviewed changes

pinkisemils force-pushed the ios-use-encrypted-dns-proxy branch from 7243087 to 4c27a41 Compare September 25, 2024 12:51

faern reviewed Sep 25, 2024

View reviewed changes

faern self-requested a review September 25, 2024 13:03

Add FFI for encrypted-dns-proxy for iOS

9604d1a

pinkisemils force-pushed the ios-use-encrypted-dns-proxy branch from 5714d71 to 9604d1a Compare September 25, 2024 14:04

pinkisemils merged commit dfa53d8 into main Sep 25, 2024
46 of 47 checks passed

pinkisemils deleted the ios-use-encrypted-dns-proxy branch September 25, 2024 14:07

faern reviewed Sep 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use encrypted dns proxy in `mullvad-ios` #6846

Use encrypted dns proxy in `mullvad-ios` #6846

MarkusPettersson98 commented Sep 23, 2024 •

edited by faern

Loading

pinkisemils commented Sep 24, 2024

faern left a comment

faern left a comment

dlon left a comment

pinkisemils left a comment

MarkusPettersson98 left a comment

faern left a comment

faern left a comment

faern left a comment

faern left a comment

faern left a comment

MarkusPettersson98 left a comment

buggmagnet left a comment

buggmagnet left a comment

MarkusPettersson98 left a comment

MarkusPettersson98 left a comment

buggmagnet left a comment

buggmagnet left a comment

buggmagnet left a comment

faern Sep 25, 2024

faern left a comment

github-actions bot commented Sep 25, 2024

faern Sep 26, 2024

Use encrypted dns proxy in mullvad-ios #6846

Use encrypted dns proxy in mullvad-ios #6846

Conversation

MarkusPettersson98 commented Sep 23, 2024 • edited by faern Loading

pinkisemils commented Sep 24, 2024

faern left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

dlon left a comment

Choose a reason for hiding this comment

pinkisemils left a comment

Choose a reason for hiding this comment

MarkusPettersson98 left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

MarkusPettersson98 left a comment

Choose a reason for hiding this comment

buggmagnet left a comment

Choose a reason for hiding this comment

buggmagnet left a comment

Choose a reason for hiding this comment

MarkusPettersson98 left a comment

Choose a reason for hiding this comment

MarkusPettersson98 left a comment

Choose a reason for hiding this comment

buggmagnet left a comment

Choose a reason for hiding this comment

buggmagnet left a comment

Choose a reason for hiding this comment

buggmagnet left a comment

Choose a reason for hiding this comment

faern Sep 25, 2024

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 25, 2024

faern Sep 26, 2024

Choose a reason for hiding this comment

Use encrypted dns proxy in `mullvad-ios` #6846

Use encrypted dns proxy in `mullvad-ios` #6846

MarkusPettersson98 commented Sep 23, 2024 •

edited by faern

Loading