Implement multihop support for Android in daemon

[kl]

This PR adds multihop support to the daemon for Android.

---

This change is 

[linear]

DROID-1365 Add a new function wgTurnOnMultihop for android that are using the multihop module

[dlon]

Reviewed 5 of 13 files at r1, all commit messages.
Reviewable status: 5 of 13 files reviewed, 4 unresolved discussions (waiting on @kl)

---

wireguard-go-rs/src/lib.rs line 108 at r1 (raw file):

    }

    /// TODO: Document

TODO

---

wireguard-go-rs/libwg/libwg_android.go line 35 at r1 (raw file):

type LogContext = C.uint64_t

// TODO: Document

TODO

---

talpid-wireguard/src/ephemeral.rs line 101 at r1 (raw file):

    let close_obfs_sender = close_obfs_sender.clone();

    // NOTE: this might be the entry?

This is only true when the exit is also the entry, so we can remove ?

---

talpid-wireguard/src/ephemeral.rs line 165 at r1 (raw file):

    .await?;

    log::info!("Config: {config:#?}");

Was this used for debugging?

[Pururun]

Reviewed 8 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @kl)

---

talpid-wireguard/src/wireguard_go/mod.rs line 139 at r1 (raw file):

}

// TODO: Does this need to be pub?

Does it?

---

talpid-wireguard/src/wireguard_go/mod.rs line 146 at r1 (raw file):

    // live long enough and get closed when the tunnel is stopped
    _tunnel_device: Tun,
    // HACK: Don't use this. Only sometimes. ;-)

Hack?

---

talpid-wireguard/src/wireguard_go/mod.rs line 195 at r1 (raw file):

}

// TODO: move into impl of Config

Should we do this now or later?

---

talpid-wireguard/src/wireguard_go/mod.rs line 203 at r1 (raw file):

}

// TODO: move into impl of Config

Should we do this now or later?

---

talpid-wireguard/src/wireguard_go/mod.rs line 211 at r1 (raw file):

}

// TODO: move into impl of Config

Should we do this now or later?

---

talpid-wireguard/src/lib.rs line 57 at r1 (raw file):

use self::wireguard_go::WgGoTunnel;

// TODO: Document why we have a type alias !

Fix TODO

---

talpid-wireguard/src/lib.rs line 970 at r1 (raw file):

    }

    // TODO: Remove?

Decide on if it should be removed?

---

talpid-wireguard/src/ephemeral.rs line 67 at r1 (raw file):

}

#[cfg(not(windows))]

Should this really be removed?

[Pururun]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dlon and @kl)

---

talpid-wireguard/src/ephemeral.rs line 101 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

This is only true when the exit is also the entry, so we can remove ?

Removed

---

talpid-wireguard/src/ephemeral.rs line 165 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Was this used for debugging?

Removed

---

wireguard-go-rs/libwg/libwg_android.go line 35 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

TODO

Removed

---

wireguard-go-rs/src/lib.rs line 108 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

TODO

Updated

[Pururun]

Reviewable status: 10 of 13 files reviewed, 4 unresolved discussions (waiting on @dlon and @kl)

---

talpid-wireguard/src/lib.rs line 57 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Fix TODO

Updated

---

talpid-wireguard/src/wireguard_go/mod.rs line 139 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Does it?

Changed to pub(crate)

---

talpid-wireguard/src/wireguard_go/mod.rs line 195 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Should we do this now or later?

Done

---

talpid-wireguard/src/wireguard_go/mod.rs line 203 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Should we do this now or later?

Done

---

talpid-wireguard/src/wireguard_go/mod.rs line 211 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Should we do this now or later?

Done

[Pururun]

Reviewed 1 of 3 files at r3, 1 of 3 files at r4, all commit messages.
Reviewable status: 9 of 13 files reviewed, 5 unresolved discussions (waiting on @dlon and @kl)

---

talpid-wireguard/src/lib.rs line 593 at r1 (raw file):

    /// Used to block traffic to other destinations while connecting on Android.
    ///
    /// TODO: This might need some patchin' now when multihop is a thing.

Check this

[Pururun]

Reviewed 2 of 3 files at r3, 1 of 3 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dlon and @kl)

[dlon]

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

---

wireguard-go-rs/libwg/tunnelcontainer/tunnelcontainer.go line 21 at r6 (raw file):

	Uapi   net.Listener
	Logger *device.Logger
	EntryDevice *device.Device

Does this need to be used in libwg_daita.go? Since DAITA should only be enabled between the client and entry peer.

[dlon]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @kl)

---

talpid-wireguard/src/config.rs line 201 at r6 (raw file):

    pub(crate) fn exit_config(&self) -> Option<Config> {
        let mut exit_config = self.clone();
        exit_config.entry_peer = self.exit_peer.clone()?;

There are probably cleaner ways to do this. The current to_userspace_format method uses the exit peer if it is set, or else the entry config.

Could we maybe add something like a entry_to_userspace_format method?

Alternatively, a new struct that contains a single peer config (which will be set to either entry or exit, plus the private key) and a to_userspace_format method.

---

talpid-wireguard/src/config.rs line 211 at r6 (raw file):

    }

    pub(crate) fn private_ip(&self) -> CString {

first_ipv4_tun_addr? Though I kind of think we should just do this directly at the callsite, since it is so specific.

---

talpid-wireguard/src/config.rs line 215 at r6 (raw file):

            CString::new(ip.to_string()).unwrap()
        } else {
            CString::default()

This should probably return None or an Err rather than default to an empty string.

---

talpid-wireguard/src/ephemeral.rs line 207 at r6 (raw file):

    let tunnel = lock.take().expect("tunnel was None");

    let new_tunnel = tunnel.better_set_config(&config).unwrap();

Is it wise to unwrap here? It doesn't seem like wg-go is guaranteed to not fail.

---

talpid-wireguard/src/wireguard_go/mod.rs line 139 at r6 (raw file):

}

// TODO: Does this need to be pub?

I don't think it needs to be pub. But you could remove it and check if it compiles.

---

talpid-wireguard/src/wireguard_go/mod.rs line 148 at r6 (raw file):

    // HACK: Don't use this. Only sometimes. ;-)
    #[cfg(target_os = "android")]
    _log_path: Option<PathBuf>,

This should not be prepended with _, I think? Since it is actually used

---

talpid-wireguard/src/wireguard_go/mod.rs line 340 at r6 (raw file):

        // multihop
        let exit_config = exit_config.unwrap();

I think it might be better to return an error here, unless we can make it impossible to call the function without an exit peer config.

[dlon]

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @kl)

---

talpid-wireguard/src/wireguard_go/mod.rs line 103 at r6 (raw file):

        match self {
            WgGoTunnel::Multihop(state) if !config.is_multihop() => {
                // Important!

Could the comment at least explain why it's important?

---

talpid-wireguard/src/wireguard_go/mod.rs line 104 at r6 (raw file):

            WgGoTunnel::Multihop(state) if !config.is_multihop() => {
                // Important!
                state.stop().unwrap();

Should we just return (?) rather than unwrap here?

---

talpid-wireguard/src/wireguard_go/mod.rs line 114 at r6 (raw file):

            }
            WgGoTunnel::Singlehop(state) if config.is_multihop() => {
                state.stop().unwrap();

Should we just return (?) rather than unwrap here?

[Pururun]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @kl)

---

talpid-wireguard/src/wireguard_go/mod.rs line 139 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

I don't think it needs to be pub. But you could remove it and check if it compiles.

We changed it from pub to (pub)crate but forgot to remove the TODO.

[kl]

Reviewable status: 10 of 13 files reviewed, 8 unresolved discussions (waiting on @dlon and @Pururun)

---

talpid-wireguard/src/config.rs line 211 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

first_ipv4_tun_addr? Though I kind of think we should just do this directly at the callsite, since it is so specific.

Moved to call site

---

talpid-wireguard/src/config.rs line 215 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

This should probably return None or an Err rather than default to an empty string.

Fixed at call site

---

talpid-wireguard/src/ephemeral.rs line 207 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

Is it wise to unwrap here? It doesn't seem like wg-go is guaranteed to not fail.

Change to returning result instead of unwrapping

---

talpid-wireguard/src/wireguard_go/mod.rs line 103 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

Could the comment at least explain why it's important?

Removed

---

talpid-wireguard/src/wireguard_go/mod.rs line 104 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should we just return (?) rather than unwrap here?

Done

---

talpid-wireguard/src/wireguard_go/mod.rs line 114 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should we just return (?) rather than unwrap here?

Done

---

talpid-wireguard/src/wireguard_go/mod.rs line 340 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

I think it might be better to return an error here, unless we can make it impossible to call the function without an exit peer config.

Made exit peer config required argument

[kl]

Reviewable status: 10 of 13 files reviewed, 8 unresolved discussions (waiting on @dlon and @Pururun)

---

talpid-wireguard/src/config.rs line 201 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

There are probably cleaner ways to do this. The current to_userspace_format method uses the exit peer if it is set, or else the entry config.

Could we maybe add something like a entry_to_userspace_format method?

Alternatively, a new struct that contains a single peer config (which will be set to either entry or exit, plus the private key) and a to_userspace_format method.

Done.

[MarkusPettersson98]

Reviewable status: 10 of 14 files reviewed, 8 unresolved discussions (waiting on @dlon, @kl, and @Pururun)

---

wireguard-go-rs/libwg/tunnelcontainer/tunnelcontainer.go line 21 at r8 (raw file):

	Uapi   net.Listener
	Logger *device.Logger
	EntryDevice *device.Device

⛏️ Please group Device and EntryDevice, thanks 👼

Code quote:

	Device *device.Device
	Uapi   net.Listener
	Logger *device.Logger
	EntryDevice *device.Device

[dlon]

Reviewed 3 of 3 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kl)

---

talpid-wireguard/src/ephemeral.rs line 207 at r6 (raw file):

Previously, kl (Kalle Lindström) wrote…

Change to returning result instead of unwrapping

It's still unwrapping here.

---

talpid-wireguard/src/wireguard_go/mod.rs line 139 at r6 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

We changed it from pub to (pub)crate but forgot to remove the TODO.

Does it need to be pub anything?

---

talpid-wireguard/src/wireguard_go/mod.rs line 429 at r8 (raw file):

        let config = &self.as_state().config;

        let peer_public_key = match config.exit_peer.as_ref() {

This is incorrect. This should use the entry peer, always.

---

talpid-wireguard/src/lib.rs line 784 at r8 (raw file):

                #[allow(clippy::needless_borrow)]
                &config,
                config.exit_peer.clone().unwrap(),

We could do something like this instead of relying on is_multihop to determine that exit_peer is set:

let tunnel = if let Some(exit_peer) = &config.exit_peer = {

    ...
    &config,
    exit_peer,

[Pururun]

Reviewed 2 of 3 files at r9, all commit messages.
Reviewable status: 13 of 14 files reviewed, 4 unresolved discussions (waiting on @dlon and @kl)

---

talpid-wireguard/src/wireguard_go/mod.rs line 139 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

Does it need to be pub anything?

Yes, because otherwise you get a warning about different visibility

---

talpid-wireguard/src/wireguard_go/mod.rs line 148 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

This should not be prepended with _, I think? Since it is actually used

Done

---

talpid-wireguard/src/wireguard_go/mod.rs line 429 at r8 (raw file):

Previously, dlon (David Lönnhager) wrote…

This is incorrect. This should use the entry peer, always.

Done

[Pururun]

Reviewed 2 of 2 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dlon and @kl)

[dlon]

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kl)

---

talpid-wireguard/src/config.rs line 201 at r6 (raw file):

Previously, kl (Kalle Lindström) wrote…

Done.

This would be more readable if instead of taking in both entry_peer and exit_peer, userspace_format only received a single &PeerConfig. You could then move this part to Config::to_userspace_format:

exit_peer
        .as_ref()
        .into_iter()
        .chain(std::iter::once(&entry_peer))

[dlon]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kl)

---

talpid-wireguard/src/config.rs line 201 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

This would be more readable if instead of taking in both entry_peer and exit_peer, userspace_format only received a single &PeerConfig. You could then move this part to Config::to_userspace_format:

exit_peer
        .as_ref()
        .into_iter()
        .chain(std::iter::once(&entry_peer))

Ah, no, because it's an iterator. This is okay as is, but you could probably take peers: impl Iterator<Item = &wireguard::PeerConfig>. In Config, simply pass self.peers(). In the other place, std::iter::once(&entry_peer) and std::iter::once(&exit_peer) or something like that.

[Pururun]

Reviewed 2 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dlon and @kl)

---

talpid-wireguard/src/ephemeral.rs line 207 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

It's still unwrapping here.

Done

---

talpid-wireguard/src/lib.rs line 784 at r8 (raw file):

Previously, dlon (David Lönnhager) wrote…

We could do something like this instead of relying on is_multihop to determine that exit_peer is set:

let tunnel = if let Some(exit_peer) = &config.exit_peer = {

    ...
    &config,
    exit_peer,

Done

[dlon]

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kl)

---

talpid-wireguard/src/lib.rs line 782 at r13 (raw file):

        let tunnel = if let Some(exit_peer) = &config.exit_peer {
            WgGoTunnel::start_multihop_tunnel(
                #[allow(clippy::needless_borrow)]

Nit: I'm pretty sure you can remove #[allow(...)] and just pass in config.

---

talpid-wireguard/src/lib.rs line 784 at r13 (raw file):

                #[allow(clippy::needless_borrow)]
                &config,
                exit_peer.clone(),

Nit: I believe you could pass this in by reference rather than cloning (requires changing the params, of course).

[kl]

Reviewable status: 13 of 14 files reviewed, 1 unresolved discussion (waiting on @dlon and @Pururun)

---

talpid-wireguard/src/config.rs line 201 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

Ah, no, because it's an iterator. This is okay as is, but you could probably take peers: impl Iterator<Item = &wireguard::PeerConfig>. In Config, simply pass self.peers(). In the other place, std::iter::once(&entry_peer) and std::iter::once(&exit_peer) or something like that.

We refactored the code again, please have a look.

[Pururun]

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

---

talpid-wireguard/src/lib.rs line 782 at r13 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: I'm pretty sure you can remove #[allow(...)] and just pass in config.

Done

---

talpid-wireguard/src/lib.rs line 784 at r13 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: I believe you could pass this in by reference rather than cloning (requires changing the params, of course).

Done

[Pururun]

Reviewed 1 of 1 files at r14, 3 of 3 files at r15, 2 of 2 files at r16, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)

---

talpid-wireguard/src/config.rs line 201 at r6 (raw file):

Previously, kl (Kalle Lindström) wrote…

We refactored the code again, please have a look.

Done

[dlon]

Reviewed 3 of 3 files at r19, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kl)

---

talpid-wireguard/src/lib.rs line 1089 at r19 (raw file):

    DaitaNotSupported,

    /// [connecitivity] error.

Nit: connectivity

[dlon]

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

[dlon]

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