Add two-hop WireGuard for iOS #913

pronebird · 2024-08-16T11:58:43Z

Add two-hop WireGuard integration for iOS
Rework OSTunProvider to match the system API available on iOS for configuring tunnel interface, routes, dns etc..
Add custom runner for wireguard on iOS. I am hoping we can harmonize the overall implementation between desktop and mobile but I have no energy to do that right now as the scope is only growing.

nym-vpn-apple/NymMixnetTunnel/PacketTunnelProvider.swift

pronebird · 2024-08-30T17:36:08Z

@octol @neacsu @zaneschepke @rokas-ambrazevicius please leave your review early next week, preferably on Monday because we gotta move on with this and @zaneschepke is very close to bringing support for wg on android too.

octol

Looking great! Well done!

I left some side comments for stuff I noticed while reading the code, some which refers to code not touched by this PR. Feel free to do what you want with them

octol · 2024-08-30T22:02:21Z

.github/workflows/ci-nym-vpn-core.yml

        run: |
-          cargo build --verbose --target aarch64-apple-ios -p nym-vpn-lib
+          rustflags="-L ${GITHUB_WORKSPACE}/build/lib/aarch64-apple-ios"
+          RUSTFLAGS=$rustflags cargo build --verbose --target aarch64-apple-ios -p nym-vpn-lib


General comment not for this PR:
The platform stuff is really reaching a point where we should try to take a step back and rework it. Also I'm kinda think that this part here is the result of building both mac and ios during a single job while splitting them into separate seems likely more appropriate (like how android is separate from linux).

Yeah that's legit. Let's add a card for this and attack it in a separate PR.

octol · 2024-08-30T22:03:13Z

nym-vpn-apple/MixnetLibrary/Package.swift

-            url: "https://github.com/nymtech/nym-vpn-client/releases/download/nym-vpn-core-v0.1.16/nym-vpn-core-v0.1.16_ios_universal.zip",
-            checksum: "41a5ecb8b0ab7cc2e6130179863761d12bcc988eda6c269cb00bd01a43449741"
+            url: "https://github.com/nymtech/nym-vpn-client/releases/download/nym-vpn-core-nightly/nym-vpn-core-v0.1.17-dev_ios_universal.zip",
+            checksum: "66a830cd46b912df1c11704b487f7a661299ff4583d142770ae04c0ceeaaad5c"


This will break after the next nightly build?

This will break. I believe this is a temp fix to get the Testflight out the door.

If you need a custom build of the vpn lib that is non-temporary you can use a named tag like nym-vpn-core-ios-testflight. But maybe if we merge this on Monday we can create a new release core then and use that?

That would be ideal 👍

we'll fix this with a follow up pr

octol · 2024-08-30T22:07:24Z

nym-vpn-core/nym-vpn-lib/src/lib.rs

    },
 };
+
+pub const DEFAULT_DNS_SERVERS: [IpAddr; 4] = [


nitpick: separate config.rs file or similar?

I'll do that in a separate PR to unblock folks that already base their work off this branch.

octol · 2024-08-30T22:10:25Z

nym-vpn-core/nym-vpn-lib/src/mobile/ios/default_path_observer.rs

+
+/// Observer type that wraps network path changes into a channel.
+#[derive(Debug)]
+pub struct DefaultPathObserver {


I like this, wrapping channels in a strong type. Imho we should do this more often

Note that this struct implements OSDefaultPathObserver trait, making it a conduit for messages received via FFI boundary.

octol · 2024-08-30T22:13:01Z

nym-vpn-core/nym-vpn-lib/src/mobile/ios/default_path_observer.rs

@@ -0,0 +1,64 @@
+use tokio::sync::mpsc::{UnboundedReceiver, UnboundedSender};


Side comment: for unbounded receivers/senders there is no confusion, but for bounded senders/receivers then the type has the same name for mpsc/broadcast/oneshot etc so it's nice to not import the whole path so it's easier to see in the code what channel it is

Correct. Bounded are different story.

octol · 2024-08-30T22:18:32Z

nym-vpn-core/nym-vpn-lib/src/mobile/runner.rs

+        tracing::info!("User agent: {user_agent}");
+
+        let generic_config = GenericNymVpnConfig {
+            mixnet_client_config: MixnetClientConfig {


Unrelated side comment: we should implement Default for MixnetClientConfig in the future

octol · 2024-08-30T22:22:26Z

nym-vpn-core/nym-vpn-lib/src/mobile/runner.rs

+        };
+
+        let bandwidth_controller =
+            BandwidthController::new(mixnet_client.clone(), self.task_manager.subscribe());


Side comment for the future:
Imho we should switch BandwidthController to use CancellationToken. Also, BandwidthController calls mixnet_client.disconnect() I just spotted, not sure what the implication of that is ...

Yeah what BandwidthController does is it seems that it just waits for task manager to shutdown before killing the mixnet client. I am not quite sure why it's done this way, this could be re-arranged.

octol · 2024-08-30T22:28:28Z

nym-vpn-core/nym-vpn-lib/src/mobile/runner.rs

+                // Put in some manual error handling, the correct long-term solution is that handling
+                // errors and diconnecting the mixnet client needs to be unified down this code path
+                // and merged with the mix tunnel one.
+                mixnet_client.disconnect().await;


I have a strong suspicion that this is a no-op since we pass in an external task_manager.subscribe() when creating the mixnet client, and the way to disconnect the mixnet_client is to call task_manager.signal_shutdown()

It might be, I ran out of time to investigate this any closer.

octol · 2024-08-30T22:39:56Z

nym-vpn-core/nym-vpn-lib/src/mobile/ios/default_path_observer.rs

+    }
+}
+
+#[derive(uniffi::Enum, Debug)]


What's the thinking when deciding between placing stuff in this mod vs platform? Is platform something that will be phased out over time?

Simply a logical grouping of related types. Everything under mobile/ios in essence is a platform-specific stuff. I didn't want to move things too apart since it breaks connection between types.

pronebird force-pushed the wg-two-hop branch 5 times, most recently from 68b73dc to 71ca1d8 Compare August 16, 2024 18:33

rokas-ambrazevicius reviewed Aug 19, 2024

View reviewed changes

nym-vpn-apple/NymMixnetTunnel/PacketTunnelProvider.swift Outdated Show resolved Hide resolved

pronebird force-pushed the wg-two-hop branch 8 times, most recently from 7f1b304 to e268f75 Compare August 21, 2024 15:49

github-actions bot requested a review from zaneschepke August 21, 2024 15:49

pronebird force-pushed the wg-two-hop branch 10 times, most recently from 2cbe126 to a1d9fdf Compare August 23, 2024 11:23

github-actions bot requested review from neacsu, octol and rokas-ambrazevicius August 23, 2024 11:23

pronebird self-assigned this Aug 23, 2024

pronebird force-pushed the wg-two-hop branch from a1d9fdf to 5d838d9 Compare August 27, 2024 12:26

pronebird force-pushed the wg-two-hop branch 7 times, most recently from 52929aa to 41eb97c Compare August 29, 2024 15:05

pronebird marked this pull request as ready for review August 29, 2024 15:34

pronebird changed the title ~~WIP: Add wg two hop for iOS~~ Add two-hop WireGuard for iOS Aug 29, 2024

zaneschepke approved these changes Aug 29, 2024

View reviewed changes

github-actions bot requested a review from zaneschepke August 29, 2024 16:02

zaneschepke approved these changes Aug 29, 2024

View reviewed changes

github-actions bot requested a review from zaneschepke August 29, 2024 16:24

zaneschepke approved these changes Aug 29, 2024

View reviewed changes

github-actions bot requested a review from zaneschepke August 29, 2024 17:40

pronebird force-pushed the wg-two-hop branch from 85b9fb5 to fc8cf10 Compare August 29, 2024 17:45

zaneschepke approved these changes Aug 29, 2024

View reviewed changes

github-actions bot requested a review from zaneschepke August 29, 2024 17:46

pronebird force-pushed the wg-two-hop branch 4 times, most recently from 685753d to 6d3b46d Compare August 30, 2024 16:03

Add wireguard-go for mobile

0fa091b

pronebird force-pushed the wg-two-hop branch from 6d3b46d to 0fa091b Compare August 30, 2024 16:20

octol approved these changes Aug 30, 2024

View reviewed changes

pronebird merged commit 3e61c8d into main Sep 2, 2024

pronebird deleted the wg-two-hop branch September 2, 2024 09:21

		@@ -0,0 +1,64 @@
		use tokio::sync::mpsc::{UnboundedReceiver, UnboundedSender};

Add two-hop WireGuard for iOS #913

Add two-hop WireGuard for iOS #913

Uh oh!

Conversation

pronebird commented Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pronebird commented Aug 30, 2024

Uh oh!

octol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pronebird Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octol Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pronebird commented Aug 16, 2024 •

edited

Loading

pronebird Sep 2, 2024 •

edited

Loading

octol Aug 30, 2024 •

edited

Loading