Implement multihop support for Android in daemon #7168

kl · 2024-11-12T14:27:04Z

This PR adds multihop support to the daemon for Android.

This change is

linear · 2024-11-12T14:27:09Z

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

(if you've made sure that PQ works)

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

kl requested a review from dlon November 12, 2024 14:27

Pururun force-pushed the implement-wgturnonmultihop-for-android-droid-1365 branch from d1ea844 to 4cc23e5 Compare November 12, 2024 14:41

dlon requested changes Nov 13, 2024

View reviewed changes

Rawa added the Android Issues related to Android label Nov 13, 2024

Pururun requested changes Nov 13, 2024

View reviewed changes

Pururun reviewed Nov 13, 2024

View reviewed changes

Pururun requested changes Nov 13, 2024

View reviewed changes

Pururun reviewed Nov 13, 2024

View reviewed changes

dlon reviewed Nov 13, 2024

View reviewed changes

dlon requested changes Nov 13, 2024

View reviewed changes

Pururun reviewed Nov 14, 2024

View reviewed changes

kl commented Nov 14, 2024

View reviewed changes

MarkusPettersson98 reviewed Nov 14, 2024

View reviewed changes

dlon requested changes Nov 14, 2024

View reviewed changes

Pururun reviewed Nov 15, 2024

View reviewed changes

Pururun requested a review from dlon November 15, 2024 08:18

Pururun reviewed Nov 15, 2024

View reviewed changes

dlon requested changes Nov 15, 2024

View reviewed changes

Pururun requested a review from dlon November 15, 2024 08:43

Pururun reviewed Nov 15, 2024

View reviewed changes

dlon reviewed Nov 15, 2024

View reviewed changes

kl commented Nov 15, 2024

View reviewed changes

MarkusPettersson98 added 3 commits November 15, 2024 11:05

Use core instead of std

20fd7bc

Bump wireguard-go to version with tun/multihoptun

d89d7af

Add wgTurnOnMultihop

d851676

MarkusPettersson98 and others added 24 commits November 15, 2024 11:05

Fix multihop with DAITA

f05177e

Use WgGoTunnel directly on Android

e36e952

Remove remaining rustc warnings

261688f

Re-enable patch_allowed_ips

f60fa34

Fix remaining clippy warnings

d7dc4d0

Use TunnelType alias everywhere

7f92b44

Fix remaining rustc warnings on non-Android platforms

65b05e8

Fix not windows

5c792de

Remove TODOs

d1a389a

Various fixes

a33502e

Remove another TODO

4526dfd

Remove more TODO

234b9b2

Remove outdated TODO

f608590

Adress review feedback

8c86442

WIP Lookup entry peer if exists

96af060

Fix Daita code

35a1a12

More refactoring

01b6d5e

Fix start daita

386836f

Fixed some things

6ce6aff

Moved some code

31da91a

More review feedback fixes

7bc59d1

Fix map err

b05f141

Fix entry peer config code

e7bffe7

Further fixes

d20f504

Pururun force-pushed the implement-wgturnonmultihop-for-android-droid-1365 branch from f6f36aa to d20f504 Compare November 15, 2024 10:05

Fix clippy issue

e8ff660

Pururun reviewed Nov 15, 2024

View reviewed changes

Improve code

0767fd0

Pururun approved these changes Nov 15, 2024

View reviewed changes

dlon approved these changes Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement multihop support for Android in daemon #7168

Implement multihop support for Android in daemon #7168

kl commented Nov 12, 2024 •

edited by faern

Loading

linear bot commented Nov 12, 2024

dlon left a comment

Pururun left a comment

Pururun left a comment

Pururun left a comment

Pururun left a comment

Pururun left a comment

dlon left a comment

dlon left a comment

dlon left a comment

Pururun left a comment

kl left a comment

kl left a comment

MarkusPettersson98 left a comment

dlon left a comment

Pururun left a comment

Pururun left a comment

dlon left a comment

dlon left a comment

Pururun left a comment

dlon left a comment

kl left a comment

Pururun left a comment

Pururun left a comment

dlon left a comment

Implement multihop support for Android in daemon #7168

Are you sure you want to change the base?

Implement multihop support for Android in daemon #7168

Conversation

kl commented Nov 12, 2024 • edited by faern Loading

linear bot commented Nov 12, 2024

dlon left a comment

Choose a reason for hiding this comment

Pururun left a comment

Choose a reason for hiding this comment

Pururun left a comment

Choose a reason for hiding this comment

Pururun left a comment

Choose a reason for hiding this comment

Pururun left a comment

Choose a reason for hiding this comment

Pururun left a comment

Choose a reason for hiding this comment

dlon left a comment

Choose a reason for hiding this comment

dlon left a comment

Choose a reason for hiding this comment

dlon left a comment

Choose a reason for hiding this comment

Pururun left a comment

Choose a reason for hiding this comment

kl left a comment

Choose a reason for hiding this comment

kl left a comment

Choose a reason for hiding this comment

MarkusPettersson98 left a comment

Choose a reason for hiding this comment

dlon left a comment

Choose a reason for hiding this comment

Pururun left a comment

Choose a reason for hiding this comment

Pururun left a comment

Choose a reason for hiding this comment

dlon left a comment

Choose a reason for hiding this comment

dlon left a comment

Choose a reason for hiding this comment

Pururun left a comment

Choose a reason for hiding this comment

dlon left a comment

Choose a reason for hiding this comment

kl left a comment

Choose a reason for hiding this comment

Pururun left a comment

Choose a reason for hiding this comment

Pururun left a comment

Choose a reason for hiding this comment

dlon left a comment

Choose a reason for hiding this comment

kl commented Nov 12, 2024 •

edited by faern

Loading