Add mullvad-encrypted-dns-proxy crate for API obfuscation

[pinkisemils]

I've added a crate that will enable us to use apisocks5 from our Rust code. It can:

• Resolve configs from arbitrary domain names
• Create proxy configurations form IPv6 addresses
• Use said proxy configurations to proxy data to our API

The rustls client configuration looks eerily similar to that of the mullvad-api's client config, but I am not sure if it should be reused. I half-expect this crate to be a dependency of mullvad-api in one way or another, but I can also remove the AsyncRead and AsyncWrite impls for the Forwarder. I also expect my crusty rust code to not be quite up to spec with the best way to write code in September of 2024, so you needn't be gentle.

Also, this is a sign to have a healthy discussion as to why this crate shouldn't be named mullvad-api-socks5-dns-obfuscated-encrypted-port-fowrard-proxy.

---

This change is 

[pinkisemils]

Should we define the default resolvers (cloudflare, cloud9, google) in this crate?
What should the default resolvers be?

[pinkisemils]

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

---

Cargo.toml line 22 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

    "mullvad-nsis",
    "mullvad-obfuscated-dns-proxy",

Done.

[Serock3]

If you bump the versions of rustls to 0.23 and webpki-roots to 0.26, this can be simplified to

let root_store = RootCertStore::from_iter(webpki_roots::TLS_SERVER_ROOTS.iter().cloned());

Note that it relies on the version of the TrustAnchor type matching for both crates, you could also leave rustls at 0.22 with the explicit conversion

let root_store = rustls::RootCertStore {
  roots: webpki_roots::TLS_SERVER_ROOTS.iter().cloned().collect(),
};

[dlon]

Reviewed 5 of 12 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 6 of 12 files reviewed, 12 unresolved discussions (waiting on @pinkisemils and @Serock3)

---

mullvad-obfuscated-dns-proxy/Cargo.lock line 1 at r2 (raw file):

# This file is automatically @generated by Cargo.

Is this supposed to have its own lock file? Is it not part of the same workspace?

---

mullvad-obfuscated-dns-proxy/src/config/plain.rs line 14 at r2 (raw file):

impl Plain {
    pub async fn forward(

Never used?

---

mullvad-obfuscated-dns-proxy/src/config/xor.rs line 31 at r2 (raw file):

        let _ = cursor.read_u16::<LittleEndian>().unwrap();
        let proxy_type = cursor.read_u16::<LittleEndian>().unwrap();
        if proxy_type != 0x03 {

Could this not be a magic constant?

---

mullvad-obfuscated-dns-proxy/src/config/xor.rs line 48 at r2 (raw file):

            .collect::<Vec<_>>();
        if xor_key.is_empty() {
            return Err(Error::EmptyXorKey);

Could "plain" not be implemented using the same code with a zeroed xor key?

---

mullvad-obfuscated-dns-proxy/src/config/xor.rs line 137 at r2 (raw file):

    xor.obfuscate(&mut obfuscated);
    dexor.obfuscate(&mut obfuscated);
    assert_eq!(input, obfuscated.as_slice());

Super-nit: This is not "obfuscated".

---

mullvad-obfuscated-dns-proxy/src/config/xor.rs line 141 at r2 (raw file):

#[test]
fn test_old_xor_addr() {

Could you rename or comment to explain the intent of this test?

---

mullvad-obfuscated-dns-proxy/src/forwarder/mod.rs line 1 at r2 (raw file):

use std::{io, task::Poll};

Module is duplicated in ./forwarder?

---

mullvad-obfuscated-dns-proxy/src/forwarder/mod.rs line 31 at r2 (raw file):

        let (client_read, client_write) = client_stream.into_split();
        let handle = tokio::spawn(async move {
            tokio::spawn(forward(self.read_obfuscator, client_read, server_write));

The double-spawn seems unnecessary. Besides, handle.await will not wait for the inner task to resolve. If that is intentional (but I'm guessing it's not), why not just not await it instead?

---

mullvad-obfuscated-dns-proxy/src/forwarder/forwarder/mod.rs line 33 at r2 (raw file):

            tokio::spawn(forward(self.read_obfuscator, client_read, server_write));
        });
        let _ = forward(self.write_obfuscator, server_read, client_write).await;

I wonder if this needs to select either task failing. If it's guaranteed to resolve, then ignore this.

---

mullvad-obfuscated-dns-proxy/src/forwarder/forwarder/mod.rs line 74 at r2 (raw file):

            }
            Poll::Pending => {
                return Poll::Pending;

Nit: Could use ready! on poll_....

---

mullvad-obfuscated-dns-proxy/src/forwarder/forwarder/mod.rs line 112 at r2 (raw file):

        let mut bytes_received = &mut buf[..n_bytes_read];

        obfuscator.obfuscate(&mut bytes_received);

Nit: It's already borrowed.

[Serock3]

Very much a style preference, but it seems you could remove the ProxyType enum entirely and use this

Suggested change

	let mut proxies = AvailableProxies {
	plain: vec![],
	xor: vec![],
	};
	for ip in ips {
	match ProxyType::try_from(ip)? {
	ProxyType::Plain => {
	proxies
	.plain
	.push(Plain::try_from(ip).map_err(Error::InvalidPlain)?);
	}
	ProxyType::XorV2 => {
	proxies
	.xor
	.push(Xor::try_from(ip).map_err(Error::InvalidXor)?);
	}
	// this type is ignored.
	ProxyType::XorV1 => continue,
	}
	}
	Ok(proxies)
	let plain = ips
	.iter()
	.filter_map(|ip| Plain::try_from(*ip).ok())
	.collect();
	let xor = ips
	.iter()
	.filter_map(|ip| Xor::try_from(*ip).ok())
	.collect();
	Ok(AvailableProxies { plain, xor })

[Serock3]

I took a look at this again, and I wonder if maybe the simplest implementation would be to also remove Plain::try_from and Xor::try_from and put all the logic in the AvailableProxies::try_from implementation like this.

const PLAIN: u16 = 0x01;
const XORV2: u16 = 0x02;
const XORV1: u16 = 0x03;

#[derive(Debug)]
pub enum Error {
    UnknownType(u16),
    InvalidXor(xor::Error),
    InvalidPlain(plain::Error),
    UnexpectedType(u16),
    EmptyXorKey,
}

impl TryFrom<Vec<Ipv6Addr>> for AvailableProxies {
    type Error = Error;

    fn try_from(ips: Vec<Ipv6Addr>) -> Result<Self, Self::Error> {
        let mut proxies = AvailableProxies {
            plain: vec![],
            xor: vec![],
        };

        for ip in ips {
            let mut data = Cursor::new(ip.octets());
            // skip the first 2 bytes since it's just padding to make the IP look more like a
            // legit IPv6 address.

            data.set_position(2);
            let proxy_type = data
                .read_u16::<LittleEndian>()
                .expect("IPv6 must have at least 16 bytes");
            match proxy_type {
                PLAIN => {
                    let mut ipv4_bytes = [0u8; 4];
                    data.read_exact(&mut ipv4_bytes).unwrap();
                    let v4_addr = Ipv4Addr::from(ipv4_bytes);

                    let port = data.read_u16::<LittleEndian>().unwrap();
                    proxies.plain.push(Plain {
                        addr: SocketAddrV4::new(v4_addr, port),
                    });
                }
                XORV2 => {
                    let mut ipv4_bytes = [0u8; 4];
                    data.read_exact(&mut ipv4_bytes).unwrap();
                    let v4_addr = Ipv4Addr::from(ipv4_bytes);

                    let port = data.read_u16::<LittleEndian>().unwrap();

                    let mut key_bytes = [0u8; 6];
                    data.read_exact(&mut key_bytes).unwrap();
                    let xor_key = key_bytes
                        .into_iter()
                        .filter(|byte| *byte != 0x00)
                        .collect::<Vec<_>>();
                    if xor_key.is_empty() {
                        return Err(Error::EmptyXorKey);
                    }
                    proxies.xor.push(Xor {
                        addr: SocketAddrV4::new(v4_addr, port),
                        xor_key,
                        key_index: 0,
                    });
                }
                // this type is ignored.
                XORV1 => continue,
                unknown => return Err(Error::UnknownType(unknown)),
            }
        }

        Ok(proxies)
    }
}

It would remove the duplicate checks of the proxy type byte and some other boilerplate.

[dlon]

Reviewed 6 of 12 files at r1.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @pinkisemils and @Serock3)

---

mullvad-obfuscated-dns-proxy/src/main.rs line 23 at r2 (raw file):

        .expect("Failed to bind listener socket");
    while let Ok((client_conn, _client_addr)) = listener.accept().await {
        let connected = crate::forwarder::Forwarder::connect(obfuscator.clone())

Nit: Unnecessary clone

[faern]

Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @pinkisemils and @Serock3)

---

mullvad-obfuscated-dns-proxy/Cargo.toml line 2 at r2 (raw file):

[package]
name = "mullvad-obfuscated-dns-proxy"

Can we please get a description here as well? We have a lot of crates and it is hard to know which one does what in certain circumstances. This is probably the least self explanatory from just the name.

---

mullvad-obfuscated-dns-proxy/Cargo.toml line 13 at r2 (raw file):

[dependencies]
tokio = { version = "1", features = [ "full" ]}

This is already available as a workspace dependency. Use that.

Can we avoid the full feature? That seems more like for development. In production we should only enable what we need.

[faern]

Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @pinkisemils and @Serock3)

---

mullvad-obfuscated-dns-proxy/src/config/mod.rs line 86 at r2 (raw file):

pub trait Obfuscator: Send {
    fn addr(&self) -> SocketAddrV4;
    fn obfuscate(&mut self, buffer: &mut [u8]);

Can we please add documentation for both these trait methods, to explain what the implementer is supposed to do.

---

mullvad-obfuscated-dns-proxy/src/config/mod.rs line 87 at r2 (raw file):

    fn addr(&self) -> SocketAddrV4;
    fn obfuscate(&mut self, buffer: &mut [u8]);
    fn clone(&self) -> Box<dyn Obfuscator>;

Seems confusing to have a custom clone method that does not come from Clone?

[faern]

The rustls client configuration looks eerily similar to that of the mullvad-api's client config, but I am not sure if it should be reused

I think they can be kept separate, to not depend on each other. Unless it's a lot of work to define them two times. Having the crates be independent has value, as they can progress without blocking each other.

[Serock3]

Why does the Forwarder use dynamic dispatch for the Obfuscator? Would it not be as simple to use generics instead?

[pinkisemils]

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

---

mullvad-obfuscated-dns-proxy/Cargo.lock line 1 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Is this supposed to have its own lock file? Is it not part of the same workspace?

Will remove, I first worked on the crate in isolation.

---

mullvad-obfuscated-dns-proxy/Cargo.toml line 13 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This is already available as a workspace dependency. Use that.

Can we avoid the full feature? That seems more like for development. In production we should only enable what we need.

Done.

---

mullvad-obfuscated-dns-proxy/src/config_resolver.rs line 90 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

If you bump the versions of rustls to 0.23 and webpki-roots to 0.26, this can be simplified to

let root_store = RootCertStore::from_iter(webpki_roots::TLS_SERVER_ROOTS.iter().cloned());

Note that it relies on the version of the TrustAnchor type matching for both crates, you could also leave rustls at 0.22 with the explicit conversion

let root_store = rustls::RootCertStore {
  roots: webpki_roots::TLS_SERVER_ROOTS.iter().cloned().collect(),
};

Yes, but hickory-dns first needs to bump it. Until they do, we can't use newer versions of rustls.

---

mullvad-obfuscated-dns-proxy/src/config/mod.rs line 35 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

        data.set_position(2);

Done.

---

mullvad-obfuscated-dns-proxy/src/config/mod.rs line 87 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Seems confusing to have a custom clone method that does not come from Clone?

I was too stupid to have a trait depend on clone and have the trait be object safe.

---

mullvad-obfuscated-dns-proxy/src/config/xor.rs line 31 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Could this not be a magic constant?

Done.

---

mullvad-obfuscated-dns-proxy/src/config/xor.rs line 48 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Could "plain" not be implemented using the same code with a zeroed xor key?

It could, but it is a distinct proxying type. Given that 2001:100::/96 will always be plain, and 2001:300::/96 will always be xor-v2.

---

mullvad-obfuscated-dns-proxy/src/forwarder/mod.rs line 34 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Perhaps a tokio::join would be more readable here

Done.

---

mullvad-obfuscated-dns-proxy/src/forwarder/mod.rs line 15 at r3 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Why does the Forwarder use dynamic dispatch for the Obfuscator? Would it not be as simple to use generics instead?

I'd argue that any user of Forwarder will then need to know and care about those types, which is not simpler. But this can be rewritten to use specific types instead. I am ambivalent as to which side we pick - I picked the more lazy option here.

---

mullvad-obfuscated-dns-proxy/src/forwarder/forwarder/mod.rs line 1 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

This file seems to be an unused duplicate of src/forwarder.mod

Done.

[dlon]

Reviewed 8 of 8 files at r3, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @faern, @pinkisemils, and @Serock3)

[faern]

Reviewable status: 11 of 12 files reviewed, 11 unresolved discussions (waiting on @dlon, @pinkisemils, and @Serock3)

a discussion (no related file):
This crate has virtually no doc comments. I'd like to request a bit more documentation what the library does.

---

[faern]

Reviewable status: 11 of 12 files reviewed, 12 unresolved discussions (waiting on @dlon, @pinkisemils, and @Serock3)

---

mullvad-obfuscated-dns-proxy/Cargo.toml line 2 at r4 (raw file):

[package]
name = "mullvad-obfuscated-dns-proxy"

Rename the crate to mullvad-encrypted-dns-proxy mayhaps? Since that's the name we finally settled on.

[faern]

Reviewable status: 11 of 12 files reviewed, 13 unresolved discussions (waiting on @dlon, @pinkisemils, and @Serock3)

---

mullvad-obfuscated-dns-proxy/src/config/mod.rs line 75 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I took a look at this again, and I wonder if maybe the simplest implementation would be to also remove Plain::try_from and Xor::try_from and put all the logic in the AvailableProxies::try_from implementation like this.

const PLAIN: u16 = 0x01;
const XORV2: u16 = 0x02;
const XORV1: u16 = 0x03;

#[derive(Debug)]
pub enum Error {
    UnknownType(u16),
    InvalidXor(xor::Error),
    InvalidPlain(plain::Error),
    UnexpectedType(u16),
    EmptyXorKey,
}

impl TryFrom<Vec<Ipv6Addr>> for AvailableProxies {
    type Error = Error;

    fn try_from(ips: Vec<Ipv6Addr>) -> Result<Self, Self::Error> {
        let mut proxies = AvailableProxies {
            plain: vec![],
            xor: vec![],
        };

        for ip in ips {
            let mut data = Cursor::new(ip.octets());
            // skip the first 2 bytes since it's just padding to make the IP look more like a
            // legit IPv6 address.

            data.set_position(2);
            let proxy_type = data
                .read_u16::<LittleEndian>()
                .expect("IPv6 must have at least 16 bytes");
            match proxy_type {
                PLAIN => {
                    let mut ipv4_bytes = [0u8; 4];
                    data.read_exact(&mut ipv4_bytes).unwrap();
                    let v4_addr = Ipv4Addr::from(ipv4_bytes);

                    let port = data.read_u16::<LittleEndian>().unwrap();
                    proxies.plain.push(Plain {
                        addr: SocketAddrV4::new(v4_addr, port),
                    });
                }
                XORV2 => {
                    let mut ipv4_bytes = [0u8; 4];
                    data.read_exact(&mut ipv4_bytes).unwrap();
                    let v4_addr = Ipv4Addr::from(ipv4_bytes);

                    let port = data.read_u16::<LittleEndian>().unwrap();

                    let mut key_bytes = [0u8; 6];
                    data.read_exact(&mut key_bytes).unwrap();
                    let xor_key = key_bytes
                        .into_iter()
                        .filter(|byte| *byte != 0x00)
                        .collect::<Vec<_>>();
                    if xor_key.is_empty() {
                        return Err(Error::EmptyXorKey);
                    }
                    proxies.xor.push(Xor {
                        addr: SocketAddrV4::new(v4_addr, port),
                        xor_key,
                        key_index: 0,
                    });
                }
                // this type is ignored.
                XORV1 => continue,
                unknown => return Err(Error::UnknownType(unknown)),
            }
        }

        Ok(proxies)
    }
}

It would remove the duplicate checks of the proxy type byte and some other boilerplate.

I like that we can avoid the extra parsing and error handling of the invalid types. A code structure that allows parsing the type once and then just instantiating the correct obfuscator instance after that would be nicer indeed.

---

mullvad-obfuscated-dns-proxy/src/config/mod.rs line 87 at r4 (raw file):

    /// Provides the endpoint for the proxy. This address must be connected and all traffic to it
    /// should first be obfuscated with `Obfuscator::obfuscate`.
    fn addr(&self) -> SocketAddrV4;

I'd argue that the address of the proxy server is unrelated to the obfuscation done on the data. You could also avoid duplicating the socket address field and related code into every implementer if it was lifted out. I imagine something like:

pub struct Proxy {
    addr: SocketAddrV4,
    obfuscator: Box<dyn Obfuscator>,
}

---

mullvad-obfuscated-dns-proxy/src/forwarder/mod.rs line 17 at r4 (raw file):

impl Forwarder {
    pub async fn connect(read_obfuscator: Box<dyn Obfuscator>) -> io::Result<Self> {

The argument name indicates the obfuscator will be used for reading. But currently it will be used for both reading and writing (it is cloned into write_obfuscator). If the current design is kept, I suggest that the argument name is changed to reflect this. So the API of Forwarder is more clear.

[Serock3]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @faern and @pinkisemils)

---

mullvad-obfuscated-dns-proxy/src/config_resolver.rs line 90 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Yes, but hickory-dns first needs to bump it. Until they do, we can't use newer versions of rustls.

So what about just bumping webpki-roots and doing the explicit conversion?

---

mullvad-obfuscated-dns-proxy/src/forwarder/mod.rs line 15 at r3 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

I'd argue that any user of Forwarder will then need to know and care about those types, which is not simpler. But this can be rewritten to use specific types instead. I am ambivalent as to which side we pick - I picked the more lazy option here.

Maybe I'm being pedantic, and I'm sure it doesn't really matter in practice, but it bothers me that the obfuscator.obfuscate(bytes_received); call in the forwarder::forward function does a v-table lookup for each message it reads. Could it not be a relatively hot part of the code?

Since the Forwarder::forward method consumes self, would it not be reasonable to combine Forwarder::connect and Forwarder::forward into a single function, and make it generic? Since also the AsyncRead and AsyncWrite implementations for the Forwarder type are unused, it seems that the entire contents of this file could be replaced with

pub async fn forward(obfuscator: impl Obfuscator, client_stream: TcpStream) -> io::Result<()> {
    let server_connection = TcpStream::connect(obfuscator.addr()).await?;
    let write_obfuscator = obfuscator.clone();
    let (server_read, server_write) = server_connection.into_split();
    let (client_read, client_write) = client_stream.into_split();
    let ((), ()) = tokio::try_join!(
        forward_inner(obfuscator, client_read, server_write),
        forward_inner(write_obfuscator, server_read, client_write)
    )?;
    Ok(())
}

async fn forward_inner(
    mut obfuscator: impl Obfuscator,
    mut source: impl AsyncRead + Unpin,
    mut sink: impl AsyncWrite + Unpin,
) -> io::Result<()> {
    use tokio::io::{AsyncReadExt, AsyncWriteExt};
    let mut buf = vec![0u8; 1024 * 64];
    while let Ok(n_bytes_read) = AsyncReadExt::read(&mut source, &mut buf).await {
        if n_bytes_read == 0 {
            break;
        }
        let bytes_received = &mut buf[..n_bytes_read];

        obfuscator.obfuscate(bytes_received);
        sink.write_all(bytes_received).await?;
    }
    Ok(())
}

(After replacing the return type of Obfuscator::clone with Self)

[Serock3]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @faern and @pinkisemils)

---

mullvad-obfuscated-dns-proxy/src/forwarder/mod.rs line 15 at r3 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Maybe I'm being pedantic, and I'm sure it doesn't really matter in practice, but it bothers me that the obfuscator.obfuscate(bytes_received); call in the forwarder::forward function does a v-table lookup for each message it reads. Could it not be a relatively hot part of the code?

Since the Forwarder::forward method consumes self, would it not be reasonable to combine Forwarder::connect and Forwarder::forward into a single function, and make it generic? Since also the AsyncRead and AsyncWrite implementations for the Forwarder type are unused, it seems that the entire contents of this file could be replaced with

pub async fn forward(obfuscator: impl Obfuscator, client_stream: TcpStream) -> io::Result<()> {
    let server_connection = TcpStream::connect(obfuscator.addr()).await?;
    let write_obfuscator = obfuscator.clone();
    let (server_read, server_write) = server_connection.into_split();
    let (client_read, client_write) = client_stream.into_split();
    let ((), ()) = tokio::try_join!(
        forward_inner(obfuscator, client_read, server_write),
        forward_inner(write_obfuscator, server_read, client_write)
    )?;
    Ok(())
}

async fn forward_inner(
    mut obfuscator: impl Obfuscator,
    mut source: impl AsyncRead + Unpin,
    mut sink: impl AsyncWrite + Unpin,
) -> io::Result<()> {
    use tokio::io::{AsyncReadExt, AsyncWriteExt};
    let mut buf = vec![0u8; 1024 * 64];
    while let Ok(n_bytes_read) = AsyncReadExt::read(&mut source, &mut buf).await {
        if n_bytes_read == 0 {
            break;
        }
        let bytes_received = &mut buf[..n_bytes_read];

        obfuscator.obfuscate(bytes_received);
        sink.write_all(bytes_received).await?;
    }
    Ok(())
}

(After replacing the return type of Obfuscator::clone with Self)

Here's a patch for the entire change in case you're interested https://github.com/mullvad/mullvadvpn-app/compare/add-obfuscated-dns-proxy...add-obfuscated-dns-proxy-patch?expand=1

[faern]

Reviewable status: 1 of 15 files reviewed, 9 unresolved discussions (waiting on @dlon, @pinkisemils, and @Serock3)

---

mullvad-obfuscated-dns-proxy/src/config/mod.rs line 75 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

I much prefer the standalone nature of those types.

I just feel like the parsing is implemented twice. First it only parses a subset of the fields to tell the program what other, more complete parser to apply to the same data again. I don't care about the performance. But it feels both like duplicated effort and things that must be kept in sync for no reason. As well as extra unwraps and stuff that would otherwise not be needed.

[faern]

Reviewable status: 1 of 15 files reviewed, 9 unresolved discussions (waiting on @dlon, @pinkisemils, and @Serock3)

---

mullvad-obfuscated-dns-proxy/Cargo.lock line 1 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Will remove, I first worked on the crate in isolation.

Ping? I see things are pushed here but this is not removed. This file makes me nervous :D

[Serock3]

Reviewable status: 1 of 15 files reviewed, 9 unresolved discussions (waiting on @dlon, @faern, and @pinkisemils)

---

mullvad-obfuscated-dns-proxy/src/forwarder/mod.rs line 15 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Performance is not an issue here. That part feels like premature optimization. Please remember that this will only transport API traffic. So it's a few bytes to kilobytes of metadata only.

That's fair. I still think removing the Forwarder, if possible, makes the code significantly more readable.

---

mullvad-obfuscated-dns-proxy/src/config/mod.rs line 75 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I just feel like the parsing is implemented twice. First it only parses a subset of the fields to tell the program what other, more complete parser to apply to the same data again. I don't care about the performance. But it feels both like duplicated effort and things that must be kept in sync for no reason. As well as extra unwraps and stuff that would otherwise not be needed.

What do you think about moving the parsing logic for a slice of the byte sequence to the individual obfuscators? E.g.

impl From<&[u8; 16]> for Plain {
    fn from(data: &[u8; 16]) -> Self {
        let (v4_bytes, mut port_bytes) = data.split_first_chunk::<4>().unwrap();

        let v4_addr = Ipv4Addr::from(*v4_bytes);
        let port = port_bytes.read_u16::<LittleEndian>().unwrap();

        Plain {
            addr: SocketAddrV4::new(v4_addr, port),
        }
    }
}

and

impl TryFrom<&[u8; 16]> for Xor {
    type Error = Error;

    fn try_from(data: &[u8; 16]) -> Result<Self, Self::Error> {
        let (v4_bytes, mut remaining_bytes) = data.split_first_chunk::<4>().unwrap();

        let v4_addr = Ipv4Addr::from(*v4_bytes);
        let port = remaining_bytes.read_u16::<LittleEndian>().unwrap();

        let xor_key: Vec<u8> = remaining_bytes[..6]
            .iter()
            .copied()
            .filter(|&byte| byte != 0x00)
            .collect();

        if xor_key.is_empty() {
            return Err(Error::EmptyXorKey);
        }
        Ok(Xor {
            addr: SocketAddrV4::new(v4_addr, port),
            xor_key,
            key_index: 0,
        })
    }
}

[Serock3]

Reviewable status: 1 of 15 files reviewed, 8 unresolved discussions (waiting on @dlon, @faern, and @pinkisemils)

---

mullvad-obfuscated-dns-proxy/src/config_resolver.rs line 90 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

I don't want to spend time here at all, but I expect that the older rustls crate might not be able to consume newer versions of webpki.

webpki-roots v0.26 can consume rustls v0.22 and above, but hickory-resolver only supports rustls v0.21 as of now. There seems to be no way to update on our end.

[dlon]

Reviewed 8 of 14 files at r5, 4 of 5 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @faern and @pinkisemils)

[pinkisemils]

Reviewable status: 7 of 15 files reviewed, 8 unresolved discussions (waiting on @dlon, @faern, and @Serock3)

a discussion (no related file):

Previously, faern (Linus Färnstrand) wrote…

This crate has virtually no doc comments. I'd like to request a bit more documentation what the library does.

Done.

---

---

mullvad-obfuscated-dns-proxy/Cargo.lock line 1 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Ping? I see things are pushed here but this is not removed. This file makes me nervous :D

Done.

---

mullvad-obfuscated-dns-proxy/Cargo.toml line 2 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Rename the crate to mullvad-encrypted-dns-proxy mayhaps? Since that's the name we finally settled on.

Done.

---

mullvad-obfuscated-dns-proxy/src/config/mod.rs line 75 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

What do you think about moving the parsing logic for a slice of the byte sequence to the individual obfuscators? E.g.

impl From<&[u8; 16]> for Plain {
    fn from(data: &[u8; 16]) -> Self {
        let (v4_bytes, mut port_bytes) = data.split_first_chunk::<4>().unwrap();

        let v4_addr = Ipv4Addr::from(*v4_bytes);
        let port = port_bytes.read_u16::<LittleEndian>().unwrap();

        Plain {
            addr: SocketAddrV4::new(v4_addr, port),
        }
    }
}

and

impl TryFrom<&[u8; 16]> for Xor {
    type Error = Error;

    fn try_from(data: &[u8; 16]) -> Result<Self, Self::Error> {
        let (v4_bytes, mut remaining_bytes) = data.split_first_chunk::<4>().unwrap();

        let v4_addr = Ipv4Addr::from(*v4_bytes);
        let port = remaining_bytes.read_u16::<LittleEndian>().unwrap();

        let xor_key: Vec<u8> = remaining_bytes[..6]
            .iter()
            .copied()
            .filter(|&byte| byte != 0x00)
            .collect();

        if xor_key.is_empty() {
            return Err(Error::EmptyXorKey);
        }
        Ok(Xor {
            addr: SocketAddrV4::new(v4_addr, port),
            xor_key,
            key_index: 0,
        })
    }
}

That wouldn't be a bad way to go about it - but I would make it From<[u8; 12]> since the header can be removed. The first 4 bytes are not significant for each of the specific proxy types.

---

mullvad-obfuscated-dns-proxy/src/config/mod.rs line 87 at r4 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

This complicates things, because then the plain case must be a special one. A forwarder needs to know where to connect a TCP socket and it need something to obfuscate the bytes - and this interface provides these details. Further, we do not know which bytes in the Ipv6 address will be used to specify the socket address, so the obfuscator itself must contain that data.

I would be open to not calling this trait Obfuscator but something like ProxyConfig, because that is essentially what it is. But it also implements obfuscation, since it is derived from the proxy configuration.

[faern]

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

---

mullvad-obfuscated-dns-proxy/src/forwarder/mod.rs line 15 at r3 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

That's fair. I still think removing the Forwarder, if possible, makes the code significantly more readable.

The Forwarder is not removed. But lots of other stuff is changed. So please re-evaluate if you think this feedback is still relevant.

[pinkisemils]

Reviewed 1 of 8 files at r3, 3 of 14 files at r5, 1 of 1 files at r7, 1 of 8 files at r9, 1 of 2 files at r12, 1 of 3 files at r13, 7 of 7 files at r14, all commit messages.
Reviewable status: 5 of 15 files reviewed, 2 unresolved discussions (waiting on @Serock3)

[dlon]

Nice cleanup.

Reviewed all commit messages.
Reviewable status: 5 of 15 files reviewed, 3 unresolved discussions (waiting on @pinkisemils and @Serock3)

---

mullvad-encrypted-dns-proxy/src/config/xor.rs line 69 at r14 (raw file):

        let key_len = key_bytes
            .iter()
            .position(|b| *b == 0x00)

You're probably aware of this, but if not I just want to point it out. This code differs from the go implementation in that it terminates after the first null byte, whereas the reference code filters out null bytes while appending to the key any remaining bytes that are non-zero.

[Serock3]

Reviewed 1 of 14 files at r5, 1 of 8 files at r9, 1 of 2 files at r11, 5 of 7 files at r14, all commit messages.
Reviewable status: 10 of 15 files reviewed, 2 unresolved discussions (waiting on @pinkisemils)

---

mullvad-obfuscated-dns-proxy/src/forwarder/mod.rs line 15 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

The Forwarder is not removed. But lots of other stuff is changed. So please re-evaluate if you think this feedback is still relevant.

Unless I am misunderstanding some aspect of the Forwarder, it's still intended to be created and then immediately consumed, which I think is more succinctly represented by a function than a type. But I won't block the PR. If you want it merged fast or don't feel like spending more time on it, then go ahead. We can always change it in the future.

---

mullvad-encrypted-dns-proxy/src/config/plain.rs line 6 at r14 (raw file):

/// API through a different IP address.
///
/// A plain configuration is represented by proxy type `ProxyType::Plain`. A plain

Great documentation!

Nit: You could mention what is expected of the function argument, e.g. "For an IPv6 address of plain proxy type, this function parses bytes 4-16 into a proxy config".

Also, ProxyType::Plain could be a doc-link, e.g.

Code snippet:

[`super::ProxyType::Plain`]

---

mullvad-encrypted-dns-proxy/src/config/xor.rs line 106 at r14 (raw file):

        }
    }
}

Here's an alternative implementation that I experimented with, you can ignore it unless you prefer it.

#[derive(Debug)]
pub struct XorObfuscator {
    key_iter: Cycle<Take<IntoIter<u8, 6>>>,
}

impl XorObfuscator {
    pub fn new(key: XorKey) -> Self {
        let key_iter = key.data.into_iter().take(key.len).cycle();
        Self { key_iter }
    }
}

impl super::Obfuscator for XorObfuscator {
    fn obfuscate(&mut self, buffer: &mut [u8]) {
        for (byte, key) in buffer.iter_mut().zip(&mut self.key_iter) {
            *byte ^= key;
        }
    }
}

Code quote:

#[derive(Debug)]
pub struct XorObfuscator {
    key: XorKey,
    key_index: usize,
}

impl XorObfuscator {
    pub fn new(key: XorKey) -> Self {
        Self { key, key_index: 0 }
    }
}

impl super::Obfuscator for XorObfuscator {
    fn obfuscate(&mut self, buffer: &mut [u8]) {
        let key_data = self.key.key_data();
        for byte in buffer {
            *byte ^= key_data[self.key_index % key_data.len()];
            self.key_index = (self.key_index + 1) % key_data.len();
        }
    }
}

[pinkisemils]

Reviewable status: 10 of 15 files reviewed, 2 unresolved discussions (waiting on @dlon)

---

mullvad-obfuscated-dns-proxy/src/forwarder/mod.rs line 15 at r3 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Unless I am misunderstanding some aspect of the Forwarder, it's still intended to be created and then immediately consumed, which I think is more succinctly represented by a function than a type. But I won't block the PR. If you want it merged fast or don't feel like spending more time on it, then go ahead. We can always change it in the future.

It can be used for forwarding, but the forwarder itself also implements AsyncRead and AsyncWrite, so it can be supplied to a TlsStream to be used directly.

---

mullvad-encrypted-dns-proxy/src/config/xor.rs line 69 at r14 (raw file):

Previously, dlon (David Lönnhager) wrote…

You're probably aware of this, but if not I just want to point it out. This code differs from the go implementation in that it terminates after the first null byte, whereas the reference code filters out null bytes while appending to the key any remaining bytes that are non-zero.

That was XOR V1 behavior, this is XOR V2 behavior.

https://github.com/mullvad/apisocks5/blob/master/internal/ipv6md/addrportxorv2/addrportxorv2.go#L78-L89

[pinkisemils]

Reviewable status: 10 of 15 files reviewed, 1 unresolved discussion (waiting on @dlon and @pinkisemils)

---

mullvad-encrypted-dns-proxy/src/config/plain.rs line 6 at r14 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Great documentation!

Nit: You could mention what is expected of the function argument, e.g. "For an IPv6 address of plain proxy type, this function parses bytes 4-16 into a proxy config".

Also, ProxyType::Plain could be a doc-link, e.g.

Done

[pinkisemils]

Reviewed 1 of 1 files at r16.
Reviewable status: 10 of 15 files reviewed, all discussions resolved

[BionicBison05]

Could this have any implications for setting a DNS-over-HTTPS server as a custom DNS server in the future?

[faern]

No it should not. Since we do our Encrypted DNS proxy DoH lookups to specific resolvers hardcoded in this library. What the OS setting/app DNS setting is does not affect how it operates.